Re: [lmi] Proposed workflow for proprietary repository

[Greg Chicares]

On 2017-11-13 13:55, Vadim Zeitlin wrote:
> On Mon, 13 Nov 2017 00:07:29 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC>  check_git_setup.sh | 55 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
[...]
> GC> Would you mind taking a look and telling me if that script
> GC> needs improvement?
[...]
>  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.

Yes, that's more readable and more robust. The only reason I used
  if [ "$(expr substr $(uname -s) 1 6)" = "CYGWIN" ]
instead is because that 'expr substr' thing was already used elsewhere.
But it's used only in one other place, so I think it's better to
use the idiom you suggest here now, test it well, and then use it in
that one other place too. It's clear that the 'case' variant asks
whether one string contains another at its beginning, but the "1 6"
indexing requires thought (in origin zero it'd be "0 5").

>  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?

OTOH, this is limited to cygwin systems only, and I think we've
concluded that we should use "core.filemode false" always on every
cygwin system--i.e., in effect, that this should be a cygwin default.
Therefore, forcing that default globally seems like a good idea,
except that this command doesn't actually do that:

> 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.

Okay, I'll remove '--global' here.

>  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

Yes. The idea was that, if it's run from a subdirectory, we want to
switch to the "toplevel" directory. However, the script should reside
in that directory anyway, and I hadn't considered what happens if...

> 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?

....so yes, I'll change that, 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

There are several changes here.

As for readlink's '-f' = '--canonicalize' option, I guess that's just
a personal preference. I can compare "2.0000" to "2" more readily if
I canonicalize both to "2" first, and the extra labor represents an
affordable price. I'm assuming that '--canonicalize' actually performs
some similar kind of canonicalization of filepaths, but let's see...
okay, it tests validity and returns an abolute filepath. It's not
portable to BSD, and GNU suggests that it's not "preferred":

http://www.gnu.org/software/coreutils/manual/html_node/readlink-invocation.html#readlink-invocation
| Note the realpath command is the preferred command to use for 
canonicalization.

but I'm in no rush to pursue that further.

I'll adopt the idea of testing the everything-is-okay case first.

It was a mistake to execute (in effect) 'mv ; ln' instead of 'mv && ln',
because mv can certainly fail.

Yet the logic proposed above doesn't seem simple enough to me, so
I'll do something a little different...

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

....also adopting that excellent idea.

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

Thanks for a careful review that helped me remove serious defects.