Matheus Tavares <matheus.bernardino@xxxxxx> writes: > 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@xxxxxxxxx> 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. > 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.
