From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Mon, 02 Apr 2012 17:20:22 GMT
Subject: Re: [PATCH] add -p: skip conflicted paths
Message-ID: <CABPQNSY8gNPZTV77AjFbHn1HA9S=fw3NC+H8bCzZOPFyLg0nHQ@mail.gmail.com>
In-Reply-To: <20120329054558.GA27604@sigill.intra.peff.net>

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