[PATCH 0/3] A better way of handling upstream information in git-branch

[PATCH 0/3] A better way of handling upstream information in git-branch

[Carlos Martín Nieto]

Hello all,

This stems from comments made by Junio and Jonathan about my proposed
changes to --set-upstream.

This should probably have a few tests, but I'd like to hear comments
about the code and documentation first. The third patch is the one I'm
not so confident about. It would be simpler to remove the whole
branch.foo configuration, but that wouldn't be very safe, as we may
have more things there (either the future git or some external tool).

Carlos Martín Nieto (3):
  branch: introduce --set-upstream-to
  branch: suggest how to undo a --set-upstream when given one branch
  branch: add --unset-upstream option

 Documentation/git-branch.txt |   14 +++++++++++-
 builtin/branch.c             |   50 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 60 insertions(+), 4 deletions(-)

-- 
1.7.10.2.1.g8c77c3c

[PATCH 1/3] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

The existing --set-uptream option can cause confusion, as it uses the
usual branch convention of assuming a starting point of HEAD if none
is specified, causing

    git branch --set-upstream origin/master

to create a new local branch 'origin/master' that tracks the current
branch. As --set-upstream already exists, we can't simply change its
behaviour. To work around this, introduce --set-upstream-to which
accepts a compulsory argument indicating what the new upstream branch
should be and one optinal argument indicating which branch to change,
defaulting to HEAD.

The new options allows us to type

    git branch --set-upstream-to origin/master

to set the current branch's upstream to be origin's master.
---
 Documentation/git-branch.txt |    9 ++++++++-
 builtin/branch.c             |   15 +++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 47235be..0f33fc7 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 	[--column[=<options>] | --no-column]
 	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
+'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
 'git branch' --edit-description [<branchname>]
@@ -48,7 +49,7 @@ branch so that 'git pull' will appropriately merge from
 the remote-tracking branch. This behavior may be changed via the global
 `branch.autosetupmerge` configuration flag. That setting can be
 overridden by using the `--track` and `--no-track` options, and
-changed later using `git branch --set-upstream`.
+changed later using `git branch --set-upstream-to`.
 
 With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
@@ -173,6 +174,12 @@ start-point is either a local or remote-tracking branch.
 	like `--track` would when creating the branch, except that where
 	branch points to is not changed.
 
+-u <upstream>::
+--set-upstream-to=<upstream>::
+	Set up <branchname>'s tracking information so <upstream> is
+	considered <branchname>'s upstream branch. If no branch is
+	specified it defaults to the current branch.
+
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
 	for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..c886fc0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -713,6 +713,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
 	int quiet = 0;
+	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
 	struct commit_list *with_commit = NULL;
@@ -726,6 +727,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			BRANCH_TRACK_EXPLICIT),
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
+		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
@@ -794,10 +796,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !edit_description && argc == 0)
+	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list > 1)
+	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)
@@ -852,6 +854,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
 			usage_with_options(builtin_branch_usage, options);
