Patchwork exchange: identify the troubled commit

login
register
mail settings
Submitter timeless@mozdev.org
Date Dec. 8, 2015, 4:40 a.m.
Message ID <7835c3c8bf67c5213c21.1449549625@waste.org>
Download mbox | patch
Permalink /patch/11923/
State Rejected
Delegated to: Pierre-Yves David
Headers show

Comments

timeless@mozdev.org - Dec. 8, 2015, 4:40 a.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1449549446 0
#      Tue Dec 08 04:37:26 2015 +0000
# Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
# Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
exchange: identify the troubled commit

Exchange iterates over heads to quickly identify if everything is
OK. But, users don't want to know the revision of the bad head,
they need to know the revision of the bad commit.

So, instead of identifying the faulty head, once we know there's
a faulty head, iterate over all of outgoing and identify the
specific commit that is troubled.

This could be improved by only looking at revisions relating to
the faulty head. But, since we're already failing, there's no real
need for an optimized algorithm.
Sean Farley - Dec. 8, 2015, 5:05 a.m.
timeless <timeless@mozdev.org> writes:

> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1449549446 0
> #      Tue Dec 08 04:37:26 2015 +0000
> # Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
> # Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
> exchange: identify the troubled commit
>
> Exchange iterates over heads to quickly identify if everything is
> OK. But, users don't want to know the revision of the bad head,
> they need to know the revision of the bad commit.
>
> So, instead of identifying the faulty head, once we know there's
> a faulty head, iterate over all of outgoing and identify the
> specific commit that is troubled.
>
> This could be improved by only looking at revisions relating to
> the faulty head. But, since we're already failing, there's no real
> need for an optimized algorithm.

I tend to agree with timeless here. While we can bikeshed more here, I
think this is a step in the right direction. Thoughts from others?
Pierre-Yves David - Dec. 8, 2015, 9:53 a.m.
On 12/07/2015 08:40 PM, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1449549446 0
> #      Tue Dec 08 04:37:26 2015 +0000
> # Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
> # Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
> exchange: identify the troubled commit
>
> Exchange iterates over heads to quickly identify if everything is
> OK. But, users don't want to know the revision of the bad head,
> they need to know the revision of the bad commit.
>
> So, instead of identifying the faulty head, once we know there's
> a faulty head, iterate over all of outgoing and identify the
> specific commit that is troubled.
>
> This could be improved by only looking at revisions relating to
> the faulty head. But, since we're already failing, there's no real
> need for an optimized algorithm.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -569,7 +569,10 @@
>                   if ctx.obsolete():
>                       raise error.Abort(mso % ctx)
>                   elif ctx.troubled():
> -                    raise error.Abort(mst[ctx.troubles()[0]] % ctx)
> +                    for tnode in outgoing.missing:
> +                        tctx = unfi[tnode]
> +                        if tctx.troubled():
> +                            raise error.Abort(mst[tctx.troubles()[0]] % tctx)

This change does not make too much sense to me in regard with the 
changesets description.

The way I read the changesets description, it looks like that:

   The warn about head X being bad while actually Y is the bad one.


However, reading the code,

If ctx.troubled() is True, There -is- trouble on ctx and reporting 
troubles on ctx isn't false.

I'm not sure what you are trying to achieve here.



In the other hand, this patch make me see an interresting step to 
improve this message. Looking at all outgoing changesets on error seems 
reasonable (ever If I would use set logic instead of ctx for speed 
purpose). We could give a more details report of "other issue"

   abort: push includes divergent changeset: 392fd25390da
   (and also other 42 unstables and 3 divergents changesets)
