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

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

[Wincent Colaiuta]

This is a revised series based on the feedback and suggestions
received to date. The purpose of the series is to implement these
two user-visible changes:

    1. "git-add --interactive" now takes optional pathspec parameters
       which can be used to limit the scope of an interactive session
    2. We now have "git-add --patch" which takes you straight to the
       "patch" subcommand using the given pathspecs and then exits

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:

    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.

    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

For consistency with many other Git commands. It also shields us from
errors if the user starts passing funny pathspecs; eg. consider what
happens if the user passes a non-existent file "foo", and that
eventually hits git-diff-files; note how the error message changes
depending on where "foo" appears in the list:

$ git diff-files --numstat --summary --
1       0       git-commit.sh
$ git diff-files --numstat --summary -- .
1       0       git-commit.sh
$ git diff-files --numstat --summary -- . foo
error: Could not access ''
$ git diff-files --numstat --summary -- foo .
error: Could not access 'foo'
$ git diff-files --numstat --summary -- . . foo
1       0       git-commit.sh

    5. Teach builtin-add to pass path arguments to git-add--interactive
    6. Add "--patch" option to git-add--interactive
    7. Teach builtin-add to handle "--patch" option

And that's all. Here's the diff stat:

 Documentation/git-add.txt         |    9 +++++-
 Documentation/git-ls-files.txt    |    7 ++++-
 builtin-add.c                     |   28 ++++++++++++--------
 builtin-ls-files.c                |   12 ++++++++-
 commit.h                          |    2 +-
 git-add--interactive.perl         |   52 +++++++++++++++++++++++++++++++-----
 t/t3020-ls-files-error-unmatch.sh |   12 ++++++++
 7 files changed, 99 insertions(+), 23 deletions(-)

[PATCH 1/7] Add -q/--quiet switch to git-ls-files

[Wincent Colaiuta]

When using the --error-unmatch switch to git-ls-files you may only be
interested in the exit status rather than the actual listing that would
normally be printed to the stdout.

To make this kind of workflow easier, the -q/--quiet switch suppresses
all non-error output.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-ls-files.txt    |    7 ++++++-
 builtin-ls-files.c                |   12 +++++++++++-
 t/t3020-ls-files-error-unmatch.sh |   12 ++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 2ec0c0d..33481b6 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -9,7 +9,7 @@ git-ls-files - Show information about files in the index and the working tree
 SYNOPSIS
 --------
 [verse]
-'git-ls-files' [-z] [-t] [-v]
+'git-ls-files' [-z] [-t] [-v] [-q]
 		(--[cached|deleted|others|ignored|stage|unmerged|killed|modified])\*
 		(-[c|d|o|i|s|u|k|m])\*
 		[-x <pattern>|--exclude=<pattern>]
@@ -107,6 +107,11 @@ OPTIONS
 	Similar to `-t`, but use lowercase letters for files
 	that are marked as 'always matching index'.
 
+-q|--quiet::
+	Suppress all non-error output. This may be useful in conjunction
+	with --error-unmatch when you are only interested in the exit
+	status.
+
 --full-name::
 	When run from a subdirectory, the command usually
 	outputs paths relative to the current directory.  This
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 7f60709..6c710a8 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -20,6 +20,7 @@ static int show_unmerged;
 static int show_modified;
 static int show_killed;
 static int show_valid_bit;
+static int quiet;
 static int line_terminator = '\n';
 
 static int prefix_len;
@@ -83,6 +84,8 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 	if (pathspec && !match(pathspec, ps_matched, ent->name, len))
 		return;
 
+	if (quiet)
+		return;
 	fputs(tag, stdout);
 	write_name_quoted(ent->name + offset, stdout, line_terminator);
 }
@@ -205,6 +208,8 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		tag = alttag;
 	}
 
+	if (quiet)
+		return;
 	if (!show_stage) {
 		fputs(tag, stdout);
 	} else {
@@ -385,7 +390,8 @@ static void overlay_tree(const char *tree_name, const char *prefix)
 }
 
 static const char ls_files_usage[] =