+	} else if (new_upstream) {
+		struct branch *branch = branch_get(argv[0]);
+
+		if (!ref_exists(branch->refname))
+		  die(_("branch '%s' does not exist"), branch->name);
+
+		/* create_branch takes care of setting up the tracking
+		   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) {
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
-- 
1.7.10.2.1.g8c77c3c

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Matthieu Moy]

Carlos Martín Nieto <cmn@elego.de> writes:

> The new options allows us to type
>
>     git branch --set-upstream-to origin/master

This is cool :-).

>  Documentation/git-branch.txt |    9 ++++++++-
>  builtin/branch.c             |   15 +++++++++++++--

I think this deserves a few new tests (probably in t/t3200-branch.sh).

> +-u <upstream>::
> +--set-upstream-to=<upstream>::
> +	Set up <branchname>'s tracking information so <upstream> is
> +	considered <branchname>'s upstream branch. If no branch is
> +	specified it defaults to the current branch.

Perhaps "if <branchname> is not specified, then it defaults to the
current branch.". The current wording does not make it very clear if "no
branch is specified" refers to <branchname> or <upstream> (although the
second option would be plain silly).

> +	} else if (new_upstream) {
> +		struct branch *branch = branch_get(argv[0]);
> +
> +		if (!ref_exists(branch->refname))
> +		  die(_("branch '%s' does not exist"), branch->name);

Indentation (2 spaces -> tab).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0e060f2..c886fc0 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -713,6 +713,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	int verbose = 0, abbrev = -1, detached = 0;
>  	int reflog = 0, edit_description = 0;
>  	int quiet = 0;
> +	const char *new_upstream = NULL;
>  	enum branch_track track;
>  	int kinds = REF_LOCAL_BRANCH;
>  	struct commit_list *with_commit = NULL;
> @@ -726,6 +727,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			BRANCH_TRACK_EXPLICIT),
>  		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
>  			BRANCH_TRACK_OVERRIDE),
> +		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
>  		OPT__COLOR(&branch_use_color, "use colored output"),
>  		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
>  			REF_REMOTE_BRANCH),
> @@ -794,10 +796,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
>  			     0);
>  
> -	if (!delete && !rename && !edit_description && argc == 0)
> +	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
>  		list = 1;
>  
> -	if (!!delete + !!rename + !!force_create + !!list > 1)
> +	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
>  		usage_with_options(builtin_branch_usage, options);

It probably is an error to have track and new_upstream together.

The remainder of [Patch 1/3] looked entirely sensible, including the
proposed log message (modulo missing sign-off).

Thanks.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Jonathan Nieder]

Hi,

Carlos Martín Nieto wrote:

> The existing --set-uptream option can cause confusion, as it uses the
> usual branch convention of assuming a starting point of HEAD if none
> is specified, causing
>
>     git branch --set-upstream origin/master
>
> to create a new local branch 'origin/master' that tracks the current
> branch. As --set-upstream already exists, we can't simply change its
> behaviour. To work around this, introduce --set-upstream-to which
> accepts a compulsory argument

Thanks.  A part of me really dislikes this --set-upstream-to which
is named more awkwardly than the deprecated mistake it replaces,
though.

Here's a patch on top to play with that names the new option
"--set-upstream=".  Untested.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git i/Documentation/git-branch.txt w/Documentation/git-branch.txt
index f572913f..57935a64 100644
--- i/Documentation/git-branch.txt
+++ w/Documentation/git-branch.txt
@@ -49,7 +49,7 @@ branch so that 'git pull' will appropriately merge from
 the remote-tracking branch. This behavior may be changed via the global
 `branch.autosetupmerge` configuration flag. That setting can be
 overridden by using the `--track` and `--no-track` options, and
-changed later using `git branch --set-upstream-to`.
+changed later using `git branch --set-upstream`.
 
 With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
@@ -174,11 +174,13 @@ start-point is either a local or remote-tracking branch.
 	like `--track` would when creating the branch, except that where
 	branch points to is not changed.
 
--u <upstream>::
---set-upstream-to=<upstream>::
+--set-upstream=<upstream>::
 	Set up <branchname>'s tracking information so <upstream> is
 	considered <branchname>'s upstream branch. If no branch is
 	specified it defaults to the current branch.
++
+If no argument is attached, for historical reasons the meaning is
+different.  See above.
 
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
diff --git i/builtin/branch.c w/builtin/branch.c
index c886fc06..0d705790 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -669,6 +669,31 @@ static int opt_parse_merge_filter(const struct option *opt, const char *arg, int
 	return 0;
 }
 
+struct set_upstream_params {
+	enum branch_track *track;
+	const char **new_upstream;
+};
+static int parse_opt_set_upstream(const struct option *opt, const char *arg, int unset)
+{
+	struct set_upstream_params *o = opt->value;
+
+	if (unset) {	/* --no-set-upstream */
+		*o->track = BRANCH_TRACK_NEVER;
+		*o->new_upstream = NULL;
+		return 0;
+	}
+
+	*o->track = BRANCH_TRACK_OVERRIDE;
+	if (!arg)	/* --set-upstream <branchname> <start-point> */
+		*o->new_upstream = NULL;
+	else	/* --set-upstream=<upstream> <branchname> */
+		*o->new_upstream = arg;
+	return 0;
+}
+#define OPT_SET_UPSTREAM(s, l, v) \
+	{ OPTION_CALLBACK, (s), (l), (v), "upstream", "change upstream info", \
+	  PARSE_OPT_OPTARG, &parse_opt_set_upstream }
+
 static const char edit_description[] = "BRANCH_DESCRIPTION";
 
 static int edit_branch_description(const char *branch_name)