Sean Farley - Dec. 8, 2015, 9:19 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 12/07/2015 08:40 PM, timeless wrote:
>> # HG changeset patch
>> # User timeless <timeless@mozdev.org>
>> # Date 1449549446 0
>> #      Tue Dec 08 04:37:26 2015 +0000
>> # Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
>> # Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
>> exchange: identify the troubled commit
>>
>> Exchange iterates over heads to quickly identify if everything is
>> OK. But, users don't want to know the revision of the bad head,
>> they need to know the revision of the bad commit.
>>
>> So, instead of identifying the faulty head, once we know there's
>> a faulty head, iterate over all of outgoing and identify the
>> specific commit that is troubled.
>>
>> This could be improved by only looking at revisions relating to
>> the faulty head. But, since we're already failing, there's no real
>> need for an optimized algorithm.
>>
>> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>> --- a/mercurial/exchange.py
>> +++ b/mercurial/exchange.py
>> @@ -569,7 +569,10 @@
>>                   if ctx.obsolete():
>>                       raise error.Abort(mso % ctx)
>>                   elif ctx.troubled():
>> -                    raise error.Abort(mst[ctx.troubles()[0]] % ctx)
>> +                    for tnode in outgoing.missing:
>> +                        tctx = unfi[tnode]
>> +                        if tctx.troubled():
>> +                            raise error.Abort(mst[tctx.troubles()[0]] % tctx)
>
> This change does not make too much sense to me in regard with the 
> changesets description.
>
> The way I read the changesets description, it looks like that:
>
>    The warn about head X being bad while actually Y is the bad one.
>
>
> However, reading the code,
>
> If ctx.troubled() is True, There -is- trouble on ctx and reporting 
> troubles on ctx isn't false.
>
> I'm not sure what you are trying to achieve here.

Yeah, hard to argue with that. I'm guessing the issue here is that both
X and Y are considered divergent and timeless had a hard time figuring
out why. Using your hash example below, a user will want a hg command:

$ hg show-me-why-this-is-divergent 392fd25390da

And for that we need to think about a UI (Pierre-Yves already knows this
probably). Perhaps, we could use our future journal store to provide the
diagnostics?

$ hg show-me-why-this-is-divergent 392fd25390da
392fd25390da was obsoleted by changeset Y when you pulled on DATETIME

Somewhere (and these are also notes for future me), we need to have docs
say,

1) Either mark X or Y as obsolete (can we provide a candidate based on
phase / journal / remotes?)

2) Use 'hg touch --duplicate' to keep the changes from X but create a
new hash

> In the other hand, this patch make me see an interresting step to 
> improve this message. Looking at all outgoing changesets on error seems 
> reasonable (ever If I would use set logic instead of ctx for speed 
> purpose). We could give a more details report of "other issue"
>
>    abort: push includes divergent changeset: 392fd25390da
>    (and also other 42 unstables and 3 divergents changesets)

Providing more details would be awesome.
Pierre-Yves David - Dec. 8, 2015, 9:50 p.m.
On 12/08/2015 01:19 PM, Sean Farley wrote:
>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 12/07/2015 08:40 PM, timeless wrote:
>>> # HG changeset patch
>>> # User timeless <timeless@mozdev.org>
>>> # Date 1449549446 0
>>> #      Tue Dec 08 04:37:26 2015 +0000
>>> # Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
>>> # Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
>>> exchange: identify the troubled commit
>>>
>>> Exchange iterates over heads to quickly identify if everything is
>>> OK. But, users don't want to know the revision of the bad head,
>>> they need to know the revision of the bad commit.
>>>
>>> So, instead of identifying the faulty head, once we know there's
>>> a faulty head, iterate over all of outgoing and identify the
>>> specific commit that is troubled.
>>>
>>> This could be improved by only looking at revisions relating to
>>> the faulty head. But, since we're already failing, there's no real
>>> need for an optimized algorithm.
>>>
>>> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>>> --- a/mercurial/exchange.py
>>> +++ b/mercurial/exchange.py
>>> @@ -569,7 +569,10 @@
>>>                    if ctx.obsolete():
>>>                        raise error.Abort(mso % ctx)
>>>                    elif ctx.troubled():
>>> -                    raise error.Abort(mst[ctx.troubles()[0]] % ctx)
>>> +                    for tnode in outgoing.missing:
>>> +                        tctx = unfi[tnode]
>>> +                        if tctx.troubled():
>>> +                            raise error.Abort(mst[tctx.troubles()[0]] % tctx)
>>
>> This change does not make too much sense to me in regard with the
>> changesets description.
>>
>> The way I read the changesets description, it looks like that:
>>
>>     The warn about head X being bad while actually Y is the bad one.
>>
>>
>> However, reading the code,
>>
>> If ctx.troubled() is True, There -is- trouble on ctx and reporting
>> troubles on ctx isn't false.
>>
>> I'm not sure what you are trying to achieve here.
>
> Yeah, hard to argue with that. I'm guessing the issue here is that both
> X and Y are considered divergent and timeless had a hard time figuring
> out why. Using your hash example below, a user will want a hg command:
>
> $ hg show-me-why-this-is-divergent 392fd25390da

