[PATCH] git-apply: apply submodule changes

[PATCH] git-apply: apply submodule changes

[Sven Verdoolaege]

Apply "Subproject commit HEX" changes produced by git-diff.
As usual in the current git, only the superproject itself is actually
modified (possibly creating empty directories for new submodules).

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
This also makes rebase handle submodules.

 builtin-apply.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index da27075..24df282 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1984,6 +1984,26 @@ 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,20 +2014,18 @@ 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)
-				return error("read of %s failed",
-					     patch->old_name);
-			alloc = size;
-		}
+		if (read_file_or_gitlink(ce, &buf, &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))
+		if (S_ISGITLINK(patch->old_mode))
+			size = snprintf(buf, alloc,
+				"Subproject commit %s\n", sha1_to_hex(ce->sha1));
+		else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
 			return error("read of %s failed", patch->old_name);
 	}
 
@@ -2098,7 +2116,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 			}
 			if (!cached)
 				changed = ce_match_stat(ce, &st, 1);
-			if (changed)
+			if (changed && !S_ISGITLINK(patch->old_mode))
 				return error("%s: does not match index",
 					     old_name);
 			if (cached)
@@ -2387,7 +2405,9 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			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)
+	if (S_ISGITLINK(mode))
+		get_sha1_hex(buf + strlen("Subproject commit "), ce->sha1);
+	else 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);
@@ -2398,6 +2418,13 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	int fd;
 	char *nbuf;
 
+	if (S_ISGITLINK(mode)) {
+		struct stat st;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
+			return 0;
+		return mkdir(path, 0777);
+	}
+
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
-- 
1.5.3.rc4.29.g74276-dirty

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

[Johannes Schindelin]

Hi,

On Fri, 10 Aug 2007, Sven Verdoolaege wrote:

> Apply "Subproject commit HEX" changes produced by git-diff.
> As usual in the current git, only the superproject itself is actually
> modified (possibly creating empty directories for new submodules).

For rebase and cherry-pick, it would be nice if git just ignored the 
changes in the submodules, provided that the submodule commit was not 
affected by the to-be-applied patches.

Hmm?

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Fri, 10 Aug 2007, Sven Verdoolaege wrote:

> @@ -2387,7 +2405,9 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
>  			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)
> +	if (S_ISGITLINK(mode))
> +		get_sha1_hex(buf + strlen("Subproject commit "), ce->sha1);
> +	else 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);

I guess that you need to catch an error from get_sha1_hex(), too.

I hope it is not to much to ask for a patch to t7400 to show what this 
patch fixes?

Ciao,
Dscho

[PATCH resend] git-apply: apply submodule changes

[Sven Verdoolaege]

Apply "Subproject commit HEX" changes produced by git-diff.
As usual in the current git, only the superproject itself is actually
modified (possibly creating empty directories for new submodules).
Any checked-out submodule is left untouched and is not required to
be up-to-date.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
This second version has a test and an extra sanity check.

I seem to be experiencing some problems receiving emails,
so I'll reply to a message from Dscho here.

Johannes Schindelin <Johannes.Schindelin <at> gmx.de> writes:
> For rebase and cherry-pick, it would be nice if git just ignored the 
> changes in the submodules, provided that the submodule commit was not 
> affected by the to-be-applied patches.

I have no idea what you mean.

The checked out copies of the submodules are ignored completely
(if that is what you were talking about, then I hope this issue
is clarified by the updated commit message).  In the superproject,
the change to the submodule is obviously not ignored, since it's
an integral part of the patch.  However, git-apply will fail if
the original submodule commit does not correspond exactly to the
"from-file" submodule commit.
I don't think there is anything else we can do without a true
recursive git-diff/git-apply.

skimo

 builtin-apply.c            |   50 ++++++++++++++++++++++++++++++++++---------
 t/t7400-submodule-basic.sh |    8 +++++++
 2 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index da27075..a38dbf1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1984,6 +1984,26 @@ 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,20 +2014,18 @@ 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)
