Johan Herland wrote:

quoted text
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html