threads / patch / 16964

patchcvsserver: change generation of CVS author names

Subject: [PATCH] cvsserver: change generation of CVS author names

## tl;dr

8 messages between Jan 2, 2009 and Jan 6, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Fabian Emmes· Jan 2, 2009, 15:40 UTC · lore

[PATCH] cvsserver: add option to configure commit message

cvsserver annotates each commit message by "via git-CVS emulator". This is made configurable via gitcvs.commitmsgannotation.

Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
---
 Documentation/config.txt |    4 ++++
 git-cvsserver.perl       |    8 +++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)
Show changes to 2 files +11 −1

Documentation/config.txt, git-cvsserver.perl

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7408bb2..8b14d8a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -723,6 +723,10 @@ gc.rerereunresolved::
 	kept for this many days when 'git-rerere gc' is run.
 	The default is 15 days.  See linkgit:git-rerere[1].
 
+gitcvs.commitmsgannotation::
+	Append this string to each commit message. Set to empty string
+	to disable this feature. Defaults to "via git-CVS emulator".
+
 gitcvs.enabled::
 	Whether the CVS server interface is enabled for this repository.
 	See linkgit:git-cvsserver[1].
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index b0a805c..cbcaeb4 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1358,7 +1358,13 @@ sub req_ci
     # write our commit message out if we have one ...
     my ( $msg_fh, $msg_filename ) = tempfile( DIR => $TEMP_DIR );
     print $msg_fh $state->{opt}{m};# if ( exists ( $state->{opt}{m} ) );
-    print $msg_fh "\n\nvia git-CVS emulator\n";
+    if ( defined ( $cfg->{gitcvs}{commitmsgannotation} ) ) {
+        if ($cfg->{gitcvs}{commitmsgannotation} !~ /^\s*$/ ) {
+            print $msg_fh "\n\n".$cfg->{gitcvs}{commitmsgannotation}."\n"
+        }
+    } else {
+        print $msg_fh "\n\nvia git-CVS emulator\n";
+    }
     close $msg_fh;
 
     my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`;
-- 
1.5.6.3
Fabian Emmes· Jan 2, 2009, 15:40 UTC · re: Fabian Emmes · lore

CVS username is generated from local part email address. We take the whole local part but restrict the character set to the Portable Filename Character Set, which is used for Unix login names according to Single Unix Specification v3.

Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
---
 git-cvsserver.perl |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)
Show changes to git-cvsserver.perl +9 −3
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index cbcaeb4..fef7faf 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2533,12 +2533,18 @@ sub open_blob_or_die
     return $fh;
 }
 
-# Generate a CVS author name from Git author information, by taking
-# the first eight characters of the user part of the email address.
+# Generate a CVS author name from Git author information, by taking the local
+# part of the email address and replacing characters not in the Portable
+# Filename Character Set (see IEEE Std 1003.1-2001, 3.276) by underscores. CVS
+# Login names are Unix login names, which should be restricted to this
+# character set.
 sub cvs_author
 {
     my $author_line = shift;
-    (my $author) = $author_line =~ /<([^>@]{1,8})/;
+    (my $author) = $author_line =~ /<([^@>]*)/;
+
+    $author =~ s/[^-a-zA-Z0-9_.]/_/g;
+    $author =~ s/^-/_/;
 
     $author;
 }
-- 
1.5.6.3
Junio C Hamano· Jan 3, 2009, 22:14 UTC · re: Fabian Emmes · lore

Re: [PATCH] cvsserver: change generation of CVS author names

Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
Show 7 quoted lines
> CVS username is generated from local part email address.
> We take the whole local part but restrict the character set to the
> Portable Filename Character Set, which is used for Unix login names
> according to Single Unix Specification v3.
>
> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>

Stating "we should have done this from day one" is one thing (even though "because some standard says so" is not particularly a good justification without "and matches the way people use CVS in the real world in practice" appended to it).

"We should suddenly change the behaviour" is quite a different thing and it depends on what follows that sentence if the change is justifiable. We do not want to hear "...; screw the existing repositories if they have nonconforming names.". It is Ok if it is "...; existing repositories will be affected, but the damage is limited to very minor set of operations, namely X, Y and Z".

In other words, is there any backward compatibility issue when a repository that has served existing CVS users and checkouts with older version switches to the patched one? If there is one, is that grave enough that we should care?

Show 29 quoted lines
>  git-cvsserver.perl |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index cbcaeb4..fef7faf 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -2533,12 +2533,18 @@ sub open_blob_or_die
>      return $fh;
>  }
>  
> -# Generate a CVS author name from Git author information, by taking
> -# the first eight characters of the user part of the email address.
> +# Generate a CVS author name from Git author information, by taking the local
> +# part of the email address and replacing characters not in the Portable
> +# Filename Character Set (see IEEE Std 1003.1-2001, 3.276) by underscores. CVS
> +# Login names are Unix login names, which should be restricted to this
> +# character set.
>  sub cvs_author
>  {
>      my $author_line = shift;
> -    (my $author) = $author_line =~ /<([^>@]{1,8})/;
> +    (my $author) = $author_line =~ /<([^@>]*)/;
> +
> +    $author =~ s/[^-a-zA-Z0-9_.]/_/g;
> +    $author =~ s/^-/_/;
>  
>      $author;
>  }
Lars Noschinski· Jan 4, 2009, 11:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] cvsserver: change generation of CVS author names

* Junio C Hamano <gitster@pobox.com> [09-01-03 23:36]:
Show 14 quoted lines
>Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
>
>> CVS username is generated from local part email address.
>> We take the whole local part but restrict the character set to the
>> Portable Filename Character Set, which is used for Unix login names
>> according to Single Unix Specification v3.
>>
>> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
>> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
>
>Stating "we should have done this from day one" is one thing (even though
>"because some standard says so" is not particularly a good justification
>without "and matches the way people use CVS in the real world in practice"
>appended to it).

Documentation about valid cvs/rcs usernames is a bit scarce. When we wrote the patch, we did not find much more information than "the cvs username is supposed to be the login name". In my limited CVS experience, I never saw CVS user names which were not (unix) login names.

After this mail, I looked to the RCS source code (see checkidentifier() in rcslex.c) which tells us that anything (encoded in ISO-8859-1) consisting of IDCHAR, LETTER, Letter, DIGIT and PERIOD, containing at least one IDCHAR, LETTER or Letter is a valid username (for the character classes, see http://avalon.hoffentlich.net/~cebewee/rcs-charmap.txt) The most important character _not_ allowed in an user name is the @ sign, so we cannot use the full mail address.

So our patch generates a valid username for any "sane" local part. In a few corner cases like "!#$%&'*+-/=?^_`.{|}~@example.com" our patch generates a result worse than the original - an empty username. This is probably something we should fix.