-				return error("read of %s failed",
-					     patch->old_name);
-			alloc = size;
-		}
+		if (read_file_or_gitlink(ce, &buf, &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))
+		if (S_ISGITLINK(patch->old_mode))
+			size = snprintf(buf, alloc,
+				"Subproject commit %s\n", sha1_to_hex(ce->sha1));
+		else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
 			return error("read of %s failed", patch->old_name);
 	}
 
@@ -2098,7 +2116,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 			}
 			if (!cached)
 				changed = ce_match_stat(ce, &st, 1);
-			if (changed)
+			if (changed && !S_ISGITLINK(patch->old_mode))
 				return error("%s: does not match index",
 					     old_name);
 			if (cached)
@@ -2387,7 +2405,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			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)
+	if (S_ISGITLINK(mode)) {
+		if (get_sha1_hex(buf + strlen("Subproject commit "), ce->sha1))
+			die("corrupt patch for subproject %s", path);
+	} else 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);
@@ -2398,6 +2419,13 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	int fd;
 	char *nbuf;
 
+	if (S_ISGITLINK(mode)) {
+		struct stat st;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
+			return 0;
+		return mkdir(path, 0777);
+	}
+
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e8ce7cd..cede2e7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -175,4 +175,12 @@ test_expect_success 'checkout superproject with subproject already present' '
 	git-checkout master
 '
 
+test_expect_success 'rebase with subproject changes' '
+	git-checkout initial &&
+	echo t > t &&
+	git add t &&
+	git-commit -m "change t" &&
+	git-rebase HEAD master
+'
+
 test_done
-- 
1.5.3.rc4.29.g74276-dirty

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

> @@ -1994,20 +2014,18 @@ 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)
> -				return error("read of %s failed",
> -					     patch->old_name);
> -			alloc = size;
> -		}
> +		if (read_file_or_gitlink(ce, &buf, &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))
> +		if (S_ISGITLINK(patch->old_mode))
> +			size = snprintf(buf, alloc,
> +				"Subproject commit %s\n", sha1_to_hex(ce->sha1));
> +		else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))

Who guarantees that ce is given to apply_data() in this codepath?

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

[Sven Verdoolaege]

On Fri, Aug 10, 2007 at 10:43:42PM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@kotnet.org> writes:
> >  	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 = snprintf(buf, alloc,
> > +				"Subproject commit %s\n", sha1_to_hex(ce->sha1));
> > +		else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
> 
> Who guarantees that ce is given to apply_data() in this codepath?

Oops.  I guess it shows that I only tested it through git-rebase.
Would changing

		if (check_index) {

on line 2093 to

		if (check_index || S_ISGITLINK(patch->old_mode)) {

be acceptable?  Adding a conditional call to read_cache(), of course.
We're not going to be able to apply submodule patches without
an index, anyway.
Or should we just refuse to apply submodule patches if --index
has not been specified?

skimo

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

[Junio C Hamano]

Sven Verdoolaege <skimo@liacs.nl> writes:

> Or should we just refuse to apply submodule patches if --index
> has not been specified?

If somebody sends you a patch that adds a new gitlink, I think
it is very natural to create a directory (or make sure a
directory exists) there when applying without --index.

My gut feeling is that it would probably be the most user
friendly to just warn but ignore if you do not have submodule
there and a patch wants to modify that path.  After all, the
fundamental idea behind our submodule support is that you as a
superproject person should not even have to have the submodule
checked out.

[PATCH v3] git-apply: apply submodule changes

[Sven Verdoolaege]

Apply "Subproject commit HEX" changes produced by git-diff.
As usual in the current git, only the superproject itself is actually
modified (possibly creating empty directories for new submodules).
Any checked-out submodule is left untouched and is not required to
be up-to-date.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
The third version also works without --index and documents
the behaviour of git-apply in the presence of submodule changes.

skimo

 Documentation/git-apply.txt |   14 +++++++++
 builtin-apply.c             |   69 +++++++++++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh  |    8 +++++
 3 files changed, 79 insertions(+), 12 deletions(-)

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.
+
+If --index is specified (explicitly or implicitly), then the submodule
+commits must match the index exactly for the patch to apply.  If any
+of the submodules are checked-out, then these check-outs are completely
+ignored, i.e., they are not required to be up-to-date or clean and they
+are not updated.
+
+If --index is not specified, then the submodule commits in the patch
+are ignored and only the absence of presence of the corresponding
+subdirectory is checked and (if possible) updated.
 
 Author
 ------
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;
+}
+
+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;
+}
+
 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 (ce) {
-			enum object_type type;
-			buf = read_sha1_file(ce->sha1, &type, &size);
-			if (!buf)
-				return error("read of %s failed",
-					     patch->old_name);
-			alloc = size;
-		}
+		if (read_file_or_gitlink(ce, &buf, &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))
+		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);
 	}
 
