Re: [PATCH v5 1/2] abspath: add a function to resolve paths with missing components

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 13, 2020, 02:35 UTC
Message-ID: <CAPig+cTFJv8LZWp6Mvi1OXabZ+5NmEw8fu8NeQuwhZDAbnA59Q@mail.gmail.com>
In-Reply-To: <20201213002529.542928-2-sandals@crustytoothpaste.net>

On Sat, Dec 12, 2020 at 7:26 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 11 quoted lines

> Currently, we have a function to resolve paths, strbuf_realpath.  This
> function canonicalizes paths like realpath(3), but permits a trailing
> component to be absent from the file system.  In other words, this is
> the behavior of the GNU realpath(1) without any arguments.
>
> In the future, we'll need this same behavior, except that we want to
> allow for any number of missing trailing components, which is the
> behavior of GNU realpath(1) with the -m option.  This is useful because
> we'll want to canonicalize a path that may point to a not yet present
> path under the .git directory.  For example, a user may want to know
> where an arbitrary ref would be stored if it existed in the file system.

This improved explanation helps the reader better understand why such functionality is wanted. Thanks.

Show 13 quoted lines

> Let's refactor strbuf_realpath to move most of the code to an internal
> function and then pass it two flags to control its behavior.  We'll add
> a strbuf_realpath_forgiving function that has our new behavior, and
> leave strbuf_realpath with the older, stricter behavior.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/abspath.c b/abspath.c
> +#define REALPATH_MANY_MISSING (1 << 0)
> +#define REALPATH_DIE_ON_ERROR (1 << 1)
> +
> +static char *strbuf_realpath_1(struct strbuf *resolved, const char *path,
> +                              int flags)

We normally declare these composed-of-bits "flags" arguments as `unsigned` rather than `int`.

> @@ -89,7 +85,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>         if (!*path) {
> -               if (die_on_error)
> +               if (flags & REALPATH_DIE_ON_ERROR)

Nit: If you declare a local variable named `die_on_error`:

    int die_on_error = flags & REALPATH_DIE_ON_ERROR;

then you won't have to touch the existing five places where this condition is checked, thus making the diff less noisy and the code (subjectively) a bit easier to read at a glance. Not at all worth a re-roll, though.

Aside from these minor comments, this version is a nice improvement over the last.