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

Jeff King [off-list ref] 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 is primarily a structure that records what path
has which blob by recording its object name and what mode, and also
in-core data when we make the contents available. There are small diff
specific data associated with it, but that could be separated out if you
really wanted to, perhaps like:

	struct diff_filespec {
        	struct filespec {
			... the generic and essential part ...
                } spec;
                /* diff specific part follows */
                const char *funcname_pattern_ident;
                int xfrm_flags;
                int rename_used;
                unsigned dirty_submodule :2;
                struct userdiff_driver *driver;
                int is_binary;
	};

and let most users use "struct filespec".

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.

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

Jeff King [off-list ref] writes:

quoted

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