@@ -2098,7 +2129,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 			}
 			if (!cached)
 				changed = ce_match_stat(ce, &st, 1);
-			if (changed)
+			if (changed && !S_ISGITLINK(patch->old_mode))
 				return error("%s: does not match index",
 					     old_name);
 			if (cached)
@@ -2354,7 +2385,11 @@ 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)) {
+			if (rmdir(patch->old_name))
+				warning("unable to remove submodule %s",
+					patch->old_name);
+		} else if (!unlink(patch->old_name) && rmdir_empty) {
 			char *name = xstrdup(patch->old_name);
 			char *end = strrchr(name, '/');
 			while (end) {
@@ -2387,7 +2422,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			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)
+	if (S_ISGITLINK(mode)) {
+		if (get_sha1_hex(buf + strlen("Subproject commit "), ce->sha1))
+			die("corrupt patch for subproject %s", path);
+	} else 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);
@@ -2398,6 +2436,13 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	int fd;
 	char *nbuf;
 
+	if (S_ISGITLINK(mode)) {
+		struct stat st;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
+			return 0;
+		return mkdir(path, 0777);
+	}
+
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e8ce7cd..cede2e7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -175,4 +175,12 @@ test_expect_success 'checkout superproject with subproject already present' '
 	git-checkout master
 '
 
+test_expect_success 'rebase with subproject changes' '
+	git-checkout initial &&
+	echo t > t &&
+	git add t &&
+	git-commit -m "change t" &&
+	git-rebase HEAD master
+'
+
 test_done
-- 
1.5.3.rc4.30.gd0c97-dirty

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

> 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"...

> 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.

>  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.

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

[Sven Verdoolaege]

On Sun, Aug 12, 2007 at 11:16:09AM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@kotnet.org> writes:
> >  	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...".

