Details
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
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. 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? |
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. |
TBH I know pretty less about git fast-import, so this help needs more description.