From: Phillip Wood Date: Mon, 24 Jun 2024 11:20:40 GMT Subject: Re: [PATCH] describe: refresh the index when 'broken' flag is used Message-ID: <054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com> In-Reply-To: Hi Abhijeet and Karthik On 24/06/2024 11:56, Karthik Nayak wrote: > Abhijeet Sonar writes: > >> When describe is run with 'dirty' flag, we refresh the index >> to make sure it is in sync with the filesystem before >> determining if the working tree is dirty. However, this is >> not done for the codepath where the 'broken' flag is used. >> >> This causes `git describe --broken --dirty` to false >> positively report the worktree being dirty. Refreshing the >> index before running diff-index fixes the problem. This is a good description of the problem the patch fixes. >> Signed-off-by: Abhijeet Sonar >> Reported-by: Paul Millar >> Suggested-by: Junio C Hamano >> --- >> builtin/describe.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/builtin/describe.c b/builtin/describe.c >> index e5287eddf2..2b443c155e 100644 >> --- a/builtin/describe.c >> +++ b/builtin/describe.c >> @@ -645,6 +645,20 @@ int cmd_describe(int argc, const char **argv, const char *prefix) >> if (argc == 0) { >> if (broken) { >> struct child_process cp = CHILD_PROCESS_INIT; >> + struct lock_file index_lock = LOCK_INIT; >> + int fd; >> + >> + setup_work_tree(); >> + prepare_repo_settings(the_repository); >> + repo_read_index(the_repository); >> + refresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED, >> + NULL, NULL, NULL); >> + fd = repo_hold_locked_index(the_repository, >> + &index_lock, 0); >> + if (0 <= fd) >> + repo_update_index_if_able(the_repository, &index_lock); >> + As we're dealing with a repository that might be broken I suspect we'd be better to run "git update-index --unmerged -q --refresh" as a subprocess in the same way that we run "git diff-index" so that "git describe --broken" does not die if the index cannot be refreshed. > I'm wondering why this needs to be done, as I can see, when we use the > '--broken' flag, we create a child process to run `git diff-index > --quiet HEAD`. As such, we shouldn't have to refresh the index here. "git diff-index" and "git diff-files" do not refresh the index. This is by design so that a script can refresh the index once and run "git diff-index" several times without wasting time updating the index each time. > Also apart from that, we should add a test to capture the changes. That would be nice Best Wishes Phillip >> cp.git_cmd = 1; >> cp.no_stdin = 1; >> -- >> 2.45.GIT