| Message ID | 20240626190801.68472-1-abhijeet.nkt@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v7] describe: refresh the index when 'broken' flag is used | expand |
I have a question: Why does --dirty code path also not call git-update-index and instead does setup_work_tree(); prepare_repo_settings(the_repository); the_repository->settings.command_requires_full_index = 0; repo_read_index(the_repository); refresh_index(...); fd = repo_hold_locked_index(...); if (0 <= fd) repo_update_index_if_able(the_repository, &index_lock); I assume they are equivalent? The commit which introduced this -- bb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index when run with --dirty) seems to be for the same objective as mu patch. Is this something you would like to see addressed? Thanks
On 27/06/24 00:55, Abhijeet Sonar wrote: > I have a question: > > Why does --dirty code path also not call git-update-index and instead does > > setup_work_tree(); > prepare_repo_settings(the_repository); > the_repository->settings.command_requires_full_index = 0; > repo_read_index(the_repository); > refresh_index(...); > fd = repo_hold_locked_index(...); > if (0 <= fd) > repo_update_index_if_able(the_repository, &index_lock); > > I assume they are equivalent? > The commit which introduced this -- > bb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index > when run with --dirty) seems to be for the same objective as mu patch. > > Is this something you would like to see addressed? > > Thanks s/mu/my
Abhijeet Sonar <abhijeet.nkt@gmail.com> writes: > I have a question: > > Why does --dirty code path also not call git-update-index and instead does > > setup_work_tree(); > prepare_repo_settings(the_repository); > the_repository->settings.command_requires_full_index = 0; > repo_read_index(the_repository); > refresh_index(...); > fd = repo_hold_locked_index(...); > if (0 <= fd) > repo_update_index_if_able(the_repository, &index_lock); > > I assume they are equivalent? Now we are going back full circles ;-)? Your earliest attempt indeed copied the above to the code paths used to handle "--broken", but then Phillip corrected the course https://lore.kernel.org/git/054c6ac1-4714-4600-afa5-7e9b6e9b0e72@gmail.com/ to avoid triggering an in-process error and instead run an equivalent "update-index --refresh" via the run_command() interface, so that we can catch potential errors. The code in the more recent rounds of your patch uses that, no? > The commit which introduced this -- > bb571486ae93d02746c4bcc8032bde306f6d399a (describe: Refresh the index > when run with --dirty) seems to be for the same objective as mu patch. Yes, but the cited message above explains the reason why the two code paths in your patch use different implementations, I would think.
On 27/06/24 21:17, Junio C Hamano wrote: > to avoid triggering an in-process error and instead run an > equivalent "update-index --refresh" via the run_command() interface, > so that we can catch potential errors. Thanks, I get it now.
diff --git a/builtin/describe.c b/builtin/describe.c index e5287eddf2..cf8edc4222 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -53,6 +53,10 @@ static const char *diff_index_args[] = { "diff-index", "--quiet", "HEAD", "--", NULL }; +static const char *update_index_args[] = { + "update-index", "--unmerged", "-q", "--refresh", NULL +}; + struct commit_name { struct hashmap_entry entry; struct object_id peeled; @@ -645,6 +649,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (argc == 0) { if (broken) { struct child_process cp = CHILD_PROCESS_INIT; + + strvec_pushv(&cp.args, update_index_args); + cp.git_cmd = 1; + cp.no_stdin = 1; + cp.no_stdout = 1; + run_command(&cp); + + child_process_init(&cp); strvec_pushv(&cp.args, diff_index_args); cp.git_cmd = 1; cp.no_stdin = 1; diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index e78315d23d..79e0f19deb 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -671,4 +671,40 @@ test_expect_success 'setup misleading taggerdates' ' check_describe newer-tag-older-commit~1 --contains unique-file~2 +test_expect_success 'describe --dirty with a file with changed stat' ' + test_when_finished "rm -fr stat-dirty" && + git init stat-dirty && + ( + cd stat-dirty && + + echo A >file && + git add file && + git commit -m A && + git tag A -a -m A && + echo "A" >expect && + + test-tool chmtime -10 file && + git describe --dirty >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe --broken --dirty with a file with changed stat' ' + test_when_finished "rm -fr stat-dirty" && + git init stat-dirty && + ( + cd stat-dirty && + + echo A >file && + git add file && + git commit -m A && + git tag A -a -m A && + echo "A" >expect && + + test-tool chmtime -10 file && + git describe --dirty --broken >actual && + test_cmp expect actual + ) +' + test_done
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 if a file has different stat info than what is recorded in the index. Running `git update-index -q --refresh` to refresh the index before running diff-index fixes the problem. Also add tests to deliberately update stat info of a file before running describe to verify it behaves correctly. Reported-by: Paul Millar <paul.millar@desy.de> Suggested-by: Junio C Hamano <gitster@pobox.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com> --- builtin/describe.c | 12 ++++++++++++ t/t6120-describe.sh | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)