Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting

From: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
Date: Jun 9, 2026, 04:41 UTC
Message-ID: <CA+rGoLdpkuigWXqNSk3bS7-uhtzCizkPx2GGtNaTyy5J1SF7Rg@mail.gmail.com>
In-Reply-To: <aicDOlJdUrgMi3sA@denethor>

Show 9 quoted lines

> > +     if (format == PATH_FORMAT_UNMODIFIED) {
> > +             strbuf_addstr(buf, path);
> > +             return;
> > +     }
> > +
> > +     if (format == PATH_FORMAT_RELATIVE) {
>
> nit: we could just continue the "else if" chain here instead of
> restarting it.

Ahh, good catch! True we can.

Show 6 quoted lines

> > +             strbuf_realpath_forgiving(&canonical_buf, path, 1);
> > +             strbuf_addbuf(buf, &canonical_buf);
>
> Do we need `canonical_buf` here? Can we just add the path to `buf`
> directly?
>

canonical_buf is necessary if I my understanding is correct. We can't pass buf directly to strbuf_realpath_forgiving() because it resets its destination buffer before writing. Since format_path() has append semantics, doing so would clobber any existing content in buf. The intermediate canonical_buf is needed to keep that safe.

Show 7 quoted lines

> > +void format_path(struct strbuf *buf, const char *path,
> > +              const char *prefix, enum path_format format);
> > +
>
> Ok so in this patch we are just adding the new path formatting
> interface and will integrate it in the next one. Overall the direction
> of this patch looks good to me.

Yup, that's correct!

Show 9 quoted lines

> > and update print_path() to act as a light wrapper around the new shared
> > engine. Resolve user-provided formatting flags directly within rev-parse
> > to pass the final determined path_format to format_path().
>
> So if the format isn't explicitly set by the user via the
> `--path-format` option, the default formatting strategy used depends on
> the path being printed. IOW, there is no consistent default path format
> here.
>

Yes, that's correct.

Show 15 quoted lines

> > +     struct strbuf sb = STRBUF_INIT;
> > +     enum path_format fmt = (arg_path_format != -1) ? arg_path_format : def_format;
>
> hmmm, so `arg_path_format` specifies what the user-provided format and
> acts as a sentinel to signal there is no value provided and the fallback
> format needs to be used. This feels a tad bit awkward to me.
>
> I wonder if we should introduce a PATH_FORMAT_DEFAULT to the
> `path_format` enum that maps to one of the existing enum values in
> `path.c:format_path()`. Here in `print_path()`, we could then intercept
> a PATH_FORMAT_DEFAULT value and override it to the specified
> `def_format`. I'm not sure if this is ultimately that much better
> though.
>
> -Justin

You're right that the -1 is awkward it forces arg_path_format to be an int rather than the enum type itself, which loses type safety.

PATH_FORMAT_DEFAULT is cleaner in that regard, but it pushes the "what does default mean?" question into format_path() which currently has no notion of a fallback. Since the fallback is call-site specific (each path type in rev-parse has its own default), I'd rather keep that logic in print_path() where the context lives.

A middle ground would be adding PATH_FORMAT_DEFAULT to the enum but not handling it in format_path().

--- enum path_format_type format = PATH_FORMAT_DEFAULT;

/* ... */

static void print_path(const char *path, const char *prefix,
                       enum path_format_type format,
                       enum path_format_type def_format)
{
        struct strbuf sb = STRBUF_INIT;
        enum path_format_type fmt =
            (format == PATH_FORMAT_DEFAULT) ? def_format : format;

        format_path(&sb, path, prefix, fmt);
        puts(sb.buf);
        strbuf_release(&sb);
}
---

Show 5 quoted lines

> > +     format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_CANONICAL);
>
> For absolute paths, I don't think we actually need the prefix, but
> providing it doesn't probably matter too much either way.
>

Yeah, true. Since the relative had the prefix I just added the prefix here too. It is consistent.

Show 12 quoted lines

> >
> > +test_repo_info_path () {
> > +     field_name=$1
> > +     expect_absolute_eval=$2
> > +     expect_relative=$3
> > +     env_prefix=$4
>
> nit: I was a bit uncertain regarding the purpose of env_prefix here.
> Since the env_prefix is not used by any tests yet, I wonder if it we
> should delay adding it until the next patch. If we want to reduce churn
> though, I think we could also swap the order of patch 3 and 4.
>

Good point I will actually swap 3 and 4 It is just better tbh.

Show 6 quoted lines

> > +                     cd test-repo/sub &&
> > +                     expect_absolute=$(eval "$expect_absolute_eval") &&
>
> Can we just compute `expect_absolute` prior to passing it instead of
> using eval here?
>

Yes, I plan to follow Lucas's suggestion from his review. passing a repo_name parameter and capturing $PWD before the cd to construct the absolute path at helper-call time. That avoids eval entirely and also addresses his other concerns about test isolation. Will fix in v3.

Show 12 quoted lines

> > +test_expect_success 'setup test repository layout for path fields' '
> > +     git init test-repo &&
> > +     mkdir -p test-repo/sub
> > +'
> > +
> > +test_repo_info_path 'gitdir' 'echo "$(cd .. && pwd)/.git"' '../.git'
>
> hmmm, do we expect the path suffix to be the same between relative and
> absolute paths for all test cases? If so, we could just have a single
> `expect_path_suffix` argument and let the helper compute the appropriate
> absolute and relative paths internally.
>

Yes it is consistent between absolute and relative. This is a good suggestion. Also aligns with what Lucas said.

Thank you, This will help building v3 much smoother.

Regards,
- K Jayatheerth