Hmmm... the documentation says that if --index is in effect
then the file to be patched in the work tree is supposed to be 
up-to-date.  Then for files it shouldn't matter if the data
comes from the index or not.  For submodules it's crucial to
look at the index (if it is available), since that's the
only place you can find the current state of the submodule.
(Remember that git currently doesn't automatically update submodules.)

If I specify --index (e.g., through git rebase), then I really
wouldn't want the patch to apply if the old submodule commit
in the patch doesn't match the submodule commit in the index.
Would you?

>  - what is alloc at this point?  it is based on the size of
>    directory st->st_size.

I assume you mean "size".  For submodules, the value isn't used,
except to test that the "file" is empty (size==0) on delete.
So it seems safe to set it to zero for submodules in any case.

> 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.

As explained above, I don't think it would a good idea to
just drop gitlink patches if --index is specified.

> 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.

Are you talking about the case where --index is specified?
Otherwise, I don't think we can make any assumptions on
what's inside the subdirectory.

skimo

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

>>  - 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...".
>
> Hmmm... the documentation says that if --index is in effect
> then the file to be patched in the work tree is supposed to be 
> up-to-date.

But that is the job of check_patch(), not this function, isn't it?

>> 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.
>
> Are you talking about the case where --index is specified?

Talking about both cases, and the division of responsibility
between check_patch() and apply_data().

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

[Sven Verdoolaege]

On Sun, Aug 12, 2007 at 12:24:29PM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@kotnet.org> writes:
> 
> >>  - 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...".
> >
> > Hmmm... the documentation says that if --index is in effect
> > then the file to be patched in the work tree is supposed to be 
> > up-to-date.
> 
> But that is the job of check_patch(), not this function, isn't it?

Ah... so you want me to check that there has been no type change
to the submodule in check_patch (and then I can use my
read_gitlink_or_skip as is).  Is that right?

> >> 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.
> >
> > Are you talking about the case where --index is specified?
> 
> Talking about both cases, and the division of responsibility
> between check_patch() and apply_data().

I'll do that for the --index case, but I really think it doesn't
make sense for the other case.  If we're not in a git repo,
then the submodule, which may very well be present, is not
going to be a git repo either.

What could make sense is to enforce that in the --index case,
the submodule directory is either empty or a git repo and
that in the non --index case it is empty.

skimo

[PATCH v4] git-apply: apply submodule changes

[Sven Verdoolaege]

Apply "Subproject commit HEX" changes produced by git-diff.
As usual in the current git, only the superproject itself is actually
modified (possibly creating empty directories for new submodules).
Any checked-out submodule is left untouched and is not required to
be up-to-date.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
This version adds an extra check to verify that a submodule
still looks like a submodule in the work tree.

 Documentation/git-apply.txt |   14 +++++++
 builtin-apply.c             |   91 +++++++++++++++++++++++++++++++++++++------
 t/t7400-submodule-basic.sh  |    8 ++++
 3 files changed, 101 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f03f661..4c7e3a2 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]
+treats these changes as follows.
+
+If --index is specified (explicitly or implicitly), then the submodule
+commits must match the index exactly for the patch to apply.  If any
+of the submodules are checked-out, then these check-outs are completely
+ignored, i.e., they are not required to be up-to-date or clean and they
+are not updated.
+
+If --index is not specified, then the submodule commits in the patch
+are ignored and only the absence of presence of the corresponding
+subdirectory is checked and (if possible) updated.
 
 Author
 ------
diff --git a/builtin-apply.c b/builtin-apply.c
index da27075..8ba20a6 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -12,6 +12,7 @@
 #include "blob.h"
 #include "delta.h"
 #include "builtin.h"
+#include "refs.h"
 
 /*
  *  --check turns on checking that the working tree matches the
@@ -1984,6 +1985,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;
+}
+
+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;
+}
+
 static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
 {
 	char *buf;
@@ -1994,20 +2029,17 @@ 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)
-				return error("read of %s failed",
-					     patch->old_name);
-			alloc = size;
-		}
+		if (read_file_or_gitlink(ce, &buf, &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))
+		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);
 	}
 
@@ -2055,6 +2087,20 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+/* Check that the directory corresponding to a gitlink is either
+ * empty or a git repo.
+ */
+static int verify_gitlink_clean(const char *path)
+{
+	unsigned char sha1[20];
+
+	if (!rmdir(path)) {
+		mkdir(path, 0777);
+		return 0;
+	}
+	return resolve_gitlink_ref(path, "HEAD", sha1);
+}
+
 static int check_patch(struct patch *patch, struct patch *prev_patch)
 {
 	struct stat st;
@@ -2096,8 +2142,15 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 				    lstat(old_name, &st))
 					return -1;
 			}
-			if (!cached)
+			if (!cached) {
 				changed = ce_match_stat(ce, &st, 1);
+				if (S_ISGITLINK(patch->old_mode)) {
+					changed &= TYPE_CHANGED;
+					if (!changed &&
+					    verify_gitlink_clean(patch->old_name))
+						changed |= TYPE_CHANGED;
+				}
+			}
 			if (changed)
 				return error("%s: does not match index",
 					     old_name);
