On Sat, Jan 20, 2024 at 3:38 PM Kristoffer Haugsbakk <code@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ 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@xxxxxxxxx wrote: > > From: Brian Lyles <brianmlyles@xxxxxxxxx> > > > > 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
