{"thread":{"id":"61675","subject":"[PATCH] describe: refresh the index when 'broken' flag is used","startedAt":"2024-06-23T21:43:23Z","lastAt":"2024-06-25T13:22:39Z","messageCount":14,"participants":["Abhijeet Sonar","Karthik Nayak","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497552","messageId":"20240623214301.143796-1-abhijeet.nkt@gmail.com","threadId":"61675","inReplyTo":null,"subject":"[PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-23T21:42:52Z","receivedAt":"2024-06-23T21:43:23Z","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. Refreshing the\nindex before running diff-index fixes the problem.\n\nSigned-off-by: Abhijeet Sonar \nReported-by: Paul Millar \nSuggested-by: Junio C Hamano \n---\n builtin/describe.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..2b443c155e 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -645,6 +645,20 @@ 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\tstruct lock_file index_lock = LOCK_INIT;\n+\t\t\tint fd;\n+\n+\t\t\tsetup_work_tree();\n+\t\t\tprepare_repo_settings(the_repository);\n+\t\t\trepo_read_index(the_repository);\n+\t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n+\t\t\t\t NULL, NULL, NULL);\n+\t\t\tfd = repo_hold_locked_index(the_repository,\n+\t\t\t\t\t\t &index_lock, 0);\n+\t\t\tif (0 <= fd)\n+\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\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-- \n2.45.GIT\n\n"},{"id":"497553","messageId":"7e3ba9fc-1ff4-49aa-8e73-c50202477719@gmail.com","threadId":"61675","inReplyTo":"20240623214301.143796-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-23T21:56:32Z","receivedAt":"2024-06-23T21:56:38Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"This is my first ever patch submission and I am excited to contribute\neven if it is such a little thing! Please let me know of any etiquette\nor convention violations, I will do my best to uphold them moving forward.\n\nThanks\n\nOn 24/06/24 03:12, Abhijeet Sonar wrote:\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. Refreshing the\n> index before running diff-index fixes the problem.\n>\n> Signed-off-by: Abhijeet Sonar \n> Reported-by: Paul Millar \n> Suggested-by: Junio C Hamano \n> ---\n> builtin/describe.c | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index e5287eddf2..2b443c155e 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -645,6 +645,20 @@ 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\tstruct lock_file index_lock = LOCK_INIT;\n> +\t\t\tint fd;\n> +\n> +\t\t\tsetup_work_tree();\n> +\t\t\tprepare_repo_settings(the_repository);\n> +\t\t\trepo_read_index(the_repository);\n> +\t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n> +\t\t\t\t NULL, NULL, NULL);\n> +\t\t\tfd = repo_hold_locked_index(the_repository,\n> +\t\t\t\t\t\t &index_lock, 0);\n> +\t\t\tif (0 <= fd)\n> +\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\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"},{"id":"497560","messageId":"CAOLa=ZRGramQ3MdzzXzZ19yeUB_rQZPbZ3u=eA=T2SfV3nhYOA@mail.gmail.com","threadId":"61675","inReplyTo":"20240623214301.143796-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-24T10:56:41Z","receivedAt":"2024-06-24T10:56:43Z","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. Refreshing the\n> index before running diff-index fixes the problem.\n>\n> Signed-off-by: Abhijeet Sonar \n> Reported-by: Paul Millar \n> Suggested-by: Junio C Hamano \n> ---\n> builtin/describe.c | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index e5287eddf2..2b443c155e 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -645,6 +645,20 @@ 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\tstruct lock_file index_lock = LOCK_INIT;\n> +\t\t\tint fd;\n> +\n> +\t\t\tsetup_work_tree();\n> +\t\t\tprepare_repo_settings(the_repository);\n> +\t\t\trepo_read_index(the_repository);\n> +\t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n> +\t\t\t\t NULL, NULL, NULL);\n> +\t\t\tfd = repo_hold_locked_index(the_repository,\n> +\t\t\t\t\t\t &index_lock, 0);\n> +\t\t\tif (0 <= fd)\n> +\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\n> +\n> +\n>\n\nI'm wondering why this needs to be done, as I can see, when we use the\n'--broken' flag, we create a child process to run `git diff-index\n--quiet HEAD`. As such, we shouldn't have to refresh the index here.\n\nCould you perhaps state how you can reproduce the issue mentioned?\n\nAlso apart from that, we should add a test to capture the changes.\n\n> \t\t\tcp.git_cmd = 1;\n> \t\t\tcp.no_stdin = 1;\n> --\n> 2.45.GIT\n"},{"id":"497561","messageId":"054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com","threadId":"61675","inReplyTo":"CAOLa=ZRGramQ3MdzzXzZ19yeUB_rQZPbZ3u=eA=T2SfV3nhYOA@mail.gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-24T11:20:40Z","receivedAt":"2024-06-24T11:20:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Abhijeet and Karthik\n\nOn 24/06/2024 11:56, Karthik Nayak wrote:\n> 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. Refreshing the\n>> index before running diff-index fixes the problem.\n\nThis is a good description of the problem the patch fixes.\n\n>> Signed-off-by: Abhijeet Sonar \n>> Reported-by: Paul Millar \n>> Suggested-by: Junio C Hamano \n>> ---\n>> builtin/describe.c | 14 ++++++++++++++\n>> 1 file changed, 14 insertions(+)\n>>\n>> diff --git a/builtin/describe.c b/builtin/describe.c\n>> index e5287eddf2..2b443c155e 100644\n>> --- a/builtin/describe.c\n>> +++ b/builtin/describe.c\n>> @@ -645,6 +645,20 @@ 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\tstruct lock_file index_lock = LOCK_INIT;\n>> +\t\t\tint fd;\n>> +\n>> +\t\t\tsetup_work_tree();\n>> +\t\t\tprepare_repo_settings(the_repository);\n>> +\t\t\trepo_read_index(the_repository);\n>> +\t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n>> +\t\t\t\t NULL, NULL, NULL);\n>> +\t\t\tfd = repo_hold_locked_index(the_repository,\n>> +\t\t\t\t\t\t &index_lock, 0);\n>> +\t\t\tif (0 <= fd)\n>> +\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\n>> +\n\nAs we're dealing with a repository that might be broken I suspect we'd \nbe better to run \"git update-index --unmerged -q --refresh\" as a \nsubprocess in the same way that we run \"git diff-index\" so that \"git \ndescribe --broken\" does not die if the index cannot be refreshed.\n\n> I'm wondering why this needs to be done, as I can see, when we use the\n> '--broken' flag, we create a child process to run `git diff-index\n> --quiet HEAD`. As such, we shouldn't have to refresh the index here.\n\n\"git diff-index\" and \"git diff-files\" do not refresh the index. This is \nby design so that a script can refresh the index once and run \"git \ndiff-index\" several times without wasting time updating the index each time.\n\n> Also apart from that, we should add a test to capture the changes.\n\nThat would be nice\n\nBest Wishes\n\nPhillip\n\n\n>> \t\t\tcp.git_cmd = 1;\n>> \t\t\tcp.no_stdin = 1;\n>> --\n>> 2.45.GIT\n"},{"id":"497562","messageId":"69662b07-e216-421b-b947-afa52e56d7ec@gmail.com","threadId":"61675","inReplyTo":"054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-24T12:20:42Z","receivedAt":"2024-06-24T12:20:53Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"On 24/06/24 16:50, Phillip Wood wrote:\n\n> This is a good description of the problem the patch fixes.\n\nThanks!\n\n> As we're dealing with a repository that might be broken I suspect we'd \n> be better to run \"git update-index --unmerged -q --refresh\" as a \n> subprocess in the same way that we run \"git diff-index\" so that \"git \n> describe --broken\" does not die if the index cannot be refreshed.\n\nI see, that makes sense.  I will change it to launch `update-index` in a \nsub-process\ninstead.\n\n>> Also apart from that, we should add a test to capture the changes.\n> That would be nice\nGot it, I will add some tests as well.\n\nThanks\n\n"},{"id":"497565","messageId":"735b4a95-fe8e-4bff-9dd3-10ddd4bdb5b5@gmail.com","threadId":"61675","inReplyTo":"69662b07-e216-421b-b947-afa52e56d7ec@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-24T13:48:33Z","receivedAt":"2024-06-24T13:48:40Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"I have a question:\n\nI would like to change the owner of a file\nin the test case I am writing -- an operation\nthat requires super-user privileges.  I am not\nsure if it is okay to do that in tests.  Since\nthat would require running tests with `sudo`.\n\nWhat would be the correct way to do this?\n\n\n"},{"id":"497577","messageId":"xmqqcyo6cnbb.fsf@gitster.g","threadId":"61675","inReplyTo":"735b4a95-fe8e-4bff-9dd3-10ddd4bdb5b5@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T16:34:00Z","receivedAt":"2024-06-24T16:34:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> I would like to change the owner of a file\n> in the test case I am writing -- an operation\n> that requires super-user privileges.  I am not\n> sure if it is okay to do that in tests.  Since\n> that would require running tests with `sudo`.\n\nWhat is the reason why you want to change the owner of a file in\nyour test?\n\nIf it is merely to make sure you cannot write to the .git/index\nfile, temporarily doing chmod of the .git directory in a test (with\nPOSIXPERM prerequisite) may be one way to do so, and you do not need\nthe second user in the system test is running.\n\nOr if you pretend that you have a second process that is holding the\nlock in .git/index by creating .git/index.lock file yourself, that\nwould also prevent your tested command from touching the index.\n\nThe latter approach would result in a test that may look like so (I\nam writing this in my mail client, and I expect there may be some\nfix ups needed):\n\ntest_expect_success 'see what --broken does upon unwritable index' '\n test_when_finished \"rm -f .git/index.lock\" &&\n test_commit A A.file &&\n echo changed >>A.file &&\n >.git/index.lock &&\n\ttest_must_fail git describe --dirty >actual 2>error &&\n\ttest_grep \"could not write index\" error &&\n\tgit describe --broken --dirty >actual 2>error &&\n\ttest_grep ! \"could not write index\" error &&\n\techo ...expected.describe.result... >expect &&\n\ttest_cmp expect actual\n'\n\nHTH.\n"},{"id":"497580","messageId":"4224c251-c6f7-4b2a-b182-b0a12ee300c8@gmail.com","threadId":"61675","inReplyTo":"xmqqcyo6cnbb.fsf@gitster.g","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-24T17:37:07Z","receivedAt":"2024-06-24T17:37:12Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"> What is the reason why you want to change the owner of a file in\n> your test?\n>\n> If it is merely to make sure you cannot write to the .git/index\n> file, temporarily doing chmod of the .git directory in a test (with\nPOSIXPERM prerequisite) may be one way to do so, and you do not need\nthe second user in the system test is running.\n\nI want to change the owner of a checked-in file and not the `.git`\ndirectory. This is because of what you noted in an earlier message:\n\n> As many attributes of each file (like the\n> last modified timestamp and who owns the file) are recorded in the\n> index for files that were verified to be unmodified (this is done so\n> that by doing lstat() on a path and comparing the result with the\n> information saved in the index, we can notice that the path was\n> modified without actually opening the file and looking at the\n> contents), after doing something (like \"git diff\") that causes this\n> information updated while the files appear to be owned by you\n\nCurrently, `git describe --dirty --broken` reports the working tree as\ndirty if you change the owner of a file. And as Phillip pointed out,\ncalling `git update-index --unmerged -q --refresh` to update the index\nfixes this.\n\nWhat I want to test looks something like this:\n\n# initially, the file is owned by a non-root user\nchown root file\ngit describe --dirty --broken # incorrectly suffixes the output with '-dirty'\n\nAs mentioned earlier, the dirty suffix goes away if the index is\nrefreshed before running describe. This is what I really want to\nassert -- that there is no '-dirty' suffix when owner of a file is changed.\nThis kind of simulates the scenario where `git describe` is run in a\ndocker container as was originally reported by Paul:\n\n> mkdir test-container\n>\n> cd test-container\n> \n> cat >Dockerfile < \n> FROM docker.io/debian:bookworm-slim\n> \n> WORKDIR /work\n> \n> RUN apt-get update && apt-get -y install git\n> \n> EOF\n> \n> podman build -t test-image .\n> \n> \n> \n> mkdir test-repo\n> \n> cd test-repo\n> \n> git init echo \"Hello, world\" > README\n> \n> git add README\n> \n> git commit -m \"Initial commit\" README\n> \n> git tag v1.0.0\n>\n>\n>\n> git describe --tags --dirty --broken\n>\n>\n>\n> podman run -v `pwd`:/work --rm -it --entrypoint '[\"/usr/bin/git\",\n> \n>    \"describe\", \"--tags\", \"--dirty\", \"--broken\"]' test-image\n\n\nThanks\n\n"},{"id":"497581","messageId":"xmqqsex2b4ti.fsf@gitster.g","threadId":"61675","inReplyTo":"4224c251-c6f7-4b2a-b182-b0a12ee300c8@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T17:58:49Z","receivedAt":"2024-06-24T17:58:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhijeet Sonar writes:\n\n> Currently, `git describe --dirty --broken` reports the working tree as\n> dirty if you change the owner of a file. And as Phillip pointed out,\n> calling `git update-index --unmerged -q --refresh` to update the index\n> fixes this.\n\nStarting from a clean state with a tracked file COPYING, I can do\nthis:\n\n $ git describe --dirty --broken\n v2.45.2-862-g39ba10deb2\n $ cat COPYING >RENAMING && mv RENAMING COPYING\n $ git diff-index --abbrev=8 HEAD\n :100644 100644 536e5552 00000000 M\tCOPYING\n $ git describe --dirty --broken\n v2.45.2-862-g39ba10deb2-dirty\n $ git describe --dirty\n v2.45.2-862-g39ba10deb2\n\nThis is with a version if Git _without_ your fix, i.e. the one whose\n\"describe --broken --dirty\" does not do \"git update-index --refresh\".\n\nIn other words, the stat-only change to cause \"diff-index\" to report\na \"suspected to be modified\" does not have to be that the file is\nowned by a different owner. So I still do not understand why you\nwant a second user in this test.\n"},{"id":"497604","messageId":"89951829-80f3-482f-87d1-d88c12a2575d@gmail.com","threadId":"61675","inReplyTo":"xmqqsex2b4ti.fsf@gitster.g","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-25T06:40:49Z","receivedAt":"2024-06-25T06:40:56Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":" > So I still do not understand why you > want a second user in this \ntest. What I really wanted to do was closely mirror the environment in \nreproduction steps mentioned in original bug report. Which I figured \ncould be done by changing the owner to a second user.\n\n\nOn 24/06/24 23:28, Junio C Hamano wrote:\n\n> $ git describe --dirty --broken\n> v2.45.2-862-g39ba10deb2\n> $ cat COPYING >RENAMING && mv RENAMING COPYING\n> $ git diff-index --abbrev=8 HEAD\n> :100644 100644 536e5552 00000000 M\tCOPYING\n> $ git describe --dirty --broken\n> v2.45.2-862-g39ba10deb2-dirty\n> $ git describe --dirty\n> v2.45.2-862-g39ba10deb2\n\nThanks, I will use this in the tests.\n\n"},{"id":"497605","messageId":"20240625064504.58286-1-abhijeet.nkt@gmail.com","threadId":"61675","inReplyTo":"xmqqsex2b4ti.fsf@gitster.g","subject":"[PATCH v2] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-25T06:44:41Z","receivedAt":"2024-06-25T06:45:33Z","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 \nSigned-off-by: Abhijeet Sonar \n---\n builtin/describe.c | 12 ++++++++++++\n t/t6120-describe.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e5287eddf2..3e751f1239 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+\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+\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..ac781a7b52 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -671,4 +671,36 @@ 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+\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+\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+\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 --broken >actual &&\n+\techo \"A\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.45.2.606.g9005149a4a.dirty\n\n"},{"id":"497616","messageId":"CAOLa=ZT5asAE8-=jY7iR2hsGGshjQtBvpjcVacQk5YFa0u3Sfg@mail.gmail.com","threadId":"61675","inReplyTo":"20240625064504.58286-1-abhijeet.nkt@gmail.com","subject":"Re: [PATCH v2] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-25T12:27:33Z","receivedAt":"2024-06-25T12:27:35Z","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> Signed-off-by: Abhijeet Sonar \n> ---\n> builtin/describe.c | 12 ++++++++++++\n> t/t6120-describe.sh | 32 ++++++++++++++++++++++++++++++++\n> 2 files changed, 44 insertions(+)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index e5287eddf2..3e751f1239 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> +\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> +\n\nWe should also call `strvec_clear(&update_index_cp.args);` to clear up\nused memory.\n\nNit: we could actually `cp` for both the child processes here.\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..ac781a7b52 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -671,4 +671,36 @@ 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\nIt would be nice to cleanup the repo at the end of the test by adding\n`test_when_finished \"rm -rf stat-dirty\" &&` here\n\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\nYou want to do everything apart from the repo init in a subshell, this\nensures we don't carry over the working directory to the next test.\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> +\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 --broken >actual &&\n> +\techo \"A\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n\nCan't this be merged with the previous test?\n"},{"id":"497617","messageId":"CAOLa=ZQEhT02TXgymD20J49HwAAkTeTqDBruuebwjkMzj1kjmQ@mail.gmail.com","threadId":"61675","inReplyTo":"054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com","subject":"Re: [PATCH] describe: refresh the index when 'broken' flag is used","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-25T12:28:16Z","receivedAt":"2024-06-25T12:28:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Phillip Wood writes:\n\n> Hi Abhijeet and Karthik\n>\n> On 24/06/2024 11:56, Karthik Nayak wrote:\n>> 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. Refreshing the\n>>> index before running diff-index fixes the problem.\n>\n> This is a good description of the problem the patch fixes.\n>\n>>> Signed-off-by: Abhijeet Sonar \n>>> Reported-by: Paul Millar \n>>> Suggested-by: Junio C Hamano \n>>> ---\n>>> builtin/describe.c | 14 ++++++++++++++\n>>> 1 file changed, 14 insertions(+)\n>>>\n>>> diff --git a/builtin/describe.c b/builtin/describe.c\n>>> index e5287eddf2..2b443c155e 100644\n>>> --- a/builtin/describe.c\n>>> +++ b/builtin/describe.c\n>>> @@ -645,6 +645,20 @@ 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\tstruct lock_file index_lock = LOCK_INIT;\n>>> +\t\t\tint fd;\n>>> +\n>>> +\t\t\tsetup_work_tree();\n>>> +\t\t\tprepare_repo_settings(the_repository);\n>>> +\t\t\trepo_read_index(the_repository);\n>>> +\t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n>>> +\t\t\t\t NULL, NULL, NULL);\n>>> +\t\t\tfd = repo_hold_locked_index(the_repository,\n>>> +\t\t\t\t\t\t &index_lock, 0);\n>>> +\t\t\tif (0 <= fd)\n>>> +\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\n>>> +\n>\n> As we're dealing with a repository that might be broken I suspect we'd\n> be better to run \"git update-index --unmerged -q --refresh\" as a\n> subprocess in the same way that we run \"git diff-index\" so that \"git\n> describe --broken\" does not die if the index cannot be refreshed.\n>\n>> I'm wondering why this needs to be done, as I can see, when we use the\n>> '--broken' flag, we create a child process to run `git diff-index\n>> --quiet HEAD`. As such, we shouldn't have to refresh the index here.\n>\n> \"git diff-index\" and \"git diff-files\" do not refresh the index. This is\n> by design so that a script can refresh the index once and run \"git\n> diff-index\" several times without wasting time updating the index each time.\n>\n\nI see. Thanks for correcting me!\n\n>> Also apart from that, we should add a test to capture the changes.\n>\n> That would be nice\n>\n> Best Wishes\n>\n> Phillip\n>\n>\n>>> \t\t\tcp.git_cmd = 1;\n>>> \t\t\tcp.no_stdin = 1;\n>>> --\n>>> 2.45.GIT\n"},{"id":"497620","messageId":"68eb8651-c56e-449f-8102-b82506607387@gmail.com","threadId":"61675","inReplyTo":"CAOLa=ZT5asAE8-=jY7iR2hsGGshjQtBvpjcVacQk5YFa0u3Sfg@mail.gmail.com","subject":"Re: [PATCH v2] describe: refresh the index when 'broken' flag is used","fromName":"Abhijeet Sonar","fromEmail":"abhijeet.nkt@gmail.com","sentAt":"2024-06-25T13:22:31Z","receivedAt":"2024-06-25T13:22:39Z","isPatch":true,"sender":{"key":"abhijeet.nkt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40241646?v=4"},"body":"Hi Karthik, thanks for taking the time to review my\npatch.\n\nOn 25/06/24 17:57, Karthik Nayak wrote:\n\n> We should also call `strvec_clear(&update_index_cp.args);` to clear up\n> used memory.\n>\n> Nit: we could actually `cp` for both the child processes here.\n\nWill do those, thanks!\n\n> It would be nice to cleanup the repo at the end of the test by adding\n> `test_when_finished \"rm -rf stat-dirty\" &&` here\n\nAll the files generated by the tests in this file are\nunder `trash directory.t6120-describe/` which seems to\nbe automatically deleted when all tests succeed.  Also,\ngiven that rest of the tests in this file don't perform\nany manual cleanup, I am not sure if doing so really\nfalls under the scope of this patch.\n\n> You want to do everything apart from the repo init in a subshell, this\n> ensures we don't carry over the working directory to the next test.\n\nAh, now I see why other tests wrap all commands except\n`git init` in parentheses.  Thanks!\n\n> Can't this be merged with the previous test?\n\nHaving those separate looked nicer to me.  I will merge\nthem together.\n\n"}]}