Patchwork [2,of,3] commit: add a -M/--reuse-message option to copy a commit message from a

login
register
mail settings
Submitter Yuya Nishihara
Date May 19, 2015, 2 p.m.
Message ID <20150519230030.495745d6932bad0c1f289d95@tcha.org>
Download mbox | patch
Permalink /patch/9171/
State Not Applicable
Headers show

Comments

Yuya Nishihara - May 19, 2015, 2 p.m.
On Mon, 18 May 2015 13:15:16 -0500, Matt Mackall wrote:
> On Mon, 2015-05-18 at 10:53 -0700, Durham Goode wrote:
> > On 5/17/15 6:57 PM, Matt Mackall wrote:
> > > On Fri, 2015-05-15 at 15:54 -0700, Tony Tung wrote:
> > >> # HG changeset patch
> > >> # User Tony Tung <tonytung@fb.com>
> > >> # Date 1429655274 25200
> > >> #      Tue Apr 21 15:27:54 2015 -0700
> > >> # Node ID dc122dd80665762d8febe2db1a08ce00a63d5ab8
> > >> # Parent  18cadf9d058931ef00e5272d15cb5cf2ebc3a248
> > >> commit: add a -M/--reuse-message option to copy a commit message from a
> > >> revspec
> > >>
> > >> One way to split up a diff that includes a refactor involves resetting to
> > >> the ancestor, and then committing the refactor first.
> > > For future reference, "resetting" is git-speak that I don't understand,
> > > despite having looked at the git-reset manpage countless times. Please
> > > don't make me learn git to understand your commit messages, because git
> > > makes me sad. Please instead use "updating to", "reverting to", or
> > > whatever version of --hard/--soft/--bouncy you mean corresponds to.
> > >
> > > I vaguely recall discussing this option with Ryan, perhaps at the
> > > sprint. I understand the value of the feature, but I think it's below
> > > the threshold for UI clutter.
> > >
> > > One way of getting around that is to make a more generically useful
> > > feature with lower UI footprint. For instance, log historically has a
> > > ton of individual options to specify which revisons to show:
> > > --keyword/date/follow/follow-first/only-branch/only-merges/no-merges/limit/user/prune, etc.
> > >
> > > We were far past the point where things were cumbersome, the
> > > combinations didn't make sense, and it still wasn't expressive enough
> > > for all the things users wanted, and new things only worked for log.
> > >
> > > Now compare that with revsets: basically everything supports revsets,
> > > they're vastly more powerful and expressive, and adding new features to
> > > revsets doesn't clutter the help and implementation of every command.
> > >
> > > So a more "generic" version of this feature might look like this:
> > >
> > >   hg commit -u @255 -m @255 -d @255  # copy the metadata from cset 255
> > >
> > > ..but would have further reaching consequences:
> > >
> > >   hg log -d @.  # show commits with the same date as the current one
> > >
> > >   hg log -r "files(@bookmark)" # show commits that touch the same files
> > >
> > > The per-command implementation would look something like:
> > >
> > >    if opts["message"]:
> > >       message = cmdutil.expandopt(repo, "description", opts["message"])
> > >
> > > There are a bunch of problems with the precise form of the above,
> > > starting with the fact that we obviously already use "@" for other
> > > things, but I think it illustrates why a generic approach is more
> > > valuable and maintainable.
> > >
> > I like the idea of a generic solution.  What about 'hg commit -m 
> > {255}'?  Templates use {} to indicate "replace this with the desired 
> > value", so we could do the same in commands and revsets, where appropriate.
> 
> I'm actually thinking "<foo>", so there's no confusion with templates.

This resembles "hg email --flag '{branch|upper}'". Can't we design it to be
compatible with the template syntax? Perhaps we can extend it to a real
template in future.

  -m '{...}'       # evaluate as a template only if starts/ends with {}
  -m '{desc@255}'  # infix operator "var@rev"
  -m '{@255}'      # prefix operator, default to {desc@255} for -m

PoC patch, only "var@rev" is allowed:
Matt Mackall - May 19, 2015, 3:21 p.m.
On Tue, 2015-05-19 at 23:00 +0900, Yuya Nishihara wrote:
> On Mon, 18 May 2015 13:15:16 -0500, Matt Mackall wrote:
> > On Mon, 2015-05-18 at 10:53 -0700, Durham Goode wrote:
> > > On 5/17/15 6:57 PM, Matt Mackall wrote:
> > > > On Fri, 2015-05-15 at 15:54 -0700, Tony Tung wrote:
> > > >> # HG changeset patch
> > > >> # User Tony Tung <tonytung@fb.com>
> > > >> # Date 1429655274 25200
> > > >> #      Tue Apr 21 15:27:54 2015 -0700
> > > >> # Node ID dc122dd80665762d8febe2db1a08ce00a63d5ab8
> > > >> # Parent  18cadf9d058931ef00e5272d15cb5cf2ebc3a248
> > > >> commit: add a -M/--reuse-message option to copy a commit message from a
> > > >> revspec
> > > >>
> > > >> One way to split up a diff that includes a refactor involves resetting to
> > > >> the ancestor, and then committing the refactor first.
> > > > For future reference, "resetting" is git-speak that I don't understand,
> > > > despite having looked at the git-reset manpage countless times. Please
> > > > don't make me learn git to understand your commit messages, because git
> > > > makes me sad. Please instead use "updating to", "reverting to", or
> > > > whatever version of --hard/--soft/--bouncy you mean corresponds to.
> > > >
> > > > I vaguely recall discussing this option with Ryan, perhaps at the
> > > > sprint. I understand the value of the feature, but I think it's below
> > > > the threshold for UI clutter.
> > > >
> > > > One way of getting around that is to make a more generically useful
> > > > feature with lower UI footprint. For instance, log historically has a
> > > > ton of individual options to specify which revisons to show:
> > > > --keyword/date/follow/follow-first/only-branch/only-merges/no-merges/limit/user/prune, etc.
> > > >
> > > > We were far past the point where things were cumbersome, the
> > > > combinations didn't make sense, and it still wasn't expressive enough
> > > > for all the things users wanted, and new things only worked for log.
> > > >
> > > > Now compare that with revsets: basically everything supports revsets,
> > > > they're vastly more powerful and expressive, and adding new features to
> > > > revsets doesn't clutter the help and implementation of every command.
> > > >
> > > > So a more "generic" version of this feature might look like this:
> > > >
> > > >   hg commit -u @255 -m @255 -d @255  # copy the metadata from cset 255
> > > >
> > > > ..but would have further reaching consequences:
> > > >
> > > >   hg log -d @.  # show commits with the same date as the current one
> > > >
> > > >   hg log -r "files(@bookmark)" # show commits that touch the same files
> > > >
> > > > The per-command implementation would look something like:
> > > >
> > > >    if opts["message"]:
> > > >       message = cmdutil.expandopt(repo, "description", opts["message"])
> > > >
> > > > There are a bunch of problems with the precise form of the above,
> > > > starting with the fact that we obviously already use "@" for other
> > > > things, but I think it illustrates why a generic approach is more
> > > > valuable and maintainable.
> > > >
> > > I like the idea of a generic solution.  What about 'hg commit -m 
> > > {255}'?  Templates use {} to indicate "replace this with the desired 
> > > value", so we could do the same in commands and revsets, where appropriate.
> > 
> > I'm actually thinking "<foo>", so there's no confusion with templates.

It's been pointed out that the potential for redirection damage is
significant.

> This resembles "hg email --flag '{branch|upper}'". Can't we design it to be
> compatible with the template syntax? Perhaps we can extend it to a real
> template in future.
>
>   -m '{...}'       # evaluate as a template only if starts/ends with {}
>   -m '{desc@255}'  # infix operator "var@rev"
>   -m '{@255}'      # prefix operator, default to {desc@255} for -m

