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

Better error message for old versions of Git #11197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Sudha247
Copy link
Collaborator

Old versions of git don't support the --no-write-fetch-head flag. This patch detects this particular failure and prints an error message hinting that. Fixes #10976. Prints the following error message --

Error: Your git version doesn't support the '--no-write-fetch-head' flag.
Hint: Please update your git version.

@Sudha247 Sudha247 force-pushed the no-write-fetch-head branch from 3125099 to 5b9b8ab Compare December 12, 2024 12:43
src/dune_pkg/rev_store.ml Outdated Show resolved Hide resolved
src/dune_rules/setup.defaults.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Nice. I like that it doesn't add additional calls to detect that flag!

Added some comments to improve and probably needs to either be rebased on #11198 when it is merged or just exclude the change to setup.default.ml.

src/dune_pkg/rev_store.ml Outdated Show resolved Hide resolved
src/dune_pkg/rev_store.ml Outdated Show resolved Hide resolved
Stdune.Io.read_file path, exit_code)
~prefix:"dune"
~suffix:"run_with_exit_code"
~dir:(Path.of_string (Filename.get_temp_dir_name ()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put

let temp_dir = Path.of_string @@ Filename.get_temp_dir_name () in

in the definition of the function since that stays constant throughout the whole run of the function, no need to compute this on every run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, it'd be computed the same number of times if it were in the function definition - I checked it with some debug prints, but maybe I'm missing something.

src/dune_rules/setup.defaults.ml Outdated Show resolved Hide resolved
@Leonidas-from-XIV
Copy link
Collaborator

@Sudha247 Can you please rebase on main? I can see that this branch contains unrelated changes that have been merged to main in #11198 and #11194.

@Sudha247 Sudha247 force-pushed the no-write-fetch-head branch from d4c5928 to d9c750f Compare December 16, 2024 06:48
Old versions of git don't support the  flag. This patch detects this particular failure and prints an error message hinting that.

Signed-off-by: Sudha Parimala <[email protected]>
Signed-off-by: Sudha Parimala <[email protected]>
@maiste maiste force-pushed the no-write-fetch-head branch from d9c750f to 346ac96 Compare December 23, 2024 09:24
Copy link
Collaborator

@maiste maiste left a comment

Choose a reason for hiding this comment

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

LGTM 👍
(CI issue is unrelated to this PR)

@Sudha247 Sudha247 requested a review from rgrinberg December 23, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dune pkg: improve behavior when available git is too old
3 participants