Re: [PATCH v7 01/10] Add support for -i/--interactive to git-clean

Jiang Xin [off-list ref] writes:

+			putchar('\n');
+
+			/* Display dels in "Would remove ..." format */
+			for_each_string_list_item(item, &del_list) {
+				qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
+				printf(_(msg_would_remove), qname);
+			}
+			putchar('\n');

+		putchar('\n');
+
+		/* Display dels in "Would remove ..." format */
+		for_each_string_list_item(item, &del_list) {
+			qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
+			printf(_(msg_would_remove), qname);
+		}
+		putchar('\n');

These two pieces of code are surprisingly similar ... Shouldn't they be
factored into a small helper function?

+		/* Confirmation dialog */
+		printf(_("Remove ([y]es/[n]o/[e]dit) ? "));

To be more consistent with "git add -p", this should use [] instead of
(), and have no space before "?".

+			die(_("clean.requireForce defaults to true and neither -i, -n nor -f given; "
 				  "refusing to clean"));

That makes it a 85 characters message, and we usually break lines before
80. Adding \n after ";" (instead of a space) would be better IMO.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

2013/5/13 Matthieu Moy [off-list ref]:

quoted

+             /* Confirmation dialog */
+             printf(_("Remove ([y]es/[n]o/[e]dit) ? "));

To be more consistent with "git add -p", this should use [] instead of
(), and have no space before "?".

Will be replaced with:

             printf(_("Remove [y/n]? "));

quoted

+                     die(_("clean.requireForce defaults to true and neither -i, -n nor -f given; "
                                "refusing to clean"));

That makes it a 85 characters message, and we usually break lines before
80. Adding \n after ";" (instead of a space) would be better IMO.

If die with multiple lines, it looks ugly. E.g.

    % git clean
    fatal: clean.requireForce defaults to true and neither -i, -n nor -f given;
    refusing to clean

I think because of this, some die messages are longer than 80 characters.
Such as:

    # builtin/apply.c:1496

    die(Q_("git diff header lacks filename information when removing "
                "%d leading pathname component (line %d)",
                "git diff header lacks filename information when removing "
                "%d leading pathname components (line %d)",

-- 
Jiang Xin
http://www.worldhello.net/