Conversation
wezm
left a comment
wezm
left a comment
There was a problem hiding this comment.
Thanks for this. I think the concept is good overall. Do you have a motivating example this allows that the current behaviour did not?
To merge this, the out-of-the-box behaviour would need to match how git-grab works now. That way if someone upgrades, their setup continues to work. The new default configuration doesn't currently do that. For example, currently git grab --dry-run https://forge.wezm.net/wezm/cports-updates would clone to ~/src/forge.wezm.net/wezm/cports-updates. However, with these changes it clones to ~/src/forge.wezm.net/wezm, which is not what I'd expect.
| --home (deprecated) [default: $GRAB_HOME] | ||
| The ~ character or {home} placeholder in the pattern expands to this | ||
| directory. |
There was a problem hiding this comment.
This description doesn't match the behaviour of --home in the current version, which I think should be preserved.
There was a problem hiding this comment.
The --pattern argument now supersedes the --home argument. If a user configures a pattern that does not start with a ~ character or use the {home} placeholder, --home will have no effect.
Here's the current description
The directory to use as "grab home", where the URLs will cloned into. Overrides the GRAB_HOME environment variable if set.
I believe the original description might be confusing in this context, so I feel the updated version is more accurate.
| #[cfg(not(feature = "clipboard"))] | ||
| const SUPPORTS_CLIPBOARD: bool = false; | ||
|
|
||
| pub struct GrabPattern(pub String); |
There was a problem hiding this comment.
Ideally the pattern would be parsed into the components here, so that we know a GrabPattern instance is valid if constructed.
| "/src/{host/}{owner/}{repo}", | ||
| "git://c9x.me/qbe.git", | ||
| "/src/c9x.me/qbe", | ||
| ), | ||
| ( | ||
| "/src/{host}/{owner}/{repo}", | ||
| "git://c9x.me/qbe.git", | ||
| "/src/c9x.me//qbe", | ||
| ), |
There was a problem hiding this comment.
Since the version without the / in the placeholders is the same, is there any reason to support slashes in the placeholders?
There was a problem hiding this comment.
They do behave differently. It's a little hard to see but visible in the test cases. In the examples, /src/{host/}{owner/}{repo} would evaluate to /src/c9x.me/qbe, whereas /src/{host}/{owner}/{repo} evaluates to /src/c9x.me//qbe. Note the double // character in the second example. Since some variables/placeholders may not be extractable based on the URL, I think this makes sense.
An alternative would be to automatically remove double slashes. This leads to less verbose configuration but feels a little more "magic" to me. I'm not sure if there is ever a case where a user would want the slash to be preserved.
| let url_components = extract_url_components(url); | ||
| let home_string = home.as_ref().map(|p| p.to_string_lossy().to_string()); | ||
|
|
||
| while !remaining.is_empty() { |
There was a problem hiding this comment.
There's some extra validation required here. --pattern currently seems to accept empty strings or strings with only whitespace.
|
Thank you for taking the time to review this and respond so quickly. The motivation behind this was that for my own use, I do not care much about the host domain of the git repositories, since I almost exclusively interact with GitHub as a forge. Personally, I wanted target paths like Regarding backwards compatibility, you are absolutely correct. I intended it to be backwards compatible but must have messed up somewhere. I will update the PR so that the code is, in fact, backwards compatible. |
|
I've updated the pull request so that the default behavior now aligns with the existing version. I also added some more test cases that hopefully convey this backwards compatibility. Two of your comments remain only partially addressed; please let me know if you disagree with my approach on these. |
|
I've given this some more thought and additional review and I don't think I'm going to merge this change. The pattern feature adds complexity and edge cases that I'm not sure are worth the flexibility it allows. The current version of For the specific use case you have of wanting to not organise by host I think this is simple and self contained enough to implement on its own. There could be a Line 71 in ac38409 |
|
Hi wezm, no worries at all. I completely understand not wanting to increase complexity. None of my time was wasted. I’ve been using my fork for the past few months and am happy to continue doing so even if this isn’t merged. Thanks for letting me know 🙏 |
2 participants
This PR introduces a
--patternargument (andGRAB_PATTERNenv var) as an alternative to the--homeargument andGRAB_HOMEenv variable, allowing users to customize destination paths move heavily using variables extracted from the git url.I did my best to match the style of the existing code and ensure there are no clippy issues. Please let me know if there's anything I should change :)