[PATCH] add -p: skip conflicted paths

[PATCH] add -p: skip conflicted paths

[Erik Faye-Lund]

When performing "git add -p" on a file in a conflicted state, we
currently spew the diff and terminate the process.

This is not very helpful to the user. Change the behaviour to
skipping the file, while outputting a warning.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

OK, here's a quick stab at fixing the "add -p" issue. Note that
I'm not very fluent in Perl, so apologies if this is not up to
standards.

 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 8f0839d..a52507f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1259,6 +1259,13 @@ sub patch_update_file {
 	my $quit = 0;
 	my ($ix, $num);
 	my $path = shift;
+
+	# skip conflicted paths
+	if (run_cmd_pipe(qw(git ls-files -u --), $path)) {
+		print colored $error_color, "Warning: $path is in conflicted state, skipping.\n";
+		return 0;
+	}
+
 	my ($head, @hunk) = parse_diff($path);
 	($head, my $mode, my $deletion) = parse_diff_header($head);
 	for (@{$head->{DISPLAY}}) {
-- 
1.7.9

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 8f0839d..a52507f 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1259,6 +1259,13 @@ sub patch_update_file {
>  	my $quit = 0;
>  	my ($ix, $num);
>  	my $path = shift;
> +
> +	# skip conflicted paths
> +	if (run_cmd_pipe(qw(git ls-files -u --), $path)) {
> +		print colored $error_color, "Warning: $path is in conflicted state, skipping.\n";
> +		return 0;
> +	}
> +

Thanks.

I have to wonder if the filtering should go to much higher level in the
callchain.

Perhaps teach list_modified(), which currently takes 'file-only' and
'index-only', to also take an option to omit (and warn if it is
appropriate) unmerged paths?

[PATCH] add -p: skip conflicted paths

[Erik Faye-Lund]

When performing "git add -p" on a file in a conflicted state, we
currently spew the diff and terminate the process.

This is not very helpful to the user. Change the behaviour to
skipping the file, while outputting a warning.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

On Wed, Mar 28, 2012 at 9:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps teach list_modified(), which currently takes 'file-only' and
> 'index-only', to also take an option to omit (and warn if it is
> appropriate) unmerged paths?

Good idea. This way, the path doesn't even get listed when using
git add -i, and no warning is spewed on "git add -p" without specifying
the path. It seems like the right thing to do.

Again, I'm no Perl-guru, so apologies if the code isn't idiomatic.

 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 8f0839d..4913203 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -270,7 +270,7 @@ sub get_empty_tree {
 # FILE_ADDDEL:	is it add/delete between index and file?
 
 sub list_modified {
-	my ($only) = @_;
+	my ($only, $omit_unmerged) = @_;
 	my (%data, @return);
 	my ($add, $del, $adddel, $file);
 	my @tracked = ();
@@ -359,6 +359,10 @@ sub list_modified {
 				next if ($it->{FILE} eq 'nothing');
 			}
 		}
+		if ($omit_unmerged && run_cmd_pipe(qw(git ls-files -u --), $_)) {
+			print colored $error_color, "Warning: $_ is in conflicted state, skipping.\n" if @ARGV;
+			next;
+		}
 		push @return, +{
 			VALUE => $_,
 			%$it,
@@ -1189,7 +1193,7 @@ sub apply_patch_for_checkout_commit {
 }
 
 sub patch_update_cmd {
-	my @all_mods = list_modified($patch_mode_flavour{FILTER});
+	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 1);
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
-- 
1.7.9

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> When performing "git add -p" on a file in a conflicted state, we
> currently spew the diff and terminate the process.
>
> This is not very helpful to the user. Change the behaviour to
> skipping the file, while outputting a warning.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>
> On Wed, Mar 28, 2012 at 9:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Perhaps teach list_modified(), which currently takes 'file-only' and
>> 'index-only', to also take an option to omit (and warn if it is
>> appropriate) unmerged paths?
>
> Good idea. This way, the path doesn't even get listed when using
> git add -i, and no warning is spewed on "git add -p" without specifying
> the path. It seems like the right thing to do.
>
> Again, I'm no Perl-guru, so apologies if the code isn't idiomatic.

I personally don't worry about Perly-ness in the early rounds of the
review.

I was hoping that we could somehow do this with a single invocation of
ls-files, instead of doing it over and over inside a loop.

Totally untested, but something along this line...

 git-add--interactive.perl |   31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..8bf1835 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -270,16 +270,35 @@ sub get_empty_tree {
 # FILE_ADDDEL:	is it add/delete between index and file?
 
 sub list_modified {
-	my ($only) = @_;
+	my ($only, $filter_unmerged) = @_;
 	my (%data, @return);
 	my ($add, $del, $adddel, $file);
 	my @tracked = ();
 
-	if (@ARGV) {
-		@tracked = map {
+	if (@ARGV || $filter_unmerged) {
+		my %unmerged = ();
+		for (run_cmd_pipe(qw(git ls-files -s --), @ARGV)) {
 			chomp $_;
-			unquote_path($_);
-		} run_cmd_pipe(qw(git ls-files --), @ARGV);
+			if (/^[0-7]+ [0-9a-f]{40} ([0-3])	(.*)/) {
+				my ($stage, $path) = ($1, unquote_path($2));
+				if ($stage eq '0') {
+					push @tracked, $path;
+				} else {
+					$unmerged{$path} = 1;
+				}
+			} else {
+				die "Oops: $_";
+			}
+		}
+		if (%unmerged) {
+			if ($filter_unmerged) {
+				for (sort keys %unmerged) {
+					warn "unmerged: $_";
+				}
+			} else {
+				@tracked = sort (@tracked, keys %unmerged);
+			}
+		}
 		return if (!@tracked);
 	}
 
@@ -1189,7 +1208,7 @@ sub apply_patch_for_checkout_commit {
 }
 
 sub patch_update_cmd {
-	my @all_mods = list_modified($patch_mode_flavour{FILTER});
+	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'filter-unmerged');
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I was hoping that we could somehow do this with a single invocation of
> ls-files, instead of doing it over and over inside a loop.
>
> Totally untested, but something along this line...

Well, probably along that line but not there.  I think the patch would be
a lot cleaner to keep the part I touched intact, and instead add an extra
"ls-files -u" that creates %unmerged hash in the way this patch does,
immediately before the last for() loop in the function.  And then the loop
can use %unmerged hash to filter the elements.

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Totally untested, but something along this line...
>
> Well, probably along that line but not there.  I think the patch would be
> a lot cleaner to keep the part I touched intact, and instead add an extra
> "ls-files -u" that creates %unmerged hash in the way this patch does,
> immediately before the last for() loop in the function.  And then the loop
> can use %unmerged hash to filter the elements.

That is, something like this.

 git-add--interactive.perl |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..ddb2e77 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -270,7 +270,7 @@ sub get_empty_tree {
 # FILE_ADDDEL:	is it add/delete between index and file?
 
 sub list_modified {
-	my ($only) = @_;
+	my ($only, $filter_unmerged) = @_;
 	my (%data, @return);
 	my ($add, $del, $adddel, $file);
 	my @tracked = ();
@@ -348,9 +348,26 @@ sub list_modified {
 		}
 	}
 
+	my %unmerged;
+	if ($filter_unmerged) {
+		for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
+			chomp $_;
+			if (/^[0-7]+ [0-9a-f]{40} [0-3]	(.*)/) {
+				my $path = unquote_path($1);
+				$unmerged{$path} = 1;
+			}
+		}
+		if (%unmerged) {
+			for (sort keys %unmerged) {
+				print colored $error_color, "ignoring unmerged: $_\n";
+			}
+		}
+	}
+
 	for (sort keys %data) {
-		my $it = $data{$_};
+		next if exists $unmerged{$_};
 
+		my $it = $data{$_};
 		if ($only) {
 			if ($only eq 'index-only') {
 				next if ($it->{INDEX} eq 'unchanged');
@@ -1189,7 +1206,7 @@ sub apply_patch_for_checkout_commit {
 }
 
 sub patch_update_cmd {
-	my @all_mods = list_modified($patch_mode_flavour{FILTER});
+	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'filter-unmerged');
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 

Re: [PATCH] add -p: skip conflicted paths

[Jeff King]

On Wed, Mar 28, 2012 at 03:14:31PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> Totally untested, but something along this line...
> >
> > Well, probably along that line but not there.  I think the patch would be
> > a lot cleaner to keep the part I touched intact, and instead add an extra
> > "ls-files -u" that creates %unmerged hash in the way this patch does,
> > immediately before the last for() loop in the function.  And then the loop
> > can use %unmerged hash to filter the elements.
> 
> That is, something like this.

That is way better. Your original one would end up putting every file in
the repo into @tracked, which would then be fed on the command-line to
"diff-index" and company. I suspect on a large repo that could overflow
the command-line limits (plus I recall that at one point we performed
way worse on "git diff $(git ls-files)" than we do on "git diff", but
that may not be the case anymore).

However, I can think of two possible improvements:

> +	my %unmerged;
> +	if ($filter_unmerged) {
> +		for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
> +			chomp $_;
> +			if (/^[0-7]+ [0-9a-f]{40} [0-3]	(.*)/) {
> +				my $path = unquote_path($1);
> +				$unmerged{$path} = 1;
> +			}
> +		}
> +		if (%unmerged) {
> +			for (sort keys %unmerged) {
> +				print colored $error_color, "ignoring unmerged: $_\n";
> +			}
> +		}
> +	}

I like that we are down to a single ls-files invocation here. But can't
we determine from the diff-files output whether an entry is unmerged. In
my simple tests, I see that --numstat will show two lines for such an
entry. Is that reliable?

>  sub patch_update_cmd {
> -	my @all_mods = list_modified($patch_mode_flavour{FILTER});
> +	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'filter-unmerged');
>  	my @mods = grep { !($_->{BINARY}) } @all_mods;

It seems like a more flexible interface would not be to filter unmerged
entries, but rather to mark them as we do with BINARY. And then the
caller can do as they please, discarding, printing warnings, etc.

Right now, the behavior is to simply skip them. But it would be cool if
we could eventually show a useful presentation of the changes and ask to
stage them. I know from our past discussions it is not quite as feeding
the combined diff to the regular hunk-selector. But I'm sure we can come
up with something reasonable.

Here's the patch that I came up with to do both of those things. Like I
said, I am somewhat unsure of the "detect double mentions as unmerged"
rule. But we could still use the "ls-files -u" output to mark the
unmerged files.

---
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..6204207 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -336,7 +336,14 @@ sub list_modified {
 			else {
 				$change = "+$add/-$del";
 			}
-			$data{$file}{FILE} = $change;
+			# If we see it twice, it's unmerged.
+			if (defined $data{$file}{FILE} &&
+			    $data{$file}{FILE} ne 'nothing') {
+				$data{$file}{FILE} = 'unmerged';
+			}
+			else {
+				$data{$file}{FILE} = $change;
+			}
 			if ($bin) {
 				$data{$file}{BINARY} = 1;
 			}
@@ -1193,6 +1200,10 @@ sub patch_update_cmd {
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
+	print colored $error_color, "ignoring unmerged: $_->{VALUE}\n"
+		for grep { $_->{FILE} eq 'unmerged' } @mods;
+	@mods = grep { $_->{FILE} ne 'unmerged' } @mods;
+
 	if (!@mods) {
 		if (@all_mods) {
 			print STDERR "Only binary files changed.\n";

Re: [PATCH] add -p: skip conflicted paths

[Erik Faye-Lund]

On Thu, Mar 29, 2012 at 7:45 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 28, 2012 at 03:14:31PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> >> Totally untested, but something along this line...
>> >
>> > Well, probably along that line but not there.  I think the patch would be
>> > a lot cleaner to keep the part I touched intact, and instead add an extra
>> > "ls-files -u" that creates %unmerged hash in the way this patch does,
>> > immediately before the last for() loop in the function.  And then the loop
>> > can use %unmerged hash to filter the elements.
>>
>> That is, something like this.
>
> That is way better. Your original one would end up putting every file in
> the repo into @tracked, which would then be fed on the command-line to
> "diff-index" and company. I suspect on a large repo that could overflow
> the command-line limits (plus I recall that at one point we performed
> way worse on "git diff $(git ls-files)" than we do on "git diff", but
> that may not be the case anymore).
>
> However, I can think of two possible improvements:
>
>> +     my %unmerged;
>> +     if ($filter_unmerged) {
>> +             for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
>> +                     chomp $_;
>> +                     if (/^[0-7]+ [0-9a-f]{40} [0-3] (.*)/) {
>> +                             my $path = unquote_path($1);
>> +                             $unmerged{$path} = 1;
>> +                     }
>> +             }
>> +             if (%unmerged) {
>> +                     for (sort keys %unmerged) {
>> +                             print colored $error_color, "ignoring unmerged: $_\n";
>> +                     }
>> +             }
>> +     }
>
> I like that we are down to a single ls-files invocation here. But can't
> we determine from the diff-files output whether an entry is unmerged. In
> my simple tests, I see that --numstat will show two lines for such an
> entry. Is that reliable?
>

Nice. I've observed the same thing (although I've seen three entries,
not two). I don't know about the reliability, but I think it kind of
makes sense; one entry for both parents, and one for the unmerged
working-copy version.

Junio, what do you think?

>>  sub patch_update_cmd {
>> -     my @all_mods = list_modified($patch_mode_flavour{FILTER});
>> +     my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'filter-unmerged');
>>       my @mods = grep { !($_->{BINARY}) } @all_mods;
>
> It seems like a more flexible interface would not be to filter unmerged
> entries, but rather to mark them as we do with BINARY. And then the
> caller can do as they please, discarding, printing warnings, etc.
>
> Right now, the behavior is to simply skip them. But it would be cool if
> we could eventually show a useful presentation of the changes and ask to
> stage them. I know from our past discussions it is not quite as feeding
> the combined diff to the regular hunk-selector. But I'm sure we can come
> up with something reasonable.
>
> Here's the patch that I came up with to do both of those things. Like I
> said, I am somewhat unsure of the "detect double mentions as unmerged"
> rule. But we could still use the "ls-files -u" output to mark the
> unmerged files.
>
> ---
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 8f0839d..6204207 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -336,7 +336,14 @@ sub list_modified {
>                        else {
>                                $change = "+$add/-$del";
>                        }
> -                       $data{$file}{FILE} = $change;
> +                       # If we see it twice, it's unmerged.
> +                       if (defined $data{$file}{FILE} &&
> +                           $data{$file}{FILE} ne 'nothing') {
> +                               $data{$file}{FILE} = 'unmerged';
> +                       }
> +                       else {
> +                               $data{$file}{FILE} = $change;
> +                       }
>                        if ($bin) {
>                                $data{$file}{BINARY} = 1;
>                        }
> @@ -1193,6 +1200,10 @@ sub patch_update_cmd {
>        my @mods = grep { !($_->{BINARY}) } @all_mods;
>        my @them;
>
> +       print colored $error_color, "ignoring unmerged: $_->{VALUE}\n"
> +               for grep { $_->{FILE} eq 'unmerged' } @mods;
> +       @mods = grep { $_->{FILE} ne 'unmerged' } @mods;
> +
>        if (!@mods) {
>                if (@all_mods) {
>                        print STDERR "Only binary files changed.\n";

I like both the result and the flexibility. There's one minor nit,
though: Now "git add -i" says "Only binary files changed.", which
isn't true. But this is a simple matter of updating the warning to
something like "Only binary or unmerged files changed.".

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Thu, Mar 29, 2012 at 7:45 AM, Jeff King <peff@peff.net> wrote:
>>
>> I like that we are down to a single ls-files invocation here. But can't
>> we determine from the diff-files output whether an entry is unmerged. In
>> my simple tests, I see that --numstat will show two lines for such an
>> entry. Is that reliable?
>
> Nice. I've observed the same thing (although I've seen three entries,
> not two). I don't know about the reliability, but I think it kind of
> makes sense; one entry for both parents, and one for the unmerged
> working-copy version.

Should I be dissappointed, or should I be happy for seeing "division of
labor" working?

By default "diff-files" compares stage #2 with the working tree files, and
at the same time shows an unmerged record.  When there is no stage #2
entry, you will only see the unmerged record.

Notice that 1_3 is shown only once in the --numstat part of the output if
you run the script attached at the end.

I think the easiest thing to get the right information is to add --raw to
get the raw entries and read the status letters off of them.


-- >8 --
#!/bin/sh

mkdir -p /tmp/xprm && mkdir /tmp/xprm/stages && cd /tmp/xprm/stages || exit

git init

for i in 1 2 3
do
	echo "$i" | git hash-object -w --stdin
done
git update-index --index-info <<EOF
100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1	123
100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2	123
100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 3	123
100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1	12_
100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2	12_
100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1	1_3
100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 3	1_3
100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2	_23
100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 3	_23
EOF

git diff-files --numstat --summary --raw --abbrev=4

Re: [PATCH] add -p: skip conflicted paths

[Jeff King]

On Mon, Apr 02, 2012 at 11:25:33AM -0700, Junio C Hamano wrote:

> >> I like that we are down to a single ls-files invocation here. But can't
> >> we determine from the diff-files output whether an entry is unmerged. In
> >> my simple tests, I see that --numstat will show two lines for such an
> >> entry. Is that reliable?
> >
> > Nice. I've observed the same thing (although I've seen three entries,
> > not two). I don't know about the reliability, but I think it kind of
> > makes sense; one entry for both parents, and one for the unmerged
> > working-copy version.
> 
> Should I be dissappointed, or should I be happy for seeing "division of
> labor" working?

The latter. I suspected something similar when writing my original
message, but didn't think too hard about it. Thanks for clarifying.

I do still think it would be nicer to pass the information out to the
caller instead of just filtering. So combining the two patches, we have
something like:

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 2ee0908..0a83f56 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -292,7 +292,7 @@ sub get_empty_tree {
 # FILE_ADDDEL:	is it add/delete between index and file?
 
 sub list_modified {
-	my ($only) = @_;
+	my ($only, $note_unmerged) = @_;
 	my (%data, @return);
 	my ($add, $del, $adddel, $file);
 	my @tracked = ();
@@ -370,6 +370,18 @@ sub list_modified {
 		}
 	}
 
+	if ($note_unmerged) {
+		for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
+			chomp $_;
+			if (/^[0-7]+ [0-9a-f]{40} [0-3]	(.*)/) {
+				my $path = unquote_path($1);
+				if (exists($data{$path})) {
+					$data{$path}{UNMERGED} = 1;
+				}
+			}
+		}
+	}
+
 	for (sort keys %data) {
 		my $it = $data{$_};
 
@@ -1211,10 +1223,14 @@ sub apply_patch_for_checkout_commit {
 }
 
 sub patch_update_cmd {
-	my @all_mods = list_modified($patch_mode_flavour{FILTER});
+	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'note-unmerged');
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
+	print colored $error_color, "ignoring unmerged: $_->{VALUE}\n"
+		for grep { $_->{UNMERGED} } @mods;
+	@mods = grep { !$_->{UNMERGED} } @mods;
+
 	if (!@mods) {
 		if (@all_mods) {
 			print STDERR "Only binary files changed.\n";

Re: [PATCH] add -p: skip conflicted paths

[Tay Ray Chuan]

On Wed, Apr 4, 2012 at 5:46 PM, Jeff King <peff@peff.net> wrote:
> I do still think it would be nicer to pass the information out to the
> caller instead of just filtering. So combining the two patches, we have
> something like:

Thanks Maestros! This has been bugging me for some time.

I gave the combined patch a shot with a trivial conflict, and it gave
me "Only binary files changed". Turns out this only happens with a
tree that has a only conflicted files; with a tree that has a mixed of
conflicted and modified paths, it doesn't show up.

Below is a quick and dirty fix:

-->8--
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 28d36f7..96f12ca 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1207,6 +1207,7 @@ sub patch_update_cmd {

        print colored $error_color, "ignoring unmerged: $_->{VALUE}\n"
                for grep { $_->{UNMERGED} } @mods;
+       @all_mods = grep { !$_->{UNMERGED} } @all_mods;
        @mods = grep { !$_->{UNMERGED} } @mods;

        if (!@mods) {
--

-- 
Cheers,
Ray Chuan

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do still think it would be nicer to pass the information out to the
> caller instead of just filtering.

Indeed.

> So combining the two patches, we have
> something like:

Hrm.  I kind of liked the idea of doing this with a single plumbing call
to diff-files (the entries that come from --raw will be mostly discarded
except for the ones that are marked with "U"), though.

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 2ee0908..0a83f56 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -292,7 +292,7 @@ sub get_empty_tree {
>  # FILE_ADDDEL:	is it add/delete between index and file?
>  
>  sub list_modified {
> -	my ($only) = @_;
> +	my ($only, $note_unmerged) = @_;
>  	my (%data, @return);
>  	my ($add, $del, $adddel, $file);
>  	my @tracked = ();
> @@ -370,6 +370,18 @@ sub list_modified {
>  		}
>  	}
>  
> +	if ($note_unmerged) {
> +		for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
> +			chomp $_;
> +			if (/^[0-7]+ [0-9a-f]{40} [0-3]	(.*)/) {
> +				my $path = unquote_path($1);
> +				if (exists($data{$path})) {
> +					$data{$path}{UNMERGED} = 1;
> +				}
> +			}
> +		}
> +	}
> +
>  	for (sort keys %data) {
>  		my $it = $data{$_};
>  
> @@ -1211,10 +1223,14 @@ sub apply_patch_for_checkout_commit {
>  }
>  
>  sub patch_update_cmd {
> -	my @all_mods = list_modified($patch_mode_flavour{FILTER});
> +	my @all_mods = list_modified($patch_mode_flavour{FILTER}, 'note-unmerged');
>  	my @mods = grep { !($_->{BINARY}) } @all_mods;
>  	my @them;
>  
> +	print colored $error_color, "ignoring unmerged: $_->{VALUE}\n"
> +		for grep { $_->{UNMERGED} } @mods;
> +	@mods = grep { !$_->{UNMERGED} } @mods;
> +
>  	if (!@mods) {
>  		if (@all_mods) {
>  			print STDERR "Only binary files changed.\n";

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I do still think it would be nicer to pass the information out to the
>> caller instead of just filtering.
>
> Indeed.
>
>> So combining the two patches, we have
>> something like:
>
> Hrm.  I kind of liked the idea of doing this with a single plumbing call
> to diff-files (the entries that come from --raw will be mostly discarded
> except for the ones that are marked with "U"), though.

That is, something like this on top of your patch.

 git-add--interactive.perl |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 28d36f7..28c28b7 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -318,7 +318,9 @@ sub list_modified {
 		}
 	}
 
-	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
+	for (run_cmd_pipe(qw(git diff-files --numstat --summary),
+			  ($note_unmerged ? ("--raw") : ()),
+			  "--", @tracked)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			$file = unquote_path($file);
@@ -340,26 +342,17 @@ sub list_modified {
 			if ($bin) {
 				$data{$file}{BINARY} = 1;
 			}
-		}
-		elsif (($adddel, $file) =
-		       /^ (create|delete) mode [0-7]+ (.*)$/) {
+		} elsif (/^:[0-7]+ [0-7]+ [0-9a-f]+ [0-9a-f]+ (.)	(.*)$/) {
+			if ($1 eq 'U') {
+				$file = unquote_path($2);
+				$data{$file}{UNMERGED} = 1;
+			}
+		} elsif (($adddel, $file) = /^ (create|delete) mode [0-7]+ (.*)$/) {
 			$file = unquote_path($file);
 			$data{$file}{FILE_ADDDEL} = $adddel;
 		}
 	}
 
-	if ($note_unmerged) {
-		for (run_cmd_pipe(qw(git ls-files -u --), @ARGV)) {
-			chomp $_;
-			if (/^[0-7]+ [0-9a-f]{40} [0-3]	(.*)/) {
-				my $path = unquote_path($1);
-				if (exists($data{$path})) {
-					$data{$path}{UNMERGED} = 1;
-				}
-			}
-		}
-	}
-
 	for (sort keys %data) {
 		my $it = $data{$_};
 

Re: [PATCH] add -p: skip conflicted paths

[Jeff King]

On Wed, Apr 04, 2012 at 11:29:21AM -0700, Junio C Hamano wrote:

> >> So combining the two patches, we have
> >> something like:
> >
> > Hrm.  I kind of liked the idea of doing this with a single plumbing call
> > to diff-files (the entries that come from --raw will be mostly discarded
> > except for the ones that are marked with "U"), though.
> 
> That is, something like this on top of your patch.

Ugh, I totally misread your original message and missed the fact that we
_could_ see what we wanted by adding "--raw". Sorry to be dense.

Yes, this is way better. I don't mind discarding the --raw entries that
are not used; they cost nothing to generate on top of what we are
already doing, so it is really just the cost of shuttling a few bytes
across the pipe.

> -	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
> +	for (run_cmd_pipe(qw(git diff-files --numstat --summary),
> +			  ($note_unmerged ? ("--raw") : ()),
> +			  "--", @tracked)) {

Maybe it is not worth even having $note_unmerged, and just filling in
the UNMERGED field unconditionally? I know other callers don't care
about the information, but it's so cheap, and it just makes the function
interface that much simpler.

-Peff

Re: [PATCH] add -p: skip conflicted paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, this is way better. I don't mind discarding the --raw entries that
> are not used; they cost nothing to generate on top of what we are
> already doing, so it is really just the cost of shuttling a few bytes
> across the pipe.
>
>> -	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
>> +	for (run_cmd_pipe(qw(git diff-files --numstat --summary),
>> +			  ($note_unmerged ? ("--raw") : ()),
>> +			  "--", @tracked)) {
>
> Maybe it is not worth even having $note_unmerged, and just filling in
> the UNMERGED field unconditionally? I know other callers don't care
> about the information, but it's so cheap, and it just makes the function
> interface that much simpler.

Perhaps.  Care to do the honors of rolling the final version perhaps with
a test?

Re: [PATCH] add -p: skip conflicted paths

[Jeff King]

On Wed, Apr 04, 2012 at 02:31:56PM -0700, Junio C Hamano wrote:

> >> -	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
> >> +	for (run_cmd_pipe(qw(git diff-files --numstat --summary),
> >> +			  ($note_unmerged ? ("--raw") : ()),
> >> +			  "--", @tracked)) {
> >
> > Maybe it is not worth even having $note_unmerged, and just filling in
> > the UNMERGED field unconditionally? I know other callers don't care
> > about the information, but it's so cheap, and it just makes the function
> > interface that much simpler.
> 
> Perhaps.  Care to do the honors of rolling the final version perhaps with
> a test?

Here it is. The three interesting changes are:

  1. It tweaks @all_mods properly to produce "No changes" or "Only
     binary changes" as appropriate. So you might see:

        ignoring unmerged: foo
        No changes.

     which I think is clear enough.

  2. Earlier iterations did not handle the "create a record with
     INDEX => 'unchanged' if it did not appear in diff-index" code path.
     So if we made an UNMERGED entry from something that had not
     appeared in "git diff-index", then it could end up not having an
     INDEX field at all.

     I'm not sure this can happen in normal use, as generally diff-index
     would mention any conflicted entries. You can trigger this with
     regular "git diff-index" if the path is missing in HEAD and in
     the working tree, but not with "git diff-index --cached". So maybe
     it is not possible to trigger, but it was a bit too subtle for my
     taste, so I took the more obvious approach you see below.

  3. I added a test. I used a more plausible merge conflict instead of
     tweaking the index as you showed in an earlier email.  While the
     latter would have caught the particular ill-conceived
     implementation I posted earlier, I thought it was more important to
     check a more real-world setup (where HEAD actually has real data in
     it).

-- >8 --
Subject: [PATCH] add--interactive: ignore unmerged entries in patch mode

When "add -p" sees an unmerged entry, it shows the combined
diff and then immediately skips the hunk. This can be
confusing in a variety of ways, depending on whether there
are other changes to stage (in which case you get the
superfluous combined diff output in between other hunks) or
not (in which case you get the combined diff and the program
exits immediately, rather than seeing "No changes").

The current behavior was not planned, and is just what the
implementation happens to do. Instead, let's explicitly
remove unmerged entries from our list of modified files, and
print a warning that we are ignoring them.

We can cheaply find which entries are unmerged by adding
"--raw" output to the "diff-files --numstat" we already run.
There is one non-obvious thing we must change when parsing
this combined output. Before this patch, when we saw a
numstat line for a file that did not have index changes, we
would create a new record with 'unchanged' in the 'INDEX'
field.  Because "--raw" comes before "--numstat", we must
move this special-case down to the raw-line case (and it is
sufficient to move it rather than handle it in both places,
since any file which has a --numstat will also have a --raw
entry).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl  |   25 ++++++++++++++++++-------
 t/t3701-add-interactive.sh |   26 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8f0839d..d948aa8 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -268,6 +268,7 @@ sub get_empty_tree {
 # FILE:		is file different from index?
 # INDEX_ADDDEL:	is it add/delete between HEAD and index?
 # FILE_ADDDEL:	is it add/delete between index and file?
+# UNMERGED:	is the path unmerged
 
 sub list_modified {
 	my ($only) = @_;
@@ -318,16 +319,10 @@ sub list_modified {
 		}
 	}
 
-	for (run_cmd_pipe(qw(git diff-files --numstat --summary --), @tracked)) {
+	for (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @tracked)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			$file = unquote_path($file);
-			if (!exists $data{$file}) {
-				$data{$file} = +{
-					INDEX => 'unchanged',
-					BINARY => 0,
-				};
-			}
 			my ($change, $bin);
 			if ($add eq '-' && $del eq '-') {
 				$change = 'binary';
@@ -346,6 +341,18 @@ sub list_modified {
 			$file = unquote_path($file);
 			$data{$file}{FILE_ADDDEL} = $adddel;
 		}
+		elsif (/^:[0-7]+ [0-7]+ [0-9a-f]+ [0-9a-f]+ (.)	(.*)$/) {
+			$file = unquote_path($2);
+			if (!exists $data{$file}) {
+				$data{$file} = +{
+					INDEX => 'unchanged',
+					BINARY => 0,
+				};
+			}
+			if ($1 eq 'U') {
+				$data{$file}{UNMERGED} = 1;
+			}
+		}
 	}
 
 	for (sort keys %data) {
@@ -1190,6 +1197,10 @@ sub apply_patch_for_checkout_commit {
 
 sub patch_update_cmd {
 	my @all_mods = list_modified($patch_mode_flavour{FILTER});
+	error_msg "ignoring unmerged: $_->{VALUE}\n"
+		for grep { $_->{UNMERGED} } @all_mods;
+	@all_mods = grep { !$_->{UNMERGED} } @all_mods;
+
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 9e236f9..098a6ae 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -330,4 +330,30 @@ test_expect_success PERL 'split hunk "add -p (edit)"' '
 	! grep "^+15" actual
 '
 
+test_expect_success 'patch mode ignores unmerged entries' '
+	git reset --hard &&
+	test_commit conflict &&
+	test_commit non-conflict &&
+	git checkout -b side &&
+	test_commit side conflict.t &&
+	git checkout master &&
+	test_commit master conflict.t &&
+	test_must_fail git merge side &&
+	echo changed >non-conflict.t &&
+	echo y | git add -p >output &&
+	! grep a/conflict.t output &&
+	cat >expected <<-\EOF &&
+	* Unmerged path conflict.t
+	diff --git a/non-conflict.t b/non-conflict.t
+	index f766221..5ea2ed4 100644
+	--- a/non-conflict.t
+	+++ b/non-conflict.t
+	@@ -1 +1 @@
+	-non-conflict
+	+changed
+	EOF
+	git diff --cached >diff &&
+	test_cmp expected diff
+'
+
 test_done
-- 
1.7.10.rc4.3.gb05f6