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

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

Jiang Xin <worldhello.net@gmail.com> writes:

> It would be more convenient to work in a centralized workflow like what
> Gerrit provided for some cases.  For example, a read-only user may run
> the following `git push` command to push commits to a special reference
> to create a code review, instead of updating a reference directly.
>
>     git push -o reviewers=user1,user2 \
>         -o oldoid=89c082363ac950d224a7259bfba3ccfbf4c560c4 \
>         origin \
>         HEAD:refs/for/<branch-name>/<session>
>
> The `<branch-name>` in the above example can be as simple as "master",
> or a more complicated branch name like "foo/bar".  The `<session>` in
> the above example command can be the local branch name of the clien-
> side, such as "my/topic".
>
> To support this kind of workflow in CGit, add a filter and a new
> handler.  The filter will check the prefix of the reference name, and
> if the command has a special reference name, the filter will add a
> specific tag (`exec_by_hook`) to the command.  Commands with this
> specific tag will be executed by a new handler (an external hook named
> "execute-commands") instead of the internal `execute_commands` function.

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

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

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

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.

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?  

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