From: Elijah Newren <newren@gmail.com>
Date: Mon, 30 Oct 2023 16:39:07 GMT
Subject: Re: [PATCH 1/1] merge-file: add an option to process object IDs
Message-ID: <CABPp-BHWjyaj9Z0Oj5UQdeSSf1hX+4DGCtYgMF+3hpWH7j40AQ@mail.gmail.com>
In-Reply-To: <ZT5p0WUw7EeaY8vW@tapette.crustytoothpaste.net>

Hi,

On Sun, Oct 29, 2023 at 7:18 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2023-10-29 at 06:17:09, Elijah Newren wrote:
> > Hi,
> >
> > Overall, looks good.  Just a couple questions...
> >
> > On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > >
> > > From: "brian m. carlson" <bk2204@github.com>
> > >
> > [...]
> > > --- a/Documentation/git-merge-file.txt
> > > +++ b/Documentation/git-merge-file.txt
> > > @@ -12,6 +12,9 @@ SYNOPSIS
> > >  'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
> > >         [--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
> > >         [--[no-]diff3] <current-file> <base-file> <other-file>
> > > +'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
> > > +       [--ours|--theirs|--union] [-q|--quiet] [--marker-size=<n>]
> > > +       [--[no-]diff3] <current-oid> <base-oid> <other-oid>
> >
> > Why was the `[-p|--stdout]` option removed in the second synopsis?
> > Elsewhere you explicitly call it out as a possibility to be used with
> > --object-id.
>
> Originally because it implied `-p`, but I changed that to write into the
> object store.  I'll restore it.
>
> > Also, why the extra synopsis instead of just adding a `[--object-id]`
> > option to the previous one?
>
> Because there's a relevant difference: the former has <current-file>,
> <base-file>, and <other-file>, and the latter has the -oid versions.

Ah, I looked over it a couple times and just kept missing that
difference.  Thanks for pointing it out.

>
> > Does "/dev/null" have any portability considerations?  (I really don't
> > know; just curious.)
>
> We already use it elsewhere in the codebase, so I assume it works.  We
> also have a test for that case and it worked in CI, so it's probably
> fine.

Seems reasonable to me; thanks.