default behaviour for `gitmerge` (no arguments)

default behaviour for `gitmerge` (no arguments)

[Gareth Adams]

Hi there, long time user; first time caller here.

I wanted to suggest an improvement to git-merge which will either save some
typing or save some network resources. It won't save huge amounts of either but
every little helps!

Currently, some of my colleagues frequently end up typing:

    git pull; ...; git checkout otherbranch; git pull

Now, we have quite a low commit rate, it's unlikely (albeit vaguely possible)
that two people are working on the branch at the same time. This means the
second pull is doing a fetch which it effectively pointless.

Now of course this is a tiny amount of wastage, and while I could argue that it
would be an issue under poor network conditions that's not my point. As a coder
I'd want to get rid of the redundant fetch if I know it's redundant.

Unfortunately my other option is:

    git pull; ...; git checkout otherbranch; git merge myremote/otherbranch

which is annoying extra typing. Even with tab completion, it's redundant extra
typing because in these cases I'm trying to merge with the branch being tracked.

My suggestion is that `git merge` defaults to the same merge that a `git pull`
would perform, and there are 2 extra factors that make me think it's a workable
idea:

1) At the moment, `git merge` does nothing. Except mock me for not giving it a
command in a format it recognises. This change wouldn't have any effect that
would cause anyone a problem

2) When I checkout a branch which has unmerged changes in the tracking branch,
git *tells me* what branch I will be taking action with "Your branch is behind
the tracked remote branch '...' by 4 commits, and can be fast-forwarded" - but
then makes me type it out explicitly anyway!

I appreciate that there are many workflows where there is an advantage in
performing a second pull in case there are additional changes since the first
pull, but I still think there is a string case for git merge having a more
sensible default, as git pull does.

What do you think?

Thanks,
Gareth

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Gareth Adams <gareth.adams@gmail.com> writes:

> Unfortunately my other option is:
>
>     git pull; ...; git checkout otherbranch; git merge myremote/otherbranch
>
> which is annoying extra typing.

Replace 'pull' with 'fetch' and a bit more regular pattern would emerge.

The code indeed knows (as you can see "git pull" can figure it out) what
other ref the current branch is configured to merge with by default.
There is even a plumbing to do this for script writers.

    $ git for-each-ref --format='%(upstream)' $(git symbolic-ref HEAD)

We can teach this short-hand to "git merge", perhaps:

    $ git merge --default

But "no argument" cannot be the short-hand, because...

> 1) At the moment, `git merge` does nothing. Except mock me for not giving it a
> command in a format it recognises. This change wouldn't have any effect that
> would cause anyone a problem

... except for people who uses a script that does

    commits=
    while some condition
    do
    	commit=$(find some other commit that should be merged)
        commits="$commits$commit "
    done
    git merge $commits

and expect the last step will fail without doing any damage when the loop
finds no new developments.  "no argument means --default" will break their
scripts.

Re: default behaviour for `gitmerge` (no arguments)

[Jeff King]

On Mon, Jan 11, 2010 at 11:43:40AM -0800, Junio C Hamano wrote:

> The code indeed knows (as you can see "git pull" can figure it out) what
> other ref the current branch is configured to merge with by default.
> There is even a plumbing to do this for script writers.
> 
>     $ git for-each-ref --format='%(upstream)' $(git symbolic-ref HEAD)
> 
> We can teach this short-hand to "git merge", perhaps:
> 
>     $ git merge --default
> 
> But "no argument" cannot be the short-hand, because...

Hmm. If we had the oft-discussed-but-never-agreed-upon shorthand for
"the upstream of" then we wouldn't need a special merge option. You
could just do:

  git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)

-Peff

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm. If we had the oft-discussed-but-never-agreed-upon shorthand for
> "the upstream of" then we wouldn't need a special merge option. You
> could just do:
>
>   git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)

I don't think "whatever _HEAD_ tracks" makes sense at the semantic level
(i.e. you don't do "branch.HEAD.merge") but a syntax for "whatever the
named _branch_ tracks" with "if a branch is not named, the current branch
is implied" (i.e. the one in parentheses) would.

It is an entirely different matter what the special syntax to trigger that
"upstream-ness" should be.  I vaguely recall @{upstream} or @{u} were the
concensus?

Re: default behaviour for `gitmerge` (no arguments)

[Jeff King]

On Tue, Jan 12, 2010 at 10:11:26AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Hmm. If we had the oft-discussed-but-never-agreed-upon shorthand for
> > "the upstream of" then we wouldn't need a special merge option. You
> > could just do:
> >
> >   git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)
> 
> I don't think "whatever _HEAD_ tracks" makes sense at the semantic level
> (i.e. you don't do "branch.HEAD.merge") but a syntax for "whatever the
> named _branch_ tracks" with "if a branch is not named, the current branch
> is implied" (i.e. the one in parentheses) would.

The patch that Dscho provided would actually convert HEAD@{upstream}
into the upstream of whatever HEAD pointed at. Which I think makes
sense. We don't do it for reflogs, but that is because it is useful to
distinguish between the reflog for a symref and the thing it points to.
But since one would presumably not make such a configuration for a
symref, that distinction is not useful.

> It is an entirely different matter what the special syntax to trigger that
> "upstream-ness" should be.  I vaguely recall @{upstream} or @{u} were the
> concensus?

Ah, right. I remembered hating "%" even as I typed it, but I had
forgotten about the followup discussion. Looking at it again, I note:

  1. The last posted patch still has a misplaced free() (patch below),
     but I think otherwise is not buggy.

  2. We don't complain on "git show @{usptream}" and we probably should.
     I remember there being some complications because the contents of
     @{} were passed to approxidate, but I think we can get around that
     by letting approxidate complain if _nothing_ in the date was
     useful. So "git show @{2.weeks.and.7.hot.dogs.ago}" would still
     work, but "git show @{totally.bogus.input}" would complain.

  3. I have actually been running with Dscho's patch for the last couple
     of months, and I don't remember using it once. So perhaps it is not
     as useful as I might have thought. :)

Anyway, fixup patch is below. I don't expect you to pick up the topic or
anything, but since I went to the trouble to find the bug once upon a
time, I thought I would post the fix for anybody who does want to pick
it up.

diff --git a/sha1_name.c b/sha1_name.c
index b73b93e..da90ebe 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -275,9 +275,9 @@ static char *substitute_branch_name(const char **string, int *len)
 		char *ref = xstrndup(*string, *len - ret);
 		struct branch *tracking = branch_get(*ref ? ref : NULL);
 
-		free(ref);
 		if (!tracking)
 			die ("No tracking branch found for '%s'", ref);
+		free(ref);
 		if (tracking->merge && tracking->merge[0]->dst) {
 			*string = xstrdup(tracking->merge[0]->dst);
 			*len = strlen(*string);

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah, right. I remembered hating "%" even as I typed it, but I had
> forgotten about the followup discussion. Looking at it again, I note:
>
>   1. The last posted patch still has a misplaced free() (patch below),
>      but I think otherwise is not buggy.
>
>   2. We don't complain on "git show @{usptream}" and we probably should.
>      I remember there being some complications because the contents of
>      @{} were passed to approxidate, but I think we can get around that
>      by letting approxidate complain if _nothing_ in the date was
>      useful. So "git show @{2.weeks.and.7.hot.dogs.ago}" would still
>      work, but "git show @{totally.bogus.input}" would complain.
>
>   3. I have actually been running with Dscho's patch for the last couple
>      of months, and I don't remember using it once. So perhaps it is not
>      as useful as I might have thought. :)

I presume we are discussing this patch?

    http://article.gmane.org/gmane.comp.version-control.git/128121

I'll squash the free() fix; thanks.

I wondered why it doesn't hook into interpret_branch_name(), and instead
adds itself to the static substitute_branch_name(); it forbids the use of
the syntax from by callers of strbuf_branchname().

I agree with your point #2 above.

