Re: Git tag: pre-receive hook issue

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2015, 21:19 UTC
Message-ID: <xmqqtwt1yxil.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAGDgvc2F7UMWTVrRFt5eK2xmbfz-kyWh6Vp-eQNEj7tixzRPYQ@mail.gmail.com>

Gaurav Chhabra <varuag.chhabra@gmail.com> writes:

Show 30 quoted lines

> @Junio: From the example you gave, i could conclude the following:
>
> 1) : gitster garbage/master; git commit --allow-empty -m third
>   [master d1f1360] third
>   : gitster garbage/master; git describe --exact-match HEAD ;# third
>   fatal: no tag exactly matches 'd1f1360b46dfde8c6f341e48ce45d761ed37e357'
>
>> Since after # third commit, no tag was applied to HEAD, so
>> --exact-match resulted in fatal error
>
> 2)  : gitster garbage/master; git commit --allow-empty -m second
> [master d1de78e] second
> : gitster garbage/master; git tag -a -m 'v0.1' v0.1
> : gitster garbage/master; git describe --exact-match HEAD^ ;# second
>     v0.1
>> Since annotated tag was applied after # second commit, so
>> --exact-match did referenced the commit as expected.
>
> 3) : gitster garbage/master; git commit --allow-empty -m initial
>   [master (root-commit) b18cac2] initial
>   : gitster garbage/master; git tag v0.0 ;# lightweight
>   : gitster garbage/master; git describe --exact-match HEAD^^ ;# first
>   fatal: no tag exactly matches 'b18cac237055d9518f9f92bb4c0a4dac825dce17'
>
>> In this case, it's a lightweight tag and i read today that by
>> default, git describe only shows annotated tags (without --all or
>> --tags). I think it's because of the missing option (--all or
>> --tags) that it resulted in fatal error in this case.
>
> Please correct me if i misunderstood any/all of the above cases.

All correct. The part that I find questionable is that by checking with "is this commit a tagged one?" and doing different things. What makes the initial and the third special to deserve checking (because they are not annotated with a tag) while skipping the validation for the second (merely because it has an annotated tag added to it)?

> My queries:
> A) When you mentioned: "I am feeding three _commits_, not tags.", i
> didn't really get what you're trying to highlight.

I was skipping one round-trip, expecting for you to say "the part that sets 'commit=$new' for a newly created branch was a mistake, and it should do 'commit=$(git rev-list $new)'". And even when $new is a tag, what comes out of `git rev-list $new` is a sequence of commit object names, never the tag object.

Show 5 quoted lines

> You've also mentioned that "And you check everything on that list.  Why?"
>> Was this comment for the portion where the code is validating
>> commits (git rev-list $old_sha1..$new_sha1) for existing branch? If
>> yes, then i didn't really get your concern. Can you kindly elaborate
>> a bit?

I do not question the validity of checking everything between $old..$new; my question was more about "even though you correctly check everything, why do you check _only_ the tip by doing 'commit=$new' (instead of doing 'commit=`git rev-list $new`') when somebody pushes to a new branch?".

Show 5 quoted lines

>>> git describe --exact-match ac28ca721e67adc04078786164939989a5112d98 2>&1 |
>>> grep -o fatal | wc -w
>>>
>>> So i'm wondering why it's not entering the IF block (as confirmed in the
>>> output link)

When you push a branch 'master' and a tag 'v1.0', these things happen in order:

 (1) all objects that are necessary to ensure that the receiving
     repository has everything reachable from 'master' and 'v1.0'
     are sent to it and stored.  If the receiving end fails to store
     this correctly, everything below is skipped and the operation
     fails.

 (2) pre-receive is run.  Notice that at this point, no refs have
     been updated yet, so "describe" will not know v1.0 tag (if it
     is a new one) exists.  But this step can assume that all new
     objects are already accessible using their object name, so

	case "$old" in
        0000000000000000000000000000000000000000) range=$new ;;
        *) bottom=$old..$new ;;
	esac &&
     	git rev-list $range |
        while read commit
        do
        	check $commit object, e.g.
                git cat-file commit $commit | grep FooBarId
	done

     is expected to work.

 (3) for each ref being updated (in this case, refs/heads/master and
     refs/tags/v1.0), the following happens:

     (3-1) built-in sanity checks may reject the push to the ref,
           e.g. refusing to update a checked out branch, refusing to
           accept a non-fast-forward push that is not forced, etc.

     (3-2) update-hook is run, which may reject the push to this
           ref.

     (3-3) the ref is updated (unless the push is atomic).

 (4) if the push is atomic, the refs are updated.

 (5) post-receive hook is run.  This is for logging and cannot
     affect the outcome of the push.

 (6) for each ref that was updated (in this case, refs/heads/master
     and refs/tags/v1.0), post-update hook is run.  This is for
     logging and cannot affect the outcome of the push.

As your requirement seems to be to validate any and all new commits, I think it is totally unnecessary to check if any of them happens to have a tag pointing at it in the first place.