threads / patch / 63214

patchremove unnecessary if statement

Subject: [PATCH 0/1] remove unnecessary if statement

## tl;dr

9 messages between Mar 29, 2025 and Apr 7, 2025. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Usman Akinyemi· Mar 29, 2025, 11:59 UTC · lore

In an earlier patch[1] which has been merged to the master, We checked `repo` is not NULL before making call to `repo_config()`. Later, in another patch series[2] which has been merged to master, `repo_config()` was taught to allow `repo` to be NULL.

So there is not need for checking if the `repo` is NULL before calling repo_config() in the earlier patch.

Also, Patrick suggested having the test inside the "t1517-outside-repo.sh"[3] instead of having it in the individual test files like[2] and I also think it is a good approach as we will have all such tests in one place. So, for this patch, I added the test inside the "t1517-outside-repo.sh". If this is accepted, I will move the test for previous builtin cmd which has already been merged to master to "t1517-outside-repo.sh" file.

[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/
[2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/
[3] https://public-inbox.org/git/Z9vCDFRUG7IzU_AG@pks.im/
 
Usman Akinyemi (1):
  builtin/update-server-info: remove unnecessary if statement

 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.49.0

Usman Akinyemi· Mar 29, 2025, 11:59 UTC · re: Usman Akinyemi · lore

[PATCH 1/1] builtin/update-server-info: remove unnecessary if statement

Since we already teach the `repo_config()` to allow `repo` to be NULL, no need to check if `repo` is NULL before calling `repo_config()`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Show changes to 2 files +9 −2

builtin/update-server-info.c, t/t1517-outside-repo.sh

diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
index d7467290a8..ba702d30ef 100644
--- a/builtin/update-server-info.c
+++ b/builtin/update-server-info.c
@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,
 		OPT_END()
 	};
 
-	if (repo)
-		repo_config(repo, git_default_config, NULL);
+	repo_config(repo, git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, options,
 			     update_server_info_usage, 0);
 	if (argc > 0)
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index dbd8cd6906..6824581317 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '
 	test_grep "^error: remote-curl" actual
 '
 
+test_expect_success 'update-server-info does not crash with -h' '
+	test_expect_code 129 git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage &&
+	test_expect_code 129 nongit git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage
+'
+
 test_done

-- 
2.49.0

Patrick Steinhardt· Mar 31, 2025, 07:16 UTC · re: Usman Akinyemi · lore

Re: [PATCH 1/1] builtin/update-server-info: remove unnecessary if statement

On Sat, Mar 29, 2025 at 05:29:25PM +0530, Usman Akinyemi wrote:

> Since we already teach the `repo_config()` to allow `repo`
> to be NULL, no need to check if `repo` is NULL before calling
> `repo_config()`.

Tiny nit: when you refer to changes made in a previous commit it makes sense to reference that commit via `git log -1 --format=reference` so that outside readers who aren't deeply familiar with the recent changes know where to look.

Other than that this patch looks good to me, thanks!

Patrick

Usman Akinyemi· Apr 6, 2025, 12:14 UTC · re: Usman Akinyemi · lore

[PATCH v2 0/1] remove unnecessary if statement

In an earlier patch[1] (1a764cdbdc (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)) which has been merged to the master, we checked `repo` is not NULL before making call to `repo_config()`. Later, in another patch series[2] which has been merged to master, `repo_config()` was taught to allow `repo` to be NULL.

So there is not need for checking if the `repo` is NULL before calling repo_config() in the earlier patch.

[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/ [2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/ [3] https://public-inbox.org/git/Z9vCDFRUG7IzU_AG@pks.im/

Changes since v1
================
- Make reference to the previous commit that this commit depends on
via "git log"

Usman Akinyemi (1):
  builtin/update-server-info: remove unnecessary if statement

 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Range-diff versus v1:
1:  d6054cbc0b ! 1:  020b228eb1 builtin/update-server-info: remove unnecessary if statement
    @@ Metadata
      ## Commit message ##
         builtin/update-server-info: remove unnecessary if statement
     
    -    Since we already teach the `repo_config()` to allow `repo`
    -    to be NULL, no need to check if `repo` is NULL before calling
    -    `repo_config()`.
    +    Since we already teach the `repo_config()` in "1a764cdbdc
    +    (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)
    +    to allow `repo` to be NULL, no need to check if `repo` is NULL
    +    before calling `repo_config()`.
     
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

-- 
2.49.0

Usman Akinyemi· Apr 6, 2025, 12:14 UTC · re: Usman Akinyemi · lore

[PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement

Since we already teach the `repo_config()` in "1a764cdbdc (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)" to allow `repo` to be NULL, no need to check if `repo` is NULL before calling `repo_config()`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Show changes to 2 files +9 −2

builtin/update-server-info.c, t/t1517-outside-repo.sh

diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
index d7467290a8..ba702d30ef 100644
--- a/builtin/update-server-info.c
+++ b/builtin/update-server-info.c
@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,
 		OPT_END()
 	};
 
-	if (repo)
-		repo_config(repo, git_default_config, NULL);
+	repo_config(repo, git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, options,
 			     update_server_info_usage, 0);
 	if (argc > 0)
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index dbd8cd6906..6824581317 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '
 	test_grep "^error: remote-curl" actual
 '
 
+test_expect_success 'update-server-info does not crash with -h' '
+	test_expect_code 129 git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage &&
+	test_expect_code 129 nongit git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage
+'
+
 test_done

-- 
2.49.0

Eric Sunshine· Apr 7, 2025, 00:24 UTC · re: Usman Akinyemi · lore

Re: [PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement

On Sun, Apr 6, 2025 at 8:15 AM Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:

> Since we already teach the `repo_config()` in "1a764cdbdc
> (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)"
> to allow `repo` to be NULL, no need to check if `repo` is NULL
> before calling `repo_config()`.

Okay, makes sense. However...

By referencing only the merge commit in the above message, you force reviewers and future readers to chase down and locate the actual commit[*] which taught repo_config() to accept NULL for `repo`. To be more friendly to those people, you should help them by instead referencing the commit[*] itself.

[*]: f29f1990b5 (config: teach repo_config to allow `repo` to be NULL, 2025-03-08)

> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

Junio C Hamano· Apr 7, 2025, 15:49 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 12 quoted lines

> On Sun, Apr 6, 2025 at 8:15 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
>> Since we already teach the `repo_config()` in "1a764cdbdc
>> (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)"
>> to allow `repo` to be NULL, no need to check if `repo` is NULL
>> before calling `repo_config()`.
>
> Okay, makes sense. However...
>
> By referencing only the merge commit in the above message, you force
> reviewers and future readers to chase down and locate the actual
> commit[*] which taught repo_config() to accept NULL for `repo`.

Thanks for raising this.

When referring to an entire long series as a whole, a reference to the concluding merge might be more useful, as the topic name (hopefully) concisely summarizes what the topic was. However, referring to a single patch series, or a single step in a longer topic, ...

> To be
> more friendly to those people, you should help them by instead
> referencing the commit[*] itself.

... this is a useful general advice.

> [*]: f29f1990b5 (config: teach repo_config to allow `repo` to be NULL,
> 2025-03-08)
>
>> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

Usman Akinyemi· Apr 7, 2025, 19:58 UTC · re: Usman Akinyemi · lore

[PATCH v2 0/1] remove unnecessary if statement

In an earlier patch[1] (f29f1990b5 (config: teach repo_config to allow `repo` to be NULL, 2025-03-08)) which has been merged to the master, we checked `repo` is not NULL before making call to `repo_config()`. Later, in another patch series[2] which has been merged to master, `repo_config()` was taught to allow `repo` to be NULL.

So there is not need for checking if the `repo` is NULL before calling repo_config() in the earlier patch.

Changes since 2 
================
- Make reference to commit that teaches repo_config() to be NULL instead
  of the merge commit.

Usman Akinyemi (1):
  builtin/update-server-info: remove unnecessary if statement

 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Range-diff versus v2:

1:  020b228eb1 ! 1:  cd359dafe8 builtin/update-server-info: remove unnecessary if statement
    @@ Metadata
      ## Commit message ##
         builtin/update-server-info: remove unnecessary if statement
     
    -    Since we already teach the `repo_config()` in "1a764cdbdc
    -    (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)
    +    Since we already teach the `repo_config()` in "f29f1990b5
    +    (config: teach repo_config to allow `repo` to be NULL, 2025-03-08)"
         to allow `repo` to be NULL, no need to check if `repo` is NULL
         before calling `repo_config()`.

-- 
2.49.0

Usman Akinyemi· Apr 7, 2025, 19:58 UTC · re: Usman Akinyemi · lore

[PATCH 1/1] builtin/update-server-info: remove unnecessary if statement

Since we already teach the `repo_config()` in "f29f1990b5 (config: teach repo_config to allow `repo` to be NULL, 2025-03-08)" to allow `repo` to be NULL, no need to check if `repo` is NULL before calling `repo_config()`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 builtin/update-server-info.c | 4 ++--
 t/t1517-outside-repo.sh      | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Show changes to 2 files +9 −2

builtin/update-server-info.c, t/t1517-outside-repo.sh

diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
index d7467290a8..ba702d30ef 100644
--- a/builtin/update-server-info.c
+++ b/builtin/update-server-info.c
@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,
 		OPT_END()
 	};
 
-	if (repo)
-		repo_config(repo, git_default_config, NULL);
+	repo_config(repo, git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, options,
 			     update_server_info_usage, 0);
 	if (argc > 0)
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index dbd8cd6906..6824581317 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '
 	test_grep "^error: remote-curl" actual
 '
 
+test_expect_success 'update-server-info does not crash with -h' '
+	test_expect_code 129 git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage &&
+	test_expect_code 129 nongit git update-server-info -h >usage &&
+	test_grep "[Uu]sage: git update-server-info " usage
+'
+
 test_done

-- 
2.49.0