# [PATCH 0/2] Trivial cleanups 5 messages from 2021-06-13 to 2021-06-15. Participants: Felipe Contreras, Junio C Hamano. Thread: https://gitlist.dev/t/55916 ## Felipe Contreras, 2021-06-13 06:37 Subject: [PATCH 0/2] Trivial cleanups Message-ID: <20210613063702.269816-1-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20210613063702.269816-1-felipe.contreras%40gmail.com ``` These perfeclty good patches from 2014 weren't picked with no good reason. [1] https://lore.kernel.org/git/1398808178-3703-1-git-send-email-felipe.contreras@gmail.com/ Felipe Contreras (2): config: avoid yoda conditions add: avoid yoda condition builtin/add.c | 2 +- config.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) -- 2.32.0 ``` ## Felipe Contreras, 2021-06-13 06:37 Subject: [PATCH 1/2] config: avoid yoda conditions Message-ID: <20210613063702.269816-2-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20210613063702.269816-2-felipe.contreras%40gmail.com In-Reply-To: <20210613063702.269816-1-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index f9c400ad30..7094530b04 100644 --- a/config.c +++ b/config.c @@ -1232,7 +1232,7 @@ static int git_parse_maybe_bool_text(const char *value) int git_parse_maybe_bool(const char *value) { int v = git_parse_maybe_bool_text(value); - if (0 <= v) + if (v >= 0) return v; if (git_parse_int(value, &v)) return !!v; @@ -1242,7 +1242,7 @@ int git_parse_maybe_bool(const char *value) int git_config_bool_or_int(const char *name, const char *value, int *is_bool) { int v = git_parse_maybe_bool_text(value); - if (0 <= v) { + if (v >= 0) { *is_bool = 1; return v; } -- 2.32.0 ``` ## Felipe Contreras, 2021-06-13 06:37 Subject: [PATCH 2/2] add: avoid yoda condition Message-ID: <20210613063702.269816-3-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20210613063702.269816-3-felipe.contreras%40gmail.com In-Reply-To: <20210613063702.269816-1-felipe.contreras@gmail.com> ``` 18 is younger than person's age. Signed-off-by: Felipe Contreras --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index b773b5a499..8c2a5a5094 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -510,7 +510,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) argc--; argv++; - if (0 <= addremove_explicit) + if (addremove_explicit >= 0) addremove = addremove_explicit; else if (take_worktree_changes && ADDREMOVE_DEFAULT) addremove = 0; /* "-u" was given but not "-A" */ -- 2.32.0 ``` ## Junio C Hamano, 2021-06-15 01:45 Subject: Re: [PATCH 0/2] Trivial cleanups Message-ID: URL: https://gitlist.dev/e/xmqqbl87zyra.fsf%40gitster.g In-Reply-To: <20210613063702.269816-1-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > These perfeclty good patches from 2014 weren't picked with no good > reason. These are safe no-op changes, but that does not mean they are good patches. It goes against Documentation/CodingGuidelines to bring it back again now, which is a good enough reason not to look at them. ``` ## Felipe Contreras, 2021-06-15 08:22 Subject: Re: [PATCH 0/2] Trivial cleanups Message-ID: <60c8632d60027_e633208db@natae.notmuch> URL: https://gitlist.dev/e/60c8632d60027_e633208db%40natae.notmuch In-Reply-To: ``` Junio C Hamano wrote: > Felipe Contreras writes: > > > These perfeclty good patches from 2014 weren't picked with no good > > reason. > > These are safe no-op changes, but that does not mean they are good > patches. They are not good patches because they are no-op, they are good changes because they correct the flow of the code to match the flow in which most people think. 18 is younger than Mary is not a sentence most people would agree make sense. > It goes against Documentation/CodingGuidelines to bring it > back again now, which is a good enough reason not to look at them. These are not style issues. Refactoring code to make it easier to understand goes beyond style. Refactoring this: static int is_same_remote(struct remote *remote) { struct remote *fetch_remote = remote_get(NULL); return (!fetch_remote || fetch_remote == remote); } int same_remote = is_same_remote(remote); To this: int same_remote = remote == remote_get(NULL); Is more than just style. But suit yourself. Sooner or later somebody is going to fix these glaring mistakes. And they are mistakes [1]. Yoda conditions are widely criticized for compromising readability by increasing the cognitive load of reading the code. Cheers. [1] https://en.wikipedia.org/wiki/Yoda_conditions#Criticism -- Felipe Contreras ```