A more complete explanation of the failure case in https://chromium-review.googlesource.com/c/angle/angle/+/666044 :

Patch Set 12: This change is ready for review.
Uploaded patch set 13: Patch Set 12 was rebased.
Patch Set 13: This change is ready for review.

if you don't specify
   --send-mail | -s 
then change becomes WIP. However, non-squash mode isn't supported by us, so it's possible that the despite --send-mail being set, the refspec is still incorrect for non-squash mode.

Sending to agable@ to triage.

Yep, angle sets "GERRIT_SQUASH_UPLOADS: False" in its codereview.settings. This is a Not Good thing that we would like to get rid of, since maintaining both workflows makes everything insanely complicated.

However, my reading of git-cl-upload[1] shows that the %wip refspec opt should be set equally on squashed and non-squashed uploads. So maybe Gerrit itself is doing something wrong here?


[1] https://chromium.googlesource.com/chromium/tools/depot_tools/+/d325eb349608f54d84a74ce124331a3fa170e5dc/git_cl.py#3035

Does this code not work with non-squared?

      if not self.GetIssue():
        refspec_opts.append('wip')

self.GetIssue might use some of the magic variable setting that only applies to squashed? In any case, thanks if you can look into this.

Regarding workflow, the ANGLE team worked with infra prior to the Gerrit transition to ensure our non-squash workflow could be accommodated, because it's important to how we work. Our choice to not squash uploads is deliberate, and we request that support for it not be abandoned. 

non-squashed*

Ah yes of course, good eye. When a branch is associated with multiple issues (due to a non-squashed upload), git-cl doesn't save all of those issues to .git/config, it saves none of them. So on a subsequent upload, self.GetIssue() would still be false, and %wip would be reapplied.

I understand that non-squash has been part of angle's workflow for a long time, and that we made effort to support it. What I'm asking now is:
1) *why* is it important to angle? All other chromium projects on chromium.googlesource.com do not use that workflow, and angle uses the Rebase Always submit strategy which breaks links between changes uploaded together anyway.
2) *what* can we do to encourage or allow angle to move to a more unified workflow?

Well, the primary reason ANGLE's workflow is better than Chromium's is because we can upload dependent stacks of patches from a single branch. For instance, in my local git repo, I might have three branches, for different tasks, and in each branch, I might have 2-15 patches that depend on each other. I can easily jump to a particular patch in a particular branch, fix a comment, then jump to the end and upload the whole patch stack with a single invocation of 'git cl upload'. Reviewers can also see the patch stack in the relation chain in Gerrit. When we submit changes, the dependency is broken when they land via cherry-pick, but it exists up until the very moment of submission.

This workflow is a bit more git-like than the legacy workflow inherited by Chromium. I've spoken to co-workers who primarily work in chrome-src and they have mentioned it would be useful to upload patch stacks without squashing as well, because they would not have to make separate branches for each dependent patch. It's even possible to do this if you know the right magic URL to push your gerrit refs to, but you lose the magic of git cl.

I don't know if it's practical for us to switch to the same workflow, I think I myself would lose quite a bit of productivity as I use very large sequences of patches (sometimes over 20 patches). I guess you could unify the workflows by switching Chromium to use non-squashed mode, but I understand why you choose to preserve this behaviour from Reitvelt.

Maybe in order to address this issue, you could add a mode to Gerrit that's "if first upload, mark as WIP, otherwise, don't change the status of the WIP bit"?

I understand your workflow (and I understand that it can be difficult to change workflows, just due to muscle memory) but I don't understand how it is better or more efficient than the squashed workflow supported by git-cl.

You say that it is nice to have 2-15 changes on a branch, and to upload all of them separately. But that means that each of those 2-15 commits has to be review-ready: well-formed, no lint errors, all the tests pass, etc. Do you never get 7 commits down your stack, and then realize that really TestFoo should have been updated along with the 2nd commit, and now you have to go into a detached-HEAD state, amend that commit, rebase the commits that are now missing a parent, and fixup your branch pointer? What about when you *don't* realize that, but then one of the early commits gets review comments and you have to modify it? Do you upload new versions of all of the commits based on it as well, even though those are all just rebases that would be taken care of anyway during the Submit process?

