svn commit: r19082 - trunk/contrib/hook-scripts

On 3/28/06, dandersontigris.org <dandersontigris.org> wrote:
> Author: danderson
> Date: Tue Mar 28 18:27:10 2006
> New Revision: 19082
>
> Added:
>    trunk/contrib/hook-scripts/detect-merge-conflicts.sh
  (contents, props changed)
>
> Log:
> Add a contrib pre-commit hook script that aborts the
commit if it
> finds things that look like merge conflict markers in
the commit diff.

My concern with this pre-commit hook is that it'd not allow
itself to
be committed.  =P

Just in case people wanted to install it on
svn.collab.net...  -- justin

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

Justin Erenkrantz wrote:
> On 3/28/06, dandersontigris.org
<dandersontigris.org> wrote:
> 
>>Author: danderson
>>Date: Tue Mar 28 18:27:10 2006
>>New Revision: 19082
>>
>>Added:
>>  
trunk/contrib/hook-scripts/detect-merge-conflicts.sh  
(contents, props changed)
>>
>>Log:
>>Add a contrib pre-commit hook script that aborts the
commit if it
>>finds things that look like merge conflict markers
in the commit diff.
> 
> My concern with this pre-commit hook is that it'd not
allow itself to
> be committed.  =P

It would, since it doesn't contain a line starting with
seven conflict mark 
characters.  But...

It won't allow many of our README and similar text files to
be committed, and 
they don't contain context markers but just
arbitrary-length lines of '=' 
characters.  This hook script ought to be more strict in
what it considers to 
be a conflict marker: after the seven marks should come a
space (for '<' and 
'>') or EOL (for '=').  (I've just checked and that
is true for standard 
"diff3" output as well as "svn"
output.)  That should significantly decrease 
the false-reject rate, but a line consisting of seven '='
characters will still 
occur reasonably often (e.g. in our
contrib/client-side/svncopy.README), so 
perhaps just looking for the '<' and '>' types of
lines would be best.

