Patchwork [RFC,V2] localrepo: don't reintroduce pruned tag entries when tagging

login
register
mail settings
Submitter Matt Harbison
Date Feb. 5, 2015, 3:10 a.m.
Message ID <de7c57345dfc58a49c25.1423105854@Envy>
Download mbox | patch
Permalink /patch/7679/
State Accepted
Headers show

Comments

Matt Harbison - Feb. 5, 2015, 3:10 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1412209593 14400
#      Wed Oct 01 20:26:33 2014 -0400
# Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
# Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
localrepo: don't reintroduce pruned tag entries when tagging

If a commit and a followup tag commit are pruned, there are no references to it
in any non obsolete version of .hgtags.  Without this change however, the next
time a tag is added to another branch, the obsolete references are appended in
.hgtags before the new entries for the current tag command.

The annotation to unfilter localrepo._tag() has been around since b3af182a1944.
The log message for it mentions computing the tag cache though, so I'm not sure
if this was misplaced?  It looks like branchmap was aware of filtering then, and
now tracks a cache per view.
Augie Fackler - Feb. 10, 2015, 6:42 p.m.
On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1412209593 14400
> #      Wed Oct 01 20:26:33 2014 -0400
> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
> localrepo: don't reintroduce pruned tag entries when tagging
>
> If a commit and a followup tag commit are pruned, there are no references to it
> in any non obsolete version of .hgtags.  Without this change however, the next
> time a tag is added to another branch, the obsolete references are appended in
> .hgtags before the new entries for the current tag command.
>
> The annotation to unfilter localrepo._tag() has been around since b3af182a1944.
> The log message for it mentions computing the tag cache though, so I'm not sure
> if this was misplaced?  It looks like branchmap was aware of filtering then, and
> now tracks a cache per view.

This looks reasonable to me. Queued.

