Turn on git diff copy detection for git-cl upload.

Turn on git diff copy detection for git-cl upload.

Enable copy detection for git-cl upload.  This makes
it possible to copy a directory containing many files,
add+commit the new path, and then upload a patch to
Rietveld that shows the files were copied.

In my tests, -C -C was needed to pick up a basic
file copy.  -C was not enough.  I'm not sure why
exactly.  The output generated in the diff looks
like:

  sh$ git diff -C -C HEAD~1..HEAD
  diff --git a/slave/Makefile b/slave2/Makefile
  similarity index 100%
  copy from slave/Makefile
  copy to slave2/Makefile
  ...

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140314

[cmp, 2012-05-21 23:51:20]

FYI bunches of tests failed due to this change, it will need more work before it
can land I suspect.

[M-A Ruel, 2012-05-22 20:26:11]

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py#newcode1116
git_cl.py:1116: '--find-copies-harder'] + args, env=env)
You mean -C -C or -M -M?
There's no meaning to -M -C.

But -C would just work.

[cmp, 2012-05-22 20:31:30]

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py#newcode1116
git_cl.py:1116: '--find-copies-harder'] + args, env=env)
On 2012/05/22 20:26:11, Marc-Antoine Ruel wrote:
> You mean -C -C or -M -M?
> There's no meaning to -M -C.

If -C implies -M, then this should change to -C --find-copies-harder.  In 'git
help diff', -C is described as "Detect copies as well as renames."  Besides
that, there's no other text in -C or -M that says one implies the other.  If
you're confident that -C implies -M, I'll change this to drop -M.

> But -C would just work.

Can you explain what you meant by this?  Does it relate to --find-copies-harder?

[M-A Ruel, 2012-05-22 20:34:44]

On 2012/05/22 20:31:30, cmp wrote:
> https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py
> File git_cl.py (right):
> 
> https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py#newcode1116
> git_cl.py:1116: '--find-copies-harder'] + args, env=env)
> On 2012/05/22 20:26:11, Marc-Antoine Ruel wrote:
> > You mean -C -C or -M -M?
> > There's no meaning to -M -C.
> 
> If -C implies -M, then this should change to -C --find-copies-harder.  In 'git
> help diff', -C is described as "Detect copies as well as renames."  Besides
> that, there's no other text in -C or -M that says one implies the other.  If
> you're confident that -C implies -M, I'll change this to drop -M.
> 
> > But -C would just work.
> 
> Can you explain what you meant by this?  Does it relate to
--find-copies-harder?

$ git help diff | grep -A 3 "  --find-copies-"
(...)
"Giving more than one -C option has the same effect."

$ git help diff | grep -A 1 "  -C"
"Detect copies as well as renames."

So "--find-copies-harder" == "-C -C" and -C is a superset of -M.

[cmp, 2012-05-22 20:43:20]

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/10412027/diff/1/git_cl.py#newcode1116
git_cl.py:1116: '--find-copies-harder'] + args, env=env)
Done.

[M-A Ruel, 2012-05-22 20:48:19]

https://chromiumcodereview.appspot.com/10412027/diff/7001/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/10412027/diff/7001/git_cl.py#newcode1115
git_cl.py:1115: ['git', 'diff', '--no-ext-diff', '--stat', '-C', '-C'] + args,
env=env)
Ok, so now that the intent is clear, I think that '-C' is sufficient. -C -C is
when you are copying file from an non-modified file and has extremely bad
performance characteristics, especially in a tree with over 36k files. I don't
think -C -C should be the default.

So overall, -C would be fine.

[cmp, 2012-05-22 20:54:37]

On 2012/05/22 20:48:19, Marc-Antoine Ruel wrote:
> Ok, so now that the intent is clear, I think that '-C' is sufficient. -C -C is
> when you are copying file from an non-modified file and has extremely bad
> performance characteristics, especially in a tree with over 36k files. I don't
> think -C -C should be the default.

My original comment in my description was:

  In my tests, -C --find-copies-harder was needed
  to pick up a basic file copy. -C was not enough.
  I'm not sure why exactly. The output generated in
  the diff looks like: 

  sh$ git diff -C --find-copies-harder HEAD~1..HEAD 
  diff --git a/slave/Makefile b/slave2/Makefile 
  similarity index 100% 
  copy from slave/Makefile 
  copy to slave2/Makefile 
  ...

