[PATCH] git-add: allow path limiting with -u

[PATCH] git-add: allow path limiting with -u

[Jeff King]

Rather than updating all working tree paths, we limit
ourselves to paths listed on the command line.

Signed-off-by: Jeff King <peff@peff.net>
---
This turned out to be quite easy to implement. Patch is slightly larger
than necessary due to removing _all_ from the variable names, but I
think that better expresses the new functionality.

I'm not sure that the documentation needs updated at all; I had just
assumed after reading it that 'git-add -u foo' would DWIM.

 builtin-add.c         |   13 ++++++-------
 t/t2200-add-update.sh |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 7 deletions(-)
 create mode 100755 t/t2200-add-update.sh

diff --git a/builtin-add.c b/builtin-add.c
index 5e6748f..1591171 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -16,7 +16,7 @@
 static const char builtin_add_usage[] =
 "git-add [-n] [-v] [-f] [--interactive | -i] [-u] [--] <filepattern>...";
 
-static int take_all_worktree_changes;
+static int take_worktree_changes;
 static const char *excludes_file;
 
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
@@ -122,11 +122,12 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-static void update_all(int verbose)
+static void update(int verbose, const char **files)
 {
 	struct rev_info rev;
 	init_revisions(&rev, "");
 	setup_revisions(0, NULL, &rev, NULL);
+	rev.prune_data = get_pathspec(rev.prefix, files);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = &verbose;
@@ -200,16 +201,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp(arg, "-u")) {
-			take_all_worktree_changes = 1;
+			take_worktree_changes = 1;
 			continue;
 		}
 		usage(builtin_add_usage);
 	}
 
-	if (take_all_worktree_changes) {
-		if (i < argc)
-			die("-u and explicit paths are incompatible");
-		update_all(verbose);
+	if (take_worktree_changes) {
+		update(verbose, argv + i);
 		goto finish;
 	}
 
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
new file mode 100755
index 0000000..83005e7
--- /dev/null
+++ b/t/t2200-add-update.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git-add -u with path limiting
+
+This test creates a working tree state with three files:
+
+  top (previously committed, modified)
+  dir/sub (previously committed, modified)
+  dir/other (untracked)
+
+and issues a git-add -u with path limiting on "dir" to add
+only the updates to dir/sub.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+echo initial >top &&
+mkdir dir &&
+echo initial >dir/sub &&
+git-add dir/sub top &&
+git-commit -m initial &&
+echo changed >top &&
+echo changed >dir/sub &&
+echo other >dir/other
+'
+
+test_expect_success 'update' 'git-add -u dir'
+
+test_expect_success 'update touched correct path' \
+  'test "`git-diff-files --name-status dir/sub`" = ""'
+
+test_expect_success 'update did not touch other tracked files' \
+  'test "`git-diff-files --name-status top`" = "M	top"'
+
+test_expect_success 'update did not touch untracked files' \
+  'test "`git-diff-files --name-status dir/other`" = ""'
+
+test_done
-- 
1.5.2.rc3.709.g07945-dirty

Re: [PATCH] git-add: allow path limiting with -u

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Rather than updating all working tree paths, we limit
> ourselves to paths listed on the command line.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This turned out to be quite easy to implement. Patch is slightly larger
> than necessary due to removing _all_ from the variable names, but I
> think that better expresses the new functionality.
>
> I'm not sure that the documentation needs updated at all; I had just
> assumed after reading it that 'git-add -u foo' would DWIM.

Wonderful, and I agree it should be almost trivial once you
understand how diff works.  Will take a look.

Re: [PATCH] git-add: allow path limiting with -u

[Jakub Narebski]

On Sat, 12 May 2007, Jeff King wrote:

> Rather than updating all working tree paths, we limit
> ourselves to paths listed on the command line.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This turned out to be quite easy to implement. Patch is slightly larger
> than necessary due to removing _all_ from the variable names, but I
> think that better expresses the new functionality.
> 
> I'm not sure that the documentation needs updated at all; I had just
> assumed after reading it that 'git-add -u foo' would DWIM.

Do git-add *needs* path specifier (even if it is '.') also for `-u'?
The changes in documentation were to reflect that `-u' is incompatibile
with explicit paths, or that `-u' does not require explicit paths
contrary to git-add without `-u'.

The fact that "add --interactive does not take any parameters" is
separate issue (which, accidentally, was adressed in the same patch).

-- 
Jakub Narebski
Poland

Re: [PATCH] git-add: allow path limiting with -u

[Jeff King]

On Sun, May 13, 2007 at 12:35:24PM +0200, Jakub Narebski wrote:

> Do git-add *needs* path specifier (even if it is '.') also for `-u'?
> The changes in documentation were to reflect that `-u' is incompatibile
> with explicit paths, or that `-u' does not require explicit paths
> contrary to git-add without `-u'.

Sorry, I don't understand if you are asking that question, or if you are
asking if the documentation needs to clarify that point.

The answer to the first is "no", the path limiting works exactly as it
does for every other command: no path specifier indicates the whole
tree.

If you are concerned about the latter, do you mean something like this:

-- >8 --
Documentation/git-add: clarify -u with path limiting

Signed-off-by: Jeff King <peff@peff.net>
---
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index ea27018..27b9c0f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -57,8 +57,11 @@ OPTIONS
 	the index.
 
 -u::
-	Update all files that git already knows about. This is what
-	"git commit -a" does in preparation for making a commit.
+	Update only files that git already knows about. This is similar
+	to what "git commit -a" does in preparation for making a commit,
+	except that the update is limited to paths specified on the
+	command line. If no paths are specified, all tracked files are
+	updated.
 
 \--::
 	This option can be used to separate command-line options from

Re: [PATCH] git-add: allow path limiting with -u

[Jeff King]

On Sun, May 13, 2007 at 12:35:24PM +0200, Jakub Narebski wrote:

> The fact that "add --interactive does not take any parameters" is
> separate issue (which, accidentally, was adressed in the same patch).

I don't generally use add --interactive, but I imagine that
path-limiting would also make sense there. I think it would be a bit
harder to implement (and test!), since there are many calls to commands
which would need the limits. So I will leave that unless somebody really
cares about it.

-Peff

Re: [PATCH] git-add: allow path limiting with -u

[Jeff King]

On Mon, May 14, 2007 at 02:50:22AM +0200, Jakub Narebski wrote:

> -'git-add' [-n] [-v] [-f] [--interactive | -i] [-u] [--] <file>...
> +'git-add' [-n] [-v] [-f] [--] <file>...
> +'git-add' [-n] [-v] [-f] -u [[--] <file>...]
> +'git-add' (--interactive | -i)

I don't see the point in splitting -u out as its own separate "mode" of
operation. To me it's conceptually a flag that says "don't add untracked
files, and remove deleted files". But I don't have a strong opinion (and
certainly splitting out -i makes sense).

