patch-id: fix stable patch id for binary / header-only
Patch-ids for binary patches are found by hashing the object ids of the before and after objects in succession. However in the --stable case, there is a bug where hunks are not flushed for binary and header-only patch ids, which would always result in a patch-id of 0000. The --unstable case is currently correct. Reorder the logic to branch into 3 cases for populating the patch body: header-only which populates nothing, binary which populates the object ids, and normal which populates the text diff. All branches will end up flushing the hunk. Don't populate the ---a/ and +++b/ lines for binary diffs, to correspond to those lines not being present in the "git diff" text output. This is necessary because we advertise that the patch-id calculated internally and used in format-patch is the same that what the builtin "git patch-id" would produce when piped from a diff. Update the test to run on both binary and normal files. Signed-off-by: Jerry Zhang <jerry@skydio.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jerry Zhang committed
Oct 24, 2022 at 20:07 UTC
0570be79ead35e47e29ee2587e2c8ea47c091d49
2 files changed
+53
-39
diff.c
+29
-29
@@ -6221,46 +6221,46 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
6221
if (p->one->mode == 0) {
6222
patch_id_add_string(&ctx, "newfilemode");
6223
patch_id_add_mode(&ctx, p->two->mode);
6224
- patch_id_add_string(&ctx, "---/dev/null");
6225
- patch_id_add_string(&ctx, "+++b/");
6226
- the_hash_algo->update_fn(&ctx, p->two->path, len2);
6224
} else if (p->two->mode == 0) {
6225
patch_id_add_string(&ctx, "deletedfilemode");
6226
patch_id_add_mode(&ctx, p->one->mode);
6230
- patch_id_add_string(&ctx, "---a/");
6231
- the_hash_algo->update_fn(&ctx, p->one->path, len1);
6232
- patch_id_add_string(&ctx, "+++/dev/null");
6233
- } else {
6234
- patch_id_add_string(&ctx, "---a/");
6235
- the_hash_algo->update_fn(&ctx, p->one->path, len1);
6236
- patch_id_add_string(&ctx, "+++b/");
6237
- the_hash_algo->update_fn(&ctx, p->two->path, len2);
6227
}
6228
6240
- if (diff_header_only)
6241
- continue;
6242
-
6243
- if (fill_mmfile(options->repo, &mf1, p->one) < 0 ||
6244
- fill_mmfile(options->repo, &mf2, p->two) < 0)
6245
- return error("unable to read files to diff");
6246
-
6247
- if (diff_filespec_is_binary(options->repo, p->one) ||
6229
+ if (diff_header_only) {
6230
+ /* don't do anything since we're only populating header info */
6231
+ } else if (diff_filespec_is_binary(options->repo, p->one) ||
6232
diff_filespec_is_binary(options->repo, p->two)) {
6233
the_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),
6234
the_hash_algo->hexsz);
6235
the_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),
6236
the_hash_algo->hexsz);
6253
- continue;
6254
- }
6255
-
6256
- xpp.flags = 0;
6257
- xecfg.ctxlen = 3;
6258
- xecfg.flags = XDL_EMIT_NO_HUNK_HDR;
6259
- if (xdi_diff_outf(&mf1, &mf2, NULL,
6260
- patch_id_consume, &data, &xpp, &xecfg))
6261
- return error("unable to generate patch-id diff for %s",
6262
- p->one->path);
6237
+ } else {
6238
+ if (p->one->mode == 0) {
6239
+ patch_id_add_string(&ctx, "---/dev/null");
6240
+ patch_id_add_string(&ctx, "+++b/");
6241
+ the_hash_algo->update_fn(&ctx, p->two->path, len2);
6242
+ } else if (p->two->mode == 0) {
6243
+ patch_id_add_string(&ctx, "---a/");
6244
+ the_hash_algo->update_fn(&ctx, p->one->path, len1);
6245
+ patch_id_add_string(&ctx, "+++/dev/null");
6246
+ } else {
6247
+ patch_id_add_string(&ctx, "---a/");
6248
+ the_hash_algo->update_fn(&ctx, p->one->path, len1);
6249
+ patch_id_add_string(&ctx, "+++b/");
6250
+ the_hash_algo->update_fn(&ctx, p->two->path, len2);
6251
+ }
6252
6253
+ if (fill_mmfile(options->repo, &mf1, p->one) < 0 ||
6254
+ fill_mmfile(options->repo, &mf2, p->two) < 0)
6255
+ return error("unable to read files to diff");
6256
+ xpp.flags = 0;
6257
+ xecfg.ctxlen = 3;
6258
+ xecfg.flags = XDL_EMIT_NO_HUNK_HDR;
6259
+ if (xdi_diff_outf(&mf1, &mf2, NULL,
6260
+ patch_id_consume, &data, &xpp, &xecfg))
6261
+ return error("unable to generate patch-id diff for %s",
6262
+ p->one->path);
6263
+ }
6264
if (stable)
6265
flush_one_hunk(oid, &ctx);
6266
}
t/t3419-rebase-patch-id.sh
+24
-10
@@ -43,15 +43,16 @@ test_expect_success 'setup: 500 lines' '
43
git add newfile &&
44
git commit -q -m "add small file" &&
45
46
- git cherry-pick main >/dev/null 2>&1
47
-'
46
+ git cherry-pick main >/dev/null 2>&1 &&
47
49
-test_expect_success 'setup attributes' '
50
- echo "file binary" >.gitattributes
48
+ git branch -f squashed main &&
49
+ git checkout -q -f squashed &&
50
+ git reset -q --soft HEAD~2 &&
51
+ git commit -q -m squashed
52
'
53
54
test_expect_success 'detect upstream patch' '
54
- git checkout -q main &&
55
+ git checkout -q main^{} &&
56
scramble file &&
57
git add file &&
58
git commit -q -m "change big file again" &&
@@ -61,14 +62,27 @@ test_expect_success 'detect upstream patch' '
62
test_must_be_empty revs
63
'
64
65
+test_expect_success 'detect upstream patch binary' '
66
+ echo "file binary" >.gitattributes &&
67
+ git checkout -q other^{} &&
68
+ git rebase main &&
69
+ git rev-list main...HEAD~ >revs &&
70
+ test_must_be_empty revs &&
71
+ test_when_finished "rm .gitattributes"
72
+'
73
+
74
test_expect_success 'do not drop patch' '
65
- git branch -f squashed main &&
66
- git checkout -q -f squashed &&
67
- git reset -q --soft HEAD~2 &&
68
- git commit -q -m squashed &&
75
git checkout -q other^{} &&
76
test_must_fail git rebase squashed &&
71
- git rebase --quit
77
+ test_when_finished "git rebase --abort"
78
+'
79
+
80
+test_expect_success 'do not drop patch binary' '
81
+ echo "file binary" >.gitattributes &&
82
+ git checkout -q other^{} &&
83
+ test_must_fail git rebase squashed &&
84
+ test_when_finished "git rebase --abort" &&
85
+ test_when_finished "rm .gitattributes"
86
'
87
88
test_done