Dependency patchsets?

174 views
Skip to first unread message

Pavel Sergeev

unread,
Jun 23, 2015, 5:37:38 PM6/23/15
to Chromium-dev
Hello,

I've noticed, that 'git cl upload' started to write messages like that:

The current branch (...) is tracking a local branch (...) with an open CL. 
> Adding https://codereview.chromium.org/.../#ps1 as a dependency patchset.

when I upload a CL that is based on another uncommitted CL.
Is this some new feature of Reitveld? Where can I find more information?

-- 
Pavel Sergeev

Sylvain Defresne

unread,
Jun 23, 2015, 5:52:05 PM6/23/15
to Chromium-dev
You can see the information on codereview new UI (go to setting and unselect the "Deprecated UI" checkbox). I don't know if there are any other usage yet, but I'm hoping this will be used to allow sending CL with dependencies to the try bots.

I'd really like to have more information on what it will be used for too if available!

See screenshot of new UI:
Inline image 1
-- Sylvain

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

Ravi Mistry

unread,
Jun 24, 2015, 7:35:32 AM6/24/15
to sdef...@chromium.org, Chromium-dev
Apologies about the lack of a PSA, I was trying to make sure the feature sticks. Looks like things are stable now.

I had been working on a way to find, upload and apply patchset dependencies (formed due to tracked local branches) in depot_tools, and displaying those dependencies in Rietveld (in the new UI).

The complete flow and implementation is detailed in the design doc here.

This feature is live and you can currently upload and view your patchset dependencies in Rietveld.


On Tue, Jun 23, 2015 at 5:51 PM, Sylvain Defresne <sdef...@chromium.org> wrote:
You can see the information on codereview new UI (go to setting and unselect the "Deprecated UI" checkbox). I don't know if there are any other usage yet, but I'm hoping this will be used to allow sending CL with dependencies to the try bots.

Yes this was one of my goals.
Unfortunately try bots will not yet apply your dependencies because that code has been commented out yesterday (https://codereview.chromium.org/1194063003) because of an issue I found:

CL1 changes file1
A dependent CL2 changes file2 and calls newly added functions from CL1.

With my changes the trybots would succeed for CL2 because apply_issue.py will apply both CL1 and CL2. CQ will also be able to submit CL2 because the patch will apply cleanly to file2.

Result: CQ will land CL2 successfully with all trybots green, but the tree will break because of missing changes from file1.



I am working on a way to re-enable the commented out code. One possible solution is to have the CQ only submit a patchset if it has no open dependencies.

Dirk Pranke

unread,
Jun 24, 2015, 12:09:57 PM6/24/15
to Ravi Mistry, Sylvain Defresne, Chromium-dev
This sounds great, Ravi! I for one am very much looking forward to using this :).

-- Dirk

Anthony Berent

unread,
Jun 24, 2015, 2:31:39 PM6/24/15
to dpr...@chromium.org, Ravi Mistry, Sylvain Defresne, Chromium-dev
I just got a strange variation of this:

The current branch (kerberos) is tracking a local branch (master) with an open CL.
Adding https://codereview.chromium.org/1151773003/#ps1 as a dependency patchset.

Strange both because I am not changing master (it is identical to origin/master) and because  https://codereview.chromium.org/1151773003 is neither open (it has landed) nor mine.

Ravi Mistry

unread,
Jun 24, 2015, 2:35:10 PM6/24/15
to Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev
On Wed, Jun 24, 2015 at 2:30 PM, Anthony Berent <abe...@chromium.org> wrote:
I just got a strange variation of this:

The current branch (kerberos) is tracking a local branch (master) with an open CL.
Adding https://codereview.chromium.org/1151773003/#ps1 as a dependency patchset.

Strange both because I am not changing master (it is identical to origin/master) and because  https://codereview.chromium.org/1151773003 is neither open (it has landed) nor mine.

That is strange. What does "git cl status" show you?

Trent Apted

unread,
Jun 24, 2015, 11:11:17 PM6/24/15
to rmi...@chromium.org, Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev
On 25 June 2015 at 04:34, Ravi Mistry <rmi...@chromium.org> wrote:

On Wed, Jun 24, 2015 at 2:30 PM, Anthony Berent <abe...@chromium.org> wrote:
I just got a strange variation of this:

The current branch (kerberos) is tracking a local branch (master) with an open CL.
Adding https://codereview.chromium.org/1151773003/#ps1 as a dependency patchset.

