{"thread":{"id":"30036","subject":"[PATCH] Make 'cvs -n commit ...' not to commit","startedAt":"2012-03-22T20:57:43Z","lastAt":"2012-03-23T19:02:04Z","messageCount":3,"participants":["ericc","Junio C Hamano","Eric Chamberland"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"187570","messageId":"20120323131100.7262D440B33@melkor.giref.ulaval.ca","threadId":"30036","inReplyTo":null,"subject":"[PATCH] Make 'cvs -n commit ...' not to commit","fromName":"ericc","fromEmail":"eric.chamberland@giref.ulaval.ca","sentAt":"2012-03-22T20:57:43Z","receivedAt":"2012-03-22T20:57:43Z","isPatch":true,"sender":{"key":"eric.chamberland@giref.ulaval.ca","avatar":null},"body":"Actually, doing a 'cvs -n commit' will _do_ the commit...\nWith this patch, it now goes through the code, but don't do the commit.\nA further progress would be to do the pre-commit hook is possible...\n\nEric Chamberland <Eric.Chamberland@giref.ulaval.ca>\n---\n git-cvsserver.perl |    9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex b8eddab..67ec4d0 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -1395,6 +1395,9 @@ sub req_ci\n         push @committedfiles, $committedfile;\n         $log->info(\"Committing $filename\");\n \n+        # Don't want to actually _DO_ the update if -n specified\n+        unless ( $state->{globaloptions}{-n} ) \n+        {\n         system(\"mkdir\",\"-p\",$dirpart) unless ( -d $dirpart );\n \n         unless ( $rmflag )\n@@ -1424,6 +1427,7 @@ sub req_ci\n             $log->info(\"Updating file '$filename'\");\n             system(\"git\", \"update-index\", $filename);\n         }\n+        }\n     }\n \n     unless ( scalar(@committedfiles) > 0 )\n@@ -1434,6 +1438,9 @@ sub req_ci\n         return;\n     }\n \n+    # Don't want to actually _DO_ the update if -n specified\n+    unless ( $state->{globaloptions}{-n} ) \n+    {\n     my $treehash = `git write-tree`;\n     chomp $treehash;\n \n@@ -1537,7 +1544,7 @@ sub req_ci\n             print \"/$filepart/1.$meta->{revision}//$kopts/\\n\";\n         }\n     }\n-\n+    }\n     cleanupWorkTree();\n     print \"ok\\n\";\n }\n-- \n1.7.3.4\n"},{"id":"187598","messageId":"7vhaxftb54.fsf@alter.siamese.dyndns.org","threadId":"30036","inReplyTo":"20120323131100.7262D440B33@melkor.giref.ulaval.ca","subject":"Re: [PATCH] Make 'cvs -n commit ...' not to commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-23T18:39:19Z","receivedAt":"2012-03-23T18:39:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ericc <eric.chamberland@giref.ulaval.ca> writes:\n\n> Actually, doing a 'cvs -n commit' will _do_ the commit...\n> With this patch, it now goes through the code, but don't do the commit.\n\nOK.\n\n> A further progress would be to do the pre-commit hook is possible...\n\nIt is not clear what you meant here.\n\n> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca>\n> ---\n>  git-cvsserver.perl |    9 ++++++++-\n>  1 files changed, 8 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-cvsserver.perl b/git-cvsserver.perl\n> index b8eddab..67ec4d0 100755\n> --- a/git-cvsserver.perl\n> +++ b/git-cvsserver.perl\n> @@ -1395,6 +1395,9 @@ sub req_ci\n>          push @committedfiles, $committedfile;\n>          $log->info(\"Committing $filename\");\n>  \n> +        # Don't want to actually _DO_ the update if -n specified\n> +        unless ( $state->{globaloptions}{-n} ) \n> +        {\n>          system(\"mkdir\",\"-p\",$dirpart) unless ( -d $dirpart );\n>  \n>          unless ( $rmflag )\n> @@ -1424,6 +1427,7 @@ sub req_ci\n>              $log->info(\"Updating file '$filename'\");\n>              system(\"git\", \"update-index\", $filename);\n>          }\n> +        }\n>      }\n\nI understand that you tried to make the patch smaller by avoiding\nre-indenting, but this is *yucky*.\n\nIt looks to me that the above part could be solved with:\n\n\tunless (...) {\n\t\tnext;\n\t}\n\nI think the function being patched is too big.  Wouldn't it be better to\nhave a refactoring patch to move the above per-path logic to a helper\nfunction that deals with a single path, and then insert the \"omit call to\nthat helper when run with -n\" code in a separate patch?\n\nThe same comment applies to the other hunk.\n\nAlso I notice that the indentation used throughout the file is somewhat\nbroken (e.g. \"Emulate by running hooks/update\" part is indented to 8\ncolumns, but earlier parts use 4 space indent).  The right structure for\nthis change may be:\n\n Patch 1: Fix indentation (and do nothing else) to uniformly indent with\n          HT;\n\n Patch 2: Refactor this big funciton using a handful of helper functions\n\t  (and do nothing else);\n\n Patch 3: Omit calls to these helper functions under -n option.\n\n\n> @@ -1434,6 +1438,9 @@ sub req_ci\n>          return;\n>      }\n>  \n> +    # Don't want to actually _DO_ the update if -n specified\n> +    unless ( $state->{globaloptions}{-n} ) \n> +    {\n>      my $treehash = `git write-tree`;\n>      chomp $treehash;\n>  \n> @@ -1537,7 +1544,7 @@ sub req_ci\n>              print \"/$filepart/1.$meta->{revision}//$kopts/\\n\";\n>          }\n>      }\n> -\n> +    }\n>      cleanupWorkTree();\n>      print \"ok\\n\";\n>  }\n"},{"id":"187603","messageId":"4F6CC8AC.4050907@giref.ulaval.ca","threadId":"30036","inReplyTo":"7vhaxftb54.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make 'cvs -n commit ...' not to commit","fromName":"Eric Chamberland","fromEmail":"eric.chamberland@giref.ulaval.ca","sentAt":"2012-03-23T19:02:04Z","receivedAt":"2012-03-23T19:02:04Z","isPatch":true,"sender":{"key":"eric.chamberland@giref.ulaval.ca","avatar":null},"body":"On 03/23/2012 02:39 PM, Junio C Hamano wrote:\n> ericc<eric.chamberland@giref.ulaval.ca>  writes:\n>\n>> Actually, doing a 'cvs -n commit' will _do_ the commit...\n>> With this patch, it now goes through the code, but don't do the commit.\n>\n> OK.\n>\n>> A further progress would be to do the pre-commit hook is possible...\n\nSorry, I wanted to write:\n\n\"A further progress would be to do the pre-commit hook *if* possible...\"\n\nhere, we are used to do \"cvs -n commit\" just to check if the \"hooks\" on \nthe cvs server will fail or not...\n\n\n>\n> I understand that you tried to make the patch smaller by avoiding\n> re-indenting, but this is *yucky*.\n>\n> It looks to me that the above part could be solved with:\n>\n> \tunless (...) {\n> \t\tnext;\n> \t}\n>\n> I think the function being patched is too big.  Wouldn't it be better to\n> have a refactoring patch to move the above per-path logic to a helper\n> function that deals with a single path, and then insert the \"omit call to\n> that helper when run with -n\" code in a separate patch?\n>\n> The same comment applies to the other hunk.\n>\n> Also I notice that the indentation used throughout the file is somewhat\n> broken (e.g. \"Emulate by running hooks/update\" part is indented to 8\n> columns, but earlier parts use 4 space indent).  The right structure for\n> this change may be:\n>\n>   Patch 1: Fix indentation (and do nothing else) to uniformly indent with\n>            HT;\n>\n>   Patch 2: Refactor this big funciton using a handful of helper functions\n> \t  (and do nothing else);\n>\n>   Patch 3: Omit calls to these helper functions under -n option.\n>\n>\n\nOk you are right...  These were my very first lines in Perl... I just \nwanted to catch the attention of someone who is able to do the changes \ncorrectly... and in a more clean way than I...\n\nEric\n"}]}