Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec

[John Keeping <john@keeping.me.uk>]

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 <branch>" 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 <pclouds@gmail.com>
> ---
>  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), "--patch=reset", pathspec.raw);
> +		return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", &pathspec);
>  	}
>  
>  	/* git reset tree [--] paths... can be used to
> diff --git a/commit.h b/commit.h
> index 4138bb4..9448fda 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -179,7 +179,7 @@ int in_merge_bases(struct commit *, struct commit *);
>  
>  extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
>  extern int run_add_interactive(const char *revision, const char *patch_mode,
> -			       const char **pathspec);
> +			       const struct pathspec *pathspec);
>  
>  static inline int single_parent(struct commit *commit)
>  {
> -- 
> 1.8.0.rc0.19.g7bbb31d