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

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

[Ramkumar Ramachandra]

Hi,

Felipe's approach to solving the problem is a recursive reinterpret()
that relies on parsing the sequence @[^{].  Seems like quite a
contrived hack, and I think we can do much better than this.

Here, I present my approach to solving the problem.  It interprets @
just like a ref in resolve_ref_unsafe(), after everything else has
been peeled off; the solution is just the few simple lines shown in
[5/5].

The core of the series tackles refactoring the @-parsing so that [5/5]
becomes possible without doing anything contrived.  [1/5] introduces
the problem I started solving: making symbolic refs work properly.
[2/5] and [3/5] are a result of me reading through the horrible
@-parsing code.  [4/5] finally solves the symbolic ref problem, and
[5/5] becomes trivial.

A side-effect of the series is that you can now do:

    $ git symbolic-ref H HEAD
    $ git show H@{u}

In other words, symbolic refs actually work now and you can use them
to achieve a lot of custom overrides.  I think it is a step in the
right direction.

Thanks for listening, and hope you enjoy reading the series as much as
I enjoyed writing it.

Ramkumar Ramachandra (5):
  t1508 (at-combinations): more tests; document failures
  sha1_name.c: don't waste cycles in the @-parsing loop
  sha1_name.c: simplify @-parsing in get_sha1_basic()
  remote.c: teach branch_get() to treat symrefs other than HEAD
  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                               | 14 ++++++
 sha1_name.c                            | 85 +++++++++++++++++++++++++---------
 t/t1400-update-ref.sh                  |  3 ++
 t/t1508-at-combinations.sh             | 15 ++++++
 7 files changed, 113 insertions(+), 26 deletions(-)

-- 
1.8.3.rc0.24.g6456091

[PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

To emphasize what we're testing in @{1}@{u}, document that @{0}@{0} is
also nonsense.  This makes it clear that @{<n>} does not resolve to a
ref whose upstream we can determine with @{u}/ reflog we can dig with
@{0}.

Since HEAD is implicit in @{}, check that HEAD@{} works fine for these
cases.  It doesn't make sense in @{-<n>} because @{-<n>} cannot be
used with anything other than HEAD, and interpret_nth_prior_checkout()
hard-codes "HEAD".

Additionally, document bugs in the @-parser by symbolic-ref'ing @ to
HEAD and re-running some tests.

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 d5d6244..efa2a2a 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -39,13 +39,27 @@ test_expect_success 'setup' '
 
 check HEAD new-two
 check "@{1}" new-one
+check "HEAD@{1}" new-one
+check "@{now}" new-two
+check "HEAD@{now}" new-two
 check "@{-1}" old-two
 check "@{-1}@{1}" old-one
 check "@{u}" upstream-two
+check "HEAD@{u}" upstream-two
 check "@{u}@{1}" upstream-one
 check "@{-1}@{u}" master-two
 check "@{-1}@{u}@{1}" master-one
 nonsense "@{u}@{-1}"
+nonsense "@{0}@{0}"
 nonsense "@{1}@{u}"
+nonsense "HEAD@{-1}"
+
+# Make sure that the @-parser isn't buggy; check things with
+# symbolic-ref @ pointing to HEAD.
+git symbolic-ref @ HEAD
+check "@@{1}" new-one
+check "@@{now}" new-two
+check "@@{u}" upstream-two failure
+nonsense "@@{-1}"
 
 test_done
-- 
1.8.3.rc0.24.g6456091

[PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop

[Ramkumar Ramachandra]

The @-parsing loop unnecessarily checks for the sequence "@{" from
len - 2 unnecessarily.  We can safely check from len - 4: write out a
comment justifying this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..be1d12c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	/* basic@{time or number or -number} format to query ref-log */
 	reflog_len = at = 0;
 	if (len && str[len-1] == '}') {
-		for (at = len-2; at >= 0; at--) {
+		/* str = @}
+		 *       ^
+		 *       len - 2; expression is senseless
+		 *
+		 * str = @{}
+		 *       ^
+		 *       len - 3; expression is still senseless
+		 *
+		 * str = @{.}
+		 *       ^
+		 *       len - 4 where . is any character; expression
+		 *       is worth investigating
+		 *
+		 * Therefore, if str ends with }, search three
+		 * characters earlier for @{
+		 */
+		for (at = len - 4; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
 				if (!upstream_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
-- 
1.8.3.rc0.24.g6456091

[PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Presently, the logic for @-parsing is horribly convoluted.  Prove that
it is very straightforward by laying out the three cases (@{N},
@{u[upstream], and @{-N}) and explaining how each case should be
handled in comments.  All tests pass, and no functional changes have
been introduced.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 65 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 20 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index be1d12c..f30e344 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -463,9 +463,26 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		 */
 		for (at = len - 4; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
+				/* Set reflog_len only if we
+				 * don't see a @{u[pstream]}.  The
+				 * @{N} and @{-N} forms have to do
+				 * with reflog digging.
+				 */
+
+				/* Setting len to at means that we are
+				 * only going to process the part
+				 * before the @ until we reach "if
+				 * (reflog)" at the end of the
+				 * function.  That is only applicable
+				 * in the @{N} case; in the @{-N} and
+				 * @{u[pstream]} cases, we will run it
+				 * through interpret_branch_name().
+				 */
+
 				if (!upstream_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
-					len = at;
+					if (str[at + 2] != '-')
+						len = at;
 				}
 				break;
 			}
@@ -476,22 +493,34 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && ambiguous_path(str, len))
 		return -1;
 
-	if (!len && reflog_len) {
-		struct strbuf buf = STRBUF_INIT;
-		int ret;
-		/* try the @{-N} syntax for n-th checkout */
-		ret = interpret_branch_name(str+at, &buf);
-		if (ret > 0) {
-			/* substitute this branch name and restart */
-			return get_sha1_1(buf.buf, buf.len, sha1, 0);
-		} else if (ret == 0) {
-			return -1;
+	if (reflog_len) {
+		if (!len)
+			/* In the @{N} case where there's nothing
+			 * before the @.
+			 */
+			refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
+		else {
+			/* The @{N} case where there is something
+			 * before the @ for dwim_log to figure out,
+			 * and the @{-N} case.
+			 */
+			refs_found = dwim_log(str, len, sha1, &real_ref);
+
+			if (!refs_found && str[2] == '-') {
+				/* The @{-N} case that resolves to a
+				 * detached HEAD (ie. not a ref)
+				 */
+				struct strbuf buf = STRBUF_INIT;
+				if (interpret_branch_name(str, &buf) > 0) {
+					get_sha1_hex(buf.buf, sha1);
+					refs_found = 1;
+					reflog_len = 0;
+				}
+				strbuf_release(&buf);
+			}
 		}
-		/* allow "@{...}" to mean the current branch reflog */
-		refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
-	} else if (reflog_len)
-		refs_found = dwim_log(str, len, sha1, &real_ref);
-	else
+	} else
+		/* The @{u[pstream]} case */
 		refs_found = dwim_ref(str, len, sha1, &real_ref);
 
 	if (!refs_found)
@@ -506,10 +535,6 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		unsigned long co_time;
 		int co_tz, co_cnt;
 
-		/* a @{-N} placed anywhere except the start is an error */
-		if (str[at+2] == '-')
-			return -1;
-
 		/* Is it asking for N-th entry, or approxidate? */
 		for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
 			char ch = str[at+2+i];
-- 
1.8.3.rc0.24.g6456091

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

[Ramkumar Ramachandra]

Currently, branch_get() either accepts either a branch name, empty
string, or the magic four-letter word "HEAD".  Make it additionally
handle symbolic refs that point to a branch.

Update sha1_name.c:interpret_branch_name() to look for "@{", not '@'
(since '@' is a valid symbolic ref).

These two changes together make the 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>
---
 remote.c                   | 14 ++++++++++++++
 sha1_name.c                |  2 +-
 t/t1508-at-combinations.sh |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 68eb99b..0f44e2e 100644
--- a/remote.c
+++ b/remote.c
@@ -1470,6 +1470,20 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
+
+	if (name && *name && (!ret || !ret->remote_name)) {
+		/* Is this a symref pointing to a valid branch, other
+		 * than HEAD?
+		 */
+		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/sha1_name.c b/sha1_name.c
index f30e344..c4a3a54 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1060,7 +1060,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 efa2a2a..73c457d 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,7 +59,7 @@ nonsense "HEAD@{-1}"
 git symbolic-ref @ HEAD
 check "@@{1}" new-one
 check "@@{now}" new-two
-check "@@{u}" upstream-two failure
+check "@@{u}" upstream-two
 nonsense "@@{-1}"
 
 test_done
-- 
1.8.3.rc0.24.g6456091

[PATCH 5/5] 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             |  7 ++++---
 5 files changed, 25 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..9b2e653 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 special pseudo-ref that refers to 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 73c457d..8f8e32d 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -54,12 +54,13 @@ nonsense "@{0}@{0}"
 nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 
-# Make sure that the @-parser isn't buggy; check things with
-# symbolic-ref @ pointing to HEAD.
-git symbolic-ref @ HEAD
+# Check everything with the pseudo-ref @
 check "@@{1}" new-one
 check "@@{now}" new-two
 check "@@{u}" upstream-two
 nonsense "@@{-1}"
 
+check "@~1" new-one
+check "@^0" new-two
+
 test_done
-- 
1.8.3.rc0.24.g6456091

Re: [PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop

[Felipe Contreras]

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> The @-parsing loop unnecessarily checks for the sequence "@{" from
> len - 2 unnecessarily.  We can safely check from len - 4: write out a
> comment justifying this.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  sha1_name.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 3820f28..be1d12c 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         /* basic@{time or number or -number} format to query ref-log */
>         reflog_len = at = 0;
>         if (len && str[len-1] == '}') {
> -               for (at = len-2; at >= 0; at--) {
> +               /* str = @}
> +                *       ^
> +                *       len - 2; expression is senseless
> +                *
> +                * str = @{}
> +                *       ^
> +                *       len - 3; expression is still senseless
> +                *
> +                * str = @{.}
> +                *       ^
> +                *       len - 4 where . is any character; expression
> +                *       is worth investigating
> +                *
> +                * Therefore, if str ends with }, search three
> +                * characters earlier for @{
> +                */

I think this comment is overkill.

> +               for (at = len - 4; at >= 0; at--) {

The change seems OK to me, but there's no need to explain where you
are starting, and if there's a need:

/* start from where reflogs can start: @{.} */

Does the trick nicely.

>                         if (str[at] == '@' && str[at+1] == '{') {
>                                 if (!upstream_mark(str + at, len - at)) {
>                                         reflog_len = (len-1) - (at+2);
> --

-- 
Felipe Contreras

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Felipe Contreras]

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Presently, the logic for @-parsing is horribly convoluted.  Prove that
> it is very straightforward by laying out the three cases (@{N},
> @{u[upstream], and @{-N}) and explaining how each case should be
> handled in comments.  All tests pass, and no functional changes have
> been introduced.

> @@ -463,9 +463,26 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                  */
>                 for (at = len - 4; at >= 0; at--) {
>                         if (str[at] == '@' && str[at+1] == '{') {
> +                               /* Set reflog_len only if we
> +                                * don't see a @{u[pstream]}.  The
> +                                * @{N} and @{-N} forms have to do
> +                                * with reflog digging.
> +                                */
> +
> +                               /* Setting len to at means that we are
> +                                * only going to process the part
> +                                * before the @ until we reach "if
> +                                * (reflog)" at the end of the
> +                                * function.  That is only applicable
> +                                * in the @{N} case; in the @{-N} and
> +                                * @{u[pstream]} cases, we will run it
> +                                * through interpret_branch_name().
> +                                */
> +

Overkill.

/* set reflog_len when using the form: @{N} */

> @@ -476,22 +493,34 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         if (len && ambiguous_path(str, len))
>                 return -1;
>
> -       if (!len && reflog_len) {
> -               struct strbuf buf = STRBUF_INIT;
> -               int ret;
> -               /* try the @{-N} syntax for n-th checkout */
> -               ret = interpret_branch_name(str+at, &buf);
> -               if (ret > 0) {
> -                       /* substitute this branch name and restart */
> -                       return get_sha1_1(buf.buf, buf.len, sha1, 0);
> -               } else if (ret == 0) {
> -                       return -1;
> +       if (reflog_len) {
> +               if (!len)
> +                       /* In the @{N} case where there's nothing
> +                        * before the @.
> +                        */

I think !len makes it clear.

> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
> +               else {
> +                       /* The @{N} case where there is something
> +                        * before the @ for dwim_log to figure out,
> +                        * and the @{-N} case.
> +                        */

I think 'else' makes it clear.

> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
> +
> +                       if (!refs_found && str[2] == '-') {

You are changing the behavior, there's no need for that in this
reorganization patch.

> +                               /* The @{-N} case that resolves to a
> +                                * detached HEAD (ie. not a ref)
> +                                */

This is not true, it resolves to a ref.

git rev-parse --symbolic-full-name @{-1}

Also, you removed a useful comment:

/* try the @{-N} syntax for n-th checkout */

> +                               struct strbuf buf = STRBUF_INIT;
> +                               if (interpret_branch_name(str, &buf) > 0) {
> +                                       get_sha1_hex(buf.buf, sha1);
> +                                       refs_found = 1;
> +                                       reflog_len = 0;
> +                               }
> +                               strbuf_release(&buf);

I'm pretty sure this is doing something totally different now. This is
certainly not "no functional changes".

> +                       }
>                 }
> -               /* allow "@{...}" to mean the current branch reflog */
> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> -       } else if (reflog_len)
> -               refs_found = dwim_log(str, len, sha1, &real_ref);
> -       else
> +       } else
> +               /* The @{u[pstream]} case */

It's not true, there might not be any @{u} in there.

Some of these changes might be good, but I would do them truly without
introducing functional changes, without removing useful comments, and
without adding paragraphs of explanation for what you are doing.

With the principle of self-documenting code, if you need paragraphs to
explain what you are doing, you already lost.

Cheers.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Currently, branch_get() either accepts either a branch name, empty
> string, or the magic four-letter word "HEAD".  Make it additionally
> handle symbolic refs that point to a branch.
>
> Update sha1_name.c:interpret_branch_name() to look for "@{", not '@'
> (since '@' is a valid symbolic ref).
>
> These two changes together make the 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>
> ---
>  remote.c                   | 14 ++++++++++++++
>  sha1_name.c                |  2 +-
>  t/t1508-at-combinations.sh |  2 +-
>  3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 68eb99b..0f44e2e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1470,6 +1470,20 @@ struct branch *branch_get(const char *name)
>                 ret = current_branch;
>         else
>                 ret = make_branch(name, 0);
> +
> +       if (name && *name && (!ret || !ret->remote_name)) {
> +               /* Is this a symref pointing to a valid branch, other
> +                * than HEAD?
> +                */
> +               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);
> +       }

But why? I'm not familiar with branch_get, but my intuition tells me
you are changing the behavior, and now branch_get() is doing something
it wasn't intended to do. And for what?

Your rationale is that it fixes the test cases below, but that's not
reason enough, since there are other ways to fix them, as my patch
series shows.

>         if (ret && ret->remote_name) {
>                 ret->remote = remote_get(ret->remote_name);
>                 if (ret->merge_nr) {
> diff --git a/sha1_name.c b/sha1_name.c
> index f30e344..c4a3a54 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1060,7 +1060,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>                 return ret - used + len;
>         }
>
> -       cp = strchr(name, '@');
> +       cp = strstr(name, "@{");

This might make sense, but it feels totally sneaked in.

>         if (!cp)
>                 return -1;
>         tmp_len = upstream_mark(cp, namelen - (cp - name));

I think these are two patches should be introduced separately, and
with a reason for them to exist independent of each other.

Cheers.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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             |  7 ++++---
>  5 files changed, 25 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..9b2e653 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 special pseudo-ref that refers to HEAD.

Does the user really cares if it's a pseudo-ref or not? Also, what
does it mean that "refers" to HEAD?

> An '@' followed
> +    by '\{' has no relationship to this and means something entirely
> +    different (see below).

If the user cares about that, the user can see that below, otherwise
there's no point in mentioning that. Just like there's no point in
mentioning that @{-N} means something totally different from @{N},
because the user can see that. If it didn't mean something different,
this bullet point wouldn't exist.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
>> +
>> +                       if (!refs_found && str[2] == '-') {
>
> You are changing the behavior, there's no need for that in this
> reorganization patch.

This is not a reorganization patch.  I said "simplify": not refactor.

>> +                               /* The @{-N} case that resolves to a
>> +                                * detached HEAD (ie. not a ref)
>> +                                */
>
> This is not true, it resolves to a ref.
>
> git rev-parse --symbolic-full-name @{-1}

> git co @~1; git co -; git rev-parse --symbolic-full-name @{-1}

If it did resolve to a ref, dwim_log() would have found it.  The
constraint guarding this `if (!refs_found && str[2] == '-')` wouldn't
have been satisfied, and we wouldn't be here.

> Also, you removed a useful comment:
>
> /* try the @{-N} syntax for n-th checkout */

I've changed the entire logic and written expensive comments; and I'm
not allowed to remove one comment which I didn't find helpful?

>> +                               struct strbuf buf = STRBUF_INIT;
>> +                               if (interpret_branch_name(str, &buf) > 0) {
>> +                                       get_sha1_hex(buf.buf, sha1);
>> +                                       refs_found = 1;
>> +                                       reflog_len = 0;
>> +                               }
>> +                               strbuf_release(&buf);
>
> I'm pretty sure this is doing something totally different now. This is
> certainly not "no functional changes".

I'm claiming that there is no functional change at the program level,
with the commenting*.  If you want to disprove my claim, you have to
write a test that breaks after this patch.

* And even if there is, it's probably an accidental corner case with
the wrong behavior.  The new logic is a straightforward implementation
of the rules.

>> +                       }
>>                 }
>> -               /* allow "@{...}" to mean the current branch reflog */
>> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>> -       } else if (reflog_len)
>> -               refs_found = dwim_log(str, len, sha1, &real_ref);
>> -       else
>> +       } else
>> +               /* The @{u[pstream]} case */
>
> It's not true, there might not be any @{u} in there.

This entire structure is a success-filter.  At the end of this, if
!refs_found, then there has been a failure.

> Some of these changes might be good, but I would do them truly without
> introducing functional changes, without removing useful comments, and
> without adding paragraphs of explanation for what you are doing.

The functional changes part is for you to prove.  And it's not even
worth proving, because I'm claiming that the new logic is an obviously
correct implementation of the rules.

> With the principle of self-documenting code, if you need paragraphs to
> explain what you are doing, you already lost.

The Git project is already suffering from a severe shortage of
comments [1], and you're complaining about too many comments?

Have you tried to read and understand the old version?  Some shining
example of self-documenting code you've brought up.

[1]: https://www.ohloh.net/p/git/factoids#FactoidCommentsLow

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> But why? I'm not familiar with branch_get, but my intuition tells me
> you are changing the behavior, and now branch_get() is doing something
> it wasn't intended to do. And for what?

Why is there a commit message?  I've explained what the behavior change is.

> Your rationale is that it fixes the test cases below, but that's not
> reason enough, since there are other ways to fix them, as my patch
> series shows.

For what exactly.  To fix a real bug: H@{u} and @@{u} don't work where
either H or @ are symbolic refs.  I want custom symbolic refs, because
they are useful.  In other words, "HEAD" is not a sacred symbolic ref.

>>         if (ret && ret->remote_name) {
>>                 ret->remote = remote_get(ret->remote_name);
>>                 if (ret->merge_nr) {
>> diff --git a/sha1_name.c b/sha1_name.c
>> index f30e344..c4a3a54 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -1060,7 +1060,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>>                 return ret - used + len;
>>         }
>>
>> -       cp = strchr(name, '@');
>> +       cp = strstr(name, "@{");
>
> This might make sense, but it feels totally sneaked in.

Sneaked in?  I have three paragraphs in my commit message.  The first
two explain two related changes, and the third one shows how they are
related.

>>         if (!cp)
>>                 return -1;
>>         tmp_len = upstream_mark(cp, namelen - (cp - name));
>
> I think these are two patches should be introduced separately, and
> with a reason for them to exist independent of each other.

I cannot justify the remote.c patch without the "@{" change.

Re: [PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> I think this comment is overkill.

That's not for one line.  It's for the whole logic following it: there
are things like (len-1) - (at+2) which are easy to visualize with this
picture.  They're int, not char *.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> To emphasize what we're testing in @{1}@{u}, document that @{0}@{0} is
> also nonsense.  This makes it clear that @{<n>} does not resolve to a
> ref whose upstream we can determine with @{u}/ reflog we can dig with
> @{0}.
>
> Since HEAD is implicit in @{},...

Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
totally different things.  @{0} and HEAD@{0} are almost always the
same, and @{1} and HEAD@{1} may often happen to be the same, but as
a blanket statement, I find "Since HEAD is implicit in @{}" very
misleading.

As you and Felipe seem to be aiming for the same "Let's allow users
to say '@' when they mean HEAD", I'll let you two figure the best
approach out.

One productive way forward might be to come up with a common test
script pieces to document what constructs that spell @ in place of
HEAD should be supported, and much more importantly, what constructs
that happen to have @ in them should not mistakenly trigger the new
machinery.

Have fun ;-)

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> [1]: https://www.ohloh.net/p/git/factoids#FactoidCommentsLow

Since this has been coming up from time to time:

I have nothing against including helpful comments where appropriate.
But one aspect which that factoid misses is that git has some very
detailed, very dense documentation available in its commit log.  Tools
like "git gui blame" and "git log -S" can show detailed historical
information about the purpose of every line of code.  A nice feature
of such documentation is that it is in a context where it cannot fall
out of date.

So for example I can do

	$ git log -S'if (len && ambiguous_path(str, len))' -- sha1_name.c
	commit 11cf8801
	Author: Nicolas Pitre <nico@cam.org>
	Date:   Thu Feb 1 17:29:33 2007 -0500

	    provide a nice @{...} syntax to always mean the current branch reflog

	    This is shorter than HEAD@{...} and being nameless it has no semantic
	    issues.

	    Signed-off-by: Nicolas Pitre <nico@cam.org>
	    Signed-off-by: Junio C Hamano <junkio@cox.net>

and then "git show 11cf8801" will show me exactly what change prompted
that "len" test.

The same is true of the Linux kernel, too.

Hope that helps,
Jonathan

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> Does the user really cares if it's a pseudo-ref or not? Also, what
> does it mean that "refers" to HEAD?

It's not about whether the user cares or not; it's about saying it in
a way that doesn't make it less precise.  I'm saying "@ is like a
symbolic-ref .git/@ ref referring to HEAD, except it doesn't sit on
the filesystem".

It's not important that end users understand it fully.  It's only
really useful in two cases: @~ and @^, and I've provided examples
there.

>> An '@' followed
>> +    by '\{' has no relationship to this and means something entirely
>> +    different (see below).
>
> If the user cares about that, the user can see that below, otherwise
> there's no point in mentioning that.

A user seeing @{} might vaguely recall @ and scroll-back here.  In
which case, this is useful.

> Just like there's no point in
> mentioning that @{-N} means something totally different from @{N},
> because the user can see that. If it didn't mean something different,
> this bullet point wouldn't exist.

Those two are right next to each other.

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Felipe Contreras]

On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
>> <artagnon@gmail.com> wrote:
>>> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
>>> +
>>> +                       if (!refs_found && str[2] == '-') {
>>
>> You are changing the behavior, there's no need for that in this
>> reorganization patch.
>
> This is not a reorganization patch.  I said "simplify": not refactor.

I'd say you should start with a reorganization, and then a simplification.

>>> +                               /* The @{-N} case that resolves to a
>>> +                                * detached HEAD (ie. not a ref)
>>> +                                */
>>
>> This is not true, it resolves to a ref.
>>
>> git rev-parse --symbolic-full-name @{-1}
>
>> git co @~1; git co -; git rev-parse --symbolic-full-name @{-1}
>
> If it did resolve to a ref, dwim_log() would have found it.  The
> constraint guarding this `if (!refs_found && str[2] == '-')` wouldn't
> have been satisfied, and we wouldn't be here.

I see.

>> Also, you removed a useful comment:
>>
>> /* try the @{-N} syntax for n-th checkout */
>
> I've changed the entire logic and written expensive comments; and I'm
> not allowed to remove one comment which I didn't find helpful?

But it is helpful.

>>> +                               struct strbuf buf = STRBUF_INIT;
>>> +                               if (interpret_branch_name(str, &buf) > 0) {
>>> +                                       get_sha1_hex(buf.buf, sha1);
>>> +                                       refs_found = 1;
>>> +                                       reflog_len = 0;
>>> +                               }
>>> +                               strbuf_release(&buf);
>>
>> I'm pretty sure this is doing something totally different now. This is
>> certainly not "no functional changes".
>
> I'm claiming that there is no functional change at the program level,
> with the commenting*.  If you want to disprove my claim, you have to
> write a test that breaks after this patch.

The burden of proof resides in the one that makes the claim, I don't
need to prove that there are functional changes, merely that you
haven't met your burden of proof.

That being said, perhaps there are no _behavioral_ changes, because
you are relegating some of the current functionality to dwim_log, but
the code most certainly is doing something completely different.
Before, get_sha1_basic recursively called itself when @{-N} resolved
to a branch name, now, you rely on dwim_log to do this for you without
recursion, which is nice, but functionally different.

>>> +                       }
>>>                 }
>>> -               /* allow "@{...}" to mean the current branch reflog */
>>> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>>> -       } else if (reflog_len)
>>> -               refs_found = dwim_log(str, len, sha1, &real_ref);
>>> -       else
>>> +       } else
>>> +               /* The @{u[pstream]} case */
>>
>> It's not true, there might not be any @{u} in there.
>
> This entire structure is a success-filter.  At the end of this, if
> !refs_found, then there has been a failure.

That's irrelevant, this 'else' case is for !reflog_len, there might or
might not be @{u} in there.

>> With the principle of self-documenting code, if you need paragraphs to
>> explain what you are doing, you already lost.
>
> The Git project is already suffering from a severe shortage of
> comments [1], and you're complaining about too many comments?

Irrelevant conclusion fallacy; let's suppose that the Git project is
indeed suffering from a shortage of comments, that doesn't imply that
*these* comments in their present form are any good.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Wed, May 1, 2013 at 1:44 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> But why? I'm not familiar with branch_get, but my intuition tells me
>> you are changing the behavior, and now branch_get() is doing something
>> it wasn't intended to do. And for what?
>
> Why is there a commit message?  I've explained what the behavior change is.

Not good enough.

>> Your rationale is that it fixes the test cases below, but that's not
>> reason enough, since there are other ways to fix them, as my patch
>> series shows.
>
> For what exactly.  To fix a real bug: H@{u} and @@{u} don't work where
> either H or @ are symbolic refs.  I want custom symbolic refs, because
> they are useful.  In other words, "HEAD" is not a sacred symbolic ref.

As I said, the @@{u} thing can be fixed through other ways.

Moreover, "HEAD" is still a special case in remote.c::branch_get()
that you just modified.

>> I think these are two patches should be introduced separately, and
>> with a reason for them to exist independent of each other.
>
> I cannot justify the remote.c patch without the "@{" change.

That's what I thought.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Wed, May 1, 2013 at 2:00 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> Does the user really cares if it's a pseudo-ref or not? Also, what
>> does it mean that "refers" to HEAD?
>
> It's not about whether the user cares or not; it's about saying it in
> a way that doesn't make it less precise.  I'm saying "@ is like a
> symbolic-ref .git/@ ref referring to HEAD, except it doesn't sit on
> the filesystem".

If I put my user shoes, I don't care how @ is implemented, I just care
that it's a shortcut for HEAD, that's what it means to me, the common
user.

-- 
Felipe Contreras

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> This is not a reorganization patch.  I said "simplify": not refactor.
>
> I'd say you should start with a reorganization, and then a simplification.

You don't think I already tried that?  There is no way to sensibly
reorganize the old logic sensibly, in a way that doesn't break
anything.

>> I've changed the entire logic and written expensive comments; and I'm
>> not allowed to remove one comment which I didn't find helpful?
>
> But it is helpful.

Okay, we'll add it back if you feel strongly about it.  I thought it
would be a sore thumb when neither @{N} nor @{upstream} are explained.

>> I'm claiming that there is no functional change at the program level,
>> with the commenting*.  If you want to disprove my claim, you have to
>> write a test that breaks after this patch.
>
> The burden of proof resides in the one that makes the claim, I don't
> need to prove that there are functional changes, merely that you
> haven't met your burden of proof.

Oh, but I have.  All the tests (along with the new ones I added in [1/5]) pass.

Scientific theories stand until you can find one observation that disproves it.

> That being said, perhaps there are no _behavioral_ changes, because
> you are relegating some of the current functionality to dwim_log, but
> the code most certainly is doing something completely different.
> Before, get_sha1_basic recursively called itself when @{-N} resolved
> to a branch name, now, you rely on dwim_log to do this for you without
> recursion, which is nice, but functionally different.

Your point being?  That I shouldn't say "no functional changes"
because the logic is changing non-trivially at this level?

>>> It's not true, there might not be any @{u} in there.
>>
>> This entire structure is a success-filter.  At the end of this, if
>> !refs_found, then there has been a failure.
>
> That's irrelevant, this 'else' case is for !reflog_len, there might or
> might not be @{u} in there.

There's no need to associate one comment with one line of code.
People can see clearly see the failure case following it.

>> The Git project is already suffering from a severe shortage of
>> comments [1], and you're complaining about too many comments?
>
> Irrelevant conclusion fallacy; let's suppose that the Git project is
> indeed suffering from a shortage of comments, that doesn't imply that
> *these* comments in their present form are any good.

Is there anything _wrong_ with the comments, apart from the fact that
they seem to be too big?  I'm saying things that I cannot say in the
commit message.

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> As I said, the @@{u} thing can be fixed through other ways.

It's not just @@{u}.  I can have lots of custom symbolic refs working
properly; I might choose D for my deleted-reflogs, M for master and so
on.

    $ git log M..

The point is that my solution solves the problem in the more general case.

> Moreover, "HEAD" is still a special case in remote.c::branch_get()
> that you just modified.

It's not.  It's just a quick shortcut for the common case because
read_config() already sets it up.

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> If I put my user shoes, I don't care how @ is implemented, I just care
> that it's a shortcut for HEAD, that's what it means to me, the common
> user.

Okay, we'll change this.

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Since this has been coming up from time to time:
> [...]

Thanks, I didn't know about 'git gui blame'.

I think both comments and commit messages have their uses.  One cannot
do the job of the other.

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

[Felipe Contreras]

On Wed, May 1, 2013 at 2:50 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> As I said, the @@{u} thing can be fixed through other ways.
>
> It's not just @@{u}.  I can have lots of custom symbolic refs working
> properly; I might choose D for my deleted-reflogs, M for master and so
> on.

And that doesn't require the changes you did to sha1_name.c, or
t/t1508-at-combinations.sh, does it?

If you are arguing in favor of arbitrary symbolic refs plus @{u} to
work, a patch that allows that, and nothing else, should make sense.
Such patch would trigger further discussion, for example, if
get_branch() is the right place to achieve that.

> The point is that my solution solves the problem in the more general case.
>
>> Moreover, "HEAD" is still a special case in remote.c::branch_get()
>> that you just modified.
>
> It's not.  It's just a quick shortcut for the common case because
> read_config() already sets it up.

I still see this in the code:

	if (!name || !*name || !strcmp(name, "HEAD"))
		ret = current_branch;
	else
		ret = make_branch(name, 0);

-- 
Felipe Contreras

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> If you are arguing in favor of arbitrary symbolic refs plus @{u} to
> work, a patch that allows that, and nothing else, should make sense.
> Such patch would trigger further discussion, for example, if
> get_branch() is the right place to achieve that.

What kind of absurd argument is this?  I am trying to solve the @ ->
HEAD problem correctly.  In the process, I fixed general symbolic-ref
handling and cleaned up the @-parsing logic.  Does anyone have a
problem with the fix, or am I missing something?

So, start the discussion.  What are you waiting for?

> I still see this in the code:
>
>         if (!name || !*name || !strcmp(name, "HEAD"))

Remove the !strcmp(name, "HEAD") and tell me what you see.  "HEAD"
will get resolved just like any other symbolic ref, via the hunk I
added.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
> totally different things.  @{0} and HEAD@{0} are almost always the
> same, and @{1} and HEAD@{1} may often happen to be the same, but as
> a blanket statement, I find "Since HEAD is implicit in @{}" very
> misleading.

When will they be different?  I'm looking at this from the parser's
point of view: when the part before @{} is missing, we dwim a "HEAD".

> As you and Felipe seem to be aiming for the same "Let's allow users
> to say '@' when they mean HEAD", I'll let you two figure the best
> approach out.

I've solved the problem in the general case of symbolic-refs and made
"@" a special case of that.

> One productive way forward might be to come up with a common test
> script pieces to document what constructs that spell @ in place of
> HEAD should be supported,

It's sufficient to test that symbolic refs work properly.  @ is a
trivial implementation of a pseudo symbolic-ref (see [5/5]).

> and much more importantly, what constructs
> that happen to have @ in them should not mistakenly trigger the new
> machinery.

At the parsing level, @ can only ever interfere with @{}; I've added
tests for those.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Jeff King]

On Thu, May 02, 2013 at 02:34:01AM +0530, Ramkumar Ramachandra wrote:

> Junio C Hamano wrote:
> > Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
> > totally different things.  @{0} and HEAD@{0} are almost always the
> > same, and @{1} and HEAD@{1} may often happen to be the same, but as
> > a blanket statement, I find "Since HEAD is implicit in @{}" very
> > misleading.
> 
> When will they be different?  I'm looking at this from the parser's
> point of view: when the part before @{} is missing, we dwim a "HEAD".

The difference is that HEAD@{} refers to HEAD's reflog, but @{} refers
to the reflog of the branch pointed to by HEAD. For example, try:

  git init repo && cd repo
  git commit --allow-empty -m one &&
  git commit --allow-empty -m two &&
  git checkout HEAD^ &&
  git commit --allow-empty -m three &&
  git checkout master &&
  for i in 0 1 2; do
    echo "HEAD@{$i}: $(git log -1 --oneline HEAD@{$i} 2>&1)"
    echo "    @{$i}: $(git log -1 --oneline     @{$i} 2>&1)"
  done

which produces:

  HEAD@{0}: 1fbb097 two
      @{0}: 1fbb097 two
  HEAD@{1}: 42f3f4d three
      @{1}: 1089d0e one
  HEAD@{2}: 1089d0e one
      @{2}: fatal: Log for '' only has 2 entries.

Unless your reflogs are screwed up, the 0th reflog should always point
to the same commit (since you just moved HEAD there), but beyond that
there is not necessarily any relation. And even for the 0th reflog
entry, the reflog information is not the same. Try this on the repo
above:

  echo "HEAD@{0}: $(git log -g -1 --format='%gd %gs' HEAD@{0})"
  echo "    @{0}: $(git log -g -1 --format='%gd %gs'     @{0})"

which yields:

  HEAD@{0}: HEAD@{0} checkout: moving from ... to master
      @{0}: master@{0} commit: two

-Peff

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

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra (5):
>   t1508 (at-combinations): more tests; document failures
>   sha1_name.c: don't waste cycles in the @-parsing loop
>   sha1_name.c: simplify @-parsing in get_sha1_basic()
>   remote.c: teach branch_get() to treat symrefs other than HEAD
>   refs.c: make @ a pseudo-ref alias to HEAD

For your convenience, pull https://github.com/artagnon/git/tree/implicit-head

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Jeff King wrote:
> The difference is that HEAD@{} refers to HEAD's reflog, but @{} refers
> to the reflog of the branch pointed to by HEAD.

Ah, I missed this.  Thanks for the testcase.  My patch changes this
behavior, and I'm looking into the problem.

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> +               if (!len)
> +                       /* In the @{N} case where there's nothing
> +                        * before the @.
> +                        */
> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);


Minor mistake here: it should be dwim_ref, not dwim_log.  Thanks to
Junio/ Jeff for poking.

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Felipe Contreras]

On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>>> This is not a reorganization patch.  I said "simplify": not refactor.
>>
>> I'd say you should start with a reorganization, and then a simplification.
>
> You don't think I already tried that?  There is no way to sensibly
> reorganize the old logic sensibly, in a way that doesn't break
> anything.

There's always a way.

See, I tried to split your patch into logical changes, so I started with this:

--- a/sha1_name.c
+++ b/sha1_name.c
@@ -460,23 +460,26 @@ static int get_sha1_basic(const char *str, int
len, unsigned char *sha1)
        if (len && ambiguous_path(str, len))
                return -1;

-       if (!len && reflog_len) {
-               struct strbuf buf = STRBUF_INIT;
-               int ret;
-               /* try the @{-N} syntax for n-th checkout */
-               ret = interpret_branch_name(str+at, &buf);
-               if (ret > 0) {
-                       /* substitute this branch name and restart */
-                       return get_sha1_1(buf.buf, buf.len, sha1, 0);
-               } else if (ret == 0) {
-                       return -1;
+       if (reflog_len) {
+               if (!len) {
+                       struct strbuf buf = STRBUF_INIT;
+                       int ret;
+                       /* try the @{-N} syntax for n-th checkout */
+                       ret = interpret_branch_name(str+at, &buf);
+                       if (ret > 0) {
+                               /* substitute this branch name and restart */
+                               return get_sha1_1(buf.buf, buf.len, sha1, 0);
+                       } else if (ret == 0) {
+                               return -1;
+                       }
+                       /* allow "@{...}" to mean the current branch reflog */
+                       refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
+               } else {
+                       refs_found = dwim_log(str, len, sha1, &real_ref);
                }
-               /* allow "@{...}" to mean the current branch reflog */
-               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
-       } else if (reflog_len)
-               refs_found = dwim_log(str, len, sha1, &real_ref);
-       else
+       } else {
                refs_found = dwim_ref(str, len, sha1, &real_ref);
+       }

        if (!refs_found)
                return -1;

Truly no functional changes at this point, but then this:

--- a/sha1_name.c
+++ b/sha1_name.c
@@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
len, unsigned char *sha1)
                                return -1;
                        }
                        /* allow "@{...}" to mean the current branch reflog */
-                       refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
+                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
                } else {
                        refs_found = dwim_log(str, len, sha1, &real_ref);
                }

Bam! Now we have a functional change. @{1} is different from HEAD@{1},
but this change makes them the same. Not only is this a functional
change, it's a behavioral change.

Of course, this would be easy to see if you had bothered to split your
patch into logical changes, but you didn't, so the change is lost in a
mess. This is why it's not recommended to do that.

>>> I'm claiming that there is no functional change at the program level,
>>> with the commenting*.  If you want to disprove my claim, you have to
>>> write a test that breaks after this patch.
>>
>> The burden of proof resides in the one that makes the claim, I don't
>> need to prove that there are functional changes, merely that you
>> haven't met your burden of proof.
>
> Oh, but I have.  All the tests (along with the new ones I added in [1/5]) pass.

That is only proof that those tests pass.

> Scientific theories stand until you can find one observation that disproves it.

Yeah, I would like to see a scientist claiming "There are exactly
three multiverses. You don't think so? Prove me wrong. Na na na na!".
Not going to happen.

You are the one making a claim, you are the one that has the burden of
proof, and you haven't met it.

And even though I don't have to prove your claim is false, I already
did; @{1} is different now. If you want more, see below.

>> That being said, perhaps there are no _behavioral_ changes, because
>> you are relegating some of the current functionality to dwim_log, but
>> the code most certainly is doing something completely different.
>> Before, get_sha1_basic recursively called itself when @{-N} resolved
>> to a branch name, now, you rely on dwim_log to do this for you without
>> recursion, which is nice, but functionally different.
>
> Your point being?  That I shouldn't say "no functional changes"
> because the logic is changing non-trivially at this level?

Exactly. You shouldn't say there are no functional changes, if, in
fact, there are functional changes.

>>>> It's not true, there might not be any @{u} in there.
>>>
>>> This entire structure is a success-filter.  At the end of this, if
>>> !refs_found, then there has been a failure.
>>
>> That's irrelevant, this 'else' case is for !reflog_len, there might or
>> might not be @{u} in there.
>
> There's no need to associate one comment with one line of code.
> People can see clearly see the failure case following it.

Is that the way you defend your comments? People can see that the
comment is wrong?

What is the point of the comment then? People can see the context, so
there's no need for a comment, specially one that is wrong.

>>> The Git project is already suffering from a severe shortage of
>>> comments [1], and you're complaining about too many comments?
>>
>> Irrelevant conclusion fallacy; let's suppose that the Git project is
>> indeed suffering from a shortage of comments, that doesn't imply that
>> *these* comments in their present form are any good.
>
> Is there anything _wrong_ with the comments, apart from the fact that
> they seem to be too big?  I'm saying things that I cannot say in the
> commit message.

I just pointed to one comment being wrong. Other than that, yeah, they
are unnecessary.

Aside from the fact that there are wrong and unnecessary comments,
unmentioned functionality changes, there are behavioral changes:

1) @{1} has changed

2) @{-1}@{-1} now doesn't return an error

3) @{-1}{0} returns an invalid object

I wrote a test to show these changes:

*** t1513-at-combinations-strict.sh ***
ok 1 - setup
ok 2 - HEAD = refs/heads/new-branch
ok 3 - @{1} = commit
ok 4 - HEAD@{3} = commit
not ok 5 - @{3} is nonsensical
#	
#		test_must_fail git rev-parse --symbolic '@{3}'
#	
ok 6 - @{-1} = refs/heads/old-branch
ok 7 - @{-1}@{1} = commit
not ok 8 - @{-1}{0} = commit
#	
#		if [ 'commit' == 'commit' ]; then
#			echo 'old-two' >expect &&
#			git log -1 --format=%s '@{-1}{0}' >actual
#		else
#			echo 'commit' >expect &&
#			git rev-parse --symbolic-full-name '@{-1}{0}' >actual
#		fi &&
#		test_cmp expect actual
#	
ok 9 - @{u} = refs/heads/upstream-branch
ok 10 - @{u}@{1} = commit
ok 11 - @{-1}@{u} = refs/heads/master
ok 12 - @{-1}@{u}@{1} = commit
ok 13 - @{u}@{-1} is nonsensical
ok 14 - @{1}@{u} is nonsensical
not ok 15 - @{-1}@{-1} is nonsensical
#	
#		test_must_fail git rev-parse --symbolic '@{-1}@{-1}'
#	
# failed 3 among 15 test(s)
1..15

They all pass without your patch. So yeah, there's a reason you were
not able to show fulfill your burden of proof; your claim wasn't true.

Here's the test:

#!/bin/sh

test_description='test various @{X} syntax combinations together'
. ./test-lib.sh

check() {
test_expect_${4:-success} "$1 = $2" "
	if [ '$2' == 'commit' ]; then
		echo '$3' >expect &&
		git log -1 --format=%s '$1' >actual
	else
		echo '$2' >expect &&
		git rev-parse --symbolic-full-name '$1' >actual
	fi &&
	test_cmp expect actual
"
}

nonsense() {
test_expect_${2:-success} "$1 is nonsensical" "
	test_must_fail git rev-parse --symbolic '$1'
"
}

test_expect_success 'setup' '
	test_commit master-one &&
	test_commit master-two &&
	git checkout -b upstream-branch &&
	test_commit upstream-one &&
	test_commit upstream-two &&
	git checkout -b old-branch master &&
	test_commit old-one &&
	test_commit old-two &&
	git checkout -b new-branch &&
	test_commit new-one &&
	test_commit new-two &&
	git branch -u master old-branch &&
	git branch -u upstream-branch new-branch
'

check HEAD refs/heads/new-branch
check "@{1}" commit new-one
check "HEAD@{3}" commit old-two
nonsense "@{3}"
check "@{-1}" refs/heads/old-branch
check "@{-1}@{1}" commit old-one
check "@{-1}{0}" commit old-two
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}"
nonsense "@{-1}@{-1}"

test_done

-- 
Felipe Contreras

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

[Felipe Contreras]

On Wed, May 1, 2013 at 3:57 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> If you are arguing in favor of arbitrary symbolic refs plus @{u} to
>> work, a patch that allows that, and nothing else, should make sense.
>> Such patch would trigger further discussion, for example, if
>> get_branch() is the right place to achieve that.
>
> What kind of absurd argument is this?  I am trying to solve the @ ->
> HEAD problem correctly.  In the process, I fixed general symbolic-ref
> handling and cleaned up the @-parsing logic.  Does anyone have a
> problem with the fix, or am I missing something?

This is not a fix, this is multiple changes blobbed into one.

> So, start the discussion.  What are you waiting for?
>
>> I still see this in the code:
>>
>>         if (!name || !*name || !strcmp(name, "HEAD"))
>
> Remove the !strcmp(name, "HEAD") and tell me what you see.  "HEAD"
> will get resolved just like any other symbolic ref, via the hunk I
> added.

If you are so certain why don't you remove that code then? I wouldn't
be so sure.

-- 
Felipe Contreras

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> You don't think I already tried that?  There is no way to sensibly
>> reorganize the old logic sensibly, in a way that doesn't break
>> anything.
>
> See, I tried to split your patch into logical changes, so I started with this:
>
> --- a/sha1_name.c
> +++ b/sha1_name.c

Thanks; I was finding this hard to do.  I'll try to continue from here.

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
> len, unsigned char *sha1)
>                                 return -1;
>                         }
>                         /* allow "@{...}" to mean the current branch reflog */
> -                       refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);

Yeah, I noticed this just a few minutes ago.  We really should have
tests testing @{N} against HEAD@{N}.

> Of course, this would be easy to see if you had bothered to split your
> patch into logical changes, but you didn't, so the change is lost in a
> mess. This is why it's not recommended to do that.

Right.  I'll try to redo this as multiple parts.

>> There's no need to associate one comment with one line of code.
>> People can see clearly see the failure case following it.
>
> Is that the way you defend your comments? People can see that the
> comment is wrong?

In that case, all the comments are wrong.  Even the ones about @{N}
and @{-N}, because we never really check @{\d+} or @{-\d+}.  Would you
like to make the comments more painful to read and write?

> 2) @{-1}@{-1} now doesn't return an error
>
> 3) @{-1}{0} returns an invalid object

