Re: [PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 20, 2012, 18:50 UTC
- Message-ID
- <7vk3wtjt3o.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1345470460-28734-4-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
Show 11 quoted lines
> This interface is error prone, and a better one (--set-upstream-to) > exists. Suggest how to fix a --set-upstream invocation in case the > user only gives one argument, which makes it likely that he meant to > do the opposite, like with > > git branch --set-upstream origin/master > > when they meant one of > > git branch --set-upstream origin/master master > git branch --set-upstream-to origin/master
In "git branch --set-upstream $A", if $A did not exist (i.e. we ended up creating $A that is forked from the current branch), and if refs/remotes/$A exists, I agree it is very likely that the user wanted to set the upstream of the current branch to remotes/$A instead.
But I am not sure about other cases. If $A already existed, it is equally likely that "git branch --set-upstream $A" wanted to make the existing $A track our current branch, or make our branch track that existing $A, isn't it? We would end up giving unwanted advice that is *wrong* for the user's situation 50% of the time, which does not sound like an acceptable thing to do.
So I would really prefer to see the condition this advice is offered limited to very specific cases (namely, only one parameter $A was given to cause us use "HEAD" as the other branch, refs/heads/$A did not exist and refs/remotes/$A did; there may be others but I think this is the one we most care about) in which we know the advice is correct with certainty.
> While we're at it, add a notice that the --set-upstream flag is > deprecated and will be removed at some point.
This part is unquestionably good.
Show 14 quoted lines
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > > --- > > This produces suboptimal output in case that A tracks B and we do > > git checkout B > git branch --set-upstream A > > as it will suggest > > git branch --set-upstream A B > > as a way of undoing what we just did.
The literal meaning of what the user did was to create B and then made A that existed (and used to build upon B) to build upon B, which is a no-op. And undoing can be done with the same command.
Which is funny.
I am sure you will get complaint from the real users. To avoid it, you would need to know what the old value was (if any), and skip the "undo" part, but I think it is probably worth it. You already have code to learn what the old value was anyway, so the ideal is just a single comparison away, no?
By the way, are you assuming that what the user wanted to do was to make B build on top of A (i.e. set branch.B.up to A) in this example? For that setting to make sense, perhaps B should already be a descendant of A, shouldn't it? If that is the case, that is another clue you can use in your logic to decide if you want to tell the user "you told me to make $A build on the current branch, but I think you meant the other way around" with more confidence.
Show 11 quoted lines
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 08068f7..33641d9 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -707,6 +707,21 @@ static int edit_branch_description(const char *branch_name)
> return status;
> }
>
> +static void print_set_upstream_warning(const char *branch, int branch_existed, const char *old_upstream)
> +{
> + fprintf(stderr, _("If you wanted to make '%s' track '%s', do this:\n\n"), head, branch);I would suggest strongly against using the verb "track" here, as it is overused and has caused confusion "is it tracking branch, remote tracking branch, do I merge in one but fetch to update the other?".
Regardless of the verb, I think there should be a line _before_ the above to tell the user what the command actually _did_, to make the distinction stand out to help the user decide.
That is, tell the user "I set upstream for that other branch to the current branch, making that other branch build on top of the current branch. If that was a mistake, and what you really wanted to do was to make the current branch build on top of the other branch, here are the things you need to do to recover". Without saying anything before "If that was a mistake, " part and giving "to do X, do Y" would leave the user confused why he is being given a way to do X in the first place, whether what he really wanted to do was X or something else.
Show 11 quoted lines
> + if (branch_existed) {
> + if (old_upstream)
> + fprintf(stderr, _(" git branch --set-upstream %s %s\n"), old_upstream, branch);
> + else
> + fprintf(stderr, _(" git branch --unset-upstream %s\n"), branch);
> + } else {
> + fprintf(stderr, _(" git branch -d %s\n"), branch);
> + }
> +
> + fprintf(stderr, _(" git branch --set-upstream-to %s\n"), branch);
> +}And I suspect the logic to call into this function needs to be tightened a lot. It may even turn out that we may not need messages when "branch_existed" is true.
Show 31 quoted lines
> int cmd_branch(int argc, const char **argv, const char *prefix)
> {
> int delete = 0, rename = 0, force_create = 0, list = 0;
> @@ -877,10 +892,30 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> git_config_set_multivar(buf.buf, NULL, NULL, 1);
> strbuf_release(&buf);
> } else if (argc > 0 && argc <= 2) {
> + struct branch *branch = branch_get(argv[0]);
> + const char *old_upstream = NULL;
> + int branch_existed = 0;
> +
> if (kinds != REF_LOCAL_BRANCH)
> die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
> +
> + if (track == BRANCH_TRACK_OVERRIDE)
> + fprintf(stderr, _("The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n"));
> +
> + /*
> + * Save what argv[0] was pointing to so we can give
> + * the --set-upstream-to hint
> + */
> + if (branch_has_merge_config(branch))
> + old_upstream = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> +
> + branch_existed = ref_exists(branch->refname);
> create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
> force_create, reflog, 0, quiet, track);
> +
> + if (argc == 1 && track == BRANCH_TRACK_OVERRIDE)
> + print_set_upstream_warning(argv[0], branch_existed, old_upstream);
> +