git_cl sanity checks

git_cl sanity checks
    
Added checks:
- Block upload of a diff from sha not in current branch
  (this includes tracking against an unrebased local master)
- Block upload of a diff containing shas from origin/master
  (should never happen)

- Use explicit calls to git merge-base instead of calling git diff
   with "<branch>..." which implicitly uses merge-base.

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

[Isaac (away), 2012-10-26 22:33:45]

Moving refactors to separate CL.  Note that this is failing some unit tests so
need to fix before submitting.

[M-A Ruel, 2012-10-27 01:29:41]

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py#newcode171
git_cl.py:171: if upstream_sha != common_ancestor:
I disagree with this check. It happens I git fetch but do not rebase, simply
because I rebased for one of the CL I'm working on but not another one.

[Isaac (away), 2012-10-27 01:32:54]

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py#newcode171
git_cl.py:171: if upstream_sha != common_ancestor:
On 2012/10/27 01:29:41, Marc-Antoine Ruel wrote:
> I disagree with this check. It happens I git fetch but do not rebase, simply
> because I rebased for one of the CL I'm working on but not another one.

This is intended to catch diffing against a rebased branch while your current
branch is not rebased.  That means the diff will pull in diff from lots of other
commits.

I'm not clear why your scenario would trigger this -- can you clarify?

[M-A Ruel, 2012-10-27 01:37:23]

Also, I'd prefer a proper docstring. I like a sanity check idea though

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/3002/git_cl.py#newcode171
git_cl.py:171: if upstream_sha != common_ancestor:
On 2012/10/27 01:32:55, Isaac wrote:
> On 2012/10/27 01:29:41, Marc-Antoine Ruel wrote:
> > I disagree with this check. It happens I git fetch but do not rebase, simply
> > because I rebased for one of the CL I'm working on but not another one.
> 
> This is intended to catch diffing against a rebased branch while your current
> branch is not rebased.  That means the diff will pull in diff from lots of
other
> commits.
> 
> I'm not clear why your scenario would trigger this -- can you clarify?


git checkout -b work origin/master
vim foo
git commit -a -m "Rock on"
git checkout -b leisure origin/master
git pull
# Assumes something was pulled.
git checkout work
git cl upload
# Note upstream == origin/master, but "ref-parse origin/master" != "merge-base
origin/master HEAD"

[Isaac (away), 2012-10-27 02:12:21]

I was expecting cl.GetUpstreamBranch() to produce the same sha that git cl
upload was diffing against.  But this assumption was wrong:

-> 'git cl upload' will, by default, diff against the branch point on your
tracking branch
-> GetUpstreamBranch returns the symbolic reference to tip of tracking branch.

Should I change GetUpstreamBranch to return the branch point, or should I make
another function?

[M-A Ruel, 2012-10-28 01:17:33]

On 2012/10/27 02:12:21, Isaac wrote:
> I was expecting cl.GetUpstreamBranch() to produce the same sha that git cl
> upload was diffing against.  But this assumption was wrong:
> 
> -> 'git cl upload' will, by default, diff against the branch point on your
> tracking branch
> -> GetUpstreamBranch returns the symbolic reference to tip of tracking branch.
> 
> Should I change GetUpstreamBranch to return the branch point, or should I make
> another function?

Probably better to make another function.

[Isaac (away), 2012-10-28 20:37:19]

ptal

[M-A Ruel, 2012-10-28 20:53:02]

https://chromiumcodereview.appspot.com/11262057/diff/12002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/12002/git_cl.py#newcode167
git_cl.py:167: def git_cl_sanity_checks(upstream_git_obj):
Would still like a oneliner docstring for the purpose of the function

https://chromiumcodereview.appspot.com/11262057/diff/12002/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/12002/scm.py#newcode115
scm.py:115: upstream_branch += '...'
A quick "git gs CaptureStatus" tells me this will affect trychange.py:291 and
presubmit_support.py:1192 in addition to git_cl.py. Can you make sure both call
won't be badly affected?

[Isaac (away), 2012-10-28 21:16:42]

https://chromiumcodereview.appspot.com/11262057/diff/12002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/12002/git_cl.py#newcode167
git_cl.py:167: def git_cl_sanity_checks(upstream_git_obj):
On 2012/10/28 20:53:02, Marc-Antoine Ruel wrote:
> Would still like a oneliner docstring for the purpose of the function

