Patchwork [4,of,4,V2] import: use ui.allowemptycommit to allow empty commits

login
register
mail settings
Submitter Durham Goode
Date May 12, 2015, 3:29 a.m.
Message ID <e9ae79f97326eb5ee5d9.1431401353@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/9024/
State Accepted
Headers show

Comments

Durham Goode - May 12, 2015, 3:29 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1431400541 25200
#      Mon May 11 20:15:41 2015 -0700
# Node ID e9ae79f97326eb5ee5d9415ba997cd5401fcf57c
# Parent  17cc5e344169e24cf5af7900f3b3bbd00993f24b
import: use ui.allowemptycommit to allow empty commits

Previously import used force=partial to allow empty commits to be made. Let's
switch it to using the new ui.allowemptycommit option.

This is the last known use of force for allowing empty commits, so let's remove
it from the allowemptycommits condition.
Durham Goode - May 12, 2015, 3:34 a.m.
On 5/11/15 8:29 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1431400541 25200
> #      Mon May 11 20:15:41 2015 -0700
> # Node ID e9ae79f97326eb5ee5d9415ba997cd5401fcf57c
> # Parent  17cc5e344169e24cf5af7900f3b3bbd00993f24b
> import: use ui.allowemptycommit to allow empty commits
>
> Previously import used force=partial to allow empty commits to be made. Let's
> switch it to using the new ui.allowemptycommit option.
>
> This is the last known use of force for allowing empty commits, so let's remove
> it from the allowemptycommits condition.
>
I don't actually think we should take patches 3 and 4 in this series.  
We can't actually get rid of the force flag entirely because it does a 
bunch of other things, so these two patches just introduce some funky 
config backup/set/restore logic in exchange for deleting 'force' from a 
single condition.

I only include them because it was mentioned that ui.allowemptycommit 
would only be worth it if the other places that require empty commits 
used the same code path.
Pierre-Yves David - May 12, 2015, 6:57 p.m.
On 05/11/2015 08:29 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1431400541 25200
> #      Mon May 11 20:15:41 2015 -0700
> # Node ID e9ae79f97326eb5ee5d9415ba997cd5401fcf57c
> # Parent  17cc5e344169e24cf5af7900f3b3bbd00993f24b
> import: use ui.allowemptycommit to allow empty commits
>
> Previously import used force=partial to allow empty commits to be made. Let's
> switch it to using the new ui.allowemptycommit option.
>
> This is the last known use of force for allowing empty commits, so let's remove
> it from the allowemptycommits condition.

I think this is good step forward simplifying the --force business. I've 
taken the four patch and pushed them to the clowncopter. The last one 
got some modification inflight.

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -896,9 +896,16 @@ def tryimportone(ui, repo, hunk, parents
>                       editor = None
>                   else:
>                       editor = getcommiteditor(editform=editform, **opts)
> -                n = repo.commit(message, opts.get('user') or user,
> -                                opts.get('date') or date, match=m,
> -                                editor=editor, force=partial)
> +                try:
> +                    allowemptybackup = repo.ui.backupconfig('ui',
> +                                                            'allowemptycommit')
> +                    if partial:
> +                        repo.ui.setconfig('ui', 'allowemptycommit', True)
> +                    n = repo.commit(message, opts.get('user') or user,
> +                                    opts.get('date') or date, match=m,
> +                                    editor=editor, force=partial)
> +                finally:
> +                    repo.ui.restoreconfig(allowemptybackup)
>               repo.dirstate.endparentchange()
>           else:
>               if opts.get('exact') or opts.get('import_branch'):

I've extracted 'allowemptycommit' assignment outside of the try except 
to prevent potential name error if the call fail.

I've also drop the 'force=partial' from this function as the tests seems 
to agree we do not need it anymore.

> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1462,7 +1462,7 @@ class localrepository(object):
>               cctx = context.workingcommitctx(self, status,
>                                               text, user, date, extra)
>
> -            allowemptycommit = (wctx.branch() != wctx.p1().branch() or force
> +            allowemptycommit = (wctx.branch() != wctx.p1().branch()
>                                   or extra.get('close') or merge or cctx.files()
>                                   or self.ui.configbool('ui', 'allowemptycommit'))
>               if not allowemptycommit:

I've move this change for localrepo.py in its own change with extended 
explanation.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -896,9 +896,16 @@  def tryimportone(ui, repo, hunk, parents
                     editor = None
                 else:
                     editor = getcommiteditor(editform=editform, **opts)
-                n = repo.commit(message, opts.get('user') or user,
-                                opts.get('date') or date, match=m,
-                                editor=editor, force=partial)
+                try:
+                    allowemptybackup = repo.ui.backupconfig('ui',
+                                                            'allowemptycommit')
+                    if partial:
+                        repo.ui.setconfig('ui', 'allowemptycommit', True)
+                    n = repo.commit(message, opts.get('user') or user,
+                                    opts.get('date') or date, match=m,
+                                    editor=editor, force=partial)
+                finally:
+                    repo.ui.restoreconfig(allowemptybackup)
             repo.dirstate.endparentchange()
         else:
             if opts.get('exact') or opts.get('import_branch'):
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1462,7 +1462,7 @@  class localrepository(object):
             cctx = context.workingcommitctx(self, status,
                                             text, user, date, extra)
 
-            allowemptycommit = (wctx.branch() != wctx.p1().branch() or force
+            allowemptycommit = (wctx.branch() != wctx.p1().branch()
                                 or extra.get('close') or merge or cctx.files()
                                 or self.ui.configbool('ui', 'allowemptycommit'))
             if not allowemptycommit: