Only check DCO on the author's commits

[soulxu]

Signed-off-by: He Jie Xu hejie.xu@intel.com

Commit Message: Only check DCO on the author's commit
Additional Description:
Sometimes missing the sign-off in the merged commit. That forces developers to skip all the checks. So changes to only verify DCO on the author's commits.

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

[moderation]

The PR should be changed to remove exclusionary language as per #11596. Please change the language and file names to "allow list".

[phlax]

im wondering if this would be better reading from a local file

as it will change fairly frequently im not sure if we want to maintain this in the repo

im also wondering when people hit this - i use no-verify always anyway for other reasons so im wondering if it happens on every push and for how long after a commit with missing DCO is landed its a problem

[snowp]

im also wondering when people hit this - i use no-verify always anyway for other reasons so im wondering if it happens on every push and for how long after a commit with missing DCO is landed its a problem

I think it mainly happens after a merge that brings in bad commits, iirc we only run the check on the diff from the remote when pushing, so once the bad commit has been pushed it won't trigger agan.

[soulxu]

im also wondering when people hit this - i use no-verify always anyway for other reasons so im wondering if it happens on every push and for how long after a commit with missing DCO is landed its a problem

I think it mainly happens after a merge that brings in bad commits, iirc we only run the check on the diff from the remote when pushing, so once the bad commit has been pushed it won't trigger agan.

Yes, mostly happened when merge with main branch, after one push then it won't be a problem. I just saw it will be asked by new contributor (included me) https://envoyproxy.slack.com/archives/C78HA81DH/p1620285466057600, so I write this up. But also agree with that it may not worth to maintain this. So just bring this up, see if people need it or not.

[soulxu]

im wondering if this would be better reading from a local file

as it will change fairly frequently im not sure if we want to maintain this in the repo

im also wondering when people hit this - i use no-verify always anyway for other reasons so im wondering if it happens on every push and for how long after a commit with missing DCO is landed its a problem

Follow the document, I remember to execute the format check, but there still have few checks https://github.com/envoyproxy/envoy/blob/main/support/hooks/pre-push#L56-L76

I think I copy those checks into my own scripts avoid copy those checks command manually

[phlax]

another possibility in wondering about

maybe we could make it that the dco error is only thrown when the author of the problematic commit matches the local author

this would prevent the issue addressed here - although its not a perfect solution and there are some corner cases where it wouldnt work or where it would throw when you didnt want it to. Still would be better than current situation i think

cc @ggreenway

[soulxu]

I found another option is using 'commit-msg' hook. https://git-scm.com/docs/githooks#_commit_msg

It checks on each git commit, the script can check the 'Signed-off-by:' exist or not. If not, then can stop the commit.

[antoniovicente]

Also checking on commit may be generally helpful as it would make errors on pre-push less likely.

[moderation]

Please rename file dco_verify_white_list.txt to dco_verify_allow_list.txt
Edit - it looks like this file is no longer required. Please remove from the PR

[snowp]

LGTM, thanks!

@moderation do you have any more comments on this one?

[phlax]

im wondering if this really does anything (i guess the same could be said for the original check)

immediately before this check is run the dco is added - is this just checking the immediately prior hook ?

[phlax]

i guess the same could be said for the original check

expanding on this - with the original check there is some marginal utility if eg you made a commit and then installed the push-hooks - it would then error and tell you that not all of your commits had DCO

i guess also the original check had some utility if eg you cherry-picked a commit from somewhere and it lacked DCO

with this check neither of the above are true, and therefore i see no utililty to the check - ie if you dont install the hooks - neither DCO is added, not DCO is checked

if you do install the hooks - DCO is added, and (pointlessly) DCO is checked

[moderation]

LGTM with exclusionary language / files removed

[soulxu]

i guess the same could be said for the original check

expanding on this - with the original check there is some marginal utility if eg you made a commit and then installed the push-hooks - it would then error and tell you that not all of your commits had DCO

pre-commit hook and per-push hook are same with checking on all your commits, the difference is the per-hook checks the commits you merged from the main branch, but you can do nothing with it, since it is history commit.

i guess also the original check had some utility if eg you cherry-picked a commit from somewhere and it lacked DCO

Yes, I checked with cherry-pick case, per-commit hook doesn't help with it. But if the cherry-pick commit is done by your own, and you install the hook, there will be a check when you commit that cherry-picked commit. If missing the DCO from cherry-picked commit, the CI is the last defensive line to check on that.

with this check neither of the above are true, and therefore i see no utililty to the check - ie if you dont install the hooks - neither DCO is added, not DCO is checked

if you do install the hooks - DCO is added, and (pointlessly) DCO is checked

Yes, I see the point. The only meaningful thing is the developer removed the signed-off unintentionally. The CI will check the DCO is the next defensive line. Another option is removing the DCO check from the hook, only depends on the CI. And CI found there is no DCO verify, notify the developer to install the hook. Or maybe this is pre-commit hook only save CI from those unintentional cases, that is a little gain. At least both ways can avoid the history commit annoying the developer.

[phlax]

Another option is removing the DCO check from the hook, only depends on the CI. And CI found there is no DCO verify, notify the developer to install the hook.

that is the conclusion i came to.

the current hook is not doing much and moving it here means it does even less

i think for reviewers, there is a wish that something would catch DCO issues before it lands in a PR as this is a common problem for first time contributors - unfortunately i think using git hooks to check this doesnt help much - which is why the current hook doesnt prevent this happening.

Once you install the hook it never triggers (aside from incorrectly in merges)

[soulxu]

Another option is removing the DCO check from the hook, only depends on the CI. And CI found there is no DCO verify, notify the developer to install the hook.

that is the conclusion i came to.

the current hook is not doing much and moving it here means it does even less

i think for reviewers, there is a wish that something would catch DCO issues before it lands in a PR as this is a common problem for first time contributors - unfortunately i think using git hooks to check this doesnt help much - which is why the current hook doesnt prevent this happening.

Once you install the hook it never triggers (aside from incorrectly in merges)

@snowp what would you think? If this is we want, I can remove the check.

[phlax]

The only meaningful thing is the developer removed the signed-off unintentionally.

re this point - i dont see how they can (other than cherry-picking from another checkout without hooks installed) - when you commit (or rebase etc) it automatically adds it - maybe im wrong on this point - but i dont see how

[phlax]

maybe im wrong on this point - but i dont see how

hmm - i am wrong - you can rebase and remove the message - so the check has that specific benefit

[snowp]

I see the argument behind this check not really doing much, so yeah it might make sense to just remove. cc @htuch that reviewed the original addition

maybe not necessary?

[htuch]

I have no memory of this place; I personally just use git aliases and automagically populate DCO in every commit, I do not use the hooks reference.

[snowp]

Alright let's just remove the DCO check from the hooks, I don't see much point in having it there if it won't really catch any real issues

[phlax]

Alright let's just remove the DCO check from the hooks, I don't see much point in having it there if it won't really catch any real issues

the other alternative is to make it check only for the current authors commits

that would mean it still checks the corner-cases it does now - ie bad rebases - but wouldnt trigger on merge

[snowp]

Sure yeah that sounds like an improvement over today, so we can go with that. I would be curious how often that actually comes into play, but we don't really have much insight. I guess as long as people aren't being hit by check failures that they aren't responsible for there's no harm in having an extra light weight check

[soulxu]

Thanks for the feedback, I will update later

[snowp]

Conceptually this seems right, maybe @phlax can review the bash?

[phlax]

lgtm - thanks @soulxu