Done.

https://chromiumcodereview.appspot.com/11262057/diff/12002/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/12002/scm.py#newcode115
scm.py:115: upstream_branch += '...'
On 2012/10/28 20:53:02, Marc-Antoine Ruel wrote:
> A quick "git gs CaptureStatus" tells me this will affect trychange.py:291 and
> presubmit_support.py:1192 in addition to git_cl.py. Can you make sure both
call
> won't be badly affected?

I wanted to make this change to avoid an extra merge-base call, which would be a
no-op.  But given GenerateDiff has the same behavior, I've reverted changes to
this file.  I think it is preferable to not do implicit calls to merge base but
it is outside the scope of this CL.

[M-A Ruel, 2012-10-28 22:29:51]

https://chromiumcodereview.appspot.com/11262057/diff/11003/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/11003/git_cl.py#newcode183
git_cl.py:183: ['rev-list', '^%s' % upstream_sha,
'remotes/origin/master']).splitlines()
I don't understand why remotes/origin/master is hardcoded here. I use git-cl
with git-svn clones frequently, where the "remote" is a svn-remote and I usually
diff against a remote named "trunk", not "origin/trunk".

[Isaac (away), 2012-10-29 00:16:40]

git_cl uploads, by default, commits in the local branch that are not in the
tracking branch.  When one wants to branch from a pinned revision, or use LKGR
in the git workflow, new branches must track the local master branch.  Because
gclient sync only rebases the current branch, not the local master, it is easy
to get into the state where git_cl tries to upload commits that are from origin.
 Do you have another way you'd prefer to solve this?

I could skip the check if remotes/origin/master doesn't exist, or if the git
repo doesn't look like a gclient checkout.

[M-A Ruel, 2012-10-29 00:31:37]

On 2012/10/29 00:16:40, Isaac wrote:
> git_cl uploads, by default, commits in the local branch that are not in the
> tracking branch.  When one wants to branch from a pinned revision, or use LKGR
> in the git workflow, new branches must track the local master branch.  Because
> gclient sync only rebases the current branch, not the local master, it is easy
> to get into the state where git_cl tries to upload commits that are from
origin.
>  Do you have another way you'd prefer to solve this?
> 
> I could skip the check if remotes/origin/master doesn't exist, or if the git
> repo doesn't look like a gclient checkout.

Another way of looking at it would be:
Implement gclient sync in a way that if the current branch is tracking a local
branch, rebase the local branch first. Iterate until the branch to be rebased is
tracking the remote being updated. The problem with that is with rebase
conflicts, but if there were a way to automate that, that'd be nice.

So if you take the same approach above, you'd have git-cl UpstreamBranch()
iterate until the upstream branch is a remote, either git-svn or git-remote.

Is this a problem worth fixing? I never personally witnessed it. I think if you
implement the iterating solution it'd fix the check to work in all cases without
side effects.

[Isaac (away), 2012-10-29 01:23:36]

That sounds a bit complex and I'm not sure would cover people who use personal
remotes to share code between computers.  How about I look for remotes/trunk,
fallback to remotes/origin/master, and gracefully fallback to a warning?

[Isaac (away), 2012-11-01 01:02:33]

ptal

[M-A Ruel, 2012-11-01 13:46:41]

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode67
git_cl.py:67: return RunCommand(['git'] + args, **kwargs).strip()
I'd prefer this to be done as a separate CL, because you added it here but
didn't remove the corresponding .strip() at call sites, like lines 109, 267,
315, etc. This will be a noisy CL so let's keep this out of this CL.

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode430
git_cl.py:430: def FetchUpstreamTuple(self, branch=None):
Let's not make it optional, it'd be easier to follow up the code.

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode435
git_cl.py:435: branch = branch or self.GetBranch()
Have the caller call self.GetBranch()

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode478
git_cl.py:478: def GetRemoteBranch(self):
I think it'd be better to split this code in parts;

1. Have a function that for a specific fully qualified branch name, like
refs/heads/working, returns the tracked branch without heuristic and without
splitting the remote, explicitly returns refs/remotes/origin/master,
refs/heads/master or whatever is tracked; or None if no tracking branch is set.

2. Have a function that implements the heuristic to guess a default tracking
branch. Basically lines 441-462.

