Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options

[Brian Lyles <brianmlyles@gmail.com>]

On Sat, Jan 20, 2024 at 3:38 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:

Hi Kristoffer,
Thank you for taking some time to review my patch.

> Initial discussion (proposal): https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/

I ought to have included this in a cover letter -- thanks for linking it
here, and I will include this with v2.

> On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> >
> > Previously, a consumer of the sequencer that wishes to take advantage of
> > either the `keep_redundant_commits` or `drop_redundant_commits` feature
> > must also specify `allow_empty`.
>
> Previously to this change? It is preferred to describe what the code
> currently does without this change in the present tense.[1] The change
> itself uses the imperative mood.[2]
>
> † 1: SubmittingPatches, “The problem statement that describes the status
>     quo …”
> † 2: SubmittingPatches, “Describe your changes in imperative mood […] as
>     if you are giving orders to the codebase to change its behavior.”

I appreciate the stylistic feedback here. I will tweak this for v2 of
the patch.

> > In a future commit, an `--empty` option will be added to
> > `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> > available in that command. For that to be possible with the current
> > implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> > would need to specify `allow_empty` with `drop_redundant_commits` as
> > well, which is an even less intuitive implication of `--allow-empty`: in
> > order to prevent redundant commits automatically, initially-empty
> > commits would need to be kept automatically.
> >
> > Instead, this commit rewrites the `allow_empty()` logic to remove the
> > over-arching requirement that `allow_empty` be specified in order to
> > reach any of the keep/drop behaviors. Only if the commit was originally
> > empty will `allow_empty` have an effect.
>
> In general, phrases like “this commit <verb>” or “this patch <verb>” can
> be rewritten to the “commanding” style (see [2]).[3] But here you’re
> starting a new paragraph after having talked about a future commit, so
> using the commanding style might be stylistically difficult to pull off
> without breaking the flow of the text.
>
> And “this [commit][patch] <verb>” seems to be used with some regularity in
> any case.
>
> 🔗 3: https://lore.kernel.org/git/xmqqedeqienh.fsf@gitster.g/

I think I should be able to adjust this to a more commanding style
without breaking the flow. I'll give that a shot in v2 as well.

> > Disclaimer: This is my first contribution to the git project, and thus
> > my first attempt at submitting a patch via `git-send-email`. It is also
> > the first time I've touched worked in C in over a decade, and I really
> > didn't work with it much before that either. I welcome any and all
> > feedback on what I may have gotten wrong regarding the patch submission
> > process, the code changes, or my commit messages.
>
> This part (after the commit message) looks like the cover letter for the
> series (the four patches). `SubmittingPatches` recommends submitting that
> in a dedicated email message (for series that have more than one
> patch). Maybe this cover letter style is just an alternative that is
> equally accepted. But most series use a separate cover letter message for
> what it’s worth.

That makes sense -- when I send v2, I can pull this part out into a
separate cover letter email.

I will hold off on sending out v2 for a few days to give others a chance
to weigh in on this series.

Thanks again,
Brian Lyles