-Peff

Re: [PATCH] git-add: allow path limiting with -u

[Jakub Narebski]

Jeff King wrote:

> If you are concerned about the latter, do you mean something like this:
> 
> -- >8 --
> Documentation/git-add: clarify -u with path limiting
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index ea27018..27b9c0f 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -57,8 +57,11 @@ OPTIONS
>  	the index.
>  
>  -u::
> -	Update all files that git already knows about. This is what
> -	"git commit -a" does in preparation for making a commit.
> +	Update only files that git already knows about. This is similar
> +	to what "git commit -a" does in preparation for making a commit,
> +	except that the update is limited to paths specified on the
> +	command line. If no paths are specified, all tracked files are
> +	updated.
>  
>  \--::
>  	This option can be used to separate command-line options from
> 

That is very nice and needed, but I actually thought about correcting
SYNOPSIS to read:

@@ -7,7 +7,9 @@ git-add - Add file contents to the changeset to be committed next
 
 SYNOPSIS
 --------
-'git-add' [-n] [-v] [-f] [--interactive | -i] [-u] [--] <file>...
+'git-add' [-n] [-v] [-f] [--] <file>...
+'git-add' [-n] [-v] [-f] -u [[--] <file>...]
+'git-add' (--interactive | -i)
 
 DESCRIPTION
 -----------

This ensures that while git-add without -u needs explicit paths
(even if it is '.'), git-add with -u can have explicit paths but
doesn't need them.
-- 
Jakub Narebski
Poland

Re: [PATCH] git-add: allow path limiting with -u

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 14, 2007 at 02:50:22AM +0200, Jakub Narebski wrote:
>
>> -'git-add' [-n] [-v] [-f] [--interactive | -i] [-u] [--] <file>...
>> +'git-add' [-n] [-v] [-f] [--] <file>...
>> +'git-add' [-n] [-v] [-f] -u [[--] <file>...]
>> +'git-add' (--interactive | -i)
>
> I don't see the point in splitting -u out as its own separate "mode" of
> operation. To me it's conceptually a flag that says "don't add untracked
> files, and remove deleted files". But I don't have a strong opinion (and
> certainly splitting out -i makes sense).

Concurred, and I agree with Jakub that your update to the
description of -u makes it much better.

Thanks, both.

Re: [PATCH] git-add: allow path limiting with -u

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, May 13, 2007 at 12:35:24PM +0200, Jakub Narebski wrote:
>
>> The fact that "add --interactive does not take any parameters" is
>> separate issue (which, accidentally, was adressed in the same patch).
>
> I don't generally use add --interactive, but I imagine that
> path-limiting would also make sense there. I think it would be a bit
> harder to implement (and test!), since there are many calls to commands
> which would need the limits. So I will leave that unless somebody really
> cares about it.

I do not use -i myself that much, but due to its "interactive"
nature, I would imagine that limiting path upfront is actually
counter-productive.  During the interactive refining process you
might notice that you wanted to handle some other paths you
excluded from the command line, but you won't be able to access
them if you limit upfront.  In other words, command line limiter
would only interfere with what the interactive user would really
want to do.