Re: [PATCH] add post-fetch hook

[Junio C Hamano <gitster@pobox.com>]

Joey Hess <joey@kitenet.net> writes:

> Junio C Hamano wrote:
>> If we _were_ to sanction the use of the hook to tweak the result, I do not
>> want to see it implemented as an ad-hoc hack that tells the hook writers
>> that it is _entirely_ their responsiblity to update the remote tracking
>> branches from what it fetched, and also update $GIT_DIR/FETCH_HEAD to
>> maintain consistency between these two places.
>> 
>> A very cursory look at the patch tells me that there are a few problems
>> with it.  It does not seem to affect what will go to $GIT_DIR/FETCH_HEAD
>> at all, and hence it does not have any way to affect the result of the
>> fetch that does not store it to any of our remote tracking branches.
>
> True, it does not update FETCH_HEAD. I had not considered using the hook
> that way.

Perhaps I misread what you wrote in the commit log message then.  I
somehow got an impression that one of the advertised way for the hook to
be used was to lie which commits the remote tracking refs point at by
letting it run "update-ref refs/*/*" and the lie is later picked up by
re-reading them, but I wasn't reading the patch very carefully.

If your use case does not involve updating the remote tracking refs nor
FETCH_HEAD (updating only one and not the other is a no-starter), then we
should explicitly forbid it in the documentation, as allowing updates will
invite inconsistencies.

Although I do not deeply care between such a "trigger to only notify, no
touching" hook and a full-blown "allow hook writers to easily lie about
what happened in the fetch" hook, I was hoping that we would get this
right and useful if we were spending our brain bandwidth on it. I am not
very fond of an easier "trigger to only notify" hook because people are
bound to misuse the interface and try updating the refs anyway, making it
easy to introduce inconsistencies between refs and FETCH_HEAD that will
confuse the later "merge" step.

As hook writers are more prone to write such an incorrect code than people
who implement the mechanism to call hooks on the git-core side, the more
the hook interface helps hook writers to avoid such mistakes, the better.

So if we were to allow the hook to lie what commits were fetched and store
something different from what we fetched in the remote tracking refs, I
think the correct place to do so would be in store_updated_refs(),
immediately before we call check_everything_connected().

 - Feed the contents of the ref_map to the hook. For each entry, the hook
   would get (at least):
   . the object name;
   . the refname at the remote;
   . the refname at the local (which could be empty when we are not
     copying it to any of our local ref); and
   . if the entry is to be used for merge.

 - The hook must read _everything_ from its standard input, and then
   return the
   re-written result in the same format as its input. The hook could
   . update the object name (i.e. re-point the remote tracking ref);
   . update the local refname (i.e. use different remote tracking ref);
   . change "merge" flag between true/false; and/or
   . add or remove entries

 - You read from the hook and replace the ref_map list that is fed to
   check_everything_connected(). This ref_map list is what is used in the
   next for() loop that calls update_local_ref() to update the remote
   tracking ref, records the entry in FETCH_HEAD, and produces the report.

This way, the hook cannot screw up, as what it tells us will consistently
be written by us to where it should go.