Skip to content
Snippets Groups Projects

Problem merging from fork

    • Closed Incident created by Ghost User @ghost

    In simopenpass, we have a problem with a merge request: https://gitlab.eclipse.org/eclipse/simopenpass/simopenpass/-/merge_requests/16

    When we try to merge to our repository from the fork, we get an error message:

    Something went wrong during merge pre-receive hook. You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others.
You must be a committer to push on behalf of others. Try again.
    Edited

    Child items ...

    • Activity

    • Martin Lowe
      Martin Lowe @malowe ·
      Reporter

      This is due to an older feature present in many of the Gerrit repositories for a long time where only people registered as Committers on a project could make a commit on someone's behalf to the project. I used it log --pretty=format:"%h:%s- %an(%ae) -> %cn(%ce)" on the itk branch to diagnose this and saw most of the commits authored by Michael were committed by Robin.

      There has been some discussion (source) on whether this is still relevant today and whether this can be removed. The discussion was kind of non-committal, but we can reopen the issue to check if this is something we can remove or not.

      //cc @cguindon @wbeaton

    • Christopher Guindon
      Christopher Guindon @cguindon ·
      Reporter

      @malowe So the issue here is:

      This particular MR includes commits from different authors but the owner of the MR is not a committer?

    • Martin Lowe
      Martin Lowe @malowe ·
      Reporter

      The MR has commits from different authors, where all commits were committed solely by the owner of the MR who is not a committer (https://projects.eclipse.org/api/projects/automotive_simopenpass, Robin Caloudis). Slight difference, but essentially yes

    • Ghost User
      Ghost User @ghost ·
      Author

      @cguindon Correct. The MR owner is a contributor who wanted us (the project) to include his contribution...

      The story goes like this:

      1. Robin forks our project
      2. Robin makes modifications
      3. Robin merges the latest developments on the original branch to his fork (that's how Michaels contributions appear in MR who IS a committer and has done work on original branch meanwhile)
      4. Robin creates an MR for the original branch in our project (maybe the MR was created before the merge 3), I don't know )
      5. Michael also reviews the MR and makes changes (probably here again his name appears)
      6. MR is approved
      7. Merge button throws the error described

      Would it have helped, if Robin skipped the merge after forking to receive the updates of the original branch?

      • Christopher Guindon
        Christopher Guindon @cguindon ·
        Reporter

        If I understand correctly, Robin merged in commits from origin/master on his development branch? If so, this could have been avoided with a rebase instead of a merge commit.

        However, I would expect the MR from Robin to be valid as-is as long as all the authors are covered by an ECA which should be the case unless one of the authors revoked their ECA since those commits where merged-in to origin/master.

        @malowe I am thinking we should remove that Gerrit Validation:

        only people registered as Committers on a project could make a commit on someone's behalf to the project.

        We simply need to make sure that all authors have an ECA on file.

        @wbeaton would you agree?

      • Arun Das
        Arun Das @adascri ·

        Hi @cguindon, Thank you for the investigation and your proposal on simplifying the Gerrit Validation. To me that sounds like a reasonable step to take, hoping that our collaboration in code will get a little bit easier.

        What are the next steps? Can we implement the change?

      • Please register or sign in to reply
      • Christopher Guindon
        Christopher Guindon @cguindon ·
        Reporter

        @malowe How much work is it for you to remove the validation that ensures that only committers can push commits on behalf of other users. This made sense with Gerrit, however, this doesn't seem to translate well in Gitlab.

        @wbeaton Do you have any concerns with us removing that validation? TLDR, a user will be able to submit a PR without being an author on one of the commits.

        My understanding is that we simply need to block contributions if one of the author's ECA is expired or invalid. Does it matter who is actually pushing the code change?

        Edited by Christopher Guindon
      • Martin Lowe
        Martin Lowe @malowe ·
        Reporter

        Not a lot of work, it's removing an if statement at the end of the validation call if we want it straight up gone. It's only marginally more work if we want to keep it in for certain endpoints

      • Christopher Guindon
        Christopher Guindon @cguindon ·
        Reporter

        I would be tempted to keep things as-is for Gerrit since it's not a problem over there. However, if @wbeaton believes that we don't need to do this anymore for Gerrit and Gitlab, we should remove it!

      • Wayne Beaton
        Wayne Beaton @wbeaton ·
        Reporter

        Trying hard not to overthink this... the requirement is that a contributor must have signed the ECA. There's another more subtle requirement: the contributors needs to have made the contribution with knowledge that it would become part of the Eclipse project. I believe that we can reasonably make the assumption that this is true in most cases and can make the argument that committers make a good faith effort to ensure that they're only merging contributions received by contributors under the ECA/DCO.

        My vote is to remove the requirement. If the author has signed the ECA, then you're good-to-go.

        So much for not overthinking...

      • Christopher Guindon
        Christopher Guindon @cguindon ·
        Reporter

        Thanks Wayne!

        @malowe can we look at adding this to your queue for next week, please? I think it's an important issue that should be prioritised since it is a barrier to a contribution.

        Edited by Christopher Guindon
      • Ghost User
        Ghost User @ghost ·
        Author

        "the contributors needs to have made the contribution with knowledge that it would become part of the Eclipse project"

        -> I strongly believe so, if they fork from an Eclispe project and apply a merge request back to the Eclipse project :-)

        "we can reasonably make the assumption that this is true in most cases"

        -> typical case:

        1. a (non-project) contributor with ECA forks, modifies, poses a merge request back to the same project
        2. a project committer reviews, makes changes (that's how another name comes into commits and causes problems) and merges the MR
      • Martin Lowe
        Martin Lowe @malowe ·
        Reporter

        I've made the PR w/ updated tests to remove the check for proxy commits. We're looking at merging this next Tuesday as we like to be able to monitor the change to make sure everything is solid, and its a long weekend up in Canada this weekend.

        https://github.com/EclipseFdn/git-eca-rest-api/pull/67

      • Christopher Guindon
        Christopher Guindon @cguindon ·
        Reporter

        Thanks, Martin for the quick turnaround!

      • Please register or sign in to reply
    • Christopher Guindon assigned to @malowe

      assigned to @malowe

    • Christopher Guindon changed the severity to Medium - S3

      changed the severity to Medium - S3

    • Martin Lowe
      Martin Lowe @malowe ·
      Reporter

      I've made the changeset that removes the proxy check live and checked its function. We should be good to go!

    • Christopher Guindon
      Christopher Guindon @cguindon ·
      Reporter

      Thanks @malowe

    • Christopher Guindon closed

      closed

    • Ghost User
      Ghost User @ghost ·
      Author

      @malowe Thank you very much! We will play it through and see next time, if it fixes the problem...

    • Frederic Gurr changed the description

      changed the description

    • Frederic Gurr added ECA label

      added ECA label

    • Jonah Graham mentioned in issue #3774 (closed)

      mentioned in issue #3774 (closed)

    Please register or sign in to reply

    Copyright © Eclipse Foundation, Inc. All Rights Reserved.     Privacy Policy | Terms of Use | Copyright Agent