Patchwork parsers: it should check the other interesting before decreasing ninteresting (issue3984)

login
register
mail settings
Submitter elson.wei@gmail.com
Date July 25, 2013, 9:53 a.m.
Message ID <024085d8dd13dcc4db48.1374746033@ElsonWei-NB.PrimeVOLT>
Download mbox | patch
Permalink /patch/1957/
State Superseded
Headers show

Comments

elson.wei@gmail.com - July 25, 2013, 9:53 a.m.
# HG changeset patch
# User Wei, Elson <elson.wei@gmail.com>
# Date 1374744953 -28800
#      Thu Jul 25 17:35:53 2013 +0800
# Branch stable
# Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
# Parent  32e502b26983eaa89574835a024a9b035ad72bf4
parsers: it should check the other interesting before decreasing ninteresting (issue3984)
Chingis Dugarzhapov - July 25, 2013, 12:38 p.m.
Hi Elson,

The result of hg log -r "ancestors(10,9)" for 2.2.2 and 2.6.3+patch is
different for me:

2.2.2:

# hg log -r
"ancestor(10,9)"

changeset:   4:f303d6efbf44
branch:      foo
parent:      2:d0f226159ee1
user:        Tavis Elliott <telliott@apptio.com>
date:        Wed Jul 17 09:11:23 2013 -0700
summary:     Added e, modified c

2.6.3+patch:

# hg log -r
"ancestor(10,9)"

changeset:   6:67c8c7ad41bf
branch:      foo
parent:      2:d0f226159ee1
user:        Tavis Elliott <telliott@apptio.com>
date:        Wed Jul 17 09:07:13 2013 -0700
summary:     added d, changed c

It means that

    assert set(ancs) = old

in revision 18988:5bae936764bb still silently fails. Is it normal?

Note that both 4 and 6 are on the same branch foo (from test case), and
they are on the same distance from root.



On Thu, Jul 25, 2013 at 11:53 AM, <elson.wei@gmail.com> wrote:

> # HG changeset patch
> # User Wei, Elson <elson.wei@gmail.com>
> # Date 1374744953 -28800
> #      Thu Jul 25 17:35:53 2013 +0800
> # Branch stable
> # Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
> # Parent  32e502b26983eaa89574835a024a9b035ad72bf4
> parsers: it should check the other interesting before decreasing
> ninteresting (issue3984)
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1359,10 +1359,10 @@
>                                 if (nsp == sp)
>                                         continue;
>                                 seen[p] = nsp;
> +                               interesting[sp] -= 1;
> +                               if (interesting[sp] == 0 &&
> interesting[nsp] > 0)
> +                                       ninteresting -= 1;
>                                 interesting[nsp] += 1;
> -                               interesting[sp] -= 1;
> -                               if (interesting[sp] == 0)
> -                                       ninteresting -= 1;
>                         }
>                 }
>                 interesting[sv] -= 1;
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
>
elson.wei@gmail.com - July 25, 2013, 3:01 p.m.
Yes, in the original version, ancestors.py, the ancestors are sorted before
to get the deepest one (line 77).
But in parser.c, the ancestors are not sorted. That's why you get the
different result.
I was wondering which one is correct. Shouldn't we get the most recent, the
largest number, revision? If the original is correct, I will send another
patch to fix it.

And by the way, does the "root" mean rev 0?

In rev 18988, It will cause exception according to this bug. It's normal.


2013/7/25 Chingis Dugarzhapov <chingis.dug@gmail.com>

> Hi Elson,
>
> The result of hg log -r "ancestors(10,9)" for 2.2.2 and 2.6.3+patch is
> different for me:
>
> 2.2.2:
>
> # hg log -r
> "ancestor(10,9)"
>
> changeset:   4:f303d6efbf44
> branch:      foo
> parent:      2:d0f226159ee1
> user:        Tavis Elliott <telliott@apptio.com>
> date:        Wed Jul 17 09:11:23 2013 -0700
> summary:     Added e, modified c
>
> 2.6.3+patch:
>
> # hg log -r
> "ancestor(10,9)"
>
> changeset:   6:67c8c7ad41bf
> branch:      foo
> parent:      2:d0f226159ee1
> user:        Tavis Elliott <telliott@apptio.com>
> date:        Wed Jul 17 09:07:13 2013 -0700
> summary:     added d, changed c
>
> It means that
>
>     assert set(ancs) = old
>
> in revision 18988:5bae936764bb still silently fails. Is it normal?
>
> Note that both 4 and 6 are on the same branch foo (from test case), and
> they are on the same distance from root.
>
>
>
> On Thu, Jul 25, 2013 at 11:53 AM, <elson.wei@gmail.com> wrote:
>
>> # HG changeset patch
>> # User Wei, Elson <elson.wei@gmail.com>
>> # Date 1374744953 -28800
>> #      Thu Jul 25 17:35:53 2013 +0800
>> # Branch stable
>> # Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
>> # Parent  32e502b26983eaa89574835a024a9b035ad72bf4
>> parsers: it should check the other interesting before decreasing
>> ninteresting (issue3984)
>>
>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>> --- a/mercurial/parsers.c
>> +++ b/mercurial/parsers.c
>> @@ -1359,10 +1359,10 @@
>>                                 if (nsp == sp)
>>                                         continue;
>>                                 seen[p] = nsp;
>> +                               interesting[sp] -= 1;
>> +                               if (interesting[sp] == 0 &&
>> interesting[nsp] > 0)
>> +                                       ninteresting -= 1;
>>                                 interesting[nsp] += 1;
>> -                               interesting[sp] -= 1;
>> -                               if (interesting[sp] == 0)
>> -                                       ninteresting -= 1;
>>                         }
>>                 }
>>                 interesting[sv] -= 1;
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>>
>
Chingis Dugarzhapov - July 25, 2013, 3:42 p.m.
On Thu, Jul 25, 2013 at 5:01 PM, elson <elson.wei@gmail.com> wrote:

