From: Junio C Hamano <hidden> Date: 2016-06-15 22:57:40
John Keeping [off-list ref] writes:
I think the first thing to do is read the "diff.algorithm" setting in
git-add--interactive and pass its value to the underlying diff-index and
diff-files commands, but should we also have a command line parameter to
git-add to specify the diff algorithm in interactive mode? And if so,
can we simply add "--diff-algorithm" to git-add, or is that too
confusing?
Making "git add--interactive" read from diff.algorithm is probably a
good idea, because the command itself definitely is a Porcelain. We
would probably need a way to defeat the configured default for
completeness, either:
git add -p --diff-algorithm=default
git -c diff.algorithm=default add -p
but I suspect that a new option to "git add" that only takes effect
together with "-p" is probably an overkill, only in order to support
the former and not having to say the latter, but I can be persuaded
either way.
As long as "git add --diff-algorithm=foo" without "-i" or "-p"
option lets the user know it has no effect (error out, give warning
and continue, etc. whose details I do not deeply care), that is.
From: Jeff King <hidden> Date: 2016-06-15 22:57:40
On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:
John Keeping [off-list ref] writes:
quoted
I think the first thing to do is read the "diff.algorithm" setting in
git-add--interactive and pass its value to the underlying diff-index and
diff-files commands, but should we also have a command line parameter to
git-add to specify the diff algorithm in interactive mode? And if so,
can we simply add "--diff-algorithm" to git-add, or is that too
confusing?
Making "git add--interactive" read from diff.algorithm is probably a
good idea, because the command itself definitely is a Porcelain. We
would probably need a way to defeat the configured default for
completeness, either:
git add -p --diff-algorithm=default
git -c diff.algorithm=default add -p
but I suspect that a new option to "git add" that only takes effect
together with "-p" is probably an overkill, only in order to support
the former and not having to say the latter, but I can be persuaded
either way.
Worse than that, you would need to add such an option to "checkout -p",
"reset -p", "stash -p", etc. I think the latter form you suggest is
probably acceptable in this case.
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.
But I cannot off the top of my head think of other options besides -U
that would be helpful. I have never particularly wanted it for "add -p",
either, though I sometimes generate patches to the list with a greater
number of context lines when I think it makes the changes to a short
function more readable.
-Peff
From: John Keeping <hidden> Date: 2016-06-15 22:57:40
On Mon, Jun 10, 2013 at 05:11:41PM -0400, Jeff King wrote:
On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:
quoted
John Keeping [off-list ref] writes:
quoted
I think the first thing to do is read the "diff.algorithm" setting in
git-add--interactive and pass its value to the underlying diff-index and
diff-files commands, but should we also have a command line parameter to
git-add to specify the diff algorithm in interactive mode? And if so,
can we simply add "--diff-algorithm" to git-add, or is that too
confusing?
Making "git add--interactive" read from diff.algorithm is probably a
good idea, because the command itself definitely is a Porcelain. We
would probably need a way to defeat the configured default for
completeness, either:
git add -p --diff-algorithm=default
git -c diff.algorithm=default add -p
but I suspect that a new option to "git add" that only takes effect
together with "-p" is probably an overkill, only in order to support
the former and not having to say the latter, but I can be persuaded
either way.
Worse than that, you would need to add such an option to "checkout -p",
"reset -p", "stash -p", etc. I think the latter form you suggest is
probably acceptable in this case.
That's what I'm planning to do at the moment, if anyone wants to extend
it further in the future then that can be built on top.
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.
I don't intend to investigate that though - respecting diff.algorithm is
good enough for my usage.
But I cannot off the top of my head think of other options besides -U
that would be helpful. I have never particularly wanted it for "add -p",
either, though I sometimes generate patches to the list with a greater
number of context lines when I think it makes the changes to a short
function more readable.
--function-context might also be useful, but that's in the same category
as -U.
The patch I'm using is below. I'm not sure how we can test this though;
it seems fragile to invent a patch that appears different with different
diff algorithms. Any suggestions?
-- >8 --
git-add--interactive.perl | 5 +++++
1 file changed, 5 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 22:57:41
On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:
quoted
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.
@@ -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 {subparse_diff{my($path)=@_;my@diff_cmd=split(" ",$patch_mode_flavour{DIFF});+if($diff_algorithmne"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
From: John Keeping <hidden> Date: 2016-06-15 22:57:43
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 <redacted>
---
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:
quoted
quoted
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.
@@ -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 {subparse_diff{my($path)=@_;my@diff_cmd=split(" ",$patch_mode_flavour{DIFF});+if($diff_algorithmne"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(+)
From: Jeff King <hidden> Date: 2016-06-15 22:57:43
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/
quoted
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