Re: [PATCH v4 1/2] rm: better error message on failure for multiple files

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

Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>
writes:

> +static void print_error_files(struct string_list *files_list,
> +			      const char *main_msg,
> +			      const char *hints_msg,
> +			      int *errs)
> +{
> +	if (files_list->nr) {
> +		struct strbuf err_msg = STRBUF_INIT;
> +		int i;
> +		strbuf_addstr(&err_msg, main_msg);
> +		for (i = 0; i < files_list->nr; i++)
> +			strbuf_addf(&err_msg,
> +				    "\n    %s",
> +				    files_list->items[i].string);
> +		strbuf_addstr(&err_msg, hints_msg);
> +		*errs = error("%s", err_msg.buf);
> +		strbuf_release(&err_msg);
> +	}
> +}
> +
>  static int check_submodules_use_gitfiles(void)
>  {
>  	int i;
>  	int errs = 0;
>  
> +	struct string_list files = STRING_LIST_INIT_NODUP;
> +
>  	for (i = 0; i < list.nr; i++) {

The blank after the initialization lines before the first statement
is very much welcom, but please drop the blank line before this new
initialization, i.e.

	int i;
        int errs = 0;
        struct string_list files = STRING_LIST_INIT_NODUP;

	for (i = 0; i < list.nr; i++) {
		...

> @@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)
>  			continue;
>  
>  		if (!submodule_uses_gitfile(name))
> +			string_list_append(&files, name);
>  	}
> +	print_error_files(&files,
> +			  Q_("the following submodule (or one of its nested "
> +			     "submodules)\n uses a .git directory:",
> +			     "the following submodules (or one of its nested "
> +			     "submodules)\n use a .git directory:",
> +			     files.nr),
> +			  _("\n(use 'rm -rf' if you really want to remove "
> +			    "it including all of its history)"),
> +			  &errs);
>  
>  	return errs;

No string_list_clear() on files?

>  }
> @@ -82,6 +109,11 @@ static int check_local_mod(unsigned char *head, int index_only)
>  	int i, no_head;
>  	int errs = 0;
>  
> +	struct string_list files_staged = STRING_LIST_INIT_NODUP;
> +	struct string_list files_cached = STRING_LIST_INIT_NODUP;
> +	struct string_list files_submodule = STRING_LIST_INIT_NODUP;
> +	struct string_list files_local = STRING_LIST_INIT_NODUP;
> +
> ...
> +	print_error_files(&files_local,
> +			  Q_("the following file has local modifications:",
> +			     "the following files have local modifications:",
> +			     files_local.nr),
> +			  _("\n(use --cached to keep the file,"
> +			    " or -f to force removal)"),
> +			  &errs);
> +

No string_list_clear() on files_*?