| Message ID | pull.1466.git.1674106587550.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: mark --update-refs as requiring the merge backend | expand |
On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > --update-refs is built in terms of the sequencer, which requires the > merge backend. It was already marked as incompatible with the apply > backend in the git-rebase manual, but the code didn't check for this > incompatibility and warn the user. Check and warn now. Thank you for submitting this version. > @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } > } > > + if (options.update_refs) > + imply_merge(&options, "--update-refs"); > + This solution is very elegant. The only downside is the lack of warning if --update-refs was implied by rebase.updateRefs=true, but I'm happy to delay implementing that warning in favor of your complete solution here. Thinking ahead to that option, are there other options that are implied by config that are required in imply_merge()? Is --autosquash in that camp? If so, then maybe it would make sense to expand imply_merge() to include a boolean config key and supply that warning within its implementation. (This consideration should not block this patch, as it is complete as-is.) > While at it, fix a typo in t3422...and fix some misleading wording (all > useful options other than --whitespace=fix have long since been > implemented in the merge backend). > # > -# Rebase has lots of useful options like --whitepsace=fix, which are > -# actually all built in terms of flags to git-am. Since neither > -# --merge nor --interactive (nor any options that imply those two) use > -# git-am, using them together will result in flags like --whitespace=fix > -# being ignored. Make sure rebase warns the user and aborts instead. > +# Rebase has a useful option, --whitespace=fix, which is actually > +# built in terms of flags to git-am. Since neither --merge nor > +# --interactive (nor any options that imply those two) use git-am, > +# using them together will result in --whitespace=fix being ignored. > +# Make sure rebase warns the user and aborts instead. > # Thanks for the update here. The -C option is also used in this test, so --whitespace=fix isn't the only option, right? At least, -C doesn't make sense to port over to the merge backend, so maybe that's what you mean by --whitespace=fix being the last one? The user could also explicitly request the apply backend with --apply, but this test script doesn't check it, strangely. That seems like an oversight, but not a drawback to your patch. > test_rebase_am_only () { > @@ -60,6 +60,11 @@ test_rebase_am_only () { > test_must_fail git rebase $opt --exec 'true' A > " > > + test_expect_success "$opt incompatible with --update-refs" " > + git checkout B^0 && > + test_must_fail git rebase $opt --update-refs A > + " > + Thanks for adding this test. I would delay the rebase.updateRefs version until there is extra behavior to check, such as the warning message. Thanks, -Stolee
On Thu, Jan 19, 2023 at 1:47 PM Derrick Stolee <derrickstolee@github.com> wrote: > > On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > --update-refs is built in terms of the sequencer, which requires the > > merge backend. It was already marked as incompatible with the apply > > backend in the git-rebase manual, but the code didn't check for this > > incompatibility and warn the user. Check and warn now. > > Thank you for submitting this version. > > > @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > } > > } > > > > + if (options.update_refs) > > + imply_merge(&options, "--update-refs"); > > + > > This solution is very elegant. The only downside is the lack of warning > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to > delay implementing that warning in favor of your complete solution here. > > Thinking ahead to that option, are there other options that are implied > by config that are required in imply_merge()? Is --autosquash in that > camp? If so, then maybe it would make sense to expand imply_merge() to > include a boolean config key and supply that warning within its > implementation. (This consideration should not block this patch, as it > is complete as-is.) Ooh, that does sound like a useful future idea to improve things further. > > While at it, fix a typo in t3422...and fix some misleading wording (all > > useful options other than --whitespace=fix have long since been > > implemented in the merge backend). > > > # > > -# Rebase has lots of useful options like --whitepsace=fix, which are > > -# actually all built in terms of flags to git-am. Since neither > > -# --merge nor --interactive (nor any options that imply those two) use > > -# git-am, using them together will result in flags like --whitespace=fix > > -# being ignored. Make sure rebase warns the user and aborts instead. > > +# Rebase has a useful option, --whitespace=fix, which is actually > > +# built in terms of flags to git-am. Since neither --merge nor > > +# --interactive (nor any options that imply those two) use git-am, > > +# using them together will result in --whitespace=fix being ignored. > > +# Make sure rebase warns the user and aborts instead. > > # > > Thanks for the update here. The -C option is also used in this test, > so --whitespace=fix isn't the only option, right? At least, -C doesn't > make sense to port over to the merge backend, so maybe that's what > you mean by --whitespace=fix being the last one? Sorry if it was confusing. I was trying to figure out how to word it to remove the old confusion, and actually spent a while thinking about this point you brought up...but after a while gave up and just submitted my patch because it was taking too long and I didn't think the -C option was worth as much brain power as has been spent on it. But since you noted the incongruity, let me explain my thought process a little... * I stated that --whitespace=... was the last *useful* option, not that it was the last option. Yes, that was an implicit assertion that -C is not useful, though I avoided stating it directly for fear I'd have to dig up proof and/or spend a bunch of time trying to reproduce an old bug and debug it. * The correct way to port the '-C' option to the merge backend would probably be to just accept the flag and ignore it. The merge backend already functions with entire files or essentially infinite context. I can't think of any case where ignoring this option would result in negative surprises, other than possibly confusing people about how the algorithm behaves and making them decide to tweak it to no avail. And even if users were to waste time twiddling that flag in combination with the merge backend, even that might be a correct "port" of the existing -C option because... * I once tried to use the '-C' option with the apply backend to see if it could workaround a bug inherent to the apply backend (where people have a source file with several very similar code blocks, and rebase or am applying changes to the wrong one due to fuzz factor and large deletions/insertions having happened upstream elsewhere in the file). It appeared to have absolutely no effect. I suspected it was buggy, but didn't feel the option was worth debugging (I thought switching the default rebase backend was a much better use of time). * We have *zero* tests of the functionality of rebase's -C option in the testsuite. The only testing we do for rebase's -C option is solely around option parsing. .... And you inspired me to do some digging. rebase -C 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. However, the way it was added to rebase was to just pass it through to git-am, with no corresponding option to format-patch. Therefore, format-patch is going to continue generating patches with 3 lines of context, while we tell git-am/git-apply to pay attention to more than 3 lines of context. The -C<num> option is documented as using all available context if <num> exceeds the number of lines of context provided in the input patches. So, the -C option to rebase has never done anything useful, and the port from the legacy rebase script to the builtin C did not accidentally introduce any modicum of utility to this option; rather, it faithfully ported this pointless option precisely as-is. So, I'll adjust this series to include a preliminary patch that rips the rebase -C option out. > The user could also explicitly request the apply backend with --apply, > but this test script doesn't check it, strangely. That seems like an > oversight, but not a drawback to your patch. > > > test_rebase_am_only () { > > @@ -60,6 +60,11 @@ test_rebase_am_only () { > > test_must_fail git rebase $opt --exec 'true' A > > " > > > > + test_expect_success "$opt incompatible with --update-refs" " > > + git checkout B^0 && > > + test_must_fail git rebase $opt --update-refs A > > + " > > + > > Thanks for adding this test. I would delay the rebase.updateRefs > version until there is extra behavior to check, such as the > warning message. > > Thanks, > -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> + if (options.update_refs) >> + imply_merge(&options, "--update-refs"); >> + > > This solution is very elegant. The only downside is the lack of warning > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to > delay implementing that warning in favor of your complete solution here. If features A and B are incompatible and both can be specified from both command line and configuration, ideally I would expect the system to operate in one of two ways. I haven't thought it through to decide which one I prefer between the two. * Take "command line trumps configuration" one step further, so that A that is configured but not asked for from the command line is defeated by B that is asked for from the command line. This way, only when A and B are both requested via the configuration, of via the command line, we'd fail the operation by saying A and B are incompatible. Otherwise, the one that is configured but overridden is turned off (either silently or with a warning). * Declare "command line trumps configuration" is only among the same feature. Regardless of how features A and B that are incompatible are requested, the command will error out, citing incompatibility. It would be very nice if the warning mentioned where the requests for features A and B came from (e.g. "You asked for -B from the command line, but you have A configured, and both cannot be active at the same time---disable A from the command line, or do not ask for B") When A is configured and B is requested from the command line, the command will error out, and the user must defeat A from the command line before the user can use B, e.g. "git cmd --no-A -B". A knee-jerk reaction to the situation is that the latter feels somewhat safer than the former, but when I imagine the actual end user who saw the error message, especially the suggested solution "disable A from the command line or do not ask for B from the command line", may say "well, I asked for B for this invocation explicitly with -B from the command line, and you(Git) should be able to make it imply --no-A", which amounts to the same thing as the former choice.
On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@pobox.com> wrote: > > Derrick Stolee <derrickstolee@github.com> writes: > > >> + if (options.update_refs) > >> + imply_merge(&options, "--update-refs"); > >> + > > > > This solution is very elegant. The only downside is the lack of warning > > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to > > delay implementing that warning in favor of your complete solution here. > > If features A and B are incompatible and both can be specified from > both command line and configuration, ideally I would expect the > system to operate in one of two ways. I agree that one of the two ways you highlight below would be ideal. Should this series be held up on extending the checks to implement this ideal, or are you just commenting for whoever later circles back to implement this? > I haven't thought it through > to decide which one I prefer between the two. > > * Take "command line trumps configuration" one step further, so > that A that is configured but not asked for from the command > line is defeated by B that is asked for from the command line. > > This way, only when A and B are both requested via the > configuration, of via the command line, we'd fail the operation > by saying A and B are incompatible. Otherwise, the one that is > configured but overridden is turned off (either silently or with > a warning). > > * Declare "command line trumps configuration" is only among the > same feature. Regardless of how features A and B that are > incompatible are requested, the command will error out, citing > incompatibility. It would be very nice if the warning mentioned > where the requests for features A and B came from (e.g. "You > asked for -B from the command line, but you have A configured, > and both cannot be active at the same time---disable A from the > command line, or do not ask for B") > > When A is configured and B is requested from the command line, > the command will error out, and the user must defeat A from the > command line before the user can use B, e.g. "git cmd --no-A -B". > > A knee-jerk reaction to the situation is that the latter feels > somewhat safer than the former, but when I imagine the actual end > user who saw the error message, especially the suggested solution > "disable A from the command line or do not ask for B from the > command line", may say "well, I asked for B for this invocation > explicitly with -B from the command line, and you(Git) should be > able to make it imply --no-A", which amounts to the same thing as > the former choice. If it is clear to the user that A and B preclude each other, then I agree with this sentiment that the former choice (silently ignoring the config) would avoid a minor frustration for some users and thus would be better. But I don't think that's applicable here. There is no reason that --whitespace=fix shouldn't be available from the merge backend other than that we haven't implemented it yet, and it's likely feasible to implement --update-refs for the apply backend with enough effort if we thought it was worth it. So, if a user sets rebase.updateRefs=true in their config because they always want included branches updated, but one time they run `git rebase --whitespace=fix`, they will likely have a negative experience like the one that inspired this patch. Perhaps we're forced to choose between possible frustration by different end users, but if so, I think trying to debug and figure out "Wait, I switched to this branch and started tweaking it but it appears to not have some relevant changes I'm sure I made to it yesterday. What happened?" is a much worse frustration than "I have to manually specify --no-A in this special case". So, when it's not at all obvious that A and B preclude each other, I think we're better off giving the error.
diff --git a/builtin/rebase.c b/builtin/rebase.c index 1481c5b6a5b..accd62fce48 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } } + if (options.update_refs) + imply_merge(&options, "--update-refs"); + if (options.type == REBASE_UNSPECIFIED) { if (!strcmp(options.default_backend, "merge")) imply_merge(&options, "--merge"); diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh index 6dabb05a2ad..5a00618d265 100755 --- a/t/t3422-rebase-incompatible-options.sh +++ b/t/t3422-rebase-incompatible-options.sh @@ -25,11 +25,11 @@ test_expect_success 'setup' ' ' # -# Rebase has lots of useful options like --whitepsace=fix, which are -# actually all built in terms of flags to git-am. Since neither -# --merge nor --interactive (nor any options that imply those two) use -# git-am, using them together will result in flags like --whitespace=fix -# being ignored. Make sure rebase warns the user and aborts instead. +# Rebase has a useful option, --whitespace=fix, which is actually +# built in terms of flags to git-am. Since neither --merge nor +# --interactive (nor any options that imply those two) use git-am, +# using them together will result in --whitespace=fix being ignored. +# Make sure rebase warns the user and aborts instead. # test_rebase_am_only () { @@ -60,6 +60,11 @@ test_rebase_am_only () { test_must_fail git rebase $opt --exec 'true' A " + test_expect_success "$opt incompatible with --update-refs" " + git checkout B^0 && + test_must_fail git rebase $opt --update-refs A + " + } test_rebase_am_only --whitespace=fix