Hi Brian
On 27/01/2024 21:22, Brian Lyles wrote:
On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 19/01/2024 05:59, brianmlyles@xxxxxxxxx wrote:
From: Brian Lyles <brianmlyles@xxxxxxxxx>
Both of these pages document very similar `--empty` options, but with
different styles. This commit aims to make them more consistent.
I think that's reasonable though the options they are worded as doing
different things. For "am" it talks about the patch being empty - i.e. a
patch of an empty commit whereas for "rebase" the option applies to
non-empty commits that become empty. What does "am" do if you try to
apply a patch whose changes are already present?
Hm -- as you mention, this does appear to have a different meaning for
git-am(1) than it does for git-rebase(1). Regardless of the `--empty`
value passed to git-am(1), a non-empty patch that is already present
appears to error and stop.
That is an unfortunate difference. I think that my updated version of
the git-am(1) docs is still easier to read, and preserves the original
meaning. So I'm inclined to say that it's still an improvement worth
making, and perhaps my commit message should just clarify that.
Thoughts?
Yes I agree the change is worthwhile but I think it would benefit from
an updated commit message.
If you're aiming for consistency then it would be worth listing the
possible values in the same order for each command.
That makes sense. I had initially maintained the existing order in which
these were documented, keeping the default option first. I think that
the updated layout makes the order less relevant by making it easier to
read and identify the default anyway.
I could see alphabetical being better, though with the changes later in
this series we'd end up with the deprecated `ask` being first or
out-or-order at the end. What are your thoughts on the ideal order for
these?
Alphabetical sounds reasonable, we could sort on the non deprecated
names with stop and ask grouped together
drop;;
...
keep;;
...
stop;;
ask;;
...
`ask` is a deprecated synonym of `stop`
+`keep`;;
+ The empty commit will be kept.
+`ask`;;
+ The rebase will halt when the empty commit is applied, allowing you to
+ choose whether to drop it, edit files more, or just commit the empty
+ changes. This option is implied when `--interactive` is specified.
Other options, like `--exec`, will use the default of drop unless
`-i`/`--interactive` is explicitly specified.
Thanks for adding a bit more detail about the default, however it looks
to me like we keep commits that become empty when --exec is specified
if (options.empty == EMPTY_UNSPECIFIED) {
if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
options.empty = EMPTY_STOP;
else if (options.exec.nr > 0)
options.empty = EMPTY_KEEP;
else
options.empty = EMPTY_DROP;
}
Off the top of my head I'm not sure why or if that is a good idea.
The two lines indicating this behavior are actually pre-existing -- I
did not change them in this patch and thus didn't even think to fact
check them.
Upon testing this, I've confirmed that you are correct about the actual
behavior. I will address this in a separate commit in v2.
Thanks, I'd missed that those were context lines in the diff.
Best Wishes
Phillip
