[Vm-dev] git bites again

Jakob Reschke forums.jakob at resfarm.de
Wed Feb 21 22:08:31 UTC 2018


Hi Eliot,

no worries, I am not offended. As I wrote in the PR: "pick those parts
of the changes that you like". Sorry that it caused trouble, which is
not very likable, of course.

$() is the POSIX syntax for command substitution and has the same
semantics as the backticks. I find it more readable than the
backticks, especially when combined with regular quotes. Also, it
simplifies quoting and nesting (which I did here).

I agree that the comparison with "false" might look like someone did
not know their way around shell programming and return codes in
particular, but rev-parse --is-inside-git-dir really does print either
"true" or "false" to stdout. ;-) And it exists with 0 in either case.
While the command substitution runs a subshell, the cd and git
rev-parse should already run in the same one and they are combined
with &&.

Rather than muting the "not a git repository" message, I would like to
understand why it occurs in the first place. But I have not yet got
around to investigate it myself. In another thread, I proposed to add
a "set -x" at the top of the script to see where the message occurs;
it might also be one of the commands in the changed lines further down
the script. I had hoped that somebody would find the time earlier than
I would.

Kind regards,
Jakob

2018-02-21 20:03 GMT+01:00 Eliot Miranda <eliot.miranda at gmail.com>:
>
> Hi Jakob, Hi Fabio,
>
> On Tue, Feb 20, 2018 at 11:43 PM, Fabio Niephaus <lists at fniephaus.com> wrote:
>>
>>
>> Hi all,
>>
>> On Wed, Feb 21, 2018 at 8:17 AM Alistair Grant <akgrant0710 at gmail.com> wrote:
>>>
>>>
>>> Hi Tim & Eliot,
>>>
>>> On 21 February 2018 at 04:11, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>>> >
>>> > Hi Tim,
>>> >
>>> > On Tue, Feb 20, 2018 at 6:29 PM, tim Rowledge <tim at rowledge.org> wrote:
>>> >>
>>> >>
>>> >> OK, I know that the entire point of git is to drive people insane but this is an especially silly problem.
>>> >>
>>> >> clone the repository with
>>> >> git clone https://github.com/OpenSmalltalk/opensmalltalk-vm.git
>>> >> wait...
>>> >> cd opensmalltalk-vm/
>>> >> ./scripts/updateSCCSVersions ->
>>> >> fatal: Not a git repository: '.git'
>>> >> fatal: unrecognized input
>>> >>
>>> >> What?
>>
>>
>> As part of the transition from SVN to Git, we had to add a script that runs after certain Git commands and inserts a timestamp into the code which is then compiled into the VM as a part of its version string. This approach does not only sound hacky, it actually is hacky, but a Git commit hash was not good enough. Unfortunately, there are many ways to work with Git repositories (e.g. submodules), so Jakob proposed a change that seemed to fix things for some use cases while apparently breaking things for other users.
>>
>>>
>>> >> The annoying part is that it worked (at least, so far as I remember) last time I loaded up a clone of the vm repo.
>>
>>
>> I've reverted Jakob's change, so please try again.
>
>
> Thanks.  It's good to know what the change is :-).
>
> So we're talking about this change, right? :
>
> diff --git a/scripts/updateSCCSVersions b/scripts/updateSCCSVersions
> index 179b008..8593f2f 100755
> --- a/scripts/updateSCCSVersions
> +++ b/scripts/updateSCCSVersions
> @@ -3,15 +3,13 @@
>  # platforms/Cross/vm/sqSCCSVersion.h to be checked-in so that its version
>  # info reflects that of the current check-in.
>
> -if [ -d `dirname $0`/../.git ]; then
> -       hooks_dir="`git rev-parse --git-dir`/hooks"
> -    mkdir -p $hooks_dir
> -    cp -f $0 $hooks_dir/post-commit
> -    cp -f $0 $hooks_dir/post-merge
> -    cp -f $0 $hooks_dir/post-checkout
> -    cd `dirname $0`/..
> +if [ "$(cd "$(dirname $0)" && git rev-parse --is-inside-git-dir)" = false ]; then
> +    hooks_dir="`git rev-parse --git-dir`/hooks"
> +    mkdir -p "$hooks_dir"
> +    cp -f "$0" "$hooks_dir/post-commit"
> +    cp -f "$0" "$hooks_dir/post-merge"
> +    cp -f "$0" "$hooks_dir/post-checkout"
>  else
> -    cd `dirname $0`/../..
>      if [[ "$(basename $0)" == "post-checkout" ]]; then
>          prevHEAD=$1
>          newHEAD=$2
>
> Jakob, my understanding is that in shell if one wants to evaluate something enc capture its output one must use backpacks.  e.g. using the dirname $0 example above one can say either `dirname $0` or "`dirname $0`", the difference being that the first can expand to multiple tokens but the last will be considered a single then.  I don't understand what the intent of "$(cd "$(dirname $0)" && git rev-parse --is-inside-git-dir)" = false is.  But in any case if "$(cd "$(dirname $0)" did work the same as "`cd \`dirname $0\` `", it would;t have the desired effect because the command is evaluated in a sub-shell and hence doesn't change the working directory for the git rev-parse --is-inside-git-dir command.  I think you need to say something like
>
> if (cd `dirname $0`; git rev-parse --is-inside-git-dir); then
>
> This evaluates the cd in a sub-shell established by the parentheses, and hence also changes the current directory for the git rev-parse --is-inside-git-dir command.  Since "if" takes a command ( /bin/[ is merely a link to /bin/test ) one doesn't need the sub-shell evaluation and can run the git command directly.  If we were in the right directory then we could write
>
> if git rev-parse --is-inside-git-dir; then
>
> The next issue is to get rid of the "fatal: not a git directory" output.  You can redirect stderr to /dev/null.  hence what I *think* you want to write is
>
> if (cd `dirname $0`; git rev-parse --is-inside-git-dir 2>/dev/null); then
>
> HTH
>
> And Jakob, I hope you're not offended!  The request to revert was not in any way a criticism of you.  We all make mistakes and often those who make most mistakes are contributing the most.
>
>
>>> > Agreed.  It is incumbent upon whoever made the change to fix this.  "you break it, you fix it".  so please, step up :-)
>>> >
>>> > _,,,^..^,,,_
>>> > best, Eliot
>>> >
>>>
>>> The offending commit is 0458c6a92651fc8d0bf86158a0407f87470ade50 from
>>> 6 Feb.  I'm not familiar enough with git to easily fix it, but backing
>>> out the change should be straightforward.
>>
>>
>> Thanks for looking into this, Alistair.
>>
>>>
>>>
>>> Cheers,
>>> Alistair
>
>
>
> _,,,^..^,,,_
> best, Eliot
>


More information about the Vm-dev mailing list