Somewhat related cleanups for apply and pull commands (including their
rebase versions).

3 patches for repository http://darcs.net/screened:

patch 106dd1e2c1792c2c6b1a167107acc721ac3fe624
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug  8 10:26:51 CEST 2019
  * re-unify finished messages in PatchAppliers
  
  We make a small simplification in the process: fetchPatches already reports
  if there are no patches to pull, so we needn't repeat that.

patch 1325eec9182cccc6826453a709cbf336385ca754
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 15 10:23:00 CEST 2019
  * cleanup applyCmd

patch 24197f876a01166aed000ad7f8090c5386d556e3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 15 10:23:43 CEST 2019
  * apply: print "reading from stdin" unless --quiet
  
  The message is indeed useful to users, since with no arguments 'darcs apply'
  will "hang" waiting for input from the terminal. But printing it only if
  --verbose is in effect defeats the purpose.

Attachments

• patch-preview.txt
• re_unify-finished-messages-in-patchappliers.dpatch
• unnamed

>  * re-unify finished messages in PatchAppliers

>  We make a small simplification in the process: fetchPatches already
>  reports if there are no patches to pull, so we needn't repeat that.

I don't think this is true. fetchPatches only reports that if the options
are NoReorder. But the text you've removed only applies when reordering is
happening.

I didn't check exactly without and with this patch, but I think it
turns the message in that case from "Nothing to pull, finished reordering."
into "Finished reordering."

I don't feel strongly that the former is superior though.

The code that calls putFinished in Rebase.hs and in ApplyPatches.hs now
looks identical, so perhaps some sharing is possible.

>>  * re-unify finished messages in PatchAppliers
> 
>>  We make a small simplification in the process: fetchPatches already
>>  reports if there are no patches to pull, so we needn't repeat that.
> 
> I don't think this is true. fetchPatches only reports that if the options
> are NoReorder. But the text you've removed only applies when reordering is
> happening.

I meant the message "No remote patches to pull in!" that fetchPatches
outputs, independent from any re-ordering options. Before this change I
was getting

> darcs pull
No remote patches to pull in!
Nothing to pull, finished reordering.

> I didn't check exactly without and with this patch, but I think it
> turns the message in that case from "Nothing to pull, finished reordering."
> into "Finished reordering."

Exactly, because we already informed the user that there are not patches
to pull. What you get now is

> darcs pull --reorder-patches
No remote patches to pull in!
Finished reordering.

and I think that is perfectly fine.

BTW, something similar is happening for apply:

> darcs apply use-darcs_util_graph_components-for-repopatchv3.dpatch
Finished applying.
> darcs apply use-darcs_util_graph_components-for-repopatchv3.dpatch
All these patches have already been applied.  Nothing to do.
> darcs apply use-darcs_util_graph_components-for-repopatchv3.dpatch
--reorder-patches
All these patches have already been applied.  Nothing to do.
Finished reordering.

(The "Nothing to do." is not exactly correct with --reorder-patches.)

On 25/08/2019 23:52, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>>>  * re-unify finished messages in PatchAppliers
>>
>>>  We make a small simplification in the process: fetchPatches already
>>>  reports if there are no patches to pull, so we needn't repeat that.
>>
>> I don't think this is true. fetchPatches only reports that if the options
>> are NoReorder. But the text you've removed only applies when reordering is
>> happening.
> 
> I meant the message "No remote patches to pull in!" that fetchPatches
> outputs, independent from any re-ordering options. Before this change I
> was getting
> 
>> darcs pull
> No remote patches to pull in!
> Nothing to pull, finished reordering.
> 
>> I didn't check exactly without and with this patch, but I think it
>> turns the message in that case from "Nothing to pull, finished reordering."
>> into "Finished reordering."
> 
> Exactly, because we already informed the user that there are not patches
> to pull. What you get now is
> 
>> darcs pull --reorder-patches
> No remote patches to pull in!
> Finished reordering.
> 
> and I think that is perfectly fine.

Ah, we're talking about two different scenarios. You're talking about
what happens when there's nothing new in the remote repo. I was looking
at the one where there are remote patches to offer but the user doesn't
select anything, which triggers the "You don't want to pull any patches"
message. That one is conditionally disabled with --reorder so in this
scenario you now end up with "Finished reordering." and nothing more.

> Ah, we're talking about two different scenarios. You're talking about
> what happens when there's nothing new in the remote repo. I was looking
> at the one where there are remote patches to offer but the user doesn't
> select anything, which triggers the "You don't want to pull any patches"
> message. That one is conditionally disabled with --reorder so in this
> scenario you now end up with "Finished reordering." and nothing more.

Okay, understood. Thanks for clearing that up.

I think the solution is to re-enable the "You don't want to pull any
patches" regardless of whether --reorder is in effect. This would keep
the two functionalities (reordering patches and pulling/applying)
orthogonal instead of coupling them.

> I think the solution is to re-enable the "You don't want to pull any
> patches" regardless of whether --reorder is in effect. This would keep
> the two functionalities (reordering patches and pulling/applying)
> orthogonal instead of coupling them.

That makes sense.

2 patches for repository http://darcs.net/screened:

patch 20b57d5e6094019a3b4f5aa3a0ea561a7c3ab3a4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 29 18:36:55 CEST 2019
  * share common start and finish parts in PatchAppliers

patch b6c7ad3ba858f0d7030f31263d4449a0717676f4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 29 18:36:55 CEST 2019
  * issue no-patches message in applyPatchesStart regardless of --reorder

Attachments

• patch-preview.txt
• share-common-start-and-finish-parts-in-patchappliers.dpatch
• unnamed

Can you take a look at my follow-ups? I have not yet pushed them to
screened in case I made some stupid mistakes.

I can't spot anything obvious wrong in the preview, I think it's fine to
screen.

I'll take a slightly closer look locally with a diff tool to check the logic 
matches up when I finish the review.

>  * re-unify finished messages in PatchAppliers
>  * share common start and finish parts in PatchAppliers
>  * issue no-patches message in applyPatchesStart regardless of --reorder

This logic looks pretty clean now, thanks. I wonder if applyPatchesStart
and applyPatchesFinish should be some kind of bracketed operation
applyPatchesWith instead, but that's not important.

>  * cleanup applyCmd
>  * apply: print "reading from stdin" unless --quiet

Both fine.