[PATCH 0/3] Updates to git-add--interactive

[PATCH 0/3] Updates to git-add--interactive

[Wincent Colaiuta]

Here's an updated series, this time hopefully with the braindead
parts removed.

[PATCH 1/3] Add "--patch" option to git-add--interactive

    Unchanged.

[PATCH 2/3] Move pathspec validation into interactive_add

    Braindeadness removed.

[PATCH 3/3] Rename patch_update_file function to patch_update_pathspec

    Only modified insofar as to make it apply on top of PATCH 2.

The one remaining issue is that pathspecs like "\*.h" don't work.
But from comments earlier on in this thread I had the impression
that they should.

They don't work because such pathspecs don't work for the underlying
git-diff-files implementation that's used by git-add--interactive:

git diff-files --numstat --summary -- \*.h

Is some alternative being proposed that would allow them to work?

[PATCH 1/3] Rename patch_update_file function to patch_update_pathspec

[Wincent Colaiuta]

The patch_update_file function really works on pathspecs, not files, so
rename it to reflect its actual purpose.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 git-add--interactive.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e347216..15b2c9f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -571,11 +571,11 @@ sub patch_update_cmd {
 				       HEADER => $status_head, },
 				     @mods);
 	for (@them) {
-		patch_update_file($_->{VALUE});
+		patch_update_pathspec($_->{VALUE});
 	}
 }
 
-sub patch_update_file {
+sub patch_update_pathspec {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
-- 
1.5.3.6.1994.g38001

[PATCH 2/3] Move pathspec validation into interactive_add

[Wincent Colaiuta]

Simplify git-add--interactive by moving the pathspec validation into the
interactive_add() function of builtin-add. We can do this because
builtin-add is the only caller of git-add--interactive.

The validate_pathspec() function added by this commit is based on a
sample posted to the mailing list by Junio Hamano.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-add.c             |   33 +++++++++++++++++++++++++++++----
 builtin-commit.c          |    2 +-
 commit.h                  |    2 +-
 git-add--interactive.perl |   12 ++----------
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index dd895df..870f4a1 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -135,12 +135,37 @@ static void refresh(int verbose, const char **pathspec)
         free(seen);
 }
 
-int interactive_add(int argc, const char **argv)
+static int validate_pathspec(const char *prefix, const char **patterns)
+{
+	int i, ret = 0;
+	char *m;
+	if (!patterns || !*patterns)
+		return 0;
+	if (read_cache() < 0)
+		die("index file corrupt");
+	(void)get_pathspec(prefix, patterns);
+	for (i = 0; patterns[i]; i++)
+		;
+	m = xcalloc(1, i);
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		(void)pathspec_match(patterns, m, ce->name, 0);
+	}
+	ret = report_path_error(m, patterns, prefix ? strlen(prefix) : 0);
+	free(m);
+	return ret;
+}
+
+int interactive_add(const char *prefix, int argc, const char **argv)
 {
 	int status;
-	const char **args = xmalloc(sizeof(const char *) * (argc + 1));
+	const char **args;
+	if ((status = validate_pathspec(prefix, argv)))
+		return status;
+	args = xmalloc(sizeof(const char *) * (argc + 2));
 	args[0] = "add--interactive";
-	memcpy((void *)args + sizeof(const char *), argv, sizeof(const char *) * argc);
+	memcpy((void *)args + sizeof(const char *),
+	    argv, sizeof(const char *) * argc);
 	args[argc + 1] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
@@ -176,7 +201,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
 	if (add_interactive)
-		exit(interactive_add(argc, argv));
+		exit(interactive_add(prefix, argc, argv));
 
 	git_config(git_default_config);
 
diff --git a/builtin-commit.c b/builtin-commit.c
index 5d27102..95d1c0d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -165,7 +165,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	const char **pathspec = NULL;
 
 	if (interactive) {
-		interactive_add(argc, argv);
+		interactive_add(prefix, argc, argv);
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
 	}
diff --git a/commit.h b/commit.h
index 9f0765b..dc6fe31 100644
--- a/commit.h
+++ b/commit.h
@@ -113,7 +113,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 
 int in_merge_bases(struct commit *, struct commit **, int);
 
-extern int interactive_add(int argc, const char **argv);
+extern int interactive_add(const char *prefix, int argc, const char **argv);
 extern int rerere(void);
 
 static inline int single_parent(struct commit *commit)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 15b2c9f..381bcbe 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -56,17 +56,9 @@ sub list_modified {
 	my ($only) = @_;
 	my (%data, @return);
 	my ($add, $del, $adddel, $file);
-	my @tracked = ();
-
-	if (@ARGV) {
-		@tracked = map {
-			chomp $_; $_;
-		} run_cmd_pipe(qw(git ls-files --exclude-standard --), @ARGV);
-		return if (!@tracked);
-	}
 
 	for (run_cmd_pipe(qw(git diff-index --cached
-			     --numstat --summary HEAD --), @tracked)) {
+			     --numstat --summary HEAD --), @ARGV)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			my ($change, $bin);
@@ -89,7 +81,7 @@ sub list_modified {
 		}
 	}
 
