[PATCH 0/8] builtin/mv.c cleanup

[PATCH 0/8] builtin/mv.c cleanup

[Nguyễn Thái Ngọc Duy]

I was looking at builtin/mv.c for pathspec support and ended up
cleaning it up a bit. The first patch is definitely good. The rest
could be questionable. Although the output in the end looks better in
my opinion.

Nguyễn Thái Ngọc Duy (8):
  mv: mark strings for translations
  mv: no "Huh?" to the user
  mv: flatten error handling code block
  mv: split submodule move preparation code out
  mv: remove an "if" that's always true
  mv: move index search code out
  mv: unindent one level for directory move code
  mv: combine two if(s)

 builtin/mv.c | 168 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 84 insertions(+), 84 deletions(-)

-- 
2.1.0.rc0.78.gc0d8480

[PATCH 1/8] mv: mark strings for translations

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 6ffe540..b892f63 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -108,7 +108,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	} else {
 		if (argc != 1)
-			die("destination '%s' is not a directory", dest_path[0]);
+			die(_("destination '%s' is not a directory"), dest_path[0]);
 		destination = dest_path;
 	}
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 2/8] mv: no "Huh?" to the user

[Nguyễn Thái Ngọc Duy]

Although if we are frisky, this could do

   static NORETURN void die_builtin(const char *err, va_list params)
   {
  -	vreportf("fatal: ", err, params);
  +	vreportf("Huh? ", err, params);
   	exit(128);
   }

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index b892f63..a7e02c0 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -135,7 +135,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			if (first >= 0) {
 				struct strbuf submodule_dotgit = STRBUF_INIT;
 				if (!S_ISGITLINK(active_cache[first]->ce_mode))
-					die (_("Huh? Directory %s is in index and no submodule?"), src);
+					die (_("Directory %s is in index and no submodule?"), src);
 				if (!is_staging_gitmodules_ok())
 					die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
 				strbuf_addf(&submodule_dotgit, "%s/.git", src);
@@ -153,8 +153,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 				first = cache_name_pos(src_w_slash, len_w_slash);
 				if (first >= 0)
-					die (_("Huh? %.*s is in index?"),
-							len_w_slash, src_w_slash);
+					die (_("%.*s is in index"), len_w_slash, src_w_slash);
 
 				first = -1 - first;
 				for (last = first; last < active_nr; last++) {
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 3/8] mv: flatten error handling code block

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a7e02c0..5c6f58f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -58,6 +58,11 @@ static const char *add_slash(const char *path)
 	return path;
 }
 
+static void move_up_one(void *p, int nmemb, int size)
+{
+	memmove(p, (char*)p + size, nmemb * size);
+}
+
 static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
@@ -224,24 +229,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		else
 			string_list_insert(&src_for_dst, dst);
 
-		if (bad) {
-			if (ignore_errors) {
-				if (--argc > 0) {
-					memmove(source + i, source + i + 1,
-						(argc - i) * sizeof(char *));
-					memmove(destination + i,
-						destination + i + 1,
-						(argc - i) * sizeof(char *));
-					memmove(modes + i, modes + i + 1,
-						(argc - i) * sizeof(enum update_mode));
-					memmove(submodule_gitfile + i,
-						submodule_gitfile + i + 1,
-						(argc - i) * sizeof(char *));
-					i--;
-				}
-			} else
-				die (_("%s, source=%s, destination=%s"),
-				     bad, src, dst);
+		if (!bad)
+			continue;
+		if (!ignore_errors)
+			die (_("%s, source=%s, destination=%s"),
+			     bad, src, dst);
+		if (--argc > 0) {
+			int n = argc - i;
+			move_up_one(source + i, n, sizeof(*source));
+			move_up_one(destination + i, n, sizeof(*destination));
+			move_up_one(modes + i, n, sizeof(*modes));
+			move_up_one(submodule_gitfile + i, n, sizeof(*submodule_gitfile));
+			i--;
 		}
 	}
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 4/8] mv: split submodule move preparation code out

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 5c6f58f..e192f2d 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -66,6 +66,23 @@ static void move_up_one(void *p, int nmemb, int size)
 static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
