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

Glob fails to link if there's only one file #282

Closed
jwpeddle opened this issue May 29, 2021 · 7 comments
Closed

Glob fails to link if there's only one file #282

jwpeddle opened this issue May 29, 2021 · 7 comments
Labels

Comments

@jwpeddle
Copy link

When I try to use glob with only one file in the source directory, dotbot fails to create the link with "Nonexistent source for.."

- defaults:
    link:
      relink: true
      glob: true
      force: true
- link:
    ~/.config/broot: broot/*
.
├── broot
│   └── conf.hjson
❯ ./install 
Nonexistent source for ~/.config/broot : /home/jay/dotfiles/broot/*

==> Some tasks were not executed successfully

Adding a second file without changing anything else makes it function as expected. I would expect it to behave the same regardless of how many files are in the dir.

Why bother using glob if there's only one file? Because it means if I add a second file later, I don't need to also edit my install.conf.yaml. Almost always, I just want to symlink whatever is in the source directory. It took some squinting to even identify what was going wrong.

@eengstrom
Copy link
Contributor

I can confirm this with a new test case I created, and that it worked as of the last tagged version (v.1.19.0), and failed after a patch (that I wrote) was merged into master. I will develop a fix.

@anishathalye
Copy link
Owner

anishathalye commented Jun 2, 2021

Thanks for looking into it!

@jwpeddle for the moment, you could use Dotbot v1.19.0 (cd dotbot; git checkout v1.19.0; cd ..; git add dotbot; git commit, to make sure the ./install doesn't revert back to whichever version you had pinned).

@eengstrom
Copy link
Contributor

I have a fix of a sort - line 73 of link.py goes from:

                elif len(glob_results) == 1 and destination[-1] != '/':

to

               elif len(glob_results) == 1 and (glob_results[0] == path):

... but I'm not sure the reason one would want to specify glob: true and yet not use any glob meta-chars. Essentially, the entire elif stanza seems like it hides errors in usage and tries to guess the/a right thing to do. The immediately preceding stanza does the same thing, but throws an error if the result is a directory.

Unless there is a clear use-case for allowing the use of glob: true and source path specifications without any glob meta-chars, I'd suggest always throwing an error.

I suppose the possible use case is when people want to put glob: true into a defaults block, but then again, the code is still hiding errors in use that aught to be corrected.

Looking for feedback.

@jwpeddle
Copy link
Author

jwpeddle commented Jun 3, 2021

@jwpeddle for the moment, you could use Dotbot v1.19.0 (cd dotbot; git checkout v1.19.0; cd ..; git add dotbot; git commit, to make sure the ./install doesn't revert back to whichever version you had pinned).

Done, and confirmed working, thanks! 🎈

@niraami
Copy link

niraami commented Jul 13, 2021

+1 on this one

but I'm not sure the reason one would want to specify glob: true and yet not use any glob meta-chars

Well, I guess the that's the fault of the question. IMO it's better to ask if specifying glob: true without using it could in any way lead to unexpected behaviour - I'm not familiar in the way dotbot handles globs, but I'd like to think that there isn't.
But then there is also the question, if using /* on a folder with a single file is really considered not using any glob meta-chars?

There are a few pro's I can think of right away:

  1. If a folder once included multiple files, but you later deleted all but one, the install script is still going to function as expected
  2. Consistency with globs elsewhere (bash, zsh, etc). As long as there is at least 1 file, you can use /* to rm, cp, mv, etc.

I don't really have any other examples, as I rarely utilize globs.

The way I found this issue is that I recently removed one of my Polybar scripts, and after re-installing my dotfiles on another machine, the whole scripts/ folder was suddenly gone. This was because in the end, I was left with only 1 file in scripts/ & this yaml syntax:

~/.config/polybar/scripts:
      glob: true
      path: config/polybar/scripts/*

Thus I got here. Well, after finally noticing what dotbot was even talking about with the "Some tasks were not executed successfully" - which leads onto my next point.

It took some squinting to even identify what was going wrong.

This is also very valid, there is no color/formatting difference between information & warnings/errors. I don't have a screenshot on hand, but I can get one if needed.

@LeuschkeTressa
Copy link

Came here after scratching my head for quiet a while.

This bug was reported more than two years ago. Is dotbot still being maintained?

If there is anyone with commit access still in the project, may I suggest to immediately add the 2-3 lines of code that would be needed to identify when the unexpected behaviour is triggered, and print a warning message - however short - to the console? That would be fair to the users in my opinion, respecting their time and possibly saving someone some frustration.

(Duplicate: #315)

./install --version
Dotbot version 1.19.1 (git b'53b3781fbb')

anishathalye added a commit that referenced this issue Jul 9, 2023
See #282 and
#315.

This patch simplifies the implementation, removing special-case handling
for the cases of zero matches and one match. Instead, any situation
where `glob: true` is specified and the path contains a glob character
(any of "?", "*", or "[") is treated as a glob case. The reason we check
both `use_glob` and `_has_glob_chars()` is to more gracefully handle the
case where the user has enabled globs by default, but most links do not
contain glob characters and should not be treated as globs.
@anishathalye
Copy link
Owner

Thanks, should be fixed in 416f32f (released in 1.19.2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants
@jwpeddle @eengstrom @anishathalye @niraami @LeuschkeTressa and others