That seems promising. It's a bit of a problem that there's not a notion
of "usage context" though: we'd like to be able to say "oh, we're
talking about users here, so @255 refers to the user." So we'd need to
pass in a default field to the helper. And we'll also need to support
arbitrary revsets.

And @ still isn't a great choice since we used it for bookmarks. Since
we want to support revsets, an enclosing form is probably best. Maybe
[]?
Yuya Nishihara - May 20, 2015, 1:05 p.m.
On Tue, 19 May 2015 10:21:31 -0500, Matt Mackall wrote:
> On Tue, 2015-05-19 at 23:00 +0900, Yuya Nishihara wrote:
> > On Mon, 18 May 2015 13:15:16 -0500, Matt Mackall wrote:
> > > On Mon, 2015-05-18 at 10:53 -0700, Durham Goode wrote:
> > > > On 5/17/15 6:57 PM, Matt Mackall wrote:
> > > > > On Fri, 2015-05-15 at 15:54 -0700, Tony Tung wrote:
> > > > >> # HG changeset patch
> > > > >> # User Tony Tung <tonytung@fb.com>
> > > > >> # Date 1429655274 25200
> > > > >> #      Tue Apr 21 15:27:54 2015 -0700
> > > > >> # Node ID dc122dd80665762d8febe2db1a08ce00a63d5ab8
> > > > >> # Parent  18cadf9d058931ef00e5272d15cb5cf2ebc3a248
> > > > >> commit: add a -M/--reuse-message option to copy a commit message from a
> > > > >> revspec
> > > > >>
> > > > >> One way to split up a diff that includes a refactor involves resetting to
> > > > >> the ancestor, and then committing the refactor first.
> > > > > For future reference, "resetting" is git-speak that I don't understand,
> > > > > despite having looked at the git-reset manpage countless times. Please
> > > > > don't make me learn git to understand your commit messages, because git
> > > > > makes me sad. Please instead use "updating to", "reverting to", or
> > > > > whatever version of --hard/--soft/--bouncy you mean corresponds to.
> > > > >
> > > > > I vaguely recall discussing this option with Ryan, perhaps at the
> > > > > sprint. I understand the value of the feature, but I think it's below
> > > > > the threshold for UI clutter.
> > > > >
> > > > > One way of getting around that is to make a more generically useful
> > > > > feature with lower UI footprint. For instance, log historically has a
> > > > > ton of individual options to specify which revisons to show:
> > > > > --keyword/date/follow/follow-first/only-branch/only-merges/no-merges/limit/user/prune, etc.
> > > > >
> > > > > We were far past the point where things were cumbersome, the
> > > > > combinations didn't make sense, and it still wasn't expressive enough
> > > > > for all the things users wanted, and new things only worked for log.
> > > > >
> > > > > Now compare that with revsets: basically everything supports revsets,
> > > > > they're vastly more powerful and expressive, and adding new features to
> > > > > revsets doesn't clutter the help and implementation of every command.
> > > > >
> > > > > So a more "generic" version of this feature might look like this:
> > > > >
> > > > >   hg commit -u @255 -m @255 -d @255  # copy the metadata from cset 255
> > > > >
> > > > > ..but would have further reaching consequences:
> > > > >
> > > > >   hg log -d @.  # show commits with the same date as the current one
> > > > >
> > > > >   hg log -r "files(@bookmark)" # show commits that touch the same files
> > > > >
> > > > > The per-command implementation would look something like:
> > > > >
> > > > >    if opts["message"]:
> > > > >       message = cmdutil.expandopt(repo, "description", opts["message"])
> > > > >
> > > > > There are a bunch of problems with the precise form of the above,
> > > > > starting with the fact that we obviously already use "@" for other
> > > > > things, but I think it illustrates why a generic approach is more
> > > > > valuable and maintainable.
> > > > >
> > > > I like the idea of a generic solution.  What about 'hg commit -m 
> > > > {255}'?  Templates use {} to indicate "replace this with the desired 
> > > > value", so we could do the same in commands and revsets, where appropriate.
> > > 
> > > I'm actually thinking "<foo>", so there's no confusion with templates.
> 
> It's been pointed out that the potential for redirection damage is
> significant.
> 
> > This resembles "hg email --flag '{branch|upper}'". Can't we design it to be
> > compatible with the template syntax? Perhaps we can extend it to a real
> > template in future.
> >
> >   -m '{...}'       # evaluate as a template only if starts/ends with {}
> >   -m '{desc@255}'  # infix operator "var@rev"
> >   -m '{@255}'      # prefix operator, default to {desc@255} for -m
> 
> That seems promising. It's a bit of a problem that there's not a notion
> of "usage context" though: we'd like to be able to say "oh, we're
> talking about users here, so @255 refers to the user." So we'd need to
> pass in a default field to the helper. And we'll also need to support
> arbitrary revsets.
> 
> And @ still isn't a great choice since we used it for bookmarks. Since
> we want to support revsets, an enclosing form is probably best. Maybe
> []?

