Re: [PATCH 3/3] RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''

From: Jeff King <peff@peff.net>
Date: Sep 20, 2010, 18:21 UTC
Message-ID: <20100920182128.GB1790@sigill.intra.peff.net>
In-Reply-To: <7ab60e32582447ad792602ff405dcee464ef1414.1284830388.git.kirr@landau.phys.spbu.ru>

On Sat, Sep 18, 2010 at 09:25:06PM +0400, Kirill Smelkov wrote:

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

I think this is absolutely a bug, and your solution is definitely in the right direction. We obviously can't just ignore the mode when deciding whether to textconv. I suspect there is similar breakage for S_IFGITLINK files, though they are perhaps less likely in practice to match another filetype's extension.

So all three patches look sane to me, with the caveat that I also don't know the blame code very well.

I agree with Matthieu's points on cleaning up the commit messages, and there is a small comment typo in this third patch:

Show 6 quoted lines

> @@ -313,21 +315,23 @@ static struct origin *get_origin(struct scoreboard *sb,
>   * for an origin is also used to pass the blame for the entire file to
>   * the parent to detect the case where a child's blob is identical to
>   * that of its parent's.
> + *
> + * This also fills origin->mode for correspoinding tree path.

Typo: s/poind/pond

-Peff