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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Mar 24, 2021, 18:46 UTC
Message-ID: <YFuJGG1MUokL4o4i@google.com>
In-Reply-To: <87im5whitx.fsf@evledraar.gmail.com>

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

Show 8 quoted lines

> 
> 
> On Thu, Mar 11 2021, Emily Shaffer wrote:
> 
> > +	switch (should_run_hookdir) {
> > +		case HOOKDIR_NO:
> 
> Style: case shouldn't be indented

Done, thanks.

Show 39 quoted lines

> 
> > +			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.
> 
> > +	}
> > +
> >  	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.

> 
> 
> 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.

Show 12 quoted lines

> 
> >  			       (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.