@@ -716,6 +741,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
+	struct set_upstream_params set_upstream_args = { &track, &new_upstream };
 	struct commit_list *with_commit = NULL;
 
 	struct option options[] = {
@@ -725,9 +751,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT__QUIET(&quiet, "suppress informational messages"),
 		OPT_SET_INT('t', "track",  &track, "set up tracking mode (see git-pull(1))",
 			BRANCH_TRACK_EXPLICIT),
-		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
-			BRANCH_TRACK_OVERRIDE),
-		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
+		OPT_SET_UPSTREAM(0, "set-upstream", &set_upstream_args),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> The existing --set-uptream option can cause confusion, as it uses the
>> usual branch convention of assuming a starting point of HEAD if none
>> is specified, causing
>>
>>     git branch --set-upstream origin/master
>>
>> to create a new local branch 'origin/master' that tracks the current
>> branch. As --set-upstream already exists, we can't simply change its
>> behaviour. To work around this, introduce --set-upstream-to which
>> accepts a compulsory argument
>
> Thanks.  A part of me really dislikes this --set-upstream-to which
> is named more awkwardly than the deprecated mistake it replaces,
> though.

I am not super excited about it either, but at least it is a vast
improvement compared to the older one, with which it was entirely
unclear if we are setting the value of upstream *to* what is given
as an option, or setting the upstream *for* what is given on the
command line.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Jonathan Nieder]

Junio C Hamano wrote:

> I am not super excited about it either, but at least it is a vast
> improvement compared to the older one, with which it was entirely
> unclear if we are setting the value of upstream *to* what is given
> as an option, or setting the upstream *for* what is given on the
> command line.

Ah, do you mean that --set-upstream is meant to have usage like
"git remote set-url" and co?

	git remote set-url <remote> <url>
	git branch --set-upstream <branch> <upstream>

That's a reasonable stance, and it seems possible to get used to it.
In that case, we should just teach --set-upstream not to create
new branches, and people will get used to it.

The immediate problem that seems to trip people up is that it is very
tempting to run

	git branch --set-upstream junio/master

in an attempt to change what is upstream to the current branch, and
the result is some other completely counterintuitive thing.  I suspect
the order of arguments to --set-upstream is a red herring, as long as
it errors out when the arguments are switched to help people catch
mistakes.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The immediate problem that seems to trip people up is that it is very
> tempting to run
>
> 	git branch --set-upstream junio/master

I think we have discussed this already a few days ago.  See my
comment in the earlier thread before this round.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> The immediate problem that seems to trip people up is that it is very
>> tempting to run
>>
>> 	git branch --set-upstream junio/master
>
> I think we have discussed this already a few days ago.  See my
> comment in the earlier thread before this round.

You wrote[*]:

| I think it was a mistake that nobody noticed that it is likely that
| the operation most often will be done for the current branch and the
| usual "give me one branch name to operate on, or I'll operate on the
| current branch" command line convention of "git branch" commannd is
| not a good fit for it, when "set upstream" feature was added

with which I completely agree.  You then moved on to

|                                                               and
[someone should have]
| suggested an alternative syntax that avoids the mistake you quoted
| above, perhaps something like:
|
| 	git branch --set-upstream-to=origin/master [HEAD]

with which I disagree.

As far as I can tell, nobody really thought very hard about what
--set-upstream would do when passed only one argument.  It should have
been made to error out and only later change if someone had an idea
about how to make it useful.

Luckily we have a way out.  Any example transition plan looks like
the following.

DAY 1.

	$ git branch --set-upstream origin/master
	Branch origin/master set up to track local branch debian-sid.
	hint: If you intended to make the current branch track
	hint: origin/master, you can recover with the following commands:
	hint:  $ git branch -d origin/master
	hint:  $ git branch --set-upstream master origin/master
	$

DAY 2.

	$ git branch --set-upstream origin/master
	Branch origin/master set up to track local branch debian-sid.
	warning: using --set-upstream when creating a new branch is deprecated
	hint: use --track instead
	hint:
	hint: If you intended to make the current branch track
	hint: origin/master, you can recover with the following commands:
	hint:  $ git branch -d origin/master
	hint:  $ git branch --set-upstream master origin/master
	$

DAY 3.

	$ git branch --set-upstream origin/master
	fatal: no such branch "origin/master"
	$

