[PATCH] sha1_name: support sha1^{note} to return note sha-1

[PATCH] sha1_name: support sha1^{note} to return note sha-1

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 It may be useful for scripting, and looks nice. Though may be not
 worth adding if there are no actual users.

 Documentation/revisions.txt |    1 +
 sha1_name.c                 |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 1725661..5a671fe 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -114,6 +114,7 @@ the '$GIT_DIR/refs' directory or from the '$GIT_DIR/packed-refs' file.
   object of that type is found or the object cannot be
   dereferenced anymore (in which case, barf).  '<rev>{caret}0'
   is a short-hand for '<rev>{caret}\{commit\}'.
+  A special type "note" can be used to return the note 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 c633113..02d28df 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -6,6 +6,7 @@
 #include "tree-walk.h"
 #include "refs.h"
 #include "remote.h"
+#include "notes.h"
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 
@@ -473,7 +474,19 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		expected_type = OBJ_NONE;
 	else if (sp[0] == '/')
 		expected_type = OBJ_COMMIT;
-	else
+	else if (!strncmp("note}", sp, 5)) {
+		const unsigned char *note;
+		struct notes_tree t;
+		if (get_sha1_1(name, sp - name - 2, outer))
+			return -1;
+		memset(&t, 0, sizeof(t));
+		init_notes(&t, NULL, NULL, 0);
+		note = get_note(&t, outer);
+		if (note)
+			hashcpy(sha1, note);
+		free_notes(&t);
+		return note ? 0 : -1;
+	} else
 		return -1;
 
 	if (get_sha1_1(name, sp - name - 2, outer))
-- 
1.7.8.36.g69ee2

Re: [PATCH] sha1_name: support sha1^{note} to return note sha-1

[Jeff King]

On Tue, May 08, 2012 at 08:14:30PM +0700, Nguyen Thai Ngoc Duy wrote:

>  It may be useful for scripting, and looks nice. Though may be not
>  worth adding if there are no actual users.

There can be many notes refs. So I think to do this right, you would
want something like:

  foo^{note:bar}

which would look in refs/notes/bar (this logic is handled by
expand_notes_ref). And "foo^{note}" would be a synonym for the default
note ref.

> @@ -473,7 +474,19 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  		expected_type = OBJ_NONE;
>  	else if (sp[0] == '/')
>  		expected_type = OBJ_COMMIT;
> -	else
> +	else if (!strncmp("note}", sp, 5)) {
> +		const unsigned char *note;
> +		struct notes_tree t;
> +		if (get_sha1_1(name, sp - name - 2, outer))
> +			return -1;
> +		memset(&t, 0, sizeof(t));
> +		init_notes(&t, NULL, NULL, 0);
> +		note = get_note(&t, outer);
> +		if (note)
> +			hashcpy(sha1, note);
> +		free_notes(&t);
> +		return note ? 0 : -1;
> +	} else

The notes code is relatively expensive to initialize, with the
assumption that the effort will be amortized across multiple lookups
(which are made faster). Is it possible to cache this initialized notes
structure in case many lookups are done?

It may not be as important as it used to be, either. I think more recent
versions of the code will progressively load the notes tree rather than
filling it all in at initialization time. But it has been a while since
I've done anything with notes.

-Peff

Re: [PATCH] sha1_name: support sha1^{note} to return note sha-1

[Nguyen Thai Ngoc Duy]

On Tue, May 8, 2012 at 11:11 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 08, 2012 at 08:14:30PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>>  It may be useful for scripting, and looks nice. Though may be not
>>  worth adding if there are no actual users.
>
> There can be many notes refs. So I think to do this right, you would
> want something like:
>
>  foo^{note:bar}
>
> which would look in refs/notes/bar (this logic is handled by
> expand_notes_ref). And "foo^{note}" would be a synonym for the default
> note ref.

Right. Thanks.

