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

Improve help text about hidden repos #1471

Merged
merged 3 commits into from
Jul 26, 2024
Merged

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Jul 26, 2024

Note: This PR is either mostly or completely was in substantial part superseded by #1470 as discussed in #1469, and has been narrowed in scope. The following description does not reflect that.

Changes to the warning message

In the first commit, this unconditionally augments the warning suggesting to use --skip-hidden-repositories so that it mentions worktrees as well. It shows that slightly longer message even in a repository that has no worktrees. This has the benefit of simplicity, is the requested change in #1469 (comment) if I understand properly, and makes sense as an initial mitigation for #1469.

The message is not much longer, and if that level of detail is acceptable, it might even be okay to have it like this longer term, though I suspect it might not be necessary given other forthcoming changes.

I considered parenthesizing "and worktrees" but it looked to like "repositories (and worktrees)" could be misread as "repositories (and their worktrees)" to mean the same thing as "repositories."

I did not make a corresponding change in the other similarly worded warning message suggesting to use --find-untracked-repositories, because there does not appear to be an analogous situation with untracked directories where a worktree of the current repository would be removed. I did some testing to check this.

Changes to the help text

It seemed like the right time to also propose changes to the help text for gix clean options directly relevant to the deletion of nested repositories, since I was already planning to do that for -r. In the second commit, this does that, at least an an initial wording that might be possible to improve on, and also applies an analogous change--but with more details, since this is help text--to the change in the warning message.

I made them separate commits so it's easy to take only one (I imagine it would be the first) if not all these changes are wanted at this time.

Commit messages

Even though this only adjusts messages, from this guideline it seems like the commits should have fix: prefixes, so I did that. I'm not sure if that's right. This PR does not carry any behavioral change other than the text of messages that are displayed.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This can be merged once the (and worktrees) portions have been removed.

@EliahKagan
Copy link
Member Author

Sounds good, I'll have a commit that removes the mention of worktrees in a couple of minutes.

@EliahKagan EliahKagan marked this pull request as ready for review July 26, 2024 09:17
@EliahKagan
Copy link
Member Author

EliahKagan commented Jul 26, 2024

I've done that in fcebc44. Because of the way it was done, all three of the commits here are prefixed fix:. Given the effect on the automatically generated release notes, maybe this is not justified. Perhaps a rebase should be done to edit the messages or to squash the commits into one. Squashing seems reasonable since the actual change here is quite minimal; that this is more than one commit is solely a result of the process that led to it.

@EliahKagan EliahKagan changed the title Improve warning and help text about hidden repos and worktrees Improve warning and help text about hidden repos Jul 26, 2024
@Byron
Copy link
Member

Byron commented Jul 26, 2024

I think it's fair to use fix: three times if adding three messages is intended or at least makes sense to you. It's definitely your choice on how to design this.

@EliahKagan EliahKagan changed the title Improve warning and help text about hidden repos Improve help text about hidden repos Jul 26, 2024
@EliahKagan
Copy link
Member Author

EliahKagan commented Jul 26, 2024

I think it's fair to use fix: three times if adding three messages is intended or at least makes sense to you. It's definitely your choice on how to design this.

Each one was something I intended at the time given what had come before, but none of these messages are really ideal now -- they basically all have to be read together.

On the other hand, they do make clear what the progression was in relation to the changes from #1470.

Maybe the best thing to do would be if I were to rebase this to edit the message in the first commit, 7a4d239, to remove fix:. I think the other two make sense together and they avoid claiming a change across versions that is actually absent. Would you be okay with that? Edit: Sorry, the "definitely your choice" wording in your comment actually answers that, and it can be undone if you decide it's not good. I'll push this change.

This slightly expands the wording of the warning suggesting to use
`--skip-hidden-repositories`, so that it notes that worktrees and
not just separate repositories are at stake.
This adds information to the help text for the `git clean` options
`-r`/`--repositories` and `--skip-hidden-repositories` to make it
clearer what their relationship is, avoid creating the false
impression that repositories are never deleted in the absence of
`-r`/`--repositories`, and note that `--skip-hidden-repositories`
is sometimes needed to preserve not only separate repositories but
the (probably rarer) case of hidden nested worktrees of the current
repository.
Since GitoxideLabs#1470 (for GitoxideLabs#1469), the repository's own nested worktrees are
no longer removed when using `gix clean`, even if they are nested
arbitarily under ignored directories. So no new mention of
worktrees (separate from "repositories") in the help and warning
message is accurate or required.

This removes that, which means really we are keeping the warning
message the same as it had been earlier, while still otherwise
bringing in new explanatory text in the `gix clean` options help.
@Byron Byron merged commit a9aac4f into GitoxideLabs:main Jul 26, 2024
19 checks passed
@Byron
Copy link
Member

Byron commented Jul 26, 2024

Thanks again for all the great work!

@EliahKagan EliahKagan deleted the hidden branch July 26, 2024 10:16
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.

2 participants