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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Abhijeet and Karthik

On 24/06/2024 11:56, Karthik Nayak wrote:
> Abhijeet Sonar <abhijeet.nkt@gmail.com> 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 <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);
>> +

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