3. Have a function that do the iterative search for a tracked branch that is
located in refs/remotes, roughly line 482-487. It could call #1 for each branch
it finds and iterate until the returned value starts with refs/remotes/. If the
loop ends up in a dead-end, e.g. #1 returns None, then call #2.

I think it'd make the code much clearer. wdyt?

[Isaac (away), 2012-11-01 21:33:40]

I haven't uploaded fixes yet, will ping thread when I do.  I'm fixing a number
of broken unit tests :-\

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode67
git_cl.py:67: return RunCommand(['git'] + args, **kwargs).strip()
On 2012/11/01 13:46:41, Marc-Antoine Ruel wrote:
> I'd prefer this to be done as a separate CL, because you added it here but
> didn't remove the corresponding .strip() at call sites, like lines 109, 267,
> 315, etc. This will be a noisy CL so let's keep this out of this CL.

Done.

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode430
git_cl.py:430: def FetchUpstreamTuple(self, branch=None):
On 2012/11/01 13:46:41, Marc-Antoine Ruel wrote:
> Let's not make it optional, it'd be easier to follow up the code.

Done.

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode435
git_cl.py:435: branch = branch or self.GetBranch()
On 2012/11/01 13:46:41, Marc-Antoine Ruel wrote:
> Have the caller call self.GetBranch()

Done.

https://chromiumcodereview.appspot.com/11262057/diff/24002/git_cl.py#newcode478
git_cl.py:478: def GetRemoteBranch(self):
On 2012/11/01 13:46:41, Marc-Antoine Ruel wrote:
> I think it'd be better to split this code in parts;
> 
> 1. Have a function that for a specific fully qualified branch name, like
> refs/heads/working, returns the tracked branch without heuristic and without
> splitting the remote, explicitly returns refs/remotes/origin/master,
> refs/heads/master or whatever is tracked; or None if no tracking branch is
set.
> 
> 2. Have a function that implements the heuristic to guess a default tracking
> branch. Basically lines 441-462.
> 
> 3. Have a function that do the iterative search for a tracked branch that is
> located in refs/remotes, roughly line 482-487. It could call #1 for each
branch
> it finds and iterate until the returned value starts with refs/remotes/. If
the
> loop ends up in a dead-end, e.g. #1 returns None, then call #2.
> 
> I think it'd make the code much clearer. wdyt?

I'd like to punt on that. The goal of this patch is not to fix git_cl and it
already improves the functionality of GetRemote by doing the recursive look up
(used by GetRemoteUrl)

[Isaac (away), 2012-11-01 23:00:45]

PTAL, unit tests fixed, finally.

[M-A Ruel, 2012-11-01 23:59:11]

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'depottools.remotebranch'])
I don't understand, there's already rietveld.upstream-remote and
rietveld.upstream-branch. What's the use case for this variable?

[iannucci, 2012-11-02 00:30:05]

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode509
git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
Maybe there's a better name for this function? It's really doing a specific
subset of sanity checks. Unless you plan to accumulate all sanity checks here
over time?  Maybe GitSanityChecks calls VerifyBranchMergeBasis (or something?)

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode509
git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
Maybe we should have a temporary --enable_git_insanity option to let people
bypass this without skipping all hooks.

https://chromiumcodereview.appspot.com/11262057/diff/31002/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/tests/git_cl_test....
tests/git_cl_test.py:180: ((['git', 'config', 'depottools.remotebranch'],),
(((''), None), 1)),
Too much parens!

[Isaac (away), 2012-11-02 00:38:27]

re M-A:

http://codereview.chromium.org/11262057/diff/31002/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/11262057/diff/31002/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'depottools.remotebranch'])
On 2012/11/01 23:59:11, Marc-Antoine Ruel wrote:
> I don't understand, there's already rietveld.upstream-remote and
> rietveld.upstream-branch. What's the use case for this variable?

Good question, but I think there's justification for it:

1) This function is not part of rietveld and is also called in the gerrit upload
case.
2) I intend for people to be able to modify this value without side effects on
whatever rietveld.upstream-remote uses.
3) This config value can actually refer to an arbitrary git sha to use as the
list of 'checked in' commits.

[Isaac (away), 2012-11-02 00:50:03]

Re: Robert

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode509
git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
On 2012/11/02 00:30:05, iannucci wrote:
> Maybe there's a better name for this function? It's really doing a specific
> subset of sanity checks. Unless you plan to accumulate all sanity checks here
> over time?  Maybe GitSanityChecks calls VerifyBranchMergeBasis (or something?)

