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

Phillip Wood wrote:

On 10/06/2021 17:32, Felipe Contreras wrote:

quoted

Phillip Wood wrote:

quoted

On 10/06/2021 15:11, Felipe Contreras wrote:

quoted

Phillip Wood wrote:

quoted

On 09/06/2021 20:28, Felipe Contreras wrote:

quoted

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?

quoted

https://git-scm.com/docs/user-manual#birdview-on-the-source-code

quoted

quoted

That is the problem. I don't know how that's not clear.

quoted

quoted

Therefore builtins like `git merge` can't call this function at the
right time.

   >

quoted

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.

quoted

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