feature request: git add--interactive --patch on regex-matched hunks only

feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

Hello Perl heroes,

Can we have "git add--interactive --patch --match=regex" where only
(splitted if possible) hunks that match regex are shown?

I've been reviewing a .po file and adding translations that I approve,
leaving the rest to be determined later. The problem with .po files is
that a lot of comment lines are changes because some translations are
moved in source code. I don't care about those comment changes and
would like to completely ignore them (even destroy them with git
checkout -p).
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Sun, Jul 24, 2011 at 12:11:29PM +0700, Nguyen Thai Ngoc Duy wrote:

> Can we have "git add--interactive --patch --match=regex" where only
> (splitted if possible) hunks that match regex are shown?

The patch below does this, but there are a lot of unanswered questions.
Such as:

  1. What does it mean to be "shown"? My patch auto-marks non-matching
     hunks as "do not stage". That means you can still switch back to
     them in the hunk list and enable them if you want. Another option
     would be to omit them entirely, and pretend that those hunks don't
     exist.

  2. What should we do with non-text changes, like mode changes are
     full-file deletion?

  3. What should be shown for a file with no matching hunks? Probably
     nothing (i.e., as if it had been limited away by pathname)? My
     patch shows the header, but that is easy to fix.

I think those depend on the intended use case. For me, it seems useful
to do something like:

  $ hack hack hack
  [oops, I need to refactor foo() and its callers first]
  $ refactor refactor refactor
  $ git add -p --match=foo
  $ git commit -m 'refactor foo'
  [resume initial hacking]

However, I'm not sure I would trust my regex to actually get all of the
changes needed for the refactoring (e.g., there might be nearby hunks
related to the refactoring that are not specifically in the same hunk as
the word "foo"). So I tend to just "git add -p" and flip through the
changes manually.

> I've been reviewing a .po file and adding translations that I approve,
> leaving the rest to be determined later. The problem with .po files is
> that a lot of comment lines are changes because some translations are
> moved in source code. I don't care about those comment changes and
> would like to completely ignore them (even destroy them with git
> checkout -p).

You can already skip around in the hunk list using "/regex". Might that
be enough for you? I think you're stuck typing "/yoursearch" over and
over, though. It would be nice if doing just "/" would search again for
the previous regex.

Anyway, here's the patch. It's just the perl bits. You'd need to write
the scaffolding to add an option to "git add", "git checkout", etc.