DAY 4.

	$ git branch --set-upstream origin/master
	usage: git branch --set-upstream <branchname> <upstream>
	$

[*] http://thread.gmane.org/gmane.comp.version-control.git/201040/focus=201051

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> [someone should have]
> | suggested an alternative syntax that avoids the mistake you quoted
> | above, perhaps something like:
> |
> | 	git branch --set-upstream-to=origin/master [HEAD]
>
> with which I disagree.

You can think of it this way.

"git branch" can not only _create_ a new branch (or list existing
ones, but that is another entirely different mode), but also can be
used to set attributes to an existing branch.  Imagine a new option,
say --set-description, to replace branch.frotz.description, for
example.  It would be used like this:

	$ git branch --set-description='add frotz feature' frotz

to set the description for the 'frotz' branch (i.e. the above would
set branch.frotz.description), and we default to HEAD if 'frotz' is
missing from the command line.  "git branch --option [<branch>]" is
about manipulating the branch, and we default the target of
manipulation to HEAD.

"upstream" is just another kind of attribute for the branch being
manipulated, whose value happens to be a branch name.

The mistake was that --set-upstream was coded by piggybacking the
existing --track implementation where a new branch was created, and
in that codepath, "git branch <name1> [<name2>]" creates <name1>
while defaulting a missing <name2> to HEAD.

Creating a new branch that is forked from the current HEAD is an
often useful thing to do, so defaulting a missing <name2> (aka
"start-point") to HEAD is very sensible, but reconfiguring a named
branch <name1> to integrate with the current branch is much less
useful than the other way around.  One major reason why it is so is
because you would more likely set any branch to integrate with a
remote tracking branch (rather than a local branch) and by
definition your HEAD cannot be a remote tracking branch.

It makes it worse that you would often want to reconfigure the
current branch; for the purpose of reconfiguring a branch <name1> to
integrate with something else <name2>, it is much more likely that
you want a missing <name1> to default to HEAD, not the other way
around to default a missing <name2> to HEAD, which is useful for
branch creation.

But switching which missing argument gets default based on what
options are used is insane.

If the very original "create this new branch starting at that point"
were spelled like this

	$ git branch [--start-point=<name2>] <name1>

and a missing <name2> defaulted to HEAD, it probably would have been
better. It would have made it very unlikely to tempt anybody to hack
the --set-upstream option into the system with the wrong parameter
order if such a command line convention was in place.

If anything, it could be a sensible longer-term direction to a more
intuitive UI to deprecate the two-name format and make the creation
to be specified with an explicit --start-point option with an
argument (which defaults to HEAD), but I think that falls into the
"if I were reinventing git without existing userbase in 2005"
category and it is too late for that.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Jonathan Nieder]

Junio C Hamano wrote:

> You can think of it this way.
>
> "git branch" can not only _create_ a new branch (or list existing
> ones, but that is another entirely different mode), but also can be
> used to set attributes to an existing branch.  Imagine a new option,
> say --set-description, to replace branch.frotz.description, for
> example.  It would be used like this:
>
> 	$ git branch --set-description='add frotz feature' frotz

That's the same question.

You say that it would be used like that.  I say that it would be
more intuitive, given how "git remote", "git config", and other
commands other than "update-index --chmod" that set attributes already
work, for it to be used like this:

	git branch --set-description frotz 'add frotz feature'

Notice how similar that is to "git remote set-head origin master".
It would just be the consistent thing to do.

The truth is that neither one of us is right.  Both conventions
could work, and which one is more intuitive will vary from person
to person.  The convention used for plain "git branch" is

	copy(target, source)

That matches memcpy() and is the opposite of what "cp" uses.  Oh
well.  The convention used for "git remote add" is

	method(this, args...)

It's generally pretty natural.  The convention used for "git
update-index --chmod" is

	action(parameters)(files...)

That matches "chmod" so it was probably a good choice.

Hoping that clarifies,
Jonathan

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The truth is that neither one of us is right.  Both conventions
> could work, and which one is more intuitive will vary from person
> to person.

It is not just person-to-person, I think.

In short, you are saying that, assuming that missing <start> and
<branch> are given a sane default values (namely "HEAD"), the
syntax:

	git branch <branch> [<start>]
	git branch --set-upstream-jrn [<branch>] <upstream>

