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

Re: [PATCH v3 07/14] apply: make parse_git_header public

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Jul 9, 2019, 21:23 UTC
Message-ID
<20190709212359.GD15477@hank.intra.tgummerer.com>
In-Reply-To
<xmqqimsbuhdy.fsf@gitster-ct.c.googlers.com>
On 07/09, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Make parse_git_header a "public" function in apply.h, so we can re-use
> > it in range-diff in a subsequent commit.

Eek, I just noticed that I forgot updating the name here. This and the Subject should say 'parse_git_diff_header()' now, instead of parse_git_header of course. Will fix that in the reroll.

Show 14 quoted lines
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> 
> Thanks for these refactoring patches on "apply" machinery in the
> early part of the series.  I noticed two small things, though.
> 
>  - The apply_state instance *does* represent a state and various
>    fields get updated as we read and process the patch.  The smaller
>    structure you invented, on the other hand, does not carry any
>    "state" at all.  Even its "linenr" field does not get incremented
>    as we read/process---you create a new copy to take a snapshot of
>    the current state from apply_state.  parse_git_header_data may
>    have been a name that reflects the nature of the structure
>    better.
Yeah, I think that's better.  Will change, thanks!

Maybe it would be even better to name it 'struct gitdiff_data', as it's really only used for those few functions?

Show 6 quoted lines
>  - I wonder if it makes the concept clearer if you did not create a
>    new instance outside the apply_state, but instead replaced the
>    three fields in the apply_state with an instance of this new
>    structure.  When you call an API function with shrunk interface,
>    you'd pass a pointer to a field inside the apply_state instance,
>    instead of copying three fields manually.

I had considered that. However I struggled to come up with a name that makes sense in both as an interface to 'parse_git_diff_header()', and inside 'struct apply_state'. 'linenr' is not specific to parsing git diff headers (or even parsing any type of diff header), but is used all over the apply code. So 'parse_git_header_data' doesn't make sense as a name anymore (and gets complicated to explain to the readers of the code I think).

At that point the name should also be <something>_state again, because we do update the linenr inside 'parse_git_diff_header()', just not inside any of the 'gitdiff_*' functions, though that is only a minor point.

So unless there's a good name for this struct that I couldn't think of, I think it's better to pass in the variables separately to 'parse_git_diff_header()', and then pass the struct just to the 'gitdiff_*' functions, as it's done currently.

> But other than that, I think these patches are generally moving bits
> in the right direction.
Thanks for the review!
> I do not have strong opinions on the later part of the series on
> range-diff proper.
> 
> Thanks.
Previous: Junio C HamanoNext: Junio C Hamano
Message 35 of 61 in “output improvements for git range-diff”
  1. 00/14 output improvements for git range-diffThomas Gummerer, Jul 5, 2019
  2. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 5, 2019
  3. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 5, 2019
  4. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 5, 2019
  5. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 5, 2019
  6. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 5, 2019
  7. 07/14 apply: make parse_git_header publicThomas Gummerer, Jul 5, 2019
  8. Johannes SchindelinJul 5, 2019
  9. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 5, 2019
  10. Johannes SchindelinJul 5, 2019
  11. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 5, 2019
  12. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 5, 2019
  13. Johannes SchindelinJul 5, 2019
  14. Thomas GummererJul 8, 2019
  15. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 5, 2019
  16. Johannes SchindelinJul 5, 2019
  17. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 5, 2019
  18. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 5, 2019
  19. Johannes SchindelinJul 5, 2019
  20. Thomas GummererJul 8, 2019
  21. Johannes SchindelinJul 8, 2019
  22. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 5, 2019
  23. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 5, 2019
  24. Johannes SchindelinJul 5, 2019
  25. Thomas GummererJul 8, 2019
  26. 00/14 output improvements for git range-diffThomas Gummerer, Jul 8, 2019
  27. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 8, 2019
  28. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 8, 2019
  29. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 8, 2019
  30. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 8, 2019
  31. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 8, 2019
  32. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 8, 2019
  33. 07/14 apply: make parse_git_header publicThomas Gummerer, Jul 8, 2019
  34. Junio C HamanoJul 9, 2019
  35. Thomas GummererJul 9, 2019
  36. Junio C HamanoJul 9, 2019
  37. Thomas GummererJul 10, 2019
  38. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 8, 2019
  39. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 8, 2019
  40. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 8, 2019
  41. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 8, 2019
  42. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 8, 2019
  43. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 8, 2019
  44. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 8, 2019
  45. 00/14 output improvements for git range-diffThomas Gummerer, Jul 11, 2019
  46. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 11, 2019
  47. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 11, 2019
  48. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 11, 2019
  49. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 11, 2019
  50. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 11, 2019
  51. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 11, 2019
  52. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 11, 2019
  53. 07/14 apply: make parse_git_diff_header publicThomas Gummerer, Jul 11, 2019
  54. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 11, 2019
  55. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 11, 2019
  56. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 11, 2019
  57. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 11, 2019
  58. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 11, 2019
  59. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 11, 2019
  60. Ramsay JonesJul 11, 2019
  61. Johannes SchindelinJul 12, 2019

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.