Eric Wong [off-list ref] 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.
quoted hunk
@@ -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
quoted
quoted
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.
quoted hunk
+ @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?
Junio C Hamano [off-list ref] wrote:
Eric Wong [off-list ref] writes:
quoted
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.
That's what rerere checks for. I'm not sure about other programs
leave 3-way merge markers besides merge(1).
quoted
@@ -24,8 +25,9 @@ perl -e '
...
+ my $in_unresolved;
Unused as far as I can see.
Oops, I was planning to store the entire hunk in @unresolved, but
decided line numbers were enough.
quoted
+ 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
quoted
quoted
quoted
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:
My code won't detect when <<< and === are removed with only >>>
left. I didn't think it was a common mistake at all.
quoted
+ 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?
I assume that any partially finished resolutions would either start with
<<< or end with >>>. I don't actually know which are the most oftenly
made mistakes when accidentally committing merge-conflicts.
- 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.
I agree. I was thinking about adding entire hunks with $in_unresolved,
but forgot or decided against it (probably out of laziness, as it was
late when I did this).
quoted
+ @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?
Oops, good catch.
<finally, moved from above>:
quoted
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.
At this point, I think this is probably the best change to make. There
are many things that a user could do that an automated checker could
miss, and there are also many things that it could be overchecking for.
- if (/^(?:[<>=]){7}/) {
+ if (/^[<>]{7} / || /^={7}$/) {
I would also make this change, because I'm pretty certain 7 characters
(and one space for [<>]) is standard for merge(1). We already rely on
that for rerere.
--
Eric Wong
Eric Wong [off-list ref] writes:
quoted
Hmm. Undecided.
At this point, I think this is probably the best change to make. There
are many things that a user could do that an automated checker could
miss, and there are also many things that it could be overchecking for.
Agreed to the latter part of the last sentence. Undecided about
the rest and the implementation.
- if (/^(?:[<>=]){7}/) {
+ if (/^[<>]{7} / || /^={7}$/) {
I would also make this change, because I'm pretty certain 7 characters
(and one space for [<>]) is standard for merge(1). We already rely on
that for rerere.
One of the things we might want is to use diff3 instead of merge
but I presume the latter is a thin wrapper around the former so
that would be OK. I am however not enthused about the @unresolved
array approach.