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

EXCLUDES not working as expected #11

Closed
pablox-cl opened this issue Oct 12, 2013 · 25 comments · Fixed by #58
Closed

EXCLUDES not working as expected #11

pablox-cl opened this issue Oct 12, 2013 · 25 comments · Fixed by #58
Milestone

Comments

@pablox-cl
Copy link

I have the following files in my ~/.dotfiles directory:

config/systemd/user
config/xfce4
systemd

I only want to exclude config/systemd/*, so I've tried the following:

EXCLUDES="config/systemd" 
$ lsrc -v 
/home/pablo/.config/systemd/user/default.target:/home/pablo/.dotfiles/config/systemd/user/default.target
/home/pablo/.config/systemd/user/dropbox@.service:/home/pablo/.dotfiles/config/systemd/user/dropbox@.service
/home/pablo/.config/systemd/user/empty.target:/home/pablo/.dotfiles/config/systemd/user/empty.target
/home/pablo/.config/systemd/user/gnome-do@.service:/home/pablo/.dotfiles/config/systemd/user/gnome-do@.service
/home/pablo/.config/systemd/user/numlockx@.service:/home/pablo/.dotfiles/config/systemd/user/numlockx@.service
[ ... a lot of output removed .... ]
/home/pablo/.systemd:/home/pablo/.dotfiles/systemd
EXCLUDES="systemd" # or "*:systemd"
$ lsrc -v 
/home/pablo/.config/xfce4/......
[ ... a lot of output removed .... ]

Following the instructions for the lsrc man page, I followed the idea of the thoughbot-dotfiles example:

EXCLUDES="config:systemd" # or "config:*systemd*"
$ lsrc -v 
/home/pablo/.config/systemd/user/default.target:/home/pablo/.dotfiles/config/systemd/user/default.target
/home/pablo/.config/systemd/user/dropbox@.service:/home/pablo/.dotfiles/config/systemd/user/dropbox@.service
/home/pablo/.config/systemd/user/empty.target:/home/pablo/.dotfiles/config/systemd/user/empty.target
/home/pablo/.config/systemd/user/gnome-do@.service:/home/pablo/.dotfiles/config/systemd/user/gnome-do@.service
/home/pablo/.config/systemd/user/numlockx@.service:/home/pablo/.dotfiles/config/systemd/user/numlockx@.service
[ ... a lot of output removed .... ]
/home/pablo/.systemd:/home/pablo/.dotfiles/systemd

Same thing... am I misunderstanding how it works?

@pbrisbin
Copy link
Contributor

pbrisbin commented Dec 9, 2013

The excludes pattern is (<source-glob>:)<file-glob>. And I believe file-glob is attempted against the full relative path within the source if it contains a slash, otherwise just the basename.

Your first example says to match "config/system" in any source. The files "config/systemd/user/..." do not match this since the slash makes them full-path checks.

Your second examples both say to match "systemd" in any source. Since this is a basename match, it filters out "config/systemd/*" and "systemd".

Your third example says to match "systemd" and "*systemd" both in the "config" source. This won't work because your source is ".dotfiles", not "config".

I would try this:

EXCLUDES="config/systemd/*"

Interestingly, this is the exact thing you stated in English, but the one permutation you didn't attempt in code :)

@pablox-cl
Copy link
Author

I'm pretty sure I tried that O.o, but oh well. Everything can happen. I'm gonna test it asap.

@pablox-cl
Copy link
Author

I actually test it and doesn't work. It just ignore that particular EXCLUDE directive:

EXCLUDES="hola/chao/*"

$ lsrc -v
recurring on /home/pablo/.hola/chao
/home/pablo/.hola/chao/archivo:/home/pablo/.dotfiles/hola/chao/archivo
/home/pablo/.hola/chao/asdf:/home/pablo/.dotfiles/hola/chao/asdf
/home/pablo/.hola/chao/file:/home/pablo/.dotfiles/hola/chao/file

With debug on:

handling the file archivo
handle_file archivo /home/pablo/.hola/chao /home/pablo/.dotfiles hola/chao 1 README.md systemd zsh.d zprezto dircolors hola/chao/archivo hola/chao/asdf hola/chao/file
is_excluded archivo README.md systemd zsh.d zprezto dircolors hola/chao/archivo hola/chao/asdf hola/chao/file

@pablox-cl pablox-cl reopened this Dec 16, 2013
@mike-burns
Copy link
Contributor

The problem is in handle_file, where it calls is_excluded. It passes the basename instead of the path.

I tried changing it to pass the path:

elif is_excluded "$dotfiles_subdir/$file" "$exclude_file_globs" "$include_file_globs"; then

but then it broke for the case when you don't pass it a glob (in the example you give, it breaks if you pass -x systemd).

While working on this I discovered that the glob can get resolved too easily, and e.g. -x *systemd* will turn into -x systemd as soon as the glob matches something.

@pbrisbin you're a sh quoting expert. Any advice here?

(I just found myself thinking, "if I converted all the strings to binary my life would be easier," and then realized just how crazy that sounds.)

@pbrisbin
Copy link
Contributor

I'm not sure this is a quoting issue. It's true that the potential globs should be quoted while being passed around and can't be quoted in the case evaluation, but that's unrelated, I think.

What about the following logic for is_excluded:

  • Accept full-path as argument to check
  • If exclude pattern contains a slash, match it as a glob against the whole path
  • If it doesn't, match it as a glob against only basename

Would that cover all cases?

@mike-burns
Copy link
Contributor

Yeah, @pbrisbin , that might work.

I'd like someone besides me to take a crack at this; I'm having trouble thinking of a great solution.

@mike-burns
Copy link
Contributor

I don't know how to pass a glob around without it being expanded:

~% ls -d *vim*
vimulator/
~% cat echo.c 
#include <stdio.h>

int main(int argc, char *argv[]) {
  int i;

  for (i = 1; i < argc; i++)
    printf("%s", argv[i]);

  printf("\n");

  return 0;
}
~% gcc -o e echo.c
~% ./e hello
hello
~% ./e *vim*
vimulator

@pbrisbin
Copy link
Contributor

pbrisbin commented Mar 5, 2014

You just have to quote it.

$ touch foo bar baz
$ for g in 'f*' 'b*'; do # strings
>   ls "$g" # still string
>   ls $g   # expandable glob
> done
ls: cannot access f*: No such file or directory
foo
ls: cannot access b*: No such file or directory
bar  baz

@mike-burns
Copy link
Contributor

OK, that resolves that a bit, but even when I quote things lsrc(1) expands them:

~% lsrc -vvv -x '*vim*' 2>&1 | head
TAGS: debian development email git laptop thoughtbot gmail notmuch debian-email gpg
DOTFILES_DIRS: 
DOTFILES_DIRS:  /home/mike/.dotfiles
COPY_ALWAYS: 
SYMLINK_DIRS: 
dotfiles_dir_excludes /home/mike/.dotfiles
  with excludes:  *vim*
exclude_file_globs: vimulator  #### THIS
dotfiles_dir_excludes /home/mike/.dotfiles
  with excludes: 

@pbrisbin
Copy link
Contributor

pbrisbin commented Mar 6, 2014

It's always quoting. I'm going to coin Pat's theorem:

In shell, any unintentional lack of quotation is broken. You just don't know it yet.

And the corollary:

Most intentional cases are broken too.

here:

for exclude in $excludes; do # globs expand
  # ...

There may be a set option to prevent globbing (which you'll have to turn on here, then immediately off again for the case). I don't know it though, or if it's POSIX. Also, this doesn't fix the fundamental problem, the lack of quotation would most likely still be broken in some way. See Pat's theorem ;)

@mike-burns
Copy link
Contributor

How can I iterate over a space-separated list of globs? Where do I put quotes to make this happen?

@mike-burns
Copy link
Contributor

I've opened #54. It doesn't fix this problem at all, but it's a decent start.

@pbrisbin
Copy link
Contributor

pbrisbin commented Mar 6, 2014

(Haven't looked at it, stuck on train but...)

How about concatting the patterns with "|" at arg parsing, then doing a
single glob comparison in the case?

This makes the full-path-vs-basename problem harder though :/

BTW, for x in $thing is always broken and should be avoided, regardless
of quoting.
On Mar 6, 2014 9:35 AM, "Mike Burns" notifications@github.com wrote:

I've opened #54 #54. It doesn't
fix this problem at all, but it's a decent start.

Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-36893061
.

@pbrisbin
Copy link
Contributor

pbrisbin commented Mar 6, 2014

If we have to keep it in a space separated list, something like this may work:

matches_patterns() {
  local file="$1" patterns="$2"

  echo "$patterns" | sed 's/ /\n/g' | while read pattern; do
    # treat as glob, file, base, etc here...

    case "$file" of
      $pattern) return 0 ;;
    esac
  done

  return 1
}

@mike-burns
Copy link
Contributor

Yeah, set -f disables path expansion.

$ foo="a b c *vim*"
$ for z in $foo; do
> echo $z
> done
a
b
c
vimulator
$ set -f
$ for z in $foo; do
> echo $z
> done
a
b
c
*vim*

@mike-burns
Copy link
Contributor

I've opened #55, too, for discussion on that topic.

mike-burns added a commit that referenced this issue Mar 7, 2014
Until we can get a handle on globbing, we cannot close #11. That bug is
the last remaining ticket blocking the 1.2.1 release. However, it may
involve a larger modification than a patch release should. Documenting
allows us to make the release while working on the bug for the next
release.

http://1389blog.com/pix/thats-a-feature-not-a-bug.jpg
mike-burns added a commit that referenced this issue Mar 7, 2014
Until we can get a handle on globbing, we cannot close #11. That bug is
the last remaining ticket blocking the 1.2.1 release. However, it may
involve a larger modification than a patch release should. Documenting
allows us to make the release while working on the bug for the next
release.

http://1389blog.com/pix/thats-a-feature-not-a-bug.jpg
@mike-burns
Copy link
Contributor

I've opened #58. I'd like to push #11 to milestone 1.3.0, merge #58, and release 1.2.1. It seems that #11 will take longer than expected.

@pablox-cl @pbrisbin any objections to this?

@pablox-cl
Copy link
Author

No. It's fine by me. Anyway, I believe for most use cases, the actual implementation works fine. The edge cases or when you want to specify a particular file inside a subdirectory it's when things go eerie.


I know the idea is to keep this project as POSIX as possible, but it seems a POSIX compatible shell wasn't built for this task. Why not using perl, sed or awk to accomplish this?

@mike-burns mike-burns modified the milestones: 1.3.0, 1.2.1 Mar 7, 2014
@mike-burns
Copy link
Contributor

The idea is to keep the project as able-to-run-on-any-machine-out-of-the-box as possible. My targets are (in order) FreeBSD, GNU, OS X, Solaris. This leaves us with POSIX sh and POSIX awk (sed won't suffice).

That said, I'd be willing to entertain the idea of a binary+VM packaged together.

@pablox-cl
Copy link
Author

And what about perl?

I guess my Archlinux distro falls in the GNU category right?

@mike-burns
Copy link
Contributor

Perl is far from standard. FreeBSD is the first example that comes to mind.

The only Linux distros I know of that are not GNU are Android, Busybox, and UNG/Linux.

@mike-burns mike-burns mentioned this issue Jun 2, 2014
@mike-burns mike-burns reopened this Jul 9, 2014
@mike-burns
Copy link
Contributor

This was closed because GitHub is clever.

@mike-burns
Copy link
Contributor

Short of the Haskell rewrite: we could apply the globs after-the-fact in lsrc(1). So collect all files, then go through the list and exclude any that match the appropriate globs.

mike-burns added a commit that referenced this issue Nov 27, 2015
This is based on #11

- Handle an exclude glob with a path.
- Handle an exclude glob without a path.

I am unsure whether we want to maintain compatibility for exclude globs
without a path.
@mike-burns
Copy link
Contributor

Captured in a test on the exclude-globs branch: https://github.com/thoughtbot/rcm/blob/exclude-globs/test/lsrc-excludes.t#L22-L32

LoicMahieu added a commit to LoicMahieu/dotfiles that referenced this issue Dec 15, 2015
Otherwise we can't exclude `packages/*`.... See thoughtbot/rcm#11
Seems to be ok to share userId: atom/metrics#18
mike-burns added a commit that referenced this issue Dec 23, 2016
This is based on #11

- Handle an exclude glob with a path.
- Handle an exclude glob without a path.

I am unsure whether we want to maintain compatibility for exclude globs
without a path.
@mike-burns
Copy link
Contributor

This might be fixed by #192.

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

Successfully merging a pull request may close this issue.

3 participants