Good point. A closing bracket will allow us to parse inner expression as
a revset, not a template. So we can avoid unwanted quotes and escapes.

  '{desc[bookmark("re:[sS]table")]}'

A sad thing is that the same syntax cannot be used in the fileset.

  'set:added()[wdir()]'  # [] is glob characters

But I agree [] will be better than @.

Patch

--- a/mercurial/templater.py
+++ b/mercurial/templater.py
@@ -17,6 +17,7 @@  import minirst
 elements = {
     "(": (20, ("group", 1, ")"), ("func", 1, ")")),
     ",": (2, None, ("list", 2)),
+    "@": (15, None, ("@", 15)),  # XXX binding, symbol, ...?
     "|": (5, None, ("|", 5)),
     "%": (6, None, ("%", 6)),
     ")": (0, None, None),
@@ -34,7 +35,7 @@  def tokenizer(data):
         c = program[pos]
         if c.isspace(): # skip inter-token whitespace
             pass
-        elif c in "(,)%|": # handle simple operators
+        elif c in "(,)@%|": # handle simple operators
             yield (c, None, pos)
         elif (c in '"\'' or c == 'r' and
               program[pos:pos + 2] in ("r'", 'r"')): # handle quoted strings
@@ -174,6 +175,22 @@  def runsymbol(context, mapping, key):
         v = list(v)
     return v
 
+def buildswitchctx(exp, context):
+    func, data = compileexp(exp[1], context, methods)
+    if exp[2][0] not in ('symbol', 'string', 'integer'):
+        raise error.ParseError(_("expected a literal, got '%s'") % exp[2][0])
+    revexpr = exp[2][1]  # XXX should allow template?
+    return (runswitchctx, (func, data, revexpr))
+
+def runswitchctx(context, mapping, data):
+    func, data, revexpr = data
+    repo = mapping['repo']
+    # XXX should we use showlist()?
+    newmapping = mapping.copy()
+    for r in revsetmod.match(repo.ui, revexpr)(repo):  # XXX scmutil.revrange?
+        newmapping['ctx'] = repo[r]  # XXX parents, cache, ...?
+        yield stringify(func(context, newmapping, data))  # XXX _evalifliteral?
+
 def buildfilter(exp, context):
     func, data = compileexp(exp[1], context, methods)
     filt = getfilter(exp[2], context)
@@ -590,6 +607,7 @@  exprmethods = {
     "symbol": lambda e, c: (runsymbol, e[1]),
     "group": lambda e, c: compileexp(e[1], c, exprmethods),
 #    ".": buildmember,
+    "@": buildswitchctx,
     "|": buildfilter,
     "%": buildmap,
     "func": buildfunc,