Check of merge commits in PR

[fabiocos]

cms-bot is checking and listing the merge commits appearing in a test of a PR. The use of cms-merge-topic in the preparation of PRs is clearly discouraged in our documentation https://cms-sw.github.io/tutorial-resolve-conflicts.html , but in practice merge commits are often found in the PRs, both as merge of branches (e.g. master to update the PR), or even using cms-merge-topic. I guess that developers find them as simpler to manage than rebasing.
The use of merge is not necessarily jeopardizing the history, and is often accepted by the reviewers, see as a relevant recent example cms-sw/cmssw#22594 (comment) . Anyway merging is potentially dangerous, and allowing different kind of merge commits may help in missing the dangerous ones, see for instance a recent example in #26201 (luckily with limited consequences).

I see two drawbacks with the bot procedure to check merge commits:

• the merge is done on top of the HEAD of the branch, but the search of merge commits looks done staring with the IB tag (according to my understanding of https://github.com/cms-sw/cms-bot/blob/master/run-pr-tests#L245 ). This means that the check could list merges coming from other PRs already merged. This is a reason why I try to accumulate PRs to be merged during the day and merge them altogether close to the IB deadline, so as to minimize the "pollution" of all tests;

• the test simply searches for any merge commits. But as most of them are in practice not a real problem, and are normally accepted, the test is not providing really useful information to discriminate potentially dangerous additions.

In order to try to isolate the really dangerous merge commits I have tried to implement what I believe could be the check procedure in the following utility:

23:11 cmsdev25 594> cat ~/bin/git-cms-check-duplicate-pr-merges 
#!/bin/bash

#set -o verbose

print_help() {
    echo "" && \
    echo "Check whether in a feature branch to be merged there are PR merges already present in the base branch." && echo && \
    echo "usage:   -b <base branch for comparison> <feature branch to be tested>" && \
    echo "options: -h display this help and exit" && \
    echo ""
}

BASE_BRANCH=""
FEATURE_BRANCH=""

while getopts "bh" OPT;
do
  case ${OPT} in
  b) BASE_BRANCH=${2} ;;
  h) print_help && exit 0 ;;
  \?) exit 1 ;;
  esac
done
shift $(( OPTIND -1 ))

if [ -z "${2}" ]  
    then echo "usage:   -b <base branch for comparison> <feature branch to be tested>" ; exit 1
else
    FEATURE_BRANCH=${2}
fi

DATE=`date +%s`

git checkout ${BASE_BRANCH}
git rev-list --topo-order --pretty=short --merges HEAD  | grep "Merge pull request" | sort > /tmp/AAA-${DATE}
last_commit=`git rev-list --topo-order HEAD | head -n 1`

echo "Testing merges beyond the last commit in the base branch:"
git-show ${last_commit}

CURRENT_BRANCH="tmp/merge-test"

test1=`git rev-parse --quiet --verify ${CURRENT_BRANCH}`
if [ -n "${test1}" ] 
    then echo "Test branch already exists, abort test" ; exit 1
fi

git checkout -b ${CURRENT_BRANCH}
git cms-merge-topic ${FEATURE_BRANCH}
git rev-list --topo-order --pretty=short --merges ${last_commit}..  | grep "Merge pull request" | sort > /tmp/BBB-${DATE}

OUTPUT=`comm -12 /tmp/AAA-${DATE} /tmp/BBB-${DATE}`

if [ -z ${OUTPUT} ]; then
    echo "No duplicated pull request merge"
else
    echo "Pull request merges already in integration branch:"
    comm -12 /tmp/AAA-${DATE} /tmp/BBB-${DATE}
fi

I have started to use it in my private tests so as to exercise it.

It would be good whether people may cross check the idea behind it, so as in case we may update the test in the bot to make it more effective.

@slava77 @perrotta @Dr15Jones @kpedro88 @davidlange6

[cmsbuild]

A new Issue was created by @fabiocos Fabio Cossutti.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[kpedro88]

Several years ago I added the "...with cms-merge-topic" in the merge commit message created when using cms-merge-topic so it would be easier to identify improper use of this command, but there was never agreement on what the bot should do with this info.

It seems here you're trying to look for duplicate merges of PRs already merged in master?

[fabiocos]

@kpedro88 yes, this looks to be the really dangerous case...

[fabiocos]

The recent update by @smuzaffar to the merge tests allows to monitor in the PR checks which other PRs are added on top of the tested one, separating from other merge commits inside the PR itself. This is very helpful to interpret the comparison results, but I guess this is not telling us whether the original PR has some pathological merge inside it.

[fabiocos]

the test can anyway be done offline.