The alternative is to use the default squashed workflow and have stacked branches. For example, I currently have a set of branches stacked 8 deep (and it was 14 deep yesterday, before I landed the first changes in the stack):
± git map-branches -vvv
origin/master                  afb87de13                
  rveld-edit                   0085cdd49 [ ahead 7 ]    https://chromium-review.googlesource.com/721846 (lgtm)
    rveld-pset                 b5f8eca2e [ ahead 3 ]    https://chromium-review.googlesource.com/721847 (lgtm)
      rveld-pset-revert        bb8f60c62 [ ahead 2 ]    https://chromium-review.googlesource.com/721251 (lgtm)
        rveld-upload           4c8766b84 [ ahead 1 ]    https://chromium-review.googlesource.com/721855 (lgtm)
          rveld-email          071d11a64 [ ahead 4 ]    https://chromium-review.googlesource.com/724022 (lgtm)
            rveld-post         028ec6549 [ ahead 1 ]    https://chromium-review.googlesource.com/723855 (waiting)
              rveld-xmpp       64331fd42 [ ahead 1 ]    https://chromium-review.googlesource.com/723856 (waiting)
                rveld-rss *    3ad541ad0 [ ahead 1 ]    https://chromium-review.googlesource.com/723612 (waiting)
But as you can see, most of those branches has multiple commits in it. Some of those commits are "whoops, accidentally broke a test, fixed it now". But those don't show up when I upload the branch for review, because the branch gets squashed. In addition, if I get review comments on a change, I can just check out that branch, create a new commit, and git-cl upload again. No amending, no detached-head, no rebasing. If I *do* want to re-upload everything that depends on the branch I modified, I can just do "git cl upload --dependencies".

We also offer tools to make managing these stacked branches easier: git new-branch to start a new stack, git new-branch --track to start the next branch down the chain, git rebase-update to update all of your branches and get rid of the ones which have been submitted, git cl archive to get rid of submitted branches without doing the rebasing dance, git map-branches to display them like above, and more. None of those tools are guaranteed to work as expected using a non-squashed workflow.

I think you would find that if you switched to the non-squashed workflow, although it would take an extra command to start a new change (git checkout -tb something) instead of just editing and committing, you'll find that jumping around between changes, responding to review comments, and landing just a subset of the changes will all become much easier and more efficient.

There are benefits to reviewers and maintainers from non-squashed patch stacks. For reviewers, being able to review an isolated incremental CL means being able to approve it without *also* being ready to sign off on all parts of an umptymillion line larger change, but Gerrit allows that it can still be seen in the context of those other changes.

One of the places this is very useful is in making it easier to separate simple renaming or minor refactoring changes from a serious functional change. Reviewing those squashed is a headache, and making developers do renaming or refactoring as a wholly separate task tends to mean that it gets disincentivized and never gotten around to.

Another benefit of the incremental stacked patch workflow is that bisects can identify more narrow windows in which failures occur, and reverts may be done on very small changes.

This workflow was one of the primary reasons we moved to gerrit ahead of the rest of Chromium-- we worked closely with cmp@ and team at the time to figure out what would work best both for infra and for ANGLE, and gerrit was able to support this, while rietveld could not.

There is nothing about the non-squashed workflow which prevents small patches; as seen above many of my branches are also still single commits.

The whole point of the squash workflow is that it provides more flexibility: it is easier to work with and manipulate branches which may consist of one or many commits than it is to manipulate necessarily-single commits along a single branch.

If people abuse the squash workflow to create huge changes that are too large to review, reviewers can simply ask them to break the change up. There's nothing preventing someone from creating a single commit which is as large as these theoretical large branches, and if someone *does* make a review which is too large, it's easier to break a branch with multiple commits up into smaller reviews than it is to break a single large commit up into multiple reviews.

And I dispute the statement that it makes separating renaming or refactoring changes easier. With the squash workflow, a developer can land as many ugly commits as they like -- refactoring files one at a time, checkpointing their work when they go home for the evening, etc -- and then just use 'git rebase -i' to order those commits into sane sequences which can be uploaded for review without losing the granularity of their local changes.

At the end of the day, I'm not philosophically opposed to supporting both workflows. In fact, I personally like the idea of providing more and better support for the non-squashed workflow, and integrating more directly with Gerrit. But a) I have yet to hear any advantages of the non-squashed workflow that the squashed workflow doesn't in fact provide, and b) I'm working with very limited (and shrinking!) resources to support git-cl. If there's any way I can reduce the support surface area, I need to do that; removing support for non-squashed would let me close a large handful of bugs and delete a couple hundred lines of code and exceptional cases.

With the squash workflow, those renaming and refactoring changes still get squashed in with the functional changes when uploaded for review, unless they're made on a separate branch. The benefit there is to the reviewer.

