-
Notifications
You must be signed in to change notification settings - Fork 41
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
Special processing for dotfiles #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a fantastic start - thanks a lot! Actually, there's really very little left to do, other than fix the small issues I've commented on here. But please do squash both commits into a single commit and then force push to amend this pull request, because code and corresponding tests should always belong together to keep the CI happy and the git history intuitive.
|
||
my @result = (); | ||
for my $part (split m{/+}, $target) { | ||
if ($part ne "dot-") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the attention to detail here :-) I think you need to also defend against dot-.
though, as that would currently be turned into ..
.
@@ -193,6 +193,20 @@ sub restore_cwd { | |||
chdir($prev) or error("Your current directory $prev seems to have vanished"); | |||
} | |||
|
|||
sub adjust_dotfile { | |||
my $target = $_[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my ($target) = @_;
would be more consistent with the result of Stow's code style.
@@ -377,6 +378,13 @@ sub stow_contents { | |||
next NODE if $node eq '..'; | |||
my $node_target = join_paths($target, $node); | |||
next NODE if $self->ignore($stow_path, $package, $node_target); | |||
|
|||
if ($self->{dotfiles}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use spaces to indent, not tabs, to stay consistent with the rest of the code base.
@@ -744,6 +753,13 @@ sub unstow_contents { | |||
next NODE if $node eq '..'; | |||
my $node_target = join_paths($target, $node); | |||
next NODE if $self->ignore($stow_path, $package, $node_target); | |||
|
|||
if ($self->{dotfiles}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use spaces not tabs here too.
@@ -801,6 +817,12 @@ sub unstow_node { | |||
# Does the existing $target actually point to anything? | |||
if (-e $existing_path) { | |||
# Does link points to the right place? | |||
|
|||
# Adjust for dotfile if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use spaces not tabs here too.
@@ -617,6 +625,7 @@ sub unstow_contents_orig { | |||
next NODE if $node eq '..'; | |||
my $node_target = join_paths($target, $node); | |||
next NODE if $self->ignore($stow_path, $package, $node_target); | |||
# $node_target = adjust_dotfile($node_target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this line, but it will be cancelled out by the next commit when you squash the two commits into one anyway, so that's OK.
@@ -0,0 +1,72 @@ | |||
#!/usr/local/bin/perl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding a new file, you also need to update MANIFEST
. That's the reason that the Travis CI builds are currently failing for this pull request.
a672135
to
af8a4d6
Compare
@aspiers Thanks for the insightful comments! I think I've addressed them all, but there might be other things still missing (for instance, I should write some docs, and do something about the decrease in test coverage). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from that indentation, it looks perfect now - thanks so much!
@@ -744,6 +752,13 @@ sub unstow_contents { | |||
next NODE if $node eq '..'; | |||
my $node_target = join_paths($target, $node); | |||
next NODE if $self->ignore($stow_path, $package, $node_target); | |||
|
|||
if ($self->{dotfiles}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation here. I'll see if GitHub lets me correct it myself though.
Hmm no, it seems that you have to enable the |
af8a4d6
to
bf6700d
Compare
bf6700d
to
5691124
Compare
5691124
to
ac17611
Compare
@aspiers Thanks for the swift feedback and sorry for the oversight. I've fixed it (and enabled the option for future use, if necessary). I also added some more tests for some corner cases in the dot expansion, and I wrote a small summary of what the option does. One remaining sore spot is the decrease in coverage. I need to figure out what lines aren't being hit, and then write a separate test for them. I'll do so as soon as possible. |
ac17611
to
182acbb
Compare
@aspiers I added a simple unstow test. Please let me know if you want to see any further changes, and thanks again for all the reviewing. |
This feature uses
I also found these files on my Mac: On the other hand, in one Emacs package I found On Alpine Linux or Ubuntu I didn't find any files with either prefix. |
FWIW, I would favor the |
It seems to be that there could be too-open debate on what the right prefix should be. Would it be better coming in as a configuration parameter optionally, with a reasonable default? |
This is a PR out of the patch that I submitted to stow-devel to allow for special processing for dotfiles. I use GNU stow to manage my dotfiles, but found it a bit hard to maintain an apparently empty Stow folder, which is full of hidden files.
To address this, the idea is to have a command-line option
--dotfiles
that would have Stow check if the files/folders to stow start withdot-
(though I should make the prefix configurable...). In this case, Stow replaces thedot-
prefix by an actual dot (.
), turning the link target into a proper hidden file.I am not a skilled Perl developer, so the code is probably rough around the edges. However, if this is a viable approach then I'm definitely up for polishing it and bringing it up to standards.
One corner case that I haven't explored yet is the interplay between this kind of processing and folding. I imagine that the dotfiles option would disable folding, but perhaps there is a way to have both.