| Message ID | 56fb230ad271dc9aa91c0f43ac8e4e7085c15775.1571085952.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch: learn --cover-from-description option | expand |
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")) {
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")) {
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")) {
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(-)