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

[Kyle Lippincott <spectral@google.com>]

On Fri, Feb 9, 2024 at 12:06 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Kyle,
>
> On Mon, 5 Feb 2024, Kyle Lippincott wrote:
>
> > 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?

Sounds good, thanks for talking through it with me.

>
> Ciao,
> Johannes