Re: [PATCH v3] Add new @ shortcut for HEAD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 1, 2013 at 12:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>
>> So HEAD@{0}~0^0 is too much to type, but we can remove '^0', and we can
>> remove '~0', and we can remove 'HEAD', which leaves us with @{0}, but we
>> can't remove '{0}'?
>>
>> This patch allows '@' to be the same as 'HEAD'.
>
> While the above reasoning is cute, it is misleading.
>
> If you start from HEAD@{1}~0^0, we can remove '^0', we can remove
> '~0', but you cannot remove HEAD from the remaining "HEAD@{1}"
> without changing what it means.  @{1} is where the current branch
> was, while HEAD@{1} is where you were---they are different when you
> have just did "git checkout anotherbranch".  HEAD@{1} is the tip of
> your previous branch, @{1} is where anotherbranch was before its tip
> became the commit you have checked out.

Replace @{1} with @{u} and it holds.

> You have to be specially talking about "HEAD@{0}" as a whole for
> that reasoning to hold; it does not work for HEAD@{$n} for an
> arbitrary value of $n.
>
> So I'd suggest toning it down, perhaps something like this:
>
>         Even though we often can do without having to type "HEAD",
>         e.g. "git log origin.." substitutes missing RHS with "HEAD",
>         sometimes we still do need to type "HEAD" (thats six f*cking
>         keystrokes "Caps Lock", "H", "E", "A", "D" and finally "Caps
>         Lock").

I don't know what RHS means, and I don't use caps lock :)

>         That is four keystrokes too many to name an often needed
>         reference.  Make "@" usable as its synonym.

Yeah, that's nice, but doesn't explain why "@", and why not something else.

>> So now we can use 'git show @~1', and all that goody goodness.
>>
>> Until now '@' was a valid name, but it conflicts with this idea, so lets
>> make it invalid. Probably very few people, if any, used this name.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
>> ---
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 76e3219..3b06e5e 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -965,6 +965,17 @@ int get_sha1_mb(const char *name, unsigned char *sha1)
>>       return st;
>>  }
>>
>> +/* parse @something syntax, when 'something' is not {.*} */
>> +static int interpret_empty_at(const char *name, int namelen, int len, struct strbuf *buf)
>> +{
>> +     if (len || name[1] == '{')
>> +             return -1;
>
> This function is to handle a string that begins with '@', so by
> definition len is zero when anything useful is done by it.  So...
>
>> +     strbuf_reset(buf);
>> +     strbuf_add(buf, "HEAD", 4);
>> +     return 1;
>> +}
>> +
>>  static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
>>  {
>>       /* we have extra data, which might need further processing */
>> @@ -1025,9 +1036,15 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>>       cp = strchr(name, '@');
>>       if (!cp)
>>               return -1;
>> +
>> +     len = interpret_empty_at(name, namelen, cp - name, buf);
>
> ... it is suboptimal (from readability point of view) to have the
> caller unconditionally call interpret_empty_at() when the function
> clearly is marked to handle something that _begins_ with '@'.
>
> I would suggest something like
>
>         if (cp == name)
>                 len = interpret_empty_at(name, namelen, buf);
>
> which people may find much easier to follow.

Why are we then doing:

  int len = interpret_nth_prior_checkout(name, buf);

This function also needs the string to begin with '@', but we don't
check that, we leave that to the function to let us know if it did
interpret it, or not.

> For that matter, it may make even more sense to just remove the
> "empty-at" function and inline its body here:
>
>         if (cp == name && name[1] != '{') {
>                 strbuf_reset(buf);
>                 strbuf_add(buf, "HEAD", 4);
>                 len = 1;
>         } else {
>                 len = -1;
>         }
>
>> +     if (len > 0)
>> +             return reinterpret(name, namelen, len, buf);
>> +

If you are going to do that, there's no need to check len separately.

	if (cp == name && name[1] != '{') {
		strbuf_reset(buf);
		strbuf_add(buf, "HEAD", 4);
		return reinterpret(name, namelen, 1, buf);
	}

But I think it's less readable.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]