Re: [PATCH v2] Introduce <branch>@{upstream} as shortcut to the tracked branch

From: Jeff King <peff@peff.net>
Date: Sep 10, 2009, 15:55 UTC
Message-ID: <20090910155547.GA12409@coredump.intra.peff.net>
In-Reply-To: <alpine.DEB.1.00.0909101724520.8306@pacific.mpi-cbg.de>

On Thu, Sep 10, 2009 at 05:25:57PM +0200, Johannes Schindelin wrote:

> 	Changes since v1:
> 	- changed to @{upstream} (and @{u})

Hmm. After applying v2, I accidentally tried "git rev-parse @{t}" and was surprised to find that it worked! The problem, of course, is that we saw that it was not one of our keywords and therefore dumped it into approxidate, which happily converted it into the current time, and provided me with HEAD@{now}.

Which is sad, because it means "HEAD@{usptream}" will silently produce incorrect results.

I don't see a way around it, though, short of tightening approxidate's parsing. Maybe it could keep a flag for "I noticed _anything_ of value in this string", and we could barf if it isn't set. That would still allow arbitrary stuff like:

  the 6th of july

and keep Linus' favorite

  I ate 6 hot dogs in July.

but disallow the most obviously wrong dates like:

  t
  usptream
  total bogosity

Show 15 quoted lines

> +	ret = tracked_suffix(*string, *len);
> +	if (ret) {
> +		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);
> +		if (tracking->merge && tracking->merge[0]->dst) {
> +			*string = xstrdup(tracking->merge[0]->dst);
> +			*len = strlen(*string);
> +			return (char *)*string;
> +		}
> +	}
> +

I don't think it is a good idea to die for !tracking, but not for !tracking->merge. That leads to inconsistent user-visible results:

  $ git checkout HEAD^0
  $ git rev-parse HEAD@{u}
  fatal: No tracking branch found for 'HEAD'
  $ git rev-parse bogus@{u}
  bogus@{u}
  fatal: ambiguous argument 'bogus@{u}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

Shouldn't both cases say the same thing?

Also, your die message has two problems:

 1. It looks at ref immediately after it is free'd, spewing junk.

 2. Ref can be the empty string, which gives you the ugly:

       fatal: No tracking branch found for ''

    Should we munge that into HEAD (or "the current branch") for the
    user?

-Peff