Regarding your point #3, I don't think the notation should be that useful
if your workflow is sane.  The original use case that triggered the
resurrection of the patch went like this:

        git fetch &&
        for local in my set of local branches
        do
                git checkout $local &&
                git merge $local@{upstream} || {
                        echo failed to merge on $local
                        break
                }
        done

and the new notation might look useful in the scenario.  But the thing is,
constantly merging with the other side, even if you haven't added anything
of value since you merged from there last time, is a bad practice to begin
with.  I added one use case that is sane _and_ will be helped by the new
notation to the rewritten version of Dscho's patch (below).

Just to refresh our memory from the old thread and make sure we are
discussing the same patch, here is what I am planning to queue.  The log
message and documentation are somewhat updated to avoid the word "track"
because it seems that everybody gets confused and starts talking different
things whenever that word is used.  For the same reason, the test script
was renamed.

In this set-up, for example:

    [remote "filfre"]
        url = ...
        fetch = +refs/heads/nitfol:refs/heads/rezrov
    [branch "frotz"]
        remote = filfre
        merge = refs/heads/nitfol

some people say rezrov tracks nitfol from filfre but Dscho's patch says
frotz tracks nitfol from filfre.  They _may_ both track, but they "track"
the other in a quite differently way, so the word has become meaningless.

I've been trying to be careful and used different words to disambiguate
whenever I had to talk about these concepts:

 - The purpose of rezrov is to keep a tab on the progress of the nitfol
   branch at the remote end.  We say rezrov is a remote tracking branch
   for nitfol from filfre.

 - On the other hand, we have branch frotz that forked from nitfol that
   came from filfre.  It builds on top of that history by occasionally
   merging with it at key points in the history.  So we say frotz builds
   on top of nitfol from filfre.  We also say nitfol at filfre is the
   upstream of frotz.

-- >8 --
Date: Thu, 10 Sep 2009 17:25:57 +0200
Subject: [PATCH] Introduce <branch>@{upstream} notation

A new notation '<branch>@{upstream}' refers to the branch <branch> is set
to build on top of.  Missing <branch> (i.e. '@{upstream}') defaults to the
current branch.

This allows you to run, for example,

	for l in list of local branches
	do
		git log --oneline --left-right $l...$l@{upstream}
	done

to inspect each of the local branches you are interested in for the
divergence from its upstream.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-rev-parse.txt |    4 ++
 sha1_name.c                     |   39 ++++++++++++++++++++--
 t/t1506-rev-parse-upstream.sh   |   69 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 3 deletions(-)
 create mode 100755 t/t1506-rev-parse-upstream.sh

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 82045a2..923b56a 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -231,6 +231,10 @@ when you run 'git-merge'.
 * The special construct '@\{-<n>\}' means the <n>th branch checked out
   before the current one.
 
+* The suffix '@{upstream}' to a ref (short form 'ref@{u}') refers to
+  the branch the ref is set to build on top of.  Missing ref defaults
+  to the current branch.
+
 * 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}'
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..fb4e214 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -5,6 +5,7 @@
 #include "blob.h"
 #include "tree-walk.h"
 #include "refs.h"
+#include "remote.h"
 
 static int find_short_object_filename(int len, const char *name, unsigned char *sha1)
 {
@@ -238,9 +239,24 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
+static inline int tracked_suffix(const char *string, int len)
+{
+	const char *suffix[] = { "@{upstream}", "@{u}" };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
+		int suffix_len = strlen(suffix[i]);
+		if (len >= suffix_len && !memcmp(string + len - suffix_len,
+					suffix[i], suffix_len))
+			return suffix_len;
+	}
+	return 0;
+}
+
 /*
  * *string and *len will only be substituted, and *string returned (for
- * later free()ing) if the string passed in is of the form @{-<n>}.
+ * later free()ing) if the string passed in is of the form @{-<n>} or
+ * of the form <branch>@{upstream}.
  */
 static char *substitute_branch_name(const char **string, int *len)
 {
@@ -254,6 +270,21 @@ static char *substitute_branch_name(const char **string, int *len)
 		return (char *)*string;
 	}
 
+	ret = tracked_suffix(*string, *len);
+	if (ret) {
+		char *ref = xstrndup(*string, *len - ret);
+		struct branch *tracking = branch_get(*ref ? ref : NULL);
+
+		if (!tracking)
+			die ("No tracking branch found for '%s'", ref);
+		free(ref);
+		if (tracking->merge && tracking->merge[0]->dst) {
+			*string = xstrdup(tracking->merge[0]->dst);
+			*len = strlen(*string);
+			return (char *)*string;
+		}
+	}
+
 	return NULL;
 }
 
@@ -340,8 +371,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && str[len-1] == '}') {
 		for (at = len-2; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
-				reflog_len = (len-1) - (at+2);
-				len = at;
+				if (!tracked_suffix(str + at, len - at)) {
+					reflog_len = (len-1) - (at+2);
+					len = at;
+				}
 				break;
 			}
 		}
diff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh
new file mode 100755
index 0000000..5abdc13
--- /dev/null
+++ b/t/t1506-rev-parse-upstream.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test <branch>@{upstream} syntax'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+
+	test_commit 1 &&
+	git checkout -b side &&
+	test_commit 2 &&
+	git checkout master &&
+	git clone . clone &&
+	test_commit 3 &&
+	(cd clone &&
+	 test_commit 4 &&
+	 git branch --track my-side origin/side)
+
+'
+
+full_name () {
+	(cd clone &&
+	 git rev-parse --symbolic-full-name "$@")
+}
+
+commit_subject () {
+	(cd clone &&
+	 git show -s --pretty=format:%s "$@")
+}
+
+test_expect_success '@{upstream} resolves to correct full name' '
+	test refs/remotes/origin/master = "$(full_name @{upstream})"
+'
+
+test_expect_success '@{u} resolves to correct full name' '
+	test refs/remotes/origin/master = "$(full_name @{u})"
+'
+
+test_expect_success 'my-side@{upstream} resolves to correct full name' '
+	test refs/remotes/origin/side = "$(full_name my-side@{u})"
+'
+
+test_expect_success 'my-side@{u} resolves to correct commit' '
+	git checkout side &&
+	test_commit 5 &&
+	(cd clone && git fetch) &&
+	test 2 = "$(commit_subject my-side)" &&
+	test 5 = "$(commit_subject my-side@{u})"
+'
+
+test_expect_success 'not-tracking@{u} fails' '
+	test_must_fail full_name non-tracking@{u} &&
+	(cd clone && git checkout --no-track -b non-tracking) &&
+	test_must_fail full_name non-tracking@{u}
+'
+
+test_expect_success '<branch>@{u}@{1} resolves correctly' '
+	test_commit 6 &&
+	(cd clone && git fetch) &&
+	test 5 = $(commit_subject my-side@{u}@{1})
+'
+
+test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
+	git checkout HEAD^0 &&
+	test_must_fail git rev-parse @{u}
+'
+
+test_done
-- 
1.6.6.280.ge295b7.dirty

Re: default behaviour for `gitmerge` (no arguments)

[Johannes Schindelin]

Hi,

On Tue, 12 Jan 2010, Junio C Hamano wrote:

> I wondered why it doesn't hook into interpret_branch_name(), and instead 
> adds itself to the static substitute_branch_name(); it forbids the use 
> of the syntax from by callers of strbuf_branchname().

I _think_ it was to allow something like

	git log -g @{u}

but frankly, this is so long ago, I do not remember, I reconstructed this 
reasoning as being the most likely.

Ciao,
Dscho

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I wondered why it doesn't hook into interpret_branch_name(), and instead 
>> adds itself to the static substitute_branch_name(); it forbids the use 
>> of the syntax from by callers of strbuf_branchname().
>
> I _think_ it was to allow something like
>
> 	git log -g @{u}
>
> but frankly, this is so long ago, I do not remember, I reconstructed this 
> reasoning as being the most likely.

