Re: [PATCH v3] git-apply: apply submodule changes
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:28
Sven Verdoolaege [off-list ref] writes:
quoted hunk
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index f03f661..804fdc3 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt@@ -171,6 +171,20 @@ apply.whitespace:: When no `--whitespace` flag is given from the command line, this configuration item is used as the default. +Submodules +---------- +If the patch contains any changes to submodules then gitlink:git-apply[1] +behaves as follows.
perhaps "as follows wrt the submodules"...
quoted hunk
diff --git a/builtin-apply.c b/builtin-apply.c index da27075..eef596b 100644 --- a/builtin-apply.c +++ b/builtin-apply.c@@ -1984,6 +1984,40 @@ static int apply_fragments(struct buffer_desc *desc, struct patch *patch) return 0; } +static int read_file_or_gitlink(struct cache_entry *ce, char **buf_p, + unsigned long *size_p) +{ + if (!ce) + return 0; + + if (S_ISGITLINK(ntohl(ce->ce_mode))) { + *buf_p = xmalloc(100); + *size_p = snprintf(*buf_p, 100, + "Subproject commit %s\n", sha1_to_hex(ce->sha1)); + } else { + enum object_type type; + *buf_p = read_sha1_file(ce->sha1, &type, size_p); + if (!*buf_p) + return -1; + } + + return 0; +}
Ok, read_file_or_gitlink() expects ce taken from the current index and fills *buf_p with the preimage to be patched from it.
+static int read_gitlink_or_skip(struct patch *patch, struct cache_entry *ce,
+ char *buf, unsigned long alloc)
+{
+ if (ce)
+ return snprintf(buf, alloc,
+ "Subproject commit %s\n", sha1_to_hex(ce->sha1));
+
+ /* We can't apply the submodule change without an index, so just
+ * skip the patch itself and only create/remove directory.
+ */
+ patch->fragments = NULL;
+ return 0;
+}Hmmmm... see below.
quoted hunk
static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce) { char *buf;@@ -1994,20 +2028,17 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * alloc = 0; buf = NULL; if (cached) { + if (read_file_or_gitlink(ce, &buf, &size)) + return error("read of %s failed", patch->old_name); + alloc = size; }
This part is consistent with the read_file_or_gitlink() semantics above...
else if (patch->old_name) {
size = xsize_t(st->st_size);
alloc = size + 8192;
buf = xmalloc(alloc);
- if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
+ if (S_ISGITLINK(patch->old_mode))
+ size = read_gitlink_or_skip(patch, ce, buf, alloc);
+ else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
return error("read of %s failed", patch->old_name);
}read_old_data() gets the lstat information from the current filesystem data at old_name, and gives the preimage to be patched, and naturally it bombs out if it is a directory, but when we are applying a change to gitlink, the patch expects old_name to be a directory. So you introduced read_gitlink_or_skip() to work it around. But this makes me wonder... - what does ce have to do in this codepath? read_old_data() does not care about what is in the index (in fact, in the index the entry can be a symlink when the path on the filesystem is a regular file, and it reads from the regular file as asked--it does not even look at ce by design). if you have a regular file there in the current version, ce would say it is a regular file blob and you would not want read_gitlink_or_skip() to say "Subproject commit xyz...". - what is alloc at this point? it is based on the size of directory st->st_size. I think dropping fragments for a patch that tries to modify a gitlink here is fine, but that can be done regardless of what ce is. The type-mismatch case to attempt to apply gitlink patch to a regular blob is covered much earlier in check_patch(). It complains if st_mode does not match patch->old_mode; I think you need to adjust it a bit to: - allow gitlink patch to a path that currently has nothing (no submodule checked out) or a directory that has ".git/" (i.e. submodule checked out). - reject gitlink patch otherwise.