Patchwork [3,of,3] cat: use the topmost repo when expanding node based format strings for output

login
register
mail settings
Submitter Matt Harbison
Date March 22, 2014, 5:08 p.m.
Message ID <f4b974de9c952b027d8f.1395508098@Envy>
Download mbox | patch
Permalink /patch/4039/
State Deferred
Headers show

Comments

Matt Harbison - March 22, 2014, 5:08 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1395456880 14400
# Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
# Parent  35686f3835c908e3e681a0abd0c805476ccdfddc
cat: use the topmost repo when expanding node based format strings for output

The format strings %H, %R, %h, %m and %r aren't documented to work with cat, but
they do, and have since at least version 0.5.  (It looks like %m doesn't
actually work- it expands to 'None' because no description is provided to
cmdutil.makefileobj().)  Expanding these variables based on the top level repo
makes the subrepo seem more integrated with the parent, and is probably less
surprising to a user that executes this command from the top level repo.

This solution isn't very satisfying because it requires passing two contexts
around, and it isn't reusable in other places that may need info about the top
level context that includes the repo.  The alternate attempt I made was to store
the parent context in the subrepo class when it is initialized.  The svn and git
subrepos already do this.  That didn't work because the chain that can be
followed up to the root of hgsubrepo is based on localrepository objects, not
subrepo instances.  But at least this allows for proper behavior until something
better can be figured out.
Siddharth Agarwal - March 24, 2014, 1:01 a.m.
On 03/22/2014 10:08 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1395456880 14400
> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
> # Parent  35686f3835c908e3e681a0abd0c805476ccdfddc
> cat: use the topmost repo when expanding node based format strings for output

Your first two patches look fine, but I'm not sure going to such 
contortions to fix undocumented features is worth it -- do we want to 
document them or not?