That is not the question I was asking.

If you compare substitute_branch_name() and interpret_branch_name() before
your patch, you will notice that they are _meant_ to do the same thing,
with different external API, only because many callers in sha1_name.c do
not use strbuf to hold their names.  The primary API is the latter (which
is extern), and the former (which is static) is merely a helping wrapper
that is internal to sha1_name.c

But with your patch, they suddenly have different semantics, and the
function that implements the primary API doesn't know anything about
this new @{upstream} syntax.

This discrepancy will affect callers of strbuf_branchname(), e.g.
merge_name() in builtin-merge.c that prepares the "Merge branch nitfol of
remote frotz" message, or delete_branches() in builtin-branch.c.

Note that I am not saying "branch -d @{upstream}" should or should not
work (at least not yet---I haven't thought the issues through).  But I
wanted to know if this subtle change in the semantics was a deliberate
choice, and if so wanted to see the reason behind it described clearly.

Re: default behaviour for `gitmerge` (no arguments)

[Johannes Schindelin]

Hi,

On Wed, 13 Jan 2010, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I wondered why it doesn't hook into interpret_branch_name(), and 
> >> instead adds itself to the static substitute_branch_name(); it 
> >> forbids the use of the syntax from by callers of strbuf_branchname().
> >
> > I _think_ it was to allow something like
> >
> > 	git log -g @{u}
> >
> > but frankly, this is so long ago, I do not remember, I reconstructed this 
> > reasoning as being the most likely.
> 
> That is not the question I was asking.
> 
> If you compare substitute_branch_name() and interpret_branch_name() before
> your patch, you will notice that they are _meant_ to do the same thing,
> with different external API, only because many callers in sha1_name.c do
> not use strbuf to hold their names.  The primary API is the latter (which
> is extern), and the former (which is static) is merely a helping wrapper
> that is internal to sha1_name.c

So you meant to say that substitute_branch_name() calls 
interpret_branch_name(), so the change should be in the latter.  (This is 
supposed to be the summary of your 4 paragraphs.)

I have no problems with that, except that I do not have the time to do it 
myself.

Ciao,
Dscho

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> So you meant to say that substitute_branch_name() calls 
> interpret_branch_name(), so the change should be in the latter.  (This is 
> supposed to be the summary of your 4 paragraphs.)

Not quite.  What I was asking was:

	*PROVIDED* *IF* you wanted to keep the same semantics between
         them, then you would have patched i-b-n, but you didn't.  Was there
         a reason callers of s-b-n should know about @{u} but callers of i-b-n
	shouldn't?

Expected answer was either:

	(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as
            a symbolic name given by the user, but it should treat it as a
            mere SHA-1 expression instead for *this and that* reason.
            Otherwise we will see *this* breakage when the user does
            *that*.  That is why i-b-n doesn't know about the new syntax;
            or

	(b) It was a thinko; all codepaths that use i-b-n should know the
            new syntax as they _are_ interested in learning the symbolic
            name when the user gives @{upstream}.

Re: default behaviour for `gitmerge` (no arguments)

[Johannes Schindelin]

Hi,

On Wed, 13 Jan 2010, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> What I was asking was:
> 
> 	*PROVIDED* *IF* you wanted to keep the same semantics between
>          them, then you would have patched i-b-n, but you didn't.  Was there
>          a reason callers of s-b-n should know about @{u} but callers of i-b-n
> 	shouldn't?
> 
> Expected answer was either:
> 
> 	(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as
>             a symbolic name given by the user, but it should treat it as a
>             mere SHA-1 expression instead for *this and that* reason.
>             Otherwise we will see *this* breakage when the user does
>             *that*.  That is why i-b-n doesn't know about the new syntax;
>             or
> 
> 	(b) It was a thinko; all codepaths that use i-b-n should know the
>             new syntax as they _are_ interested in learning the symbolic
>             name when the user gives @{upstream}.

And I gave answer (c): I do not remember.

Ciao,
Dscho

Re: default behaviour for `gitmerge` (no arguments)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> What I was asking was:
>> 
>> 	*PROVIDED* *IF* you wanted to keep the same semantics between
>>          them, then you would have patched i-b-n, but you didn't.  Was there
>>          a reason callers of s-b-n should know about @{u} but callers of i-b-n
>> 	shouldn't?
>> 
>> Expected answer was either:
>> 
>> 	(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as
>>             a symbolic name given by the user, but it should treat it as a
>>             mere SHA-1 expression instead for *this and that* reason.
>>             Otherwise we will see *this* breakage when the user does
>>             *that*.  That is why i-b-n doesn't know about the new syntax;
>>             or
>> 
>> 	(b) It was a thinko; all codepaths that use i-b-n should know the
>>             new syntax as they _are_ interested in learning the symbolic
>>             name when the user gives @{upstream}.
>
> And I gave answer (c): I do not remember.

That's fine.  As long as we know it is (c) (your earlier response sounded
as if you were saying (b)), we know that there might be issues we need to
find and carefully look at before using this topic.

[PATCH 0/2] @{u} updates

[Junio C Hamano]

Earlier I wondered if the approach Dscho's patch takes to teach the new
@{upstream} syntax to substitute_branch_name() (hence dwim_ref()) without
teaching it to interpret_branch_name() (hence strbuf_branchname()) was a
bad idea.  I thought about this a bit more; there are some downsides for
not doing so.

The first patch adds a handful of tests that show why strbuf_branchname()
callers may also want to learn about the new syntax.  The second patch
moves the logic to interpret_branch_name() to make them happier.

The name of the key function was changed from tracked_suffix() to
upstream_mark(), not only because the syntax talks about @{upstream}, but
because the parsing needs to recognize the @{u}/@{upstream} mark at the
beginning of the given string (that is a suffix to some other string), and
strip it (the earlier code wanted @{u} to be at the very end but the
callers need to have it at the beginning).


Junio C Hamano (2):
  t1506: more test for @{upstream} syntax
  Teach @{upstream} syntax to strbuf_branchanme()

 sha1_name.c                   |  116 ++++++++++++++++++++++++++---------------
 t/t1506-rev-parse-upstream.sh |   41 ++++++++++++++
 2 files changed, 115 insertions(+), 42 deletions(-)

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

[Junio C Hamano]

This adds a few more tests that exercises @{upstream} syntax by commands
that operate differently when they are given branch name as opposed to a
refname (i.e. where "master" and "refs/heads/master" makes a difference).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1506-rev-parse-upstream.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh
index 5abdc13..a2c7f92 100755
--- a/t/t1506-rev-parse-upstream.sh
+++ b/t/t1506-rev-parse-upstream.sh
@@ -66,4 +66,45 @@ test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
 	test_must_fail git rev-parse @{u}
 '
 
+test_expect_success 'checkout -b new my-side@{u} forks from the same' '
+(
+	cd clone &&
+	git checkout -b new my-side@{u} &&
+	git rev-parse --symbolic-full-name my-side@{u} >expect &&
+	git rev-parse --symbolic-full-name new@{u} >actual &&
+	test_cmp expect actual
+)
+'
+
+test_expect_failure 'merge my-side@{u} records the correct name' '
+(
+	sq="'\''" &&
+	cd clone || exit
+	git checkout master || exit
+	git branch -D new ;# can fail but is ok
+	git branch -t new my-side@{u} &&
+	git merge -s ours new@{u} &&
+	git show -s --pretty=format:%s >actual &&
+	echo "Merge remote branch ${sq}origin/side${sq}" >expect &&
+	test_cmp expect actual
+)
+'
+
+test_expect_failure 'branch -d other@{u}' '
+	git checkout -t -b other master &&
+	git branch -d @{u} &&
+	git for-each-ref refs/heads/master >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'checkout other@{u}' '
+	git branch -f master HEAD &&
+	git checkout -t -b another master &&
+	git checkout @{u} &&
+	git symbolic-ref HEAD >actual &&
+	echo refs/heads/master >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.6.6.513.g63f4c

[PATCH 2/2] Teach @{upstream} syntax to strbuf_branchanme()

[Junio C Hamano]

This teaches @{upstream} syntax to interpret_branch_name(), instead
of dwim_ref() machinery.

There are places in git UI that behaves differently when you give a local
branch name and when you give an extended SHA-1 expression that evaluates
to the commit object name at the tip of the branch.  The intent is that
the special syntax such as @{-1} can stand in as if the user spelled the
name of the branch in such places.

The name of the branch "frotz" to switch to ("git checkout frotz"), and
the name of the branch "nitfol" to fork a new branch "frotz" from ("git
checkout -b frotz nitfol"), are examples of such places.  These places
take only the name of the branch (e.g. "frotz"), and they are supposed to
act differently to an equivalent refname (e.g. "refs/heads/frotz"), so
hooking the @{upstream} and @{-N} syntax to dwim_ref() is insufficient
when we want to deal with cases a local branch is forked from another
local branch and use "forked@{upstream}" to name the forkee branch.

The "upstream" syntax "forked@{u}" is to specify the ref that "forked" is
configured to merge with, and most often the forkee is a remote tracking
branch, not a local branch.  We cannot simply return a local branch name,
but that does not necessarily mean we have to returns the full refname
(e.g. refs/remotes/origin/frotz, when returning origin/frotz is enough).
This update calls shorten_unambiguous_ref() to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c                   |  116 ++++++++++++++++++++++++++---------------
 t/t1506-rev-parse-upstream.sh |    6 +-
 2 files changed, 77 insertions(+), 45 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index fb4e214..2376c6d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -239,24 +239,10 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
-static inline int tracked_suffix(const char *string, int len)
-{
-	const char *suffix[] = { "@{upstream}", "@{u}" };
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
-		int suffix_len = strlen(suffix[i]);
-		if (len >= suffix_len && !memcmp(string + len - suffix_len,
-					suffix[i], suffix_len))
-			return suffix_len;
-	}
-	return 0;
-}
-
 /*
  * *string and *len will only be substituted, and *string returned (for
- * later free()ing) if the string passed in is of the form @{-<n>} or
- * of the form <branch>@{upstream}.
+ * later free()ing) if the string passed in is a magic short-hand form
+ * to name a branch.
  */
 static char *substitute_branch_name(const char **string, int *len)
 {
@@ -270,21 +256,6 @@ static char *substitute_branch_name(const char **string, int *len)
 		return (char *)*string;
 	}
 
-	ret = tracked_suffix(*string, *len);
-	if (ret) {
-		char *ref = xstrndup(*string, *len - ret);
-		struct branch *tracking = branch_get(*ref ? ref : NULL);
-
-		if (!tracking)
-			die ("No tracking branch found for '%s'", ref);
-		free(ref);
-		if (tracking->merge && tracking->merge[0]->dst) {
-			*string = xstrdup(tracking->merge[0]->dst);
-			*len = strlen(*string);
-			return (char *)*string;
-		}
-	}
-
 	return NULL;
 }
 
@@ -354,6 +325,20 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 	return logs_found;
 }
 
+static inline int upstream_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{upstream}", "@{u}" };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
+		int suffix_len = strlen(suffix[i]);
+		if (suffix_len <= len
+		    && !memcmp(string, suffix[i], suffix_len))
+			return suffix_len;
+	}
+	return 0;
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
@@ -371,7 +356,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && str[len-1] == '}') {
 		for (at = len-2; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
-				if (!tracked_suffix(str + at, len - at)) {
+				if (!upstream_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -773,17 +758,10 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 }
 
 /*
- * This reads "@{-N}" syntax, finds the name of the Nth previous
- * branch we were on, and places the name of the branch in the given
- * buf and returns the number of characters parsed if successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
+ * Parse @{-N} syntax, return the number of characters parsed
+ * if successful; otherwise signal an error with negative value.
  */
-int interpret_branch_name(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
 {
 	long nth;
 	int i, retval;
@@ -828,6 +806,60 @@ release_return:
 }
 
 /*
+ * This reads short-hand syntax that not only evaluates to a commit
+ * object name, but also can act as if the end user spelled the name
+ * of the branch from the command line.
+ *
+ * - "@{-N}" finds the name of the Nth previous branch we were on, and
+ *   places the name of the branch in the given buf and returns the
+ *   number of characters parsed if successful.
+ *
+ * - "<branch>@{upstream}" finds the name of the other ref that
+ *   <branch> is configured to merge with (missing <branch> defaults
+ *   to the current branch), and places the name of the branch in the
+ *   given buf and returns the number of characters parsed if
+ *   successful.
+ *
+ * If the input is not of the accepted format, it returns a negative
+ * number to signal an error.
+ *
+ * If the input was ok but there are not N branch switches in the
+ * reflog, it returns 0.
+ */
+int interpret_branch_name(const char *name, struct strbuf *buf)
+{
+	char *cp;
+	struct branch *upstream;
+	int namelen = strlen(name);
+	int len = interpret_nth_prior_checkout(name, buf);
+	int tmp_len;
+
+	if (!len)
+		return len; /* syntax Ok, not enough switches */
+	if (0 < len)
+		return len; /* consumed from the front */
+	cp = strchr(name, '@');
+	if (!cp)
+		return -1;
+	tmp_len = upstream_mark(cp, namelen - (cp - name));
+	if (!tmp_len)
+		return -1;
+	len = cp + tmp_len - name;
+	cp = xstrndup(name, cp - name);
+	upstream = branch_get(*cp ? cp : NULL);
+	if (!upstream
+	    || !upstream->merge
+	    || !upstream->merge[0]->dst)
+		return error("No upstream branch found for '%s'", cp);
+	free(cp);
+	cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
+	strbuf_reset(buf);
+	strbuf_addstr(buf, cp);
+	free(cp);
+	return len;
+}
+
+/*
  * This is like "get_sha1_basic()", except it allows "sha1 expressions",
  * notably "xyz^" for "parent of xyz"
  */
diff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh
index a2c7f92..95c9b09 100755
--- a/t/t1506-rev-parse-upstream.sh
+++ b/t/t1506-rev-parse-upstream.sh
@@ -76,7 +76,7 @@ test_expect_success 'checkout -b new my-side@{u} forks from the same' '
 )
 '
 
-test_expect_failure 'merge my-side@{u} records the correct name' '
+test_expect_success 'merge my-side@{u} records the correct name' '
 (
 	sq="'\''" &&
 	cd clone || exit
@@ -90,7 +90,7 @@ test_expect_failure 'merge my-side@{u} records the correct name' '
 )
 '
 
-test_expect_failure 'branch -d other@{u}' '
+test_expect_success 'branch -d other@{u}' '
 	git checkout -t -b other master &&
 	git branch -d @{u} &&
 	git for-each-ref refs/heads/master >actual &&
@@ -98,7 +98,7 @@ test_expect_failure 'branch -d other@{u}' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'checkout other@{u}' '
+test_expect_success 'checkout other@{u}' '
 	git branch -f master HEAD &&
 	git checkout -t -b another master &&
 	git checkout @{u} &&
-- 
1.6.6.513.g63f4c

Re: [PATCH 0/2] @{u} updates

[Johannes Schindelin]

Hi,

On Wed, 20 Jan 2010, Junio C Hamano wrote:

> Earlier I wondered if the approach Dscho's patch takes to teach the new
> @{upstream} syntax to substitute_branch_name() (hence dwim_ref()) without
> teaching it to interpret_branch_name() (hence strbuf_branchname()) was a
> bad idea.  I thought about this a bit more; there are some downsides for
> not doing so.
> 
> The first patch adds a handful of tests that show why strbuf_branchname()
> callers may also want to learn about the new syntax.  The second patch
> moves the logic to interpret_branch_name() to make them happier.

Looks good to me.

Ciao,
Dscho

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

[Jeff King]

On Wed, Jan 20, 2010 at 01:38:41AM -0800, Junio C Hamano wrote:

> This adds a few more tests that exercises @{upstream} syntax by commands
> that operate differently when they are given branch name as opposed to a
> refname (i.e. where "master" and "refs/heads/master" makes a difference).

Overall this looks good, but there are a few minor defects. I haven't
had a chance to fix them yet, but here are tests showing them. I hope to
get to them pre-1.7.0, but please feel free to take a crack at them if
you want.

The first one is that @{usptream} silently becomes @{0}. I think
we need to double-check whether approxidate found absolutely nothing,
and complain if that is the case.

diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
new file mode 100755
index 0000000..da43386
--- /dev/null
+++ b/t/t0101-at-syntax.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='various @{whatever} syntax tests'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two
+'
+
+check_at() {
+	echo "$2" >expect &&
+	git log -1 --format=%s "$1" >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success '@{0} shows current' '
+	check_at @{0} two
+'
+
+test_expect_success '@{1} shows old' '
+	check_at @{1} one
+'
+
+test_expect_success '@{now} shows current' '
+	check_at @{now} two
+'
+
+test_expect_success '@{30.years.ago} shows old' '
+	check_at @{30.years.ago} one
+'
+
+test_expect_success 'silly approxidates work' '
+	check_at @{3.hot.dogs.and.30.years.ago} one
+'
+
+test_expect_failure 'complain about total nonsense' '
+	test_must_fail git log -1 --format=%s @{utter.bogosity}
+'
+
+test_done

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

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 95c9b09..cbe1b25 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -107,4 +107,18 @@ test_expect_success 'checkout other@{u}' '
 	test_cmp expect actual
 '
 
+cat >expect <<EOF
+commit 8f489d01d0cc65c3b0f09504ec50b5ed02a70bd5
+Reflog: refs/heads/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_failure 'log -g other@{u}' '
+	git log -1 -g other@{u} >actual &&
+	test_cmp expect actual
+'
+
 test_done

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The first one is that @{usptream} silently becomes @{0}. I think
> we need to double-check whether approxidate found absolutely nothing,
> and complain if that is the case.

This is not a new problem introduced by Dscho's @{u} series; it was there
even before 861f00e (fix reflog approxidate parsing bug, 2008-04-30).

Nevertheless, it would be nice to fix it.

-- >8 --
Subject: approxidate_careful() reports errorneous date string

For a long time, the time based reflog syntax (e.g. master@{yesterday})
didn't complain when the "human readable" timestamp was misspelled, as
the underlying mechanism tried to be as lenient as possible.  The funny
thing was that parsing of "@{now}" even relied on the fact that anything
not recognized by the machinery returned the current timestamp.

Introduce approxidate_careful() that takes an optional pointer to an
integer, that gets assigned 1 when the input does not make sense as a
timestamp.

As I am too lazy to fix all the callers that use approxidate(), most of
the callers do not take advantage of the error checking, but convert the
code to parse reflog to use it as a demonstration.

Tests are mostly from Jeff King.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h              |    3 ++-
 date.c               |   43 +++++++++++++++++++++++++++++++++++--------
 sha1_name.c          |    5 ++++-
 t/t0101-at-syntax.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index b3370eb..f0fea2d 100644
--- a/cache.h
+++ b/cache.h
@@ -762,7 +762,8 @@ const char *show_date_relative(unsigned long time, int tz,
 			       size_t timebuf_size);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
-unsigned long approxidate(const char *);
+#define approxidate(s) approxidate_careful(s, NULL)
+unsigned long approxidate_careful(const char *, int *);
 unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
diff --git a/date.c b/date.c
index 45f3684..002aa3c 100644
--- a/date.c
+++ b/date.c
@@ -696,6 +696,11 @@ static unsigned long update_tm(struct tm *tm, struct tm *now, unsigned long sec)
 	return n;
 }
 
+static void date_now(struct tm *tm, struct tm *now, int *num)
+{
+	update_tm(tm, now, 0);
+}
+
 static void date_yesterday(struct tm *tm, struct tm *now, int *num)
 {
 	update_tm(tm, now, 24*60*60);
@@ -770,6 +775,7 @@ static const struct special {
 	{ "PM", date_pm },
 	{ "AM", date_am },
 	{ "never", date_never },
+	{ "now", date_now },
 	{ NULL }
 };
 
@@ -790,7 +796,7 @@ static const struct typelen {
 	{ NULL }
 };
 
-static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm *now, int *num)
+static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm *now, int *num, int *touched)
 {
 	const struct typelen *tl;
 	const struct special *s;
@@ -804,6 +810,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 		int match = match_string(date, month_names[i]);
 		if (match >= 3) {
 			tm->tm_mon = i;
+			*touched = 1;
 			return end;
 		}
 	}
@@ -812,6 +819,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 		int len = strlen(s->name);
 		if (match_string(date, s->name) == len) {
 			s->fn(tm, now, num);
+			*touched = 1;
 			return end;
 		}
 	}
@@ -821,11 +829,14 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 			int len = strlen(number_name[i]);
 			if (match_string(date, number_name[i]) == len) {
 				*num = i;
+				*touched = 1;
 				return end;
 			}
 		}
