Re: [PATCH (Apple Git) 05/13] t5701: git --version can have SP in it

[Jeremy Huddleston Sequoia <jeremyhu@apple.com>]

> On Jan 30, 2019, at 05:36, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> 
> 
> That commit message is again very short. Only because I remember the
> previous patch's commit message do I have a clue what this is about.

Yeah, I became lax with commit messages when I was under the impression that the git community was not interested engaging with Apple.  I should have taken a pass through these commits before sending, but I half expected these patches to be ignored.  I am delighted to see that I was wrong and will go back and provide more details in the ones that I feel are upstreamable like this one.

> You definitely need to write something here about customized forks of Git
> adding suffixes including spaces to the Git version.
> 
> And you will need to state where those spaces are converted to dots in
> Git's capability advertisement. The reason for this requirement: should
> that logic change at any stage in the future, your patch will fail,
> somebody will investigate and find this commit and *needs* a helpful
> commit message.

Thanks, I'll add you to the CC of this patch, so you can critique my wording in the updated message.

> 
>> ---
>> t/t5701-git-serve.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
>> index ae79c6bbc0..7bc25700fa 100755
>> --- a/t/t5701-git-serve.sh
>> +++ b/t/t5701-git-serve.sh
>> @@ -7,7 +7,7 @@ test_description='test git-serve and server commands'
>> test_expect_success 'test capability advertisement' '
>> 	cat >expect <<-EOF &&
>> 	version 2
>> -	agent=git/$(git version | cut -d" " -f3)
>> +	agent=git/$(git --version | sed -e "s/git version //" -e "s/ /\./g")
> 
> This `git version` needs to be anchored,

Good catch, thanks.

> and it would be much conciser to
> use `-e "y/ /./"`, which even BSD sed understands according to
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Also done, thanks.

> 
> Ciao,
> Johannes
> 
>> 	ls-refs
>> 	fetch=shallow
>> 	server-option
>> -- 
>> 2.20.0 (Apple Git-115)
>> 
>>