Re: [PATCH v4] git-apply: apply submodule changes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Here is my take on it.  Major part of it is identical to your
version 4 because it is directly based on it, but I think the
division of labor is more in line with the original code.
The part to write out the result may be quite different,
though.

 * check_patch() is where all the validation of the patch
   happens.  Namely, it should

   - check if the filesystem and index are in sync for affected
     paths, if we are looking at the index (i.e. --index or
     --cached); "if (old_name)" part, especially its "if
     (check_index)" subpart is about this check.  For gitlinks,
     when we are looking at the index, we need to ignore the
     differences ce_match_stat() may find, but still we would
     make sure the directory exists, as we run checkout_entry()
     which currently makes sure there is a directory exists.

   - check and make sure that the filesystem has the path to be
     patched, unless we are operating under --cached.  "else if
     (stat_ret < 0)" part is about this check.  Because we do
     allow not checking out subprojects at all, ENOENT is Ok for
     gitlinks.

 * apply_data() is where you prepare the result of the patch in
   core, without touching the work tree with anything from the
   patch we just read.  The initial half of the function is
   about reading the preimage to be patched.  This part is
   almost the same from your version 4, but I lifted the magic
   "patch->fragments = NULL" assignment out of the other
   function to make it clear there is something unusual
   happening in this function.

 * write_out_one_result() calls remove_file() and create_file()
   to match the work tree to the result you prepared with
   apply_data().

   - remove_file() is changed not to do any for gitlink.  We
     _might_ want to try rmdir() if there is an otherwise empty
     directory there, but currently we cannot do much to the
     failure on that, so I did not bother with it.

   - create_file() does three things:
     - create a file in the work tree to match the result;
     - update the index with the patch result;
     - invalidate cache-tree entry for the path.

     For the first task, create_one_file() is usually used to
     create a blob (either regular file or a symlink).  For
     gitlinks, we do not affect the work tree for now, just like
     checkout_entry().

     The second task is usually done by actually reading the
     resulting blob out to find out the new blob object name;
     for gitlinks, apply_data() fakes this with the "Submodule
     commit abc...." message and we pick up the object name from
     there.

---

 builtin-apply.c            |  117 ++++++++++++++++++++++++++++++++++----------
 t/t7400-submodule-basic.sh |   17 ++++++
 2 files changed, 108 insertions(+), 26 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index da27075..eab257a 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1984,6 +1984,25 @@ 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;
+}
+
 static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
 {
 	char *buf;
@@ -1994,22 +2013,32 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 	alloc = 0;
 	buf = NULL;
 	if (cached) {
-		if (ce) {
-			enum object_type type;
-			buf = read_sha1_file(ce->sha1, &type, &size);
-			if (!buf)
+		if (read_file_or_gitlink(ce, &buf, &size))
+			return error("read of %s failed", patch->old_name);
+		alloc = size;
+	} else if (patch->old_name) {
+		if (S_ISGITLINK(patch->old_mode)) {
+			if (ce)
+				read_file_or_gitlink(ce, &buf, &size);
+			else {
+				/*
+				 * There is no way to apply subproject
+				 * patch without looking at the index.
+				 */
+				patch->fragments = NULL;
+				size = 0;
+			}
+		}
+		else {
+			size = xsize_t(st->st_size);
+			alloc = size + 8192;
+			buf = xmalloc(alloc);
+			if (read_old_data(st, patch->old_name,
+					  &buf, &alloc, &size))
 				return error("read of %s failed",
 					     patch->old_name);
-			alloc = size;
 		}
 	}
-	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))
-			return error("read of %s failed", patch->old_name);
-	}
 
 	desc.size = size;
 	desc.alloc = alloc;
