Right now, we get:
  darcs failed:  Refusing to apply patches leading to conflicts.
  If you would rather apply the patch and mark the conflicts,
  use the --mark-conflicts option There are conflicts in the following files:

The basic problem is that darcs push executes darcs apply (i.e. as if on the
command line).  A simple solution might be to capture the output and search for
the text 'If you would rather apply'...

I reviewed this, and I think the "ProbablyEasy" Topic tag could be removed. :)

Because we "exec and ignore"  the darcs apply command as part  of darcs push, we
have no control over the output currently, that I can tell. 

kowey suggested a possible re-design that would involve capturing the output
instead of using exec. It seems tricky to me to piping input into a command and
also capturing the output of it. 

Another approach would be to pass a flag to the remote darcs so that the context
is known, and errors can be reported differently. This wouldn't necessarily have
to be user-visible. Maybe

"--called--remotely"

Of course, this approach has the problem that older darcs will die with message
about an unknown argument, so we would have to either retry without the flag or
bundle this change with one of the others that would require upgraded darcs on
both ends. 

Yet another option is just make "darcs push --verbose" explain what might
happen. This option isn't as nice as actually giving a more useful error message
in the first place.

On Wed, Jan 02, 2008 at 04:07:30AM -0000, Mark Stosberg wrote:
> I reviewed this, and I think the "ProbablyEasy" Topic tag could be
> removed. :)

:)

> Because we "exec and ignore"  the darcs apply command as part  of darcs push, we
> have no control over the output currently, that I can tell. 
> 
> kowey suggested a possible re-design that would involve capturing the output
> instead of using exec. It seems tricky to me to piping input into a command and
> also capturing the output of it. 

Actually, the output is already captured, and returned as a "Doc" object.
See:

 out <- remote_apply opts repodir body
 putDocLn out

Here "out" is the output of the remote apply command.  We could convert
this to a string with renderString and then search for the incorrect text.
It's a pretty fragile solution (breaks if remote darcs outputs some
different error text), but isolated, in that we wouldn't need to pass any
extra data around.

> Another approach would be to pass a flag to the remote darcs so that the context
> is known, and errors can be reported differently. This wouldn't necessarily have
> to be user-visible. Maybe
> 
> "--called--remotely"
> 
> Of course, this approach has the problem that older darcs will die with message
> about an unknown argument, so we would have to either retry without the flag or
> bundle this change with one of the others that would require upgraded darcs on
> both ends. 

Yeah, this is definitely a major downside to this (much cleaner) solution.
We could handle this by adding the flag to apply but not actually using it
until a few darcs releases after we add it.  Or we could add this flag to
the darcs 2.0 and then only use it when we're pushing to a darcs-2
repository.

I think I'd prefer --called-by-push if we used this approach, since the
error message should explicitly recommend something like "pull first and
resolve conflicts before pushing again... and ensure remote repository has
no unrecorded changes" which really only works for when apply is called by
push itself.  If it's called (for instance) by a remote mutt, this flag
wouldn't really be appropriate.

> Yet another option is just make "darcs push --verbose" explain what might
> happen. This option isn't as nice as actually giving a more useful error message
> in the first place.

Yeah, I definitely think we should go with one of the above solutions.
-- 
David Roundy
Department of Physics
Oregon State University

>> Because we "exec and ignore"  the darcs apply command as part  of darcs push, we
>> have no control over the output currently, that I can tell.
>>
>> kowey suggested a possible re-design that would involve capturing the output
>> instead of using exec. It seems tricky to me to piping input into a command and
>> also capturing the output of it.
>
> Actually, the output is already captured, and returned as a "Doc" object.
> See:
>
> out <- remote_apply opts repodir body
> putDocLn out
>
> Here "out" is the output of the remote apply command.  We could convert
> this to a string with renderString and then search for the incorrect text.
> It's a pretty fragile solution (breaks if remote darcs outputs some
> different error text), but isolated, in that we wouldn't need to pass any
> extra data around.

I had found that before I replied, and actually figured out to use
"renderString" and created a special case to capture the error. I tested
it, but the special case didn't get triggered.

That's when I looked up the meaning of 'exec'-- which is to replace the
current process with a new one...I couldn't see how it could be
returning anything in that case, so I figured this approach wasn't
possible.

It was only about 3 lines of code, but unfortunately I threw it away
when the test failed. (And as a beginner Haskeller, I'm embarrassed to
say how long it took to come up with those three lines!).

So, was I wrong: does "out" actually get populated here? If it does,
checking the output this way would be easiest.

Or is there a "think-o" in the code here, expecting that an exec'ed
command returns anything?

     Mark

On Sat, Jan 05, 2008 at 04:23:11PM -0000, Mark Stosberg wrote:
> I had found that before I replied, and actually figured out to use
> "renderString" and created a special case to capture the error. I tested
> it, but the special case didn't get triggered.
> 
> That's when I looked up the meaning of 'exec'-- which is to replace the
> current process with a new one...I couldn't see how it could be
> returning anything in that case, so I figured this approach wasn't
> possible.
> 
> It was only about 3 lines of code, but unfortunately I threw it away
> when the test failed. (And as a beginner Haskeller, I'm embarrassed to
> say how long it took to come up with those three lines!).
>
> So, was I wrong: does "out" actually get populated here? If it does,
> checking the output this way would be easiest.

Yes, this ought to work, our version of exec does something to grab the
output (perhaps redirects it to a file and then reads that file?).
-- 
David Roundy
Department of Physics
Oregon State University

Note:  This should be re-tested, as I changed the exec behavior for push over ssh.

Taking for testing.

I updated "ssh.sh" today to test for this, and also sent a patch to further
clarify the error message. It now looks like this:

darcs failed:  Refusing to apply patches leading to conflicts.
If you would rather apply the patch and mark the conflicts,
use the --mark-conflicts or --allow-conflicts options to apply
These can set as defaults by adding
 apply mark-conflicts
to _darcs/prefs/defaults in the target repo.
Backing up ./a(-darcs-backup0)
There are conflicts in the following files:
./a
Apply failed!

Anticipating patch acceptance, I'm marking this as resolved-in-unstable.