Re: [PATCH] cvsimport: strip all inappropriate tag strings

2012-09-06 Thread Junio C Hamano
Ken Dreyer ktdre...@ktdreyer.com writes:

 Thanks Andreas for catching that ref.c in the comments ought to be
 refs.c. I've corrected that in this latest version of the patch.

Yeah, thanks, all.  Will queue.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] cvsimport: strip all inappropriate tag strings

2012-09-05 Thread Alex Vandiver
On Tue, 2012-09-04 at 22:26 -0600, Ken Dreyer wrote:
 When importing CVS tags, strip all the inappropriate strings from the
 tag names as we translate them to git tag names.

 [snip]
 diff --git a/git-cvsimport.perl b/git-cvsimport.perl
 index 8d41610..0dc598d 100755
 --- a/git-cvsimport.perl
 +++ b/git-cvsimport.perl
 @@ -889,7 +889,25 @@ sub commit {
   $xtag =~ s/\s+\*\*.*$//; # Remove stuff like ** INVALID ** and 
 ** FUNKY **
   $xtag =~ tr/_/\./ if ( $opt_u );
   $xtag =~ s/[\/]/$opt_s/g;
 - $xtag =~ s/\[//g;
 +
 + # See ref.c for these rules.
 + # Tag cannot end with a '/' - this is already handled above.
 + # Tag cannot contain bad chars. See bad_ref_char in ref.c.
 + $xtag =~ s/[ ~\^:\\\*\?\[]//g;
 + # Tag cannot contain '..'.
 + $xtag =~ s/\.\.//g;
 + # Tag cannot contain '@{'.
 + $xtag =~ s/\@{//g;
 + # Tag cannot end with '.lock'.
 + $xtag =~ s/(?:\.lock)+$//;
 + # Tag cannot begin or end with '.'.
 + $xtag =~ s/^\.+//;
 + $xtag =~ s/\.+$//;
 + # Tag cannot consist of a single '.' - already handled above.
 + # Tag cannot be empty.
 + if ($xtag eq '') {
 + return;
 + }

Unfortunately, this isn't quite sufficient.  Consider the case of a tag
named foo.lock.  The .lock rule doesn't match, because it's not at the
end of the string -- but after s/\.+$// runs, it _is_ at the end, and
hence invalid.  A similar problem exists with a tag named a.@{.b,
given the ordering of @{ and .. removal.

Something like the following would suffice:

1 while $xtag =~ s/
   (?: \.\.# Tag cannot contain '..'.
   |   \@{ # Tag cannot contain '@{'.
   |   \.lock $# Tag cannot end with '.lock'.
   | ^ \.  # Tag cannot begin...
   |   \. $# ...or end with '.'
   )//xg;

 - Alex

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] cvsimport: strip all inappropriate tag strings

2012-09-05 Thread Junio C Hamano
Ken Dreyer ktdre...@ktdreyer.com writes:

 Certain characters such as ? can be present in a CVS tag name, but
 git does not allow these characters in tags. If git-cvsimport
 encounters a CVS tag that git cannot handle, cvsimport will error and
 refuse to continue the import beyond that point.

 When importing CVS tags, strip all the inappropriate strings from the
 tag names as we translate them to git tag names.

 Signed-off-by: Ken Dreyer ktdre...@ktdreyer.com
 ---

 Thanks Junio and Alex for your review and comments. I've implemented
 both of your suggestions in this patch.

Thanks.

Do we want to give a warning instead of silently dropping a tag on
the floor, or is the output verbose enough that such a warning will
be drowned in the noise?

  git-cvsimport.perl | 18 +-
  1 file changed, 17 insertions(+), 1 deletion(-)

 diff --git a/git-cvsimport.perl b/git-cvsimport.perl
 index 8d41610..dda8a6d 100755
 --- a/git-cvsimport.perl
 +++ b/git-cvsimport.perl
 @@ -889,7 +889,23 @@ sub commit {
   $xtag =~ s/\s+\*\*.*$//; # Remove stuff like ** INVALID ** and 
 ** FUNKY **
   $xtag =~ tr/_/\./ if ( $opt_u );
   $xtag =~ s/[\/]/$opt_s/g;
 - $xtag =~ s/\[//g;
 +
 + # See ref.c for these rules.
 + # Tag cannot contain bad chars. See bad_ref_char in ref.c.
 + $xtag =~ s/[ ~\^:\\\*\?\[]//g;
 + # Other bad strings for tags:
 + 1 while $xtag =~ s/
 + (?: \.\.# Tag cannot contain '..'.
 + |   \@{ # Tag cannot contain '@{'.
 + | ^ -   # Tag cannot begin with '-'.
 + |   \.lock $# Tag cannot end with '.lock'.
 + | ^ \.  # Tag cannot begin...
 + |   \. $# ...or end with '.'
 + )//xg;
 + # Tag cannot be empty.
 + if ($xtag eq '') {

That is, adding something like:

print STDERR warning: ignoring tag '$tag' with invalid tagname;

here.

 + return;
 + }
  
   system('git' , 'tag', '-f', $xtag, $cid) == 0
   or die Cannot create tag $xtag: $!\n;

It also may be worthwhile to show the original tagname ($tag)
somewhere in this message to help diagnosis.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html