So I saw evidence that contradicts your statement that when doing a copy+commit
of a file from a non-modified file, git diff -C was not sufficient to find that
copy.  I agree it's weird that -C is not sufficient, and I agree
--find-copies-harder may have bad perf.  Have you tested -C locally to see it's
sufficient in your environment?  My test is:

  cd build/
  rsync -av slave/ slave2/
  git add slave2
  git commit -a -v -m 'test'
  git diff -C HEAD~1..HEAD
  git diff -C -C HEAD~1..HEAD

[M-A Ruel, 2012-05-22 21:02:01]

On 2012/05/22 20:54:37, cmp wrote:
> On 2012/05/22 20:48:19, Marc-Antoine Ruel wrote:
> > Ok, so now that the intent is clear, I think that '-C' is sufficient. -C -C
is
> > when you are copying file from an non-modified file and has extremely bad
> > performance characteristics, especially in a tree with over 36k files. I
don't
> > think -C -C should be the default.
> 
> My original comment in my description was:
> 
>   In my tests, -C --find-copies-harder was needed
>   to pick up a basic file copy. -C was not enough.
>   I'm not sure why exactly. The output generated in
>   the diff looks like: 
> 
>   sh$ git diff -C --find-copies-harder HEAD~1..HEAD 
>   diff --git a/slave/Makefile b/slave2/Makefile 
>   similarity index 100% 
>   copy from slave/Makefile 
>   copy to slave2/Makefile 
>   ...

I know about that, but a file copy without cleaning up the original file is
surprisingly rare in practice. That's why I'm saying having -C -C the default is
a large cost, roughly 2x longer diff so it initially adds ~50ms for a trivial CL
to an additional ~1s for large CLs to the total diff time.

Also note that for the actual CL, it doesn't change much from a git perspective.

> So I saw evidence that contradicts your statement that when doing a
copy+commit
> of a file from a non-modified file, git diff -C was not sufficient to find
that
> copy.

I didn't say that.

> I agree it's weird that -C is not sufficient, and I agree
> --find-copies-harder may have bad perf.  Have you tested -C locally to see
it's
> sufficient in your environment?  My test is:
> 
>   cd build/
>   rsync -av slave/ slave2/
>   git add slave2
>   git commit -a -v -m 'test'
>   git diff -C HEAD~1..HEAD
>   git diff -C -C HEAD~1..HEAD

What triggered the need this in the first place?

[cmp, 2012-05-22 21:11:45]

On 2012/05/22 21:02:01, Marc-Antoine Ruel wrote:
> I know about that, but a file copy without cleaning up the original file is
> surprisingly rare in practice.

When I re-run the steps I point out, then remove slave, commit, and run 'git
diff -C', I see diff show 'rename from/ rename to' lines.  I see the same thing
with 'git diff -M', though.  So I think copying files to a new location and
removing the old location is equivalent to a rename.  Which makes sense to me,
and I'd expect -M to cover that case already.

> That's why I'm saying having -C -C the default is
> a large cost, roughly 2x longer diff so it initially adds ~50ms for a trivial
CL
> to an additional ~1s for large CLs to the total diff time.

If -C -C isn't necessary, I agree it's useless to add that cost.  I hope we can
find a way for just -C to work for file copies (and not just renames, moves, or
copies+deletes).

> Also note that for the actual CL, it doesn't change much from a git
perspective.

?

> > I agree it's weird that -C is not sufficient, and I agree
> > --find-copies-harder may have bad perf.  Have you tested -C locally to see
> it's
> > sufficient in your environment?  My test is:
> > 
> >   cd build/
> >   rsync -av slave/ slave2/
> >   git add slave2
> >   git commit -a -v -m 'test'
> >   git diff -C HEAD~1..HEAD
> >   git diff -C -C HEAD~1..HEAD
> 
> What triggered the need this in the first place?

I sent you the CL that triggered the need for this over IM.

[M-A Ruel, 2012-05-22 22:36:04]

lgtm

[cmp, 2012-05-30 21:47:20]

fyi, i found that upload.py (not git_cl.py) is responsible for the
real-diff-sending part of the upload.  i've updated this patch to also change
upload.py, and now my uploads appear to do the right thing for rietveld where
i've copied files.

