Add ability to skip dependency checks and uploads for particular branches

Add ability to skip dependency checks and uploads for particular branches.

After this CL is in users will be able to skip dependency checks and uploads for specific branches locally by running (using test2 as a branch name):
git config branch.test2.skip-deps-uploads True
To undo the skipping:
git config --unset branch.test2.skip-deps-uploads

To do the above commands globally (across all checkouts):
git config --global branch.test2.skip-deps-uploads True
git config --global --unset branch.test2.skip-deps-uploads


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

[rmistry, 2015-06-29 13:07:22]

rmistry@google.com changed reviewers:
+ agable@chromium.org, iannucci@chromium.org

[rmistry, 2015-06-29 13:07:23]

Robbie, I 100% agree with your comments in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/nZbX5...
about how options to opt out adds to the maintenance burden of the tools
overall.

I plan to soon make the CQ fail patchsets if there is an open dependency. I
expect that CQ change to cause many complains (as we have already seen in the
chromium-dev thread) and issues being opened unless there is a way to skip the
dependency uploads to make different workflows continue to submit as before into
the CQ.

[iannucci, 2015-06-29 16:46:07]

On mobile, but you can make the food smaller by doing 'if skip: return'
instead of the else. Well take a look in a bit.

Can we change the option to non-standard-branch-layout? I don't want one of
these for every improvement made. Then the directions can day "do it this
way, but if you have a workflow that had worked for years and you don't
want to change at this option and we'll let you do everything manually", or
something to that effect?

On Mon, Jun 29, 2015, 06:07  <rmistry@google.com> wrote:

> Reviewers: iannucci, agable,
>
> Message:
>
> Robbie, I 100% agree with your comments in
>
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/nZbX5...
> about how options to opt out adds to the maintenance burden of the tools
> overall.
>
> I plan to soon make the CQ fail patchsets if there is an open dependency. I
> expect that CQ change to cause many complains (as we have already seen in
> the
> chromium-dev thread) and issues being opened unless there is a way to skip
> the
> dependency uploads to make different workflows continue to submit as before
> into
> the CQ.
>
> Description:
> Add ability to be able to skip dependency checks and uploads.
>
> After this CL is in users will be able to skip dependency checks and
> uploads
> locally by running:
> git config rietveld.skip-deps-uploads True
> To undo the skipping:
> git config --unset rietveld.skip-deps-uploads
>
> To do the above commands globally (across all checkouts):
> git config --global rietveld.skip-deps-uploads True
> git config --global --unset rietveld.skip-deps-uploads
>
>
> BUG=chromium:504832
>
> Please review this at https://codereview.chromium.org/1210903005/
>
> Base URL:
> https://chromium.googlesource.com/chromium/tools/depot_tools.git@master
>
> Affected files (+30, -21 lines):
>    M git_cl.py
>
>
> Index: git_cl.py
> diff --git a/git_cl.py b/git_cl.py
> index
>
>
f99f5e99a269bcaafa9b1bae48a65941e9fdea5f..cb119a07c34c4959f8bfcdfd3139a918922fb34a
> 100755
> --- a/git_cl.py
> +++ b/git_cl.py
> @@ -554,6 +554,9 @@ class Settings(object):
>     def GetBugPrefix(self):
>       return self._GetRietveldConfig('bug-prefix', error_ok=True)
>
> +  def GetIsSkipDependencyUploads(self):
> +    return self._GetRietveldConfig('skip-deps-uploads', error_ok=True)
> +
>     def GetRunPostUploadHook(self):
>       run_post_upload_hook = self._GetRietveldConfig(
>           'run-post-upload-hook', error_ok=True)
> @@ -2185,27 +2188,33 @@ def RietveldUpload(options, args, cl, change):
>       if target_ref:
>         upload_args.extend(['--target_ref', target_ref])
>
> -    # Look for dependent patchsets. See crbug.com/480453 for more
> details.
> -    remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
> -    upstream_branch = ShortBranchName(upstream_branch)
> -    if remote is '.':
> -      # A local branch is being tracked.
> -      local_branch = ShortBranchName(upstream_branch)
> -      auth_config = auth.extract_auth_config_from_options(options)
> -      branch_cl = Changelist(branchref=local_branch,
> auth_config=auth_config)
> -      branch_cl_issue_url = branch_cl.GetIssueURL()
> -      branch_cl_issue = branch_cl.GetIssue()
> -      branch_cl_patchset = branch_cl.GetPatchset()
> -      if branch_cl_issue_url and branch_cl_issue and branch_cl_patchset:
> -        upload_args.extend(
> -            ['--depends_on_patchset', '%s:%s' % (
> -                 branch_cl_issue, branch_cl_patchset)])
> -        print
> -        print ('The current branch (%s) is tracking a local branch (%s)
> with '
> -               'an associated CL.') % (cl.GetBranch(), local_branch)
> -        print 'Adding %s/#ps%s as a dependency patchset.' % (
> -            branch_cl_issue_url, branch_cl_patchset)
> -        print
> +    if settings.GetIsSkipDependencyUploads():
> +      print
> +      print ('Skipping dependent patchsets check because git config '
> +             'rietveld.skip-deps-uploads is set to True.')
> +      print
> +    else:
> +      # Look for dependent patchsets. See crbug.com/480453 for more
> details.
> +      remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
> +      upstream_branch = ShortBranchName(upstream_branch)
> +      if remote is '.':
> +        # A local branch is being tracked.
> +        local_branch = ShortBranchName(upstream_branch)
> +        auth_config = auth.extract_auth_config_from_options(options)
> +        branch_cl = Changelist(branchref=local_branch,
> auth_config=auth_config)
> +        branch_cl_issue_url = branch_cl.GetIssueURL()
> +        branch_cl_issue = branch_cl.GetIssue()
> +        branch_cl_patchset = branch_cl.GetPatchset()
> +        if branch_cl_issue_url and branch_cl_issue and branch_cl_patchset:
> +          upload_args.extend(
> +              ['--depends_on_patchset', '%s:%s' % (
> +                   branch_cl_issue, branch_cl_patchset)])
> +          print
> +          print ('The current branch (%s) is tracking a local branch (%s)
> with '
> +                 'an associated CL.') % (cl.GetBranch(), local_branch)
> +          print 'Adding %s/#ps%s as a dependency patchset.' % (
> +              branch_cl_issue_url, branch_cl_patchset)
> +          print
>
>     project = settings.GetProject()
>     if project:
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2015-06-29 16:46:20]

Er.... diff smaller.

On Mon, Jun 29, 2015, 09:45 Robert Iannucci <iannucci@chromium.org> wrote:

