Patchwork merge: warn about adding unstable commit

login
register
mail settings
Submitter timeless@mozdev.org
Date Dec. 28, 2015, 7:49 p.m.
Message ID <6b68288cf77cefcea45e.1451332184@waste.org>
Download mbox | patch
Permalink /patch/12379/
State Rejected
Headers show

Comments

timeless@mozdev.org - Dec. 28, 2015, 7:49 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1451330894 0
#      Mon Dec 28 19:28:14 2015 +0000
# Node ID 6b68288cf77cefcea45e168c6bf85cc86f5db52c
# Parent  23541bdd1610c08af247f9c8719045cf247ce541
merge: warn about adding unstable commit
Augie Fackler - Dec. 29, 2015, 11:21 p.m.
On Mon, Dec 28, 2015 at 01:49:44PM -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1451330894 0
> #      Mon Dec 28 19:28:14 2015 +0000
> # Node ID 6b68288cf77cefcea45e168c6bf85cc86f5db52c
> # Parent  23541bdd1610c08af247f9c8719045cf247ce541
> merge: warn about adding unstable commit
>

+1

>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -5202,6 +5202,8 @@
>          displayer.close()
>          return 0
>
> +    if len(repo.revs('(. + %s) and (obsolete() or unstable())' % repo[node])):
> +        ui.warn(_('warning: merge will create an unstable commit\n'))
>      try:
>          # ui.forcemerge is an internal variable, do not document
>          repo.ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), 'merge')
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -408,6 +408,12 @@
>    1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 (Thu Jan 01 00:22:19 1970 +0000) {'user': 'test'}
>    5601fb93a350734d935195fee37f4054c529ff39 6f96419950729f3671185b847352890f074f7557 1 (Thu Jan 01 00:22:18 1970 +0000) {'user': 'test'}
>
> +  $ cd clone-dest
> +  $ hg merge --hidden 5
> +  warning: merge will create an unstable commit
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
> +  $ cd ..
>
>  Destination repo have existing data
>  ---------------------------------------
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 30, 2015, 7:09 p.m.
On 12/30/2015 12:21 AM, Augie Fackler wrote:
> On Mon, Dec 28, 2015 at 01:49:44PM -0600, timeless wrote:
>> # HG changeset patch
>> # User timeless <timeless@mozdev.org>
>> # Date 1451330894 0
>> #      Mon Dec 28 19:28:14 2015 +0000
>> # Node ID 6b68288cf77cefcea45e168c6bf85cc86f5db52c
>> # Parent  23541bdd1610c08af247f9c8719045cf247ce541
>> merge: warn about adding unstable commit
>>
>
> +1

Not really enthusiastic. What happen here is that you have an obsolete 
working copy parent and that will lead to unstable changeset on commit.

We -already- have a message for obsolete working copy parent. It live in 
the evolve extensions. (used by various command including hg update` and 
`hg pull`)
We should:

1) Use the same message in both case,
2) improve that unified message if we think we should,
3) (probably) move this message from evolve into code now that we have a 
clearer policy about "This is experimental stuff in Core, we can change 
it until we lift the experimental adjective to t

Let me know if my feedback is clear enough.
timeless - Dec. 31, 2015, 3:26 p.m.
Pierre-Yves David wrote:
> Not really enthusiastic.

> What happen here is that you have an obsolete
> working copy parent and that will lead to unstable changeset on commit.

Correct

> We -already- have a message for obsolete working copy parent.
> It live in the evolve extensions.

something parent related:
        ui.warn(_('working directory parent is obsolete!\n'))
        if (not ui.quiet) and obsolete.isenabled(repo, commandopt):
            ui.warn(_('(use "hg evolve" to update to its successor)\n'))

something generic:
        ui.warn(_('%i new unstable changesets\n') % newunstables)

These are the only messages I could find. They aren't very helpful.

> (used by various command including hg update` and `hg pull`)

Pulls are things which can happen automatically in unrelated actions.
I certainly don't pay attention to them.

> We should:
>
> 1) Use the same message in both case,