[cmp, 2012-05-30 22:00:01]

updated tests, will land soon

[cmp, 2012-05-30 22:40:35]

I saw that git-svn had some -C/--copy-similarity arg handling and tried using it
in the hopes that whatever it did "just worked" to copy files then apply patches
locally.  In my simple test, this did not work, though, and the resulting commit
looks like I added files.  So in the dcommit case, at least, this will not be
solved (yet?).

Here's the patch I applied to git_cl.py while testing using git-svn dcommit
-C...:

diff --git a/git_cl.py b/git_cl.py
index d0f4f6e..0a4b2a0 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1227,7 +1227,7 @@ def SendUpstream(parser, args, cmd):

   branches = [base_branch, cl.GetBranchRef()]
   if not options.force:
-    subprocess2.call(['git', 'diff', '--stat'] + branches)
+    subprocess2.call(['git', 'diff', '--stat', '-C', '-C'] + branches)
     ask_for_data('About to commit; enter to confirm.')

   # We want to squash all this branch's commits into one commit with the
@@ -1267,7 +1267,7 @@ def SendUpstream(parser, args, cmd):
     else:
       # dcommit the merge branch.
       retcode, output = RunGitWithCode(['svn', 'dcommit',
-                                        '--no-rebase', '--rmdir'])
+                                        '--no-rebase', '--rmdir', '-C0',
'-C0'])
   finally:
     # And then swap back to the original branch and clean up.
     RunGit(['checkout', '-q', cl.GetBranch()])

[M-A Ruel, 2012-05-30 22:58:10]

lgtm

[cmp, 2012-06-01 19:25:35]

fyi, marc-antoine says that things will just work with trybots+commit queue
after this lands

[commit-bot: I haz the power, 2012-06-04 16:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmp@chromium.org/10412027/19001

[commit-bot: I haz the power, 2012-06-04 16:40:23]

Presubmit check for 10412027-19001 failed and returned exit status 1.

warning: code.google.com certificate with fingerprint
95:e2:82:70:b9:09:5d:e0:fb:33:06:86:72:41:94:49:5e:47:ed:e0 not verified (check
hostfingerprints or web.cacerts config setting)
Running presubmit commit checks ...
Checking out rietveld...
Running push-basic.sh
Running save-description-on-failure.sh
Running basic.sh
Running upload-stale.sh
Running upload-local-tracking-branch.sh
Running patch.sh
Running submit-from-new-dir.sh
Running post-dcommit-hook-test.sh
Running hooks.sh
Running abandon.sh
Running upstream.sh

** Presubmit ERRORS **
tests/git_cl_test.py failed!
Command tests/git_cl_test.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/tools/depot_tools
......FFF.FF
======================================================================
FAIL: test_no_reviewer (__main__.TestGitCl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 253, in test_no_reviewer
    [])
  File "tests/git_cl_test.py", line 245, in _run_reviewer_test
    git_cl.main(['upload'] + upload_args)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1548, in main
    return command(parser, argv[1:])
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1122, in CMDupload
    return RietveldUpload(options, args, cl)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1025, in RietveldUpload
    ['--', '-C'])
  File "tests/git_cl_test.py", line 242, in check_upload
    self.assertEquals(self._cmd_line(final_description, reviewers), args)
AssertionError: ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'desc\n\nBUG=\nTEST=\n', '--cc',
'joe@example.com', 'master...'] != ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'desc\n\nBUG=\nTEST=\n', '--cc',
'joe@example.com', 'master...', '--', '-C']

======================================================================
FAIL: test_reviewer_multiple (__main__.TestGitCl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 285, in test_reviewer_multiple
    ['--reviewers', 'reviewer@example.com,another@example.com'])
  File "tests/git_cl_test.py", line 245, in _run_reviewer_test
    git_cl.main(['upload'] + upload_args)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1548, in main
    return command(parser, argv[1:])
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1122, in CMDupload
    return RietveldUpload(options, args, cl)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1025, in RietveldUpload
    ['--', '-C'])
  File "tests/git_cl_test.py", line 242, in check_upload
    self.assertEquals(self._cmd_line(final_description, reviewers), args)
