Re: [PATCH] add--interactive: respect diff.algorithm
John Keeping writes:
> 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}";
That inserts the extra options as the first thing on the command
line, which is also fine.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add--interactive: respect diff.algorithm
On Sun, Jun 23, 2013 at 12:19:05PM -0700, Junio C Hamano wrote:
> John Keeping 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}";
> -- >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
> ---
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add--interactive: respect diff.algorithm
John Keeping 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.
-- >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
---
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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add--interactive: respect diff.algorithm
On Wed, Jun 12, 2013 at 07:44:10PM +0100, John Keeping wrote:
> When staging hunks interactively it is sometimes useful to use an
> alternative diff algorithm which splits the changes into hunks in a more
> logical manner. This is not possible because the plumbing commands
> called by add--interactive ignore the "diff.algorithm" configuration
> option (as they should).
>
> Since add--interactive is a porcelain command it should respect this
> configuration variable. To do this, make it read diff.algorithm and
> pass its value to the underlying diff-index and diff-files invocations.
>
> At this point, do not add options to "git add", "git reset" or "git
> checkout" (all of which can call git-add--interactive). If a user want
s/want/wants/
> > if (defined $diff_algorithm) {
> > push @diff_cmd, "--diff-algorithm=$diff_algorithm";
> > }
>
> OK. The "default" is actually "the value that is equivalent to 'myers'
> for diff.algorithm" and I was originally going to add --diff-algorithm
> to the command line unconditionally.
Yeah, that might have made sense, too, but the in-between (we know that
"default" is a special token and don't pass it) does not to me.
Patch looks obviously correct to me. Thanks.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] add--interactive: respect diff.algorithm
When staging hunks interactively it is sometimes useful to use an
alternative diff algorithm which splits the changes into hunks in a more
logical manner. This is not possible because the plumbing commands
called by add--interactive ignore the "diff.algorithm" configuration
option (as they should).
Since add--interactive is a porcelain command it should respect this
configuration variable. To do this, make it read diff.algorithm and
pass its value to the underlying diff-index and diff-files invocations.
At this point, do not add options to "git add", "git reset" or "git
checkout" (all of which can call git-add--interactive). If a user want
to override the value on the command line they can use:
git -c diff.algorithm=$ALGO ...
Signed-off-by: John Keeping
---
On Mon, Jun 10, 2013 at 05:56:56PM -0400, Jeff King wrote:
> On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:
>
> > > 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 think you'd need to re-run the diff over the whole file and then skip
hunks until you reach one that overlaps with the original hunk. But I
suspect it would end up being quite a lot more complicated than that.
> > 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";
> }
OK. The "default" is actually "the value that is equivalent to 'myers'
for diff.algorithm" and I was originally going to add --diff-algorithm
to the command line unconditionally.
But I think it's better to add it only when it has been specified so
I've changed it as you suggest.
git-add--interactive.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d2c4ce6..5310959 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');
+
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 (defined $diff_algorithm) {
+ push @diff_cmd, "--diff-algorithm=${diff_algorithm}";
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, $patch_mode_revision;
}
--
1.8.3.779.g691e267
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
