[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2009-05-31 20:28:05
Message-ID: 7vk53xqap6.fsf () alter ! siamese ! dyndns ! org
[Download RAW message or body]

Alex Bennee <kernel-hacker@bennee.com> writes:

> My CVS repo is converted by parsecvs and although I don't often mess
> around in the headers I did hit a problem when exporting some commits
> that had removed some lines which would have had an expanded
> $Revision$ in a code string.
>
> Rather than make it default behaviour I've added a quick filter to
> cvsexportcommit so I don't have to go back to patching and committing
> those commits by ahdn :-)
>
>
> From a1a9477d6e332617526aaab488602552b77832d9 Mon Sep 17 00:00:00 2001

Thanks.

You would want to put all the commentary up to here below the "---" line,
if you want to give an introductory text that is not part of your proposed
commit log message.

> From: Alex Bennee <alex@bennee.com>
> Date: Thu, 28 May 2009 14:31:51 +0100
> Subject: [PATCH] Add -k option to cvsexportcommit to squash CVS keywords

You generally do not want these three lines, unless the "From: " your
e-mail shows is different from the name and e-mail of the real author of
the patch.

I presume you would want alex-at-bennee-dot-com in the commit, so in this
case it might be Ok, but then it may be more straightforward to use that
name on the "From: " line of your e-mail to begin with, but on the other
hand, you have a commit as kernel-dash-hacker-at-the-same-domain in our
history already.  If you prefer the latter, you do not need any of the
above three lines (the subject is the same as your e-mail "Subject: "
anyway).

> Depending on how your CVS->GIT conversion went you will have some
> unexpanded CVS keywords in your
> GIT repo. If any of your git commits touch these lines then the patch
> application will fail. This
> patch addresses that by filtering files before applying the patch
> ---

The message is wrapped in a funny way, and it lacks sign-off.

Isn't it "expanded" (not "unexpanded") keyword the problem you are trying
to address?  "By filtering files" in what way?  I can guess "filtering them
back to unexpanded form", but please do not make me guess.

> diff --git a/Documentation/git-cvsexportcommit.txt
> b/Documentation/git-cvsexportcommit.txt
> index 2da8588..b328dd4 100644
> --- a/Documentation/git-cvsexportcommit.txt
> +++ b/Documentation/git-cvsexportcommit.txt
> @@ -63,6 +63,9 @@ OPTIONS
>  -u::
>  	Update affected files from CVS repository before attempting export.
>
> +-k::
> +	Filter CVS keywords (like $Revision$) before applying patch.
> +	

"Filter in what way" applies here as well.

Is it really sufficient to unmunge "$Revision$? and "$Id"?  What about
"$Date$", for example?

> @@ -266,9 +266,32 @@ foreach my $f (@files) {
>  	$dirty = 1;
>  	warn "File $f not up to date but has status '$cvsstat{$f}' in your
> CVS checkout!\n";
>      }
> +
> +    # Depending on how your GIT tree got imported some of the CVS
> +    # expansion keywords would have been squashed. This will break
> +    # application of the patch if you touched any lines that had them.

I am not quite getting this comment.  "Squashed" sounds like "$Revision$"
without expansion instead of "$Revision: 1.4 $"; I thought the issue you
are addressing is that the automated change that comes from the CVS side
to the expanded keyword gets in the way, i.e. if these always were
"squashed", then you would not have to fight with spurious conflicts.

> +    if ($opt_k)
> +    {

This open brace come on the same line as "if", like "if (...) {" to match
the style of the surrounding code.

> +	my $orig_file ="$f.orig";
> +	rename $f, $orig_file;
> +	open(FILTER_IN, "<$orig_file") or die "Cannot open $orig_file\n";
> +	open(FILTER_OUT, ">$f") or die "Cannot open $f\n";
> +	while (<FILTER_IN>)
> +	{
> +	    my $line = $_;
> +	    $line =~ s#\$Revision:[ \.\d]+ \$#\$Revision\$#;
> +	    $line =~ s#\$Id: [^\$]+\$#\$Id\$#;

When there is no '/' in substitution or pattern, it is _far_ easier to
read if you used the standard s/foo/bar/, not custom s#foo#bar#.

Can "$Revision:" immediately be followed by a digit while "$Id:" must
always be followed by a whitespace?  I doubt it.

Why isn't this something like:

	$line =~ s/\$(Revision|Id|Date|....):[^\$]+\$/\$\1\$/g;

or even (not bothering to enumerate the possible set of keywords):

	$line =~ s/\$([A-Z][a-z]+):[^\$]+\$/\$\1\$/g;

> +	    print FILTER_OUT $line;
> +	}
> +	close FILTER_IN;
> +	close FILTER_OUT;
> +    }
>  }
> +
>  if ($dirty) {
> -    if ($opt_f) {	warn "The tree is not clean -- forced merge\n";
> +    if ($opt_f) {
> +	warn "The tree is not clean -- forced merge\n";

This may be a good change, but is distracting, and does not have anything
to do with what you are doing with this patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[prev in list] [next in list] [prev in thread] [next in thread]