I'm +0..-1/2 on this. I'm not sure what the message in evolve says (I
looked and found the two I mentioned above), but if it says "+2000
unstable", that's absolutely not a helpful message.

Messages need to be *relevant* to the current action, if they're
unrelated/irrelevant, they aren't helpful, and might as well be noise
(and thus would be better served by being omitted).

> 2) improve that unified message if we think we should,

I'm leaning toward -1/2 on "unified".
I'm +1 on trying to improve the evolve ux, although not this year
(i.e. 2015), and I'd really request someone set up a pushgate.

> 3) (probably) move this message from evolve into code now that we have a
> clearer policy about "This is experimental stuff in Core, we can change it
> until we lift the experimental adjective to t

The terminology bothers me a bunch of ways.
But, I can definitely have obsolete markers from histedit/rebase +
exchange, so an out of core thing is definitely the wrong place for
them.

> Let me know if my feedback is clear enough.

Your feedback is clear.

FWIW, an alternative to setting up a pushgate is just to move evolve
into core's hgext with a big warning saying that it is going to change
a lot. I'm not sure how we feel about that (if anyone wants to reply
on this point, please create a new thread).
Sean Farley - Dec. 31, 2015, 7:39 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 12/30/2015 12:21 AM, Augie Fackler wrote:
>> On Mon, Dec 28, 2015 at 01:49:44PM -0600, timeless wrote:
>>> # HG changeset patch
>>> # User timeless <timeless@mozdev.org>
>>> # Date 1451330894 0
>>> #      Mon Dec 28 19:28:14 2015 +0000
>>> # Node ID 6b68288cf77cefcea45e168c6bf85cc86f5db52c
>>> # Parent  23541bdd1610c08af247f9c8719045cf247ce541
>>> merge: warn about adding unstable commit
>>>
>>
>> +1
>
> Not really enthusiastic. What happen here is that you have an obsolete 
> working copy parent and that will lead to unstable changeset on commit.
>
> We -already- have a message for obsolete working copy parent. It live in 
> the evolve extensions. (used by various command including hg update` and 
> `hg pull`)
> We should:
>
> 1) Use the same message in both case,
> 2) improve that unified message if we think we should,
> 3) (probably) move this message from evolve into code now that we have a 
> clearer policy about "This is experimental stuff in Core, we can change 
> it until we lift the experimental adjective to t

It can also happen if some kind of pointer exists on an obsolete
changeset (such as a bookmark named 'foo') and the user is on another
head and types 'hg merge foo'.

We'll also need a utility function on bitbucket to determine when a user
is trying to do this in the case of a pull request. Just leaving my
thoughts here before I forget.
timeless - Dec. 31, 2015, 8:50 p.m.
Bookmarks are indeed how I hit this...
On Dec 31, 2015 2:40 PM, "Sean Farley" <sean@farley.io> wrote:

>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
> > On 12/30/2015 12:21 AM, Augie Fackler wrote:
> >> On Mon, Dec 28, 2015 at 01:49:44PM -0600, timeless wrote:
> >>> # HG changeset patch
> >>> # User timeless <timeless@mozdev.org>
> >>> # Date 1451330894 0
> >>> #      Mon Dec 28 19:28:14 2015 +0000
> >>> # Node ID 6b68288cf77cefcea45e168c6bf85cc86f5db52c
> >>> # Parent  23541bdd1610c08af247f9c8719045cf247ce541
> >>> merge: warn about adding unstable commit
> >>>
> >>
> >> +1
> >
> > Not really enthusiastic. What happen here is that you have an obsolete
> > working copy parent and that will lead to unstable changeset on commit.
> >
> > We -already- have a message for obsolete working copy parent. It live in
> > the evolve extensions. (used by various command including hg update` and
> > `hg pull`)
> > We should:
> >
> > 1) Use the same message in both case,
> > 2) improve that unified message if we think we should,
> > 3) (probably) move this message from evolve into code now that we have a
> > clearer policy about "This is experimental stuff in Core, we can change
> > it until we lift the experimental adjective to t
>
> It can also happen if some kind of pointer exists on an obsolete
> changeset (such as a bookmark named 'foo') and the user is on another
> head and types 'hg merge foo'.
>
> We'll also need a utility function on bitbucket to determine when a user
> is trying to do this in the case of a pull request. Just leaving my
> thoughts here before I forget.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Jan. 2, 2016, 12:13 p.m.
On 12/31/2015 04:26 PM, timeless wrote:
> Pierre-Yves David wrote:
>> Not really enthusiastic.
>
>> What happen here is that you have an obsolete
>> working copy parent and that will lead to unstable changeset on commit.
>
> Correct
>
>> We -already- have a message for obsolete working copy parent.
>> It live in the evolve extensions.
>
> something parent related:
>          ui.warn(_('working directory parent is obsolete!\n'))
>          if (not ui.quiet) and obsolete.isenabled(repo, commandopt):
>              ui.warn(_('(use "hg evolve" to update to its successor)\n'))
>
> something generic:
>          ui.warn(_('%i new unstable changesets\n') % newunstables)
>
> These are the only messages I could find.

