Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not use git set-branches if the target branch is not currently available in the repository #491

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

tromai
Copy link
Member

@tromai tromai commented Sep 28, 2023

Closes #486

Bug description

This bug happened because of the way branch check out is handling in this method.

At the moment, the process Macaron in which Macaron tries to checkout the correct branch and commit for the analysis is as follows:

  • The branch to checkout is either the value provided by the user (via --branch in the CLI) or the default branch of the repository (implementation):

    • We are using git rev-parse --abbrev-ref origin/HEAD to obtain the value origin/<default_branch_name> (references).
  • Macaron try to check out that branch (implementation here)..

    • The way we check if a given branch exists in the repository:
      • res_branch in [os.path.basename(ref.name) for ref in org_remote.refs]
      • The refs in org_remote instance would return git.RemoteReference instances (according to gitpython documentation).
      • The documentation for git remote references can be seen here.
      • Therefore, each git.RemoteReference can be considered a reference to the latest commit of a remote branch (from the last time we communicate with remote). Therefore, org_remote.refs will return an iterator of remote references that this local repository currently have (which mean that it could be missing any new remote branches, etc.).
      • I have manually check in a shallow clone, a call to org_remote.refs does not return all available remote branches. It only contains: 1. refs/remotes/origin/<branch_name> which point to the branch which we shallow clone (if no branch is provided e.g. git clone --depth=1 <url>), this will be refs/remotes/origin/<default_branch>. 2. if it's a shallow clone of the default branch, there will be also a refs/remotes/origin/HEAD.
  • If Macaron cannot find the target branch from org_remote.refs, Macaron will try to setup a local branch that track that branch from origin remote (implementation here).

    • The full command is: git remote set-branches --add origin '<target_branch>' (references) What this command do is that it setup the branch list to track a branch <target_branch> from the remote. The --add will make sure that we only add the new tracked branch into the list of tracked branches. The next time we fetch, git will also try to fetch this branch from the remote repository. The reason I did it this way is to make sure that we only need to fetch the branch that we are interested in when working with shallow clones.

    • This is the command that causes the bug in Repository checkout fails in some cases #486:

      • This command this will update the .git/config file in the target repository with <target_branch>. Therefore, if the branch doesn't exist, subsequent fetching/pulling will always fail because git will try to ask the remote for this non-exist branch and return error everytime.
      • The second issue: there is a fetching operation happened here without checking if offline_mode is set or not.
      • The third issue is that we don't have a good way to revert the effect of set-branches (https://stackoverflow.com/questions/47726087/undo-set-branches-on-git-remote). Overall, it's not a good idea to perform git operations that alter .git/config.

Solution.

To resolve this bug, I simplify the logic for checkout branch and commit for a target repository with the following goals:

  • Remove any conditional logic for handling shallow-clone repositories:
    • The usage of git remote set-branches --add origin '<target_branch>'
    • Unshallow when trying to pull from latest (note here).
  • Make sure that the only online operations included in the check_out_repo_target method are:
    • git fetch: happen before checkout branch.
    • git pull --ff-only before checkout a specific commit.

If the checkout operation fails for either branch or commit, we return errors.

@tromai tromai self-assigned this Sep 28, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 28, 2023
@tromai tromai changed the title fix: do not run git set-branches if the target branch is not currently available in the repository fix: do not run git set-branches if the target branch is not currently available in the repository (WIP) Sep 28, 2023
… available in the repository

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
@tromai tromai force-pushed the 486-repository-checkout-fails-in-some-cases branch from 185e9df to abc8d14 Compare September 29, 2023 02:37
@tromai tromai changed the title fix: do not run git set-branches if the target branch is not currently available in the repository (WIP) fix: not using git set-branches if the target branch is not currently available in the repository (WIP) Sep 29, 2023
@tromai tromai marked this pull request as ready for review September 29, 2023 04:37
@tromai tromai changed the title fix: not using git set-branches if the target branch is not currently available in the repository (WIP) fix: not using git set-branches if the target branch is not currently available in the repository Oct 3, 2023
src/macaron/slsa_analyzer/git_url.py Outdated Show resolved Hide resolved
src/macaron/slsa_analyzer/git_url.py Outdated Show resolved Hide resolved
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
@behnazh-w behnazh-w changed the title fix: not using git set-branches if the target branch is not currently available in the repository fix: do not use git set-branches if the target branch is not currently available in the repository Oct 26, 2023
@tromai tromai merged commit 1e78aaf into staging Oct 26, 2023
10 checks passed
@tromai tromai deleted the 486-repository-checkout-fails-in-some-cases branch October 26, 2023 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants