Fix handling of filenames with spaces (regression in #179)

[cincodenada]

I just pulled in the latest changes and found that my password list was very broken - I have many password files with spaces in them. I looked into it and found that the changes in #179 make it so such files are broken into multiple entries, one per word, which entirely breaks rofi-pass for any password files that have spaces in them.

Trying to the output from find into a bash array proved troublesome, so since the function wasn't actually using the bash array and just used it to generate a newline-delimited string, I removed the bash array altogether and just used a while loop.

If you look at just the first commit it has an earlier version that uses sed instead and is a little shorter - I have no preference between the two, so feel free to revert the last commit and use that version if it seems better.

#179 also introduced some line-leading spaces, I tidied those up and also grabbed a couple other spaces that had snuck in as well while I was at it.

Edit: I originally referenced #192 here, but upon reading noticed that #192 actually addressed this issue and is almost identical to my first commit using sed, and in addition fixed the issue in listgpg() and updates dependencies, which #179 didn't do.

I've merged the changes from #192 into this PR, and integrated my bash solution. If the sed version is preferred, it's easy enough to swap in.

[carnager]

Nah... the fix is much simpler, the pw_list variable simply needs quotes

carnager@caprica ~/test > pw_list=($(find -L * -name "*.gpg"))       
carnager@caprica ~/test > printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
bar
foo
xxx
carnager@caprica ~/test > pw_list="$(find -L * -name "*.gpg")"        
carnager@caprica ~/test > printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
foo bar xxx

[carnager]

Why not simply quote the original pw_list? Seems to fix it for me

pw_list="$(find -L * -name "*.gpg")"        
printf '%s\n' "${pw_list[@]%.gpg}" | sort -n

[cincodenada]

That's the first thing I tried, but that breaks most of the point of the second line, which is to remove the .gpg suffix:

~/test/ › pw_list=($(find -L * -name "*.gpg"))                                            
~/test/ › printf '%s\n' "${pw_list[@]%.gpg}" | sort -n                                    
bar
foo
xxx
~/test/ › pw_list="$(find -L * -name "*.gpg")"                                            
~/test/ › printf '%s\n' "${pw_list[@]%.gpg}" | sort -n                                    
bar.gpg
foo.gpg
xxx

[carnager]

I can't follow, seems to act just fine here

carnager@caprica ~/test > ls                                          
'foo bar xxx.gpg'
carnager@caprica ~/test > pw_list="$(find -L * -name "*.gpg")"        
carnager@caprica ~/test > printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
foo bar xxx

[cincodenada]

Also, the history on this PR is now kind of a mess, I can rebase things to be less of a mess. I pulled in the changes from #191 and also noticed that list_passwords can just call to listgpg which avoids having this behavior split into two places where someone (like me!) can miss the second implementation and not fix it.

[cincodenada]

Oh, sorry, should have included an ls:

~/test › ls                                                                              
bar.gpg  foo.gpg  xxx.gpg

[carnager]

Hmmm, interesting. I wonder why this happens, since its not depending on spaces...

EDIT Ok, i was fooled by the sort -n, it always only removes the last extension

carnager@caprica ~/test > ls
 bar.gpg  'foo bar xxx.gpg'   foo.gpg
carnager@caprica ~/test > pw_list="$(find -L * -name "*.gpg")"       
carnager@caprica ~/test > printf '%s\n' "${pw_list[@]}" | sort -n     
bar.gpg
foo bar xxx.gpg
foo.gpg
carnager@caprica ~/test > printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
bar.gpg
foo
foo bar xxx.gpg

[cincodenada]

For completeness, I added one with spaces as well:

~/test/› ls
 bar.gpg  'baz quux.gpg'   foo.gpg   xxx.gpg
~/test/› pw_list=($(find -L * -name "*.gpg"))
~/test/› printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
bar
baz
foo
quux
xxx
~/test/› pw_list="$(find -L * -name "*.gpg")"
~/test/› printf '%s\n' "${pw_list[@]%.gpg}" | sort -n
bar.gpg
baz quux.gpg
foo.gpg
xxx
~/test/› find -L * -name "*.gpg" | sed 's/.gpg$//' | sort -n
bar
baz quux
foo
xxx

[cincodenada]

I believe what's happening is that when we quote it, pw_list is no longer a bash array (which is what the parentheses were doing). Instead it's just one long string with a bunch of newlines in it now, so bash's string subsittution treats it as such, and only replaces the last .gpg. Then when we put it back through sort it's still a newline-separated list, with only the last item (before sorting) stripped of its suffix because it was at the end of the string.

Before, pw_list was a bash array, so the string substitution did the replacement on each item in the array, which is the desired behavior.

Does that make sense?

EDIT: Saw your edit, yeah the sort tripped me up for a sec too, but I see we arrived at the same conclusion 🙂

[cincodenada]

(In case you're checking in via email, I've edited my previous comment substantially for clarity)

[cincodenada]

Oh, and circling back around to the original issue with spaces: (**/*.gpg) worked fine, I assume because it was glob expansion and bash knows how to split those items into array items and preserve spaces. ($(find ...)) doesn't work as well, and results in bash splitting it into array items based on any whitespace. One of the first things I tried was just modifying the find to manually quote each line, but bash didn't remove those quotes when converting to an array, so they showed up in the final output which was undesirable.

[carnager]

ok, so I guess a manual loop over find's output is the only way to go for now, which is what you are doing after you removed the sed.

I guess this is fine, cleaning up the PR a bit, this should be good to go

[cincodenada]

Yeah, agreed the loop is awkward, but it's the best solution I found - if you prefer the sed, that's functionally equivalent, it just introduces another dependency.

Do you want to clean up the PR, or shall I? If you are, part of my intention in merging with #191 was to credit its author by having their commit included, but of course do as you wish!

Thanks for the careful attention, and of course for maintaining this thing! It's made using pass so much better over the last few years.

[carnager]

Personally I prefer the loop over subshells. If you are willing to do the clean up I am all for it and yes, of course I want to give credit where credit is due.

And thanks 😃

[cincodenada]

Great, I'll tidy up this evening. Should I just force-push to this PR, or would you rather open a new one?

[cincodenada]

Okay, I've tidied up and pushed to a new branch: master...cincodenada:fix-spaces-tidy

I can force-push that to this branch whenever if it looks good.

[blueyed]

Suggested change

	# get all password files and ouput as newline-delimited text
	# get all password files and output as newline-delimited text

[blueyed]

listgpg sorts it already, so sort -n could be removed here.

Using sed might look better, but should have the dot escaped then: listgpg | sed -e 's/\.gpg$//'.