> On mobile, but you can make the food smaller by doing 'if skip: return'
> instead of the else. Well take a look in a bit.
>
> Can we change the option to non-standard-branch-layout? I don't want one
> of these for every improvement made. Then the directions can day "do it
> this way, but if you have a workflow that had worked for years and you
> don't want to change at this option and we'll let you do everything
> manually", or something to that effect?
>
> On Mon, Jun 29, 2015, 06:07  <rmistry@google.com> wrote:
>
>> Reviewers: iannucci, agable,
>>
>> Message:
>>
>> Robbie, I 100% agree with your comments in
>>
>>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/nZbX5...
>> about how options to opt out adds to the maintenance burden of the tools
>> overall.
>>
>> I plan to soon make the CQ fail patchsets if there is an open dependency.
>> I
>> expect that CQ change to cause many complains (as we have already seen in
>> the
>> chromium-dev thread) and issues being opened unless there is a way to skip
>> the
>> dependency uploads to make different workflows continue to submit as
>> before
>> into
>> the CQ.
>>
>> Description:
>> Add ability to be able to skip dependency checks and uploads.
>>
>> After this CL is in users will be able to skip dependency checks and
>> uploads
>> locally by running:
>> git config rietveld.skip-deps-uploads True
>> To undo the skipping:
>> git config --unset rietveld.skip-deps-uploads
>>
>> To do the above commands globally (across all checkouts):
>> git config --global rietveld.skip-deps-uploads True
>> git config --global --unset rietveld.skip-deps-uploads
>>
>>
>> BUG=chromium:504832
>>
>> Please review this at https://codereview.chromium.org/1210903005/
>>
>> Base URL:
>> https://chromium.googlesource.com/chromium/tools/depot_tools.git@master
>>
>> Affected files (+30, -21 lines):
>>    M git_cl.py
>>
>>
>> Index: git_cl.py
>> diff --git a/git_cl.py b/git_cl.py
>> index
>>
>>
f99f5e99a269bcaafa9b1bae48a65941e9fdea5f..cb119a07c34c4959f8bfcdfd3139a918922fb34a
>> 100755
>> --- a/git_cl.py
>> +++ b/git_cl.py
>> @@ -554,6 +554,9 @@ class Settings(object):
>>     def GetBugPrefix(self):
>>       return self._GetRietveldConfig('bug-prefix', error_ok=True)
>>
>> +  def GetIsSkipDependencyUploads(self):
>> +    return self._GetRietveldConfig('skip-deps-uploads', error_ok=True)
>> +
>>     def GetRunPostUploadHook(self):
>>       run_post_upload_hook = self._GetRietveldConfig(
>>           'run-post-upload-hook', error_ok=True)
>> @@ -2185,27 +2188,33 @@ def RietveldUpload(options, args, cl, change):
>>       if target_ref:
>>         upload_args.extend(['--target_ref', target_ref])
>>
>> -    # Look for dependent patchsets. See crbug.com/480453 for more
>> details.
>> -    remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
>> -    upstream_branch = ShortBranchName(upstream_branch)
>> -    if remote is '.':
>> -      # A local branch is being tracked.
>> -      local_branch = ShortBranchName(upstream_branch)
>> -      auth_config = auth.extract_auth_config_from_options(options)
>> -      branch_cl = Changelist(branchref=local_branch,
>> auth_config=auth_config)
>> -      branch_cl_issue_url = branch_cl.GetIssueURL()
>> -      branch_cl_issue = branch_cl.GetIssue()
>> -      branch_cl_patchset = branch_cl.GetPatchset()
>> -      if branch_cl_issue_url and branch_cl_issue and branch_cl_patchset:
>> -        upload_args.extend(
>> -            ['--depends_on_patchset', '%s:%s' % (
>> -                 branch_cl_issue, branch_cl_patchset)])
>> -        print
>> -        print ('The current branch (%s) is tracking a local branch (%s)
>> with '
>> -               'an associated CL.') % (cl.GetBranch(), local_branch)
>> -        print 'Adding %s/#ps%s as a dependency patchset.' % (
>> -            branch_cl_issue_url, branch_cl_patchset)
>> -        print
>> +    if settings.GetIsSkipDependencyUploads():
>> +      print
>> +      print ('Skipping dependent patchsets check because git config '
>> +             'rietveld.skip-deps-uploads is set to True.')
>> +      print
>> +    else:
>> +      # Look for dependent patchsets. See crbug.com/480453 for more
>> details.
>> +      remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
>> +      upstream_branch = ShortBranchName(upstream_branch)
>> +      if remote is '.':
>> +        # A local branch is being tracked.
>> +        local_branch = ShortBranchName(upstream_branch)
>> +        auth_config = auth.extract_auth_config_from_options(options)
>> +        branch_cl = Changelist(branchref=local_branch,
>> auth_config=auth_config)
>> +        branch_cl_issue_url = branch_cl.GetIssueURL()
>> +        branch_cl_issue = branch_cl.GetIssue()
>> +        branch_cl_patchset = branch_cl.GetPatchset()
>> +        if branch_cl_issue_url and branch_cl_issue and
>> branch_cl_patchset:
>> +          upload_args.extend(
>> +              ['--depends_on_patchset', '%s:%s' % (
>> +                   branch_cl_issue, branch_cl_patchset)])
>> +          print
>> +          print ('The current branch (%s) is tracking a local branch (%s)
>> with '
>> +                 'an associated CL.') % (cl.GetBranch(), local_branch)
>> +          print 'Adding %s/#ps%s as a dependency patchset.' % (
>> +              branch_cl_issue_url, branch_cl_patchset)
>> +          print
>>
>>     project = settings.GetProject()
>>     if project:
>>
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-06-29 17:27:54]

On 2015/06/29 16:46:20, iannucci wrote:
> Er.... diff smaller.
> 
> On Mon, Jun 29, 2015, 09:45 Robert Iannucci <mailto:iannucci@chromium.org>
wrote:
> 
> > On mobile, but you can make the food smaller by doing 'if skip: return'
> > instead of the else. Well take a look in a bit.

I could not do "if skip: return" because it is too early to return and also
there is no loop to break out of. Let me know if you see a way of doing this.

> >
> > Can we change the option to non-standard-branch-layout? I don't want one
> > of these for every improvement made. Then the directions can day "do it
> > this way, but if you have a workflow that had worked for years and you
> > don't want to change at this option and we'll let you do everything
> > manually", or something to that effect?

Done @Patchset2. PTAL.

