Disallow applying patches when git tree is dirty

Disallow applying patches when git tree is dirty

This allows proper clean-up after failing to apply a patch. Therefore,
if "git cl diff" ends up having conflict, it could be cleaned up.

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

[wychen, 2015-02-02 15:14:46]

wychen@chromium.org changed reviewers:
+ iannucci+depot_tools@chromium.org, sbc@chromium.org

[wychen, 2015-02-02 15:14:46]

PTAL

[iannucci, 2015-02-02 15:49:41]

iannucci@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2015-02-02 15:49:42]

This seems reasonable to me, but I don't understand the diff change.

https://codereview.chromium.org/896453002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/896453002/diff/1/git_cl.py#newcode2777
git_cl.py:2777: return 1
Hm... why is this necessary?

[wychen, 2015-02-02 17:09:26]

Added some comments to explain why this is necessary.

https://codereview.chromium.org/896453002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/896453002/diff/1/git_cl.py#newcode2777
git_cl.py:2777: return 1
On 2015/02/02 15:49:42, iannucci wrote:
> Hm... why is this necessary?

This is basically a safety measure. If the user have local uncommitted changes
(dirty tree), these changes might get destroyed by "reset --hard" if the
patching step has merging conflict.

I thought about automatically "git stash" first so the user don't have to do
anything due to this change. However, if the user has partially staged changes,
it could also be slightly harmful because the user has to do "add -p" again.

If some local uncommitted changes are staged, the behavior of "git cl diff"
would be unexpected (staged changes count towards last upload).

I should probably add some comments in the code to explain this.

[Sam Clegg, 2015-02-02 17:43:18]

https://codereview.chromium.org/896453002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/896453002/diff/20001/git_cl.py#newcode2470
git_cl.py:2470: RunGit(['reset', '--hard'])
I find this a little scary. Perhaps skip this and update the line below to be
"Failed to apply the patch (use git reset --hard to cleanup remnants)'.  I'm not
adamant about this I just have a fear of scripts that do "reset --hard" at any
point.  If you are sure this is not going to burn anyone I'm OK with leaving it
in as I think it is technically correct.

If you do choose to leave then in then perhaps assert(!is_dirty_git_tree) at
start of PatchIssue in case anyone else tries to call it later?

[wychen, 2015-02-03 02:33:45]

Added assert(!is_dirty_git_tree) and some comments. PTAL.

https://codereview.chromium.org/896453002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/896453002/diff/20001/git_cl.py#newcode2470
git_cl.py:2470: RunGit(['reset', '--hard'])
On 2015/02/02 17:43:18, Sam Clegg wrote:
> I find this a little scary. Perhaps skip this and update the line below to be
> "Failed to apply the patch (use git reset --hard to cleanup remnants)'.  I'm
not
> adamant about this I just have a fear of scripts that do "reset --hard" at any
> point.  If you are sure this is not going to burn anyone I'm OK with leaving
it
> in as I think it is technically correct.
> 
> If you do choose to leave then in then perhaps assert(!is_dirty_git_tree) at
> start of PatchIssue in case anyone else tries to call it later?

Done.

[Sam Clegg, 2015-02-03 02:41:30]

On 2015/02/03 02:33:45, Wei-Yin Chen wrote:
> Added assert(!is_dirty_git_tree) and some comments. PTAL.
> 
> https://codereview.chromium.org/896453002/diff/20001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/896453002/diff/20001/git_cl.py#newcode2470
> git_cl.py:2470: RunGit(['reset', '--hard'])
> On 2015/02/02 17:43:18, Sam Clegg wrote:
> > I find this a little scary. Perhaps skip this and update the line below to
be
> > "Failed to apply the patch (use git reset --hard to cleanup remnants)'.  I'm
> not
> > adamant about this I just have a fear of scripts that do "reset --hard" at
any
> > point.  If you are sure this is not going to burn anyone I'm OK with leaving
> it
> > in as I think it is technically correct.
> > 
> > If you do choose to leave then in then perhaps assert(!is_dirty_git_tree) at
> > start of PatchIssue in case anyone else tries to call it later?
> 
> Done.

Ok, lgtm.

[wychen, 2015-02-03 05:20:41]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-02-03 05:21:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896453002/40001

