[PATCH] cvsserver: add option to configure commit message

[PATCH] cvsserver: add option to configure commit message

[Fabian Emmes]

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(-)

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

[PATCH] cvsserver: change generation of CVS author names

[Fabian Emmes]

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(-)

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

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

[Junio C Hamano]

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).

"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?

>  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;
>  }

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

[Lars Noschinski]

* Junio C Hamano <gitster@pobox.com> [09-01-03 23:36]:
>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.

>"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.

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

[Junio C Hamano]

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>

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

[Junio C Hamano]

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"?

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.

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

[Lars Noschinski]

* Junio C Hamano <gitster@pobox.com> [09-01-04 12:13]:
>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.

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

[Junio C Hamano]

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.