{"thread":{"id":"19556","subject":"[PATCH] Add -k option to cvsexportcommit to squash CVS keywords","startedAt":"2009-05-28T13:36:34Z","lastAt":"2009-06-01T06:57:41Z","messageCount":3,"participants":["Alex Bennee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"114922","messageId":"b2cdc9f30905280636p404525e2u6f3eb21700b6da66@mail.gmail.com","threadId":"19556","inReplyTo":null,"subject":"[PATCH] Add -k option to cvsexportcommit to squash CVS keywords","fromName":"Alex Bennee","fromEmail":"kernel-hacker@bennee.com","sentAt":"2009-05-28T13:36:34Z","receivedAt":"2009-05-28T13:36:34Z","isPatch":true,"sender":{"key":"kernel-hacker@bennee.com","avatar":null},"body":"My CVS repo is converted by parsecvs and although I don't often mess\naround in the headers I did hit a problem when exporting some commits\nthat had removed some lines which would have had an expanded\n$Revision$ in a code string.\n\nRather than make it default behaviour I've added a quick filter to\ncvsexportcommit so I don't have to go back to patching and committing\nthose commits by ahdn :-)\n\n\n>From a1a9477d6e332617526aaab488602552b77832d9 Mon Sep 17 00:00:00 2001\nFrom: Alex Bennee \nDate: Thu, 28 May 2009 14:31:51 +0100\nSubject: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords\n\nDepending on how your CVS->GIT conversion went you will have some\nunexpanded CVS keywords in your\nGIT repo. If any of your git commits touch these lines then the patch\napplication will fail. This\npatch addresses that by filtering files before applying the patch\n---\n Documentation/git-cvsexportcommit.txt | 3 +++\n git-cvsexportcommit.perl | 31 +++++++++++++++++++++++++++----\n 2 files changed, 30 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-cvsexportcommit.txt\nb/Documentation/git-cvsexportcommit.txt\nindex 2da8588..b328dd4 100644\n--- a/Documentation/git-cvsexportcommit.txt\n+++ b/Documentation/git-cvsexportcommit.txt\n@@ -63,6 +63,9 @@ OPTIONS\n -u::\n \tUpdate affected files from CVS repository before attempting export.\n\n+-k::\n+\tFilter CVS keywords (like $Revision$) before applying patch.\n+\t\n -w::\n \tSpecify the location of the CVS checkout to use for the export. This\n \toption does not require GIT_DIR to be set before execution if the\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 6d9f0ef..dd41b0c 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -8,9 +8,9 @@ use File::Basename qw(basename dirname);\n use File::Spec;\n use Git;\n\n-our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m,\n$opt_d, $opt_u, $opt_w, $opt_W);\n+our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m,\n$opt_d, $opt_u, $opt_w, $opt_W, $opt_k);\n\n-getopts('uhPpvcfam:d:w:W');\n+getopts('uhPpvcfkam:d:w:W');\n\n $opt_h && usage();\n\n@@ -266,9 +266,32 @@ foreach my $f (@files) {\n \t$dirty = 1;\n \twarn \"File $f not up to date but has status '$cvsstat{$f}' in your\nCVS checkout!\\n\";\n }\n+\n+ # Depending on how your GIT tree got imported some of the CVS\n+ # expansion keywords would have been squashed. This will break\n+ # application of the patch if you touched any lines that had them.\n+ if ($opt_k)\n+ {\n+\tmy $orig_file =\"$f.orig\";\n+\trename $f, $orig_file;\n+\topen(FILTER_IN, \"<$orig_file\") or die \"Cannot open $orig_file\\n\";\n+\topen(FILTER_OUT, \">$f\") or die \"Cannot open $f\\n\";\n+\twhile ()\n+\t{\n+\t my $line = $_;\n+\t $line =~ s#\\$Revision:[ \\.\\d]+ \\$#\\$Revision\\$#;\n+\t $line =~ s#\\$Id: [^\\$]+\\$#\\$Id\\$#;\n+\n+\t print FILTER_OUT $line;\n+\t}\n+\tclose FILTER_IN;\n+\tclose FILTER_OUT;\n+ }\n }\n+\n if ($dirty) {\n- if ($opt_f) {\twarn \"The tree is not clean -- forced merge\\n\";\n+ if ($opt_f) {\n+\twarn \"The tree is not clean -- forced merge\\n\";\n \t$dirty = 0;\n } else {\n \tdie \"Exiting: your CVS tree is not clean for this merge.\";\n@@ -370,7 +393,7 @@ sleep(1);\n\n sub usage {\n \tprint STDERR < writes:\n\n> My CVS repo is converted by parsecvs and although I don't often mess\n> around in the headers I did hit a problem when exporting some commits\n> that had removed some lines which would have had an expanded\n> $Revision$ in a code string.\n>\n> Rather than make it default behaviour I've added a quick filter to\n> cvsexportcommit so I don't have to go back to patching and committing\n> those commits by ahdn :-)\n>\n>\n> From a1a9477d6e332617526aaab488602552b77832d9 Mon Sep 17 00:00:00 2001\n\nThanks.\n\nYou would want to put all the commentary up to here below the \"---\" line,\nif you want to give an introductory text that is not part of your proposed\ncommit log message.\n\n> From: Alex Bennee \n> Date: Thu, 28 May 2009 14:31:51 +0100\n> Subject: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords\n\nYou generally do not want these three lines, unless the \"From: \" your\ne-mail shows is different from the name and e-mail of the real author of\nthe patch.\n\nI presume you would want alex-at-bennee-dot-com in the commit, so in this\ncase it might be Ok, but then it may be more straightforward to use that\nname on the \"From: \" line of your e-mail to begin with, but on the other\nhand, you have a commit as kernel-dash-hacker-at-the-same-domain in our\nhistory already. If you prefer the latter, you do not need any of the\nabove three lines (the subject is the same as your e-mail \"Subject: \"\nanyway).\n\n> Depending on how your CVS->GIT conversion went you will have some\n> unexpanded CVS keywords in your\n> GIT repo. If any of your git commits touch these lines then the patch\n> application will fail. This\n> patch addresses that by filtering files before applying the patch\n> ---\n\nThe message is wrapped in a funny way, and it lacks sign-off.\n\nIsn't it \"expanded\" (not \"unexpanded\") keyword the problem you are trying\nto address? \"By filtering files\" in what way? I can guess \"filtering them\nback to unexpanded form\", but please do not make me guess.\n\n> diff --git a/Documentation/git-cvsexportcommit.txt\n> b/Documentation/git-cvsexportcommit.txt\n> index 2da8588..b328dd4 100644\n> --- a/Documentation/git-cvsexportcommit.txt\n> +++ b/Documentation/git-cvsexportcommit.txt\n> @@ -63,6 +63,9 @@ OPTIONS\n> -u::\n> \tUpdate affected files from CVS repository before attempting export.\n>\n> +-k::\n> +\tFilter CVS keywords (like $Revision$) before applying patch.\n> +\t\n\n\"Filter in what way\" applies here as well.\n\nIs it really sufficient to unmunge \"$Revision$? and \"$Id\"? What about\n\"$Date$\", for example?\n\n> @@ -266,9 +266,32 @@ foreach my $f (@files) {\n> \t$dirty = 1;\n> \twarn \"File $f not up to date but has status '$cvsstat{$f}' in your\n> CVS checkout!\\n\";\n> }\n> +\n> + # Depending on how your GIT tree got imported some of the CVS\n> + # expansion keywords would have been squashed. This will break\n> + # application of the patch if you touched any lines that had them.\n\nI am not quite getting this comment. \"Squashed\" sounds like \"$Revision$\"\nwithout expansion instead of \"$Revision: 1.4 $\"; I thought the issue you\nare addressing is that the automated change that comes from the CVS side\nto the expanded keyword gets in the way, i.e. if these always were\n\"squashed\", then you would not have to fight with spurious conflicts.\n\n> + if ($opt_k)\n> + {\n\nThis open brace come on the same line as \"if\", like \"if (...) {\" to match\nthe style of the surrounding code.\n\n> +\tmy $orig_file =\"$f.orig\";\n> +\trename $f, $orig_file;\n> +\topen(FILTER_IN, \"<$orig_file\") or die \"Cannot open $orig_file\\n\";\n> +\topen(FILTER_OUT, \">$f\") or die \"Cannot open $f\\n\";\n> +\twhile ()\n> +\t{\n> +\t my $line = $_;\n> +\t $line =~ s#\\$Revision:[ \\.\\d]+ \\$#\\$Revision\\$#;\n> +\t $line =~ s#\\$Id: [^\\$]+\\$#\\$Id\\$#;\n\nWhen there is no '/' in substitution or pattern, it is _far_ easier to\nread if you used the standard s/foo/bar/, not custom s#foo#bar#.\n\nCan \"$Revision:\" immediately be followed by a digit while \"$Id:\" must\nalways be followed by a whitespace? I doubt it.\n\nWhy isn't this something like:\n\n\t$line =~ s/\\$(Revision|Id|Date|....):[^\\$]+\\$/\\$\\1\\$/g;\n\nor even (not bothering to enumerate the possible set of keywords):\n\n\t$line =~ s/\\$([A-Z][a-z]+):[^\\$]+\\$/\\$\\1\\$/g;\n\n> +\t print FILTER_OUT $line;\n> +\t}\n> +\tclose FILTER_IN;\n> +\tclose FILTER_OUT;\n> + }\n> }\n> +\n> if ($dirty) {\n> - if ($opt_f) {\twarn \"The tree is not clean -- forced merge\\n\";\n> + if ($opt_f) {\n> +\twarn \"The tree is not clean -- forced merge\\n\";\n\nThis may be a good change, but is distracting, and does not have anything\nto do with what you are doing with this patch.\n"},{"id":"115180","messageId":"b2cdc9f30905312357k3b2e876ald60a212d11213a8c@mail.gmail.com","threadId":"19556","inReplyTo":"7vk53xqap6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords","fromName":"Alex Bennee","fromEmail":"kernel-hacker@bennee.com","sentAt":"2009-06-01T06:57:41Z","receivedAt":"2009-06-01T06:57:41Z","isPatch":true,"sender":{"key":"kernel-hacker@bennee.com","avatar":null},"body":"2009/5/31 Junio C Hamano :\n> Alex Bennee writes:\n>\n>> \n>>\n>>\n>> From a1a9477d6e332617526aaab488602552b77832d9 Mon Sep 17 00:00:00 2001\n>\n> Thanks.\n>\n> You would want to put all the commentary up to here below the \"---\" line,\n> if you want to give an introductory text that is not part of your proposed\n> commit log message.\n\nI'm not sure I quite follow. The email is an email with description\nabove followed by the output of git-format-patch -1. Or should the\nemail commentary be part of the commit commentary? Should I have just\nfired the commit at the mailing list direct from the command line?\n\n>\n>> From: Alex Bennee \n>> Date: Thu, 28 May 2009 14:31:51 +0100\n>> Subject: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords\n>\n> You generally do not want these three lines, unless the \"From: \" your\n> e-mail shows is different from the name and e-mail of the real author of\n> the patch.\n>\n> I presume you would want alex-at-bennee-dot-com in the commit, so in this\n> case it might be Ok, but then it may be more straightforward to use that\n> name on the \"From: \" line of your e-mail to begin with, but on the other\n> hand, you have a commit as kernel-dash-hacker-at-the-same-domain in our\n> history already.  If you prefer the latter, you do not need any of the\n> above three lines (the subject is the same as your e-mail \"Subject: \"\n> anyway).\n\nHmmm yes. My actual email is as referenced, kernel-hacker is the alias\nI use for my mailing list activity. If people have questions they wish\nto email me directly about I would assume the master email makes the\nbest option for the commit message?\n\n>\n>> Depending on how your CVS->GIT conversion went you will have some\n>> unexpanded CVS keywords in your\n>> GIT repo. If any of your git commits touch these lines then the patch\n>> application will fail. This\n>> patch addresses that by filtering files before applying the patch\n>> ---\n>\n> The message is wrapped in a funny way, and it lacks sign-off.\n\nArrgh. It looks like the Google mail client has futzed with it. Trying\nto get inline patches not munged by mail clients seems to be an\nexercise in frustration. Usually I would create an attachment which\nwould then stay unmolested, however inline is the preferred posting\nstyle for the list.\n\nI'll resend with a sign-off.\n\n>\n> Isn't it \"expanded\" (not \"unexpanded\") keyword the problem you are trying\n> to address?  \"By filtering files\" in what way?  I can guess \"filtering them\n> back to unexpanded form\", but please do not make me guess.\n\nThe files in the CVS tree will have expanded keywords, in the GIT tree\nthey will be in the unexpanded state. This patch will return the files\nin the working CVS tree to the unexpanded state so the patch from the\nGIT tree applies cleanly.\n\nI'll try and re-word to make it clearer.\n\n>\n>> diff --git a/Documentation/git-cvsexportcommit.txt\n>> b/Documentation/git-cvsexportcommit.txt\n>> index 2da8588..b328dd4 100644\n>> --- a/Documentation/git-cvsexportcommit.txt\n>> +++ b/Documentation/git-cvsexportcommit.txt\n>> @@ -63,6 +63,9 @@ OPTIONS\n>>  -u::\n>>       Update affected files from CVS repository before attempting export.\n>>\n>> +-k::\n>> +     Filter CVS keywords (like $Revision$) before applying patch.\n>> +\n>\n> \"Filter in what way\" applies here as well.\n>\n> Is it really sufficient to unmunge \"$Revision$? and \"$Id\"?  What about\n> \"$Date$\", for example?\n\nYou're right. Lazily I had only hit the previous cases in the actual\nproblem files I had. I'll have a dig in the manual and add the rest.\n\n>\n>> @@ -266,9 +266,32 @@ foreach my $f (@files) {\n>>       $dirty = 1;\n>>       warn \"File $f not up to date but has status '$cvsstat{$f}' in your\n>> CVS checkout!\\n\";\n>>      }\n>> +\n>> +    # Depending on how your GIT tree got imported some of the CVS\n>> +    # expansion keywords would have been squashed. This will break\n>> +    # application of the patch if you touched any lines that had them.\n>\n> I am not quite getting this comment.  \"Squashed\" sounds like \"$Revision$\"\n> without expansion instead of \"$Revision: 1.4 $\"; I thought the issue you\n> are addressing is that the automated change that comes from the CVS side\n> to the expanded keyword gets in the way, i.e. if these always were\n> \"squashed\", then you would not have to fight with spurious conflicts.\n>\n>> +    if ($opt_k)\n>> +    {\n>\n> This open brace come on the same line as \"if\", like \"if (...) {\" to match\n> the style of the surrounding code.\n>\n>> +     my $orig_file =\"$f.orig\";\n>> +     rename $f, $orig_file;\n>> +     open(FILTER_IN, \"<$orig_file\") or die \"Cannot open $orig_file\\n\";\n>> +     open(FILTER_OUT, \">$f\") or die \"Cannot open $f\\n\";\n>> +     while ()\n>> +     {\n>> +         my $line = $_;\n>> +         $line =~ s#\\$Revision:[ \\.\\d]+ \\$#\\$Revision\\$#;\n>> +         $line =~ s#\\$Id: [^\\$]+\\$#\\$Id\\$#;\n>\n> When there is no '/' in substitution or pattern, it is _far_ easier to\n> read if you used the standard s/foo/bar/, not custom s#foo#bar#.\n\nI'd used # as there where \\'s to escape some of the meta-characters\nand I find large numbers of \\/'s hard to follow.\n\n>\n> Can \"$Revision:\" immediately be followed by a digit while \"$Id:\" must\n> always be followed by a whitespace?  I doubt it.\n>\n> Why isn't this something like:\n>\n>        $line =~ s/\\$(Revision|Id|Date|....):[^\\$]+\\$/\\$\\1\\$/g;\n>\n> or even (not bothering to enumerate the possible set of keywords):\n>\n>        $line =~ s/\\$([A-Z][a-z]+):[^\\$]+\\$/\\$\\1\\$/g;\n\nI bow to your superior regex formulation ;-)\n\nI'll resend in a bit.\n\n-- \nAlex, homepage: http://www.bennee.com/~alex/\nCV: http://www.bennee.com/~alex/cv.php\n"}]}