git cl: cleanup branch config get/set/unset.

git cl: cleanup branch config get/set/unset.

Previously there was a soup with add-hoc formatting with
current branch name, which wasn't always set (see bug 611020).
This CL makes sure all such operations now:
 * properly use types --int and --bool
 * go through the *only* appropriate get/set/unset function.

Furthermore, tests were a mess wrt to raising exceptions when
git processes terminated with an exception. This CL cleaned up,
though I didn't go through all expectations, so some returns of
empty stdout instead of raising CalledProcess error are likely
remaining.

Disclaimer: this CL is not necessarily fixing the referenced bug
below, but it should at least provide better stacktrace when
the bug manifestst itself.

BUG=611020
R=agable@chromium.org

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/5d48c32f3d75691589c371266152b00aab355602

[tandrii(chromium), 2016-08-18 20:23:54]

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

[commit-bot: I haz the power, 2016-08-18 20:23:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2016-08-18 20:24:07]

Patchset #1 (id:1) has been deleted

[tandrii(chromium), 2016-08-18 20:25:08]

PTAL

This is a bit of a horror CL, but I'm fed up burning hours on fixing git cl
tests, so fix was long overdue.

My function names are really long. If you have good suggestions for truncating
them, please LMK.

[commit-bot: I haz the power, 2016-08-18 20:26:49]

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

[commit-bot: I haz the power, 2016-08-18 20:26:50]

Dry run: This issue passed the CQ dry run.

[agable, 2016-08-18 20:47:54]

This is a really nice cleanup. Just a few comments.

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

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode161
git_cl.py:161: assert branch
Have a helpful message along with your assertions, here and elsewhere

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode191
git_cl.py:191: def _git_unset_branch_config_value(setting, branch=None,
**kwargs):
I'd have this forward to _git_set_branch_config_value, and have that function
treat value==None as a sign to append '--unset', just like isinstance(value,
bool) is a sign to append '--bool'. That way you get all of the error checking.

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode1064
git_cl.py:1064: kwargs['branch'] = self.GetBranch()
Maybe assert that someone isn't trying to already specify the branch to be
something else other than self.GetBranch()?

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode1814
git_cl.py:1814: def PatchsetSettingSuffix(cls):
Don't call these suffixes, call them keys or config items or something like
that. They're only a suffix in the CLI "type.name.key" syntax, not in the .ini
file itself.

[tandrii(chromium), 2016-08-18 21:23:55]

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

[tandrii(chromium), 2016-08-18 21:24:04]

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

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode161
git_cl.py:161: assert branch
On 2016/08/18 20:47:54, agable wrote:
> Have a helpful message along with your assertions, here and elsewhere

Done.

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode191
git_cl.py:191: def _git_unset_branch_config_value(setting, branch=None,
**kwargs):
On 2016/08/18 20:47:54, agable wrote:
> I'd have this forward to _git_set_branch_config_value, and have that function
> treat value==None as a sign to append '--unset', just like isinstance(value,
> bool) is a sign to append '--bool'. That way you get all of the error
checking.

Done.

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode1064
git_cl.py:1064: kwargs['branch'] = self.GetBranch()
On 2016/08/18 20:47:54, agable wrote:
> Maybe assert that someone isn't trying to already specify the branch to be
> something else other than self.GetBranch()?

Done.

https://codereview.chromium.org/2259043002/diff/20001/git_cl.py#newcode1814
git_cl.py:1814: def PatchsetSettingSuffix(cls):
On 2016/08/18 20:47:54, agable wrote:
> Don't call these suffixes, call them keys or config items or something like
> that. They're only a suffix in the CLI "type.name.key" syntax, not in the .ini
> file itself.

How about ConfigKey (see next PS)?

[commit-bot: I haz the power, 2016-08-18 21:24:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 21:26:14]

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

[commit-bot: I haz the power, 2016-08-18 21:26:15]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30b85be849e75e10)

[agable, 2016-08-18 21:34:56]

lgtm

[tandrii(chromium), 2016-08-18 22:04:31]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2016-08-18 22:04:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 22:07:48]

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

[commit-bot: I haz the power, 2016-08-18 22:07:50]

Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30b882cc4206e810)

[martiniss, 2016-08-18 23:16:19]

The CQ bit was checked by martiniss@chromium.org

[commit-bot: I haz the power, 2016-08-18 23:16:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 23:19:43]

Description was changed from

==========
git cl: cleanup branch config get/set/unset.

Previously there was a soup with add-hoc formatting with
current branch name, which wasn't always set (see bug 611020).
This CL makes sure all such operations now:
 * properly use types --int and --bool
 * go through the *only* appropriate get/set/unset function.

Furthermore, tests were a mess wrt to raising exceptions when
git processes terminated with an exception. This CL cleaned up,
though I didn't go through all expectations, so some returns of
empty stdout instead of raising CalledProcess error are likely
remaining.

Disclaimer: this CL is not necessarily fixing the referenced bug
below, but it should at least provide better stacktrace when
the bug manifestst itself.

BUG=611020
R=agable@chromium.org
==========

to

==========
git cl: cleanup branch config get/set/unset.

Previously there was a soup with add-hoc formatting with
current branch name, which wasn't always set (see bug 611020).
This CL makes sure all such operations now:
 * properly use types --int and --bool
 * go through the *only* appropriate get/set/unset function.

Furthermore, tests were a mess wrt to raising exceptions when
git processes terminated with an exception. This CL cleaned up,
though I didn't go through all expectations, so some returns of
empty stdout instead of raising CalledProcess error are likely
remaining.

Disclaimer: this CL is not necessarily fixing the referenced bug
below, but it should at least provide better stacktrace when
the bug manifestst itself.

BUG=611020
R=agable@chromium.org

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/5d48c32f3d7569...
==========

[commit-bot: I haz the power, 2016-08-18 23:19:44]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/5d48c32f3d7569...