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

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

[Johannes Schindelin]

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(-)

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

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

[Johannes Schindelin]

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(-)

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

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

[Junio C Hamano]

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?

"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

>  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

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

[Johannes Schindelin]

Hi,

On Wed, 14 May 2008, Junio C Hamano wrote:

> 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

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

[Robin Rosenberg]

> 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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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(-)

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

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

[Junio C Hamano]

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?

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

[Johannes Schindelin]

Hi,

On Wed, 14 May 2008, Junio C Hamano wrote:

> 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