Re: [RFC PATCH v2 08/16] remote-helpers: Support custom transport options

On Tue, 13 Oct 2009, Shawn O. Pearce wrote:

Daniel Barkalow [off-list ref] wrote:

quoted

quoted

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
+'option' <name>::
+	This helper supports the option <name> under fetch-multiple.
+

I'm a bit surprised that the options only apply in a fetch-multiple 
section, rather than getting set at the beginning and applying to 
everything for that run. At least, I think an "option" command should be 
useable outside of a fetch-multiple (or possible future grouping 
construct) and have global scope.

In hindsight, I agree with you.

I'll respin the series so the set_option method in the transport
forwards the options immediately to the helper and lets the helper
decide whether it accepts or rejects the option string.  This will
clean up the capabilities interface since we no longer need to dump
the list of options we support in the helper, and as you point out,
it will make a lot more sense to just set the options for this
transport instance.

Great. That also makes the "fetch-multiple" start flag no longer 
strictly necessary.

quoted

quoted

 REF LIST ATTRIBUTES
 -------------------
 

@@ -76,10 +80,26 @@ None are defined yet, but the caller must accept any which are supplied.
 
 FETCH OPTIONS
 -------------
+To enable an option the helper must list it in 'capabilities'.
 
 'option verbose'::
 	Print more verbose activity messages to stderr.

I think you mis-split the above part; your previoud patch declared this 
option without declaring any way to use it. Might be worth allowing 
multiple "verboses" and "quiet" or "option verbosity quiet"/"option 
verbosity verbose verbose".

Hmmph.  "option verbosity verbose verbose" is a bit verbose, don't
you think?  :-)

I think we should just forward the verbosity setting from the
frontend: "option verbosity [0-n]" where n is the number of
times -v appeared on the command line/how verbose the user wants.

I think it can go down to -1. Also, I remember needing to have a "-v" 
get eaten by the fetch/pull itself, so that you can have fetch/pull tell 
you stuff without having the actual protocol interaction getting more 
verbose, so it's not exactly the same numbers that get through.

quoted

quoted

+'option uploadpack' <command>::
+	The program to use on the remote side to generate a pack.

I sort of feel like the helper ought to read this one out of the config 
file itself if it wants it.

Eh, true, but you can also set this on the command line.  An open
question I still have for myself is how to set this in HTTP
transports.

Good point. I doubt people really want to change the executable name for 
the same remote between different runs, though.

The reason why I care is Gerrit Code Review has overloaded the
'git-receive-pack' executable and taught it more command line flags:

  $ ssh r git receive-pack -h
  git receive-pack PROJECT.git [--cc EMAIL ...] [--help (-h)] [--reviewer (--re) EMAIL ...]

   PROJECT.git             : project name
   --cc EMAIL              : CC user on change(s)
   --help (-h)             : display this help text
   --reviewer (--re) EMAIL : request reviewer for change(s)

Which is typically invoked as:

  git push --receive-pack "git-receive-pack --reviewer spearce@spearce.org" URL REFSPEC

It seems to me like what we really want is a channel to communicate extra 
information to hooks on the server side (or a modified command on the 
server). Like:

git push --recv-opt "--reviewer spearce@spearce.org"

or something like that. This would arrange to pass that option to whatever 
server-side code is responsible for accepting pushes, without specifying 
how it gets there.

Folks actually have scripts which make this invocation for them, so
they can insert in the proper reviewer and/or cc arguments.  Since
the arguments vary its hard to set this up in the configuration file.

Over SSH this is fine, we obtain the arguments off the SSH command
line string and its no big deal.  Over git:// this would fail as
git-daemon can't parse the line anymore.  Over HTTP this also is not
going to work since the service can't receive arbitrary arguements.

My primary motivator for doing smart HTTP now is folks who are
stuck behind firewalls that permit only HTTP through their local
proxy servers are unable to communicate with a Gerrit Code Review
instance over SSH on port 29418.  That --reviewer flag above is a
very useful feature of Gerrit that I somehow have to support for
the HTTP transport too.

I started down the road of currying this data into the backend by
at least exposing the option to the helper.  How the helper reads
and uses it is up to the helper.

But given that the value can come in from the command line or from
the configuration file, I think this should be handled by fetch
or push porcelain and fed through the helper protocol, and not be
something that the helper reads from the config directly.

I agree that the porcelain and transport.c should handle "--reviewer 
spearce@spearce.org", but I still think they shouldn't handle the
"git-receive-pack" part, and it would probably be easier on everyone if it 
was separated in the porcelain, and the native transport knew to stick it 
into the receive-pack command line.

quoted

In general, it would be good to have 
transport.c and remote.c out of the business of knowing this sort of 
protocol-specific (albiet specific now to two protocols) information. (Of 
course, the native protocol's transport methods are in transport.c, so 
that's there, but I'd like to move that to a transport-native.c someday.)

Agreed, but I have no solution for you due to the --receive-pack
and --upload-pack arguments supported by the command line git push
and git fetch/pull porcelain.

But I have been trying to extend the helper interface in a way
that would allow us to eject the native transport code entirely
into a helper.  We may never bother, there are some advantages to
being in the push/fetch client process, but I also didn't want to
get stuck in a corner.

I think with my series we do almost everything we need to support
native git:// in an external helper process rather than builtin.
We honor the pack lock file system used by fetch to maintain safe
concurrent mutations.  We use push_refs API and signal back the
complete information from the remote side.  We permit arbitrary
message strings per ref to be returned by the helper.  Etc.

I think it would be a worthwhile exercise to actually write the series to 
eject all of the transports into helpers. Then I think we should probably 
make the extensions to the helper code this requires, eject the rsync one, 
and put bundles and native in contrib instead of using them.

quoted

quoted

+'option followtags'::
+	Aggressively fetch annotated tags if possible.

I assume this means to fetch tags which annotate objects we have or are 
fetching? (As opposed to fetching any annotated tag we could possibly 
fetch, even if we don't otherwise care about the tag or the thing it 
tags.) It's obvious in the context of git's config options, but I'd like 
this document to avoid assuming that context, and the option could apply 
more generally.

Yes.  I'll extend the documentation further in the next iteration.
 

quoted

quoted

+'option thin'::
+	Transfer the data as a thin pack if possible.

Does anyone still use non-default thinness? 

Its a command line option on the porcelain.  Until we remove
the command line flag I think we should still try to honor it
in implementations that understand that notion.

Actually, the command line supports turning it on, and it defaults to on. 
So I think your helper can safely assume that it's on. :)

	-Daniel
*This .sig left intentionally blank*