rebase: fix several code/testing/documentation issues around flag incompatibilities #1466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1466.git.1674106587550.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): 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 |
|
User |
newren
force-pushed
the
rebase-update-refs-imply-merge
branch
from
f7459f0
to
2e44d0b
Compare
|
On the Git mailing list, Elijah Newren wrote (reply to this): 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 |
|
User |
|
/submit |
|
Submitted as pull.1466.v2.git.1674190573.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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.
|
newren
force-pushed
the
rebase-update-refs-imply-merge
branch
from
2e44d0b
to
d44526e
Compare
|
On the Git mailing list, Phillip Wood wrote (reply to this), regarding 2e44d0b (outdated): Hi Elijah
Thanks for working on this
On 20/01/2023 04:56, 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.
Strictly speaking we die rather than warn but I don't think that warrants a re-roll. I just had a quick look to see how easy it would be to add the advice Stolee's patch had if the user has set rebase.updaterefs but does not pass "--no-update-refs" when using the apply backend but it looks a bit fiddly unfortunately as we could die in imply_merge() or later on.
Thinking more generally, imply_merge() does a good job of telling the user which option is incompatible with "--apply" but if the user passes a merge option with "--whitespace=fix" and omits "--apply" then we just print a generic message saying "apply options and merge options cannot be used together" which isn't terribly helpful to the user (doubly so when the merge option come from a config setting).
I've also noticed that "--autosquash" is ignored if we end up using the apply backend. That's a separate issue but shares the "this may have come from a config setting rather than a command line argument" problem.
All in all I'm not sure if it is friendlier to die when the user has rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or if it is better just to ignore the config in that case. If we can find a way to print some help when we die in that case it would be nicer for the user.
Best Wishes
Phillip
> 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).
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> builtin/rebase.c | 3 +++
> t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
> 2 files changed, 13 insertions(+), 5 deletions(-)
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ace8bd4a41c..e8bcdbf9fcd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1507,6 +1507,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 ebcbd79ab8a..d72c863b21b 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 |
|
User |
|
On the Git mailing list, Elijah Newren wrote (reply to this): 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. |
newren
force-pushed
the
rebase-update-refs-imply-merge
branch
from
d44526e
to
0e8b061
Compare
newren
changed the title
newren
changed the title
|
On the Git mailing list, Elijah Newren wrote (reply to this), regarding 2e44d0b (outdated): On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> Thanks for working on this
>
> On 20/01/2023 04:56, 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.
>
> Strictly speaking we die rather than warn but I don't think that
> warrants a re-roll.
Oh, good catch. I'm re-rolling anyway, so I might as well fix this.
> I just had a quick look to see how easy it would be
> to add the advice Stolee's patch had if the user has set
> rebase.updaterefs but does not pass "--no-update-refs" when using the
> apply backend but it looks a bit fiddly unfortunately as we could die in
> imply_merge() or later on.
Yeah, and it gets even more finicky than that. If the user specifies
_any_ merge-specific options on the command line together with an
apply-specific option, then there's no point bringing up
rebase.updaterefs (or rebase.autosquash). We only want to bring up
those config options if they are the only reasons for getting a
backends-are-incompatible error message.
> Thinking more generally, imply_merge() does a good job of telling the
> user which option is incompatible with "--apply" but if the user passes
> a merge option with "--whitespace=fix" and omits "--apply" then we just
> print a generic message saying "apply options and merge options cannot
> be used together" which isn't terribly helpful to the user (doubly so
> when the merge option come from a config setting).
That's not specific to --whitespace=fix (it also happens with -C, and
in the past happened with other options that used to only work with
the apply backend). In particular, it's whenever both backends are
implied -- in those cases, we don't try to keep track of which options
implied it and thus only provide a very generic error message.
> I've also noticed that "--autosquash" is ignored if we end up using the
> apply backend. That's a separate issue but shares the "this may have
> come from a config setting rather than a command line argument" problem.
Yeah, Stolee also pointed this one out...and --autosquash was missing
the same incompatible-with-apply-options warnings too.
> All in all I'm not sure if it is friendlier to die when the user has
> rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or
> if it is better just to ignore the config in that case. If we can find a
> way to print some help when we die in that case it would be nicer for
> the user.
I think ignoring it would be worse, as I argued over at [1]. But
another thing to keep in mind is that we can eventually make the
question obsolete by deprecating and eventually removing the apply
backend, as suggested by Junio[2]. That would allow us to remove all
the incompatibility checking and simplify the manual.
[1] https://lore.kernel.org/git/CABPp-BHDhpSVpuaubTP=smWaf7FBmpzB-_Frh0Dn5oN+vx0xzw@mail.gmail.com/
[2] See "longer term goal" of
https://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/ |
|
/submit |
|
Submitted as pull.1466.v3.git.1674266126.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
newren
force-pushed
the
rebase-update-refs-imply-merge
branch
from
0e8b061
to
f402594
Compare
--[no-]reapply-cherry-picks was traditionally only supported by the sequencer. Support was added for the apply backend, when --keep-base is also specified, in commit ce5238a ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17). Make the code error out when --[no-]reapply-cherry-picks is specified AND the apply backend is used AND --keep-base is not specified. Also, clarify a number of comments surrounding the interaction of these flags. Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Elijah Newren <newren@gmail.com>
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 one 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. Signed-off-by: Elijah Newren <newren@gmail.com>
--edit-todo was documented as being incompatible with any of the options for the apply backend. However, it is also incompatible with any of the options for the merge backend, and is incompatible with any options that are not backend specific as well. The same can be said for --continue, --skip, --abort, --quit, etc. This is already somewhat implicitly covered by the synopsis, but since "[<options>]" in the first two variants are vague it might be easy to miss this. That might not be a big deal, but since the rebase manpage has to spend so much verbiage about incompatibility of options, making a separate section for these options that are incompatible with everything else seems clearer. Do that, and remove the needless inclusion of --edit-todo in the explicit incompatibility list. Signed-off-by: Elijah Newren <newren@gmail.com>
Commit ce5238a ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17) accidentally added some blank lines that cause extra paragraphs about --reapply-cherry-picks to be considered not part of the documentation of that option. Remove the blank lines to make it clear we are still discussing --reapply-cherry-picks. Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
When config which selects the merge backend (currently, rebase.autosquash=true or rebase.updateRefs=true) conflicts with other options on the command line (such as --whitespace=fix), make the error message specifically call out the config option and specify how to override that config option on the command line. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
rebase-update-refs-imply-merge
branch
from
10380c5
to
328f5a1
Compare
|
/submit |
|
Submitted as pull.1466.v5.git.1674619434.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Elijah
On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:
> We had a report about --update-refs being ignored when --whitespace=fix was
> passed, confusing an end user. These were already marked as incompatible in
> the manual, but the code didn't check for the incompatibility and provide an
> error to the user.
> > Folks brought up other flags tangentially when reviewing an earlier round of
> this series, and I found we had more that were missing checks, and that we
> were missing some testcases, and that the documentation was wrong on some of
> the relevant options. So, I ended up getting lots of little fixes to
> straighten these all out.
This version looks good to me. Thanks for working on it - it turned out to be quite a bit of work in the end but it is nice improvement, especially the last patch.
Best Wishes
Phillip
> Changes since v4:
> > * Pulled out the changes regarding incompatibility detection for
> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> with understanding the behavior, suggesting changes, and getting the
> wording right, and I think it deserves its own patch with its own
> explanation.
> > Changes since v3:
> > * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
> the testcases (Thanks to Phillip for pointing out my error)
> * I went ahead and implemented the better error message when the merge
> backend is triggered solely via config options.
> > Changes since v2:
> > * Remove the extra patch and reword the comment in t3422 more thoroughly.
> * Add tests for other flag incompatibilities that were missing
> * Add code that was missing for checking various flag incompatibilities
> * Fix incorrect claims in the documentation around incompatible options
> * A few other miscellaneous fixups noticed while doing all the above
> > Changes since v1:
> > * Add a patch nuking the -C option to rebase (fixes confusion around the
> comment in t3422 and acknowledges the fact that the option is totally and
> utterly useless and always has been. It literally never affects the
> results of a rebase.)
> > Signed-off-by: Elijah Newren newren@gmail.com
> > Elijah Newren (10):
> rebase: mark --update-refs as requiring the merge backend
> rebase: flag --apply and --merge as incompatible
> rebase: remove --allow-empty-message from incompatible opts
> rebase: fix docs about incompatibilities with --root
> rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
> rebase: add coverage of other incompatible options
> rebase: clarify the OPT_CMDMODE incompatibilities
> rebase: fix formatting of rebase --reapply-cherry-picks option in docs
> rebase: put rebase_options initialization in single place
> rebase: provide better error message for apply options vs. merge
> config
> > Documentation/git-rebase.txt | 77 ++++++++++++-------------
> builtin/rebase.c | 79 +++++++++++++++++++-------
> t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--
> 3 files changed, 163 insertions(+), 64 deletions(-)
> > > base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1466
> > Range-diff vs v4:
> > 1: 8a676e6ec1a = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
> 2: cc129b87185 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible
> 3: 9464bbbe9ba = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
> 4: b702f15ed7c = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root
> -: ----------- > 5: 3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
> 5: 5e4851e611e ! 6: 2777dd2788a rebase: add coverage of other incompatible options
> @@ Commit message
> > 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
> + code checks for one of these, which could result in command line
> options being silently ignored.
> > Also, note that adding a check for autosquash means that using
> @@ Commit message
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > - ## Documentation/git-rebase.txt ##
> -@@ Documentation/git-rebase.txt: are incompatible with the following options:
> - * --exec
> - * --no-keep-empty
> - * --empty=
> -- * --reapply-cherry-picks
> -+ * --[no-]reapply-cherry-picks
> - * --edit-todo
> - * --update-refs
> - * --root when used without --onto
> -
> ## builtin/rebase.c ##
> -@@ builtin/rebase.c: 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.
> -+ */
> -+ if (options.reapply_cherry_picks >= 0)
> -+ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
> -+ "--no-reapply-cherry-picks");
> -+
> - /*
> - * --keep-base defaults to --reapply-cherry-picks to avoid losing
> - * commits when using this option.
> -@@ builtin/rebase.c: 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)
> -- imply_merge(&options, "--reapply-cherry-picks");
> --
> - if (gpg_sign)
> - options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> -
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.update_refs)
> imply_merge(&options, "--update-refs");
> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> + test_must_fail git rebase $opt --empty=ask A
> + "
> +
> -+ test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
> -+ git checkout B^0 &&
> -+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
> -+ "
> -+
> -+ test_expect_success "$opt incompatible with --reapply-cherry-picks" "
> -+ git checkout B^0 &&
> -+ test_must_fail git rebase $opt --reapply-cherry-picks A
> -+ "
> -+
> - test_expect_success "$opt incompatible with --update-refs" "
> + test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
> git checkout B^0 &&
> - test_must_fail git rebase $opt --update-refs A
> + test_must_fail git rebase $opt --no-reapply-cherry-picks A
> 6: 21ae9e05313 ! 7: 0d0541ea243 rebase: clarify the OPT_CMDMODE incompatibilities
> @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
> @@ Documentation/git-rebase.txt: are incompatible with the following options:
> * --no-keep-empty
> * --empty=
> - * --[no-]reapply-cherry-picks
> + * --[no-]reapply-cherry-picks when used without --keep-base
> - * --edit-todo
> * --update-refs
> * --root when used without --onto
> 7: 74a216bf0c0 = 8: 01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
> 8: a8adf7fda61 = 9: f646abee524 rebase: put rebase_options initialization in single place
> 9: 5cb00e5103b = 10: 328f5a1d534 rebase: provide better error message for apply options vs. merge config
> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v4:
>
> * Pulled out the changes regarding incompatibility detection for
> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> with understanding the behavior, suggesting changes, and getting the
> wording right, and I think it deserves its own patch with its own
> explanation.
Hmph, does this replace the previous round that has already been in
'next' for two days? I do not mind reverting the merge and requeuing
it, but just wanted ot make sure before doing anything.
Thanks. |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v4:
> >
> > * Pulled out the changes regarding incompatibility detection for
> > --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> > with understanding the behavior, suggesting changes, and getting the
> > wording right, and I think it deserves its own patch with its own
> > explanation.
>
> Hmph, does this replace the previous round that has already been in
> 'next' for two days? I do not mind reverting the merge and requeuing
> it, but just wanted ot make sure before doing anything.
Sorry, I didn't realize it had merged to next. With Phillip's ongoing
reviews and requests for changes, which you had even commented on
(https://lore.kernel.org/git/xmqqpmb4j8e9.fsf@gitster.g/ and the
thread that was in the middle of), I assumed it was still only in
seen.
But yes, if you could revert from next and replace, that would be
great. Now that v5 has Phillip's blessing (and multiple of his
corrections), I think it's good to merge down. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@490d014. |
|
This patch series was integrated into next via git@35a67cf. |
|
This patch series was integrated into seen via git@56c5e67. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Elijah,
On Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:
> We had a report about --update-refs being ignored when --whitespace=fix
> was passed, confusing an end user. These were already marked as
> incompatible in the manual, but the code didn't check for the
> incompatibility and provide an error to the user.
Thank you for working on this!
FWIW this report (and your patch series) made me wistful about that Google
Summer of Code project that I hoped would bring about the trick to combine
`--whitespace=fix` with interactive rebases. But in that GSoC project,
this goal was treated as a "bonus feature" and pushed so far back that we
did not even get to analyzing the complexity of the task, let alone any
details.
So I sat down and started that analysis. The result is
https://github.com/gitgitgadget/git/issues/1472 where you can see that
addressing the incompatibility is definitely outside of trivial. It does
seem doable, if Outreachy/GSoC project-sized.
Ciao,
Johannes |
|
User |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Thu, Feb 2, 2023 at 2:29 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:
>
> > We had a report about --update-refs being ignored when --whitespace=fix
> > was passed, confusing an end user. These were already marked as
> > incompatible in the manual, but the code didn't check for the
> > incompatibility and provide an error to the user.
>
> Thank you for working on this!
>
> FWIW this report (and your patch series) made me wistful about that Google
> Summer of Code project that I hoped would bring about the trick to combine
> `--whitespace=fix` with interactive rebases. But in that GSoC project,
> this goal was treated as a "bonus feature" and pushed so far back that we
> did not even get to analyzing the complexity of the task, let alone any
> details.
>
> So I sat down and started that analysis. The result is
> https://github.com/gitgitgadget/git/issues/1472 where you can see that
> addressing the incompatibility is definitely outside of trivial. It does
> seem doable, if Outreachy/GSoC project-sized.
Nice! Thanks for digging in and documenting what is needed! |
|
There was a status update in the "Cooking" section about the branch "git rebase" often ignored incompatible options instead of complaining, which has been corrected. Will merge to 'master'. Replaces en/rebase-update-refs-needs-merge-backend. source: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@2c6e5b3. |
|
This patch series was integrated into master via git@2c6e5b3. |
|
This patch series was integrated into next via git@2c6e5b3. |
|
Closed via 2c6e5b3. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had a report about --update-refs being ignored when --whitespace=fix was passed, confusing an end user. These were already marked as incompatible in the manual, but the code didn't check for the incompatibility and provide an error to the user.
Folks brought up other flags tangentially when reviewing an earlier round of this series, and I found we had more that were missing checks, and that we were missing some testcases, and that the documentation was wrong on some of the relevant options. So, I ended up getting lots of little fixes to straighten these all out.
Changes since v4:
Changes since v3:
Changes since v2:
Changes since v1:
Signed-off-by: Elijah Newren newren@gmail.com
cc: Derrick Stolee derrickstolee@github.com
cc: Elijah Newren newren@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Martin Ågren martin.agren@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de