> >
> > On Mon, Jun 29, 2015, 06:07  <mailto:rmistry@google.com> wrote:
> >
> >> Reviewers: iannucci, agable,
> >>
> >> Message:
> >>
> >> Robbie, I 100% agree with your comments in
> >>
> >>
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/nZbX5...
> >> about how options to opt out adds to the maintenance burden of the tools
> >> overall.
> >>
> >> I plan to soon make the CQ fail patchsets if there is an open dependency.
> >> I
> >> expect that CQ change to cause many complains (as we have already seen in
> >> the
> >> chromium-dev thread) and issues being opened unless there is a way to skip
> >> the
> >> dependency uploads to make different workflows continue to submit as
> >> before
> >> into
> >> the CQ.
> >>
> >> Description:
> >> Add ability to be able to skip dependency checks and uploads.
> >>
> >> After this CL is in users will be able to skip dependency checks and
> >> uploads
> >> locally by running:
> >> git config rietveld.skip-deps-uploads True
> >> To undo the skipping:
> >> git config --unset rietveld.skip-deps-uploads
> >>
> >> To do the above commands globally (across all checkouts):
> >> git config --global rietveld.skip-deps-uploads True
> >> git config --global --unset rietveld.skip-deps-uploads
> >>
> >>
> >> BUG=chromium:504832
> >>
> >> Please review this at https://codereview.chromium.org/1210903005/
> >>
> >> Base URL:
> >> https://chromium.googlesource.com/chromium/tools/depot_tools.git@master
> >>
> >> Affected files (+30, -21 lines):
> >>    M git_cl.py
> >>
> >>
> >> Index: git_cl.py
> >> diff --git a/git_cl.py b/git_cl.py
> >> index
> >>
> >>
>
f99f5e99a269bcaafa9b1bae48a65941e9fdea5f..cb119a07c34c4959f8bfcdfd3139a918922fb34a
> >> 100755
> >> --- a/git_cl.py
> >> +++ b/git_cl.py
> >> @@ -554,6 +554,9 @@ class Settings(object):
> >>     def GetBugPrefix(self):
> >>       return self._GetRietveldConfig('bug-prefix', error_ok=True)
> >>
> >> +  def GetIsSkipDependencyUploads(self):
> >> +    return self._GetRietveldConfig('skip-deps-uploads', error_ok=True)
> >> +
> >>     def GetRunPostUploadHook(self):
> >>       run_post_upload_hook = self._GetRietveldConfig(
> >>           'run-post-upload-hook', error_ok=True)
> >> @@ -2185,27 +2188,33 @@ def RietveldUpload(options, args, cl, change):
> >>       if target_ref:
> >>         upload_args.extend(['--target_ref', target_ref])
> >>
> >> -    # Look for dependent patchsets. See crbug.com/480453 for more
> >> details.
> >> -    remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
> >> -    upstream_branch = ShortBranchName(upstream_branch)
> >> -    if remote is '.':
> >> -      # A local branch is being tracked.
> >> -      local_branch = ShortBranchName(upstream_branch)
> >> -      auth_config = auth.extract_auth_config_from_options(options)
> >> -      branch_cl = Changelist(branchref=local_branch,
> >> auth_config=auth_config)
> >> -      branch_cl_issue_url = branch_cl.GetIssueURL()
> >> -      branch_cl_issue = branch_cl.GetIssue()
> >> -      branch_cl_patchset = branch_cl.GetPatchset()
> >> -      if branch_cl_issue_url and branch_cl_issue and branch_cl_patchset:
> >> -        upload_args.extend(
> >> -            ['--depends_on_patchset', '%s:%s' % (
> >> -                 branch_cl_issue, branch_cl_patchset)])
> >> -        print
> >> -        print ('The current branch (%s) is tracking a local branch (%s)
> >> with '
> >> -               'an associated CL.') % (cl.GetBranch(), local_branch)
> >> -        print 'Adding %s/#ps%s as a dependency patchset.' % (
> >> -            branch_cl_issue_url, branch_cl_patchset)
> >> -        print
> >> +    if settings.GetIsSkipDependencyUploads():
> >> +      print
> >> +      print ('Skipping dependent patchsets check because git config '
> >> +             'rietveld.skip-deps-uploads is set to True.')
> >> +      print
> >> +    else:
> >> +      # Look for dependent patchsets. See crbug.com/480453 for more
> >> details.
> >> +      remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
> >> +      upstream_branch = ShortBranchName(upstream_branch)
> >> +      if remote is '.':
> >> +        # A local branch is being tracked.
> >> +        local_branch = ShortBranchName(upstream_branch)
> >> +        auth_config = auth.extract_auth_config_from_options(options)
> >> +        branch_cl = Changelist(branchref=local_branch,
> >> auth_config=auth_config)
> >> +        branch_cl_issue_url = branch_cl.GetIssueURL()
> >> +        branch_cl_issue = branch_cl.GetIssue()
> >> +        branch_cl_patchset = branch_cl.GetPatchset()
> >> +        if branch_cl_issue_url and branch_cl_issue and
> >> branch_cl_patchset:
> >> +          upload_args.extend(
> >> +              ['--depends_on_patchset', '%s:%s' % (
> >> +                   branch_cl_issue, branch_cl_patchset)])
> >> +          print
> >> +          print ('The current branch (%s) is tracking a local branch (%s)
> >> with '
> >> +                 'an associated CL.') % (cl.GetBranch(), local_branch)
> >> +          print 'Adding %s/#ps%s as a dependency patchset.' % (
> >> +              branch_cl_issue_url, branch_cl_patchset)
> >> +          print
> >>
> >>     project = settings.GetProject()
> >>     if project:
> >>
> >>
> >>
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-06-29 20:01:48]

I have concerns over the functionality as described, i.e. this is really clunky
and it's not clear 
to me that there are enough people affected by this to justify supporting it,
especially since 
in some meaningful sense they'd be using git-cl "wrong", and there are simple
ways to fix
or work around their issues.

If we did decide to support this, it's not clear what
"non-standard-branch-layout" means. Perhaps
"skip-patch-dependency-checks" instead?

As far as needing something like this for the follow-on work, I'm not sure I
totally understand
that part. I fully support having the CQ fail (by default) to land things w/
open dependencies,
but perhaps the way to work around that is via the normal mechanisms for
bypassing checks?
Or perhaps we can add another annotation to the CL description
(NO_DEPENDENCY_CHECKS=true 
or something)?

[rmistry, 2015-06-29 20:12:18]

On 2015/06/29 20:01:48, Dirk Pranke wrote:
> I have concerns over the functionality as described, i.e. this is really
clunky
> and it's not clear 
> to me that there are enough people affected by this to justify supporting it,
> especially since 
> in some meaningful sense they'd be using git-cl "wrong", and there are simple
> ways to fix
> or work around their issues.

I agree, but I expect the resistance in
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/EbujXmwLNWU
to get worse when the CQ starts enforcing this and I really do not want to have
to address numerous follow up emails / issues about this.

> 
> If we did decide to support this, it's not clear what
> "non-standard-branch-layout" means. Perhaps
> "skip-patch-dependency-checks" instead?

It was called skip-deps-uploads @Patchset1. You and Robbie need to have a chat
:) I am ok with either conclusion.

> 
> As far as needing something like this for the follow-on work, I'm not sure I
> totally understand
> that part. I fully support having the CQ fail (by default) to land things w/
> open dependencies,
> but perhaps the way to work around that is via the normal mechanisms for
> bypassing checks?
> Or perhaps we can add another annotation to the CL description
> (NO_DEPENDENCY_CHECKS=true 
> or something)?

