[PATCH v2 0/9] A natural solution to the @ -> HEAD problem

[PATCH v2 0/9] A natural solution to the @ -> HEAD problem

[Ramkumar Ramachandra]

Hi,

So, we're back with v2 of the series.  Six patches add comprehensive
tests to at-combinations, guarding against any future bugs.  [7/9] is
the most important part, where I've fixed branch_get() with a long
justification.  [8/9] and [9/9] are almost trivial.

As Felipe pointed out, [9/9] might require more documentation, but I
don't know where to put it.  Help in the area is appreciated.

I've excluded all @-parsing simplification in this series.  It will
come as a later series based on this one: I'm currently integrating
Felipe's work with mine.

Felipe: can I have your sign-off for the first three parts?

Thanks.

Felipe Contreras (3):
  t1508 (at-combinations): simplify setup
  t1508 (at-combinations): test branches separately
  t1508 (at-combinations): improve nonsense()

Ramkumar Ramachandra (6):
  t1508 (at-combinations): increase coverage
  t1508 (at-combinations): document @{N} versus HEAD@{N}
  t1508 (at-combinations): test with symbolic refs
  remote.c: teach branch_get() to treat symrefs other than HEAD
  sha1_name.c: fix @-parsing bug in interpret_branch_name()
  refs.c: make @ a pseudo-ref alias to HEAD

 Documentation/git-check-ref-format.txt |  2 +
 Documentation/revisions.txt            |  8 +++-
 refs.c                                 | 12 +++++-
 remote.c                               | 23 ++++++++++--
 sha1_name.c                            |  2 +-
 t/t1400-update-ref.sh                  |  3 ++
 t/t1508-at-combinations.sh             | 68 ++++++++++++++++++++++++++--------
 7 files changed, 94 insertions(+), 24 deletions(-)

-- 
1.8.3.rc0.40.g09a0447

[PATCH 1/9] t1508 (at-combinations): simplify setup

[Ramkumar Ramachandra]

From: Felipe Contreras <felipe.contreras@gmail.com>

Instead of setting the configuration variables branch.<name>.merge and
branch.<name>.remote by hand, use the shortcut 'git branch -u', which
is exactly equivalent.  It's much more readable now.

[rr: commit message]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index d5d6244..46e3f16 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -31,10 +31,8 @@ test_expect_success 'setup' '
 	git checkout -b new-branch &&
 	test_commit new-one &&
 	test_commit new-two &&
-	git config branch.old-branch.remote . &&
-	git config branch.old-branch.merge refs/heads/master &&
-	git config branch.new-branch.remote . &&
-	git config branch.new-branch.merge refs/heads/upstream-branch
+	git branch -u master old-branch &&
+	git branch -u upstream-branch new-branch
 '
 
 check HEAD new-two
-- 
1.8.3.rc0.40.g09a0447

[PATCH 2/9] t1508 (at-combinations): test branches separately

[Ramkumar Ramachandra]

From: Felipe Contreras <felipe.contreras@gmail.com>

In the tests involving @{-1} and @{u} as the final component, what we
really want to check is if it's pointing to the right ref.  We
currently check the tip commit of the ref, but we can clarify this by
separating out checking for commits versus checking for refs at
check().

[rr: commit message, fix arguments in check()]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 46e3f16..cacb2d0 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'
 . ./test-lib.sh
 
 check() {
-test_expect_${3:-success} "$1 = $2" "
-	echo '$2' >expect &&
-	git log -1 --format=%s '$1' >actual &&
+test_expect_${4:-success} "$1 = ${3:-$2}" "
+	if [ '$2' == 'commit' ]; then
+		echo '$3' >expect &&
+		git log -1 --format=%s '$1' >actual
+	else
+		echo '${3:-$2}' >expect &&
+		git rev-parse --symbolic-full-name '$1' >actual
+	fi &&
 	test_cmp expect actual
 "
 }
