Re: [PATCH v8 05/37] hook: teach hook.runHookDir

On Wed, Mar 24 2021, Emily Shaffer wrote:

On Fri, Mar 12, 2021 at 09:33:46AM +0100, �var Arnfj�r� Bjarmason wrote:

quoted

On Thu, Mar 11 2021, Emily Shaffer wrote:

quoted

+	switch (should_run_hookdir) {
+		case HOOKDIR_NO:

Style: case shouldn't be indented

Done, thanks.

quoted

quoted

+			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
+			break;
+		case HOOKDIR_ERROR:
+			strbuf_addstr(&hookdir_annotation, _(" (will error and not run)"));
+			break;
+		case HOOKDIR_INTERACTIVE:
+			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
+			break;
+		case HOOKDIR_WARN:
+			strbuf_addstr(&hookdir_annotation, _(" (will warn but run)"));
+			break;
+		case HOOKDIR_YES:
+		/*
+		 * The default behavior should agree with
+		 * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
+		 * do the default behavior.
+		 */
+		case HOOKDIR_UNKNOWN:
+		default:
+			break;

We should avoid this sort of translation lego.

quoted

+	}
+
 	list_for_each(pos, head) {
 		struct hook *item = list_entry(pos, struct hook, list);
 		item = list_entry(pos, struct hook, list);
 		if (item) {
 			/* Don't translate 'hookdir' - it matches the config */
-			printf("%s: %s\n",
+			printf("%s: %s%s\n",

native speakers in some languages to read the sentance backwards.
Because if you concatenate strings like this you force.

(We don't currently have a RTL language in po/, still, but let's not
create churn for if/when we do if we can help it)>

Yeah, you are absolutely right. I'll take a look at your suggestion,
thanks.

quoted

I have a patch on top to fix this, will send it as some general reply of
proposed fixup.s

FWIW, I found this format of suggestions really hard to navigate. I had
to go find your fixup (and I think you sent two different ones) and then
had to scroll around and find what you're referring to from here. If I
were to apply the fixup directly, I'd have to split it up and find the
appropriate commits to associate each part of the diff with. I know
generating scissors patches for each review is a pain, but I'd even
prefer a prose "How about printing in each part of the case instead, so
each string can be translated in full" or a non-formatted example inline
to the catchall fixups patch you sent.

Sorry about that. FWIW I did try, but found myself jumping a bit too
much between different commits (as noted in some "maybe squash?"
comments elsewhere), and eventually just gave up and produced some
RFC-just-for-discussion patch on top of the whole thing and mostly
looked at the entire diff forthe series.

quoted

quoted

 			       (item->from_hookdir
+	git hook list pre-commit >actual &&
+	# the hookdir annotation is translated
+	test_i18ncmp expected actual

This (and the rest of test_i18ncmp in this series) can and should just
be "test_cmp" or "test_i18ncmp", the poison mode is dead. See my recent
patches to search/replace test_i18ncmp.

The reason the function isn't gone entirely was to help a series like
yours in "seen", but if we're re-rolling...

Oh cool, thanks, will do.