Seems reasonable but I think long term we want to move git checks into a single
function, so might as well start now.

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode509
git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
On 2012/11/02 00:30:05, iannucci wrote:
> Maybe we should have a temporary --enable_git_insanity option to let people
> bypass this without skipping all hooks.

Actually, thinking about this again, I think the config option provides
sufficient escape here.  I clarified the documentation.

In particular:

git config depottools.remotebranch <base sha>
git cl upload <base sha>

will bypass both checks.

https://chromiumcodereview.appspot.com/11262057/diff/31002/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/tests/git_cl_test....
tests/git_cl_test.py:180: ((['git', 'config', 'depottools.remotebranch'],),
(((''), None), 1)),
On 2012/11/02 00:30:05, iannucci wrote:
> Too much parens!

Nice catch, done.

[M-A Ruel, 2012-11-02 00:51:54]

http://codereview.chromium.org/11262057/diff/31002/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/11262057/diff/31002/git_cl.py#newcode509
git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
On 2012/11/02 00:30:05, iannucci wrote:
> Maybe we should have a temporary --enable_git_insanity option to let people
> bypass this without skipping all hooks.

I don't think there's a use case where this check would fail and that would be a
valid use case. What would it be?

http://codereview.chromium.org/11262057/diff/31002/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'depottools.remotebranch'])
On 2012/11/02 00:38:27, Isaac wrote:
> On 2012/11/01 23:59:11, Marc-Antoine Ruel wrote:
> > I don't understand, there's already rietveld.upstream-remote and
> > rietveld.upstream-branch. What's the use case for this variable?
> 
> Good question, but I think there's justification for it:
> 
> 1) This function is not part of rietveld and is also called in the gerrit
upload
> case.

I agree the rietveld name wasn't a great idea.

> 2) I intend for people to be able to modify this value without side effects on
> whatever rietveld.upstream-remote uses.

Why?

> 3) This config value can actually refer to an arbitrary git sha to use as the
> list of 'checked in' commits.

I'd rather not support that. In git, you track branches, not commit hashes.

I'm still mildly unconvinced. Anyhow, I'd prefer not using as generic depottools
(rietveld was also a bad idea), git-cl would be fine.

[iannucci, 2012-11-02 01:01:48]

lgtm... this should definitely keep people from getting all tangled up in a web
of gclient/git/rietveld/diff-induced madness :)

On 2012/11/02 00:51:54, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/11262057/diff/31002/git_cl.py
> File git_cl.py (right):
> 
> http://codereview.chromium.org/11262057/diff/31002/git_cl.py#newcode509
> git_cl.py:509: def GitSanityChecks(self, upstream_git_obj):
> On 2012/11/02 00:30:05, iannucci wrote:
> > Maybe we should have a temporary --enable_git_insanity option to let people
> > bypass this without skipping all hooks.
> 
> I don't think there's a use case where this check would fail and that would be
a
> valid use case. What would it be?
> 

I was thinking about allowing a back door for use cases that we don't know
about, but even so, it would be easy to add the option later if a pressing use
case came up.

> http://codereview.chromium.org/11262057/diff/31002/git_cl.py#newcode524
> git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
> 'depottools.remotebranch'])
> On 2012/11/02 00:38:27, Isaac wrote:
> > On 2012/11/01 23:59:11, Marc-Antoine Ruel wrote:
> > > I don't understand, there's already rietveld.upstream-remote and
> > > rietveld.upstream-branch. What's the use case for this variable?
> > 
> > Good question, but I think there's justification for it:
> > 
> > 1) This function is not part of rietveld and is also called in the gerrit
> upload
> > case.
> 
> I agree the rietveld name wasn't a great idea.
> 
> > 2) I intend for people to be able to modify this value without side effects
on
> > whatever rietveld.upstream-remote uses.
> 
> Why?
> 
> > 3) This config value can actually refer to an arbitrary git sha to use as
the
> > list of 'checked in' commits.
> 
> I'd rather not support that. In git, you track branches, not commit hashes.
> 
> I'm still mildly unconvinced. Anyhow, I'd prefer not using as generic
depottools
> (rietveld was also a bad idea), git-cl would be fine.

