[Bug] branch.*.merge interpreted too strictly by tracking logic

[Junio C Hamano]

Start from an empty repository like so:

: gitster track; git init
Initialized empty Git repository in /var/tmp/x/track/.git/
: gitster track/master; git commit --allow-empty -m initial
[master (root-commit) abdcd1c] initial
: gitster track/master; git branch foo
: gitster track/master; git branch bar
: gitster track/master; git commit --allow-empty -m second
[master 78e28f4] second

Now, 'master' has two commits, while 'foo' and 'bar' both are one
commit behind, pointing at 'master^1'.

Let's tell these branches that they are both supposed to be building
on top of 'master'.

: gitster track/master; git config branch.foo.remote .
: gitster track/master; git config branch.foo.merge refs/heads/master
: gitster track/master; git config branch.bar.remote .
: gitster track/master; git config branch.bar.merge master

The difference between the two is that 'foo' spells the @{upstream}
branch in full (which is the recommended practice), while 'bar' is
loose and asks for 'master'.

Let's see what happens on these two branches. First 'foo':

: gitster track/master; git checkout foo
Switched to branch 'foo'
Your branch is behind 'master' by 1 commit, and can be
fast-forwarded.
(use "git pull" to update your local branch)
: gitster track/foo; git pull
From .
* branch master -> FETCH_HEAD
Updating abdcd1c..78e28f4
Fast-forward

The 'checkout' correctly reports that 'foo' is building on (local)
'master'. 'pull' works as expected, of course.

Now, here is the bug part. The same thing on 'bar':

: gitster track/foo; git checkout bar
Switched to branch 'bar'
Your branch is based on 'master', but the upstream is gone.
(use "git branch --unset-upstream" to fixup)

It knows about 'master', but what is "the upstream is gone"? It is
our local repository and it definitely is not gone.

But 'pull' of course works as expected (this behaviour is older and
stable for a long time since even before @{upstream} was invented).

: gitster track/bar; git pull
From .
* branch master -> FETCH_HEAD
Updating abdcd1c..78e28f4
Fast-forward

I suspect that the real culprit is somewhere in wt-status.c

: gitster track/bar; git status
On branch bar
Your branch is based on 'master', but the upstream is gone.
(use "git branch --unset-upstream" to fixup)

nothing to commit, working directory clean

Resolving @{upstream} works just fine for both.

: gitster track/bar; git rev-parse --symbolic-full-name foo@{upstream}
refs/heads/master
: gitster track/bar; git rev-parse --symbolic-full-name bar@{upstream}
refs/heads/master

Thanks.

[Jeff King]

Post by Junio C Hamano
Let's tell these branches that they are both supposed to be building
on top of 'master'.
: gitster track/master; git config branch.foo.remote .
: gitster track/master; git config branch.foo.merge refs/heads/master
: gitster track/master; git config branch.bar.remote .
: gitster track/master; git config branch.bar.merge master
branch in full (which is the recommended practice), while 'bar' is
loose and asks for 'master'.
Actually, it is broken in a lot of places. for-each-ref relies on
the same code as "git status", "git checkout", etc, which will all
fail to display tracking info. I believe the same code is also used
for updating tracking branches on push. So I'm not sure if it was
ever intended to be a valid setting.

It wasn't. Some places may accept them gracefully by either being
extra nice or by accident.

I don't recall us ever doing anything after that. I don't have a problem
with making it work, of course, but I am not sure if it is really a bug.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/121671

[Junio C Hamano]

Post by Jeff King

Post by Junio C Hamano
Actually, it is broken in a lot of places. for-each-ref relies on
the same code as "git status", "git checkout", etc, which will all
fail to display tracking info. I believe the same code is also used
for updating tracking branches on push. So I'm not sure if it was
ever intended to be a valid setting.

It wasn't. Some places may accept them gracefully by either being
extra nice or by accident.
I don't recall us ever doing anything after that. I don't have a problem
with making it work, of course, but I am not sure if it is really a bug.

Once people get used to us being extra nice in some places, other
less nice places start looking more and more like bugs. It is an
unfortunate fact of life, but fixing them up is a good thing for
users. As long as we can make those less nice places nicer
uniformly without bending backwards or introducing unnecessary
ambiguities, that is, and I think this one can be done without
such downsides.

[Jeff King]

Post by Junio C Hamano

Post by Jeff King
I don't recall us ever doing anything after that. I don't have a problem
with making it work, of course, but I am not sure if it is really a bug.

