Re: [PATCH] add--interactive: respect diff.algorithm
- From
John Keeping <john@keeping.me.uk>
- Date
- Jun 23, 2013, 19:50 UTC
- Message-ID
- <20130623195017.GI4676@serenity.lan>
- In-Reply-To
- <7v8v20fxee.fsf@alter.siamese.dyndns.org>
On Sun, Jun 23, 2013 at 12:19:05PM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> John Keeping <john@keeping.me.uk> writes:
>
> >> > +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}";
> >> > + }
>
> This is not exactly sanitary for "stash -p", whose DIFF element is
> defined like so:
>
> 'stash' => {
> DIFF => 'diff-index -p HEAD',
>
> and you will end up appending an option after a non-option argument,
>
> It may happen to be accepted by the command line parser which is
> overly lax, but we would want to tighten it in the longer term.
>
> As a band-aid, we could do something like the attached patch, but
> for the longer term, we might need to rethink the way the tweaking
> of the command line is done by $patch_mode_revision.I originally used splice here then for some reason decided that just appending the option was okay (I think I thought we were already appending an option with $patch_mode_revision).
The patch below involves deeper Perl magic than I fully grok, but wouldn't it be simpler to simply use the fact that the string is "command --options..." and use:
splice @diff_cmd 1, 0, "--diff-algorithm=${diff_algorithm}";Show 50 quoted lines
> -- >8 --
> Subject: add -i: add extra options at the right place in "diff" command line
>
> Appending "--diff-algorithm=histogram" at the end of canned command
> line for various modes of "diff" is correct for most of them but not
> for "stash" that has a non-option already wired in, like so:
>
> 'stash' => {
> DIFF => 'diff-index -p HEAD',
>
> Appending an extra option after non-option may happen to work due to
> overly lax command line parser, but that is not something we should
> rely on. Instead, splice in the extra argument immediately after a
> '-p' option, which is an option to ask for textual diff output that
> has to be in all variants.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> git-add--interactive.perl | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 5310959..b50551a 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -730,11 +730,23 @@ sub run_git_apply {
> return close $fh;
> }
>
> +# The command array must have a single "-p" to ask for output in the
> +# patch form. Splice additional options immediately after it; we
> +# should not be randomly appending them, as some of the canned command.
> +# has non-option argument like HEAD already on it.
> +
> +sub splice_diff_options {
> + my $diff_cmd = shift;
> + @$diff_cmd = map {
> + ($_ eq '-p') ? ($_, @_) : $_;
> + } @$diff_cmd;
> +}
> +
> sub parse_diff {
> my ($path) = @_;
> my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
> if (defined $diff_algorithm) {
> - push @diff_cmd, "--diff-algorithm=${diff_algorithm}";
> + splice_diff_options(\@diff_cmd, "--diff-algorithm=${diff_algorithm}");
> }
> if (defined $patch_mode_revision) {
> push @diff_cmd, $patch_mode_revision;