Have apply_patch.py/checkout.py stage git patches instead of committing them

Have apply_patch.py/checkout.py stage git patches instead of committing them

For a bot_update Git world, we don't really want to commit patches.  Instead
we just want to leave them unstaged.

BUG=370503
TEST=ran locally with and without flag

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

[Ryan Tseng, 2014-05-07 02:11:59]

ptal, adding this in, and then will call it from bot_update.py

[M-A Ruel, 2014-05-07 11:56:26]

Why?

[Ryan Tseng, 2014-05-07 18:48:24]

My reasoning is that in the context of a tryjob, committing the patch doesn't
gain us very much, but:
* It messes with the state of the checkout.  When you run a git log --format=%H
on the checkout, you're not getting a useful hash.  Instead there needs to be
extra code to traverse backwards to the first non "Commited patch" commit to get
the correct got_revision.
* If you have a patch that touches multiple repositories (eg src/chrome/blah,
src/third_party/WebKit/blah), it becomes even messier to try to commit all of
them in their respective repositories.

So its more like I don't see the benefits of committing, and things become
messier when we do try to commit.

[iannucci, 2014-05-07 18:56:13]

On 2014/05/07 18:48:24, Ryan T. wrote:
> My reasoning is that in the context of a tryjob, committing the patch doesn't
> gain us very much, but:
> * It messes with the state of the checkout.  When you run a git log
--format=%H
> on the checkout, you're not getting a useful hash.  Instead there needs to be
> extra code to traverse backwards to the first non "Commited patch" commit to
get
> the correct got_revision.
> * If you have a patch that touches multiple repositories (eg src/chrome/blah,
> src/third_party/WebKit/blah), it becomes even messier to try to commit all of
> them in their respective repositories.
> 
> So its more like I don't see the benefits of committing, and things become
> messier when we do try to commit.

To further clarify: We also cannot rely on the fact that apply_issue actually
comitted the change either (on the bots) for the purposes of cleanup. For
example, the process can crash halfway through, or the build might modify a
committed file. So it also doesn't gain us anything from a bot-maintenance
perspective because we still need to do a clean.

[M-A Ruel, 2014-05-07 19:01:46]

On 2014/05/07 18:56:13, iannucci wrote:
> On 2014/05/07 18:48:24, Ryan T. wrote:
> > My reasoning is that in the context of a tryjob, committing the patch
doesn't
> > gain us very much, but:
> > * It messes with the state of the checkout.  When you run a git log
> --format=%H
> > on the checkout, you're not getting a useful hash.  Instead there needs to
be
> > extra code to traverse backwards to the first non "Commited patch" commit to
> get
> > the correct got_revision.
> > * If you have a patch that touches multiple repositories (eg
src/chrome/blah,
> > src/third_party/WebKit/blah), it becomes even messier to try to commit all
of
> > them in their respective repositories.
> > 
> > So its more like I don't see the benefits of committing, and things become
> > messier when we do try to commit.
> 
> To further clarify: We also cannot rely on the fact that apply_issue actually
> comitted the change either (on the bots) for the purposes of cleanup. For
> example, the process can crash halfway through, or the build might modify a
> committed file. So it also doesn't gain us anything from a bot-maintenance
> perspective because we still need to do a clean.

What about the code would always stage the change but never commit? Then you
don't need the flag anymore.

[iannucci, 2014-05-07 19:12:25]

On 2014/05/07 19:01:46, M-A Ruel wrote:
> On 2014/05/07 18:56:13, iannucci wrote:
> > On 2014/05/07 18:48:24, Ryan T. wrote:
> > > My reasoning is that in the context of a tryjob, committing the patch
> doesn't
> > > gain us very much, but:
> > > * It messes with the state of the checkout.  When you run a git log
> > --format=%H
> > > on the checkout, you're not getting a useful hash.  Instead there needs to
> be
> > > extra code to traverse backwards to the first non "Commited patch" commit
to
> > get
> > > the correct got_revision.
> > > * If you have a patch that touches multiple repositories (eg
> src/chrome/blah,
> > > src/third_party/WebKit/blah), it becomes even messier to try to commit all
> of
> > > them in their respective repositories.
> > > 
> > > So its more like I don't see the benefits of committing, and things become
> > > messier when we do try to commit.
> > 
> > To further clarify: We also cannot rely on the fact that apply_issue
actually
> > comitted the change either (on the bots) for the purposes of cleanup. For
> > example, the process can crash halfway through, or the build might modify a
> > committed file. So it also doesn't gain us anything from a bot-maintenance
> > perspective because we still need to do a clean.
> 
> What about the code would always stage the change but never commit? Then you
> don't need the flag anymore.

Hm... I actually think the commit behavior for devs is useful, but having
apply_issue stage (but not commit) the change would be useful for the bots (or,
at least, as useful as not committing it at all).