Thanks for the tests!  I'll look into the problem.

Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Felipe Contreras]

On Wed, May 1, 2013 at 5:26 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

>>> There's no need to associate one comment with one line of code.
>>> People can see clearly see the failure case following it.
>>
>> Is that the way you defend your comments? People can see that the
>> comment is wrong?
>
> In that case, all the comments are wrong.  Even the ones about @{N}
> and @{-N}, because we never really check @{\d+} or @{-\d+}.  Would you
> like to make the comments more painful to read and write?

If what I see in the code and what I read in the comments tell me
conflicting stories, I'd say the comments are not fulfilling their
purpose. Either add comments that explain what the code is _actually_
doing, or don't bother with them at all.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 02, 2013 at 02:34:01AM +0530, Ramkumar Ramachandra wrote:
>
>> Junio C Hamano wrote:
>> > Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
>> > totally different things.  @{0} and HEAD@{0} are almost always the
>> > same, and @{1} and HEAD@{1} may often happen to be the same, but as
>> > a blanket statement, I find "Since HEAD is implicit in @{}" very
>> > misleading.
>> 
>> When will they be different?  I'm looking at this from the parser's
>> point of view: when the part before @{} is missing, we dwim a "HEAD".
>
> The difference is that HEAD@{} refers to HEAD's reflog, but @{} refers
> to the reflog of the branch pointed to by HEAD. For example, try:
>
>   git init repo && cd repo
>   git commit --allow-empty -m one &&
>   git commit --allow-empty -m two &&
>   git checkout HEAD^ &&
>   git commit --allow-empty -m three &&
>   git checkout master &&
>   for i in 0 1 2; do
>     echo "HEAD@{$i}: $(git log -1 --oneline HEAD@{$i} 2>&1)"
>     echo "    @{$i}: $(git log -1 --oneline     @{$i} 2>&1)"
>   done
>
> which produces:
>
>   HEAD@{0}: 1fbb097 two
>       @{0}: 1fbb097 two
>   HEAD@{1}: 42f3f4d three
>       @{1}: 1089d0e one
>   HEAD@{2}: 1089d0e one
>       @{2}: fatal: Log for '' only has 2 entries.
>
> Unless your reflogs are screwed up, the 0th reflog should always point
> to the same commit (since you just moved HEAD there), but beyond that
> there is not necessarily any relation. And even for the 0th reflog
> entry, the reflog information is not the same. Try this on the repo
> above:
>
>   echo "HEAD@{0}: $(git log -g -1 --format='%gd %gs' HEAD@{0})"
>   echo "    @{0}: $(git log -g -1 --format='%gd %gs'     @{0})"
>
> which yields:
>
>   HEAD@{0}: HEAD@{0} checkout: moving from ... to master
>       @{0}: master@{0} commit: two
>
> -Peff

