On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> --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
