Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> Hi Abhijeet and Karthik
>
> On 24/06/2024 11:56, Karthik Nayak wrote:
>> Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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@xxxxxxxxx>
>>> Reported-by: Paul Millar <paul.millar@xxxxxxx>
>>> Suggested-by: Junio C Hamano <gitster@xxxxxxxxx>
>>> ---
>>> 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.
>
I see. Thanks for correcting me!
>> 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
Attachment:
signature.asc
Description: PGP signature
