Thread (82 messages) 82 messages, 6 authors, 10d ago

Re: [GSoC PATCH v2 1/4] path: introduce format_path() for centralized path formatting

From: K Jayatheerth <hidden>
Date: 2026-06-09 02:47:24

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!

quoted
+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.
quoted
+ 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!
quoted
+
+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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help