Some quick comments, before my morning coffee:
On Mon, Dec 29, 2008 at 09:54:47PM -0800, John Gardiner Myers wrote:
> Stefan Sperling wrote:
> > On Mon, Dec 29, 2008 at 12:19:00PM -0800, John Gardiner Myers wrote:
> >
> >> Stefan Sperling wrote:
> >>
> >>> 1) Why not just abort on exit code != 0?
> >>>
> >> Because the current behavior of proceeding with the merge, leaving the
> >> conflicts is more useful in the common case.
> >>
> >
> > OK, so we're talking about having three different possibilities:
> >
> > (cases coded by exit code)
> > 0) External tool merges files successfully
> > -> merge proceeds, file is marked as merged
> >
> Correct.
OK.
> > 1) External tool fails merging files, leaving conflict markers
> > -> merge proceeds, file remains in conflict
> >
> This would be reasonable behavior, but is not the current behavior.
:(
> > 2) External tool fails merging files, leaving conflict markers,
> > and wants to abort the merge
> > -> merge is aborted, file remains in conflict
> >
> This is the case I am attempting to add.
OK.
> > What do we do with tools that do exit 2 for some other reason?
> > Not all tools people might want to use with Subversion might be
> > aware of any exit code conventions we might have. Could this
> > be a problem?
> >
> According to the book, Subversion treats 0 and 1 differently than any
> other error code for external diff and diff3 programs. I believe this
> is ample precedent for treating 2 differently than 1.
Right, the book says:
"When it finally does exit, it should return an error code of 0 if the
merge was successful, or 1 if unresolved conflicts remain in the
output -- any other error code is considered a fatal error. When it
finally does exit, it should return an error code of 0 if the merge was
successful, or 1 if unresolved conflicts remain in the output-any other
error code is considered a fatal error."
http://svnbook.red-bean.com/en/1.5/svn.advanced.externaldifftools.html
This punts on the question whether the merge is aborted or continued.
> >> There are two aspects of the current behavior I have issue with:
> >>
> >> a) Why, when running "--accept launch", it doesn't try running the merge
> >> script on subsequent conflicting files.
> >>
> >
> > Do you mean:
> >
> > ... svn doesn't try running the merge script on subsequent conflicting
> > files if the tool exits unsuccessfully (!= 0)
> >
> > ?
> >
> >
> That sentence was in the context of the exit code being != 0, so yes.
OK.
> >> While this would make sense for
> >> SVN_ERR_CL_NO_EXTERNAL_MERGE_TOOL, just because the merge tool wasn't
> >> able to resolve conflicts for one particular file doesn't mean it
> >> wouldn't be able to resolve conflicts for subsequent files in the same
> >> merge.
> >>
> >
> > This statement is confusing me. I thought the current behaviour
> > was just that: Leave the file in conflict and proceed with the merge
> > even though the tool failed to merge the file (exit != 0).
> >
> No, the behavior is to leave the conflict markers in the file, mark that
> file as being resolved,
Ouch, that sounds really wrong...
> and handle any subsequent conflicted files as if
> the merge had been run with "--accept postpone".
Hmm.
> In item (a), I am taking issue with the behavior of handling any
> subsequent conflicted files as if the merge had been run with "--accept
> postpone". While that may be reasonable for the case when no merge tool
> was specified, that makes less sense in the case where the merge tool
> returns an exit code of 1.
Yes.
> >> b) Why it chooses merged instead of postpone for the (first) conflicted
> >> file the merge tool is unable to resolve. This appears to me to be a bug.
> >>
> >
> > Hmmm... what do you mean by "unable to resolve"?
> > Leaving conflict markers in the file but doing exit 0?
> > Or doing exit 1?
> >
> By "unable to resolve", I mean doing anything other than exit 0.
Right.
> > Can you provide a reproduction script that shows the behaviour
> > you are describing? Maybe using /bin/true or /bin/false as fake
> > merge tools?
> >
> I reproduce by creating a merge script that writes a line to stderr and
> does an exit 1 and then doing a "svn merge --accept launch" that creates
> two or more conflicted files.
Can you post the script?
> > As far as I can tell, the current code just cares about "error" or
> > "success". If the tool indicates success (exit 0), it assumes the file
> > was merged correctly. Otherwise, it does not assume so, and the file
> > is left in conflict.
> >
> The first conflicting file is left with conflict markers, but is left in
> "M" status. As I mentioned previously, I believe this to be a bug.
It may well be.
> The remaining conflicting files are left in "C" status. The merge
> script is only ever invoked once.
OK, I think I understand what you're seeing now.
> > I get this impression from reading the code, not from running it,
> > so I might be wrong -- that's why I'm asking for a reproduction script.
> > This would help a lot to clarify what we're talking about.
> >
> None of the existing test scripts cover "svn merge" in combination with
> a merge tool. I am unfamiliar with your test harness framework, so
> creating a whole new class of tests is daunting.
Oh, I am not talking about a python regression test.
A reproduction script is just a unix (or windows) shell script that
creates a repository and runs some svn commands on it to demonstate
a bug. In your case, the script would run some merge using your merge
script, and at the end do 'svn status' or something to show the screwed
up state the files are left in (one M, the others C).
> > That's bad. Please add this information, if you can.
> I would be willing to do so once my patches were accepted, but as the
> book uses a separate update procedure, I am unable to include such
> changes in the patches that I submit here.
Of course, we can handle this separately.
I'm glad you're willing to submit diffs :)
Thanks,
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=995761
Received on 2008-12-30 10:55:19 CET