Strange both because I am not changing master (it is identical to origin/master) and because  https://codereview.chromium.org/1151773003 is neither open (it has landed) nor mine.

I just got this as well.

The current branch (20150625-AppList-RemoveEnableFlag) is tracking a local branch (master) with an associated CL.

Looking in .git/config, I have:

[branch "master"]
        remote = origin
        merge = refs/heads/master
        rietveldissue = 938603002
        rietveldpatchset = 20001


So I think one day I must have done a `git cl patch 938603002` while on branch `master`. Likely it was followed by a `git reset --hard`, but this metadata stuck around.

I'll just edit .git/config manually, but perhaps the CL-depends feature should blacklist any metadata sitting on `master` anyway. Perhaps a diagnostic, ~"You appear to have <cruft> on master that was ignored by `git cl upload` - you might want to manually <fix cruft>".

Anthony Berent

unread,
Jun 25, 2015, 5:02:06 AM6/25/15
to Ravi Mistry, Dirk Pranke, Sylvain Defresne, Chromium-dev
(Now sending to the whole thread)

git cl status says:

======================================================
Branches associated with reviews:
  KerberosInstrumentation : None 
                 kerberos : https://codereview.chromium.org/1128043007 (lgtm)
           kerberosPolicy : None 
       layeringViolations : None 
                   master : https://codereview.chromium.org/1151773003 (closed)
              oldKerberos : None 
          parseExperiment : None 
         removeHeaderRefs : https://codereview.chromium.org/1044733003 (closed)

Current branch: kerberos
Issue description:
  Support Kerberos on Android  As part of this, allow asynchronous
  token return. This should be allowed anyway, but is particularly
  important on Android, where getting a token may cause user
  interaction.  BUG=474943
====================================================

One possible reason. I did, some time before it landed, do a "git cl patch 1044733003" (to test a change I was making against it). I thought I had only done this on a branch, but maybe I did it on master as well. I also know I reverted this, but presumably doing a git reset doesn't change git cl's view of such relationships.

-- Sylvain

Anthony Berent

unread,
Jun 25, 2015, 5:04:23 AM6/25/15
to Ravi Mistry, Dirk Pranke, Sylvain Defresne, Chromium-dev
One further question based on this. How do I clear the issue number (without setting a new one) on a branch? git cl issue without an issue number just shows me the current status.

Pavel Sergeev

unread,
Jun 25, 2015, 5:28:55 AM6/25/15
to abe...@chromium.org, Ravi Mistry, Dirk Pranke, Sylvain Defresne, Chromium-dev
git cl issue 0

Ravi Mistry

unread,
Jun 25, 2015, 7:37:28 AM6/25/15
to Pavel Sergeev, abe...@chromium.org, Dirk Pranke, Sylvain Defresne, Chromium-dev
Right, "git cl issue 0" will clear the issue number.

In this case it appears that this is working as intended. If you have an old already closed CL patched in the tracked local branch then it shows up as a dependency but it shows up as a closed dependency (with a strikethrough) so it can safely be ignored. The messaging (about the CL being open) was a bug, I fixed this in cl/1204963005.

The change I am looking at making in the CQ is to reject a patchset if it has an open dependency. There could be situations where you do have an old unrelated open CL in a tracked branch that will cause the CQ to complain, but this could be useful information to help you clean up old cruft that may have unexpected consequences.
Also, I realize that if the CQ is going to start rejecting patchsets with open dependencies, then dependencies should also be displayed in the old UI. I will work on this as well.

Nico Weber

unread,
Jun 25, 2015, 9:13:04 PM6/25/15
to Ravi, Pavel Sergeev, Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev
For what it's worth, "master" on all my checkouts gets associated with some issue at some point, due to me accidentally running `git cl patch` while on master. Does it ever make sense to associate master with a CL? If not, maybe that branch could be special-cased?

Mattias Nissler

unread,
Jun 26, 2015, 3:14:06 AM6/26/15
to Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 3:12 AM, Nico Weber <tha...@chromium.org> wrote:
For what it's worth, "master" on all my checkouts gets associated with some issue at some point, due to me accidentally running `git cl patch` while on master. Does it ever make sense to associate master with a CL? If not, maybe that branch could be special-cased?

Hiccups like these are exactly the reason why I do "git branch -D master" first thing after I create a new checkout :)

