Patchwork [stable?] import: respect diff.noprefix config (issue4639)

login
register
mail settings
Submitter Ryan McElroy
Date Jan. 22, 2016, 2:51 p.m.
Message ID <40555a31b98b027c91b1.1453474296@devbig314.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12865/
State Deferred
Headers show

Comments

Ryan McElroy - Jan. 22, 2016, 2:51 p.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1453473839 28800
#      Fri Jan 22 06:43:59 2016 -0800
# Branch stable
# Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
# Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
import: respect diff.noprefix config (issue4639)

Previously export-import round-trips would not work if diff.nopreofix was
truthy, because import did not pay attention to that setting. Now we inspect
that setting unless an explicit strip level is specified.
Siddharth Agarwal - Jan. 22, 2016, 6:43 p.m.
On 1/22/16 06:51, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1453473839 28800
> #      Fri Jan 22 06:43:59 2016 -0800
> # Branch stable
> # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
> import: respect diff.noprefix config (issue4639)
>
> Previously export-import round-trips would not work if diff.nopreofix was
> truthy, because import did not pay attention to that setting. Now we inspect
> that setting unless an explicit strip level is specified.

I don't think this is the right solution here -- much of the time hg 
import is used on a repository that's different from the one where hg 
export was used. Could we figure out from the patch whether to use -p0 
or -p1?

- Siddharth


