[PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[Stefan Beller]

A single patch evolves into a series.
The second patch is essentially a resend with Junios suggestion squashed[1].

That patch alone doesn't quite fix it yet, as we need to make sure the
submodule code respects the ignore_errors flag as instructed by the user.
Patch 1 libifies the used functions from submodule.c and handles the errors
appropriately.

[1] http://thread.gmane.org/gmane.comp.version-control.git/291810/focus=291829

Thanks,
Stefan

Stefan Beller (2):
  mv submodule: respect ignore_errors for errors in submodule code
  mv: allow moving nested submodules

 builtin/mv.c  | 32 +++++++++++++++++++++++---------
 submodule.c   | 33 ++++++++++++++++++++++++---------
 submodule.h   |  6 ++++--
 t/t7001-mv.sh | 16 ++++++++++++++++
 4 files changed, 67 insertions(+), 20 deletions(-)

-- 
2.8.1.211.g24879d1

[PATCH 1/2] mv submodule: respect ignore_errors for errors in submodule code

[Stefan Beller]

The error handling via passing around a strbuf is well exercised in the
refs code, so apply that pattern here, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/mv.c | 15 ++++++++++++---
 submodule.c  | 33 ++++++++++++++++++++++++---------
 submodule.h  |  6 ++++--
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index aeae855..74516f4 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -247,6 +247,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < argc; i++) {
+		struct strbuf err = STRBUF_INIT;
 		const char *src = source[i], *dst = destination[i];
 		enum update_mode mode = modes[i];
 		int pos;
@@ -256,9 +257,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			if (rename(src, dst) < 0 && !ignore_errors)
 				die_errno(_("renaming '%s' failed"), src);
 			if (submodule_gitfile[i]) {
-				if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
-					connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
-				if (!update_path_in_gitmodules(src, dst))
+				if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
+				    connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||
+				    update_path_in_gitmodules(src, dst, &err)) {
+					if (err.len) {
+						if (ignore_errors) {
+							warning("%s", err.buf);
+							continue;
+						} else
+							die("%s", err.buf);
+					}
+				} else
 					gitmodules_modified = 1;
 			}
 		}
diff --git a/submodule.c b/submodule.c
index 90825e1..ed18d34 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,7 +51,8 @@ int is_staging_gitmodules_ok(void)
  * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
  * with the correct path=<oldpath> setting was found and we could update it.
  */
