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

[Junio C Hamano <gitster@pobox.com>]

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


>  		}
>  	}