[v5,1/3] format-patch: change erroneous and condition
diff mbox series

Message ID 56fb230ad271dc9aa91c0f43ac8e4e7085c15775.1571085952.git.liu.denton@gmail.com
State New
Headers show
Series
  • format-patch: learn --cover-from-description option
Related show

Commit Message

Denton Liu Oct. 14, 2019, 8:46 p.m. UTC
Commit 30984ed2e9 (format-patch: support deep threading, 2009-02-19),
introduced the following lines:

	#define THREAD_SHALLOW 1

	[...]

	thread = git_config_bool(var, value) && THREAD_SHALLOW;

Since git_config_bool() returns a bool, the trailing `&& THREAD_SHALLOW`
is a no-op.

In Python, `x and y` is equivalent to `y if x else x`[1]. Since this
seems to be a Python-ism that's mistakenly leaked into our code, convert
this to the equivalent C expression.

[1]: https://docs.python.org/3/reference/expressions.html#boolean-operations

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Oct. 15, 2019, 2:16 a.m. UTC | #1
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.

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

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")) {
Denton Liu Oct. 15, 2019, 3:45 a.m. UTC | #2
Hi Junio,

Thanks for the feedback.

On Tue, Oct 15, 2019 at 11:16:35AM +0900, Junio C Hamano wrote:
> 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

> 
> 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")) {

Patch
diff mbox series

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")) {