On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:
> An update. I tried your reproduction recipe with 1.6.5.2 and it doesn't
> reproduce, but with 1.6.5.3 it does.
Thanks, both, for a very helpful bug report. 24ab81a was totally bogus,
but we lacked a test for deleting a non-empty file. That test and a fix
for the problem are in the patch below.
I am still slightly concerned that James's
git diff | sed '/^deleted file/d' | git apply --cached
behaves as it does. What should git-apply do with a patch like:
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
@@ -1 +0,0 @@
-foo
? I can see either turning it into a deletion patch (because /dev/null
is special) or barfing (because /dev/null as a special case should have
appeared in the "diff" line). But creating a dev/null file seems very
wrong.
But maybe it is not worth worrying about too much. That patch format is
not generated intentionally by any known software.
Here is the fix directly on top of 24ab81a.
-- >8 --
Subject: [PATCH] add-interactive: fix deletion of non-empty files
Commit 24ab81a fixed the deletion of empty files, but broke
deletion of non-empty files. The approach it took was to
factor out the "deleted" line from the patch header into its
own hunk, the same way we do for mode changes. However,
unlike mode changes, we only showed the special "delete this
file" hunk if there were no other hunks. Otherwise, the user
would annoyingly be presented with _two_ hunks: one for
deleting the file and one for deleting the content.
Instead, this patch takes a separate approach. We leave the
deletion line in the header, so it will be used as usual by
non-empty files if their deletion hunk is staged. For empty
files, we create a deletion hunk with no content; it doesn't
add anything to the patch, but by staging it we trigger the
application of the header, which does contain the deletion.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
There is a slightly different approach we could take, too: keep the
"deletion" hunk as a first-class hunk, and just meld the content hunk's
output into it. Then both cases would get the "Stage deletion" question
instead of the "Stage this hunk" you get now for non-empty files (which
just happens to trigger a deletion due to the headers).
That would take some refactoring, though, as pulling the deletion hunk
out means we are re-ordering the headers. So right now if you did that
your ($head, @hunk) output would be something like:
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
deleted file mode 100644
@@ -1 +0,0 @@
-foo
which is pretty weird. On the other hand, we already do that funny
ordering for mode hunks, and git-apply is just fine with it. A mode hunk
with content change looks like this:
diff --git a/foo b/foo
index 257cc56..19c6cc1
--- a/foo
+++ b/foo
old mode 100644
new mode 100755
And it also opens the door to editing the hunk to stop the deletion, but
still tweak the content change. Right now if you edit a deletion patch,
you can't remove the 'deleted' bit, and if your edit result keeps any
content in the file, apply will complain. I'm not sure that particular
feature would be useful though (I have certainly never wanted it).
git-add--interactive.perl | 18 +++++++++++-------
t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..f4b95b1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -731,17 +731,19 @@ sub parse_diff_header {
my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
- my $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };
+ my $is_deletion;
for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
my $dest =
$src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ? $mode :
- $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :
$head;
push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
+ if ($src->{TEXT}->[$i] =~ /^deleted file/) {
+ $is_deletion = 1;
+ }
}
- return ($head, $mode, $deletion);
+ return ($head, $mode, $is_deletion);
}
sub hunk_splittable {
@@ -1209,7 +1211,7 @@ sub patch_update_file {
my ($ix, $num);
my $path = shift;
my ($head, @hunk) = parse_diff($path);
- ($head, my $mode, my $deletion) = parse_diff_header($head);
+ ($head, my $mode, my $is_deletion) = parse_diff_header($head);
for (@{$head->{DISPLAY}}) {
print;
}
@@ -1217,8 +1219,8 @@ sub patch_update_file {
if (@{$mode->{TEXT}}) {
unshift @hunk, $mode;
}
- if (@{$deletion->{TEXT}} && !@hunk) {
- @hunk = ($deletion);
+ if ($is_deletion && !@hunk) {
+ @hunk = ({TEXT => [], DISPLAY => [], TYPE => 'deletion'});
}
$num = scalar @hunk;
@@ -1441,14 +1443,16 @@ sub patch_update_file {
@hunk = coalesce_overlapping_hunks(@hunk);
my $n_lofs = 0;
+ my $hunks_used = 0;
my @result = ();
for (@hunk) {
if ($_->{USE}) {
push @result, @{$_->{TEXT}};
+ $hunks_used++;
}
}
- if (@result) {
+ if ($hunks_used) {
my $fh;
my @patch = (@{$head->{TEXT}}, @result);
my $apply_routine = $patch_mode_flavour{APPLY};
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index aa5909b..0926b91 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '
'
cat >expected <<EOF
+diff --git a/non-empty b/non-empty
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/non-empty
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+test_expect_success 'deleting a non-empty file' '
+ git reset --hard &&
+ echo content >non-empty &&
+ git add non-empty &&
+ git commit -m non-empty &&
+ rm non-empty &&
+ echo y | git add -p non-empty &&
+ git diff --cached >diff &&
+ test_cmp expected diff
+'
+
+cat >expected <<EOF
diff --git a/empty b/empty
deleted file mode 100644
index e69de29..0000000
--
1.6.5.1.g24ab.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html