Thanks for a good bug report.

>  but changed the upstream relationship when I started to work on the third (ImplementThrottled)

how did you do that? I am frequently using the workflow you described and I change upstream with "git reparent-branch" and I haven't experienced problems.
Can you also cat .git/config for relevant branches, if you still have it?


Also, this isn't Rietveld specific, afaiu. That's why I want to know more, as I'm afraid it's broken for Gerrit git cl due to the same underlying cause.

Huh.  I used "git branch --set-upstream-to="; I'll take a look at git reparent-branch and see if it does something that doesn't.

Since I filed this bug, as a workaround I've doing (in ImplementThrottled), "git branch --set-upstream-to=master", which gets try jobs to work but makes the review harder (I figured I'd set it back on my next round of review).  If I set it back to NetworkStreamThrottler by the same method, this is what I see in my .git/config (excerpting for relevant branches):

[branch "FixMinPri"]
	remote = .
	merge = refs/heads/master
	rietveldissue = 1866483002
	rietveldserver = https://codereview.chromium.org
	base = 9ff75c568c5feb797f1dd2dec7bd74577f8e0212
	base-upstream = refs/heads/master
	rietveldpatchset = 140001
	last-upload-hash = 16c2991bbf66c64d38fa2e5d43f1b5281ddbf4aa
[branch "NetworkStreamThrottler"]
	remote = .
	merge = refs/heads/FixMinpri
	base = 16c2991bbf66c64d38fa2e5d43f1b5281ddbf4aa
	base-upstream = refs/heads/FixMinpri
	rietveldissue = 1901533002
	rietveldserver = https://codereview.chromium.org
	rietveldpatchset = 420001
	last-upload-hash = c140df8e4bdcf96160b5ef31465b567e076f9298
[branch "ImplementThrottled"]
	remote = .
	merge = refs/heads/NetworkStreamThrottler
	base = 9ff75c568c5feb797f1dd2dec7bd74577f8e0212
	base-upstream = refs/heads/master
	rietveldissue = 2130493002
	rietveldserver = https://codereview.chromium.org
	rietveldpatchset = 200001
	last-upload-hash = afcac566de1ed45b0d29d07219460241979dcc5e

I'm failing git cl try now because of a patch failure; I'll try to look into that later today and update this bug as to whether the problem's still present or not.

I believe the problem is still present.  I updated all three branches (master -> ToT, FixMinPri->master, NetworkStreamThrottler->FixMinPri, ImplementThrottled->NetworkStreamThrottler).  Looking at the CLs, there does not appear to be a dependent relationship between NetworkStreamThrottler & FixMinPri (though as you can see above there is in git), and the try jobs in the ImplementThrottled CL appear to fail in the way I'd expect them to if FixMinPri wasn't being automatically merged in.  

Looking at git-reparent-branch, it looks like it does a git branch --set-upstream-to= (which is what I do) and does a merge in python (which I'd rather do myself, at least without examining the python code to understand exactly what it's doing).  So I'd be surprised if that helped in any way.  