NO_DEPENDENCY_CHECKS is a good idea. Feel free to convince the masses in
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/EbujXmwLNWU
(actually even if you don't I can add it because it sounds useful in general).

[iannucci, 2015-06-29 21:57:18]

I'm mainly concerned with adding a bunch of boolean options which have
compounding effects... Several of the tools in depot_tools already assume that
the branch layout has meaning and will do the wrong thing (mostly just abort) if
you've set up your branches in a non-standard way.

If you have a branch layout like:

origin/master
  feature1
    feature2

That /means/ that feature2 depends on feature1 which depends on origin/master.
The git tool itself understands that those refs all have upstream relationships
with each other, and git subcommands (like git pull) use this information to do
the right thing. AFAIU, there are folks who are claiming that they should be
able to have this layout but where feature1 and feature2 are actually
independent (e.g. the layout doesn't imply dependency information). That's why I
was thinking that a "non-standard-branch-layout" made the most sense (instead of
having a bunch of booleans, one for each feature which relies on branch layout
being meaningful). It would be a way for the developer to tell the tooling "I'm
doing my own thing, so don't assume that the branch layout I have means
anything". Otherwise as the tools in depot_tools grow, we'll end up having a
bunch of little booleans, each one to disable some feature which is broken
(subtly or otherwise) by the developer's custom branch layout.

FWIW, I see this as analogous to "I want to talk HTTPS to that server, but I
want an option to not do encryption, because it breaks my telnet-only workflow"
but "just use HTTP instead" is somehow not a valid answer. As I mentioned in the
thread, there are at least 3 other ways to indicate that the feature branches
don't have dependencies on each other already.

Maybe I'm also being too general here, and the option people really want is
"keep-local-master"? But what if users put new commits on their local master
and/or a CL number (and then branch off of master)? What is the correct behavior
of the tools? If I accidentally had this situation, I would want the tools to
abort/warn/upload master changes as dependent code, but I understand that others
are happy to not be notified of this, even if it means their feature branch code
is incomplete/not-patchable?

What if we had a per-branch setting like 'skip-dependency' so you could mark
master (or any other branch) as not-to-be-included in the upload process? That
could also be useful in this case (which I've used myself):

origin/master
  enable_super_verbose_debug_logging
    fix_bug *

Maybe? Then it's not so much about being non-standard as it is about explicitly
marking specific dependencies as not-to-be-uploaded? I think I would like that
better than a command line flag (which you can forget to add), or an env var
(which can be put in a .bashrc and forgotten about).

[rmistry, 2015-06-30 11:26:26]

On 2015/06/29 20:12:18, rmistry wrote:
> On 2015/06/29 20:01:48, Dirk Pranke wrote:
> > I have concerns over the functionality as described, i.e. this is really
> clunky
> > and it's not clear 
> > to me that there are enough people affected by this to justify supporting
it,
> > especially since 
> > in some meaningful sense they'd be using git-cl "wrong", and there are
simple
> > ways to fix
> > or work around their issues.
> 
> I agree, but I expect the resistance in
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/EbujXmwLNWU
> to get worse when the CQ starts enforcing this and I really do not want to
have
> to address numerous follow up emails / issues about this.
> 
> > 
> > If we did decide to support this, it's not clear what
> > "non-standard-branch-layout" means. Perhaps
> > "skip-patch-dependency-checks" instead?
> 
> It was called skip-deps-uploads @Patchset1. You and Robbie need to have a chat
> :) I am ok with either conclusion.
> 
> > 
> > As far as needing something like this for the follow-on work, I'm not sure I
> > totally understand
> > that part. I fully support having the CQ fail (by default) to land things w/
> > open dependencies,
> > but perhaps the way to work around that is via the normal mechanisms for
> > bypassing checks?
> > Or perhaps we can add another annotation to the CL description
> > (NO_DEPENDENCY_CHECKS=true 
> > or something)?
> 
> NO_DEPENDENCY_CHECKS is a good idea. Feel free to convince the masses in
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/EbujXmwLNWU
> (actually even if you don't I can add it because it sounds useful in general).

Added support for NO_DEPENDENCY_CHECKS in
https://chromereviews.googleplex.com/221697013/ (patchsets 4 and 5).

[rmistry, 2015-06-30 12:55:10]

On 2015/06/29 21:57:18, iannucci wrote:
> I'm mainly concerned with adding a bunch of boolean options which have
> compounding effects... Several of the tools in depot_tools already assume that
> the branch layout has meaning and will do the wrong thing (mostly just abort)
if
> you've set up your branches in a non-standard way.
> 
> If you have a branch layout like:
> 
> origin/master
>   feature1
>     feature2
> 
> That /means/ that feature2 depends on feature1 which depends on origin/master.
> The git tool itself understands that those refs all have upstream
relationships
> with each other, and git subcommands (like git pull) use this information to
do
> the right thing. AFAIU, there are folks who are claiming that they should be
> able to have this layout but where feature1 and feature2 are actually
> independent (e.g. the layout doesn't imply dependency information). That's why
I
> was thinking that a "non-standard-branch-layout" made the most sense (instead
of
> having a bunch of booleans, one for each feature which relies on branch layout
> being meaningful). It would be a way for the developer to tell the tooling
"I'm
> doing my own thing, so don't assume that the branch layout I have means
> anything". Otherwise as the tools in depot_tools grow, we'll end up having a
> bunch of little booleans, each one to disable some feature which is broken
> (subtly or otherwise) by the developer's custom branch layout.
> 
> FWIW, I see this as analogous to "I want to talk HTTPS to that server, but I
> want an option to not do encryption, because it breaks my telnet-only
workflow"
> but "just use HTTP instead" is somehow not a valid answer. As I mentioned in
the
> thread, there are at least 3 other ways to indicate that the feature branches
> don't have dependencies on each other already.
> 
> Maybe I'm also being too general here, and the option people really want is
> "keep-local-master"? But what if users put new commits on their local master
> and/or a CL number (and then branch off of master)? What is the correct
behavior
> of the tools? If I accidentally had this situation, I would want the tools to
> abort/warn/upload master changes as dependent code, but I understand that
others
> are happy to not be notified of this, even if it means their feature branch
code
> is incomplete/not-patchable?
> 
> What if we had a per-branch setting like 'skip-dependency' so you could mark
> master (or any other branch) as not-to-be-included in the upload process? That
> could also be useful in this case (which I've used myself):
> 
> origin/master
>   enable_super_verbose_debug_logging
>     fix_bug *
> 
> Maybe? Then it's not so much about being non-standard as it is about
explicitly
> marking specific dependencies as not-to-be-uploaded? I think I would like that
> better than a command line flag (which you can forget to add), or an env var
> (which can be put in a .bashrc and forgotten about).

I like the idea of making the config branch specific. Users whose workflow
involves leftover CLs in "master" can just run
"git config --global branch.master.skip-deps-uploads True"
to skip deps uploads on all local "master" branches.

[rmistry, 2015-06-30 12:59:25]

On 2015/06/30 12:55:10, rmistry wrote:
> On 2015/06/29 21:57:18, iannucci wrote:
> > I'm mainly concerned with adding a bunch of boolean options which have
> > compounding effects... Several of the tools in depot_tools already assume
that
> > the branch layout has meaning and will do the wrong thing (mostly just
abort)
> if
> > you've set up your branches in a non-standard way.
> > 
> > If you have a branch layout like:
> > 
> > origin/master
> >   feature1
> >     feature2
> > 
> > That /means/ that feature2 depends on feature1 which depends on
origin/master.
> > The git tool itself understands that those refs all have upstream
> relationships
> > with each other, and git subcommands (like git pull) use this information to
> do
> > the right thing. AFAIU, there are folks who are claiming that they should be
> > able to have this layout but where feature1 and feature2 are actually
> > independent (e.g. the layout doesn't imply dependency information). That's
why
> I
> > was thinking that a "non-standard-branch-layout" made the most sense
(instead
> of
> > having a bunch of booleans, one for each feature which relies on branch
layout
> > being meaningful). It would be a way for the developer to tell the tooling
> "I'm
> > doing my own thing, so don't assume that the branch layout I have means
> > anything". Otherwise as the tools in depot_tools grow, we'll end up having a
> > bunch of little booleans, each one to disable some feature which is broken
> > (subtly or otherwise) by the developer's custom branch layout.
> > 
> > FWIW, I see this as analogous to "I want to talk HTTPS to that server, but I
> > want an option to not do encryption, because it breaks my telnet-only
> workflow"
> > but "just use HTTP instead" is somehow not a valid answer. As I mentioned in
> the
> > thread, there are at least 3 other ways to indicate that the feature
branches
> > don't have dependencies on each other already.
> > 
> > Maybe I'm also being too general here, and the option people really want is
> > "keep-local-master"? But what if users put new commits on their local master
> > and/or a CL number (and then branch off of master)? What is the correct
> behavior
> > of the tools? If I accidentally had this situation, I would want the tools
to
> > abort/warn/upload master changes as dependent code, but I understand that
> others
> > are happy to not be notified of this, even if it means their feature branch
> code
> > is incomplete/not-patchable?
> > 
> > What if we had a per-branch setting like 'skip-dependency' so you could mark
> > master (or any other branch) as not-to-be-included in the upload process?
That
> > could also be useful in this case (which I've used myself):
> > 
> > origin/master
> >   enable_super_verbose_debug_logging
> >     fix_bug *
> > 
> > Maybe? Then it's not so much about being non-standard as it is about
> explicitly
> > marking specific dependencies as not-to-be-uploaded? I think I would like
that
> > better than a command line flag (which you can forget to add), or an env var
> > (which can be put in a .bashrc and forgotten about).
> 
> I like the idea of making the config branch specific. Users whose workflow
> involves leftover CLs in "master" can just run
> "git config --global branch.master.skip-deps-uploads True"
> to skip deps uploads on all local "master" branches.

Made the config branch specific @Patchset3.

To summarize:
patchset1 - skip-deps-uploads checkout config
patchset2 - non-standard-branch-layout checkout config
patchset3 - skip-deps-uploads branch config

I like the combination of patchset3 branch config and the NO_DEPENDENCY_CHECKS I
added in https://chromereviews.googleplex.com/221697013/.

Robbie and Dirk PTAL.

[jochen (gone - plz use gerrit), 2015-06-30 13:13:24]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-30 13:13:25]

why would the global config key contain the branch name?

[rmistry, 2015-06-30 13:19:27]

On 2015/06/30 13:13:25, jochen wrote:
> why would the global config key contain the branch name?

So you can explicitly specify the local "master" branch to be skipped instead of
skipping dependency uploads for all branches in a checkout. Leftover CLs in
"master" in particular seems to be the main complain in the chromium-dev thread.

[jochen (gone - plz use gerrit), 2015-06-30 13:21:13]

On 2015/06/30 at 13:19:27, rmistry wrote:
> On 2015/06/30 13:13:25, jochen wrote:
> > why would the global config key contain the branch name?
> 
> So you can explicitly specify the local "master" branch to be skipped instead
of skipping dependency uploads for all branches in a checkout. Leftover CLs in
"master" in particular seems to be the main complain in the chromium-dev thread.

imo that's just an instance of "I don't want your workflow". I'd much rather
have an option that doesn't try to be smart about how I use git, e.g., a global
config using_golden_depot_tools_workflow = false

[rmistry, 2015-06-30 13:46:19]

On 2015/06/30 13:21:13, jochen wrote:
> On 2015/06/30 at 13:19:27, rmistry wrote:
> > On 2015/06/30 13:13:25, jochen wrote:
> > > why would the global config key contain the branch name?
> > 
> > So you can explicitly specify the local "master" branch to be skipped
instead
> of skipping dependency uploads for all branches in a checkout. Leftover CLs in
> "master" in particular seems to be the main complain in the chromium-dev
thread.
> 
> imo that's just an instance of "I don't want your workflow". I'd much rather
> have an option that doesn't try to be smart about how I use git, e.g., a
global
> config using_golden_depot_tools_workflow = false

That is what I had implemented in patchset2 but the feedback from Dirk was to
change that (comment #6).

[Dirk Pranke, 2015-06-30 16:54:21]

On Tue, Jun 30, 2015 at 6:46 AM, <rmistry@google.com> wrote:

> On 2015/06/30 13:21:13, jochen wrote:
>
>> On 2015/06/30 at 13:19:27, rmistry wrote:
>> > On 2015/06/30 13:13:25, jochen wrote:
>> > > why would the global config key contain the branch name?
>> >
>> > So you can explicitly specify the local "master" branch to be skipped
>>
> instead
>
>> of skipping dependency uploads for all branches in a checkout. Leftover
>> CLs in
>> "master" in particular seems to be the main complain in the chromium-dev
>>
> thread.
>
>  imo that's just an instance of "I don't want your workflow". I'd much
>> rather
>> have an option that doesn't try to be smart about how I use git, e.g., a
>>
> global
>
>> config using_golden_depot_tools_workflow = false
>>
>
> That is what I had implemented in patchset2 but the feedback from Dirk was
> to
> change that (comment #6).
>

I'm open to being outvoted :).

My concern about the "I don't want your workflow" setting is that I don't
know how to scope that or explain what it does (or doesn't do). It seems
like we'd end up needing to document a list of how it effects specific
features. Maybe that's better than having N different flags that can
potentially interact, I'm not sure ...

Mostly I want to make sure that we're not putting hooks in for just a small
percentage of users; as long as there are workarounds for what they want to
do, we should require them to use the workarounds instead.

To make this clearer using Robbie's example: if a user has origin/master ->
feature1 -> feature2 and they don't want feature2 to act as if feature1 is
a dependency, then a valid workaround is to change feature2's upstream
before uploading.

However, I actually often have that scenario, so I think another valid
workaround is to either set NO_DEPENDENCY_CHECKS, or have a flag that
allows you to override the upstream on upload  (git-cl upload
--upstream=origin/master or something).

At any rate, I don't feel strongly enough about this at this point to want
to block things, I think we're all roughly on the same page, and I'm kinda
theoretically on vacation, so I defer further substantive reviews to
iannucci :).

-- Dirk

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2015-06-30 16:59:29]

Doesn't git-cl upload also allow you to specify the base revision too? How
does that interact with this feature? Would that be a viable workaround (I
don't remember if it's decorated with --upstream or if it's just a bare
argument).

On Tue, Jun 30, 2015 at 9:54 AM Dirk Pranke <dpranke@chromium.org> wrote:

> On Tue, Jun 30, 2015 at 6:46 AM, <rmistry@google.com> wrote:
>
>> On 2015/06/30 13:21:13, jochen wrote:
>>
>>> On 2015/06/30 at 13:19:27, rmistry wrote:
>>> > On 2015/06/30 13:13:25, jochen wrote:
>>> > > why would the global config key contain the branch name?
>>> >
>>> > So you can explicitly specify the local "master" branch to be skipped
>>>
>> instead
>>
>>> of skipping dependency uploads for all branches in a checkout. Leftover
>>> CLs in
>>> "master" in particular seems to be the main complain in the chromium-dev
>>>
>> thread.
>>
>>  imo that's just an instance of "I don't want your workflow". I'd much
>>> rather
>>> have an option that doesn't try to be smart about how I use git, e.g., a
>>>
>> global
>>
>>> config using_golden_depot_tools_workflow = false
>>>
>>
>> That is what I had implemented in patchset2 but the feedback from Dirk
>> was to
>> change that (comment #6).
>>
>
> I'm open to being outvoted :).
>
> My concern about the "I don't want your workflow" setting is that I don't
> know how to scope that or explain what it does (or doesn't do). It seems
> like we'd end up needing to document a list of how it effects specific
> features. Maybe that's better than having N different flags that can
> potentially interact, I'm not sure ...
>
> Mostly I want to make sure that we're not putting hooks in for just a
> small percentage of users; as long as there are workarounds for what they
> want to do, we should require them to use the workarounds instead.
>
> To make this clearer using Robbie's example: if a user has origin/master
> -> feature1 -> feature2 and they don't want feature2 to act as if feature1
> is a dependency, then a valid workaround is to change feature2's upstream
> before uploading.
>
> However, I actually often have that scenario, so I think another valid
> workaround is to either set NO_DEPENDENCY_CHECKS, or have a flag that
> allows you to override the upstream on upload  (git-cl upload
> --upstream=origin/master or something).
>
> At any rate, I don't feel strongly enough about this at this point to want
> to block things, I think we're all roughly on the same page, and I'm kinda
> theoretically on vacation, so I defer further substantive reviews to
> iannucci :).
>
> -- Dirk
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-06-30 17:03:24]

(copying from an email ... sorry for the duplication):

On Tue, Jun 30, 2015 at 6:46 AM, <rmistry@google.com> wrote:
> That is what I had implemented in patchset2 but the feedback from Dirk was to
> change that (comment #6).

I'm open to being outvoted :).

My concern about the "I don't want your workflow" setting is that I don't know
how to scope that or explain what it does (or doesn't do). It seems like we'd
end up needing to document a list of how it effects specific features. Maybe
that's better than having N different flags that can potentially interact, I'm
not sure ...

Mostly I want to make sure that we're not putting hooks in for just a small
percentage of users; as long as there are workarounds for what they want to do,
we should require them to use the workarounds instead.

To make this clearer using Robbie's example: if a user has origin/master ->
feature1 -> feature2 and they don't want feature2 to act as if feature1 is a
dependency, then a valid workaround is to change feature2's upstream before
uploading.

However, I actually often have that scenario, so I think another valid
workaround is to either set NO_DEPENDENCY_CHECKS, or have a flag that allows you
to override the upstream on upload  (git-cl upload --upstream=origin/master or
something).

At any rate, I don't feel strongly enough about this at this point to want to
block things, I think we're all roughly on the same page, and I'm kinda
theoretically on vacation, so I defer further substantive reviews to iannucci
:).

[Dirk Pranke, 2015-06-30 17:04:55]

On 2015/06/30 16:59:29, iannucci wrote:
> Doesn't git-cl upload also allow you to specify the base revision too? How
> does that interact with this feature? Would that be a viable workaround (I
> don't remember if it's decorated with --upstream or if it's just a bare
> argument).

I thought it did, too, but last I looked I couldn't figure out how to do that.
Perhaps I
was just being slow.

Yes, I would consider that an acceptable workaround.

[rmistry, 2015-06-30 17:28:05]

On 2015/06/30 17:04:55, Dirk Pranke wrote:
> On 2015/06/30 16:59:29, iannucci wrote:
> > Doesn't git-cl upload also allow you to specify the base revision too? How
> > does that interact with this feature? Would that be a viable workaround (I
> > don't remember if it's decorated with --upstream or if it's just a bare
> > argument).
> 
> I thought it did, too, but last I looked I couldn't figure out how to do that.
> Perhaps I
> was just being slow.
> 
> Yes, I would consider that an acceptable workaround.

I did not think about "git cl upload ${hash}". Currently it still uploads
dependency information even when the hash is specified.
I am not sure what the solution to this is:
* If a hash is specified then always skip uploading deps because you are not
sure how it relates with your open dep.
* Only upload deps if "git cherry ${hash} tracked_branch" if not empty? If hash
is not specified then it will use "origin/master".

Regarding the 2nd one, johme@ mentioned "git cherry" in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...
I am curious what your thoughts are about it (either here or in the chromium-dev
thread).

[jochen (gone - plz use gerrit), 2015-07-02 06:56:17]

On 2015/06/30 at 17:28:05, rmistry wrote:
> On 2015/06/30 17:04:55, Dirk Pranke wrote:
> > On 2015/06/30 16:59:29, iannucci wrote:
> > > Doesn't git-cl upload also allow you to specify the base revision too? How
> > > does that interact with this feature? Would that be a viable workaround (I
> > > don't remember if it's decorated with --upstream or if it's just a bare
> > > argument).
> > 
> > I thought it did, too, but last I looked I couldn't figure out how to do
that.
> > Perhaps I
> > was just being slow.
> > 
> > Yes, I would consider that an acceptable workaround.
> 
> I did not think about "git cl upload ${hash}". Currently it still uploads
dependency information even when the hash is specified.
> I am not sure what the solution to this is:
> * If a hash is specified then always skip uploading deps because you are not
sure how it relates with your open dep.
> * Only upload deps if "git cherry ${hash} tracked_branch" if not empty? If
hash is not specified then it will use "origin/master".
> 
> Regarding the 2nd one, johme@ mentioned "git cherry" in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...
> I am curious what your thoughts are about it (either here or in the
chromium-dev thread).

My thoughts are that if you take a tool that is already called "git" and then
try make it smart, you're bound to fail.

From that point of view, me having a master branch is also wrong, granted. Maybe
we can just fix depot_tools to not create one for blink? Otherwise, i'd really
prefer if things like adding dependencies etc.. where opt-in features

[iannucci, 2015-07-02 07:42:43]

On 2015/07/02 06:56:17, jochen wrote:
> On 2015/06/30 at 17:28:05, rmistry wrote:
> > On 2015/06/30 17:04:55, Dirk Pranke wrote:
> > > On 2015/06/30 16:59:29, iannucci wrote:
> > > > Doesn't git-cl upload also allow you to specify the base revision too?
How
> > > > does that interact with this feature? Would that be a viable workaround
(I
> > > > don't remember if it's decorated with --upstream or if it's just a bare
> > > > argument).
> > > 
> > > I thought it did, too, but last I looked I couldn't figure out how to do
> that.
> > > Perhaps I
> > > was just being slow.
> > > 
> > > Yes, I would consider that an acceptable workaround.
> > 
> > I did not think about "git cl upload ${hash}". Currently it still uploads
> dependency information even when the hash is specified.
> > I am not sure what the solution to this is:
> > * If a hash is specified then always skip uploading deps because you are not
> sure how it relates with your open dep.
> > * Only upload deps if "git cherry ${hash} tracked_branch" if not empty? If
> hash is not specified then it will use "origin/master".
> > 
> > Regarding the 2nd one, johme@ mentioned "git cherry" in
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...
> > I am curious what your thoughts are about it (either here or in the
> chromium-dev thread).
> 
> My thoughts are that if you take a tool that is already called "git" and then
> try make it smart, you're bound to fail.
> 
> From that point of view, me having a master branch is also wrong, granted.
Maybe
> we can just fix depot_tools to not create one for blink? Otherwise, i'd really
> prefer if things like adding dependencies etc.. where opt-in features

Well... it's opt-in in the sense that a dependency is introduced if you make a
branch-off-of-a-branch. Or were you meaning something else?

[Dirk Pranke, 2015-07-02 17:41:22]

On Wed, Jul 1, 2015 at 11:56 PM, <jochen@chromium.org> wrote:

> On 2015/06/30 at 17:28:05, rmistry wrote:
>
>> On 2015/06/30 17:04:55, Dirk Pranke wrote:
>> > On 2015/06/30 16:59:29, iannucci wrote:
>> > > Doesn't git-cl upload also allow you to specify the base revision
>> too? How
>> > > does that interact with this feature? Would that be a viable
>> workaround (I
>> > > don't remember if it's decorated with --upstream or if it's just a
>> bare
>> > > argument).
>> >
>> > I thought it did, too, but last I looked I couldn't figure out how to do
>>
> that.
>
>> > Perhaps I
>> > was just being slow.
>> >
>> > Yes, I would consider that an acceptable workaround.
>>
>
>  I did not think about "git cl upload ${hash}". Currently it still uploads
>>
> dependency information even when the hash is specified.
>
>> I am not sure what the solution to this is:
>> * If a hash is specified then always skip uploading deps because you are
>> not
>>
> sure how it relates with your open dep.
>
>> * Only upload deps if "git cherry ${hash} tracked_branch" if not empty? If
>>
> hash is not specified then it will use "origin/master".
>
>  Regarding the 2nd one, johme@ mentioned "git cherry" in
>>
>
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...
>
>> I am curious what your thoughts are about it (either here or in the
>>
> chromium-dev thread).
>
> My thoughts are that if you take a tool that is already called "git" and
> then
> try make it smart, you're bound to fail.
>
> From that point of view, me having a master branch is also wrong, granted.
> Maybe
> we can just fix depot_tools to not create one for blink?


I just landed agable's change to fix this. Hopefully that will help you :).

-- Dirk



> Otherwise, i'd really
> prefer if things like adding dependencies etc.. where opt-in features
>
> https://codereview.chromium.org/1210903005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-07-06 12:33:35]

On 2015/07/02 17:41:22, Dirk Pranke wrote:
> On Wed, Jul 1, 2015 at 11:56 PM, <mailto:jochen@chromium.org> wrote:
> 
> > On 2015/06/30 at 17:28:05, rmistry wrote:
> >
> >> On 2015/06/30 17:04:55, Dirk Pranke wrote:
> >> > On 2015/06/30 16:59:29, iannucci wrote:
> >> > > Doesn't git-cl upload also allow you to specify the base revision
> >> too? How
> >> > > does that interact with this feature? Would that be a viable
> >> workaround (I
> >> > > don't remember if it's decorated with --upstream or if it's just a
> >> bare
> >> > > argument).
> >> >
> >> > I thought it did, too, but last I looked I couldn't figure out how to do
> >>
> > that.
> >
> >> > Perhaps I
> >> > was just being slow.
> >> >
> >> > Yes, I would consider that an acceptable workaround.
> >>
> >
> >  I did not think about "git cl upload ${hash}". Currently it still uploads
> >>
> > dependency information even when the hash is specified.
> >
> >> I am not sure what the solution to this is:
> >> * If a hash is specified then always skip uploading deps because you are
> >> not
> >>
> > sure how it relates with your open dep.
> >
> >> * Only upload deps if "git cherry ${hash} tracked_branch" if not empty? If
> >>
> > hash is not specified then it will use "origin/master".
> >
> >  Regarding the 2nd one, johme@ mentioned "git cherry" in
> >>
> >
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...
> >
> >> I am curious what your thoughts are about it (either here or in the
> >>
> > chromium-dev thread).
> >
> > My thoughts are that if you take a tool that is already called "git" and
> > then
> > try make it smart, you're bound to fail.
> >
> > From that point of view, me having a master branch is also wrong, granted.
> > Maybe
> > we can just fix depot_tools to not create one for blink?
> 
> 
> I just landed agable's change to fix this. Hopefully that will help you :).
> 
> -- Dirk
> 
> 
> 
> > Otherwise, i'd really
> > prefer if things like adding dependencies etc.. where opt-in features
> >
> > https://codereview.chromium.org/1210903005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The CQ CL (https://chromereviews.googleplex.com/221697013/) that rejects
patchsets with open dependencies and adds a keyword to skip the check, has
approval and is ready to land.

How should we proceed with this CL? The three patchsets represent different
approaches.

Looks like some developers do seem to prefer creating and branching off a
"master" branch (eg:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/EbujXmwLNWU/x6w-a...).
The skip-deps-uploads config (@patchset1) and non-standard-branch-layout config
(@patchset2) would force developers to completely skip the feature because of
their "master" branch preference.

I prefer being able to skip dependency checks and uploads for particular
branches (@patchset3). Developers could then skip only the "master" (or some
other branch they commonly patch CLs to) while still being able to use the
feature.

[agable, 2015-07-06 17:03:22]

I think that doing so is fundamentally broken, and encourages broken behavior.
If developers are *always* skipping the upload of a certain branch which all of
their real branches are based off of, either:
1) That branch serves no purpose and should be deleted; or
2) That branch serves a purpose and something is fundamentally broken and should
be fixed.

I like having an option to skip the upload of dependencies on a one-shot basis.
But being able to always ignore a branch or two just seems overly conservative.
But maybe I'm too pushy; happy to be overruled.

[iannucci, 2015-07-06 17:53:06]

There's a case where you want to add a bunch of debug symbols or logs and
then have a branch based on that to commit an actual fix (but don't want to
commit the non conflicting logging changes). In that case I'd want to mark
the debug change to not be a required dependency.

Branches which textually conflict, however, are probably always an error
not to depend on though.

On Mon, Jul 6, 2015, 10:03  <agable@chromium.org> wrote:

> I think that doing so is fundamentally broken, and encourages broken
> behavior.
> If developers are *always* skipping the upload of a certain branch which
> all of
> their real branches are based off of, either:
> 1) That branch serves no purpose and should be deleted; or
> 2) That branch serves a purpose and something is fundamentally broken and
> should
> be fixed.
>
> I like having an option to skip the upload of dependencies on a one-shot
> basis.
> But being able to always ignore a branch or two just seems overly
> conservative.
> But maybe I'm too pushy; happy to be overruled.
>
> https://codereview.chromium.org/1210903005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-07-06 20:51:51]

I think I'm now lost as to who is arguing for what :).

If I understand things correctly, I would argue that we don't actually need to
change anything; if a branch has an upstream with an issue, we should treat it
as a dependency; if we want the upstream to not be a dependency, make it not
have an issue. This way, if people want to use a local master as LKGR (and I'm
more-or-less one of them), they just have to make sure it doesn't have an issue.


We all agree that this works, right, and the only question is to whether we need
to enable people to not treat branches that *do* have issues as dependencies
and, if so, how? 

For the latter case, Ravi's other CL --
https://chromereviews.googleplex.com/221697013/ -- should enable
deps-check-skipping on a per-CL basis, right? 

So, then we'd just need to enable branches to be persistently skipped, and we
have one of the three choices in this CL?

Personally, I think this is too much of a corner case to support, so I would see
if we can get away with not supporting it. If we do need to support it, I think
we should do via a branch-specific setting like in patchset 3, because the
"non-standard workflow" is not something we should be encouraging or enabling if
we can help it. 

So, to sum up, my votes are:

1) do nothing (apart from landing the *other* CL)
2) failing 1), do patchset 3.

I believe Ravi either agrees with me, or would vote for patchset 3.

Aaron, Robbie, Jochen, what are your votes?

[rmistry, 2015-07-06 21:00:31]

On 2015/07/06 20:51:51, Dirk Pranke wrote:
> I think I'm now lost as to who is arguing for what :).
> 
> If I understand things correctly, I would argue that we don't actually need to
> change anything; if a branch has an upstream with an issue, we should treat it
> as a dependency; if we want the upstream to not be a dependency, make it not
> have an issue. This way, if people want to use a local master as LKGR (and I'm
> more-or-less one of them), they just have to make sure it doesn't have an
issue.
> 
> 
> We all agree that this works, right, and the only question is to whether we
need
> to enable people to not treat branches that *do* have issues as dependencies
> and, if so, how? 
> 
> For the latter case, Ravi's other CL --
> https://chromereviews.googleplex.com/221697013/ -- should enable
> deps-check-skipping on a per-CL basis, right? 
> 
> So, then we'd just need to enable branches to be persistently skipped, and we
> have one of the three choices in this CL?
> 
> Personally, I think this is too much of a corner case to support, so I would
see
> if we can get away with not supporting it. If we do need to support it, I
think
> we should do via a branch-specific setting like in patchset 3, because the
> "non-standard workflow" is not something we should be encouraging or enabling
if
> we can help it. 
> 
> So, to sum up, my votes are:
> 
> 1) do nothing (apart from landing the *other* CL)
> 2) failing 1), do patchset 3.
> 
> I believe Ravi either agrees with me, or would vote for patchset 3.