@@ -2354,7 +2407,11 @@ 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)) {
+			if (rmdir(patch->old_name))
+				warning("unable to remove submodule %s",
+					patch->old_name);
+		} else if (!unlink(patch->old_name) && rmdir_empty) {
 			char *name = xstrdup(patch->old_name);
 			char *end = strrchr(name, '/');
 			while (end) {
@@ -2387,7 +2444,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			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)
+	if (S_ISGITLINK(mode)) {
+		if (get_sha1_hex(buf + strlen("Subproject commit "), ce->sha1))
+			die("corrupt patch for subproject %s", path);
+	} else 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);
@@ -2398,6 +2458,13 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	int fd;
 	char *nbuf;
 
+	if (S_ISGITLINK(mode)) {
+		struct stat st;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
+			return 0;
+		return mkdir(path, 0777);
+	}
+
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e8ce7cd..cede2e7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -175,4 +175,12 @@ test_expect_success 'checkout superproject with subproject already present' '
 	git-checkout master
 '
 
+test_expect_success 'rebase with subproject changes' '
+	git-checkout initial &&
+	echo t > t &&
+	git add t &&
+	git-commit -m "change t" &&
+	git-rebase HEAD master
+'
+
 test_done
-- 
1.5.3.rc4.68.g4411e-dirty

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

> +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;
> +}
> @@ -1994,20 +2029,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;
> ...
>  	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);
>  	}

I think the logic here sounds sane.  The read_file_or_gitlink()
abstraction in the first if() part was so nice that I was hoping
we could do something similar in this "else if" part, without
hiding the assignment to patch->fragments as an obscure side
effect of calling read_gitlink_or_skip().  That assignment is a
gitlink specific hack so it might be easier to read if this part
is written like:

	} else if (patch->old_name && S_ISGITLINK(patch->old_mode)) {
		if (ce)
                	size = snprintf(....)
		else {
                	/* we cannot apply gitlink mods without index */
			size = 0;
			patch->fragments = NULL;
		}
        } else if (patch->old_name) {
		... original code here ...

> @@ -2055,6 +2087,20 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
>  	return 0;
>  }
>  
> +/* Check that the directory corresponding to a gitlink is either
> + * empty or a git repo.
> + */
> +static int verify_gitlink_clean(const char *path)
> +{
> +	unsigned char sha1[20];
> +
> +	if (!rmdir(path)) {
> +		mkdir(path, 0777);
> +		return 0;
> +	}
> +	return resolve_gitlink_ref(path, "HEAD", sha1);
> +}

Is it the responsibility of the caller of this function to make
sure that path is either absent or is a directory?  Can there be
a regular file at path, which would cause rmdir to fail?  I do
not think it is sensible to call resolve_gitlink_ref() on such a
path, so let's say the caller will make sure that path is
gitlink in the current (i.e. in index) tree before calling this
function.

>  static int check_patch(struct patch *patch, struct patch *prev_patch)
>  {
>  	struct stat st;
> @@ -2096,8 +2142,15 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
>  				    lstat(old_name, &st))
>  					return -1;
>  			}
> -			if (!cached)
> +			if (!cached) {
>  				changed = ce_match_stat(ce, &st, 1);

Here, ce_match_stat() sees if the index and work tree match.
You could have a regular file there but you haven't checked
(which is not a crime, yet).

> +				if (S_ISGITLINK(patch->old_mode)) {

I think the rmdir/mkdir sequence should be done only when ce is
a gitlink.  Perhaps it is just the matter of:

                                if (S_ISGITLINK(patch->old_mode) &&
                                    S_ISGITLINK(ntohl(ce->ce_mode))) {
                                        ...
                                }

> +					changed &= TYPE_CHANGED;
> +					if (!changed &&
> +					    verify_gitlink_clean(patch->old_name))
> +						changed |= TYPE_CHANGED;
> +				}
> +			}

