Re: [PATCH v4 2/2] rev-parse: add option for absolute or relative path formatting

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 7, 2020, 00:30 UTC
Message-ID: <CAPig+cS-2Vw84rejMFAiDeF8dd5gtBOmQZUMpOy2ufA8nU7W7g@mail.gmail.com>
In-Reply-To: <20201206225349.3392790-3-sandals@crustytoothpaste.net>

On Sun, Dec 6, 2020 at 5:58 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 16 quoted lines

> [...]
> To help out the scripter, let's provide an option which turns most of
> the paths printed by git rev-parse to be either relative to the current
> working directory or absolute and canonical.  Document which options are
> affected and which are not so that users are not confused.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> @@ -212,6 +212,15 @@ Options for Files
> +--path-format=(absolute|relative)::
> +       Controls the behavior of certain other options following it on the command
> +       line. If specified as absolute, the paths printed by those options will be
> +       absolute and canonical. If specified as relative, the paths will be relative
> +       to the current working directory if that is possible.  The default is option
> +       specific.

I read the commit message and looked at the implementation so I know that this option can be specified multiple times, but this documentation only vaguely hints at it by saying "options following it". We could do a better job of imparting that knowledge to the reader by saying so explicitly, perhaps like this:

    Controls the behavior of some path-printing options. If
    'absolute', [...]. If 'relative', [...]. May be specified multiple
    times, each time affecting the path-printing options which
    follow it. The default path format is option-specific.

Since this option can be specified multiple times, should it also recognize `default` to request the default behavior in addition to `absolute` and `relative`? (Genuine question. Maybe real-world use-cases wouldn't need it, but it would be easy to support and make it functionally complete.)

Show 15 quoted lines

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -583,6 +583,73 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
> +static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
> +{
> +               struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
> +               if (!is_absolute_path(path)) {
> +                       strbuf_realpath_missing(&realbuf, path, 1, 1);
> +                       path = realbuf.buf;
> +               }
> +               if (!is_absolute_path(prefix)) {
> +                       strbuf_realpath_missing(&prefixbuf, prefix, 1, 1);
> +                       prefix = prefixbuf.buf;
> +               }
> +               puts(relative_path(path, prefix, &buf));
> +               strbuf_release(&buf);

Leaking `realbuf` and `prefixbuf` here.