Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1

Johan Herland wrote:

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.