Re: [PATCH 1/1] merge-file: add an option to process object IDs
- From
Elijah Newren <newren@gmail.com>
- Date
- Oct 30, 2023, 16:39 UTC
- 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:
Show 34 quoted lines
> > 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.
Show 7 quoted lines
> > > 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.