Re: Fetch-hooks

From: Leo Gaspard <leo@gaspard.io>
Date: Feb 9, 2018, 23:49 UTC
Message-ID: <87e7c3b8-3b3c-1cb0-9b11-e4bf3044e539@gaspard.io>
In-Reply-To: <20180209223011.GA24578@sigill.intra.peff.net>

On 02/09/2018 11:30 PM, Jeff King wrote:

Show 24 quoted lines

> On Fri, Feb 09, 2018 at 11:04:17PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> One thing that's not discussed yet, and I know just enough about for it
>> to tingle my spidey sense, but not enough to say for sure (CC'd Jeff &
>> Brandon who know more) is that this feature once shipped might cause
>> higher load on git hosting providers.
>>
>> This is because people will inevitably use it in popular projects for
>> some custom filtering, and because you're continually re-fetching and
>> inspecting stuff what used to be a really cheap no-op "pull" most of the
>> time is a more expensive negotiation every time before the client
>> rejects the refs again, and worse for hosting providers because you have
>> bespoke ref fetching strategies you have less odds of being able to
>> cache both the negotiation and the pack you serve.
> 
> Most of the discussion so far seems to be about "accept this ref or
> don't accept this ref", which seems OK. But if you are going to do
> custom tweaking like rewriting objects, or making it common to refuse
> some refs, then I think things get pretty inefficient for _everybody_.
> 
> The negotiation for future fetches uses the existing refs as the
> starting point. And if we don't know that we have the objects because
> there are no refs pointing at them, they're going to get transferred
> again. That's extra load no the server, and extra time for the user
> waiting on the network.

Oh. I thought the protocol git used was something like client: I want to fetch refs A and B server: so you'll need objects 12345678 and 90ABCDEF, A and B both point to 12345678 client: please give me object 12345678 server: here it is [...]

I was clearly wrong, thanks! (and thanks Ævar for your explanation in the side-thread, too!)

Show 5 quoted lines

> I tend to agree with the direction of thinking you outlined: you're
> generally better off completing the fetch to a local namespace that
> tracks the other side completely, and then manipulating the local refs
> as you see fit (e.g., fetching into refs/quarantine, and then migrating
> "good" refs over to refs/remotes/origin).

Hmm... so do I understand it correctly when I say the process you're
thinking about works like this?
 * User installs hook for my-remote by running [something]
 * User runs git fetch
 * git fetch fetches remote refs/heads/* to local refs/quarantine/* (so
I guess [something] changes the remote.my-remote.fetch refmap)
 * When this is done `git fetch` runs a notification-only post-fetch
hook (that would need to be added)
 * The post-fetch hook then performs whatever it wants and updates the
references in refs/remotes/my-remote/*

So the changes that are required are:
 * Adding a notification-only post-fetch hook
 * For handling tags, there is a need to have a refmap for tags. Maybe
adding a remote.my-remote.fetchTags refmap, that would be used when
running with --tags, and having it default to “refs/tags/*:refs/tags/*”
to keep the current behavior by default?

The only remaining issue I can think of is: How do we avoid the issue of the trigger-only-hook-inciting-bad-behavior-by-hook-authors-who-really-want-modification raised in the side-thread that Junio wrote in [1]? Maybe just writing in the documentation that the hook should use a quarantine-like approach if it wants modification would be enough to not have hook authors try to modify the ref in the post-fetch hook?

Thanks for all your thoughts, and hope we're getting somewhere! Leo

PS: I'll read over the reviews once I'm all clear as to what exactly is
wanted for this patch, as most likely they'll just be dumped, given the
current state of affairs.

[1] https://marc.info/?l=git&m=132480559712592&w=2