From: Phillip Wood <phillip.wood123@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>,
Philippe Blain <levraiphilippeblain@gmail.com>,
Git mailing list <git@vger.kernel.org>,
Derrick Stolee <dstolee@microsoft.com>,
Elijah Newren <newren@gmail.com>
Subject: Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
Date: Wed, 18 Jan 2023 16:16:56 +0000 [thread overview]
Message-ID: <8a86074e-3f76-472c-15e2-9aa53762ef8f@dunelm.org.uk> (raw)
In-Reply-To: <9445830b-d172-c3b6-ef60-ae4931cab84b@github.com>
Hi Stolee
On 18/01/2023 15:51, Derrick Stolee wrote:
> On 1/17/2023 5:02 PM, Philippe Blain wrote:
> 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.
I think that for other options such as "--exec" that require the "merge"
backend we call imply_merge() and then have some code that will error
out if there are also options that require the "apply" backend. Is it
possible to do that here?
> 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.
I agree that requiring an explicit --no-update-refs in that case is best.
Best Wishes
Phillip
> 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
> +'
> +
> # This must be the last test in this file
> test_expect_success '$EDITOR and friends are unchanged' '
> test_editor_unchanged
next prev parent reply other threads:[~2023-01-18 16:21 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 22:02 [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced Philippe Blain
2023-01-18 15:51 ` Derrick Stolee
2023-01-18 16:16 ` Phillip Wood [this message]
2023-01-18 16:24 ` Elijah Newren
2023-01-18 17:03 ` Derrick Stolee
2023-01-18 18:07 ` Philippe Blain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8a86074e-3f76-472c-15e2-9aa53762ef8f@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=derrickstolee@github.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=levraiphilippeblain@gmail.com \
--cc=newren@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).