threads / patch / 30469

patchsha1_name: support sha1^{note} to return note sha-1

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

## tl;dr

5 messages between May 8, 2012 and May 9, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Nguyễn Thái Ngọc Duy· May 8, 2012, 13:14 UTC · lore

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

Show changes to 2 files +15 −1

Documentation/revisions.txt, sha1_name.c

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

Jeff King· May 8, 2012, 16:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 18 quoted lines

> @@ -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

Nguyen Thai Ngoc Duy· May 9, 2012, 08:25 UTC · re: Jeff King · lore

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

On Tue, May 8, 2012 at 11:11 PM, Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

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

Show 23 quoted lines

>> @@ -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

Johan Herland· May 9, 2012, 14:09 UTC · re: Nguyen Thai Ngoc Duy · lore

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

On Wed, May 9, 2012 at 10:25 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

Show 28 quoted lines

> 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

Jeff King· May 9, 2012, 17:26 UTC · re: Johan Herland · lore

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

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

Show 9 quoted lines

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