-int update_path_in_gitmodules(const char *oldpath, const char *newpath)
+int update_path_in_gitmodules(const char *oldpath, const char *newpath,
+			      struct strbuf *err)
 {
 	struct strbuf entry = STRBUF_INIT;
 	const struct submodule *submodule;
@@ -59,8 +60,10 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
 		return -1;
 
-	if (gitmodules_is_unmerged)
-		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+	if (gitmodules_is_unmerged) {
+		strbuf_addf(err, _("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+		return -1;
+	}
 
 	submodule = submodule_from_path(null_sha1, oldpath);
 	if (!submodule || !submodule->name) {
@@ -1102,27 +1105,39 @@ int merge_submodule(unsigned char result[20], const char *path,
 }
 
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+int connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir,
+				  struct strbuf *err)
 {
+	int ret = 0;
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
 	const char *real_work_tree = xstrdup(real_path(work_tree));
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, real_work_tree, &rel_path));
+	if (write_file_gently(file_name.buf, "gitdir: %s",
+	    relative_path(git_dir, real_work_tree, &rel_path))) {
+		strbuf_addf(err, _("could not write .git file (%s): %s"),
+				file_name.buf, strerror(errno));
+		ret = -1;
+		goto out;
+	}
 
 	/* Update core.worktree setting */
 	strbuf_reset(&file_name);
 	strbuf_addf(&file_name, "%s/config", git_dir);
-	git_config_set_in_file(file_name.buf, "core.worktree",
-			       relative_path(real_work_tree, git_dir,
-					     &rel_path));
+	if (git_config_set_in_file_gently(file_name.buf, "core.worktree",
+	    relative_path(real_work_tree, git_dir, &rel_path))) {
+		strbuf_addf(err, _("could not set core.worktree in %s"), file_name.buf);
+		ret = -1;
+		goto out;
+	}
 
+out:
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
 	free((void *)real_work_tree);
+	return ret;
 }
 
 int parallel_submodules(void)
diff --git a/submodule.h b/submodule.h
index 7ef3775..bf816e5 100644
--- a/submodule.h
+++ b/submodule.h
@@ -30,7 +30,8 @@ struct submodule_update_strategy {
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
 int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
+int update_path_in_gitmodules(const char *oldpath, const char *newpath,
+			      struct strbuf *err);
 int remove_path_from_gitmodules(const char *path);
 void stage_updated_gitmodules(void);
 void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
@@ -58,7 +59,8 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
 int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
 		struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+int connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir,
+				  struct strbuf *err);
 int parallel_submodules(void);
 
 #endif
-- 
2.8.1.211.g24879d1

[PATCH 2/2] mv: allow moving nested submodules

[Stefan Beller]

When directories are moved using `git mv` all files in the directory
have been just moved, but no further action was taken on them. This
was done by assigning the mode = WORKING_DIRECTORY to the files
inside a moved directory.

submodules however need to update their link to the git directory as
well as updates to the .gitmodules file. By removing the condition of
`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for
the required submodule actions, we perform these for submodules in a
moved directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/mv.c  | 39 ++++++++++++++++++++++-----------------
 t/t7001-mv.sh | 16 ++++++++++++++++
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 74516f4..2deb95b 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -253,23 +253,28 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		int pos;
 		if (show_only || verbose)
 			printf(_("Renaming %s to %s\n"), src, dst);
-		if (!show_only && mode != INDEX) {
-			if (rename(src, dst) < 0 && !ignore_errors)
-				die_errno(_("renaming '%s' failed"), src);
-			if (submodule_gitfile[i]) {
-				if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
-				    connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||
-				    update_path_in_gitmodules(src, dst, &err)) {
-					if (err.len) {
-						if (ignore_errors) {
-							warning("%s", err.buf);
-							continue;
-						} else
-							die("%s", err.buf);
-					}
-				} else
-					gitmodules_modified = 1;
-			}
+		if (show_only)
+			continue;
+		if (mode != INDEX &&
+		    rename(src, dst) < 0) {
+			if (ignore_errors)
+				continue;
+			die_errno(_("renaming '%s' failed"), src);
+		}
+
+		if (submodule_gitfile[i]) {
+			if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
+			    connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||
+			    update_path_in_gitmodules(src, dst, &err)) {
+				if (err.len) {
+					if (ignore_errors) {
+						warning("%s", err.buf);
+						continue;
+					} else
+						die("%s", err.buf);
+				}
+			} else
+				gitmodules_modified = 1;
 		}
 
 		if (mode == WORKING_DIRECTORY)
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 4008fae..4a2570e 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '
 	echo content >file &&
 	git add file &&
 	git commit -m "added sub and file" &&
+	mkdir -p deep/directory/hierachy &&
+	git submodule add ./. deep/directory/hierachy/sub &&
+	git commit -m "added another submodule" &&
 	git branch submodule
 '
 
@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' '
 	git checkout .
 '
 
+test_expect_success 'moving a submodule in nested directories' '
+	(
+		cd deep &&
+		git mv directory ../ &&
+		# git status would fail if the update of linking git dir to
+		# work dir of the submodule failed.
+		git status &&
+		git config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual &&
+		echo "directory/hierachy/sub" >../expect
+	) &&
+	test_cmp actual expect
+'
+
 test_done
-- 
2.8.1.211.g24879d1

Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> A single patch evolves into a series.

Power of code inspection to see bugs that are not reported, perhaps
;-)?

I wonder if we can come up with test cases to cover these potential
issues that are addressed in [1/2]?

Thanks.

Re: [PATCH 2/2] mv: allow moving nested submodules

[Jacob Keller]

On Mon, Apr 18, 2016 at 4:41 PM, Stefan Beller <sbeller@google.com> wrote:
> When directories are moved using `git mv` all files in the directory
> have been just moved, but no further action was taken on them. This
> was done by assigning the mode = WORKING_DIRECTORY to the files
> inside a moved directory.
>
> submodules however need to update their link to the git directory as
> well as updates to the .gitmodules file. By removing the condition of
> `mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for
> the required submodule actions, we perform these for submodules in a
> moved directory.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>


The patch looks good to me. I've marked some comments that I thought
through while reviewing but it looks correct.

> ---
>  builtin/mv.c  | 39 ++++++++++++++++++++++-----------------
>  t/t7001-mv.sh | 16 ++++++++++++++++
>  2 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 74516f4..2deb95b 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -253,23 +253,28 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                 int pos;
>                 if (show_only || verbose)
>                         printf(_("Renaming %s to %s\n"), src, dst);
> -               if (!show_only && mode != INDEX) {
> -                       if (rename(src, dst) < 0 && !ignore_errors)
> -                               die_errno(_("renaming '%s' failed"), src);
> -                       if (submodule_gitfile[i]) {
> -                               if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
> -                                   connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||
> -                                   update_path_in_gitmodules(src, dst, &err)) {
> -                                       if (err.len) {
> -                                               if (ignore_errors) {
> -                                                       warning("%s", err.buf);
> -                                                       continue;
> -                                               } else
> -                                                       die("%s", err.buf);
> -                                       }
> -                               } else
> -                                       gitmodules_modified = 1;
> -                       }
> +               if (show_only)
> +                       continue;

So here, we skip the item after displaying when we're in show only
mode. That seems correct.

> +               if (mode != INDEX &&
> +                   rename(src, dst) < 0) {
> +                       if (ignore_errors)
> +                               continue;
> +                       die_errno(_("renaming '%s' failed"), src);
> +               }

Then when the mode isn't INDEX, we attempt the rename.

> +
> +               if (submodule_gitfile[i]) {
> +                       if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
> +                           connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||
> +                           update_path_in_gitmodules(src, dst, &err)) {
> +                               if (err.len) {
> +                                       if (ignore_errors) {
> +                                               warning("%s", err.buf);
> +                                               continue;
> +                                       } else
> +                                               die("%s", err.buf);
> +                               }
> +                       } else
> +                               gitmodules_modified = 1;
>                 }