>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -501,7 +501,6 @@
>          """
>          return hook.hook(self.ui, self, name, throw, **args)
>
> -    @unfilteredmethod
>      def _tag(self, names, node, message, local, user, date, extra={},
>               editor=False):
>          if isinstance(names, str):
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -753,3 +753,45 @@
>    $ hg tags
>    visible                            0:193e9254ce7e
>    tip                                0:193e9254ce7e
> +
> +  $ hg init a
> +  $ cd a
> +  $ touch foo
> +  $ hg add foo
> +  $ hg ci -mfoo
> +  $ touch bar
> +  $ hg add bar
> +  $ hg ci -mbar
> +  $ hg up 0
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ touch quux
> +  $ hg add quux
> +  $ hg ci -m quux
> +  created new head
> +  $ hg up 1
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ hg tag 1.0
> +
> +  $ hg up 2
> +  1 files updated, 0 files merged, 2 files removed, 0 files unresolved
> +  $ hg log -G
> +  o  3:bc47fc7e1c1d (draft) [tip ] Added tag 1.0 for changeset 50c889141114
> +  |
> +  | @  2:3d7f255a0081 (draft) [ ] quux
> +  | |
> +  o |  1:50c889141114 (draft) [1.0 ] bar
> +  |/
> +  o  0:1f7b0de80e11 (draft) [ ] foo
> +
> +  $ hg debugobsolete `getid bar`
> +  $ hg debugobsolete `getid 1.0`
> +  $ hg tag 1.0
> +  $ hg log -G
> +  @  4:f9f2ab71ffd5 (draft) [tip ] Added tag 1.0 for changeset 3d7f255a0081
> +  |
> +  o  2:3d7f255a0081 (draft) [1.0 ] quux
> +  |
> +  o  0:1f7b0de80e11 (draft) [ ] foo
> +
> +  $ cat .hgtags
> +  3d7f255a008103380aeb2a7d581fe257f40969e7 1.0
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 25, 2015, 2:32 p.m.
On 02/10/2015 06:42 PM, Augie Fackler wrote:
> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1412209593 14400
>> #      Wed Oct 01 20:26:33 2014 -0400
>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>> localrepo: don't reintroduce pruned tag entries when tagging
>>
>> If a commit and a followup tag commit are pruned, there are no references to it
>> in any non obsolete version of .hgtags.  Without this change however, the next
>> time a tag is added to another branch, the obsolete references are appended in
>> .hgtags before the new entries for the current tag command.
>>
>> The annotation to unfilter localrepo._tag() has been around since b3af182a1944.
>> The log message for it mentions computing the tag cache though, so I'm not sure
>> if this was misplaced?  It looks like branchmap was aware of filtering then, and
>> now tracks a cache per view.
>
> This looks reasonable to me. Queued.

This looks highly unreasonable to me ;-)

The tags mechanism involves caching and needs to be aware of filtering 
level before we can move forward. Dropping the "unfiltered" decoration 
before that will likely cause obscure bug. Matt, description give me the 
impression that we need to dig deeper before dropping


Can we drop/backout this?
Augie Fackler - Feb. 25, 2015, 2:51 p.m.
On Wed, Feb 25, 2015 at 9:32 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 02/10/2015 06:42 PM, Augie Fackler wrote:
>>
>> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>>>
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1412209593 14400
>>> #      Wed Oct 01 20:26:33 2014 -0400
>>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>>> localrepo: don't reintroduce pruned tag entries when tagging
>>>
>>> If a commit and a followup tag commit are pruned, there are no references
>>> to it
>>> in any non obsolete version of .hgtags.  Without this change however, the
>>> next
>>> time a tag is added to another branch, the obsolete references are
>>> appended in
>>> .hgtags before the new entries for the current tag command.
>>>
>>> The annotation to unfilter localrepo._tag() has been around since
>>> b3af182a1944.
>>> The log message for it mentions computing the tag cache though, so I'm
>>> not sure
>>> if this was misplaced?  It looks like branchmap was aware of filtering
>>> then, and
>>> now tracks a cache per view.
>>
>>
>> This looks reasonable to me. Queued.
>
>
> This looks highly unreasonable to me ;-)
>
> The tags mechanism involves caching and needs to be aware of filtering level
> before we can move forward. Dropping the "unfiltered" decoration before that
> will likely cause obscure bug. Matt, description give me the impression that
> we need to dig deeper before dropping
>
>
> Can we drop/backout this?

I'm not sure what your objection is - do you think this introduces new bugs?

(It's queued largely because it resolves a bug, but I can be persuaded
that I'm missing something.)

>
>
> --
> Pierre-Yves David
Pierre-Yves David - Feb. 25, 2015, 2:55 p.m.
On 02/25/2015 02:51 PM, Augie Fackler wrote:
> On Wed, Feb 25, 2015 at 9:32 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 02/10/2015 06:42 PM, Augie Fackler wrote:
>>>
>>> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1412209593 14400
>>>> #      Wed Oct 01 20:26:33 2014 -0400
>>>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>>>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>>>> localrepo: don't reintroduce pruned tag entries when tagging
>>>>
>>>> If a commit and a followup tag commit are pruned, there are no references
>>>> to it
>>>> in any non obsolete version of .hgtags.  Without this change however, the
>>>> next
>>>> time a tag is added to another branch, the obsolete references are
>>>> appended in
>>>> .hgtags before the new entries for the current tag command.
>>>>
>>>> The annotation to unfilter localrepo._tag() has been around since
>>>> b3af182a1944.
>>>> The log message for it mentions computing the tag cache though, so I'm
>>>> not sure
>>>> if this was misplaced?  It looks like branchmap was aware of filtering
>>>> then, and
>>>> now tracks a cache per view.
>>>
>>>
>>> This looks reasonable to me. Queued.
>>
>>
>> This looks highly unreasonable to me ;-)
>>
>> The tags mechanism involves caching and needs to be aware of filtering level
>> before we can move forward. Dropping the "unfiltered" decoration before that
>> will likely cause obscure bug. Matt, description give me the impression that
>> we need to dig deeper before dropping
>>
>>
>> Can we drop/backout this?
>
> I'm not sure what your objection is - do you think this introduces new bugs?
>
> (It's queued largely because it resolves a bug, but I can be persuaded
> that I'm missing something.)

 From where I'm I read this patch as: the tags cache is always 
infiltered, this is a issue when using another view, we drop the 
filtering. As a result, the tag cache end up being computed for the 
first view asking for it and globally cached for all other view.

I would like someone to fix this or convince me I'm wrong before moving 
forward.
Gregory Szorc - Feb. 25, 2015, 4:23 p.m.
> On Feb 25, 2015, at 6:55, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
>> On 02/25/2015 02:51 PM, Augie Fackler wrote:
>> On Wed, Feb 25, 2015 at 9:32 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>> 
>>> 
>>>> On 02/10/2015 06:42 PM, Augie Fackler wrote:
>>>> 
>>>>> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>>>>> 
>>>>> # HG changeset patch
>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>> # Date 1412209593 14400
>>>>> #      Wed Oct 01 20:26:33 2014 -0400
>>>>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>>>>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>>>>> localrepo: don't reintroduce pruned tag entries when tagging
>>>>> 
>>>>> If a commit and a followup tag commit are pruned, there are no references
>>>>> to it
>>>>> in any non obsolete version of .hgtags.  Without this change however, the
>>>>> next
>>>>> time a tag is added to another branch, the obsolete references are
>>>>> appended in
>>>>> .hgtags before the new entries for the current tag command.
>>>>> 
>>>>> The annotation to unfilter localrepo._tag() has been around since
>>>>> b3af182a1944.
>>>>> The log message for it mentions computing the tag cache though, so I'm
>>>>> not sure
>>>>> if this was misplaced?  It looks like branchmap was aware of filtering
>>>>> then, and
>>>>> now tracks a cache per view.
>>>> 
>>>> 
>>>> This looks reasonable to me. Queued.
>>> 
>>> 
>>> This looks highly unreasonable to me ;-)
>>> 
>>> The tags mechanism involves caching and needs to be aware of filtering level
>>> before we can move forward. Dropping the "unfiltered" decoration before that
>>> will likely cause obscure bug. Matt, description give me the impression that
>>> we need to dig deeper before dropping
>>> 
>>> 
>>> Can we drop/backout this?
>> 
>> I'm not sure what your objection is - do you think this introduces new bugs?
>> 
>> (It's queued largely because it resolves a bug, but I can be persuaded
>> that I'm missing something.)
> 
> From where I'm I read this patch as: the tags cache is always infiltered, this is a issue when using another view, we drop the filtering. As a result, the tag cache end up being computed for the first view asking for it and globally cached for all other view.
> 
> I would like someone to fix this or convince me I'm wrong before moving forward.

See issue 4550 and my proposed patch series for stable I submitted the other day.

I didn't even realize this patch was related until I read your replies!
Matt Harbison - Feb. 26, 2015, 1:20 a.m.
On Wed, 25 Feb 2015 09:32:13 -0500, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 02/10/2015 06:42 PM, Augie Fackler wrote:
>> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1412209593 14400
>>> #      Wed Oct 01 20:26:33 2014 -0400
>>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>>> localrepo: don't reintroduce pruned tag entries when tagging
>>>
>>> If a commit and a followup tag commit are pruned, there are no  
>>> references to it
>>> in any non obsolete version of .hgtags.  Without this change however,  
>>> the next
>>> time a tag is added to another branch, the obsolete references are  
>>> appended in
>>> .hgtags before the new entries for the current tag command.
>>>
>>> The annotation to unfilter localrepo._tag() has been around since  
>>> b3af182a1944.
>>> The log message for it mentions computing the tag cache though, so I'm  
>>> not sure
>>> if this was misplaced?  It looks like branchmap was aware of filtering  
>>> then, and
>>> now tracks a cache per view.
>>
>> This looks reasonable to me. Queued.
>
> This looks highly unreasonable to me ;-)

LOL.  I knew a change so simple was too good to be true.

Assuming Greg's cache patches are OK, maybe this should be grafted to  
stable?  The only reason I put it on default was because I wasn't sure of  
any unexpected side effects.  It isn't a critical bug for me.

> The tags mechanism involves caching and needs to be aware of filtering  
> level before we can move forward. Dropping the "unfiltered" decoration  
> before that will likely cause obscure bug. Matt, description give me the  
> impression that we need to dig deeper before dropping
>
>
> Can we drop/backout this?
>
Matt Mackall - March 3, 2015, 10:04 p.m.
On Wed, 2015-02-25 at 20:20 -0500, Matt Harbison wrote:
> On Wed, 25 Feb 2015 09:32:13 -0500, Pierre-Yves David  
> <pierre-yves.david@ens-lyon.org> wrote:
> 
> >
> >
> > On 02/10/2015 06:42 PM, Augie Fackler wrote:
> >> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
> >>> # HG changeset patch
> >>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>> # Date 1412209593 14400
> >>> #      Wed Oct 01 20:26:33 2014 -0400
> >>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
> >>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
> >>> localrepo: don't reintroduce pruned tag entries when tagging
> >>>
> >>> If a commit and a followup tag commit are pruned, there are no  
> >>> references to it
> >>> in any non obsolete version of .hgtags.  Without this change however,  
> >>> the next
> >>> time a tag is added to another branch, the obsolete references are  
> >>> appended in
> >>> .hgtags before the new entries for the current tag command.
> >>>
> >>> The annotation to unfilter localrepo._tag() has been around since  
> >>> b3af182a1944.
> >>> The log message for it mentions computing the tag cache though, so I'm  
> >>> not sure
> >>> if this was misplaced?  It looks like branchmap was aware of filtering  
> >>> then, and
> >>> now tracks a cache per view.
> >>
> >> This looks reasonable to me. Queued.
> >
> > This looks highly unreasonable to me ;-)
> 
> LOL.  I knew a change so simple was too good to be true.
> 
> Assuming Greg's cache patches are OK, maybe this should be grafted to  
> stable?  The only reason I put it on default was because I wasn't sure of  
> any unexpected side effects.  It isn't a critical bug for me.
> 
> > The tags mechanism involves caching and needs to be aware of filtering  
> > level before we can move forward. Dropping the "unfiltered" decoration  
> > before that will likely cause obscure bug. Matt, description give me the  
> > impression that we need to dig deeper before dropping
> >
> >
> > Can we drop/backout this?

For better or worse, this landed on default. So if people think this is
wrong, they should probably submit a follow-up.
Pierre-Yves David - March 5, 2015, 10:49 p.m.
On 03/03/2015 02:04 PM, Matt Mackall wrote:
> On Wed, 2015-02-25 at 20:20 -0500, Matt Harbison wrote:
>> On Wed, 25 Feb 2015 09:32:13 -0500, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 02/10/2015 06:42 PM, Augie Fackler wrote:
>>>> On Wed, Feb 04, 2015 at 10:10:54PM -0500, Matt Harbison wrote:
>>>>> # HG changeset patch
>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>> # Date 1412209593 14400
>>>>> #      Wed Oct 01 20:26:33 2014 -0400
>>>>> # Node ID de7c57345dfc58a49c25f97070b084e65b1cb0a0
>>>>> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
>>>>> localrepo: don't reintroduce pruned tag entries when tagging
>>>>>
>>>>> If a commit and a followup tag commit are pruned, there are no
>>>>> references to it
>>>>> in any non obsolete version of .hgtags.  Without this change however,
>>>>> the next
>>>>> time a tag is added to another branch, the obsolete references are
>>>>> appended in
>>>>> .hgtags before the new entries for the current tag command.
>>>>>
>>>>> The annotation to unfilter localrepo._tag() has been around since
>>>>> b3af182a1944.
>>>>> The log message for it mentions computing the tag cache though, so I'm
>>>>> not sure
>>>>> if this was misplaced?  It looks like branchmap was aware of filtering
>>>>> then, and
>>>>> now tracks a cache per view.
>>>>
>>>> This looks reasonable to me. Queued.
>>>
>>> This looks highly unreasonable to me ;-)
>>
>> LOL.  I knew a change so simple was too good to be true.
>>
>> Assuming Greg's cache patches are OK, maybe this should be grafted to
>> stable?  The only reason I put it on default was because I wasn't sure of
>> any unexpected side effects.  It isn't a critical bug for me.
>>
>>> The tags mechanism involves caching and needs to be aware of filtering
>>> level before we can move forward. Dropping the "unfiltered" decoration
>>> before that will likely cause obscure bug. Matt, description give me the
>>> impression that we need to dig deeper before dropping
>>>
>>>
>>> Can we drop/backout this?
>
> For better or worse, this landed on default. So if people think this is
> wrong, they should probably submit a follow-up.

I've pushed a backout to the clowncopter.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -501,7 +501,6 @@ 
         """
         return hook.hook(self.ui, self, name, throw, **args)
 