Ryan wdyt?

[Ryan Tseng, 2014-05-07 19:23:16]

I don't have a opinion as to whether or not to stage, but it sounds fine to me,
and it would remove a bit of the special casing in this CL.

[M-A Ruel, 2014-05-07 19:48:44]

On 2014/05/07 19:23:16, Ryan T. wrote:
> I don't have a opinion as to whether or not to stage, but it sounds fine to
me,
> and it would remove a bit of the special casing in this CL.

Please do that then.

[Ryan Tseng, 2014-05-07 21:02:40]

Done.

Tested with "apply_issue --issue 267713009 --patchset 100001 --no_commit"  and
"apply_issue --issue 267713009 --patchset 100001" in a chromium/src checkout

[Ryan Tseng, 2014-05-07 23:40:55]

Modified checkout.py to never commit, always stage

[Ryan Tseng, 2014-05-07 23:45:17]

Fixed bug, tested and works with "apply_issue --issue 274773003 --patchset 1" in
a chromium checkout.

[iannucci, 2014-05-08 07:28:43]

On 2014/05/07 23:45:17, Ryan T. wrote:
> Fixed bug, tested and works with "apply_issue --issue 274773003 --patchset 1"
in
> a chromium checkout.

Hm. Since this changes the default behavior (and, I think it still overwrites
the cl # in the git config for this branch?), it would probably be necessary to:
  * send a PSA
  * have this print a message stating that the changes have been staged but not
committed.

I honestly think this should still be an option --no-commit.

[iannucci, 2014-05-08 07:29:48]

https://codereview.chromium.org/273543002/diff/70001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/273543002/diff/70001/checkout.py#newcode630
checkout.py:630: def apply_patch(self, patches, post_processors=None,
verbose=False):
(also, this code terrifies me to no end. There is literally no reason why this
whole thing shouldn't just be `git apply < diff`.)

[M-A Ruel, 2014-05-08 16:40:17]

Please update the CL description.

On 2014/05/08 07:28:43, iannucci wrote:
> Hm. Since this changes the default behavior (and, I think it still overwrites
> the cl # in the git config for this branch?), it would probably be necessary
to:
>   * send a PSA
>   * have this print a message stating that the changes have been staged but
not
> committed.

I don't see the need. apply_issue.py is not a user facing tool even if it is in
the root directory, it has never been advertised as such.

> I honestly think this should still be an option --no-commit.

Options are the root of all untested code paths.

https://codereview.chromium.org/273543002/diff/70001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/273543002/diff/70001/checkout.py#newcode630
checkout.py:630: def apply_patch(self, patches, post_processors=None,
verbose=False):
On 2014/05/08 07:29:48, iannucci wrote:
> (also, this code terrifies me to no end. There is literally no reason why this
> whole thing shouldn't just be `git apply < diff`.)

It is simple;
- It properly applies svn generated patches.
- Rietveld mangles the patch.

[iannucci, 2014-05-08 17:47:00]

On 2014/05/08 16:40:17, M-A Ruel wrote:
> Please update the CL description.
> 
> On 2014/05/08 07:28:43, iannucci wrote:
> > Hm. Since this changes the default behavior (and, I think it still
overwrites
> > the cl # in the git config for this branch?), it would probably be necessary
> to:
> >   * send a PSA
> >   * have this print a message stating that the changes have been staged but
> not
> > committed.
> 
> I don't see the need. apply_issue.py is not a user facing tool even if it is
in
> the root directory, it has never been advertised as such.
> 

Oh, I misunderstood... I thought this code was used for `git cl patch`?

> > I honestly think this should still be an option --no-commit.
> 
> Options are the root of all untested code paths.

Yes, agreed. However, 'tools which are expected to work flawlessly on dev
machines and bot machines' are the root of all depot_tools confusion. Though if
this doesn't run on dev machines then it's not an issue :)

> 
> https://codereview.chromium.org/273543002/diff/70001/checkout.py
> File checkout.py (right):
> 
> https://codereview.chromium.org/273543002/diff/70001/checkout.py#newcode630
> checkout.py:630: def apply_patch(self, patches, post_processors=None,
> verbose=False):
> On 2014/05/08 07:29:48, iannucci wrote:
> > (also, this code terrifies me to no end. There is literally no reason why
this
> > whole thing shouldn't just be `git apply < diff`.)
> 
> It is simple;
> - It properly applies svn generated patches.
> - Rietveld mangles the patch.

[iannucci, 2014-05-08 17:48:24]

On 2014/05/08 17:47:00, iannucci wrote:
> On 2014/05/08 16:40:17, M-A Ruel wrote:
> > Please update the CL description.
> > 
> > On 2014/05/08 07:28:43, iannucci wrote:
> > > Hm. Since this changes the default behavior (and, I think it still
> overwrites
> > > the cl # in the git config for this branch?), it would probably be
necessary
> > to:
> > >   * send a PSA
> > >   * have this print a message stating that the changes have been staged
but
> > not
> > > committed.
> > 
> > I don't see the need. apply_issue.py is not a user facing tool even if it is
> in
> > the root directory, it has never been advertised as such.
> > 
> 
> Oh, I misunderstood... I thought this code was used for `git cl patch`?
> 
> > > I honestly think this should still be an option --no-commit.
> > 
> > Options are the root of all untested code paths.
> 
> Yes, agreed. However, 'tools which are expected to work flawlessly on dev
> machines and bot machines' are the root of all depot_tools confusion. Though
if
> this doesn't run on dev machines then it's not an issue :)
> 
> > 
> > https://codereview.chromium.org/273543002/diff/70001/checkout.py
> > File checkout.py (right):
> > 
> > https://codereview.chromium.org/273543002/diff/70001/checkout.py#newcode630
> > checkout.py:630: def apply_patch(self, patches, post_processors=None,
> > verbose=False):
> > On 2014/05/08 07:29:48, iannucci wrote:
> > > (also, this code terrifies me to no end. There is literally no reason why
> this
> > > whole thing shouldn't just be `git apply < diff`.)
> > 
> > It is simple;
> > - It properly applies svn generated patches.
> > - Rietveld mangles the patch.

Yeah, but I wonder if it wouldn't be more robust to generate a `git am` style
patch from the mangled rietveld garbage and use `git apply` to apply it. This
code fails when you do:
  * change a
  * mv a b

or 
  * mv a b
  * change b

or
  * mv a b
  * mv b a

or
...

[Ryan Tseng, 2014-05-09 22:17:46]

git cl patch uses its own logic and doesn't touch apply_issue.py :)

[Ryan Tseng, 2014-05-09 22:18:35]

CL title updated

[iannucci, 2014-05-09 22:45:50]

lgtm

[Ryan Tseng, 2014-05-09 23:14:06]

The CQ bit was checked by hinoka@google.com

[commit-bot: I haz the power, 2014-05-09 23:16:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hinoka@chromium.org/273543002/70001

[commit-bot: I haz the power, 2014-05-09 23:19:09]

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

[commit-bot: I haz the power, 2014-05-09 23:19:09]

Presubmit check for 273543002-70001 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/checkout_test.py (41.91s) failed
To /tmp/trialddKCO0/repos/git/repo_1
 ! [rejected]        working_branch -> master (non-fast-forward)
error: failed to push some refs to '/tmp/trialddKCO0/repos/git/repo_1'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. Check out this branch and merge the remote changes
hint: (e.g. 'git pull') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
E.fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working
tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
E....................
======================================================================
ERROR: testAll (__main__.GitCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/checkout_test.py", line 455, in testAll
    self._check_base(self._get_co(None), root, None)
  File "tests/checkout_test.py", line 402, in _check_base
    revision = co.commit(u'msg', fake_author)
  File "/b/infra_internal/commit_queue/workdir/tools/depot_tools/checkout.py",
line 736, in commit
    '--quiet'])
  File "/b/infra_internal/commit_queue/workdir/tools/depot_tools/checkout.py",
line 751, in _check_call_git
    return subprocess2.check_call_out(['git'] + 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 push origin working_branch:master --quiet
returned non-zero exit status 1 in
/tmp/trialddKCO0/__main__.GitCheckout.testAll/foo

======================================================================
ERROR: testMove (__main__.GitCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/checkout_test.py", line 473, in testMove
    ['git', 'diff', 'HEAD~', '--name-status'], cwd=co.project_path)
  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
478, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
CalledProcessError: Command git diff HEAD~ --name-status returned non-zero exit
status 128 in /tmp/trialddKCO0/__main__.GitCheckout.testMove/foo

----------------------------------------------------------------------
Ran 23 tests in 41.440s

FAILED (errors=2)


Presubmit checks took 140.1s to calculate.

[hinoka, 2014-05-10 00:36:27]

Added new PS to fix test breakage.  I think the commit function can just be
removed (confirmed by agable), but thats not in the scope of this CL.

[hinoka, 2014-05-10 00:36:33]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2014-05-10 00:37:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hinoka@chromium.org/273543002/90001

[commit-bot: I haz the power, 2014-05-10 00:38:55]

Change committed as 269468

[hinoka, 2014-05-10 02:05:55]

A revert of this CL has been created in
https://codereview.chromium.org/271283003/ by hinoka@chromium.org.

The reason for reverting is: Borked all the tryjobs due to missing --base_ref,
needed to land with https://codereview.chromium.org/273543002/.