-		if (match_string(date, "last") == 4)
+		if (match_string(date, "last") == 4) {
 			*num = 1;
+			*touched = 1;
+		}
 		return end;
 	}
 
@@ -835,6 +846,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 		if (match_string(date, tl->type) >= len-1) {
 			update_tm(tm, now, tl->length * *num);
 			*num = 0;
+			*touched = 1;
 			return end;
 		}
 		tl++;
@@ -852,6 +864,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 			diff += 7*n;
 
 			update_tm(tm, now, diff * 24 * 60 * 60);
+			*touched = 1;
 			return end;
 		}
 	}
@@ -866,6 +879,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 			tm->tm_year--;
 		}
 		tm->tm_mon = n;
+		*touched = 1;
 		return end;
 	}
 
@@ -873,6 +887,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 		update_tm(tm, now, 0); /* fill in date fields if needed */
 		tm->tm_year -= *num;
 		*num = 0;
+		*touched = 1;
 		return end;
 	}
 
@@ -929,9 +944,12 @@ static void pending_number(struct tm *tm, int *num)
 	}
 }
 
-static unsigned long approxidate_str(const char *date, const struct timeval *tv)
+static unsigned long approxidate_str(const char *date,
+				     const struct timeval *tv,
+				     int *error_ret)
 {
 	int number = 0;
+	int touched = 0;
 	struct tm tm, now;
 	time_t time_sec;
 
@@ -951,33 +969,42 @@ static unsigned long approxidate_str(const char *date, const struct timeval *tv)
 		if (isdigit(c)) {
 			pending_number(&tm, &number);
 			date = approxidate_digit(date-1, &tm, &number);
+			touched = 1;
 			continue;
 		}
 		if (isalpha(c))
-			date = approxidate_alpha(date-1, &tm, &now, &number);
+			date = approxidate_alpha(date-1, &tm, &now, &number, &touched);
 	}
 	pending_number(&tm, &number);
