This is not for screened yet as it introduces incompatibility in the UI and
thus needs discussion and/or approval.

1 patch for repository http://darcs.net/screened:

patch 9d5fef08e395ad9b9330cd78eee015cac65f47f9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Apr  1 11:48:55 CEST 2017
  * remove the match option hack for clone
  
  Since ancient times the clone command had --to-patch, --to-match, etc but
  actually treated them as --patch, --match etc. This patch fixes that: clone
  now takes the correct options --match, --patch etc. and no longer supports
  --to-whatever.
  Tests have been adapted accordingly.

Attachments

• patch-preview.txt
• remove-the-match-option-hack-for-clone.dpatch
• unnamed

I am of a mind to push this into screened, since there have been no
protests so far. If you think this is a bad idea, speak up!

The --to-XXX and --XXX options always confuse me. My vague intuition is 
that --XXX will select a particular patch, and often implicitly its 
dependencies depending on the command, but nothing else. --to-XXX will 
select a patch, and every patch currently before it in the repository.

Is that intuition right in general, and is that the end result of your 
changes to clone?

> Ganesh Sittampalam <ganesh@earth.li> added the comment:
> 
> The --to-XXX and --XXX options always confuse me. 

You are not alone ;-)

> My vague intuition is 
> that --XXX will select a particular patch, and often implicitly its 
> dependencies depending on the command, but nothing else. --to-XXX will 
> select a patch, and every patch currently before it in the repository.

This is how I always understood it, too. However my understanding is no
less based on intuition as yours (and perhaps, long ago, reading some
documentation). The code that implements the matching is, unfortunately,
rather mysterious to me. (The most mysterious aspect of the patch
matching is what happens when more than one of these matching options
are supplied. I remember that I was regularly surprised that the actual
behavior did not match my intuition when I tried that; I guess I stopped
trying at some point...)

> Is that intuition right in general, and is that the end result of your 
> changes to clone?

I think so. The precise end result depends, of course, on whether our
intuitions about how these options are handled is correct. The only
thing I can claim with confidence is that:

Previously, for the clone command, --xxx was not supported, but --to-xxx
was not, and then --to-xxx was silently converted to --xxx.

With my patch, clone directly accepts --xxx and no longer --to-xxx and
there is no longer any conversion happening between these.

So with this patch, usage of clone --to-xxx must be converted to clone
--xxx and should then behave exactly the same as before. This is what I
have done in the test scripts. On my machine the tests pass.

BTW, systematic tests might be a way to specify our expectations about
what matching options do. We could then see how well our intuition
matches the actual behavior (pun intended).

Cheers
Ben

Sorry for answering so late. 

I have looked at the code and I'm not sure we should accept this patch.

Ben you're saying:

> Previously, for the clone command, --xxx was not supported,
> but --to-xxx was [not], and then --to-xxx was silently converted
> to --xxx.

But the help of 'clone' says:

~~~
You can also make a copy of an untagged state using the `--to-patch` or
`--to-match` options, which exclude patches *after* the first matching
patch.  Because these options treat the set of patches as an ordered
sequence, you may get different results after reordering with `darcs
optimize reorder`.
~~~

It seems to me that internally, the flag conversion is a 'hack' because
of the implementation of clone uses --XXX flags even if they really mean
--to-XXX.

If we want to fix that, we should probably change that implementation so
that it actually uses the --to-XXX flags, except --to-tag which it
should reject and only accept --tag (because of
http://bugs.darcs.net/issue2199 ).

gh: I tested this on an example repo and it appears you are right: even
though the old version converts --to-patch to --patch, what it actually
does is more like what we'd expect from --to-patch. I should have
written (and run) some tests before making this change. I also think
that --to-xxx is the more useful behavior.

Still, there is one aspect of the patch that speaks for it: it /does/
make clone consistent with show files/contents, dist, and annotate.
These all take --patch/--match but what they actually do is --to-patch,
since they use the same function (annotate: getOnePatchset) as clone
does, or a similar one (others: getNonrangeMatch). But I think now that
it is better to change the UI of these other commands (from --xxx to
--to-xxx) rather than clone.

However, the flag list transformation remains a bad hack and I would
rather not duplicate it for the other commands. Instead, the matching
code, especially have/getNonrangeMatch should be fixed to accept the
--to-xxx flags. Note that for none of these commands selecting a single
patch makes much sense. They all refer to some state of the repository,
so they really should use --to-xxx.

Furthermore, I think that (for reasons of consistency and orthogonality)
all of annotate, show files/contents, and dist should accept --context,
too, and clone should support --index (in the sense of --to-index,
perhaps renaming the option) i.e. all should accept the same set of
matching options. We could then re-factor the implementation as well as
the documentation for the match options for these commands.

Note that none of the mentioned commands use patch selection so there is
no worry about selecting one or multiple patches.

So I created http://bugs.darcs.net/issue2546 and screened the following
patch:

1 patch for repository http://darcs.net:

patch 52dc8b8b0922bc3645573f5aced68bd1cc78b8d3
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Mon Sep  4 17:20:52 -03 2017
  * rollback 'remove the match option hack for clone'
  The orginal UI was actually right, while indeed the
  internal matching code used by 'clone' is hackish.

Attachments

• rollback-_remove-the-match-option-hack-for-clone_.dpatch

"Accepting" both patches.