[PATCH v2] peel_onion(): add support for <rev>^{tag}

[PATCH v2] peel_onion(): add support for <rev>^{tag}

[Richard Hansen]

Complete the <rev>^{<type>} family of object specifiers by having
<rev>^{tag} dereference <rev> until a tag object is found (or fail if
unable).

At first glance this may not seem very useful, as commits, trees, and
blobs cannot be peeled to a tag, and a tag would just peel to itself.
However, this can be used to ensure that <rev> names a tag object:

    $ git rev-parse --verify v1.8.4^{tag}
    04f013dc38d7512eadb915eba22efc414f18b869
    $ git rev-parse --verify master^{tag}
    error: master^{tag}: expected tag type, but the object dereferences to tree type
    fatal: Needed a single revision

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
Changes since v1 (2013-06-18, see
<http://thread.gmane.org/gmane.comp.version-control.git/228323>):
  * improved commit message
  * added usage note to gitrevisions[7]

 Documentation/revisions.txt | 3 +++
 sha1_name.c                 | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index d477b3f..b3322ad 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -121,6 +121,9 @@ some output processing may assume ref names in UTF-8.
 object that exists, without requiring 'rev' to be a tag, and
 without dereferencing 'rev'; because a tag is already an object,
 it does not have to be dereferenced even once to get to an object.
++
+'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
+existing tag object.
 
 '<rev>{caret}\{\}', e.g. 'v0.99.8{caret}\{\}'::
   A suffix '{caret}' followed by an empty brace pair
diff --git a/sha1_name.c b/sha1_name.c
index 65ad066..6dc496d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	sp++; /* beginning of type name, or closing brace for empty */
 	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
 		expected_type = OBJ_COMMIT;
+	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+		expected_type = OBJ_TAG;
 	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
 		expected_type = OBJ_TREE;
 	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
-- 
1.8.4

Re: [PATCH v2] peel_onion(): add support for <rev>^{tag}

[Jeff King]

On Mon, Sep 02, 2013 at 01:42:31AM -0400, Richard Hansen wrote:

> Complete the <rev>^{<type>} family of object specifiers by having
> <rev>^{tag} dereference <rev> until a tag object is found (or fail if
> unable).
> 
> At first glance this may not seem very useful, as commits, trees, and
> blobs cannot be peeled to a tag, and a tag would just peel to itself.
> However, this can be used to ensure that <rev> names a tag object:
> 
>     $ git rev-parse --verify v1.8.4^{tag}
>     04f013dc38d7512eadb915eba22efc414f18b869
>     $ git rev-parse --verify master^{tag}
>     error: master^{tag}: expected tag type, but the object dereferences to tree type
>     fatal: Needed a single revision
> 
> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---

FWIW, this makes sense to me. You can already accomplish the same thing
by checking the output of $(git cat-file -t $name), but this is a
natural extension of the other ^{} rules, and I can see making some
callers more natural.

>  Documentation/revisions.txt | 3 +++
>  sha1_name.c                 | 2 ++

Can you please add a test (probably in t1511) that checks the behavior,
similar to what you wrote in the commit message?

> diff --git a/sha1_name.c b/sha1_name.c
> index 65ad066..6dc496d 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  	sp++; /* beginning of type name, or closing brace for empty */
>  	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
>  		expected_type = OBJ_COMMIT;
> +	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
> +		expected_type = OBJ_TAG;

This is not a problem you are introducing to this code, but the use of
opaque constants like commit_type along with the magic number "6"
assuming that it contains "commit" seems like a maintenance nightmare
(the only thing saving us is that it will almost certainly never change
from "commit"; but then why do we have the opaque type in the first
place?).

I wonder if we could do better with:

  #define COMMIT_TYPE "commit"
  ...
  if (!strncmp(COMMIT_TYPE, sp, strlen(COMMIT_TYPE))
      && sp[strlen(COMMIT_TYPE)] == '}')

Any compiler worth its salt will optimize the strlen on a string
constant into a constant itself. The length makes it a bit less
readable, though.

I wonder if we could do even better with:

  const char *x;
  ...
  if ((x = skip_prefix(sp, commit_type)) && *x == '}')

which avoids the magic lengths altogether (though the compiler cannot
optimize out the strlen call inside skip_prefix, because we declare
commit_type and friends as an extern. It probably doesn't matter in
peel_onion, though, which should not generally be performance critical
anyway).

-Peff

Re: [PATCH v2] peel_onion(): add support for <rev>^{tag}

[Richard Hansen]

On 2013-09-03 03:05, Jeff King wrote:
> FWIW, this makes sense to me.

Thank you for the feedback.  I posted a reroll of the patch that you've
already replied to, but for the benefit of others searching the mailing
list archive, v3 can be found at
<http://thread.gmane.org/gmane.comp.version-control.git/233752>.

I have a patch submission question:  Is it OK that I didn't use the
'--in-reply-to' argument to 'git send-email' when I sent the v3 reroll?
 Should I have marked it as a reply to the v2 email?  Or should I have
marked it as a reply to your review of the v2 email?

> You can already accomplish the same thing
> by checking the output of $(git cat-file -t $name), but this is a
> natural extension of the other ^{} rules, and I can see making some
> callers more natural.

Exactly.  I updated the commit message to explain this so that other
people know why it might be a good feature to have.

> Can you please add a test (probably in t1511) that checks the behavior,
> similar to what you wrote in the commit message?

Done.  I see by your reply that my fear of going a bit overboard in the
test was justified.  :)  I don't mind rerolling if you'd prefer a
simpler test.

