Skip to main content

Public

Project privacy set to public. By default, its content is available to everyone (authenticated or not). Please note that more restrictive permissions might exist on some items.

    •  
      request #15082 FastForward not detected when checking mergeability
    Actions
    Infos
    #15082
    Guilhem Bonnefille (CS) (gbonnefille)
    2020-07-20 10:39
    2020-07-07 10:34
    16351
    Details
    FastForward not detected when checking mergeability
    In our process, the merge of a feature branch onto the trunk branch is made manually.
    Due to a recent issue with huge commits (request #15077), we detected a possible inversion on arguments in a call.

    In our situation, the code went wrong trying to evaluate the mergeability and the potential conflicts. But the function `detectMergeabilityStatus` is supposed to test FastForward before checking conflicts.
    Looking deeper, it seems there is an inversion of parameters in the call to `isAncestor` in `isFastForwardable`.

    I'm sorry to not propose a fix right now, and some tests to validate the issue and solution. But I was stuck by investigating on this issue and there is still urgent work to fix.
    Pull Request
    Empty
    Empty
    • [ ] enhancement
    • [ ] internal improvement
    Empty
    Stage
    Thomas Gerbet (tgerbet)
    Closed
    2020-07-20
    Attachments
    Artifact links

    Releases

    Fixed in

    Artifact ID Project Tracker Summary Status Last Update Date Submitted By Assigned to
    rel #14817 Tuleap Releases 11.17 Delivered 2020-07-23 10:57 Manuel Vacelet (vaceletm)
    Empty
    References
    Referencing request #15082

    Artifact Tracker v5

    request #15077 Pull Request processing fails on huge Git merge

    Git commit

    tuleap/tuleap/stable

    Detect the manual merge of a PR before updating the merge statuses 1805fb4ebf
    User avatar Thomas Gerbet (tgerbet) , 2020-07-16 14:36
    Display settings

    Follow-ups

    User avatar
    Yannis ROSSETTO (rossettoy)2020-07-20 10:39
    Integrated into Tuleap 11.16.99.172
    • Status changed from Under review to Closed
    • Connected artifacts
    • Close date changed from 2020-07-08 to 2020-07-20
    User avatar
    Thomas Gerbet (tgerbet)2020-07-16 14:18
    You are correct, at the very least it's a waste of resources. Detection of a manual merge is faster than the detection of the mergeability (at least when the merge is not fast forward) and we can avoid doing the detection at all if the branch has been merged.

    I re-open the request, I'm going to fix this.
    • Category set to Pull Request
    • Status changed from Declined to New
    • Assigned to changed from None to Thomas Gerbet (tgerbet)
    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2020-07-08 17:34
    > For PR that are manually merged it already works that way.

    In our experience, when we push the merge in the main branch, the mergeability is still computed (and due to request #15077 can run wrong).
    Detecting as soon as possible that branches are already merged and eventually returning NO_FASTFORWARD_MERGE like detectMergeConflict does in such situation (there is no more conflict as they are merged) can improve the situation.
    User avatar
    Thomas Gerbet (tgerbet)2020-07-08 17:13
    For PR that are manually merged it already works that way. However there is indeed an issue when you create a new PR, I have opened request #15092 to track progress on this.
    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2020-07-08 16:45
    Oups, you're right. I reviewed the code with the point of view of an **already** merged branch.

    A -- B -- D # main branch
    \ /
    C # pr branch

    What about adding an ALREADY_MERGED status here?
    User avatar
    Thomas Gerbet (tgerbet)2020-07-08 16:00

    Hello,

    The existing code to detect if a fast forward merge is possible seems to be correct to me.

    If we take the following situation:

    A -- B # main branch
            \
             C  # pr branch

    To determine if the "pr branch" can be merged into the "main branch" you need to determine if B is ancestor of C [0]. Tuleap does this verification with merge-base [1]. For our example, the generated command is "git merge-base --is-ancestor B C".

    Functional tests seems to confirm the existing code is correct and that the call to GitExec::mergeBase in the PullRequestMerger::isFastForwardable method should not be changed. However, I will admit the naming of the variables is not great and it makes the whole thing a bit confusing.

    [0] https://git-scm.com/docs/git-merge#Documentation/git-merge.txt---ff-only
    [1] https://git-scm.com/docs/git-merge-base#Documentation/git-merge-base.txt---is-ancestor

    • Status changed from New to Declined
    • Close date set to 2020-07-08