Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks

From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date: Sep 21, 2010, 18:39 UTC
Message-ID: <20100921183959.GB4390@landau.phys.spbu.ru>
In-Reply-To: <vpqvd6086fq.fsf@bauges.imag.fr>

On Mon, Sep 20, 2010 at 11:13:13PM +0200, Matthieu Moy wrote:

Show 5 quoted lines

> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > (Description partly by Matthieu Moy)
> 
> Better put such statements at the end, to avoid distracting the reader.

Ok

Show 8 quoted lines

> > ~~~~
> >
> > 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...

Show 8 quoted lines

> 
> 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@grenoble-inp.fr>

Thanks :)

Is it for this one patch, or does it apply to the whole series?

Thanks, Kirill