Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced

>
> On 1/17/2023 5:02 PM, Philippe Blain wrote:
> > Hi Elijah and Stolee,
> >
> > First, Stolee, thanks a lot for adding '--update-refs', it is very useful (I've
> > added it to my config so I don't forget to use it).
>
> I'm glad you're using it, and thanks for the report!
>
> > I recently learned that 'git rebase --whitespace=fix' exists, which is also
> > great but since it uses the apply backend, it can't be used with --update-refs.
> > I understand this, and the fact that adding '--whitespace=fix' to the merge
> > backend would be a big task; this is not what this email is about.
>
> I also use --whitespace=fix, and am disappointed that it doesn't work with
> --update-refs. I guess I just haven't used it in my workflows that depend on
> --update-refs.
>
> > I think there is a bug in that the code does not enforce the fact that
> > both options can't be used together.  Whenever '--whitespace' or '-C' are used,
> > the code defaults to the apply backend:
> ...
> > but 'is_merge' only checks if 'opts->type == REBASE_MERGE', so the check only
> > works if --merge was given explicitely, but not when none of '--merge' or '--apply'
> > were given (and so the default "merge" backend is used).
> >
> > I would have expected the code to die telling me --update-refs and --whitespace
> > are incompatible. But instead it defaulted to --apply, and (of course) did not
> > update the refs in my history (which I found out later).
>
> Yes, I agree that there should be an error message (and a die(), not just a
> warning). I quickly whipped up the patch below, which should resolve your
> concern.
>
> Note that I was a bit worried about users with the config option and not
> just those that specify the option over the command-line. I put in an extra
> warning for those users, but I could see the community wanting different
> behavior there.
>
> Let me know what you think. If we need a new version, I'll create a new
> thread for that review.

> --- >8 ---
> From fe310b37796b0b15554481eb1cfa3942a9200b4b Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <derrickstolee@github.com>
> Date: Wed, 18 Jan 2023 10:38:18 -0500
> Subject: [PATCH] rebase: die for both --apply and --update-refs
>
> The --apply backend is not compatible with the --update-refs option,
> which requires the interactive backend. Without any warning, users
> running 'git rebase --whitespace=fix' with --update-refs (or
> rebase.updateRefs=true in their config) will realize that their non-HEAD
> branches did not come along for the ride.
>
> Fix this with a die() message in the presence of both options. Since we
> cannot distinguish between the --update-refs option and the config
> option at this point, do an extra check to see if --update-refs was
> implied by the config and present a helpful warning to use
> --no-update-refs.
>
> It is possible that users might want to be able to use options such as
> --whitespace=fix with rebase.updateRefs=true in their config without
> explicitly adding --no-update-refs. However, it is probably safest to
> require them to explicitly opt-in to that behavior. Users with the
> config option likely expect that their refs will be automatically
> updated and this will help warn them that the action they are doing is
> likely destructive to their branch organization.
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  builtin/rebase.c              | 21 +++++++++++++++++++++
>  t/t3404-rebase-interactive.sh | 12 ++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5b..5330258c865 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1247,6 +1247,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 die(_("The --edit-todo action can only be used during "
>                       "interactive rebase."));
>
> +       /* Check for arguments that imply --apply before checking --apply itself. */
> +       if (options.update_refs) {
> +               const char *incompatible = NULL;
> +               if (options.git_am_opts.nr)
> +                       incompatible = options.git_am_opts.v[0];
> +               else if (options.type == REBASE_APPLY)
> +                       incompatible = "--apply";

> +
> +               if (incompatible) {
> +                       int from_config = 0;
> +                       if (!git_config_get_bool("rebase.updaterefs", &from_config) &&
> +                           from_config) {
> +                               warning(_("you have 'rebase.updateRefs' enabled in your config, "
> +                                         "but it is incompatible with one or more options;"));
> +                               warning(_("run again with '--no-update-refs' to resolve the issue"));
> +                       }
> +                       die(_("options '%s' and '%s' cannot be used together"),
> +                           "--upate-refs", incompatible);
> +               }
> +       }

> +
>         if (trace2_is_enabled()) {
>                 if (is_merge(&options))
>                         trace2_cmd_mode("interactive");
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 462cefd25df..8681c8a07f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2123,6 +2123,18 @@ test_expect_success '--update-refs: check failed ref update' '
>         test_cmp expect err.trimmed
>  '
>
> +test_expect_success '--apply options are incompatible with --update-refs' '
> +       for opt in "--whitespace=fix" "-C1" "--apply"
> +       do
> +               test_must_fail git rebase "$opt" --update-refs HEAD~1 2>err &&
> +               grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> +
> +               test_must_fail git -c rebase.updateRefs=true rebase "$opt" HEAD~1 2>err &&
> +               grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> +               grep "you have '\''rebase.updateRefs'\'' enabled" err || return 1
> +       done
> +'
> +