This part is very confusing.  You discard all changes other than
TYPE_CHANGED, and give TYPE_CHANGED and nothing else if gitlink
is not clean.  I suspect "changed &= ~TYPE_CHANGED" might be
what you meant, but I do not know what you are trying to do
here.

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

[Sven Verdoolaege]

On Mon, Aug 13, 2007 at 11:27:38PM -0700, Junio C Hamano wrote:
>  * 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.

We could at least warn about it, which is what my patch did.

>    - 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().

It creates the subdirectory, though, and git-apply should do so
too since it expects the subdirectory to be there for subsequent
patches (at least in the --index case).

> diff --git a/builtin-apply.c b/builtin-apply.c

Did you remove the documentation on purpose ?

> +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;
> +	}

Not a big deal, but ce_match_stat already checks for that.
That's why I was checking for TYPE_CHANGED in its return
value.

> @@ -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));
>  		}

Shouldn't you be consistent with the --index case and require the
subdirectory to exist?

skimo

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

>> diff --git a/builtin-apply.c b/builtin-apply.c
>
> Did you remove the documentation on purpose ?

No, I just wanted to get a feedback on a (possibly partial)
cleanup, as I couldn't make heads or tails of your patch
especially around that TYPE_CHANGED part, and also the part to
write out the results.

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

> @@ -2096,8 +2142,15 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
>  				    lstat(old_name, &st))
>  					return -1;
>  			}
> -			if (!cached)
> +			if (!cached) {
>  				changed = ce_match_stat(ce, &st, 1);
> +				if (S_ISGITLINK(patch->old_mode)) {
> +					changed &= TYPE_CHANGED;
> +					if (!changed &&
> +					    verify_gitlink_clean(patch->old_name))
> +						changed |= TYPE_CHANGED;
> +				}
> +			}

In this codepath, we know the patch wants to either modify the
path at old_name or remove old_name.  If we are going to affect
the work tree, we have run lstat on it, and ran checkout_entry() 
if we did not have anything there and did lstat() again.