-	"git-ls-files [-z] [-t] [-v] (--[cached|deleted|others|stage|unmerged|killed|modified])* "
+	"git-ls-files [-z] [-t] [-v] [-q|--quiet] "
+	"(--[cached|deleted|others|stage|unmerged|killed|modified])* "
 	"[ --ignored ] [--exclude=<pattern>] [--exclude-from=<file>] "
 	"[ --exclude-per-directory=<filename> ] [--exclude-standard] "
 	"[--full-name] [--abbrev] [--] [<file>]*";
@@ -423,6 +429,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 				show_valid_bit = 1;
 			continue;
 		}
+		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
+			quiet = 1;
+			continue;
+		}
 		if (!strcmp(arg, "-c") || !strcmp(arg, "--cached")) {
 			show_cached = 1;
 			continue;
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index c83f820..5178322 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -23,5 +23,17 @@ test_expect_success \
     'git ls-files --error-unmatch should succeed eith matched paths.' \
     'git ls-files --error-unmatch foo bar'
 
+test_expect_success \
+    'git ls-files -q --error-unmatch should be quiet with unmatched path.' \
+    'git ls-files -q --error-unmatch foo bar-does-not-match 1> out 2> err ||
+     test $(cat out | wc -l) -eq 0 &&
+     test $(cat err | wc -l) -gt 0'
+
+test_expect_success \
+   'git ls-files -q --error-unmatch should be quiet with matched paths.' \
+   'git ls-files -q --error-unmatch foo bar 1> out 2> err ||
+    test $(cat out | wc -l) -eq 0 &&
+    test $(cat err | wc -l) -eq 0'
+
 test_done
 1
-- 
1.5.3.6.886.g3364

[PATCH 2/7] 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 fb1e92a..81575e5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -564,10 +564,10 @@ sub patch_update_cmd {
 				     IMMEDIATE => 1,
 				     HEADER => $status_head, },
 				   @mods);
-	patch_update_file($it->{VALUE}) if ($it);
+	patch_update_pathspec($it->{VALUE}) if ($it);
 }
 
-sub patch_update_file {
+sub patch_update_pathspec {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
-- 
1.5.3.6.886.g3364

[PATCH 3/7] Add path-limiting to git-add--interactive

[Wincent Colaiuta]

Implement Junio's suggestion that git-add--interactive should reproduce the
path-limiting semantics of non-interactive git-add.

In otherwords, if "git add -i" (unrestricted) shows paths from a set A,
"git add -i paths..." should show paths from a subset of the set A and that
subset should be defined with the existing ls-files pathspec semantics.

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 81575e5..5fc48a5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -37,7 +37,7 @@ sub list_untracked {
 		chomp $_;
 		$_;
 	}
-	run_cmd_pipe(qw(git ls-files --others --exclude-standard --), @_);
+	run_cmd_pipe(qw(git ls-files --others --exclude-standard --), @ARGV);
 }
 
 my $status_fmt = '%12s %12s %s';
@@ -58,7 +58,7 @@ sub list_modified {
 	my ($add, $del, $adddel, $file);
 
 	for (run_cmd_pipe(qw(git diff-index --cached
-			     --numstat --summary HEAD))) {
+			     --numstat --summary HEAD --), @ARGV)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			my ($change, $bin);
@@ -81,7 +81,7 @@ sub list_modified {
 		}
 	}
 
-	for (run_cmd_pipe(qw(git diff-files --numstat --summary))) {
+	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.886.g3364

[PATCH 4/7] Bail if user supplies an invalid pathspec

[Wincent Colaiuta]

Use git-ls-files to detect whether the user has supplied an invalid
pathspec.

Note that the run_cmd_pipe function unfortunately does not transmit the
exit status of the child process, so there is no way to use it in
conjunction with a command like "git-ls-files --error-unmatch".

The alternative is to use Perl's system() function, but that floods the
standard output. So here we make use of the new -q switch to git-ls-files
so that we can both obtain the exit status of the command and not have to
worry about the standard output. Error conditions are still echoed back to
the user because they come through on the standard error.

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5fc48a5..8706528 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -775,6 +775,12 @@ add untracked - add contents of untracked files to the staged set of changes
 EOF
 }
 
