patchcvsimport: strip question marks from tags

The "?" character can be present in a CVS tag name, but git's bad_ref_char does not allow question marks in git tags. If git-cvsimport encounters a CVS tag with a question mark, it will error and refuse to continue the import beyond that point.

When importing CVS tags, strip "?" characters from the tag names as we translate them to git tag names.

Signed-off-by: Ken Dreyer <ktdreyer@ktdreyer.com>
---
 git-cvsimport.perl | 1 +
 1 file changed, 1 insertion(+)

Ken Dreyer <ktdreyer@ktdreyer.com> writes:

I do not think this is a right and sustainable approach. The next patch would probably be to strip "~" and then another patch that strips "^", and yet another that squashes ".." into one would surely follow.

How about extending the s/\[//g we can see in the context to cover everything that are unacceptable (see refs.c:bad_ref_char()) once and for all? The result needs to be further massaged to avoid component that has two or more dots in a row, a dot at the beginning or at the end (see the comment at the beginning of refs.c, and also refs.c:check_refname_component()).

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 <ktdreyer@ktdreyer.com>
---

Thank you Junio for the review. I've taken your suggestion and amended my patch to eliminate all the bad strings in ref.c.

 git-cvsimport.perl | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Ken Dreyer <ktdreyer@ktdreyer.com> writes:

Thanks, will queue.

I think we also forbid tagnames (or branchnames for that matter) that begin with a dash on the creation side, even though the reading side tries to be lenient (i.e. if for some bad tool already created a file .git/refs/tags/-foobar, we allow "git show tags/-foobar" to show it). The routines in refs.c enforces primarily on the reading codepath. So this part:

may need to become

	# Tag cannot begin with '.' or '-', or end with '.'.
	$xtag =~ s/^[-.]+//;
	$xtag =~ s/\.+$//;

or something.

On Tue, 2012-09-04 at 22:26 -0600, Ken Dreyer wrote:

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

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 <ktdreyer@ktdreyer.com>
---

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

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

Ken Dreyer <ktdreyer@ktdreyer.com> writes:

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?

That is, adding something like:

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

here.

It also may be worthwhile to show the original tagname ($tag) somewhere in this message to help diagnosis.

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.

Provide more debugging information to the user if we've altered the tag and the "git tag" command still fails. Also, warn the user if we end up skipping an (unusable) tag altogether.

Signed-off-by: Ken Dreyer <ktdreyer@ktdreyer.com>
---

Thanks Junio for your suggestion about diagnosis messages. I've implemented your suggestion by adding a warning statement if we skip a tag altogether, and I also added some output if we've translated a tag and the system() call still fails.

 git-cvsimport.perl | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

Ken Dreyer <ktdreyer@ktdreyer.com> writes:

s/ref.c/refs.c/

Andreas.

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.

Provide more debugging information to the user if we've altered the tag and the "git tag" command still fails. Also, warn the user if we end up skipping an (unusable) tag altogether.

Signed-off-by: Ken Dreyer <ktdreyer@ktdreyer.com>
---

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.

 git-cvsimport.perl | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

Ken Dreyer <ktdreyer@ktdreyer.com> writes:

Yeah, thanks, all.  Will queue.