+	if (!touched)
+		*error_ret = 1;
 	return update_tm(&tm, &now, 0);
 }
 
 unsigned long approxidate_relative(const char *date, const struct timeval *tv)
 {
 	char buffer[50];
+	int errors = 0;
 
 	if (parse_date(date, buffer, sizeof(buffer)) > 0)
 		return strtoul(buffer, NULL, 0);
 
-	return approxidate_str(date, tv);
+	return approxidate_str(date, tv, &errors);
 }
 
-unsigned long approxidate(const char *date)
+unsigned long approxidate_careful(const char *date, int *error_ret)
 {
 	struct timeval tv;
 	char buffer[50];
+	int dummy = 0;
+	if (!error_ret)
+		error_ret = &dummy;
 
-	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+	if (parse_date(date, buffer, sizeof(buffer)) > 0) {
+		*error_ret = 0;
 		return strtoul(buffer, NULL, 0);
+	}
 
 	gettimeofday(&tv, NULL);
-	return approxidate_str(date, &tv);
+	return approxidate_str(date, &tv, error_ret);
 }
diff --git a/sha1_name.c b/sha1_name.c
index 9215ad1..ed4c028 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		} else if (0 <= nth)
 			at_time = 0;
 		else {
+			int errors = 0;
 			char *tmp = xstrndup(str + at + 2, reflog_len);
-			at_time = approxidate(tmp);
+			at_time = approxidate_careful(tmp, &errors);
+			if (errors)
+				die("Bogus timestamp '%s'", tmp);
 			free(tmp);
 		}
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
new file mode 100755
index 0000000..ccabc37
--- /dev/null
+++ b/t/t0101-at-syntax.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='various @{whatever} syntax tests'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two
+'
+
+check_at() {
+	echo "$2" >expect &&
+	git log -1 --format=%s "$1" >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success '@{0} shows current' '
+	check_at @{0} two
+'
+
+test_expect_success '@{1} shows old' '
+	check_at @{1} one
+'
+
+test_expect_success '@{now} shows current' '
+	check_at @{now} two
+'
+
+test_expect_success '@{30.years.ago} shows old' '
+	check_at @{30.years.ago} one
+'
+
+test_expect_success 'silly approxidates work' '
+	check_at @{3.hot.dogs.and.30.years.ago} one
+'
+
+test_expect_success 'notice misspelled upstream' '
+	test_must_fail git log -1 --format=%s @{usptream}
+'
+
+test_expect_success 'complain about total nonsense' '
+	test_must_fail git log -1 --format=%s @{utter.bogosity}
+'
+
+test_done

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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

[Jeff King]

On Tue, Jan 26, 2010 at 11:58:00AM -0800, Junio C Hamano wrote:

> > The first one is that @{usptream} silently becomes @{0}. I think
> > we need to double-check whether approxidate found absolutely nothing,
> > and complain if that is the case.
> 
> This is not a new problem introduced by Dscho's @{u} series; it was there
> even before 861f00e (fix reflog approxidate parsing bug, 2008-04-30).

True, though now that there is something besides an approxidate to
misspell, it is slightly worse. :)