+sub check_args {
+	return unless @ARGV;
+	exit $? if system(qw(git ls-files -q --exclude-standard
+	    --error-unmatch --with-tree=HEAD --), @ARGV);
+}
+
 sub main_loop {
 	my @cmd = ([ 'status', \&status_cmd, ],
 		   [ 'update', \&update_cmd, ],
@@ -804,5 +810,6 @@ sub main_loop {
 }
 
 refresh();
+check_args();
 status_cmd();
 main_loop();
-- 
1.5.3.6.886.g3364

[PATCH 5/7] Teach builtin-add to pass path arguments to git-add--interactive

[Wincent Colaiuta]

The previous patches in the series taught git-add--interactive to handle
optional path arguments. This patch teaches builtin-add to pass
these arguments through to the script.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-add.txt |    4 +++-
 builtin-add.c             |   23 +++++++++++++----------
 commit.h                  |    2 +-
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 63829d9..0b0ab1d 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -61,7 +61,9 @@ 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 files in the working tree.
+	See ``Interactive mode'' for details.
 
 -u::
 	Update only files that git already knows about. This is similar
diff --git a/builtin-add.c b/builtin-add.c
index cf815a0..3646a45 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -135,11 +135,17 @@ static void refresh(int verbose, const char **pathspec)
         free(seen);
 }
 
-int interactive_add(void)
+int interactive_add(int argc, const char **argv)
 {
-	const char *argv[2] = { "add--interactive", NULL };
-
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	int status;
+	const char **args = xmalloc(sizeof(const char *) * (argc + 1));
+	args[0] = "add--interactive";
+	memcpy((void *)args + sizeof(const char *), argv, sizeof(const char *) * argc);
+	args[argc + 1] = NULL;
+
+	status = run_command_v_opt(args, RUN_GIT_CMD);
+	free(args);
+	return status;
 }
 
 static struct lock_file lock_file;
@@ -163,17 +169,14 @@ static struct option builtin_add_options[] = {
 
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd, orig_argc = argc;
+	int i, newfd = argc;
 	const char **pathspec;
 	struct dir_struct dir;
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
-	if (add_interactive) {
-		if (add_interactive != 1 || orig_argc != 2)
-			die("add --interactive does not take any parameters");
-		exit(interactive_add());
-	}
+	if (add_interactive)
+		exit(interactive_add(argc, argv));
 
 	git_config(git_default_config);
 
diff --git a/commit.h b/commit.h
index aa67986..d82b8bc 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(void);
+extern int interactive_add(int argc, const char **argv);
 extern void add_files_to_cache(int verbose, const char *prefix, const char **files);
 extern int rerere(void);
 
-- 
1.5.3.6.886.g3364

[PATCH 6/7] 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.

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>
---
 git-add--interactive.perl |   35 ++++++++++++++++++++++++++++++++---
 1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8706528..43a5344 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,6 +1,10 @@
 #!/usr/bin/perl -w
 
 use strict;
+use Getopt::Long;
+
+# command line options
+my $patch;
 
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
@@ -335,7 +339,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 +576,7 @@ sub patch_update_pathspec {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
+	return unless $head;
 	for (@{$head->{TEXT}}) {
 		print;
 	}
@@ -781,6 +787,19 @@ sub check_args {
 	    --error-unmatch --with-tree=HEAD --), @ARGV);
 }
 
+sub usage {
+	print <<EOT;
+git-add--interactive [options] [--] [<filepattern>...]
+Options:
+     -p, --patch        Execute the "patch" subcommand and exit
+EOT
+	exit(1);
+}
+
+sub process_options {
+	GetOptions("patch!" => \$patch) or usage();
+}
+
 sub main_loop {
 	my @cmd = ([ 'status', \&status_cmd, ],
 		   [ 'update', \&update_cmd, ],
@@ -809,7 +828,17 @@ sub main_loop {
 	}
 }
 
+process_options();
 refresh();
 check_args();
-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.886.g3364

[PATCH 7/7] Teach builtin-add to handle "--patch" option

[Wincent Colaiuta]

Now builtin-add can take a "--patch" option; when found, it passes control
over to git-add--interactive, passing the --patch switch and the supplied
pathspecs.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-add.txt |    5 +++++
 builtin-add.c             |   13 ++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 0b0ab1d..1acf10f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -65,6 +65,11 @@ OPTIONS
 	operation to a subset of the files in 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
 	to what "git commit -a" does in preparation for making a commit,
diff --git a/builtin-add.c b/builtin-add.c
index 3646a45..c8335f1 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -19,6 +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;
 
@@ -138,10 +139,12 @@ 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));
+	int pre_argc = patch_interactive ? 2 : 1;
+	const char **args = xmalloc(sizeof(const char *) * (argc + pre_argc + 1));
 	args[0] = "add--interactive";
