From: Kirill Smelkov Date: Tue, 21 Sep 2010 18:39:59 GMT Subject: Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks Message-ID: <20100921183959.GB4390@landau.phys.spbu.ru> In-Reply-To: On Mon, Sep 20, 2010 at 11:13:13PM +0200, Matthieu Moy wrote: > Kirill Smelkov 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 Thanks :) Is it for this one patch, or does it apply to the whole series? Thanks, Kirill