On 07/09, Junio C Hamano wrote:

Thomas Gummerer [off-list ref] writes:

quoted

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.

quoted

Signed-off-by: Thomas Gummerer <redacted>
---

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?

 - 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.