Re: [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Phillip Wood <phillip.wood123@gmail.com>
Cc: "Felipe Contreras" <felipe.contreras@gmail.com>,
	git@vger.kernel.org, "David Aguilar" <davvid@gmail.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 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3
Date: Fri, 11 Jun 2021 08:42:38 -0500	[thread overview]
Message-ID: <60c3684eaf033_3ad920828@natae.notmuch> (raw)
In-Reply-To: <xmqq5yyl5buz.fsf@gitster.g>

Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to
> > XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant
> > alone.
> 
> True.

But why? When we look back in history few people would care what the
previous name of XDL_MERGE_STYLE_DIFF3 was, and if they do, they don't
necessarily need it in the title.

> >> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL
> >> could be confused with a valid value instead of XDL_MERGE_DIFF3, which
> >> it isn't.
> >
> > I don't object to the rename but what is the source of the confusion
> > with XDL_MERGE_MINIMAL?
> 
> I do not see any confusion, either, but the current XDL_MERGE_DIFF3
> being a boolean

But it's not a boolean: git_xmerge_style is currently -1 by default.

> (i.e. if false, use the output style of the 'merge'
> command) and our lack of an enumeration constant for 'merge' means
> that a future addition of the third output style would require us to
> add XDL_MERGE_$STYLE for both the new style and the traditional
> 'merge' style.  And If we would end up with XDL_MERGE_DIFF3,
> XDL_MERGE_MERGE and XDL_MERGE_FOO for that third output style.

But can you put XDL_MERGE_FOO in xmp.level? Or XDL_MERGE_BAR in
xmp.style?

> The 'merge' one simply looks strange in that context.  And from that
> point of view, this change might be a good way to futureproof the
> codebase.

Yes.

-- 
Felipe Contreras