threads / patch / 13515

patch, 2 partscvsexportcommit: chomp only removes trailing whitespace

Subject: [PATCH 1/2] cvsexportcommit: chomp only removes trailing whitespace

## tl;dr

9 messages between May 14, 2008 and May 15, 2008. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Johannes Schindelin· May 14, 2008, 14:27 UTC · lore

In commit fef3a7cc(cvsexportcommit: be graceful when "cvs status" reorders the arguments), caution was taken to get the status even for files with leading or trailing whitespace.

However, the author of that commit missed that chomp() removes only trailing whitespace. But the author realized his mistake.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Really my fault.
 git-cvsexportcommit.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to git-cvsexportcommit.perl +1 −0
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index b6036bd..3b20bd1 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -211,6 +211,7 @@ if (@canstatusfiles) {
 
 	$basename = "no file " . $basename if (exists($added{$basename}));
 	chomp($basename);
+	$basename =~ s/^\s+//;
 
 	if (!exists($fullname{$basename})) {
 	  $fullname{$basename} = $name;
-- 
1.5.5.1.375.g1becb
Johannes Schindelin· May 14, 2008, 14:29 UTC · re: Johannes Schindelin · lore

[PATCH 2/2] cvsexportcommit: introduce -W for shared working trees (between Git and CVS)

If you have a CVS checkout, it is easy to import the CVS history by calling "git cvsimport". However, interacting with the CVS repository using "git cvsexportcommit" was cumbersome, since that script assumes separate working directories for Git and CVS.

Now, you can call cvsexportcommit with the -W option. This will automatically discover the GIT_DIR, and it will check out the parent commit before exporting the commit.

The intended workflow is this:

$ CVSROOT=$URL cvs co module $ cd module $ git cvsimport hack, hack, hack, making two commits, cleaning them up using rebase -i. $ git cvsexportcommit -W -c -p -u HEAD^ $ git cvsexportcommit -W -c -p -u HEAD

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	This makes cvsexportcommit slightly more usable for me.  It is
	not (yet) a "git cvs dcommit", because it does not perform the
	test what to commit (and possibly commit more than one patch),
	and it does not rebase either.
 Documentation/git-cvsexportcommit.txt |    7 +++++-
 git-cvsexportcommit.perl              |   35 ++++++++++++++++++++++++++++----
 t/t9200-git-cvsexportcommit.sh        |   17 ++++++++++++++++
 3 files changed, 53 insertions(+), 6 deletions(-)
Show changes to 3 files +53 −6

Documentation/git-cvsexportcommit.txt, git-cvsexportcommit.perl, t/t9200-git-cvsexportcommit.sh

diff --git a/Documentation/git-cvsexportcommit.txt b/Documentation/git-cvsexportcommit.txt
index 9a47b4c..b78c9f3 100644
--- a/Documentation/git-cvsexportcommit.txt
+++ b/Documentation/git-cvsexportcommit.txt
@@ -8,7 +8,7 @@ git-cvsexportcommit - Export a single commit to a CVS checkout
 
 SYNOPSIS
 --------
-'git-cvsexportcommit' [-h] [-u] [-v] [-c] [-P] [-p] [-a] [-d cvsroot] [-w cvsworkdir] [-f] [-m msgprefix] [PARENTCOMMIT] COMMITID
+'git-cvsexportcommit' [-h] [-u] [-v] [-c] [-P] [-p] [-a] [-d cvsroot] [-w cvsworkdir] [-W] [-f] [-m msgprefix] [PARENTCOMMIT] COMMITID
 
 
 DESCRIPTION
@@ -67,6 +67,11 @@ OPTIONS
 	option does not require GIT_DIR to be set before execution if the
 	current directory is within a git repository.
 
+-W::
+	Tell cvsexportcommit that the current working directory is not only
+	a Git checkout, but also the CVS checkout.  Therefore, Git will
+	reset the working directory to the parent commit before proceeding.
+
 -v::
 	Verbose.
 
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 3b20bd1..7105825 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -7,15 +7,15 @@ use Data::Dumper;
 use File::Basename qw(basename dirname);
 use File::Spec;
 
-our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m, $opt_d, $opt_u, $opt_w);
+our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m, $opt_d, $opt_u, $opt_w, $opt_W);
 