[Isaac (away), 2012-11-02 06:57:59]

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/31002/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'depottools.remotebranch'])
On 2012/11/02 00:51:54, Marc-Antoine Ruel wrote:
> On 2012/11/02 00:38:27, Isaac wrote:
> > On 2012/11/01 23:59:11, Marc-Antoine Ruel wrote:
> > > I don't understand, there's already rietveld.upstream-remote and
> > > rietveld.upstream-branch. What's the use case for this variable?
> > 
> > Good question, but I think there's justification for it:
> > 
> > 1) This function is not part of rietveld and is also called in the gerrit
> upload
> > case.
> 
> I agree the rietveld name wasn't a great idea.
> 
> > 2) I intend for people to be able to modify this value without side effects
on
> > whatever rietveld.upstream-remote uses.
> 
> Why?
> 
> > 3) This config value can actually refer to an arbitrary git sha to use as
the
> > list of 'checked in' commits.
> 
> I'd rather not support that. In git, you track branches, not commit hashes.
> 
> I'm still mildly unconvinced. Anyhow, I'd prefer not using as generic
depottools
> (rietveld was also a bad idea), git-cl would be fine.

My thought of using the depottools section was that the rietveld stuff could be
consolidated there.  I'll move to 'gitcl.remotebranch' tomorrow if I don't hear
otherwise.

[M-A Ruel, 2012-11-02 13:37:29]

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'gitcl.remotebranch'])
But shouldn't that configuration be branch-specific?

[Isaac (away), 2012-11-02 16:12:02]

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'gitcl.remotebranch'])
On 2012/11/02 13:37:29, Marc-Antoine Ruel wrote:
> But shouldn't that configuration be branch-specific?

It is per-repo, not per-branch.  It is used here to generate a list of commits
already in the remote repository.

[M-A Ruel, 2012-11-02 17:24:56]

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'gitcl.remotebranch'])
On 2012/11/02 16:12:02, Isaac wrote:
> On 2012/11/02 13:37:29, Marc-Antoine Ruel wrote:
> > But shouldn't that configuration be branch-specific?
> 
> It is per-repo, not per-branch.  It is used here to generate a list of commits
> already in the remote repository.

So technically, it's per remote? I still fail to see how this can be useful. 
Let's take a use case I have as an example;
I have 3 remotes, origin, inet and local, where both inet and local fetch from
the same server but one through the local network the other through the internet
when I'm on the go. Both inet and local fetches to the same refs/remotes/inet/
even if git doesn't like that much.
In that case, the CL I'm basing off could be based against:
- origin/master
- inet/master
- inet/<anything>
- a local branch
- a local branch tracking any of the above
In that case, having a global gitcl.remotebranch doesn't seem particularly
useful.

Let's take another workflow I know about but that I don't use personally:
A company X has a local fork of a git repository and is tracking both upstream
and the local fork. The devs could have two remotes depending if they are
upstreaming a CL or they are working on their fork. Having a global
gitcl.remotebranch doesn't seem particularly useful there either.

I'd rather commit without support for gitcl.remotebranch and leave a CL ready in
case it's needed than preemptively add a config check for something that doesn't
have a clear use case yet.

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode527
git_cl.py:527: remote_branch = 'refs/remotes/' +
'/'.join(self.GetRemoteBranch())
This is not strictly true, e.g. you can fetch from a remote to something named
differently. I don't know if other parts have this assumption, I haven't
reviewed. I do use this occasionally but I don't think I ever used git-cl on
these repositories. (I'd have to try, I don't recall)

[Isaac (away), 2012-11-02 17:44:22]

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode524
git_cl.py:524: code, remote_branch = RunGitWithCode(['config',
'gitcl.remotebranch'])
In your case, if you set gitcl.remotebranch to refs/remotes/inet/master, git-cl
would always compare the list of shas you wanted to upload against the list of
shas visible from the inet master branch and make sure there wasn't overlap.

For the company X workflow, I don't think this config is needed.  They could
upload local changes to chromium from branches that track either remote.

The main use case I'm trying to support with this configuration is a user-hosted
remote.  Some chromium devs use hosted git repo to synchronize changes between
mulitple computers.  Each computer checks out a local branch that tracks the
personal remote.  We don't want to block uploads of shas present in this remote,
so for these users they could set gitcl.remotebranch to
refs/remotes/origin/master and then they can create and submit branches off of
the personal remote without having to fight git-cl warnings.

