Patchwork [STABLE] import: mark --exact as experimental

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 25, 2016, 9:53 a.m.
Message ID <a412b87ce53add0d4304.1456394006@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/13380/
State Deferred
Headers show

Comments

Pierre-Yves David - Feb. 25, 2016, 9:53 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1456060026 -3600
#      Sun Feb 21 14:07:06 2016 +0100
# Branch stable
# Node ID a412b87ce53add0d4304de0170140bff1dc266b3
# Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
# EXP-Topic expexact
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r a412b87ce53a
import: mark --exact as experimental

The feature have been plagued by very serious bugs for years. Let's hide if from
unsuspecting users until theses get fixed.

The main issue is that --exact is going to test that the resulting node match
the one recorded in the patch, matches the resulting commit. However, as we
don't export most of the "extra" contents in the patch, they are not preserved
and the hash changes, leading to the import being aborted even if everything
else applied cleanly.
Augie Fackler - Feb. 25, 2016, 4:12 p.m.
On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1456060026 -3600
> #      Sun Feb 21 14:07:06 2016 +0100
> # Branch stable
> # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
> # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
> # EXP-Topic expexact
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r a412b87ce53a
> import: mark --exact as experimental

+1, but I'm unsure if it belongs on stable. Matt?


>
> The feature have been plagued by very serious bugs for years. Let's hide if from
> unsuspecting users until theses get fixed.
>
> The main issue is that --exact is going to test that the resulting node match
> the one recorded in the patch, matches the resulting commit. However, as we
> don't export most of the "extra" contents in the patch, they are not preserved
> and the hash changes, leading to the import being aborted even if everything
> else applied cleanly.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4593,15 +4593,15 @@ def identify(ui, repo, source=None, rev=
>      ('', 'bypass', None,
>       _("apply patch without touching the working directory")),
>      ('', 'partial', None,
>       _('commit even if some hunks fail')),
>      ('', 'exact', None,
> -     _('apply patch to the nodes from which it was generated')),
> +     _('apply patch to the nodes from which it was generated (EXPERIMENTAL)')),
>      ('', 'prefix', '',
>       _('apply patch to subdirectory'), _('DIR')),
>      ('', 'import-branch', None,
> -     _('use any branch information in patch (implied by --exact)'))] +
> +     _('use any branch information in patch'))] + # (implied by --exact)
>      commitopts + commitopts2 + similarityopts,
>      _('[OPTION]... PATCH...'))
>  def import_(ui, repo, patch1=None, *patches, **opts):
>      """import an ordered set of patches
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Mackall - Feb. 26, 2016, 8:17 p.m.
On Thu, 2016-02-25 at 11:12 -0500, Augie Fackler wrote:
> On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@fb.com>
> > # Date 1456060026 -3600
> > #      Sun Feb 21 14:07:06 2016 +0100
> > # Branch stable
> > # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
> > # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
> > # EXP-Topic expexact
> > # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> > #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> > a412b87ce53a
> > import: mark --exact as experimental
> 
> +1, but I'm unsure if it belongs on stable. Matt?

I don't like this at all, sorry.

It works as advertised:

   If --exact is specified, import will set the working directory to the
    parent of each patch before applying it, and will abort if the resulting
    changeset has a different ID than the one recorded in the patch. This may
    happen due to character set problems or other deficiencies in the text
    patch format.

This feature has never been a promise to perfectly transfer every possible
changeset. We've never been able to do that and it's unlikely that we ever will.
And extra fields aren't even a significant obstacle to that.

Instead this feature is a promise to perfectly transfer OR REJECT anything that
isn't perfectly transferred. So everything you apparently think is a bug is
actually it doing its data-integrity-protecting job.

> The feature have been plagued by very serious bugs for years.

issue3616 from 3 years ago was indeed a very serious bug, but it's the only one
I'm aware of.

-- 
Mathematics is the supreme nostalgia of our time.
Pierre-Yves David - Feb. 26, 2016, 11:18 p.m.
On 02/26/2016 09:17 PM, Matt Mackall wrote:
> On Thu, 2016-02-25 at 11:12 -0500, Augie Fackler wrote:
>> On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1456060026 -3600
>>> #      Sun Feb 21 14:07:06 2016 +0100
>>> # Branch stable
>>> # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
>>> # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
>>> # EXP-Topic expexact
>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>> a412b87ce53a
>>> import: mark --exact as experimental
>>
>> +1, but I'm unsure if it belongs on stable. Matt?
>
> I don't like this at all, sorry.
>
> It works as advertised:
>
>     If --exact is specified, import will set the working directory to the
>      parent of each patch before applying it, and will abort if the resulting
>      changeset has a different ID than the one recorded in the patch. This may
>      happen due to character set problems or other deficiencies in the text
>      patch format.
>
> This feature has never been a promise to perfectly transfer every possible
> changeset. We've never been able to do that and it's unlikely that we ever will.
> And extra fields aren't even a significant obstacle to that.
>
> Instead this feature is a promise to perfectly transfer OR REJECT anything that
> isn't perfectly transferred. So everything you apparently think is a bug is
> actually it doing its data-integrity-protecting job.

Okay, that precise feature work as advertised "A perfect importer with 
integrity check". And integrity check are cool, their are not what I'm 
calling a bug here. The issue lies in the fact that this perfect 
importer is pretty much useless to anyone because we don't really have a 
perfect exported. 'hg export' is the best patch generator around, and it 
is unable to produce a patch that will pass 'hg import --exact' in most 
case.

'hg import --exact' does not exists in the vacum, people who attempt to 
use it pretty much always try to use it in conjunction with 'hg export'. 
As 'hg export' does not produce good content -for-simple-case-, this 
whole feature line breaks. It is useless and frustrating to all users 
I've seen trying to use this feature.

Of course there will be corner case with various encoding stuff or other 
special case where 'hg export | hg import --exact' might break. But we 
are talking about common case here.

And, I insist, the common case is broken. 'hg export' does not convey 
the content of the 'extra' fields (except for a couple of exception). 
This make the generated patch omit important information that are part 
of the node idea. Dooming the patch to never pass 'hg import --exact'. 
And such extra field are not corner cases. In the last 1000 changesets 
in main, there is 872 usage of unexpected extra. This make less that 15% 
of the last changesets in Mercurial own repository able to pass a simple 
'hg export | hg import --exact'.

To conclude, this feature chain is currently broken in the common case. 
Extra are currently the most significant reason for this breakage. There 
is various way to solve the extra problem once and for all and reinstall 
this feature chain to its full glory. In the mean time I would like to 
mark this flag as EXPERIMENTAL to prevent unsuspecting user to find it, 
expect it to be useful and finally, be deceived

Cheers,
Augie Fackler - Feb. 26, 2016, 11:23 p.m.
> On Feb 26, 2016, at 6:18 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 02/26/2016 09:17 PM, Matt Mackall wrote:
>> On Thu, 2016-02-25 at 11:12 -0500, Augie Fackler wrote:
>>> On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1456060026 -3600
>>>> #      Sun Feb 21 14:07:06 2016 +0100
>>>> # Branch stable
>>>> # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
>>>> # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
>>>> # EXP-Topic expexact
>>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>>> a412b87ce53a
>>>> import: mark --exact as experimental
>>> 
>>> +1, but I'm unsure if it belongs on stable. Matt?
>> 
>> I don't like this at all, sorry.
>> 
>> It works as advertised:
>> 
>>    If --exact is specified, import will set the working directory to the
>>     parent of each patch before applying it, and will abort if the resulting
>>     changeset has a different ID than the one recorded in the patch. This may
>>     happen due to character set problems or other deficiencies in the text
>>     patch format.
>> 
>> This feature has never been a promise to perfectly transfer every possible
>> changeset. We've never been able to do that and it's unlikely that we ever will.
>> And extra fields aren't even a significant obstacle to that.
>> 
>> Instead this feature is a promise to perfectly transfer OR REJECT anything that
>> isn't perfectly transferred. So everything you apparently think is a bug is
>> actually it doing its data-integrity-protecting job.
> 
> Okay, that precise feature work as advertised "A perfect importer with integrity check". And integrity check are cool, their are not what I'm calling a bug here. The issue lies in the fact that this perfect importer is pretty much useless to anyone because we don't really have a perfect exported. 'hg export' is the best patch generator around, and it is unable to produce a patch that will pass 'hg import --exact' in most case.

It’s not actually most cases if you don’t use extensions. I’m with mpm on this - we can make more cases work with —exact, but the flag does in fact behave as advertised, so we’re better off fixing the bugs than trying to hide the feature for now.

It sure stinks for rebase users et al, but in general I think bug fixes are the way to go rather than marking the flag as experimental.
Martin von Zweigbergk - Feb. 26, 2016, 11:23 p.m.
On Fri, Feb 26, 2016 at 3:18 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 02/26/2016 09:17 PM, Matt Mackall wrote:
>>
>> On Thu, 2016-02-25 at 11:12 -0500, Augie Fackler wrote:
>>>
>>> On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1456060026 -3600
>>>> #      Sun Feb 21 14:07:06 2016 +0100
>>>> # Branch stable
>>>> # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
>>>> # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
>>>> # EXP-Topic expexact
>>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>>> a412b87ce53a
>>>> import: mark --exact as experimental
>>>
>>>
>>> +1, but I'm unsure if it belongs on stable. Matt?
>>
>>
>> I don't like this at all, sorry.
>>
>> It works as advertised:
>>
>>     If --exact is specified, import will set the working directory to the
>>      parent of each patch before applying it, and will abort if the
>> resulting
>>      changeset has a different ID than the one recorded in the patch. This
>> may
>>      happen due to character set problems or other deficiencies in the
>> text
>>      patch format.
>>
>> This feature has never been a promise to perfectly transfer every possible
>> changeset. We've never been able to do that and it's unlikely that we ever
>> will.
>> And extra fields aren't even a significant obstacle to that.
>>
>> Instead this feature is a promise to perfectly transfer OR REJECT anything
>> that
>> isn't perfectly transferred. So everything you apparently think is a bug
>> is
>> actually it doing its data-integrity-protecting job.
>
>
> Okay, that precise feature work as advertised "A perfect importer with
> integrity check". And integrity check are cool, their are not what I'm
> calling a bug here.

FWIW, I used --exact not in the hopes that it would check the
integrity of the patch, but that it would apply cleanly, so I could
then rebase it and get nice 3-way file merges rather than dealing with
.rej files. If that's what others (the people Pierre-Yves mentioned
were frustrated) were trying to do as well, how about leaving --exact
alone and add a new --preserve-parent or so?
Pierre-Yves David - Feb. 26, 2016, 11:31 p.m.
On 02/27/2016 12:23 AM, Augie Fackler wrote:
>
>> On Feb 26, 2016, at 6:18 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 02/26/2016 09:17 PM, Matt Mackall wrote:
>>> On Thu, 2016-02-25 at 11:12 -0500, Augie Fackler wrote:
>>>> On Thu, Feb 25, 2016 at 10:53:26AM +0100, Pierre-Yves David wrote:
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>>> # Date 1456060026 -3600
>>>>> #      Sun Feb 21 14:07:06 2016 +0100
>>>>> # Branch stable
>>>>> # Node ID a412b87ce53add0d4304de0170140bff1dc266b3
>>>>> # Parent  cb6a952efbf48d306a7c82d7fe46115f072cbb1d
>>>>> # EXP-Topic expexact
>>>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>>>> a412b87ce53a
>>>>> import: mark --exact as experimental
>>>>
>>>> +1, but I'm unsure if it belongs on stable. Matt?
>>>
>>> I don't like this at all, sorry.
>>>
>>> It works as advertised:
>>>
>>>     If --exact is specified, import will set the working directory to the
>>>      parent of each patch before applying it, and will abort if the resulting
>>>      changeset has a different ID than the one recorded in the patch. This may
>>>      happen due to character set problems or other deficiencies in the text
>>>      patch format.
>>>
>>> This feature has never been a promise to perfectly transfer every possible
>>> changeset. We've never been able to do that and it's unlikely that we ever will.
>>> And extra fields aren't even a significant obstacle to that.
>>>
>>> Instead this feature is a promise to perfectly transfer OR REJECT anything that
>>> isn't perfectly transferred. So everything you apparently think is a bug is
>>> actually it doing its data-integrity-protecting job.
>>
>> Okay, that precise feature work as advertised "A perfect importer with integrity check". And integrity check are cool, their are not what I'm calling a bug here. The issue lies in the fact that this perfect importer is pretty much useless to anyone because we don't really have a perfect exported. 'hg export' is the best patch generator around, and it is unable to produce a patch that will pass 'hg import --exact' in most case.
>
> It’s not actually most cases if you don’t use extensions. I’m with mpm on this - we can make more cases work with —exact, but the flag does in fact behave as advertised, so we’re better off fixing the bugs than trying to hide the feature for now.
>
> It sure stinks for rebase users et al, but in general I think bug fixes are the way to go rather than marking the flag as experimental.

The extra issue is known for year and have not been fixed yet. I would 
rather hide the flag until someone actually commit to fix the extra 
issue. All I've seem in the wild about --exact is user frustraction. 
multiple multiple company and community.

Note: Core feature like amend also add extras. And other extensions 
adding extras are thing like 'rebase' which is very widely used.

Cheers,

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4593,15 +4593,15 @@  def identify(ui, repo, source=None, rev=
     ('', 'bypass', None,
      _("apply patch without touching the working directory")),
     ('', 'partial', None,
      _('commit even if some hunks fail')),
     ('', 'exact', None,
-     _('apply patch to the nodes from which it was generated')),
+     _('apply patch to the nodes from which it was generated (EXPERIMENTAL)')),
     ('', 'prefix', '',
      _('apply patch to subdirectory'), _('DIR')),
     ('', 'import-branch', None,
-     _('use any branch information in patch (implied by --exact)'))] +
+     _('use any branch information in patch'))] + # (implied by --exact)
     commitopts + commitopts2 + similarityopts,
     _('[OPTION]... PATCH...'))
 def import_(ui, repo, patch1=None, *patches, **opts):
     """import an ordered set of patches