I agree with your votes. I slightly prefer (2) because I believe doing nothing
will incur the wrath of the not-so-silent corner-case-supporting minority. I
will not mind this as long as Dirk, Robbie, Aaron help by responding to the
protestors on the chromium-dev thread after I submit my CQ CL
(https://chromereviews.googleplex.com/221697013/).


> 
> Aaron, Robbie, Jochen, what are your votes?

[iannucci, 2015-07-06 22:29:21]

On 2015/07/06 21:00:31, rmistry wrote:
> On 2015/07/06 20:51:51, Dirk Pranke wrote:
> > I think I'm now lost as to who is arguing for what :).
> > 
> > If I understand things correctly, I would argue that we don't actually need
to
> > change anything; if a branch has an upstream with an issue, we should treat
it
> > as a dependency; if we want the upstream to not be a dependency, make it not
> > have an issue. This way, if people want to use a local master as LKGR (and
I'm
> > more-or-less one of them), they just have to make sure it doesn't have an
> issue.
> > 
> > 
> > We all agree that this works, right, and the only question is to whether we
> need
> > to enable people to not treat branches that *do* have issues as dependencies
> > and, if so, how? 
> > 
> > For the latter case, Ravi's other CL --
> > https://chromereviews.googleplex.com/221697013/ -- should enable
> > deps-check-skipping on a per-CL basis, right? 
> > 
> > So, then we'd just need to enable branches to be persistently skipped, and
we
> > have one of the three choices in this CL?
> > 
> > Personally, I think this is too much of a corner case to support, so I would
> see
> > if we can get away with not supporting it. If we do need to support it, I
> think
> > we should do via a branch-specific setting like in patchset 3, because the
> > "non-standard workflow" is not something we should be encouraging or
enabling
> if
> > we can help it. 
> > 
> > So, to sum up, my votes are:
> > 
> > 1) do nothing (apart from landing the *other* CL)
> > 2) failing 1), do patchset 3.

