add -p: fix editing of intent-to-add paths
A popular way of partially staging a new file is to run `git add -N <path>` and then use the hunk editing of `git add -p` to select the part of the file that the user wishes to stage. Since 85953a3187 ("diff-files --raw: show correct post-image of intent-to-add files", 2020-07-01) this has stopped working as intent-to-add paths are now show as new files rather than changes to an empty blob and `git apply` refused to apply a creation patch for a path that was marked as intent-to-add. 7cfde3fa0f ("apply: allow "new file" patches on i-t-a entries", 2020-08-06) fixed the problem with apply but it still wasn't possible to edit the added hunk properly. 2c8bd8471a ("checkout -p: handle new files correctly", 2020-05-27) had previously changed `add -p` to handle new files but it did not implement patch editing correctly. The perl version simply forbade editing and the C version opened the editor with the full diff rather that just the hunk which meant that the user had to edit the hunk header manually to get it to work. The root cause of the problem is that added files store the diff header with the hunk data rather than separating the two as we do for other changes. Changing added files to store the diff header separately fixes the editing problem at the expense of having to special case empty additions as they no longer have any hunks associated with them, only the diff header. The changes move some existing code into a conditional changing the indentation, they are best viewed with --color-moved-ws=allow-indentation-change (or --ignore-space-change works well to get an overview of the changes) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Reported-by: Thomas Sullivan <tom@msbit.com.au> Reported-by: Yuchen Ying <ych@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood committed
Sep 9, 2020 at 13:58 UTC
75a009dc29d943554b39233286f02d34ead9a666
3 files changed
+173
-94
add-patch.c
+79
-61
@@ -451,7 +451,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
451
pend = p + plain->len;
452
while (p != pend) {
453
char *eol = memchr(p, '\n', pend - p);
454
- const char *deleted = NULL, *added = NULL, *mode_change = NULL;
454
+ const char *deleted = NULL, *mode_change = NULL;
455
456
if (!eol)
457
eol = pend;
@@ -468,12 +468,11 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
468
} else if (p == plain->buf)
469
BUG("diff starts with unexpected line:\n"
470
"%.*s\n", (int)(eol - p), p);
471
- else if (file_diff->deleted || file_diff->added)
471
+ else if (file_diff->deleted)
472
; /* keep the rest of the file in a single "hunk" */
473
else if (starts_with(p, "@@ ") ||
474
(hunk == &file_diff->head &&
475
- (skip_prefix(p, "deleted file", &deleted) ||
476
- skip_prefix(p, "new file", &added)))) {
475
+ (skip_prefix(p, "deleted file", &deleted)))) {
476
if (marker == '-' || marker == '+')
477
/*
478
* Should not happen; previous hunk did not end
@@ -491,8 +490,6 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
490
491
if (deleted)
492
file_diff->deleted = 1;
494
- else if (added)
495
- file_diff->added = 1;
493
else if (parse_hunk_header(s, hunk) < 0)
494
return -1;
495
@@ -501,6 +498,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
498
* split
499
*/
500
marker = *p;
501
+ } else if (hunk == &file_diff->head &&
502
+ starts_with(p, "new file")) {
503
+ file_diff->added = 1;
504
} else if (hunk == &file_diff->head &&
505
skip_prefix(p, "old mode ", &mode_change) &&
506
is_octal(mode_change, eol - mode_change)) {
@@ -1362,7 +1362,8 @@ static int patch_update_file(struct add_p_state *s,
1362
ALLOW_EDIT = 1 << 6
1363
} permitted = 0;
1364
1365
- if (!file_diff->hunk_nr)
1365
+ /* Empty added files have no hunks */
1366
+ if (!file_diff->hunk_nr && !file_diff->added)
1367
return 0;
1368
1369
strbuf_reset(&s->buf);
@@ -1371,21 +1372,25 @@ static int patch_update_file(struct add_p_state *s,
1372
for (;;) {
1373
if (hunk_index >= file_diff->hunk_nr)
1374
hunk_index = 0;
1374
- hunk = file_diff->hunk + hunk_index;
1375
-
1375
+ hunk = file_diff->hunk_nr
1376
+ ? file_diff->hunk + hunk_index
1377
+ : &file_diff->head;
1378
undecided_previous = -1;
1377
- for (i = hunk_index - 1; i >= 0; i--)
1378
- if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
1379
- undecided_previous = i;
1380
- break;
1381
- }
1382
-
1379
undecided_next = -1;
1384
- for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
1385
- if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
1386
- undecided_next = i;
1387
- break;
1388
- }
1380
+
1381
+ if (file_diff->hunk_nr) {
1382
+ for (i = hunk_index - 1; i >= 0; i--)
1383
+ if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
1384
+ undecided_previous = i;
1385
+ break;
1386
+ }
1387
+
1388
+ for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
1389
+ if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
1390
+ undecided_next = i;
1391
+ break;
1392
+ }
1393
+ }
1394
1395
/* Everything decided? */
1396
if (undecided_previous < 0 && undecided_next < 0 &&
@@ -1393,38 +1398,40 @@ static int patch_update_file(struct add_p_state *s,
1398
break;
1399
1400
strbuf_reset(&s->buf);
1396
- render_hunk(s, hunk, 0, colored, &s->buf);
1397
- fputs(s->buf.buf, stdout);
1401
+ if (file_diff->hunk_nr) {
1402
+ render_hunk(s, hunk, 0, colored, &s->buf);
1403
+ fputs(s->buf.buf, stdout);
1404
1399
- strbuf_reset(&s->buf);
1400
- if (undecided_previous >= 0) {
1401
- permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
1402
- strbuf_addstr(&s->buf, ",k");
1403
- }
1404
- if (hunk_index) {
1405
- permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
1406
- strbuf_addstr(&s->buf, ",K");
1407
- }
1408
- if (undecided_next >= 0) {
1409
- permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
1410
- strbuf_addstr(&s->buf, ",j");
1411
- }
1412
- if (hunk_index + 1 < file_diff->hunk_nr) {
1413
- permitted |= ALLOW_GOTO_NEXT_HUNK;
1414
- strbuf_addstr(&s->buf, ",J");
1415
- }
1416
- if (file_diff->hunk_nr > 1) {
1417
- permitted |= ALLOW_SEARCH_AND_GOTO;
1418
- strbuf_addstr(&s->buf, ",g,/");
1419
- }
1420
- if (hunk->splittable_into > 1) {
1421
- permitted |= ALLOW_SPLIT;
1422
- strbuf_addstr(&s->buf, ",s");
1423
- }
1424
- if (hunk_index + 1 > file_diff->mode_change &&
1425
- !file_diff->deleted) {
1426
- permitted |= ALLOW_EDIT;
1427
- strbuf_addstr(&s->buf, ",e");
1405
+ strbuf_reset(&s->buf);
1406
+ if (undecided_previous >= 0) {
1407
+ permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
1408
+ strbuf_addstr(&s->buf, ",k");
1409
+ }
1410
+ if (hunk_index) {
1411
+ permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
1412
+ strbuf_addstr(&s->buf, ",K");
1413
+ }
1414
+ if (undecided_next >= 0) {
1415
+ permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
1416
+ strbuf_addstr(&s->buf, ",j");
1417
+ }
1418
+ if (hunk_index + 1 < file_diff->hunk_nr) {
1419
+ permitted |= ALLOW_GOTO_NEXT_HUNK;
1420
+ strbuf_addstr(&s->buf, ",J");
1421
+ }
1422
+ if (file_diff->hunk_nr > 1) {
1423
+ permitted |= ALLOW_SEARCH_AND_GOTO;
1424
+ strbuf_addstr(&s->buf, ",g,/");
1425
+ }
1426
+ if (hunk->splittable_into > 1) {
1427
+ permitted |= ALLOW_SPLIT;
1428
+ strbuf_addstr(&s->buf, ",s");
1429
+ }
1430
+ if (hunk_index + 1 > file_diff->mode_change &&
1431
+ !file_diff->deleted) {
1432
+ permitted |= ALLOW_EDIT;
1433
+ strbuf_addstr(&s->buf, ",e");
1434
+ }
1435
}
1436
if (file_diff->deleted)
1437
prompt_mode_type = PROMPT_DELETION;
@@ -1438,7 +1445,9 @@ static int patch_update_file(struct add_p_state *s,
1445
color_fprintf(stdout, s->s.prompt_color,
1446
"(%"PRIuMAX"/%"PRIuMAX") ",
1447
(uintmax_t)hunk_index + 1,
1441
- (uintmax_t)file_diff->hunk_nr);
1448
+ (uintmax_t)(file_diff->hunk_nr
1449
+ ? file_diff->hunk_nr
1450
+ : 1));
1451
color_fprintf(stdout, s->s.prompt_color,
1452
_(s->mode->prompt_mode[prompt_mode_type]),
1453
s->buf.buf);
@@ -1458,16 +1467,24 @@ soft_increment:
1467
hunk->use = SKIP_HUNK;
1468
goto soft_increment;
1469
} else if (ch == 'a') {
1461
- for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
1462
- hunk = file_diff->hunk + hunk_index;
1463
- if (hunk->use == UNDECIDED_HUNK)
1464
- hunk->use = USE_HUNK;
1470
+ if (file_diff->hunk_nr) {
1471
+ for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
1472
+ hunk = file_diff->hunk + hunk_index;
1473
+ if (hunk->use == UNDECIDED_HUNK)
1474
+ hunk->use = USE_HUNK;
1475
+ }
1476
+ } else if (hunk->use == UNDECIDED_HUNK) {
1477
+ hunk->use = USE_HUNK;
1478
}
1479
} else if (ch == 'd' || ch == 'q') {
1467
- for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
1468
- hunk = file_diff->hunk + hunk_index;
1469
- if (hunk->use == UNDECIDED_HUNK)
1470
- hunk->use = SKIP_HUNK;
1480
+ if (file_diff->hunk_nr) {
1481
+ for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
1482
+ hunk = file_diff->hunk + hunk_index;
1483
+ if (hunk->use == UNDECIDED_HUNK)
1484
+ hunk->use = SKIP_HUNK;
1485
+ }
1486
+ } else if (hunk->use == UNDECIDED_HUNK) {
1487
+ hunk->use = SKIP_HUNK;
1488
}
1489
if (ch == 'q') {
1490
quit = 1;
@@ -1625,7 +1642,8 @@ soft_increment:
1642
if (file_diff->hunk[i].use == USE_HUNK)
1643
break;
1644
1628
- if (i < file_diff->hunk_nr) {
1645
+ if (i < file_diff->hunk_nr ||
1646
+ (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
1647
/* At least one hunk selected: apply */
1648
strbuf_reset(&s->buf);
1649
reassemble_patch(s, file_diff, 0, &s->buf);
git-add--interactive.perl
+56
-33
@@ -754,13 +754,16 @@ sub parse_diff_header {
754
my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
755
my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
756
my $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };
757
- my $addition = { TEXT => [], DISPLAY => [], TYPE => 'addition' };
757
+ my $addition;
758
759
for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
760
+ if ($src->{TEXT}->[$i] =~ /^new file/) {
761
+ $addition = 1;
762
+ $head->{TYPE} = 'addition';
763
+ }
764
my $dest =
765
$src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ? $mode :
766
$src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :
763
- $src->{TEXT}->[$i] =~ /^new file/ ? $addition :
767
$head;
768
push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
769
push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
@@ -1501,12 +1504,6 @@ sub patch_update_file {
1504
push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
1505
}
1506
@hunk = ($deletion);
1504
- } elsif (@{$addition->{TEXT}}) {
1505
- foreach my $hunk (@hunk) {
1506
- push @{$addition->{TEXT}}, @{$hunk->{TEXT}};
1507
- push @{$addition->{DISPLAY}}, @{$hunk->{DISPLAY}};
1508
- }
1509
- @hunk = ($addition);
1507
}
1508
1509
$num = scalar @hunk;
@@ -1516,6 +1513,7 @@ sub patch_update_file {
1513
my ($prev, $next, $other, $undecided, $i);
1514
$other = '';
1515
1516
+ last if ($ix and !$num);
1517
if ($num <= $ix) {
1518
$ix = 0;
1519
}
@@ -1548,35 +1546,51 @@ sub patch_update_file {
1546
last;
1547
}
1548
}
1551
- last if (!$undecided);
1549
+ last if (!$undecided && ($num || !$addition));
1550
1553
- if ($hunk[$ix]{TYPE} eq 'hunk' &&
1554
- hunk_splittable($hunk[$ix]{TEXT})) {
1555
- $other .= ',s';
1556
- }
1557
- if ($hunk[$ix]{TYPE} eq 'hunk') {
1558
- $other .= ',e';
1559
- }
1560
- for (@{$hunk[$ix]{DISPLAY}}) {
1561
- print;
1551
+ if ($num) {
1552
+ if ($hunk[$ix]{TYPE} eq 'hunk' &&
1553
+ hunk_splittable($hunk[$ix]{TEXT})) {
1554
+ $other .= ',s';
1555
+ }
1556
+ if ($hunk[$ix]{TYPE} eq 'hunk') {
1557
+ $other .= ',e';
1558
+ }
1559
+ for (@{$hunk[$ix]{DISPLAY}}) {
1560
+ print;
1561
+ }
1562
}
1563
- print colored $prompt_color, "(", ($ix+1), "/$num) ",
1564
- sprintf(__($patch_update_prompt_modes{$patch_mode}{$hunk[$ix]{TYPE}}), $other);
1563
+ my $type = $num ? $hunk[$ix]{TYPE} : $head->{TYPE};
1564
+ print colored $prompt_color, "(", ($ix+1), "/", ($num ? $num : 1), ") ",
1565
+ sprintf(__($patch_update_prompt_modes{$patch_mode}{$type}), $other);
1566
1567
my $line = prompt_single_character;
1568
last unless defined $line;
1569
if ($line) {
1570
if ($line =~ /^y/i) {
1570
- $hunk[$ix]{USE} = 1;
1571
+ if ($num) {
1572
+ $hunk[$ix]{USE} = 1;
1573
+ } else {
1574
+ $head->{USE} = 1;
1575
+ }
1576
}
1577
elsif ($line =~ /^n/i) {
1573
- $hunk[$ix]{USE} = 0;
1578
+ if ($num) {
1579
+ $hunk[$ix]{USE} = 0;
1580
+ } else {
1581
+ $head->{USE} = 0;
1582
+ }
1583
}
1584
elsif ($line =~ /^a/i) {
1576
- while ($ix < $num) {
1577
- if (!defined $hunk[$ix]{USE}) {
1578
- $hunk[$ix]{USE} = 1;
1585
+ if ($num) {
1586
+ while ($ix < $num) {
1587
+ if (!defined $hunk[$ix]{USE}) {
1588
+ $hunk[$ix]{USE} = 1;
1589
+ }
1590
+ $ix++;
1591
}
1592
+ } else {
1593
+ $head->{USE} = 1;
1594
$ix++;
1595
}
1596
next;
@@ -1613,19 +1627,28 @@ sub patch_update_file {
1627
next;
1628
}
1629
elsif ($line =~ /^d/i) {
1616
- while ($ix < $num) {
1617
- if (!defined $hunk[$ix]{USE}) {
1618
- $hunk[$ix]{USE} = 0;
1630
+ if ($num) {
1631
+ while ($ix < $num) {
1632
+ if (!defined $hunk[$ix]{USE}) {
1633
+ $hunk[$ix]{USE} = 0;
1634
+ }
1635
+ $ix++;
1636
}
1637
+ } else {
1638
+ $head->{USE} = 0;
1639
$ix++;
1640
}
1641
next;
1642
}
1643
elsif ($line =~ /^q/i) {
1625
- for ($i = 0; $i < $num; $i++) {
1626
- if (!defined $hunk[$i]{USE}) {
1627
- $hunk[$i]{USE} = 0;
1644
+ if ($num) {
1645
+ for ($i = 0; $i < $num; $i++) {
1646
+ if (!defined $hunk[$i]{USE}) {
1647
+ $hunk[$i]{USE} = 0;
1648
+ }
1649
}
1650
+ } elsif (!defined $head->{USE}) {
1651
+ $head->{USE} = 0;
1652
}
1653
$quit = 1;
1654
last;
@@ -1743,7 +1766,7 @@ sub patch_update_file {
1766
}
1767
}
1768
1746
- @hunk = coalesce_overlapping_hunks(@hunk);
1769
+ @hunk = coalesce_overlapping_hunks(@hunk) if ($num);
1770
1771
my $n_lofs = 0;
1772
my @result = ();
@@ -1753,7 +1776,7 @@ sub patch_update_file {
1776
}
1777
}
1778
1756
- if (@result) {
1779
+ if (@result or $head->{USE}) {
1780
my @patch = reassemble_patch($head->{TEXT}, @result);
1781
my $apply_routine = $patch_mode_flavour{APPLY};
1782
&$apply_routine(@patch);
t/t3701-add-interactive.sh
+38
@@ -814,6 +814,44 @@ test_expect_success 'checkout -p works with pathological context lines' '
814
test_cmp expect a
815
'
816
817
+# This should be called from a subshell as it sets a temporary editor
818
+setup_new_file() {
819
+ write_script new-file-editor.sh <<-\EOF &&
820
+ sed /^#/d "$1" >patch &&
821
+ sed /^+c/d patch >"$1"
822
+ EOF
823
+ test_set_editor "$(pwd)/new-file-editor.sh" &&
824
+ test_write_lines a b c d e f >new-file &&
825
+ test_write_lines a b d e f >new-file-expect &&
826
+ test_write_lines "@@ -0,0 +1,6 @@" +a +b +c +d +e +f >patch-expect
827
+}
828
+
829
+test_expect_success 'add -N followed by add -p patch editing' '
830
+ git reset --hard &&
831
+ (
832
+ setup_new_file &&
833
+ git add -N new-file &&
834
+ test_write_lines e n q | git add -p &&
835
+ git cat-file blob :new-file >actual &&
836
+ test_cmp new-file-expect actual &&
837
+ test_cmp patch-expect patch
838
+ )
839
+'
840
+
841
+test_expect_success 'checkout -p patch editing of added file' '
842
+ git reset --hard &&
843
+ (
844
+ setup_new_file &&
845
+ git add new-file &&
846
+ git commit -m "add new file" &&
847
+ git rm new-file &&
848
+ git commit -m "remove new file" &&
849
+ test_write_lines e n q | git checkout -p HEAD^ &&
850
+ test_cmp new-file-expect new-file &&
851
+ test_cmp patch-expect patch
852
+ )
853
+'
854
+
855
test_expect_success 'show help from add--helper' '
856
git reset --hard &&
857
cat >expect <<-EOF &&