From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 21 Oct 2010 05:12:32 GMT
Subject: Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
Message-ID: <20101021051232.GA2413@burratino>
In-Reply-To: <1287626936-32232-6-git-send-email-johan@herland.net>

Johan Herland wrote:

> Clearly specify how combine_notes functions are expected to handle null_sha1
> in input. Also specify (and implement) that returning null_sha1 from a
> combine_notes function will cause the note in question to be removed.

Ack again on patches 1-4.  As for this one, I still think the log message
does not make the goal obvious.

 1. Clearly specify how combine_notes functions are expected to
    handle null_sha1 in input.

Wasn't it already clear?  I guess you mean that the documentation was
updated.  But surely that is less important than:

 2. Also specify (and implement) that returning null_sha1 from a
    combine_notes function will ...

A person reading this for the first time could be forgiven for thinking
this is like (1), i.e., documenting an edge case.  But actually it's
the main point, and the part I omitted with "..." is the important
part!

Why not say something like:

	Allow combine_notes functions to request that a note be
	removed, by returning the object id of the empty blob.

	For consistency, also teach note_tree_insert() to skip
	insertion of an empty note when there is no note to
	combine it with.

	In general, an empty note is treated identically to no
	note at all: for example, when merging two notes trees,
	one of which does not have a certain note, combine_notes()
	will be called as though that tree had an empty note
	instead.  Document this.

The above includes guesses, so please do not use it verbatim
unless it's true. :)

Of course these are minor nitpicks as compared to the content of
the patch itself.  The patch still looks good.