This is an archive of the discontinued Mercurial Phabricator instance.

hgext: initial version of fastexport extension
ClosedPublic

Authored by joerg.sonnenberger on Dec 27 2019, 3:12 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 accepted this revision as: durin42.Jan 8 2020, 2:30 PM
durin42 added a subscriber: durin42.

I only did a light pass, but looks good to me.

Any plans for a fast-export importer as well?

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.

hgext/fastexport.py
6

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

106

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

111

These temporary variables can be prevented.

122

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

124

This one seems same as ctx.files()

152

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.

157

This function needs some documentation love.

167

this temporary variable can be prevented too

tests/test-fastexport.t
118

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

joerg.sonnenberger marked 8 inline comments as done.Feb 6 2020, 8:09 AM
joerg.sonnenberger updated this revision to Diff 19932.

Now blank and Python 3 clean.

hgext/fastexport.py
111

Will be dropped.

122

For a merge, ctx.files() doesn't contain added or removed files relative to one parent. This is covered by tests.

124

It is nowadays.

152

This follows the terminology used in other implementations of this functionality and being consistent on that front seems more important. That said, I'm changing it to marks file consistently. A single flag doesn't help with typical use cases. For incremental conversion, you only want to use the newly created marks file when the other part of the conversion (e.g. git fast-import) was successful too.

167

I don't like doing the lookup twice, so I would prefer to keep the variable in this case.

tests/test-fastexport.t
118

It was easier for testing. Dropping this part.

pulkit accepted this revision.Feb 7 2020, 3:28 PM
This revision is now accepted and ready to land.Feb 7 2020, 3:28 PM
This revision was automatically updated to reflect the committed changes.