From: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
Date: Tue, 09 Jun 2026 02:47:11 GMT
Subject: Re: [GSoC PATCH v2 1/4] path: introduce format_path() for centralized path formatting
Message-ID: <CA+rGoLdJY7d=331c_DyG9WXJbFxmaabag3yJpbts22uFWTjYPg@mail.gmail.com>
In-Reply-To: <22E79E77-BCC3-4622-BD39-F4ED7DDA9511@gmail.com>

>
> Nitpick: the documentation is clear to me, but maybe the function name
> "format" and the parameter name "buf" can mislead the user to think
> that it only formats the path without appending to the existing string
> in `buf`. My suggestion is to rename them to something like
> `append_formatted_path` and `dest`, respectively.
>

Ok, that's a good point!
I will add this in the next series!


>
> > +test_repo_info_path () {
> > + field_name=$1
> > + expect_absolute_eval=$2
> > + expect_relative=$3
> > + env_prefix=$4
>
> This helper function needs a documentation.
>

Alright, I will add that.

> > + test_expect_success "query individual key: path.$field_name.absolute${env_prefix:+ ($env_prefix)}" '
>
> This makes the output polluted. What about changing it by something like:
>
>         test_expect_success "absolute: $label' '...'
>         test_expect_success "relative: $label' '...'
>
> with a custom label?
>

Ahh, interesting.
I agree, I will look into this!

> > +
> > +test_expect_success 'setup test repository layout for path fields' '
> > + git init test-repo &&
> > + mkdir -p test-repo/sub
> > +'
>
> The helper function `test_repo_info_path` is relying too much on the
> existence of the `test-repo`. I think it would be better to add a new
> parameter `repo_name` (or similar) because
>
> 1. You could move this creation to the helper function and
>    you won't need to place the test after that creation
>
> 2. You could use different for each (test_repo_info_path call, path format)
>    pair. Currently, if more than one test fails, its result is overwritten
>    and the `expect` and `actual` files from the trash directory will be
>    the last of the broken tests.
>
> 3. You won't need to use the hacky 'echo "$(cd .. && pwd)'
>
> This applies my suggestions (feel free to use, adapt or discard it):
>

Thanks!
That is helpful.

Regards,
- K Jayatheerth