>
> The format strings %H, %R, %h, %m and %r aren't documented to work with cat, but
> they do, and have since at least version 0.5.  (It looks like %m doesn't
> actually work- it expands to 'None' because no description is provided to
> cmdutil.makefileobj().)  Expanding these variables based on the top level repo
> makes the subrepo seem more integrated with the parent, and is probably less
> surprising to a user that executes this command from the top level repo.
>
> This solution isn't very satisfying because it requires passing two contexts
> around, and it isn't reusable in other places that may need info about the top
> level context that includes the repo.  The alternate attempt I made was to store
> the parent context in the subrepo class when it is initialized.  The svn and git
> subrepos already do this.  That didn't work because the chain that can be
> followed up to the root of hgsubrepo is based on localrepository objects, not
> subrepo instances.  But at least this allows for proper behavior until something
> better can be figured out.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1812,11 +1812,11 @@
>       forgot.extend(forget)
>       return bad, forgot
>   
> -def cat(ui, repo, ctx, matcher, prefix, **opts):
> +def cat(ui, repo, ctx, matcher, prefix, rootctx, **opts):
>       err = 1
>   
>       def write(path):
> -        fp = makefileobj(repo, opts.get('output'), ctx.node(),
> +        fp = makefileobj(rootctx._repo, opts.get('output'), rootctx.node(),
>                            pathname=os.path.join(prefix, path))
>           data = ctx[path].data()
>           if opts.get('decode'):
> @@ -1857,7 +1857,7 @@
>               submatch = matchmod.narrowmatcher(subpath, matcher)
>   
>               if not sub.cat(ui, submatch, os.path.join(prefix, sub._path),
> -                           **opts):
> +                           rootctx, **opts):
>                   err = 0
>           except error.RepoLookupError:
>               ui.status(_("skipping missing subrepository: %s\n")
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1171,7 +1171,7 @@
>       ctx = scmutil.revsingle(repo, opts.get('rev'))
>       m = scmutil.match(ctx, (file1,) + pats, opts)
>   
> -    return cmdutil.cat(ui, repo, ctx, m, '', **opts)
> +    return cmdutil.cat(ui, repo, ctx, m, '', ctx, **opts)
>   
>   @command('^clone',
>       [('U', 'noupdate', None,
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -439,7 +439,7 @@
>       def add(self, ui, match, dryrun, listsubrepos, prefix, explicitonly):
>           return []
>   
> -    def cat(self, ui, match, prefix, **opts):
> +    def cat(self, ui, match, prefix, rootctx, **opts):
>           return 1
>   
>       def status(self, rev2, **opts):
> @@ -612,10 +612,10 @@
>                              os.path.join(prefix, self._path), explicitonly)
>   
>       @annotatesubrepoerror
> -    def cat(self, ui, match, prefix, **opts):
> +    def cat(self, ui, match, prefix, rootctx, **opts):
>           rev = self._state[1]
>           ctx = self._repo[rev]
> -        return cmdutil.cat(ui, self._repo, ctx, match, prefix, **opts)
> +        return cmdutil.cat(ui, self._repo, ctx, match, prefix, rootctx, **opts)
>   
>       @annotatesubrepoerror
>       def status(self, rev2, **opts):
> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -759,7 +759,25 @@
>     test
>     test
>     $ mkdir -p tmp/sub/repo
> +  $ hg cat --output tmp/HH_%H sub/repo/foo
> +  $ hg cat --output tmp/RR_%R sub/repo/foo
> +  $ hg cat --output tmp/h_%h sub/repo/foo
> +  $ hg cat --output tmp/m_%m sub/repo/foo
> +  $ hg cat --output tmp/r_%r sub/repo/foo
> +  $ hg cat --output tmp/%s_s sub/repo/foo
> +  $ hg cat --output tmp/%d_d sub/repo/foo
>     $ hg cat -r 0 --output tmp/%p_p sub/repo/foo
> +  $ hg log -r . --template "{rev}: {node|short}\n"
> +  1: be5eb94e7215
> +  $ find tmp -type f | sort
> +  tmp/HH_be5eb94e72150f9a8f9be0819fc35d935cbd2f30
> +  tmp/RR_1
> +  tmp/foo_s
> +  tmp/h_be5eb94e7215
> +  tmp/m_None
> +  tmp/r_1
> +  tmp/sub/repo/foo_p
> +  tmp/sub/repo_d
>     $ cat tmp/sub/repo/foo_p
>     test
>     $ mv sub/repo sub_
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Harbison - March 24, 2014, 3:04 a.m.
Siddharth Agarwal wrote:
> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1395456880 14400
>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>> # Parent 35686f3835c908e3e681a0abd0c805476ccdfddc
>> cat: use the topmost repo when expanding node based format strings for
>> output
>
> Your first two patches look fine, but I'm not sure going to such
> contortions to fix undocumented features is worth it -- do we want to
> document them or not?
>

I'm not sure- I wasn't aware these worked either, and only stumbled 
across them accidentally while looking at something else.  I was really 
surprised how long they have been available and not documented, but 
maybe cat'ing to stdout and using shell redirection is how most people 
use it and nobody else thought to try these either?

I don't feel strongly about this one- I just submitted it because if 
somebody does document these someday, I wasn't sure if we would be able 
to make this (or equivalent) change in the future due to backward 
compatibility rules.  It's not likely anybody would think to test 
subrepos when simply documenting what already works.

It also looks easy enough to disable these- just don't pass the node to 
makefileobj().  But the hash and rev expansions at least seem marginally 
useful.

--Matt
Pierre-Yves David - April 14, 2014, 5:16 a.m.
On 03/23/2014 11:04 PM, Matt Harbison wrote:
> Siddharth Agarwal wrote:
>> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1395456880 14400
>>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>>> # Parent 35686f3835c908e3e681a0abd0c805476ccdfddc
>>> cat: use the topmost repo when expanding node based format strings for
>>> output
>>
>> Your first two patches look fine, but I'm not sure going to such
>> contortions to fix undocumented features is worth it -- do we want to
>> document them or not?
>>
>
> I'm not sure- I wasn't aware these worked either, and only stumbled
> across them accidentally while looking at something else.  I was really
> surprised how long they have been available and not documented, but
> maybe cat'ing to stdout and using shell redirection is how most people
> use it and nobody else thought to try these either?
>
> I don't feel strongly about this one- I just submitted it because if
> somebody does document these someday, I wasn't sure if we would be able
> to make this (or equivalent) change in the future due to backward
> compatibility rules.  It's not likely anybody would think to test
> subrepos when simply documenting what already works.
>
> It also looks easy enough to disable these- just don't pass the node to
> makefileobj().  But the hash and rev expansions at least seem marginally
> useful.

Gentle bump about this patches. Do we want this fixed or not?
Pierre-Yves David - April 14, 2014, 5:19 a.m.
On 03/23/2014 09:01 PM, Siddharth Agarwal wrote:
> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1395456880 14400
>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>> # Parent  35686f3835c908e3e681a0abd0c805476ccdfddc
>> cat: use the topmost repo when expanding node based format strings for
>> output
>
> Your first two patches look fine,

Fine as in "I, hereby Siddharth Agarwal, would be happy to clowcopterize 
them?" or Fine as in "they are nice but make no sense without Patch 3" ?
Siddharth Agarwal - April 14, 2014, 5:57 p.m.
On 04/13/2014 10:19 PM, Pierre-Yves David wrote:
>
>
> On 03/23/2014 09:01 PM, Siddharth Agarwal wrote:
>> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1395456880 14400
>>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>>> # Parent  35686f3835c908e3e681a0abd0c805476ccdfddc
>>> cat: use the topmost repo when expanding node based format strings for
>>> output
>>
>> Your first two patches look fine,
>
> Fine as in "I, hereby Siddharth Agarwal, would be happy to 
> clowcopterize them?" or Fine as in "they are nice but make no sense 
> without Patch 3" ?
>

As in I'd be happy to queue them, yes. Patch 2 makes sense independently.
Pierre-Yves David - April 14, 2014, 6:18 p.m.
On 04/14/2014 01:57 PM, Siddharth Agarwal wrote:
> On 04/13/2014 10:19 PM, Pierre-Yves David wrote:
>>
>>
>> On 03/23/2014 09:01 PM, Siddharth Agarwal wrote:
>>> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1395456880 14400
>>>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>>>> # Parent  35686f3835c908e3e681a0abd0c805476ccdfddc
>>>> cat: use the topmost repo when expanding node based format strings for
>>>> output
>>>
>>> Your first two patches look fine,
>>
>> Fine as in "I, hereby Siddharth Agarwal, would be happy to
>> clowcopterize them?" or Fine as in "they are nice but make no sense
>> without Patch 3" ?
>>
>
> As in I'd be happy to queue them, yes. Patch 2 makes sense independently.

Please do.
Matt Harbison - April 15, 2014, 3:36 a.m.
Pierre-Yves David wrote:
>
>
> On 03/23/2014 09:01 PM, Siddharth Agarwal wrote:
>> On 03/22/2014 10:08 AM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1395456880 14400
>>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
>>> # Parent 35686f3835c908e3e681a0abd0c805476ccdfddc
>>> cat: use the topmost repo when expanding node based format strings for
>>> output
>>
>> Your first two patches look fine,
>
> Fine as in "I, hereby Siddharth Agarwal, would be happy to clowcopterize
> them?" or Fine as in "they are nice but make no sense without Patch 3" ?
>

Should I drop support for the undocumented formatters, which would make 
patch 3 completely unnecessary?  Nobody responded to Siddharth's 
question about "Do we want to document them or not?"

--Matt
Matt Mackall - April 15, 2014, 5:26 a.m.
On Mon, 2014-04-14 at 23:36 -0400, Matt Harbison wrote:
> Pierre-Yves David wrote:
> >
> >
> > On 03/23/2014 09:01 PM, Siddharth Agarwal wrote:
> >> On 03/22/2014 10:08 AM, Matt Harbison wrote:
> >>> # HG changeset patch
> >>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>> # Date 1395456880 14400
> >>> # Node ID f4b974de9c952b027d8f137eb4d302d72eb7ea60
> >>> # Parent 35686f3835c908e3e681a0abd0c805476ccdfddc
> >>> cat: use the topmost repo when expanding node based format strings for
> >>> output
> >>
> >> Your first two patches look fine,
> >
> > Fine as in "I, hereby Siddharth Agarwal, would be happy to clowcopterize
> > them?" or Fine as in "they are nice but make no sense without Patch 3" ?
> >
> 
> Should I drop support for the undocumented formatters, which would make 
> patch 3 completely unnecessary?  Nobody responded to Siddharth's 
> question about "Do we want to document them or not?"

Let's document them. I'm not sure about $SUBJECT though.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1812,11 +1812,11 @@ 
     forgot.extend(forget)
     return bad, forgot
 
-def cat(ui, repo, ctx, matcher, prefix, **opts):
+def cat(ui, repo, ctx, matcher, prefix, rootctx, **opts):
     err = 1
 
     def write(path):
-        fp = makefileobj(repo, opts.get('output'), ctx.node(),
+        fp = makefileobj(rootctx._repo, opts.get('output'), rootctx.node(),
                          pathname=os.path.join(prefix, path))
         data = ctx[path].data()
         if opts.get('decode'):
@@ -1857,7 +1857,7 @@ 
             submatch = matchmod.narrowmatcher(subpath, matcher)
 
             if not sub.cat(ui, submatch, os.path.join(prefix, sub._path),
-                           **opts):
+                           rootctx, **opts):
                 err = 0
         except error.RepoLookupError:
             ui.status(_("skipping missing subrepository: %s\n")
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1171,7 +1171,7 @@ 
     ctx = scmutil.revsingle(repo, opts.get('rev'))
     m = scmutil.match(ctx, (file1,) + pats, opts)
 
-    return cmdutil.cat(ui, repo, ctx, m, '', **opts)
+    return cmdutil.cat(ui, repo, ctx, m, '', ctx, **opts)
 
 @command('^clone',
     [('U', 'noupdate', None,
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -439,7 +439,7 @@ 
     def add(self, ui, match, dryrun, listsubrepos, prefix, explicitonly):
         return []
 
-    def cat(self, ui, match, prefix, **opts):
+    def cat(self, ui, match, prefix, rootctx, **opts):
         return 1
 
     def status(self, rev2, **opts):
@@ -612,10 +612,10 @@ 
                            os.path.join(prefix, self._path), explicitonly)
 
     @annotatesubrepoerror
-    def cat(self, ui, match, prefix, **opts):
+    def cat(self, ui, match, prefix, rootctx, **opts):
         rev = self._state[1]
         ctx = self._repo[rev]
-        return cmdutil.cat(ui, self._repo, ctx, match, prefix, **opts)
+        return cmdutil.cat(ui, self._repo, ctx, match, prefix, rootctx, **opts)
 
     @annotatesubrepoerror
     def status(self, rev2, **opts):
diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -759,7 +759,25 @@ 
   test
   test
   $ mkdir -p tmp/sub/repo
+  $ hg cat --output tmp/HH_%H sub/repo/foo
+  $ hg cat --output tmp/RR_%R sub/repo/foo
+  $ hg cat --output tmp/h_%h sub/repo/foo
+  $ hg cat --output tmp/m_%m sub/repo/foo
+  $ hg cat --output tmp/r_%r sub/repo/foo
+  $ hg cat --output tmp/%s_s sub/repo/foo
+  $ hg cat --output tmp/%d_d sub/repo/foo
   $ hg cat -r 0 --output tmp/%p_p sub/repo/foo
+  $ hg log -r . --template "{rev}: {node|short}\n"
+  1: be5eb94e7215
+  $ find tmp -type f | sort
+  tmp/HH_be5eb94e72150f9a8f9be0819fc35d935cbd2f30
+  tmp/RR_1
+  tmp/foo_s
+  tmp/h_be5eb94e7215
+  tmp/m_None
+  tmp/r_1
+  tmp/sub/repo/foo_p
+  tmp/sub/repo_d
   $ cat tmp/sub/repo/foo_p
   test
   $ mv sub/repo sub_