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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

On Fri, Feb 09, 2024 at 09:15:15AM +0100, Johannes Schindelin wrote:
> 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@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).

Yeah, that explanation sounds reasonable to me, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]