Re: [GSoC Patch v4 2/4] rev-parse: use append_formatted_path() for path formatting

From: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
Date: Jun 16, 2026, 04:19 UTC
Message-ID: <CA+rGoLf6Tj-j0r3cCReBaKK5bGFUALJ638-yPi2GSoRML0kbgA@mail.gmail.com>
In-Reply-To: <ajAy1it6CGDQzVes@denethor>

Hey Justin,

Show 6 quoted lines

>
> Without context, it might be a bit confusing to readers as to why we
> override PATH_FORMAT_DEFAULT without our own provided default. It may be
> worth leaving a comment to provide some breadcrumbs.
>
> The rest of this patch looks good to me.

That makes sense. That's a minor change. I will add proper comments!

Show 15 quoted lines

> > Add a test helper test_repo_info_path that creates isolated
> > repositories per test case to prevent state leaks, captures the repo
> > root before changing directories to avoid eval, and accepts an optional
> > init_command to cover environment variable overrides such as
> > GIT_COMMON_DIR and GIT_DIR.
>
> I'm not sure this last paragraph in the log message provides much value.
> To me it's a bit verbose and focuses mostly on what the test helper is
> doing. Maybe we can just omit this section? If we want to have a note
> though maybe we could say something like:
>
>   Each path key is expected to have an absolute and relative form. To
>   reduce duplication, a test_repo_info_path helper function is
>   introduced to configure and exercise both cases.
>

Now that I think about it Maybe removing it is a better option.

I mean the patch itself contains the test and it has comments explaining the test itself.

I am gonna remove the last para in the next series. Thanks for pointing that out!

Show 13 quoted lines

> > +test_repo_info_path () {
> > +     label=$1
> > +     field_name=$2
> > +     repo_name=$3
> > +     expect_absolute_suffix=$4
> > +     expect_relative=$5
> > +     init_command=$6
>
> I may be overthinking it, but I can't help but feel this test helper is
> overly complicated. I wonder if we can simlify and reduce the number of
> arguments. For example, could we programatically construct the label
> from the field name and init_command instead of explicitly passing it?
>

That’s a fair question But I personally don't think the helper is overly complicated. I think a lot of the current helper can be mapped with test_repo_info's structure itself.

The existing helper uses a very similar 5-argument signature (label, init_command, repo_name, key, expected_value) and separates the setup step from the assertion steps.

Regarding the labels, I'd prefer to keep them explicitly passed in. Programmatically constructing the label from the init_command could result in messy or hard-to-read test descriptions in the console output, and having explicit strings makes it much easier to debug when a specific test fails.

Show 20 quoted lines

> > +     absolute_root="$repo_name-absolute"
> > +     relative_root="$repo_name-relative"
> > +
> > +     test_expect_success "setup: $label" '
> > +             git init "$absolute_root" &&
> > +             git init "$relative_root" &&
> > +             mkdir -p "$absolute_root/sub" "$relative_root/sub"
> > +     '
>
> Do really need this setup test case? Could we instead embed the setup in
> both test cases below? Something like:
>
>         test_when_finished rm -rf repo &&
>         git init repo &&
>         (
>           mkdir repo/sub &&
>           cd repo/sub &&
>           ...
>         )
>

That's a much more elegant way to handle it. I will incorporate this in v5!

Show 5 quoted lines

> With something like this, each test case is responsible to creating its
> own repo and cleaning it up when finished. Then we could avoid have to
> provide a separate repo name for each set of test cases and remove the
> repo_name argument.
>

True, Thanks!

Show 18 quoted lines

> > +test_repo_info_path 'commondir standard' 'commondir' 'commondir-std' \
> > +     '.git' '../.git'
> > +
> > +test_repo_info_path 'commondir with GIT_COMMON_DIR and GIT_DIR' 'commondir' \
> > +     'commondir-envs' 'custom-common' '../custom-common' \
> > +     'GIT_COMMON_DIR="$ROOT/custom-common" && export GIT_COMMON_DIR &&
> > +      GIT_DIR="../.git" && export GIT_DIR &&
> > +      git init --bare "$ROOT/custom-common"'
> > +
> > +test_repo_info_path 'commondir with only GIT_DIR' 'commondir' \
> > +     'commondir-only-gitdir' '.git' '../.git' \
>
> For each of these test cases, the `expect_absolute_suffix` and
> `expect_relative` and exactly the same. This also appears to be the case
> for the test cases in the next patch. Do these really need to be
> configurable at all? Can we just embed them directly in each test case
> assertion? Or maybe future keys will need this to be configurable?
>

You're right that passing both is completely redundant! However, the path does still need to be configurable because the directory name changes between test cases (e.g., `.git` in the standard case vs. `custom-common` when GIT_COMMON_DIR is exported).

Since the relative path is always just `../` appended to the directory name, I will condense these two arguments into a single `expected_dir` argument in v5. The helper will then just construct `$ROOT/$expected_dir` and `../$expected_dir` internally.

Show 6 quoted lines

> > +     'GIT_DIR="../.git" && export GIT_DIR'
> > +
> >  test_done
>
> Overall the rest of the patch looks good to me.
>

Thanks again! These are helpful.

Regards,
- K Jayatheerth