-	memcpy((void *)args + sizeof(const char *), argv, sizeof(const char *) * argc);
-	args[argc + 1] = NULL;
+	args[1] = "--patch";
+	memcpy((void *)args + sizeof(const char *) * pre_argc, argv, sizeof(const char *) * argc);
+	args[argc + pre_argc] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
 	free(args);
@@ -154,13 +157,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"),
@@ -175,7 +178,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(argc, argv));
 
 	git_config(git_default_config);
-- 
1.5.3.6.886.g3364

Re: [PATCH 1/7] Add -q/--quiet switch to git-ls-files

[Wincent Colaiuta]

El 23/11/2007, a las 20:20, Wincent Colaiuta escribió:

> +test_expect_success \
> +    'git ls-files -q --error-unmatch should be quiet with unmatched  
> path.' \
> +    'git ls-files -q --error-unmatch foo bar-does-not-match 1> out  
> 2> err ||
> +     test $(cat out | wc -l) -eq 0 &&
> +     test $(cat err | wc -l) -gt 0'
> +
> +test_expect_success \
> +   'git ls-files -q --error-unmatch should be quiet with matched  
> paths.' \
> +   'git ls-files -q --error-unmatch foo bar 1> out 2> err ||
> +    test $(cat out | wc -l) -eq 0 &&
> +    test $(cat err | wc -l) -eq 0'
> +
> test_done

Doh, in that last test it should be:

+   'git ls-files -q --error-unmatch foo bar 1> out 2> err &&

Instead of:

+   'git ls-files -q --error-unmatch foo bar 1> out 2> err ||

Cheers,
Wincent

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;
}

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

[Wincent Colaiuta]

El 23/11/2007, a las 22:07, Junio C Hamano escribió:

> 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'.

Well, no problem, I'll redo this tomorrow if I can get some free time.  
Otherwise it will have to wait to Monday.

>>    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.

The problem is that run_cmd_pipe has two code paths, one for Windows  
and one for all the rest.

I could "fix" the non-Windows path by reading in all the command input  
at once and explicitly doing a close() before returning. In that case  
I believe that the exit status would be correctly set before the  
function returns. The Windows code path, however, I am reluctant to  
touch as I don't know its behaviour and can't test it.

The other alternative was to use qx() or backticks, but then I would  
have to start worrying about sanitizing paths and once again because  
of the dual-platform issues I was hesitant to do that. And how do you  
redirect to /dev/null on Windows?

In any case, I actually *like* the addition of -q to git-ls-files. I  
can see some places in the codebase (look in git-commit.sh, for  
instance) where git-ls-files is run and the output is thrown away  
(redirected to /dev/null) because it's not at all interesting to the  
caller. It seems that there is a real use case there which amounts to  
"just tell me whether these pathspecs are valid or not, I don't care  
about any of the rest".

>>    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".

Well, you're probably right. I was thinking, "just in case we ever  
need to add more options"; but it may be that we never do, and perhaps  
git-add--interactive will become builtin one day anyway.

