{"thread":{"id":"10512","subject":"[PATCH (resend)] gitweb: Fix and simplify \"split patch\" detection","startedAt":"2007-10-30T00:35:05Z","lastAt":"2007-10-30T00:35:05Z","messageCount":1,"participants":["Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"57499","messageId":"11937045052921-git-send-email-jnareb@gmail.com","threadId":"10512","inReplyTo":null,"subject":"[PATCH (resend)] gitweb: Fix and simplify \"split patch\" detection","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-10-30T00:35:05Z","receivedAt":"2007-10-30T00:35:05Z","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\nformat) corresponds to more than one patch in the patchset git-diff\noutput; we call this situation \"split patch\". Old code misdetected\nsubsequent patches (for different files) with the same pre-image and\npost-image as fragments of \"split patch\", leading to mislabeled\nfrom-file/to-file diff header etc.\n\nOld code used pre-image and post-image SHA-1 identifier ('from_id' and\n'to_id') to check if current patch corresponds to old raw diff format\nline, to find if one difftree raw line coresponds to more than one\npatch in the patch format.  Now we use post-image filename for that.\nThis assumes that post-image filename alone can be used to identify\ndifftree raw line.  In the case this changes (which is unlikely\nconsidering current diff engine) we can add 'from_id' and 'to_id'\nto detect \"patch splitting\" together with 'to_file'.\n\nBecause old code got pre-image and post-image SHA-1 identifier for the\npatch from the \"index\" line in extended diff header, diff header had\nto be buffered.  New code takes post-image filename from \"git diff\"\nheader, which is first line of a patch; this allows to simplify\ngit_patchset_body code.  A side effect of resigning diff header\nbuffering is that there is always \"diff extended_header\" div, even\nif extended diff header is empty.\n\n\nAlternate solution would be to check when git splits patches, and do\nnot check if parsed info from current patch corresponds to current or\nnext raw diff format output line.  Git splits patches only for 'T'\n(typechange) status filepair, and there always two patches\ncorresponding to one raw diff line.  It was not used because it would\ntie gitweb code to minute details of git diff output.\n\n\nWhile at it, use newly introduced parsed_difftree_line wrapper\nsubroutine in git_difftree_body.\n\nNoticed-by: Yann Dirson <ydirson@altern.org>\nDiagnosed-by: Petr Baudis <pasky@suse.cz>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nJunio has decided in\n  Message-ID: <7vmyw4ob7z.fsf@gitster.siamese.dyndns.org>\nthat tying gitweb to minute details of git diff output, namely that\nwe have \"split patch\" (two patches of patchset diff format for single\nline of raw ditt format) only for typechange (status 'T') diffs.\n\nWe make other assumption instead, that post-image filename uniquely\ndefines line of raw git diff output. Currently diffcore is not\ncapable of producing other output; see Junio in\n  Message-ID: <7vtzqcj9ni.fsf@gitster.siamese.dyndns.org>\nThis assumption simplifies git_patchset_body, as we no longer need\nto cache extended diff header for \"split patch\" detection.\n\n gitweb/gitweb.perl |  152 +++++++++++++++++++++++-----------------------------\n 1 files changed, 67 insertions(+), 85 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 48e21da..1537b0e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2000,6 +2000,19 @@ sub parse_difftree_raw_line {\n \treturn wantarray ? %res : \\%res;\n }\n \n+# wrapper: return parsed line of git-diff-tree \"raw\" output\n+# (the argument might be raw line, or parsed info)\n+sub parsed_difftree_line {\n+\tmy $line_or_ref = shift;\n+\n+\tif (ref($line_or_ref) eq \"HASH\") {\n+\t\t# pre-parsed (or generated by hand)\n+\t\treturn $line_or_ref;\n+\t} else {\n+\t\treturn parse_difftree_raw_line($line_or_ref);\n+\t}\n+}\n+\n # parse line of git-ls-tree output\n sub parse_ls_tree_line ($;%) {\n \tmy $line = shift;\n@@ -2043,6 +2056,7 @@ sub parse_from_to_diffinfo {\n \t\t\t}\n \t\t}\n \t} else {\n+\t\t# ordinary (not combined) diff\n \t\t$from->{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'};\n \t\tif ($diffinfo->{'status'} ne \"A\") { # not new (added) file\n \t\t\t$from->{'href'} = href(action=>\"blob\", hash_base=>$hash_parent,\n@@ -2766,6 +2780,7 @@ sub git_print_tree_entry {\n ## ......................................................................\n ## functions printing large fragments of HTML\n \n+# get pre-image filenames for merge (combined) diff\n sub fill_from_file_info {\n \tmy ($diff, @parents) = @_;\n \n@@ -2782,28 +2797,25 @@ sub fill_from_file_info {\n \treturn $diff;\n }\n \n-# parameters can be strings, or references to arrays of strings\n-sub from_ids_eq {\n-\tmy ($a, $b) = @_;\n-\n-\tif (ref($a) eq \"ARRAY\" && ref($b) eq \"ARRAY\" && @$a == @$b) {\n-\t\tfor (my $i = 0; $i < @$a; ++$i) {\n-\t\t\treturn 0 unless ($a->[$i] eq $b->[$i]);\n-\t\t}\n-\t\treturn 1;\n-\t} elsif (!ref($a) && !ref($b)) {\n-\t\treturn $a eq $b;\n-\t} else {\n-\t\treturn 0;\n-\t}\n-}\n-\n+# is current raw difftree line of file deletion\n sub is_deleted {\n \tmy $diffinfo = shift;\n \n \treturn $diffinfo->{'to_id'} eq ('0' x 40);\n }\n \n+# does patch correspond to [previous] difftree raw line\n+# $diffinfo  - hashref of parsed raw diff format\n+# $patchinfo - hashref of parsed patch diff format\n+#              (the same keys as in $diffinfo)\n+sub is_patch_split {\n+\tmy ($diffinfo, $patchinfo) = @_;\n+\n+\treturn defined $diffinfo && defined $patchinfo\n+\t\t&& ($diffinfo->{'to_file'} || $diffinfo->{'file'}) eq $patchinfo->{'to_file'};\n+}\n+\n+\n sub git_difftree_body {\n \tmy ($difftree, $hash, @parents) = @_;\n \tmy ($parent) = $parents[0];\n@@ -2840,13 +2852,7 @@ sub git_difftree_body {\n \tmy $alternate = 1;\n \tmy $patchno = 0;\n \tforeach my $line (@{$difftree}) {\n-\t\tmy $diff;\n-\t\tif (ref($line) eq \"HASH\") {\n-\t\t\t# pre-parsed (or generated by hand)\n-\t\t\t$diff = $line;\n-\t\t} else {\n-\t\t\t$diff = parse_difftree_raw_line($line);\n-\t\t}\n+\t\tmy $diff = parsed_difftree_line($line);\n \n \t\tif ($alternate) {\n \t\t\tprint \"<tr class=\\\"dark\\\">\\n\";\n@@ -3117,10 +3123,12 @@ sub git_patchset_body {\n \tmy ($fd, $difftree, $hash, @hash_parents) = @_;\n \tmy ($hash_parent) = $hash_parents[0];\n \n+\tmy $is_combined = (@hash_parents > 1);\n \tmy $patch_idx = 0;\n \tmy $patch_number = 0;\n \tmy $patch_line;\n \tmy $diffinfo;\n+\tmy $to_name;\n \tmy (%from, %to);\n \n \tprint \"<div class=\\\"patchset\\\">\\n\";\n@@ -3134,73 +3142,46 @@ 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# parse \"git diff\" header line\n+\t\tif ($patch_line =~ m/^diff --git (\\\"(?:[^\\\\\\\"]*(?:\\\\.[^\\\\\\\"]*)*)\\\"|[^ \"]*) (.*)$/) {\n+\t\t\t# $1 is from_name, which we do not use\n+\t\t\t$to_name = unquote($2);\n+\t\t\t$to_name =~ s!^b/!!;\n+\t\t} elsif ($patch_line =~ m/^diff --(cc|combined) (\"?.*\"?)$/) {\n+\t\t\t# $1 is 'cc' or 'combined', which we do not use\n+\t\t\t$to_name = unquote($2);\n+\t\t} else {\n+\t\t\t$to_name = undef;\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 (is_patch_split($diffinfo, { 'to_file' => $to_name })) {\n \t\t\t# this is continuation of a split patch\n \t\t\tprint \"<div class=\\\"patch cont\\\">\\n\";\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 \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\t\t$to_name = $1;\n-\t\t\t}\n-\n-\t\t\tdo {\n-\t\t\t\t# read and prepare patch information\n-\t\t\t\tif (ref($difftree->[$patch_idx]) eq \"HASH\") {\n-\t\t\t\t\t# pre-parsed (or generated by hand)\n-\t\t\t\t\t$diffinfo = $difftree->[$patch_idx];\n-\t\t\t\t} else {\n-\t\t\t\t\t$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);\n-\t\t\t\t}\n+\t\t\t# read and prepare patch information\n+\t\t\t$diffinfo = parsed_difftree_line($difftree->[$patch_idx]);\n \n-\t\t\t\t# check if current raw line has no patch (it got simplified)\n-\t\t\t\tif (defined $to_name && $to_name ne $diffinfo->{'to_file'}) {\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\tif ($is_combined) {\n+\t\t\t\twhile ($to_name ne $diffinfo->{'to_file'}) {\n \t\t\t\t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\" .\n \t\t\t\t\t      format_diff_cc_simplified($diffinfo, @hash_parents) .\n \t\t\t\t\t      \"</div>\\n\";  # class=\"patch\"\n \n \t\t\t\t\t$patch_idx++;\n \t\t\t\t\t$patch_number++;\n+\n+\t\t\t\t\tlast if $patch_idx > $#$difftree;\n+\t\t\t\t\t$diffinfo = parsed_difftree_line($difftree->[$patch_idx]);\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+\t\t\t}\n \n \t\t\t# modifies %from, %to hashes\n \t\t\tparse_from_to_diffinfo($diffinfo, \\%from, \\%to, @hash_parents);\n@@ -3210,30 +3191,36 @@ sub git_patchset_body {\n \t\t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\";\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@@ -3258,16 +3245,11 @@ sub git_patchset_body {\n \n \t# for compact combined (--cc) format, with chunk and patch simpliciaction\n \t# patchset might be empty, but there might be unprocessed raw lines\n-\tfor ($patch_idx++ if $patch_number > 0;\n+\tfor (++$patch_idx if $patch_number > 0;\n \t     $patch_idx < @$difftree;\n-\t     $patch_idx++) {\n+\t     ++$patch_idx) {\n \t\t# read and prepare patch information\n-\t\tif (ref($difftree->[$patch_idx]) eq \"HASH\") {\n-\t\t\t# pre-parsed (or generated by hand)\n-\t\t\t$diffinfo = $difftree->[$patch_idx];\n-\t\t} else {\n-\t\t\t$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);\n-\t\t}\n+\t\t$diffinfo = parsed_difftree_line($difftree->[$patch_idx]);\n \n \t\t# generate anchor for \"patch\" links in difftree / whatchanged part\n \t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\" .\n-- \n1.5.3\n"}]}