Thanks for helping with a basic education I found no time for
myself today.

Re: [PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop

[Junio C Hamano]

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

> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> The @-parsing loop unnecessarily checks for the sequence "@{" from
>> len - 2 unnecessarily.  We can safely check from len - 4: write out a
>> comment justifying this.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  sha1_name.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 3820f28..be1d12c 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>         /* basic@{time or number or -number} format to query ref-log */
>>         reflog_len = at = 0;
>>         if (len && str[len-1] == '}') {
>> -               for (at = len-2; at >= 0; at--) {
>> +               /* str = @}
>> +                *       ^
>> +                *       len - 2; expression is senseless
>> +                *
>> +                * str = @{}
>> +                *       ^
>> +                *       len - 3; expression is still senseless
>> +                *
>> +                * str = @{.}
>> +                *       ^
>> +                *       len - 4 where . is any character; expression
>> +                *       is worth investigating
>> +                *
>> +                * Therefore, if str ends with }, search three
>> +                * characters earlier for @{
>> +                */
>
> I think this comment is overkill.
>
>> +               for (at = len - 4; at >= 0; at--) {
>
> The change seems OK to me, but there's no need to explain where you
> are starting, and if there's a need:
>
> /* start from where reflogs can start: @{.} */
>
> Does the trick nicely.

As the fact that nobody noticed nor bothered with the two-byte
optimization opportunity shows that this is trickier than trivial, I
agree with both of you that this change deserves an in-code comment.

    Start checking at len - 4, because there has to be at least one
    byte inside "@{.}" for it to be worth checking.

would be sufficient.  The 16-line comment is way overkill.

Not that I think this change really matters, though.

>>                         if (str[at] == '@' && str[at+1] == '{') {
>>                                 if (!upstream_mark(str + at, len - at)) {
>>                                         reflog_len = (len-1) - (at+2);
>> --

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Felipe Contreras]

On Wed, May 1, 2013 at 1:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

> As you and Felipe seem to be aiming for the same "Let's allow users
> to say '@' when they mean HEAD", I'll let you two figure the best
> approach out.
>
> One productive way forward might be to come up with a common test
> script pieces to document what constructs that spell @ in place of
> HEAD should be supported, and much more importantly, what constructs
> that happen to have @ in them should not mistakenly trigger the new
> machinery.

The difference is basically that my approach is less intrusive; it
only touches code that is relevant for revision parsing. This means
the scope and side-effects are known; the same as @{-N}.

Ramkumar's approach has no precedent, so it's not clear what
side-effects could there be. His approach relies on modifying
branch_get() a function that barely has been touched since 2007, and
has other side-effects, plus there isn't even a proper patch with the
isolated change, and an explanation why it makes sense.

While it might or might not make sense to teach branch_get() to accept
symbolic refs, it's not needed to achieve the desired functionality,
either on my approach, or his.

Moreover, the symbolic-ref 'HEAD' is quite special, it's mentioned
everywhere in the documentation, and the code has special cases for
it. It's not reasonable to expect all relevant places to be updated
for this functionality, and certainly 'Documentation/revisions.txt' is
not the only one.

For example, in Ramkumar's approach:

 % git branch -u master @

Would replace '@' with HEAD, however:

 % git branch --edit-description @

Would not. These inconsistencies are not due to Ramkumar's code, but
why would the user expect '@' to be replaced with anything if 'git
branch' documentation doesn't mention any revision parsing, which is
the only place where the '@' shortcut is documented.

In my opinion, if 'git branch X @{-1}' doesn't work, neither should
'git branch X @', and that's what my approach does.

My approach is isolated to revision parsing, which means we minimize
the potential for surprises and unintended consequences.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> [...]

Yes, I'm working on a re-roll.

> Moreover, the symbolic-ref 'HEAD' is quite special, it's mentioned
> everywhere in the documentation, and the code has special cases for
> it. It's not reasonable to expect all relevant places to be updated
> for this functionality, and certainly 'Documentation/revisions.txt' is
> not the only one.

I'm not denying that HEAD is special.  I'm just improving the general
support for symbolic-refs, so that the difference between HEAD and any
other symbolic-ref is smaller.

> For example, in Ramkumar's approach:
>
>  % git branch -u master @
>
> Would replace '@' with HEAD, however:
>
>  % git branch --edit-description @
>
> Would not.

git branch -u master <any symbolic ref> will work just fine.  It has
nothing to do with my series.

Does git branch --edit-description HEAD work?  Then why do you expect
git branch --edit-description <any other symbolic ref> to work?  git
branch --edit-description operates on non-symbolic refs.

Let me make this clear: @ is just another symbolic-ref that always
points to the same thing as HEAD.  Nothing less, nothing more.

> In my opinion, if 'git branch X @{-1}' doesn't work, neither should
> 'git branch X @', and that's what my approach does.

Why shouldn't (doesn't) git branch X @{-1} work?  git branch X <any
expression with or without a symbolic ref> works fine, and it has
nothing to do with my series.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Felipe Contreras]

On Thu, May 2, 2013 at 4:07 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> [...]
>
> Yes, I'm working on a re-roll.
>
>> Moreover, the symbolic-ref 'HEAD' is quite special, it's mentioned
>> everywhere in the documentation, and the code has special cases for
>> it. It's not reasonable to expect all relevant places to be updated
>> for this functionality, and certainly 'Documentation/revisions.txt' is
>> not the only one.
>
> I'm not denying that HEAD is special.  I'm just improving the general
> support for symbolic-refs, so that the difference between HEAD and any
> other symbolic-ref is smaller.

That is orthogonal to the issue at hand.

>> For example, in Ramkumar's approach:
>>
>>  % git branch -u master @
>>
>> Would replace '@' with HEAD, however:
>>
>>  % git branch --edit-description @
>>
>> Would not.
>
> git branch -u master <any symbolic ref> will work just fine.  It has
> nothing to do with my series.

% git symbolic-ref TEST refs/heads/master
% git branch -u origin/master TEST
fatal: branch 'TEST' does not exist

You mean it will work just fine _after_ your patch series. So it has
everything to do with your series.

> Does git branch --edit-description HEAD work?  Then why do you expect
> git branch --edit-description <any other symbolic ref> to work?  git
> branch --edit-description operates on non-symbolic refs.
>
> Let me make this clear: @ is just another symbolic-ref that always
> points to the same thing as HEAD.  Nothing less, nothing more.

But HEAD is special, @ is not. HEAD is documented, @ is not.

Where is it documented that @ points to HEAD? Where is it documented
that 'branch -u foo @' would replace @ with HEAD?
Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
revisions, so that's not the answer. And there's many other places
that don't do revision parsing either.

Your approach is more like a hack, it has the consequences we want,
but it has many other unintentional and undocumented consequences.

If I find a single place where 'HEAD' is hard-coded, and your patch
doesn't replace '@' correctly, would you then accept that there are
unintentional consequences, and that this approach is no the best
precisely for that reason?

>> In my opinion, if 'git branch X @{-1}' doesn't work, neither should
>> 'git branch X @', and that's what my approach does.
>
> Why shouldn't (doesn't) git branch X @{-1} work?

That's a red herring. The fact is that right now it doesn't work.

My patch does what it says it does, and documents what it does, and
does not mess with anything else.

> git branch X <any
> expression with or without a symbolic ref> works fine, and it has
> nothing to do with my series.

No, it doesn't.

% git symbolic-ref TEST refs/heads/master
% git branch -u origin/master TEST
fatal: branch 'TEST' does not exist
% git branch --edit-description TEST
error: No branch named 'TEST'.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> But HEAD is special, @ is not. HEAD is documented, @ is not.

Your point being?  That we should document @?  Yes, I agree.

> Where is it documented that @ points to HEAD? Where is it documented
> that 'branch -u foo @' would replace @ with HEAD?
> Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
> revisions, so that's not the answer. And there's many other places
> that don't do revision parsing either.

You're confusing parsing revisions with parsing symbolic-refs.  I've
documented @ right next to HEAD, FETCH_HEAD and the other symbolic
refs in revisions.txt.  Yes, we have to update the documentation of
commands like 'git branch -u foo' to make it clear that they can
operate on symbolic-refs (not just "HEAD") that point to branches, not
just plain branch refs.

Maybe even a fresh page on symbolic refs?

> Your approach is more like a hack,

Now you're just saying rubbish.  Neither of the approaches is more of
a "hack" than the other.  You've implemented @ as a revision, while
I've implemented it as a symbolic-ref.

Your approach requires less effort to document, and my approach yields
an implementation that is almost trivial.  That is not the basis for
determining which approach is "better".

> it has the consequences we want,
> but it has many other unintentional and undocumented consequences.

Who said it wasn't intentional?  Yes, I agree with your criticism
about it being undocumented: please help fixing this.

It's very much intentional.  I _want_ these to work:

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

In other words, I want commands that operate on "HEAD" to also operate
on other symbolic refs similarly.  Is this an unreasonable request?

> If I find a single place where 'HEAD' is hard-coded, and your patch
> doesn't replace '@' correctly, would you then accept that there are
> unintentional consequences, and that this approach is no the best
> precisely for that reason?

You'd have found a bug then, and we must fix it.  Why are you throwing
useful features out the window simply because of difficulty of
documentation/ historical inertia?

> Ramkumar Ramachandra wrote:
>> git branch X <any
>> expression with or without a symbolic ref> works fine, and it has
>> nothing to do with my series.
>
> No, it doesn't.
>
> % git symbolic-ref TEST refs/heads/master
> % git branch -u origin/master TEST
> fatal: branch 'TEST' does not exist
> % git branch --edit-description TEST
> error: No branch named 'TEST'.

Are you reading what you're responding to?  I said:

% git branch X @{-1}
% git branch X HEAD
% git symbolic-ref M refs/heads/master
% git branch X M

Will work with or without my patch.  This is because git branch <1>
<2> runs <2> through the revision parser.

This will work with my patch:

% git symbolic-ref M refs/heads/master
% git branch -u origin/master M

precisely because:

% git branch -u origin/master HEAD

works.  And precisely because this does not:

% git branch -u origin/master @{-1}

In other words, git branch -u <1> <2> expects <2> to be a ref or
symbolic-ref (currently limited to "HEAD"), not a revision.  It
doesn't run <2> through the revision parser to check if it resolves to
a ref.

The following will not work:

% git symbolic-ref M refs/heads/master
% git branch --edit-description M

precisely because:

% git branch --edit-description HEAD

does not work.  This is because git branch --edit-description <1>
expects <1> to be a non-symbolic ref.  It doesn't even hard-code
"HEAD".

Why are you blaming my patch for existing inconsistencies in the UI?

There is one limitation worth nothing: a symbolic-ref can only point
to a ref (or another symbolic ref).  The revision parser doesn't kick
in at the resolve_ref_unsafe() stage.  So, it's quite non-trivial to
implement what Thomas asked for (git symbolic-ref U @{u}).  However, I
think my series is one step in the right direction.  I'd really love
symbolic refs I can take along with me (so M -> master can be in my
.gitconfig): we have to hook resolve_ref_unsafe() to the config API to
achieve this.

In other words, I'm thinking about the future of symbolic refs.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> [...]

Disclaimer: I'm not saying that my implementation is Correct and
Final.  I will be more thorough in my re-roll about justifying my
changes.

What I am saying is that we should fix symbolic refs, and that @
should be implemented at the ref-level to maximize usefulness.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Felipe Contreras]

On Thu, May 2, 2013 at 6:03 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> But HEAD is special, @ is not. HEAD is documented, @ is not.
>
> Your point being?  That we should document @?  Yes, I agree.

Where?

>> Where is it documented that @ points to HEAD? Where is it documented
>> that 'branch -u foo @' would replace @ with HEAD?
>> Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
>> revisions, so that's not the answer. And there's many other places
>> that don't do revision parsing either.
>
> You're confusing parsing revisions with parsing symbolic-refs.  I've
> documented @ right next to HEAD, FETCH_HEAD and the other symbolic
> refs in revisions.txt.  Yes, we have to update the documentation of
> commands like 'git branch -u foo' to make it clear that they can
> operate on symbolic-refs (not just "HEAD") that point to branches, not
> just plain branch refs.

FETCH_HEAD is not a symbolic ref, there's no other special symbolic
ref, HEAD is the only one.

> Maybe even a fresh page on symbolic refs?

Just for '@'? Then what? Are you going to manually inspect the whole
code-base to find in which instances is '@' going to be resoled, and
link the documentation of that command to this new symbolic ref?

Go ahead, let's see what you come up with.

>> Your approach is more like a hack,
>
> Now you're just saying rubbish.  Neither of the approaches is more of
> a "hack" than the other.  You've implemented @ as a revision, while
> I've implemented it as a symbolic-ref.

The former has precedents, the later doesn't. The former can be easily
documented, the later can't. The former is a simple single patch, the
later is bound to dubious changes. The former has no unintended
consequences, the later can have many.

So yeah, I'd say the later is a hack.

> Your approach requires less effort to document,

Your approach can NOT be documented.

> and my approach yields
> an implementation that is almost trivial.

No, it requires multiple changes, THIS is trivial:

--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1025,6 +1025,13 @@ int interpret_branch_name(const char *name,
struct strbuf *buf)
        cp = strchr(name, '@');
        if (!cp)
                return -1;
+
+       if (cp == name && name[1] != '{') {
+               strbuf_reset(buf);
+               strbuf_add(buf, "HEAD", 4);
+               return reinterpret(name, namelen, 1, buf);
+       }
+
        tmp_len = upstream_mark(cp, namelen - (cp - name));
        if (!tmp_len)
                return -1;

That's it, that's the whole patch that implements my approach.

> That is not the basis for
> determining which approach is "better".

Yes it is. If it can be easily documented it's a sign that you are
doing it in the right place. If it's *impossible* to document, it's a
sign that what you are doing is a hack.

If you need multiple changes all over the place it's a sign that what
you are doing is not a natural extension of anything. If it follows
naturally from what people have done in the past, it's a sign that it
is a natural extension.

>> it has the consequences we want,
>> but it has many other unintentional and undocumented consequences.
>
> Who said it wasn't intentional?  Yes, I agree with your criticism
> about it being undocumented: please help fixing this.

Why? Why document a hack all over the place? Especially when we do NOT
have to. We don't need this hack.

> It's very much intentional.  I _want_ these to work:
>
> % git symbolic-ref M refs/heads/master
> % git show M@{u}
> % git branch -u ram/master M
>
> In other words, I want commands that operate on "HEAD" to also operate
> on other symbolic refs similarly.  Is this an unreasonable request?

That has absolutely nothing to do with the '@' shortcut. Nothing at all.

The mere fact that we are discussing about random symbolic refs is
proof that you can't focus on the issue at hand.

>> If I find a single place where 'HEAD' is hard-coded, and your patch
>> doesn't replace '@' correctly, would you then accept that there are
>> unintentional consequences, and that this approach is no the best
>> precisely for that reason?
>
> You'd have found a bug then, and we must fix it.

So that's it. There's absolutely no way to convince you that your
approach is the wrong one. No amount of evidence to the contrary will
change your mind.

There's no point in discussing then.

> Why are you throwing
> useful features out the window simply because of difficulty of
> documentation/ historical inertia?

I'm not throwing any useful features, I'm implementing them correctly.

>> Ramkumar Ramachandra wrote:
>>> git branch X <any
>>> expression with or without a symbolic ref> works fine, and it has
>>> nothing to do with my series.
>>
>> No, it doesn't.
>>
>> % git symbolic-ref TEST refs/heads/master
>> % git branch -u origin/master TEST
>> fatal: branch 'TEST' does not exist
>> % git branch --edit-description TEST
>> error: No branch named 'TEST'.
>
> Are you reading what you're responding to?  I said:
>
> % git branch X @{-1}
> % git branch X HEAD
> % git symbolic-ref M refs/heads/master
> % git branch X M
>
> Will work with or without my patch.  This is because git branch <1>
> <2> runs <2> through the revision parser.

The branch commands that run through the revision parser are
irrelevant. 'git branch foo @' will work with my patch too.

> This will work with my patch:
>
> % git symbolic-ref M refs/heads/master
> % git branch -u origin/master M

Which is irrelevant to the discussion about '@'.

> precisely because:
>
> % git branch -u origin/master HEAD
>
> works.  And precisely because this does not:
>
> % git branch -u origin/master @{-1}
>
> In other words, git branch -u <1> <2> expects <2> to be a ref or
> symbolic-ref (currently limited to "HEAD"), not a revision.  It
> doesn't run <2> through the revision parser to check if it resolves to
> a ref.
>
> The following will not work:
>
> % git symbolic-ref M refs/heads/master
> % git branch --edit-description M
>
> precisely because:
>
> % git branch --edit-description HEAD
>
> does not work.  This is because git branch --edit-description <1>
> expects <1> to be a non-symbolic ref.  It doesn't even hard-code
> "HEAD".
>
> Why are you blaming my patch for existing inconsistencies in the UI?

Your patch is introducing more, and you can NOT document all the
changes you are introducing.

> There is one limitation worth nothing: a symbolic-ref can only point
> to a ref (or another symbolic ref).  The revision parser doesn't kick
> in at the resolve_ref_unsafe() stage.  So, it's quite non-trivial to
> implement what Thomas asked for (git symbolic-ref U @{u}).  However, I
> think my series is one step in the right direction.  I'd really love
> symbolic refs I can take along with me (so M -> master can be in my
> .gitconfig): we have to hook resolve_ref_unsafe() to the config API to
> achieve this.
>
> In other words, I'm thinking about the future of symbolic refs.

That has absolutely nothing to do with the '@' shortcut.

Since you can't talk about the issue of the '@' without mentioning
other random symlinks, it's basically impossible to have a fruitful
discussion.

And since there's absolutely no evidence or logical argument that
would convince you that your approach is not the right one, there is
no point in discussing.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> Your approach can NOT be documented.

Ah, I missed that.  The explanation I was looking for is:

HEAD has been special right from the start, and we cannot elevate
anything else to its status now.

Thanks.  And sorry it took me so long.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Felipe Contreras]

