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

Silence git checkout warnings #128

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Conversation

Johennes
Copy link
Collaborator

This silences the git checkout warnings. I wasn't sure if you wanted to remove output from the other git commands, too?

Fixes: #126

let mut cmd = Command::new("git");
cmd.current_dir(self.directory(project_root)?)
.arg("checkout")
.arg("--quiet") // Suppress detached head and dangling commit warnings
Copy link
Owner

Choose a reason for hiding this comment

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

I don't love this, but it does solve the proximal problem.

I think (and it doesn't have to be in this PR) I would prefer to not get into a detached HEAD state in the first place, not just to keep quiet about it.

When you're checking out to a tag, it'll be impossible to avoid getting into a detached HEAD state and we should warn, but when it's a branch then we should avoid it.

Unless you want to keep going on it, perhaps we should land this now and file another issue to enable this more elaborate fix. What do you think?

Copy link
Collaborator Author

@Johennes Johennes Oct 16, 2024

Choose a reason for hiding this comment

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

I kind of feel obliged to fix the mess I created and sorry for causing it in the first place. 😕

I pushed another attempt. This now calls git remote set-branches origin '*' after cloning which enables tracking of all remote branches in the shallow fork. That in turn enables checking out branch names without getting into a detached HEAD state. I also added tests to assert which checkout targets cause detached HEAD states:

  • Checkout without target: no detached HEAD
  • Checkout with branch name: no detached HEAD
  • Checkout with tag name: detached HEAD
  • Checkout with SHA: detached HEAD

This appears to work. Alas, the checkout logic has become somewhat involved overall. If you're not happy with this, maybe it'll be better to revert back to non-shallow clones. I do enjoy the speediness of shallow checkouts but I suppose most people won't be doing checkouts overly often. So maybe I've been over-optimizing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've colored the test headings in the script output so you can more easily see the output of each checkout command in https://github.com/jhugman/uniffi-bindgen-react-native/actions/runs/11371743080/job/31634502135?pr=128 now.

Copy link
Owner

Choose a reason for hiding this comment

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

If you're not happy with this, maybe it'll be better to revert back to non-shallow clones.

I am very happy. Thank you so much.

I do enjoy the speediness of shallow checkouts but I suppose most people won't be doing checkouts overly often. So maybe I've been over-optimizing here.

Hard agree. Let's see how people use this, and then optimize for that use case if it ever needs to be optimized.

I've colored the test headings in the script output

Love it. It's the little things that add that feel of polish.

@jhugman jhugman merged commit 25ddc6a into jhugman:main Oct 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ubrn checkout gives detached HEAD warning to stderr
2 participants