On 07/09, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@xxxxxxxxx> 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. > > Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx> > > --- > > 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.
