Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling

[Felipe Contreras <felipe.contreras@gmail.com>]

Phillip Wood wrote:
> On 10/06/2021 17:32, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> On 10/06/2021 15:11, Felipe Contreras wrote:
> >>> Phillip Wood wrote:
> >>>> On 09/06/2021 20:28, Felipe Contreras wrote:
> >>>>> Currently both merge.conflictStyle and `git commit --merge
> >>>>> --conflict=diff3` don't work together, since the former wrongly
> >>>>> overrides the later.
> >>>>>
> >>>>> The way merge configurations are handled is not correct.
> >>>>> It should be possible to do git_config(merge_recursive_config, ...) just
> >>>>> like we can with git_diff_basic_config and others.
> >>>>
> >>>> It would be helpful to explain what the problem with
> >>>> merge_recursive_config() actually is rather than just saying "it should
> >>>> be possible ..."
> >>>
> >>> The problem is that you can't do this:
> >>>
> >>>     git_config(merge_recursive_config, NULL);
> >>>
> >>> As it was explained.
> >>
> >> You do not explain why you cannot do that
> > 
> >    % git grep merge_recursive_config
> >    static void merge_recursive_config(struct merge_options *opt)
> > 
> > For starters it's a static function.
> > 
> > Second, clearly the type of functions git_config() receives are not
> > `void (*)(struct merge_options *)`.
> > 
> > I mean, I do see value in explaning as much detail as needed in the
> > commit message, but it shouldn't be a lesson on git's codebase:
> > git_config() is a standard thing, and it's even mentioned in the user
> > manual.
> 
> I'm not asking for a lesson on git's config system, I'm asking you to 
> add a sentence to the commit message to explain what the problem is 
> rather than just saying "you should be able to do this but you can't".

Yes, but why? What possible reason could there be for this to fail? Can
you list two?

  git_config(merge_recursive_config, NULL);

But more importantly, why does it matter? How would it change the patch?

> > https://git-scm.com/docs/user-manual#birdview-on-the-source-code
> > 
> >>> That is the problem. I don't know how that's not clear.
> >>>
> >>>>> Therefore builtins like `git merge` can't call this function at the
> >>>>> right time.
> >>>>    >
> >>>>> We shuffle the functions a little bit so at least merge_recursive_config
> >>>>> doesn't call git_xmerge_config directly and thus override previous
> >>>>> configurations.
> >>>>
> >>>> Rather than papering of the problem, how difficult would it be to add a
> >>>> field to ll_merge_options and pass the conflict style with that rather
> >>>> than fiddling with the order that we set a global variable.
> >>>
> >>> Probably not that difficult, but then we also need a parser that
> >>> converts from "diff3" to whatever values we decide in that new field. We
> >>> would need a new parse_config_conflict_style() function.
> >>> And that function will be only used by `git checkout` and nothing else.
> >>> So I don't think there's much value in it.
> >>
> >> It would allow us to add a --conflict option to all the mergey commands
> >> in the future and would be much easier to reason about than the approach
> >> of juggling where we call git_xmerge_config().
> > 
> > Feel free to write a patch for that.
> > 
> >> This patch requires us to audit all the code paths that end up in
> >> merge_recursive_config() to make sure they now call
> >> git_xmerge_config() themselves. You don't seem to have done that as
> >> you don't know if am/apply are affected or not.
> > 
> > That is a separate issue, that I already mentioned...
> 
> I know you are going to add some tests but the point is that when making 
> a change like this you need to actively audit all the callers of 
> init_merge_options() and ensure that they are now calling 
> git_config(git_xmerge_config) and base you tests on what you find while 
> doing that.

And what makes you think I haven't?

If developers were perfect there would be no need for code review.

> Have you run 'grep init_merge_options()' to see where it is 
> being called?

Now that is insulting.

Cheers.

-- 
Felipe Contreras