{"thread":{"id":"61679","subject":"[PATCH v3] describe: refresh the index when 'broken' flag is used","startedAt":"2024-06-25T13:36:30Z","lastAt":"2024-07-03T20:41:23Z","messageCount":32,"participants":["Abhijeet Sonar","Junio C Hamano","Karthik Nayak","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"497621","messageId":"20240625133534.223579-1-abhijeet.nkt@gmail.com","threadId":"61679","inReplyTo":null,"subject":"[PATCH v3] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-25T13:35:19Z","receivedAt":"2024-06-25T13:36:30Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"When describe is run with 'dirty' flag, we refresh the index\nto make sure it is in sync with the filesystem before\ndetermining if the working tree is dirty. However, this is\nnot done for the codepath where the 'broken' flag is used.\n\nThis causes `git describe --broken --dirty` to false\npositively report the worktree being dirty if a file has\ndifferent stat info than what is recorded in the index.\nRunning `git update-index -q --refresh` to refresh the index\nbefore running diff-index fixes the problem.\n\nAlso add tests to deliberately update stat info of a\nfile before running describe to verify it behaves correctly.\n\nReported-by: Paul Millar \nSuggested-by: Junio C Hamano \nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Abhijeet Sonar \n---\n builtin/describe.c | 11 +++++++++++\n t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..deec19b29a 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n };\n \n+static const char *update_index_args[] = {\n+\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n+};\n+\n struct commit_name {\n \tstruct hashmap_entry entry;\n \tstruct object_id peeled;\n@@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstrvec_pushv(&cp.args, update_index_args);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\t\t\trun_command(&cp);\n+\t\t\tstrvec_clear(&cp.args);\n+\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e78315d23d..6c396e7abc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n \n check_describe newer-tag-older-commit~1 --contains unique-file~2\n \n+test_expect_success 'describe --dirty with a file with changed stat' '\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n+\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty --broken >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\n\nRange-diff against v2:\n1: d60fc0fa02 ! 1: 1da5fa48d9 describe: refresh the index when 'broken' flag is used\n @@ Commit message\n Reported-by: Paul Millar \n Suggested-by: Junio C Hamano \n Helped-by: Junio C Hamano \n + Helped-by: Phillip Wood \n Signed-off-by: Abhijeet Sonar \n \n ## builtin/describe.c ##\n @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n -+\t\t\tstruct child_process update_index_cp = CHILD_PROCESS_INIT;\n -+\n -+\t\t\tstrvec_pushv(&update_index_cp.args, update_index_args);\n -+\t\t\tupdate_index_cp.git_cmd = 1;\n -+\t\t\tupdate_index_cp.no_stdin = 1;\n -+\t\t\tupdate_index_cp.no_stdout = 1;\n -+\t\t\trun_command(&update_index_cp);\n ++\t\t\tstrvec_pushv(&cp.args, update_index_args);\n ++\t\t\tcp.git_cmd = 1;\n ++\t\t\tcp.no_stdin = 1;\n ++\t\t\tcp.no_stdout = 1;\n ++\t\t\trun_command(&cp);\n ++\t\t\tstrvec_clear(&cp.args);\n +\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n @@ t/t6120-describe.sh: test_expect_success 'setup misleading taggerdates' '\n \n +test_expect_success 'describe --dirty with a file with changed stat' '\n +\tgit init stat-dirty &&\n -+\tcd stat-dirty &&\n -+\n -+\techo A >file &&\n -+\tgit add file &&\n -+\tgit commit -m A &&\n -+\tgit tag A -a -m A &&\n -+\n -+\tcat file >file.new &&\n -+\tmv file.new file &&\n -+\tgit describe --dirty >actual &&\n -+\techo \"A\" >expected &&\n -+\ttest_cmp expected actual\n -+'\n ++\t(\n ++\t\tcd stat-dirty &&\n +\n -+test_expect_success 'describe --dirty --broken with a file with changed stat' '\n -+\tgit init stat-dirty-broken &&\n -+\tcd stat-dirty-broken &&\n ++\t\techo A >file &&\n ++\t\tgit add file &&\n ++\t\tgit commit -m A &&\n ++\t\tgit tag A -a -m A &&\n +\n -+\techo A >file &&\n -+\tgit add file &&\n -+\tgit commit -m A &&\n -+\tgit tag A -a -m A &&\n ++\t\tcat file >file.new &&\n ++\t\tmv file.new file &&\n ++\t\tgit describe --dirty >actual &&\n ++\t\techo \"A\" >expected &&\n ++\t\ttest_cmp expected actual &&\n +\n -+\tcat file >file.new &&\n -+\tmv file.new file &&\n -+\tgit describe --dirty --broken >actual &&\n -+\techo \"A\" >expected &&\n -+\ttest_cmp expected actual\n ++\t\tcat file >file.new &&\n ++\t\tmv file.new file &&\n ++\t\tgit describe --dirty --broken >actual &&\n ++\t\techo \"A\" >expected &&\n ++\t\ttest_cmp expected actual\n ++\t)\n +'\n +\n test_done\n-- \n2.45.2.606.g9005149a4a.dirty\n\n"},{"id":"497624","messageId":"xmqq34p1813n.fsf@gitster.g","threadId":"61679","inReplyTo":"20240625133534.223579-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v3] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-25T15:59:40Z","receivedAt":"2024-06-25T15:59:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> \t\tif (broken) {\n> \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\tstrvec_pushv(&cp.args, update_index_args);\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.no_stdout = 1;\n> +\t\t\trun_command(&cp);\n> +\t\t\tstrvec_clear(&cp.args);\n\nWhy clear .args here?\n\nEither \"struct child_process\" is reusable after finish_command()\nthat is called as the last step of run_command() returns\nsuccessfully, or it shouldn't be reused at all. And when\nfinish_command() is called, .args as well as .env are cleared\nbecause it calls child_process_clear().\n\nI am wondering if the last part need to be more like\n\n\t...\n\tcp.no_stdout = 1;\n\tif (run_command(&cp))\n\t\tchild_process_clear(&cp);\n\n> +\n> \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n> \t\t\tcp.git_cmd = 1;\n> \t\t\tcp.no_stdin = 1;\n\nThanks.\n\n\n(#leftoverbit)\n\nOutside the scope of this patch, I'd prefer to see somebody makes\nsure that it is truly equivalent to prepare a separate and new\nstruct child_process for each run_command() call and to reuse the\nsame struct child_process after calling child_process_clear() each\ntime. It is unclear if they are equivalent in general, even though\nin this particular case I think we should be OK.\n\nThere _might_ be other things in the child_process structure that\nneed to be reset to the initial state before it can be reused, but\nare not cleared by child_process_clear(). .git_cmd and other flags\nas well as in/out/err file descriptors do not seem to be cleared,\nand other callers of run_command() may even be depending on the\ncurrent behaviour that they are kept.\n"},{"id":"497625","messageId":"xmqqy16t6m8u.fsf@gitster.g","threadId":"61679","inReplyTo":"xmqq34p1813n.fsf@gitster.g","subject":"Re: [PATCH v3] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-25T16:05:53Z","receivedAt":"2024-06-25T16:05:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> (#leftoverbit)\n>\n> Outside the scope of this patch, I'd prefer to see somebody makes\n> sure that it is truly equivalent to prepare a separate and new\n> struct child_process for each run_command() call and to reuse the\n> same struct child_process after calling child_process_clear() each\n> time. It is unclear if they are equivalent in general, even though\n> in this particular case I think we should be OK.\n>\n> There _might_ be other things in the child_process structure that\n> need to be reset to the initial state before it can be reused, but\n> are not cleared by child_process_clear(). .git_cmd and other flags\n> as well as in/out/err file descriptors do not seem to be cleared,\n> and other callers of run_command() may even be depending on the\n> current behaviour that they are kept.\n\nAhh, the reuse of the same struct came directly from Karthik's\nreview on the second iteration. I guess Karthik volunteered himself\ninto this #leftoverbit task? I am not convinced that\n\n (1) the selective clearing done by current child_process_clear() is\n the best thing we can do to make child_process reusable, and\n\n (2) among the current callers, there is nobody that depends on the\n state left by the previous use of child_process in another\n run_command() call that is left uncleared by child_process_clear().\n\nIf (1) is false, then reusing child_process structure is not quite\nsafe, and if (2) is false, updating child_process_clear() to really\nclear everything will first need to adjust some callers.\n\nThanks.\n"},{"id":"497671","messageId":"751d79f1-3cb7-45bf-a276-fc4852039e09@gmail.com","threadId":"61679","inReplyTo":"xmqq34p1813n.fsf@gitster.g","subject":"Re: [PATCH v3] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T06:11:59Z","receivedAt":"2024-06-26T06:12:07Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 25/06/24 21:29, Junio C Hamano wrote:\n\n> Why clear .args here?\n\nI overlooked the fact that run_command already clears .args if the \ncommand exits with a zero exit code. Will fix, thank you.\n\n> I am wondering if the last part need to be more like\n> \n> \t...\n> \tcp.no_stdout = 1;\n> \tif (run_command(&cp))\n> \t\tchild_process_clear(&cp);\n> \n\nThat makes sense to me, will do.\n\nThanks.\n"},{"id":"497672","messageId":"20240626064143.18945-1-abhijeet.nkt@gmail.com","threadId":"61679","inReplyTo":"xmqq34p1813n.fsf@gitster.g","subject":"[PATCH v4] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T06:37:57Z","receivedAt":"2024-06-26T06:42:16Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"When describe is run with 'dirty' flag, we refresh the index\nto make sure it is in sync with the filesystem before\ndetermining if the working tree is dirty. However, this is\nnot done for the codepath where the 'broken' flag is used.\n\nThis causes `git describe --broken --dirty` to false\npositively report the worktree being dirty if a file has\ndifferent stat info than what is recorded in the index.\nRunning `git update-index -q --refresh` to refresh the index\nbefore running diff-index fixes the problem.\n\nAlso add tests to deliberately update stat info of a\nfile before running describe to verify it behaves correctly.\n\nReported-by: Paul Millar \nSuggested-by: Junio C Hamano \nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Abhijeet Sonar \n---\n builtin/describe.c | 11 ++\n t/t6120-describe.sh | 24 +++\n ...esh-the-index-when-broken-flag-is-us.patch | 178 ++++++++++++++++++\n 3 files changed, 213 insertions(+)\n create mode 100644 v3-0001-describe-refresh-the-index-when-broken-flag-is-us.patch\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..7cb9d50b36 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n };\n \n+static const char *update_index_args[] = {\n+\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n+};\n+\n struct commit_name {\n \tstruct hashmap_entry entry;\n \tstruct object_id peeled;\n@@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstrvec_pushv(&cp.args, update_index_args);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\t\t\tif (run_command(&cp))\n+\t\t\t\tchild_process_clear(&cp);\n+\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e78315d23d..6c396e7abc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n \n check_describe newer-tag-older-commit~1 --contains unique-file~2\n \n+test_expect_success 'describe --dirty with a file with changed stat' '\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n+\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty --broken >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\ndiff --git a/v3-0001-describe-refresh-the-index-when-broken-flag-is-us.patch b/v3-0001-describe-refresh-the-index-when-broken-flag-is-us.patch\nnew file mode 100644\nindex 0000000000..22e295d5eb\n--- /dev/null\n+++ b/v3-0001-describe-refresh-the-index-when-broken-flag-is-us.patch\n@@ -0,0 +1,178 @@\n+From 1da5fa48d913e1cefb2b6e1bc58df565076ee438 Mon Sep 17 00:00:00 2001\n+From: Abhijeet Sonar \n+Date: Mon, 24 Jun 2024 02:57:59 +0530\n+Subject: [PATCH v3] describe: refresh the index when 'broken' flag is used\n+To: git@vger.kernel.org\n+\n+When describe is run with 'dirty' flag, we refresh the index\n+to make sure it is in sync with the filesystem before\n+determining if the working tree is dirty. However, this is\n+not done for the codepath where the 'broken' flag is used.\n+\n+This causes `git describe --broken --dirty` to false\n+positively report the worktree being dirty if a file has\n+different stat info than what is recorded in the index.\n+Running `git update-index -q --refresh` to refresh the index\n+before running diff-index fixes the problem.\n+\n+Also add tests to deliberately update stat info of a\n+file before running describe to verify it behaves correctly.\n+\n+Reported-by: Paul Millar \n+Suggested-by: Junio C Hamano \n+Helped-by: Junio C Hamano \n+Helped-by: Phillip Wood \n+Signed-off-by: Abhijeet Sonar \n+---\n+ builtin/describe.c | 11 +++++++++++\n+ t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n+ 2 files changed, 35 insertions(+)\n+\n+diff --git a/builtin/describe.c b/builtin/describe.c\n+index e5287eddf2..deec19b29a 100644\n+--- a/builtin/describe.c\n++++ b/builtin/describe.c\n+@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n+ \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n+ };\n+ \n++static const char *update_index_args[] = {\n++\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n++};\n++\n+ struct commit_name {\n+ \tstruct hashmap_entry entry;\n+ \tstruct object_id peeled;\n+@@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n+ \tif (argc == 0) {\n+ \t\tif (broken) {\n+ \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n++\t\t\tstrvec_pushv(&cp.args, update_index_args);\n++\t\t\tcp.git_cmd = 1;\n++\t\t\tcp.no_stdin = 1;\n++\t\t\tcp.no_stdout = 1;\n++\t\t\trun_command(&cp);\n++\t\t\tstrvec_clear(&cp.args);\n++\n+ \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n+ \t\t\tcp.git_cmd = 1;\n+ \t\t\tcp.no_stdin = 1;\n+diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n+index e78315d23d..6c396e7abc 100755\n+--- a/t/t6120-describe.sh\n++++ b/t/t6120-describe.sh\n+@@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n+ \n+ check_describe newer-tag-older-commit~1 --contains unique-file~2\n+ \n++test_expect_success 'describe --dirty with a file with changed stat' '\n++\tgit init stat-dirty &&\n++\t(\n++\t\tcd stat-dirty &&\n++\n++\t\techo A >file &&\n++\t\tgit add file &&\n++\t\tgit commit -m A &&\n++\t\tgit tag A -a -m A &&\n++\n++\t\tcat file >file.new &&\n++\t\tmv file.new file &&\n++\t\tgit describe --dirty >actual &&\n++\t\techo \"A\" >expected &&\n++\t\ttest_cmp expected actual &&\n++\n++\t\tcat file >file.new &&\n++\t\tmv file.new file &&\n++\t\tgit describe --dirty --broken >actual &&\n++\t\techo \"A\" >expected &&\n++\t\ttest_cmp expected actual\n++\t)\n++'\n++\n+ test_done\n+\n+Range-diff against v2:\n+1: d60fc0fa02 ! 1: 1da5fa48d9 describe: refresh the index when 'broken' flag is used\n+ @@ Commit message\n+ Reported-by: Paul Millar \n+ Suggested-by: Junio C Hamano \n+ Helped-by: Junio C Hamano \n+ + Helped-by: Phillip Wood \n+ Signed-off-by: Abhijeet Sonar \n+ \n+ ## builtin/describe.c ##\n+ @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n+ \tif (argc == 0) {\n+ \t\tif (broken) {\n+ \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+ -+\t\t\tstruct child_process update_index_cp = CHILD_PROCESS_INIT;\n+ -+\n+ -+\t\t\tstrvec_pushv(&update_index_cp.args, update_index_args);\n+ -+\t\t\tupdate_index_cp.git_cmd = 1;\n+ -+\t\t\tupdate_index_cp.no_stdin = 1;\n+ -+\t\t\tupdate_index_cp.no_stdout = 1;\n+ -+\t\t\trun_command(&update_index_cp);\n+ ++\t\t\tstrvec_pushv(&cp.args, update_index_args);\n+ ++\t\t\tcp.git_cmd = 1;\n+ ++\t\t\tcp.no_stdin = 1;\n+ ++\t\t\tcp.no_stdout = 1;\n+ ++\t\t\trun_command(&cp);\n+ ++\t\t\tstrvec_clear(&cp.args);\n+ +\n+ \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n+ \t\t\tcp.git_cmd = 1;\n+ @@ t/t6120-describe.sh: test_expect_success 'setup misleading taggerdates' '\n+ \n+ +test_expect_success 'describe --dirty with a file with changed stat' '\n+ +\tgit init stat-dirty &&\n+ -+\tcd stat-dirty &&\n+ -+\n+ -+\techo A >file &&\n+ -+\tgit add file &&\n+ -+\tgit commit -m A &&\n+ -+\tgit tag A -a -m A &&\n+ -+\n+ -+\tcat file >file.new &&\n+ -+\tmv file.new file &&\n+ -+\tgit describe --dirty >actual &&\n+ -+\techo \"A\" >expected &&\n+ -+\ttest_cmp expected actual\n+ -+'\n+ ++\t(\n+ ++\t\tcd stat-dirty &&\n+ +\n+ -+test_expect_success 'describe --dirty --broken with a file with changed stat' '\n+ -+\tgit init stat-dirty-broken &&\n+ -+\tcd stat-dirty-broken &&\n+ ++\t\techo A >file &&\n+ ++\t\tgit add file &&\n+ ++\t\tgit commit -m A &&\n+ ++\t\tgit tag A -a -m A &&\n+ +\n+ -+\techo A >file &&\n+ -+\tgit add file &&\n+ -+\tgit commit -m A &&\n+ -+\tgit tag A -a -m A &&\n+ ++\t\tcat file >file.new &&\n+ ++\t\tmv file.new file &&\n+ ++\t\tgit describe --dirty >actual &&\n+ ++\t\techo \"A\" >expected &&\n+ ++\t\ttest_cmp expected actual &&\n+ +\n+ -+\tcat file >file.new &&\n+ -+\tmv file.new file &&\n+ -+\tgit describe --dirty --broken >actual &&\n+ -+\techo \"A\" >expected &&\n+ -+\ttest_cmp expected actual\n+ ++\t\tcat file >file.new &&\n+ ++\t\tmv file.new file &&\n+ ++\t\tgit describe --dirty --broken >actual &&\n+ ++\t\techo \"A\" >expected &&\n+ ++\t\ttest_cmp expected actual\n+ ++\t)\n+ +'\n+ +\n+ test_done\n+-- \n+2.45.2.606.g9005149a4a.dirty\n+\n\nRange-diff against v3:\n1: 1da5fa48d9 ! 1: 9ff85435b1 describe: refresh the index when 'broken' flag is used\n @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n +\t\t\tcp.git_cmd = 1;\n +\t\t\tcp.no_stdin = 1;\n +\t\t\tcp.no_stdout = 1;\n -+\t\t\trun_command(&cp);\n -+\t\t\tstrvec_clear(&cp.args);\n ++\t\t\tif (run_command(&cp))\n ++\t\t\t\tchild_process_clear(&cp);\n +\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n @@ t/t6120-describe.sh: test_expect_success 'setup misleading taggerdates' '\n +'\n +\n test_done\n +\n + ## v3-0001-describe-refresh-the-index-when-broken-flag-is-us.patch (new) ##\n +@@\n ++From 1da5fa48d913e1cefb2b6e1bc58df565076ee438 Mon Sep 17 00:00:00 2001\n ++From: Abhijeet Sonar \n ++Date: Mon, 24 Jun 2024 02:57:59 +0530\n ++Subject: [PATCH v3] describe: refresh the index when 'broken' flag is used\n ++To: git@vger.kernel.org\n ++\n ++When describe is run with 'dirty' flag, we refresh the index\n ++to make sure it is in sync with the filesystem before\n ++determining if the working tree is dirty. However, this is\n ++not done for the codepath where the 'broken' flag is used.\n ++\n ++This causes `git describe --broken --dirty` to false\n ++positively report the worktree being dirty if a file has\n ++different stat info than what is recorded in the index.\n ++Running `git update-index -q --refresh` to refresh the index\n ++before running diff-index fixes the problem.\n ++\n ++Also add tests to deliberately update stat info of a\n ++file before running describe to verify it behaves correctly.\n ++\n ++Reported-by: Paul Millar \n ++Suggested-by: Junio C Hamano \n ++Helped-by: Junio C Hamano \n ++Helped-by: Phillip Wood \n ++Signed-off-by: Abhijeet Sonar \n ++---\n ++ builtin/describe.c | 11 +++++++++++\n ++ t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n ++ 2 files changed, 35 insertions(+)\n ++\n ++diff --git a/builtin/describe.c b/builtin/describe.c\n ++index e5287eddf2..deec19b29a 100644\n ++--- a/builtin/describe.c\n +++++ b/builtin/describe.c\n ++@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n ++ \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n ++ };\n ++ \n +++static const char *update_index_args[] = {\n +++\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n +++};\n +++\n ++ struct commit_name {\n ++ \tstruct hashmap_entry entry;\n ++ \tstruct object_id peeled;\n ++@@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n ++ \tif (argc == 0) {\n ++ \t\tif (broken) {\n ++ \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n +++\t\t\tstrvec_pushv(&cp.args, update_index_args);\n +++\t\t\tcp.git_cmd = 1;\n +++\t\t\tcp.no_stdin = 1;\n +++\t\t\tcp.no_stdout = 1;\n +++\t\t\trun_command(&cp);\n +++\t\t\tstrvec_clear(&cp.args);\n +++\n ++ \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n ++ \t\t\tcp.git_cmd = 1;\n ++ \t\t\tcp.no_stdin = 1;\n ++diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n ++index e78315d23d..6c396e7abc 100755\n ++--- a/t/t6120-describe.sh\n +++++ b/t/t6120-describe.sh\n ++@@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n ++ \n ++ check_describe newer-tag-older-commit~1 --contains unique-file~2\n ++ \n +++test_expect_success 'describe --dirty with a file with changed stat' '\n +++\tgit init stat-dirty &&\n +++\t(\n +++\t\tcd stat-dirty &&\n +++\n +++\t\techo A >file &&\n +++\t\tgit add file &&\n +++\t\tgit commit -m A &&\n +++\t\tgit tag A -a -m A &&\n +++\n +++\t\tcat file >file.new &&\n +++\t\tmv file.new file &&\n +++\t\tgit describe --dirty >actual &&\n +++\t\techo \"A\" >expected &&\n +++\t\ttest_cmp expected actual &&\n +++\n +++\t\tcat file >file.new &&\n +++\t\tmv file.new file &&\n +++\t\tgit describe --dirty --broken >actual &&\n +++\t\techo \"A\" >expected &&\n +++\t\ttest_cmp expected actual\n +++\t)\n +++'\n +++\n ++ test_done\n ++\n ++Range-diff against v2:\n ++1: d60fc0fa02 ! 1: 1da5fa48d9 describe: refresh the index when 'broken' flag is used\n ++ @@ Commit message\n ++ Reported-by: Paul Millar \n ++ Suggested-by: Junio C Hamano \n ++ Helped-by: Junio C Hamano \n ++ + Helped-by: Phillip Wood \n ++ Signed-off-by: Abhijeet Sonar \n ++ \n ++ ## builtin/describe.c ##\n ++ @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n ++ \tif (argc == 0) {\n ++ \t\tif (broken) {\n ++ \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n ++ -+\t\t\tstruct child_process update_index_cp = CHILD_PROCESS_INIT;\n ++ -+\n ++ -+\t\t\tstrvec_pushv(&update_index_cp.args, update_index_args);\n ++ -+\t\t\tupdate_index_cp.git_cmd = 1;\n ++ -+\t\t\tupdate_index_cp.no_stdin = 1;\n ++ -+\t\t\tupdate_index_cp.no_stdout = 1;\n ++ -+\t\t\trun_command(&update_index_cp);\n ++ ++\t\t\tstrvec_pushv(&cp.args, update_index_args);\n ++ ++\t\t\tcp.git_cmd = 1;\n ++ ++\t\t\tcp.no_stdin = 1;\n ++ ++\t\t\tcp.no_stdout = 1;\n ++ ++\t\t\trun_command(&cp);\n ++ ++\t\t\tstrvec_clear(&cp.args);\n ++ +\n ++ \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n ++ \t\t\tcp.git_cmd = 1;\n ++ @@ t/t6120-describe.sh: test_expect_success 'setup misleading taggerdates' '\n ++ \n ++ +test_expect_success 'describe --dirty with a file with changed stat' '\n ++ +\tgit init stat-dirty &&\n ++ -+\tcd stat-dirty &&\n ++ -+\n ++ -+\techo A >file &&\n ++ -+\tgit add file &&\n ++ -+\tgit commit -m A &&\n ++ -+\tgit tag A -a -m A &&\n ++ -+\n ++ -+\tcat file >file.new &&\n ++ -+\tmv file.new file &&\n ++ -+\tgit describe --dirty >actual &&\n ++ -+\techo \"A\" >expected &&\n ++ -+\ttest_cmp expected actual\n ++ -+'\n ++ ++\t(\n ++ ++\t\tcd stat-dirty &&\n ++ +\n ++ -+test_expect_success 'describe --dirty --broken with a file with changed stat' '\n ++ -+\tgit init stat-dirty-broken &&\n ++ -+\tcd stat-dirty-broken &&\n ++ ++\t\techo A >file &&\n ++ ++\t\tgit add file &&\n ++ ++\t\tgit commit -m A &&\n ++ ++\t\tgit tag A -a -m A &&\n ++ +\n ++ -+\techo A >file &&\n ++ -+\tgit add file &&\n ++ -+\tgit commit -m A &&\n ++ -+\tgit tag A -a -m A &&\n ++ ++\t\tcat file >file.new &&\n ++ ++\t\tmv file.new file &&\n ++ ++\t\tgit describe --dirty >actual &&\n ++ ++\t\techo \"A\" >expected &&\n ++ ++\t\ttest_cmp expected actual &&\n ++ +\n ++ -+\tcat file >file.new &&\n ++ -+\tmv file.new file &&\n ++ -+\tgit describe --dirty --broken >actual &&\n ++ -+\techo \"A\" >expected &&\n ++ -+\ttest_cmp expected actual\n ++ ++\t\tcat file >file.new &&\n ++ ++\t\tmv file.new file &&\n ++ ++\t\tgit describe --dirty --broken >actual &&\n ++ ++\t\techo \"A\" >expected &&\n ++ ++\t\ttest_cmp expected actual\n ++ ++\t)\n ++ +'\n ++ +\n ++ test_done\n ++-- \n ++2.45.2.606.g9005149a4a.dirty\n ++\n-- \n2.45.2.606.g9005149a4a.dirty\n\n"},{"id":"497673","messageId":"272afd08-e2b3-4d4a-81d2-8ccc80d87dd7@gmail.com","threadId":"61679","inReplyTo":"20240626064143.18945-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v4] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T06:50:03Z","receivedAt":"2024-06-26T06:50:08Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"Apologies, please ignore the above patch as it also includes the patch \nfiles I commited by mistake.\n"},{"id":"497674","messageId":"20240626065223.28154-1-abhijeet.nkt@gmail.com","threadId":"61679","inReplyTo":"xmqq34p1813n.fsf@gitster.g","subject":"[PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T06:52:11Z","receivedAt":"2024-06-26T06:53:04Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"When describe is run with 'dirty' flag, we refresh the index\nto make sure it is in sync with the filesystem before\ndetermining if the working tree is dirty. However, this is\nnot done for the codepath where the 'broken' flag is used.\n\nThis causes `git describe --broken --dirty` to false\npositively report the worktree being dirty if a file has\ndifferent stat info than what is recorded in the index.\nRunning `git update-index -q --refresh` to refresh the index\nbefore running diff-index fixes the problem.\n\nAlso add tests to deliberately update stat info of a\nfile before running describe to verify it behaves correctly.\n\nReported-by: Paul Millar \nSuggested-by: Junio C Hamano \nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Abhijeet Sonar \n---\n builtin/describe.c | 11 +++++++++++\n t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..7cb9d50b36 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n };\n \n+static const char *update_index_args[] = {\n+\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n+};\n+\n struct commit_name {\n \tstruct hashmap_entry entry;\n \tstruct object_id peeled;\n@@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstrvec_pushv(&cp.args, update_index_args);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\t\t\tif (run_command(&cp))\n+\t\t\t\tchild_process_clear(&cp);\n+\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e78315d23d..6c396e7abc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n \n check_describe newer-tag-older-commit~1 --contains unique-file~2\n \n+test_expect_success 'describe --dirty with a file with changed stat' '\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n+\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual &&\n+\n+\t\tcat file >file.new &&\n+\t\tmv file.new file &&\n+\t\tgit describe --dirty --broken >actual &&\n+\t\techo \"A\" >expected &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\n\nRange-diff against v4:\n1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used\n @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n +\t\t\tcp.git_cmd = 1;\n +\t\t\tcp.no_stdin = 1;\n +\t\t\tcp.no_stdout = 1;\n -+\t\t\trun_command(&cp);\n -+\t\t\tstrvec_clear(&cp.args);\n ++\t\t\tif (run_command(&cp))\n ++\t\t\t\tchild_process_clear(&cp);\n +\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n-- \n2.45.2.606.g9005149a4a.dirty\n\n"},{"id":"497677","messageId":"CAOLa=ZTPm9CjMMyQ+nr8vLCSnEhnQMZwcMyu+7qbNKzo3ymM9w@mail.gmail.com","threadId":"61679","inReplyTo":"xmqqy16t6m8u.fsf@gitster.g","subject":"Re: [PATCH v3] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-26T11:16:35Z","receivedAt":"2024-06-26T11:16:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> (#leftoverbit)\n>>\n>> Outside the scope of this patch, I'd prefer to see somebody makes\n>> sure that it is truly equivalent to prepare a separate and new\n>> struct child_process for each run_command() call and to reuse the\n>> same struct child_process after calling child_process_clear() each\n>> time. It is unclear if they are equivalent in general, even though\n>> in this particular case I think we should be OK.\n>>\n>> There _might_ be other things in the child_process structure that\n>> need to be reset to the initial state before it can be reused, but\n>> are not cleared by child_process_clear(). .git_cmd and other flags\n>> as well as in/out/err file descriptors do not seem to be cleared,\n>> and other callers of run_command() may even be depending on the\n>> current behaviour that they are kept.\n>\n> Ahh, the reuse of the same struct came directly from Karthik's\n> review on the second iteration. I guess Karthik volunteered himself\n> into this #leftoverbit task? I am not convinced that\n>\n\nHehe. I'll take it up!\n\n> (1) the selective clearing done by current child_process_clear() is\n> the best thing we can do to make child_process reusable, and\n>\n> (2) among the current callers, there is nobody that depends on the\n> state left by the previous use of child_process in another\n> run_command() call that is left uncleared by child_process_clear().\n>\n> If (1) is false, then reusing child_process structure is not quite\n> safe, and if (2) is false, updating child_process_clear() to really\n> clear everything will first need to adjust some callers.\n>\n> Thanks.\n>\n\nI think it would be best to write some unit tests to capture the current\nbehavior and based on the findings and as you suggested, we can decide\nthe path forward.\n\nKarthik\n"},{"id":"497678","messageId":"CAOLa=ZRz2KEGiBnX1YP6JG1nXXHLfw9A3dHKO3s_ViLhq+bWww@mail.gmail.com","threadId":"61679","inReplyTo":"20240626065223.28154-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-26T11:30:19Z","receivedAt":"2024-06-26T11:30:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Abhijeet Sonar writes:\n\n> When describe is run with 'dirty' flag, we refresh the index\n> to make sure it is in sync with the filesystem before\n> determining if the working tree is dirty. However, this is\n> not done for the codepath where the 'broken' flag is used.\n>\n> This causes `git describe --broken --dirty` to false\n> positively report the worktree being dirty if a file has\n> different stat info than what is recorded in the index.\n> Running `git update-index -q --refresh` to refresh the index\n> before running diff-index fixes the problem.\n>\n> Also add tests to deliberately update stat info of a\n> file before running describe to verify it behaves correctly.\n>\n> Reported-by: Paul Millar \n> Suggested-by: Junio C Hamano \n> Helped-by: Junio C Hamano \n> Helped-by: Phillip Wood \n> Signed-off-by: Abhijeet Sonar \n> ---\n> builtin/describe.c | 11 +++++++++++\n> t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 35 insertions(+)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index e5287eddf2..7cb9d50b36 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n> \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n> };\n>\n> +static const char *update_index_args[] = {\n> +\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n> +};\n> +\n> struct commit_name {\n> \tstruct hashmap_entry entry;\n> \tstruct object_id peeled;\n> @@ -645,6 +649,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \tif (argc == 0) {\n> \t\tif (broken) {\n> \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\tstrvec_pushv(&cp.args, update_index_args);\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.no_stdout = 1;\n> +\t\t\tif (run_command(&cp))\n> +\t\t\t\tchild_process_clear(&cp);\n> +\n> \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n> \t\t\tcp.git_cmd = 1;\n> \t\t\tcp.no_stdin = 1;\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index e78315d23d..6c396e7abc 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -671,4 +671,28 @@ test_expect_success 'setup misleading taggerdates' '\n>\n> check_describe newer-tag-older-commit~1 --contains unique-file~2\n>\n> +test_expect_success 'describe --dirty with a file with changed stat' '\n> +\tgit init stat-dirty &&\n> +\t(\n> +\t\tcd stat-dirty &&\n> +\n> +\t\techo A >file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m A &&\n> +\t\tgit tag A -a -m A &&\n> +\n> +\t\tcat file >file.new &&\n> +\t\tmv file.new file &&\n> +\t\tgit describe --dirty >actual &&\n> +\t\techo \"A\" >expected &&\n> +\t\ttest_cmp expected actual &&\n> +\n> +\t\tcat file >file.new &&\n> +\t\tmv file.new file &&\n> +\t\tgit describe --dirty --broken >actual &&\n> +\t\techo \"A\" >expected &&\n> +\t\ttest_cmp expected actual\n\nNot worth a reroll, but you don't have to create file.new twice.\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 6c396e7abc..6c4b20fec7 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -683,14 +683,12 @@ test_expect_success 'describe --dirty with a\nfile with changed stat' '\n\n \t\tcat file >file.new &&\n \t\tmv file.new file &&\n-\t\tgit describe --dirty >actual &&\n \t\techo \"A\" >expected &&\n+\n+\t\tgit describe --dirty >actual &&\n \t\ttest_cmp expected actual &&\n\n-\t\tcat file >file.new &&\n-\t\tmv file.new file &&\n \t\tgit describe --dirty --broken >actual &&\n-\t\techo \"A\" >expected &&\n \t\ttest_cmp expected actual\n \t)\n '\n\n> +\t)\n> +'\n> +\n> test_done\n>\n> Range-diff against v4:\n> 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used\n> @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.no_stdout = 1;\n> -+\t\t\trun_command(&cp);\n> -+\t\t\tstrvec_clear(&cp.args);\n> ++\t\t\tif (run_command(&cp))\n> ++\t\t\t\tchild_process_clear(&cp);\n> +\n> \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n> \t\t\tcp.git_cmd = 1;\n> --\n> 2.45.2.606.g9005149a4a.dirty\n\nOther than this, this looks good to me.\n\nThanks!\n"},{"id":"497682","messageId":"2e80306e-2474-4254-95eb-c2902a56ffdd@gmail.com","threadId":"61679","inReplyTo":"CAOLa=ZRz2KEGiBnX1YP6JG1nXXHLfw9A3dHKO3s_ViLhq+bWww@mail.gmail.com","subject":"Re: [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T12:06:25Z","receivedAt":"2024-06-26T12:06:31Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 26/06/24 17:00, Karthik Nayak wrote:\n> Not worth a reroll, but you don't have to create file.new twice.\n\nActually, now that I think of it, those two were better off being \nseparate tests. It might so happen the first call to describe refreshes \nthe index, due to which the second call with the --broken option does \nnot bug-out in the way it would if the command was run by itself. \nHaving them separate would give them enough isolation so that previous \ncommand does not interfere with the later.\n\n>> Range-diff against v4:\n>> 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used\n>> @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n>> +\t\t\tcp.git_cmd = 1;\n>> +\t\t\tcp.no_stdin = 1;\n>> +\t\t\tcp.no_stdout = 1;\n>> -+\t\t\trun_command(&cp);\n>> -+\t\t\tstrvec_clear(&cp.args);\n>> ++\t\t\tif (run_command(&cp))\n>> ++\t\t\t\tchild_process_clear(&cp);\n>> +\n>> \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n>> \t\t\tcp.git_cmd = 1;\n>> --\n>> 2.45.2.606.g9005149a4a.dirty\n> \n> Other than this, this looks good to me.\nI am not sure if I follow this one. Am I expected to not share the \nstruct child_process between the two sub-process calls?\n\nThanks\n\n"},{"id":"497703","messageId":"xmqqmsn7698h.fsf@gitster.g","threadId":"61679","inReplyTo":"CAOLa=ZRz2KEGiBnX1YP6JG1nXXHLfw9A3dHKO3s_ViLhq+bWww@mail.gmail.com","subject":"Re: [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T14:59:10Z","receivedAt":"2024-06-26T14:59:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> +\t\tcat file >file.new &&\n>> +\t\tmv file.new file &&\n>> +\t\tgit describe --dirty >actual &&\n>> +\t\techo \"A\" >expected &&\n>> +\t\ttest_cmp expected actual &&\n>> +\n>> +\t\tcat file >file.new &&\n>> +\t\tmv file.new file &&\n>> +\t\tgit describe --dirty --broken >actual &&\n>> +\t\techo \"A\" >expected &&\n>> +\t\ttest_cmp expected actual\n>\n> Not worth a reroll, but you don't have to create file.new twice.\n\nI think you have to make the \"file\" stat-dirty again after the first\ntest, as a successful first test _should_ refresh the index (and\nthat was why my manual illustration in an earlier message\n did \"--dirty --broken\" first before\n\"--dirty\" alone, knowing that the former fails to refresh the\nindex).\n\nYou are right to point out that the expected results for \"--dirty\"\nand \"--dirty --broken\" are the same and we do not have to create it\ntwice, though.\n\nIf the filesystem has a usable inode number, then replacing \"file\"\nwith a copy, i.e. \"cat file >file.new && mv file.new file\", is an\nexcellent way to ensure that the stat data becomes dirty even when\nit is done as a part of a quick series of commands. But not all\nfilesystems we run our tests on may have usable inode number, so it\nmay not be the best thing to do in an automated test. We could use\n\"test-tool chmtime\" instead, perhaps like:\n\n ... make the index and the working tree are clean wrt HEAD ...\n # we do not expect -dirty suffix in the output\n echo A >expect &&\n\n # make \"file\" stat-dirty\n test-tool chmtime -10 file &&\n # \"describe --dirty\" refreshes and does not get fooled\n git describe --dirty >actual &&\n test_cmp expect actual &&\n\n # make \"file\" stat-dirty again\n test-tool chmtime -10 file &&\n # \"describe --dirty --broken\" refreshes and does not get fooled\n git describe --dirty --broken >actual &&\n test_cmp expect actual\n\nwith the extra comments stripped (I added them only to explain what\nis going on to the readers of this e-mail message).\n"},{"id":"497707","messageId":"xmqqikxv4t1v.fsf_-_@gitster.g","threadId":"61679","inReplyTo":"2e80306e-2474-4254-95eb-c2902a56ffdd@gmail.com","subject":"Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T15:34:04Z","receivedAt":"2024-06-26T15:34:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> On 26/06/24 17:00, Karthik Nayak wrote:\n>> Not worth a reroll, but you don't have to create file.new twice.\n>\n> Actually, now that I think of it, those two were better off being\n> separate tests. It might so happen the first call to describe\n> refreshes the index, due to which the second call with the --broken\n> option does not bug-out in the way it would if the command was run by\n> itself. Having them separate would give them enough isolation so that\n> previous command does not interfere with the later.\n\nGood thinking. Yes, we may end up having a few commands that are\nduplicated in these two tests (for setting the stage up, for\nexample), but it would be better to test these two separately.\n\n>>> Range-diff against v4:\n>>> 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used\n>>> @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr\n>>> +\t\t\tcp.git_cmd = 1;\n>>> +\t\t\tcp.no_stdin = 1;\n>>> +\t\t\tcp.no_stdout = 1;\n>>> -+\t\t\trun_command(&cp);\n>>> -+\t\t\tstrvec_clear(&cp.args);\n>>> ++\t\t\tif (run_command(&cp))\n>>> ++\t\t\t\tchild_process_clear(&cp);\n>>> +\n>>> \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n>>> \t\t\tcp.git_cmd = 1;\n>>> --\n>>> 2.45.2.606.g9005149a4a.dirty\n>> Other than this, this looks good to me.\n> I am not sure if I follow this one. Am I expected to not share the\n> struct child_process between the two sub-process calls?\n\nWithout reusing and instead of using two, we do not have to worry\nabout the reusablility of the child_process structure in the first\nplace, which is a huge plus, but in the longer run we should make\nsure it is safe to reuse child_process and document the safe way to\nreuse it (run-command.h does document a way to use it once and then\nclean it up, but the \"clean-up\" extends only to not leaking\nresources after we are done---it does not guarantee that it is OK to\nreuse it).\n\nI think with the updated \"we clear cp ourselves if run_command() fails\",\nit should be safe to reuse, but it probably is even safer to do\nsomething like this:\n\n\t... the first run ...\n\tif (run_command(&cp))\n\t\tchild_process_clear(&cp);\n\n\tchild_process_init(&cp);\n\t\n ... setup for the second run ...\n\tstrvec_pushv(&cp.args, diff_index_args);\n\tcp.git_cmd = 1;\n\t... full set-up without relying on anything done earlier ...\n\nThe extra child_process_init() call may serve as an extra\ndocumentation that we are reusing the same struct here (we often do\n\"git grep\" for use of a specific API function before tree wide code\nclean-up, and child_process_init() would be a good key to look for).\n\n... goes and looks ...\n\nOh, I found an interesting one. builtin/fsck.c:cmd_fsck() does this\nin a loop:\n\n\tstruct child_process verify = CHILD_PROCESS_INIT;\n\n\t... setup ...\n\tfor (... loop ...) {\n\t\tchild_process_init(&verify);\n\t\t... set up various .members of verify struct ...\n\t\tstrvec_pushl(&verify.args, ... command line ...);\n\t\tif (run_command(&verify))\n\t\t\terrors_found |= ...;\n\t}\n\nThis code clearly assumes that it is safe to reuse the child_process\nstructure after you run_command() and let it clean-up if you do\nanother child_process_init(). And I think that is a sensible\nassumption.\n\nThe code in builtin/fsck.c:cmd_fsck() is buggy when run_command()\nfails, I think. Without doing child_process_clear() there, doesn't\nit leak the strvec?\n\n------- >8 ------------- >8 ------------- >8 -------\nSubject: [PATCH] fsck: clear child_process after failed run_command()\n\nThere are two loops that calls run_command() using the same\nchild_process struct near the end of cmd_fsck(). 4d0984be (fsck: do\nnot reuse child_process structs, 2018-11-12) tightened these code\npaths to reinitialize the structure in order to safely reuse it.\n\n The run-command API makes no promises about what is left in a struct\n child_process after a command finishes, and it's not safe to simply\n reuse it again for a similar command.\n\nUpon failure, run_command() can return without releasing the\nresource held by the child_process structure, which is done by\ncalling finish_command() which in turn calls child_process_clear().\n\nReinitializing the structure without calling child_process_clear()\nfor the next round would leak the .args and .env strvecs.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/fsck.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/fsck.c w/builtin/fsck.c\nindex d13a226c2e..398b492184 100644\n--- c/builtin/fsck.c\n+++ w/builtin/fsck.c\n@@ -1078,8 +1078,10 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_push(&commit_graph_verify.args, \"--progress\");\n \t\t\telse\n \t\t\t\tstrvec_push(&commit_graph_verify.args, \"--no-progress\");\n-\t\t\tif (run_command(&commit_graph_verify))\n+\t\t\tif (run_command(&commit_graph_verify)) {\n+\t\t\t\tchild_process_clear(&commit_graph_verify);\n \t\t\t\terrors_found |= ERROR_COMMIT_GRAPH;\n+\t\t\t}\n \t\t}\n \t}\n \n@@ -1096,8 +1098,10 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_push(&midx_verify.args, \"--progress\");\n \t\t\telse\n \t\t\t\tstrvec_push(&midx_verify.args, \"--no-progress\");\n-\t\t\tif (run_command(&midx_verify))\n+\t\t\tif (run_command(&midx_verify)) {\n+\t\t\t\tchild_process_clear(&midx_verify);\n \t\t\t\terrors_found |= ERROR_MULTI_PACK_INDEX;\n+\t\t\t}\n \t\t}\n \t}\n \n\n\n\n\n\n"},{"id":"497708","messageId":"xmqqcyo33cgu.fsf@gitster.g","threadId":"61679","inReplyTo":"xmqqikxv4t1v.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T16:17:37Z","receivedAt":"2024-06-26T16:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Subject: [PATCH] fsck: clear child_process after failed run_command()\n>\n> There are two loops that calls run_command() using the same\n> child_process struct near the end of cmd_fsck(). 4d0984be (fsck: do\n> not reuse child_process structs, 2018-11-12) tightened these code\n> paths to reinitialize the structure in order to safely reuse it.\n>\n> The run-command API makes no promises about what is left in a struct\n> child_process after a command finishes, and it's not safe to simply\n> reuse it again for a similar command.\n>\n> Upon failure, run_command() can return without releasing the\n> resource held by the child_process structure, which is done by\n> calling finish_command() which in turn calls child_process_clear().\n\nSorry, but I have to take this back. \n\nThe error return code paths in the start_command() function does\nseem to call clear_child_process(), which means that there is no\nneed to call clear_child_process() ourselves after a failed\nrun_command() call.\n\nThe need for reinitializing that established by 4d0984be (fsck: do\nnot reuse child_process structs, 2018-11-12) still stand, so\n\n\t... the first run ...\n\trun_command(&cp);\n\t/* no need for separate child_process_clear(&cp) */\n\n\tchild_process_init(&cp); /* prepare for reuse */\n\t\n ... setup for the second run ...\n\tstrvec_pushv(&cp.args, diff_index_args);\n\tcp.git_cmd = 1;\n\t... full set-up without relying on anything done earlier ...\n\nwould still be needed.\n\nOr alternatively, we could do this to ensure that the child_process\nstructure is always reusable.\n\n run-command.c | 1 +\n run-command.h | 6 +++++-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git c/run-command.c w/run-command.c\nindex 6ac1d14516..aba250fbe1 100644\n--- c/run-command.c\n+++ w/run-command.c\n@@ -26,6 +26,7 @@ void child_process_clear(struct child_process *child)\n {\n \tstrvec_clear(&child->args);\n \tstrvec_clear(&child->env);\n+\tchild_process_init(child);\n }\n \n struct child_to_clean {\ndiff --git c/run-command.h w/run-command.h\nindex 55f6631a2a..6e203c22f6 100644\n--- c/run-command.h\n+++ w/run-command.h\n@@ -204,7 +204,8 @@ int start_command(struct child_process *);\n \n /**\n * Wait for the completion of a sub-process that was started with\n- * start_command().\n+ * start_command(). The child_process structure is cleared and\n+ * reinitialized.\n */\n int finish_command(struct child_process *);\n \n@@ -214,6 +215,9 @@ int finish_command_in_signal(struct child_process *);\n * A convenience function that encapsulates a sequence of\n * start_command() followed by finish_command(). Takes a pointer\n * to a `struct child_process` that specifies the details.\n+ * The child_process structure is cleared and reinitialized,\n+ * even when the command fails to start or an error is detected\n+ * in finish_command().\n */\n int run_command(struct child_process *);\n \n\n"},{"id":"497710","messageId":"bbc223a3-2c82-4108-adf1-5e8518ff776e@gmail.com","threadId":"61679","inReplyTo":"xmqqcyo33cgu.fsf@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T17:29:22Z","receivedAt":"2024-06-26T17:29:28Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 26/06/24 21:47, Junio C Hamano wrote:\n> Or alternatively, we could do this to ensure that the child_process\n> structure is always reusable.\n> \n> run-command.c | 1 +\n> run-command.h | 6 +++++-\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git c/run-command.c w/run-command.c\n> index 6ac1d14516..aba250fbe1 100644\n> --- c/run-command.c\n> +++ w/run-command.c\n> @@ -26,6 +26,7 @@ void child_process_clear(struct child_process *child)\n> {\n> \tstrvec_clear(&child->args);\n> \tstrvec_clear(&child->env);\n> +\tchild_process_init(child);\n> }\n\nTo me, this looks much better. child_process_clear's name already\nsuggests that is sort of like a destructor, so it makes sense to\nre-initialize everything here. I even wonder why it was not that way to\nbegin with. I suppose no callers are assuming that it only clears args\nand env though?\n\n> struct child_to_clean {\n> diff --git c/run-command.h w/run-command.h\n> index 55f6631a2a..6e203c22f6 100644\n> --- c/run-command.h\n> +++ w/run-command.h\n> @@ -204,7 +204,8 @@ int start_command(struct child_process *);\n> \n> /**\n> * Wait for the completion of a sub-process that was started with\n> - * start_command().\n> + * start_command(). The child_process structure is cleared and\n> + * reinitialized.\n> */\n> int finish_command(struct child_process *);\n> \n> @@ -214,6 +215,9 @@ int finish_command_in_signal(struct child_process *);\n> * A convenience function that encapsulates a sequence of\n> * start_command() followed by finish_command(). Takes a pointer\n> * to a `struct child_process` that specifies the details.\n> + * The child_process structure is cleared and reinitialized,\n> + * even when the command fails to start or an error is detected\n> + * in finish_command().\n> */\n> int run_command(struct child_process *);\n> \n\n\n"},{"id":"497711","messageId":"xmqqsewz1ua5.fsf@gitster.g","threadId":"61679","inReplyTo":"bbc223a3-2c82-4108-adf1-5e8518ff776e@gmail.com","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T17:35:46Z","receivedAt":"2024-06-26T17:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> To me, this looks much better. child_process_clear's name already\n> suggests that is sort of like a destructor, so it makes sense to\n> re-initialize everything here. I even wonder why it was not that way to\n> begin with. I suppose no callers are assuming that it only clears args\n> and env though?\n\nI guess that validating that supposition is a prerequisite to\ndeclare the change as \"much better\" and \"makes sense\".\n\n"},{"id":"497712","messageId":"xmqqmsn71ttw.fsf@gitster.g","threadId":"61679","inReplyTo":"xmqqsewz1ua5.fsf@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T17:45:31Z","receivedAt":"2024-06-26T17:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Abhijeet Sonar writes:\n>\n>> To me, this looks much better. child_process_clear's name already\n>> suggests that is sort of like a destructor, so it makes sense to\n>> re-initialize everything here. I even wonder why it was not that way to\n>> begin with. I suppose no callers are assuming that it only clears args\n>> and env though?\n>\n> I guess that validating that supposition is a prerequisite to\n> declare the change as \"much better\" and \"makes sense\".\n\nHaving said that, a lot more plausible reason is that most callers\nare expected to use a single struct just once without reusing it, so\nthe cost of re-initialization is wasted cycles for them if it were\npart of child_process_clear() which is primarily about releasing\nresources (instead of leaking them).\n\nIt can be argued that it is a sensible design decision to make those\nminority callers that do reuse the same struct to pay the cost of\nreinitialization themselves, instead of the majority callers that\nuse it once and then release resources held in there.\n"},{"id":"497713","messageId":"f7d0abce-b389-45ae-992a-adbc7ec10d50@gmail.com","threadId":"61679","inReplyTo":"xmqqsewz1ua5.fsf@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T18:07:29Z","receivedAt":"2024-06-26T18:07:35Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 26/06/24 23:05, Junio C Hamano wrote:\n> Abhijeet Sonar writes:\n> \n>> To me, this looks much better. child_process_clear's name already\n>> suggests that is sort of like a destructor, so it makes sense to\n>> re-initialize everything here. I even wonder why it was not that way to\n>> begin with. I suppose no callers are assuming that it only clears args\n>> and env though?\n> \n> I guess that validating that supposition is a prerequisite to\n> declare the change as \"much better\" and \"makes sense\".\n\nOK. I found one: at the end of submodule.c:push_submodule()\n\n\tif (...) {\n\t\t...some setup...\n\t\tif (run_command(&cp))\n\t\t\treturn 0;\n\t\tclose(cp.out);\n\t}\n\n"},{"id":"497716","messageId":"xmqqpls3zhc2.fsf@gitster.g","threadId":"61679","inReplyTo":"20240626065223.28154-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T18:31:25Z","receivedAt":"2024-06-26T18:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> When describe is run with 'dirty' flag, we refresh the index\n> to make sure it is in sync with the filesystem before\n> determining if the working tree is dirty. However, this is\n> not done for the codepath where the 'broken' flag is used.\n>\n> This causes `git describe --broken --dirty` to false\n> positively report the worktree being dirty if a file has\n> different stat info than what is recorded in the index.\n> Running `git update-index -q --refresh` to refresh the index\n> before running diff-index fixes the problem.\n>\n> Also add tests to deliberately update stat info of a\n> file before running describe to verify it behaves correctly.\n>\n> Reported-by: Paul Millar \n> Suggested-by: Junio C Hamano \n> Helped-by: Junio C Hamano \n> Helped-by: Phillip Wood \n> Signed-off-by: Abhijeet Sonar \n> ---\n\nAs I screwed up with the suggestion to do child_process_clear()\nafter a failed run_command(), let me fix that up. A suggested\nchange that can be squashed into this patch is attached at the end.\n\n * (style) a blank line between a block of variable declarations and\n the first statement;\n\n * run_command(&cp) cleans cp so no need for separate\n child_process_clear(&cp);\n\n * but child_process_init(&cp) is needed, just as 4d0984be (fsck: do\n not reuse child_process structs, 2018-11-12) explains, before\n reusing the structure for a separate call.\n\n * instead of \"replace with a different file\" which relies on having\n a working inum, use \"test-tool chmtime\" to reliably force dirty\n mtime.\n\n * everybody else compares \"actual\" against \"expect\", not\n \"expected\". imitate them.\n\n * test \"--dirty\" and \"--dirty --broken\" separately in two separate\n tests.\n\nThanks.\n\n\n builtin/describe.c | 5 +++--\n t/t6120-describe.sh | 28 ++++++++++++++++++++--------\n 2 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git c/builtin/describe.c w/builtin/describe.c\nindex a58f6134f0..e936d2c19f 100644\n--- c/builtin/describe.c\n+++ w/builtin/describe.c\n@@ -649,13 +649,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n \t\t\tstrvec_pushv(&cp.args, update_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\n \t\t\tcp.no_stdout = 1;\n-\t\t\tif (run_command(&cp))\n-\t\t\t\tchild_process_clear(&cp);\n+\t\t\trun_command(&cp);\n \n+\t\t\tchild_process_init(&cp);\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\ndiff --git c/t/t6120-describe.sh w/t/t6120-describe.sh\nindex 6c396e7abc..79e0f19deb 100755\n--- c/t/t6120-describe.sh\n+++ w/t/t6120-describe.sh\n@@ -672,6 +672,7 @@ test_expect_success 'setup misleading taggerdates' '\n check_describe newer-tag-older-commit~1 --contains unique-file~2\n \n test_expect_success 'describe --dirty with a file with changed stat' '\n+\ttest_when_finished \"rm -fr stat-dirty\" &&\n \tgit init stat-dirty &&\n \t(\n \t\tcd stat-dirty &&\n@@ -680,18 +681,29 @@ test_expect_success 'describe --dirty with a file with changed stat' '\n \t\tgit add file &&\n \t\tgit commit -m A &&\n \t\tgit tag A -a -m A &&\n+\t\techo \"A\" >expect &&\n \n-\t\tcat file >file.new &&\n-\t\tmv file.new file &&\n+\t\ttest-tool chmtime -10 file &&\n \t\tgit describe --dirty >actual &&\n-\t\techo \"A\" >expected &&\n-\t\ttest_cmp expected actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe --broken --dirty with a file with changed stat' '\n+\ttest_when_finished \"rm -fr stat-dirty\" &&\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n \n-\t\tcat file >file.new &&\n-\t\tmv file.new file &&\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\t\techo \"A\" >expect &&\n+\n+\t\ttest-tool chmtime -10 file &&\n \t\tgit describe --dirty --broken >actual &&\n-\t\techo \"A\" >expected &&\n-\t\ttest_cmp expected actual\n+\t\ttest_cmp expect actual\n \t)\n '\n \n"},{"id":"497720","messageId":"xmqq8qyrzgi5.fsf@gitster.g","threadId":"61679","inReplyTo":"f7d0abce-b389-45ae-992a-adbc7ec10d50@gmail.com","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T18:49:22Z","receivedAt":"2024-06-26T18:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> On 26/06/24 23:05, Junio C Hamano wrote:\n>> Abhijeet Sonar writes:\n>> \n>>> To me, this looks much better. child_process_clear's name already\n>>> suggests that is sort of like a destructor, so it makes sense to\n>>> re-initialize everything here. I even wonder why it was not that way to\n>>> begin with. I suppose no callers are assuming that it only clears args\n>>> and env though?\n>> \n>> I guess that validating that supposition is a prerequisite to\n>> declare the change as \"much better\" and \"makes sense\".\n>\n> OK. I found one: at the end of submodule.c:push_submodule()\n>\n> \tif (...) {\n> \t\t...some setup...\n> \t\tif (run_command(&cp))\n> \t\t\treturn 0;\n> \t\tclose(cp.out);\n> \t}\n\nThis is curious.\n\n * What is this thing trying to do? When run_command() fails, it\n wants to leave cp.out open, so that the caller this returns to\n can write into it??? That cannot be the case, as cp itself is\n internal. So does this \"close(cp.out)\" really matter?\n\n * Even though we are running child_process_clear() to release the\n resources in run_command() we are not closing the file descriptor\n cp.out in the child_process_clear() and force the caller to close\n it instead. An open file descriptor is a resource, and a file\n descriptor opened but forgotten is considered a leak. I wonder\n if child_process_clear() should be closing the file descriptor,\n at least the ones it opened or dup2()ed.\n\nIn any case, you found a case where child_process_clear() may not\nwant to do the full re-initialization and at the same time it is not\ndoing its job sufficiently well. Let's decide, at least for now,\nnot to do the reinitialization from child_process_clear(), then.\n\nThanks.\n\n"},{"id":"497724","messageId":"20240626190801.68472-1-abhijeet.nkt@gmail.com","threadId":"61679","inReplyTo":"xmqqpls3zhc2.fsf@gitster.g","subject":"[PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T19:08:00Z","receivedAt":"2024-06-26T19:09:46Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"When describe is run with 'dirty' flag, we refresh the index\nto make sure it is in sync with the filesystem before\ndetermining if the working tree is dirty. However, this is\nnot done for the codepath where the 'broken' flag is used.\n\nThis causes `git describe --broken --dirty` to false\npositively report the worktree being dirty if a file has\ndifferent stat info than what is recorded in the index.\nRunning `git update-index -q --refresh` to refresh the index\nbefore running diff-index fixes the problem.\n\nAlso add tests to deliberately update stat info of a\nfile before running describe to verify it behaves correctly.\n\nReported-by: Paul Millar \nSuggested-by: Junio C Hamano \nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Abhijeet Sonar \n---\n builtin/describe.c | 12 ++++++++++++\n t/t6120-describe.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..cf8edc4222 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -53,6 +53,10 @@ static const char *diff_index_args[] = {\n \t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n };\n \n+static const char *update_index_args[] = {\n+\t\"update-index\", \"--unmerged\", \"-q\", \"--refresh\", NULL\n+};\n+\n struct commit_name {\n \tstruct hashmap_entry entry;\n \tstruct object_id peeled;\n@@ -645,6 +649,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (argc == 0) {\n \t\tif (broken) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\t\t\tstrvec_pushv(&cp.args, update_index_args);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\t\t\trun_command(&cp);\n+\n+\t\t\tchild_process_init(&cp);\n \t\t\tstrvec_pushv(&cp.args, diff_index_args);\n \t\t\tcp.git_cmd = 1;\n \t\t\tcp.no_stdin = 1;\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e78315d23d..79e0f19deb 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -671,4 +671,40 @@ test_expect_success 'setup misleading taggerdates' '\n \n check_describe newer-tag-older-commit~1 --contains unique-file~2\n \n+test_expect_success 'describe --dirty with a file with changed stat' '\n+\ttest_when_finished \"rm -fr stat-dirty\" &&\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n+\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\t\techo \"A\" >expect &&\n+\n+\t\ttest-tool chmtime -10 file &&\n+\t\tgit describe --dirty >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe --broken --dirty with a file with changed stat' '\n+\ttest_when_finished \"rm -fr stat-dirty\" &&\n+\tgit init stat-dirty &&\n+\t(\n+\t\tcd stat-dirty &&\n+\n+\t\techo A >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\t\techo \"A\" >expect &&\n+\n+\t\ttest-tool chmtime -10 file &&\n+\t\tgit describe --dirty --broken >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.45.2.606.g9005149a4a.dirty\n\n"},{"id":"497725","messageId":"03628ece-4f47-40d5-a926-acce684a21e5@gmail.com","threadId":"61679","inReplyTo":"20240626190801.68472-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-26T19:25:01Z","receivedAt":"2024-06-26T19:25:08Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"I have a question:\n\nWhy does --dirty code path also not call git-update-index and instead does\n\n\tsetup_work_tree();\n\tprepare_repo_settings(the_repository);\n\tthe_repository->settings.command_requires_full_index = 0;\n\trepo_read_index(the_repository);\n\trefresh_index(...);\n\tfd = repo_hold_locked_index(...);\n\tif (0 <= fd)\n\t\trepo_update_index_if_able(the_repository, &index_lock);\n\nI assume they are equivalent?\nThe commit which introduced this --\nbb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index\nwhen run with --dirty) seems to be for the same objective as mu patch.\n\nIs this something you would like to see addressed?\n\nThanks\n"},{"id":"497726","messageId":"20240626203415.GA441931@coredump.intra.peff.net","threadId":"61679","inReplyTo":"xmqq8qyrzgi5.fsf@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-26T20:34:15Z","receivedAt":"2024-06-26T20:34:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 26, 2024 at 11:49:22AM -0700, Junio C Hamano wrote:\n\n> Abhijeet Sonar writes:\n> \n> > On 26/06/24 23:05, Junio C Hamano wrote:\n> >> Abhijeet Sonar writes:\n> >> \n> >>> To me, this looks much better. child_process_clear's name already\n> >>> suggests that is sort of like a destructor, so it makes sense to\n> >>> re-initialize everything here. I even wonder why it was not that way to\n> >>> begin with. I suppose no callers are assuming that it only clears args\n> >>> and env though?\n> >> \n> >> I guess that validating that supposition is a prerequisite to\n> >> declare the change as \"much better\" and \"makes sense\".\n> >\n> > OK. I found one: at the end of submodule.c:push_submodule()\n> >\n> > \tif (...) {\n> > \t\t...some setup...\n> > \t\tif (run_command(&cp))\n> > \t\t\treturn 0;\n> > \t\tclose(cp.out);\n> > \t}\n> \n> This is curious.\n> \n> * What is this thing trying to do? When run_command() fails, it\n> wants to leave cp.out open, so that the caller this returns to\n> can write into it??? That cannot be the case, as cp itself is\n> internal. So does this \"close(cp.out)\" really matter?\n\nI think it's totally broken. Using cp.out, cp.in, etc, with\nrun_command() is a deadlock waiting to happen, since it implies opening\na pipe, not doing anything with our end, and then doing a waitpid() on\nthe child.\n\nYou'd always want to use start_command(), and then do something useful\nwith the pipe, and then finish_command(). Arguably run_command() should\nbug if cp.out, etc are non-zero.\n\nIn this case the code leaves cp.out as 0, so we are not asking for a\npipe. That is good, and we are not subject to any race/deadlock. But\nthen...what is the cleanup trying to do? This will always just close(0),\nthe parent's stdin. That is certainly surprising, but I guess nobody\never noticed because git-push does not usually read from its stdin.\n\nSo I think the close() is superfluous and should be deleted. It goes all\nthe way back to eb21c732d6 (push: teach --recurse-submodules the\non-demand option, 2012-03-29). I guess at one point the author thought\nwe'd read output from a pipe (rather than letting it just go to the\nparent's stdout) and this was leftover?\n\n> * Even though we are running child_process_clear() to release the\n> resources in run_command() we are not closing the file descriptor\n> cp.out in the child_process_clear() and force the caller to close\n> it instead. An open file descriptor is a resource, and a file\n> descriptor opened but forgotten is considered a leak. I wonder\n> if child_process_clear() should be closing the file descriptor,\n> at least the ones it opened or dup2()ed.\n\nUsually the caller will have handled this already (since it cares about\nexactly _when_ to close), so we'd end up double-closing in most cases.\nThis is sometimes harmless (you'll get EBADF), but is a problem if the\ndescriptor was assigned to something else in the meantime.\n\nIn most cases callers shouldn't be using child_process_clear() at all\nafter start_command(), etc. Either:\n\n - start_command() failed, in which case it cleans up everything\n already (both in the struct and any pipes it opened)\n\n - we succeeded in starting the command, in which case\n child_process_clear() is insufficient. You need to actually\n finish_command() to avoid leaking the pid.\n\n - after finish_command(), the struct has been cleaned already. You do\n need to close pipes in the caller, but you'd have to do so before\n calling finish_command() anyway, to avoid the deadlock.\n\nYou really only need to call child_process_clear() yourself when you set\nup a struct but didn't actually start the process. And from a quick skim\nover the grep results, it looks like that's how it's used.\n\n-Peff\n"},{"id":"497736","messageId":"CAOLa=ZR9qQNDRy_gRGxw-78=CkqbqV_6-AFtdJo7WEjQNqQyAw@mail.gmail.com","threadId":"61679","inReplyTo":"xmqq8qyrzgi5.fsf@gitster.g","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-26T21:23:26Z","receivedAt":"2024-06-26T21:23:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Abhijeet Sonar writes:\n>\n>> On 26/06/24 23:05, Junio C Hamano wrote:\n>>> Abhijeet Sonar writes:\n>>>\n>>>> To me, this looks much better. child_process_clear's name already\n>>>> suggests that is sort of like a destructor, so it makes sense to\n>>>> re-initialize everything here. I even wonder why it was not that way to\n>>>> begin with. I suppose no callers are assuming that it only clears args\n>>>> and env though?\n>>>\n>>> I guess that validating that supposition is a prerequisite to\n>>> declare the change as \"much better\" and \"makes sense\".\n>>\n>> OK. I found one: at the end of submodule.c:push_submodule()\n>>\n>> \tif (...) {\n>> \t\t...some setup...\n>> \t\tif (run_command(&cp))\n>> \t\t\treturn 0;\n>> \t\tclose(cp.out);\n>> \t}\n>\n> This is curious.\n>\n> * What is this thing trying to do? When run_command() fails, it\n> wants to leave cp.out open, so that the caller this returns to\n> can write into it??? That cannot be the case, as cp itself is\n> internal. So does this \"close(cp.out)\" really matter?\n>\n\nThis is curious one indeed, we only need to close the file descriptor\nwhen we set `cp.out = -1`, otherwise there is no need to close `cp.out`\nsince `start_command()` takes care of it internally. So the\n`close(cp.out)` can really be removed here.\n\nWonder if this was copied over from other parts of the submodule code,\nwhere we actually set `cp.out = -1`.\n\n>\n> * Even though we are running child_process_clear() to release the\n> resources in run_command() we are not closing the file descriptor\n> cp.out in the child_process_clear() and force the caller to close\n> it instead. An open file descriptor is a resource, and a file\n> descriptor opened but forgotten is considered a leak. I wonder\n> if child_process_clear() should be closing the file descriptor,\n> at least the ones it opened or dup2()ed.\n>\n\nIf you see the documentation for run_command.h::child_process, it states\n\n /*\n * Using .in, .out, .err:\n * - Specify 0 for no redirections. No new file descriptor is allocated.\n * (child inherits stdin, stdout, stderr from parent).\n * - Specify -1 to have a pipe allocated as follows:\n * .in: returns the writable pipe end; parent writes to it,\n * the readable pipe end becomes child's stdin\n * .out, .err: returns the readable pipe end; parent reads from\n * it, the writable pipe end becomes child's stdout/stderr\n * The caller of start_command() must close the returned FDs\n * after it has completed reading from/writing to it!\n * - Specify > 0 to set a channel to a particular FD as follows:\n * .in: a readable FD, becomes child's stdin\n * .out: a writable FD, becomes child's stdout/stderr\n * .err: a writable FD, becomes child's stderr\n * The specified FD is closed by start_command(), even in case\n * of errors!\n */\n int in;\n int out;\n int err;\n\nSo this makes sense right? run_command() doesn't support the pipe\noptions, meaning clients have to call start_command() + finish_command()\nmanually. When clients do call start_command() with '-1' set to these\noptions, they are in charge of closing the created pipes.\n\n> In any case, you found a case where child_process_clear() may not\n> want to do the full re-initialization and at the same time it is not\n> doing its job sufficiently well. Let's decide, at least for now,\n> not to do the reinitialization from child_process_clear(), then.\n>\n> Thanks.\n"},{"id":"497739","messageId":"20240627003323.GA1460039@coredump.intra.peff.net","threadId":"61679","inReplyTo":"20240626203415.GA441931@coredump.intra.peff.net","subject":"Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-27T00:33:23Z","receivedAt":"2024-06-27T00:33:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 26, 2024 at 04:34:17PM -0400, Jeff King wrote:\n\n> > * What is this thing trying to do? When run_command() fails, it\n> > wants to leave cp.out open, so that the caller this returns to\n> > can write into it??? That cannot be the case, as cp itself is\n> > internal. So does this \"close(cp.out)\" really matter?\n> \n> I think it's totally broken. Using cp.out, cp.in, etc, with\n> run_command() is a deadlock waiting to happen, since it implies opening\n> a pipe, not doing anything with our end, and then doing a waitpid() on\n> the child.\n> \n> You'd always want to use start_command(), and then do something useful\n> with the pipe, and then finish_command(). Arguably run_command() should\n> bug if cp.out, etc are non-zero.\n\nOh, I guess I should have looked at the code. We do indeed BUG() in this\ncase already, courtesy of c29b3962af (run-command: forbid using\nrun_command with piped output, 2015-03-22).\n\nSo all is well there. This caller is still completely wrong to call\nclose() though.\n\n-Peff\n"},{"id":"497740","messageId":"1734a7ff-b169-462f-afc1-52a912d8d56c@gmail.com","threadId":"61679","inReplyTo":"03628ece-4f47-40d5-a926-acce684a21e5@gmail.com","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-27T06:01:30Z","receivedAt":"2024-06-27T06:01:35Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 27/06/24 00:55, Abhijeet Sonar wrote:\n> I have a question:\n> \n> Why does --dirty code path also not call git-update-index and instead does\n> \n> \tsetup_work_tree();\n> \tprepare_repo_settings(the_repository);\n> \tthe_repository->settings.command_requires_full_index = 0;\n> \trepo_read_index(the_repository);\n> \trefresh_index(...);\n> \tfd = repo_hold_locked_index(...);\n> \tif (0 <= fd)\n> \t\trepo_update_index_if_able(the_repository, &index_lock);\n> \n> I assume they are equivalent?\n> The commit which introduced this --\n> bb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index\n> when run with --dirty) seems to be for the same objective as mu patch.\n> \n> Is this something you would like to see addressed?\n> \n> Thanks\n\ns/mu/my\n"},{"id":"497759","messageId":"xmqqfrsyv155.fsf@gitster.g","threadId":"61679","inReplyTo":"03628ece-4f47-40d5-a926-acce684a21e5@gmail.com","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-27T15:47:02Z","receivedAt":"2024-06-27T15:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> I have a question:\n>\n> Why does --dirty code path also not call git-update-index and instead does\n>\n> \tsetup_work_tree();\n> \tprepare_repo_settings(the_repository);\n> \tthe_repository->settings.command_requires_full_index = 0;\n> \trepo_read_index(the_repository);\n> \trefresh_index(...);\n> \tfd = repo_hold_locked_index(...);\n> \tif (0 <= fd)\n> \t\trepo_update_index_if_able(the_repository, &index_lock);\n>\n> I assume they are equivalent?\n\nNow we are going back full circles ;-)?\n\nYour earliest attempt indeed copied the above to the code paths used\nto handle \"--broken\", but then Phillip corrected the course\n\n https://lore.kernel.org/git/054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com/\n\nto avoid triggering an in-process error and instead run an\nequivalent \"update-index --refresh\" via the run_command() interface,\nso that we can catch potential errors. The code in the more recent\nrounds of your patch uses that, no?\n\n> The commit which introduced this --\n> bb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index\n> when run with --dirty) seems to be for the same objective as mu patch.\n\nYes, but the cited message above explains the reason why the two\ncode paths in your patch use different implementations, I would\nthink.\n"},{"id":"497760","messageId":"90640733-a34e-4b3d-8019-a0ee53908946@gmail.com","threadId":"61679","inReplyTo":"xmqqfrsyv155.fsf@gitster.g","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-27T17:33:03Z","receivedAt":"2024-06-27T17:33:10Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 27/06/24 21:17, Junio C Hamano wrote:\n\n> to avoid triggering an in-process error and instead run an\n> equivalent \"update-index --refresh\" via the run_command() interface,\n> so that we can catch potential errors.\n\nThanks, I get it now.\n\n"},{"id":"497856","messageId":"CAOLa=ZS359bMtUd+ktvJgHsiG-0=VVdGWYA2mKCNjc_1BrzcvQ@mail.gmail.com","threadId":"61679","inReplyTo":"xmqqfrsyv155.fsf@gitster.g","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-30T16:12:12Z","receivedAt":"2024-06-30T16:12:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Abhijeet Sonar writes:\n>\n>> I have a question:\n>>\n>> Why does --dirty code path also not call git-update-index and instead does\n>>\n>> \tsetup_work_tree();\n>> \tprepare_repo_settings(the_repository);\n>> \tthe_repository->settings.command_requires_full_index = 0;\n>> \trepo_read_index(the_repository);\n>> \trefresh_index(...);\n>> \tfd = repo_hold_locked_index(...);\n>> \tif (0 <= fd)\n>> \t\trepo_update_index_if_able(the_repository, &index_lock);\n>>\n>> I assume they are equivalent?\n>\n> Now we are going back full circles ;-)?\n>\n> Your earliest attempt indeed copied the above to the code paths used\n> to handle \"--broken\", but then Phillip corrected the course\n>\n> https://lore.kernel.org/git/054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com/\n>\n> to avoid triggering an in-process error and instead run an\n> equivalent \"update-index --refresh\" via the run_command() interface,\n> so that we can catch potential errors. The code in the more recent\n> rounds of your patch uses that, no?\n>\n\nThis explains for why 'broken' must use a subprocess, but there is\nnothing stopping 'dirty' from also using a subprocess, right? It\ncurrently uses an in-process index refresh but it _could_ be a\nsubprocess too.\n\nDoes it need to be a subprocess? I can't think of any reason to make it.\n"},{"id":"497915","messageId":"xmqqle2lvsmz.fsf@gitster.g","threadId":"61679","inReplyTo":"CAOLa=ZS359bMtUd+ktvJgHsiG-0=VVdGWYA2mKCNjc_1BrzcvQ@mail.gmail.com","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T19:06:44Z","receivedAt":"2024-07-01T19:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> This explains for why 'broken' must use a subprocess, but there is\n> nothing stopping 'dirty' from also using a subprocess, right? It\n> currently uses an in-process index refresh but it _could_ be a\n> subprocess too.\n\nCorrect, except that it does not make sense to do any and all things\nthat you _could_ do. So...\n\n"},{"id":"497967","messageId":"CAOLa=ZR-g4G0FaxnQjjkOST-zeRxBXXK1gpJ=P3xdbi_9eN_rg@mail.gmail.com","threadId":"61679","inReplyTo":"xmqqle2lvsmz.fsf@gitster.g","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-02T10:13:30Z","receivedAt":"2024-07-02T10:13:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> This explains for why 'broken' must use a subprocess, but there is\n>> nothing stopping 'dirty' from also using a subprocess, right? It\n>> currently uses an in-process index refresh but it _could_ be a\n>> subprocess too.\n>\n> Correct, except that it does not make sense to do any and all things\n> that you _could_ do. So...\n\nWell, In this context, I think there is some merit though. There are two\nblocks of code `--broken` and `--dirty` one after the other which both\nneed to refresh the index. With this patch, 'broken' will use a child\nprocess to do so while 'dirty' will use `refresh_index(...)`. To someone\nreading the code it would seem a bit confusing. I agree there is no\nmerit in using a child process in 'dirty' by itself. But I also think we\nshould leave a comment there for readers to understand the distinction.\n"},{"id":"498051","messageId":"xmqq1q4acpcv.fsf@gitster.g","threadId":"61679","inReplyTo":"CAOLa=ZR-g4G0FaxnQjjkOST-zeRxBXXK1gpJ=P3xdbi_9eN_rg@mail.gmail.com","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T18:17:04Z","receivedAt":"2024-07-03T18:17:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> This explains for why 'broken' must use a subprocess, but there is\n>>> nothing stopping 'dirty' from also using a subprocess, right? It\n>>> currently uses an in-process index refresh but it _could_ be a\n>>> subprocess too.\n>>\n>> Correct, except that it does not make sense to do any and all things\n>> that you _could_ do. So...\n>\n> Well, In this context, I think there is some merit though. There are two\n> blocks of code `--broken` and `--dirty` one after the other which both\n> need to refresh the index. With this patch, 'broken' will use a child\n> process to do so while 'dirty' will use `refresh_index(...)`. To someone\n> reading the code it would seem a bit confusing.\n\nYes, that much I very much agree.\n\n> I agree there is no\n> merit in using a child process in 'dirty' by itself.\n\nYes, that made me puzzled why you brought it up, as it was way too\noblique suggestion to ...\n\n> But I also think we\n> should leave a comment there for readers to understand the distinction.\n\n... improve the \"documentation\" to help future developers who wonder\nwhy the code are in the shape as it is.\n\nIn this particular case, I think it is borderline if the issue\nwarrants in-code comment or if it is a bit too much. Describing the\nsame thing in the log message would probably be a valid alternative,\nas \"git blame\" can lead those readers to the commit that introduced\nthe distinction (in other words, this one).\n\nThanks.\n\ndiff --git i/builtin/describe.c w/builtin/describe.c\nindex e936d2c19f..bc2ad60b35 100644\n--- i/builtin/describe.c\n+++ w/builtin/describe.c\n@@ -648,6 +648,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \tif (argc == 0) {\n \t\tif (broken) {\n+\t\t\t/* \n+\t\t\t * Avoid doing these \"update-index --refresh\"\n+\t\t\t * and \"diff-index\" operations in-process\n+\t\t\t * (like how the code path for \"--dirty\"\n+\t\t\t * without \"--broken\" does so below), as we\n+\t\t\t * are told to prepare for a broken repository\n+\t\t\t * where running these may lead to die().\n+\t\t\t */\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tstrvec_pushv(&cp.args, update_index_args);\n"},{"id":"498059","messageId":"CAOLa=ZRm_-9aewE-7JXa2LOBoddoBTJgcTTcJ7FoUvrLRR=_JQ@mail.gmail.com","threadId":"61679","inReplyTo":"xmqq1q4acpcv.fsf@gitster.g","subject":"Re: [PATCH v7] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-03T20:41:21Z","receivedAt":"2024-07-03T20:41:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> Karthik Nayak writes:\n>>>\n>>>> This explains for why 'broken' must use a subprocess, but there is\n>>>> nothing stopping 'dirty' from also using a subprocess, right? It\n>>>> currently uses an in-process index refresh but it _could_ be a\n>>>> subprocess too.\n>>>\n>>> Correct, except that it does not make sense to do any and all things\n>>> that you _could_ do. So...\n>>\n>> Well, In this context, I think there is some merit though. There are two\n>> blocks of code `--broken` and `--dirty` one after the other which both\n>> need to refresh the index. With this patch, 'broken' will use a child\n>> process to do so while 'dirty' will use `refresh_index(...)`. To someone\n>> reading the code it would seem a bit confusing.\n>\n> Yes, that much I very much agree.\n>\n>> I agree there is no\n>> merit in using a child process in 'dirty' by itself.\n>\n> Yes, that made me puzzled why you brought it up, as it was way too\n> oblique suggestion to ...\n>\n\nYeah, I should've been more verbose there.\n\n>> But I also think we\n>> should leave a comment there for readers to understand the distinction.\n>\n> ... improve the \"documentation\" to help future developers who wonder\n> why the code are in the shape as it is.\n>\n> In this particular case, I think it is borderline if the issue\n> warrants in-code comment or if it is a bit too much. Describing the\n> same thing in the log message would probably be a valid alternative,\n> as \"git blame\" can lead those readers to the commit that introduced\n> the distinction (in other words, this one).\n>\n\nI think it is always best to err on the side of more documentation than\nless in such situations.\n\n> Thanks.\n>\n> diff --git i/builtin/describe.c w/builtin/describe.c\n> index e936d2c19f..bc2ad60b35 100644\n> --- i/builtin/describe.c\n> +++ w/builtin/describe.c\n> @@ -648,6 +648,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n>\n> \tif (argc == 0) {\n> \t\tif (broken) {\n> +\t\t\t/*\n> +\t\t\t * Avoid doing these \"update-index --refresh\"\n> +\t\t\t * and \"diff-index\" operations in-process\n> +\t\t\t * (like how the code path for \"--dirty\"\n> +\t\t\t * without \"--broken\" does so below), as we\n> +\t\t\t * are told to prepare for a broken repository\n> +\t\t\t * where running these may lead to die().\n> +\t\t\t */\n> \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>\n> \t\t\tstrvec_pushv(&cp.args, update_index_args);\n\nThis looks perfect, thanks!\n"}]}