> 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. Yes sorry, I actually commited a patch where the weird loop control was not fixed yet. My mistake, will fix this asap. About the print STDERR lines, what should we do about them ? They do grow the patch up but if we remove them completely the script seems to be frozen because of no text output for the user. Should we put them in a verbose mode ? > Two and half issues: > > - You are writing Perl no? Don't call grep/sed from it. Your > environment is much richer and more flexible ;-). Okay it's true that we discovered Perl as we started this project so we were more comfortable with this kind of commands. Will work on that. > > - 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). You are totally right. We actually were trying to find a command that did this without having to parse the git log, git notes --ref=mediawiki show refs/mediawiki/$remotename/master does this perfectly. In practice, a mediawiki/$remotename/master commit should ALWAYS have a note attached to it, so we did not try to protect against this case here. Should we be concerned and do something about it ? > 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. Yes you are right, that slipped too. Thanks for noticing this, fixing it. As always, thanks for the feedback, working on improving this. -- Jérémie Nikaes -- 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