> Introduce approxidate_careful() that takes an optional pointer to an
> integer, that gets assigned 1 when the input does not make sense as a
> timestamp.

A minor nit, but wouldn't:

  int approxidate_careful(const char *str, unsigned long *out);

returning an error code be the more usual pattern for a function with
error-plus-output (your approxidate wrapper would have to be a function then,
not a macro)?

> As I am too lazy to fix all the callers that use approxidate(), most of
> the callers do not take advantage of the error checking, but convert the
> code to parse reflog to use it as a demonstration.

I think that is fine, and was the approach I was also going to take.
Approxidate was meant to be "try to make sense of anything". It is
mainly a problem here because we are combining it with other input, and
it is hard to tell a misspelling of that other input from a nonsensical
approxidate. Now that the _careful infrastructure is there, we can fix
other callsites if people care.

> @@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  		} else if (0 <= nth)
>  			at_time = 0;
>  		else {
> +			int errors = 0;
>  			char *tmp = xstrndup(str + at + 2, reflog_len);
> -			at_time = approxidate(tmp);
> +			at_time = approxidate_careful(tmp, &errors);
> +			if (errors)
> +				die("Bogus timestamp '%s'", tmp);
>  			free(tmp);

I was just going to "return -1" here, which yields:

  $ git show @{bogosity}
  fatal: ambiguous argument '@{bogosity}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

instead of

  $ git show @{bogosity}
  fatal: Bogus timestamp 'bogosity'

I can't imagine anybody being upset that they had a path '@{usptream}'
in their repository which was prematurely interpreted as a bogus ref
(especially since it is _always_ a ref in the current behavior), but it
just seemed more consistent with the rest of get_sha1_basic, which
generally does not die.

On the other hand, I think the error message the user sees in your case
is probably more helpful.

> +test_expect_success '@{30.years.ago} shows old' '
> +	check_at @{30.years.ago} one

Side note: I chose this because we needed to go back from the current
time beyond where test_tick would place the commit. Which means this
test has a 2035 bug. :)

I had originally had 100.years.ago, but we seem to have some data-type
issues with our date handling. We represent seconds-since-epoch as
unsigned long, which means:

  - even though approxidate handles it internally, we can't represent
    dates earlier than the epoch. If you simply store approxidate's
    output as a signed time_t, as test-date does, it does work back to
    1901. But the reflog code treats it as unsigned, so 100.years.ago,
    though representable, is considered to be far in the future.

  - on many platforms ulong is 32 bits, meaning we have 2038 problems
    (though because it's unsigned, I am not sure if we actually have
    2106 problems; I suspect it may be a mix because of the way
    different parts of the system use time_t versus ulong). Presumably
    time_t will move to 64 bits in the next decade or two, and hopefully
    ulong along with it.

Now obviously neither is a pressing issue. Probably nobody is importing
any pre-epoch commits, and we have a few decades to worry about future
issues. But I did legitimately see the bug when trying to guess a "long
time ago" value. It might be nice to use a signed time_t, and to deal
with the overflow (even if it is just to barf and say "bad time" instead
of silently producing wrong, future results).

-Peff

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

[Jeff King]

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;

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> A minor nit, but wouldn't:
>
>   int approxidate_careful(const char *str, unsigned long *out);
>
> returning an error code be the more usual pattern for a function with
> error-plus-output (your approxidate wrapper would have to be a function then,
> not a macro)?

I don't have strong preference either way; the one in the patch was
modelled after setup_git_directory_gently(&nongit_ok), and slightly easier
to work with for existing callers that don't care enough.

>> @@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>  		} else if (0 <= nth)
>>  			at_time = 0;
>>  		else {
>> +			int errors = 0;
>>  			char *tmp = xstrndup(str + at + 2, reflog_len);
>> -			at_time = approxidate(tmp);
>> +			at_time = approxidate_careful(tmp, &errors);
>> +			if (errors)
>> +				die("Bogus timestamp '%s'", tmp);
>>  			free(tmp);
>
> I was just going to "return -1" here, which yields:
>
>   $ git show @{bogosity}
>   fatal: ambiguous argument '@{bogosity}': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions
>
> instead of
>
>   $ git show @{bogosity}
>   fatal: Bogus timestamp 'bogosity'

Good point.  Let's change it to silently return -1 and let the caller take
care of it.  Perhaps there are some callers that say "does this name an
object?  If not, let's try pathname".

>> +test_expect_success '@{30.years.ago} shows old' '
>> +	check_at @{30.years.ago} one
>
> Side note: I chose this because we needed to go back from the current
> time beyond where test_tick would place the commit. Which means this
> test has a 2035 bug. :)

Can't we use an absolute date, given that test_tick gives fixed timestamp
sequence to pretend as if we were still in 2005 when we are running these
tests?

 sha1_name.c          |    4 ++--
 t/t0101-at-syntax.sh |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index f4a74fe..04fb3b8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -398,9 +398,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 			int errors = 0;
 			char *tmp = xstrndup(str + at + 2, reflog_len);
 			at_time = approxidate_careful(tmp, &errors);
-			if (errors)
-				die("Bogus timestamp '%s'", tmp);
 			free(tmp);
+			if (errors)
+				return -1;
 		}
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
index ccabc37..5e298c5 100755
--- a/t/t0101-at-syntax.sh
+++ b/t/t0101-at-syntax.sh
@@ -26,8 +26,8 @@ test_expect_success '@{now} shows current' '
 	check_at @{now} two
 '
 
