Re: [PATCH 5/2] push -s: receiving end

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2011, 16:43 UTC
Message-ID: <7vvct3x9vn.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201109081131.58362.johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 19 quoted lines

>> +static void get_note_text(struct strbuf *buf, struct notes_tree *t,
>> +			  const unsigned char *object)
>> +{
>> +	const unsigned char *sha1 = get_note(t, object);
>> +	char *text;
>> +	unsigned long len;
>> +	enum object_type type;
>> +
>> +	if (!sha1)
>> +		return;
>> +	text = read_sha1_file(sha1, &type, &len);
>> +	if (text && len && type == OBJ_BLOB)
>> +		strbuf_add(buf, text, len);
>> +	free(text);
>> +}
>> +
>
> What about adding this function to notes.h as a convenience to other 
> users of the notes API?

I actually was hoping to hear that I do not have to do this "check existing and concatenate", and should let the add_note() function run its default combine_notes method to do the concatenation.

I found a few things I wasn't quite sure in the notes/notes-merge API, by the way.

 - The combine_notes callback is run when a note is inserted into the
   in-core notes tree. I felt that this is way too early if you want to
   avoid racing with another process (and the patch tries to wrap
   create-notes-commit with lock-ref/write-ref-sha1 pair), but perhaps
   this is to deal with a case where the calling program calls add_notes()
   on the same object multiple times.

 - create_notes_commit() dies under a few conditions, but some callers
   that are recording advisory/optional notes might want to get an error
   and continue.

I think ideally this patch should handle notes like the following, which is not quite how I coded it:

 - initialize in-core notes tree;

 - add bunch of notes, without regard to the existing ones, to in-core
   notes tree by calling add_notes();

 - lock the notes ref and read the "parent"; we may want to add "wait and
   retry for a few times until we get the lock" support at lockfile API
   level, but doing it at the application level would be fine.

 - call create-notes-commit, which in turn merges the in-core
   notes with what collides with those already in "parent" by
   calling the combine-notes callback, merges and re-balances
   the notes tree, and makes a notes commit object;

 - update the notes ref with that notes commit, releasing the lock on
   the ref.