is easier to understand, while I think

	git branch <branch> [<start>]
        git branch --set-upstream-to=<upstream> [<branch>]

so that omitted things can come uniformly at the end (of course,
unless the --option=argument in the middle is omitted, that is)
makes things more consistent.

I do not think it is productive to keep agreeing that we disagree
and continuing to talk between ourselves without waiting for others
to catch up, so I'll stop here.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Jonathan Nieder]

Junio C Hamano wrote:

> In short, you are saying that, assuming that missing <start> and
> <branch> are given a sane default values (namely "HEAD"), the
> syntax:
>
>	git branch <branch> [<start>]
>	git branch --set-upstream-jrn [<branch>] <upstream>
>
> is easier to understand

I didn't propose allowing the branch argument to be omitted, actually.
It would be clearest, _especially_ because one argument currently
means something different, to make that error out.  Sorry for the lack
of clarity.

One more detail I didn't mention before: I think a convenience feature

	git branch --set-upstream-to <upstream>

that takes exactly one argument and means

	git branch --set-upstream HEAD <upstream>

would be fine.  Having a second command to do the same thing as
--set-upstream does (or adding new --set-other-things commands that
use this proposed convention where the value comes before the key) and
migrating awkwardly to it is what I object to.

Clearer?
Jonathan

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> is easier to understand, while I think
>
> 	git branch <branch> [<start>]
>         git branch --set-upstream-to=<upstream> [<branch>]

