Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
On Tue, Mar 19, 2013 at 08:58:23AM +0700, Duy Nguyen wrote: > On Tue, Mar 19, 2013 at 1:26 AM, John Keeping wrote: > > On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote: > >> This passes the pathspec, more or less unmodified, to > >> git-add--interactive. The command itself does not process pathspec. It > >> simply passes the pathspec to other builtin commands. So if all those > >> commands support pathspec, we're good. > > > > This breaks "git reset --keep" in a subdirectory for me. > > > > I ran "git reset --keep " in a subdirectory and got: > > > > fatal: BUG: parse_pathspec cannot take no argument in this case > > > > Bisecting points to this commit. > > > > The simplest test case is: > > > > ( cd t && ../bin-wrappers/git reset --keep HEAD ) > > > > which works on master but not pu. > > Beautiful. I got messed up with C operator precedence. This should fix > it. I'll check the rest of parse_pathspec calls later. Yes, this fixes it. Thanks. > diff --git a/builtin/reset.c b/builtin/reset.c > index ab3917d..b665218 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -219,7 +219,7 @@ static void parse_args(struct pathspec *pathspec, > *rev_ret = rev; > parse_pathspec(pathspec, 0, >PATHSPEC_PREFER_FULL | > - patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, > + (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), >prefix, argv); > } > > -- > Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
On Tue, Mar 19, 2013 at 1:26 AM, John Keeping wrote: > On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote: >> This passes the pathspec, more or less unmodified, to >> git-add--interactive. The command itself does not process pathspec. It >> simply passes the pathspec to other builtin commands. So if all those >> commands support pathspec, we're good. > > This breaks "git reset --keep" in a subdirectory for me. > > I ran "git reset --keep " in a subdirectory and got: > > fatal: BUG: parse_pathspec cannot take no argument in this case > > Bisecting points to this commit. > > The simplest test case is: > > ( cd t && ../bin-wrappers/git reset --keep HEAD ) > > which works on master but not pu. Beautiful. I got messed up with C operator precedence. This should fix it. I'll check the rest of parse_pathspec calls later. diff --git a/builtin/reset.c b/builtin/reset.c index ab3917d..b665218 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -219,7 +219,7 @@ static void parse_args(struct pathspec *pathspec, *rev_ret = rev; parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL | - patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, + (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), prefix, argv); } -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote: > This passes the pathspec, more or less unmodified, to > git-add--interactive. The command itself does not process pathspec. It > simply passes the pathspec to other builtin commands. So if all those > commands support pathspec, we're good. This breaks "git reset --keep" in a subdirectory for me. I ran "git reset --keep " in a subdirectory and got: fatal: BUG: parse_pathspec cannot take no argument in this case Bisecting points to this commit. The simplest test case is: ( cd t && ../bin-wrappers/git reset --keep HEAD ) which works on master but not pu. > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > builtin/add.c | 26 ++ > builtin/checkout.c | 9 - > builtin/reset.c| 8 > commit.h | 2 +- > 4 files changed, 19 insertions(+), 26 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index ec6fbe3..2b20d7d 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -139,16 +139,12 @@ static void refresh(int verbose, const char **pathspec) > } > > int run_add_interactive(const char *revision, const char *patch_mode, > - const char **pathspec) > + const struct pathspec *pathspec) > { > - int status, ac, pc = 0; > + int status, ac, i; > const char **args; > > - if (pathspec) > - while (pathspec[pc]) > - pc++; > - > - args = xcalloc(sizeof(const char *), (pc + 5)); > + args = xcalloc(sizeof(const char *), (pathspec->nr + 6)); > ac = 0; > args[ac++] = "add--interactive"; > if (patch_mode) > @@ -156,11 +152,9 @@ int run_add_interactive(const char *revision, const char > *patch_mode, > if (revision) > args[ac++] = revision; > args[ac++] = "--"; > - if (pc) { > - memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc); > - ac += pc; > - } > - args[ac] = NULL; > + for (i = 0; i < pathspec->nr; i++) > + /* pass original pathspec, to be re-parsed */ > + args[ac++] = pathspec->items[i].original; > > status = run_command_v_opt(args, RUN_GIT_CMD); > free(args); > @@ -175,17 +169,17 @@ int interactive_add(int argc, const char **argv, const > char *prefix, int patch) >* git-add--interactive itself does not parse pathspec. It >* simply passes the pathspec to other builtin commands. Let's >* hope all of them support all magic, or we'll need to limit > - * the magic here. There is still a problem with prefix. But > - * that'll be worked on later on. > + * the magic here. >*/ > parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, > PATHSPEC_PREFER_FULL | > -PATHSPEC_SYMLINK_LEADING_PATH, > +PATHSPEC_SYMLINK_LEADING_PATH | > +PATHSPEC_PREFIX_ORIGIN, > prefix, argv); > > return run_add_interactive(NULL, > patch ? "--patch" : NULL, > -pathspec.raw); > +&pathspec); > } > > static int edit_patch(int argc, const char **argv, const char *prefix) > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 3c19cb4..2ddff95 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -256,7 +256,7 @@ static int checkout_paths(const struct checkout_opts > *opts, > > if (opts->patch_mode) > return run_add_interactive(revision, "--patch=checkout", > -opts->pathspec.raw); > +&opts->pathspec); > > lock_file = xcalloc(1, sizeof(struct lock_file)); > > @@ -1115,10 +1115,9 @@ int cmd_checkout(int argc, const char **argv, const > char *prefix) >* cannot handle. Magic mask is pretty safe to be >* lifted for new magic when opts.patch_mode == 0. >*/ > - parse_pathspec(&opts.pathspec, > -opts.patch_mode == 0 ? 0 : > -(PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP), > -0, prefix, argv); > + parse_pathspec(&opts.pathspec, 0, > +opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, > +prefix, argv); > > if (!opts.pathspec.nr) > die(_("invalid path specification")); > diff --git a/builtin/reset.c b/builtin/reset.c > index da7282e..7c6e8b6 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -216,9 +216,9 @@ static void parse_args(struct pathspec *pathspec, > } > } > *rev_ret = rev; > - parse_pathspec(pathspec, > -patch_mode ? PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP : 0, > -PATHSPE
[PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
This passes the pathspec, more or less unmodified, to git-add--interactive. The command itself does not process pathspec. It simply passes the pathspec to other builtin commands. So if all those commands support pathspec, we're good. Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/add.c | 26 ++ builtin/checkout.c | 9 - builtin/reset.c| 8 commit.h | 2 +- 4 files changed, 19 insertions(+), 26 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ec6fbe3..2b20d7d 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -139,16 +139,12 @@ static void refresh(int verbose, const char **pathspec) } int run_add_interactive(const char *revision, const char *patch_mode, - const char **pathspec) + const struct pathspec *pathspec) { - int status, ac, pc = 0; + int status, ac, i; const char **args; - if (pathspec) - while (pathspec[pc]) - pc++; - - args = xcalloc(sizeof(const char *), (pc + 5)); + args = xcalloc(sizeof(const char *), (pathspec->nr + 6)); ac = 0; args[ac++] = "add--interactive"; if (patch_mode) @@ -156,11 +152,9 @@ int run_add_interactive(const char *revision, const char *patch_mode, if (revision) args[ac++] = revision; args[ac++] = "--"; - if (pc) { - memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc); - ac += pc; - } - args[ac] = NULL; + for (i = 0; i < pathspec->nr; i++) + /* pass original pathspec, to be re-parsed */ + args[ac++] = pathspec->items[i].original; status = run_command_v_opt(args, RUN_GIT_CMD); free(args); @@ -175,17 +169,17 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) * git-add--interactive itself does not parse pathspec. It * simply passes the pathspec to other builtin commands. Let's * hope all of them support all magic, or we'll need to limit -* the magic here. There is still a problem with prefix. But -* that'll be worked on later on. +* the magic here. */ parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, PATHSPEC_PREFER_FULL | - PATHSPEC_SYMLINK_LEADING_PATH, + PATHSPEC_SYMLINK_LEADING_PATH | + PATHSPEC_PREFIX_ORIGIN, prefix, argv); return run_add_interactive(NULL, patch ? "--patch" : NULL, - pathspec.raw); + &pathspec); } static int edit_patch(int argc, const char **argv, const char *prefix) diff --git a/builtin/checkout.c b/builtin/checkout.c index 3c19cb4..2ddff95 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -256,7 +256,7 @@ static int checkout_paths(const struct checkout_opts *opts, if (opts->patch_mode) return run_add_interactive(revision, "--patch=checkout", - opts->pathspec.raw); + &opts->pathspec); lock_file = xcalloc(1, sizeof(struct lock_file)); @@ -1115,10 +1115,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) * cannot handle. Magic mask is pretty safe to be * lifted for new magic when opts.patch_mode == 0. */ - parse_pathspec(&opts.pathspec, - opts.patch_mode == 0 ? 0 : - (PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP), - 0, prefix, argv); + parse_pathspec(&opts.pathspec, 0, + opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, + prefix, argv); if (!opts.pathspec.nr) die(_("invalid path specification")); diff --git a/builtin/reset.c b/builtin/reset.c index da7282e..7c6e8b6 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -216,9 +216,9 @@ static void parse_args(struct pathspec *pathspec, } } *rev_ret = rev; - parse_pathspec(pathspec, - patch_mode ? PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP : 0, - PATHSPEC_PREFER_FULL, + parse_pathspec(pathspec, 0, + PATHSPEC_PREFER_FULL | + patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, prefix, argv); } @@ -296,7 +296,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (patch_mode) { if (reset_type != NONE) die(_("--patch is incompatible with --{hard,mixed,soft}")); - return run_add_interactive(sha1_to_hex(sha1), "