-test_expect_success '@{30.years.ago} shows old' '
-	check_at @{30.years.ago} one
+test_expect_success '@{2001-09-17} (before the first commit) shows old' '
+	check_at @{2001-09-17} one
 '
 
 test_expect_success 'silly approxidates work' '

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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}").

I wanted to do something like what your patch does by iterating over the
input inside get_sha1_basic() while we still see @{...}, parsing pieces
from the beginning, not from the end like the original "do we have the
reflog indicator at the end?  If so strip it and deal with what we have at
the front".  Your patch to i-b-n does that by recursing inside, which is a
nice solution.

Care to roll a patch with additional tests, to build on top of 105e473
(Fix log -g this@{upstream}, 2010-01-26)?

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

[Jeff King]

On Wed, Jan 27, 2010 at 10:50:07AM -0800, Junio C Hamano wrote:

> > A minor nit, but wouldn't:
> >
> >   int approxidate_careful(const char *str, unsigned long *out);
> >
> > returning an error code be the more usual pattern for a function with
> > error-plus-output (your approxidate wrapper would have to be a function then,
> > not a macro)?
> 
> I don't have strong preference either way; the one in the patch was
> modelled after setup_git_directory_gently(&nongit_ok), and slightly easier
> to work with for existing callers that don't care enough.

Looks like you have already pushed out the original patch, so let's not
worry about it.

> >> +test_expect_success '@{30.years.ago} shows old' '
> >> +	check_at @{30.years.ago} one
> >
> > Side note: I chose this because we needed to go back from the current
> > time beyond where test_tick would place the commit. Which means this
> > test has a 2035 bug. :)
> 
> Can't we use an absolute date, given that test_tick gives fixed timestamp
> sequence to pretend as if we were still in 2005 when we are running these
> tests?
> [...]
> --- a/t/t0101-at-syntax.sh
> +++ b/t/t0101-at-syntax.sh
> @@ -26,8 +26,8 @@ test_expect_success '@{now} shows current' '
>  	check_at @{now} two
>  '
>  
> -test_expect_success '@{30.years.ago} shows old' '
> -	check_at @{30.years.ago} one
> +test_expect_success '@{2001-09-17} (before the first commit) shows old' '
> +	check_at @{2001-09-17} one
>  '
>  
>  test_expect_success 'silly approxidates work' '

Yes, I don't know why I was so concerned with using a relative
approxidate when an absolute one would suffice. However, we should make
a matching change in the silly approxidate entry, too. Like this:

-- >8 --
Subject: [PATCH] t0101: use absolute date

The original version used relative approxidates, which don't
reproduce as reliably as absolute ones. Commit 6c647a fixed
this for one case, but missed the "silly" case.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0101-at-syntax.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
index 5e298c5..a1998b5 100755
--- a/t/t0101-at-syntax.sh
+++ b/t/t0101-at-syntax.sh
@@ -31,7 +31,7 @@ test_expect_success '@{2001-09-17} (before the first commit) shows old' '
 '
 
 test_expect_success 'silly approxidates work' '
-	check_at @{3.hot.dogs.and.30.years.ago} one
+	check_at @{3.hot.dogs.on.2001-09-17} one
 '
 
 test_expect_success 'notice misspelled upstream' '

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

[Jeff King]

On Wed, Jan 27, 2010 at 11:10:21AM -0800, Junio C Hamano wrote:

> I wanted to do something like what your patch does by iterating over the
> input inside get_sha1_basic() while we still see @{...}, parsing pieces
> from the beginning, not from the end like the original "do we have the
> reflog indicator at the end?  If so strip it and deal with what we have at
> the front".  Your patch to i-b-n does that by recursing inside, which is a
> nice solution.

Yeah, I wanted to do that too, but it just ended up very messy. I
suppose the i-b-n solution is reasonably elegant, and it should
correctly handle non-get-sha1 instances like:

  git checkout @{-1}@{u}

> Care to roll a patch with additional tests, to build on top of 105e473
> (Fix log -g this@{upstream}, 2010-01-26)?

Yep, series to follow:

  [1/3]: test combinations of @{} syntax
  [2/3]: fix parsing of @{-1}@{u} combination
  [3/3]: reject @{-1} not at beginning of object name

-Peff

[PATCH 1/3] test combinations of @{} syntax

[Jeff King]

Now that we have several different types of @{} syntax, it
is a good idea to test them together, which reveals some
failures.

Signed-off-by: Jeff King <peff@peff.net>
---
While this is conceptually similar to the tests in t0101, it feels
better to me to test features in combination only after they have been
tested by themselves. Thus a new script > 1507.

 t/t1508-at-combinations.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100755 t/t1508-at-combinations.sh

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
new file mode 100755
index 0000000..59f0463
--- /dev/null
+++ b/t/t1508-at-combinations.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+
+test_description='test various @{X} syntax combinations together'
+. ./test-lib.sh
+
+check() {
+test_expect_${3:-success} "$1 = $2" "
+	echo '$2' >expect &&
+	git log -1 --format=%s '$1' >actual &&
+	test_cmp expect actual
+"
+}
+nonsense() {
+test_expect_${2:-success} "$1 is nonsensical" "
+	test_must_fail git log -1 '$1'
+"
+}
+fail() {
+	"$@" failure
+}
+
+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 &&
+	test_commit old-one &&
+	test_commit old-two &&
+	git checkout -b new-branch &&
+	test_commit new-one &&
+	test_commit new-two &&
+	git config branch.old-branch.remote . &&
+	git config branch.old-branch.merge refs/heads/master &&
+	git config branch.new-branch.remote . &&
+	git config branch.new-branch.merge refs/heads/upstream-branch
+'
+
+check HEAD new-two
+check "@{1}" new-one
+check "@{-1}" old-two
+check "@{-1}@{1}" old-one
+check "@{u}" upstream-two
+check "@{u}@{1}" upstream-one
+fail check "@{-1}@{u}" master-two
+fail check "@{-1}@{u}@{1}" master-one
+fail nonsense "@{u}@{-1}"
+nonsense "@{1}@{u}"
+
+test_done
-- 
1.7.0.rc0.41.g538720

[PATCH 2/3] fix parsing of @{-1}@{u} combination

[Jeff King]

Previously interpret_branch_name would see @{-1} and stop
parsing, leaving the @{u} as cruft that provoked an error.
Instead, we should recurse if there is more to parse.

Signed-off-by: Jeff King <peff@peff.net>
---
A straight repost of the previous "how about this" patch, but marking
successful tests.

 sha1_name.c                |   24 ++++++++++++++++++++++--
 t/t1508-at-combinations.sh |    4 ++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7f1510..00fc415 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;
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 59f0463..2a46af2 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -43,8 +43,8 @@ check "@{-1}" old-two
 check "@{-1}@{1}" old-one
 check "@{u}" upstream-two
 check "@{u}@{1}" upstream-one
-fail check "@{-1}@{u}" master-two
-fail check "@{-1}@{u}@{1}" master-one
+check "@{-1}@{u}" master-two
+check "@{-1}@{u}@{1}" master-one
 fail nonsense "@{u}@{-1}"
 nonsense "@{1}@{u}"
 
-- 
1.7.0.rc0.41.g538720

[PATCH 3/3] reject @{-1} not at beginning of object name

[Jeff King]

Something like foo@{-1} is nonsensical, as the @{-N} syntax
is reserved for "the Nth last branch", and is not an actual
reflog selector. We should not feed such nonsense to
approxidate at all.

Signed-off-by: Jeff King <peff@peff.net>
---
We didn't discuss this one, but I came across it while trying to be
complete in testing the combinations. Right now "foo@{-1}" is
interpreted as a reflog entry at approxidate "-1". Approxidate doesn't
signal an error because it thinks it has found something useful. But
AFAIK we have declared all @{-...} to be Nth last branch, so it is
simply a semantic error.

