Subject: Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

From: Dennis Kaarsemaker <[email-masked]>
Date: Thu, 01 Sep 2016 18:19:09 +0200

On do, 2016-09-01 at 17:17 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> > 
> > > 
> > > +static int is_fixup(enum todo_command command)
> > > +{
> > > +	return command == TODO_FIXUP || command == TODO_SQUASH;
> > > +}
> > It sounds wrong to have a function named is_fixup return true when
> > the
> > command isn't a fixup but a squash. Maybe name it
> > changes_previous_commit or something?
> I can see how that may sound confusing, unless you understand that a
> squash is a fixup that lets the user edit the commit message, too. So
> essentially squash = fixup + edit, if you will.
> 
> Maybe the name is more appropriate in that light?

Kinda makes sense. It's not how I use fixup/squash as a user of rebase
-i though. But we can't go there, that's bikeshed country :)

> > > +static const char *nth_for_number(int n)
> > > +{
> > > +	int n1 = n % 10, n10 = n % 100;
> > > +
> > > +	if (n1 == 1 && n10 != 11)
> > > +		return "st";
> > > +	if (n1 == 2 && n10 != 12)
> > > +		return "nd";
> > > +	if (n1 == 3 && n10 != 13)
> > > +		return "rd";
> > > +	return "th";
> > > +}
> > > 
> > > 8---
> > > 
> > > +	if (command == TODO_SQUASH) {
> > > +		unlink(rebase_path_fixup_msg());
> > > +		strbuf_addf(&buf, "\n%c This is the %d%s commit
> > > message:\n\n%s",
> > > +			comment_line_char,
> > > +			count, nth_for_number(count), body);
> > > +	}
> > > +	else if (command == TODO_FIXUP) {
> > > +		strbuf_addf(&buf,
> > > +			"\n%c The %d%s commit message will be
> > > skipped:\n\n",
> > > +			comment_line_char, count,
> > > nth_for_number(count));
> > > +		strbuf_add_commented_lines(&buf, body,
> > > strlen(body));
> > > +	}
> > This way of handling numbers is not translatable, and I really
> > think we
> > should mark these strings for translation, like they are in the .sh
> > version.
> Ah, this is the risk of working on something as big as rebase
> --helper.
> Back when I started with it, the relevant code in git-rebase
> --interactive
> read like this:
> 
> 	nth_string () {
> 		case "$1" in
> 		*1[0-9]|*[04-9]) echo "$1"th;;
> 		*1) echo "$1"st;;
> 		*2) echo "$1"nd;;
> 		*3) echo "$1"rd;;
> 		esac
> 	}
> 
> I merely did a faithful translation of that...
> 
> Now, I see that git-rebase--interactive was switched to use
> eval_gettext,
> which in turn is handled in git-sh-i18n whose code is quite
> convoluted. In
> the absence of gettext, it uses git-sh-i18n--envsubst, which has no C
> API
> whatsoever.
> 
> And I see that the beautiful ordinal computation was given up in
> favor of
> a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd"
> etc).
> 
> In any case, translation is not my main concern until v2.10.0, so
> I'll
> take care of this after that release.

Hmm, not sure if I agree with that. I'd see it as a regression to lose
the i18n there.

D.