-	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
+	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @ARGV)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			if (!exists $data{$file}) {
-- 
1.5.3.6.1994.g38001

[PATCH 3/3] Add "--patch" option to git-add--interactive

[Wincent Colaiuta]

When the "--patch" option is supplied, the patch_update_file function is
called, once for each supplied pathspec argument, and then we exit.

Seeing as builtin-add is the only caller of git-add--interactive we can
impose a strict requirement on the format of the arguments to avoid
possible ambiguity: an "--" argument must be used whenever any pathspecs
are passed, both with the "--patch" option and without it.

This commit adds an early return mechanism to the patch_update_pathspec
function to prevent spurious line feeds from being echoed when the user
passes in pathspecs which match unchanged files.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-add.txt |    9 ++++++++-
 builtin-add.c             |   17 ++++++++++-------
 git-add--interactive.perl |   35 ++++++++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 63829d9..ce22de8 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -61,7 +61,14 @@ OPTIONS
 
 -i, \--interactive::
 	Add modified contents in the working tree interactively to
-	the index.
+	the index. Optional path arguments may be supplied to limit
+	operation to a subset of the working tree. See ``Interactive
+	mode'' for details.
+
+-p, \--patch:
+	Similar to Interactive mode but the initial command loop is
+	bypassed and the 'patch' subcommand is invoked using each of
+	the specified filepatterns before exiting.
 
 -u::
 	Update only files that git already knows about. This is similar
diff --git a/builtin-add.c b/builtin-add.c
index 870f4a1..2172e7e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -19,7 +19,7 @@ static const char * const builtin_add_usage[] = {
 	"git-add [options] [--] <filepattern>...",
 	NULL
 };
-
+static int patch_interactive = 0, add_interactive = 0;
 static int take_worktree_changes;
 
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
@@ -158,15 +158,18 @@ static int validate_pathspec(const char *prefix, const char **patterns)
 
 int interactive_add(const char *prefix, int argc, const char **argv)
 {
-	int status;
+	int status, pre_argc;
 	const char **args;
 	if ((status = validate_pathspec(prefix, argv)))
 		return status;
-	args = xmalloc(sizeof(const char *) * (argc + 2));
+	pre_argc = patch_interactive ? 3 : 2;
+	args = xmalloc(sizeof(const char *) * (argc + pre_argc + 1));
 	args[0] = "add--interactive";
-	memcpy((void *)args + sizeof(const char *),
+	args[1]	= patch_interactive ? "--patch" : "--";
+	args[2] = "--";
+	memcpy((void *)args + sizeof(const char *) * pre_argc,
 	    argv, sizeof(const char *) * argc);
-	args[argc + 1] = NULL;
+	args[argc + pre_argc] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
 	free(args);
@@ -179,13 +182,13 @@ static const char ignore_error[] =
 "The following paths are ignored by one of your .gitignore files:\n";
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int add_interactive = 0;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only),
 	OPT__VERBOSE(&verbose),
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
+	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
@@ -200,7 +203,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
-	if (add_interactive)
+	if (add_interactive || patch_interactive)
 		exit(interactive_add(prefix, argc, argv));
 
 	git_config(git_default_config);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 381bcbe..7d62ceb 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,9 @@
 
 use strict;
 
+# command line options
+my $patch;
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -335,7 +338,8 @@ sub add_untracked_cmd {
 
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path)
+	    or return undef;
 	my (@hunk) = { TEXT => [] };
 
 	for (@diff) {
@@ -571,6 +575,7 @@ sub patch_update_pathspec {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
+	return unless $head;
 	for (@{$head->{TEXT}}) {
 		print;
 	}
@@ -775,6 +780,20 @@ add untracked - add contents of untracked files to the staged set of changes
 EOF
 }
 
+sub process_args {
+	return unless @ARGV;
+	my $arg = shift @ARGV;
+	if ($arg eq "--patch") {
+		$patch = 1;
+		$arg = shift @ARGV or die "missing --";
+		die "invalid argument $arg, expecting --"
+		    unless $arg eq "--";
+	}
+	elsif ($arg ne "--") {
+		die "invalid argument $arg, expecting --";
+	}
+}
+
 sub main_loop {
 	my @cmd = ([ 'status', \&status_cmd, ],
 		   [ 'update', \&update_cmd, ],
@@ -803,6 +822,16 @@ sub main_loop {
 	}
 }
 
+process_args();
 refresh();
-status_cmd();
-main_loop();
+if ($patch) {
+	print "No filepattern specified: what did you want to patch?\n"
+	    unless @ARGV;
+	foreach my $pathspec (@ARGV) {
+		patch_update_pathspec($pathspec);
+	}
+}
+else {
+	status_cmd();
+	main_loop();
+}
-- 
1.5.3.6.1994.g38001

Re: [PATCH 1/3] Rename patch_update_file function to patch_update_pathspec

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> The patch_update_file function really works on pathspecs, not files, so
> rename it to reflect its actual purpose.

I do not think this is a correct change.  patch_update_file() should
work on files not pathspecs.

A list of exact pathnames are valid pathspecs, and because the matching
function takes exact match first, if you feed a command
(e.g. diff-files) paths expanded from the pathspec given by the user,
you should be able to get desired results, no?

patch_update_cmd() grabs list of modified _files_, not pathspecs.  Then
the user chooses the ones to select hunks from via list_and_choose(),
and you may want to pretend that everything was chosen if you are
operating with pathspec and with --patch option.  Iterating over them is
iterating over files, not pathspecs, at that point.

Have you tried the one that is in 'next'?  I think it does the right
thing except "the bogus pathspec validation at the beginning" part as
far as the pathspec handling is concerned.

I sent out two fixes on top of the series.

[PATCH] builtin-add: fix command line building to call interactive

[Junio C Hamano]

The earlier 7c0ab4458994aa895855abc4a504cf693ecc0cf1 (Teach builtin-add
to pass multiple paths to git-add--interactive) did not allocate enough,
and had unneeded (void*) pointer arithmetic.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin-add.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index dd895df..7c6a296 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -138,9 +138,10 @@ static void refresh(int verbose, const char **pathspec)
 int interactive_add(int argc, const char **argv)
 {
 	int status;
-	const char **args = xmalloc(sizeof(const char *) * (argc + 1));
+	const char **args = xcalloc(sizeof(const char *), (argc + 2));
+
 	args[0] = "add--interactive";
-	memcpy((void *)args + sizeof(const char *), argv, sizeof(const char *) * argc);
+	memcpy(&(args[1]), argv, sizeof(const char *) * argc);
 	args[argc + 1] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
-- 
1.5.3.6.2014.g7500f

Re: [PATCH] builtin-add: fix command line building to call interactive

[Wincent Colaiuta]

El 25/11/2007, a las 19:07, Junio C Hamano escribió:

> The earlier 7c0ab4458994aa895855abc4a504cf693ecc0cf1 (Teach builtin- 
> add
> to pass multiple paths to git-add--interactive) did not allocate  
> enough,

Yes, it was off by one; sorry about that. You may have noticed that I  
fixed that up in the patches I sent out yesterday and today. May need  
to redo them now to apply on top of this.

Cheers,
Wincent

Re: [PATCH] builtin-add: fix command line building to call interactive

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> El 25/11/2007, a las 19:07, Junio C Hamano escribi󺊊> The earlier 7c0ab4458994aa895855abc4a504cf693ecc0cf1 (Teach builtin- 
>> add
>> to pass multiple paths to git-add--interactive) did not allocate  
>> enough,
>
> Yes, it was off by one; sorry about that. You may have noticed that I  
> fixed that up in the patches I sent out yesterday and today. May need  
> to redo them now to apply on top of this.

I'd suggest you to slow down, apply the two patches on top of 'next' and
take a look at the result.

I _think_ the only remaining thing is --patch, and none of the pathspec
thing is needed.

Re: [PATCH] builtin-add: fix command line building to call interactive

[Wincent Colaiuta]

El 25/11/2007, a las 19:36, Junio C Hamano escribió:

> I _think_ the only remaining thing is --patch, and none of the  
> pathspec
> thing is needed.

You're probably right; the pathspec validation is probably not  
necessary (and you may recall that my original patch didn't include  
it; I only tried adding after you said it might be appropriate to have  
the "--error-unmatch" behaviour). This is probably more convenient for  
the user, as it allows them to pass "sloppy" parameters like the  
following:

	git-add -i *.h

(Note that's "*.h" and not "\*.h"). In the Git repository, without  
validation, this just works. With strict validation, it would complain:

	error: pathspec 'common-cmds.h' did not match any file(s) known to git.

So just forgetting about the validation is probably the right thing to  
do.

As for adding the --patch option, I'll stand back and see if someone  
more skilled than I wants to do it; should only be a few lines and  
will save traffic to the list because they'll probably get it right  
first time.

Cheers,
Wincent

Re: [PATCH] builtin-add: fix command line building to call interactive

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

>>  I _think_ the only remaining thing is --patch, and none of the  
>> pathspec
>> thing is needed.
>
> You're probably right; the pathspec validation is probably not  
> necessary (and you may recall that my original patch didn't include  
> it; I only tried adding after you said it might be appropriate to have  
> the "--error-unmatch" behaviour)...

Oh, I did not mean the validation bits I left out in the second "fixup"
patch I sent earlier by that comment.  What I meant was the changes you
had in git-add--interactive::patch_update_cmd() to make it take non
files but pathspecs.

> This is probably more convenient for  
> the user, as it allows them to pass "sloppy" parameters like the  
> following:
>
> 	git-add -i *.h
>
> (Note that's "*.h" and not "\*.h"). In the Git repository, without  
> validation, this just works. With strict validation, it would complain...

I'd mostly agree, but we need to realize that this is a two edged sword.
Pathspecs can be leading-directories or fileglobs.  For fileglobs, you
are right.  The user can let the shell do the globbing.  Not validating,
however, also means that

	git add -p Documentatoin

would report "there is nothing to patch" without being helpful, pointing
out that the name of the directory is misspelled.

> As for adding the --patch option, I'll stand back and see if someone  
> more skilled than I wants to do it; should only be a few lines and  
> will save traffic to the list because they'll probably get it right  
> first time.

And that skilled person turns out to be you ;-).

At the end of this message is your 3/3 from yesterday, minimally
adjusted to the current tip of 'next'.

I tweaked it to allow:

	git add -p

without any pathspecs, which I think is a valid usage.

-- >8 --
[PATCH] Add "--patch" option to git-add--interactive

When the "--patch" option is supplied, the patch_update_cmd() function is
called bypassing the main_loop() and exits.

Seeing as builtin-add is the only caller of git-add--interactive we can
impose a strict requirement on the format of the arguments to avoid
possible ambiguity: an "--" argument must be used whenever any pathspecs
are passed, both with the "--patch" option and without it.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-add.txt |    9 +++++++-
 builtin-add.c             |   24 ++++++++++++++-------
 git-add--interactive.perl |   51 ++++++++++++++++++++++++++++++++++++--------
 3 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 63829d9..ce22de8 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -61,7 +61,14 @@ OPTIONS
 
 -i, \--interactive::
 	Add modified contents in the working tree interactively to
-	the index.
+	the index. Optional path arguments may be supplied to limit
+	operation to a subset of the working tree. See ``Interactive
+	mode'' for details.
+
+-p, \--patch:
+	Similar to Interactive mode but the initial command loop is
+	bypassed and the 'patch' subcommand is invoked using each of
+	the specified filepatterns before exiting.
 
 -u::
 	Update only files that git already knows about. This is similar
diff --git a/builtin-add.c b/builtin-add.c
index 865c475..5c29cc2 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -19,7 +19,7 @@ static const char * const builtin_add_usage[] = {
 	"git-add [options] [--] <filepattern>...",
 	NULL
 };
-
+static int patch_interactive = 0, add_interactive = 0;
 static int take_worktree_changes;
 
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
@@ -144,7 +144,7 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p
 
 int interactive_add(int argc, const char **argv, const char *prefix)
 {
-	int status;
+	int status, ac;
 	const char **args;
 	const char **pathspec = NULL;
 
@@ -154,11 +154,17 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 			return -1;
 	}
 
-	args = xcalloc(sizeof(const char *), (argc + 2));
-	args[0] = "add--interactive";
-	if (argc)
-		memcpy(&(args[1]), pathspec, sizeof(const char *) * argc);
-	args[argc + 1] = NULL;
+	args = xcalloc(sizeof(const char *), (argc + 4));
+	ac = 0;
+	args[ac++] = "add--interactive";
+	if (patch_interactive)
+		args[ac++] = "--patch";
+	args[ac++] = "--";
+	if (argc) {
+		memcpy(&(args[ac]), pathspec, sizeof(const char *) * argc);
+		ac += argc;
+	}
+	args[ac] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
 	free(args);
@@ -171,13 +177,13 @@ static const char ignore_error[] =
 "The following paths are ignored by one of your .gitignore files:\n";
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int add_interactive = 0;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only),
 	OPT__VERBOSE(&verbose),
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
+	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
@@ -192,6 +198,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
+	if (patch_interactive)
+		add_interactive = 1;
 	if (add_interactive)
 		exit(interactive_add(argc, argv, prefix));
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e347216..df5df3e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,9 @@
 
 use strict;
 
+# command line options
+my $patch_mode;
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -552,8 +555,8 @@ sub help_patch_cmd {
 	print <<\EOF ;
 y - stage this hunk
 n - do not stage this hunk
-a - stage this and all the remaining hunks
-d - do not stage this hunk nor any of the remaining hunks
+a - stage this and all the remaining hunks in the file
+d - do not stage this hunk nor any of the remaining hunks in the file
 j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
@@ -563,13 +566,21 @@ EOF
 }
 
 sub patch_update_cmd {
-	my @mods = list_modified('file-only');
-	@mods = grep { !($_->{BINARY}) } @mods;
-	return if (!@mods);
+	my @mods = grep { !($_->{BINARY}) } list_modified('file-only');
+	my @them;
 
-	my (@them) = list_and_choose({ PROMPT => 'Patch update',
-				       HEADER => $status_head, },
-				     @mods);
+	if (!@mods) {
+		print STDERR "No changes.\n";
+		return 0;
+	}
+	if ($patch_mode) {
+		@them = @mods;
+	}
+	else {
+		@them = list_and_choose({ PROMPT => 'Patch update',
+					  HEADER => $status_head, },
+					@mods);
+	}
 	for (@them) {
 		patch_update_file($_->{VALUE});
 	}
@@ -783,6 +794,20 @@ add untracked - add contents of untracked files to the staged set of changes
 EOF
 }
 
+sub process_args {
+	return unless @ARGV;
+	my $arg = shift @ARGV;
+	if ($arg eq "--patch") {
+		$patch_mode = 1;
+		$arg = shift @ARGV or die "missing --";
+		die "invalid argument $arg, expecting --"
+		    unless $arg eq "--";
+	}
+	elsif ($arg ne "--") {
+		die "invalid argument $arg, expecting --";
+	}
+}
+
 sub main_loop {
 	my @cmd = ([ 'status', \&status_cmd, ],
 		   [ 'update', \&update_cmd, ],
@@ -811,6 +836,12 @@ sub main_loop {
 	}
 }
 
+process_args();
 refresh();
-status_cmd();
-main_loop();
+if ($patch_mode) {
+	patch_update_cmd();
+}
+else {
+	status_cmd();
+	main_loop();
+}
-- 
1.5.3.6.2018.g84394

Re: [PATCH] builtin-add: fix command line building to call interactive

[Jeff King]

On Sun, Nov 25, 2007 at 11:48:28AM -0800, Junio C Hamano wrote:

> > 	git-add -i *.h
> >
> > (Note that's "*.h" and not "\*.h"). In the Git repository, without  
> > validation, this just works. With strict validation, it would complain...
> 
> I'd mostly agree, but we need to realize that this is a two edged sword.
> Pathspecs can be leading-directories or fileglobs.  For fileglobs, you
> are right.  The user can let the shell do the globbing.  Not validating,
> however, also means that
> 
> 	git add -p Documentatoin
> 
> would report "there is nothing to patch" without being helpful, pointing
> out that the name of the directory is misspelled.

I think the problem there is not validation, but that the previous
proposal was validating the wrong thing. IOW, the user doesn't want a
complaint "this file is not tracked by git" (which catches untracked
things with *.h) but rather "this file does not even exist" (which
catches typos like Documentatoin).

So it is not really a git pathspec being provided (from the user's point
of view), but rather something else (a pathspec _or_ a working tree
file).

-Peff

[PATCH] add -i: Fix running from a subdirectory

[Junio C Hamano]

This fixes the pathspec interactive_add() passes to the underlying
git-add--interactive helper.  When the command was run from a
subdirectory, cmd_add() already has gone up to the toplevel of the work
tree, and the helper will be spawned from there.  The pathspec given on
the command line from the user needs to be adjusted for this.

This adds "validate_pathspec()" function in the callchain, but it does
not validate yet.  The function can be changed to barf if there are
unmatching pathspec given by the user, but that is not strictly
necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The reason it is not "strictly necessary" is because if you give
   unmatching pathspecs, you simply would not see their effects (good or
   bad) in any of the subcommands in git-add--interactive.

 builtin-add.c    |   24 ++++++++++++++++++++----
 builtin-commit.c |    2 +-
 commit.h         |    2 +-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 7c6a296..865c475 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -135,13 +135,29 @@ static void refresh(int verbose, const char **pathspec)
         free(seen);
 }
 
-int interactive_add(int argc, const char **argv)
+static const char **validate_pathspec(int argc, const char **argv, const char *prefix)
+{
+	const char **pathspec = get_pathspec(prefix, argv);
+
+	return pathspec;
+}
+
+int interactive_add(int argc, const char **argv, const char *prefix)
 {
 	int status;
-	const char **args = xcalloc(sizeof(const char *), (argc + 2));
+	const char **args;
+	const char **pathspec = NULL;
+
+	if (argc) {
+		pathspec = validate_pathspec(argc, argv, prefix);
+		if (!pathspec)
+			return -1;
+	}
 
+	args = xcalloc(sizeof(const char *), (argc + 2));
 	args[0] = "add--interactive";
-	memcpy(&(args[1]), argv, sizeof(const char *) * argc);
+	if (argc)
+		memcpy(&(args[1]), pathspec, sizeof(const char *) * argc);
 	args[argc + 1] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
@@ -177,7 +193,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
 	if (add_interactive)
-		exit(interactive_add(argc, argv));
+		exit(interactive_add(argc, argv, prefix));
 
 	git_config(git_default_config);
 
diff --git a/builtin-commit.c b/builtin-commit.c
index 5d27102..45e51b1 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -165,7 +165,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	const char **pathspec = NULL;
 
 	if (interactive) {
-		interactive_add(argc, argv);
+		interactive_add(argc, argv, prefix);
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
 	}
diff --git a/commit.h b/commit.h
index 9f0765b..10e2b5d 100644
--- a/commit.h
+++ b/commit.h
@@ -113,7 +113,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 
 int in_merge_bases(struct commit *, struct commit **, int);
 
-extern int interactive_add(int argc, const char **argv);
+extern int interactive_add(int argc, const char **argv, const char *prefix);
 extern int rerere(void);
 
 static inline int single_parent(struct commit *commit)
-- 
1.5.3.6.2014.g7500f

Re: [PATCH v2] git-add--interactive pathspec and patch additions

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> The series implements these changes in seven steps that apply on top of
> "master"; these patches are rebased/squashed ones which *replace* the
> ones sent the other day:

That's very unfortunate, as some already usable bits from the
series are already in 'next'.

>     1. Add -q/--quiet switch to git-ls-files
>
> Needed because run_cmd_pipe() doesn't propagate the child exit status
> and system() likes to be chatty on the standard out. Of the possible
> workarounds adding this switch seems to be the cleanest and most
> portable.

I do not like this very much.  If it is a problem that
run_cmd_pipe() does not properly signal you an error, wouldn't
it be much better to fix _that_ problem?  That way we do not
have to add kludge to all commands we need to run and get the
exit status out of.

>     2. Rename patch_update_file function to patch_update_pathspec
>
> Merely cosmetic.
>
>     3. Add path-limiting to git-add--interactive
>     4. Bail if user supplies an invalid pathspec

On the first read, I did not quite like 4, but I'd agree it is
probably the cleanest implementation for 3 to reject a wrong
invocation early.

>     5. Teach builtin-add to pass path arguments to git-add--interactive

I think this is already in 'next'.

>     6. Add "--patch" option to git-add--interactive
>     7. Teach builtin-add to handle "--patch" option

These should be straightforward, but use of Getopt::Long feels
way overkill for an internal command like add--interactive which
is called by only a very limited known callers (exactly one).

If we assume "a single caller", we probably can do without 1 and
4, by making the caller in builtin-add to validate the list of
pathspecs, reusing the code for "ls-files --error-unmatch",
before calling the external helper "add--interactive".

There are functions refactored as part of the builtin-commit
series to be usable from outside "ls-files", and you can build a
imple function called from interactive_add(ac, av) using them:

static int validate_pathspec(const char *prefix, const char **pattern)
{
	int i, ret;
	char *m;

	if (!pattern || !*pattern)
        	return 0;

	for (i = 0; pattern[i]; i++)
		;
	m = xcalloc(1, i);

	for (i = 0; i < active_nr; i++) {
		struct cache_entry *ce = active_cache[i];
                (void) pathspec_match(pattern, m, ce->name, 0);
	}
        ret = report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
        free(m);
        return ret;
}

[PATCH 0/3] Updates to git-add--interactive

[Wincent Colaiuta]

Based on feedback from Junio last night here is a new series. This one
goes on top of the "next" branch.

I moved the pathspec validity checking into builtin-add, thus
simplifying things considerably. I also enforced some strict assumptions
about how the optional arguments will be passed from builtin-add to
git-add--interactive; I can do this because it is the only caller, and
it once again makes things a bit simpler.

I don't expect this to actually be applied as-is, as there are some
doubts I have which I have added to the emails for each patch. In
particular there is a performance issue with the pathspec validity
checking (no idea why it is so much slower than just invoking
git-ls-files), and the behaviour is not exactly where I'd like it to be
(things like "git-add -i ." don't work because "." is considered
invalid).

So looking for feedback on how to address this issues and will resubmit.

[PATCH 1/3] Rename patch_update_file function to patch_update_pathspec

[Wincent Colaiuta]

The patch_update_file function really works on pathspecs, not files, so
rename it to reflect its actual purpose.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 git-add--interactive.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e347216..15b2c9f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -571,11 +571,11 @@ sub patch_update_cmd {
 				       HEADER => $status_head, },
 				     @mods);
 	for (@them) {
-		patch_update_file($_->{VALUE});
+		patch_update_pathspec($_->{VALUE});
 	}
 }
 
-sub patch_update_file {
+sub patch_update_pathspec {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
-- 
1.5.3.6.1992.gd646-dirty