Don't clean up after conflict in "git cl patch"

Don't clean up after conflict in "git cl patch"

After crrev.com/896453002, if "git cl diff" ends up having conflict, it
would be cleaned up. However, if "git cl patch" ends up with conflict,
the user should still be able to manually resolve them.

BUG=438362
TEST=tests/git_cl_test.py

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

[wychen, 2015-04-18 03:23:08]

wychen@chromium.org changed reviewers:
+ dpranke@chromium.org, sbc@chromium.org

[wychen, 2015-04-18 03:23:09]

PTAL

[Sam Clegg, 2015-04-20 23:32:16]

lgtm

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

https://codereview.chromium.org/1091283004/diff/1/git_cl.py#newcode2587
git_cl.py:2587: # not to destroy users' data, make sure the tree is not dirty
here.
How about:

PatchIssue should never be called with a dirty tree.  Its up to the
caller to check this, but just in case we assert here since the
consequences of the caller not checking this could be dire.

https://codereview.chromium.org/1091283004/diff/1/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/1091283004/diff/1/tests/git_cl_test.py#newcod...
tests/git_cl_test.py:117: expected_args, result, exception = top
nit: Do the raise here and avoid the "exception = None" line above?

Actually, how about this:

if len(top) > 2 and top[2]:
    raise top[2]

expected_args, result = top[:2]

[Dirk Pranke, 2015-04-21 20:37:06]

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

[Dirk Pranke, 2015-04-21 20:37:07]

Seems reasonable, but one of the infra people should also sanity-check this.

+hinoka.

[wychen, 2015-04-22 00:23:19]

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

https://codereview.chromium.org/1091283004/diff/1/git_cl.py#newcode2587
git_cl.py:2587: # not to destroy users' data, make sure the tree is not dirty
here.
On 2015/04/20 23:32:16, Sam Clegg wrote:
> How about:
> 
> PatchIssue should never be called with a dirty tree.  Its up to the
> caller to check this, but just in case we assert here since the
> consequences of the caller not checking this could be dire.

Done.

https://codereview.chromium.org/1091283004/diff/1/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/1091283004/diff/1/tests/git_cl_test.py#newcod...
tests/git_cl_test.py:117: expected_args, result, exception = top
On 2015/04/20 23:32:16, Sam Clegg wrote:
> nit: Do the raise here and avoid the "exception = None" line above?
> 
> Actually, how about this:
> 
> if len(top) > 2 and top[2]:
>     raise top[2]
> 
> expected_args, result = top[:2]

Done.

[wychen, 2015-04-22 16:56:24]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[wychen, 2015-04-22 16:56:24]

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/1091283004/#ps20001
(title: "address sbc's comments and fix long lines")

[commit-bot: I haz the power, 2015-04-22 16:56:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091283004/20001

[commit-bot: I haz the power, 2015-04-22 17:00:22]

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

[commit-bot: I haz the power, 2015-04-22 17:00:23]

Dry run: Presubmit check for 1091283004-20001 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
    depot_tools/tests/git_cl_test.py

Presubmit checks took 213.6s to calculate.

[wychen, 2015-04-27 23:29:59]

Hinoka, could you help sanity check this CL? Thanks!

[Ryan Tseng, 2015-04-28 01:10:34]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2015-04-28 01:10:34]

lgtm

[yoichio, 2015-04-28 06:43:04]

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

[yoichio, 2015-04-28 06:43:05]

lgtm

[wychen, 2015-04-28 23:21:47]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-04-28 23:22:11]

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

[commit-bot: I haz the power, 2015-04-28 23:25:36]

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

[commit-bot: I haz the power, 2015-04-28 23:25:37]

Presubmit check for 1091283004-20001 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
    depot_tools/tests/git_cl_test.py

Presubmit checks took 195.5s to calculate.

[hinoka, 2015-04-28 23:30:28]

lgtm

[wychen, 2015-04-28 23:30:47]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-04-28 23:34:33]

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

[commit-bot: I haz the power, 2015-04-28 23:42:21]

Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295051