* [RFC PATCH] {checkout,reset} -p: make patch direction configurable
@ 2009-11-19 22:03 Thomas Rast
2009-11-20 10:08 ` Štěpán Němec
2009-11-27 6:41 ` Jeff King
0 siblings, 2 replies; 3+ messages in thread
From: Thomas Rast @ 2009-11-19 22:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
When we implemented the -p mode for checkout, reset and stash, some
discussion revolved around the involved patch direction.
Make this configurable for reset and checkout with the following
choices:
index/HEAD other
forward undo addition undo addition
mixed undo addition apply removal
reverse apply removal apply removal
Where for added lines, 'undo addition' means that you will see a '+'
patch and can reverse-apply it; 'apply removal' means you will see a
'-' patch and can forward-apply it.
The default is 'mixed', to keep the behaviour consistent with that
from before the patch.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This patch is much easier to read with --color-words.
ISTR that Peff wanted this, and maybe some others. I'm not too
interested because I'm still convinced 'mixed' is the Right Option,
but it was somewhere deep on my todo stack and maybe you like it ;-)
We could do the same for stash, but I don't really see the use there;
it does not have any of the direction-switching issues because it does
not support a rev argument.
Documentation/config.txt | 12 +++++
git-add--interactive.perl | 100 +++++++++++++++++++++++++++++++-------------
2 files changed, 82 insertions(+), 30 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index cb73d75..f5f9b80 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1146,6 +1146,18 @@ interactive.singlekey::
linkgit:git-add[1]. Note that this setting is silently
ignored if portable keystroke input is not available.
+interactive.reset.direction::
+interactive.checkout.direction::
+ Direction of diffs used in `git reset -p` and `git checkout
+ -p`. Must be one of 'forward', 'mixed' (default), or
+ 'reverse'.
++
+With 'forward', diffs are taken forward and applied in reverse, i.e.,
+if you added a block of code you see an addition patch and are asked
+if you want to remove it. 'reverse' instead shows a removal patch
+and asks if you to apply it. 'mixed' is the same as 'forward' when
+comparing to HEAD or the index, and 'reverse' otherwise.
+
log.date::
Set default date-time mode for the log command. Setting log.date
value is similar to using 'git-log'\'s --date option. The value is one of the
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8ce1ec9..051e47e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -74,6 +74,25 @@
my $patch_mode;
my $patch_mode_revision;
+sub get_patch_direction_config {
+ my ($key, $default, $forward, $reverse) = @_;
+ my $value = $repo->config($key, $default) || $default;
+ if ($value =~ /^forward$/i) {
+ return (0, 0);
+ } elsif ($value =~ /^mixed$/i) {
+ return (0, 1);
+ } elsif ($value =~ /^reverse?/i) {
+ return (1, 1);
+ } else {
+ die "$key must be one of 'forward', 'mixed' or 'reverse'";
+ }
+}
+
+my ($reset_reverse_head, $reset_reverse_nothead)
+ = get_patch_direction_config("interactive.reset.direction", "mixed");
+my ($checkout_reverse_head, $checkout_reverse_nothead)
+ = get_patch_direction_config("interactive.checkout.direction", "mixed");
+
sub apply_patch;
sub apply_patch_for_checkout_commit;
sub apply_patch_for_stash;
@@ -98,48 +117,69 @@
FILTER => undef,
},
'reset_head' => {
- DIFF => 'diff-index -p --cached',
- APPLY => sub { apply_patch 'apply -R --cached', @_; },
- APPLY_CHECK => 'apply -R --cached',
- VERB => 'Unstage',
- TARGET => '',
- PARTICIPLE => 'unstaging',
+ DIFF => 'diff-index -p --cached '
+ . ($reset_reverse_head ? '-R' : ''),
+ APPLY => sub {
+ apply_patch 'apply --cached '
+ . ($reset_reverse_head ? '' : '-R'), @_;
+ },
+ APPLY_CHECK => 'apply --cached '
+ . ($reset_reverse_head ? '' : '-R'),
+ VERB => $reset_reverse_head ? 'Apply' : 'Unstage',
+ TARGET => $reset_reverse_head ? ' to index' : '',
+ PARTICIPLE => $reset_reverse_head ? 'applying' : 'unstaging',
FILTER => 'index-only',
},
'reset_nothead' => {
- DIFF => 'diff-index -R -p --cached',
- APPLY => sub { apply_patch 'apply --cached', @_; },
- APPLY_CHECK => 'apply --cached',
- VERB => 'Apply',
- TARGET => ' to index',
- PARTICIPLE => 'applying',
+ DIFF => 'diff-index -p --cached '
+ . ($reset_reverse_nothead ? '-R' : ''),
+ APPLY => sub {
+ apply_patch 'apply --cached '
+ . ($reset_reverse_nothead ? '' : '-R'), @_;
+ },
+ APPLY_CHECK => 'apply --cached '
+ . ($reset_reverse_nothead ? '' : '-R'),
+ VERB => $reset_reverse_nothead ? 'Apply' : 'Unstage',
+ TARGET => $reset_reverse_nothead ? ' to index' : '',
+ PARTICIPLE => $reset_reverse_nothead ? 'applying' : 'unstaging',
FILTER => 'index-only',
},
'checkout_index' => {
- DIFF => 'diff-files -p',
- APPLY => sub { apply_patch 'apply -R', @_; },
- APPLY_CHECK => 'apply -R',
- VERB => 'Discard',
- TARGET => ' from worktree',
- PARTICIPLE => 'discarding',
+ DIFF => 'diff-files -p ' . ($checkout_reverse_head ? '-R' : ''),
+ APPLY => sub {
+ apply_patch 'apply '
+ . ($checkout_reverse_head ? '' : '-R'), @_;
+ },
+ APPLY_CHECK => 'apply ' . ($checkout_reverse_head ? '' : '-R'),
+ VERB => $checkout_reverse_head ? 'Apply' : 'Discard',
+ TARGET => $checkout_reverse_head ? ' to worktree' : ' from worktree',
+ PARTICIPLE => $checkout_reverse_head ? 'applying' : 'discarding',
FILTER => 'file-only',
},
'checkout_head' => {
- DIFF => 'diff-index -p',
- APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
- APPLY_CHECK => 'apply -R',
- VERB => 'Discard',
- TARGET => ' from index and worktree',
- PARTICIPLE => 'discarding',
+ DIFF => 'diff-index -p ' . ($checkout_reverse_head ? '-R' : ''),
+ APPLY => sub {
+ apply_patch_for_checkout_commit
+ $checkout_reverse_head ? '' : '-R', @_;
+ },
+ APPLY_CHECK => 'apply ' . ($checkout_reverse_head ? '' : '-R'),
+ VERB => $checkout_reverse_head ? 'Apply' : 'Discard',
+ TARGET => ($checkout_reverse_head ? ' to index and worktree'
+ : ' from index and worktree'),
+ PARTICIPLE => $checkout_reverse_head ? 'applying' : 'discarding',
FILTER => undef,
},
'checkout_nothead' => {
- DIFF => 'diff-index -R -p',
- APPLY => sub { apply_patch_for_checkout_commit '', @_ },
- APPLY_CHECK => 'apply',
- VERB => 'Apply',
- TARGET => ' to index and worktree',
- PARTICIPLE => 'applying',
+ DIFF => 'diff-index -p ' . ($checkout_reverse_nothead ? '-R' : ''),
+ APPLY => sub {
+ apply_patch_for_checkout_commit
+ $checkout_reverse_nothead ? '' : '-R', @_;
+ },
+ APPLY_CHECK => 'apply ' . ($checkout_reverse_nothead ? '' : '-R'),
+ VERB => $checkout_reverse_nothead ? 'Apply' : 'Discard',
+ TARGET => ($checkout_reverse_nothead ? ' to index and worktree'
+ : ' from index and worktree'),
+ PARTICIPLE => $checkout_reverse_nothead ? 'applying' : 'discarding',
FILTER => undef,
},
);
--
1.6.5.3.439.g7a64a6.dirty
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable
2009-11-19 22:03 [RFC PATCH] {checkout,reset} -p: make patch direction configurable Thomas Rast
@ 2009-11-20 10:08 ` Štěpán Němec
2009-11-27 6:41 ` Jeff King
1 sibling, 0 replies; 3+ messages in thread
From: Štěpán Němec @ 2009-11-20 10:08 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King
Hello,
I bet it would get caught in the review process anyway, but just in
case -- a small typo:
> +With 'forward', diffs are taken forward and applied in reverse, i.e.,
> +if you added a block of code you see an addition patch and are asked
> +if you want to remove it. 'reverse' instead shows a removal patch
> +and asks if you to apply it. 'mixed' is the same as 'forward' when
Should be 'asks if you /want/ to apply it.'
Štěpán
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable
2009-11-19 22:03 [RFC PATCH] {checkout,reset} -p: make patch direction configurable Thomas Rast
2009-11-20 10:08 ` Štěpán Němec
@ 2009-11-27 6:41 ` Jeff King
1 sibling, 0 replies; 3+ messages in thread
From: Jeff King @ 2009-11-27 6:41 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
On Thu, Nov 19, 2009 at 11:03:57PM +0100, Thomas Rast wrote:
> When we implemented the -p mode for checkout, reset and stash, some
> discussion revolved around the involved patch direction.
>
> Make this configurable for reset and checkout with the following
> choices:
>
> index/HEAD other
> forward undo addition undo addition
> mixed undo addition apply removal
> reverse apply removal apply removal
> [...]
> ISTR that Peff wanted this, and maybe some others. I'm not too
> interested because I'm still convinced 'mixed' is the Right Option,
> but it was somewhere deep on my todo stack and maybe you like it ;-)
Actually, I am pretty happy with the current "discard this hunk" most of
the time. It is easy enough to see "you made this change, did you want
to get rid of it?". The one exception is during patch editing. Try
something simple like:
cat >file <<EOF
this
is
a
file
with
some
content
in
it
EOF
git add file
git commit -m base
cat >file <<EOF
this
is
a
file
with
some
other
content
EOF
git checkout -p
Now try to 'e'dit the patch to throw away the addition of "other", but
keep the deletion of the other two lines. Do you find it easy or hard?
Now try it with interactive.checkout.direction set to forward. I find
editing the forward direction _much_ simpler.
Assuming you agree, I'm not sure what that tells us, though. I probably
wouldn't personally set interactive.*.direction for the yes/no part. But
I would find it more convenient to do patch editing always in the
forward direction. I'm worried that it would be too jarring to the user,
though, to see the patch presented in one direction but edit it in the
opposite direction. Maybe it would be nice to have yet another
interactive option to swap between the two for this particular hunk, and
then you could edit the direction you prefer.
Junio raised the question of consistency in another thread. I don't see
a consistency problem here. This is by definition an interactive
procedure. I don't see a reason not to allow the user their preferred
style. But I think if I could swap when editing, I would personally not
care all that much about the other direction.
-Peff
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-11-27 6:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-19 22:03 [RFC PATCH] {checkout,reset} -p: make patch direction configurable Thomas Rast
2009-11-20 10:08 ` Štěpán Němec
2009-11-27 6:41 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.