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.
On reflection that is only true for (B). I agree that we should error
out on (A) which we don't at the moment.
I'd support a change that errors out on (A) and (C) but continues to
allow (B) and (D). I think we can do that with the diff below
Best Wishes
Phillip
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5..66aef356b8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const
char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
- /*
- * --keep-base defaults to --reapply-cherry-picks to avoid losing
- * commits when using this option.
- */
- if (options.reapply_cherry_picks < 0)
- options.reapply_cherry_picks = keep_base;
if (options.root && options.fork_point > 0)
die(_("options '%s' and '%s' cannot be used
together"), "--root", "--fork-point");
@@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv,
const char *prefix)
if (options.empty != EMPTY_UNSPECIFIED)
imply_merge(&options, "--empty");
- /*
- * --keep-base implements --reapply-cherry-picks by altering
upstream so
- * it works with both backends.
- */
- if (options.reapply_cherry_picks && !keep_base)
+ if (options.reapply_cherry_picks < 0)
+ /*
+ * --keep-base defaults to --reapply-cherry-picks to
+ * avoid losing commits when using this option.
+ */
+ options.reapply_cherry_picks = keep_base;
+ else if (!keep_base)
+ /*
+ * --keep-base implements --reapply-cherry-picks by
+ * altering upstream so it works with both backends.
+ */
imply_merge(&options, "--reapply-cherry-picks");
if (gpg_sign)