If you really want to build current ToT, you can still do "git checkout origin/master" and build that.

Ravi Mistry

unread,
Jun 26, 2015, 9:01:04 AM6/26/15
to Nico Weber, Pavel Sergeev, Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev, Jochen Eisinger
On Thu, Jun 25, 2015 at 9:12 PM, Nico Weber <tha...@chromium.org> wrote:
For what it's worth, "master" on all my checkouts gets associated with some issue at some point, due to me accidentally running `git cl patch` while on master. Does it ever make sense to associate master with a CL? If not, maybe that branch could be special-cased?

What about this:

* If the local tracked branch is "master" then add a prompt "xyz was found as a dependency patchset on master. Would you like to add it as a dependency? [y/n]".
You can then say "n" here and fix the leftover CL in master so that it does not prompt you again for subsequent uploads. This may be annoying to users who have a real CL in master and have to keep entering "y" during all uploads.

or/and

* Add a new flag "--skip-master-deps" which will skip master. But then you will have to remember to always use this flag depending on your workflow (or add it to a git cl upload alias).

Scott Graham

unread,
Jun 26, 2015, 2:13:10 PM6/26/15
to Mattias Nissler, Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Dirk Pranke, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 12:12 AM, Mattias Nissler <mnis...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 3:12 AM, Nico Weber <tha...@chromium.org> wrote:
For what it's worth, "master" on all my checkouts gets associated with some issue at some point, due to me accidentally running `git cl patch` while on master. Does it ever make sense to associate master with a CL? If not, maybe that branch could be special-cased?

Hiccups like these are exactly the reason why I do "git branch -D master" first thing after I create a new checkout :)

+1, could we make `fetch chromium` not end up with a "master" or does something need it?

Dirk Pranke

unread,
Jun 26, 2015, 3:18:27 PM6/26/15
to Scott Graham, Mattias Nissler, Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 11:12 AM, Scott Graham <sco...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 12:12 AM, Mattias Nissler <mnis...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 3:12 AM, Nico Weber <tha...@chromium.org> wrote:
For what it's worth, "master" on all my checkouts gets associated with some issue at some point, due to me accidentally running `git cl patch` while on master. Does it ever make sense to associate master with a CL? If not, maybe that branch could be special-cased?

Hiccups like these are exactly the reason why I do "git branch -D master" first thing after I create a new checkout :)

+1, could we make `fetch chromium` not end up with a "master" or does something need it?

I believe that by default when you run `fetch chromium` you end up with an "unmanaged" git checkout and no master these days.

-- Dirk

Jochen Eisinger

unread,
Jun 26, 2015, 3:22:23 PM6/26/15
to dpr...@chromium.org, Scott Graham, Mattias Nissler, Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Sylvain Defresne, Chromium-dev
fetch blink gives me a master branch on blink.

Dirk Pranke

unread,
Jun 26, 2015, 3:30:29 PM6/26/15
to Jochen Eisinger, Aaron Gable, Scott Graham, Mattias Nissler, Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Sylvain Defresne, Chromium-dev
Yeah, that's crbug.com/289742 :). It should not give you a local master branch of chromium, though.

Apparently there's even a fix for this from agable@ that's been approved but not landed (hint, hint) :).

-- Dirk

Jochen Eisinger

unread,
Jun 26, 2015, 3:32:00 PM6/26/15
to Dirk Pranke, Aaron Gable, Scott Graham, Mattias Nissler, Nico Weber, Ravi, Pavel Sergeev, Anthony Berent, Sylvain Defresne, Chromium-dev
but ... but... I like my master branch.

Pavel Sergeev

unread,
Jun 26, 2015, 3:41:38 PM6/26/15
to Jochen Eisinger, Dirk Pranke, Aaron Gable, Scott Graham, Mattias Nissler, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
Hey, I like my local master as well. Please don't remove it.

Mattias Nissler

unread,
Jun 26, 2015, 3:45:35 PM6/26/15
to Pavel Sergeev, Jochen Eisinger, Dirk Pranke, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
Whoever wants one can keep one :)

If it's missing just do: git checkout -b master origin/master

I think the more important conclusion here should be that different people use different git workflows, so the tooling should ideally not make any heavy-weight assumptions on presence of certain branches with certain names and special case them significantly. In particular, I don't think removing master from existing checkouts would be a good idea :)

Dirk Pranke

