This patch includes the code from http://codereview.appspot.com/2589 so you should review that one first, and ...
15 years, 8 months ago
(2008-07-18 06:14:30 UTC)
#1
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!)
Nice piece of work. Let me give a few comments from someone who comes from ...
15 years, 8 months ago
(2008-07-31 19:37:20 UTC)
#6
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.
Hi Evan, here are some first comments but I had not time yet to test ...
15 years, 7 months ago
(2008-08-19 04:59:03 UTC)
#8
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 ;-)
LGTM I'm not very familiar with git, so I did only some basic tests, but ...
15 years, 7 months ago
(2008-08-19 19:48:34 UTC)
#9
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")
Issue 2602: git support
(Closed)
Created 15 years, 8 months ago by Evan Martin
Modified 14 years, 8 months ago
Reviewers: Andi Albrecht, GvR, ondrej.certik, joachim
Base URL: http://rietveld.googlecode.com/svn/trunk/
Comments: 19