>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -878,6 +878,9 @@ def tryimportone(ui, repo, hunk, parents
>       importbranch = opts.get('import_branch')
>       update = not opts.get('bypass')
>       strip = opts["strip"]
> +    if strip == patch.STRIPUNSET:
> +        noprefix = ui.configbool('diff', 'noprefix')
> +        strip = 0 if noprefix else 1
>       prefix = opts["prefix"]
>       sim = float(opts.get('similarity') or 0)
>       if not tmpname:
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4582,7 +4582,7 @@ def identify(ui, repo, source=None, rev=
>       ui.write("%s\n" % ' '.join(output))
>   
>   @command('import|patch',
> -    [('p', 'strip', 1,
> +    [('p', 'strip', patch.STRIPUNSET,
>        _('directory strip option for patch. This has the same '
>          'meaning as the corresponding patch option'), _('NUM')),
>       ('b', 'base', '', _('base path (DEPRECATED)'), _('PATH')),
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -37,6 +37,8 @@ from . import (
>       util,
>   )
>   
> +STRIPUNSET = -1 # a flag that means strip has not been explicitly set
> +
>   gitre = re.compile('diff --git a/(.*) b/(.*)')
>   tabsplitter = re.compile(r'(\t+|[^\t]+)')
>   
> diff --git a/tests/test-import.t b/tests/test-import.t
> --- a/tests/test-import.t
> +++ b/tests/test-import.t
> @@ -1763,3 +1763,19 @@ Importing some extra header
>     $ hg log --debug -r . | grep extra
>     extra:       branch=default
>     extra:       foo=bar
> +
> +Exporting then importing with diff.noprefix=True
> +
> +  $ hg expor . --config diff.noprefix=True > $TESTTMP/patch
> +  $ hg import $TESTTMP/patch
> +  applying $TESTTMP/patch (glob)
> +  abort: unable to strip away 1 of 1 dirs from a
> +  [255]
> +  $ hg import $TESTTMP/patch --config diff.noprefix=True --strip 1
> +  applying $TESTTMP/patch (glob)
> +  abort: unable to strip away 1 of 1 dirs from a
> +  [255]
> +  $ hg import $TESTTMP/patch --strip 0
> +  applying $TESTTMP/patch (glob)
> +  $ hg import $TESTTMP/patch --config diff.noprefix=True
> +  applying $TESTTMP/patch (glob)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Jan. 22, 2016, 7:15 p.m.
On 01/22/2016 10:43 AM, Siddharth Agarwal wrote:
> On 1/22/16 06:51, Ryan McElroy wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy@fb.com>
>> # Date 1453473839 28800
>> #      Fri Jan 22 06:43:59 2016 -0800
>> # Branch stable
>> # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>> import: respect diff.noprefix config (issue4639)
>>
>> Previously export-import round-trips would not work if diff.nopreofix was
>> truthy, because import did not pay attention to that setting. Now we
>> inspect
>> that setting unless an explicit strip level is specified.
>
> I don't think this is the right solution here -- much of the time hg
> import is used on a repository that's different from the one where hg
> export was used. Could we figure out from the patch whether to use -p0
> or -p1?

I'm pretty unenthusiastic for this solution too. changing the behavior 
of import based on an option meant for diff display seems sloppy to me.

I all cases, this is too much of a behavior change for the freeze. Let's 
discuss it in February.
Ryan McElroy - Jan. 22, 2016, 10:08 p.m.
On 1/22/2016 7:15 PM, Pierre-Yves David wrote:
>
>
> On 01/22/2016 10:43 AM, Siddharth Agarwal wrote:
>> On 1/22/16 06:51, Ryan McElroy wrote:
>>> # HG changeset patch
>>> # User Ryan McElroy <rmcelroy@fb.com>
>>> # Date 1453473839 28800
>>> #      Fri Jan 22 06:43:59 2016 -0800
>>> # Branch stable
>>> # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
>>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>>> import: respect diff.noprefix config (issue4639)
>>>
>>> Previously export-import round-trips would not work if 
>>> diff.nopreofix was
>>> truthy, because import did not pay attention to that setting. Now we
>>> inspect
>>> that setting unless an explicit strip level is specified.
>>
>> I don't think this is the right solution here -- much of the time hg
>> import is used on a repository that's different from the one where hg
>> export was used. Could we figure out from the patch whether to use -p0
>> or -p1?
>
> I'm pretty unenthusiastic for this solution too. changing the behavior 
> of import based on an option meant for diff display seems sloppy to me.
>
> I all cases, this is too much of a behavior change for the freeze. 
> Let's discuss it in February.
>

Then we need something in bugzilla that makes it more clear if an issue 
is eligible for a fix during the freeze.

Bug-squash-a-thons like the one we held today are useless if we can't 
figure out the right bugs to work on.

Cheers,

~Ryan
Pierre-Yves David - Jan. 22, 2016, 11:19 p.m.
On 01/22/2016 02:08 PM, Ryan McElroy wrote:
> On 1/22/2016 7:15 PM, Pierre-Yves David wrote:
>>
>> On 01/22/2016 10:43 AM, Siddharth Agarwal wrote:
>>> On 1/22/16 06:51, Ryan McElroy wrote:
>>>> # HG changeset patch
>>>> # User Ryan McElroy <rmcelroy@fb.com>
>>>> # Date 1453473839 28800
>>>> #      Fri Jan 22 06:43:59 2016 -0800
>>>> # Branch stable
>>>> # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
>>>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>>>> import: respect diff.noprefix config (issue4639)
>>>>
>>>> Previously export-import round-trips would not work if
>>>> diff.nopreofix was
>>>> truthy, because import did not pay attention to that setting. Now we
>>>> inspect
>>>> that setting unless an explicit strip level is specified.
>>>
>>> I don't think this is the right solution here -- much of the time hg
>>> import is used on a repository that's different from the one where hg
>>> export was used. Could we figure out from the patch whether to use -p0
>>> or -p1?
>>
>> I'm pretty unenthusiastic for this solution too. changing the behavior
>> of import based on an option meant for diff display seems sloppy to me.
>>
>> I all cases, this is too much of a behavior change for the freeze.
>> Let's discuss it in February.
>>
>
> Then we need something in bugzilla that makes it more clear if an issue
> is eligible for a fix during the freeze.

Let's aims at updating the code freeze page to clarify how not all bug 
fixes are suitable.

https://www.mercurial-scm.org/wiki/TimeBasedReleasePlan#Rules_for_code_freeze_and_stable_branch_commits
Matt Mackall - Jan. 22, 2016, 11:21 p.m.
On Fri, 2016-01-22 at 10:43 -0800, Siddharth Agarwal wrote:
> On 1/22/16 06:51, Ryan McElroy wrote:
> > # HG changeset patch
> > # User Ryan McElroy <rmcelroy@fb.com>
> > # Date 1453473839 28800
> > #      Fri Jan 22 06:43:59 2016 -0800
> > # Branch stable
> > # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
> > # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
> > import: respect diff.noprefix config (issue4639)
> > 
> > Previously export-import round-trips would not work if diff.nopreofix was
> > truthy, because import did not pay attention to that setting. Now we inspect
> > that setting unless an explicit strip level is specified.
> 
> I don't think this is the right solution here -- much of the time hg 
> import is used on a repository that's different from the one where hg 
> export was used. Could we figure out from the patch whether to use -p0 
> or -p1?

In fact I asked Sid to do the above when he added noprefix, so I still favor
this method. Detecting the lack of a/b prefixes is only a little tricky (you
have to consider the case of the repo which actually has "a" or "b" as directory
names).

But I don't think this is freeze-appropriate.

-- 
Mathematics is the supreme nostalgia of our time.
Siddharth Agarwal - Jan. 22, 2016, 11:22 p.m.
On 1/22/16 15:21, Matt Mackall wrote:
> On Fri, 2016-01-22 at 10:43 -0800, Siddharth Agarwal wrote:
>> On 1/22/16 06:51, Ryan McElroy wrote:
>>> # HG changeset patch
>>> # User Ryan McElroy <rmcelroy@fb.com>
>>> # Date 1453473839 28800
>>> #      Fri Jan 22 06:43:59 2016 -0800
>>> # Branch stable
>>> # Node ID 40555a31b98b027c91b1ccf61477151b3307deb2
>>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>>> import: respect diff.noprefix config (issue4639)
>>>
>>> Previously export-import round-trips would not work if diff.nopreofix was
>>> truthy, because import did not pay attention to that setting. Now we inspect
>>> that setting unless an explicit strip level is specified.
>> I don't think this is the right solution here -- much of the time hg
>> import is used on a repository that's different from the one where hg
>> export was used. Could we figure out from the patch whether to use -p0
>> or -p1?
> In fact I asked Sid to do the above when he added noprefix, so I still favor
> this method. Detecting the lack of a/b prefixes is only a little tricky (you
> have to consider the case of the repo which actually has "a" or "b" as directory
> names).

Oh, you did? Wow, I completely forgot.

> But I don't think this is freeze-appropriate.
>
> -- 
> Mathematics is the supreme nostalgia of our time.
>

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -878,6 +878,9 @@  def tryimportone(ui, repo, hunk, parents
     importbranch = opts.get('import_branch')
     update = not opts.get('bypass')
     strip = opts["strip"]
+    if strip == patch.STRIPUNSET:
+        noprefix = ui.configbool('diff', 'noprefix')
+        strip = 0 if noprefix else 1
     prefix = opts["prefix"]
     sim = float(opts.get('similarity') or 0)
     if not tmpname:
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4582,7 +4582,7 @@  def identify(ui, repo, source=None, rev=
     ui.write("%s\n" % ' '.join(output))
 
 @command('import|patch',
-    [('p', 'strip', 1,
+    [('p', 'strip', patch.STRIPUNSET,
      _('directory strip option for patch. This has the same '
        'meaning as the corresponding patch option'), _('NUM')),
     ('b', 'base', '', _('base path (DEPRECATED)'), _('PATH')),
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -37,6 +37,8 @@  from . import (
     util,
 )
 
+STRIPUNSET = -1 # a flag that means strip has not been explicitly set
+
 gitre = re.compile('diff --git a/(.*) b/(.*)')
 tabsplitter = re.compile(r'(\t+|[^\t]+)')
 
diff --git a/tests/test-import.t b/tests/test-import.t
--- a/tests/test-import.t
+++ b/tests/test-import.t
@@ -1763,3 +1763,19 @@  Importing some extra header
   $ hg log --debug -r . | grep extra
   extra:       branch=default
   extra:       foo=bar
+
+Exporting then importing with diff.noprefix=True
+
+  $ hg expor . --config diff.noprefix=True > $TESTTMP/patch
+  $ hg import $TESTTMP/patch
+  applying $TESTTMP/patch (glob)
+  abort: unable to strip away 1 of 1 dirs from a
+  [255]
+  $ hg import $TESTTMP/patch --config diff.noprefix=True --strip 1
+  applying $TESTTMP/patch (glob)
+  abort: unable to strip away 1 of 1 dirs from a
+  [255]
+  $ hg import $TESTTMP/patch --strip 0
+  applying $TESTTMP/patch (glob)
+  $ hg import $TESTTMP/patch --config diff.noprefix=True
+  applying $TESTTMP/patch (glob)