Re: [PATCH 0/7] New execute-commands hook for centralized workflow

From: Jiang Xin <worldhello.net@gmail.com>
Date: Mar 5, 2020, 16:51 UTC
Message-ID: <CANYiYbE2V3bLfEgH-aRDv4Y5V+_BTZn-oUN7fOVpARm+_14BdQ@mail.gmail.com>
In-Reply-To: <xmqqv9nj4zxk.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> 于2020年3月5日周四 上午4:39写道：

>
> I do not claim to be great at naming, but you are worse ;-)

I totally agree that I am not good at naming, for example my daughter's name.

Show 13 quoted lines

>  - Any hook is about executing command(s), so "execute-commands"
>    hook does not give any information to users.
>
>  - IIUC, this is only about what happens when accepting a push and
>    is not called at any other time.  Naming your hook without
>    "receive" anywhere in its name would mean other people won't be
>    able to add hook that "executes" commands upon cues other than
>    receiving a push.
>
> I can guess why you chose that name, because I know there is a
> function called execute_commands() in "git receive-pack", but that
> is not somethhing you can expect your end users, who are not
> intimate to our codebase, to know.

Yes, it's better to name the hook "* -receive", because the hooks are for different commands, such as "commit-msg" is for `git commit`.

Show 6 quoted lines

> > We can use the external "execute-commands" hook to create pull requests
> > or send emails.
>
> You can create pull requests or send emails out of the post-receive
> hook, so that is not a convincing justification why we want a new
> hook.

Another solution is using "pre-receive" + "post-receive" to handle a push to "refs/for/master". The "post-receive" hook is used to create a pull requst and delete the special reference "refs/for/master" created between these two hooks. But having a temporary reference created is not safe for concurrent pushes.

Show 15 quoted lines

> Now, I understand that Gerrit-style "notice a push to for/<target>,
> take over the whole operation that happens after receiving the pack
> data and do something entirely different, such as attempting to run
> a merge with refs/heads/<target> and update refs/heads/<target>
> instead, or fail the push if automerge fails" is not easy to arrange
> within the current "pre-receive" + "post-receive" framework (by the
> way, we should start considering to deprecate "update", and
> "post-update" hooks as these "*-receive" hooks were added to replace
> them, perhaps we should leave a #leftoverbits mark here).  And I
> think it is reasonable to add a new hook that takes over the whole
> flow in "git receive-pack" to do so.
>
> I just do not think "the execute-commands hook" is a good name for
> it.  Perhaps "divert-receive" (as it diverts large portion of what
> receive does) or something?  I dunno.

I suggest naming the hook as "process-receive", which is executed between the other two "p*-receive" hooks, and no need to create a special "pre-receive" for "process-receive".

> How do Gerrit folks deal with the "we pushed to the server, so let's
> pretend to have turned around and fetched from the same server
> immediately after doing so" client-side hack, by the way?

In the following example, I push a local commit to a special reference (refs/for/master) of the remote Gerrit server. The "report()" function (if Gerrit has one) says a new reference "refs/for/master" has been created. But in deed, there is no such reference created in the remote repository, Gerrit will create another reference instead, such as "refs/changes/71/623871/1", for user to download the code review . Because the local repository only has normal "remote.<name>.fetch" config variables for remote tracking, so git will not create a tracking reference for "refs/for/master". Command line tool, such as Android "repo" (or the reimplemented git-repo in Golang), will create a special reference (refs/published/<local/branch>) for tracking, and these tools are responsible for banch tracking.

    $ git push --receive-pack="gerrit receive-pack" origin
refs/heads/master:refs/for/master
    Enumerating objects: 13, done.
    Counting objects: 100% (13/13), done.
    Delta compression using up to 8 threads
    Compressing objects: 100% (11/11), done.
    Writing objects: 100% (12/12), 1.34 KiB | 171.00 KiB/s, done.
    Total 12 (delta 2), reused 0 (delta 0), pack-reused 0
    remote: Resolving deltas: 100% (2/2)
    remote: Processing changes: refs: 1, new: 1, done
    remote:
    remote: SUCCESS
    remote:
    remote: New Changes:
    remote:   http://gerrit.example.com/c/my/repo/+/623889 Test commit
    To ssh://gerrit.example.com:29418/my/repo
     * [new branch]      master -> refs/for/master

Show 5 quoted lines

> A vanilla "git push" on the client side does not know a push to
> refs/for/master would result in an update to refs/heads/master on
> the server side, and it would not know the result of the munging
> done on the server side (whether it is to rebase what is received on
> top of 'master' or to merge it to 'master') anyway, the

Neither Gerrit nor our AGit-Flow server will update the master branch. Our AGit-Flow server will create a special reference (like GitHub's "refs/pull/<number>/head") for reviewers to download commits.

Show 12 quoted lines

> remote-tracking branch refs/remotes/origin/master on the client side
> would be left stale.  If we wanted to help them pretend to have
> fetched immediately after, I think we need to extend the protocol.
> Right now, after accepting "git push", the server end will say, for
> each proposed update for a ref, if the push updated successfully or
> not, but to support the "push to for/<target>, get heads/<target>
> updated" interaction, the reporting of the result (done in the
> report() function in builtin/receive-pack.c) needs to be able to say
> what ref (it may be a ref that "git push" did not think it pushed
> to) got updated to what value (it may be an object the client does
> not yet have---and we may have to actually turn around and fetch
> from them internally if we want to keep the illusion).

I have no idea now how to make a simple patch to give an accurate report.