https://chromiumcodereview.appspot.com/11262057/diff/28003/git_cl.py#newcode527
git_cl.py:527: remote_branch = 'refs/remotes/' +
'/'.join(self.GetRemoteBranch())
Hmm, I didn't know that, but I think it's a reasonable assumption to make.  I've
accidentally created a local branch 'origin/master' before, and I didn't want
git-cl using that one, if it happened to exist.

[M-A Ruel, 2012-11-02 17:57:55]

On 2012/11/02 17:44:22, Isaac wrote:
> The main use case I'm trying to support with this configuration is a
user-hosted
> remote.  Some chromium devs use hosted git repo to synchronize changes between
> mulitple computers.  Each computer checks out a local branch that tracks the
> personal remote.  We don't want to block uploads of shas present in this
remote,
> so for these users they could set gitcl.remotebranch to
> refs/remotes/origin/master and then they can create and submit branches off of
> the personal remote without having to fight git-cl warnings.

That's also what I described and I do and in that case, you want to fix the
upstream tracking branch first before uploading.

anyway, lgtm

[commit-bot: I haz the power, 2012-11-02 18:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/11262057/28003

[commit-bot: I haz the power, 2012-11-02 18:08:47]

Presubmit check for 11262057-28003 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 **
submit-from-new-dir.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/submit-from-new-dir.sh
returned non-zero exit status 1 in
/b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds

WARNING: Use -t or --title to set the title of the patchset.
In the near future, -m or --message will send a message instead.
See http://goo.gl/JGg0Z for details.

WARNING:root:Could not determine which remote this change is associated with. 
You may prevent this message by running "git svn info" as documented here: 
http://code.google.com/p/chromium/wiki/UsingNewGit
fatal: ambiguous argument 'refs/remotes/./HEAD': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions
Command "git rev-list ^92627c24977d6ae4c82e559532e08251240ed008
refs/remotes/./HEAD" failed.

FAILURE: upload succeeds


abandon.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/abandon.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds

WARNING: Use -t or --title to set the title of the patchset.
In the near future, -m or --message will send a message instead.
See http://goo.gl/JGg0Z for details.

WARNING:root:Could not determine which remote this change is associated with. 
You may prevent this message by running "git svn info" as documented here: 
http://code.google.com/p/chromium/wiki/UsingNewGit
fatal: ambiguous argument 'refs/remotes/./HEAD': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions
Command "git rev-list ^01f1e4e0dcac3c05add99ad1cdf914487e93d35b
refs/remotes/./HEAD" failed.

FAILURE: upload succeeds


patch.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/patch.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds (needs a server running on localhost)

WARNING: Use -t or --title to set the title of the patchset.
In the near future, -m or --message will send a message instead.
See http://goo.gl/JGg0Z for details.

WARNING:root:Could not determine which remote this change is associated with. 
You may prevent this message by running "git svn info" as documented here: 
http://code.google.com/p/chromium/wiki/UsingNewGit
fatal: ambiguous argument 'refs/remotes/./HEAD': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions
Command "git rev-list ^d27f3c02c800a06cef3c7bea1210f93ab12247a3
refs/remotes/./HEAD" failed.

FAILURE: upload succeeds (needs a server running on localhost)


basic.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/basic.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: git-cl upload wants a server
TESTING: git-cl status has no issue
TESTING: upload succeeds (needs a server running on localhost)

WARNING: Use -t or --title to set the title of the patchset.
In the near future, -m or --message will send a message instead.
See http://goo.gl/JGg0Z for details.

WARNING:root:Could not determine which remote this change is associated with. 
You may prevent this message by running "git svn info" as documented here: 
http://code.google.com/p/chromium/wiki/UsingNewGit
fatal: ambiguous argument 'refs/remotes/./HEAD': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions
Command "git rev-list ^6a59d3df716764c88049a267dbfbd3f8798aacac
refs/remotes/./HEAD" failed.

FAILURE: upload succeeds (needs a server running on localhost)


Presubmit checks took 143.7s to calculate.

[Isaac (away), 2012-11-05 01:42:19]

Fixed presubmit errors from git-svn branch handling, CQing.

[commit-bot: I haz the power, 2012-11-05 01:42:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/11262057/35005

[commit-bot: I haz the power, 2012-11-05 01:45:21]

Change committed as 165887