Once people get used to us being extra nice in some places, other
less nice places start looking more and more like bugs. It is an
unfortunate fact of life, but fixing them up is a good thing for
users. As long as we can make those less nice places nicer
uniformly without bending backwards or introducing unnecessary
ambiguities, that is, and I think this one can be done without
such downsides.

Oh, absolutely, and I do not think we are breaking anything to start
handling it better (my "I don't have a problem..." above). But I guess I
am doubting that people are actually doing this at all now. I'd expect
most people to have the config set automatically by "branch" or
"checkout", or to use "branch --set-upstream-to". Did your report come
out of a real case, or was it just something you noticed?

-Peff

[Junio C Hamano]

.... Did your report come
out of a real case, or was it just something you noticed?

Some git-wrappers (like "repo") are reported to muck with the
configuration files.

[Junio C Hamano]

When checking out a branch that is set to build on top of another
branch (often, a remote-tracking branch), "git checkout" reports how
your work relates to the other branch, e.g.

Your branch is behind 'origin/master', and can be fast-forwarded.

Back when this feature was introduced, this was only done for
branches that build on remote-tracking branches, but 5e6e2b48 (Make
local branches behave like remote branches when --tracked,
2009-04-01) added support to give the same report for branches that
build on other local branches (i.e. branches whose branch.*.remote
variables are set to '.'). Unlike the support for the branches
building on remote-tracking branches, however, this did not take
into account the fact that branch.*.merge configuration is allowed
to record a shortened branch name.

When branch.*.merge is set to 'master' (not 'refs/heads/master'),
i.e. "my branch builds on the local 'master' branch", this caused
"git checkout" to report:

Your branch is based on 'master', but the upstream is gone.

The upstream is our repository and is definitely not gone, so this
output is nonsense.

The fix is fairly obvious; just like the branch name is DWIMed when
"git pull" merges from the 'master' branch without complaint on such
a branch, the name of the branch the current branch builds upon
needs to be DWIMed the same way.

Signed-off-by: Junio C Hamano <***@pobox.com>
---
remote.c | 34 +++++++++++++++++++++++-----------
t/t2024-checkout-dwim.sh | 18 ++++++++++++++++++
2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index 0e9459c..ecbe363 100644
--- a/remote.c
+++ b/remote.c
@@ -1611,6 +1611,27 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
}
}

+static void set_merge(struct branch *ret)
+{
+ char *ref;
+ unsigned char sha1[20];
+ int i;
+
+ ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
+ for (i = 0; i < ret->merge_nr; i++) {
+ ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
+ ret->merge[i]->src = xstrdup(ret->merge_name[i]);
+ if (!remote_find_tracking(ret->remote, ret->merge[i]) ||
+ strcmp(ret->remote_name, "."))
+ continue;
+ if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
+ sha1, &ref) == 1)
+ ret->merge[i]->dst = ref;
+ else
+ ret->merge[i]->dst = xstrdup(ret->merge_name[i]);
+ }
+}
+
struct branch *branch_get(const char *name)
{
struct branch *ret;
@@ -1622,17 +1643,8 @@ struct branch *branch_get(const char *name)
ret = make_branch(name, 0);
if (ret && ret->remote_name) {
ret->remote = remote_get(ret->remote_name);
- if (ret->merge_nr) {
- int i;
- ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
- for (i = 0; i < ret->merge_nr; i++) {
- ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
- ret->merge[i]->src = xstrdup(ret->merge_name[i]);
- if (remote_find_tracking(ret->remote, ret->merge[i])
- && !strcmp(ret->remote_name, "."))
- ret->merge[i]->dst = xstrdup(ret->merge_name[i]);
- }
- }
+ if (ret->merge_nr)
+ set_merge(ret);
}
return ret;
}
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 6ecb559..468a000 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -185,4 +185,22 @@ test_expect_success 'checkout <branch> -- succeeds, even if a file with the same
test_branch_upstream spam repo_c spam
'

+test_expect_success 'loosely defined local base branch is reported correctly' '
+
+ git checkout master &&
+ git branch strict &&
+ git branch loose &&
+ git commit --allow-empty -m "a bit more" &&
+
+ test_config branch.strict.remote . &&
+ test_config branch.loose.remote . &&
+ test_config branch.strict.merge refs/heads/master &&
+ test_config branch.loose.merge master &&
+
+ git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
+ git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
+
+ test_cmp expect actual
+'
+
test_done