Re: [PATCH 1/2] t1506: more test for @{upstream} syntax

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:06

Jeff King [off-list ref] writes:

The second one is that "log -g branch@{u}" shows the correct commits
(from the upstream of "branch"), but displays the incorrect reflog
information (it shows information for "branch", not for its upstream).

That "walk-reflog" code is tricky.  How about this?

I don't know if it deals with things like "@{-1}@{u}@{now}"; the users
should have every right to expect it to, but I didn't consciously try to
make that work with this patch.

 revision.c                    |   18 ++++++++++++++----
 t/t1507-rev-parse-upstream.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index f54d43f..75071af 100644
--- a/revision.c
+++ b/revision.c

@@ -134,10 +134,20 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o
 {
 	if (revs->no_walk && (obj->flags & UNINTERESTING))
 		revs->no_walk = 0;
-	if (revs->reflog_info && obj->type == OBJ_COMMIT &&
-			add_reflog_for_walk(revs->reflog_info,
-				(struct commit *)obj, name))
-		return;
+	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
+		struct strbuf buf = STRBUF_INIT;
+		int len = interpret_branch_name(name, &buf);
+		int st;
+
+		if (len && name[len])
+			strbuf_addstr(&buf, name + len);
+		st = add_reflog_for_walk(revs->reflog_info,
+					 (struct commit *)obj,
+					 buf.buf[0] ? buf.buf: name);
+		strbuf_release(&buf);
+		if (st)
+			return;
+	}
 	add_object_array_with_mode(obj, name, &revs->pending, mode);
 }

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 95c9b09..8c8dfda 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh

@@ -107,4 +107,33 @@ test_expect_success 'checkout other@{u}' '
 	test_cmp expect actual
 '
 
+cat >expect <<EOF
+commit 8f489d01d0cc65c3b0f09504ec50b5ed02a70bd5
+Reflog: master@{0} (C O Mitter <committer@example.com>)
+Reflog message: branch: Created from HEAD
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3
+EOF
+test_expect_success 'log -g other@{u}' '
+	git log -1 -g other@{u} >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<EOF
+commit 8f489d01d0cc65c3b0f09504ec50b5ed02a70bd5
+Reflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter <committer@example.com>)
+Reflog message: branch: Created from HEAD
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3
+EOF
+
+test_expect_success 'log -g other@{u}@{now}' '
+	git log -1 -g other@{u}@{now} >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH 1/2] t1506: more test for @{upstream} syntax

From: Jeff King <hidden>
Date: 2016-06-15 22:48:06

On Tue, Jan 26, 2010 at 01:32:07PM -0800, Junio C Hamano wrote:

I don't know if it deals with things like "@{-1}@{u}@{now}"; the users
should have every right to expect it to, but I didn't consciously try to
make that work with this patch.

Nice. This also fixes "git log -g @{-1}". Static uses like "git show
@{u}@{1.week.ago}" and "git show @{-1}@{1.week.ago}" were already fine,
so I think the bug was really confined to the reflog walker (and your
fix is therefore correct).

Using "git show @{-1}@{u}" is still broken, though.

I tried tracing the parsing through get_sha1_basic and
interpret_branch_name, but it's pretty confusing. Especially as we seem
to deal with @{upstream}, @{now}, and @{-1} at different places.

I think the patch below does what we want, but the whole thing feels
overly complicated to me, especially with the split of parsing @{...}
between get_sha1_basic and interpret_branch_name. I guess we have spots
that don't take reflogs but do take branch names, but I think the code
would be much simpler if the syntax were parsed in one place, and then
we threw out or complained about bogus semantics (like "checkout
@{now}").

---

diff --git a/sha1_name.c b/sha1_name.c
index ed4c028..ef8f3fa 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -881,8 +881,28 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 
 	if (!len)
 		return len; /* syntax Ok, not enough switches */
-	if (0 < len)
-		return len; /* consumed from the front */
+	if (0 < len && len == namelen)
+		return len; /* consumed all */
+	else if (0 < len) {
+		/* we have extra data, which might need further processing */
+		struct strbuf tmp = STRBUF_INIT;
+		int used = buf->len;
+		int ret;
+
+		strbuf_add(buf, name + len, namelen - len);
+		ret = interpret_branch_name(buf->buf, &tmp);
+		/* that data was not interpreted, remove our cruft */
+		if (ret < 0) {
+			strbuf_setlen(buf, used);
+			return len;
+		}
+		strbuf_reset(buf);
+		strbuf_addbuf(buf, &tmp);
+		strbuf_release(&tmp);
+		/* tweak for size of {-N} versus expanded ref name */
+		return ret - used + len;
+	}
+
 	cp = strchr(name, '@');
 	if (!cp)
 		return -1;

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help