[commit-bot: I haz the power, 2015-02-03 05:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 05:24:21]

Presubmit check for 896453002-40001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/git_cl.py

tests/checkout_test.py (37.84s) failed
Cloning into '/tmp/trialLSMOM6/__main__.GitCheckout.testAll/foo'...
done.
Switched to branch 'master'
Already on 'master'
.ECloning into '/tmp/trialLSMOM6/__main__.GitCheckout.testMove/foo'...
done.
.Cloning into '/tmp/trialLSMOM6/__main__.GitCheckout.testProcess/foo'...
done.
....................
======================================================================
ERROR: testException (__main__.GitCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/checkout_test.py", line 347, in setUp
    self.enabled = self.FAKE_REPOS.set_up_git()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 359, in set_up_git
    self.set_up()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 211, in set_up
    self.cleanup_dirt()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 229, in cleanup_dirt
    if not self.tear_down_git():
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 276, in tear_down_git
    wait_for_port_to_free(self.host, self.git_port)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 161, in wait_for_port_to_free
    assert False, '%d is still bound' % port
AssertionError: 20000 is still bound

----------------------------------------------------------------------
Ran 23 tests in 37.536s

FAILED (errors=1)


Presubmit checks took 171.6s to calculate.

[wychen, 2015-02-03 20:03:27]

Hi Robbie, PTAL. Thanks!

[wychen, 2015-02-27 09:35:20]

wychen@chromium.org changed reviewers:
+ dpranke+depot_tools@chromium.org
- iannucci@chromium.org

[wychen, 2015-02-27 09:35:21]

PTAL.

[wychen, 2015-04-03 07:17:07]

Rebased. PTAL. Thanks!

[Sam Clegg, 2015-04-03 16:04:03]

slgtm

[wychen, 2015-04-03 18:12:15]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-04-03 18:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sbc@chromium.org
Link to the patchset: https://codereview.chromium.org/896453002/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2015-04-03 18:12:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896453002/60001

[wychen, 2015-04-03 18:12:37]

The CQ bit was unchecked by wychen@chromium.org

[commit-bot: I haz the power, 2015-04-03 18:14:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-03 18:14:51]

Presubmit check for 896453002-60001 failed and returned exit status 1.

Running presubmit commit checks ...
** unknown exception encountered, please report by visiting
** http://mercurial.selenic.com/wiki/BugTracker
** Python 2.7.6 (default, Mar 22 2014, 22:59:56) [GCC 4.8.2]
** Mercurial Distributed SCM (version 2.8.2)
** Extensions loaded: 
Traceback (most recent call last):
  File "/usr/bin/hg", line 38, in <module>
    mercurial.dispatch.run()
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 28, in run
    sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 69, in
dispatch
    ret = _runcatch(req)
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 133, in
_runcatch
    return _dispatch(req)
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 806, in
_dispatch
    cmdpats, cmdoptions)
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 585, in
runcommand
    ret = _runcommand(ui, options, cmd, d)
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 897, in
_runcommand
    return checkargs()
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 868, in
checkargs
    return cmdfunc()
  File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 803, in
<lambda>
    d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
  File "/usr/lib/python2.7/dist-packages/mercurial/util.py", line 512, in check
    return func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/mercurial/commands.py", line 1286, in
clone
    branch=opts.get('branch'))
  File "/usr/lib/python2.7/dist-packages/mercurial/hg.py", line 369, in clone
    revs = [srcpeer.lookup(r) for r in rev]
  File "/usr/lib/python2.7/dist-packages/mercurial/wireproto.py", line 115, in
plain
    encresref.set(self._submitone(f.func_name, encargsorres))
  File "/usr/lib/python2.7/dist-packages/mercurial/wireproto.py", line 163, in
_submitone
    return self._call(op, **args)
  File "/usr/lib/python2.7/dist-packages/mercurial/httppeer.py", line 173, in
_call
    return fp.read()
  File "/usr/lib/python2.7/dist-packages/mercurial/keepalive.py", line 422, in
read
    s = self._rbuf + self._raw_read(amt)
  File "/usr/lib/python2.7/httplib.py", line 543, in read
    return self._read_chunked(amt)
  File "/usr/lib/python2.7/dist-packages/mercurial/keepalive.py", line 445, in
_read_chunked
    raise httplib.IncompleteRead(value)
