Patchwork [1,of,4,V2] hg: combine the remainder of clean() and update() into updaterepo()

login
register
mail settings
Submitter Matt Harbison
Date Jan. 6, 2013, 10:16 p.m.
Message ID <a35f6d98d65f24d5d6f1.1357510598@Envy>
Download mbox | patch
Permalink /patch/403/
State Changes Requested
Headers show

Comments

Matt Harbison - Jan. 6, 2013, 10:16 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison at yahoo.com>
# Date 1357448404 18000
# Node ID a35f6d98d65f24d5d6f1a7928342b2bc479ef44c
# Parent  2c1276825e938872ebc099c191eb202f0dbadfcc
hg: combine the remainder of clean() and update() into updaterepo()

This sets the stage for largefiles to override updaterepo() instead of clean()
and update(), while not altering the order of the largefile changes output and
the updated/merged/removed/unresolved output.  The need to override updaterepo()
is to allow largefiles in a subrepo to be updated when the subrepo is updated.
This change also opens the door to subrepos printing their statistics (it
currently doesn't because it calls updaterepo() without setting showstats,
having previously lacked the capability entirely).

Note that the 'use hg resolve' line was never output for subrepos, because
updaterepo() previously didn't print anything, and prior to 17c030014ddf,
subrepos were only cleaned.  The message probably shouldn't occur with subrepos
because of the '(l)ocal or (r)emote' prompt when merging.
Mads Kiilerich - Jan. 10, 2013, 2:25 p.m.
On 01/06/2013 11:16 PM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1357448404 18000
> # Node ID a35f6d98d65f24d5d6f1a7928342b2bc479ef44c
> # Parent  2c1276825e938872ebc099c191eb202f0dbadfcc
> hg: combine the remainder of clean() and update() into updaterepo()
>
> This sets the stage for largefiles to override updaterepo() instead of clean()
> and update(), while not altering the order of the largefile changes output and
> the updated/merged/removed/unresolved output.  The need to override updaterepo()
> is to allow largefiles in a subrepo to be updated when the subrepo is updated.
> This change also opens the door to subrepos printing their statistics (it
> currently doesn't because it calls updaterepo() without setting showstats,
> having previously lacked the capability entirely).
>
> Note that the 'use hg resolve' line was never output for subrepos, because
> updaterepo() previously didn't print anything, and prior to 17c030014ddf,
> subrepos were only cleaned.  The message probably shouldn't occur with subrepos
> because of the '(l)ocal or (r)emote' prompt when merging.
>
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -463,20 +463,24 @@
>       repo.ui.status(_("%d files updated, %d files merged, "
>                        "%d files removed, %d files unresolved\n") % stats)
>   
> -def updaterepo(repo, node, overwrite):
> +def updaterepo(repo, node, overwrite, showstats=False):
>       """Update the working directory to node.
>   
>       When overwrite is set, changes are clobbered, merged else
>   
>       returns stats (see pydoc mercurial.merge.applyupdates)"""
> -    return mergemod.update(repo, node, False, overwrite, None)
> +    stats = mergemod.update(repo, node, False, overwrite, None)
> +    if showstats:
> +        _showstats(repo, stats)
> +
> +    if not overwrite and stats[3]:
> +        repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))

Wouldn't it be better to keep this message in update() ?

> +
> +    return stats

You are changing the function anyway ... so why not change it to return 
stats[3]>0 like all other functions using mergemod.update do.

>   
>   def update(repo, node):
>       """update the working directory to node, merging linear changes"""
> -    stats = updaterepo(repo, node, False)
> -    _showstats(repo, stats)
> -    if stats[3]:
> -        repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
> +    stats = updaterepo(repo, node, False, True)
>       return stats[3] > 0
>   
>   # naming conflict in clone()
> @@ -484,9 +488,7 @@
>   
>   def clean(repo, node, show_stats=True):
>       """forcibly switch the working directory to node, clobbering changes"""
> -    stats = updaterepo(repo, node, True)
> -    if show_stats:
> -        _showstats(repo, stats)
> +    stats = updaterepo(repo, node, True, show_stats)
>       return stats[3] > 0
>   
>   def merge(repo, node, force=None, remind=True):

