Issue 4651: Mercurial support for upload.py (Closed)
|
Created:
17 years, 1 month ago by Antoine Pitrou Modified:
16 years, 3 months ago Visibility:
Public. |
DescriptionThis patch implements Mercurial support for upload.py. It can be run from the base repository directory or from any subdirectory of it (in which case it will only consider files inside the subtree).
This very patch was uploaded using the modified version.
Patch Set 1 #Patch Set 2 : New attempt - adding a newline of the end of diff to see if it solves the problem #Patch Set 3 : New patch using output of "hg parent" instead of "tip" #Patch Set 4 : Updated patch against trunk, correcting all remarks and implementing support for --rev #Patch Set 5 : New patch with handling of binary and image files #Patch Set 6 : Fix bug with removed files #MessagesTotal messages: 20
Ouch. The first time I tried to display the side-by-side diff, it worked. But after having edited the issue to add the reviewers, it doesn't work anymore ! (I get the infamous "Bad content" message)
Sign in to reply to this message.
It seems to be working better... http://codereview.appspot.com/4651/diff/202/4 File static/upload.py (right): http://codereview.appspot.com/4651/diff/202/4#newcode792 Line 792: self.base_rev = "tip" This selects "tip" inconditionally but perhaps we should use the current parent revision instead. Any advice? (this comment also serves as a test btw.)
Sign in to reply to this message.
On 2008/09/05 19:42:17, Antoine Pitrou wrote: > It seems to be working better... > > http://codereview.appspot.com/4651/diff/202/4 > File static/upload.py (right): > > http://codereview.appspot.com/4651/diff/202/4#newcode792 > Line 792: self.base_rev = "tip" > This selects "tip" inconditionally but perhaps we should use the current parent > revision instead. Any advice? > > (this comment also serves as a test btw.) Thanks for working on it. I started to use git recently, so I am not so much interested in mercurial anymore. So if you could finish it, it's be awesome. Thanks, Ondrej
Sign in to reply to this message.
http://codereview.appspot.com/4651/diff/202/4 File static/upload.py (right): http://codereview.appspot.com/4651/diff/202/4#newcode792 Line 792: self.base_rev = "tip" On 2008/09/05 19:42:17, Antoine Pitrou wrote: > This selects "tip" inconditionally but perhaps we should use the current parent > revision instead. Any advice? Have a look at the Bazaar patch (http://codereview.appspot.com/2891), it introduces a --diff_base flag for DVCS implementations to specify the base for the diff. Maybe "tip" could be the default here if the flag isn't set? > > (this comment also serves as a test btw.) >
Sign in to reply to this message.
Hi, (I just tested the patch under Windows. It seems to work there too) On 2008/09/05 20:06:41, andi.albrecht wrote: > Have a look at the Bazaar patch (http://codereview.appspot.com/2891), it > introduces a --diff_base flag for DVCS implementations to specify the base for > the diff. Maybe "tip" could be the default here if the flag isn't set? Agreed, but I didn't want to make my patch depend on another one. If you commit the Bazaar patch very soon (I can't really review it, sorry, as I don't know bzr and I don't know Rietveld very much either), then I'll rework my patch to take it into account. By the way, "--diff-base" is horrible. Why not simply "-r"? It's the standard for specifying revision ids (e.g. "svn diff -r", "hg diff -r").
Sign in to reply to this message.
On 2008/09/05 20:20:09, Antoine Pitrou wrote: > Hi, > > (I just tested the patch under Windows. It seems to work there too) > > > On 2008/09/05 20:06:41, andi.albrecht wrote: > > Have a look at the Bazaar patch (http://codereview.appspot.com/2891), it > > introduces a --diff_base flag for DVCS implementations to specify the base for > > the diff. Maybe "tip" could be the default here if the flag isn't set? > > Agreed, but I didn't want to make my patch depend on another one. If you commit > the Bazaar patch very soon (I can't really review it, sorry, as I don't know bzr > and I don't know Rietveld very much either), then I'll rework my patch to take > it into account. > > By the way, "--diff-base" is horrible. Why not simply "-r"? It's the standard > for specifying revision ids (e.g. "svn diff -r", "hg diff -r"). I had no time for a full review yet, but I think your patch should go in before the Bazaar patch because I have some minor change for Bazaar support. Agreed, "--diff_base" isn't probably the best choice... I've discussed this with Evan some time ago. He had some concerns using "--revision" because you mostly compare against branches when using DVCS. Somehow I came out with this ugly name. After having a look at the various tools I would switch back to "--revision" which was my first choice. I'll add this option when Mercurial support is in for both hg and git, using "hg parent" in the meantime should be ok. I'll do a full review of your patch ASAP.
Sign in to reply to this message.
http://codereview.appspot.com/4651/diff/205/9 File static/upload.py (right): http://codereview.appspot.com/4651/diff/205/9#newcode792 Line 792: self.base_rev = RunShell(["hg", "parent", "-q"]).split(':')[1].strip() hm, maybe I've did something wrong (I'm no hg user). Does this mean that without choosing a revision we always only fetch uncommitted changes? In that case, I'd prefer to have the --revision option to upload.py first because IMO it's a common case to compare two branches. Sorry, I misunderstood the "hg parent" command... I've uploaded a small patch that adds this option without the Bazaar stuff (http://codereview.appspot.com/4842). http://codereview.appspot.com/4651/diff/205/9#newcode803 Line 803: cmd += ["."] upload.py allows to specify filenames as command line arguments. The docstring is not very clear as it only mentions [-- diff_options] but at least it works and maybe some people are using it. Not sure if we should treat this case here. http://codereview.appspot.com/4651/diff/205/9#newcode814 Line 814: assert filename == m.group(2) AssertionError is raised for moved files, e.g. $ hg mv README README.txt $ hg st --rev d3e56cec6b05 A README.txt R README The second group is the name of the new file (README.txt in that case). http://codereview.appspot.com/4651/diff/205/9#newcode848 Line 848: status = RunShell(["hg", "st", "--rev", self.base_rev, relpath]) Results in an error message if a previously empty file was modified: No output from ['hg', 'cat', '-r', 'd3e56cec6b05', '__init__.py'] http://codereview.appspot.com/4651/diff/205/9#newcode851 Line 851: return ("", "A") Status "R" (deleted) is not handled here. A removed file shows up as modified (M). http://codereview.appspot.com/4651/diff/205/9#newcode936 Line 936: # Try running it, but don't die if we don't have hg installed. I'd add a comment that this test should run before the Subversion part as hg can sit upon svn.
Sign in to reply to this message.
On 2008/09/07 17:21:30, andi.albrecht wrote: > hm, maybe I've did something wrong (I'm no hg user). Does this mean that without > choosing a revision we always only fetch uncommitted changes? Yes. > In that case, I'd prefer to have the --revision option to upload.py first > because IMO it's a common case to compare two branches. I'm all for --revision :) > http://codereview.appspot.com/4651/diff/205/9#newcode803 > Line 803: cmd += ["."] > upload.py allows to specify filenames as command line arguments. Oops, I didn't know. I'll try to modify my patch to take it into account. > http://codereview.appspot.com/4651/diff/205/9#newcode814 > Line 814: assert filename == m.group(2) > AssertionError is raised for moved files, e.g. Ok, I'll remove it. > Results in an error message if a previously empty file was modified: > No output from ['hg', 'cat', '-r', 'd3e56cec6b05', '__init__.py'] Ok, I guess I have to add an option to RunShell(). > http://codereview.appspot.com/4651/diff/205/9#newcode851 > Line 851: return ("", "A") > Status "R" (deleted) is not handled here. A removed file shows up as modified > (M). Mmmh, I'll look into this as well. > I'd add a comment that this test should run before the Subversion part as hg can > sit upon svn. Ok. Thanks for the review! I think we must now focus on getting your --revision patch committed and I'll work on top of it.
Sign in to reply to this message.
I'll let others review this. I think it's a good idea though!
Sign in to reply to this message.
Antoine, the --rev option is now committed.
Sign in to reply to this message.
On 2008/09/17 08:12:14, andi.albrecht wrote: > Antoine, the --rev option is now committed. This new patch (patch set 4) is against current trunk, it handles all the comments made to the previous patch, has been tested with added/modified/renamed/deleted files, and supports the --rev option (very handy with named branches!). Please review.
Sign in to reply to this message.
Looks good for me, but I'm no experienced Mercurial user... ;-) @Ondrej: Do you have time for a review? @John: AFAICT the Mercurial implementation always uses is_binary=False in GetBaseFile. Is this a problem somewhere?
Sign in to reply to this message.
On 2008/09/28 07:09:18, andi.albrecht wrote: > > @John: AFAICT the Mercurial implementation always uses is_binary=False in > GetBaseFile. What are new_content and is_binary supposed to be used for? I just mimicked the Git implementation which always leave those to None and False (respectively).
Sign in to reply to this message.
Attached a new patch which adds handling of binary and image files, and uses the unabbreviated versions of hg commands (so as to avoid potential clashes if hg extensions register commands starting with the same two letters as builtin ones). Based on suggestions and patch sent in private by Tom Lynn.
Sign in to reply to this message.
Mostly looks good to me. I've had a problem with removed files and --rev option, but I'm not sure if if was my fault... ,-) http://codereview.appspot.com/4651/diff/1402/1601 File static/upload.py (right): http://codereview.appspot.com/4651/diff/1402/1601#newcode1053 Line 1053: base_content = RunShell(["hg", "cat", "-r", self.base_rev, oldrelpath], I've had a problem here with deleted files: Got error status from ['hg', 'cat', '-r', '41eda4bdbd27', 'R TODO'] Note 'R TODO' as the filename. but maybe it was my fault... So here's what I've done: 1.) hg init in Rietveld's directory 2.) hg add & commit 3.) hg rm TODO 4.) hg commit 5.) Run upload.py with --rev <COMMIT FROM 2>
Sign in to reply to this message.
Uploaded new patch to fix the problem with removed files. Please review :) http://codereview.appspot.com/4651/diff/1402/1601 File static/upload.py (right): http://codereview.appspot.com/4651/diff/1402/1601#newcode1053 Line 1053: base_content = RunShell(["hg", "cat", "-r", self.base_rev, oldrelpath], On 2008/10/03 07:56:53, andi.albrecht wrote: > I've had a problem here with deleted files: > Oh, "hg st" outputs an error message before the status line in that case: $ hg status -C --rev 0 TODO TODO: No such file or directory R TODO Uploaded new patch to circumvent the problem, and opened a bug in Mercurial (http://www.selenic.com/mercurial/bts/issue1323)
Sign in to reply to this message.
On second thought, it might be better to separate stdout from stderr in RunShell() (at least with an option)... it would make it easier to strip unwanted error messages.
Sign in to reply to this message.
Committed r339. Thanks Antoine!! --> http://codereview.appspot.com/static/upload.py
Sign in to reply to this message.
On 2008/10/03 22:21:58, andi.albrecht wrote: > Committed r339. Thanks Antoine!! > --> http://codereview.appspot.com/static/upload.py Antoine, you can close this issue now. Thanks again!
Sign in to reply to this message.
On 2008/10/08 04:52:45, andi.albrecht wrote: > On 2008/10/03 22:21:58, andi.albrecht wrote: > > Committed r339. Thanks Antoine!! > > --> http://codereview.appspot.com/static/upload.py > > Antoine, you can close this issue now. Thanks again! Oops, indeed. Sorry!
Sign in to reply to this message.
|
