On Wed, Aug 11, 2021 at 4:22 PM Jani Nikula <jani.nik...@intel.com> wrote:
> On Wed, 11 Aug 2021, "Vivi, Rodrigo" <rodrigo.v...@intel.com> wrote:
> > On Wed, 2021-08-11 at 13:58 +0300, Jani Nikula wrote:
> >> It's not exactly trivial to add the smarts to properly check for
> >> pushing
> >> backmerges, rebases, topic branches and subtree branches (such as
> >> gvt). For a start, prompt the user with hints about what's going on.
> >>
> >> Cc: Daniel Vetter <dan...@ffwll.ch>
> >> Signed-off-by: Jani Nikula <jani.nik...@intel.com>
> >>
> >> ---
> >>
> >> Untested.
> >> ---
> >> dim | 14 ++++++++++----
> >> 1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/dim b/dim
> >> index 56463eb0c0a6..9fc2d78b8617 100755
> >> --- a/dim
> >> +++ b/dim
> >> @@ -989,7 +989,7 @@ function checkpatch_commit_push_range
> >> # push.
> >> function dim_push_branch
> >> {
> >> - local branch remote committer_email count
> >> + local branch remote committer_email commit_count merge_count
> >>
> >> branch=${1:?$usage}
> >> shift
> >> @@ -1004,9 +1004,15 @@ function dim_push_branch
> >>
> >> # Apart from maintainers pushing merges or rebases, most
> >> patches should
> >> # be pushed in small batches.
> >> - count=$(git rev-list --count --first-parent
> >> "$branch@{u}..$branch")
> >> - if [[ $count -gt 10 ]]; then
> >> - if ! ask_user "Pushing $count commits. Are you
> >> sure?"; then
> >> + commit_count=$(git rev-list --count --no-merges --first-
> >> parent "$branch@{u}..$branch")
> >> + merge_count=$(git rev-list --count --merges --first-parent
> >> "$branch@{u}..$branch")
> >> + if [[ $merge_count -gt 0 ]]; then
> >> + if ! ask_user "Pushing $merge_count merges and
> >> $commit_count non-merge commits. Merges should only be pushed by
> >> maintainers. Are you sure?"; then
> >
> > With our flow of drm-intel-next and drm-intel-gt-next, there are a few
> > cases where it is much easier a topic branch to get merged to both.
> >
> > On a recent case I asked Matt Roper to go ahead and merge to both
> > directly since I was sure that all of them had the same base and all
> > patches really ready for merge.
> >
> > Are we going to block cases like this and force pull request on all
> > kind of topic branches? or with the right maintainer acks we can
> > get this path and then we'd need to adjust this message?
>
> We have the workflow and tooling in place to send and apply pull
> requests via mail specifically to do merge checks and add an audit trail
> of sorts on what happened. We get the diffstat in the pull request, and
> dim also adds the Link: tag to the merge commit.
>
> So yeah, I want a pull request on the list even for topic branches.
> This is what I've done with all the topic branches I've set up and
> merged. The bar should be as high for merges as for patches. Or even
> higher.
+1. Maintainers playing cowboy on the tree directly without a public
audit trail is bad, no matter who does it or why. The rare exception
is when the problem their fixing is worse than the solution, like when
someone managed to push all of drm-tip onto drm-misc-next, then
maintainers have to use their powers.
> It just looks really bad to have a merge commit in the branch by a
> non-maintainer with no pull request on the list and no acks recorded in
> the merge commit.
>
> Whether we should let committers merge the pull request with maintainer
> acks, recorded in the merge commit, is, I think, a separate discussion.
Agreed, that's imo up to maintainer teams. For drm-misc I've strongly
suggested that only maintainers do that, mostly because drm-misc is
all volunteers and so it just makes sense if that rarely needed
special flow knowledge is concentrated. It avoids screw-ups. drm-intel
I guess you guys can make a case that platform enabling needs to do
this regularly enough that they can start the topic branch.
Oh also: dim has tooling for topic branches too. Was that used, or
not? Without that the topic branch won't be included in drm-tip, which
means you can really test it before baking in the topic branch for
good.
Anywy, I think the patch is the right thing to do.
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
When we have a tested version I think it'd be good to Cc: all current
maintainers just so they're aware. Dave might be confused if dim has
even more to complain :-)
-Daniel
>
> BR,
> Jani.
>
>
>
> >
> >> + echoerr "NOTE: Branch not pushed."
> >> + return 1
> >> + fi
> >> + elif [[ $commit_count -gt 10 ]]; then
> >> + if ! ask_user "Pushing $commit_count commits. Commits
> >> should be only be pushed in relatively small batches. Are you sure?";
> >> then
> >> echoerr "NOTE: Branch not pushed."
> >> return 1
> >> fi
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