(I will probably be resetting the parent of the ImplementThrottled branch to master, and re-uploading to https://codereview.chromium.org/2130493002, to run try jobs, so you should look at PS 13 on that CL if you're matching things to the above summary.)

Let me know if there's anything else you need from me; thanks!

The problem can be seen pretty clearly in the difference between PS 13 and PS 14 on https://codereview.chromium.org/2130493002; the code's the same, the only difference is whether the upstream branch is the chain described in this issue or master.

Wait: i think i'm starting to get it. You have master branch, but you don't mention it in your .git/config and my guess it's that it doesn't have a CL attached to it. So, I think I'm finally getting it. Gotta do some tests.

Nope, that's not a problem. I don't know what's going on.

Ah, wait, I know - here is example of your tryjob failure: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/54396/steps/bot_update/logs/patch%20error
which says:

error: patch failed: net/http/http_network_transaction_unittest.cc:979
  Falling back to three-way merge...
  Applied patch to 'net/http/http_network_transaction_unittest.cc' cleanly.
  Applied patch net/http/http_network_transaction_unittest.cc cleanly.


What's interesting about this file is that in the first CL you add it, and in the following CL you remove it. Rietveld patches don't handle it well because we don't commit the patch right after application.

So, the bug is not in git cl, it actually tracks branches properly. The bug is in apply_issue which doesn't handle well dependent patches. 

Specifically somewhere here: https://cs.chromium.org/chromium/tools/depot_tools/apply_issue.py?q=apply_issue&sq=package:chromium&dr=C&l=286

I believe that's because after patch application of one CL, the diff isn't committed.

The potential fix is to add git commit call after the reference above so that the next patch would apply, and then commit with --amend, and then do "git reset --soft". However, there are likely complications, and hence I'll not commit my time to this focusing on bringing Gerrit instead, which has this thing already working.

That said, this buy really is annoying, so if somebody else wants to spend ~4 hours to fix it, i can assist in reviews.

You're the expert, and if it works in Gerrit, great, but I'll not that I'm not clear that the bug is in git cl, since if you look at the CLs, http://codereview.chromium.org/1901533002 does not show up as dependent on http://codereview.chromium.org/1866483002, despite the fact that their corresponding branches in my checkout do have a dependency.  So just at that level, it looks like there's a bug in git cl.

(I also don't see any removal of http_network_transaction_unittest.cc in any of my CLs, but maybe you're referring to adding and removing lines in the file?)

I thought that above you actually intentionally uploaded without dependencies so as to run tryjobs. git cl certainly has bugs, I just don't yet see how to repro them given above, but once I get repro, i'll fix the bug.

So, if you can still repro uploading CL of a dependent branch && having the dependency not shown in Rietveld, then:
 * re-upload with  "git cl upload -v -v -v"
 * give me again your .git/config of the whole chain including master branch && origin config
 * version of depot_tools you are using

Even better would be (that's what I'd have to do it locally given above) if you can re-create the state you are in from scratch, otherwise I have to guess it :(


> (I also don't see any removal of http_network_transaction_unittest.cc 
> in any of my CLs, but maybe you're referring to adding and removing lines in the file?)

Oh, yes. AFAIU, +-3 lines around each patch chunk of CL2 must be known to git index, but since we don't add CL1 to index, CL2 fails to apply.

Let's narrow this down a bit.  The Branch/CL I'm actively working on is the final one of the three (ImplementThrottled/http://codereview.chromium.org/2130493002).  It's the only one I'm flipping back and forth between parents.  But it's not where I think the problem is, so let's actually leave it out of the picture.

The problem (IM Not Very Expert O) is between the other two CLs/Branches: NetworkStreamThrottler/http://codereview.chromium.org/1901533002 and FixMinPri/http://codereview.chromium.org/1866483002.  NST depends on FMP, but that does not appear to be reflected in the CLs.  That doesn't show up in the try jobs of either of the two CLs because they don't actually have any overlapping code; it only shows up in the try jobs of the third CL.  But unless you feel like it's a red herring, it would strike me that it's useful to debug the apparent inconsistency between those CLs and their branch relationship.  And I haven't changed their relationship in a while.

My .git/config for just those two CLs, master, and origin, is:
[remote "origin"]
	url = https://chromium.googlesource.com/chromium/src.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	fetch = +refs/tags/*:refs/tags/*
	fetch = +refs/branch-heads/*:refs/remotes/branch-heads/*
[branch "master"]
	base = 5b48ea6b4cb0c6e9bdf74ac1eec6ec81a497c4cf
	base-upstream = origin/master
	remote = origin
	merge = refs/heads/master
[branch "NetworkStreamThrottler"]
	remote = .
	merge = refs/heads/FixMinpri
	base = 6b86751361544b8aac03ccc0e3d3aaaf5c085541
	base-upstream = refs/heads/FixMinpri
	rietveldissue = 1901533002
	rietveldserver = https://codereview.chromium.org
	rietveldpatchset = 440001
	last-upload-hash = 9ec0b48004bae01da212ffbbfda27a1edbf8be8a
[branch "FixMinPri"]
	remote = .
	merge = refs/heads/master
	rietveldissue = 1866483002
	rietveldserver = https://codereview.chromium.org
	base = e9eb3d1bdb63fd93662acaa41b86c2c53ead4748
	base-upstream = refs/heads/master
	rietveldpatchset = 160001
	last-upload-hash = 6b86751361544b8aac03ccc0e3d3aaaf5c085541

rdsmith-macbookpro:../chrome/src [ImplementThrottled]  $ gclient --version
gclient --version
gclient.py 0.7
rdsmith-macbookpro:../chrome/src [ImplementThrottled]  $ 

Given the above, which branches would you like me to re-upload with -v -v -v?

Log of git cl upload -v -v -v --bypass-hooks on NetworkStreamThrottler attached.

Deleted: upload-log.txt 11.9 KB

upload-log.txt 11.9 KB View Download

Actually we are getting somewhere! The interesting bit is https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=247783#132
lines 132 and 133:

DEBUG:root:git config --int branch.FixMinpri.rietveldissue
DEBUG:root:git config --int branch.FixMinpri.gerritissue

Gerrit issue shouldn't be checked if rietveldissue is set.
So, two thigns:

can you ran:
$ git config --int branch.FixMinpri.rietveldissue
and also
$ git config branch.FixMinpri.rietveldissue
and capture output.

My guess is that something is wrong there.
Then two other versions of depot_tools to try "git cl upload -v -v -v --bypass-hooks" with:

1) go to depot_tools and 
git checkout 6d0a5acdc43f959770d844a3ccb8bc3a5ad9afb6.

2) go to depot_tools and
git fetch && git checkout origin/master && git cl patch -b try-more-debug https://codereview.chromium.org/2267833002 

rdsmith-macbookpro:../chrome/src [NetworkStreamThrottler]  $ git config --int branch.FixMinPri.rietveldissue
git config --int branch.FixMinPri.rietveldissue
1866483002
rdsmith-macbookpro:../chrome/src [NetworkStreamThrottler]  $ git config branch.FixMinPri.rietveldissue
git config branch.FixMinPri.rietveldissue
1866483002

## Presuming in above you meant to target the branches rather than a variant of the branches with different capitalization.

git cl upload -v -v -v --bypass-hooks results:
* 6d0a5acd-log.txt
* 2267833002-log.txt

Let me know if you need other things.

Deleted: 6d0a5acd-log.txt 13.1 KB

6d0a5acd-log.txt 13.1 KB View Download

Deleted: 2267833002-log.txt 77.1 KB

2267833002-log.txt 77.1 KB View Download

Hm, i suspect this has smth to do with mac. If you ran command manually, the rietveldissue is returned as expected. But the git cl run with and without --int produces this ( https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=247794#152 ):

DEBUG:root:git config branch.FixMinpri.rietveldissue
DEBUG:root:Failed running ['git', 'config', 'branch.FixMinpri.rietveldissue']

So, the ultimate issue was that git config's upstream branch had a typo in capitalization:
FixMinpri  instead of FixMinPri

and that's why git cl upload couldn't determine the upstream CL.

See new title. We can't really fix --set-upstream, but we could:

1. git reparent-branch (not many users) - check current branches first, ensure new branch is among them.

2. git cl upload
  2.1  if upstream is local branch, check that it exists. If not, warn (and check for case sensitivity).
  2.2. if upstream local branch has no CL is attached to it, warn user before proceeding. This is useful if say you forgot to upload the base.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This entire bug chain is about CL dependencies on Rietveld, which is no longer relevant with the switch to Gerrit.