Re: [PATCH 2/3] sequencer: make commit options more extensible

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2017, 16:04 UTC
Message-ID: <xmqqa88cf08d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c0c836e1f1739af25ad875419ee6125b53ac6a1a.1490194846.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 15 quoted lines

> It was pointed out during review of the sequencer-i patch series (which
> taught the sequencer to execute an interactive rebase) that it may be
> cumbersome to keep extending the signature of the run_git_commit()
> function whenever a new commit option is needed.
>
> While that concern had merit, back then I was reluctant to change even
> more than was already asked for (which typically introduces regressions,
> this late in the review process, which is no fun for nobody).
>
> Now, with fresh eyes, and with an actual need, is a good time to change
> the strategy from adding individual flag parameters to coalescing them
> into a single flags parameter.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I think the first two paragraphs are not about explaining this commit. You do not have to justify the past---some suggestions given during the review may end up not getting followed for different reasons, which may be good or bad, but what happened has happened, and the backstory is not useful for understanding why this is a good change to a reader of "git log".

What belongs to the explanation / justification for this change is that the interface to run_git_commit() that forces to add one boolean parameter whenever a new "switch" is required is cumbersome and it is better to use a flags word that is a collection of bits. When you reroll this for s/signed int/unsigned int/, please just describe that in the log. Having the backstory under three-dash lines may help reviewers who remember the original topic that introduced the maintenance burden this commit fixes.

Thanks.