On Thu, May 2, 2013 at 11:56 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> Your approach can NOT be documented.
>
> Ah, I missed that.  The explanation I was looking for is:
>
> HEAD has been special right from the start, and we cannot elevate
> anything else to its status now.

We can, but do we _need_ to? Adding another magical symbolic ref that
has limited scope should not be a huge task, as the effects can be
documented, but another magical symbolic ref that does *exactly* the
same as HEAD would require too many changes in the code and
documentation it's not even worth considering, especially if we don't
_need_ to.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> HEAD has been special right from the start, and we cannot elevate
> anything else to its status now.

As to why I agree with you: I audited the callers of branch_get() and
found out there are some things that cannot be fixed just by fixing
branch_get():

For instance, 'git fetch origin M' will not work, and it's not
branch_get()'s fault.  And that is just one instance: there are tons
of other instances; and going and changing each one of them will take
forever, and totally not be worth the pain.

In other words; symbolic refs are a dead end.  The future is in
getting everything to use the standard revision parser.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Felipe Contreras]

On Thu, May 2, 2013 at 12:02 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> HEAD has been special right from the start, and we cannot elevate
>> anything else to its status now.
>
> As to why I agree with you: I audited the callers of branch_get() and
> found out there are some things that cannot be fixed just by fixing
> branch_get():
>
> For instance, 'git fetch origin M' will not work, and it's not
> branch_get()'s fault.  And that is just one instance: there are tons
> of other instances; and going and changing each one of them will take
> forever, and totally not be worth the pain.