-    @unfilteredmethod
     def _tag(self, names, node, message, local, user, date, extra={},
              editor=False):
         if isinstance(names, str):
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -753,3 +753,45 @@ 
   $ hg tags
   visible                            0:193e9254ce7e
   tip                                0:193e9254ce7e
+
+  $ hg init a
+  $ cd a
+  $ touch foo
+  $ hg add foo
+  $ hg ci -mfoo
+  $ touch bar
+  $ hg add bar
+  $ hg ci -mbar
+  $ hg up 0
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ touch quux
+  $ hg add quux
+  $ hg ci -m quux
+  created new head
+  $ hg up 1
+  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg tag 1.0
+
+  $ hg up 2
+  1 files updated, 0 files merged, 2 files removed, 0 files unresolved
+  $ hg log -G
+  o  3:bc47fc7e1c1d (draft) [tip ] Added tag 1.0 for changeset 50c889141114
+  |
+  | @  2:3d7f255a0081 (draft) [ ] quux
+  | |
+  o |  1:50c889141114 (draft) [1.0 ] bar
+  |/
+  o  0:1f7b0de80e11 (draft) [ ] foo
+  
+  $ hg debugobsolete `getid bar`
+  $ hg debugobsolete `getid 1.0`
+  $ hg tag 1.0
+  $ hg log -G
+  @  4:f9f2ab71ffd5 (draft) [tip ] Added tag 1.0 for changeset 3d7f255a0081
+  |
+  o  2:3d7f255a0081 (draft) [1.0 ] quux
+  |
+  o  0:1f7b0de80e11 (draft) [ ] foo
+  
+  $ cat .hgtags
+  3d7f255a008103380aeb2a7d581fe257f40969e7 1.0