Re: [PATCH 5/5] combine-diff: respect textconv attributes

[Jeff King <peff@peff.net>]

On Tue, May 24, 2011 at 09:20:53AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This patch converts file contents according to textconv attributes. The
> > implementation is slightly ugly; because the textconv code is tightly
> > linked with the diff_filespec code, we temporarily create a
> > diff_filespec during conversion.
> 
> After reading this patch again, I think this aversion to diff_filespec is
> probably unjustified.

It's not an aversion to diff_filespec; it's an aversion to only half
buying-into diff_filespec. I think the best thing would be rewriting all
of combine-diff in terms of diff_filespec; I just started to do it and
it looked big and ugly for not much gain.

It's a little ugly to have to manually do the conversion into another
data structure just to call a function like fill_textconv, but I can
live with it. What I worry about more is that the ad-hoc use of a
structure like diff_filespec means we are violating some assumption that
other code has about the data structure (e.g., the bug Jay already
uncovered that we must have a valid name in the "path" field).

I think what is there now is correct; it's just that as a general rule,
switching data formats or abstractions in the middle of code makes me
feel I'm doing something wrong (or that the code interfaces need to be
refactored).

> If anything else, we should be using the type in _more_ codepaths that are
> not diff related but want to represent a path with its contents in the git
> namespace (be it from working tree, index or a tree), not less, and in the
> longer term weaken functions like fill_textconv() that take diff_filespec
> to take filespec so that they can be made more reusable.

Yeah, I would agree with that.

-Peff