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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-38699] Limit CLI git operations to the workspace #692

Merged
merged 10 commits into from
Apr 3, 2021
Merged

[JENKINS-38699] Limit CLI git operations to the workspace #692

merged 10 commits into from
Apr 3, 2021

Conversation

arpoch
Copy link
Contributor

@arpoch arpoch commented Mar 20, 2021 •
edited by MarkEWaite

JENKINS-38699 - Validate .git directory with --resolve-git-dir

As the validation of a git repository presently is checked using git rev-parse --is-inside-work-tree but if the .git directory is corrupted then current repository's parent directories are searched for a valid .git directory. To limit/confine the traversal, --is-inside-work-tree is replaced by --resolve-git-dir which check if path to the .git directory of the current git repository is valid.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

What types of changes does your code introduce?

  • Bug fix (non-breaking change which fixes an issue)

Further comments

Switched from the original implementation as a breaking change to instead add a new API that allows the caller to define if parent directories should be searched for a git repository. Retains the existing behavior for the CLI git implementation (searching parent directories if the workspace contains a .git directory) and the existing behavior of the JGit implementation (never searching parent directories). Retains the previous implementation and includes a statement in the javadoc about the behavioral difference between the CLI git and JGit implementations of hasGitRepo().

@arpoch arpoch changed the title [JENKINS-38699] Validate git repository with --resolve-git-dir [JENKINS-38699] Validate git repository with rev-parse --resolve-git-dir Mar 20, 2021
MarkEWaite
MarkEWaite reviewed Mar 28, 2021
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java Outdated Show resolved Hide resolved
@MarkEWaite
Add hasGitRepo(boolean) to fix JENKINS-38996
444b4a5 
The JGit implementation of hasGitRepo() always ignored parent
directories.  The CLI git impelementation of hasGitRepo() ignored parent
directories if the workspace did not contain a '.git' directory.
If the workspace contains an empty '.git' directory, then CLI git
will search parent directories.

That's a strange behavior, but I'd rather not learn by hard
experience which implementations depend on that strange behavior.

Introduces a new API, hasGitRepo(boolean checkParentDirectories).  The
JENKINS-38996 fix will require a new release of the git plugint that
depends on this release of the git client plugin and uses the new
API to avoid the poor behavior of the existing hasGitRepo() implementation.
@MarkEWaite
Remvoe redundant JGit code
8ef1573 
Already checking directory existence earlier in method and cannot use
the hasGitRepo method for bare repositories because it requires a
subdirectory named '.git'.
@MarkEWaite
Annotate test with JENKINS-38699
2527eca
@MarkEWaite
Use same hasGitRepo pattern in JGit as CliGit
73426d8
@MarkEWaite
Remove duplicate tests
dd7c0f4
@arpoch
Copy link
Contributor Author

arpoch commented Apr 3, 2021

Thanks @MarkEWaite for getting involved and providing a new functionality. But JENKINS-38699 issue still persists as the git-plugin currently uses the hasGitRepo() method. So should a PR for the issue(JENKINS-38699) be opened in git-plugin which could use the functionality implemented by this PR?

@MarkEWaite
Copy link
Contributor

Thanks @MarkEWaite for getting involved and providing a new functionality. But JENKINS-38699 issue still persists as the git-plugin currently uses the hasGitRepo() method. So should a PR for the issue(JENKINS-38699) be opened in git-plugin which could use the functionality implemented by this PR?

Thanks @arpoch . Yes, that's what will be needed. I'm preparing that change now with a series of interactive and automated tests to confirm that JENKINS-38699 is resolved. As far as I can tell from reading the git plugin code, all the cases of hasGitRepo() should be replaced with hasGitRepo(false). Would you be willing to look at the git plugin and see if that matches your reading as well?

The behavior of hasGitRepo() for CLI git was different enough from the behavior for JGit that I didn't want to risk changing either of their implementations. I don't know which plugins depend on the JGit behavior and which depend on the CLI git behavior. The new methods are consistent between CliGit and JGit.

@arpoch
Copy link
Contributor Author

arpoch commented Apr 3, 2021 •
edited

I completely agree on replacing hasGitRepo() with hasGitRepo(false), I will definitely look into the git-plugin code-base and update you further.

@MarkEWaite
Copy link
Contributor

@arpoch See jenkinsci/git-plugin#1064 for the proposed change to git plugin

@arpoch
Copy link
Contributor Author

arpoch commented Apr 3, 2021

I searched for hasGitRepo() method usage in git-plugin source code and automated tests as well, the jenkinsci/git-plugin#1064(jenkinsci/git-plugin#1064) changes match with my results.

@MarkEWaite MarkEWaite changed the title [JENKINS-38699] Validate git repository with rev-parse --resolve-git-dir [JENKINS-38699] Limit git operations to the workspace Apr 3, 2021
@MarkEWaite MarkEWaite changed the title [JENKINS-38699] Limit git operations to the workspace [JENKINS-38699] Limit CLI git operations to the workspace Apr 3, 2021
@MarkEWaite MarkEWaite merged commit d7cb0ef into jenkinsci:master Apr 3, 2021
@MarkEWaite MarkEWaite added bug Incorrect or flawed behavior and removed bugfix labels Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimklimov jimklimov jimklimov left review comments

@MarkEWaite MarkEWaite MarkEWaite approved these changes

Assignees
No one assigned
Labels
bug Incorrect or flawed behavior
Projects
None yet
Milestone
No milestone
3 participants
@arpoch @MarkEWaite @jimklimov