I'm not sure I'm following the argument about flexibility-- you're saying it's easier to work with several branches of one or more commits which aren't preserved on upload than it is to manipulate branches of one or more commits which are preserved on upload?  As you mention, local commits can be reordered with rebase -i (or tools like StGit), so I'm not sure what flexibility is added? As jmadill@ describes his workflow, he already keeps separate branches per-task, with multiple commits per branch. Squash workflow changes nothing, if I understand correctly, except that the reviewer and the canonical repository no longer see discrete CLs. It's a net loss of information, no?

I understand that you're working with limited resources-- we have generally been as well-- and really don't want to make your life more difficult! We're just trying to preserve a workflow that has been important to us, and that we were really careful to vet before integrating with the rest of Chrome.

The flexibility comes from the fact that you can have multiple commits per branch but you can't have multiple branches per commit. In other words, you are welcome to keep developing exactly as you always have -- just label each of your commits with a branch and upload them separately (or upload them all at the same time with "git cl upload --dependencies"). Basically, it is possible to make the squash workflow behave like your current workflow, just by tagging each commit with a branch. The reverse is not true: it's impossible to make the no-squash workflow behave like the squash workflow.

The squash workflow lets you have commits which aren't standalone reviewable. Commits that just fix lints, or commits which fix functionality and tests in two separate commits, or commits which just respond to review comments. The no-squash workflow precludes you from having those kinds of WIP commits locally, or forces you to clean them up manually before review.

Those are the two big ways in which the squash workflow is inherently more flexible than no-squash.

In order to get the squashed workflow to produce the same end-result as our current workflow, though, we're forced to move to single-commit-per-branch. If we have multiple commits on a branch, they get squashed at upload and can no longer be bisected between or reverted incrementally. StGit and the like would not work on our dependency chains if they're split across branches, but it sounds like git cl has its own notion of dependency between branches at this point, so it's possible that might be equivalent. We also lose the branch-per-topic separation that we have, IIUC, since it becomes branch-per-change. Does the dependency between branches get reflected on the gerrit review page, or are they treated as wholly separate?

We already have commits that fix functionality and tests in separate commits-- they're not required to be atomic for us. And fixup patches are what rebase autosquash is made for, aren't they? But if we're spreading our commits one-per-branch to avoid squashed history, then I'm not sure default squash on upload saves us anything other than actually physically typing "git squash" once, which is no more of a benefit than an extra command to start a dependent branch is a loss. 

So from the POV of our workflow, we either lose local branch-per-topic and the ability to use some tools, or we lose revert- and bisect-ability to incremental CLs. In return, we gain the ability to leave fixup CLs locally unsquashed, which I'm not sure would be useful to anyone on our team, and which is not that helpful for commits spread one-per-branch.

 Issue 770252  has been merged into this issue.

Sorry for the slow response, I've been meaning to try and test out the git cl workflow you've described to see if it's flexible enough to accommodate the kinds of things I do with my workflow. I will say I don't know where it is documented, which is a bit of a problem. For instance, now I have a stack of 13 patches that would be good to test with, but I don't know how to access the right info.

The commands I would usually do are:

- add new patches to the stack, at any position (including the middle)
- delete patches from the stack, from any position (including the middle)
- go to a specific patch (this should be checkout I would think)
- reorder patches with push, pop (pop can reorder by removing a prior patch, pop similarly), float and sink (which float patches to the top or sink to bottom)
- rebase the whole stack (I think you covered this) and resolve conflicts in turn
- upload the whole stack (again, I think you covered this)
- amend the description of a patch via the command line for the next full upload
- list patches in a stack
- list patches in separate stacks
- upload a full stack to a different remote for easy transfer between machines

All of these I can do quite efficiently now. As Shannon was saying, the squashed mode is not a useful for us, as we're used to Git and "upload is what you get" in your CL. If I do want access to the reflog, the tool I use for stacked patch management makes this easy for me, but I think I've maybe used that once or twice over my tenure at Google. Moreover there are also prior revisions of the patch if they're uploaded to Gerrit, which my workflow makes quite trivial.

To answer your earlier questions, our currently workflow does preserve the dependency chain in Gerrit, which is good for reviewers to quickly jump between reviews in large stacks. Most importantly it allows us to use the trybots on any dependent patch. The try jobs will inherit any failures or fixes from previous patches, which is another reason why I like to upload full stacks to Gerrit quite often, and I can't think of another way of doing this.

I will try to find some time to try the new methods you added, although if you're short on time as well, I think for now just fixing the start review problem with non-squashed up would keep us happy and productive. It's become a recurring problem, and seems to affect external contributors (reviews idling for multiple days).

Is `-s` currently the best work around for using `--no-squash`?