AssertionError: ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'Foo
Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n', '--reviewers',
'reviewer@example.com,another@example.com', '--cc', 'joe@example.com',
'master...'] != ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'Foo
Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n', '--reviewers',
'reviewer@example.com,another@example.com', '--cc', 'joe@example.com',
'master...', '--', '-C']

======================================================================
FAIL: test_reviewer_send_mail (__main__.TestGitCl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 295, in test_reviewer_send_mail
    ['--reviewers', 'reviewer@example.com', '--send_mail'])
  File "tests/git_cl_test.py", line 245, in _run_reviewer_test
    git_cl.main(['upload'] + upload_args)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1548, in main
    return command(parser, argv[1:])
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1122, in CMDupload
    return RietveldUpload(options, args, cl)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1025, in RietveldUpload
    ['--', '-C'])
  File "tests/git_cl_test.py", line 242, in check_upload
    self.assertEquals(self._cmd_line(final_description, reviewers), args)
AssertionError: ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'Foo
Bar\nR=reviewer@example.com\n', '--reviewers', 'reviewer@example.com',
'--send_mail', '--cc', 'joe@example.com', 'master...'] != ['upload',
'--assume_yes', '--server', 'https://codereview.example.com', '--message', 'Foo
Bar\nR=reviewer@example.com\n', '--reviewers', 'reviewer@example.com',
'--send_mail', '--cc', 'joe@example.com', 'master...', '--', '-C']

======================================================================
FAIL: test_reviewer_tbr_overriden (__main__.TestGitCl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 274, in test_reviewer_tbr_overriden
    ['--reviewers', 'reviewer@example.com'])
  File "tests/git_cl_test.py", line 245, in _run_reviewer_test
    git_cl.main(['upload'] + upload_args)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1548, in main
    return command(parser, argv[1:])
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1122, in CMDupload
    return RietveldUpload(options, args, cl)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1025, in RietveldUpload
    ['--', '-C'])
  File "tests/git_cl_test.py", line 242, in check_upload
    self.assertEquals(self._cmd_line(final_description, reviewers), args)
AssertionError: ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'Foo
Bar\nTBR=reviewer@example.com\n', '--reviewers', 'reviewer@example.com', '--cc',
'joe@example.com', 'master...'] != ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message', 'Foo
Bar\nTBR=reviewer@example.com\n', '--reviewers', 'reviewer@example.com', '--cc',
'joe@example.com', 'master...', '--', '-C']

======================================================================
FAIL: test_reviewers_cmd_line (__main__.TestGitCl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 263, in test_reviewers_cmd_line
    ['--reviewers', 'foo@example.com'])
  File "tests/git_cl_test.py", line 245, in _run_reviewer_test
    git_cl.main(['upload'] + upload_args)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1548, in main
    return command(parser, argv[1:])
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1122, in CMDupload
    return RietveldUpload(options, args, cl)
  File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line
1025, in RietveldUpload
    ['--', '-C'])
  File "tests/git_cl_test.py", line 242, in check_upload
    self.assertEquals(self._cmd_line(final_description, reviewers), args)
AssertionError: ['upload', '--assume_yes', '--server',
'https://codereview.example.com', '--message',
'desc\n\nR=foo@example.com\nBUG=\nTEST=\n', '--reviewers', 'foo@example.com',
'--cc', 'joe@example.com', 'master...'] != ['upload', '--assume_yes',
'--server', 'https://codereview.example.com', '--message',
'desc\n\nR=foo@example.com\nBUG=\nTEST=\n', '--reviewers', 'foo@example.com',
'--cc', 'joe@example.com', 'master...', '--', '-C']

----------------------------------------------------------------------
Ran 12 tests in 0.029s

FAILED (failures=5)
Description: 'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'
Description: 'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'

Got exception while uploading -- saving description to
/home/chrome-bot/.git_cl_description_backup


Got exception while uploading -- saving description to
/home/chrome-bot/.git_cl_description_backup


Got exception while uploading -- saving description to
/home/chrome-bot/.git_cl_description_backup


Got exception while uploading -- saving description to
/home/chrome-bot/.git_cl_description_backup


Got exception while uploading -- saving description to
/home/chrome-bot/.git_cl_description_backup



Presubmit checks took 430.8s to calculate.

[commit-bot: I haz the power, 2012-06-04 17:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmp@chromium.org/10412027/24001

[commit-bot: I haz the power, 2012-06-04 17:17:40]

Change committed as 140314