git support

This patch builds on the abstraction added in issue 2589 to support using Git
for upload.py.

[Evan Martin]

This patch includes the code from
  http://codereview.appspot.com/2589
so you should review that one first, and then I can regenerate this into a
smaller patch.

But I thought you might like to see how it would be used.

(Once these patches are applied, I can use the local_base feature and some git
magic to provide diffs for separate reviews as I go!)

[GvR]

[Just changing reviewers]

[ondrej.certik]

The patch looks ok to me. +1

[GvR]

Just a few nits while I'm awaiting the clean-up of issue 2589...

http://codereview.appspot.com/2602/diff/65/47
File static/upload.py (right):

http://codereview.appspot.com/2602/diff/65/47#newcode20
Line 20: With Subversion, diff_options are passed to "svn diff".
Insert a blank line above here.

http://codereview.appspot.com/2602/diff/65/47#newcode531
Line 531: data = RunShell(cmd, self.diff_args)
This refactoring could also be added to your issue 2589.

http://codereview.appspot.com/2602/diff/65/47#newcode612
Line 612: class GitVCS:
Style issues:

- the class needs a docstring

- no blank line before the class docstring, one blank line after it

- single blank line between methods

- double blank lines between classes and/or top-level functions

http://codereview.appspot.com/2602/diff/65/47#newcode669
Line 669: 
Two blank lines is enough. :-)

http://codereview.appspot.com/2602/diff/65/47#newcode672
Line 672: """Guess what version control system we're using, and return a new
instance
Style nit (see PEP 8): please make the docstring have a one-line summary, then a
blank line, and then more text.

http://codereview.appspot.com/2602/diff/65/47#newcode678
Line 678: if os.path.exists('.svn'):
Double quotes to be consistent.

http://codereview.appspot.com/2602/diff/65/47#newcode681
Line 681: # Git has a command to test if you're in a git tree.
Add a comment explaining that if git isn't installed this will also silently
fail.

[GvR]

Can someone who uses git do a thorough test of this?

[joachim]

Nice piece of work.  Let me give a few comments from someone who comes from a
git perspective and does not have a real clue as to the inner workings of
rietveld. :-/

http://codereview.appspot.com/2602/diff/74/75
File static/upload.py (right):

http://codereview.appspot.com/2602/diff/74/75#newcode692
Line 692: self.diff_from = "master"
"master" is not a reserved name in git.  It seems like HEAD^ would be a very
viable alternative.  Alternatively, require exactly one argument?  Alternatively
yet, if zero arguments are given, should the diff be produced "from" the git
index, just like git diff would?

More broadly, this class requires that the uploaded diff be visible in the
working directory (well, the "to" part of that diff).  This might be an
annoyance in some cases:  Maybe you want to upload the diff between, say,
HEAD^^^ and HEAD^^, without first having to checkout (reset --hard to) HEAD^^. 
(In particular, this might screw up the mtime of various files, which might
interact badly with your build system.)  So it seems like you might want for
both ends of the diff to be any of:
- some named commit;
- the index; or
- the state of the working directory.

For simplicity's sake, then, I would suggest that the "to" part of the diff
always be taken from the index (i.e., add "--cached" to all your invocations of
git-diff), and that the "from" part be HEAD, unless specified in this one
optional argument here.  What do you think?

http://codereview.appspot.com/2602/diff/74/75#newcode699
Line 699: gitdiff = RunShell("git diff", [self.diff_from])
It would seem to be more robust if you used GIT_EXTERNAL_DIFF as described in
git(7) rather than parsing the diff that git itself produces.  I'm not sure,
however, how well that would tie in with the interface here.

http://codereview.appspot.com/2602/diff/74/75#newcode718
Line 718: 
You should probably have a somewhat more elaborate version of
CheckForUnknownFiles().  If you go with the current design, you should check
that "git diff --cached" is empty.  If you go with my proposed design above,
GetUnknownFiles() would always return an empty set of files, and
CheckForUnknownFiles() should ask the user if they are sure of what they are
doing if "git diff" (without the --cached) is non-empty.

http://codereview.appspot.com/2602/diff/74/75#newcode720
Line 720: status = RunShell("git diff --abbrev=40 --raw", [self.diff_from,
filename])
You probably want an additional "--" between the diff_from and the filename.

Also, if I understand the intent of this function correctly, don't you need the
-M (and, possibly, the -C and/or the --find-copies-harder) option?  If so, then,
by restricting the diff to a single file, though, you would never see a rename. 
It seems like you want the full (raw) diff, from which you would then grep for
the correct filename.

[Evan Martin]

Updated to work again.  This diff is now based on the code in issue 2949.
http://codereview.appspot.com/2949

[Andi Albrecht]

Hi Evan,

here are some first comments but I had not time yet to test it... ;-)

http://codereview.appspot.com/2602/diff/108/127
File static/upload.py (right):

http://codereview.appspot.com/2602/diff/108/127#newcode17
Line 17: """Tool for uploading diffs to the codereview app.
I think we should make clear that it uploads diffs between a working copy and a
repository (and not diffs produced by the diff command).

http://codereview.appspot.com/2602/diff/108/127#newcode22
Line 22: With Git, diff_options are passed to "git diff".
I'd unify this message to something like "diff_options are passed to the diff
command of the version control system."

http://codereview.appspot.com/2602/diff/108/127#newcode732
Line 732: match = re.match(r"diff --git a/(.*) b/.*", line)
We should wrap the regular expression in '^...$' to avoid matching the pattern
somewhere else, e.g. in comments.

http://codereview.appspot.com/2602/diff/108/127#newcode741
Line 741: match = re.match(r"index (\w+)\.\.", line)
'^...$' here too.

http://codereview.appspot.com/2602/diff/108/127#newcode745
Line 745: if not filecount:
Do we need to run an additional sanity check here? If the above first regex
matches, but the second not, we have an index line in svndiff but no
corresponding entry in base_hashes.
I can't tell if this is a real problem... I was just thinking about diffs that
are somehow broken or if the pattern for the git-style index line doesn't match
for some reason.

http://codereview.appspot.com/2602/diff/108/127#newcode848
Line 848: if os.system("git rev-parse --is-inside-work-tree >/dev/null 2>&1") ==
0:
What about Windows users? I'm not sure how the syntax on a Windows shell looks
like, but I think they have no /dev/null ;-)

[Andi Albrecht]

LGTM

I'm not very familiar with git, so I did only some basic tests, but without any
problems.

Guido, do you have any concerns about adding git support now?

http://codereview.appspot.com/2602/diff/114/131
File static/upload.py (right):

http://codereview.appspot.com/2602/diff/114/131#newcode846
Line 846: error if we can't figure it out.
Thanks for adding some more docstrings and comments!

http://codereview.appspot.com/2602/diff/114/131#newcode891
Line 891: # For future use. SubversionVCS.GuessBase() already checks this
I think we can enable local_base here now for all other VCS. In my current
Bazaar patch this part is:

if not base and not options.local_base:
  options.local_base = True
  logging.info("Enabled upload of base file")

[Andi Albrecht]

Committed r266 with a slight modification: local_base is enabled by default for
other VCS than Subversion.

[GvR]

Congrats, Evan!  Can you close this issue now?