git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
May 24, 2011, 18:52 UTC
Message-ID
<20110524185246.GA584@sigill.intra.peff.net>
In-Reply-To
<7vk4dg83yy.fsf@alter.siamese.dyndns.org>
On Tue, May 24, 2011 at 09:20:53AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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).

Show 5 quoted lines
> 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
Previous: Junio C HamanoNext: Jay Soffian
Message 22 of 37 in “combined diff does not detect binary files and ignores -diff attribute”
  1. Jay SoffianMay 22, 2011
  2. Michael J GruberMay 23, 2011
  3. Jay SoffianMay 23, 2011
  4. Junio C HamanoMay 23, 2011
  5. Jeff KingMay 23, 2011
  6. Jeff KingMay 23, 2011
  7. 1/5 combine-diff: split header printing into its own functionJeff King, May 23, 2011
  8. 2/5 combine-diff: calculate mode_differs earlierJeff King, May 23, 2011
  9. 3/5 combine-diff: handle binary files as binaryJeff King, May 23, 2011
  10. Junio C HamanoMay 23, 2011
  11. Jeff KingMay 23, 2011
  12. Junio C HamanoMay 30, 2011
  13. Jeff KingMay 30, 2011
  14. Jeff KingMay 30, 2011
  15. Junio C HamanoMay 30, 2011
  16. Junio C HamanoMay 31, 2011
  17. 4/5 refactor get_textconv to not require diff_filespecJeff King, May 23, 2011
  18. 5/5 combine-diff: respect textconv attributesJeff King, May 23, 2011
  19. Junio C HamanoMay 23, 2011
  20. Jeff KingMay 23, 2011
  21. Junio C HamanoMay 24, 2011
  22. Jeff KingMay 24, 2011
  23. Jay SoffianMay 23, 2011
  24. Jay SoffianMay 23, 2011
  25. Jeff KingMay 23, 2011
  26. Jay SoffianMay 24, 2011
  27. Jeff KingMay 23, 2011
  28. Junio C HamanoMay 24, 2011
  29. Michael J GruberMay 24, 2011
  30. Junio C HamanoMay 24, 2011
  31. Michael J GruberMay 24, 2011
  32. Junio C HamanoMay 24, 2011
  33. Jay SoffianMay 24, 2011
  34. Jeff KingMay 24, 2011
  35. Michael J GruberMay 25, 2011
  36. Jeff KingMay 25, 2011
  37. Jay SoffianMay 24, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.