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

SKARA-461: Do a git version check #707

Closed
wants to merge 1 commit into from

Conversation

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Jul 29, 2020

Hi,

This PR adds a git version check to the git skara CLI. This will print a warning if the git version is not a known working version. Since outdated versions can lead to IOExceptions due to e.g. unsupported git CLI options, hopefully the warning message will help to inform users about their use of an unsupported git version, and a potential way to solve the problem i.e. upgrading to a more recent version.

Currently at least git 2.22 is required since we use the --combined-all-paths option of git log (see GitCommits), which was added in 2.22.

Thanks,
Jorn


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

Reviewers

  • Kevin Rushforth (kcr - no project role)
  • Robin Westberg (rwestberg - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/skara pull/707/head:pull/707
$ git checkout pull/707

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 29, 2020

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 29, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jul 29, 2020

How hard would it be to actually support older versions? Requiring 2.22 might be a barrier for some users. I just checked and 2 of the 4 machines I use, including my main development platform, have versions older than that. I'm not a heavy user of the Skara CLI tools, but am concerned about those who are.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jul 29, 2020

Btw, I think this PR is fine unless / until the Skara CLI tools support older versions.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 29, 2020

@kevinrushforth Thanks for taking a look.

My intent was not to say the CLI tools should require 2.22 or some other version, but the current list of known working versions is adjusted to the fact that the current implementation requires version 2.22 to run successfully (otherwise e.g. https://bugs.openjdk.java.net/browse/SKARA-454 can occur).

I think the intent was to support older versions as well, since a lot of package managers provide older versions of git, but currently using a version older than 2.22 causes problems. I've added the other versions to the code as well, but commented them out for now.

The required version should probably be adjusted downwards once lesser versions have been confirmed to work. (i.e. this is more of a stopgap solution)

Copy link
Member

@rwestberg rwestberg left a comment

Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2020

@JornVernee This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

461: Do a git version check

Reviewed-by: kcr, rwestberg
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the master branch:

  • 233758e: 456: Read value of resolved in build properly when considering backports

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 233758ef9b266a6f6c831f574547c0e20e9c2420.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jul 30, 2020
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 30, 2020

/integrate

@openjdk openjdk bot closed this Jul 30, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 30, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2020

@JornVernee The following commits have been pushed to master since your change was applied:

  • 233758e: 456: Read value of resolved in build properly when considering backports

Your commit was automatically rebased without conflicts.

Pushed as commit a813417.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants