On Tue, May 24, 2011 at 09:20:53AM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html