D7733: hgext: initial version of fastexport extension

pulkit (Pulkit Goyal) phabricator at mercurial-scm.org
Fri Jan 24 08:52:56 EST 2020


pulkit added a comment.
pulkit added subscribers: martinvonz, pulkit.


  The logic looks fine, the code needs to be polished a bit and need some documentation. I left inline comments/nits. It will be nice to have the next version of this patch py3 compatible.

INLINE COMMENTS

> fastexport.py:5
> +# GNU General Public License version 2 or any later version.
> +'''export repositories as git fast-import stream'''
> +from __future__ import absolute_import

TBH I know pretty less about git fast-import, so this help needs more description.

> fastexport.py:105
> +
> +    mark = len(marks) + 1
> +    marks[revid] = mark

it's hard to follow why this is done, need a comment

> fastexport.py:110
> +    ref = convert_to_git_ref(ctx.branch())
> +    description = ctx.description()
> +    buf = ['commit %s\n' % ref,

These temporary variables can be prevented.

> fastexport.py:121
> +        p0ctx = repo[parents[0]]
> +        files = ctx.manifest().diff(p0ctx.manifest())
> +    else:

This one is also same as `ctx.files()` I guess. I remember @martinvonz  did some cleanup here.

> fastexport.py:123
> +    else:
> +        files = ctx.repo().changelog.readfiles(ctx.node())
> +    filebuf = []

This one seems same as `ctx.files()`

> fastexport.py:151
> +    ('e', 'export-marks', '',
> +     _('new marker file to write'), _('FILE')),
> +    ('A', 'authormap', '',

s/marks/marker seems clearer in the flag name.
It's not clear what a marker means. There seems to be no tests for these flags too.

Also, what do you think about having a single flag where you read markers from that file and write back to it.

> fastexport.py:156
> +    helpcategory=command.CATEGORY_IMPORT_EXPORT)
> +def fastexport(ui, repo, *revs, **opts):
> +    opts = pycompat.byteskwargs(opts)

This function needs some documentation love.

> fastexport.py:166
> +        raise error.Abort(_('no revisions matched'))
> +    authorfile = opts.get('authormap')
> +    if authorfile:

this temporary variable can be prevented too

> test-fastexport.t:117
> +
> +  $ hg fastexport > fastexport.blob
> +  $ cat fastexport.blob

any reason you redirect the output in a file and then cat it instead of just printing them on stdout?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7733/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7733

To: joerg.sonnenberger, #hg-reviewers, durin42
Cc: pulkit, martinvonz, durin42, mjpieters, mercurial-devel


More information about the Mercurial-devel mailing list