Re: [PATCH] describe: refresh the index when 'broken' flag is used

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 24, 2024, 10:56 UTC
Message-ID: <CAOLa=ZRGramQ3MdzzXzZ19yeUB_rQZPbZ3u=eA=T2SfV3nhYOA@mail.gmail.com>
In-Reply-To: <20240623214301.143796-1-abhijeet.nkt@gmail.com>

Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:

Show 39 quoted lines

> 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.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> Reported-by: Paul Millar <paul.millar@desy.de>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> ---
>  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);
> +
> +
>

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.

Could you perhaps state how you can reproduce the issue mentioned?

Also apart from that, we should add a test to capture the changes.

>  			cp.git_cmd = 1;
>  			cp.no_stdin = 1;
> --
> 2.45.GIT