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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 24, 2021, 22:38 UTC
Message-ID: <87o8f8p48b.fsf@evledraar.gmail.com>
In-Reply-To: <YFuJGG1MUokL4o4i@google.com>

On Wed, Mar 24 2021, Emily Shaffer wrote:

Show 69 quoted lines

> On Fri, Mar 12, 2021 at 09:33:46AM +0100, ï¿½var Arnfjï¿½rï¿½ Bjarmason wrote:
>> 
>> 
>> On Thu, Mar 11 2021, Emily Shaffer wrote:
>> 
>> > +	switch (should_run_hookdir) {
>> > +		case HOOKDIR_NO:
>> 
>> Style: case shouldn't be indented
>
> Done, thanks.
>
>> 
>> > +			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.

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.

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