{"thread":{"id":"63214","subject":"[PATCH 0/1] remove unnecessary if statement","startedAt":"2025-03-29T12:03:34Z","lastAt":"2025-04-07T19:59:02Z","messageCount":9,"participants":["Usman Akinyemi","Patrick Steinhardt","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"515271","messageId":"20250329120327.105925-1-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":null,"subject":"[PATCH 0/1] remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-03-29T11:59:24Z","receivedAt":"2025-03-29T12:03:34Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"In an earlier patch[1] which has been merged to the master,\nWe checked `repo` is not NULL before making call to `repo_config()`.\nLater, in another patch series[2] which has been merged to master,\n`repo_config()` was taught to allow `repo` to be NULL.\n\nSo there is not need for checking if the `repo` is NULL before calling\nrepo_config() in the earlier patch.\n\nAlso, Patrick suggested having the test inside the\n\"t1517-outside-repo.sh\"[3] instead of having it in the individual test\nfiles like[2] and I also think it is a good approach as we will\nhave all such tests in one place. So, for this patch, I added the\ntest inside the \"t1517-outside-repo.sh\". If this is accepted, I will\nmove the test for previous builtin cmd which has already been merged\nto master to \"t1517-outside-repo.sh\" file.\n\n[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/\n[2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/\n[3] https://public-inbox.org/git/Z9vCDFRUG7IzU_AG@pks.im/\n \nUsman Akinyemi (1):\n builtin/update-server-info: remove unnecessary if statement\n\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\n-- \n2.49.0\n\n"},{"id":"515272","messageId":"20250329120327.105925-2-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":"20250329120327.105925-1-usmanakinyemi202@gmail.com","subject":"[PATCH 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-03-29T11:59:25Z","receivedAt":"2025-03-29T12:03:37Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"Since we already teach the `repo_config()` to allow `repo`\nto be NULL, no need to check if `repo` is NULL before calling\n`repo_config()`.\n\nMentored-by: Christian Couder \nSigned-off-by: Usman Akinyemi \n---\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/update-server-info.c b/builtin/update-server-info.c\nindex d7467290a8..ba702d30ef 100644\n--- a/builtin/update-server-info.c\n+++ b/builtin/update-server-info.c\n@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (repo)\n-\t\trepo_config(repo, git_default_config, NULL);\n+\trepo_config(repo, git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t update_server_info_usage, 0);\n \tif (argc > 0)\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex dbd8cd6906..6824581317 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '\n \ttest_grep \"^error: remote-curl\" actual\n '\n \n+test_expect_success 'update-server-info does not crash with -h' '\n+\ttest_expect_code 129 git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage &&\n+\ttest_expect_code 129 nongit git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage\n+'\n+\n test_done\n-- \n2.49.0\n\n"},{"id":"515325","messageId":"Z-pBXOxmtJu9a9R4@pks.im","threadId":"63214","inReplyTo":"20250329120327.105925-2-usmanakinyemi202@gmail.com","subject":"Re: [PATCH 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-31T07:16:44Z","receivedAt":"2025-03-31T07:16:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Mar 29, 2025 at 05:29:25PM +0530, Usman Akinyemi wrote:\n> Since we already teach the `repo_config()` to allow `repo`\n> to be NULL, no need to check if `repo` is NULL before calling\n> `repo_config()`.\n\nTiny nit: when you refer to changes made in a previous commit it makes\nsense to reference that commit via `git log -1 --format=reference` so\nthat outside readers who aren't deeply familiar with the recent changes\nknow where to look.\n\nOther than that this patch looks good to me, thanks!\n\nPatrick\n"},{"id":"515718","messageId":"20250406121513.154084-1-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":"20250329120327.105925-1-usmanakinyemi202@gmail.com","subject":"[PATCH v2 0/1] remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-04-06T12:14:11Z","receivedAt":"2025-04-06T12:15:20Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"In an earlier patch[1] (1a764cdbdc (Merge branch \n'ua/some-builtins-wo-the-repository', 2025-03-26))\nwhich has been merged to the master, we checked `repo` is not NULL\nbefore making call to `repo_config()`. Later, in another patch series[2]\nwhich has been merged to master, `repo_config()` was taught to allow\n`repo` to be NULL.\n\nSo there is not need for checking if the `repo` is NULL before calling\nrepo_config() in the earlier patch.\n\nAlso, Patrick suggested having the test inside the\n\"t1517-outside-repo.sh\"[3] instead of having it in the individual test\nfiles like[2] and I also think it is a good approach as we will\nhave all such tests in one place. So, for this patch, I added the\ntest inside the \"t1517-outside-repo.sh\". If this is accepted, I will\nmove the test for previous builtin cmd which has already been merged\nto master to \"t1517-outside-repo.sh\" file.\n\n[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/\n[2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/\n[3] https://public-inbox.org/git/Z9vCDFRUG7IzU_AG@pks.im/\n\nChanges since v1\n================\n- Make reference to the previous commit that this commit depends on\nvia \"git log\" \n\nUsman Akinyemi (1):\n builtin/update-server-info: remove unnecessary if statement\n\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\nRange-diff versus v1:\n1: d6054cbc0b ! 1: 020b228eb1 builtin/update-server-info: remove unnecessary if statement\n @@ Metadata\n ## Commit message ##\n builtin/update-server-info: remove unnecessary if statement\n \n - Since we already teach the `repo_config()` to allow `repo`\n - to be NULL, no need to check if `repo` is NULL before calling\n - `repo_config()`.\n + Since we already teach the `repo_config()` in \"1a764cdbdc\n + (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)\n + to allow `repo` to be NULL, no need to check if `repo` is NULL\n + before calling `repo_config()`.\n \n Mentored-by: Christian Couder \n Signed-off-by: Usman Akinyemi \n\n-- \n2.49.0\n\n"},{"id":"515719","messageId":"20250406121513.154084-2-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":"20250406121513.154084-1-usmanakinyemi202@gmail.com","subject":"[PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-04-06T12:14:12Z","receivedAt":"2025-04-06T12:15:24Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"Since we already teach the `repo_config()` in \"1a764cdbdc\n(Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)\"\nto allow `repo` to be NULL, no need to check if `repo` is NULL\nbefore calling `repo_config()`.\n\nMentored-by: Christian Couder \nSigned-off-by: Usman Akinyemi \n---\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/update-server-info.c b/builtin/update-server-info.c\nindex d7467290a8..ba702d30ef 100644\n--- a/builtin/update-server-info.c\n+++ b/builtin/update-server-info.c\n@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (repo)\n-\t\trepo_config(repo, git_default_config, NULL);\n+\trepo_config(repo, git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t update_server_info_usage, 0);\n \tif (argc > 0)\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex dbd8cd6906..6824581317 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '\n \ttest_grep \"^error: remote-curl\" actual\n '\n \n+test_expect_success 'update-server-info does not crash with -h' '\n+\ttest_expect_code 129 git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage &&\n+\ttest_expect_code 129 nongit git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage\n+'\n+\n test_done\n-- \n2.49.0\n\n"},{"id":"515739","messageId":"CAPig+cShdouOzG_jKz_Z6+bSprZ5ZEsx9wZR-_LuD1P2kaOWwg@mail.gmail.com","threadId":"63214","inReplyTo":"20250406121513.154084-2-usmanakinyemi202@gmail.com","subject":"Re: [PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-07T00:24:18Z","receivedAt":"2025-04-07T00:24:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 6, 2025 at 8:15 AM Usman Akinyemi\n wrote:\n> Since we already teach the `repo_config()` in \"1a764cdbdc\n> (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)\"\n> to allow `repo` to be NULL, no need to check if `repo` is NULL\n> before calling `repo_config()`.\n\nOkay, makes sense. However...\n\nBy referencing only the merge commit in the above message, you force\nreviewers and future readers to chase down and locate the actual\ncommit[*] which taught repo_config() to accept NULL for `repo`. To be\nmore friendly to those people, you should help them by instead\nreferencing the commit[*] itself.\n\n[*]: f29f1990b5 (config: teach repo_config to allow `repo` to be NULL,\n2025-03-08)\n\n> Signed-off-by: Usman Akinyemi \n"},{"id":"515781","messageId":"xmqqa58snf9c.fsf@gitster.g","threadId":"63214","inReplyTo":"CAPig+cShdouOzG_jKz_Z6+bSprZ5ZEsx9wZR-_LuD1P2kaOWwg@mail.gmail.com","subject":"Re: [PATCH v2 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-07T15:49:19Z","receivedAt":"2025-04-07T15:49:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Apr 6, 2025 at 8:15 AM Usman Akinyemi\n> wrote:\n>> Since we already teach the `repo_config()` in \"1a764cdbdc\n>> (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)\"\n>> to allow `repo` to be NULL, no need to check if `repo` is NULL\n>> before calling `repo_config()`.\n>\n> Okay, makes sense. However...\n>\n> By referencing only the merge commit in the above message, you force\n> reviewers and future readers to chase down and locate the actual\n> commit[*] which taught repo_config() to accept NULL for `repo`.\n\nThanks for raising this. \n\nWhen referring to an entire long series as a whole, a reference to\nthe concluding merge might be more useful, as the topic name\n(hopefully) concisely summarizes what the topic was. However,\nreferring to a single patch series, or a single step in a longer\ntopic, ...\n\n> To be\n> more friendly to those people, you should help them by instead\n> referencing the commit[*] itself.\n\n... this is a useful general advice.\n\n> [*]: f29f1990b5 (config: teach repo_config to allow `repo` to be NULL,\n> 2025-03-08)\n>\n>> Signed-off-by: Usman Akinyemi \n"},{"id":"515797","messageId":"20250407195850.299348-1-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":"20250406121513.154084-1-usmanakinyemi202@gmail.com","subject":"[PATCH v2 0/1] remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-04-07T19:58:40Z","receivedAt":"2025-04-07T19:58:58Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"In an earlier patch[1] (f29f1990b5 (config:\nteach repo_config to allow `repo` to be NULL, 2025-03-08))\nwhich has been merged to the master, we checked `repo` is not NULL\nbefore making call to `repo_config()`. Later, in another patch series[2]\nwhich has been merged to master, `repo_config()` was taught to allow\n`repo` to be NULL.\n\nSo there is not need for checking if the `repo` is NULL before calling\nrepo_config() in the earlier patch.\n\nAlso, Patrick suggested having the test inside the\n\"t1517-outside-repo.sh\"[3] instead of having it in the individual test\nfiles like[2] and I also think it is a good approach as we will\nhave all such tests in one place. So, for this patch, I added the\ntest inside the \"t1517-outside-repo.sh\". If this is accepted, I will\nmove the test for previous builtin cmd which has already been merged\nto master to \"t1517-outside-repo.sh\" file.\n\n[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/\n[2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/\n[3] https://public-inbox.org/git/Z9vCDFRUG7IzU_AG@pks.im/\n\nChanges since 2 \n================\n- Make reference to commit that teaches repo_config() to be NULL instead\n of the merge commit. \n\nUsman Akinyemi (1):\n builtin/update-server-info: remove unnecessary if statement\n\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\nRange-diff versus v2:\n\n1: 020b228eb1 ! 1: cd359dafe8 builtin/update-server-info: remove unnecessary if statement\n @@ Metadata\n ## Commit message ##\n builtin/update-server-info: remove unnecessary if statement\n \n - Since we already teach the `repo_config()` in \"1a764cdbdc\n - (Merge branch 'ua/some-builtins-wo-the-repository', 2025-03-26)\n + Since we already teach the `repo_config()` in \"f29f1990b5\n + (config: teach repo_config to allow `repo` to be NULL, 2025-03-08)\"\n to allow `repo` to be NULL, no need to check if `repo` is NULL\n before calling `repo_config()`.\n\n-- \n2.49.0\n\n"},{"id":"515798","messageId":"20250407195850.299348-2-usmanakinyemi202@gmail.com","threadId":"63214","inReplyTo":"20250407195850.299348-1-usmanakinyemi202@gmail.com","subject":"[PATCH 1/1] builtin/update-server-info: remove unnecessary if statement","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-04-07T19:58:41Z","receivedAt":"2025-04-07T19:59:02Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"Since we already teach the `repo_config()` in \"f29f1990b5\n(config: teach repo_config to allow `repo` to be NULL, 2025-03-08)\"\nto allow `repo` to be NULL, no need to check if `repo` is NULL\nbefore calling `repo_config()`.\n\nMentored-by: Christian Couder \nSigned-off-by: Usman Akinyemi \n---\n builtin/update-server-info.c | 4 ++--\n t/t1517-outside-repo.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/update-server-info.c b/builtin/update-server-info.c\nindex d7467290a8..ba702d30ef 100644\n--- a/builtin/update-server-info.c\n+++ b/builtin/update-server-info.c\n@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (repo)\n-\t\trepo_config(repo, git_default_config, NULL);\n+\trepo_config(repo, git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t update_server_info_usage, 0);\n \tif (argc > 0)\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex dbd8cd6906..6824581317 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -107,4 +107,11 @@ test_expect_success LIBCURL 'remote-http outside repository' '\n \ttest_grep \"^error: remote-curl\" actual\n '\n \n+test_expect_success 'update-server-info does not crash with -h' '\n+\ttest_expect_code 129 git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage &&\n+\ttest_expect_code 129 nongit git update-server-info -h >usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage\n+'\n+\n test_done\n-- \n2.49.0\n\n"}]}