@@ -35,14 +40,14 @@ test_expect_success 'setup' '
 	git branch -u upstream-branch new-branch
 '
 
-check HEAD new-two
-check "@{1}" new-one
-check "@{-1}" old-two
-check "@{-1}@{1}" old-one
-check "@{u}" upstream-two
-check "@{u}@{1}" upstream-one
-check "@{-1}@{u}" master-two
-check "@{-1}@{u}@{1}" master-one
+check HEAD refs/heads/new-branch
+check "@{1}" commit new-one
+check "@{-1}" refs/heads/old-branch
+check "@{-1}@{1}" commit old-one
+check "@{u}" refs/heads/upstream-branch
+check "@{u}@{1}" commit upstream-one
+check "@{-1}@{u}" refs/heads/master
+check "@{-1}@{u}@{1}" commit master-one
 nonsense "@{u}@{-1}"
 nonsense "@{1}@{u}"
 
-- 
1.8.3.rc0.40.g09a0447

[PATCH 3/9] t1508 (at-combinations): improve nonsense()

[Ramkumar Ramachandra]

From: Felipe Contreras <felipe.contreras@gmail.com>

nonsense() is checking at the wrong abstraction layer: it doesn't need
to invoke a 'git log' at all; it is sufficient to check that 'git
rev-parse' can't resolve the argument.