Let me know if that is not the case (that is, if it was intentional to
leave foo@{-1} as the reflog at date "-1" because it has some meaning
that I am missing) and we can drop this patch.

 sha1_name.c                |    4 ++++
 t/t1508-at-combinations.sh |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 00fc415..7729925 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -399,6 +399,10 @@ 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];
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 2a46af2..d5d6244 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -45,7 +45,7 @@ check "@{u}" upstream-two
 check "@{u}@{1}" upstream-one
 check "@{-1}@{u}" master-two
 check "@{-1}@{u}@{1}" master-one
-fail nonsense "@{u}@{-1}"
+nonsense "@{u}@{-1}"
 nonsense "@{1}@{u}"
 
 test_done
-- 
1.7.0.rc0.41.g538720

Re: [PATCH 3/3] reject @{-1} not at beginning of object name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Something like foo@{-1} is nonsensical, as the @{-N} syntax
> is reserved for "the Nth last branch", and is not an actual
> reflog selector. We should not feed such nonsense to
> approxidate at all.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> We didn't discuss this one, but I came across it while trying to be
> complete in testing the combinations. Right now "foo@{-1}" is
> interpreted as a reflog entry at approxidate "-1". Approxidate doesn't
> signal an error because it thinks it has found something useful. But
> AFAIK we have declared all @{-...} to be Nth last branch, so it is
> simply a semantic error.
>
> Let me know if that is not the case (that is, if it was intentional to
> leave foo@{-1} as the reflog at date "-1" because it has some meaning
> that I am missing) and we can drop this patch.

I think the patch is fine as is.

We might want to use @{-some string that has non digit} for other purposes
and it may be a safer change to tweak the "do we only have digits" check
in the post-context to detect and reject only @{-<all digits>}.

But what I am puzzled by the code structure of get_sha1_basic(), which
looks like this:

    get_sha1_basic() {

     - do we have @{...} at end?  If so, and if it is not a magic like
       @{u}, set "len" (points at the end of stuff that should name a ref)
       and "reflog_len" (the length of the reflog time/num specifier that
       is applied to that ref).

     - did we find @{...} in the above check, and is it at the beginning?
       Then it is not a reflog syntax, but is a N-th branch switch.
       Substitute that @{...} with the real refname and retry.  If it is
       not @{-N}, then that @{...} reflog derefence should apply to the
       current branch, so set it to real_ref and go to "reflog" part.

     - if we have @{...} at the end, get the canonical name of the ref the
       reflog notation is applied to.

     - otherwise, get the canonical name of the ref; in this case,
       there is no @{...} at the end, si this is what is eventually
       returned.

     - "reflog" part:
       by now, real_ref holds the ref @{...} is being applied to.  Read
       from its reflog.

   }

And the place that parses @{-1} and @{u} are different, even though both
dwim_log() called by the third one and dwim_ref() called by the fourth one
call substitute_branch_name() and they are perfectly capable of resolving
@{-1} and @{u} (and even nested stuff like @{-1}@{u}@{u} with your patch).

But somehow we kept the special case code to parse @{-1} in the second
one.

    Side note.  I am wondering if dwim_log()'s current implementation is
    even correct in the first place.  When you have two "ambiguous" refs,
    it appears to me that you will get a warning from dwim_ref(), but if
    only one of them has a reflog associated with it, dwim_log() won't
    complain.  Why isn't the function be (1) dwim_ref() to find the ref
    from abbreviated refname given in str; and then (2) check if the log
    exists for that ref?

It might be cleaner if the logic went like this instead:

    - find the last @{...} in the string that is not what i-b-n should
      resolve (i.e. @{-1} and @{u}); that is @{time/num} reflog
      reference.  You can have at most one reflog reference and it always
      has to come at the end.

    - feed the remainder to (an updated) i-b-n that knows how to grok:

      - @{-N} is nth-priour checkout; it has to come at the beginning and
        you can have at most one.  If you find it, substitute it with the
        real branch name and continue. (e.g. @{-1}@{u} becomes master@{u})

      - does it begin with @{...}?  If not, the part before @{...} is the
        name of the ref (e.g. "next" in "next@{u}@{u}") the later magic
        sequence (e.g. "@{u}@{u}") is applied to; otherwise apply the
        magic to what HEAD points at (e.g. "@{u}" applies @{u} to the
        current branch).  Remember that ref and strip it away from the
        input.

      - while we see sequence of @{...}, apply the magic to the ref
        repeatedly (e.g. "next@{u}@{u}" has remembered "refs/heads/next" in
        the previous step, and @{u} is applied to produce "master" if next
        follows master, and then applying @{u} to that result will tell us
        that it follows "refs/remotes/origin/master").

    - now we have what ref the caller was talking about with the beginning
      part of the input (i.e. without "@{time/num}" at the end).  If we
      had @{time/num} in the original input, open the reflog and find an
      appropriate entry in it.  Otherwise what we received from (the
      updated) i-b-n is what we found.  Find out what object the ref
      points at.

But that is a kind of code churn that may not be worth doing.  I dunno.

Re: [PATCH 3/3] reject @{-1} not at beginning of object name

[Jeff King]

On Thu, Jan 28, 2010 at 12:02:53PM -0800, Junio C Hamano wrote:

> We might want to use @{-some string that has non digit} for other purposes
> and it may be a safer change to tweak the "do we only have digits" check
> in the post-context to detect and reject only @{-<all digits>}.

I considered that, but I didn't think it was really worth it. If we
later want to make @{-foobar} meaningful, we can loosen the safety check
then.

> But what I am puzzled by the code structure of get_sha1_basic(), which
> looks like this:

You are not the only one who is puzzled. :)

But yes, your analysis of what is there now looks right to me.

> And the place that parses @{-1} and @{u} are different, even though both
> dwim_log() called by the third one and dwim_ref() called by the fourth one
> call substitute_branch_name() and they are perfectly capable of resolving
> @{-1} and @{u} (and even nested stuff like @{-1}@{u}@{u} with your patch).

Ooh, gross. I didn't try @{u}@{u} in my tests, but it should work.

>     Side note.  I am wondering if dwim_log()'s current implementation is
>     even correct in the first place.  When you have two "ambiguous" refs,
>     it appears to me that you will get a warning from dwim_ref(), but if
>     only one of them has a reflog associated with it, dwim_log() won't
>     complain.  Why isn't the function be (1) dwim_ref() to find the ref
>     from abbreviated refname given in str; and then (2) check if the log
>     exists for that ref?

I guess the original rationale was that you might have reflog'd one, so
by asking for "foo@{yesterday}" you are disambiguating as "the one with
a reflog". But that seems kind of useless to me since:

  1. It is somewhat error-prone, as it assumes that from the user's
     perspective, the fact that one ref has a log and the other does not
     is somehow a meaningful disambiguation. Which implies that users
     carefully figure out which refs have reflogs and which do not, and
     I don't think that is true.

  2. For quite a while, we have had logallrefupdates on by default (and
     I don't remember the exact semantics before that, but wasn't it
     enough to simply create a "logs" directory, which meant that you
     either logged everything or nothing?). So I don't even know how you
     would get into a situation where one ref has a log and the other
     does not.

In other words, I totally agree with your statement, and we could
probably just drop the dwim_log code.

> It might be cleaner if the logic went like this instead:

Your logic makes sense to me. I think we could also simply do a
left-to-right parse, eating refs, @{-N}, and @{u} as we go and
converting them into a "real ref". If we get to something else, we stop.
If we have a @{} left, it's a reflog.  Otherwise, it's bogus (I think ^
suffixes and such have already been stripped off at this point).

Interpret_branch_name already does most of the "eating..." part above
(and it needs to remain separate from get_sha1_basic, as things like
"checkout" need to use it directly).

But I didn't really look too hard at it, as:

> But that is a kind of code churn that may not be worth doing.  I dunno.

Yeah, the code was sufficiently nasty and sufficiently core that I
didn't really want to risk breaking it for the sake of cleanup
(especially not during the -rc cycle).

-Peff