Finally for all modes, we perform the steps for submodules. That makes
sense to me.
>

This version results in a much larger diff, but I think the resulting
code is much easier to follow. The use of the continue allows us to
drop a layer of indentation making the remaining code a bit easier on
the eyes. The patch description doesn't at first glance match the code
change, since now it's a much larger chunk of moved code. However on
inspection, I don't think anything needs to change, as it's just the
conversion to if (show_only) { continue; }

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[Stefan Beller]

On Mon, Apr 18, 2016 at 5:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> A single patch evolves into a series.
>
> Power of code inspection to see bugs that are not reported, perhaps
> ;-)?
>
> I wonder if we can come up with test cases to cover these potential
> issues that are addressed in [1/2]?

I think we can do a chmod a-w on the .git file and then updating that file
fails. (renaming is fine, but putting the new, possibly different link
to the git dir
in there fails.)

I do not quite know what we'd expect from a "git mv -k" on that.
It is documented as "skip move/rename errors", so maybe we need
to undo the rename to actually skip it once an error occurs?

That said I think I know how to write a test for that, but I am unsure
if patch 1 is a good idea.

Thanks,
Stefan


>
> Thanks.
>

Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> ..., but I am unsure
> if patch 1 is a good idea.

Then let's postpone it for now.  I too would like to hear opinion
from other submodule folks, especially Jens, for what 1/2 does
before committing us to the course.

Can you do only the 2/2 on top of maint (or maint-2.6) for now then?