Re: [PATCH v2] Make ident dynamic, not just a hardcoded value of "$Id".

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2021, 00:58 UTC
Message-ID: <xmqqr1e77q01.fsf@gitster.g>
In-Reply-To: <20210826203713.45139-1-matheus.bernardino@usp.br>

Matheus Tavares <matheus.bernardino@usp.br> writes:

Show 24 quoted lines

> Hi, Maksym
>
> I haven't read the entire patch (and I don't normally use the ident feature),
> but I left a few comments below: 
>
> On Thu, Aug 26, 2021 at 1:28 AM Maksym Sobolyev via GitGitGadget <gitgitgadget@gmail.com> wrote:
>>
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index 83fd4e19a4..9e486f3e8d 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -382,6 +382,14 @@ sign `$` upon checkout.  Any byte sequence that begins with
>>  `$Id:` and ends with `$` in the worktree file is replaced
>>  with `$Id$` upon check-in.
>>
>> +The `ident` attribute can also provide an optional value,
>> +which if supplied is going to be used for expansion instead of
>> +the string `Id`.
>> +
>> +------------------------
>> +*.[ch]         ident=FreeBSD
>> +------------------------
>
> What happens if there is a ':' or '$' in the custom id name?

It is a valid question. I actually think these two bytes should be excluded from the custom ID string --- FWIW, I also do not think there is any practical problem if we limited the set of characters to [A-Za-z0-9] and nothing else.

Show 21 quoted lines

> You might want to take a look at the other ident tests in
> t0021-conversion.sh and perhaps addapt/copy some of them to ensure that
> the expected behavior persists when using a custom id name. For example,
> quickly changing the "$Id:" references to "$customId:" and replacing
> "ident" with "ident=customId", I seem to get at least one test failure:
>
> --- expected-output     2021-08-26 19:40:07.181596662 +0000
> +++ expanded-keywords   2021-08-26 19:40:07.188263463 +0000
> @@ -6,5 +6,5 @@
>  $customId: bebd07c752ffffd6779e1056db5de66c3bb733ed $
>  $customId: bebd07c752ffffd6779e1056db5de66c3bb733ed $
>  $customId: NoTerminatingSymbol
> -$customId: Foreign Commit With Spaces $
> +$customId: bebd07c752ffffd6779e1056db5de66c3bb733ed $
>  $customId: NoTerminatingSymbolAtEOF
> \ No newline at end of file
> error: last command exited with $?=1
> not ok 3 - expanded_in_repo
>
> (But I haven't dig further, and this was just a quick test, so it could be my
> fault.)

If we are not restricting the characters that can be used in the custom ID placeholder, we probably should have tests that use allowed but unusual characters.

Thanks.