We probably should fix those, but that is orthogonal to the '@' shortcut.

We can have the '@' shortcut *today*, with minimal changes to the code
and the documentation, in a limited and understood scope, with no
surprises.

We can fix the symbolic ref stuff slowly, step by step, no need to
delay the '@' shortcut for that.

-- 
Felipe Contreras

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> We probably should fix those, but that is orthogonal to the '@' shortcut.
>
> We can have the '@' shortcut *today*, with minimal changes to the code
> and the documentation, in a limited and understood scope, with no
> surprises.
>
> We can fix the symbolic ref stuff slowly, step by step, no need to
> delay the '@' shortcut for that.

Agreed.

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[David Aguilar]

On Thu, May 2, 2013 at 10:09 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> We probably should fix those, but that is orthogonal to the '@' shortcut.
>>
>> We can have the '@' shortcut *today*, with minimal changes to the code
>> and the documentation, in a limited and understood scope, with no
>> surprises.
>>
>> We can fix the symbolic ref stuff slowly, step by step, no need to
>> delay the '@' shortcut for that.
>
> Agreed.

If only we didn't care about windows then we could have this feature today:

    cd .git
    ln -s HEAD @

...and everything works, in the most natural way.
Has anyone else tweaked their repo this way?

Is an alternative implementation to change the template repo to ship
with a symlink?

