Hi Elijah
On 24/01/2023 02:36, Elijah Newren wrote:
Hi Phillip,
On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
Hi Elijah
On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <newren@xxxxxxxxx>
The git-rebase manual noted several sets of incompatible options, but
we were missing tests for a few of these. Further, we were missing
code checks for some of these, which could result in command line
options being silently ignored.
Also, note that adding a check for autosquash means that using
--whitespace=fix together with the config setting rebase.autosquash=true
will trigger an error. A subsequent commit will improve the error
message.
Thanks for updating the commit message and for the new commits at the
end of the series.
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
+ /*
+ * The apply backend does not support --[no-]reapply-cherry-picks.
+ * The behavior it implements by default is equivalent to
+ * --no-reapply-cherry-picks (due to passing --cherry-picks to
+ * format-patch), but --keep-base alters the upstream such that no
+ * cherry-picks can be found (effectively making it act like
+ * --reapply-cherry-picks).
+ *
+ * Now, if the user does specify --[no-]reapply-cherry-picks, but
+ * does so in such a way that options.reapply_cherry_picks ==
+ * keep_base, then the behavior they get will match what they
+ * expect despite options.reapply_cherry_picks being ignored. We
+ * could just allow the flag in that case, but it seems better to
+ * just alert the user that they've specified a flag that the
+ * backend ignores.
+ */
I'm a bit confused by this. --keep-base works with either
--reapply-cherry-picks (which is the default if --keep-base is given) or
--no-reapply-cherry-picks. Just below this hunk we have
if (options.reapply_cherry_picks < 0)
options.reapply_cherry_picks = keep_base;
So we only set options.reapply_cherry_picks to match keep_base if the
user did not specify -[-no]-reapply-cherry-picks on the commandline.
options.reapply_cherry_picks is totally ignored by the apply backend,
regardless of whether it's set by the user or the setup code in
builtin/rebase.c. And if we have an option which is ignored, isn't it
nicer to provide an error message to the user if they tried to set it?
Said another way, while users could start with these command lines:
(Y) git rebase --whitespace=fix
(Z) git rebase --whitespace=fix --keep-base
and modify them to include flags that would be ignored, we could allow:
(A) git rebase --whitespace=fix --no-reapply-cherry-picks
(B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks
But we could not allow commands like
(C) git rebase --whitespace=fix --reapply-cherry-picks
(D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks
(C) is already an error
(D) is currently allowed and I think works as expected (--keep-base only
implies --reapply-cherry-picks, the user is free to override that with
--no-reapply-cherry-picks) so I don't see why we'd want to make it an error.
For all four cases (A)-(D), the apply backend will ignore whatever
--[no-]reapply-cherry-picks flag is provided.
For (D) the flag is respected, (C) errors out, the other cases
correspond to the default so it's like saying
git rebase --merge --no-reapply-cherry-picks
ignores the flag. Arguably it is confusing that the apply backend only
supports -[-no]-reapply-cherry-picks if --keep-base is given but I'm not
sure that is a good reason to reject a combination that currently works
as expected.
Best Wishes
Phillip
For (A) and (B), the > behavior the apply backend provides happens to match what the user
is requesting, while for (C) and (D) the behavior does not match.
So we should at least reject (C) and (D). But, although we could
technically allow (A) and (B), what advantage would it provide? I
think the results of allowing those two commands would be:
1) Confusion by end users -- why should (C) & (D) throw errors if
(A) and (B) are accepted? That's not an easy rule to understand.
2) More confusion by end users -- the documentation for years has
stated that --reapply-cherry-picks is incompatible with the apply
backend, suggesting users would be surprised if at least (B) and
probably (A) didn't throw error messages.
3) Confusing documentation -- If we don't want to throw errors for
(A) and (B), how do we modify the "INCOMPATIBLE OPTIONS" section
of Documentation/git-rebase.txt to explain the relevant details
of when these flags are (or are not) incompatible with the apply
backend? I think it'd end up with a very verbose explanation
that likely confuses more than it helps.
4) Excessively complicated code -- The previous attempts to
implement this got it wrong. Prior to ce5238a690 ("rebase
--keep-base: imply --reapply-cherry-picks", 2022-10-17), the code
would error out on (B) and (C). After that commit, it would only
error out on (C). Both solutions are incorrect since they miss
(D), and I think the code just becomes hard to hard to follow in
order to only error out on both (C) and (D) without (A) and (B).
(#2 and #3 might just be a repeat of the same issue, documentation,
but it seemed easier to write separately.)
I think it's simpler for the code, for the documentation, and for end
users to just error out on all of (A), (B), (C), and (D).
--[no-]reapply-cherry-picks is not supported by the apply backend.
But, given this lengthy email, perhaps I should split out the handling
of --[no-]reapply-cherry-picks into its own commit and copy some or
all of the description above into the commit message?