Isn't one problem with this that even if a "--set-upstream-to" option
exists, inevitably some [and I'm guessing, many] people will not be
aware of it (after all, nobody reads documentation more than they have
to), and will attempt to use "--set-upstream" with an argument
(that's the natural thing to do, after all) -- which may succeed with
weird results ...?

-miles

-- 
One of the lessons of history is that nothing is often a good thing to
do, and always a clever thing to say.  -- Will Durant

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Miles Bader <miles@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>> is easier to understand, while I think
>>
>> 	git branch <branch> [<start>]
>>         git branch --set-upstream-to=<upstream> [<branch>]
>
> Isn't one problem with this that even if a "--set-upstream-to" option
> exists, inevitably some [and I'm guessing, many] people will not be
> aware of it (after all, nobody reads documentation more than they have
> to), and will attempt to use "--set-upstream" with an argument
> (that's the natural thing to do, after all) -- which may succeed with
> weird results ...?

In the part you quoted in the message you are responding to in the
subthread between Jonathan and, I was expressing doubts about his
"upon seeing a single argument for operations that need two pieces
of info, sometimes the first one is assumed to be missing and gets
the default, some other times the second one is assumed to be
missing and gets the default" design, which I felt would be
unnecessarily confusing.

The issue of possible confusion you raised is real, was discussed in
the main thread of discussion of the earlier round, and has been
addressed in this round of the patch series, I think, with warnings
and/or advises.

[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 <cmn@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

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

[Matthieu Moy]

Carlos Martín Nieto <cmn@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
 */

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

Broken indentation.

> +		if (argc == 1) {
> +			printf("If you wanted to make '%s' track '%s', do this:\n", head, argv[0]);

Could be marked for translation with _("...").

> +			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.

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

> +			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:

You can suppress this message by setting them explicitly:

    git config --global user.name "Your Name"
    git config --global user.email you@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)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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 <cmn@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 you@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)
> 

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

[Junio C Hamano]

Carlos Martín Nieto <cmn@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 <cmn@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);

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 <cmn@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 <cmn@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

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@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.

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

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

[PATCH 3/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

We have ways of setting the upstream information, but if we want to
unset it, we need to resort to modifying the configuration manually.

Teach branch an --unset-upstream option that unsets this information.

---

create_branch() uses install_branch_config() which may also set
branch.foo.rebase, so this version might leave some configuration
laying around.

I wonder if deleting the whole branch.foo section would be better. Can
we be sure that nothing else shows up there?

 Documentation/git-branch.txt |    5 +++++
 builtin/branch.c             |   17 ++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 0f33fc7..c7cab08 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
+'git branch' --unset-upstream [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
 'git branch' --edit-description [<branchname>]
@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.
 	considered <branchname>'s upstream branch. If no branch is
 	specified it defaults to the current branch.
 
+--unset-upstream::
+	Remove the upstream information for <branchname>. If no branch
+	is specified it defaults to the current branch.
+
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
 	for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 5551227..1d1bf8e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, rename = 0, force_create = 0, list = 0;
 	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
-	int quiet = 0;
+	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
 		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
+		OPT_BOOLEAN(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
+	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
+	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)
@@ -863,6 +864,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		/* create_branch takes care of setting up the tracking
 		   info and making sure new_upstream is correct */
 		create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+	} else if (unset_upstream) {
+		struct branch *branch = branch_get(argv[0]);
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_addf(&buf, "branch.%s.remote", branch->name);
+		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "branch.%s.merge", branch->name);
+		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;
-- 
1.7.10.2.1.g8c77c3c

Re: [PATCH 3/3] branch: add --unset-upstream option

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> We have ways of setting the upstream information, but if we want to
> unset it, we need to resort to modifying the configuration manually.
>
> Teach branch an --unset-upstream option that unsets this information.
>
> ---
>
> create_branch() uses install_branch_config() which may also set
> branch.foo.rebase, so this version might leave some configuration
> laying around.
>
> I wonder if deleting the whole branch.foo section would be better. Can
> we be sure that nothing else shows up there?

"branch.foo.$unknown" may not be related to upstream at all, so that
will not fly.  Besides, we already have unknown=description, no?

If you are removing the branch "foo", it would make sense, though.

> +	} else if (unset_upstream) {
> +		struct branch *branch = branch_get(argv[0]);
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_addf(&buf, "branch.%s.remote", branch->name);
> +		git_config_set_multivar(buf.buf, NULL, NULL, 1);

This part makes sense, as "--set-upstream" is about setting the
value of branch.foo.remote to 'origin' or whatever.

> +		strbuf_reset(&buf);
> +		strbuf_addf(&buf, "branch.%s.merge", branch->name);
> +		git_config_set_multivar(buf.buf, NULL, NULL, 1);

This also makes sense because "branch.foo.merge" names a ref in the
context of the remote.  A branch may have integrated with the "dev"
branch at "origin" repository; when it is modified to slurp changes
from "central" repository from now on, there is nothing that says
that the branch "dev" at this different remote corresponds to the
"dev" branch at the original "origin" repository (such a branch may
not even exist at the new "central" repository).  There is no point
leaving the "branch.foo.merge" configuration behind when you unset
the upstream information.



> +		strbuf_release(&buf);
>  	} else if (argc > 0 && argc <= 2) {
>  		struct branch *branch = branch_get(argv[0]);
>  		const char *old_upstream = NULL;

Re: [PATCH 3/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 11:02 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > We have ways of setting the upstream information, but if we want to
> > unset it, we need to resort to modifying the configuration manually.
> >
> > Teach branch an --unset-upstream option that unsets this information.
> >
> > ---
> >
> > create_branch() uses install_branch_config() which may also set
> > branch.foo.rebase, so this version might leave some configuration
> > laying around.
> >
> > I wonder if deleting the whole branch.foo section would be better. Can
> > we be sure that nothing else shows up there?
> 
> "branch.foo.$unknown" may not be related to upstream at all, so that
> will not fly.  Besides, we already have unknown=description, no?

Ah yes, that exists. I've added a bit of code to also remove
branch.foo.rebase, which I'd also consider to be part of the upstream
information.

   cmn

Re: [PATCH 3/3] branch: add --unset-upstream option

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> I've added a bit of code to also remove branch.foo.rebase, which
> I'd also consider to be part of the upstream information.

If "git branch -t" or "git branch --set-upstream" took another
option "--integrate-with=[rebase|merge]" to set the variable, I
would agree that removing branch.$name.rebase would be the right
thing to do, but because it is not, I do not know if it is sensible
to remove it upon --unset-upstream.

I actually thought about commenting on that exact variable in my
review, saying that the patch was correct that it didn't touch it.

Re: [PATCH 3/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

On Wed, 2012-07-11 at 09:53 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > I've added a bit of code to also remove branch.foo.rebase, which
> > I'd also consider to be part of the upstream information.
> 
> If "git branch -t" or "git branch --set-upstream" took another
> option "--integrate-with=[rebase|merge]" to set the variable, I
> would agree that removing branch.$name.rebase would be the right
> thing to do, but because it is not, I do not know if it is sensible
> to remove it upon --unset-upstream.

I'll drop it then.

   cmn