Re: [PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch
On Mon, 2012-08-20 at 11:50 -0700, Junio C Hamano wrote:
Carlos Martín Nieto c...@elego.de writes:
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.
I'm not sure about the 50%, I've personally never used the one-argument
version (correctly), but the restriction does make sense as
--set-upstream origin/master is the main thing we want to protect
against.
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.
Signed-off-by: Carlos Martín Nieto c...@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?
Yes, but this comparison is already two or three levels deep in ifs,
which is why I didn't put it in this version, as it was unlikely to be
triggered anyway. As per the next paragraph, this probably won't be an
issue.
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.
I am pretty much assuming that every use of --set-upstream with a single
argument was meant to use the --set-upstream-to semantics. This
particular examples is an edge-case I found when trying to figure out
the possible combinations, not a sane one I expect to see in the wild.
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.
I don't like the word either, but that's what 'branch --set-upstream'
and 'checkout -t' use (or anything that ends up calling
install_branch_config()). There is such a message (see below) and that's
how it says it. I worry that changing the wording might be even more
confusing, but I'll play with some wording (unless you want to change
the message in install_branch_config()).
That is, tell the user I set upstream for that other branch to the
[PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch
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
While we're at it, add a notice that the --set-upstream flag is
deprecated and will be removed at some point.
Signed-off-by: Carlos Martín Nieto c...@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. Avoiding it becomes a bit messy
(yet another layer of ifs), so I've left it out. Anybody reckon it's
worth recognising this?
---
builtin/branch.c | 35 +++
t/t3200-branch.sh | 52
2 files changed, 87 insertions(+)
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);
+ 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);
+}
+
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);
+
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 93e5d6e..702bffa 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -399,6 +399,58 @@ test_expect_success 'test --unset-upstream on a particular
branch' \
test_must_fail git config branch.my14.remote
test_must_fail git config branch.my14.merge'
+test_expect_success 'test --set-upstream help message with one arg' \
+'git branch --set-upstream origin/master 2actual
+ test_when_finished git branch -d origin/master
+ cat expected EOF
+The --set-upstream flag is deprecated and will be removed. Consider using
--track or --set-upstream-to
+If you wanted to make ''master'' track ''origin/master'', do this:
+
+git branch -d origin/master
+git branch --set-upstream-to origin/master
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with a branch that already has an
upstream' \
+'git branch --set-upstream-to my12 master
+ git branch --set-upstream-to my13 my12
+ test_when_finished git branch --unset-upstream my12
+ test_when_finished git branch --unset-upstream my13
+ git branch --set-upstream my12 2actual
+ cat actual
+ cat expected EOF
+The --set-upstream flag is deprecated and will be removed. Consider using
--track or --set-upstream-to
+If you wanted to make ''master'' track ''my12'', do this:
+
+git branch
Re: [PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch
Carlos Martín Nieto c...@elego.de writes:
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.
Signed-off-by: Carlos Martín Nieto c...@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.
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.
+ 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.
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, force_create = 0, list = 0;
@@ -877,10
