Re: [lmi] Proposed workflow for proprietary repository

[Vadim Zeitlin]

On Mon, 13 Nov 2017 00:07:29 +0000 Greg Chicares <address@hidden> wrote:

GC> Therefore, I've moved all that stuff into a script:
GC> 
GC> commit a8e0d0258afe16d334d824226503849645749a6f
GC> Author: Gregory W. Chicares <address@hidden>
GC> Date:   2017-11-12T23:40:22+00:00
GC> 
GC>     Replace complex manual commands with a script
GC> 
GC>  check_git_setup.sh | 55 
+++++++++++++++++++++++++++++++++++++++++++++++++++++++
GC>  gwc/develop2.txt   | 37 +++++--------------------------------
GC>  2 files changed, 60 insertions(+), 32 deletions(-)
GC> 
GC> Would you mind taking a look and telling me if that script
GC> needs improvement?

 Hi,

 Nothing that I'd characterize as "needing" improvement, but a couple of
things that might be improved.

 The first one is really trivial: I think that

        case `uname -s` in
                CYGWIN*)
                        printf "cygwin detected\n"
                        ;;
        esac

is a more idiomatic way of doing things in shell than using the $(expr
substr())" expression but, following my own advice not to start battles for
purity when the entire war is hopelessly lost, I'm certainly not going to
start seriously worrying about the best style of writing shell scripts.

 The second question is about using "--global" when setting core.filemode
to false. This seems questionable because you only care about this for this
particular repository, so "--local" (which is the default anyhow) would
seem to be more logical, why should this script change things for all the
other Git repositories on the same system? Also, if, somehow, there is
already a local setting of this option, it's going to take precedence over
the global one, while you really want to impose this setting. I realize
that both of these things (using other, unrelated, Git repositories and
having a local setting of core.filemode) are very unlikely to happen, but
by simply not using "--global" we wouldn't have to think about them at all.

 Next one is about the purpose of "cd $(git rev-parse --show-toplevel)": it
seems not to do anything if the script is run from the repository directory
and potentially do something quite wrong if it's run (e.g. using its full
path) from a directory which just happens to contain another repository.
Normally I'd use something like "cd $(dirname $(readlink -f $0))" instead,
shouldn't it be done like this here too?

 Then there is the snippet which does use "readlink" but here I wonder why
do we need to do these indirect tests with "readlink -f" instead of just
using normal "readlink .git/hooks" and comparing the result with
"../hooks", i.e. directly checking for what we want. Again, I can't
actually think of any situation in which the current code would fail, so
it's not a problem, but it just seems unnecessary roundabout to me compared
to (warning, untested shell code ahead):

        if [ "$(readlink .git/hooks)" = "../hooks" ]; then
                printf "git hooks directory is properly symlinked\n"
        else
                if [ -x .git/hooks ]; then
                        printf "must reconfigure git hooks directory, original 
saved as hooks-orig\n"
                        mv .git/hooks .git/hooks-orig
                else
                        printf "must create git hooks directory symlink\n"
                fi
                ln --symbolic --no-dereference ../hooks .git
        fi

 Final enhancement would be to exit the script with 0 only if all checks
passed and some non-zero exit code otherwise.

 Sorry for a long email about mostly trivial issues, hopefully mentioning
at least some of them wasn't useless,
VZ