/Mads
Matt Harbison - Jan. 11, 2013, 3:20 a.m.
Mads Kiilerich wrote:
> On 01/06/2013 11:16 PM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1357448404 18000
>> # Node ID a35f6d98d65f24d5d6f1a7928342b2bc479ef44c
>> # Parent 2c1276825e938872ebc099c191eb202f0dbadfcc
>> hg: combine the remainder of clean() and update() into updaterepo()
>>
>> This sets the stage for largefiles to override updaterepo() instead of
>> clean()
>> and update(), while not altering the order of the largefile changes
>> output and
>> the updated/merged/removed/unresolved output. The need to override
>> updaterepo()
>> is to allow largefiles in a subrepo to be updated when the subrepo is
>> updated.
>> This change also opens the door to subrepos printing their statistics (it
>> currently doesn't because it calls updaterepo() without setting
>> showstats,
>> having previously lacked the capability entirely).
>>
>> Note that the 'use hg resolve' line was never output for subrepos,
>> because
>> updaterepo() previously didn't print anything, and prior to 17c030014ddf,
>> subrepos were only cleaned. The message probably shouldn't occur with
>> subrepos
>> because of the '(l)ocal or (r)emote' prompt when merging.
>>
>> diff --git a/mercurial/hg.py b/mercurial/hg.py
>> --- a/mercurial/hg.py
>> +++ b/mercurial/hg.py
>> @@ -463,20 +463,24 @@
>> repo.ui.status(_("%d files updated, %d files merged, "
>> "%d files removed, %d files unresolved\n") % stats)
>> -def updaterepo(repo, node, overwrite):
>> +def updaterepo(repo, node, overwrite, showstats=False):
>> """Update the working directory to node.
>> When overwrite is set, changes are clobbered, merged else
>> returns stats (see pydoc mercurial.merge.applyupdates)"""
>> - return mergemod.update(repo, node, False, overwrite, None)
>> + stats = mergemod.update(repo, node, False, overwrite, None)
>> + if showstats:
>> + _showstats(repo, stats)
>> +
>> + if not overwrite and stats[3]:
>> + repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
>
> Wouldn't it be better to keep this message in update() ?

Initially, that's where I left it, then I moved it thinking it might be 
needed, and then when it didn't appear in the tests, I reasoned that the 
(l)ocal/(r)emote prompt was why.

I wasn't sure it mattered at that point so I left it, justifying it in 
my mind by thinking someone might see how subrepos are calling 
updaterepo(), add that in new code and miss the prompt.  I think calling 
that directly is wrong, because clean() and update() are so much more 
readable, but maybe it avoids future problems?

I can change it back if you want, but I'm curious why you think one way 
has merit over another?

>> +
>> + return stats
>
> You are changing the function anyway ... so why not change it to return
> stats[3]>0 like all other functions using mergemod.update do.

I thought about that too, and almost did it, but then thought that if 
some extension somewhere called it, that code would break.  I figured it 
was less risky this close to the freeze.

>> def update(repo, node):
>> """update the working directory to node, merging linear changes"""
>> - stats = updaterepo(repo, node, False)
>> - _showstats(repo, stats)
>> - if stats[3]:
>> - repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
>> + stats = updaterepo(repo, node, False, True)
>> return stats[3] > 0
>> # naming conflict in clone()
>> @@ -484,9 +488,7 @@
>> def clean(repo, node, show_stats=True):
>> """forcibly switch the working directory to node, clobbering changes"""
>> - stats = updaterepo(repo, node, True)
>> - if show_stats:
>> - _showstats(repo, stats)
>> + stats = updaterepo(repo, node, True, show_stats)
>> return stats[3] > 0
>> def merge(repo, node, force=None, remind=True):
>
> /Mads
>
Mads Kiilerich - Jan. 11, 2013, 11:57 p.m.
On 01/11/2013 04:20 AM, Matt Harbison wrote:
> Mads Kiilerich wrote:
>> On 01/06/2013 11:16 PM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1357448404 18000
>>> # Node ID a35f6d98d65f24d5d6f1a7928342b2bc479ef44c
>>> # Parent 2c1276825e938872ebc099c191eb202f0dbadfcc
>>> hg: combine the remainder of clean() and update() into updaterepo()
>>>
>>> This sets the stage for largefiles to override updaterepo() instead of
>>> clean()
>>> and update(), while not altering the order of the largefile changes
>>> output and
>>> the updated/merged/removed/unresolved output. The need to override
>>> updaterepo()
>>> is to allow largefiles in a subrepo to be updated when the subrepo is
>>> updated.
>>> This change also opens the door to subrepos printing their 
>>> statistics (it
>>> currently doesn't because it calls updaterepo() without setting
>>> showstats,
>>> having previously lacked the capability entirely).
>>>
>>> Note that the 'use hg resolve' line was never output for subrepos,
>>> because
>>> updaterepo() previously didn't print anything, and prior to 
>>> 17c030014ddf,
>>> subrepos were only cleaned. The message probably shouldn't occur with
>>> subrepos
>>> because of the '(l)ocal or (r)emote' prompt when merging.
>>>
>>> diff --git a/mercurial/hg.py b/mercurial/hg.py
>>> --- a/mercurial/hg.py
>>> +++ b/mercurial/hg.py
>>> @@ -463,20 +463,24 @@
>>> repo.ui.status(_("%d files updated, %d files merged, "
>>> "%d files removed, %d files unresolved\n") % stats)
>>> -def updaterepo(repo, node, overwrite):
>>> +def updaterepo(repo, node, overwrite, showstats=False):
>>> """Update the working directory to node.
>>> When overwrite is set, changes are clobbered, merged else
>>> returns stats (see pydoc mercurial.merge.applyupdates)"""
>>> - return mergemod.update(repo, node, False, overwrite, None)
>>> + stats = mergemod.update(repo, node, False, overwrite, None)
>>> + if showstats:
>>> + _showstats(repo, stats)
>>> +
>>> + if not overwrite and stats[3]:
>>> + repo.ui.status(_("use 'hg resolve' to retry unresolved file 
>>> merges\n"))
>>
>> Wouldn't it be better to keep this message in update() ?
>
> Initially, that's where I left it, then I moved it thinking it might 
> be needed, and then when it didn't appear in the tests, I reasoned 
> that the (l)ocal/(r)emote prompt was why.
>
> I wasn't sure it mattered at that point so I left it, justifying it in 
> my mind by thinking someone might see how subrepos are calling 
> updaterepo(), add that in new code and miss the prompt.  I think 
> calling that directly is wrong, because clean() and update() are so 
> much more readable, but maybe it avoids future problems?
>
> I can change it back if you want, but I'm curious why you think one 
> way has merit over another?

updaterepo seems to be a more low level function that doesn't have as 
much ui interaction. The UI messages are added at a higher level (except 
for showstats which however is controlled directly). It seems nice to 
keep it that way.

We also want this message to appear after any largefiles messages. And 
we probably want another and more help message for subrepos.

> I thought about that too, and almost did it, but then thought that if 
> some extension somewhere called it, that code would break.  I figured 
> it was less risky this close to the freeze.

We are before freeze and the stable rules do not apply. And we are 
changing the semantics anyway ... and extensions should follow Mercurial 
anyway.

Anyway, I have given some input. Please resend if you for this or other 
reasons would propose different patches.

/Mads

Patch

diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -463,20 +463,24 @@ 
     repo.ui.status(_("%d files updated, %d files merged, "
                      "%d files removed, %d files unresolved\n") % stats)
 
-def updaterepo(repo, node, overwrite):
+def updaterepo(repo, node, overwrite, showstats=False):
     """Update the working directory to node.
 
     When overwrite is set, changes are clobbered, merged else
 
     returns stats (see pydoc mercurial.merge.applyupdates)"""
-    return mergemod.update(repo, node, False, overwrite, None)
+    stats = mergemod.update(repo, node, False, overwrite, None)
+    if showstats:
+        _showstats(repo, stats)
+
+    if not overwrite and stats[3]:
+        repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
+
+    return stats
 
 def update(repo, node):
     """update the working directory to node, merging linear changes"""
-    stats = updaterepo(repo, node, False)
-    _showstats(repo, stats)
-    if stats[3]:
-        repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
+    stats = updaterepo(repo, node, False, True)
     return stats[3] > 0
 
 # naming conflict in clone()
@@ -484,9 +488,7 @@ 
 
 def clean(repo, node, show_stats=True):
     """forcibly switch the working directory to node, clobbering changes"""
-    stats = updaterepo(repo, node, True)
-    if show_stats:
-        _showstats(repo, stats)
+    stats = updaterepo(repo, node, True, show_stats)
     return stats[3] > 0
 
 def merge(repo, node, force=None, remind=True):