+1 to this

> > 
> > I believe Ravi either agrees with me, or would vote for patchset 3.
> 
> I agree with your votes. I slightly prefer (2) because I believe doing nothing
> will incur the wrath of the not-so-silent corner-case-supporting minority. I
> will not mind this as long as Dirk, Robbie, Aaron help by responding to the
> protestors on the chromium-dev thread after I submit my CQ CL
> (https://chromereviews.googleplex.com/221697013/).
> 
> 
> > 
> > Aaron, Robbie, Jochen, what are your votes?

[jochen (gone - plz use gerrit), 2015-07-07 11:22:19]

lgtm on patchset 3

[rmistry, 2015-07-07 21:01:55]

On 2015/07/07 11:22:19, jochen wrote:
> lgtm on patchset 3

Looks like Jochen voted for patchset 3. I too slightly prefer patchset 3 over
doing nothing.

Can I have an owner's LGTM?

[jochen (gone - plz use gerrit), 2015-07-07 21:38:50]

I am an owner of this stuff..

[iannucci, 2015-07-07 21:40:31]

Oh before it gets lost, this config value should be documented... I can't
remember if there's a man page for git cl? If not, maybe add it to the help
preamble for git cl upload for now?

On Tue, Jul 7, 2015, 14:38  <jochen@chromium.org> wrote:

> I am an owner of this stuff..
>
> https://codereview.chromium.org/1210903005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-07-07 21:45:26]

On 2015/07/07 21:38:50, jochen wrote:
> I am an owner of this stuff..

Oops sorry, I did not look closely.

[rmistry, 2015-07-07 21:45:55]

On 2015/07/07 21:40:31, iannucci wrote:
> Oh before it gets lost, this config value should be documented... I can't
> remember if there's a man page for git cl? If not, maybe add it to the help
> preamble for git cl upload for now?

Thanks for the reminder. I will add it tomorrow morning before submitting.

> 
> On Tue, Jul 7, 2015, 14:38  <mailto:jochen@chromium.org> wrote:
> 
> > I am an owner of this stuff..
> >
> > https://codereview.chromium.org/1210903005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-07-08 17:26:59]

On 2015/07/07 21:45:55, rmistry wrote:
> On 2015/07/07 21:40:31, iannucci wrote:
> > Oh before it gets lost, this config value should be documented... I can't
> > remember if there's a man page for git cl? If not, maybe add it to the help
> > preamble for git cl upload for now?
> 
> Thanks for the reminder. I will add it tomorrow morning before submitting.

Documented the config value @patchset4. I will wait a few hours before
submitting incase there are any comments.

> 
> > 
> > On Tue, Jul 7, 2015, 14:38  <mailto:jochen@chromium.org> wrote:
> > 
> > > I am an owner of this stuff..
> > >
> > > https://codereview.chromium.org/1210903005/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-07-08 23:07:16]

The CQ bit was checked by rmistry@google.com

[rmistry, 2015-07-08 23:07:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1210903005/#ps60001
(title: "Document config value")

[commit-bot: I haz the power, 2015-07-08 23:07:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210903005/60001

[commit-bot: I haz the power, 2015-07-08 23:10:00]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295953