For windows, perhaps we can use this new code behind an #ifdef?

Anyways, it's just a crazy idea.  I very much like this feature,
and in a tweaked repo @{0}{1} actually works.

Is there no way to tweak this at some really low level to trick git
into believing the link exists (even when it doesn't)?

I guess that's what these patches do, but the limitations seem unfortunate.
--
David

Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

[David Aguilar]

On Sat, May 4, 2013 at 1:10 AM, David Aguilar <davvid@gmail.com> wrote:
> On Thu, May 2, 2013 at 10:09 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Felipe Contreras wrote:
>>> We probably should fix those, but that is orthogonal to the '@' shortcut.
>>>
>>> We can have the '@' shortcut *today*, with minimal changes to the code
>>> and the documentation, in a limited and understood scope, with no
>>> surprises.
>>>
>>> We can fix the symbolic ref stuff slowly, step by step, no need to
>>> delay the '@' shortcut for that.
>>
>> Agreed.
>
> If only we didn't care about windows then we could have this feature today:
>
>     cd .git
>     ln -s HEAD @
>
> ...and everything works, in the most natural way.
> Has anyone else tweaked their repo this way?
>
> Is an alternative implementation to change the template repo to ship
> with a symlink?
>
> For windows, perhaps we can use this new code behind an #ifdef?
>
> Anyways, it's just a crazy idea.  I very much like this feature,
> and in a tweaked repo @{0}{1} actually works.
>
> Is there no way to tweak this at some really low level to trick git
> into believing the link exists (even when it doesn't)?
>
> I guess that's what these patches do, but the limitations seem unfortunate.

Nevermind.  @{0}{1} is nothing special and symbolic-ref happily
replaces symlinks with a file, so new code is needed regardless.  I'll
start testing these patches instead.
--
David