Re: [PATCH v2 1/2] rm: better error message on failure for multiple files
Mathieu Liénard--Mayor writes: > Well the current code is only using errs=error(...), using the same > variable errs over and over, no matter how many times it loops. > That's why i implemented it similarly. OK, consistency is a good argument then. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] rm: better error message on failure for multiple files
Le 2013-06-10 16:38, Matthieu Moy a écrit :
Mathieu Lienard--Mayor
writes:
When 'git rm' fails, it now displays a single message
with the list of files involved, instead of displaying
a list of messages with one file each.
As an example, the old message:
error: 'foo.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
error: 'bar.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
would now be displayed as:
error: the following files have changes staged in the index:
foo.txt
bar.txt
(use --cached to keep the file, or -f to force removal)
Signed-off-by: Mathieu Lienard--Mayor
Signed-off-by: Jorge Juan Garcia Garcia
Signed-off-by: Matthieu Moy list
There's a "list" after my email, probably a typo.
yes, that's a leftover from a rebase-i
+/*
+ * PRECONDITION: files_list is a non-empty string_list
+ */
Avoid repeating in comments what the code already says. "file_list is
non-empty" is sufficient, we already know it's a string_list.
Okay
+ if (files_staged.nr)
+ errs = print_error_files(&files_staged,
+_("the following files have staged "
+ "content different from both the"
+ "\nfile and the HEAD:"),
+_("\n(use -f to force removal)"));
+ if (files_cached.nr)
+ errs = print_error_files(&files_cached,
+_("the following files have changes "
+ "staged in the index:"),
+_("\n(use --cached to keep the file, "
+ "or -f to force removal)"));
What happens if both conditions are true? It seems the second will
override the first. I think it'd be OK because what matters is that
errs
is set by someone, no matter who, and the error message is displayed
on
screen, not contained in the variable, but this looks weird.
I'd find it more readable with "errs |= print_error_files(...)".
Well the current code is only using errs=error(...), using the same
variable errs over and over, no matter how many times it loops.
That's why i implemented it similarly.
And actually, you may want to move the if (nr) inside
print_error_files (wich could then be called
print_error_files_maybe).
At least, there should be a test where two conditions are true.
I'll do that, to be sure about the behaviour.
+ if (files_submodule.nr)
+ errs = print_error_files(&files_submodule,
+_("the following submodules (or one "
+ "of its nested submodule) use a "
+ ".git directory:"),
+_("\n(use 'rm -rf' if you really "
+ "want to remove i including all "
i -> it
?
--
Mathieu Liénard--Mayor,
2nd year at Grenoble INP - ENSIMAG
(+33)6 80 56 30 02
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] rm: better error message on failure for multiple files
Mathieu Lienard--Mayor writes:
> When 'git rm' fails, it now displays a single message
> with the list of files involved, instead of displaying
> a list of messages with one file each.
>
> As an example, the old message:
> error: 'foo.txt' has changes staged in the index
> (use --cached to keep the file, or -f to force removal)
> error: 'bar.txt' has changes staged in the index
> (use --cached to keep the file, or -f to force removal)
>
> would now be displayed as:
> error: the following files have changes staged in the index:
> foo.txt
> bar.txt
> (use --cached to keep the file, or -f to force removal)
>
> Signed-off-by: Mathieu Lienard--Mayor
> Signed-off-by: Jorge Juan Garcia Garcia
>
> Signed-off-by: Matthieu Moy list
There's a "list" after my email, probably a typo.
> +/*
> + * PRECONDITION: files_list is a non-empty string_list
> + */
Avoid repeating in comments what the code already says. "file_list is
non-empty" is sufficient, we already know it's a string_list.
> + if (files_staged.nr)
> + errs = print_error_files(&files_staged,
> + _("the following files have staged "
> +"content different from both the"
> +"\nfile and the HEAD:"),
> + _("\n(use -f to force removal)"));
> + if (files_cached.nr)
> + errs = print_error_files(&files_cached,
> + _("the following files have changes "
> +"staged in the index:"),
> + _("\n(use --cached to keep the file, "
> +"or -f to force removal)"));
What happens if both conditions are true? It seems the second will
override the first. I think it'd be OK because what matters is that errs
is set by someone, no matter who, and the error message is displayed on
screen, not contained in the variable, but this looks weird.
I'd find it more readable with "errs |= print_error_files(...)".
And actually, you may want to move the if (nr) inside
print_error_files (wich could then be called print_error_files_maybe).
At least, there should be a test where two conditions are true.
> + if (files_submodule.nr)
> + errs = print_error_files(&files_submodule,
> + _("the following submodules (or one "
> +"of its nested submodule) use a "
> +".git directory:"),
> + _("\n(use 'rm -rf' if you really "
> +"want to remove i including all "
i -> it
?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] rm: better error message on failure for multiple files
When 'git rm' fails, it now displays a single message
with the list of files involved, instead of displaying
a list of messages with one file each.
As an example, the old message:
error: 'foo.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
error: 'bar.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
would now be displayed as:
error: the following files have changes staged in the index:
foo.txt
bar.txt
(use --cached to keep the file, or -f to force removal)
Signed-off-by: Mathieu Lienard--Mayor
Signed-off-by: Jorge Juan Garcia Garcia
Signed-off-by: Matthieu Moy
---
Changes since v1:
-introduction of print_error_files() to avoid repetitions
-implementation with string_list instead of strbuf
-typo "fileand" switched to "file and"
builtin/rm.c | 74 ++--
t/t3600-rm.sh | 48 +
2 files changed, 108 insertions(+), 14 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..76dfc5b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -70,6 +70,27 @@ static int check_submodules_use_gitfiles(void)
return errs;
}
+/*
+ * PRECONDITION: files_list is a non-empty string_list
+ */
+static int print_error_files(struct string_list *files_list,
+const char *main_msg,
+const char *hints_msg)
+{
+ int errs = 0;
+ 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);
+
+ return errs;
+}
+
static int check_local_mod(unsigned char *head, int index_only)
{
/*
@@ -82,6 +103,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;
+
no_head = is_null_sha1(head);
for (i = 0; i < list.nr; i++) {
struct stat st;
@@ -171,29 +197,49 @@ static int check_local_mod(unsigned char *head, int
index_only)
*/
if (local_changes && staged_changes) {
if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))
- errs = error(_("'%s' has staged content
different "
-"from both the file and the HEAD\n"
-"(use -f to force removal)"),
name);
+ string_list_append(&files_staged, name);
}
else if (!index_only) {
if (staged_changes)
- errs = error(_("'%s' has changes staged in the
index\n"
-"(use --cached to keep the file, "
-"or -f to force removal)"), name);
+ string_list_append(&files_cached, name);
if (local_changes) {
if (S_ISGITLINK(ce->ce_mode) &&
!submodule_uses_gitfile(name)) {
- errs = error(_("submodule '%s' (or one
of its nested "
-"submodules) uses a .git
directory\n"
-"(use 'rm -rf' if you
really want to remove "
-"it including all of its
history)"), name);
- } else
- errs = error(_("'%s' has local
modifications\n"
-"(use --cached to keep the
file, "
-"or -f to force
removal)"), name);
+ string_list_append(&files_submodule,
+ name);
+ } else {
+ string_list_append(&files_local, name);
+ }
}
}
}
+ if (files_staged.nr)
+ errs = print_error_files(&files_staged,
+_("the following files have staged "
+ "content different from both the"
+
