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

Bad checking of the --pattern argument #3982

Closed
elibarzilay opened this issue Jul 7, 2017 · 4 comments · Fixed by #11782
Closed

Bad checking of the --pattern argument #3982

elibarzilay opened this issue Jul 7, 2017 · 4 comments · Fixed by #11782
Assignees
Labels
Milestone

Comments

@elibarzilay
Copy link
Contributor

First of all, the --pattern argument description is giving very little
information: "used for files globbing"?? I had to try it out to see
how it's used.

Second, the way that it's implemented is probably going to be surprising
for most people. I'm guessing that almost everyone would expect a
pattern like x* to match all files that begin with x, but it looks
like the pattern is actually matched against the full path that is
copied, and therefore a pattern like x* will either copy a whole
toplevel directory that starts with x or nothing at all if there is
none.

Related to this, it should be clear exactly what gets matched. In an
attempt to avoid bogus matches in a list that iterates over a list of
globs, I prepended */ to each one (yes, that was a mistake) which
resulted in skipping a toplevel index.html file, and this is
regardless of specifying a full path as my --source or cding into
the directory and using a --source .. So I'm guessing that this
pattern is matched against the full path without the prefix --source
directory (which is the same as the full container + / + blob name
without the part that is specified in --destination). Summarizing
this in a short CLI description is going to be difficult, but it is
very needed.

Yet another bit that must be documented is the fact that * matches
any character, including /. It would have been much better to
have * not match /s, and instead add a ** pattern for that.

Given that there are probably scripts that rely on this behavior of
--pattern (at least the script that I wrote does so), a way to resolve
this would be to add some new --file-pattern that gets matched only
against the basename of files, and not against any directories. Maybe
also another --path-pattern flag with * not matching /s, and **
that does.

@tjprescott
Copy link
Member

@troydai can speak to more detail, but you can use the --dryrun flag to see what files/blobs would be targeted with --pattern without actually performing the operation.

@yonzhan yonzhan added this to the S165 milestone Dec 23, 2019
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 23, 2019

add to S165.

@Juliehzl
Copy link
Contributor

Juliehzl commented Jan 6, 2020

Hi @elibarzilay, thanks a lot for your feedback. Have you tried --dryrun flag to see the summary of target files?
I will add more detailed description and additional examples for --pattern usage. If you have any suggestion, feel free to let me know.

@Juliehzl Juliehzl modified the milestones: S165, S164 Jan 6, 2020
@elibarzilay
Copy link
Contributor Author

@juhee0202: first, a dryrun flag is useful, but shouldn't come at the expense of documenting things. Second, I think that it should be very clear how the match is done, see my comments in the original issue text. For example, saying that * matches anything including / is useful -- but would ? match / too? What about [!x]? Will there be a match if the pattern matches any part of the name, or must it match from the start, from the end, or maybe it must match the whole thing?

I see in your PR that you added some text and some examples, but IMO it's still not answering many of these important questions?

(Extended note: the reason dryrun is insufficient: when you write some script to do something, you want to know that it will continue to behave as it should in the future. So a dry run on some patterns with some current state of my storage will not be sufficient.)

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

Successfully merging a pull request may close this issue.

5 participants