I feel like there's a fundamental misunderstanding going on here. When git-cl upload squashes your patch for upload, it *does not touch* the work you have locally on your branch. It squashes it in the background, creating a hidden commit object that only it knows the sha1 of, and uploads that. Your local work is untouched, and you can keep doing all of your normal operations (amending, floating, sinking, (although those are local aliases you've created, not part of a normal git workflow), etc.) as normal. When you upload the next patch, it knows which review that whole branch had been uploaded to previously, squashes again in the background, and uploads to the same place, again leaving your local work untouched.

Some of the other comments confuse me. For example, the comment saying that "multiple commits on a branch... can no longer be bisected between or reverted incrementally". Yes, of course, because the multiple commits which are on your local branch are local work that the rest of the world doesn't need to see. Like "fix that dumb typo".

At the end of the day, yes, it would require you to switch to "single-commit-per-branch". But only "single-commit" in the sense of "single commit which lands in the final repository", not "single commit at all ever". The squash workflow frees you to have as many or as few commits as you like on each branch, to have your local workspace as messy or as clean as you like with no need to keep every single commit message perfectly formatted and ordered.

The squash workflow handles gerrit dependencies exactly like the unsquashed workflow: local branches which are based on each other get uploaded as single commits which are based on each other, just like your individual commits do today. The trybots treat them exactly the same as well.

Finally, yes, "-s" will always cause the review to be started.

Removing myself from all Pri-2/3 Gerrit issues and setting them to Available, to more accurately reflect the fact that there are a bunch of Pri-1 bugs open, and I'm splitting my time between codereview and codesearch.

To my understanding, it is possible to summarize Jamie's requirements from #15 as follows:
1. Have a "simple way" (one short command line) to arbitrarily reorder dependencies between a stack of Gerrit CLs (including adding new CLs and removing old CLs from a stack).
2. Be able to merge 2 Gerrit CLs into 1.
3. Have a "simple way" (one that doesn't require editing in several places) of managing Gerrit CL descriptions.

Re #17 "you can keep doing all of your normal operations ... as normal" - these operations are easily done on git commits, but very hard up to impossible to do on git branches.

Looks like I've missed:
4. Be able to recreate the dependency stack of Gerrit CLs in a clean local git checkout.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/276da0b4d90aad69aa0adcf3e6a792b5e364c1ee

commit 276da0b4d90aad69aa0adcf3e6a792b5e364c1ee
Author: Jamie Madill <jmadill@chromium.org>
Date: Tue May 08 14:25:01 2018

git cl: Do not set WIP when using no_squash.

When using non-squashed uploads, there usually isn't an associated
issue with a branch. Thus setting WIP when the issue tag wasn't
present would continually set WIP even on trivial rebases. Not
adding the WIP tag when uploading allows the user to set WIP manually,
while not forcing the WIP flag after every new upload.

Also updates the unit tests that use no-squash.

Bug:  767439 
Change-Id: I990a82139fefe1a0c5c7b149d39045cf985539b7
Reviewed-on: https://chromium-review.googlesource.com/1033187
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/276da0b4d90aad69aa0adcf3e6a792b5e364c1ee/tests/git_cl_test.py
[modify] https://crrev.com/276da0b4d90aad69aa0adcf3e6a792b5e364c1ee/git_cl.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb1156b991fc5d3feaf54498ea1ec935316b5852

commit fb1156b991fc5d3feaf54498ea1ec935316b5852
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Tue May 08 19:01:07 2018

Roll src/third_party/depot_tools/ 5ae86d202..276da0b4d (1 commit)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/5ae86d202127..276da0b4d90a

$ git log 5ae86d202..276da0b4d --date=short --no-merges --format='%ad %ae %s'
2018-04-27 jmadill git cl: Do not set WIP when using no_squash.

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:767439 


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: Ifd085ed96d5ee4b37679c9d16faf171da45f2d1a
Reviewed-on: https://chromium-review.googlesource.com/1049955
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#556907}
[modify] https://crrev.com/fb1156b991fc5d3feaf54498ea1ec935316b5852/DEPS

Will check back in a month to see if there's any progress on the  issue gerrit:8866 

The NextAction date has arrived: 2018-06-08

No updates yet, will check back in another month.

The NextAction date has arrived: 2018-07-08

Thanks to some very appreciated contributions to Gerrit there is now a user and project setting to default new changes to WIP.

Andrii is it possible to enable this setting for the ANGLE Gerrit separately from Chromium? We can ask new users to enable it manually. Just wondering if there is a systematic way we can enable the setting for ANGLE.

I don't know  -- this question is beyond my knowledge of Gerrit.
I recommend you ask this on g/gerritcodereview-users

Never mind, I found it. Thanks! Closing this out.