| 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
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
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.
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
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
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.
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)