> Yes, in the original version, ancestors.py, the ancestors are sorted
> before to get the deepest one (line 77).
> But in parser.c, the ancestors are not sorted. That's why you get the
> different result.
> I was wondering which one is correct. Shouldn't we get the most recent,
> the largest number, revision? If the original is correct, I will send
> another patch to fix it.
>

According to old behavior, deepest function (ancestor.py:71) was returning
*both* 4 and 6, as they are both "deepest". And only manual check
(revlog.py:716) was filtering the winner (rev 4). It's not the case of your
patch ("C-style analogue of deepest" returns only 6).



> And by the way, does the "root" mean rev 0?
>

I suppose yes.



> In rev 18988, It will cause exception according to this bug. It's normal.
>


Yes, but then backward-compatibility check was dropped, for unknown (at
least for me) reasons.
Siddharth Agarwal - July 25, 2013, 3:56 p.m.
On 07/25/2013 02:53 AM, elson.wei@gmail.com wrote:
> # HG changeset patch
> # User Wei, Elson <elson.wei@gmail.com>
> # Date 1374744953 -28800
> #      Thu Jul 25 17:35:53 2013 +0800
> # Branch stable
> # Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
> # Parent  32e502b26983eaa89574835a024a9b035ad72bf4
> parsers: it should check the other interesting before decreasing ninteresting (issue3984)

I'm somewhat familiar with this code, so I'll try poking at this a bit. 
My main concern is that the same repo can have a different rev number 
order in a different clone, and produce different results. That is why 
http://selenic.com/repo/hg/file/3ac1735a2265/mercurial/revlog.py#l705 
compares the SHA1s, which are the same across repositories, to find a 
winner.
Siddharth Agarwal - July 25, 2013, 4:07 p.m.
On 07/25/2013 05:38 AM, Chingis Dugarzhapov wrote:
> Hi Elson,
>
> The result of hg log -r "ancestors(10,9)" for 2.2.2 and 2.6.3+patch is 
> different for me:
>
> 2.2.2:
>
> # hg log -r "ancestor(10,9)"
> changeset:   4:f303d6efbf44
> branch:      foo
> parent:      2:d0f226159ee1
> user:        Tavis Elliott <telliott@apptio.com 
> <mailto:telliott@apptio.com>>
> date:        Wed Jul 17 09:11:23 2013 -0700
> summary:     Added e, modified c
>
> 2.6.3+patch:
>
> # hg log -r "ancestor(10,9)"
> changeset:   6:67c8c7ad41bf
> branch:      foo
> parent:      2:d0f226159ee1
> user:        Tavis Elliott <telliott@apptio.com 
> <mailto:telliott@apptio.com>>
> date:        Wed Jul 17 09:07:13 2013 -0700
> summary:     added d, changed c

Actually, this looks OK -- 2.2.2's output is wrong because it picks the 
lowest rev number and not the lowest SHA1, as explained in my other 
email. Note that 6 has a lower SHA1 than 4.
Siddharth Agarwal - July 25, 2013, 8:10 p.m.
On 07/25/2013 02:53 AM, elson.wei@gmail.com wrote:
> # HG changeset patch
> # User Wei, Elson <elson.wei@gmail.com>
> # Date 1374744953 -28800
> #      Thu Jul 25 17:35:53 2013 +0800
> # Branch stable
> # Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
> # Parent  32e502b26983eaa89574835a024a9b035ad72bf4
> parsers: it should check the other interesting before decreasing ninteresting (issue3984)

This patch LGTM, though a better summary and perhaps a test would be 
nice. With this patch, the invariant that ninteresting is the number of 
non-zero elements in the interesting array is preserved.
Siddharth Agarwal - July 25, 2013, 8:41 p.m.
On 07/25/2013 01:10 PM, Siddharth Agarwal wrote:
> This patch LGTM, though a better summary and perhaps a test would be 
> nice. With this patch, the invariant that ninteresting is the number 
> of non-zero elements in the interesting array is preserved.

Unfortunately, there's another bug in the C code with the calculation of 
the final list. I'll send an updated patchset once I figure out how to 
test this.

Patch

# HG changeset patch
# User Wei, Elson <elson.wei@gmail.com>
# Date 1374744953 -28800
#      Thu Jul 25 17:35:53 2013 +0800
# Branch stable
# Node ID 024085d8dd13dcc4db48c7f99ed04c1ca5864597
# Parent  32e502b26983eaa89574835a024a9b035ad72bf4
parsers: it should check the other interesting before decreasing ninteresting (issue3984)

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1359,10 +1359,10 @@ 
 				if (nsp == sp)
 					continue;
 				seen[p] = nsp;
+				interesting[sp] -= 1;
+				if (interesting[sp] == 0 && interesting[nsp] > 0)
+					ninteresting -= 1;
 				interesting[nsp] += 1;
-				interesting[sp] -= 1;
-				if (interesting[sp] == 0)
-					ninteresting -= 1;
 			}
 		}
 		interesting[sv] -= 1;