The first one is the one I refer to. When you merge with an obsolete 
changeset, you get and obsolete working copy parent.

>  They aren't very helpful.

Hence (2) improve that unified message if we think we should,

>
>> (used by various command including hg update` and `hg pull`)
>
> Pulls are things which can happen automatically in unrelated actions.
> I certainly don't pay attention to them.
>
>> We should:
>>
>> 1) Use the same message in both case,
>
> I'm +0..-1/2 on this. I'm not sure what the message in evolve says (I
> looked and found the two I mentioned above), but if it says "+2000
> unstable", that's absolutely not a helpful message.
>
> Messages need to be *relevant* to the current action, if they're
> unrelated/irrelevant, they aren't helpful, and might as well be noise
> (and thus would be better served by being omitted).

I do not understant what you are trying to convey here.

>> 2) improve that unified message if we think we should,
>
> I'm leaning toward -1/2 on "unified".
> I'm +1 on trying to improve the evolve ux, although not this year
> (i.e. 2015), and I'd really request someone set up a pushgate.

Unified is very important here. Message related to having an obsolete 
working copy parent have no reason to be different. Do you have argument 
in the other direction?

>> 3) (probably) move this message from evolve into code now that we have a
>> clearer policy about "This is experimental stuff in Core, we can change it
>> until we lift the experimental adjective to t
>
> The terminology bothers me a bunch of ways.

Which are?

> But, I can definitely have obsolete markers from histedit/rebase +
> exchange, so an out of core thing is definitely the wrong place for
> them.

No. When we move obsmarker creation into core we left the message in 
evolve for two reasons:
- backward compatibility on output meant we could not change them even 
(I believe this policy have been relaxed)
- performance, some of the messages involves naif and expensive computation.

>> Let me know if my feedback is clear enough.
>
> Your feedback is clear.
>
> FWIW, an alternative to setting up a pushgate is just to move evolve
> into core's hgext with a big warning saying that it is going to change
> a lot. I'm not sure how we feel about that (if anyone wants to reply
> on this point, please create a new thread).

There is already discussion about plan about this.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -5202,6 +5202,8 @@ 
         displayer.close()
         return 0
 
+    if len(repo.revs('(. + %s) and (obsolete() or unstable())' % repo[node])):
+        ui.warn(_('warning: merge will create an unstable commit\n'))
     try:
         # ui.forcemerge is an internal variable, do not document
         repo.ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), 'merge')
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -408,6 +408,12 @@ 
   1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 (Thu Jan 01 00:22:19 1970 +0000) {'user': 'test'}
   5601fb93a350734d935195fee37f4054c529ff39 6f96419950729f3671185b847352890f074f7557 1 (Thu Jan 01 00:22:18 1970 +0000) {'user': 'test'}
 
+  $ cd clone-dest
+  $ hg merge --hidden 5
+  warning: merge will create an unstable commit
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ cd ..
 
 Destination repo have existing data
 ---------------------------------------