-getopts('uhPpvcfam:d:w:');
+getopts('uhPpvcfam:d:w:W');
 
 $opt_h && usage();
 
 die "Need at least one commit identifier!" unless @ARGV;
 
-if ($opt_w) {
+if ($opt_w || $opt_W) {
 	# Remember where GIT_DIR is before changing to CVS checkout
 	unless ($ENV{GIT_DIR}) {
 		# No GIT_DIR set. Figure it out for ourselves
@@ -25,7 +25,9 @@ if ($opt_w) {
 	}
 	# Make sure GIT_DIR is absolute
 	$ENV{GIT_DIR} = File::Spec->rel2abs($ENV{GIT_DIR});
+}
 
+if ($opt_w) {
 	if (! -d $opt_w."/CVS" ) {
 		die "$opt_w is not a CVS checkout";
 	}
@@ -116,6 +118,15 @@ if ($parent) {
     }
 }
 
+my $go_back_to = 0;
+
+if ($opt_W) {
+    $opt_v && print "Resetting to $parent\n";
+    $go_back_to = `git symbolic-ref HEAD 2> /dev/null ||
+	git rev-parse HEAD` || die "Could not determine current branch";
+    system("git checkout -q $parent^0") && die "Could not check out $parent^0";
+}
+
 $opt_v && print "Applying to CVS commit $commit from parent $parent\n";
 
 # grab the commit message
@@ -260,7 +271,11 @@ if ($dirty) {
 }
 
 print "Applying\n";
-`GIT_DIR= git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch";
+if ($opt_W) {
+    system("git checkout -q $commit^0") && die "cannot patch";
+} else {
+    `GIT_DIR= git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch";
+}
 
 print "Patch applied successfully. Adding new files and directories to CVS\n";
 my $dirtypatch = 0;
@@ -313,7 +328,9 @@ if ($dirtypatch) {
     print "using a patch program. After applying the patch and resolving the\n";
     print "problems you may commit using:";
     print "\n    cd \"$opt_w\"" if $opt_w;
-    print "\n    $cmd\n\n";
+    print "\n    $cmd\n";
+    print "\n    git checkout $go_back_to\n" if $go_back_to;
+    print "\n";
     exit(1);
 }
 
@@ -333,6 +350,14 @@ if ($opt_c) {
 # clean up
 unlink(".cvsexportcommit.diff");
 
+if ($opt_W) {
+    system("git checkout $go_back_to") && die "cannot move back to $go_back_to";
+    if (!($go_back_to =~ /^[0-9a-fA-F]{40}$/)) {
+	system("git symbolic-ref HEAD $go_back_to") &&
+	    die "cannot move back to $go_back_to";
+    }
+}
+
 # CVS version 1.11.x and 1.12.x sleeps the wrong way to ensure the timestamp
 # used by CVS and the one set by subsequence file modifications are different.
 # If they are not different CVS will not detect changes.
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 42b144b..b1dc32d 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -297,4 +297,21 @@ test_expect_success 'commit a file with leading spaces in the name' '
 
 '
 
+test_expect_success 'use the same checkout for Git and CVS' '
+
+	(mkdir shared &&
+	 cd shared &&
+	 unset GIT_DIR &&
+	 cvs co . &&
+	 git init &&
+	 git add " space" &&
+	 git commit -m "fake initial commit" &&
+	 echo Hello >> " space" &&
+	 git commit -m "Another change" " space" &&
+	 git cvsexportcommit -W -p -u -c HEAD &&
+	 grep Hello " space" &&
+	 git diff-files)
+
+'
+
 test_done
-- 
1.5.5.1.375.g1becb
Junio C Hamano· May 14, 2008, 17:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] cvsexportcommit: chomp only removes trailing whitespace

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 11 quoted lines
> In commit fef3a7cc(cvsexportcommit: be graceful when "cvs status"
> reorders the arguments), caution was taken to get the status even
> for files with leading or trailing whitespace.
>
> However, the author of that commit missed that chomp() removes only
> trailing whitespace.  But the author realized his mistake.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	Really my fault.
I am not quite sure if I understand what is going on correctly.

Is this about a filename that has leading or trailing whitespace, or lazily not parsing a protocol message but attempting to match with possible whitespaces around the place where a filename should be?

If you are saying that the output from cvs status is so unreliable that we can only strip all whitespaces from both ends and hope for the best (e.g. files " a" (two leading spaces in the name), "a " (two trailing spaces in the name), and "a" (no such funny spaces) cannot be distinguished from cvs status output), then perhaps you would also need to remove as many trailing whitespaces as you can?

"chomp()" chomps only a single line terminator, and only if one exists.
        sub foo {
                my ($a) = @_;
                chomp($a);
                print join(" ", map { sprintf "%02x", ord($_) } split(//, $a)), "\n";
        }
        foo("abc");    # 61 62 63
        foo("def\n");  # 64 65 66
        foo("gh \n");  # 67 68 20
        foo("ij\n\n"); # 69 6a 0a
Show 17 quoted lines
>  git-cvsexportcommit.perl |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
> index b6036bd..3b20bd1 100755
> --- a/git-cvsexportcommit.perl
> +++ b/git-cvsexportcommit.perl
> @@ -211,6 +211,7 @@ if (@canstatusfiles) {
>  
>  	$basename = "no file " . $basename if (exists($added{$basename}));
>  	chomp($basename);
> +	$basename =~ s/^\s+//;
>  
>  	if (!exists($fullname{$basename})) {
>  	  $fullname{$basename} = $name;
> -- 
> 1.5.5.1.375.g1becb
Johannes Schindelin· May 14, 2008, 18:38 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] cvsexportcommit: chomp only removes trailing whitespace

Hi,
On Wed, 14 May 2008, Junio C Hamano wrote:
Show 26 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In commit fef3a7cc(cvsexportcommit: be graceful when "cvs status" 
> > reorders the arguments), caution was taken to get the status even for 
> > files with leading or trailing whitespace.
> >
> > However, the author of that commit missed that chomp() removes only 
> > trailing whitespace.  But the author realized his mistake.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	Really my fault.
> 
> I am not quite sure if I understand what is going on correctly.
> 
> Is this about a filename that has leading or trailing whitespace, or 
> lazily not parsing a protocol message but attempting to match with 
> possible whitespaces around the place where a filename should be?
> 
> If you are saying that the output from cvs status is so unreliable that 
> we can only strip all whitespaces from both ends and hope for the best 
> (e.g. files " a" (two leading spaces in the name), "a " (two trailing 
> spaces in the name), and "a" (no such funny spaces) cannot be 
> distinguished from cvs status output), then perhaps you would also need 
> to remove as many trailing whitespaces as you can?

Yes, that is the idea. The point is: there are at least two different implementations of cvs, and I do not want to rely on a particular one.

To prevent bad things from happening, the status is checked on a set of files which have unique names with regard to the chomp()ed name (well, whatever we do to the name, really).

So yes, this patch needs an update.

Will do so in a couple of hours, Dscho

Robin Rosenberg· May 14, 2008, 20:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] cvsexportcommit: chomp only removes trailing whitespace

> Yes, that is the idea.  The point is: there are at least two different 
> implementations of cvs, and I do not want to rely on a particular one.
Does CVSNT add extra spaces?

A question arises. Can we use cvs update instead? It can be used to retrieve the latest version (as the -u flag to cvsexportcommit does) and it will tell you what status each affected file has. cvs update -n will just give us the status with unambigous file names.

-- robin
Johannes Schindelin· May 14, 2008, 22:15 UTC · re: Robin Rosenberg · lore

Re: [PATCH 1/2] cvsexportcommit: chomp only removes trailing whitespace

Hi,
On Wed, 14 May 2008, Robin Rosenberg wrote:
> > Yes, that is the idea.  The point is: there are at least two different 
> > implementations of cvs, and I do not want to rely on a particular one.
> 
> Does CVSNT add extra spaces?
Probably.  At least a CR I would expect.
> A question arises. Can we use cvs update instead? It can be used to 
> retrieve the latest version (as the -u flag to cvsexportcommit does) and 
> it will tell you what status each affected file has. cvs update -n will 
> just give us the status with unambigous file names.

Will it? The thing is: as far as I can see, cvs is only "porcelain", i.e. the output is meant for users' consumption, not scripts'.

So I wanted to be safe, and strip all leading and trailing whitespace, call "cvs status" (if necessary, in several runs, so that no ambiguous file names can be returned).

Ciao, Dscho

Johannes Schindelin· May 14, 2008, 22:30 UTC · re: Johannes Schindelin · lore

[PATCH 1/2 v2] cvsexportcommit: chomp only removes trailing whitespace

In commit fef3a7cc(cvsexportcommit: be graceful when "cvs status" reorders the arguments), caution was taken to get the status even for files with leading or trailing whitespace.

However, the author of that commit missed that chomp() removes only trailing newlines. With help of the mailing list, the author realized his mistake and provided this patch.

The idea is that we do not want to rely on a certain layout of the output of "cvs status". Therefore we only call it with files that are unambiguous after stripping leading and trailing whitespace.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Wed, 14 May 2008, Johannes Schindelin wrote:
	> To prevent bad things from happening, the status is checked on a 
	> set of files which have unique names with regard to the chomp()ed
	> name (well, whatever we do to the name, really).
	> 
	> So yes, this patch needs an update.
	> 
	> Will do so in a couple of hours,
	And so I did.
 git-cvsexportcommit.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-cvsexportcommit.perl +1 −1
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index b6036bd..52ba7de 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -210,7 +210,7 @@ if (@canstatusfiles) {
 	my $basename = basename($name);
 
 	$basename = "no file " . $basename if (exists($added{$basename}));
-	chomp($basename);
+	$basename =~ s/^\s+(.*?)\s*$/$1/;
 
 	if (!exists($fullname{$basename})) {
 	  $fullname{$basename} = $name;
-- 
1.5.5.1.375.g1becb
Junio C Hamano· May 15, 2008, 02:27 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2 v2] cvsexportcommit: chomp only removes trailing whitespace

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> @@ -210,7 +210,7 @@ if (@canstatusfiles) {
>  	my $basename = basename($name);
>  
>  	$basename = "no file " . $basename if (exists($added{$basename}));
> -	chomp($basename);
> +	$basename =~ s/^\s+(.*?)\s*$/$1/;

Isn't this no-op for a basename that does not begin with a whitespace? Don't you want to still strip trailing whitespaces in such a case?

Johannes Schindelin· May 15, 2008, 03:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2 v2] cvsexportcommit: chomp only removes trailing whitespace

Hi,
On Wed, 14 May 2008, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > @@ -210,7 +210,7 @@ if (@canstatusfiles) {
> >  	my $basename = basename($name);
> >  
> >  	$basename = "no file " . $basename if (exists($added{$basename}));
> > -	chomp($basename);
> > +	$basename =~ s/^\s+(.*?)\s*$/$1/;
> 
> Isn't this no-op for a basename that does not begin with a whitespace?
> Don't you want to still strip trailing whitespaces in such a case?
D'oh.  Time for bed.

Ciao, Dscho

← back to recent threads