Re: Different diff strategies in add --interactive

From: Jeff King <peff@peff.net>
Date: Jun 10, 2013, 21:56 UTC
Message-ID: <20130610215656.GA28345@sigill.intra.peff.net>
In-Reply-To: <20130610214638.GK22905@serenity.lan>

On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:

Show 15 quoted lines

> > Overall, I think respecting diff.algorithm in add--interactive is a very
> > sane thing to do. I would even be tempted to say we should allow a few
> > other select diff options (e.g., fewer or more context lines). If you
> > allowed diff options like this:
> > 
> >   git add --patch="--patience -U5"
> > 
> > that is very flexible, but I would not want to think about what the code
> > does when you pass --patch="--raw" or equal nonsense.
> 
> An alternative would be to permit them to be set from within the
> interactive UI.  I'd find it quite useful to experiment with various
> diff options when I encounter a hunk that isn't as easy to pick as I'd
> like.  I expect it would be very hard to do that on a per-hunk basis,
> although per-file doesn't seem like it would be too hard.

That's an interesting idea, for a subset of options (e.g., "increase context for this hunk"). I suspect implementing it would be painful, though, as you would have to re-run diff, and you have no guarantee of getting the same set of hunks (e.g., the hunk might end up coalesced with another).

> I don't intend to investigate that though - respecting diff.algorithm is
> good enough for my usage.

I don't blame you. :)

Show 22 quoted lines

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index d2c4ce6..0b0fac2 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -44,6 +44,8 @@ my ($diff_new_color) =
>  
>  my $normal_color = $repo->get_color("", "reset");
>  
> +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');
> +
>  my $use_readkey = 0;
>  my $use_termcap = 0;
>  my %term_escapes;
> @@ -731,6 +733,9 @@ sub run_git_apply {
>  sub parse_diff {
>  	my ($path) = @_;
>  	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
> +	if ($diff_algorithm ne "default") {
> +		push @diff_cmd, "--diff-algorithm=${diff_algorithm}";
> +	}
>  	if (defined $patch_mode_revision) {
>  		push @diff_cmd, $patch_mode_revision;

Yeah, that looks like the sane way to do it to me. As a perl style thing, I think the usual way of spelling 'default' is 'undef'. I.e.:

  my $diff_algorithm = $repo->config('diff.algorithm');
  ...
  if (defined $diff_algorithm) {
          push @diff_cmd, "--diff-algorithm=$diff_algorithm";
  }

-Peff