Patchwork [3,of,6,mergedriver] mergestate: add a constructor that sets up a fresh merge state

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 17, 2015, 11:58 p.m.
Message ID <bb1b25b554d6efe340f3.1447804721@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11447/
State Changes Requested
Headers show

Comments

Siddharth Agarwal - Nov. 17, 2015, 11:58 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1447797080 28800
#      Tue Nov 17 13:51:20 2015 -0800
# Node ID bb1b25b554d6efe340f3c74b58fec40cd56fd557
# Parent  a879917dec18a4621476c8af18724f5d5696c663
mergestate: add a constructor that sets up a fresh merge state

Eventually, we'll move the read call out of the constructor. This will:
- avoid unnecessary reads when we're going to nuke the merge state anyway
- avoid raising an exception if there's an unsupported merge record

I chose 'fresh' for the name because I wanted to avoid anything with the word
'new' in it, and 'reset' is more an action performed on a merge state than a
way to get a new merge state.
Martin von Zweigbergk - Nov. 18, 2015, 12:20 a.m.
On Tue, Nov 17, 2015 at 4:01 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1447797080 28800
> #      Tue Nov 17 13:51:20 2015 -0800
> # Node ID bb1b25b554d6efe340f3c74b58fec40cd56fd557
> # Parent  a879917dec18a4621476c8af18724f5d5696c663
> mergestate: add a constructor that sets up a fresh merge state
>
> Eventually, we'll move the read call out of the constructor. This will:
> - avoid unnecessary reads when we're going to nuke the merge state anyway
> - avoid raising an exception if there's an unsupported merge record
>
> I chose 'fresh' for the name because I wanted to avoid anything with the
> word
> 'new' in it, and 'reset' is more an action performed on a merge state than
> a
> way to get a new merge state.
>

When I saw patch 4/6, I was wondering how the merge state got removed from
disk. It turns out that ms.reset() does that. So this method (fresh()) *is*
a method for performing an action after all, right?


>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -75,6 +75,14 @@ class mergestate(object):
>      statepathv1 = 'merge/state'
>      statepathv2 = 'merge/state2'
>
> +    @staticmethod
> +    def fresh(repo, node=None, other=None):
> +        """Initialize a fresh merge state, removing any existing state on
> +        disk."""
> +        ms = mergestate(repo)
> +        ms.reset(node, other)
> +        return ms
> +
>      def __init__(self, repo):
>          self._repo = repo
>          self._dirty = False
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 18, 2015, 12:24 a.m.
On 11/17/15 16:20, Martin von Zweigbergk wrote:
> When I saw patch 4/6, I was wondering how the merge state got removed 
> from disk. It turns out that ms.reset() does that. So this method 
> (fresh()) *is* a method for performing an action after all, right?

Right, but something like mergestate.reset() returning a new merge state 
sounds really weird to my (admittedly non-native) ears.
Martin von Zweigbergk - Nov. 18, 2015, 12:33 a.m.
On Tue, Nov 17, 2015 at 4:25 PM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/17/15 16:20, Martin von Zweigbergk wrote:
> > When I saw patch 4/6, I was wondering how the merge state got removed
> > from disk. It turns out that ms.reset() does that. So this method
> > (fresh()) *is* a method for performing an action after all, right?
>
> Right, but something like mergestate.reset() returning a new merge state
> sounds really weird to my (admittedly non-native) ears.


I agree, but I'd rather have something that sounds like it does something
also return something than have something that sounds like it only return
something also do something. Unexpected side-effects are worse than
unexpected return values. It's pretty common to have methods like
list.pop() that modify the list and return something, after all. Not a
perfect analogy, I know. Maybe some other name?
Siddharth Agarwal - Nov. 18, 2015, 12:49 a.m.
On 11/17/15 16:33, Martin von Zweigbergk wrote:
> I agree, but I'd rather have something that sounds like it does 
> something also return something than have something that sounds like 
> it only return something also do something. Unexpected side-effects 
> are worse than unexpected return values. It's pretty common to have 
> methods like list.pop() that modify the list and return something, 
> after all. Not a perfect analogy, I know. Maybe some other name?

OK -- did 'fresh' not convey that to you?

'new' is similar but is a massively overloaded term in programming.

What about 'newreset'? Then the other one, which I'm calling 'read' 
right now, can be 'newread'.

- Siddharth
Martin von Zweigbergk - Nov. 18, 2015, 12:57 a.m.
On Tue, Nov 17, 2015 at 4:49 PM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/17/15 16:33, Martin von Zweigbergk wrote:
> > I agree, but I'd rather have something that sounds like it does
> > something also return something than have something that sounds like
> > it only return something also do something. Unexpected side-effects
> > are worse than unexpected return values. It's pretty common to have
> > methods like list.pop() that modify the list and return something,
> > after all. Not a perfect analogy, I know. Maybe some other name?
>
> OK -- did 'fresh' not convey that to you?
>

I think the reason it didn't convey that is because I read your explanation
saying that you didn't like 'reset' because it is more like an action, so I
interpreted that as saying that 'fresh' is not an action. If I had just
seen the method name without a motivation, I'm not sure what I would have
thought. I would probably have looked at the docstring, which does say that
it removes state on disk.


>
> 'new' is similar but is a massively overloaded term in programming.
>
> What about 'newreset'? Then the other one, which I'm calling 'read'
> right now, can be 'newread'.
>
> - Siddharth
>

I think I would prefer mergestate.clean(). I would not be surprised that it
returns the clean state. But then again, perhaps 'fresh' would also be fine
if looking at it without preconception. You pick. If you choose to keep
'fresh', I can just queue the series.
Siddharth Agarwal - Nov. 18, 2015, 12:58 a.m.
On 11/17/15 16:57, Martin von Zweigbergk wrote:
> I think I would prefer mergestate.clean(). I would not be surprised 
> that it returns the clean state. But then again, perhaps 'fresh' would 
> also be fine if looking at it without preconception. You pick. If you 
> choose to keep 'fresh', I can just queue the series.

I like clean(). If you queue up the first two I'll send a V2 of the next 
four.
Martin von Zweigbergk - Nov. 18, 2015, 1 a.m.
On Tue, Nov 17, 2015 at 4:58 PM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/17/15 16:57, Martin von Zweigbergk wrote:
> > I think I would prefer mergestate.clean(). I would not be surprised
> > that it returns the clean state. But then again, perhaps 'fresh' would
> > also be fine if looking at it without preconception. You pick. If you
> > choose to keep 'fresh', I can just queue the series.
>
> I like clean(). If you queue up the first two I'll send a V2 of the next
> four.
>

Deal!

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -75,6 +75,14 @@  class mergestate(object):
     statepathv1 = 'merge/state'
     statepathv2 = 'merge/state2'
 
+    @staticmethod
+    def fresh(repo, node=None, other=None):
+        """Initialize a fresh merge state, removing any existing state on
+        disk."""
+        ms = mergestate(repo)
+        ms.reset(node, other)
+        return ms
+
     def __init__(self, repo):
         self._repo = repo
         self._dirty = False