From: Elijah Newren <newren@gmail.com>
Date: Sun, 29 Oct 2023 06:17:09 GMT
Subject: Re: [PATCH 1/1] merge-file: add an option to process object IDs
Message-ID: <CABPp-BG9Y6aZ+TWdkL4QE9e12fu3n61V16G6DLtawEDe=g9F4w@mail.gmail.com>
In-Reply-To: <20231024195655.2413191-2-sandals@crustytoothpaste.net>

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.

Also, why the extra synopsis instead of just adding a `[--object-id]`
option to the previous one?

[...]
> @@ -80,12 +88,21 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>
>                 fname = prefix_filename(prefix, argv[i]);
>
> -               if (read_mmfile(mmf, fname))
> +               if (object_id) {
> +                       if (repo_get_oid(the_repository, argv[i], &oid))
> +                               ret = -1;
> +                       else if (!oideq(&oid, the_hash_algo->empty_blob))
> +                               read_mmblob(mmf, &oid);
> +                       else
> +                               read_mmfile(mmf, "/dev/null");

Does "/dev/null" have any portability considerations?  (I really don't
know; just curious.)