[PATCH 3 of 3 evolve-ext V2] commands: introduce a new command to edit commit metadata

Siddharth Agarwal sid at less-broken.com
Fri Apr 15 20:14:07 UTC 2016


Following up on this --

On 3/27/16 15:14, Pierre-Yves David wrote:

> On 03/19/2016 10:02 PM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0 at fb.com>
>> # Date 1458450093 25200
>> #      Sat Mar 19 22:01:33 2016 -0700
>> # Branch stable
>> # Node ID 6570300a9b764dacb8c5551aebb55251153787c1
>> # Parent  62cee591d6b306cfa8d1f30acbcc4cae972d79ff
>> commands: introduce a new command to edit commit metadata
>
> This patches is both a bit too big and a bit too light. I would prefer 
> to have a minimal first patches for this command and then add further 
> feature (and testing). In addition, I would like the command to 
> support multiple revision before we take a --fold flag. I would like 
> to avoid just getting `hg fold` under another name here.
>
> I would expect to see something similar to:
> - patch 1: add a `hg metaedit` command,
> - patch 2: allow multiple revisions with `hg metaedit`,

I'm really not inclined to do this right now -- can I do something like 
"accept multiple revisions with `hg metaedit` but abort"?

> - patch 3: add a --fold flag,
>
>> This patch introduces metaedit, a command to metadata of a set of 
>> revisions
>> without updating working copy. It also supports folding a stack in 
>> the same
>> operation. This is particularly relevant for repository where 
>> changing the
>> parent of the working copy is time-consuming. We could add more stack
>> manipulation operations to metaedit in the future.
>>
>> Crucially, it is different from 'hg fold --exact' in that it also allows
>> 'folding' a single commit and rewriting its metadata. This is really 
>> useful to
>> have as a single logical operation, for example while preparing a 
>> series of
>> multiple local changesets that will need to be pushed as a single 
>> changeset.
>>
>> diff --git a/hgext/evolve.py b/hgext/evolve.py
>> --- a/hgext/evolve.py
>> +++ b/hgext/evolve.py
>> @@ -2997,6 +2997,128 @@ def fold(ui, repo, *revs, **opts):
>>       finally:
>>           lockmod.release(lock, wlock)
>>
>> + at command('^metaedit',
>
> We probably don't want to (yep, evolve is a bit a mess here, I'm 
> trying to put some order in that)

We probably don't want to what, precisely?