>> @@ -473,7 +474,19 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>>               expected_type = OBJ_NONE;
>>       else if (sp[0] == '/')
>>               expected_type = OBJ_COMMIT;
>> -     else
>> +     else if (!strncmp("note}", sp, 5)) {
>> +             const unsigned char *note;
>> +             struct notes_tree t;
>> +             if (get_sha1_1(name, sp - name - 2, outer))
>> +                     return -1;
>> +             memset(&t, 0, sizeof(t));
>> +             init_notes(&t, NULL, NULL, 0);
>> +             note = get_note(&t, outer);
>> +             if (note)
>> +                     hashcpy(sha1, note);
>> +             free_notes(&t);
>> +             return note ? 0 : -1;
>> +     } else
>
> The notes code is relatively expensive to initialize, with the
> assumption that the effort will be amortized across multiple lookups
> (which are made faster). Is it possible to cache this initialized notes
> structure in case many lookups are done?

No idea. I have never worked/used notes until yesterday (and these
patches were the result). I will look into it.

> It may not be as important as it used to be, either. I think more recent
> versions of the code will progressively load the notes tree rather than
> filling it all in at initialization time. But it has been a while since
> I've done anything with notes.
-- 
Duy

Re: [PATCH] sha1_name: support sha1^{note} to return note sha-1

[Johan Herland]

On Wed, May 9, 2012 at 10:25 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Tue, May 8, 2012 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, May 08, 2012 at 08:14:30PM +0700, Nguyen Thai Ngoc Duy wrote:
>>> @@ -473,7 +474,19 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>>>               expected_type = OBJ_NONE;
>>>       else if (sp[0] == '/')
>>>               expected_type = OBJ_COMMIT;
>>> -     else
>>> +     else if (!strncmp("note}", sp, 5)) {
>>> +             const unsigned char *note;
>>> +             struct notes_tree t;
>>> +             if (get_sha1_1(name, sp - name - 2, outer))
>>> +                     return -1;
>>> +             memset(&t, 0, sizeof(t));
>>> +             init_notes(&t, NULL, NULL, 0);
>>> +             note = get_note(&t, outer);
>>> +             if (note)
>>> +                     hashcpy(sha1, note);
>>> +             free_notes(&t);
>>> +             return note ? 0 : -1;
>>> +     } else
>>
>> The notes code is relatively expensive to initialize, with the
>> assumption that the effort will be amortized across multiple lookups
>> (which are made faster). Is it possible to cache this initialized notes
>> structure in case many lookups are done?
>
> No idea. I have never worked/used notes until yesterday (and these
> patches were the result). I will look into it.

IIRC, the initialization loads the top-level notes tree object into
memory. Subtrees (if any) are loaded on demand. FTR, if you have less
then ~256 notes in the notes tree, there will be no subtrees. As the
number of notes grows, the number of subtree levels grow roughly
logarithmically with the total number of notes (see determine_fanout()
for more details).

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] sha1_name: support sha1^{note} to return note sha-1

[Jeff King]

On Wed, May 09, 2012 at 04:09:35PM +0200, Johan Herland wrote:

> > No idea. I have never worked/used notes until yesterday (and these
> > patches were the result). I will look into it.
> 
> IIRC, the initialization loads the top-level notes tree object into
> memory. Subtrees (if any) are loaded on demand. FTR, if you have less
> then ~256 notes in the notes tree, there will be no subtrees. As the
> number of notes grows, the number of subtree levels grow roughly
> logarithmically with the total number of notes (see determine_fanout()
> for more details).

Ah, right. I was thinking back to my original crappy implementation that
didn't do fanout. So I don't think an init is that bad. It does look up
the ref each time, but it will only load the top-level tree object
(which you would need to do a lookup anyway).

So it is probably OK to just do an init/lookup/free each time if it
makes the code simpler (and I think it does). We probably won't be
looking up sha1^{note} in a tight loop, anyway, since sha1 expressions
like that generally come from the command line.

-Peff