I think the check "S_ISGITLINK(patch->old_mode)" is wrong
(that's where my confusion while reading your patch came from).
It has to check ce's mode, not patch->old_mode, because we are
verifying if the index matches with the work tree in this
codepath.  If you fix it to S_ISGITLINK(ntohl(ce->ce_mode)),
I think I can see what you are trying to do.

When ce is not a gitlink, you keep the original behaviour, which
is assuring that you did not break things for people who do not
use gitlink.

I am still having trouble with the TYPE_CHANGED bits.  You
discard everything other than TYPE_CHANGED, and 

 - if ce_match_stat() returned TYPE_CHANGED, then that is given
   to later processing to cause us to fail "oops, path is not up
   to date";

 - if ce_match_stat() did not return TYPE_CHANGED, that means we
   found a directory at the path (ce_match_stat_basic() says
   so).  In such a case you call verify_gitlink_clean(), but it
   essentially says "make sure there is either an empty
   directory or some repository".  Maybe we do not even have to
   have this extra check?

When ce is a gitlink, ce_match_stat() says DATA_CHANGED if the
commit in the work tree of the subproject is different.  From
the earlier discussions, we do want to discard DATA_CHANGED for
this codepath.

So it looks almost Ok after spending a few days looking at this
code.  Finally.

However, if it takes _me_ three days to understand this hunk,
(admittably, the parameter to S_ISGITLINK() completely confused
me originally, and I also had other things to do, so it was not
"72 hours"), I do not think the code with your patch is
maintainable by anybody.  At least we would need to have a few
words of comment to describe what is going on there.

	if (!cached) {
        	changed = ce_match_stat(ce, &st, 1);
                if (S_ISGITLINK(ntohl(ce->ce_mode)))
                	/*
			 * ce_match_stat() reports the
			 * difference between the commit object
                         * name in the index and what is checked
			 * out in the work tree of subproject;
                         * because we do not recurse, we do not
			 * want to insist on them matching with
                         * each other.
                         */
                	changed &= ~DATA_CHANGED;
	}
        if (changed)
        	return error("%s: does not match index", old_name);

[PATCH v6] git-apply: apply submodule changes

[Sven Verdoolaege]

Apply "Subproject commit HEX" changes produced by git-diff.
As usual in the current git, only the superproject itself is actually
modified (possibly creating empty directories for new submodules).
Any checked-out submodule is left untouched and is not required to
be up-to-date.

With clean-ups from Junio C Hamano.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
On Tue, Aug 14, 2007 at 02:09:35AM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@kotnet.org> writes:
> >> diff --git a/builtin-apply.c b/builtin-apply.c
> >
> > Did you remove the documentation on purpose ?
> 
> No, I just wanted to get a feedback on a (possibly partial)
> cleanup, as I couldn't make heads or tails of your patch
> especially around that TYPE_CHANGED part, and also the part to
> write out the results.

I agree that the TYPE_CHANGED thing may have been confusing,
so I kept your version (although I switched the tests around,
since there is no point in checking if the stat info matches
if you're going to ignore the result anyway).

Other than that, the only change wrt to your version is that
I added back the creation and (attempt at) removal of the
corresponding subdirectory.

I'm not sure if you intented to remove the check for
either an empty dir or a git repo.  You asked me to add
it before, but then you removed it in your version.
I didn't add it back again.

skimo

 Documentation/git-apply.txt |   14 +++++
 builtin-apply.c             |  112 +++++++++++++++++++++++++++++++++----------
 t/t7400-submodule-basic.sh  |   17 +++++++
 3 files changed, 118 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f03f661..4c7e3a2 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]
+treats these changes as follows.
+
+If --index is specified (explicitly or implicitly), then the submodule
+commits must match the index exactly for the patch to apply.  If any
+of the submodules are checked-out, then these check-outs are completely
+ignored, i.e., they are not required to be up-to-date or clean and they
+are not updated.
+
+If --index is not specified, then the submodule commits in the patch
+are ignored and only the absence of presence of the corresponding
+subdirectory is checked and (if possible) updated.
 
 Author
 ------
diff --git a/builtin-apply.c b/builtin-apply.c
index da27075..8055c7d 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,16 @@ 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 (S_ISGITLINK(ntohl(ce->ce_mode))) {
+		if (!S_ISDIR(st->st_mode))
+			return -1;
+		return 0;
+	}
+	return ce_match_stat(ce, st, 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,15 +2142,12 @@ 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)
+		} else if (stat_ret < 0)
 			return error("%s: %s", old_name, strerror(errno));
 
 		if (!cached)
@@ -2354,7 +2397,11 @@ 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)) {
+			if (rmdir(patch->old_name))
+				warning("unable to remove submodule %s",
+					patch->old_name);
+		} else if (!unlink(patch->old_name) && rmdir_empty) {
 			char *name = xstrdup(patch->old_name);
 			char *end = strrchr(name, '/');
 			while (end) {
@@ -2382,13 +2429,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);
 }
@@ -2398,6 +2453,13 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	int fd;
 	char *nbuf;
 
+	if (S_ISGITLINK(mode)) {
+		struct stat st;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
+			return 0;
+		return mkdir(path, 0777);
+	}
+
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
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
-- 
1.5.3.rc5.1.g7de89

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

[Junio C Hamano]

Sven Verdoolaege <skimo@kotnet.org> writes:

> I agree that the TYPE_CHANGED thing may have been confusing,
> so I kept your version (although I switched the tests around,
> since there is no point in checking if the stat info matches
> if you're going to ignore the result anyway).
>
> Other than that, the only change wrt to your version is that
> I added back the creation and (attempt at) removal of the
> corresponding subdirectory.

Makes much more sense than what I wrote.  Thanks.