Patchwork [2,of,4] subrepo: remove unnecessary else clause in hgsubrepo._get

login
register
mail settings
Submitter Angel Ezquerra
Date Nov. 25, 2013, 8:46 p.m.
Message ID <15941436e0ab88dee0f5.1385412408@Angel-PC.localdomain>
Download mbox | patch
Permalink /patch/3135/
State Superseded
Commit c5aef7a6660769d989344c12299ed773b71dc7bf
Headers show

Comments

Angel Ezquerra - Nov. 25, 2013, 8:46 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1385255580 -3600
#      Sun Nov 24 02:13:00 2013 +0100
# Node ID 15941436e0ab88dee0f5321f737bcf4bc9774fce
# Parent  adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
subrepo: remove unnecessary else clause in hgsubrepo._get

This revision has no behaviour change. It simply removes an unnecessary else
that follows an if / return block. The change looks big because a big chunk of
code has been unindented one level.
Angel Ezquerra - Jan. 16, 2014, 9:18 p.m.
Forwarding Pierre-Yves┬┤s comment to the list:

---------- Forwarded message ----------
From: Pierre-Yves David <pierre-yves.david@ens-lyon.org>
Date: Wed, Jan 15, 2014 at 4:03 PM
Subject: Re: [PATCH 2 of 4] subrepo: remove unnecessary else clause in
hgsubrepo._get
To: Angel Ezquerra <angel.ezquerra@gmail.com>


On 11/25/2013 12:46 PM, Angel Ezquerra wrote:
>
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1385255580 -3600
> #      Sun Nov 24 02:13:00 2013 +0100
> # Node ID 15941436e0ab88dee0f5321f737bcf4bc9774fce
> # Parent  adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
> subrepo: remove unnecessary else clause in hgsubrepo._get
>
> This revision has no behaviour change. It simply removes an unnecessary else
> that follows an if / return block. The change looks big because a big chunk of
> code has been unindented one level.


I quite love the idea.

Not that you could ship a white space ignorant version of your diff
too. Not sure what I prefer.

--
Pierre-Yves
Angel Ezquerra - Jan. 16, 2014, 9:19 p.m.
> From: Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> Date: Wed, Jan 15, 2014 at 4:03 PM
> Subject: Re: [PATCH 2 of 4] subrepo: remove unnecessary else clause in
> hgsubrepo._get
> To: Angel Ezquerra <angel.ezquerra@gmail.com>
>
>
> On 11/25/2013 12:46 PM, Angel Ezquerra wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1385255580 -3600
>> #      Sun Nov 24 02:13:00 2013 +0100
>> # Node ID 15941436e0ab88dee0f5321f737bcf4bc9774fce
>> # Parent  adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
>> subrepo: remove unnecessary else clause in hgsubrepo._get
>>
>> This revision has no behaviour change. It simply removes an unnecessary else
>> that follows an if / return block. The change looks big because a big chunk of
>> code has been unindented one level.
>
>
> I quite love the idea.
>
> Not that you could ship a white space ignorant version of your diff
> too. Not sure what I prefer.

Umm, I'm not quite sure what you mean by that... what is a white space
ignorant version of a diff?

Cheers,

Angel
Pierre-Yves David - Jan. 17, 2014, 11:12 p.m.
On 01/16/2014 01:19 PM, Angel Ezquerra wrote:
> Umm, I'm not quite sure what you mean by that... what is a white space
> ignorant version of a diff?

result of `hg diff --ignore-space-change`
Angel Ezquerra - Jan. 17, 2014, 11:35 p.m.
On Fri, Jan 17, 2014 at 3:12 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> On 01/16/2014 01:19 PM, Angel Ezquerra wrote:
>>
>> Umm, I'm not quite sure what you mean by that... what is a white space
>> ignorant version of a diff?
>
>
> result of `hg diff --ignore-space-change`
>

Ah, I see. That would make the review better, but the actual patch
would still be hard to read so I think it is best to split the patch
as I did. Do you agree?

Angel
Pierre-Yves David - Jan. 17, 2014, 11:39 p.m.
On 01/17/2014 03:35 PM, Angel Ezquerra wrote:
> On Fri, Jan 17, 2014 at 3:12 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> On 01/16/2014 01:19 PM, Angel Ezquerra wrote:
>>> Umm, I'm not quite sure what you mean by that... what is a white space
>>> ignorant version of a diff?
>>
>> result of `hg diff --ignore-space-change`
>>
> Ah, I see. That would make the review better, but the actual patch
> would still be hard to read so I think it is best to split the patch
> as I did. Do you agree?

yeah

Patch

# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1385255580 -3600
#      Sun Nov 24 02:13:00 2013 +0100
# Node ID 15941436e0ab88dee0f5321f737bcf4bc9774fce
# Parent  adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
subrepo: remove unnecessary else clause in hgsubrepo._get

This revision has no behaviour change. It simply removes an unnecessary else
that follows an if / return block. The change looks big because a big chunk of
code has been unindented one level.

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -651,32 +651,31 @@ 
         urepo = self._repo.unfiltered()
         if revision in urepo:
             return
+        self._repo._subsource = source
+        srcurl = _abssource(self._repo)
+        other = hg.peer(self._repo, {}, srcurl)
+        if len(self._repo) == 0:
+            self._repo.ui.status(_('cloning subrepo %s from %s\n')
+                                 % (subrelpath(self), srcurl))
+            parentrepo = self._repo._subparent
+            shutil.rmtree(self._repo.path)
+            other, cloned = hg.clone(self._repo._subparent.baseui, {},
+                                     other, self._repo.root,
+                                     update=False)
+            self._repo = cloned.local()
+            self._initrepo(parentrepo, source, create=True)
+            self._cachestorehash(srcurl)
         else:
-            self._repo._subsource = source
-            srcurl = _abssource(self._repo)
-            other = hg.peer(self._repo, {}, srcurl)
-            if len(self._repo) == 0:
-                self._repo.ui.status(_('cloning subrepo %s from %s\n')
-                                     % (subrelpath(self), srcurl))
-                parentrepo = self._repo._subparent
-                shutil.rmtree(self._repo.path)
-                other, cloned = hg.clone(self._repo._subparent.baseui, {},
-                                         other, self._repo.root,
-                                         update=False)
-                self._repo = cloned.local()
-                self._initrepo(parentrepo, source, create=True)
+            self._repo.ui.status(_('pulling subrepo %s from %s\n')
+                                 % (subrelpath(self), srcurl))
+            cleansub = self.storeclean(srcurl)
+            remotebookmarks = other.listkeys('bookmarks')
+            self._repo.pull(other)
+            bookmarks.updatefromremote(self._repo.ui, self._repo,
+                                       remotebookmarks, srcurl)
+            if cleansub:
+                # keep the repo clean after pull
                 self._cachestorehash(srcurl)
-            else:
-                self._repo.ui.status(_('pulling subrepo %s from %s\n')
-                                     % (subrelpath(self), srcurl))
-                cleansub = self.storeclean(srcurl)
-                remotebookmarks = other.listkeys('bookmarks')
-                self._repo.pull(other)
-                bookmarks.updatefromremote(self._repo.ui, self._repo,
-                                           remotebookmarks, srcurl)
-                if cleansub:
-                    # keep the repo clean after pull
-                    self._cachestorehash(srcurl)
 
     @annotatesubrepoerror
     def get(self, state, overwrite=False):