Re: [PATCH v3 6/8] fast-import: document C-style escapes for paths

[Thalia Archibald <thalia@archibald.dev>]

On Apr 10, 2024, at 11:28, Junio C Hamano <gitster@pobox.com> wrote:
> Thalia Archibald <thalia@archibald.dev> writes:
>> 
>> -test_path_eol_success 'quoted spaces'   '" hello world.c "' ' hello world.c '
>> -test_path_eol_success 'unquoted spaces' ' hello world.c '   ' hello world.c '
>> +test_path_eol_success 'quoted spaces'   '" hello world.c "'  ' hello world.c '
>> +test_path_eol_success 'unquoted spaces' ' hello world.c '    ' hello world.c '
>> +test_path_eol_success 'octal escapes'   '"\150\151\056\143"' 'hi.c'
> 
> It is annoying to see these changes (and the same for the next
> hunk).  Would it make a lot of damage to existing lines in this file
> if we just say "do not align with extra spaces in between strings"?
> If so, that is a good reason to keep doing things this way, but if I
> recall correctly, these test_path_eol/space_success are what this
> series added to the file, so if we stop such alignment from the get-go,
> it may be alright.
> 
>> -test_path_space_success 'quoted spaces'      '" hello world.c "' ' hello world.c '
>> -test_path_space_success 'no unquoted spaces' 'hello_world.c'     'hello_world.c'
>> +test_path_space_success 'quoted spaces'      '" hello world.c "'  ' hello world.c '
>> +test_path_space_success 'no unquoted spaces' 'hello_world.c'      'hello_world.c'
>> +test_path_space_success 'octal escapes'      '"\150\151\056\143"' ‘hi.c'

Is it a style problem, that you prefer parameters to not be aligned? I
think it reads nicer this way, especially because there are quotes
within quotes and spaces at the starts and ends of strings, which can
lead to reinterpreting the boundaries of the strings on a less-careful
read through. They’re like a table of tests. But ultimately, it should
be the Git style that prevails not mine, so if that’s it, I’ll change
it.

Or I could preemptively align them according to the final alignment in
this series. I expect there wouldn't be many changes to these tests
later, so it should be stable.

I expected more pushback with 3/8, where 9 tests were indented to place
them inside loops in order to test them with multiple values for root,
so it seems not to be purely about whitespace changes in diffs.

In any case, it’s not a big deal and I’m happy to go with your
direction.

Thalia