|
|
Created:
10 years, 10 months ago by Nasser Grainawi Modified:
9 years, 7 months ago CC:
chromium-reviews, chromium-os-reviews_chromium.org, M-A Ruel Visibility:
Public. |
Descriptionsync @branchname git support
Also improve GIT.update error handling and verbosity levels
TEST=unit tests
BUG=http://crosbug.com/480
BUG=http://crosbug.com/1136
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39717
Patch Set 1 #
Total comments: 34
Patch Set 2 : response to first round of comments #
Total comments: 16
Patch Set 3 : 2nd round of comments #Patch Set 4 : Move clone to its own function and add 'files' population and info printing to AttemptRebase #
Total comments: 12
Patch Set 5 : 4th round of comments #
Total comments: 6
Patch Set 6 : Make _AttemptRebase args bools #
Total comments: 6
Patch Set 7 : 6th round of comments #
Total comments: 1
Patch Set 8 : final fixes? #Patch Set 9 : presubmit tests pass #Patch Set 10 : add tests #Patch Set 11 : gclient_scm_tests pass #
Total comments: 2
Patch Set 12 : final set #Messages
Total messages: 33 (0 generated)
This is currently failing the GitWrapperTestCase.testUpdateConflict unit test because we now prompt for a user response when we find a rebase situation. Anyone know how to handle that in the test code?
If only you were using merge, it would leave you with nice conflict markers in the file! :P
On 2010/02/01 19:17:09, Evan Martin wrote: > If only you were using merge, it would leave you with nice conflict markers in > the file! :P rebase leaves you with conflict markers too...
On 2010/02/01 19:09:58, Nasser Grainawi wrote: > This is currently failing the GitWrapperTestCase.testUpdateConflict unit test > because we now prompt for a user response when we find a rebase situation. > > Anyone know how to handle that in the test code? You can write to stdin in the mocking part of the unit tests, it is already mocked. http://codereview.chromium.org/559003/diff/1/2 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/1/2#newcode189 gclient_scm.py:189: revType = "branch" This file is using var_name style. http://codereview.chromium.org/559003/diff/1/2#newcode201 gclient_scm.py:201: new_branch = revision.replace('heads', 'remotes/origin') Always hardcode to origin? http://codereview.chromium.org/559003/diff/1/2#newcode202 gclient_scm.py:202: elif revision.find('refs/tags/') is 0: elif not ... http://codereview.chromium.org/559003/diff/1/2#newcode211 gclient_scm.py:211: self._Run(['checkout', '-b', short_rev, new_branch]) Don't you want redirect_stdout=False ? http://codereview.chromium.org/559003/diff/1/2#newcode214 gclient_scm.py:214: print "" why? http://codereview.chromium.org/559003/diff/1/2#newcode256 gclient_scm.py:256: elif upstream_branch.find('refs/remotes') is 0: elif not ... http://codereview.chromium.org/559003/diff/1/2#newcode262 gclient_scm.py:262: remote_output,remote_err = self.Capture(['remote'] + verbose + ['update'], remote_output, remote_err http://codereview.chromium.org/559003/diff/1/2#newcode263 gclient_scm.py:263: self.checkout_path, print_error=False) weird alignment. Why not use print_error=options.verbose ? http://codereview.chromium.org/559003/diff/1/2#newcode269 gclient_scm.py:269: # this is a big hammer, debatable if it should even be here... nit: We try to capitalize the first letter of each sentence. http://codereview.chromium.org/559003/diff/1/2#newcode329 gclient_scm.py:329: merge_output,merge_err = self.Capture(['merge', '--ff-only', upstream_branch], 80 cols http://codereview.chromium.org/559003/diff/1/2#newcode344 gclient_scm.py:344: yes = re.compile(r'yes|y', re.I) No need to precompile the regexp. Python's re module keeps a cache of last compiled regexp, and you are recompiling it *inside* the loop anyway. http://codereview.chromium.org/559003/diff/1/2#newcode348 gclient_scm.py:348: if yes.match(action): So for example: if re.match(r'yes|y', action, re.I): http://codereview.chromium.org/559003/diff/1/2#newcode349 gclient_scm.py:349: files = self._Run(['diff', upstream_branch, '--name-only']).split() 80 cols http://codereview.chromium.org/559003/diff/1/2#newcode389 gclient_scm.py:389: print "Checked out revision %s" % self.revinfo(options, (), None) I think overall this function is too long and should be split into helper functions if possible, I have a hard time following all the control flows. http://codereview.chromium.org/559003/diff/1/2#newcode435 gclient_scm.py:435: def _AttemptRebase(self, upstream, verbose=[], onto=[], newbase=[], branch=[]): 80 cols Don't use list as default value, this is a python specific issue where the same "unnamed global variable" will be reused across function calls, which is Real Bad (tm). Use None instead. http://codereview.chromium.org/559003/diff/1/2#newcode445 gclient_scm.py:445: if (onto != "--onto" and onto) or (not newbase and onto) or (not onto and newbase): 80 cols http://codereview.chromium.org/559003/diff/1/2#newcode470 gclient_scm.py:470: rebase_output,rebase_err = self.Capture(rebase_cmd, self.checkout_path) 80 cols
(I would suggest adding a flag like --no-prompts that just leaves your repo in an inconsistent state for the sake of the test.)
On 2010/02/01 19:31:06, Evan Martin wrote: > (I would suggest adding a flag like --no-prompts that just leaves your repo in > an inconsistent state for the sake of the test.) Urg, sorry, listen to M-A not me. :)
http://codereview.chromium.org/559003/diff/1/2 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/1/2#newcode189 gclient_scm.py:189: revType = "branch" On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > This file is using var_name style. Done. http://codereview.chromium.org/559003/diff/1/2#newcode201 gclient_scm.py:201: new_branch = revision.replace('heads', 'remotes/origin') On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > Always hardcode to origin? yes, we didn't specify a '-o <name>' option to the clone, so 'origin' will always be the remote in that particular block. http://codereview.chromium.org/559003/diff/1/2#newcode202 gclient_scm.py:202: elif revision.find('refs/tags/') is 0: On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > elif not ... isn't that unnecessarily obfuscating the code? find() is returning an index, not a boolean... http://codereview.chromium.org/559003/diff/1/2#newcode211 gclient_scm.py:211: self._Run(['checkout', '-b', short_rev, new_branch]) On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > Don't you want redirect_stdout=False ? Done. http://codereview.chromium.org/559003/diff/1/2#newcode214 gclient_scm.py:214: print "" On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > why? Mmm, yeah, this should get removed. I need to fix gclient.py still and add some sort of a progress meter and some spacing between deps, otherwise a non-verbose gclient sync is awfully quiet with this patch. http://codereview.chromium.org/559003/diff/1/2#newcode256 gclient_scm.py:256: elif upstream_branch.find('refs/remotes') is 0: On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > elif not ... same comment about .find() as above http://codereview.chromium.org/559003/diff/1/2#newcode262 gclient_scm.py:262: remote_output,remote_err = self.Capture(['remote'] + verbose + ['update'], On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > remote_output, remote_err Done. http://codereview.chromium.org/559003/diff/1/2#newcode263 gclient_scm.py:263: self.checkout_path, print_error=False) On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > weird alignment. Why not use print_error=options.verbose ? We want to be able to print both stdout and stderr (in that order) when options.verbose is True. http://codereview.chromium.org/559003/diff/1/2#newcode269 gclient_scm.py:269: # this is a big hammer, debatable if it should even be here... On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > nit: We try to capitalize the first letter of each sentence. Done (except for where the first word is a var name) http://codereview.chromium.org/559003/diff/1/2#newcode329 gclient_scm.py:329: merge_output,merge_err = self.Capture(['merge', '--ff-only', upstream_branch], On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > 80 cols Done. http://codereview.chromium.org/559003/diff/1/2#newcode344 gclient_scm.py:344: yes = re.compile(r'yes|y', re.I) On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > No need to precompile the regexp. Python's re module keeps a cache of last > compiled regexp, and you are recompiling it *inside* the loop anyway. Done. http://codereview.chromium.org/559003/diff/1/2#newcode348 gclient_scm.py:348: if yes.match(action): On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > So for example: > if re.match(r'yes|y', action, re.I): Done. http://codereview.chromium.org/559003/diff/1/2#newcode349 gclient_scm.py:349: files = self._Run(['diff', upstream_branch, '--name-only']).split() On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > 80 cols Done. http://codereview.chromium.org/559003/diff/1/2#newcode389 gclient_scm.py:389: print "Checked out revision %s" % self.revinfo(options, (), None) On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > I think overall this function is too long and should be split into helper > functions if possible, I have a hard time following all the control flows. Yeah I agree, I wanted to get the functionality up here for review before I tried to optimize it though. If everyone generally agrees with what this is doing I'll try to break up this function into smaller ones. http://codereview.chromium.org/559003/diff/1/2#newcode435 gclient_scm.py:435: def _AttemptRebase(self, upstream, verbose=[], onto=[], newbase=[], branch=[]): On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > 80 cols > Don't use list as default value, this is a python specific issue where the same > "unnamed global variable" will be reused across function calls, which is Real > Bad (tm). Use None instead. Done. http://codereview.chromium.org/559003/diff/1/2#newcode445 gclient_scm.py:445: if (onto != "--onto" and onto) or (not newbase and onto) or (not onto and newbase): On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > 80 cols Done. http://codereview.chromium.org/559003/diff/1/2#newcode470 gclient_scm.py:470: rebase_output,rebase_err = self.Capture(rebase_cmd, self.checkout_path) On 2010/02/01 19:26:15, Marc-Antoine Ruel wrote: > 80 cols Done.
http://codereview.chromium.org/559003/diff/6001/6002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/6001/6002#newcode330 gclient_scm.py:330: upstream_branch], align +1 to [ http://codereview.chromium.org/559003/diff/6001/6002#newcode341 gclient_scm.py:341: "rebase? (y)es / (n)o / (s)kip : ")) maybe no should be renamed (q)uit? Otherwise the difference between no and skip isn't obvious to me. Maybe it's just me. http://codereview.chromium.org/559003/diff/6001/6002#newcode433 gclient_scm.py:433: def _AttemptRebase(self, upstream, verbose=None, onto=None, newbase=None, \ don't use \ it's unneeded http://codereview.chromium.org/559003/diff/6001/6002#newcode434 gclient_scm.py:434: branch=None): align branch to ( on the other line http://codereview.chromium.org/559003/diff/6001/6002#newcode435 gclient_scm.py:435: if verbose and (verbose != "--verbose" or verbose != "-v"): if verbose not in ('-v', '--verbose') and verbose But this seems a bit moot. verbose should just be True/False. http://codereview.chromium.org/559003/diff/6001/6002#newcode438 gclient_scm.py:438: if (onto != "--onto" and onto) or (not newbase and onto) or \ Don't use \, use parenthesis instead. http://codereview.chromium.org/559003/diff/6001/6002#newcode444 gclient_scm.py:444: for arg in [verbose, onto, newbase, upstream, branch]: This looks like you use just use a "args" parameter with whatever the caller wants. http://codereview.chromium.org/559003/diff/6001/6002#newcode453 gclient_scm.py:453: print_error=False) alignment
http://codereview.chromium.org/559003/diff/6001/6002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/6001/6002#newcode330 gclient_scm.py:330: upstream_branch], On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > align +1 to [ Done. http://codereview.chromium.org/559003/diff/6001/6002#newcode341 gclient_scm.py:341: "rebase? (y)es / (n)o / (s)kip : ")) On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > maybe no should be renamed (q)uit? Otherwise the difference between no and skip > isn't obvious to me. Maybe it's just me. Done. http://codereview.chromium.org/559003/diff/6001/6002#newcode433 gclient_scm.py:433: def _AttemptRebase(self, upstream, verbose=None, onto=None, newbase=None, \ On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > don't use \ it's unneeded Done. http://codereview.chromium.org/559003/diff/6001/6002#newcode434 gclient_scm.py:434: branch=None): On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > align branch to ( on the other line Done. http://codereview.chromium.org/559003/diff/6001/6002#newcode435 gclient_scm.py:435: if verbose and (verbose != "--verbose" or verbose != "-v"): On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > if verbose not in ('-v', '--verbose') and verbose > > But this seems a bit moot. verbose should just be True/False. options.verbose is True/False, but since I use the var in list concatenation I thought it would be easier to keep the value I'd already set in the calling function. However, I did change this to: if verbose and ''.join(verbose) not in '--verbose': http://codereview.chromium.org/559003/diff/6001/6002#newcode438 gclient_scm.py:438: if (onto != "--onto" and onto) or (not newbase and onto) or \ On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > Don't use \, use parenthesis instead. Done. http://codereview.chromium.org/559003/diff/6001/6002#newcode444 gclient_scm.py:444: for arg in [verbose, onto, newbase, upstream, branch]: On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > This looks like you use just use a "args" parameter with whatever the caller > wants. Except that I want to specifically validate some of those args to keep from doing something silly/dangerous with a rebase operation. Is there a better way of doing that? http://codereview.chromium.org/559003/diff/6001/6002#newcode453 gclient_scm.py:453: print_error=False) On 2010/02/02 16:23:39, Marc-Antoine Ruel wrote: > alignment Done.
Re-org'ed everything but case 3. http://codereview.chromium.org/559003/diff/9001/9002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/9001/9002#newcode428 gclient_scm.py:428: def _AttemptRebase(self, upstream, files, verbose=None, onto=None, newbase=None, oops, I'll fix this in the next one.
Still some style nits. I'd like someone else to verify the git usage correctness. http://codereview.chromium.org/559003/diff/9001/9002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/9001/9002#newcode241 gclient_scm.py:241: if not upstream_branch or upstream_branch.find('refs/remotes') is -1: would "if not upstream_branch or not upstream_branch.startswith('refs/remotes')" work? http://codereview.chromium.org/559003/diff/9001/9002#newcode245 gclient_scm.py:245: elif upstream_branch.find('refs/remotes') is 0: elif upstream_branch.startswith('refs/remotes'): http://codereview.chromium.org/559003/diff/9001/9002#newcode251 gclient_scm.py:251: remote_output, remote_err = self.Capture(['remote'] + verbose + ['update'], Won't git-svn remotes ask for svn credentials? http://codereview.chromium.org/559003/diff/9001/9002#newcode286 gclient_scm.py:286: raise gclient_utils.Error("Switching upstream branch from %s to %s\n" % I think you should create a named string, it'd make that more readable. Also we put the + at the end of the line and not at the beginning. http://codereview.chromium.org/559003/diff/9001/9002#newcode428 gclient_scm.py:428: def _AttemptRebase(self, upstream, files, verbose=None, onto=None, newbase=None, Most of our code is 80 cols enforced, you may want to configure your editor to warn you. It may be annoying if you edit a lot of non-80-cols source files. And I still think most parameters should just be concatenated in a "args" parameter.
Any git users available to test this out and/or verify the usage is correct? http://codereview.chromium.org/559003/diff/9001/9002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/9001/9002#newcode241 gclient_scm.py:241: if not upstream_branch or upstream_branch.find('refs/remotes') is -1: On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > would "if not upstream_branch or not upstream_branch.startswith('refs/remotes')" > work? Done. http://codereview.chromium.org/559003/diff/9001/9002#newcode245 gclient_scm.py:245: elif upstream_branch.find('refs/remotes') is 0: On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > elif upstream_branch.startswith('refs/remotes'): Done. http://codereview.chromium.org/559003/diff/9001/9002#newcode251 gclient_scm.py:251: remote_output, remote_err = self.Capture(['remote'] + verbose + ['update'], On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > Won't git-svn remotes ask for svn credentials? Hmm, quite possibly. I'll test that out tomorrow. http://codereview.chromium.org/559003/diff/9001/9002#newcode286 gclient_scm.py:286: raise gclient_utils.Error("Switching upstream branch from %s to %s\n" % On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > I think you should create a named string, it'd make that more readable. Also we > put the + at the end of the line and not at the beginning. Done. http://codereview.chromium.org/559003/diff/9001/9002#newcode428 gclient_scm.py:428: def _AttemptRebase(self, upstream, files, verbose=None, onto=None, newbase=None, On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > Most of our code is 80 cols enforced, you may want to configure your editor to > warn you. It may be annoying if you edit a lot of non-80-cols source files. > I have some highlighting going now, it's nice. > And I still think most parameters should just be concatenated in a "args" > parameter. Still not sure I understand why. If we know exactly what can (should) be going in, why would we make our interface more confusing by not specifying exactly what's needed for the function?
http://codereview.chromium.org/559003/diff/9001/9002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/9001/9002#newcode251 gclient_scm.py:251: remote_output, remote_err = self.Capture(['remote'] + verbose + ['update'], On 2010/02/03 00:01:56, Nasser Grainawi wrote: > On 2010/02/02 21:53:46, Marc-Antoine Ruel wrote: > > Won't git-svn remotes ask for svn credentials? > > Hmm, quite possibly. I'll test that out tomorrow. > I think for now this is a non-issue. Gclient doesn't actually support cloning a git-svn repo (it supports cloning a git repo created using git-svn, but in that case there's no svn auth).
On 2010/02/03 00:01:56, Nasser Grainawi wrote: > Still not sure I understand why. If we know exactly what can (should) be going > in, why would we make our interface more confusing by not specifying exactly > what's needed for the function? You have less checks to do in the function if you just do a True/False check instead of enforcing the caller to use specific list constructions. http://codereview.chromium.org/559003/diff/13001/13002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/13001/13002#newcode251 gclient_scm.py:251: # TODO Check that git-svn doesn't try to prompt for a password here Keep the todo aligned with the rest http://codereview.chromium.org/559003/diff/13001/13002#newcode430 gclient_scm.py:430: return unnecessary http://codereview.chromium.org/559003/diff/13001/13002#newcode435 gclient_scm.py:435: if verbose and ''.join(verbose) not in '--verbose': you could also just use an assert assert not verbose or verbose == ['--verbose'] I still think forcing a list of string instead of just a bool is complicating the interface for no gain. e.g. the caller could just do verbose=options.verbose and the callee could just if verbose: args.append('--verbose') http://codereview.chromium.org/559003/diff/13001/13002#newcode459 gclient_scm.py:459: if type(arg) is type(list()): if isinstance(arg, (list, tuple)): but as I said earlier, that is complicating the interface for string that should be hardcoded in the callee anyway.
Ok, took a stab at the args as bools thing. Let me know where it needs work still. http://codereview.chromium.org/559003/diff/13001/13002 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/13001/13002#newcode251 gclient_scm.py:251: # TODO Check that git-svn doesn't try to prompt for a password here On 2010/02/03 21:00:34, Marc-Antoine Ruel wrote: > Keep the todo aligned with the rest Yeah, intended to remove it before uploading, but a good thing to know in the future. http://codereview.chromium.org/559003/diff/13001/13002#newcode430 gclient_scm.py:430: return On 2010/02/03 21:00:34, Marc-Antoine Ruel wrote: > unnecessary Done.
lgtm with 3 nits. Still waiting for someone else to make sure the workflow is fine. ping. ping. ping. :) http://codereview.chromium.org/559003/diff/15001/7004 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/15001/7004#newcode251 gclient_scm.py:251: # TODO Check that git-svn doesn't try to prompt for a password here ? http://codereview.chromium.org/559003/diff/15001/7004#newcode418 gclient_scm.py:418: self._Run(['clone'] + verbose + [url, self.checkout_path], That works when verbose=None? I'd be surprised. http://codereview.chromium.org/559003/diff/15001/7004#newcode456 gclient_scm.py:456: rebase_cmd.append('--onto') you can use rebase_cmd.extend(['--onto', newbase]) if you want to save one line.
I don't know why but chromium-reviews@googlegroups.com keeps on being added.
Getting there... http://codereview.chromium.org/559003/diff/15001/7004 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/15001/7004#newcode251 gclient_scm.py:251: # TODO Check that git-svn doesn't try to prompt for a password here On 2010/02/03 21:32:37, Marc-Antoine Ruel wrote: > ? Wow, I fail. http://codereview.chromium.org/559003/diff/15001/7004#newcode418 gclient_scm.py:418: self._Run(['clone'] + verbose + [url, self.checkout_path], On 2010/02/03 21:32:37, Marc-Antoine Ruel wrote: > That works when verbose=None? I'd be surprised. Probably not. I'll change this to be false like I did for AttemptRebase. http://codereview.chromium.org/559003/diff/15001/7004#newcode456 gclient_scm.py:456: rebase_cmd.append('--onto') On 2010/02/03 21:32:37, Marc-Antoine Ruel wrote: > you can use rebase_cmd.extend(['--onto', newbase]) if you want to save one line. Done.
Please harass either Evan or Mandeep for the logic. http://codereview.chromium.org/559003/diff/15003/7006 File gclient_scm.py (right): http://codereview.chromium.org/559003/diff/15003/7006#newcode442 gclient_scm.py:442: def _AttemptRebase(self, upstream, files, verbose=False, onto=False, Remove onto, it's unnecessary from the current callers and the way it's used in the function. onto always equals (newbase != None)
lgtm
LGTM. Thanks! But could you please add test cases for the four cases.
PTAL
lgtm with a style nit. Congrats! :) http://codereview.chromium.org/559003/diff/20004/21005 File tests/gclient_scm_test.py (right): http://codereview.chromium.org/559003/diff/20004/21005#newcode532 tests/gclient_scm_test.py:532: exception = \ we use this form instead: exception = ( "error: Your local changes to 'b' would be overwritten by merge. Aborting." "\n" "Please, commit your changes or stash them before you can merge.\n") (you can also reduce to two lines or split as you want, it's just we don't use \)
committing http://codereview.chromium.org/559003/diff/20004/21005 File tests/gclient_scm_test.py (right): http://codereview.chromium.org/559003/diff/20004/21005#newcode532 tests/gclient_scm_test.py:532: exception = \ On 2010/02/23 14:26:15, Marc-Antoine Ruel wrote: > we use this form instead: > exception = ( > "error: Your local changes to 'b' would be overwritten by merge. Aborting." > "\n" > "Please, commit your changes or stash them before you can merge.\n") > > (you can also reduce to two lines or split as you want, it's just we don't use > \) Doh! I knew that. Oddly though, quite a bit of this file does use \. |