Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 19:20 +0200, Matthieu Moy wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  --- a/builtin/branch.c
  +++ b/builtin/branch.c
  @@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const 
  char *prefix)
 info and making sure new_upstream is correct */
  create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
  BRANCH_TRACK_OVERRIDE);
  } 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));
  +
  +   /* Save what argv[0] was pointing to so we can give
  +  the --set-upstream-to hint */
 
 Multi-line comments are usually written in Git as
 
 /*
  * multi-line
  * comment
  */

I've seen this style often, but sure.

 
  +   if (branch_has_merge_config(branch))
  + old_upstream = shorten_unambiguous_ref(branch-merge[0]-dst, 
  0);
 
 Broken indentation.

Yeah, sorry. New laptop, hadn't got the default style fixed in the
config.

 
  +   if (argc == 1) {
  +   printf(If you wanted to make '%s' track '%s', do 
  this:\n, head, argv[0]);
 
 Could be marked for translation with _(...).

Done.

 
  +   if (branch_existed)
  +   printf( $ git branch --set-upstream '%s' 
  '%s'\n, argv[0], old_upstream);
 
 old_upstream may be NULL at this point. I guess you want to skip this
 line if old_upsteam is NULL.

We've just set up tracking for it, so we'd want to undo that. Which
means --unset-upstream would have to move earlier in the series so we
can suggest that.

 
 The fact that I could find this bug suggests that this lacks a few new
 tests too ;-).

Indeed :) the next round will have them.

 
  +   else
  +   printf( $ git branch -d '%s'\n, argv[0]);
  +
  +   printf( $ git branch --set-upstream-to '%s'\n, 
  argv[0]);
 
 For the 3 printf()s: we usually display commands without the $, and
 separate them from text with a blank line. See for example what git
 commit says when you didn't provide authorship:

Yeah, I was going by what Junio wrote in his mail. We should probably
have a double-LF as well, like in the message below.

 
 You can suppress this message by setting them explicitly:
 
 git config --global user.name Your Name
 git config --global user.email y...@example.com
 
 After doing this, you may fix the identity used for this commit with:
 
 git commit --amend --reset-author
 
 (the absence of $ sign avoids the temptation to cut-and-paste it)
 



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 10:40 -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
 
  Signed-off-by: Carlos Martín Nieto c...@elego.de
 
 The new code does not seem to depend on the value of track (which
 is set by either -t or --set-upstream) in any way.  Shouldn't it be
 done only when it is set to track-override?

Yes, yes it should.

 
 Doesn't git branch [-f] frotz without any other argument trigger
 the warning?

It does. Oops. Fixed.

   cmn


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 18:00 -0500, Jonathan Nieder wrote:
 Junio C Hamano wrote:
 
I think it
  is better to leave them emitted unconditionally to the standard
  error stream, in order to train users away from using the old option
  that has its arguments wrong (the option does not take an argument
  it should, and makes the command line to look as if it takes two
  branch arguments in the wrong order).
 
 I thought we already discussed that that is a side-issue?

The current --set-upstream is the whole reason for this series existing.

 
 The option is a mode option for the command, like -m, -d, or
 --edit-description.  I genuinely don't think the order of options it
 takes is counter-intuitive.  The second argument defaulting to HEAD
 and the behavior of creating the branch named by the first argument
 when it does not exist are quite counter-intuitive.

This is confusing. First you say that you don't think it's
counter-intuitive but then you say it is? Or is the first part about -m
and -d?

The second part of the paragraph is indeed what I'm trying to solve with
this series. If you want to create a new branch, you should be using -t.

 
 Transitioning to a different argument order seems like it would just
 make the command more complicated.  After the transition, there are
 two options to explain, and during the transition, it is easy to make
 scripts with gratuitous incompatibilities that won't work on older
 systems.
 
 Where is my thinking going wrong?

We're not transitioning to a new order as such, particularly not with
the same option name. The incompatibilities would ensue with the other
patch I send which did change the order for --set-upstream, but what
this does is _add_ --set-upstream-to=upstream such that the option
takes one argument and the command takes one optional argument, which
makes it closer to what one would expect, specially as changing the
upstream information is something you're most likely to do on the
current branch.

   cmn


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

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

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 builtin/branch.c |   22 ++
 1 file changed, 22 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index c886fc0..5551227 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
   info and making sure new_upstream is correct */
