Re: [PATCH v5 1/3] format-patch: change erroneous and condition

From: Denton Liu <liu.denton@gmail.com>
Date: Oct 15, 2019, 03:45 UTC
Message-ID: <20191015034546.GA61505@generichostname>
In-Reply-To: <xmqqeezeda7w.fsf@gitster-ct.c.googlers.com>

Hi Junio,

Thanks for the feedback.

On Tue, Oct 15, 2019 at 11:16:35AM +0900, Junio C Hamano wrote:

Show 10 quoted lines

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > .... Since this
> > seems to be a Python-ism that's mistakenly leaked into our code, convert
> 
> The conclusion is OK, but as the inventor of format-patch and a
> non-pythonista, I do not think the above claim is correct, and even
> if Thomas thought it was a good idea to follow Python style in
> 30984ed2 ("format-patch: support deep threading", 2009-02-19), which
> I doubt he did, I do not think there is much point in speculating.

I agree, I probably shouldn't be putting speculation in the log messages. I'll change this for the next reroll.

> 
> Both the log message and the patch text in the previous round were
> better than this round, I would have to say.

I'll probably keep the patch text, however. In the previous version, we were implicitly relying on the value of THREAD_SHALLOW to be 1. This seems a little bit flimsy to me since it's possible that the enum can be changed in the future and it may invalidate that assumption.

I'll keep it explicit so that it's a little bit more robust and also, so that it's more obvious to future readers what's going on.

Thanks,

Denton

Show 18 quoted lines

> 
> Thanks.
> 
> 
> 
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 44b10b3415..351f4ffcfd 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -835,7 +835,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
> >  			thread = THREAD_SHALLOW;
> >  			return 0;
> >  		}
> > -		thread = git_config_bool(var, value) && THREAD_SHALLOW;
> > +		thread = git_config_bool(var, value) ? THREAD_SHALLOW : THREAD_UNSET;
> >  		return 0;
> >  	}
> >  	if (!strcmp(var, "format.signoff")) {