Obviously, the short names generated are not necessarily unique, which can be irritating, but is not a problem from a technical point of view. Improving this would probably require to store a map of mail addresses to cvs user names.

Show 11 quoted lines
>"We should suddenly change the behaviour" is quite a different thing and
>it depends on what follows that sentence if the change is justifiable.  We
>do not want to hear "...; screw the existing repositories if they have
>nonconforming names.".  It is Ok if it is "...; existing repositories will
>be affected, but the damage is limited to very minor set of operations,
>namely X, Y and Z".
>
>In other words, is there any backward compatibility issue when a
>repository that has served existing CVS users and checkouts with older
>version switches to the patched one?  If there is one, is that grave
>enough that we should care?

Obviously the reported user names change. To the best of my knowledge (but I'm just a barely experienced CVS user) those names are not stored anywhere on the client and are regenerated by git-cvsserver for every request, so even old repositories get the new names for all commits.

   - Lars.
Junio C Hamano· Jan 6, 2009, 08:18 UTC · re: Lars Noschinski · lore

Re: [PATCH] cvsserver: change generation of CVS author names

Lars Noschinski <lars@public.noschinski.de> writes:
> Obviously the reported user names change. To the best of my knowledge
> (but I'm just a barely experienced CVS user) those names are not stored
> anywhere on the client and are regenerated by git-cvsserver for every
> request, so even old repositories get the new names for all commits.

Thanks for a clarification. I'll amend the commit log message and queue the result for 'next'.

commit d500a1ee8fe4424beb7a98e4fa6159677e7569d0
Author: Fabian Emmes <fabian.emmes@rwth-aachen.de>
Date:   Fri Jan 2 16:40:14 2009 +0100
    cvsserver: change generation of CVS author names
    
    CVS username is generated from local part email address.
    We take the whole local part but restrict the character set to the
    Portable Filename Character Set, which is used for Unix login names
    according to Single Unix Specification v3.
    
    This will obviously report different usernames from existing repositories
    for commits with the local part of the author e-mail address that contains
    characters outside the PFCS.  Hopefully this won't break an old CVS
    checkout from an earlier version of git-cvsserver, because the names are
    always shown afresh to the CVS clients and not kept on the client side.
    
    Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
    Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano· Jan 4, 2009, 10:01 UTC · re: Fabian Emmes · lore

Re: [PATCH] cvsserver: add option to configure commit message

Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
Show 5 quoted lines
> cvsserver annotates each commit message by "via git-CVS emulator". This is
> made configurable via gitcvs.commitmsgannotation.
>
> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>

I do not see the development history behind this and am somewhat puzzled by these two S-o-b lines. Is it "Fabian developed it, showed it to Lars who cleaned it up and/or enhanced it and here is the result"? Or is it "Lars developed it, circulated it in his closer circle, Fabian found it useful and worthy for inclusion and sending it to the mailing list"?

Whichever it is, I just will take it as "This is co-developed and between the authors Fabian is the primary author" and apply.

Thanks.
Lars Noschinski· Jan 4, 2009, 11:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] cvsserver: add option to configure commit message

* Junio C Hamano <gitster@pobox.com> [09-01-04 12:13]:
Show 13 quoted lines
>Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
>
>> cvsserver annotates each commit message by "via git-CVS emulator". This is
>> made configurable via gitcvs.commitmsgannotation.
>>
>> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
>> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
>
>I do not see the development history behind this and am somewhat puzzled
>by these two S-o-b lines.  Is it "Fabian developed it, showed it to Lars
>who cleaned it up and/or enhanced it and here is the result"?  Or is it
>"Lars developed it, circulated it in his closer circle, Fabian found it
>useful and worthy for inclusion and sending it to the mailing list"?

It is "Fabian and Lars developed it and Fabian is the one who mailed it for inclusion". We could just leave off the second S-o-b line, if this is less irritating?

>Whichever it is, I just will take it as "This is co-developed and between
>the authors Fabian is the primary author" and apply.
Fine with me.
  - Lars.
Junio C Hamano· Jan 4, 2009, 19:20 UTC · re: Lars Noschinski · lore

Re: [PATCH] cvsserver: add option to configure commit message

Lars Noschinski <lars@public.noschinski.de> writes:
> It is "Fabian and Lars developed it and Fabian is the one who mailed it
> for inclusion". We could just leave off the second S-o-b line, if this
> is less irritating?

Oh, no, no. It is not an irritation at all. I found it unusual and that's all.

Thanks.

← back to recent threads