create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
} 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));
+
+   /* 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) {
+   printf(If you wanted to make '%s' track '%s', do 
this:\n, head, argv[0]);
+   if (branch_existed)
+   printf( $ git branch --set-upstream '%s' 
'%s'\n, argv[0], old_upstream);
+   else
+   printf( $ git branch -d '%s'\n, argv[0]);
+
+   printf( $ git branch --set-upstream-to '%s'\n, 
argv[0]);
+   }
+
} else
usage_with_options(builtin_branch_usage, options);
 
-- 
1.7.10.2.1.g8c77c3c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Junio C Hamano]

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

 Signed-off-by: Carlos Martín Nieto c...@elego.de

The new code does not seem to depend on the value of track (which
is set by either -t or --set-upstream) in any way.  Shouldn't it be
done only when it is set to track-override?

Doesn't git branch [-f] frotz without any other argument trigger
the warning?

  builtin/branch.c |   22 ++
  1 file changed, 22 insertions(+)

 diff --git a/builtin/branch.c b/builtin/branch.c
 index c886fc0..5551227 100644
 --- a/builtin/branch.c
 +++ b/builtin/branch.c
 @@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
  info and making sure new_upstream is correct */
   create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
 BRANCH_TRACK_OVERRIDE);
   } 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));
 +
 + /* 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) {
 + printf(If you wanted to make '%s' track '%s', do 
 this:\n, head, argv[0]);
 + if (branch_existed)
 + printf( $ git branch --set-upstream '%s' 
 '%s'\n, argv[0], old_upstream);
 + else
 + printf( $ git branch -d '%s'\n, argv[0]);
 +
 + printf( $ git branch --set-upstream-to '%s'\n, 
 argv[0]);
 + }
 +
   } else
   usage_with_options(builtin_branch_usage, options);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Jonathan Nieder]

Hi,

Quick nitpicks.

Carlos Martín Nieto wrote:

 --- a/builtin/branch.c
 +++ b/builtin/branch.c
 @@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
  info and making sure new_upstream is correct */
   create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
 BRANCH_TRACK_OVERRIDE);
   } 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));
 +
 + /* 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);

Whitespace is odd here.  Maybe this case could be factored out as a
new function to make room on the right margin and make cmd_branch()
easier to read straight through.

 +
 + 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) {
 + printf(If you wanted to make '%s' track '%s', do 
 this:\n, head, argv[0]);
 + if (branch_existed)
 + printf( $ git branch --set-upstream '%s' 
 '%s'\n, argv[0], old_upstream);
 + else
 + printf( $ git branch -d '%s'\n, argv[0]);
 +
 + printf( $ git branch --set-upstream-to '%s'\n, 
 argv[0]);

Message should go on stderr and be guarded with an advice option (see
advice.c).

Like this:

const char *arg;

...
if (argc != 1 || !advice_old_fashioned_set_upstream)
return 0; /* ok. */

arg = argv[0];
advise(If you wanted to make '%s' track '%s', do this:,
head, arg);
if (branch_existed)
advise( $ git branch --set-upstream-to='%s' '%s',
old_upstream, arg);
else
advise( $ git branch -d '%s', arg);
advise( $ git branch --set-upstream-to='%s', arg);

If an argument contains single-quotes, the quoting will be wrong, but
that's probably not worth worrying about.

Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 Message should go on stderr and be guarded with an advice option (see
 advice.c).

 Like this:

   const char *arg;

   ...
   if (argc != 1 || !advice_old_fashioned_set_upstream)
   return 0; /* ok. */

   arg = argv[0];
   advise(If you wanted to make '%s' track '%s', do this:,
   head, arg);
   if (branch_existed)
   advise( $ git branch --set-upstream-to='%s' '%s',
   old_upstream, arg);
   else
   advise( $ git branch -d '%s', arg);
   advise( $ git branch --set-upstream-to='%s', arg);

 If an argument contains single-quotes, the quoting will be wrong, but
 that's probably not worth worrying about.

In principle, I would agree that this is a kind of thing that falls
into the advice categiry, but with the fact that we plan to
deprecate --set-upstream, combined with the fact that [PATCH 1/3]
introduced the new option --set-upstream-to together with a short
and sweet -u synonym already at this point in the series, I think it
is better to leave them emitted unconditionally to the standard
error stream, in order to train users away from using the old option
that has its arguments wrong (the option does not take an argument
it should, and makes the command line to look as if it takes two
branch arguments in the wrong order).

Actually, we should probably add the deprecation warning in this
commit.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Jonathan Nieder]

Junio C Hamano wrote:

   I think it
 is better to leave them emitted unconditionally to the standard
 error stream, in order to train users away from using the old option
 that has its arguments wrong (the option does not take an argument
 it should, and makes the command line to look as if it takes two
 branch arguments in the wrong order).

I thought we already discussed that that is a side-issue?

The option is a mode option for the command, like -m, -d, or
--edit-description.  I genuinely don't think the order of options it
takes is counter-intuitive.  The second argument defaulting to HEAD
and the behavior of creating the branch named by the first argument
when it does not exist are quite counter-intuitive.

Transitioning to a different argument order seems like it would just
make the command more complicated.  After the transition, there are
two options to explain, and during the transition, it is easy to make
scripts with gratuitous incompatibilities that won't work on older
systems.

Where is my thinking going wrong?

Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html