Jeremie Nikaes <jeremie.nikaes@xxxxxxxxxxxxxxx> writes:
> Implement a gate between git and mediawiki, allowing git users to
> ...
> For now, the whole wiki is cloned, but it will be possible to clone only
> some pages: the clone is based on a list of pages which is now all
> pages.
>
> Code clarified & improved with the help of Jeff King and Junio C Hamano.
>
> We were not able to reproduce the empty timestamp bug noticed by Jeff
> King, thus needing some further testing. A placeholder is still
> implemented in case.
>
> Signed-off-by: JÃrÃmie Nikaes <jeremie.nikaes@xxxxxxxxxxxxxxx>
> Signed-off-by: Arnaud Lacurie <arnaud.lacurie@xxxxxxxxxxxxxxx>
> Signed-off-by: Claire Fousse <claire.fousse@xxxxxxxxxxxxxxx>
> Signed-off-by: David Amouyal <david.amouyal@xxxxxxxxxxxxxxx>
> Signed-off-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
> Signed-off-by: Sylvain Boulmà <sylvain.boulme@xxxxxxx>
> ---
> contrib/mw-to-git/git-remote-mediawiki | 306 ++++++++++++++++++++++++++++
> contrib/mw-to-git/git-remote-mediawiki.txt | 7 +
> 2 files changed, 313 insertions(+), 0 deletions(-)
This seems to have grown a bit.
I won't repeat issues I pointed out in the earlier round but not updated
in this patch to save time.
> +########################## Functions ##############################
> +
> +sub get_last_local_revision {
> + # Get last commit sha1
> + my $commit_sha1 = `git rev-parse refs/mediawiki/$remotename/master 2>/dev/null`;
> +
> + # Get note regarding that commit
> + chomp($commit_sha1);
> + my $note = `git log --notes=mediawiki 2>/dev/null | grep "mediawiki_revision: " | sed -n 1p`;
Two and half issues:
- You are writing Perl no? Don't call grep/sed from it. Your
environment is much richer and more flexible ;-).
- You grab $commit_sha1 but never use it. Did you mean to throw it at the
"git log" above?
- Is there a reason you use "git log" to traverse the history all the way
down to the root commit? Wouldn't
git notes --ref=mediawiki show $commit_sha1
or even better yet, just doing
git notes --ref=mediawiki show refs/mediawiki/$remotename/master
without the first rev-parse sufficient?
Are you protecting against the case where some commits in the history
leading to mediawiki/$remotename/master may not have the "mediawiki"
note, and falling back to the latest commit that has note? You may find
such a commit, but that may be different from the commit at the tip of
mediawiki/$remotename/master branch. Is that a correct thing to do?
IOW, does _any_ previous version do for the purpose of this function?
(This paragraph is not a rhetorical question).
> +sub mw_capabilities {
> + # Revisions are imported to the private namespace
> + # refs/mediawiki/$remotename/ by the helper and fetched into
> + # refs/remotes/$remotename later by fetch.
> + print STDOUT "refspec refs/heads/*:refs/mediawiki/$remotename/*\n";
> + print STDOUT "import\n";
> + print STDOUT "list\n";
> + print STDOUT "option\n";
> + print STDOUT "push\n";
> + print STDOUT "\n";
There are many explicit references to STDOUT like this, and also many
unqualified "print" that spits out to the default which is STDOUT in the
codepath to feed fast-import. Is that intentional, or is it just coming
from difference in style of people who worked on different parts of the
code?
If there is no reason to use two styles, please pick one and stick to
it. If there _is_ reason, please document what are the criteria to choose
which one in each codepath. Otherwise you would waste time of your
reviewers who have to wonder which one is correct in which codepath.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html