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.
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.
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
