| Message ID | 20250406121513.154084-2-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove unnecessary if statement | expand |
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>
Eric Sunshine <sunshine@sunshineco.com> writes: > 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>
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
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(-)