Hi Patrick,
On Tue, 6 Feb 2024, Patrick Steinhardt wrote:
> On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> [snip]
> > diff --git a/rerere.c b/rerere.c
> > index ca7e77ba68c..13c94ded037 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,
> > mmfile[i].ptr = repo_read_object_file(the_repository,
> > &ce->oid, &type,
> > &size);
> > + if (!mmfile[i].ptr)
> > + die(_("unable to read %s"),
> > + oid_to_hex(&ce->oid));
> > mmfile[i].size = size;
> > }
> > }
>
> A few lines below this we check whether `mmfile[i].ptr` is `NULL` and
> replace it with the empty string if so. So this patch here is basically
> a change in behaviour where we now die instead of falling back to the
> empty value.
>
> I'm not familiar enough with the code to say whether the old behaviour
> is intended or not -- it certainly feels somewhat weird to me. But it
> did leave me wondering and could maybe use some explanation.
Hmm. That's a good point. The `mmfile[i].ptr == NULL` situation is indeed
handled specifically.
However, after reading the code I come to the conclusion that the `i`
refers to the stage of an index entry, i.e. that loop
(https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the
case where conflicts are due to deletions (where one side of the merge
deleted the file) or double-adds (where both sides of the merge added the
file, with different contents).
Therefore I would suggest that ignoring missing blobs (as is the pre-patch
behavior) would mishandle the available data and paper over a corruption
of the database (the blob is reachable via the Git index, but is missing).
Ciao,
Johannes
