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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 9, 2024, 08:06 UTC
Message-ID: <5fd95ae0-cd50-ddc4-5095-ee953c2640b3@gmx.de>
In-Reply-To: <CAO_smVhrMn=-uF1B6+RA8A+VLCEN=o57zbQPtr8hpxRKY=qJRQ@mail.gmail.com>

Hi Kyle,

On Mon, 5 Feb 2024, Kyle Lippincott wrote:

Show 17 quoted lines

> On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > index e65cae0bcf7..caf20fd5bdd 100644
> > --- a/builtin/notes.c
> > +++ b/builtin/notes.c
> > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >                 struct strbuf buf = STRBUF_INIT;
> >                 char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
> >
> > -               if (prev_buf && size)
> > +               if (!prev_buf)
> > +                       die(_("unable to read %s"), oid_to_hex(note));
>
> This changes the behavior of this function. Previously, it would not
> add prev_buf output, but still succeed. This now dies.

It does change behavior. The previous behavior looked up the note OID, then tried to read it, and if it was missing just pretended that there had not been a note.

I'm not quite sure whether we should keep that behavior, as it is unclear in which scenarios it would be desirable to paper over missing objects.

In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to have this behavior: It would lose information.

And even in scenarios where the `notes` ref is fetch shallowly, I would expect all of the actual notes blobs to be present, and I would _want_ the `git note edit ...` command to error out when that blob is not found.

Does that reasoning make sense to you?

Ciao, Johannes