httplib.IncompleteRead: IncompleteRead(31 bytes read)
Checking out rietveld...

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/git_cl.py

Failed to checkout rietveld. Do you have mercurial installed?
Command hg clone -q -u 9349cab9a3bb -r 9349cab9a3bb
https://code.google.com/p/rietveld/
/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld
returned non-zero exit status 1

Presubmit checks took 140.0s to calculate.

[Dirk Pranke, 2015-04-03 20:11:56]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-04-03 20:11:56]

lgtm

[wychen, 2015-04-03 20:29:18]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-04-03 20:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896453002/60001

[commit-bot: I haz the power, 2015-04-03 20:32:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-03 20:32:54]

Presubmit check for 896453002-60001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
tests/gclient_smoketest.py (52.55s) failed
Ffatal: reference is not a tree: f3843afd3fed0e8fb53583cddb6bc958e0609cbf
EE..............................................
======================================================================
ERROR: testBlinkDEPSChangeUsingGit (__main__.BlinkDEPSTransitionSmokeTest)
Like testBlinkDEPSChangeUsingGclient, but move the main project using
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1630, in testBlinkDEPSChangeUsingGit
    cwd=self.checkout_path)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
484, in check_call
    check_call_out(args, **kwargs)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
478, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
CalledProcessError: Command git checkout -q
f3843afd3fed0e8fb53583cddb6bc958e0609cbf returned non-zero exit status 128 in
/tmp/trialsBnefm/__main__.BlinkDEPSTransitionSmokeTest.testBlinkDEPSChangeUsingGit/src

======================================================================
ERROR: testBlinkLocalBranchesArePreserved
(__main__.BlinkDEPSTransitionSmokeTest)
Checks that the state of local git branches are effectively preserved
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1656, in
testBlinkLocalBranchesArePreserved
    self.CheckStatusPreMergePoint()
  File "tests/gclient_smoketest.py", line 1558, in CheckStatusPreMergePoint
    self.blink), self.blink_git_url)
  File "/b/infra_internal/commit_queue/workdir/tools/depot_tools/scm.py", line
121, in Capture
    cwd=cwd, stderr=subprocess2.PIPE, env=env, **kwargs)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
515, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
475, in check_call_out
    out, returncode = communicate(args, **kwargs)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
449, in communicate
    proc = Popen(args, **kwargs)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
253, in __init__
    % (str(e), kwargs.get('cwd'), args[0]))
OSError: Execution failed with error: [Errno 2] No such file or directory:
'/tmp/trialsBnefm/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit'.
Check that
/tmp/trialsBnefm/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit
or git exist and have execution permission.

======================================================================
FAIL: testBlinkDEPSChangeUsingGclient (__main__.BlinkDEPSTransitionSmokeTest)
Checks that {src,blink} repos are consistent when syncing going back and
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1601, in
testBlinkDEPSChangeUsingGclient
    self.assertEqual(res[2], 0, 'DEPS change sync failed.')
AssertionError: DEPS change sync failed.

----------------------------------------------------------------------
Ran 49 tests in 52.306s

FAILED (failures=1, errors=2)


Presubmit checks took 194.6s to calculate.

[wychen, 2015-04-03 20:59:54]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-04-03 21:01:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896453002/60001

[commit-bot: I haz the power, 2015-04-03 21:04:52]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294679

[yoichio, 2015-04-27 07:52:30]

yoichio@chromium.org changed reviewers:
+ yoichio@chromium.org

[yoichio, 2015-04-27 07:52:31]

https://codereview.chromium.org/896453002/diff/60001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/896453002/diff/60001/git_cl.py#newcode2605
git_cl.py:2605: RunGit(['reset', '--hard'])
This resets working tree. Why?
I prefer the state reset because we can edit conflicts in local.

[wychen, 2015-04-27 17:45:16]

On 2015/04/27 07:52:31, yoichio wrote:
> https://codereview.chromium.org/896453002/diff/60001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/896453002/diff/60001/git_cl.py#newcode2605
> git_cl.py:2605: RunGit(['reset', '--hard'])
> This resets working tree. Why?
> I prefer the state reset because we can edit conflicts in local.

Thanks for reporting this bug. I already had a patch in progress here:
https://codereview.chromium.org/1091283004/