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

Avoid creating local files for "matchless" patterns #1470

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@shashurup

shashurup commented Jun 2, 2016

When there is no match for a pattern fabric creates empty local file
with pattern name.

Avoid creating local files for "matchless" patterns
When there is no match for a pattern fabric creates empty local file
with pattern name.
@mmalchuk

This comment has been minimized.

mmalchuk commented Jun 28, 2016

+1 this fix is really needed
also closes #935

@bitprophet bitprophet added this to the 1.10.4 milestone Jul 3, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2016

Actually, this is not quite the right fix, it hides the error that is naturally raised on no glob matches.

The real bug is in fabric.sftp.SFTP.glob.

Wrote a test to prove this issue exists & another one to enforce the "errors on glob miss" behavior; will merge shortly.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2016

Also I don't see that this does fix #935 really, it doesn't make directory globbing function or document that it's not implemented =/

bitprophet added a commit that referenced this pull request Aug 22, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2016

Put in a "better" fix that updates sftp.glob to return empty list when no matches, then an explicit test for that situation in get which raises a similar IOError to the old behavior.

I also tested OpenSSH's sftp to make sure that it does itself error out when globs fail to find anything - and they do. (Otherwise I might have just omitted the explicit IOError...)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2016

Grump, that breaks other stuff (like grabbing whole directories). So glad I am getting rid of this horrible get/put shite in 2.0 :( Reimplementing SFTP/rsync has been nothing but pain.

It looks like I can address the other breakage by removing the isdir test entirely; this all smells like organically decomposing code. Certainly, grabbing paths which are directories, still works recursively now, with no need to use globbing to obtain their contents (which I suspect was the original goal of that statement).

@shashurup

This comment has been minimized.

shashurup commented Aug 23, 2016

I agree that returning just empty list from glob() would be resonable. However it is considered natural glob behaviour to return the pattern itself when there are no matches, take a look, for example, at bash.

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