{"thread":{"id":"5529","subject":"[PATCH] gitweb: reset input record separator in parse_commit even on error","startedAt":"2006-09-09T15:12:36Z","lastAt":"2006-09-09T22:00:18Z","messageCount":3,"participants":["Sven Verdoolaege","Jeff King","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26640","messageId":"20060909151236.GA25518@liacs.nl","threadId":"5529","inReplyTo":null,"subject":"[PATCH] gitweb: reset input record separator in parse_commit even on error","fromName":"Sven Verdoolaege","fromEmail":"skimo@liacs.nl","sentAt":"2006-09-09T15:12:36Z","receivedAt":"2006-09-09T15:12:36Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"---\nThis probably never happens, but just in case...\n\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7afdf33..60dd598 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -897,8 +897,8 @@ sub parse_commit {\n \t\tmy $fd = git_pipe(\"rev-list\", \"--header\", \"--parents\", \"--max-count=1\", $commit_id)\n \t\t\tor return;\n \t\t@commit_lines = split '\\n', <$fd>;\n-\t\tclose $fd or return;\n \t\t$/ = \"\\n\";\n+\t\tclose $fd or return;\n \t\tpop @commit_lines;\n \t}\n \tmy $header = shift @commit_lines;\n-- \n0.99.8c.g64e8-dirty\n"},{"id":"26649","messageId":"20060909205159.GC16906@coredump.intra.peff.net","threadId":"5529","inReplyTo":"20060909151236.GA25518@liacs.nl","subject":"Re: [PATCH] gitweb: reset input record separator in parse_commit even on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-09-09T20:51:59Z","receivedAt":"2006-09-09T20:51:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 09, 2006 at 05:12:36PM +0200, Sven Verdoolaege wrote:\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7afdf33..60dd598 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -897,8 +897,8 @@ sub parse_commit {\n>  \t\tmy $fd = git_pipe(\"rev-list\", \"--header\", \"--parents\", \"--max-count=1\", $commit_id)\n>  \t\t\tor return;\n>  \t\t@commit_lines = split '\\n', <$fd>;\n> -\t\tclose $fd or return;\n>  \t\t$/ = \"\\n\";\n> +\t\tclose $fd or return;\n>  \t\tpop @commit_lines;\n>  \t}\n>  \tmy $header = shift @commit_lines;\n\nYou missed the other early return from git_pipe. However, I think the\napproach is wrong; this is a great opportunity to use the dynamic\nscoping offered by 'local':\n\n  else {\n    local $/ = \"\\0\";\n    # do stuff with $/ as \"\\0\"\n  }\n  # $/ has automatically been reset at the end of the block\n\nLooking further at this bit of code, it seems even more confusing,\nthough. We split the output of rev-list on NUL, grab presumably the\nentire thing (since there shouldn't be any NULs in the output, right?)\nand then split it on newline into a list. Why aren't we doing this:\n  else {\n    open my $fd, ...;\n    @commit_lines = map { chomp; $_ } <$fd>;\n    ...\n\n-Peff\n"},{"id":"26655","messageId":"edvdgb$mtt$1@sea.gmane.org","threadId":"5529","inReplyTo":"20060909205159.GC16906@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: reset input record separator in parse_commit even on error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-09T22:00:18Z","receivedAt":"2006-09-09T22:00:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n\n> On Sat, Sep 09, 2006 at 05:12:36PM +0200, Sven Verdoolaege wrote:\n> \n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 7afdf33..60dd598 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -897,8 +897,8 @@ sub parse_commit {\n>>              my $fd = git_pipe(\"rev-list\", \"--header\", \"--parents\", \"--max-count=1\", $commit_id)\n>>                      or return;\n>>              @commit_lines = split '\\n', <$fd>;\n>> -            close $fd or return;\n>>              $/ = \"\\n\";\n>> +            close $fd or return;\n>>              pop @commit_lines;\n>>      }\n>>      my $header = shift @commit_lines;\n> \n> You missed the other early return from git_pipe. However, I think the\n> approach is wrong; this is a great opportunity to use the dynamic\n> scoping offered by 'local':\n> \n>   else {\n>     local $/ = \"\\0\";\n>     # do stuff with $/ as \"\\0\"\n>   }\n>   # $/ has automatically been reset at the end of the block\n\nAnd this should be done consistently, for all plain formats \n(blob_plain, blobdiff_plain, commitdiff_plain), and some other\n places.\n\nSometimes it is\n  local $/ = \"\\0\";\nmore often\n  local $/ = undef;\n(or equivalently but slightly less human friendly, just 'local $/');\n\n> Looking further at this bit of code, it seems even more confusing,\n> though. We split the output of rev-list on NUL, grab presumably the\n> entire thing (since there shouldn't be any NULs in the output, right?)\n> and then split it on newline into a list. Why aren't we doing this:\n>   else {\n>     open my $fd, ...;\n>     @commit_lines = map { chomp; $_ } <$fd>;\n>     ...\n\nThat is because by default git-rev-list separates output for different\ncommits (when --header option is used) bu NULL... only because of \n\"--max-count=1\" there is only _one_ record... nevertheless it ends with \n\"\\0\" (NULL) character.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"}]}