Re: [RFC PATCHv1 0/4] Push options in C Git

From: Stefan Beller <sbeller@google.com>
Date: Jul 1, 2016, 17:07 UTC
Message-ID: <CAGZ79kY7j3r5b0rz=Lb6Ec2s2f8=UBb-hTtZw0H09KM_9H+7=Q@mail.gmail.com>
In-Reply-To: <20160701070902.GF5358@sigill.intra.peff.net>

On Fri, Jul 1, 2016 at 12:09 AM, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Wed, Jun 29, 2016 at 05:59:47PM -0700, Stefan Beller wrote:
>
>> Allow a user to pass information along a push to the pre/post-receive hook
>> on the remote.
>
> Seems like a reasonable thing to want, and related to:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/285124
>
> which was specifically interested in making "--quiet" and "--force"
> available, but stopped short of arbitrary data. (It looks like that
> topic got overlooked, and Dennis never prodded again).

Doh! I should have searched for such a topic. :/ I looked at the patches and they differ conceptually in what goes over the wire.

In Dennis series that is a hook_options=<string> in the capability selection line, that is after the first update line, i.e. the hook options are restricted to not contain white spaces and be of length < ~60k characters IIUC (reserve about 4k for the update, ref and other selected capabilities)

In this proposal we only indicate that we want to transmit options in the negotiation phase, and then after the update transmission is done, you can have as many packets as you want excluding flush packets (the flush packet indicates we're done transmitting these push/hook options)

So I might be biased by my code, but I'd claim that is more future proof.

Show 10 quoted lines

>
>> When using a remote that is more than just a plain Git host (e.g. Gerrit,
>> Git{hub/lab}, etc) this may become more obvious: The (server backend specific)
>> push options can instruct the server to:
>> * open a pull request
>
> I know this is just a "for example", but I don't know if we would ever
> support opening a pull request directly via push, if only because a good
> pull request has actual parameters that the user needs to fill out (like
> the cover letter message).

Sure, just an example! You could have

    "git push --read-push-option-from-file=coverletter.html".

as this really only disallows flush packets (i.e. anything goes, including images) and the server must be smart enough to interpret the file.

Show 5 quoted lines

>
> So you'd probably want some client tool to help the user figure out what
> to put in the PR, and of course that already exists, because GitHub has
> an HTTP API that it can talk to. Shoe-horning it into Git doesn't buy
> much.

Let's say the example was bad. the reality is that Dennis and I implemented a needed feature independently of each other and you have the out-of-the-box-Github-does-HTTP-too solution. So I think we'd want *something* in Git to have information in the receiving hooks available?

It seems to me there are many other Git-wrappers, such as the repo tool or git-review[2], and most of them start with the premise: "Git doesn't support X yet, so while they take their time to figure out how to do it properly upstream, we'll just use this hacky script" and then eventually you end up with an xml parsing beast with thousands lines of python code (The story of the repo tool and submodules).

[1] https://code.google.com/p/git-repo/ [2] http://linux.die.net/man/1/git-review

>
> I'm not sure if your other examples would be better served by just using
> an HTTP API or not.

The decision for the HTTP API would be the decision for a wrapper tool. And all these wrapper tools differ, so it is not easy to switch from one to another, on the other hand, if you know Git well enough you can easily adapt to other work flows as it is not a new tool, but just a new command line option or so?

> I think Gerrit is funny in this regard because it
> eschews branches entirely. E.g., in a GitHub PR, you push to branch
> "foo", and then you open a PR using "foo" as the source.

Once upon a time you could also open a pull request using the sha1? (I did that once and then was asked to do some fixes before pulling and I had to abandon and reopen a proper branch PR)

> But with
> Gerrit, you push to the magic refs/for/master, and you have no real way
> to cross-reference that submission later.

What do you mean by cross reference?

I could ask you to look at https://gerrit-review.googlesource.com/#/c/79354/ and in the upper right corner you'll find a download button that let's you copy "git fetch <URL> refs/changes/54/79354/3 && git checkout FETCH_HEAD". That allows you to inspect that thing alone later (or any of the revisions as the structure is refs/changes/<2 digit sharding>/<change id>/<revision count> "revision" translates to the concept of resending a patch series v2,v3 here)

Alternatively I could ask you to look for a "Change-Id" that is a hash in the footer of a commit, so you can find it in the Git-history or on the review site.

Gerrits flaws are off topic to this series though (maybe?)

>
> Whereas in Dennis's patches, it was about specific information directly
> related to the act of pushing.

But does it cover all the informations related to pushing? like

  "I am a bot, down-prioritize me, please"
  "I am a bot, therefore I do not care about the internal replication
on the server"

The last one is interesting as it is very specific to our servers. So you could argue we'd want to roll our own fork of Git that also covers such a hook option, but I think it is easier for both the Git community as well as us here to allow Git transmit free form text and the server can decide how to act upon it?

Show 11 quoted lines

>
>> * When pushing to Gerrit you can already attach some of this information by
>>   adding a '%' followed by the parameter to the ref, i.e. when interacting with
>>   Gerrit it is possible to do things like[1]:
>>
>>     git push origin HEAD:refs/for/master%draft%topic=example%cc=jon.doe@example.org
>>
>>   This is not appealing to our users as it looks like hacks upon hacks to make
>>   it work. It would read better if it was spelled as:
>
> Heh. It _is_ hacks upon hacks, isn't it? ;)

I do not disagree. Now is the time to do it right, but we like the convenience of having just one tool (Git) that can do it all for us. We don't want to write yet another wrapper script that does the HTTP API thing. ;)

I might have missed the point of this email completely as I seem to try to defend the patch series (and Gerrit a bit). So do you think the functionality of this series is overkill and we'd rather go with Dennis series?

I just think that is not future proof on the wire as it restricts what you can put on the wire as well as not future proof as it specifies what you options you can select over the wire.

Gits spirit (Oh no, not this discussion!) is to allow a broad range of uses and work flows. It doesn't hinder you from shooting into your feet if that's what you're into, and that is what this series does precisely.

>
> -Peff

Thanks, Stefan