New issue
Advanced search Search tips

Issue 616912 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Add flag to git-cl to allow uploading while ignoring local branch tracking

Project Member Reported by gab@chromium.org, Jun 2 2016

Issue description

On https://codereview.chromium.org/2034713002/ I was using NO_DEPENDENCY_CHECKS=true to commit despite not even having lgtm on upstream (independent change piped locally to stack cleanups).

But presubmit failed saying I didn't have owner lgtm and looking at the output it looks like presubmit is evaluating the files in the "dependent" CL as well..?

Opening this issue per discussion on chromium-dev with Ravi who said:
"""
Right, NO_DEPENDENCY_CHECKS only disregards the CQ's dependency check but apply_issue.py still goes ahead and applies changes from the dependencies which appears to confuse the owners check.
Could you please file a crbug and CC me?
"""
 
Labels: -Build-Toolchain Build-Tools

Comment 2 by rmis...@google.com, Jun 3 2016

Cc: rmis...@chromium.org
Components: Infra
Labels: Infra-DX OS-All
Owner: ----
Status: Available (was: Assigned)
Changing to available incase somebody can pick it up before me.

Comment 3 by gab@chromium.org, Jun 8 2016

Labels: -Pri-3 Pri-2
This is really annoying, it essentially makes NO_DEPENDENCY_CHECKS useless in practice.
If you're attempting to land a CL without landing its dependencies first, why not just "git reparent-branch --root" and the upload a new patchset? The latest patchset won't have any dependencies and can be landed without special flags.

Comment 5 by gab@chromium.org, Jun 9 2016

When doing a massive refactor (e.g.  issue 612843 ) one often ends up piping changes until it compiles, but each change is independent. Having to rebase each one on master is unnecessary overhead, the diff generated is correct I just don't want it to be associated with the CL of the branch it's based on locally.

This is exactly what NO_DEPENDENCY_CHECKS=true is intended for.

I'm not trying to "land without landing dependencies", there are no dependencies, but git cl automatically adds one which I can't remove (I like that it does that in general but in that case I don't want it and I can't remove it).

This was discussed on chromium-dev some time ago and people agreed this was an issue. It was suggested that we use NO_DEPENDENCY_CHECKS=true. That works for me, except that it doesn't pass presubmit if "dependent" CL doesn't have lgtms (this bug)...
Components: -Infra Infra>Git Infra>Codereview

Comment 7 by gab@chromium.org, Sep 14 2016

ping? this is still slowing me down when piping parallel refactorings

Comment 8 by gab@chromium.org, Sep 14 2016

Could we just add an "X" besides the identified dependency on the UI so that the owner can manually suppress it if he deems it's not useful?

Comment 9 by gab@chromium.org, Nov 4 2016

Cc: aga...@chromium.org
Ping, still a problem. The owners' check should use the immediate CL's list of files (i.e. not include the dependent's). This is a problem even if the "dependent" CL was lgtm'ed by all its proper OWNERS as was just the case for me in https://codereview.chromium.org/2470403002.
Cc: tandrii@chromium.org
Labels: -Build-Tools Proj-Gerrit-Migration Milestone-Fishfood
Summary: Add flag to git-cl to allow uploading while ignoring local branch tracking (was: Incorrect presubmit check when using NO_DEPENDENCY_CHECKS)
Thanks for CCing me.

Unfortunately, the tags like "NO_DEPENDENCY_CHECKS" are only used by the CQ process itself, not by the bots which happen to be triggered by the CQ -- things like apply_issue and PRESUBMIT don't know about them. So we can't use that to say "ignore the files from earlier patchsets when computing presubmit checks". But even if we could, that would be the wrong solution: if the author of the patch is saying "hey trust me, the dependencies of this CL don't need to be committed first", then, honestly, we shouldn't trust them -- we should test their commit without the dependencies, just like it will land on master. (Honestly, this CQ flag should go away entirely, once we've implemented what I suggest below.)

So it does seem like a good solution here is to let CL owners tell Rietveld to remove those "depends on" relationships. An "X" button next to the dependency in the UI seems like a good approach. The only problem with this approach is that we're not investing any engineering time in adding features to Rietveld, because we're moving to Gerrit in the relatively near future instead. If you would like to add this feature, patches are welcome.

Instead, I think the best solution is to make sure that authors can tell git-cl to upload their patch as though it was directly tracking origin/master, instead of tracking whatever set of nested branches you happen to have locally. This will mean adding a new flag to git-cl, but that flag will work equally well on both Rietveld and Gerrit, and will remain useful into the future.

I've added this to the PolyGerrit migration project, and to the fishfood milestone, to make sure that we track this and take care of it very soon.

Comment 11 by gab@chromium.org, Nov 4 2016

A flag on git cl SGTM. It should also be persistent per branch after being specified on the first upload IMO (i.e. just like --similarity).
Great.

In chat, gab@ also mentioned that we appear to respect "git config branch.{branchname}.skip-deps-upload True". It has some weird properties (it is inherited by child branches?) but that means there's already code it git-cl to skip uploading dependencies, and already code to make it persistent per-branch. That's a big head-start on this.
Ok, I did some digging, and this isn't quite as straightforward as we thought. A few notes:
* Right now, uploading to Rietveld respects "git config branch.<branchname>.skip-deps-uploads", but uploading to Gerrit does not
* Setting that config value on your *current* branch does nothing -- it is meant to be set on a parent branch, so that all children of that parent branch won't note that they're dependent on it. (This is for the use-case of "I have a crazy patch to make things work locally, but am doing real work on top of that".)
* Skipping dependencies on upload for Gerrit is actually exactly the same as rebasing the change, in the background. It'll make the upload take a really long time. So I'm actually unconvinced that we really want to do this -- it'll be a bad user experience, and encourage behavior which prolongs bad experiences, and disincentivize people from just rebasing their branches on their own.

Comment 14 by gab@chromium.org, Dec 22 2016

But why can't we just upload the diff versus the local upstream branch?
Because Gerrit doesn't deal in diffs. It deals in commits, and commits have parents, and that parent will be the upstream branch that you don't want to set as a dependency. So the alternative is to take your current commits and rebase them against the parent of the parent (or origin/master, or whatever you choose), and upload that instead. At which point you're rebasing, and I don't see any way around that.
Status: WontFix (was: Available)
I'm going to close this because I don't think it is a widely-used use-case, will take seriously significant effort, and encourages workflows that we don't want to encourage. If you feel strongly enough, feel free to reopen in the Afterglow milestone and we can reconsider it later.

Sign in to add a comment