unread,
Jun 26, 2015, 3:53:25 PM6/26/15
to Mattias Nissler, Pavel Sergeev, Jochen Eisinger, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
FWIW, I prefer master branches and managed checkouts myself. 

However, there is truth in what Mattias is saying, and the feedback from infra is that defaulting to unmanaged seems to result in less confusion (lower support costs for them).

One could imagine adding a '--managed' switch to fetch. Fetch doesn't get a lot of mods these days, but patches are always welcome :).

-- Dirk

Jochen Eisinger

unread,
Jun 26, 2015, 3:55:33 PM6/26/15
to Dirk Pranke, Mattias Nissler, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
well, the point of this thread is that git cl doesn't really support having a master branch anymore... :-/

Dirk Pranke

unread,
Jun 26, 2015, 4:07:25 PM6/26/15
to Jochen Eisinger, Mattias Nissler, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
My understanding is that a local master would only start to be a problem if it had patches associated with it. Don't do that?

Or am I missing something?

-- Dirk

Mattias Nissler

unread,
Jun 26, 2015, 4:23:46 PM6/26/15
to Dirk Pranke, Jochen Eisinger, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 10:06 PM, Dirk Pranke <dpr...@chromium.org> wrote:
My understanding is that a local master would only start to be a problem if it had patches associated with it. Don't do that?

Well, the git cl implementation assumes that if the current branch has a local upstream branch which has a CL number attached, that CL must be a change the CL on the current branch depends on. Maybe that's too strong of an assumption? I might have set the upstream pointer to simplify rebasing etc., but the CLs may actually not have code dependencies so I don't want git cl to pick up upstream as a dependency. Or I might prefer to keep upstream pointing to origin/master even though I have dependent open CLs as commits on the current branch, in which case git cl has no chance to pick up the correct dependencies.

It may make sense to have a flag to override git cl's automatic dependency detection?

FWIW, gerrit solves all this by putting a Change-Id: line into the commit message, which allows it to find dependencies by looking at commit messages of previous commits on the current branch.

Dirk Pranke

unread,
Jun 26, 2015, 4:49:40 PM6/26/15
to Mattias Nissler, Jochen Eisinger, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 1:22 PM, Mattias Nissler <mnis...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 10:06 PM, Dirk Pranke <dpr...@chromium.org> wrote:
My understanding is that a local master would only start to be a problem if it had patches associated with it. Don't do that?

Well, the git cl implementation assumes that if the current branch has a local upstream branch which has a CL number attached, that CL must be a change the CL on the current branch depends on. Maybe that's too strong of an assumption? I might have set the upstream pointer to simplify rebasing etc., but the CLs may actually not have code dependencies so I don't want git cl to pick up upstream as a dependency. Or I might prefer to keep upstream pointing to origin/master even though I have dependent open CLs as commits on the current branch, in which case git cl has no chance to pick up the correct dependencies.

