Patchwork [2,of,2] patch.internalpatch: accept a prefix parameter

login
register
mail settings
Submitter Siddharth Agarwal
Date March 9, 2015, 4:59 p.m.
Message ID <ee9d52883035d7d32b18.1425920384@devbig136.prn2.facebook.com>
Download mbox | patch
Permalink /patch/7951/
State Accepted
Commit 60c279ab7bd3bb08779cd6c74230d6739a63ebea
Delegated to: Pierre-Yves David
Headers show

Comments

Siddharth Agarwal - March 9, 2015, 4:59 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1425710627 28800
#      Fri Mar 06 22:43:47 2015 -0800
# Node ID ee9d52883035d7d32b18421b33653e597f4e4cf5
# Parent  f4f59f9484d44f3941b13b76d168f50e91c3cbb3
patch.internalpatch: accept a prefix parameter
Pierre-Yves David - March 9, 2015, 6:25 p.m.
On 03/09/2015 09:59 AM, Siddharth Agarwal wrote:> # HG changeset patch
 > # User Siddharth Agarwal <sid0@fb.com>
 > # Date 1425710627 28800
 > #      Fri Mar 06 22:43:47 2015 -0800
 > # Node ID ee9d52883035d7d32b18421b33653e597f4e4cf5
 > # Parent  f4f59f9484d44f3941b13b76d168f50e91c3cbb3
 > patch.internalpatch: accept a prefix parameter
 >

These patches looks good to me
(although this would have been a great occasion to add some 
documentation to these functions)

I'm not sure why you do not provide a default value for the argument. 
But this probably have to do with breaking the signature (but would be 
happy to have clarification).

I'll let a non-colleague one push them
Siddharth Agarwal - March 9, 2015, 6:28 p.m.
On 03/09/2015 11:25 AM, Pierre-Yves David wrote:
> On 03/09/2015 09:59 AM, Siddharth Agarwal wrote:> # HG changeset patch
> > # User Siddharth Agarwal <sid0@fb.com>
> > # Date 1425710627 28800
> > #      Fri Mar 06 22:43:47 2015 -0800
> > # Node ID ee9d52883035d7d32b18421b33653e597f4e4cf5
> > # Parent  f4f59f9484d44f3941b13b76d168f50e91c3cbb3
> > patch.internalpatch: accept a prefix parameter
> >
>
> These patches looks good to me
> (although this would have been a great occasion to add some
> documentation to these functions)
>
> I'm not sure why you do not provide a default value for the argument.
> But this probably have to do with breaking the signature (but would be
> happy to have clarification).

(1) In general I don't like default arguments, especially for internal
methods.
(2) prefix and strip logically go together, so wherever strip doesn't
have a default argument I didn't have one for prefix either.
Pierre-Yves David - March 10, 2015, 2:55 a.m.
On 03/09/2015 11:28 AM, Siddharth Agarwal wrote:
> On 03/09/2015 11:25 AM, Pierre-Yves David wrote:
>> On 03/09/2015 09:59 AM, Siddharth Agarwal wrote:> # HG changeset patch
>>> # User Siddharth Agarwal <sid0@fb.com>
>>> # Date 1425710627 28800
>>> #      Fri Mar 06 22:43:47 2015 -0800
>>> # Node ID ee9d52883035d7d32b18421b33653e597f4e4cf5
>>> # Parent  f4f59f9484d44f3941b13b76d168f50e91c3cbb3
>>> patch.internalpatch: accept a prefix parameter
>>>
>>
>> These patches looks good to me
>> (although this would have been a great occasion to add some
>> documentation to these functions)
>>
>> I'm not sure why you do not provide a default value for the argument.
>> But this probably have to do with breaking the signature (but would be
>> happy to have clarification).
>
> (1) In general I don't like default arguments, especially for internal
> methods.
> (2) prefix and strip logically go together, so wherever strip doesn't
> have a default argument I didn't have one for prefix either.

Ok, as nobody growled about them, they are pushed to the clowncopter.

Patch

diff --git a/hgext/record.py b/hgext/record.py
--- a/hgext/record.py
+++ b/hgext/record.py
@@ -592,7 +592,7 @@ 
                 try:
                     ui.debug('applying patch\n')
                     ui.debug(fp.getvalue())
-                    patch.internalpatch(ui, repo, fp, 1, eolmode=None)
+                    patch.internalpatch(ui, repo, fp, 1, '', eolmode=None)
                 except patch.PatchError, err:
                     raise util.Abort(str(err))
             del fp
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -1521,12 +1521,12 @@ 
         raise PatchError(_('patch failed to apply'))
     return ret > 0
 
-def internalpatch(ui, repo, patchobj, strip, files=None, eolmode='strict',
-                  similarity=0):
+def internalpatch(ui, repo, patchobj, strip, prefix, files=None,
+                  eolmode='strict', similarity=0):
     """use builtin patch to apply <patchobj> to the working directory.
     returns whether patch was applied with fuzz factor."""
     backend = workingbackend(ui, repo, similarity)
-    return patchbackend(ui, backend, patchobj, strip, '', files, eolmode)
+    return patchbackend(ui, backend, patchobj, strip, prefix, files, eolmode)
 
 def patchrepo(ui, repo, ctx, store, patchobj, strip, files=None,
               eolmode='strict'):
@@ -1552,7 +1552,7 @@ 
     if patcher:
         return _externalpatch(ui, repo, patcher, patchname, strip,
                               files, similarity)
-    return internalpatch(ui, repo, patchname, strip, files, eolmode,
+    return internalpatch(ui, repo, patchname, strip, '', files, eolmode,
                          similarity)
 
 def changedfiles(ui, repo, patchpath, strip=1):