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