| Message ID | a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: mark --update-refs as requiring the merge backend | expand |
On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM] > to git-am", 2007-02-08). Based on feedback on the patch, the author > added the -C option not just to git-am but also to git-rebase. But did > it such that the option was just passed through to git-am (which passes > it along to git-apply), with no corresponding option to format-patch. > > As per the git-apply documentation for the `-C` option: > Ensure at least <n> lines of surrounding context match...When fewer > lines of surrounding context exist they all must match. > > The fact that format-patch was not passed a -U option to increase the > number of context lines meant that there would still only be 3 lines of > context to match on. So, anyone attempting to use this option in > git-rebase would just be spinning their wheels and wasting time. I was > one such user a number of years ago. > > Since this option can at best waste users time and is nothing more than > a confusing no-op, and has never been anything other than a confusing > no-op, and no one has ever bothered to create a testcase for it that > goes beyond option parsing, simply excise the option. > > Signed-off-by: Elijah Newren <newren@gmail.com> Is there a chance that this patch could break some existing tooling or someone's workflow or alias? If so, perhaps it would be better to continue recognizing it, but as a proper no-op? That is: (1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such (2) completely ignore the option (3) in case someone runs across it in some existing script or example and wants to know what it does, leave one mention of it in the documentation, such as: -C<n> Does nothing. This option is accepted for backward compatibility only. Do not use.
On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM] > > to git-am", 2007-02-08). Based on feedback on the patch, the author > > added the -C option not just to git-am but also to git-rebase. But did > > it such that the option was just passed through to git-am (which passes > > it along to git-apply), with no corresponding option to format-patch. > > > > As per the git-apply documentation for the `-C` option: > > Ensure at least <n> lines of surrounding context match...When fewer > > lines of surrounding context exist they all must match. > > > > The fact that format-patch was not passed a -U option to increase the > > number of context lines meant that there would still only be 3 lines of > > context to match on. So, anyone attempting to use this option in > > git-rebase would just be spinning their wheels and wasting time. I was > > one such user a number of years ago. > > > > Since this option can at best waste users time and is nothing more than > > a confusing no-op, and has never been anything other than a confusing > > no-op, and no one has ever bothered to create a testcase for it that > > goes beyond option parsing, simply excise the option. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > Is there a chance that this patch could break some existing tooling or > someone's workflow or alias? If so, perhaps it would be better to > continue recognizing it, but as a proper no-op? That is: > > (1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such > > (2) completely ignore the option > > (3) in case someone runs across it in some existing script or example > and wants to know what it does, leave one mention of it in the > documentation, such as: > > -C<n> > Does nothing. This option is accepted for backward compatibility > only. Do not use. If an option becomes a no-op over time due to other changes, this would likely be the route I'd lean towards. I'm just not sure it's merited in this case. We already turned numerous no-op choices in rebase into errors with commit 8295ed690b ("rebase: make the backend configurable via config setting", 2020-02-15), where we started pointing out that apply-based and merge-based options were incompatible (as opposed to silently accepting competing options and ignoring ones not supported by whichever backend happened to trump at the time based on the options.) So, there's recent precedent to jump directly to errors with no-ops in rebase. Those cases are slightly different in that options were only conditionally no-ops, whereas in this case we have an option that is unconditionally a no-op, suggesting we might be able to do something stronger. On top of that, I just can't find any evidence of anyone ever using this option: (a) it was only put in as an afterthought by the original author (who was only really interested in git-am -C), (b) there are absolutely no testcases in the testsuite covering its behavior, (c) my searches of the mailing list and google don't turn up any hits though admittedly it's hard to figure out what to search on, and (d) while I tried to use the option at times, it was only because I was doing work to make backends consistent and switch the default one and trying to understand all the inner workings, and even then I gave up on it and punted it down the road. And, of course, the option never worked as advertised anyway. That combination makes me think nuking it would go completely unnoticed outside this mailing list. If others would rather see the more cautious route despite all the above, I can implement it, but likely alter your step (2) from ignoring into throwing an error message (or at the very least a warning).
On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > As per the git-apply documentation for the `-C` option: > Ensure at least <n> lines of surrounding context match...When fewer > lines of surrounding context exist they all must match. > > The fact that format-patch was not passed a -U option to increase the > number of context lines meant that there would still only be 3 lines of > context to match on. So, anyone attempting to use this option in > git-rebase would just be spinning their wheels and wasting time. I was > one such user a number of years ago. I suppose someone might have something like GIT_DIFF_OPTS="--unified=20" meaning they would actually have more context for their `-C` to act on. So I guess there is a chance that someone somewhere has actually been able to make use of `git rebase -C` after all? I'm not really arguing either way -- just noting the possibility, as remote as it may be. Martin
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM] > to git-am", 2007-02-08). > ... > As per the git-apply documentation for the `-C` option: > Ensure at least <n> lines of surrounding context match...When fewer > lines of surrounding context exist they all must match. > > The fact that format-patch was not passed a -U option to increase the > number of context lines meant that there would still only be 3 lines of > context to match on. I am afraid that this is only less than half true. Isn't the intended use of -C<num> similar to how "patch --fuzz" is used? That is, even when a patch does not cleanly apply with full context in the incoming diff, by requiring *smaller* number of lines to match, the diff *could* be forced to apply with reduced precision? My read of "even if context has changed a bit" in the log of that commit is exactly that. And for such a use case (which I think is the primary use case for the feature), you do not need to futz with the patch generation side at all. commit 67dad687ad15d26d8e26f4d27874af0bc0965ce2 Author: Michael S. Tsirkin <mst@kernel.org> Date: Thu Feb 8 15:57:08 2007 +0200 add -C[NUM] to git-am Add -C[NUM] to git-am and git-rebase so that patches can be applied even if context has changed a bit. Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il> Signed-off-by: Junio C Hamano <junkio@cox.net>
On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM] > > to git-am", 2007-02-08). > > ... > > As per the git-apply documentation for the `-C` option: > > Ensure at least <n> lines of surrounding context match...When fewer > > lines of surrounding context exist they all must match. > > > > The fact that format-patch was not passed a -U option to increase the > > number of context lines meant that there would still only be 3 lines of > > context to match on. > > I am afraid that this is only less than half true. Isn't the > intended use of -C<num> similar to how "patch --fuzz" is used? > > That is, even when a patch does not cleanly apply with full context > in the incoming diff, by requiring *smaller* number of lines to > match, the diff *could* be forced to apply with reduced precision? Oh! Reducing the number of lines of context to pay attention to never even occurred to me for whatever reason. I'll drop the patch.
On Fri, Jan 20, 2023 at 1:55 AM Martin Ågren <martin.agren@gmail.com> wrote: > > On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > As per the git-apply documentation for the `-C` option: > > Ensure at least <n> lines of surrounding context match...When fewer > > lines of surrounding context exist they all must match. > > > > The fact that format-patch was not passed a -U option to increase the > > number of context lines meant that there would still only be 3 lines of > > context to match on. So, anyone attempting to use this option in > > git-rebase would just be spinning their wheels and wasting time. I was > > one such user a number of years ago. > > I suppose someone might have something like GIT_DIFF_OPTS="--unified=20" > meaning they would actually have more context for their `-C` to act on. > So I guess there is a chance that someone somewhere has actually been > able to make use of `git rebase -C` after all? I'm not really arguing > either way -- just noting the possibility, as remote as it may be. > > Martin Ah, good point. And combined with Junio's point that -C is apparently about reducing rather than increasing context, I should just drop the patch.
Elijah Newren <newren@gmail.com> writes: > On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote: >> ... >> That is, even when a patch does not cleanly apply with full context >> in the incoming diff, by requiring *smaller* number of lines to >> match, the diff *could* be forced to apply with reduced precision? > > Oh! Reducing the number of lines of context to pay attention to never > even occurred to me for whatever reason. I'll drop the patch. Yup. "completely useless" on the title is less than half correct, but still correct for a minor use case where -C is used to use *more* context lines than the default. We could update "rebase --apply" codepath to increase the context lines generated by format-patch. That would make the "completely useless" totally incorrect ;-)
On Fri, Jan 20, 2023 at 8:16 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote: > >> ... > >> That is, even when a patch does not cleanly apply with full context > >> in the incoming diff, by requiring *smaller* number of lines to > >> match, the diff *could* be forced to apply with reduced precision? > > > > Oh! Reducing the number of lines of context to pay attention to never > > even occurred to me for whatever reason. I'll drop the patch. > > Yup. "completely useless" on the title is less than half correct, > but still correct for a minor use case where -C is used to use > *more* context lines than the default. > > We could update "rebase --apply" codepath to increase the context > lines generated by format-patch. That would make the "completely > useless" totally incorrect ;-) haha :-) I'd probably go for that, but since in my mind the plan is still to deprecate and remove the apply backend as you suggested at [1], I'm not particularly motivated to improve/extend options specific to the apply backend of rebase. [1] https://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/
Elijah Newren <newren@gmail.com> writes: > I'd probably go for that, but since in my mind the plan is still to > deprecate and remove the apply backend as you suggested at [1], I'm > not particularly motivated to improve/extend options specific to the > apply backend of rebase. I still consider that deprecating and removing the "am|format-patch pipeline" mode is a good longer term goal, but it seems we still have some features that are only available with the mode and not the other "sequence of cherry-pick" mode? We'd need to port that over to the other backend before we can wean ourselves off of the apply backend. Until then, ...
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index f9675bd24e6..412887deda7 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -416,14 +416,6 @@ include::rerere-options.txt[] Allows the pre-rebase hook to run, which is the default. This option can be used to override `--no-verify`. See also linkgit:githooks[5]. --C<n>:: - Ensure at least `<n>` lines of surrounding context match before - and after each change. When fewer lines of surrounding - context exist they all must match. By default no context is - ever ignored. Implies `--apply`. -+ -See also INCOMPATIBLE OPTIONS below. - --no-ff:: --force-rebase:: -f:: @@ -631,7 +623,6 @@ The following options: * --apply * --whitespace - * -C are incompatible with the following options: diff --git a/builtin/rebase.c b/builtin/rebase.c index 1481c5b6a5b..ace8bd4a41c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1067,8 +1067,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("ignore author date and use current date")), OPT_HIDDEN_BOOL(0, "ignore-date", &options.ignore_date, N_("synonym of --reset-author-date")), - OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"), - N_("passed to 'git apply'"), 0), OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace, N_("ignore changes in whitespace")), OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, @@ -1390,12 +1388,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (!strcmp(option, "--whitespace=fix") || !strcmp(option, "--whitespace=strip")) allow_preemptive_ff = 0; - else if (skip_prefix(option, "-C", &p)) { - while (*p) - if (!isdigit(*(p++))) - die(_("switch `C' expects a " - "numerical value")); - } else if (skip_prefix(option, "--whitespace=", &p)) { + else if (skip_prefix(option, "--whitespace=", &p)) { if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") && strcmp(p, "error") && strcmp(p, "error-all")) die("Invalid whitespace option: '%s'", p); diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh index ceca1600053..c8dca845dd7 100755 --- a/t/t3406-rebase-message.sh +++ b/t/t3406-rebase-message.sh @@ -78,13 +78,6 @@ test_expect_success 'rebase --onto outputs the invalid ref' ' test_i18ngrep "invalid-ref" err ' -test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' - test_must_fail git rebase -Cnot-a-number HEAD 2>err && - test_i18ngrep "numerical value" err && - test_must_fail git rebase --whitespace=bad HEAD 2>err && - test_i18ngrep "Invalid whitespace option" err -' - write_reflog_expect () { if test $mode = --apply then diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh index 6dabb05a2ad..ebcbd79ab8a 100755 --- a/t/t3422-rebase-incompatible-options.sh +++ b/t/t3422-rebase-incompatible-options.sh @@ -63,6 +63,5 @@ test_rebase_am_only () { } test_rebase_am_only --whitespace=fix -test_rebase_am_only -C4 test_done