Okay, but neither of those things contradicts what I wrote or makes a local master useless with the current implementation, right? (Just making sure we're on the same page).
 
It may make sense to have a flag to override git cl's automatic dependency detection?

I could agree that that might make sense.
 
FWIW, gerrit solves all this by putting a Change-Id: line into the commit message, which allows it to find dependencies by looking at commit messages of previous commits on the current branch.

I'm not familiar w/ how Gerrit solves this, but AFAICT Gerrit's model for branches and commits is quite different from Rietveld and it's not clear to me how we would map things over.

-- Dirk

Mattias Nissler

unread,
Jun 26, 2015, 4:57:17 PM6/26/15
to Dirk Pranke, Jochen Eisinger, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
On Fri, Jun 26, 2015 at 10:45 PM, Dirk Pranke <dpr...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 1:22 PM, Mattias Nissler <mnis...@chromium.org> wrote:


On Fri, Jun 26, 2015 at 10:06 PM, Dirk Pranke <dpr...@chromium.org> wrote:
My understanding is that a local master would only start to be a problem if it had patches associated with it. Don't do that?

Well, the git cl implementation assumes that if the current branch has a local upstream branch which has a CL number attached, that CL must be a change the CL on the current branch depends on. Maybe that's too strong of an assumption? I might have set the upstream pointer to simplify rebasing etc., but the CLs may actually not have code dependencies so I don't want git cl to pick up upstream as a dependency. Or I might prefer to keep upstream pointing to origin/master even though I have dependent open CLs as commits on the current branch, in which case git cl has no chance to pick up the correct dependencies.

Okay, but neither of those things contradicts what I wrote or makes a local master useless with the current implementation, right? (Just making sure we're on the same page).

Yes, totally agree with your points about master.
 
 
It may make sense to have a flag to override git cl's automatic dependency detection?

I could agree that that might make sense.
 
FWIW, gerrit solves all this by putting a Change-Id: line into the commit message, which allows it to find dependencies by looking at commit messages of previous commits on the current branch.

I'm not familiar w/ how Gerrit solves this, but AFAICT Gerrit's model for branches and commits is quite different from Rietveld and it's not clear to me how we would map things over.

The models are different indeed. My intention was merely to point out that there are other working approaches to solve the dependent CL problem that don't rely as strongly on upstream branches. I trust the infra people to figure out what the best model for our use case is.

Robert Iannucci

unread,
Jun 27, 2015, 1:42:47 PM6/27/15
to mnis...@chromium.org, Dirk Pranke, Jochen Eisinger, Pavel Sergeev, Aaron Gable, Scott Graham, Nico Weber, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
Note that you can branch off of origin/master instead of master to avoid this problem. That's precisely what `git new-branch` does (see `man depot_tools_tutorial` or open depot_tools/man/html/depot_tools_tutorial.html for an idea of how the flow using these tools looks, or `git new-branch --help` directly). If you use the recommended flow, you can always see what the tools' idea of the world looks like by running `git map-branches -vvv`.

So to recap, there are already multiple ways of avoiding having a stale/incorrect master dependency:
  * delete local master, use origin/master to view upstream HEAD and branch directly off origin/master or other feature branches for feature work (recommended)
  * have a local master, and ensure that you never start a CL off it (or run `git cl issue 0` to remove the issue from it), branch from that (or other features) to do feature work
  * have a local master, sometimes have a CL on it, but branch off origin/master (or other features) for new features

If you have a branch master (with a CL) and then another branch foo (with a CL), you're telling git (and the tools in depot_tools): "I have a series of changes called master (based on origin/master), and then another series of changes depending on it called foo". I don't think it makes sense to add a flag to the tool to tell it to ignore master in this case (i.e. "even though I told you I depend on that thing, pretend I don't"). 

It may make sense to add a flag (or a git config option) which says "Don't upload any dependency information" for alternate branch layouts, but every option we add combinatorially adds to the maintenance burden of the tools overall. I'm not completely opposed to having a single "don't do anything fancy, I'm not using the recommended flow so my branches don't mean what you think they do" flag which all the tools use before they try to infer structure from the branch/commit layout though... at least in that world the recommended flow could gain new features without breaking people who have opted into their own thing (Ravi's change here would be an example of that).  Our team isn't very big, and we don't get a lot of contributions from outside the team (but we're extremely grateful for the ones we do!), so more options hurts our ability to ensure that the entire toolchain works well with itself (self-consistency).

I think adding another way to tell the tools to ignore the content of master is appealing mainly for the reason of "changing workflow is annoying"[1], but I don't think in the grand scheme of things it's really good for the tools (higher maintenance), for chromium overall (adds complexity for every developer, current and incoming. `git cl upload --help` already takes an alarming amount of screen real estate), or necessarily even for those asking for the flag (only you can be the judge of that, but such a new flag would probably be prone to breakage or misinterpretation over time because the high degree of interdependency between the tools which assume the repo to be in a certain state).

@someone re: gerrit: The way that gerrit handles dependencies is that every commit is a CL, and a chain of commits means dependencies between the CLs. To modify a CL, you --amend the commit, and rebase everything else on top of it. There are no flags/modes/options to change that behavior, and everyone used to interacting with gerrit (both uploaders and reviewers), understands that relationship.

Anyway... that's my 2 cents.
R

[1]: oblig, don't hate me, pls: https://xkcd.com/1172/

Nico Weber

unread,
Jun 27, 2015, 9:16:27 PM6/27/15
to Robert Iannucci, Mattias Nissler, Dirk Pranke, Jochen Eisinger, Pavel Sergeev, Aaron Gable, Scott Graham, Ravi, Anthony Berent, Sylvain Defresne, Chromium-dev
On Sat, Jun 27, 2015 at 10:41 AM, Robert Iannucci <iann...@chromium.org> wrote:
Note that you can branch off of origin/master instead of master to avoid this problem. That's precisely what `git new-branch` does (see `man depot_tools_tutorial` or open depot_tools/man/html/depot_tools_tutorial.html for an idea of how the flow using these tools looks, or `git new-branch --help` directly). If you use the recommended flow, you can always see what the tools' idea of the world looks like by running `git map-branches -vvv`.

So to recap, there are already multiple ways of avoiding having a stale/incorrect master dependency:
  * delete local master, use origin/master to view upstream HEAD and branch directly off origin/master or other feature branches for feature work (recommended)
  * have a local master, and ensure that you never start a CL off it (or run `git cl issue 0` to remove the issue from it), branch from that (or other features) to do feature work
  * have a local master, sometimes have a CL on it, but branch off origin/master (or other features) for new features

Neither of those match my workflow.

I don't know git well. My (incomplete, incorrect, simplified) view of the world is that master is the upstream code (unless I screw something up – when things look off, I run `git reset --hard origin/master`) and my local branches are my changes.

If I had no local master branch, I wouldn't know how to sync (I run `git checkout master; git pull` for that). I always branch off master for working on features. master is associated with some random CL because of me not being careful in every of my checkouts that I've checked so far (on this machine, it's apparently https://codereview.chromium.org/429793006).
 
If you have a branch master (with a CL) and then another branch foo (with a CL), you're telling git (and the tools in depot_tools): "I have a series of changes called master (based on origin/master), and then another series of changes depending on it called foo". I don't think it makes sense to add a flag to the tool to tell it to ignore master in this case (i.e. "even though I told you I depend on that thing, pretend I don't"). 

It may make sense to add a flag (or a git config option) which says "Don't upload any dependency information" for alternate branch layouts, but every option we add combinatorially adds to the maintenance burden of the tools overall. I'm not completely opposed to having a single "don't do anything fancy, I'm not using the recommended flow so my branches don't mean what you think they do" flag which all the tools use before they try to infer structure from the branch/commit layout though... at least in that world the recommended flow could gain new features without breaking people who have opted into their own thing (Ravi's change here would be an example of that).  Our team isn't very big, and we don't get a lot of contributions from outside the team (but we're extremely grateful for the ones we do!), so more options hurts our ability to ensure that the entire toolchain works well with itself (self-consistency).

I think adding another way to tell the tools to ignore the content of master is appealing mainly for the reason of "changing workflow is annoying"[1], but I don't think in the grand scheme of things it's really good for the tools (higher maintenance), for chromium overall (adds complexity for every developer, current and incoming. `git cl upload --help` already takes an alarming amount of screen real estate), or necessarily even for those asking for the flag (only you can be the judge of that, but such a new flag would probably be prone to breakage or misinterpretation over time because the high degree of interdependency between the tools which assume the repo to be in a certain state).

I don't want more options either, I agree there are too many. But it'd be nice if new features (that solve problems I'm not sure I'm having) wouldn't break workflows that have worked fine for years.

Thiago Farina

unread,
Jun 27, 2015, 11:14:45 PM6/27/15
to tha...@chromium.org, Robert Iannucci, Chromium-dev


On Saturday, June 27, 2015, Nico Weber <tha...@chromium.org> wrote:
On Sat, Jun 27, 2015 at 10:41 AM, Robert Iannucci <iann...@chromium.org> wrote:
Note that you can branch off of origin/master instead of master to avoid this problem. That's precisely what `git new-branch` does (see `man depot_tools_tutorial` or open depot_tools/man/html/depot_tools_tutorial.html for an idea of how the flow using these tools looks, or `git new-branch --help` directly). If you use the recommended flow, you can always see what the tools' idea of the world looks like by running `git map-branches -vvv`.

So to recap, there are already multiple ways of avoiding having a stale/incorrect master dependency:
  * delete local master, use origin/master to view upstream HEAD and branch directly off origin/master or other feature branches for feature work (recommended)
  * have a local master, and ensure that you never start a CL off it (or run `git cl issue 0` to remove the issue from it), branch from that (or other features) to do feature work
  * have a local master, sometimes have a CL on it, but branch off origin/master (or other features) for new features

Neither of those match my workflow.

I don't know git well. My (incomplete, incorrect, simplified) view of the world is that master is the upstream code (unless I screw something up – when things look off, I run `git reset --hard origin/master`) and my local branches are my changes.

If I had no local master branch, I wouldn't know how to sync (I run `git checkout master; git pull` for that). I always branch off master for working on features. master is associated with some random CL because of me not being careful in every of my checkouts that I've checked so far (on this machine, it's apparently https://codereview.chromium.org/429793006).
You can sync just fine without your master branch. Master branch is no special than any other branch you create. You can do `git checkout -b work origin/master; git branch -D master; git pull --rebase` and just live fine with that. 
 


--
Thiago Farina

Robert Iannucci

unread,
Jun 28, 2015, 3:24:55 PM6/28/15
to Thiago Farina, tha...@chromium.org, Chromium-dev
What Thiago said is correct.

Essentially what you have is:

  <googlesource.com> -> origin/master -> master -> feature_branch [ -> other_feature]
  # where master can get ahead/behind origin/master with accidental commits, and forces you to do two levels of rebases on every update

and infra recommends

  <googlesource.com> -> origin/master -> feature_branch [ -> other_feature]

Creating a new branch is `git checkout -b work origin/master` (or just `git new-branch work` if you're using depot tools, `git new-branch --upstream other_feature work` for dependent changes).

Pulling code from googlesource into your working branch is `git pull --rebase` if you're based on origin/master.

R

P.S. We also wrote a tool in depot_tools called `git rebase-update` which will manage all your local branches for you. It:
  * freezes the current branch you're on (e.g. stashes uncommitted changes)
  * pulls the latest code from googlesource.com
  * rebases all your local branches on the new code (in dependency order, if you have nested changes, it knows how to walk the dependency tree)
  * if some of your branches were committed upstream (e.g. with CQ or land), it automatically prunes them and rebases any dependent branches on the committed branch's parent.
  * checks out the original branch you were on and thaws it out (restoring work-in-progress changes)

I'm guessing that you like doing all that stuff manually however ;-). It can unfortunately be a bit slow for chormium/src (since it's a huge repo), but it's still faster than doing all the steps manually.

John Mellor

unread,
Jun 30, 2015, 1:05:44 PM6/30/15
to Robbie Iannucci, Thiago Farina, tha...@chromium.org, Chromium-dev
Can't the tool be smarter, and treat local upstream branches as dependencies, only if the commits on the local branch aren't already present in origin/master?

Specifically, if `git cherry origin/master BRANCH` has empty output, then don't mark BRANCH (or its upstreams) as a dependency (note that git cherry is smart enough to understand rebased/cherry-picked patches).

[FWIW, I also prefer having a local master branch, since it acts as a kind of personal lkgr ensuring that all my branches fork off origin/master at the same point. Hence switching branches is quick (less to rebuild) and safe (less chance of a broken build). If you base branches off origin/master, and use `git pull --rebase`, they'll end up forking off origin/master at different points, and switching branches becomes slower/riskier. Using `git rebase-update` would solve that, but forces you to solve merge conflicts for branches you're not actively working on...]

Ravi

unread,
Jul 14, 2015, 7:59:00 AM7/14/15
to chromi...@chromium.org, joh...@chromium.org, tfa...@chromium.org, iann...@chromium.org, tha...@chromium.org

Updates


There is now a way to skip dependency checks and uploads for specific local branches by running (using "test" as a branch name):

git config branch.test.skip-deps-uploads True


To undo the skipping:

git config --unset branch.test.skip-deps-uploads


To do the above commands globally (across all checkouts):

git config --global branch.test.skip-deps-uploads True

git config --global --unset branch.test.skip-deps-uploads


We decided on the above approach in cl/1210903005.


Next steps


The CQ will start rejecting CLs with open patchset dependencies. I plan to turn this on tomorrow.


Similar to the other CQ checks, you will be able to skip the open dependency check by using "NO_DEPENDENCY_CHECKS=true" in your CL’s description. Open dependency checks will be automatically skipped for dry runs.


After this check is in place in the CQ, apply_issue.py will be modified to apply all dependencies of a patchset. This will allow trybots to successfully test your partial diff uploaded due to tracked local branches (crbug/480453).



To recap, there will be two ways to skip the dependency checks:

* Use the git config settings mentioned above to skip dependency uploads of specific branches (eg: master). This is already live.

* Use the new keyword "NO_DEPENDENCY_CHECKS=true" to skip the CQ open dependency check on a per CL basis. This will be live tomorrow.
Reply all
Reply to author
Forward
0 new messages