git-apply: handle a patch that touches the same path more than once better
When working with a lot of people who backport patches all day long, every once in a while I get a patch that modifies the same file more than once inside the same patch. git-apply either fails if the second change relies on the first change or silently drops the first change if the second change is independent. The silent part is the scary scenario for us. Also this behaviour is different from the patch-utils. I have modified git-apply to create a table of the filenames of files it modifies such that if a later patch chunk modifies a file in the table it will buffer the previously changed file instead of reading the original file from disk. Logic has been put in to handle creations/deletions/renames/copies. All the relevant tests of git-apply succeed. A new test has been added to cover the cases I addressed. The fix is relatively straight-forward. Signed-off-by: Don Zickus <dzickus@redhat.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Don Zickus committed
Jun 27, 2008 at 14:39 UTC
7a07841c0ba8d791dd4c7363eaf004b4a7d11fb6
2 files changed
+157
-10
builtin-apply.c
+72
-10
@@ -12,6 +12,7 @@
12
#include "blob.h"
13
#include "delta.h"
14
#include "builtin.h"
15
+#include "path-list.h"
16
17
/*
18
* --check turns on checking that the working tree matches the
@@ -185,6 +186,13 @@ struct image {
186
struct line *line;
187
};
188
189
+/*
190
+ * Records filenames that have been touched, in order to handle
191
+ * the case where more than one patches touch the same file.
192
+ */
193
+
194
+static struct path_list fn_table;
195
+
196
static uint32_t hash_line(const char *cp, size_t len)
197
{
198
size_t i;
@@ -2176,15 +2184,62 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)
2184
return 0;
2185
}
2186
2187
+static struct patch *in_fn_table(const char *name)
2188
+{
2189
+ struct path_list_item *item;
2190
+
2191
+ if (name == NULL)
2192
+ return NULL;
2193
+
2194
+ item = path_list_lookup(name, &fn_table);
2195
+ if (item != NULL)
2196
+ return (struct patch *)item->util;
2197
+
2198
+ return NULL;
2199
+}
2200
+
2201
+static void add_to_fn_table(struct patch *patch)
2202
+{
2203
+ struct path_list_item *item;
2204
+
2205
+ /*
2206
+ * Always add new_name unless patch is a deletion
2207
+ * This should cover the cases for normal diffs,
2208
+ * file creations and copies
2209
+ */
2210
+ if (patch->new_name != NULL) {
2211
+ item = path_list_insert(patch->new_name, &fn_table);
2212
+ item->util = patch;
2213
+ }
2214
+
2215
+ /*
2216
+ * store a failure on rename/deletion cases because
2217
+ * later chunks shouldn't patch old names
2218
+ */
2219
+ if ((patch->new_name == NULL) || (patch->is_rename)) {
2220
+ item = path_list_insert(patch->old_name, &fn_table);
2221
+ item->util = (struct patch *) -1;
2222
+ }
2223
+}
2224
+
2225
static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
2226
{
2227
struct strbuf buf;
2228
struct image image;
2229
size_t len;
2230
char *img;
2231
+ struct patch *tpatch;
2232
2233
strbuf_init(&buf, 0);
2187
- if (cached) {
2234
+
2235
+ if ((tpatch = in_fn_table(patch->old_name)) != NULL) {
2236
+ if (tpatch == (struct patch *) -1) {
2237
+ return error("patch %s has been renamed/deleted",
2238
+ patch->old_name);
2239
+ }
2240
+ /* We have a patched copy in memory use that */
2241
+ strbuf_add(&buf, tpatch->result, tpatch->resultsize);
2242
+ } else if (cached) {
2243
if (read_file_or_gitlink(ce, &buf))
2244
return error("read of %s failed", patch->old_name);
2245
} else if (patch->old_name) {
@@ -2211,6 +2266,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
2266
return -1; /* note with --reject this succeeds. */
2267
patch->result = image.buf;
2268
patch->resultsize = image.len;
2269
+ add_to_fn_table(patch);
2270
free(image.line_allocated);
2271
2272
if (0 < patch->is_delete && patch->resultsize)
@@ -2255,6 +2311,7 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)
2311
static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)
2312
{
2313
const char *old_name = patch->old_name;
2314
+ struct patch *tpatch;
2315
int stat_ret = 0;
2316
unsigned st_mode = 0;
2317
@@ -2268,12 +2325,17 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
2325
return 0;
2326
2327
assert(patch->is_new <= 0);
2271
- if (!cached) {
2328
+ if ((tpatch = in_fn_table(old_name)) != NULL) {
2329
+ if (tpatch == (struct patch *) -1) {
2330
+ return error("%s: has been deleted/renamed", old_name);
2331
+ }
2332
+ st_mode = tpatch->new_mode;
2333
+ } else if (!cached) {
2334
stat_ret = lstat(old_name, st);
2335
if (stat_ret && errno != ENOENT)
2336
return error("%s: %s", old_name, strerror(errno));
2337
}
2276
- if (check_index) {
2338
+ if (check_index && !tpatch) {
2339
int pos = cache_name_pos(old_name, strlen(old_name));
2340
if (pos < 0) {
2341
if (patch->is_new < 0)
@@ -2325,7 +2387,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
2387
return 0;
2388
}
2389
2328
-static int check_patch(struct patch *patch, struct patch *prev_patch)
2390
+static int check_patch(struct patch *patch)
2391
{
2392
struct stat st;
2393
const char *old_name = patch->old_name;
@@ -2342,8 +2404,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
2404
return status;
2405
old_name = patch->old_name;
2406
2345
- if (new_name && prev_patch && 0 < prev_patch->is_delete &&
2346
- !strcmp(prev_patch->old_name, new_name))
2407
+ if (in_fn_table(new_name) == (struct patch *) -1)
2408
/*
2409
* A type-change diff is always split into a patch to
2410
* delete old, immediately followed by a patch to
@@ -2393,15 +2454,14 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
2454
2455
static int check_patch_list(struct patch *patch)
2456
{
2396
- struct patch *prev_patch = NULL;
2457
int err = 0;
2458
2399
- for (prev_patch = NULL; patch ; patch = patch->next) {
2459
+ while (patch) {
2460
if (apply_verbosely)
2461
say_patch_name(stderr,
2462
"Checking patch ", patch, "...\n");
2403
- err |= check_patch(patch, prev_patch);
2404
- prev_patch = patch;
2463
+ err |= check_patch(patch);
2464
+ patch = patch->next;
2465
}
2466
return err;
2467
}
@@ -2919,6 +2979,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
2979
struct patch *list = NULL, **listp = &list;
2980
int skipped_patch = 0;
2981
2982
+ /* FIXME - memory leak when using multiple patch files as inputs */
2983
+ memset(&fn_table, 0, sizeof(struct path_list));
2984
strbuf_init(&buf, 0);
2985
patch_input_file = filename;
2986
read_patch_file(&buf, fd);
t/t4127-apply-same-fn.sh
new
+85
@@ -0,0 +1,85 @@
1
+#!/bin/sh
2
+
3
+test_description='apply same filename'
4
+
5
+. ./test-lib.sh
6
+
7
+test_expect_success setup '
8
+ for i in a b c d e f g h i j k l m
9
+ do
10
+ echo $i
11
+ done >same_fn &&
12
+ cp same_fn other_fn &&
13
+ git add same_fn other_fn &&
14
+ git commit -m initial
15
+'
16
+test_expect_success 'apply same filename with independent changes' '
17
+ sed -i -e "s/^d/z/" same_fn &&
18
+ git diff > patch0 &&
19
+ git add same_fn &&
20
+ sed -i -e "s/^i/y/" same_fn &&
21
+ git diff >> patch0 &&
22
+ cp same_fn same_fn2 &&
23
+ git reset --hard &&
24
+ git-apply patch0 &&
25
+ diff same_fn same_fn2
26
+'
27
+
28
+test_expect_success 'apply same filename with overlapping changes' '
29
+ git reset --hard
30
+ sed -i -e "s/^d/z/" same_fn &&
31
+ git diff > patch0 &&
32
+ git add same_fn &&
33
+ sed -i -e "s/^e/y/" same_fn &&
34
+ git diff >> patch0 &&
35
+ cp same_fn same_fn2 &&
36
+ git reset --hard &&
37
+ git-apply patch0 &&
38
+ diff same_fn same_fn2
39
+'
40
+
41
+test_expect_success 'apply same new filename after rename' '
42
+ git reset --hard
43
+ git mv same_fn new_fn
44
+ sed -i -e "s/^d/z/" new_fn &&
45
+ git add new_fn &&
46
+ git diff -M --cached > patch1 &&
47
+ sed -i -e "s/^e/y/" new_fn &&
48
+ git diff >> patch1 &&
49
+ cp new_fn new_fn2 &&
50
+ git reset --hard &&
51
+ git apply --index patch1 &&
52
+ diff new_fn new_fn2
53
+'
54
+
55
+test_expect_success 'apply same old filename after rename -- should fail.' '
56
+ git reset --hard
57
+ git mv same_fn new_fn
58
+ sed -i -e "s/^d/z/" new_fn &&
59
+ git add new_fn &&
60
+ git diff -M --cached > patch1 &&
61
+ git mv new_fn same_fn
62
+ sed -i -e "s/^e/y/" same_fn &&
63
+ git diff >> patch1 &&
64
+ git reset --hard &&
65
+ test_must_fail git apply patch1
66
+'
67
+
68
+test_expect_success 'apply A->B (rename), C->A (rename), A->A -- should pass.' '
69
+ git reset --hard
70
+ git mv same_fn new_fn
71
+ sed -i -e "s/^d/z/" new_fn &&
72
+ git add new_fn &&
73
+ git diff -M --cached > patch1 &&
74
+ git commit -m "a rename" &&
75
+ git mv other_fn same_fn
76
+ sed -i -e "s/^e/y/" same_fn &&
77
+ git add same_fn &&
78
+ git diff -M --cached >> patch1 &&
79
+ sed -i -e "s/^g/x/" same_fn &&
80
+ git diff >> patch1 &&
81
+ git reset --hard HEAD^ &&
82
+ git apply patch1
83
+'
84
+
85
+test_done