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