Patchwork convert: .hgtags files contain SKIP lines (issue3736)

login
register
mail settings
Submitter John Peacock
Date Dec. 26, 2012, 6:14 p.m.
Message ID <0198939c241c0ed4f6ba.1356545653@jpeacock.int.messagesystems.com>
Download mbox | patch
Permalink /patch/296/
State Changes Requested
Headers show

Comments

John Peacock - Dec. 26, 2012, 6:14 p.m.
# HG changeset patch
# User John Peacock <john.peacock at messagesystems.com>
# Date 1356542038 18000
# Branch bugfix/3736
# Node ID 0198939c241c0ed4f6ba40196e24f36eb85aea56
# Parent  730b769fb6347d8da3c9e0674e7133bdf61d7a36
convert: .hgtags files contain SKIP lines (issue3736)

convert is writing SKIP lines into the various .hgtags files when a
tag corresponds to a revision marked to be skipped in the conversion
(e.g. with filemap).  These lines should only be maintained in the
shamap file, not in the .hgtags (where they emit unhelpful warnings).
Minimal patch to ignore SKIP lines when updating the .hgtags files.
Mads Kiilerich - Dec. 26, 2012, 9:37 p.m.
John Peacock wrote, On 12/26/2012 07:14 PM:
> # HG changeset patch
> # User John Peacock <john.peacock at messagesystems.com>
> # Date 1356542038 18000
> # Branch bugfix/3736
> # Node ID 0198939c241c0ed4f6ba40196e24f36eb85aea56
> # Parent  730b769fb6347d8da3c9e0674e7133bdf61d7a36
> convert: .hgtags files contain SKIP lines (issue3736)
>
> convert is writing SKIP lines into the various .hgtags files when a
> tag corresponds to a revision marked to be skipped in the conversion
> (e.g. with filemap).  These lines should only be maintained in the
> shamap file, not in the .hgtags (where they emit unhelpful warnings).
> Minimal patch to ignore SKIP lines when updating the .hgtags files.

Please try to phrase the patch description as a description of the patch 
and the solution. It might be a good idea to include a description of 
the problem, but it should make it clear when it is describing the 
previous bad behaviour and when it is describing the new and better 
behaviour.

Please also try to include a test in the patch. Tweak one of the 
existing convert tests in such a way that it would demonstrate the 
problem if the fix was left out.

/Mads

>
> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
> --- a/hgext/convert/hg.py
> +++ b/hgext/convert/hg.py
> @@ -23,7 +23,7 @@
>   from mercurial.node import bin, hex, nullid
>   from mercurial import hg, util, context, bookmarks, error
>   
> -from common import NoRepo, commit, converter_source, converter_sink
> +from common import SKIPREV, NoRepo, commit, converter_source, converter_sink
>   
>   class mercurial_sink(converter_sink):
>       def __init__(self, ui, path):
> @@ -126,6 +126,8 @@
>               revid = revmap.get(source.lookuprev(s[0]))
>               if not revid:
>                   continue
> +            if revid == SKIPREV:
> +                continue
>               fp.write('%s %s\n' % (revid, s[1]))
>           return fp.getvalue()
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Jan. 9, 2013, 9:34 p.m.
On Wed, 2012-12-26 at 22:37 +0100, Mads Kiilerich wrote:
> John Peacock wrote, On 12/26/2012 07:14 PM:
> > # HG changeset patch
> > # User John Peacock <john.peacock@messagesystems.com>
> > # Date 1356542038 18000
> > # Branch bugfix/3736
> > # Node ID 0198939c241c0ed4f6ba40196e24f36eb85aea56
> > # Parent  730b769fb6347d8da3c9e0674e7133bdf61d7a36
> > convert: .hgtags files contain SKIP lines (issue3736)
> >
> > convert is writing SKIP lines into the various .hgtags files when a
> > tag corresponds to a revision marked to be skipped in the conversion
> > (e.g. with filemap).  These lines should only be maintained in the
> > shamap file, not in the .hgtags (where they emit unhelpful warnings).
> > Minimal patch to ignore SKIP lines when updating the .hgtags files.
> 
> Please try to phrase the patch description as a description of the patch 
> and the solution. It might be a good idea to include a description of 
> the problem, but it should make it clear when it is describing the 
> previous bad behaviour and when it is describing the new and better 
> behaviour.

As a more concrete example:

BAD:  convert: .hgtags files contain SKIP lines (issue3736)
GOOD: convert: don't write SKIP lines to .hgtags (issue3736)

BAD:  convert is writing SKIP lines...
GOOD: convert was writing SKIP lines...
John Peacock - Jan. 9, 2013, 9:39 p.m.
On 01/09/2013 04:34 PM, Matt Mackall wrote:
> As a more concrete example:
>
> BAD:  convert: .hgtags files contain SKIP lines (issue3736)
> GOOD: convert: don't write SKIP lines to .hgtags (issue3736)
>
> BAD:  convert is writing SKIP lines...
> GOOD: convert was writing SKIP lines...
>

Thanks.  I'm planning on resubmitting but I'm not having any luck 
creating a test case that doesn't involve my huge repo... ;-)

John
Matt Mackall - Jan. 10, 2013, 12:43 a.m.
On Wed, 2013-01-09 at 16:39 -0500, John Peacock wrote:
> On 01/09/2013 04:34 PM, Matt Mackall wrote:
> > As a more concrete example:
> >
> > BAD:  convert: .hgtags files contain SKIP lines (issue3736)
> > GOOD: convert: don't write SKIP lines to .hgtags (issue3736)
> >
> > BAD:  convert is writing SKIP lines...
> > GOOD: convert was writing SKIP lines...
> >
> 
> Thanks.  I'm planning on resubmitting but I'm not having any luck 
> creating a test case that doesn't involve my huge repo... ;-)

Hmm, seems we might not have a SKIP in the test suite.

We should, so:

a) minimal change to one of existing test cases to SKIP something

Then:

b) make sure that the skipped rev is tagged to trigger the issue

Patch

diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -23,7 +23,7 @@ 
 from mercurial.node import bin, hex, nullid
 from mercurial import hg, util, context, bookmarks, error
 
-from common import NoRepo, commit, converter_source, converter_sink
+from common import SKIPREV, NoRepo, commit, converter_source, converter_sink
 
 class mercurial_sink(converter_sink):
     def __init__(self, ui, path):
@@ -126,6 +126,8 @@ 
             revid = revmap.get(source.lookuprev(s[0]))
             if not revid:
                 continue
+            if revid == SKIPREV:
+                continue
             fp.write('%s %s\n' % (revid, s[1]))
         return fp.getvalue()