@@ -2055,6 +2084,17 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static int verify_index_match(struct cache_entry *ce, struct stat *st)
+{
+	if (!ce_match_stat(ce, st, 1))
+		return 0;
+	if (S_ISGITLINK(ntohl(ce->ce_mode))) {
+		if (S_ISDIR(st->st_mode))
+			return 0;
+	}
+	return -1;
+}
+
 static int check_patch(struct patch *patch, struct patch *prev_patch)
 {
 	struct stat st;
@@ -2065,8 +2105,14 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 	int ok_if_exists;
 
 	patch->rejected = 1; /* we will drop this after we succeed */
+
+	/*
+	 * Make sure that we do not have local modifications from the
+	 * index when we are looking at the index.  Also make sure
+	 * we have the preimage file to be patched in the work tree,
+	 * unless --cached, which tells git to apply only in the index.
+	 */
 	if (old_name) {
-		int changed = 0;
 		int stat_ret = 0;
 		unsigned st_mode = 0;
 
@@ -2096,16 +2142,22 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 				    lstat(old_name, &st))
 					return -1;
 			}
-			if (!cached)
-				changed = ce_match_stat(ce, &st, 1);
-			if (changed)
+			if (!cached && verify_index_match(ce, &st))
 				return error("%s: does not match index",
 					     old_name);
 			if (cached)
 				st_mode = ntohl(ce->ce_mode);
+		} else if (stat_ret < 0) {
+			if (errno == ENOENT && S_ISGITLINK(patch->old_mode))
+				/*
+				 * It is Ok not to have the submodule
+				 * checked out at all.
+				 */
+				;
+			else
+				return error("%s: %s", old_name,
+					     strerror(errno));
 		}
-		else if (stat_ret < 0)
-			return error("%s: %s", old_name, strerror(errno));
 
 		if (!cached)
 			st_mode = ntohl(ce_mode_from_stat(ce, st.st_mode));
@@ -2354,7 +2406,9 @@ static void remove_file(struct patch *patch, int rmdir_empty)
 		cache_tree_invalidate_path(active_cache_tree, patch->old_name);
 	}
 	if (!cached) {
-		if (!unlink(patch->old_name) && rmdir_empty) {
+		if (S_ISGITLINK(patch->old_mode))
+			;
+		else if (!unlink(patch->old_name) && rmdir_empty) {
 			char *name = xstrdup(patch->old_name);
 			char *end = strrchr(name, '/');
 			while (end) {
@@ -2382,13 +2436,21 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 	memcpy(ce->name, path, namelen);
 	ce->ce_mode = create_ce_mode(mode);
 	ce->ce_flags = htons(namelen);
-	if (!cached) {
-		if (lstat(path, &st) < 0)
-			die("unable to stat newly created file %s", path);
-		fill_stat_cache_info(ce, &st);
+	if (S_ISGITLINK(mode)) {
+		const char *s = buf;
+
+		if (get_sha1_hex(s + strlen("Subproject commit "), ce->sha1))
+			die("corrupt patch for subproject %s", path);
+	} else {
+		if (!cached) {
+			if (lstat(path, &st) < 0)
+				die("unable to stat newly created file %s",
+				    path);
+			fill_stat_cache_info(ce, &st);
+		}
+		if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0)
+			die("unable to create backing store for newly created file %s", path);
 	}
-	if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0)
-		die("unable to create backing store for newly created file %s", path);
 	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
 		die("unable to add cache entry for %s", path);
 }
@@ -2485,7 +2547,10 @@ static void create_file(struct patch *patch)
 
 	if (!mode)
 		mode = S_IFREG | 0644;
-	create_one_file(path, mode, buf, size);
+	if (S_ISGITLINK(mode))
+		; /* we do not automatically check submodule out */
+	else
+		create_one_file(path, mode, buf, size);
 	add_index_file(path, mode, buf, size);
 	cache_tree_invalidate_path(active_cache_tree, path);
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e8ce7cd..9d142ed 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -175,4 +175,21 @@ test_expect_success 'checkout superproject with subproject already present' '
 	git-checkout master
 '
 
+test_expect_success 'apply submodule diff' '
+	git branch second &&
+	(
+		cd lib &&
+		echo s >s &&
+		git add s &&
+		git commit -m "change subproject"
+	) &&
+	git update-index --add lib &&
+	git-commit -m "change lib" &&
+	git-format-patch -1 --stdout >P.diff &&
+	git checkout second &&
+	git apply --index P.diff &&
+	D=$(git diff --cached master) &&
+	test -z "$D"
+'
+
 test_done

-
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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux