Patchwork [1,of,5,shelve-ext] shelve: move temporary commit creation to a separate function

login
register
mail settings
Submitter Kostia Balytskyi
Date Nov. 13, 2016, 11:39 a.m.
Message ID <45dbc9a803375958310b.1479037174@dev1902.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17544/
State Accepted
Headers show

Comments

Kostia Balytskyi - Nov. 13, 2016, 11:39 a.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1479036952 28800
#      Sun Nov 13 03:35:52 2016 -0800
# Node ID 45dbc9a803375958310bced301227b02802372b0
# Parent  2188194ca1ee86953855e0d2fb9396ec18636ed9
shelve: move temporary commit creation to a separate function

Committing working copy changes before rebasing a shelved commit
on top of them is an independent piece of behavior, which fits
into its own function.

Similar to the previous serier, this and a couple of following
patches are for unshelve refactoring.
Yuya Nishihara - Nov. 19, 2016, 10:02 a.m.
On Sun, 13 Nov 2016 03:39:34 -0800, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1479036952 28800
> #      Sun Nov 13 03:35:52 2016 -0800
> # Node ID 45dbc9a803375958310bced301227b02802372b0
> # Parent  2188194ca1ee86953855e0d2fb9396ec18636ed9
> shelve: move temporary commit creation to a separate function

Looks all good. Queued them, thanks.

> +def _commitworkingcopychanges(ui, repo, opts, tmpwctx):
> +    """Temporarily commit working copy changes before moving unshelve commit"""
> +    # Store pending changes in a commit and remember added in case a shelve
> +    # contains unknown files that are part of the pending change
> +    s = repo.status()
> +    addedbefore = frozenset(s.added)
> +    if not (s.modified or s.added or s.removed or s.deleted):
> +        return tmpwctx, addedbefore
> +    ui.status(_("temporarily committing pending changes "
> +                "(restore with 'hg unshelve --abort')\n"))
> +    commitfunc = getcommitfunc(extra=None, interactive=False,
> +                               editor=False)
> +    tempopts = {}
> +    tempopts['message'] = "pending changes temporary commit"
> +    tempopts['date'] = opts.get('date')
> +    ui.quiet = True
> +    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
> +    tmpwctx = repo[node]
> +    return tmpwctx, addedbefore

This and the next unbalanced ui.quiet hack can be a source of future bugs.
I hope they'll be fixed by a follow-up patch.
Kostia Balytskyi - Nov. 20, 2016, 12:42 a.m.
On 11/19/16, 10:02 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    On Sun, 13 Nov 2016 03:39:34 -0800, Kostia Balytskyi wrote:
    > # HG changeset patch

    > # User Kostia Balytskyi <ikostia@fb.com>

    > # Date 1479036952 28800

    > #      Sun Nov 13 03:35:52 2016 -0800

    > # Node ID 45dbc9a803375958310bced301227b02802372b0

    > # Parent  2188194ca1ee86953855e0d2fb9396ec18636ed9

    > shelve: move temporary commit creation to a separate function

    
    Looks all good. Queued them, thanks.
    
    > +def _commitworkingcopychanges(ui, repo, opts, tmpwctx):

    > +    """Temporarily commit working copy changes before moving unshelve commit"""

    > +    # Store pending changes in a commit and remember added in case a shelve

    > +    # contains unknown files that are part of the pending change

    > +    s = repo.status()

    > +    addedbefore = frozenset(s.added)

    > +    if not (s.modified or s.added or s.removed or s.deleted):

    > +        return tmpwctx, addedbefore

    > +    ui.status(_("temporarily committing pending changes "

    > +                "(restore with 'hg unshelve --abort')\n"))

    > +    commitfunc = getcommitfunc(extra=None, interactive=False,

    > +                               editor=False)

    > +    tempopts = {}

    > +    tempopts['message'] = "pending changes temporary commit"

    > +    tempopts['date'] = opts.get('date')

    > +    ui.quiet = True

    > +    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)

    > +    tmpwctx = repo[node]

    > +    return tmpwctx, addedbefore

    
    This and the next unbalanced ui.quiet hack can be a source of future bugs.
    I hope they'll be fixed by a follow-up patch.
Can you explain please? I was trying to keep the ui.quiet behavior unchanged in this refactoring series.
Yuya Nishihara - Nov. 20, 2016, 4:10 a.m.
On Sun, 20 Nov 2016 00:42:53 +0000, Kostia Balytskyi wrote:
> On 11/19/16, 10:02 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
> 
>     On Sun, 13 Nov 2016 03:39:34 -0800, Kostia Balytskyi wrote:
>     > # HG changeset patch
>     > # User Kostia Balytskyi <ikostia@fb.com>
>     > # Date 1479036952 28800
>     > #      Sun Nov 13 03:35:52 2016 -0800
>     > # Node ID 45dbc9a803375958310bced301227b02802372b0
>     > # Parent  2188194ca1ee86953855e0d2fb9396ec18636ed9
>     > shelve: move temporary commit creation to a separate function
>     
>     Looks all good. Queued them, thanks.
>     
>     > +def _commitworkingcopychanges(ui, repo, opts, tmpwctx):
>     > +    """Temporarily commit working copy changes before moving unshelve commit"""
>     > +    # Store pending changes in a commit and remember added in case a shelve
>     > +    # contains unknown files that are part of the pending change
>     > +    s = repo.status()
>     > +    addedbefore = frozenset(s.added)
>     > +    if not (s.modified or s.added or s.removed or s.deleted):
>     > +        return tmpwctx, addedbefore
>     > +    ui.status(_("temporarily committing pending changes "
>     > +                "(restore with 'hg unshelve --abort')\n"))
>     > +    commitfunc = getcommitfunc(extra=None, interactive=False,
>     > +                               editor=False)
>     > +    tempopts = {}
>     > +    tempopts['message'] = "pending changes temporary commit"
>     > +    tempopts['date'] = opts.get('date')
>     > +    ui.quiet = True
>     > +    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
>     > +    tmpwctx = repo[node]
>     > +    return tmpwctx, addedbefore
>     
>     This and the next unbalanced ui.quiet hack can be a source of future bugs.
>     I hope they'll be fixed by a follow-up patch.

> Can you explain please? I was trying to keep the ui.quiet behavior unchanged in this refactoring series.

You've split set/restore of ui.quiet to separate functions. It's unclear that
we have to care about the state of ui.quiet when adding some code to
_dounshelve(). So IMO, each function should restore ui.quiet at the end.
Pierre-Yves David - Jan. 3, 2017, 11:13 a.m.
On 11/20/2016 05:10 AM, Yuya Nishihara wrote:
> On Sun, 20 Nov 2016 00:42:53 +0000, Kostia Balytskyi wrote:
>> On 11/19/16, 10:02 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>>
>>     On Sun, 13 Nov 2016 03:39:34 -0800, Kostia Balytskyi wrote:
>>     > # HG changeset patch
>>     > # User Kostia Balytskyi <ikostia@fb.com>
>>     > # Date 1479036952 28800
>>     > #      Sun Nov 13 03:35:52 2016 -0800
>>     > # Node ID 45dbc9a803375958310bced301227b02802372b0
>>     > # Parent  2188194ca1ee86953855e0d2fb9396ec18636ed9
>>     > shelve: move temporary commit creation to a separate function
>>
>>     Looks all good. Queued them, thanks.
>>
>>     > +def _commitworkingcopychanges(ui, repo, opts, tmpwctx):
>>     > +    """Temporarily commit working copy changes before moving unshelve commit"""
>>     > +    # Store pending changes in a commit and remember added in case a shelve
>>     > +    # contains unknown files that are part of the pending change
>>     > +    s = repo.status()
>>     > +    addedbefore = frozenset(s.added)
>>     > +    if not (s.modified or s.added or s.removed or s.deleted):
>>     > +        return tmpwctx, addedbefore
>>     > +    ui.status(_("temporarily committing pending changes "
>>     > +                "(restore with 'hg unshelve --abort')\n"))
>>     > +    commitfunc = getcommitfunc(extra=None, interactive=False,
>>     > +                               editor=False)
>>     > +    tempopts = {}
>>     > +    tempopts['message'] = "pending changes temporary commit"
>>     > +    tempopts['date'] = opts.get('date')
>>     > +    ui.quiet = True
>>     > +    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
>>     > +    tmpwctx = repo[node]
>>     > +    return tmpwctx, addedbefore
>>
>>     This and the next unbalanced ui.quiet hack can be a source of future bugs.
>>     I hope they'll be fixed by a follow-up patch.
>
>> Can you explain please? I was trying to keep the ui.quiet behavior unchanged in this refactoring series.
>
> You've split set/restore of ui.quiet to separate functions. It's unclear that
> we have to care about the state of ui.quiet when adding some code to
> _dounshelve(). So IMO, each function should restore ui.quiet at the end.

I don't think this followup ever happen (but having a quick look at the 
code. What is the status of this?

Cheers,
Yuya Nishihara - Jan. 4, 2017, 1:04 p.m.
On Tue, 3 Jan 2017 12:13:35 +0100, Pierre-Yves David wrote:
> On 11/20/2016 05:10 AM, Yuya Nishihara wrote:
> > On Sun, 20 Nov 2016 00:42:53 +0000, Kostia Balytskyi wrote:
> >> On 11/19/16, 10:02 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
> >>     > +def _commitworkingcopychanges(ui, repo, opts, tmpwctx):
> >>     > +    """Temporarily commit working copy changes before moving unshelve commit"""
> >>     > +    # Store pending changes in a commit and remember added in case a shelve
> >>     > +    # contains unknown files that are part of the pending change
> >>     > +    s = repo.status()
> >>     > +    addedbefore = frozenset(s.added)
> >>     > +    if not (s.modified or s.added or s.removed or s.deleted):
> >>     > +        return tmpwctx, addedbefore
> >>     > +    ui.status(_("temporarily committing pending changes "
> >>     > +                "(restore with 'hg unshelve --abort')\n"))
> >>     > +    commitfunc = getcommitfunc(extra=None, interactive=False,
> >>     > +                               editor=False)
> >>     > +    tempopts = {}
> >>     > +    tempopts['message'] = "pending changes temporary commit"
> >>     > +    tempopts['date'] = opts.get('date')
> >>     > +    ui.quiet = True
> >>     > +    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
> >>     > +    tmpwctx = repo[node]
> >>     > +    return tmpwctx, addedbefore
> >>
> >>     This and the next unbalanced ui.quiet hack can be a source of future bugs.
> >>     I hope they'll be fixed by a follow-up patch.
> >
> >> Can you explain please? I was trying to keep the ui.quiet behavior unchanged in this refactoring series.
> >
> > You've split set/restore of ui.quiet to separate functions. It's unclear that
> > we have to care about the state of ui.quiet when adding some code to
> > _dounshelve(). So IMO, each function should restore ui.quiet at the end.
> 
> I don't think this followup ever happen (but having a quick look at the 
> code. What is the status of this?

Not yet, but perhaps b0a8337ba9af "ui: add configoverride context manager"
was the patch to start addressing this.

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -631,6 +631,26 @@  def unshelvecontinue(ui, repo, state, op
         unshelvecleanup(ui, repo, state.name, opts)
         ui.status(_("unshelve of '%s' complete\n") % state.name)
 
+def _commitworkingcopychanges(ui, repo, opts, tmpwctx):
+    """Temporarily commit working copy changes before moving unshelve commit"""
+    # Store pending changes in a commit and remember added in case a shelve
+    # contains unknown files that are part of the pending change
+    s = repo.status()
+    addedbefore = frozenset(s.added)
+    if not (s.modified or s.added or s.removed or s.deleted):
+        return tmpwctx, addedbefore
+    ui.status(_("temporarily committing pending changes "
+                "(restore with 'hg unshelve --abort')\n"))
+    commitfunc = getcommitfunc(extra=None, interactive=False,
+                               editor=False)
+    tempopts = {}
+    tempopts['message'] = "pending changes temporary commit"
+    tempopts['date'] = opts.get('date')
+    ui.quiet = True
+    node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
+    tmpwctx = repo[node]
+    return tmpwctx, addedbefore
+
 @command('unshelve',
          [('a', 'abort', None,
            _('abort an incomplete unshelve operation')),
@@ -753,21 +773,8 @@  def _dounshelve(ui, repo, *shelved, **op
         # and shelvectx is the unshelved changes. Then we merge it all down
         # to the original pctx.
 
-        # Store pending changes in a commit and remember added in case a shelve
-        # contains unknown files that are part of the pending change
-        s = repo.status()
-        addedbefore = frozenset(s.added)
-        if s.modified or s.added or s.removed or s.deleted:
-            ui.status(_("temporarily committing pending changes "
-                        "(restore with 'hg unshelve --abort')\n"))
-            commitfunc = getcommitfunc(extra=None, interactive=False,
-                                       editor=False)
-            tempopts = {}
-            tempopts['message'] = "pending changes temporary commit"
-            tempopts['date'] = opts.get('date')
-            ui.quiet = True
-            node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
-            tmpwctx = repo[node]
+        tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
+                                                         tmpwctx)
 
         ui.quiet = True
         shelvedfile(repo, basename, 'hg').applybundle()