For future reference, is there a preference for putting tests of a new
feature in a separate commit?  In the same commit?  Doesn't really matter?

>> diff --git a/sha1_name.c b/sha1_name.c
>> index 65ad066..6dc496d 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>>  	sp++; /* beginning of type name, or closing brace for empty */
>>  	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
>>  		expected_type = OBJ_COMMIT;
>> +	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
>> +		expected_type = OBJ_TAG;
> 
> This is not a problem you are introducing to this code, but the use of
> opaque constants like commit_type along with the magic number "6"
> assuming that it contains "commit" seems like a maintenance nightmare
> (the only thing saving us is that it will almost certainly never change
> from "commit"; but then why do we have the opaque type in the first
> place?).

I agree.  I didn't address this in the reroll.

> 
> I wonder if we could do better with:
> 
>   #define COMMIT_TYPE "commit"
>   ...
>   if (!strncmp(COMMIT_TYPE, sp, strlen(COMMIT_TYPE))
>       && sp[strlen(COMMIT_TYPE)] == '}')
> 
> Any compiler worth its salt will optimize the strlen on a string
> constant into a constant itself. The length makes it a bit less
> readable, though.

True, and I'm not a huge fan of macros.

> 
> I wonder if we could do even better with:
> 
>   const char *x;
>   ...
>   if ((x = skip_prefix(sp, commit_type)) && *x == '}')
> 
> which avoids the magic lengths altogether

Not bad, especially since skip_prefix() already exists.

> (though the compiler cannot
> optimize out the strlen call inside skip_prefix, because we declare
> commit_type and friends as an extern.  It probably doesn't matter in
> peel_onion, though, which should not generally be performance critical
> anyway).

Yeah, I can't see performance being a problem there.

There's also this awkward approach, which would avoid strlen() altogether:

commit.h:

    extern const char *commit_type;
    extern const size_t commit_type_len;

commit.c:

    const char commit_type_array[] = "commit";
    const char *commit_type = &commit_type_array[0];
    const size_t commit_type_len = sizeof(commit_type_array) - 1;

sha1_name.c peel_onion():

    if (!strncmp(commit_type, sp, commit_type_len)
        && sp[commit_type_len] == '}')

but I prefer your skip_prefix() suggestion.

-Richard

Re: [PATCH v2] peel_onion(): add support for <rev>^{tag}

[Jeff King]

On Tue, Sep 03, 2013 at 02:36:39PM -0400, Richard Hansen wrote:

> I have a patch submission question:  Is it OK that I didn't use the
> '--in-reply-to' argument to 'git send-email' when I sent the v3 reroll?
>  Should I have marked it as a reply to the v2 email?  Or should I have
> marked it as a reply to your review of the v2 email?

I generally prefer if they are kept in the same thread, for two reasons:

  1. People reading v3 may want to refer back to v2. You can provide a
     link to the previous discussion in the archive, but in-reply-to is
     a machine-readable way of doing the same thing (so it also works in
     people's MUAs without having to jump out to the archive).

  2. People reading v2 may be doing so after v3 has been published, and
     of course v2 cannot have linked to v3 at the time it was written.
     If the reader has a threaded MUA (or uses gmane), then the two are
     grouped together, and they can easily see that reading v2 carefully
     may be a waste of time, as it is already obsolete.

Between replying to the review versus the original patch, I don't have a
preference (any decent reader should group the whole thread, which is
enough to accomplish either of the above two).

> Done.  I see by your reply that my fear of going a bit overboard in the
> test was justified.  :)  I don't mind rerolling if you'd prefer a
> simpler test.

I think what you have is fine (modulo dropping the "&& true", which I
somehow failed to notice on first read). It is more thorough, and I do
not think it hurts the readability of the end result (i.e., I can still
tell what the point of the test is). But I am happy either way (I only
mentioned the simpler version because you asked).

> For future reference, is there a preference for putting tests of a new
> feature in a separate commit?  In the same commit?  Doesn't really matter?

I usually put them in the same commit for a small enhancement or fix
like this. Seeing the test along with the change often helps the reader
understand what the patch is doing by providing a concrete example.

Sometimes for a set of changes that needs a large set of refactoring
patches, I'll introduce a group of related failing tests at the
beginning (using test_expect_failure), and then mark them as
expect_success as they are fixed by individual commits.

So you can use your judgement about how the split will communicate to
the reviewer (and later readers of the commit history). But the one
thing you _can't_ do is introduce a failing test and mark it as
expect_success. We try to keep "make test" successful in each commit,
which lets people bisect the history more easily.

> > I wonder if we could do even better with:
> > 
> >   const char *x;
> >   ...
> >   if ((x = skip_prefix(sp, commit_type)) && *x == '}')
> > 
> > which avoids the magic lengths altogether
> 
> Not bad, especially since skip_prefix() already exists.

I'll post a follow-up patch in a second.

> There's also this awkward approach, which would avoid strlen() altogether:
> 
> commit.h:
> 
>     extern const char *commit_type;
>     extern const size_t commit_type_len;
> 
> commit.c:
> 
>     const char commit_type_array[] = "commit";
>     const char *commit_type = &commit_type_array[0];
>     const size_t commit_type_len = sizeof(commit_type_array) - 1;

That is a little manual and awkward for my tastes. skip_prefix() is
an inline specifically so that the compiler _can_ optimize out the
strlen in such cases, without us having to resort to writing the code
ourselves. So if we cared about the strlen here (and I don't think we
do), it would be cleaner to simply convert commit_type to a macro,
static string literal, or static constant array.

-Peff