> 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)

Ok, thanks for the tip, Junio. Like I said, will look at this either  
tomorrow or Monday and get something posted that applies on top of  
"next".

Cheers,
Wincent

[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

[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>
---

Note that I made a couple of changes to the code posted by Junio on the
mailing list to make the behaviour a little more convenient. At this
stage it's just a proposal, and there are some issues that I'd like to
see ironed out.

Instead of throwing away the return status of pathspec_match() I am
keeping it, and any successful match breaks out of the loop early. I
observed that passing in a file parameter like "commit.h" in the Git
repo the loop would execute over a thousand times, with pathspec_match
returning 0 some of the time and 1 at others. Evidently any time it
returns 1 that's enough to indicate that it's a valid pathspec.

One issue is that performance is very slow. If you do something like
"git-add -i *.c" then there is a very noticeable delay.

Another issue is that simple shell shortcuts don't work, so something
as simple as "git-add -i ." will report:

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

It would be nice if that could be detected as valid because it does
work when you pass it to git-add--interactive.

Likewise it's not possible to validate pathspecs like "\*.sh" either,
but seeing as these won't work when passed to git-add--interactive
guess it doesn't matter (because they don't work with git-diff-files
either, which is what git-add--interactive is using).

 builtin-add.c             |   38 ++++++++++++++++++++++++++++++++++----
 builtin-commit.c          |    2 +-
 commit.h                  |    2 +-
 git-add--interactive.perl |   12 ++----------
 4 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index dd895df..06212c3 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -135,10 +135,40 @@ 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 **pattern)
 {
-	int status;
-	const char **args = xmalloc(sizeof(const char *) * (argc + 1));
+	int i, ret = 0;
+	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];
+		if ((ret = pathspec_match(pattern, m, ce->name, 0)))
+			break;
+	}
+	if (!ret)
+		report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
+	free(m);
+	return ret;
+}
+
+int interactive_add(const char *prefix, int argc, const char **argv)
+{
+	int i, status;
+	const char **args;
+	if (read_cache() < 0)
+		die("index file corrupt");
+	for (i = 0; i < argc; i++) {
+		if (!(status = validate_pathspec(prefix, &argv[i])))
+			return 1;
+	}
+
+	args = xmalloc(sizeof(const char *) * (argc + 2));
 	args[0] = "add--interactive";
 	memcpy((void *)args + sizeof(const char *), argv, sizeof(const char *) * argc);
 	args[argc + 1] = NULL;
@@ -176,7 +206,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.1992.gd646-dirty

[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>
---

I am sure that someone who actually knows Perl will be able to come up
with a more compact implementation of the process_args() function.
This works, but it's written by a Perl amateur.

 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 06212c3..e69bc13 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)
@@ -159,7 +159,7 @@ static int validate_pathspec(const char *prefix, const char **pattern)
 
 int interactive_add(const char *prefix, int argc, const char **argv)
 {
-	int i, status;
+	int i, status, pre_argc;
 	const char **args;
 	if (read_cache() < 0)
 		die("index file corrupt");
@@ -168,10 +168,13 @@ int interactive_add(const char *prefix, int argc, const char **argv)
 			return 1;
 	}
 
-	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 *), argv, sizeof(const char *) * argc);
-	args[argc + 1] = NULL;
+	args[1]	= patch_interactive ? "--patch" : "--";
+	args[2] = "--";
+	memcpy((void *)args + sizeof(const char *) * pre_argc, argv, sizeof(const char *) * argc);
+	args[argc + pre_argc] = NULL;
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
 	free(args);
@@ -184,13 +187,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"),
@@ -205,7 +208,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.1992.gd646-dirty

Re: [PATCH 2/3] Move pathspec validation into interactive_add

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:


> Instead of throwing away the return status of pathspec_match() I am
> keeping it, and any successful match breaks out of the loop early.

Leaving it early before checking if all the given pathspecs are
used defeats the whole "error-unmatch" business, doesn't it?