It won't allow some of our test files (e.g. 
subversion/tests/cmdline/trans_tests.py) which intentionally
contain real 
conflict markers.  I don't see any practical way to avoid
this.  (Obviously, 
tests could be re-written to not include real conflict
markers directly, but 
that's not a general solution.)  It shouldn't be a real
possibility in most 
people's repositories, only for people who are developing
tools that deal with 
"patch" and "merge" behaviour, and
those people (including us) can avoid using 
this script.

Both of these points would be less important if there were a
way to over-ride 
the hook's decision.


>   echo "Some parts of your commit look
suspiciously like merge" >&2
>   echo "conflict markers.  Please double-check
your diff and try" >&2
>   echo "committing again." >&2
>   exit 1

I think this message would be better if it made clear that
simply checking and 
trying again won't get you any further: you need to remove
the changes that 
look like conflict markers.  In order to help the user in
awkward situations, 
and remembering that the user will usually not have access
to the script's 
source code to discover exactly what it dislikes, it would
be worth this error 
message being a bit more specific about what it found and/or
where it found it, 
e.g. printing the first line that it encountered that it
didn't like.

- Julian

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

* Julian Foad <julianfoadbtopenworld.com>
[2006-03-29 05:11:41]:
> It won't allow many of our README and similar text
files to be committed,
> and they don't contain context markers but just
arbitrary-length lines of
> '=' characters.  This hook script ought to be more
strict in what it
> considers to be a conflict marker: after the seven
marks should come a
> space (for '<' and '>') or EOL (for '='). 
(I've just checked and that is
> true for standard "diff3" output as well as
"svn" output.)  That should
> significantly decrease the false-reject rate, but a
line consisting of
> seven '=' characters will still occur reasonably
often (e.g. in our
> contrib/client-side/svncopy.README), so perhaps just
looking for the '<'
> and '>' types of lines would be best.

Maybe, but does that catch enough cases?  The point was to
catch all
suspicious commits, not a subset of them.

Also, note that the script only catches adds of suspicious
lines, so
working inside a README, or inside a test script that has
explicit
conflict markers embedded is no problem.  Once a commit has
been
accepted, content is ignored by the script. However,
touching section
headers, or the part of the script that contains the
markers, is
indeed a problem.

> Both of these points would be less important if there
were a way to
> over-ride the hook's decision.

I was thinking of including such a mechanism, using a marker
given in
the commit log message.  I didn't include any because I'm
not sure
what format this marker could have to be as unintrusive as
possible,
but adding such a check is rather trivial.  If a commit log
marker
seems satisfactory, I'll commit an update to the script to
do that.

> >  echo "Some parts of your commit look
suspiciously like merge" >&2
> >  echo "conflict markers.  Please
double-check your diff and try" >&2
> >  echo "committing again." >&2
> >  exit 1
>
> I think this message would be better if it made clear
that simply checking
> and trying again won't get you any further: you need
to remove the changes
> that look like conflict markers.  In order to help the
user in awkward
> situations, and remembering that the user will usually
not have access to
> the script's source code to discover exactly what it
dislikes, it would be
> worth this error message being a bit more specific
about what it found
> and/or where it found it, e.g. printing the first line
that it encountered
> that it didn't like.

Right.  The first iteration of this script actually took the
output of
svnlook changed, and then scanned each file's diff
individually, so
that it could say "Something wrong in
foo.c:493". The downside is that
it makes the script a little more complex (though not much
when it
comes down to it).

If you have no further objections wrt. my reply, I'll
commit an update
to the script, to include bypassing capability as well as a
clarified
and verbose error (or submit it as a patch here for review,
if you
prefer?).

- Dave.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

David Anderson wrote:
> * Julian Foad <julianfoadbtopenworld.com>
[2006-03-29 05:11:41]:
> 
>>It won't allow many of our README and similar text
files to be committed,
>>and they don't contain context markers but just
arbitrary-length lines of
>>'=' characters.  This hook script ought to be more
strict in what it
>>considers to be a conflict marker: after the seven
marks should come a
>>space (for '<' and '>') or EOL (for
'=').  (I've just checked and that is
>>true for standard "diff3" output as well
as "svn" output.)  That should
>>significantly decrease the false-reject rate, but a
line consisting of
>>seven '=' characters will still occur reasonably
often (e.g. in our
>>contrib/client-side/svncopy.README), so perhaps just
looking for the '<'
>>and '>' types of lines would be best.
> 
> Maybe, but does that catch enough cases?  The point was
to catch all
> suspicious commits, not a subset of them.

It would still catch all commits that include conflicts that
haven't been 
noticed.  It would only fail to catch ones that have been
partly resolved 
(removing the "<<<<<<<" and
">>>>>>>" but leaving the
"=======") and I think 
that's absolutely fine because if someone incorrectly
resolves a conflict there 
are unlimited ways for them to get it wrong and failing to
detect this 
particular way doesn't matter.

> Also, note that the script only catches adds of
suspicious lines, so
> working inside a README, or inside a test script that
has explicit
> conflict markers embedded is no problem.  Once a commit
has been
> accepted, content is ignored by the script. However,
touching section
> headers, or the part of the script that contains the
markers, is
> indeed a problem.

Yes, I realised that.


>>Both of these points would be less important if
there were a way to
>>over-ride the hook's decision.
> 
> I was thinking of including such a mechanism, using a
marker given in
> the commit log message.  I didn't include any because
I'm not sure
> what format this marker could have to be as unintrusive
as possible,
> but adding such a check is rather trivial.  If a commit
log marker
> seems satisfactory, I'll commit an update to the
script to do that.

That would be one way.  Other, half-baked ideas include:

* Have the user set a revision property to indicate
"ignore conflict markers in 
this commit", and have the script obey this and then
perhaps remove the 
property before allowing the commit.  Problem: I don't
think we've a user 
interface for providing arbitrary rev-props on a commit.

* running something like this as a post-commit warning
rather than a pre-commit 
blocking mode.

* trying to make it remember the fact that this user tried
the commit, and 
allow a second attempt by the same user (if within a certain
time, and/or if 
with the same set of files affected, and/or ...)


>>> echo "Some parts of your commit look
suspiciously like merge" >&2
>>> echo "conflict markers.  Please
double-check your diff and try" >&2
>>> echo "committing again." >&2
>>> exit 1
>>
>>I think this message would be better if it made
clear that simply checking
>>and trying again won't get you any further: you
need to remove the changes
>>that look like conflict markers.  In order to help
the user in awkward
>>situations, and remembering that the user will
usually not have access to
>>the script's source code to discover exactly what
it dislikes, it would be
>>worth this error message being a bit more specific
about what it found
>>and/or where it found it, e.g. printing the first
line that it encountered
>>that it didn't like.
> 
> Right.  The first iteration of this script actually
took the output of
> svnlook changed, and then scanned each file's diff
individually, so
> that it could say "Something wrong in
foo.c:493". The downside is that
> it makes the script a little more complex (though not
much when it
> comes down to it).
> 
> If you have no further objections wrt. my reply, I'll
commit an update
> to the script, to include bypassing capability as well
as a clarified
> and verbose error (or submit it as a patch here for
review, if you
> prefer?).

It seems there's no obvious single solution, so it might be
a good idea to post 
here first.

- Julian

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org