Patchwork [V2] changelog: fix bug in native head computation

login
register
mail settings
Submitter Laurent Charignon
Date May 21, 2015, 7:56 p.m.
Message ID <86a3b8c985bd1d7ab831.1432238206@lcharignon-mbp.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/9216/
State Changes Requested
Headers show

Comments

Laurent Charignon - May 21, 2015, 7:56 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1432236833 25200
#      Thu May 21 12:33:53 2015 -0700
# Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
# Parent  451df92cec4912aefac57a4cf82e9268192c867b
changelog: fix bug in native head computation

Native head computations was not taking filtering properly into account pending
changes in the index. We also add a test to avoid introducing regression.
Laurent Charignon - May 21, 2015, 8:04 p.m.
I am not very familiar with the pending changes in index.
If anyone can think of an easier test case, feel free to suggest it.

Thanks,

Laurent

On 5/21/15, 12:56 PM, "Laurent Charignon" <lcharignon@fb.com> wrote:

># HG changeset patch
># User Laurent Charignon <lcharignon@fb.com>
># Date 1432236833 25200
>#      Thu May 21 12:33:53 2015 -0700
># Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
># Parent  451df92cec4912aefac57a4cf82e9268192c867b
>changelog: fix bug in native head computation
>
>Native head computations was not taking filtering properly into account
>pending
>changes in the index. We also add a test to avoid introducing regression.
>
>diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>--- a/mercurial/parsers.c
>+++ b/mercurial/parsers.c
>@@ -1263,7 +1263,7 @@
> 			goto bail;
> 		}
> 
>-		isfiltered = check_filter(filter, i);
>+		isfiltered = check_filter(filter, i + self->raw_length);
> 		if (isfiltered == -1) {
> 			PyErr_SetString(PyExc_TypeError,
> 				"unable to check filter");
>@@ -1271,7 +1271,7 @@
> 		}
> 
> 		if (isfiltered) {
>-			nothead[i] = 1;
>+			nothead[i + self->raw_length] = 1;
> 			continue;
> 		}
> 
>diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
>--- a/tests/test-obsolete.t
>+++ b/tests/test-obsolete.t
>@@ -883,6 +883,35 @@
>   200 Script output follows
> 
>   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
>-
> #endif
> 
>+Test heads computation on pending index changes with obsolescence markers
>+  $ cd ..
>+  $ cat >$TESTTMP/test_extension.py  << EOF
>+  > from mercurial import cmdutil
>+  > from mercurial.i18n import _
>+  > 
>+  > cmdtable = {}
>+  > command = cmdutil.command(cmdtable)
>+  > @command("amendtransient",[], _('hg amendtransient [rev]'))
>+  > def amend(ui, repo, *pats, **opts):
>+  >   def commitfunc(ui, repo, message, match, opts):
>+  >     return repo.commit(message, repo['.'].user(), repo['.'].date(),
>match)
>+  >   opts['message'] = 'Test'
>+  >   opts['logfile'] = None
>+  >   cmdutil.amend(ui, repo, commitfunc, repo['.'], {}, pats, opts)
>+  >   print repo.changelog.headrevs()
>+  > EOF
>+  $ cat >> $HGRCPATH << EOF
>+  > [extensions]
>+  > testextension=$TESTTMP/test_extension.py
>+  > EOF
>+  $ hg init repo-issue-nativerevs-pending-changes
>+  $ cd repo-issue-nativerevs-pending-changes
>+  $ mkcommit a
>+  $ mkcommit b
>+  $ hg up ".^"
>+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>+  $ echo aa > a
>+  $ hg amendtransient
>+  [1, 3]
>_______________________________________________
>Mercurial-devel mailing list
>Mercurial-devel@selenic.com
>http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - May 21, 2015, 9:15 p.m.
On Thu, 2015-05-21 at 12:56 -0700, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1432236833 25200
> #      Thu May 21 12:33:53 2015 -0700
> # Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
> # Parent  451df92cec4912aefac57a4cf82e9268192c867b
> changelog: fix bug in native head computation
> 
> Native head computations was not taking filtering properly into account pending
> changes in the index. We also add a test to avoid introducing regression.
> 
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1263,7 +1263,7 @@
>  			goto bail;
>  		}
>  
> -		isfiltered = check_filter(filter, i);
> +		isfiltered = check_filter(filter, i + self->raw_length);

This seems very mysterious to me?

The index object stores two pieces: the packed form from disk.. and a
list of elements we've added in memory. The number of elements from the
first part is self->raw_length. It's not clear why this has any relation
to filtering.

Instead, it seems:

- this function should iterate over the entire length (locally named
'len')
- index_deref only handles up to raw_length
- index_get hands back a Python tuple
- index_get_parents does what you want
Laurent Charignon - May 21, 2015, 10:57 p.m.
On 5/21/15, 2:15 PM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Thu, 2015-05-21 at 12:56 -0700, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1432236833 25200
>> #      Thu May 21 12:33:53 2015 -0700
>> # Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
>> # Parent  451df92cec4912aefac57a4cf82e9268192c867b
>> changelog: fix bug in native head computation
>> 
>> Native head computations was not taking filtering properly into account
>>pending
>> changes in the index. We also add a test to avoid introducing
>>regression.
>> 
>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>> --- a/mercurial/parsers.c
>> +++ b/mercurial/parsers.c
>> @@ -1263,7 +1263,7 @@
>>  			goto bail;
>>  		}
>>  
>> -		isfiltered = check_filter(filter, i);
>> +		isfiltered = check_filter(filter, i + self->raw_length);
>
>This seems very mysterious to me?

We were just checking the wrong index in the filter, this works by adding
the offset in the second loop.

You are right and it is much better to simplify this function to avoid the
two loops, I will send a V3.


>
>The index object stores two pieces: the packed form from disk.. and a
>list of elements we've added in memory. The number of elements from the
>first part is self->raw_length. It's not clear why this has any relation
>to filtering.
>
>Instead, it seems:
>
>- this function should iterate over the entire length (locally named
>'len')
>- index_deref only handles up to raw_length
>- index_get hands back a Python tuple
>- index_get_parents does what you want
>
>-- 
>Mathematics is the supreme nostalgia of our time.
>
Pierre-Yves David - May 22, 2015, 4:20 a.m.
On 05/21/2015 05:57 PM, Laurent Charignon wrote:
>
>
> On 5/21/15, 2:15 PM, "Matt Mackall" <mpm@selenic.com> wrote:
>
>> On Thu, 2015-05-21 at 12:56 -0700, Laurent Charignon wrote:
>>> # HG changeset patch
>>> # User Laurent Charignon <lcharignon@fb.com>
>>> # Date 1432236833 25200
>>> #      Thu May 21 12:33:53 2015 -0700
>>> # Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
>>> # Parent  451df92cec4912aefac57a4cf82e9268192c867b
>>> changelog: fix bug in native head computation

I know this function take its inspiration from another one. We should 
probably simplify it in the same go if it still exist. Durham do you 
remember what was your base?
Durham Goode - May 22, 2015, 4:42 a.m.
On 5/21/15, 9:20 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 05/21/2015 05:57 PM, Laurent Charignon wrote:
>>
>>
>> On 5/21/15, 2:15 PM, "Matt Mackall" <mpm@selenic.com> wrote:
>>
>>> On Thu, 2015-05-21 at 12:56 -0700, Laurent Charignon wrote:
>>>> # HG changeset patch
>>>> # User Laurent Charignon <lcharignon@fb.com>
>>>> # Date 1432236833 25200
>>>> #      Thu May 21 12:33:53 2015 -0700
>>>> # Node ID 86a3b8c985bd1d7ab831ccf5f00ddfba266c9e58
>>>> # Parent  451df92cec4912aefac57a4cf82e9268192c867b
>>>> changelog: fix bug in native head computation
>
>I know this function take its inspiration from another one. We should
>probably simplify it in the same go if it still exist. Durham do you
>remember what was your base?

I don't remember this particular part being inspired by some place else
and I don't see anything like it when I scan through parsers.c, so this
part may be unique to here.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1263,7 +1263,7 @@ 
 			goto bail;
 		}
 
-		isfiltered = check_filter(filter, i);
+		isfiltered = check_filter(filter, i + self->raw_length);
 		if (isfiltered == -1) {
 			PyErr_SetString(PyExc_TypeError,
 				"unable to check filter");
@@ -1271,7 +1271,7 @@ 
 		}
 
 		if (isfiltered) {
-			nothead[i] = 1;
+			nothead[i + self->raw_length] = 1;
 			continue;
 		}
 
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -883,6 +883,35 @@ 
   200 Script output follows
 
   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
-
 #endif
 
+Test heads computation on pending index changes with obsolescence markers
+  $ cd ..
+  $ cat >$TESTTMP/test_extension.py  << EOF
+  > from mercurial import cmdutil
+  > from mercurial.i18n import _
+  > 
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > @command("amendtransient",[], _('hg amendtransient [rev]'))
+  > def amend(ui, repo, *pats, **opts):
+  >   def commitfunc(ui, repo, message, match, opts):
+  >     return repo.commit(message, repo['.'].user(), repo['.'].date(), match)
+  >   opts['message'] = 'Test'
+  >   opts['logfile'] = None
+  >   cmdutil.amend(ui, repo, commitfunc, repo['.'], {}, pats, opts)
+  >   print repo.changelog.headrevs()
+  > EOF
+  $ cat >> $HGRCPATH << EOF
+  > [extensions]
+  > testextension=$TESTTMP/test_extension.py
+  > EOF
+  $ hg init repo-issue-nativerevs-pending-changes
+  $ cd repo-issue-nativerevs-pending-changes
+  $ mkcommit a
+  $ mkcommit b
+  $ hg up ".^"
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo aa > a
+  $ hg amendtransient
+  [1, 3]