On Mon, Sep 20, 2010 at 11:13:13PM +0200, Matthieu Moy wrote: > Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx> writes: > > > (Description partly by Matthieu Moy) > > Better put such statements at the end, to avoid distracting the reader. Ok > > ~~~~ > > > > NOTE: git diff doesn't try to textconv the pathnames, it runs the > > textual diff without textconv, which is the expected behavior. > > It's not clear whether this is intended to stay in the commit message. > If not, it should go below the ---. If yes, then I'd incorporate this > into the message itself. The ~~~~ and NOTE look odd. I know about --- and that content after it and up to patch itself goes to /dev/null. The text here was intended to stay in the commit message, and ~~~~ served as a separator in that message (git commit hook merges several blank lines into one, so one can't separate text parts with several empty lines, that's why I used this separator). If it's ugly, let's omit it - I don't insist, but i don't understand why 'NOTE:' looks odd? And also, do you remember your first question to this series? It was about does git diff --textconv misbehaves too or not. So I consider this note is important to put into description, and isn't the right place for such a note this patch, where show tests that demonstrate fauilures? This note clearly says "git diff is not affected, that's why we don't write new tests for it". Something like that... > > For example (in next patch): > > | Instead get the mode from either worktree, index, .git, or origin > | entries when blaming and pass it to textconv_object() as context. > | > | The reason to do it is not to run textconv filters on symlinks > + (just like "git diff" already does). See above about my rationale on the note place. > Anyway, I'm bikeshedding. With or without these remarks, > > Reviewed-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> Thanks :) Is it for this one patch, or does it apply to the whole series? Thanks, Kirill -- 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