{"thread":{"id":"9755","subject":"[PATCH] gitweb: Fix and simplify \"split patch\" detection","startedAt":"2007-09-02T20:18:42Z","lastAt":"2007-09-03T11:00:49Z","messageCount":4,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"52277","messageId":"200709022218.43042.jnareb@gmail.com","threadId":"9755","inReplyTo":null,"subject":"[PATCH] gitweb: Fix and simplify \"split patch\" detection","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-09-02T20:18:42Z","receivedAt":"2007-09-02T20:18:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"There are some cases when one line from \"raw\" git-diff output (raw format)\ncorresponds to more than one patch in the patchset git-diff output; we\ncall this situation \"split patch\". Current code misdetected subsequent\npatches (for different files) with the same pre-image and post-image\nas fragments of \"split patch\", leading to mislabeled from-file/to-file\ndiff header etc.\n\n\nWe could parse from_file and to_file (if to_file exists) from the \"git\ndiff header\" in the patch, i.e. from the \"^diff\" line in patchset, and\nconsider patch \"split\" or \"continued\" if not only pre-image and\npost-image (from_id and to_id) matches, but only when also from_file\nand to_file (if it exists) matches.  This has the advantage of being\ngeneric, not depending on details of git diff output formatting, and\nhow patch output relates to raw diff output, but it adds yet another\ncomplication.\n\nNote: both from_name and to_name can be quoted and contain spaces;\ntheir separation is nontrivial.\n\n\nAlternate solution, which we did chose, is to check when git splits\npatches, and do not check if parsed info from current patch corresponds\nto current or next raw diff format output line.  Git splits patches\nonly for 'T' (typechange) status filepair, and there always two patches\ncorresponding to one raw diff line.\n\nAnd we can get rid of buffering extended diff header and parsing it,\nThis simplifies git_patchset_body, making it easier to understand and\nmaintain.  At the other hand it fixes gitweb to git diff output details.\n\n\nWhile at it we added 'status_str' to diffinfo output, which stores\nstatus (also for merge commit) as a string.  This allows for easy\nchecking if there is given status among all for merge commit, e.g.\n  $diffinfo->{'status_str'} =~ /T/\n\nThere are no users of from_ids_eq subroutine.\n\nNoticed-by: Yann Dirson <ydirson@altern.org>\nDiagnosed-by: Petr Baudis <pasky@suse.cz>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is post 1.5.3 resend of the patch\n  Message-Id: <200708290208.08191.jnareb@gmail.com>\nposted in the \"[BUG] gitweb on repo.or.cz shows buggy commitdiff\"\nthread.\n\nI'm a tiny little bit ambivalent about this patch: on one hand it\nfixes the bug and simplifies git_patchset_body code making it easier\nto maintain, on the other hand it ties patch handling code in gitweb\nwith the details of git-diff patch vs. raw output format.\n\n gitweb/gitweb.perl |   63 +++++++++++++++++++--------------------------------\n 1 files changed, 24 insertions(+), 39 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex b2bae1b..8c1e02c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1963,7 +1963,7 @@ sub parse_difftree_raw_line {\n \t\t$res{'to_mode'} = $2;\n \t\t$res{'from_id'} = $3;\n \t\t$res{'to_id'} = $4;\n-\t\t$res{'status'} = $5;\n+\t\t$res{'status'} = $res{'status_str'} = $5;\n \t\t$res{'similarity'} = $6;\n \t\tif ($res{'status'} eq 'R' || $res{'status'} eq 'C') { # renamed or copied\n \t\t\t($res{'from_file'}, $res{'to_file'}) = map { unquote($_) } split(\"\\t\", $7);\n@@ -1979,6 +1979,7 @@ sub parse_difftree_raw_line {\n \t\t$res{'to_mode'} = pop @{$res{'from_mode'}};\n \t\t$res{'from_id'} = [ split(' ', $3) ];\n \t\t$res{'to_id'} = pop @{$res{'from_id'}};\n+\t\t$res{'status_str'} = $4;\n \t\t$res{'status'} = [ split('', $4) ];\n \t\t$res{'to_file'} = unquote($5);\n \t}\n@@ -3112,6 +3113,7 @@ sub git_patchset_body {\n \tmy $patch_line;\n \tmy $diffinfo;\n \tmy (%from, %to);\n+\tmy $patches_per_difftree_line = 1;\n \n \tprint \"<div class=\\\"patchset\\\">\\n\";\n \n@@ -3124,42 +3126,13 @@ sub git_patchset_body {\n \n  PATCH:\n \twhile ($patch_line) {\n-\t\tmy @diff_header;\n-\t\tmy ($from_id, $to_id);\n-\n-\t\t# git diff header\n-\t\t#assert($patch_line =~ m/^diff /) if DEBUG;\n-\t\t#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed\n-\t\t$patch_number++;\n-\t\tpush @diff_header, $patch_line;\n-\n-\t\t# extended diff header\n-\tEXTENDED_HEADER:\n-\t\twhile ($patch_line = <$fd>) {\n-\t\t\tchomp $patch_line;\n-\n-\t\t\tlast EXTENDED_HEADER if ($patch_line =~ m/^--- |^diff /);\n-\n-\t\t\tif ($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/) {\n-\t\t\t\t$from_id = $1;\n-\t\t\t\t$to_id   = $2;\n-\t\t\t} elsif ($patch_line =~ m/^index ((?:[0-9a-fA-F]{40},)+[0-9a-fA-F]{40})..([0-9a-fA-F]{40})/) {\n-\t\t\t\t$from_id = [ split(',', $1) ];\n-\t\t\t\t$to_id   = $2;\n-\t\t\t}\n-\n-\t\t\tpush @diff_header, $patch_line;\n-\t\t}\n-\t\tmy $last_patch_line = $patch_line;\n \n \t\t# check if current patch belong to current raw line\n \t\t# and parse raw git-diff line if needed\n-\t\tif (defined $diffinfo &&\n-\t\t    defined $from_id && defined $to_id &&\n-\t\t    from_ids_eq($diffinfo->{'from_id'}, $from_id) &&\n-\t\t    $diffinfo->{'to_id'} eq $to_id) {\n+\t\tif ($patches_per_difftree_line > 1) {\n \t\t\t# this is continuation of a split patch\n \t\t\tprint \"<div class=\\\"patch cont\\\">\\n\";\n+\t\t\t$patches_per_difftree_line--;\n \t\t} else {\n \t\t\t# advance raw git-diff output if needed\n \t\t\t$patch_idx++ if defined $diffinfo;\n@@ -3167,7 +3140,7 @@ sub git_patchset_body {\n \t\t\t# compact combined diff output can have some patches skipped\n \t\t\t# find which patch (using pathname of result) we are at now\n \t\t\tmy $to_name;\n-\t\t\tif ($diff_header[0] =~ m!^diff --cc \"?(.*)\"?$!) {\n+\t\t\tif ($patch_line =~ m!^diff --cc \"?(.*)\"?$!) {\n \t\t\t\t$to_name = $1;\n \t\t\t}\n \n@@ -3191,6 +3164,7 @@ sub git_patchset_body {\n \t\t\t\t}\n \t\t\t} until (!defined $to_name || $to_name eq $diffinfo->{'to_file'} ||\n \t\t\t         $patch_idx > $#$difftree);\n+\n \t\t\t# modifies %from, %to hashes\n \t\t\tparse_from_to_diffinfo($diffinfo, \\%from, \\%to, @hash_parents);\n \t\t\tif ($diffinfo->{'nparents'}) {\n@@ -3232,32 +3206,43 @@ sub git_patchset_body {\n \t\t\t# this is first patch for raw difftree line with $patch_idx index\n \t\t\t# we index @$difftree array from 0, but number patches from 1\n \t\t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\";\n+\n+\t\t\t# typechange diff with 'T' status has two patches per one raw line\n+\t\t\tif ($diffinfo->{'status_str'} =~ /T/) {\n+\t\t\t\t$patches_per_difftree_line = 2;\n+\t\t\t}\n \t\t}\n \n+\t\t# git diff header\n+\t\t#assert($patch_line =~ m/^diff /) if DEBUG;\n+\t\t#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed\n+\t\t$patch_number++;\n \t\t# print \"git diff\" header\n-\t\t$patch_line = shift @diff_header;\n \t\tprint format_git_diff_header_line($patch_line, $diffinfo,\n \t\t                                  \\%from, \\%to);\n \n \t\t# print extended diff header\n-\t\tprint \"<div class=\\\"diff extended_header\\\">\\n\" if (@diff_header > 0);\n+\t\tprint \"<div class=\\\"diff extended_header\\\">\\n\";\n \tEXTENDED_HEADER:\n-\t\tforeach $patch_line (@diff_header) {\n+\t\twhile ($patch_line = <$fd>) {\n+\t\t\tchomp $patch_line;\n+\n+\t\t\tlast EXTENDED_HEADER if ($patch_line =~ m/^--- |^diff /);\n+\n \t\t\tprint format_extended_diff_header_line($patch_line, $diffinfo,\n \t\t\t                                       \\%from, \\%to);\n \t\t}\n-\t\tprint \"</div>\\n\"  if (@diff_header > 0); # class=\"diff extended_header\"\n+\t\tprint \"</div>\\n\"; # class=\"diff extended_header\"\n \n \t\t# from-file/to-file diff header\n-\t\t$patch_line = $last_patch_line;\n \t\tif (! $patch_line) {\n \t\t\tprint \"</div>\\n\"; # class=\"patch\"\n \t\t\tlast PATCH;\n \t\t}\n \t\tnext PATCH if ($patch_line =~ m/^diff /);\n \t\t#assert($patch_line =~ m/^---/) if DEBUG;\n-\t\t#assert($patch_line eq $last_patch_line) if DEBUG;\n \n+\t\tmy $last_patch_line = $patch_line;\n \t\t$patch_line = <$fd>;\n \t\tchomp $patch_line;\n \t\t#assert($patch_line =~ m/^\\+\\+\\+/) if DEBUG;\n-- \n1.5.2.5\n"},{"id":"52282","messageId":"7vmyw4ob7z.fsf@gitster.siamese.dyndns.org","threadId":"9755","inReplyTo":"200709022218.43042.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Fix and simplify \"split patch\" detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-03T00:15:44Z","receivedAt":"2007-09-03T00:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Alternate solution, which we did chose, is to check when git splits\n> patches, and do not check if parsed info from current patch corresponds\n> to current or next raw diff format output line.  Git splits patches\n> only for 'T' (typechange) status filepair, and there always two patches\n> corresponding to one raw diff line.\n\nNot that I can think of a better way offhand than what you\nalready mentioned, but I have to say that I am not entirely\nhappy with this implementation.  A really old git showed two\npatches (one creation and one deletion) for \"complete rewrite\",\nwhich was corrected long time ago.  I do not think we will\nchange the typechange output in a similar way in the future, but\nrelying on that level of detail feels somewhat ugly.\n\nAs you are reading from --patch-with-raw, you already know the\norder of patches that will be given to you when you finished\nreading the \"raw\" part.  The patches will come in the same\norder.  So it might be possible to keep track of patches to what\npath you are expecting and decide if it is part of what you are\nprocessing at the point you process \"diff --git\" line.\n"},{"id":"52334","messageId":"200709031147.32910.jnareb@gmail.com","threadId":"9755","inReplyTo":"7vmyw4ob7z.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Fix and simplify \"split patch\" detection","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-09-03T09:47:32Z","receivedAt":"2007-09-03T09:47:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > Alternate solution, which we did chose, is to check when git splits\n> > patches, and do not check if parsed info from current patch corresponds\n> > to current or next raw diff format output line.  Git splits patches\n> > only for 'T' (typechange) status filepair, and there always two patches\n> > corresponding to one raw diff line.\n> \n> Not that I can think of a better way offhand than what you\n> already mentioned, but I have to say that I am not entirely\n> happy with this implementation.\n\nI'm also bit unhappy with tying git_patchset_body code to the\nminute details of git-diff output.\n\n>                                    A really old git showed two \n> patches (one creation and one deletion) for \"complete rewrite\",\n> which was corrected long time ago.  I do not think we will\n> change the typechange output in a similar way in the future, but\n> relying on that level of detail feels somewhat ugly.\n\nGitweb requires git with --git-dir at least, so I don't think it\n(it meaning \"complete rewrite\" patch being \"split patch\") is\na problem here.\n\n> As you are reading from --patch-with-raw, you already know the\n> order of patches that will be given to you when you finished\n> reading the \"raw\" part.  The patches will come in the same\n> order.  So it might be possible to keep track of patches to what\n> path you are expecting and decide if it is part of what you are\n> processing at the point you process \"diff --git\" line.\n\nI'll try to come with the second solution.\n\nI wonder if the post-image name is unique in raw format of diff\noutput, and can be used alone to check if there are two patches\nper one raw output format line...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"52337","messageId":"7vtzqcj9ni.fsf@gitster.siamese.dyndns.org","threadId":"9755","inReplyTo":"200709031147.32910.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Fix and simplify \"split patch\" detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-03T11:00:49Z","receivedAt":"2007-09-03T11:00:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> I wonder if the post-image name is unique in raw format of diff\n> output, and can be used alone to check if there are two patches\n> per one raw output format line...\n\nI think that is a very sane assumption.\n\nYou may have a situation that a single preimage is used to\nproduce more than one postimage, but it does not make sense to\nsay this postimage is produced by doing this to that preimage\nand there is another way to produce the same postimage using\nsomething else.\n\nAt least, I do not think the diffcore chain is _capable_ of\ncoming up with such alternatives even if it wanted to ;-).\n"}]}