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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johannes Schindelin <johannes.schindelin@xxxxxx> writes:

> 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@xxxxxx>
> ---

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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]