git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: Phillip Wood <phillip.wood123@gmail.com>,
	Felipe Contreras <felipe.contreras@gmail.com>,
	git@vger.kernel.org
Cc: "David Aguilar" <davvid@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Sergey Organov" <sorganov@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Denton Liu" <liu.denton@gmail.com>
Subject: Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling
Date: Fri, 11 Jun 2021 09:34:40 -0500	[thread overview]
Message-ID: <60c37480c6ab9_3ad920882@natae.notmuch> (raw)
In-Reply-To: <5164b67c-d701-077f-7743-aea4ae13779d@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

  reply	other threads:[~2021-06-11 14:36 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-09 19:28 [PATCH 0/7] Make diff3 the default conflict style Felipe Contreras
2021-06-09 19:28 ` [PATCH 1/7] test: add merge style config test Felipe Contreras
2021-06-09 19:42   ` Eric Sunshine
2021-06-09 20:29     ` Felipe Contreras
2021-06-10  9:18   ` Phillip Wood
2021-06-10 13:26     ` Felipe Contreras
2021-06-10 14:54       ` Phillip Wood
2021-06-10 16:34         ` Felipe Contreras
2021-06-10 14:58       ` Phillip Wood
2021-06-10 16:47         ` Felipe Contreras
2021-06-11  9:19           ` Phillip Wood
2021-06-11 14:39             ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 2/7] merge-tree: fix merge.conflictstyle handling Felipe Contreras
2021-06-09 19:28 ` [PATCH 3/7] notes: " Felipe Contreras
2021-06-09 19:28 ` [PATCH 4/7] checkout: " Felipe Contreras
2021-06-10  9:32   ` Phillip Wood
2021-06-10 14:11     ` Felipe Contreras
2021-06-10 14:50       ` Phillip Wood
2021-06-10 16:32         ` Felipe Contreras
2021-06-11  9:18           ` Phillip Wood
2021-06-11 14:34             ` Felipe Contreras [this message]
2021-06-11  9:18   ` Phillip Wood
2021-06-09 19:28 ` [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3 Felipe Contreras
2021-06-10  9:21   ` Phillip Wood
2021-06-10 13:33     ` Felipe Contreras
2021-06-11  3:17     ` Junio C Hamano
2021-06-11 13:42       ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 6/7] xdiff: simplify style assignments Felipe Contreras
2021-06-10  9:26   ` Phillip Wood
2021-06-10 13:50     ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 7/7] xdiff: make diff3 the default conflictStyle Felipe Contreras
2021-06-10  6:41   ` Johannes Sixt
2021-06-10  7:53     ` Đoàn Trần Công Danh
2021-06-10 13:18       ` Felipe Contreras
2021-06-10 13:18     ` Felipe Contreras
2021-06-10 13:49     ` Jeff King
2021-06-10 16:00       ` Felipe Contreras
2021-06-10 16:31         ` Jeff King
2021-06-11  1:20       ` Junio C Hamano
2021-06-11  6:23         ` Johannes Sixt
2021-06-11  6:43           ` Junio C Hamano
2021-06-11  7:02             ` Johannes Sixt
2021-06-11  7:14               ` Junio C Hamano
2021-06-11 11:51                 ` Sergey Organov
2021-06-11 15:32                   ` Felipe Contreras
2021-06-11 15:52                     ` Sergey Organov
2021-06-11 16:36                       ` Felipe Contreras
     [not found]                     ` <CABPp-BHRQSF2_aYTBfpfnW4Bh3Hz7vLFj_QNGj8R4WeCS6_utw@mail.gmail.com>
2021-06-11 17:57                       ` Felipe Contreras
2021-06-11 19:02                         ` Elijah Newren
2021-06-11 21:05                           ` Felipe Contreras
2021-06-11 21:40                             ` Elijah Newren
2021-06-13 14:34                               ` Felipe Contreras
2021-06-11 16:41                   ` Johannes Sixt
2021-06-11 17:21                     ` Felipe Contreras
2021-06-11 17:40                       ` Sergey Organov
2021-06-11 18:10                         ` Felipe Contreras
2021-06-11 18:22                           ` Sergey Organov
2021-06-11 14:28                 ` Felipe Contreras
2021-06-11 14:25               ` Felipe Contreras
2021-06-11 16:53                 ` Johannes Sixt
     [not found]                 ` <CABPp-BH0aRiSUw03nSK6jHRNQ+zcpUzr6WjeJ5GpdUCqCKxbag@mail.gmail.com>
2021-06-11 17:32                   ` Felipe Contreras
2021-06-11 17:57                     ` Elijah Newren
2021-06-11 18:28                       ` Felipe Contreras
2021-06-11 14:20           ` Felipe Contreras
2021-06-11 14:09         ` Felipe Contreras
2021-06-10  9:40   ` Phillip Wood
2021-06-10 14:19     ` Felipe Contreras
2021-06-17 17:40 ` [PATCH 0/7] Make diff3 the default conflict style Phillip Wood
2021-06-17 18:24   ` Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=60c37480c6ab9_3ad920882@natae.notmuch \
    --to=felipe.contreras@gmail.com \
    --cc=avarab@gmail.com \
    --cc=bagasdotme@gmail.com \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=sorganov@gmail.com \
    --subject='Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).