Re: [PATCH 01/11] Resumable clone: create service git-prime-clone

From: Kevin Wern <kevin.m.wern@gmail.com>
Date: Sep 28, 2016, 04:40 UTC
Message-ID: <20160928044005.GA3762@kwern-HP-Pavilion-dv5-Notebook-PC>
In-Reply-To: <xmqqzin7in2c.fsf@gitster.mtv.corp.google.com>

On Fri, Sep 16, 2016 at 01:53:15PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Kevin Wern <kevin.m.wern@gmail.com> writes:
> 
> > Create git-prime-clone, a program to be executed on the server that
> > returns the location and type of static resource to download before
> > performing the rest of a clone.
> >
> > Additionally, as this executable's location will be configurable (see:
> > upload-pack and receive-pack), add the program to
> > BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
> > git-prime-clone executable to gitignore, as well
> >
> > Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
> > ---
> 
> I wonder if we even need a separate service like this.
> 
> Wouldn't a new protocol capability that is advertised from
> upload-pack sufficient to tell the "git clone" that it can
> and should consider priming from this static resource?

The short answer is yes, it could be done that way. Both methods--extending upload-pack and creating a new service--were suggested in different discussions.

However, my thought was to implement the separate service because:
	- It is much easier for an admin trying the feature to sanity check the
	  output of an executable, compared to passing messages to upload-pack.
	- In the other scenario, upload-pack might get too expansive in size
	  and scope--not only codewise, but in terms of config namespace if
	  "uploadpack" concerns too many things that are only tangentially
	  related (the properties of the primer resource).
	- The transport_prime_clone API can be called independent of other
	  transport API functions, which might prove useful when revisiting or
	  refactoring code.
	- You favored the creation of a service in our original discussion [1].
	  I'm not sure if your reasoning was similar to mine.

It definitely was a tight decision--for me, ultimately weighing the value added in usability (point 1) against the need for a "failsafe" implementation. All the other points are more speculative, IMO, but the first was strong enough for me.

What do you think?

[1] http://www.spinics.net/lists/git/msg269992.html

Show 8 quoted lines

> Two minor comments:
> 
>  - For whom are you going to localize these strings?  This program
>    is running on the server side and we do not know the locale
>    preferred by the end-user who is sitting on the other end of the
>    connection, no?
> 
>  - Turn "}\n\s+else " into "} else ", please.

These are fair points. Changing for the revised version.

- Kevin