+static void prepare_move_submodule(const char *src, int first,
+				   const char **submodule_gitfile)
+{
+	struct strbuf submodule_dotgit = STRBUF_INIT;
+	if (!S_ISGITLINK(active_cache[first]->ce_mode))
+		die (_("Directory %s is in index and no submodule?"), src);
+	if (!is_staging_gitmodules_ok())
+		die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
+	strbuf_addf(&submodule_dotgit, "%s/.git", src);
+	*submodule_gitfile = read_gitfile(submodule_dotgit.buf);
+	if (*submodule_gitfile)
+		*submodule_gitfile = xstrdup(*submodule_gitfile);
+	else
+		*submodule_gitfile = SUBMODULE_WITH_GITDIR;
+	strbuf_release(&submodule_dotgit);
+}
+
 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
 	int i, gitmodules_modified = 0;
@@ -137,20 +154,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			bad = _("cannot move directory over file");
 		else if (src_is_dir) {
 			int first = cache_name_pos(src, length);
-			if (first >= 0) {
-				struct strbuf submodule_dotgit = STRBUF_INIT;
-				if (!S_ISGITLINK(active_cache[first]->ce_mode))
-					die (_("Directory %s is in index and no submodule?"), src);
-				if (!is_staging_gitmodules_ok())
-					die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
-				strbuf_addf(&submodule_dotgit, "%s/.git", src);
-				submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);
-				if (submodule_gitfile[i])
-					submodule_gitfile[i] = xstrdup(submodule_gitfile[i]);
-				else
-					submodule_gitfile[i] = SUBMODULE_WITH_GITDIR;
-				strbuf_release(&submodule_dotgit);
-			} else {
+			if (first >= 0)
+				prepare_move_submodule(src, first,
+						       submodule_gitfile + i);
+			else {
 				const char *src_w_slash = add_slash(src);
 				int last, len_w_slash = length + 1;
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 5/8] mv: remove an "if" that's always true

[Nguyễn Thái Ngọc Duy]

This is inside an "else" block of "if (last - first < 1)", so we know
that "last - first >= 1" when we come here. No need to check
"last - first > 0".

While at there, save "argc + last - first" to a variable to shorten
the statements a bit.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index e192f2d..a45226e 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -179,22 +179,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				if (last - first < 1)
 					bad = _("source directory is empty");
 				else {
-					int j, dst_len;
+					int j, dst_len, n;
 
-					if (last - first > 0) {
-						source = xrealloc(source,
-								(argc + last - first)
-								* sizeof(char *));
-						destination = xrealloc(destination,
-								(argc + last - first)
-								* sizeof(char *));
-						modes = xrealloc(modes,
-								(argc + last - first)
-								* sizeof(enum update_mode));
-						submodule_gitfile = xrealloc(submodule_gitfile,
-								(argc + last - first)
-								* sizeof(char *));
-					}
+					n = argc + last - first;
+					source = xrealloc(source, n * sizeof(char *));
+					destination = xrealloc(destination, n * sizeof(char *));
+					modes = xrealloc(modes, n * sizeof(enum update_mode));
+					submodule_gitfile =
+						xrealloc(submodule_gitfile, n * sizeof(char *));
 
 					dst = add_slash(dst);
 					dst_len = strlen(dst);
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 6/8] mv: move index search code out

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a45226e..f8d65e2 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -83,6 +83,29 @@ static void prepare_move_submodule(const char *src, int first,
 	strbuf_release(&submodule_dotgit);
 }
 
+static int index_range_of_same_dir(const char *src, int length,
+				   int *first_p, int *last_p)
+{
+	const char *src_w_slash = add_slash(src);
+	int first, last, len_w_slash = length + 1;
+
+	first = cache_name_pos(src_w_slash, len_w_slash);
+	if (first >= 0)
+		die (_("%.*s is in index"), len_w_slash, src_w_slash);
+
+	first = -1 - first;
+	for (last = first; last < active_nr; last++) {
+		const char *path = active_cache[last]->name;
+		if (strncmp(path, src_w_slash, len_w_slash))
+			break;
+	}
+	if (src_w_slash != src)
+		free((char *)src_w_slash);
+	*first_p = first;
+	*last_p = last;
+	return last - first;
+}
+
 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
 	int i, gitmodules_modified = 0;
@@ -158,24 +181,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				prepare_move_submodule(src, first,
 						       submodule_gitfile + i);
 			else {
-				const char *src_w_slash = add_slash(src);
-				int last, len_w_slash = length + 1;
+				int last;
 
 				modes[i] = WORKING_DIRECTORY;
-
-				first = cache_name_pos(src_w_slash, len_w_slash);
-				if (first >= 0)
-					die (_("%.*s is in index"), len_w_slash, src_w_slash);
-
-				first = -1 - first;
-				for (last = first; last < active_nr; last++) {
-					const char *path = active_cache[last]->name;
-					if (strncmp(path, src_w_slash, len_w_slash))
-						break;
-				}
-				if (src_w_slash != src)
-					free((char *)src_w_slash);
-
+				index_range_of_same_dir(src, length, &first, &last);
 				if (last - first < 1)
 					bad = _("source directory is empty");
 				else {
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 7/8] mv: unindent one level for directory move code

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index f8d65e2..a2e33b5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -176,42 +176,37 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				&& lstat(dst, &st) == 0)
 			bad = _("cannot move directory over file");
 		else if (src_is_dir) {
-			int first = cache_name_pos(src, length);
+			int first = cache_name_pos(src, length), last;
 			if (first >= 0)
 				prepare_move_submodule(src, first,
 						       submodule_gitfile + i);
-			else {
-				int last;
-
+			else if (index_range_of_same_dir(src, length,
+							 &first, &last) < 1) {
 				modes[i] = WORKING_DIRECTORY;
-				index_range_of_same_dir(src, length, &first, &last);
 				if (last - first < 1)
 					bad = _("source directory is empty");
-				else {
-					int j, dst_len, n;
+			} else { /* last - first >= 1 */
+				int j, dst_len, n;
 
-					n = argc + last - first;
-					source = xrealloc(source, n * sizeof(char *));
-					destination = xrealloc(destination, n * sizeof(char *));
-					modes = xrealloc(modes, n * sizeof(enum update_mode));
-					submodule_gitfile =
-						xrealloc(submodule_gitfile, n * sizeof(char *));
+				modes[i] = WORKING_DIRECTORY;
+				n = argc + last - first;
+				source = xrealloc(source, n * sizeof(char *));
+				destination = xrealloc(destination, n * sizeof(char *));
+				modes = xrealloc(modes, n * sizeof(enum update_mode));
+				submodule_gitfile = xrealloc(submodule_gitfile, n * sizeof(char *));
 
-					dst = add_slash(dst);
-					dst_len = strlen(dst);
+				dst = add_slash(dst);
+				dst_len = strlen(dst);
 
-					for (j = 0; j < last - first; j++) {
-						const char *path =
-							active_cache[first + j]->name;
-						source[argc + j] = path;
-						destination[argc + j] =
-							prefix_path(dst, dst_len,
-								path + length + 1);
-						modes[argc + j] = INDEX;
-						submodule_gitfile[argc + j] = NULL;
-					}
-					argc += last - first;
+				for (j = 0; j < last - first; j++) {
+					const char *path = active_cache[first + j]->name;
+					source[argc + j] = path;
+					destination[argc + j] =
+						prefix_path(dst, dst_len, path + length + 1);
+					modes[argc + j] = INDEX;
+					submodule_gitfile[argc + j] = NULL;
 				}
+				argc += last - first;
 			}
 		} else if (cache_name_pos(src, length) < 0)
 			bad = _("not under version control");
-- 
2.1.0.rc0.78.gc0d8480

[PATCH 8/8] mv: combine two if(s)

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a2e33b5..4eb420b 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -276,10 +276,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (gitmodules_modified)
 		stage_updated_gitmodules();
 
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+	if (active_cache_changed &&
+	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die(_("Unable to write new index file"));
-	}
 
 	return 0;
 }
-- 
2.1.0.rc0.78.gc0d8480

Re: [PATCH 2/8] mv: no "Huh?" to the user

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Although if we are frisky, this could do
>
>    static NORETURN void die_builtin(const char *err, va_list params)
>    {
>   -	vreportf("fatal: ", err, params);
>   +	vreportf("Huh? ", err, params);
>    	exit(128);
>    }

;-)

While at it we may want to remove the extra SP between dies and
their opening parentheses.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/mv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index b892f63..a7e02c0 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -135,7 +135,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			if (first >= 0) {
>  				struct strbuf submodule_dotgit = STRBUF_INIT;
>  				if (!S_ISGITLINK(active_cache[first]->ce_mode))
> -					die (_("Huh? Directory %s is in index and no submodule?"), src);
> +					die (_("Directory %s is in index and no submodule?"), src);
>  				if (!is_staging_gitmodules_ok())
>  					die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
>  				strbuf_addf(&submodule_dotgit, "%s/.git", src);
> @@ -153,8 +153,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  				first = cache_name_pos(src_w_slash, len_w_slash);
>  				if (first >= 0)
> -					die (_("Huh? %.*s is in index?"),
> -							len_w_slash, src_w_slash);
> +					die (_("%.*s is in index"), len_w_slash, src_w_slash);
>  
>  				first = -1 - first;
>  				for (last = first; last < active_nr; last++) {

Re: [PATCH 3/8] mv: flatten error handling code block

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/mv.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index a7e02c0..5c6f58f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -58,6 +58,11 @@ static const char *add_slash(const char *path)
>  	return path;
>  }
>  
> +static void move_up_one(void *p, int nmemb, int size)
> +{
> +	memmove(p, (char*)p + size, nmemb * size);
> +}
> +
>  static struct lock_file lock_file;
>  #define SUBMODULE_WITH_GITDIR ((const char *)1)
>  
> @@ -224,24 +229,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		else
>  			string_list_insert(&src_for_dst, dst);
>  
> -		if (bad) {
> -			if (ignore_errors) {
> -				if (--argc > 0) {
> -					memmove(source + i, source + i + 1,
> -						(argc - i) * sizeof(char *));
> -					memmove(destination + i,
> -						destination + i + 1,
> -						(argc - i) * sizeof(char *));
> -					memmove(modes + i, modes + i + 1,
> -						(argc - i) * sizeof(enum update_mode));
> -					memmove(submodule_gitfile + i,
> -						submodule_gitfile + i + 1,
> -						(argc - i) * sizeof(char *));
> -					i--;
> -				}
> -			} else
> -				die (_("%s, source=%s, destination=%s"),
> -				     bad, src, dst);
> +		if (!bad)
> +			continue;
> +		if (!ignore_errors)
> +			die (_("%s, source=%s, destination=%s"),
> +			     bad, src, dst);
> +		if (--argc > 0) {
> +			int n = argc - i;
> +			move_up_one(source + i, n, sizeof(*source));
> +			move_up_one(destination + i, n, sizeof(*destination));
> +			move_up_one(modes + i, n, sizeof(*modes));
> +			move_up_one(submodule_gitfile + i, n, sizeof(*submodule_gitfile));
> +			i--;

The resulting end-of-loop code structure certainly looks a lot
better, even if the original memmove()s were left inline without the
helper.

The helper itself however looks a bit half-hearted.  It may be more
appropriate to go one step further to have a macro whose use looks
like this, perhaps, to avoid the last remaining repetition?

	MOVE_UP_BY_ONE(source, i, n);


>  		}
>  	}