Re: [PATCH 1/1] Exit on invalid diff status of diff_filepair

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2025, 13:23 UTC
Message-ID: <xmqq8qngzcpg.fsf@gitster.g>
In-Reply-To: <CAPig+cRehhM-z0md_iV-VYCk_Qwv0A4zS1TfPqPxsrLZ3eYxvA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 17 quoted lines

>>     git (main) $ cd ~/Repos/ECS50-3/hw3-skeleton-broken/
>>     hw3-skeleton-broken (main) $ ~/Build/git/git diff
>>     Segmentation fault (core dumped)
>>     hw3-skeleton-broken (main) $
>>
>> Let me know if you have any feedback or suggestions.
>
> Do you have a reproduction recipe which demonstrates the problem which
> your patch fixes? Including the recipe in the patch's commit message
> would help reviewers better understand the circumstances under which
> the crash occurs since the descriptions provided by both the original
> problem report[1] and the submitted patch[2] seem rather nebulous[3].
>
> More importantly, if you have a reproduction recipe, then it can be
> used as the basis for creating a test which should accompany the patch
> (and which should be added to one of the `t/t40xx-*.sh` files). We can
> help you convert the reproduction recipe into a test if desired.

Nicely put.

It is a bug _elsewhere_ in the code for the .status member to be unassigned when the control reaches that point, so a patch to exit after that happens is not all that interesting. Instead of exiting there, we would want to see a patch against the place where it should have set the .status member but fails to do so.

Maybe with a corrupted repository, some of the blob objects we read for comparison may fail to load and the function may be taking an early return instead of complaining and dying upon unreadable blob (I am not saying that is the only or even a likely case; just giving an example situation for illustrate the point), in which case we would want to fix _that_ code to complain and die properly.

Thanks.