> Another issue is that simple shell shortcuts don't work, so something
> as simple as "git-add -i ." will report:
>
>   error: pathspec '.' did not match any file(s) known to git.

The sample code snippet I sent you probably is not doing 
get_pathspec() before using the "pattern" thing.  And I suspect
that ...

> Likewise it's not possible to validate pathspecs like "\*.sh" either,

... may be related to that.

Re: [PATCH 2/3] Move pathspec validation into interactive_add

[Wincent Colaiuta]

El 24/11/2007, a las 20:15, Junio C Hamano escribió:

> Wincent Colaiuta <win@wincent.com> writes:
>
>> Instead of throwing away the return status of pathspec_match() I am
>> keeping it, and any successful match breaks out of the loop early.
>
> Leaving it early before checking if all the given pathspecs are
> used defeats the whole "error-unmatch" business, doesn't it?

No, I probably didn't explain that clearly enough... If you look at  
the patch, I break out of the *inner* loop the first time a particular  
pattern matches. Then I move on to the next pattern, and any single  
invalid pattern will be enough to provide the "error-unmatch"  
indication.

>> Another issue is that simple shell shortcuts don't work, so something
>> as simple as "git-add -i ." will report:
>>
>>  error: pathspec '.' did not match any file(s) known to git.
>
> The sample code snippet I sent you probably is not doing
> get_pathspec() before using the "pattern" thing.  And I suspect
> that ...
>
>> Likewise it's not possible to validate pathspecs like "\*.sh" either,
>
> ... may be related to that.


Ok, thanks for the pointer. Didn't know what the get_pathspec()  
function was for, but will have a play with it.

Cheers,
Wincent

Re: [PATCH 2/3] Move pathspec validation into interactive_add

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> El 24/11/2007, a las 20:15, Junio C Hamano escribió:
>
>> Wincent Colaiuta <win@wincent.com> writes:
>>
>>> Instead of throwing away the return status of pathspec_match() I am
>>> keeping it, and any successful match breaks out of the loop early.
>>
>> Leaving it early before checking if all the given pathspecs are
>> used defeats the whole "error-unmatch" business, doesn't it?
>
> No, I probably didn't explain that clearly enough... If you look at
> the patch, I break out of the *inner* loop the first time a particular
> pattern matches. Then I move on to the next pattern, and any single
> invalid pattern will be enough to provide the "error-unmatch"
> indication.

Heh, I missed that the code was doing something so stupid ;-).

The helper function pathspec_match() takes a single path and a
set of pathspecs (NULL terminated), and says if the path is
covered with that set, while recording which one of the
pathspecs caused the match in the ps_matched array.

You are checking first with the full set of patterns, then the
full set minus the first pattern, and then the full set minus
the first two patterns, and so on.  No wonder it is inefficient.

Why are you trying to micro-optimize this part in the first
place?  Have you benchmarked and determined that iterating over
full cache is the bottleneck?

In order to prove that there is a pathspec (or more) that does
not match anything in the cache, you need to interate over the
whole cache at least once.  The only case you can short-cut is
when you can tell that all of them have been used before you
iterated over all cache entries.

So lose that silly outer loop, and replace the inner function
with something like this:

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

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

                for (num_patterns = 0; pattern[num_patterns]; num_patterns++)
                        ;
                m = xcalloc(1, num_patterns + 1);
                for (i = 0; i < active_nr; i++) {
                        struct cache_entry *ce = active_cache[i];

                        if (pathspec_match(pattern, m, ce->name, 0)) {
                                /*
                                 * You could micro optimize by leaving
                                 * the loop early when you notice that all
				 * patterns are used.
				 *
                                 * if (strlen(m) == num_patterns)
                                 * 	goto ok;
				 *
                                 */ 
				; /* nothing */
			}
                }
                report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
        ok:
                free(m);
                return ret;
        }

My gut feeling is that the micro-optimization is not worth it here, but
I didn't try.  I think without the micro-optimization the above is the
same as I gave you earlier.

[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?