>
>> +         [('r', 'rev', [], _("revisions to edit")),
>> +          ('', 'fold', None, _("also fold specified revisions into 
>> one"))
>> +         ] + commitopts + commitopts2,
>> +         _('hg metaedit [OPTION]... [-r] [REV]'))
>> +def metaedit(ui, repo, *revs, **opts):
>> +    """edit commit information
>> +
>> +    Edits the commit information for the specified revisions. By 
>> default, edits
>> +    commit information for the working directory parent.
>> +
>> +    With --fold, also folds multiple revisions into one if 
>> necessary. In this
>> +    case, the given revisions must form a linear unbroken chain.
>> +
>> +    .. container:: verbose
>> +
>> +     Some examples:
>> +
>> +     - Edit the commit message for the working directory parent::
>> +
>> +         hg metaedit
>> +
>> +     - Change the username for the working directory parent::
>> +
>> +         hg metaedit --user 'New User <new-email at example.com>'
>> +
>> +     - Combine all draft revisions that are ancestors of foo but not 
>> of @ into
>> +       one::
>> +
>> +         hg metaedit --fold 'draft() and only(foo,@)'
>> +
>> +       See :hg:`help phases` for more about draft revisions and
>> +       :hg:`help revsets` for more about the `draft()` and `only()` 
>> keywords
>> +    """
>> +    revs = list(revs)
>> +    revs.extend(opts['rev'])
>> +    if not revs:
>> +        if opts['fold']:
>> +            raise error.Abort(_('revisions must be specified with 
>> --fold'))
>> +        revs = ['.']
>> +
>> +    revs = scmutil.revrange(repo, revs)
>> +    if not opts['fold'] and len(revs) > 1:
>> +        # TODO: handle the non-fold case with multiple revisions. 
>> This is
>> +        # somewhat tricky because if we want to edit a series of 
>> commits:
>> +        #
>> +        #   a ---- b ---- c
>> +        #
>> +        # we need to rewrite a first, then directly rewrite b on top 
>> of the new
>> +        # a, then rewrite c on top of the new b. So we need to 
>> handle revisions
>> +        # in topological order.
>> +        raise error.Abort(_('editing multiple revisions without 
>> --fold is not '
>> +                            'currently supported'))
>
> I would be happy to see this lifted. It looks like sorting the 
> revision should be sufficient to get them on topological order, and a 
> small mapping to forward parent. Am I missing someting?

There's also other UI issues like how to accept the commit message for 
multiple revisions, etc. That's why I'm inclined to not deal with this 
problem right now.

>
>> +    if opts['fold']:
>> +        root, head = _foldcheck(repo, revs)
>> +    else:
>> +        if _willcreatedisallowedunstable(repo, revs):
>> +            raise error.Abort(_('cannot edit commit information in 
>> the middle '
>> +                                'of a stack'))
>
> If you can add information about a problematic node, that would help 
> the user to debug the situation

OK -- but this code is copied verbatim from other places where we do 
similar checks. We'll probably need to do those sorts of fixups anyway.

>
>> +        # check above ensures only one revision
>> +        root = head = repo[revs.first()]
>> +        if not root.mutable():
>
> Directly check repo.revs("%lr and public()", revs) That will be 
> simpler and faster.

OK.

>
>> +            raise error.Abort(_('cannot edit commit information for 
>> public '
>> +                                'revisions'))
>> +
>> +    wlock = lock = None
>> +    try:
>> +        wlock = repo.wlock()
>> +        lock = repo.lock()
>
> The repository is locked too late. We should compute the target and 
> validate its content (especially the unstable detection).

Good point -- though a lot of other code is also buggy I suspect.

>
>> +        wctx = repo[None]
>> +        p1, p2 = wctx.p1(), wctx.p2()
>> +        tr = repo.transaction('metaedit')
>> +        newp1 = None
>> +        try:
>> +            commitopts = opts.copy()
>> +            allctx = [repo[r] for r in revs]
>> +            targetphase = max(c.phase() for c in allctx)
>> +
>> +            if commitopts.get('message') or commitopts.get('logfile'):
>> +                commitopts['edit'] = False
>> +            else:
>> +                if opts['fold']:
>> +                    msgs = ["HG: This is a fold of %d changesets." % 
>> len(allctx)]
>> +                    msgs += ["HG: Commit message of changeset 
>> %s.\n\n%s\n" %
>> +                             (c.rev(), c.description()) for c in 
>> allctx]
>> +                else:
>> +                    msgs = [head.description()]
>> +                commitopts['message'] =  "\n".join(msgs)
>> +                commitopts['edit'] = True
>> +
>> +            # TODO: if the author and message are the same, don't 
>> create a new
>> +            # hash. Right now we create a new hash because the date 
>> can be
>> +            # different.
>> +            newid, created = rewrite(repo, root, allctx, head,
>> +                                     [root.p1().node(), 
>> root.p2().node()],
>> +                                     commitopts=commitopts)
>> +            # Optimization: if the working copy parent is a *head* 
>> (not root,
>> +            # not in between) of a commit or commit series that got 
>> rewritten,
>> +            # just use localrepo.setparents and avoid any working copy
>> +            # updates. It's easier to do this if we don't also have 
>> to worry
>> +            # about p2.
>
> Why do we need this? isn't and update good enough?

It probably is -- I'll get rid of this.

>
>> +            if not p2 and head == p1:
>> +                newp1 = newid
>> +            if created:
>> +                phases.retractboundary(repo, tr, targetphase, [newid])
>> +                obsolete.createmarkers(repo, [(ctx, (repo[newid],))
>> +                                              for ctx in allctx])
>> +            else:
>> +                ui.status(_("nothing changed\n"))
>> +            tr.close()
>> +        finally:
>> +            tr.release()
>> +        if opts['fold']:
>> +            ui.status('%i changesets folded\n' % len(revs))
>> +        if newp1 is not None:
>> +            repo.setparents(newp1)
>> +        elif p1.rev() in revs:
>> +            hg.update(repo, newid)
>> +    finally:
>> +        lockmod.release(lock, wlock)
>> +
>>   def _foldcheck(repo, revs):
>>       roots = repo.revs('roots(%ld)', revs)
>>       if len(roots) > 1:
>



More information about the Mercurial-devel mailing list