Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options

From: Stefan Beller <sbeller@google.com>
Date: Jul 7, 2016, 21:50 UTC
Message-ID: <CAGZ79kb_oHZUv1=8Q0FSV8yVP5PrwStchXAyhZF=mHhv91APHQ@mail.gmail.com>
In-Reply-To: <xmqqeg75p5cn.fsf@gitster.mtv.corp.google.com>

On Thu, Jul 7, 2016 at 1:20 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> What is suboptimal about the structure of the series is that we
> won't bisect down to any of the potential bugs in the above code
> even if we ever see any bug in the future.
> It also does not hint
> where push_options is expected to be read in the code in the
> subsequent patches in the series.  If I were doing this series, I
> would probably have done 2/4 first without plumbing it through
> (i.e. it is sent and accumulated in a string list at the receiver,
> and then cleared and freed without being used), and then added the
> processing (i.e. this step) as the second patch.

But your first patch (2/4) would not yet advertise the capability? Or advertise and then just ignoring it?

That shadows other bugs that would not properly bisect, I'd imagine?

It is better for documentation purposes in this patch though. It makes the other patch harder as "it allows transmitting push options, but in that patch nothing of value is done with them."

So I'll see if I can reorder easily.

Thanks, Stefan