[rr: commit message, minor modification]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index cacb2d0..55fdd0f 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -17,7 +17,7 @@ test_expect_${4:-success} "$1 = ${3:-$2}" "
 }
 nonsense() {
 test_expect_${2:-success} "$1 is nonsensical" "
-	test_must_fail git log -1 '$1'
+	test_must_fail git rev-parse '$1'
 "
 }
 fail() {
-- 
1.8.3.rc0.40.g09a0447

[PATCH 4/9] t1508 (at-combinations): increase coverage

[Ramkumar Ramachandra]

Add more tests exercising documented functionality.

It is worth nothing that HEAD@{-<n>} is senseless because @{-<n>}
cannot be used with anything other than HEAD anyway.

[fc: contribute a couple of tests]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 55fdd0f..eaa5337 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -42,13 +42,21 @@ test_expect_success 'setup' '
 
 check HEAD refs/heads/new-branch
 check "@{1}" commit new-one
+check "HEAD@{1}" commit new-one
+check "@{now}" commit new-two
+check "HEAD@{now}" commit new-two
 check "@{-1}" refs/heads/old-branch
+check "@{-1}@{0}" commit old-two
 check "@{-1}@{1}" commit old-one
 check "@{u}" refs/heads/upstream-branch
+check "HEAD@{u}" refs/heads/upstream-branch
 check "@{u}@{1}" commit upstream-one
 check "@{-1}@{u}" refs/heads/master
 check "@{-1}@{u}@{1}" commit master-one
 nonsense "@{u}@{-1}"
+nonsense "@{0}@{0}"
 nonsense "@{1}@{u}"
+nonsense "HEAD@{-1}"
+nonsense "@{-1}@{-1}"
 
 test_done
-- 
1.8.3.rc0.40.g09a0447

[PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

[Ramkumar Ramachandra]

All the tests so far check that @{N} is the same as HEAD@{N} (for
positive N).  However, this is not always the case: write a couple of
tests for this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index eaa5337..29ffd73 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,4 +59,18 @@ nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 nonsense "@{-1}@{-1}"
 
+# Document differences between @{N} and HEAD@{N}.  The former resolves
+# HEAD to a branch and looks up the logs for that branch, while the
+# latter looks for the logs of HEAD.
+check "HEAD@{3}" commit old-two
+nonsense "@{3}"
+
+test_expect_success 'switch to old-branch' '
+	git checkout old-branch
+'
+
+check HEAD refs/heads/old-branch
+check "HEAD@{1}" commit new-two
+check "@{1}" commit old-one
+
 test_done
-- 
1.8.3.rc0.40.g09a0447

[PATCH 6/9] t1508 (at-combinations): test with symbolic refs

[Ramkumar Ramachandra]

Symbolic refs don't work with @{u}.  Document this failure.  Also
create a symbolic-ref @ (which is a valid ref name), and test
everything with it to make sure that there is no bug in the @-parser.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 29ffd73..bb86c79 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,6 +59,19 @@ nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 nonsense "@{-1}@{-1}"
 
+# Symbolic refs
+git symbolic-ref H HEAD
+check "H@{1}" commit new-one
+check "H@{now}" commit new-two
+check "H@{u}" ref refs/heads/upstream-branch failure
+
+# To make sure that the @-parser isn't buggy, check things with the
+# symbolic-ref @
+git symbolic-ref @ HEAD
+check "@@{1}" commit new-one
+check "@@{now}" commit new-two
+check "@@{u}" ref refs/heads/upstream-branch failure
+
 # Document differences between @{N} and HEAD@{N}.  The former resolves
 # HEAD to a branch and looks up the logs for that branch, while the
 # latter looks for the logs of HEAD.
-- 
1.8.3.rc0.40.g09a0447

[PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

[Ramkumar Ramachandra]

Broadly, there are two ways to resolve a command-line argument.  The
first approach is to treat it as a revision and resolve it using the
revision-parsing mechanism get_sha1().  The other approach is to treat
it as a ref, and use a reduced mechanism like branch_get().

Compare and contrast the implementations of @{N} and @{u} in
sha1_name.c to see where exactly branch_get() is useful:

In <...>@{N}, the <...> is passed through dwim_log() which first calls
substitute_branch_name() and then resolve_ref_unsafe() on that.  The
resolved ref could be any ref (symbolic/ non-symbolic), and not just a
refs/heads/* ref (branch).

However, in the <...>@{u} case, the <...> can only be a ref/heads/*
ref or a symbolic ref pointing to a refs/heads/* ref.  It uses
branch_get() to find the relevant ref.

Unfortunately, branch_get() doesn't call resolve_ref_unsafe() on what
is passed to it at all.  Instead it hard-codes the special four-letter
word "HEAD" (which is resolved by read_config() which is called
first), and rejects any other symbolic ref.  This is a historical
mistake because of which some callers of branch_get() suffer.  To put
it another way, the commands that accept "HEAD" (and resolve it into a
branch) can benefit from accepting any other symbolic ref that
resolves to a branch.  For example, the following fail:

    $ git symbolic-ref M refs/heads/master
    $ git show M@{u}
    $ git branch -u ram/master M

This patch fixes branch_get() directly, making these work.

Notice that branch_get() calls read_config(), which in turn
preemptively calls resolve_ref_unsafe() on "HEAD", when the caller
might not even be requesting the current branch.  After copying out
two critical tasks that are required for make_remote() to work
properly:

    git_config(handle_config, NULL);
    alias_all_urls();

we can remove read_config() and call resolve_ref_unsafe() on the
argument that is passed ourselves.  HEAD is therefore not a special
case anymore.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c                   | 23 ++++++++++++++++++++---
 t/t1508-at-combinations.sh |  2 +-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 68eb99b..3ac302f 100644
--- a/remote.c
+++ b/remote.c
@@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 struct branch *branch_get(const char *name)
 {
-	struct branch *ret;
+	struct branch *ret = NULL;
+
+	/* Setup */
+	git_config(handle_config, NULL);
+	alias_all_urls();
 
-	read_config();
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = current_branch;
+		name = "HEAD";
 	else
 		ret = make_branch(name, 0);
+
+	if (!ret || !ret->remote_name) {
+		/* Is this a symbolic-ref like HEAD, pointing to a
+		 * valid branch?
+		 */
+		const char *this_ref;
+		unsigned char sha1[20];
+		int flag;
+
+		this_ref = resolve_ref_unsafe(name, sha1, 0, &flag);
+		if (this_ref && (flag & REF_ISSYMREF) &&
+			!prefixcmp(this_ref, "refs/heads/"))
+			ret = make_branch(this_ref + strlen("refs/heads/"), 0);
+	}
 	if (ret && ret->remote_name) {
 		ret->remote = remote_get(ret->remote_name);
 		if (ret->merge_nr) {
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index bb86c79..424caf5 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -63,7 +63,7 @@ nonsense "@{-1}@{-1}"
 git symbolic-ref H HEAD
 check "H@{1}" commit new-one
 check "H@{now}" commit new-two
-check "H@{u}" ref refs/heads/upstream-branch failure
+check "H@{u}" refs/heads/upstream-branch
 
 # To make sure that the @-parser isn't buggy, check things with the
 # symbolic-ref @
-- 
1.8.3.rc0.40.g09a0447

[PATCH 8/9] sha1_name.c: fix @-parsing bug in interpret_branch_name()

[Ramkumar Ramachandra]

Symbolic refs work in the general case, but making @ a symbolic-ref
doesn't work due to a parsing bug in interpret_branch_name().  Update
the function to parse "@{", not '@' (since '@' is a valid symbolic
ref).

This makes a failing test in t1508 (at-combinations) pass.  In other
words, you can now do:

    $ git symbolic-ref @ HEAD

And expect the following to work:

    $ git rev-parse @@{u}

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c                | 2 +-
 t/t1508-at-combinations.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..850e6d7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1019,7 +1019,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 		return ret - used + len;
 	}
 
-	cp = strchr(name, '@');
+	cp = strstr(name, "@{");
 	if (!cp)
 		return -1;
 	tmp_len = upstream_mark(cp, namelen - (cp - name));
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 424caf5..6a69f9e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -70,7 +70,7 @@ check "H@{u}" refs/heads/upstream-branch
 git symbolic-ref @ HEAD
 check "@@{1}" commit new-one
 check "@@{now}" commit new-two
-check "@@{u}" ref refs/heads/upstream-branch failure
+check "@@{u}" refs/heads/upstream-branch
 
 # Document differences between @{N} and HEAD@{N}.  The former resolves
 # HEAD to a branch and looks up the logs for that branch, while the
-- 
1.8.3.rc0.40.g09a0447

[PATCH 9/9] refs.c: make @ a pseudo-ref alias to HEAD

[Ramkumar Ramachandra]

First, make sure that check_refname_format() rejects the a refname
beginning with a '@'.  Add a test to t1400 (update-ref) demonstrating
that update-ref forbids the user from updating a ref named "@".

Now, resolve_ref_unsafe() is built to resolve any refs that have a
corresponding file inside $GITDIR.  Our "@" ref is a special
pseudo-ref and does not have a filesystem counterpart.  So,
hard-interpret "@" as "HEAD" and resolve .git/HEAD as usual.  This
means that we can drop the 'git symbolic-ref @ HEAD' line in t1508
(at-combinations), and everything will continue working as usual.

If the user does manage to create a '.git/@' unsafely (via
symbolic-ref or otherwise), it will be ignored.

In practice, this means that you will now be able to do:

    $ git show @~1
    $ git log @^2

Advertise these features in the tests and documentation.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-check-ref-format.txt |  2 ++
 Documentation/revisions.txt            |  8 ++++++--
 refs.c                                 | 12 ++++++++++--
 t/t1400-update-ref.sh                  |  3 +++
 t/t1508-at-combinations.sh             |  4 +---
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index ec1739a..3de9adc 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -52,6 +52,8 @@ Git imposes the following rules on how references are named:
 
 . They cannot end with a dot `.`.
 
+. They cannot be the single character `@`.
+
 . They cannot contain a sequence `@{`.
 
 . They cannot contain a `\`.
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index d477b3f..02bfc90 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -27,6 +27,10 @@ blobs contained in a commit.
   When ambiguous, a '<refname>' is disambiguated by taking the
   first match in the following rules:
 
+  . '@' is a synonym for 'HEAD'.  An '@' followed by '\{' has no
+    relationship to this and means something entirely different (see
+    below).
+
   . If '$GIT_DIR/<refname>' exists, that is what you mean (this is usually
     useful only for 'HEAD', 'FETCH_HEAD', 'ORIG_HEAD', 'MERGE_HEAD'
     and 'CHERRY_PICK_HEAD');
@@ -93,7 +97,7 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build on
   top of.  A missing branchname defaults to the current one.
 
-'<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
+'<rev>{caret}', e.g. '@{caret}2', 'HEAD{caret}', 'v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
   that commit object.  '{caret}<n>' means the <n>th parent (i.e.
   '<rev>{caret}'
@@ -101,7 +105,7 @@ some output processing may assume ref names in UTF-8.
   '<rev>{caret}0' means the commit itself and is used when '<rev>' is the
   object name of a tag object that refers to a commit object.
 
-'<rev>{tilde}<n>', e.g. 'master{tilde}3'::
+'<rev>{tilde}<n>', e.g. '@{tilde}1', 'master{tilde}3'::
   A suffix '{tilde}<n>' to a revision parameter means the commit
   object that is the <n>th generation ancestor of the named
   commit object, following only the first parents.  I.e. '<rev>{tilde}3' is
diff --git a/refs.c b/refs.c
index de2d8eb..6a75f77 100644
--- a/refs.c
+++ b/refs.c
@@ -72,6 +72,9 @@ int check_refname_format(const char *refname, int flags)
 {
 	int component_len, component_count = 0;
 
+	if (!strcmp(refname, "@"))
+		return -1;
+
 	while (1) {
 		/* We are at the start of a path component. */
 		component_len = check_refname_component(refname, flags);
@@ -1093,8 +1096,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 	if (flag)
 		*flag = 0;
 
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		return NULL;
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) < 0) {
+		/* Handle the pseudo-ref @ */
+		if (!strcmp(refname, "@"))
+			refname = "HEAD";
+		else
+			return NULL;
+	}
 
 	for (;;) {
 		char path[PATH_MAX];
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..ee93979 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -127,6 +127,9 @@ test_expect_success '(not) change HEAD with wrong SHA1' "
 test_expect_success "(not) changed .git/$m" "
 	! test $B"' = $(cat .git/'"$m"')
 '
+test_expect_success 'disallow creating a ref with name @' '
+	test_must_fail git update-ref @ HEAD
+'
 rm -f .git/$m
 
 : a repository with working tree always has reflog these days...
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 6a69f9e..c50c3f7 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -65,9 +65,7 @@ check "H@{1}" commit new-one
 check "H@{now}" commit new-two
 check "H@{u}" refs/heads/upstream-branch
 
-# To make sure that the @-parser isn't buggy, check things with the
-# symbolic-ref @
-git symbolic-ref @ HEAD
+# Check everything with pseudo-ref @
 check "@@{1}" commit new-one
 check "@@{now}" commit new-two
 check "@@{u}" refs/heads/upstream-branch
-- 
1.8.3.rc0.40.g09a0447

Re: [PATCH 4/9] t1508 (at-combinations): increase coverage

[Eric Sunshine]

On Thu, May 2, 2013 at 9:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Add more tests exercising documented functionality.
>
> It is worth nothing that HEAD@{-<n>} is senseless because @{-<n>}

s/nothing/noting/

> cannot be used with anything other than HEAD anyway.
>
> [fc: contribute a couple of tests]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> [...]

So sorry about this, but this breaks some tests in t1507
(rev-parse-upstream).  I'm looking into this now.

In the meantime, reviewers can focus on the commit message.

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> So sorry about this, but this breaks some tests in t1507
> (rev-parse-upstream).  I'm looking into this now.
>
> In the meantime, reviewers can focus on the commit message.

So, it turns out that some callers expect it to read_config().

If we're still adamant about not touching branch_get(), the
alternative is to call resolve_ref_unsafe() before the branch_get()
call in interpret_branch_name().  We'll still retain the H@{u}, but
we'll lose the 'git branch -u ram/master M'.

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

[Felipe Contreras]

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

>     $ git symbolic-ref M refs/heads/master
>     $ git show M@{u}
>     $ git branch -u ram/master M
>
> This patch fixes branch_get() directly, making these work.

It's not a fix, it's implementing brand new functionality, and
changing old behaviors.

> --- a/remote.c
> +++ b/remote.c
> @@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>
>  struct branch *branch_get(const char *name)
>  {
> -       struct branch *ret;
> +       struct branch *ret = NULL;
> +
> +       /* Setup */

I thought the preferred style for these kinds of comments was:

  /* setup */

> +       git_config(handle_config, NULL);
> +       alias_all_urls();
>
> -       read_config();
>         if (!name || !*name || !strcmp(name, "HEAD"))

I still see "HEAD" there. Didn't you say your patch makes "HEAD" less
of a special ref? Why is it still here?

> -               ret = current_branch;
> +               name = "HEAD";
>         else
>                 ret = make_branch(name, 0);
> +
> +       if (!ret || !ret->remote_name) {
> +               /* Is this a symbolic-ref like HEAD, pointing to a
> +                * valid branch?
> +                */

The style is:

  /*
   * This is the preferred style for multi-line
   * comments in the Linux kernel source code.
   * Please use it consistently.
   *
   * Description:  A column of asterisks on the left side,
   * with beginning and ending almost-blank lines.
   */

-- 
Felipe Contreras

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Felipe Contreras (3):
>   t1508 (at-combinations): simplify setup
>   t1508 (at-combinations): test branches separately
>   t1508 (at-combinations): improve nonsense()
>
> Ramkumar Ramachandra (6):
>   t1508 (at-combinations): increase coverage
>   t1508 (at-combinations): document @{N} versus HEAD@{N}
>   t1508 (at-combinations): test with symbolic refs

Can we just okay these six now, and drop the rest?

Thanks.

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

[Felipe Contreras]

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> In the tests involving @{-1} and @{u} as the final component, what we
> really want to check is if it's pointing to the right ref.  We
> currently check the tip commit of the ref, but we can clarify this by
> separating out checking for commits versus checking for refs at
> check().
>
> [rr: commit message, fix arguments in check()]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index 46e3f16..cacb2d0 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'
>  . ./test-lib.sh
>
>  check() {
> -test_expect_${3:-success} "$1 = $2" "
> -       echo '$2' >expect &&
> -       git log -1 --format=%s '$1' >actual &&
> +test_expect_${4:-success} "$1 = ${3:-$2}" "
> +       if [ '$2' == 'commit' ]; then
> +               echo '$3' >expect &&
> +               git log -1 --format=%s '$1' >actual
> +       else
> +               echo '${3:-$2}' >expect &&
> +               git rev-parse --symbolic-full-name '$1' >actual
> +       fi &&
>         test_cmp expect actual
>  "
>  }

I'm not sure about this. If we introduce a check that fails, we would
have to do:

check HEAD refs/heads/new-branch "" failure

Which doesn't seem clean. Perhaps it makes more sense to always add
the type of check:

check HEAD ref refs/heads/new-branch

-- 
Felipe Contreras

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

[Felipe Contreras]

On Thu, May 2, 2013 at 12:18 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> Felipe Contreras (3):
>>   t1508 (at-combinations): simplify setup
>>   t1508 (at-combinations): test branches separately
>>   t1508 (at-combinations): improve nonsense()
>>
>> Ramkumar Ramachandra (6):
>>   t1508 (at-combinations): increase coverage
>>   t1508 (at-combinations): document @{N} versus HEAD@{N}
>>   t1508 (at-combinations): test with symbolic refs
>
> Can we just okay these six now, and drop the rest?

I'm generally OK with them, but the one for 'symbolic refs' is relying
on a symbolic ref named '@', so it would immediately conflict with my
patch series.

-- 
Felipe Contreras

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

[Felipe Contreras]

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> All the tests so far check that @{N} is the same as HEAD@{N} (for
> positive N).  However, this is not always the case: write a couple of
> tests for this.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1508-at-combinations.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index eaa5337..29ffd73 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -59,4 +59,18 @@ nonsense "@{1}@{u}"
>  nonsense "HEAD@{-1}"
>  nonsense "@{-1}@{-1}"
>
> +# Document differences between @{N} and HEAD@{N}.

That's enough explanation, except that we not documenting them, we are
testing them.

> The former resolves
> +# HEAD to a branch and looks up the logs for that branch, while the
> +# latter looks for the logs of HEAD.
> +check "HEAD@{3}" commit old-two
> +nonsense "@{3}"

> +test_expect_success 'switch to old-branch' '
> +       git checkout old-branch
> +'
> +
> +check HEAD refs/heads/old-branch
> +check "HEAD@{1}" commit new-two
> +check "@{1}" commit old-one

And here we are testing something completely different.

-- 
Felipe Contreras

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> I'm not sure about this. If we introduce a check that fails, we would
> have to do:
>
> check HEAD refs/heads/new-branch "" failure
>
> Which doesn't seem clean. Perhaps it makes more sense to always add
> the type of check:
>
> check HEAD ref refs/heads/new-branch

I think you misunderstood.  Failure looks like this:

    check "@@{u}" ref refs/heads/upstream-branch failure

And corresponding success like this:

    check "@@{u}" refs/heads/upstream-branch

We can make the "ref" compulsory if you like.  I thought about it too.

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

[Ramkumar Ramachandra]

Felipe Contreras wrote:
>> +test_expect_success 'switch to old-branch' '
>> +       git checkout old-branch
>> +'
>> +
>> +check HEAD refs/heads/old-branch
>> +check "HEAD@{1}" commit new-two
>> +check "@{1}" commit old-one
>
> And here we are testing something completely different.

How?  I'm showing that HEAD@{1} can be different from @{1}.

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

[Felipe Contreras]

On Thu, May 2, 2013 at 12:28 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> I'm not sure about this. If we introduce a check that fails, we would
>> have to do:
>>
>> check HEAD refs/heads/new-branch "" failure
>>
>> Which doesn't seem clean. Perhaps it makes more sense to always add
>> the type of check:
>>
>> check HEAD ref refs/heads/new-branch
>
> I think you misunderstood.  Failure looks like this:
>
>     check "@@{u}" ref refs/heads/upstream-branch failure
>
> And corresponding success like this:
>
>     check "@@{u}" refs/heads/upstream-branch
>
> We can make the "ref" compulsory if you like.  I thought about it too.

I think it's less surprising.

-- 
Felipe Contreras

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> I'm generally OK with them, but the one for 'symbolic refs' is relying
> on a symbolic ref named '@', so it would immediately conflict with my
> patch series.

Right.  I'll re-roll when we've finished with the rest.

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

[Felipe Contreras]

On Thu, May 2, 2013 at 12:30 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>>> +test_expect_success 'switch to old-branch' '
>>> +       git checkout old-branch
>>> +'
>>> +
>>> +check HEAD refs/heads/old-branch
>>> +check "HEAD@{1}" commit new-two
>>> +check "@{1}" commit old-one
>>
>> And here we are testing something completely different.
>
> How?  I'm showing that HEAD@{1} can be different from @{1}.

Never mind, I thought you were testing detached HEAD checkouts.

-- 
Felipe Contreras

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

[Ramkumar Ramachandra]

Felipe Contreras wrote:
>> +# Document differences between @{N} and HEAD@{N}.
>
> That's enough explanation, except that we not documenting them, we are
> testing them.

The difference is subtle, and I couldn't find this documented clearly
anywhere else.  If someone comes asking what the difference is, just
point to this comment along with the tests.