{"thread":{"id":"15753","subject":"[PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit \"status\" each file in turn)","startedAt":"2008-10-02T11:07:41Z","lastAt":"2008-10-02T17:04:38Z","messageCount":4,"participants":["Nick Woolley","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92140","messageId":"48E4AB7D.5030705@yahoo.co.uk","threadId":"15753","inReplyTo":null,"subject":"[PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit \"status\" each file in turn)","fromName":"Nick Woolley","fromEmail":"nickwoolley@yahoo.co.uk","sentAt":"2008-10-02T11:07:41Z","receivedAt":"2008-10-02T11:07:41Z","isPatch":true,"sender":{"key":"nick@noodlefactory.co.uk","avatar":null},"body":"Hi,\n\nI've encountered a problem with the Ubuntu Hardy version of \ngit-cvsexportcommit (part of git-cvs 1:1.5.4.3-1ubuntu2). This seems to \nbe the same problem describedin August last year in the thread on this \nlist I referenced (Make git-cvsexportcommit \"status\" each file in turn).\n\nThe problem is that cvsexportcommit rejects a patch erroneously when CVS \ndoes not report file statuses in the expected order.  It confuses one \nfile's status for another and says (at least in my case):\n\n> Checking if patch will apply\n> cvs server: nothing known about XXXXXXXXX\n> cvs server: nothing known about XXXXXXXXX\n> File XXXXXXXXX is already known in your CVS checkout   -- perhaps it\\\n >  has been added by another user. Or this may indicate that it exists\\\n >  on a different branch. If this is the case, use -f to force the\\\n >  merge.\n> Status was: Up-to-date\n> File XXXXXXXXX not up to date but has status   'Unknown' in your\\\n >  CVS checkout!\n> Exiting: your CVS tree is not clean for this merge. at /usr/bin/git\\\n >  -cvsexportcommit line 235.\n\nI searched this list - including the release announcements - and the web \nand it did not seem that anyone had applied this patch or otherwise \naddressed it.\n\nI then found the git release repository and it *does* seem to have been \naddressed in 1.6.0.  However, and correct me if I'm mistaken, but the \nimplementation seems to assume that the committed files' basenames are \nunique?  This can't be guaranteed in general, can it?\n\nAnyway, by then I'd already had a go at fixing it myself, and I had an \nalternative approach, partly based on the suggestions from Robin \nRosenberg in the referenced thread. It doesn't assume basenames are \nunique, and therefore I think should be more robust.\n\nSo, attached is a patch against v1.5.4.  It works for me, but has only \nbeen tested with my particular circumstance, so there could be \nassumptions I made which aren't universally true.  I did try using the \ntest suite in git 1.6.0 but it didn't work for reasons I don't want to \nspend too much more time investigating.  I think the patch as it is \nshould be easy to follow and integrate with 1.6.0 for someone familiar \nwith the codebase.\n\nSome notes about the patch.\n\n  - It parses the output of CVS status / update, getting the\n    file status much as before\n\n  - save_pipe_capture() is modified to capture STDERR as well as STDOUT\n\n  - The STDERR message 'nothing known about <path>' is used\n    preferentially to get a file's path.\n\n  - Otherwise, uses the 'Repository revision' field, extracting the\n    relevant part by removing the repository root constructed from\n    meta-data in the CVS/Root and CVS/Repository files.\n\nI hope this helps.  Comments welcome.\n\nThanks,\n\nNick\n\n\n\n--- /usr/bin/git-cvsexportcommit.orig\t2008-10-02 01:16:23.000000000 +0100\n+++ /home/nick/bin/git-cvsexportcommit\t2008-10-02 01:17:13.000000000 +0100\n@@ -7,6 +7,18 @@\n use Data::Dumper;\n use File::Basename qw(basename dirname);\n \n+# read in the first line of a file\n+sub first_line {\n+    my ($file) = @_;\n+    my $line = '';\n+   \n+    return unless open my $fh, \"<$file\";\n+    chomp ($line = <$fh>);\n+    close $fh;\n+    return $line;\n+}\n+\n+\n our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m, $opt_d, $opt_u, $opt_w);\n \n getopts('uhPpvcfam:d:w:');\n@@ -193,21 +205,63 @@\n     push @canstatusfiles, $f;\n }\n \n+\n+# Get the root path the CVS working directory thinks its respository is at.\n+# (We're in the working dir). Note we assume the repository agrees on this\n+# throughout, which isn't necessarily true, but if not it's just bonkers.\n+my $root_path = first_line(\"CVS/Root\") || '';\n+\n+# likewise the module\n+my $module_path = first_line(\"CVS/Repository\") || '';\n+        \n+# try and strip off all but the path from the root - we assume the path has no colons\n+$root_path =~ s/^.*://;\n+$root_path =~ s{/*$}{/$module_path}; # concatenate the paths\n+\n+\n+# lightly tested - seems ok with unknown files, and CVS repos using non-symbolic modules\n+    \n+# get the files' statuses\n my %cvsstat;\n if (@canstatusfiles) {\n     if ($opt_u) {\n       my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\n     }\n+\n     my @cvsoutput;\n     @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles);\n-    my $matchcount = 0;\n-    foreach my $l (@cvsoutput) {\n-        chomp $l;\n-        if ( $l =~ /^File:/ and  $l =~ /Status: (.*)$/ ) {\n-            $cvsstat{$canstatusfiles[$matchcount]} = $1;\n-            $matchcount++;\n+ \n+    chomp @cvsoutput;\n+    my ($status, $path, $repo_path);\n+    my %expected_paths = map { $_ => 1 } @canstatusfiles;\n+\n+    # Use $_ implcitly here to simplify the regex expressions.\n+    # Note that foreach saves and restores $_ for us\n+    foreach (@cvsoutput) {\n+        # Grab the information when we see it\n+        $status = $1, next if /^File:.*Status:\\s*(.*)/i;\n+        $path = $1,   next if /^cvs server: nothing known about (.*)/i;\n+\n+        next unless m{Repository revision:[\\s\\d.]*(.*)}i;\n+        $repo_path = $1;\n+\n+        # At this point we can try and reconstruct the file path\n+        if (!$path && $repo_path !~ /No entry for/i) { \n+            $repo_path =~ s{Attic/?}{}i;\n+            $repo_path =~ s{,v\\s*$}{}i;\n+            $repo_path =~ s{^$root_path/}{};\n+            \n+            $path = $repo_path;\n         }\n+\n+        warn \"Failed to get a path from the CVS status entry containing:\\n$_\"\n+            unless $path;\n+        warn \"Found a path in the CVS output we didn't ask for: '$path'\" \n+            unless exists $expected_paths{$path};\n+                \n+        $cvsstat{$path} = $status;\n+        $path = $repo_path = $status = undef; \n     }\n }\n \n@@ -331,7 +385,8 @@\n \t@output = (<$child>);\n \tclose $child or die join(' ',@_).\": $! $?\";\n     } else {\n-\texec(@_) or die \"$! $?\"; # exec() can fail the executable can't be found\n+        open STDERR, \">&STDOUT\" or warn \"child can't dup STDERR\";;\n+        exec(@_) or die \"$! $?\"; # exec() can fail the executable can't be found\n     }\n     return wantarray ? @output : join('',@output);\n }\n"},{"id":"92142","messageId":"alpine.DEB.1.00.0810021428170.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15753","inReplyTo":"48E4AB7D.5030705@yahoo.co.uk","subject":"Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit \"status\" each file in turn)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-02T12:32:48Z","receivedAt":"2008-10-02T12:32:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Oct 2008, Nick Woolley wrote:\n\n> I've encountered a problem with the Ubuntu Hardy version of \n> git-cvsexportcommit (part of git-cvs 1:1.5.4.3-1ubuntu2). This seems to \n> be the same problem describedin August last year in the thread on this \n> list I referenced (Make git-cvsexportcommit \"status\" each file in turn).\n> \n> The problem is that cvsexportcommit rejects a patch erroneously when CVS \n> does not report file statuses in the expected order.  It confuses one \n> file's status for another and says (at least in my case):\n> \n> > Checking if patch will apply\n> > cvs server: nothing known about XXXXXXXXX\n> > cvs server: nothing known about XXXXXXXXX\n> > File XXXXXXXXX is already known in your CVS checkout   -- perhaps it\\\n> >  has been added by another user. Or this may indicate that it exists\\\n> >  on a different branch. If this is the case, use -f to force the\\\n> >  merge.\n> > Status was: Up-to-date\n> > File XXXXXXXXX not up to date but has status   'Unknown' in your\\\n> >  CVS checkout!\n> > Exiting: your CVS tree is not clean for this merge. at /usr/bin/git\\\n> >  -cvsexportcommit line 235.\n> \n> I searched this list - including the release announcements - and the web \n> and it did not seem that anyone had applied this patch or otherwise \n> addressed it.\n> \n> I then found the git release repository and it *does* seem to have been \n> addressed in 1.6.0.  However, and correct me if I'm mistaken, but the \n> implementation seems to assume that the committed files' basenames are \n> unique? This can't be guaranteed in general, can it?\n\nPlease research a bit better.  If the basenames are not unique, several \ncvs status calls are used.  See commit\nfef3a7cc5593d3951a5f95c92986fb9982c2cc86.\n\n> Anyway, by then I'd already had a go at fixing it myself, and I had an \n> alternative approach, partly based on the suggestions from Robin \n> Rosenberg in the referenced thread. It doesn't assume basenames are \n> unique, and therefore I think should be more robust.\n> \n> So, attached is a patch against v1.5.4.  It works for me, but has only \n> been tested with my particular circumstance, so there could be \n> assumptions I made which aren't universally true.  I did try using the \n> test suite in git 1.6.0 but it didn't work for reasons I don't want to \n> spend too much more time investigating.  I think the patch as it is \n> should be easy to follow and integrate with 1.6.0 for someone familiar \n> with the codebase.\n> \n> Some notes about the patch.\n> \n>  - It parses the output of CVS status / update, getting the\n>    file status much as before\n> \n>  - save_pipe_capture() is modified to capture STDERR as well as STDOUT\n> \n>  - The STDERR message 'nothing known about <path>' is used\n>    preferentially to get a file's path.\n> \n>  - Otherwise, uses the 'Repository revision' field, extracting the\n>    relevant part by removing the repository root constructed from\n>    meta-data in the CVS/Root and CVS/Repository files.\n> \n> I hope this helps.  Comments welcome.\n\nI can only assume that you have not really hung out on this list for very \nlong; this is no way near the way patches are expected here.  For more \nhelp, refer to Documentation/SubmittingPatches, as has been mentioned in \nthe notes from the maintainer quite a number of times.  Or imitate how \nother people submit their patches.\n\nAlso, given the fact that you actually verified that it was fixed in \n1.6.0, what exactly is your proposed course of action?  Revert the fix in \n1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking a \nrelease 1.5.6.6 with your patch?\n\nPuzzled,\nDscho\n"},{"id":"92178","messageId":"48E4E3F2.4000401@yahoo.co.uk","threadId":"15753","inReplyTo":"alpine.DEB.1.00.0810021428170.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit \"status\" each file in turn)","fromName":"Nick Woolley","fromEmail":"nickwoolley@yahoo.co.uk","sentAt":"2008-10-02T15:08:34Z","receivedAt":"2008-10-02T15:08:34Z","isPatch":true,"sender":{"key":"nick@noodlefactory.co.uk","avatar":null},"body":"Johannes Schindelin wrote:\n> Please research a bit better.  If the basenames are not unique, several \n> cvs status calls are used.  See commit\n> fef3a7cc5593d3951a5f95c92986fb9982c2cc86.\n\nYes, I see. I did spend some time searching for prior art on this issue, \nbut I obviously wasn't looking in the right places.\n\n> I can only assume that you have not really hung out on this list for very \n> long; this is no way near the way patches are expected here.\n\nCorrect.\n\n> Also, given the fact that you actually verified that it was fixed in \n> 1.6.0, what exactly is your proposed course of action?  Revert the fix in \n> 1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking a \n> release 1.5.6.6 with your patch?\n\nNeither. At the moment, I can only offer what I have: a patch for \n1.5.6.5, representing an idea that may contribute something small to \n1.6.0 (using information from CVS on STDERR). It should be simple to \nadapt if it's useful, if not please ignore it.\n\nBy posting, I hoped to learn if it was useful and where to look for the \ninformation I was missing, which you've been helpful enough to point out.\n\nThanks,\n\nN\n"},{"id":"92185","messageId":"alpine.DEB.1.00.0810021745090.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15753","inReplyTo":"48E4E3F2.4000401@yahoo.co.uk","subject":"Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit \"status\" each file in turn)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-02T17:04:38Z","receivedAt":"2008-10-02T17:04:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Oct 2008, Nick Woolley wrote:\n\n> Johannes Schindelin wrote:\n> \n> > Also, given the fact that you actually verified that it was fixed in \n> > 1.6.0, what exactly is your proposed course of action?  Revert the fix \n> > in 1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking \n> > a release 1.5.6.6 with your patch?\n> \n> Neither. At the moment, I can only offer what I have: a patch for \n> 1.5.6.5, representing an idea that may contribute something small to \n> 1.6.0 (using information from CVS on STDERR). It should be simple to \n> adapt if it's useful, if not please ignore it.\n\nSince you did not use the preferred form of a patch, you also do not have \na commit message describing the idea of your patch.  It is a bit hard to \nfind out the intention from reading the source, and I am not motivated \nenough to find out, given that it is fixed with the commit I referred you \nto.\n\nTo save you time: the idea of the commit I referred you to is not exactly \nto use the basename, but the trimmed names.  Now only sets of unique names \n(where it is also checked that a name could not be mistaken for another, \ndeleted file) are passed to cvs status.\n\nBTW next time you search for some fix in Git's source code, you might want \nto use either \"git log -- <file>\" or \"git bisect\" to find the commit in \nquestion.\n\nHth,\nDscho\n"}]}