{"thread":{"id":"12166","subject":"[PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","startedAt":"2008-02-18T01:31:36Z","lastAt":"2008-02-18T20:29:25Z","messageCount":15,"participants":["Johannes Schindelin","Junio C Hamano","Martin Langhoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"69069","messageId":"alpine.LSU.1.00.0802180127100.30505@racer.site","threadId":"12166","inReplyTo":null,"subject":"[PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T01:31:36Z","receivedAt":"2008-02-18T01:31:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIn my use cases, \"cvs status\" sometimes reordered the passed filenames,\nwhich often led to a misdetection of a dirty state (when it was in\nreality a clean state).\n\nI finally tracked it down to two filenames having the same basename.\n\nSo no longer trust the order of the results blindly, but actually check\nthe file name.\n\nSince \"cvs status\" only returns the basename (and the complete path on the\nserver which is useless for our purposes), run \"cvs status\" several times\nwith lists consisting of files with unique (chomped) basenames.\n\nThis makes cvsexportcommit slightly slower, when the list of changed files\nhas non-unique basenames, but at least it is accurate now.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tSo I finally broke down.  I really need a working cvsexportcommit, \n\teverything else is just too painful.\n\n\tFeel free to criticise/educate me on my Perl style.\n\n git-cvsexportcommit.perl       |   33 +++++++++++++++++++++++++--------\n t/t9200-git-cvsexportcommit.sh |   23 +++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 8 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 2a8ad1e..7fd05c7 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -197,15 +197,32 @@ if (@canstatusfiles) {\n       my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\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+    # \"cvs status\" reorders the parameters, notably when there are multiple\n+    # arguments with the same basename.  So be precise here.\n+    while (@canstatusfiles) {\n+      my @canstatusfiles2 = ();\n+      my %basenames = ();\n+      for (my $i = 0; $i <= $#canstatusfiles; $i++) {\n+        my $name = $canstatusfiles[$i];\n+\tmy $basename = $name;\n+\t$basename =~ s/.*\\///;\n+\t$basename = \"no file \" . $basename if (grep {$_ eq $basename} @afiles);\n+\tchomp($basename);\n+\tif (!defined($basenames{$basename})) {\n+\t  $basenames{$basename} = $name;\n+\t  push (@canstatusfiles2, $name);\n+\t  splice (@canstatusfiles, $i, 1);\n+\t  $i--;\n         }\n+      }\n+      my @cvsoutput;\n+      @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);\n+      foreach my $l (@cvsoutput) {\n+          chomp $l;\n+          if ( $l =~ /^File:\\s+(.*\\S)\\s+Status: (.*)$/ ) {\n+            $cvsstat{$basenames{$1}} = $2 if defined($basenames{$1});\n+          }\n+      }\n     }\n }\n \ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex 49d57a8..483d8fa 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -262,4 +262,27 @@ test_expect_success '-w option should work with relative GIT_DIR' '\n       )\n '\n \n+test_expect_success 'check files before directories' '\n+\n+\techo Notes > release-notes &&\n+\tgit add release-notes &&\n+\tgit commit -m \"Add release notes\" release-notes &&\n+\tid=$(git rev-parse HEAD) &&\n+\tgit cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+\n+\techo new > DS &&\n+\techo new > E/DS &&\n+\techo modified > release-notes &&\n+\tgit add DS E/DS release-notes &&\n+\tgit commit -m \"Add two files with the same basename\" &&\n+\tid=$(git rev-parse HEAD) &&\n+\tgit cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+\tcheck_entries \"$CVSWORK/E\" \"DS/1.1/|newfile5.txt/1.1/\" &&\n+\tcheck_entries \"$CVSWORK\" \"DS/1.1/|release-notes/1.2/\" &&\n+\tdiff -u \"$CVSWORK/DS\" DS &&\n+\tdiff -u \"$CVSWORK/E/DS\" E/DS &&\n+\tdiff -u \"$CVSWORK/release-notes\" release-notes\n+\n+'\n+\n test_done\n-- \n1.5.4.2.217.g468be\n"},{"id":"69077","messageId":"7vbq6fvudp.fsf@gitster.siamese.dyndns.org","threadId":"12166","inReplyTo":"alpine.LSU.1.00.0802180127100.30505@racer.site","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T03:03:14Z","receivedAt":"2008-02-18T03:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I finally tracked it down to two filenames having the same basename.\n\nWonderful.\n\n> \tFeel free to criticise/educate me on my Perl style.\n\nHere it goes ;-)\n\n> +    # \"cvs status\" reorders the parameters, notably when there are multiple\n> +    # arguments with the same basename.  So be precise here.\n> +    while (@canstatusfiles) {\n> +      my @canstatusfiles2 = ();\n> +      my %basenames = ();\n> +      for (my $i = 0; $i <= $#canstatusfiles; $i++) {\n\nThe \"$index <= $#array\" termination condition feels so Perl4-ish.\n\n\tfor (my $i = 0; $i < @canstatusfiles; $i++) {\n\n> +        my $name = $canstatusfiles[$i];\n\n> +\tmy $basename = $name;\n> +\t$basename =~ s/.*\\///;\n\nThe script uses File::Basename upfront so perhaps just simply...\n\n\tmy $basename = basename($name);\n\n> +\t$basename = \"no file \" . $basename if (grep {$_ eq $basename} @afiles);\n\nHuh?  Perl or no Perl that is too ugly a hack...  What special\ntreatment do \"added files\" need?  We would want to make sure\nthat the files are not reported from \"cvs status\"?\n\n> +\tchomp($basename);\n\nHuh?  Perhaps you wanted to chomp at the very beginning of the loop?\n\n> +\tif (!defined($basenames{$basename})) {\n> +\t  $basenames{$basename} = $name;\n> +\t  push (@canstatusfiles2, $name);\n> +\t  splice (@canstatusfiles, $i, 1);\n> +\t  $i--;\n>          }\n> +      }\n\n> +      my @cvsoutput;\n> +      @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);\n> +      foreach my $l (@cvsoutput) {\n> +          chomp $l;\n> +          if ( $l =~ /^File:\\s+(.*\\S)\\s+Status: (.*)$/ ) {\n> +            $cvsstat{$basenames{$1}} = $2 if defined($basenames{$1});\n> +          }\n> +      }\n\nI think \"exists $hash{$index}\" would be easier to read and more\nlogical here and also if () condition above.\n\nWithout understanding what is really going on with the \"added\nfiles\" case, here is how I would write your patch.\n\nSide note.  I personally do not like naming hashes and arrays\nplural, and call a hash of paths and list of files %path and\n@file respectively.  That convention makes it easier to read\nthings like these:\n\n\t$file[4] ;# fourth file, not $files[4]\n\t$path{'hello.c'} ;# path for 'hello.c', not $paths{'hello.c'}\n\n---\n\n git-cvsexportcommit.perl |   43 ++++++++++++++++++++++++++++++++++---------\n 1 files changed, 34 insertions(+), 9 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 2a8ad1e..06e7fda 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -197,15 +197,40 @@ if (@canstatusfiles) {\n       my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\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+\n+    my %added = map { $_ => 1 } @afiles;\n+\n+    while (@canstatusfiles) {\n+\t    my %basename = ();\n+\t    my @status = ();\n+\t    my @leftover = ();\n+\t    for (my $i = 0; $i < @canstatusfiles; $i++) {\n+\t\t    my $name = $canstatusfiles[$i];\n+\t\t    my $basename = basename($name);\n+\t\t    if (exists $basename{$basename}) {\n+\t\t\t    push @leftover, $name;\n+\t\t\t    next;\n+\t\t    }\n+\t\t    if (exists $added{$name}) {\n+\t\t\t    # Hmph...\n+\t\t\t    next;\n+\t\t    }\n+\t\t    $basename{$basename} = $name;\n+\t\t    push @status, $name;\n+\t    }\n+\t    my @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @status);\n+\t    for my $l (@cvsoutput) {\n+\t\t    chomp $l;\n+\t\t    if ($l =~ /^File:\\s+(.*\\S)\\s+Status: (.*)$/) {\n+\t\t\t    my ($n, $s) = ($1, $2);\n+\t\t\t    if (!exists $basename{$n}) {\n+\t\t\t\t    print STDERR \"Huh ($n)?\\n\";\n+\t\t\t    } else {\n+\t\t\t\t    $cvsstat{$basename{$n}} = $s;\n+\t\t\t    }\n+\t\t    }\n+\t    }\n+\t    @canstatusfiles = @leftover;\n     }\n }\n \n"},{"id":"69080","messageId":"7vwsp3uf0u.fsf@gitster.siamese.dyndns.org","threadId":"12166","inReplyTo":"7vbq6fvudp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T03:20:17Z","receivedAt":"2008-02-18T03:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Side note.  I personally do not like naming hashes and arrays\n> plural, and call a hash of paths and list of files %path and\n> @file respectively.  That convention makes it easier to read\n> things like these:\n>\n> \t$file[4] ;# fourth file, not $files[4]\n> \t$path{'hello.c'} ;# path for 'hello.c', not $paths{'hello.c'}\n> ...\n> +    while (@canstatusfiles) {\n> +\t    my %basename = ();\n> +\t    my @status = ();\n> +\t    my @leftover = ();\n> +\t    for (my $i = 0; $i < @canstatusfiles; $i++) {\n> +\t\t    my $name = $canstatusfiles[$i];\n> +\t\t    my $basename = basename($name);\n\nSide note to the side note.\n\nA related naming guideline I failed to follow (because I was\nmostly copying your code) suggests that the hash here should be\nnamed %fullname, instead of %basename.  Then logically:\n\n\t$fullname{'hello.c'} = 'a/b/hello.c';\n\nthat is, you consult %fullname hash using the basename as the\nkey to extract the corresponding fullname.  The naming guideline\nis \"Name the dictionary after its values, not after its keys.\"\n"},{"id":"69162","messageId":"47B9A354.7070905@catalyst.net.nz","threadId":"12166","inReplyTo":"7vwsp3uf0u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2008-02-18T15:25:08Z","receivedAt":"2008-02-18T15:25:08Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n> A related naming guideline I failed to follow (because I was\n> mostly copying your code) suggests that the hash here should be\n> named %fullname, instead of %basename.  Then logically:\n\nDouble ACK on your logic and arguments - I was thinking \"fullname\" as I\nread your first email. Not sure how stable the output is across CVS\nversions/ports WRT leading slashes, might be a good idea to try to\ncanonicalise the paths.\n\nI am travelling at the moment, but I'll try and review the patch with\nthe actual code. Some of the ugliness you're complaining about might be\nmine (plurals, and perhaps even the $#array) but I refuse to recognise\nthe grep as mine.\n\nAnnotate might still put me to shame - perhaps I was drunk?\n\ncheers,\n\n\nm\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington\nWEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St\nNZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224\n      Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n"},{"id":"69164","messageId":"alpine.LSU.1.00.0802181624490.30505@racer.site","threadId":"12166","inReplyTo":"47B9A354.7070905@catalyst.net.nz","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T16:27:00Z","receivedAt":"2008-02-18T16:27:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Feb 2008, Martin Langhoff wrote:\n\n> Junio C Hamano wrote:\n> > A related naming guideline I failed to follow (because I was mostly \n> > copying your code) suggests that the hash here should be named \n> > %fullname, instead of %basename.  Then logically:\n> \n> Double ACK on your logic and arguments - I was thinking \"fullname\" as I \n> read your first email.\n\nOkay, will change.\n\n> Not sure how stable the output is across CVS versions/ports WRT leading \n> slashes, might be a good idea to try to canonicalise the paths.\n\nNote that for this reason, only the \"File:\" output -- which does not show \nslashes, but only the basenames -- is used to match the files.  We need \nthe full path in the git repository, though, to apply the patches.\n\n> I am travelling at the moment, but I'll try and review the patch with \n> the actual code.\n\nThanks.  I am confident that I will have posted another version by the \ntime you come around to review it.\n\nCiao,\nDscho\n"},{"id":"69166","messageId":"47B9B35B.7040200@catalyst.net.nz","threadId":"12166","inReplyTo":"alpine.LSU.1.00.0802181624490.30505@racer.site","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2008-02-18T16:33:31Z","receivedAt":"2008-02-18T16:33:31Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Johannes Schindelin wrote:\n> Note that for this reason, only the \"File:\" output -- which does not show \n> slashes, but only the basenames -- is used to match the files.  We need \n> the full path in the git repository, though, to apply the patches.\n\nYes - that's ugly. We have a couple of options\n\n - Run cvs status once per directory we touch. Use -l tomake it\nnon-recursive. It will be a tad slower/chattier.\n\n - Parse the 'Repository revision:' line to find out what path on the\nserver matches our repo 'root'.\n\n> Thanks.  I am confident that I will have posted another version by the \n> time you come around to review it.\n\nGreat! Careful, you might end up enjoying Perl ;-)\n\n\n\nm\n-- \n"},{"id":"69178","messageId":"alpine.LSU.1.00.0802181739450.30505@racer.site","threadId":"12166","inReplyTo":"47B9B35B.7040200@catalyst.net.nz","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T17:43:08Z","receivedAt":"2008-02-18T17:43:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Feb 2008, Martin Langhoff wrote:\n\n> Johannes Schindelin wrote:\n>\n> > Note that for this reason, only the \"File:\" output -- which does not \n> > show slashes, but only the basenames -- is used to match the files.  \n> > We need the full path in the git repository, though, to apply the \n> > patches.\n> \n> Yes - that's ugly. We have a couple of options\n> \n>  - Run cvs status once per directory we touch. Use -l tomake it\n> non-recursive. It will be a tad slower/chattier.\n\nI think that my approach is a bit faster: make lists with unique \nbasenames.  In the most common case, there will be only one list, I \nsuspect.\n\n>  - Parse the 'Repository revision:' line to find out what path on the\n> server matches our repo 'root'.\n\nI am not so sure.  It _should_ be reconstructible by CVS/Repository + \ndirname + basename, but I guess that it makes us susceptible to other CVS \nbreakages.  Whereas I think that we will be fine, relying on the basename \nin the File: line.\n\n> > Thanks.  I am confident that I will have posted another version by the \n> > time you come around to review it.\n> \n> Great! Careful, you might end up enjoying Perl ;-)\n\nHeh.  I always said that I like Perl, because it's easier to tell who's \nan expert, than for, say, Python.  Unfortunately, that means that it is \neasy to see that I am not an expert myself :-(\n\nCiao,\nDscho\n"},{"id":"69179","messageId":"alpine.LSU.1.00.0802181627340.30505@racer.site","threadId":"12166","inReplyTo":"7vbq6fvudp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T17:54:27Z","receivedAt":"2008-02-18T17:54:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 17 Feb 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > \tFeel free to criticise/educate me on my Perl style.\n> \n> Here it goes ;-)\n\nVery much appreciated.\n\n> > +    # \"cvs status\" reorders the parameters, notably when there are multiple\n> > +    # arguments with the same basename.  So be precise here.\n> > +    while (@canstatusfiles) {\n> > +      my @canstatusfiles2 = ();\n> > +      my %basenames = ();\n> > +      for (my $i = 0; $i <= $#canstatusfiles; $i++) {\n> \n> The \"$index <= $#array\" termination condition feels so Perl4-ish.\n> \n> \tfor (my $i = 0; $i < @canstatusfiles; $i++) {\n\nIt looks nicer, granted.  But because of my use of splice(), it does not \nwork.  However, it seems that I introduced another breakage there...\n\nSo this is how I will do it: have a hash with all remaining fullnames, and \n\"delete\" them when they are done.\n\n> > +        my $name = $canstatusfiles[$i];\n> \n> > +\tmy $basename = $name;\n> > +\t$basename =~ s/.*\\///;\n> \n> The script uses File::Basename upfront so perhaps just simply...\n\nI tried that.  But as the file need not exist, \"basename\" went on strike.\n\nSo I'll keep the (ugly) version.\n\n> \tmy $basename = basename($name);\n> \n> > +\t$basename = \"no file \" . $basename if (grep {$_ eq $basename} @afiles);\n> \n> Huh?  Perl or no Perl that is too ugly a hack...  What special treatment \n> do \"added files\" need?  We would want to make sure that the files are \n> not reported from \"cvs status\"?\n\nTo the contrary, they _are_ reported, with an ugly \"no file \" prepended.  \n\nSo in order to verify those, I have to make sure that there is no file \nnamed \"no file <blabla>\", which would not be distinguishable from the \nreported for the non-existing file \"<blabla>\".\n\nBut I'll just use your %added idea.\n\n> > +\tchomp($basename);\n> \n> Huh?  Perhaps you wanted to chomp at the very beginning of the loop?\n\nNo, I want to do that after the \"no file \" prepending.  Because that is \nthe way \"cvs status\" reports them... with no good way for me to tell how \nmuch leading/trailing white space there is.\n\nBut you're right, I should add a test to verify that a filename with \nleading spaces is added correctly.\n\nSo I will do that.\n\n> > +\tif (!defined($basenames{$basename})) {\n> > +\t  $basenames{$basename} = $name;\n> > +\t  push (@canstatusfiles2, $name);\n> > +\t  splice (@canstatusfiles, $i, 1);\n> > +\t  $i--;\n> >          }\n> > +      }\n> \n> > +      my @cvsoutput;\n> > +      @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);\n> > +      foreach my $l (@cvsoutput) {\n> > +          chomp $l;\n> > +          if ( $l =~ /^File:\\s+(.*\\S)\\s+Status: (.*)$/ ) {\n> > +            $cvsstat{$basenames{$1}} = $2 if defined($basenames{$1});\n> > +          }\n> > +      }\n> \n> I think \"exists $hash{$index}\" would be easier to read and more\n> logical here and also if () condition above.\n\nRight.\n\nThanks for your review,\nDscho\n"},{"id":"69180","messageId":"alpine.LSU.1.00.0802181754451.30505@racer.site","threadId":"12166","inReplyTo":"47B9B35B.7040200@catalyst.net.nz","subject":"[PATCH v2] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T17:55:22Z","receivedAt":"2008-02-18T17:55:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIn my use cases, \"cvs status\" sometimes reordered the passed filenames,\nwhich often led to a misdetection of a dirty state (when it was in\nreality a clean state).\n\nI finally tracked it down to two filenames having the same basename.\n\nSo no longer trust the order of the results blindly, but actually check\nthe file name.\n\nSince \"cvs status\" only returns the basename (and the complete path on the\nserver which is useless for our purposes), run \"cvs status\" several times\nwith lists consisting of files with unique (chomped) basenames.\n\nBe a bit clever about new files: these are reported as \"no file <blabla>\",\nso in order to discern it from existing files, prepend \"no file \" to the\nbasename.\n\nIn other words, one call to \"cvs status\" will not ask for two files\n\"blabla\" (which does not yet exist) and \"no file blabla\" (which exists).\n\nThis patch makes cvsexportcommit slightly slower, when the list of changed\nfiles has non-unique basenames, but at least it is accurate now.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tThis should address your concerns, Junio!\n\n git-cvsexportcommit.perl       |   40 ++++++++++++++++++++++++++++++++--------\n t/t9200-git-cvsexportcommit.sh |   35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 8 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 2a8ad1e..c00368b 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -197,15 +197,39 @@ if (@canstatusfiles) {\n       my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\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+    # \"cvs status\" reorders the parameters, notably when there are multiple\n+    # arguments with the same basename.  So be precise here.\n+\n+    my %added = map { $_ => 1 } @afiles;\n+    my %todo = map { $_ => 1 } @canstatusfiles;\n+\n+    while (%todo) {\n+      my @canstatusfiles2 = ();\n+      my %fullname = ();\n+      foreach my $name (keys %todo) {\n+\tmy $basename = $name;\n+\t$basename =~ s/.*\\///;\n+\t$basename = \"no file \" . $basename if (exists($added{$basename}));\n+\tchomp($basename);\n+\n+\tif (!exists($fullname{$basename})) {\n+\t  $fullname{$basename} = $name;\n+\t  push (@canstatusfiles2, $name);\n+\t  delete($todo{$name});\n         }\n+      }\n+      my @cvsoutput;\n+      @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);\n+      foreach my $l (@cvsoutput) {\n+        chomp $l;\n+        if ($l =~ /^File:\\s+(.*\\S)\\s+Status: (.*)$/) {\n+\t  if (!exists($fullname{$1})) {\n+\t    print STDERR \"Huh? Status reported for unexpected file '$1'\\n\";\n+\t  } else {\n+\t    $cvsstat{$fullname{$1}} = $2;\n+\t  }\n+\t}\n+      }\n     }\n }\n \ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex 49d57a8..a55a203 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -262,4 +262,39 @@ test_expect_success '-w option should work with relative GIT_DIR' '\n       )\n '\n \n+test_expect_success 'check files before directories' '\n+\n+\techo Notes > release-notes &&\n+\tgit add release-notes &&\n+\tgit commit -m \"Add release notes\" release-notes &&\n+\tid=$(git rev-parse HEAD) &&\n+\tgit cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+\n+\techo new > DS &&\n+\techo new > E/DS &&\n+\techo modified > release-notes &&\n+\tgit add DS E/DS release-notes &&\n+\tgit commit -m \"Add two files with the same basename\" &&\n+\tid=$(git rev-parse HEAD) &&\n+\tgit cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+\tcheck_entries \"$CVSWORK/E\" \"DS/1.1/|newfile5.txt/1.1/\" &&\n+\tcheck_entries \"$CVSWORK\" \"DS/1.1/|release-notes/1.2/\" &&\n+\tdiff -u \"$CVSWORK/DS\" DS &&\n+\tdiff -u \"$CVSWORK/E/DS\" E/DS &&\n+\tdiff -u \"$CVSWORK/release-notes\" release-notes\n+\n+'\n+\n+test_expect_success 'commit a file with leading spaces in the name' '\n+\n+\techo space > \" space\" &&\n+\tgit add \" space\" &&\n+\tgit commit -m \"Add a file with a leading space\" &&\n+\tid=$(git rev-parse HEAD) &&\n+\tgit cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+\tcheck_entries \"$CVSWORK\" \" space/1.1/|DS/1.1/|release-notes/1.2/\" &&\n+\tdiff -u \"$CVSWORK/ space\" \" space\"\n+\n+'\n+\t\n test_done\n-- \n1.5.4.2.217.g468be\n"},{"id":"69184","messageId":"7v1w7ap0vo.fsf@gitster.siamese.dyndns.org","threadId":"12166","inReplyTo":"alpine.LSU.1.00.0802181627340.30505@racer.site","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T18:36:59Z","receivedAt":"2008-02-18T18:36:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> The script uses File::Basename upfront so perhaps just simply...\n>\n> I tried that.  But as the file need not exist, \"basename\" went on strike.\n>\n> So I'll keep the (ugly) version.\n\nThe reason why \"basename\" is not being used needs documented in\nthe code then.  The next person will likely to make the same\nmistake as I did.\n"},{"id":"69186","messageId":"alpine.LSU.1.00.0802181849410.30505@racer.site","threadId":"12166","inReplyTo":"7v1w7ap0vo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T18:50:24Z","receivedAt":"2008-02-18T18:50:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 18 Feb 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> The script uses File::Basename upfront so perhaps just simply...\n> >\n> > I tried that.  But as the file need not exist, \"basename\" went on \n> > strike.\n> >\n> > So I'll keep the (ugly) version.\n> \n> The reason why \"basename\" is not being used needs documented in the code \n> then.  The next person will likely to make the same mistake as I did.\n\nRight.  Could you be so good and squash this in, then, please?\n\nThanks,\nDscho\n\n-- snipsnap --\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex c00368b..6eff42c 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -208,6 +208,7 @@ if (@canstatusfiles) {\n       my %fullname = ();\n       foreach my $name (keys %todo) {\n \tmy $basename = $name;\n+\t# cannot use basename(), since $name need not exist in the CVS tree\n \t$basename =~ s/.*\\///;\n \t$basename = \"no file \" . $basename if (exists($added{$basename}));\n \tchomp($basename);\n"},{"id":"69188","messageId":"47B9D484.1020304@catalyst.net.nz","threadId":"12166","inReplyTo":"7v1w7ap0vo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2008-02-18T18:55:00Z","receivedAt":"2008-02-18T18:55:00Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n>>> The script uses File::Basename upfront so perhaps just simply...\n>> I tried that.  But as the file need not exist, \"basename\" went on strike.\n>>\n>> So I'll keep the (ugly) version.\n\n?! basename() never touches the disk. I just read it to confirm -\n$VERSION is 2.74 and I'm somewhat disappointed to find it's not as\nportable as I'd expect (perhaps it gets hardcoded during install?).\n\nAnd my /usr/bin/basename doesn't care if the file exists either\n\n  $ type basename\n  basename is /usr/bin/basename\n  $ basename /foo/bar/baz\n  baz\n  $ stat /foo/bar/baz\n  stat: cannot stat `/foo/bar/baz': No such file or directory\n\nSo I am fairly confident that we can safely use File::Basename's\nbasename() on arbitrary strings that look like a path. We use basename()\nquite a bit in our perl scripts in git.\n\ncheers,\n\n\nm\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington\nWEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St\nNZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224\n      Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n"},{"id":"69194","messageId":"alpine.LSU.1.00.0802181942230.30505@racer.site","threadId":"12166","inReplyTo":"47B9D484.1020304@catalyst.net.nz","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T19:42:54Z","receivedAt":"2008-02-18T19:42:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Feb 2008, Martin Langhoff wrote:\n\n> ?! basename() never touches the disk. I just read it to confirm - \n> $VERSION is 2.74 and I'm somewhat disappointed to find it's not as \n> portable as I'd expect (perhaps it gets hardcoded during install?).\n\nWell, please try for yourself.  If it works for you, then I probably had \nanother error in my patch.\n\nCiao,\nDscho\n"},{"id":"69196","messageId":"47B9E561.8040605@catalyst.net.nz","threadId":"12166","inReplyTo":"alpine.LSU.1.00.0802181942230.30505@racer.site","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2008-02-18T20:06:57Z","receivedAt":"2008-02-18T20:06:57Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Johannes Schindelin wrote:\n> Well, please try for yourself.  If it works for you, then I probably had \n> another error in my patch.\n\n$ perl -MFile::Basename -e 'print basename(\"/foo/bar/baz\");'\nbaz\n\nJohannes, what are you smoking? No PUI here! ;-)\n\n\n\nm\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington\nWEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St\nNZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224\n      Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n"},{"id":"69197","messageId":"alpine.LSU.1.00.0802182025340.30505@racer.site","threadId":"12166","inReplyTo":"47B9E561.8040605@catalyst.net.nz","subject":"Re: [PATCH] cvsexportcommit: be graceful when \"cvs status\" reorders the arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-18T20:29:25Z","receivedAt":"2008-02-18T20:29:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Feb 2008, Martin Langhoff wrote:\n\n> Johannes Schindelin wrote:\n>\n> > Well, please try for yourself.  If it works for you, then I probably \n> > had another error in my patch.\n> \n> $ perl -MFile::Basename -e 'print basename(\"/foo/bar/baz\");'\n> baz\n> \n> Johannes, what are you smoking? No PUI here! ;-)\n\nUnfortunately, I am not smoking, because my throat is inflamed...\n\nChecked again, and sure enough, it works.  So, this is a replacement patch \nto be squashed in... So: I'm sorry...\n\nCiao,\nDscho\n\n-- snipsnap --\n git-cvsexportcommit.perl |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex c00368b..b8114f7 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -207,8 +207,7 @@ if (@canstatusfiles) {\n       my @canstatusfiles2 = ();\n       my %fullname = ();\n       foreach my $name (keys %todo) {\n-\tmy $basename = $name;\n-\t$basename =~ s/.*\\///;\n+\tmy $basename = basename($name);\n \t$basename = \"no file \" . $basename if (exists($added{$basename}));\n \tchomp($basename);\n \n"}]}