Re: [PATCH] Always check the return value of `repo_read_object_file()`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 9, 2024, 08:15 UTC
Message-ID: <3f14077f-c70c-5eef-5b25-984fdf7b3b68@gmx.de>
In-Reply-To: <ZcHW_bc6N5umk2G4@tanuki>

Hi Patrick,

On Tue, 6 Feb 2024, Patrick Steinhardt wrote:

Show 26 quoted lines

> On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [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