[PATCH evolve_ext V2] fold: take an explicit list of revisions (BC)

[Jordi Gutiérrez Hermoso]

On Thu, 2014-06-26 at 08:41 -0400, Greg Ward wrote:
> On 24 June 2014, Jordi Gutiérrez Hermoso said:

> > fold: rehaul handling of multiple and single revisions (BC)
>         ^^^^^^
> 
> "overhaul"

Oops, how did I think that "rehaul" was a word?

> *IF* we accept Pierre-Yves' judgement that "specified revision and all
> of its descendants up to ." should be the default, then there is no
> need to accept multiple revisions. 

There is, actually, because one thing you might reasonably want to do
is `hg fold -r 'draft()'` or other revsets that might result in
multiple revisions.

> > +    If specifying multiple revisions, use --exact for folding those
> > +    revisions while ignoring the parent of the working directory. In
> > +    this case, the given revisions must form a linear unbroken chain.
> 
> No. Yuck. Confusing. Do not like.

Right, I get this is contentious. Pierre-Yves actually won me over to
his side when he suggested how people would want to use revsets to
fold. Sadly, we can't afford usability studies to decide which way to
go about this. Polling on the mailing list only results in
self-selected respondents who are already a very minority kind of hg
user (i.e. the kind that subscribes to mailing lists).

I really wish we had a general method for deciding these usability
debates, because arguing on the mailing list about them is kind of
pointless.

> >      """
> >      revs = list(revs)
> > -    if revs:
> > -        if opts.get('rev', ()):
> > -            raise util.Abort("cannot specify both --rev and a target revision")
> > -        targets = scmutil.revrange(repo, revs)
> > -        revs = repo.revs('(%ld::.) or (.::%ld)', targets, targets)
> > -    elif 'rev' in opts:
> > -        revs = scmutil.revrange(repo, opts['rev'])
> > -    else:
> > -        revs = ()
> > +    revs.extend(opts['rev'])
> 
> Assuming this is the best way to implement "-r before REV is
> optional", all you need to add is
> 
>   if len(revs) > 1:
>       raise util.Abort(_('too many revisions specified'))
>   rev = revs[0]
> 
> and then forget about handling multiple revs.

As I said above, I really do think we need to handle multiple
revisions, due to revsets.

> >      if not revs:
> > -        ui.write_err('no revision to fold\n')
> > +        ui.write_err(_('no revisions specified\n'))
> >          return 1
> 
> FWIW, graft just aborts in this case. (But see my comment below about
> mixing multiple changes together: you're doing it here.)

Oh, I suppose strip does too. I can amend this to abort. I was just
following the intent of the original, without really thinking about
it.

> You're mixing multiple changes together. Improving error messages,
> adding hints, and I18N are all good things ... but they are distinct
> things from "overhaul handling of revisions".

Blegh, I hate splitting this. The error messages are related to the
new functionality. The new functionality needs to go all at once. If I
make --rev optional, then I would create an intermediate broken commit
where you cannot work without folding with ".". I could also make
intermediate pointless commits that don't use i18n in the error
messages, but why would you want non-i18n messages to begin with? It's
not like adding _() is that big of a change.

I refuse to create intermediate broken states, so I refuse to split
this patch. I also refuse to create artificial working states that do
not reflect the direction that this patch is taking. I do not see
logical boundaries where individual parts of this patch could work
without the rest. Nor do I see a need to do so.

It's a large patch, and sorry it's difficult to review it all at once,
but I feel it's as atomic as it can be for such a large BC.

- Jordi G. H.