Re: [PATCH] pre-commit hook: less easily-tripped conflict marker detection

[Junio C Hamano <junkio@cox.net>]

Eric Wong <normalperson@yhbt.net> writes:

> This should make adding asciidoc files to Documentation easier.
>
> Only complain about conflict markers if we see that we have
> some combination of '<<<<<<< ', '>>>>>>> ', and '======='.

Are you sure everybody uses exactly seven?  I did not have SP
after a run of '<' and '>' because I didn't know.

> Also add a NO_VERIFY environment check to this hook, in case
> there's something that we want to force in but still gets
> tripped by this hook.

Hmm.  Undecided.

> @@ -24,8 +25,9 @@ perl -e '
>...
> +    my $in_unresolved;

Unused as far as I can see.

> +    sub show_unresolved {
> +	# if we want even less easily-tripped checks,
> +	# change the "||" to "&&" here.  Right now, we can deal with
> +	# the case where somebody removed one of the <{7} or >{7} lines
> +	# but left the other one (as well as ={7}) in there.

I think '||' is fine as is -- I think <<< and === removed with
>>> left is a common mistake (think of superseding a smallish
"our change" between <<< and === with much larger and polished
upstream "their change" after ===).  But I am not sure about the
code around here:

> +	if (($unresolved[0]->[0] =~ /^<{7} / ||
> +				$unresolved[-1]->[0] =~ /^>{7} /) &&
> +				grep { $_->[0] =~ /^={7}$/ } @unresolved) {
> +	    bad_common();
> +	    foreach my $l (@unresolved) {
> +		print STDERR "* unresolved merge conflict (line $l->[1])\n";
> +		print STDERR "$filename:$l->[1]:$l->[0]\n"
> +	    }
> +	}

 - why check only the first and last element in @unresolved but
   use all of them without checking the ones in-between?

 - if you are keeping the range in an array, maybe the error
   message can point at the range.

Printing $l->[0] is not so useful (the user sees only the
conflict marker) but the original code did so only because it
operated on one-line at a time.

> +	@unresolved = ();
> +    }
> +
>      while (<>) {
>  	if (m|^diff --git a/(.*) b/\1$|) {
>  	    $filename = $1;
> +	    show_unresolved() if @unresolved;
>  	    next;
>  	}
>  	if (/^@@ -\S+ \+(\d+)/) {
> @@ -61,8 +85,8 @@ perl -e '
>  	    if (/^\s* 	/) {
>  		bad_line("indent SP followed by a TAB", $_);
>  	    }
> -	    if (/^(?:[<>=]){7}/) {
> -		bad_line("unresolved merge conflict", $_);
> +	    if (/^[<>]{7} / || /^={7}$/) {
> +		push @unresolved, [ $_, $lineno ];
>  	    }
>  	}
>      }

Maybe you are missing show_unresolved() for the last patch here?