{"thread":{"id":"4677","subject":"[PATCH] pre-commit hook: less easily-tripped conflict marker detection","startedAt":"2006-06-27T09:06:17Z","lastAt":"2006-06-28T01:58:11Z","messageCount":4,"participants":["Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"22652","messageId":"11513991771758-git-send-email-normalperson@yhbt.net","threadId":"4677","inReplyTo":null,"subject":"[PATCH] pre-commit hook: less easily-tripped conflict marker detection","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2006-06-27T09:06:17Z","receivedAt":"2006-06-27T09:06:17Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This should make adding asciidoc files to Documentation easier.\n\nOnly complain about conflict markers if we see that we have\nsome combination of '<<<<<<< ', '>>>>>>> ', and '======='.\n\nAlso add a NO_VERIFY environment check to this hook, in case\nthere's something that we want to force in but still gets\ntripped by this hook.  It'd be a lot more work to add\n--no-verify flags to all things that could potentially call this\nhook.\n\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n templates/hooks--pre-commit |   32 ++++++++++++++++++++++++++++----\n 1 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit\nindex 723a9ef..1657f96 100644\n--- a/templates/hooks--pre-commit\n+++ b/templates/hooks--pre-commit\n@@ -1,4 +1,5 @@\n #!/bin/sh\n+test -n \"$NO_VERIFY\" && exit 0\n #\n # An example hook script to verify what is about to be committed.\n # Called by git-commit with no arguments.  The hook should\n@@ -24,8 +25,9 @@ perl -e '\n     my $filename;\n     my $reported_filename = \"\";\n     my $lineno;\n-    sub bad_line {\n-\tmy ($why, $line) = @_;\n+    my $in_unresolved;\n+    my @unresolved;\n+    sub bad_common {\n \tif (!$found_bad) {\n \t    print STDERR \"*\\n\";\n \t    print STDERR \"* You have some suspicious patch lines:\\n\";\n@@ -36,12 +38,34 @@ perl -e '\n \t    print STDERR \"* In $filename\\n\";\n \t    $reported_filename = $filename;\n \t}\n+    }\n+    sub bad_line {\n+\tmy ($why, $line) = @_;\n+\tbad_common();\n \tprint STDERR \"* $why (line $lineno)\\n\";\n \tprint STDERR \"$filename:$lineno:$line\\n\";\n     }\n+    sub show_unresolved {\n+\t# if we want even less easily-tripped checks,\n+\t# change the \"||\" to \"&&\" here.  Right now, we can deal with\n+\t# the case where somebody removed one of the <{7} or >{7} lines\n+\t# but left the other one (as well as ={7}) in there.\n+\tif (($unresolved[0]->[0] =~ /^<{7} / ||\n+\t\t\t\t$unresolved[-1]->[0] =~ /^>{7} /) &&\n+\t\t\t\tgrep { $_->[0] =~ /^={7}$/ } @unresolved) {\n+\t    bad_common();\n+\t    foreach my $l (@unresolved) {\n+\t\tprint STDERR \"* unresolved merge conflict (line $l->[1])\\n\";\n+\t\tprint STDERR \"$filename:$l->[1]:$l->[0]\\n\"\n+\t    }\n+\t}\n+\t@unresolved = ();\n+    }\n+\n     while (<>) {\n \tif (m|^diff --git a/(.*) b/\\1$|) {\n \t    $filename = $1;\n+\t    show_unresolved() if @unresolved;\n \t    next;\n \t}\n \tif (/^@@ -\\S+ \\+(\\d+)/) {\n@@ -61,8 +85,8 @@ perl -e '\n \t    if (/^\\s* \t/) {\n \t\tbad_line(\"indent SP followed by a TAB\", $_);\n \t    }\n-\t    if (/^(?:[<>=]){7}/) {\n-\t\tbad_line(\"unresolved merge conflict\", $_);\n+\t    if (/^[<>]{7} / || /^={7}$/) {\n+\t\tpush @unresolved, [ $_, $lineno ];\n \t    }\n \t}\n     }\n-- \n1.4.1.rc1.g2faf-dirty\n"},{"id":"22680","messageId":"7vodwe8qbc.fsf@assigned-by-dhcp.cox.net","threadId":"4677","inReplyTo":"11513991771758-git-send-email-normalperson@yhbt.net","subject":"Re: [PATCH] pre-commit hook: less easily-tripped conflict marker detection","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-27T17:24:23Z","receivedAt":"2006-06-27T17:24:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> This should make adding asciidoc files to Documentation easier.\n>\n> Only complain about conflict markers if we see that we have\n> some combination of '<<<<<<< ', '>>>>>>> ', and '======='.\n\nAre you sure everybody uses exactly seven?  I did not have SP\nafter a run of '<' and '>' because I didn't know.\n\n> Also add a NO_VERIFY environment check to this hook, in case\n> there's something that we want to force in but still gets\n> tripped by this hook.\n\nHmm.  Undecided.\n\n> @@ -24,8 +25,9 @@ perl -e '\n>...\n> +    my $in_unresolved;\n\nUnused as far as I can see.\n\n> +    sub show_unresolved {\n> +\t# if we want even less easily-tripped checks,\n> +\t# change the \"||\" to \"&&\" here.  Right now, we can deal with\n> +\t# the case where somebody removed one of the <{7} or >{7} lines\n> +\t# but left the other one (as well as ={7}) in there.\n\nI think '||' is fine as is -- I think <<< and === removed with\n>>> left is a common mistake (think of superseding a smallish\n\"our change\" between <<< and === with much larger and polished\nupstream \"their change\" after ===).  But I am not sure about the\ncode around here:\n\n> +\tif (($unresolved[0]->[0] =~ /^<{7} / ||\n> +\t\t\t\t$unresolved[-1]->[0] =~ /^>{7} /) &&\n> +\t\t\t\tgrep { $_->[0] =~ /^={7}$/ } @unresolved) {\n> +\t    bad_common();\n> +\t    foreach my $l (@unresolved) {\n> +\t\tprint STDERR \"* unresolved merge conflict (line $l->[1])\\n\";\n> +\t\tprint STDERR \"$filename:$l->[1]:$l->[0]\\n\"\n> +\t    }\n> +\t}\n\n - why check only the first and last element in @unresolved but\n   use all of them without checking the ones in-between?\n\n - if you are keeping the range in an array, maybe the error\n   message can point at the range.\n\nPrinting $l->[0] is not so useful (the user sees only the\nconflict marker) but the original code did so only because it\noperated on one-line at a time.\n\n> +\t@unresolved = ();\n> +    }\n> +\n>      while (<>) {\n>  \tif (m|^diff --git a/(.*) b/\\1$|) {\n>  \t    $filename = $1;\n> +\t    show_unresolved() if @unresolved;\n>  \t    next;\n>  \t}\n>  \tif (/^@@ -\\S+ \\+(\\d+)/) {\n> @@ -61,8 +85,8 @@ perl -e '\n>  \t    if (/^\\s* \t/) {\n>  \t\tbad_line(\"indent SP followed by a TAB\", $_);\n>  \t    }\n> -\t    if (/^(?:[<>=]){7}/) {\n> -\t\tbad_line(\"unresolved merge conflict\", $_);\n> +\t    if (/^[<>]{7} / || /^={7}$/) {\n> +\t\tpush @unresolved, [ $_, $lineno ];\n>  \t    }\n>  \t}\n>      }\n\nMaybe you are missing show_unresolved() for the last patch here?\n"},{"id":"22689","messageId":"20060627223226.GA10178@soma","threadId":"4677","inReplyTo":"7vodwe8qbc.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] pre-commit hook: less easily-tripped conflict marker detection","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2006-06-27T22:32:26Z","receivedAt":"2006-06-27T22:32:26Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> Eric Wong <normalperson@yhbt.net> writes:\n> \n> > This should make adding asciidoc files to Documentation easier.\n> >\n> > Only complain about conflict markers if we see that we have\n> > some combination of '<<<<<<< ', '>>>>>>> ', and '======='.\n> \n> Are you sure everybody uses exactly seven?  I did not have SP\n> after a run of '<' and '>' because I didn't know.\n\nThat's what rerere checks for.  I'm not sure about other programs\nleave 3-way merge markers besides merge(1).\n\n> > @@ -24,8 +25,9 @@ perl -e '\n> >...\n> > +    my $in_unresolved;\n> \n> Unused as far as I can see.\n\nOops, I was planning to store the entire hunk in @unresolved, but\ndecided line numbers were enough.\n\n> > +    sub show_unresolved {\n> > +\t# if we want even less easily-tripped checks,\n> > +\t# change the \"||\" to \"&&\" here.  Right now, we can deal with\n> > +\t# the case where somebody removed one of the <{7} or >{7} lines\n> > +\t# but left the other one (as well as ={7}) in there.\n> \n> I think '||' is fine as is -- I think <<< and === removed with\n> >>> left is a common mistake (think of superseding a smallish\n> \"our change\" between <<< and === with much larger and polished\n> upstream \"their change\" after ===).  But I am not sure about the\n> code around here:\n\nMy code won't detect when <<< and === are removed with only >>>\nleft.  I didn't think it was a common mistake at all.\n\n> > +\tif (($unresolved[0]->[0] =~ /^<{7} / ||\n> > +\t\t\t\t$unresolved[-1]->[0] =~ /^>{7} /) &&\n> > +\t\t\t\tgrep { $_->[0] =~ /^={7}$/ } @unresolved) {\n> > +\t    bad_common();\n> > +\t    foreach my $l (@unresolved) {\n> > +\t\tprint STDERR \"* unresolved merge conflict (line $l->[1])\\n\";\n> > +\t\tprint STDERR \"$filename:$l->[1]:$l->[0]\\n\"\n> > +\t    }\n> > +\t}\n> \n>  - why check only the first and last element in @unresolved but\n>    use all of them without checking the ones in-between?\n\nI assume that any partially finished resolutions would either start with\n<<< or end with >>>.  I don't actually know which are the most oftenly\nmade mistakes when accidentally committing merge-conflicts.\n\n>  - if you are keeping the range in an array, maybe the error\n>    message can point at the range.\n> \n> Printing $l->[0] is not so useful (the user sees only the\n> conflict marker) but the original code did so only because it\n> operated on one-line at a time.\n\nI agree.  I was thinking about adding entire hunks with $in_unresolved,\nbut forgot or decided against it (probably out of laziness, as it was\nlate when I did this).\n\n> > +\t@unresolved = ();\n> > +    }\n> > +\n> >      while (<>) {\n> >  \tif (m|^diff --git a/(.*) b/\\1$|) {\n> >  \t    $filename = $1;\n> > +\t    show_unresolved() if @unresolved;\n> >  \t    next;\n> >  \t}\n> >  \tif (/^@@ -\\S+ \\+(\\d+)/) {\n> > @@ -61,8 +85,8 @@ perl -e '\n> >  \t    if (/^\\s* \t/) {\n> >  \t\tbad_line(\"indent SP followed by a TAB\", $_);\n> >  \t    }\n> > -\t    if (/^(?:[<>=]){7}/) {\n> > -\t\tbad_line(\"unresolved merge conflict\", $_);\n> > +\t    if (/^[<>]{7} / || /^={7}$/) {\n> > +\t\tpush @unresolved, [ $_, $lineno ];\n> >  \t    }\n> >  \t}\n> >      }\n> \n> Maybe you are missing show_unresolved() for the last patch here?\n\nOops, good catch.\n\n<finally, moved from above>:\n> > Also add a NO_VERIFY environment check to this hook, in case\n> > there's something that we want to force in but still gets\n> > tripped by this hook.\n> \n> Hmm.  Undecided.\n\nAt this point, I think this is probably the best change to make.  There\nare many things that a user could do that an automated checker could\nmiss, and there are also many things that it could be overchecking for.\n\n\n-\t    if (/^(?:[<>=]){7}/) {\n+\t    if (/^[<>]{7} / || /^={7}$/) {\n\nI would also make this change, because I'm pretty certain 7 characters\n(and one space for [<>]) is standard for merge(1).  We already rely on\nthat for rerere.\n\n-- \nEric Wong\n"},{"id":"22698","messageId":"7vhd2659e4.fsf@assigned-by-dhcp.cox.net","threadId":"4677","inReplyTo":"20060627223226.GA10178@soma","subject":"Re: [PATCH] pre-commit hook: less easily-tripped conflict marker detection","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-28T01:58:11Z","receivedAt":"2006-06-28T01:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n>> Hmm.  Undecided.\n>\n> At this point, I think this is probably the best change to make.  There\n> are many things that a user could do that an automated checker could\n> miss, and there are also many things that it could be overchecking for.\n\nAgreed to the latter part of the last sentence.  Undecided about\nthe rest and the implementation.\n\n>\n> -\t    if (/^(?:[<>=]){7}/) {\n> +\t    if (/^[<>]{7} / || /^={7}$/) {\n>\n> I would also make this change, because I'm pretty certain 7 characters\n> (and one space for [<>]) is standard for merge(1).  We already rely on\n> that for rerere.\n\nOne of the things we might want is to use diff3 instead of merge\nbut I presume the latter is a thin wrapper around the former so\nthat would be OK.  I am however not enthused about the @unresolved\narray approach.\n"}]}