{"thread":{"id":"59246","subject":"[PATCH 0/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup","startedAt":"2023-02-15T10:43:12Z","lastAt":"2023-02-22T15:39:23Z","messageCount":9,"participants":["Idriss Fekir","Shuqi Liang","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"472129","messageId":"20230215104246.8919-1-mcsm224@gmail.com","threadId":"59246","inReplyTo":null,"subject":"[PATCH 0/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup","fromName":"Idriss Fekir","fromEmail":"mcsm224@gmail.com","sentAt":"2023-02-15T10:42:45Z","receivedAt":"2023-02-15T10:43:12Z","isPatch":true,"sender":{"key":"mcsm224@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48716714?v=4"},"body":"Hi, this is my first ever contribution to git.\nThis is about the #FIXME in trace_repo_setup, which had prefix as a parameter before prefix was added to startup_info\n\n\nidriss fekir (1):\n remove parameter (prefix) from trace_repo_setup\n\n git.c | 2 +-\n trace.c | 6 +++---\n trace.h | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\n\nbase-commit: c867e4fa180bec4750e9b54eb10f459030dbebfd\n-- \n2.39.1\n\n"},{"id":"472130","messageId":"20230215104246.8919-2-mcsm224@gmail.com","threadId":"59246","inReplyTo":"20230215104246.8919-1-mcsm224@gmail.com","subject":"[PATCH 1/1] remove parameter (prefix) from trace_repo_setup","fromName":"Idriss Fekir","fromEmail":"mcsm224@gmail.com","sentAt":"2023-02-15T10:42:46Z","receivedAt":"2023-02-15T10:43:34Z","isPatch":true,"sender":{"key":"mcsm224@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48716714?v=4"},"body":"From: idriss fekir \n\nSigned-off-by: Idriss Fekir \n---\n git.c | 2 +-\n trace.c | 6 +++---\n trace.h | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 96b0a2837d..6171fd6769 100644\n--- a/git.c\n+++ b/git.c\n@@ -430,7 +430,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n \t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(prefix);\n+\t\ttrace_repo_setup();\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/trace.c b/trace.c\nindex 794a087c21..316070a43e 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -292,9 +292,9 @@ static const char *quote_crnl(const char *path)\n }\n \n /* FIXME: move prefix to startup_info struct and get rid of this arg */\n-void trace_repo_setup(const char *prefix)\n+void trace_repo_setup()\n {\n-\tconst char *git_work_tree;\n+\tconst char *git_work_tree, *prefix = startup_info->prefix;\n \tchar *cwd;\n \n \tif (!trace_want(&trace_setup_key))\n@@ -305,7 +305,7 @@ void trace_repo_setup(const char *prefix)\n \tif (!(git_work_tree = get_git_work_tree()))\n \t\tgit_work_tree = \"(null)\";\n \n-\tif (!prefix)\n+\tif (!startup_info->prefix)\n \t\tprefix = \"(null)\";\n \n \ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\ndiff --git a/trace.h b/trace.h\nindex 4e771f86ac..ca943037c6 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -93,7 +93,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n extern struct trace_key trace_setup_key;\n \n-void trace_repo_setup(const char *prefix);\n+void trace_repo_setup();\n \n /**\n * Checks whether the trace key is enabled. Used to prevent expensive\n-- \n2.39.1\n\n"},{"id":"472150","messageId":"20230215175510.3631-1-cheskaqiqi@gmail.com","threadId":"59246","inReplyTo":"20230215104246.8919-2-mcsm224@gmail.com","subject":"Re:[PATCH 1/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-02-15T17:55:10Z","receivedAt":"2023-02-15T17:55:45Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hello Idriss,\n\nI have some suggestions：\n\nSeen like you didn't describe the existing problem and how you fix it\nin the commit message. You can start with a description of the\nexisting problem in the present tense in your commit message. Also,\nyou should use the imperative mood to describe the change you make.\n\nMaybe you can write something like the below in your commit message\nafter you use git commit -s.\n----------------------------------------------------------------------------------------\ntrace.c, git.c: removed unnecessary parameter to trace_repo_setup\n\nyour description of the existing problem.............\n\nFix them.\n\nSigned-off-by: ...\n-------------------------------------------------------------------------------------------\n\n\n\n\n\nRegards,\nShuqi\n\n\n"},{"id":"472151","messageId":"xmqqh6vmzub3.fsf@gitster.g","threadId":"59246","inReplyTo":"20230215104246.8919-2-mcsm224@gmail.com","subject":"Re: [PATCH 1/1] remove parameter (prefix) from trace_repo_setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-15T17:56:32Z","receivedAt":"2023-02-15T17:56:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Idriss Fekir writes:\n\n> From: idriss fekir \n\nThis blank space is for you to explain why this is a good thing to\ndo, which is missing here.\n\n> diff --git a/trace.c b/trace.c\n> index 794a087c21..316070a43e 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -292,9 +292,9 @@ static const char *quote_crnl(const char *path)\n> }\n> \n> /* FIXME: move prefix to startup_info struct and get rid of this arg */\n\nI do not think this comment was meant as an instruction to BLINDLY\nremove the parameter and instead use from startup_info. Instead,\nthe \"FIX\" in \"FIXME\" would involve that whoever does the fix checks\nthe caller that reaches this function and makes sure that \"prefix\"\nvalue the caller passes exactly matches what is in the prefix member\nof the startup_info. Documenting that work should become a major\npart of the proposed log message. After doing that, this comment\nis no longer valid and needs to be removed.\n\n> -void trace_repo_setup(const char *prefix)\n> +void trace_repo_setup()\n\n\"void trace_repo_setup(void)\"\n\n> -void trace_repo_setup(const char *prefix);\n> +void trace_repo_setup();\n\n\"void trace_repo_setup(void);\"\n\nThanks.\n"},{"id":"472171","messageId":"20230215231428.68040-1-mcsm224@gmail.com","threadId":"59246","inReplyTo":"20230215104246.8919-1-mcsm224@gmail.com","subject":"[PATCH 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup","fromName":"Idriss Fekir","fromEmail":"mcsm224@gmail.com","sentAt":"2023-02-15T23:14:28Z","receivedAt":"2023-02-15T23:15:00Z","isPatch":true,"sender":{"key":"mcsm224@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48716714?v=4"},"body":"From: idriss fekir \n\ntrace_repo_setup of trace.c is called with the argument 'prefix' from\nonly one location, run_builtin of git.c, which sets 'prefix' to\nthe return value of setup_git_directory or setup_git_directory_gently\n(a wrapper of the former).\n\nNow that \"prefix\" is in startup_info there is no need for the parameter\nof trace_repo_setup because setup_git_directory sets\n\"startup_info->prefix\" to the same value it returns. It would be less\nconfusing to use \"prefix\" from startup_info instead of passing it as\na parameter.\n\nSigned-off-by: Idriss Fekir \n---\nThank you very much, Shuqi Liang and Junio C Hamano for your valuable critique and suggestions.\n\n git.c | 2 +-\n trace.c | 7 +++----\n trace.h | 2 +-\n 3 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 96b0a2837d..6171fd6769 100644\n--- a/git.c\n+++ b/git.c\n@@ -430,7 +430,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n \t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(prefix);\n+\t\ttrace_repo_setup();\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/trace.c b/trace.c\nindex 794a087c21..efa4e2d8e0 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -291,10 +291,9 @@ static const char *quote_crnl(const char *path)\n \treturn new_path.buf;\n }\n \n-/* FIXME: move prefix to startup_info struct and get rid of this arg */\n-void trace_repo_setup(const char *prefix)\n+void trace_repo_setup(void)\n {\n-\tconst char *git_work_tree;\n+\tconst char *git_work_tree, *prefix = startup_info->prefix;\n \tchar *cwd;\n \n \tif (!trace_want(&trace_setup_key))\n@@ -305,7 +304,7 @@ void trace_repo_setup(const char *prefix)\n \tif (!(git_work_tree = get_git_work_tree()))\n \t\tgit_work_tree = \"(null)\";\n \n-\tif (!prefix)\n+\tif (!startup_info->prefix)\n \t\tprefix = \"(null)\";\n \n \ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\ndiff --git a/trace.h b/trace.h\nindex 4e771f86ac..b6e35b9470 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -93,7 +93,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n extern struct trace_key trace_setup_key;\n \n-void trace_repo_setup(const char *prefix);\n+void trace_repo_setup(void);\n \n /**\n * Checks whether the trace key is enabled. Used to prevent expensive\n-- \n2.39.2\n\n"},{"id":"472308","messageId":"CAP8UFD2=6CarUN1v-vavEJza0PyzZLt3xjVS0BubYUD9=fy46w@mail.gmail.com","threadId":"59246","inReplyTo":"20230215231428.68040-1-mcsm224@gmail.com","subject":"Re: [PATCH 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-02-18T18:35:24Z","receivedAt":"2023-02-18T18:35:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 16, 2023 at 12:36 AM Idriss Fekir wrote:\n>\n> From: idriss fekir \n\nI think the subject could be improved a bit more, and it looks like\nit's the second version of the patch, so something like the following\nmight have been better:\n\n[PATCH v2 1/1] trace: remove unnecessary parameter to trace_repo_setup()\n\n(Please use \"v3\" if you resend it with some changes.)\n\n> trace_repo_setup of trace.c is called with the argument 'prefix' from\n> only one location, run_builtin of git.c, which sets 'prefix' to\n> the return value of setup_git_directory or setup_git_directory_gently\n> (a wrapper of the former).\n\nIt might be a bit easier to read when functions names have \"()\" after\nthem like \"trace_repo_setup()\" instead of \"trace_repo_setup\".\n\nOtherwise your patch looks good to me. Thanks!\n"},{"id":"472310","messageId":"20230219002527.84315-1-mcsm224@gmail.com","threadId":"59246","inReplyTo":"20230215104246.8919-1-mcsm224@gmail.com","subject":"[PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()","fromName":"Idriss Fekir","fromEmail":"mcsm224@gmail.com","sentAt":"2023-02-19T00:25:27Z","receivedAt":"2023-02-19T00:25:55Z","isPatch":true,"sender":{"key":"mcsm224@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48716714?v=4"},"body":"From: idriss fekir \n\ntrace_repo_setup() of trace.c is called with the argument 'prefix' from\nonly one location, run_builtin of git.c, which sets 'prefix' to the return\nvalue of setup_git_directory() or setup_git_directory_gently() (a wrapper\nof the former).\n\nNow that \"prefix\" is in startup_info there is no need for the parameter\nof trace_repo_setup() because setup_git_directory() sets \"startup_info->prefix\"\nto the same value it returns. It would be less confusing to use \"prefix\"\nfrom startup_info instead of passing it as an argument.\n\nSigned-off-by: Idriss Fekir \n---\nThank you very much, Christian Couder.\n git.c | 2 +-\n trace.c | 7 +++----\n trace.h | 2 +-\n 3 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 96b0a2837d..6171fd6769 100644\n--- a/git.c\n+++ b/git.c\n@@ -430,7 +430,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n \t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(prefix);\n+\t\ttrace_repo_setup();\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/trace.c b/trace.c\nindex 794a087c21..efa4e2d8e0 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -291,10 +291,9 @@ static const char *quote_crnl(const char *path)\n \treturn new_path.buf;\n }\n \n-/* FIXME: move prefix to startup_info struct and get rid of this arg */\n-void trace_repo_setup(const char *prefix)\n+void trace_repo_setup(void)\n {\n-\tconst char *git_work_tree;\n+\tconst char *git_work_tree, *prefix = startup_info->prefix;\n \tchar *cwd;\n \n \tif (!trace_want(&trace_setup_key))\n@@ -305,7 +304,7 @@ void trace_repo_setup(const char *prefix)\n \tif (!(git_work_tree = get_git_work_tree()))\n \t\tgit_work_tree = \"(null)\";\n \n-\tif (!prefix)\n+\tif (!startup_info->prefix)\n \t\tprefix = \"(null)\";\n \n \ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\ndiff --git a/trace.h b/trace.h\nindex 4e771f86ac..b6e35b9470 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -93,7 +93,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n extern struct trace_key trace_setup_key;\n \n-void trace_repo_setup(const char *prefix);\n+void trace_repo_setup(void);\n \n /**\n * Checks whether the trace key is enabled. Used to prevent expensive\n-- \n2.39.2\n\n"},{"id":"472395","messageId":"xmqqttzeeqlk.fsf@gitster.g","threadId":"59246","inReplyTo":"20230219002527.84315-1-mcsm224@gmail.com","subject":"Re: [PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-21T20:00:23Z","receivedAt":"2023-02-21T20:00:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Idriss Fekir writes:\n\n> From: idriss fekir \n>\n> trace_repo_setup() of trace.c is called with the argument 'prefix' from\n> only one location, run_builtin of git.c, which sets 'prefix' to the return\n> value of setup_git_directory() or setup_git_directory_gently() (a wrapper\n> of the former).\n\nThe former is the wrapper of the latter, though ;-)\n\n> Now that \"prefix\" is in startup_info there is no need for the parameter\n> of trace_repo_setup() because setup_git_directory() sets \"startup_info->prefix\"\n> to the same value it returns. It would be less confusing to use \"prefix\"\n> from startup_info instead of passing it as an argument.\n\n... but for commands with neither RUN_SETUP|RUN_SETUP_GENTLY bits\nrequested, the prefix is set it to NULL by run_builtin(), and\nsetup_git_directory. What value does startup_info->prefix get in\nthat case? If we know it will always NULL (and I suspect that is\nthe case, but I haven't followed the codepaths myself to find it out\nlately, so you should without taking my word blindly), saying that\nat the same place you mentioned the return value of setup_git_*()\nfunctions in the first paragraph would make the reasoning perfect.\n\nOtherwise, very nicely done.\n\nThanks.\n\n"},{"id":"472445","messageId":"20230222153835.9794-1-mcsm224@gmail.com","threadId":"59246","inReplyTo":"xmqqttzeeqlk.fsf@gitster.g","subject":"Re:[PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()","fromName":"Idriss Fekir","fromEmail":"mcsm224@gmail.com","sentAt":"2023-02-22T15:38:35Z","receivedAt":"2023-02-22T15:39:23Z","isPatch":true,"sender":{"key":"mcsm224@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48716714?v=4"},"body":"On 2/21/23 21:00, Junio C Hamano wrote:\n > Idriss Fekir writes:\n >\n >> From: idriss fekir \n >>\n >> trace_repo_setup() of trace.c is called with the argument 'prefix' from\n >> only one location, run_builtin of git.c, which sets 'prefix' to the\nreturn\n >> value of setup_git_directory() or setup_git_directory_gently() (a\nwrapper\n >> of the former).\n >\n > The former is the wrapper of the latter, though ;-)\nOh sorry about that, that's what i meant but wrote it in reverse.\n\n >\n >> Now that \"prefix\" is in startup_info there is no need for the parameter\n >> of trace_repo_setup() because setup_git_directory() sets\n\"startup_info->prefix\"\n >> to the same value it returns. It would be less confusing to use \"prefix\"\n >> from startup_info instead of passing it as an argument.\n >\n > ... but for commands with neither RUN_SETUP|RUN_SETUP_GENTLY bits\n > requested, the prefix is set it to NULL by run_builtin(), and\n > setup_git_directory. What value does startup_info->prefix get in\n > that case? If we know it will always NULL (and I suspect that is\n > the case, but I haven't followed the codepaths myself to find it out\n > lately, so you should without taking my word blindly), saying that\n > at the same place you mentioned the return value of setup_git_*()\n > functions in the first paragraph would make the reasoning perfect.\n >\n > Otherwise, very nicely done.\n >\n > Thanks.\n >\nI did check if startup_info->prefix is *always* set, that doesn't seem\nto be the case, but for trace_repo_setup() this doesn't matter as it will\nnever be called if neither RUN_SETUP nor RUN_SETUP_GENTLY are set\n(\"if (run_setup &&...\"). I also think there is a typo in the comment after\nthat if-statement, it should be \"/* set_git_dir()..\" instead of\n\"/* get_git_dir()...\". I have a question, why isn't startup_info initialized\nwith default values in setup.c?\n\n\nThanks a lot for your valuable feedback.\n\nPS: I'm so sorry if this email has been sent multiple times, i thought it wasn't at first.\n\nKind regards.\n"}]}