---
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 2ee0908..07896d4 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -86,6 +86,7 @@ sub colored {
 # command line options
 my $patch_mode;
 my $patch_mode_revision;
+my $patch_match;
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
@@ -1277,6 +1278,17 @@ sub display_hunks {
 	return $i;
 }
 
+sub want_hunk {
+	my ($re, $hunk) = @_;
+
+	return 1 if $hunk->{TYPE} ne 'hunk';
+
+	foreach my $line (@{$hunk->{TEXT}}) {
+		return 1 if $line =~ $re;
+	}
+	return 0;
+}
+
 sub patch_update_file {
 	my $quit = 0;
 	my ($ix, $num);
@@ -1301,6 +1313,20 @@ sub patch_update_file {
 	$num = scalar @hunk;
 	$ix = 0;
 
+	if ($patch_match) {
+		# mark non-matching text hunks as "do not want"
+		foreach my $hunk (@hunk) {
+			if (!want_hunk($patch_match, $hunk)) {
+				$hunk->{USE} = 0;
+			}
+		}
+		# and then advance us to the first undecided hunk
+		while ($ix < $num) {
+			last unless defined $hunk[$ix]{USE};
+			$ix++;
+		}
+	}
+
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
 		$other = '';
@@ -1606,6 +1632,10 @@ sub process_args {
 		} else {
 			$patch_mode = 'stage';
 			$arg = shift @ARGV or die "missing --";
+			if ($arg =~ /--match=(.*)/) {
+				$patch_match = qr/$1/;
+				$arg = shift @ARGV or die "missing --";
+			}
 		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, I'm not sure I would trust my regex to actually get all of the
> changes needed for the refactoring (e.g., there might be nearby hunks
> related to the refactoring that are not specifically in the same hunk as
> the word "foo"). So I tend to just "git add -p" and flip through the
> changes manually.

Yes, that is a valid concern.

> You can already skip around in the hunk list using "/regex". Might that
> be enough for you? I think you're stuck typing "/yoursearch" over and
> over, though. It would be nice if doing just "/" would search again for
> the previous regex.

Yes, I think that makes a lot more sense than the current "Ah, empty? ask
from the terminal" behaviour.

 git-add--interactive.perl |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..0b6f8a6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1257,7 +1257,7 @@ sub display_hunks {
 
 sub patch_update_file {
 	my $quit = 0;
-	my ($ix, $num);
+	my ($ix, $num, $last_search_string);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
 	($head, my $mode, my $deletion) = parse_diff_header($head);
@@ -1395,11 +1395,12 @@ sub patch_update_file {
 			}
 			elsif ($line =~ m|^/(.*)|) {
 				my $regex = $1;
-				if ($1 eq "") {
-					print colored $prompt_color, "search for regex? ";
-					$regex = <STDIN>;
-					if (defined $regex) {
-						chomp $regex;
+				if ($regex eq "") {
+					if ($last_search_string) {
+						$regex = $last_search_string;
+					} else {
+						error_msg "Need a regexp to search\n";
+						next;
 					}
 				}
 				my $search_string;
@@ -1412,6 +1413,7 @@ sub patch_update_file {
 					error_msg "Malformed search regexp $exp: $err\n";
 					next;
 				}
+				$last_search_string = $regex;
 				my $iy = $ix;
 				while (1) {
 					my $text = join ("", @{$hunk[$iy]{TEXT}});

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

On Tue, Jul 26, 2011 at 4:55 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Jul 24, 2011 at 12:11:29PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> Can we have "git add--interactive --patch --match=regex" where only
>> (splitted if possible) hunks that match regex are shown?
>
> The patch below does this,

Thanks!

> but there are a lot of unanswered questions.
> Such as:
>
>  1. What does it mean to be "shown"? My patch auto-marks non-matching
>     hunks as "do not stage". That means you can still switch back to
>     them in the hunk list and enable them if you want. Another option
>     would be to omit them entirely, and pretend that those hunks don't
>     exist.

Either way is ok. Maybe the option in this case could be changed to
--nostage=regex to reflect the behavior.

>  2. What should we do with non-text changes, like mode changes are
>     full-file deletion?

Probably invalid use case for --match.

>  3. What should be shown for a file with no matching hunks? Probably
>     nothing (i.e., as if it had been limited away by pathname)? My
>     patch shows the header, but that is easy to fix.

Printing "Nothing to add" would be nice.

> However, I'm not sure I would trust my regex to actually get all of the
> changes needed for the refactoring (e.g., there might be nearby hunks
> related to the refactoring that are not specifically in the same hunk as
> the word "foo"). So I tend to just "git add -p" and flip through the
> changes manually.

Well, I do "git diff --cached" and "git diff"  (the search in the
diff) before committing just to make sure I don't miss any changes.

> You can already skip around in the hunk list using "/regex". Might that
> be enough for you? I think you're stuck typing "/yoursearch" over and
> over, though. It would be nice if doing just "/" would search again for
> the previous regex.

Yes I'm stuck typing /mysearch. Yes, "/" alone would be nice, but in
this case I really want mass ignore certain changes so I can focus on
important strings first.
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Mon, Jul 25, 2011 at 04:44:01PM -0700, Junio C Hamano wrote:

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 8f0839d..0b6f8a6 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1257,7 +1257,7 @@ sub display_hunks {
>  
>  sub patch_update_file {
>  	my $quit = 0;
> -	my ($ix, $num);
> +	my ($ix, $num, $last_search_string);
>  	my $path = shift;
>  	my ($head, @hunk) = parse_diff($path);
>  	($head, my $mode, my $deletion) = parse_diff_header($head);
> @@ -1395,11 +1395,12 @@ sub patch_update_file {
>  			}
>  			elsif ($line =~ m|^/(.*)|) {
>  				my $regex = $1;
> -				if ($1 eq "") {
> -					print colored $prompt_color, "search for regex? ";
> -					$regex = <STDIN>;
> -					if (defined $regex) {
> -						chomp $regex;
> +				if ($regex eq "") {
> +					if ($last_search_string) {
> +						$regex = $last_search_string;
> +					} else {
> +						error_msg "Need a regexp to search\n";
> +						next;

How does this interact with single-key mode? I imagine we just get the
"/" at that point and have to read the rest of the regex manually. Which
is probably why this code was here in the first place.

So I think we might have to do something like:

  my $regex = $1;
  if ($use_readkey) {
    print colored $prompt_color, "search for regex?";
    $regex = <STDIN>;
    chomp $regex;
  }
  if ($regex eq "") {
    ...
  }

And then that would give single-key people an opportunity to input a new
regex, or to hit enter to just use the last one.

-Peff

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Tue, Jul 26, 2011 at 10:03:31AM +0700, Nguyen Thai Ngoc Duy wrote:

> On Tue, Jul 26, 2011 at 4:55 AM, Jeff King <peff@peff.net> wrote:
> > On Sun, Jul 24, 2011 at 12:11:29PM +0700, Nguyen Thai Ngoc Duy wrote:
> >
> >> Can we have "git add--interactive --patch --match=regex" where only
> >> (splitted if possible) hunks that match regex are shown?
> >
> > The patch below does this,
> 
> Thanks!

So I did this as a quick "that should be really easy" proof of concept.
I'm not too interested personally in taking it all the way to an
acceptable patch.  I'm happy to help out with the perl parts, though, if
you want to do the rest (C scaffolding for calling add--interactive,
tests, and docs).

> >  1. What does it mean to be "shown"? My patch auto-marks non-matching
> >     hunks as "do not stage". That means you can still switch back to
> >     them in the hunk list and enable them if you want. Another option
> >     would be to omit them entirely, and pretend that those hunks don't
> >     exist.
> 
> Either way is ok. Maybe the option in this case could be changed to
> --nostage=regex to reflect the behavior.

I can't help but think there is some way to relate it to the
path-limiting that we do elsewhere. It's sort of like diffcore's "-G",
except that is about picking whole files, not individual hunks. I guess
most of git doesn't operate at the hunk level in quite the same way, so
there is no analagous part.

The more I think about it, it is probably simpler both conceptually and
in implementation to filter out those hunks entirely instead of marking
them used. The latter gives the user a chance to manually find them and
go back to them via 'J' and 'K'. But I find the chance that that is
useful to be much less than the chance that somebody gets confused or
annoyed by them being there (because they were iterating, or because
they did a "/" search).

And it's not like everything needs to be in a single "add" call. The
point of "add" is that you could call it multiple times, or even call
it, then check what "git diff" says, then decide you want to stage some
more.

> >  2. What should we do with non-text changes, like mode changes are
> >     full-file deletion?
> 
> Probably invalid use case for --match.

Invalid, like we should have an error? I think that would be annoying,
because you want to be able to do "git add -p --match=foo" even when
there is an unrelated mode change. I think in the mindset of "this is a
filter for things you want to see", it probably makes sense to just
pretend they aren't there (and the user can always follow up with
another "add", as I mentioned above).

> >  3. What should be shown for a file with no matching hunks? Probably
> >     nothing (i.e., as if it had been limited away by pathname)? My
> >     patch shows the header, but that is easy to fix.
> 
> Printing "Nothing to add" would be nice.

If we treat it like path limiting, I think we could just print nothing
for that file. That is, doing:

  git add -p foo

would not even look at "bar" or print anything about it. Similarly:

  git add -p --match=foo

should probably say nothing whatsoever about files that didn't mention
foo at all.

For reference, here's the C scaffolding I used to test the patch I sent
earlier. I didn't include it earlier because it made the perl bits hard
to find amidst all of the changes.

Probably the other interactive modes (checkout, reset, etc) should get a
similar option; it would just be a matter of passing the regex through
to the perl script.

---
diff --git a/builtin/add.c b/builtin/add.c
index 822ee3a..096dbc7 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -220,20 +220,25 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p
 }
 
 int run_add_interactive(const char *revision, const char *patch_mode,
-			const char **pathspec)
+			const char *match, const char **pathspec)
 {
 	int status, ac, pc = 0;
 	const char **args;
+	struct strbuf match_arg = STRBUF_INIT;
 
 	if (pathspec)
 		while (pathspec[pc])
 			pc++;
 
-	args = xcalloc(sizeof(const char *), (pc + 5));
+	args = xcalloc(sizeof(const char *), (pc + 6));
 	ac = 0;
 	args[ac++] = "add--interactive";
 	if (patch_mode)
 		args[ac++] = patch_mode;
+	if (match) {
+		strbuf_addf(&match_arg, "--match=%s", match);
+		args[ac++] = match_arg.buf;
+	}
 	if (revision)
 		args[ac++] = revision;
 	args[ac++] = "--";
@@ -245,10 +250,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 
 	status = run_command_v_opt(args, RUN_GIT_CMD);
 	free(args);
+	strbuf_release(&match_arg);
 	return status;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix, int patch)
+int interactive_add(int argc, const char **argv, const char *prefix, int patch,
+		    const char *match)
 {
 	const char **pathspec = NULL;
 
@@ -260,7 +267,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 
 	return run_add_interactive(NULL,
 				   patch ? "--patch" : NULL,
-				   pathspec);
+				   match, pathspec);
 }
 
 static int edit_patch(int argc, const char **argv, const char *prefix)
@@ -317,6 +324,7 @@ N_("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 ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
+static const char *patch_match;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, "dry run"),
@@ -324,6 +332,8 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "select hunks interactively"),
+	OPT_STRING(0, "match", &patch_match, "regex",
+		   "find pattern within --patch hunks"),
 	OPT_BOOLEAN('e', "edit", &edit_interactive, "edit current diff and apply"),
 	OPT__FORCE(&ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', "update", &take_worktree_changes, "update tracked files"),
@@ -384,7 +394,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (patch_interactive)
 		add_interactive = 1;
 	if (add_interactive)
-		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
+		exit(interactive_add(argc - 1, argv + 1, prefix,
+				     patch_interactive, patch_match));
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d647a31..8319bb2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -765,7 +765,8 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 static int interactive_checkout(const char *revision, const char **pathspec,
 				struct checkout_opts *opts)
 {
-	return run_add_interactive(revision, "--patch=checkout", pathspec);
+	return run_add_interactive(revision, "--patch=checkout", NULL,
+				   pathspec);
 }
 
 struct tracking_name_data {
diff --git a/builtin/commit.c b/builtin/commit.c
index e50694e..24d90a4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -366,7 +366,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 		old_index_env = getenv(INDEX_ENVIRONMENT);
 		setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
 
-		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
+		if (interactive_add(argc, argv, prefix, patch_interactive,
+				    NULL) != 0)
 			die(_("interactive add failed"));
 
 		if (old_index_env && *old_index_env)
diff --git a/builtin/reset.c b/builtin/reset.c
index 811e8e2..924c104 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -165,7 +165,7 @@ static int interactive_reset(const char *revision, const char **argv,
 	if (*argv)
 		pathspec = get_pathspec(prefix, argv);
 
-	return run_add_interactive(revision, "--patch=reset", pathspec);
+	return run_add_interactive(revision, "--patch=reset", NULL, pathspec);
 }
 
 static int read_from_tree(const char *prefix, const char **argv,
diff --git a/commit.h b/commit.h
index b23f33b..5ab0072 100644
--- a/commit.h
+++ b/commit.h
@@ -167,9 +167,10 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 int is_descendant_of(struct commit *, struct commit_list *);
 int in_merge_bases(struct commit *, struct commit **, int);
 
-extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
+extern int interactive_add(int argc, const char **argv, const char *prefix,
+			   int patch, const char *match);
 extern int run_add_interactive(const char *revision, const char *patch_mode,
-			       const char **pathspec);
+			       const char *match, const char **pathspec);
 
 static inline int single_parent(struct commit *commit)
 {
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 2ee0908..07896d4 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -86,6 +86,7 @@ sub colored {
 # command line options
 my $patch_mode;
 my $patch_mode_revision;
+my $patch_match;
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
@@ -1277,6 +1278,17 @@ sub display_hunks {
 	return $i;
 }
 
+sub want_hunk {
+	my ($re, $hunk) = @_;
+
+	return 1 if $hunk->{TYPE} ne 'hunk';
+
+	foreach my $line (@{$hunk->{TEXT}}) {
+		return 1 if $line =~ $re;
+	}
+	return 0;
+}
+
 sub patch_update_file {
 	my $quit = 0;
 	my ($ix, $num);
@@ -1301,6 +1313,20 @@ sub patch_update_file {
 	$num = scalar @hunk;
 	$ix = 0;
 
+	if ($patch_match) {
+		# mark non-matching text hunks as "do not want"
+		foreach my $hunk (@hunk) {
+			if (!want_hunk($patch_match, $hunk)) {
+				$hunk->{USE} = 0;
+			}
+		}
+		# and then advance us to the first undecided hunk
+		while ($ix < $num) {
+			last unless defined $hunk[$ix]{USE};
+			$ix++;
+		}
+	}
+
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
 		$other = '';
@@ -1606,6 +1632,10 @@ sub process_args {
 		} else {
 			$patch_mode = 'stage';
 			$arg = shift @ARGV or die "missing --";
+			if ($arg =~ /--match=(.*)/) {
+				$patch_match = qr/$1/;
+				$arg = shift @ARGV or die "missing --";
+			}
 		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

On Tue, Jul 26, 2011 at 12:14 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 26, 2011 at 10:03:31AM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> On Tue, Jul 26, 2011 at 4:55 AM, Jeff King <peff@peff.net> wrote:
>> > On Sun, Jul 24, 2011 at 12:11:29PM +0700, Nguyen Thai Ngoc Duy wrote:
>> >
>> >> Can we have "git add--interactive --patch --match=regex" where only
>> >> (splitted if possible) hunks that match regex are shown?
>> >
>> > The patch below does this,
>>
>> Thanks!
>
> So I did this as a quick "that should be really easy" proof of concept.
> I'm not too interested personally in taking it all the way to an
> acceptable patch.  I'm happy to help out with the perl parts, though, if
> you want to do the rest (C scaffolding for calling add--interactive,
> tests, and docs).

It's my itch. I'm more than happy if you take care of the Perl part,
I'll do the rest. While you're doing the Perl part, should we have
both --match and --no-match? One filters in. The other filters out. I
guess people may find filter-in case more helpful than the other.

> The more I think about it, it is probably simpler both conceptually and
> in implementation to filter out those hunks entirely instead of marking
> them used. The latter gives the user a chance to manually find them and
> go back to them via 'J' and 'K'. But I find the chance that that is
> useful to be much less than the chance that somebody gets confused or
> annoyed by them being there (because they were iterating, or because
> they did a "/" search).

I thought of this way initially, but I needed to split hunks in
parse_diff and was stuck because I was not familiar with data
structure used in git-add--interactive.

> And it's not like everything needs to be in a single "add" call. The
> point of "add" is that you could call it multiple times, or even call
> it, then check what "git diff" says, then decide you want to stage some
> more.

Yes, the point of this new feature, is to get me focus on a selected
parts I care.

>> >  2. What should we do with non-text changes, like mode changes are
>> >     full-file deletion?
>>
>> Probably invalid use case for --match.
>
> Invalid, like we should have an error? I think that would be annoying,
> because you want to be able to do "git add -p --match=foo" even when
> there is an unrelated mode change. I think in the mindset of "this is a
> filter for things you want to see", it probably makes sense to just
> pretend they aren't there (and the user can always follow up with
> another "add", as I mentioned above).

Agreed.

>> >  3. What should be shown for a file with no matching hunks? Probably
>> >     nothing (i.e., as if it had been limited away by pathname)? My
>> >     patch shows the header, but that is easy to fix.
>>
>> Printing "Nothing to add" would be nice.
>
> If we treat it like path limiting, I think we could just print nothing
> for that file. That is, doing:
>
>  git add -p foo
>
> would not even look at "bar" or print anything about it. Similarly:
>
>  git add -p --match=foo
>
> should probably say nothing whatsoever about files that didn't mention
> foo at all.

Hmm.. I did not thought of doing multiple files. Printing nothing
makes sense this case.
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Tue, Jul 26, 2011 at 12:57:46PM +0700, Nguyen Thai Ngoc Duy wrote:

> It's my itch. I'm more than happy if you take care of the Perl part,
> I'll do the rest. While you're doing the Perl part, should we have
> both --match and --no-match? One filters in. The other filters out. I
> guess people may find filter-in case more helpful than the other.

Yeah. Also, what about grep options like case-insensitivity? I don't
want to go too overboard with adding options that are unlikely to be
used. It is perl, so technically you can do "(?i:foo)" to match "foo"
case-insensitively. I don't think there's a way to do --no-match style
negation in the regex itself, though.

Speaking of which: the name "--match" is way too generic for "git add".
Something like "--hunk-match" would be better. It also is obviously only
useful with "--patch". Should it imply "--patch"? Should it actually be
"--patch=..."?

> I thought of this way initially, but I needed to split hunks in
> parse_diff and was stuck because I was not familiar with data
> structure used in git-add--interactive.

Oh yeah, you mentioned splitting before. I'm not sure how that would
interact with this feature. In some cases, you'd want to split as much
as possible, and in other cases, you'd want a bigger hunk. So I actually
think it is an orthogonal feature to "autosplit" hunks before we show
them to the user. I.e., something like:

  git add -p --autosplit-hunks --hunk-match=...

(where maybe the names could be better, but hopefully you get the
point).

-Peff

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

On Tue, Jul 26, 2011 at 1:09 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 26, 2011 at 12:57:46PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> It's my itch. I'm more than happy if you take care of the Perl part,
>> I'll do the rest. While you're doing the Perl part, should we have
>> both --match and --no-match? One filters in. The other filters out. I
>> guess people may find filter-in case more helpful than the other.
>
> Yeah. Also, what about grep options like case-insensitivity? I don't
> want to go too overboard with adding options that are unlikely to be
> used. It is perl, so technically you can do "(?i:foo)" to match "foo"
> case-insensitively.

Or even standard regex with [Ff][Oo][Oo]. We can wait until users
complain about the lack of feature. I won't complain because I may
need to match non-ascii letters (unless pcre does support unicode,
then yes pleasssse)

> I don't think there's a way to do --no-match style negation in the
> regex itself, though.

Your coding skills are needed :)

>> I thought of this way initially, but I needed to split hunks in
>> parse_diff and was stuck because I was not familiar with data
>> structure used in git-add--interactive.
>
> Oh yeah, you mentioned splitting before. I'm not sure how that would
> interact with this feature. In some cases, you'd want to split as much
> as possible, and in other cases, you'd want a bigger hunk. So I actually
> think it is an orthogonal feature to "autosplit" hunks before we show
> them to the user. I.e., something like:
>
>  git add -p --autosplit-hunks --hunk-match=...

Yep. Sometimes I do want --autosplit-hunks alone.

> (where maybe the names could be better, but hopefully you get the
> point).

Speaking of names, I'm usually bad at naming, but here goes. How about
--hunks=regex, --no-hunks=regex and --split-hunks? We may have
--[no-]case-hunks later on but that does sound bad.

<over-engineering>maybe we should support multiple --hunks (or
--no-hunks, but not a mix of them), all must be matched, because there
are many lines in a hunk and people may want set patterns across
lines</over-engineering>
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

It would be even more cool if --hunks (or whatever the name will be)
could work without -p. I mean, if "git diff" supports it, then I can
fine tune my regex to meet a selection of hunks I want, and verify it
really is what I want. Then "git add --hunks=magic" and voila! (The
"git add --hunks" without -p surely can be workaround by adding "-p",
then accept all hunks).

And if diff machinery learns this, we would have "git log --hunks" too.

OK I'm asking too much..
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Tue, Jul 26, 2011 at 07:44:06PM +0700, Nguyen Thai Ngoc Duy wrote:

> > I don't think there's a way to do --no-match style negation in the
> > regex itself, though.
> 
> Your coding skills are needed :)

OK. Patches to follow. :)

> Speaking of names, I'm usually bad at naming, but here goes. How about
> --hunks=regex, --no-hunks=regex and --split-hunks? We may have
> --[no-]case-hunks later on but that does sound bad.
> 
> <over-engineering>maybe we should support multiple --hunks (or
> --no-hunks, but not a mix of them), all must be matched, because there
> are many lines in a hunk and people may want set patterns across
> lines</over-engineering>

My series lets you do multiple --hunks, and a hunk just needs to match
any of them.

Having "--no-hunks" implies that you can negate just some of the
filters. Like:

  git add -p --hunks=foo --no-hunks=bar

My series doesn't support that, though it would not be that big a deal
to do so (the tricky part is defining the OR-ing and AND-ing sensibly).
Something like:

  git add -p --negate-hunks --hunks=foo --hunks=bar

makes it clear that you are either selecting or de-selecting with your
filter.

Technically somebody could also want:

  git add -p --hunk=foo --and --hunk=bar

but I didn't want to get into parsing arbitrary boolean expressions.

An easy flexible thing would be to just eval perl code like:

  git add -p --hunk-filter='/foo/ && !/bar/'

but I don't think we want to tie ourselves to the implementation being
in perl forever.

Anyway, the series is:

  [1/5]: add--interactive: refactor patch mode argument processing
  [2/5]: add--interactive: factor out regex error handling
  [3/5]: add--interactive: allow hunk filtering on command line
  [4/5]: add--interactive: allow negatation of hunk filters
  [5/5]: add--interactive: add option to autosplit hunks

Lightly tested by me. You can either build on top, or just squash them
into your commits as appropriate.

-Peff

[PATCH 1/5] add--interactive: refactor patch mode argument processing

[Jeff King]

The existing parser was very inflexible and tree-like;
seeing "--patch=$mode" would put us into a conditional for
parsing the options for that particular $mode. That made it
very difficult to add additional options that would work for
all patch modes.

This cleanup turns the argument processing into a proper
loop ready to handle multiple arguments. To replace the
mode-specific parsing, we restructure the mode definitions
to better reflect the various cases (i.e., no revision given
versus "HEAD" versus an arbitrary revision).

As a bonus, our parsing is now a little more robust. For
example, we catch the bogus "add--interactive --patch HEAD
--", which is meaningless, but was silently accepted before.
In practice this probably doesn't matter, since we are
always called by other parts of git, but it might catch a
bug in another part of git.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |  212 ++++++++++++++++++++++++---------------------
 1 files changed, 112 insertions(+), 100 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..c5cd300 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -93,74 +93,86 @@ sub apply_patch_for_stash;
 
 my %patch_modes = (
 	'stage' => {
-		DIFF => 'diff-files -p',
-		APPLY => sub { apply_patch 'apply --cached', @_; },
-		APPLY_CHECK => 'apply --cached',
-		VERB => 'Stage',
-		TARGET => '',
-		PARTICIPLE => 'staging',
-		FILTER => 'file-only',
-		IS_REVERSE => 0,
+		default => 'index',
+		index => {
+			DIFF => 'diff-files -p',
+			APPLY => sub { apply_patch 'apply --cached', @_; },
+			APPLY_CHECK => 'apply --cached',
+			VERB => 'Stage',
+			TARGET => '',
+			PARTICIPLE => 'staging',
+			FILTER => 'file-only',
+			IS_REVERSE => 0,
+		},
 	},
 	'stash' => {
-		DIFF => 'diff-index -p HEAD',
-		APPLY => sub { apply_patch 'apply --cached', @_; },
-		APPLY_CHECK => 'apply --cached',
-		VERB => 'Stash',
-		TARGET => '',
-		PARTICIPLE => 'stashing',
-		FILTER => undef,
-		IS_REVERSE => 0,
+		default => 'head',
+		head => {
+			DIFF => 'diff-index -p',
+			APPLY => sub { apply_patch 'apply --cached', @_; },
+			APPLY_CHECK => 'apply --cached',
+			VERB => 'Stash',
+			TARGET => '',
+			PARTICIPLE => 'stashing',
+			FILTER => undef,
+			IS_REVERSE => 0,
+		},
 	},
-	'reset_head' => {
-		DIFF => 'diff-index -p --cached',
-		APPLY => sub { apply_patch 'apply -R --cached', @_; },
-		APPLY_CHECK => 'apply -R --cached',
-		VERB => 'Unstage',
-		TARGET => '',
-		PARTICIPLE => 'unstaging',
-		FILTER => 'index-only',
-		IS_REVERSE => 1,
+	'reset' => {
+		default => 'head',
+		head => {
+			DIFF => 'diff-index -p --cached',
+			APPLY => sub { apply_patch 'apply -R --cached', @_; },
+			APPLY_CHECK => 'apply -R --cached',
+			VERB => 'Unstage',
+			TARGET => '',
+			PARTICIPLE => 'unstaging',
+			FILTER => 'index-only',
+			IS_REVERSE => 1,
+		},
+		nothead => {
+			DIFF => 'diff-index -R -p --cached',
+			APPLY => sub { apply_patch 'apply --cached', @_; },
+			APPLY_CHECK => 'apply --cached',
+			VERB => 'Apply',
+			TARGET => ' to index',
+			PARTICIPLE => 'applying',
+			FILTER => 'index-only',
+			IS_REVERSE => 0,
+		},
 	},
-	'reset_nothead' => {
-		DIFF => 'diff-index -R -p --cached',
-		APPLY => sub { apply_patch 'apply --cached', @_; },
-		APPLY_CHECK => 'apply --cached',
-		VERB => 'Apply',
-		TARGET => ' to index',
-		PARTICIPLE => 'applying',
-		FILTER => 'index-only',
-		IS_REVERSE => 0,
-	},
-	'checkout_index' => {
-		DIFF => 'diff-files -p',
-		APPLY => sub { apply_patch 'apply -R', @_; },
-		APPLY_CHECK => 'apply -R',
-		VERB => 'Discard',
-		TARGET => ' from worktree',
-		PARTICIPLE => 'discarding',
-		FILTER => 'file-only',
-		IS_REVERSE => 1,
-	},
-	'checkout_head' => {
-		DIFF => 'diff-index -p',
-		APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
-		APPLY_CHECK => 'apply -R',
-		VERB => 'Discard',
-		TARGET => ' from index and worktree',
-		PARTICIPLE => 'discarding',
-		FILTER => undef,
-		IS_REVERSE => 1,
-	},
-	'checkout_nothead' => {
-		DIFF => 'diff-index -R -p',
-		APPLY => sub { apply_patch_for_checkout_commit '', @_ },
-		APPLY_CHECK => 'apply',
-		VERB => 'Apply',
-		TARGET => ' to index and worktree',
-		PARTICIPLE => 'applying',
-		FILTER => undef,
-		IS_REVERSE => 0,
+	'checkout' => {
+		default => 'index',
+		index => {
+			DIFF => 'diff-files -p',
+			APPLY => sub { apply_patch 'apply -R', @_; },
+			APPLY_CHECK => 'apply -R',
+			VERB => 'Discard',
+			TARGET => ' from worktree',
+			PARTICIPLE => 'discarding',
+			FILTER => 'file-only',
+			IS_REVERSE => 1,
+		},
+		head => {
+			DIFF => 'diff-index -p',
+			APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
+			APPLY_CHECK => 'apply -R',
+			VERB => 'Discard',
+			TARGET => ' from index and worktree',
+			PARTICIPLE => 'discarding',
+			FILTER => undef,
+			IS_REVERSE => 1,
+		},
+		nothead => {
+			DIFF => 'diff-index -R -p',
+			APPLY => sub { apply_patch_for_checkout_commit '', @_ },
+			APPLY_CHECK => 'apply',
+			VERB => 'Apply',
+			TARGET => ' to index and worktree',
+			PARTICIPLE => 'applying',
+			FILTER => undef,
+			IS_REVERSE => 0,
+		},
 	},
 );
 
@@ -1546,45 +1558,45 @@ EOF
 
 sub process_args {
 	return unless @ARGV;
-	my $arg = shift @ARGV;
-	if ($arg =~ /--patch(?:=(.*))?/) {
-		if (defined $1) {
-			if ($1 eq 'reset') {
-				$patch_mode = 'reset_head';
+
+	while (@ARGV) {
+		if ($ARGV[0] =~ /--patch(?:=(.*))?/) {
+			$patch_mode = defined $1 ? $1 : 'stage';
+		}
+		else {
+			last;
+		}
+		shift @ARGV;
+	}
+
+	if (@ARGV && $ARGV[0] ne '--') {
+		$patch_mode_revision = shift @ARGV;
+	}
+	@ARGV or die "missing --";
+	shift @ARGV;
+
+	if (defined $patch_mode) {
+		my $mode = $patch_modes{$patch_mode}
+			or die "unknown --patch mode: $patch_mode";
+
+		my $submode;
+		if (!defined $patch_mode_revision) {
+			$submode = $mode->{default};
+			if ($submode eq 'head') {
 				$patch_mode_revision = 'HEAD';
-				$arg = shift @ARGV or die "missing --";
-				if ($arg ne '--') {
-					$patch_mode_revision = $arg;
-					$patch_mode = ($arg eq 'HEAD' ?
-						       'reset_head' : 'reset_nothead');
-					$arg = shift @ARGV or die "missing --";
-				}
-			} elsif ($1 eq 'checkout') {
-				$arg = shift @ARGV or die "missing --";
-				if ($arg eq '--') {
-					$patch_mode = 'checkout_index';
-				} else {
-					$patch_mode_revision = $arg;
-					$patch_mode = ($arg eq 'HEAD' ?
-						       'checkout_head' : 'checkout_nothead');
-					$arg = shift @ARGV or die "missing --";
-				}
-			} elsif ($1 eq 'stage' or $1 eq 'stash') {
-				$patch_mode = $1;
-				$arg = shift @ARGV or die "missing --";
-			} else {
-				die "unknown --patch mode: $1";
 			}
-		} else {
-			$patch_mode = 'stage';
-			$arg = shift @ARGV or die "missing --";
 		}
-		die "invalid argument $arg, expecting --"
-		    unless $arg eq "--";
-		%patch_mode_flavour = %{$patch_modes{$patch_mode}};
-	}
-	elsif ($arg ne "--") {
-		die "invalid argument $arg, expecting --";
+		elsif ($patch_mode_revision eq 'HEAD') {
+			$submode = 'head';
+		}
+		else {
+			$submode = 'nothead';
+		}
+
+		if (!exists $mode->{$submode}) {
+			die "mode '$patch_mode' cannot handle '$submode'";
+		}
+		%patch_mode_flavour = %{$mode->{$submode}};
 	}
 }
 
-- 
1.7.5.4.31.ge4d5e

[PATCH 2/5] add--interactive: factor out regex error handling

[Jeff King]

When perl complains about compiling a regex the user has
given us, we trim the error to get a nicer message. It's
only one line of code, but it's ugly and non-obvious, so
let's factor it into a reusable function.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index c5cd300..3e4c8a4 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1267,6 +1267,12 @@ sub display_hunks {
 	return $i;
 }
 
+sub trim_error {
+	local $_ = shift;
+	s/ at .*git-add--interactive line \d+, <STDIN> line \d+.*$//;
+	return $_;
+}
+
 sub patch_update_file {
 	my $quit = 0;
 	my ($ix, $num);
@@ -1419,9 +1425,8 @@ sub patch_update_file {
 					$search_string = qr{$regex}m;
 				};
 				if ($@) {
-					my ($err,$exp) = ($@, $1);
-					$err =~ s/ at .*git-add--interactive line \d+, <STDIN> line \d+.*$//;
-					error_msg "Malformed search regexp $exp: $err\n";
+					my $exp = $1;
+					error_msg "Malformed search regexp $exp: " . trim_error($@) . "\n";
 					next;
 				}
 				my $iy = $ix;
-- 
1.7.5.4.31.ge4d5e

[PATCH 3/5] add--interactive: allow hunk filtering on command line

[Jeff King]

If you have a lot of uninteresting hunks (e.g., because you
have a file with boilerplate that changed, but you want to
add only the interesting bits and discard the rest), it can
be cumbersome to sift through the boring hunks.

This patch provides an option to match hunks by regular
expression, pretending as if the non-matching changes do not
exist at all (similar to how path-limiting ignores
non-matching paths entirely).

There are two special case pseudo-hunks to consider in the
code: mode hunks and deletion hunks.

  - If a filter is given, this patch will never include mode
    hunks, since the user has asked for a specific subset of
    hunks, and the mode has no text to match.

  - Deletion hunks actually require no special care. In a
    deletion there will be only a single giant diff hunk of
    deleted content. If it matches the filter, then the
    deletion can be presented as normal. If it doesn't
    match, then we exit early from patch_update_file and
    never consider showing the deletion at all (since there
    is nothing to delete).

This patch provides only the perl portion; it is up to
individual callers of add--interactive to pass the option
through to the perl code.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3e4c8a4..bb16f71 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -86,6 +86,7 @@ sub colored {
 # command line options
 my $patch_mode;
 my $patch_mode_revision;
+my @patch_mode_hunk_filter;
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
@@ -1273,17 +1274,33 @@ sub trim_error {
 	return $_;
 }
 
+sub want_hunk {
+	my $hunk = shift;
+	my $text = join('', @{$hunk->{TEXT}});
+
+	foreach my $re (@patch_mode_hunk_filter) {
+		return 1 if $text =~ $re;
+	}
+	return 0;
+}
+
 sub patch_update_file {
 	my $quit = 0;
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
+
+	if (@patch_mode_hunk_filter) {
+		@hunk = grep { want_hunk($_) } @hunk;
+		return unless @hunk;
+	}
+
 	($head, my $mode, my $deletion) = parse_diff_header($head);
 	for (@{$head->{DISPLAY}}) {
 		print;
 	}
 
-	if (@{$mode->{TEXT}}) {
+	if (@{$mode->{TEXT}} && !@patch_mode_hunk_filter) {
 		unshift @hunk, $mode;
 	}
 	if (@{$deletion->{TEXT}}) {
@@ -1568,6 +1585,11 @@ sub process_args {
 		if ($ARGV[0] =~ /--patch(?:=(.*))?/) {
 			$patch_mode = defined $1 ? $1 : 'stage';
 		}
+		elsif ($ARGV[0] =~ /--hunk-filter=(.*)/) {
+			my $re = eval { qr{$1}m }
+				or die "malformed hunk filter $1: " . trim_error($@);
+			push @patch_mode_hunk_filter, $re;
+		}
 		else {
 			last;
 		}
-- 
1.7.5.4.31.ge4d5e

[PATCH 4/5] add--interactive: allow negatation of hunk filters

[Jeff King]

This patch lets you filter out certain boring hunks, rather
than trying to match particular interesting ones (which may
be much easier, depending on your file contents).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index bb16f71..917b2a9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -87,6 +87,7 @@ sub colored {
 my $patch_mode;
 my $patch_mode_revision;
 my @patch_mode_hunk_filter;
+my $patch_mode_negate_filter;
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
@@ -1279,9 +1280,9 @@ sub want_hunk {
 	my $text = join('', @{$hunk->{TEXT}});
 
 	foreach my $re (@patch_mode_hunk_filter) {
-		return 1 if $text =~ $re;
+		return !$patch_mode_negate_filter if $text =~ $re;
 	}
-	return 0;
+	return $patch_mode_negate_filter;
 }
 
 sub patch_update_file {
@@ -1590,6 +1591,9 @@ sub process_args {
 				or die "malformed hunk filter $1: " . trim_error($@);
 			push @patch_mode_hunk_filter, $re;
 		}
+		elsif ($ARGV[0] eq '--negate-hunk-filter') {
+			$patch_mode_negate_filter = 1;
+		}
 		else {
 			last;
 		}
-- 
1.7.5.4.31.ge4d5e

[PATCH 5/5] add--interactive: add option to autosplit hunks

[Jeff King]

It is sometimes useful to see changes in their smallest
possible form, rather than complete hunks with overlapping
context. You can do this manually, of course, with the
's'plit hunk command. But when filtering hunks, you may want
to pre-split to give your filtering a finer granularity.

This patch adds only the perl infrastructure; to expose this
to the user, the various callers of add--interactive need to
pass the new option through to the perl script.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 917b2a9..41c7c6f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -88,6 +88,7 @@ my $patch_mode;
 my $patch_mode_revision;
 my @patch_mode_hunk_filter;
 my $patch_mode_negate_filter;
+my $patch_mode_autosplit;
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
@@ -1291,6 +1292,10 @@ sub patch_update_file {
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
 
+	if ($patch_mode_autosplit) {
+		@hunk = map { split_hunk($_->{TEXT}, $_->{DISPLAY}) } @hunk;
+	}
+
 	if (@patch_mode_hunk_filter) {
 		@hunk = grep { want_hunk($_) } @hunk;
 		return unless @hunk;
@@ -1594,6 +1599,9 @@ sub process_args {
 		elsif ($ARGV[0] eq '--negate-hunk-filter') {
 			$patch_mode_negate_filter = 1;
 		}
+		elsif ($ARGV[0] eq '--split-hunks') {
+			$patch_mode_autosplit = 1;
+		}
 		else {
 			last;
 		}
-- 
1.7.5.4.31.ge4d5e

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Tue, Jul 26, 2011 at 08:03:01PM +0700, Nguyen Thai Ngoc Duy wrote:

> It would be even more cool if --hunks (or whatever the name will be)
> could work without -p. I mean, if "git diff" supports it, then I can
> fine tune my regex to meet a selection of hunks I want, and verify it
> really is what I want. Then "git add --hunks=magic" and voila! (The
> "git add --hunks" without -p surely can be workaround by adding "-p",
> then accept all hunks).

Yeah, doing "yes | git add -p --hunks=foo" would probably do what you
want. But you don't really have a good way of verifying what it will add
(you could check after the fact what's left in "git diff", or what's now
in "git diff --cached", of course).

> And if diff machinery learns this, we would have "git log --hunks" too.
> 
> OK I'm asking too much..

Probably. :)

I'm not sure how useful "log --hunks" would be. The changes you commit
don't tend to be that big (well, not if you're doing it right). It seems
much more likely to have the case you brought up, which is that some
file has a bunch of boring boilerplate that doesn't need to be
changed, and you need to pick out the interesting changes from the
boilerplate changes.

I suppose if somebody committed all of the boilerplate changes (like .po
comment changes), then you would want to be able to pick them apart. But
that just seems like the wrong thing (i.e., if those comments really are
uninteresting, they should not be committed). But I don't work with .po
files at all, so maybe there is a good reason to commit them.

-Peff

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Nguyen Thai Ngoc Duy]

On Wed, Jul 27, 2011 at 3:10 PM, Jeff King <peff@peff.net> wrote:
> I'm not sure how useful "log --hunks" would be. The changes you commit
> don't tend to be that big (well, not if you're doing it right). It seems
> much more likely to have the case you brought up, which is that some
> file has a bunch of boring boilerplate that doesn't need to be
> changed, and you need to pick out the interesting changes from the
> boilerplate changes.
>
> I suppose if somebody committed all of the boilerplate changes (like .po
> comment changes), then you would want to be able to pick them apart. But
> that just seems like the wrong thing (i.e., if those comments really are
> uninteresting, they should not be committed). But I don't work with .po
> files at all, so maybe there is a good reason to commit them.

I was thinking of it as an extension of "git log -Sregex", where as -S
shows full diff of matched files, the new option only shows hunks that
actually match. Not sure if that is really useful though. On the other
hand, "git diff --hunk" is useful for me, I'll see if I can add that
option.
-- 
Duy

Re: feature request: git add--interactive --patch on regex-matched hunks only

[Jeff King]

On Wed, Jul 27, 2011 at 04:02:48PM +0700, Nguyen Thai Ngoc Duy wrote:

> I was thinking of it as an extension of "git log -Sregex", where as -S
> shows full diff of matched files, the new option only shows hunks that
> actually match. Not sure if that is really useful though. On the other
> hand, "git diff --hunk" is useful for me, I'll see if I can add that
> option.

I think it is more like "log -Gregex", which actually greps within the
diff. It would not be too hard to adapt the "-G" code to return the set
of hunks, rather than a simple "yes we have it". If you did that, it
would make the perl bits way simpler: they would simply call "diff
-Gregex" when creating the patch.

I don't think "git diff --hunk" would be that hard, either. The
trickiest part is that the diff code tries to output lines as soon as we
get them. You'd have to buffer whole chunks.

-Peff