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@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).
Yeah, that explanation sounds reasonable to me, thanks!
Patrick
Attachment:
signature.asc
Description: PGP signature