The next step on this topic is to get `hg evolve --list` running.

https://bz.mercurial-scm.org/show_bug.cgi?id=4845

Laurent started playing with it and got to the point were we realised 
that the formatter could not handle lists of lists.

Ping me if you need more details to move forward here.
Sean Farley - Dec. 8, 2015, 10:09 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 12/08/2015 01:19 PM, Sean Farley wrote:
>>
>> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>>
>>> On 12/07/2015 08:40 PM, timeless wrote:
>>>> # HG changeset patch
>>>> # User timeless <timeless@mozdev.org>
>>>> # Date 1449549446 0
>>>> #      Tue Dec 08 04:37:26 2015 +0000
>>>> # Node ID 7835c3c8bf67c5213c21276645442e21e4180b7d
>>>> # Parent  42aa0e570eaa364a622bc4443b0bcb79b1100a58
>>>> exchange: identify the troubled commit
>>>>
>>>> Exchange iterates over heads to quickly identify if everything is
>>>> OK. But, users don't want to know the revision of the bad head,
>>>> they need to know the revision of the bad commit.
>>>>
>>>> So, instead of identifying the faulty head, once we know there's
>>>> a faulty head, iterate over all of outgoing and identify the
>>>> specific commit that is troubled.
>>>>
>>>> This could be improved by only looking at revisions relating to
>>>> the faulty head. But, since we're already failing, there's no real
>>>> need for an optimized algorithm.
>>>>
>>>> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>>>> --- a/mercurial/exchange.py
>>>> +++ b/mercurial/exchange.py
>>>> @@ -569,7 +569,10 @@
>>>>                    if ctx.obsolete():
>>>>                        raise error.Abort(mso % ctx)
>>>>                    elif ctx.troubled():
>>>> -                    raise error.Abort(mst[ctx.troubles()[0]] % ctx)
>>>> +                    for tnode in outgoing.missing:
>>>> +                        tctx = unfi[tnode]
>>>> +                        if tctx.troubled():
>>>> +                            raise error.Abort(mst[tctx.troubles()[0]] % tctx)
>>>
>>> This change does not make too much sense to me in regard with the
>>> changesets description.
>>>
>>> The way I read the changesets description, it looks like that:
>>>
>>>     The warn about head X being bad while actually Y is the bad one.
>>>
>>>
>>> However, reading the code,
>>>
>>> If ctx.troubled() is True, There -is- trouble on ctx and reporting
>>> troubles on ctx isn't false.
>>>
>>> I'm not sure what you are trying to achieve here.
>>
>> Yeah, hard to argue with that. I'm guessing the issue here is that both
>> X and Y are considered divergent and timeless had a hard time figuring
>> out why. Using your hash example below, a user will want a hg command:
>>
>> $ hg show-me-why-this-is-divergent 392fd25390da
>
> The next step on this topic is to get `hg evolve --list` running.
>
> https://bz.mercurial-scm.org/show_bug.cgi?id=4845
>
> Laurent started playing with it and got to the point were we realised 
> that the formatter could not handle lists of lists.
>
> Ping me if you need more details to move forward here.

Ah, that's great. I'm working on merge code now but would love to see
some work in this area :-D

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -569,7 +569,10 @@ 
                 if ctx.obsolete():
                     raise error.Abort(mso % ctx)
                 elif ctx.troubled():
-                    raise error.Abort(mst[ctx.troubles()[0]] % ctx)
+                    for tnode in outgoing.missing:
+                        tctx = unfi[tnode]
+                        if tctx.troubled():
+                            raise error.Abort(mst[tctx.troubles()[0]] % tctx)
 
         discovery.checkheads(pushop)
     return True
diff --git a/tests/test-obsolete-divergent.t b/tests/test-obsolete-divergent.t
--- a/tests/test-obsolete-divergent.t
+++ b/tests/test-obsolete-divergent.t
@@ -87,7 +87,7 @@ 
   $ hg push ../other
   pushing to ../other
   searching for changes
-  abort: push includes divergent changeset: 392fd25390da!
+  abort: push includes divergent changeset: 82623d38b9ba!
   [255]
 
   $ cd ..