Re: [PATCH 6/7] xdiff: simplify style assignments

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 6/7] xdiff: simplify style assignments
Date: Thu, 10 Jun 2021 08:50:28 -0500	[thread overview]
Message-ID: <60c218a4e47e9_b25b1208ef@natae.notmuch> (raw)
In-Reply-To: <cc8bbc34-1048-0e7b-c783-33f556843a3f@gmail.com>

Phillip Wood wrote:
> On 09/06/2021 20:28, Felipe Contreras wrote:
> 
> I don't find the commit message explains this change very well
> 
> > There is little value in checking that git_xmerge_style isn't 0 before
> > changing it's default value.
> 
> I think the check is actually that git_xmerge_style isn't -1.

Actually I meant "< 0", but yeah, that's mainly to check the -1 case.

> Why is there little value in the check?

It's explained in the very next sentence.

> > Most of the time it isn't 0 anyway, so just assign the value directly.
> 
> Why to the times when it is zero (or -1) not matter?

When it's 0 it's a no-op, and now it can't be -1.

By default structures are zeroed in git, so the defaults of integers
are 0, and in the case of xmparam_t.style that is no exception.

These are all the same:

	static xmparam_t xmp;

	static xmparam_t xmp;
	xmp.style = 0;

	static xmparam_t xmp;
	if (1)
		xmp.style = 0;

But of course as it's explained most of the time that's not what
happens, what happens is:

  if (1)
    xml.style = 1;

Perhaps this is clearer:

  There is little value in checking that git_xmerge_style isn't < 0
  before changing its value to xmp.style. If it's 0 then assigning 0 to
  xmp.style is a no-op, and if it's 1 (as it usually is), we are going
  to assign the value anyway.

  The only exception is when git_xmerge_style is -1, but there is no
  value in having that as default, so we just don't, and set the default
  to 0.

Cheers.

-- 
Felipe Contreras