| Message ID | 52e7a84a2ee61d6481286f6a32751107efc234d0.1572895605.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add --pathspec-from-file option for reset, commit | expand |
Hi Alexandr Thanks for working on this. I've got a couple of comments about improving the error messages but this looks fine to me On 04/11/2019 19:26, Alexandr Miloslavskiy via GitGitGadget wrote: > From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > > This will be used to support the new option '--pathspec-from-file' in > `git add`, `git-commit`, `git reset` etc. > > Note also that we specifically handle CR/LF line endings to support > Windows better. > > To simplify code, file is first parsed into `argv_array`. This allows > to avoid refactoring `parse_pathspec()`. > > I considered adding `nul_term_line` to `flags` instead, but decided > that it doesn't fit there. > > The new code is mostly taken from `cmd_update_index()` and > `split_mail_conv()`. > > Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > --- > pathspec.c | 41 +++++++++++++++++++++++++++++++++++++++++ > pathspec.h | 10 ++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/pathspec.c b/pathspec.c > index 12c2b322b3..97d4e77875 100644 > --- a/pathspec.c > +++ b/pathspec.c > @@ -3,6 +3,8 @@ > #include "dir.h" > #include "pathspec.h" > #include "attr.h" > +#include "argv-array.h" > +#include "quote.h" > > /* > * Finds which of the given pathspecs match items in the index. > @@ -613,6 +615,45 @@ void parse_pathspec(struct pathspec *pathspec, > } > } > > +void parse_pathspec_file(struct pathspec *pathspec, unsigned magic_mask, > + unsigned flags, const char *prefix, > + const char *file, int nul_term_line) > +{ > + struct argv_array parsed_file = ARGV_ARRAY_INIT; > + strbuf_getline_fn getline_fn = nul_term_line ? strbuf_getline_nul : > + strbuf_getline; > + struct strbuf buf = STRBUF_INIT; > + struct strbuf unquoted = STRBUF_INIT; > + FILE *in = NULL; > + > + if (!strcmp(file, "-")) > + in = stdin; > + else > + in = fopen(file, "r"); > + > + if (!in) > + die(_("could not open '%s' for reading"), file); die_errno() would give a more informative message here > + while (getline_fn(&buf, in) != EOF) { > + if (!nul_term_line && buf.buf[0] == '"') { > + strbuf_reset(&unquoted); > + if (unquote_c_style(&unquoted, buf.buf, NULL)) > + die(_("line is badly quoted")); It would be nice to show the offending line in the error message > + strbuf_swap(&buf, &unquoted); > + } > + argv_array_push(&parsed_file, buf.buf); > + strbuf_reset(&buf); > + } > + > + strbuf_release(&unquoted); > + strbuf_release(&buf); > + if (in != stdin) > + fclose(in); > + > + parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.argv); > + argv_array_clear(&parsed_file); > +} > + > void copy_pathspec(struct pathspec *dst, const struct pathspec *src) > { > int i, j; > diff --git a/pathspec.h b/pathspec.h > index 1c18a2c90c..d72e0b4c4a 100644 > --- a/pathspec.h > +++ b/pathspec.h > @@ -85,6 +85,16 @@ void parse_pathspec(struct pathspec *pathspec, > unsigned flags, > const char *prefix, > const char **args); > +/* > + * Same as parse_pathspec() but uses file as input. > + * When 'file' is exactly "-" it uses 'stdin' instead. > + */ > +void parse_pathspec_file(struct pathspec *pathspec, > + unsigned magic_mask, > + unsigned flags, > + const char *prefix, > + const char *file, > + int nul_term_line); Do these align with the 's' in "struct pathspec" ? Best Wishes Phillip > void copy_pathspec(struct pathspec *dst, const struct pathspec *src); > void clear_pathspec(struct pathspec *); > >
On 05.11.2019 16:02, Phillip Wood wrote: >> + if (!in) >> + die(_("could not open '%s' for reading"), file); > > die_errno() would give a more informative message here Thanks for the pointer! >> + if (unquote_c_style(&unquoted, buf.buf, NULL)) >> + die(_("line is badly quoted")); > > It would be nice to show the offending line in the error message Good idea. >> +void parse_pathspec_file(struct pathspec *pathspec, >> + unsigned magic_mask, >> + unsigned flags, >> + const char *prefix, >> + const char *file, >> + int nul_term_line); > > Do these align with the 's' in "struct pathspec" ? Sorry, still struggling with my VS that tries to do crazy things. Will fix.
I think I have implemented all suggestions in PatchV2. Thanks!
diff --git a/pathspec.c b/pathspec.c index 12c2b322b3..97d4e77875 100644 --- a/pathspec.c +++ b/pathspec.c @@ -3,6 +3,8 @@ #include "dir.h" #include "pathspec.h" #include "attr.h" +#include "argv-array.h" +#include "quote.h" /* * Finds which of the given pathspecs match items in the index. @@ -613,6 +615,45 @@ void parse_pathspec(struct pathspec *pathspec, } } +void parse_pathspec_file(struct pathspec *pathspec, unsigned magic_mask, + unsigned flags, const char *prefix, + const char *file, int nul_term_line) +{ + struct argv_array parsed_file = ARGV_ARRAY_INIT; + strbuf_getline_fn getline_fn = nul_term_line ? strbuf_getline_nul : + strbuf_getline; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; + FILE *in = NULL; + + if (!strcmp(file, "-")) + in = stdin; + else + in = fopen(file, "r"); + + if (!in) + die(_("could not open '%s' for reading"), file); + + while (getline_fn(&buf, in) != EOF) { + if (!nul_term_line && buf.buf[0] == '"') { + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) + die(_("line is badly quoted")); + strbuf_swap(&buf, &unquoted); + } + argv_array_push(&parsed_file, buf.buf); + strbuf_reset(&buf); + } + + strbuf_release(&unquoted); + strbuf_release(&buf); + if (in != stdin) + fclose(in); + + parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.argv); + argv_array_clear(&parsed_file); +} + void copy_pathspec(struct pathspec *dst, const struct pathspec *src) { int i, j; diff --git a/pathspec.h b/pathspec.h index 1c18a2c90c..d72e0b4c4a 100644 --- a/pathspec.h +++ b/pathspec.h @@ -85,6 +85,16 @@ void parse_pathspec(struct pathspec *pathspec, unsigned flags, const char *prefix, const char **args); +/* + * Same as parse_pathspec() but uses file as input. + * When 'file' is exactly "-" it uses 'stdin' instead. + */ +void parse_pathspec_file(struct pathspec *pathspec, + unsigned magic_mask, + unsigned flags, + const char *prefix, + const char *file, + int nul_term_line); void copy_pathspec(struct pathspec *dst, const struct pathspec *src); void clear_pathspec(struct pathspec *);