[PATCH] For Real - Fixed pluralization in diff reports

[PATCH] For Real - Fixed pluralization in diff reports

[Jon Forrest]

[I must have accidentally removed the "s" in "deletions" before.
I just rebuilt everything and remade the patch. All looks well
this time. This is my first submitted patch to anything
so my fingers are still learning.]

I got irritated by the

      1 files changed, 0 insertions(+), 1 deletions(-)

lack of pluralization so I fixed it. Now it says

      1 file changed, 0 insertions(+), 1 deletion(-)

and so forth.

Signed-off-by: Jon Forrest <nobozo@gmail.com>
---
  diff.c |   10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 93ef9a2..a179b24 100644
--- a/diff.c
+++ b/diff.c
@@ -1465,8 +1465,9 @@ static void show_stats(struct diffstat_t *data, 
struct diff_options *options)
  	}
  	fprintf(options->file, "%s", line_prefix);
  	fprintf(options->file,
-	       " %d files changed, %d insertions(+), %d deletions(-)\n",
-	       total_files, adds, dels);
+	       " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
+	       total_files, total_files == 1 ? "" : "s", adds, adds == 1 ? "" 
: "s", dels,
+		dels == 1 ? "" : "s");
  }

  static void show_shortstats(struct diffstat_t *data, struct 
diff_options *options)
@@ -1496,8 +1497,9 @@ static void show_shortstats(struct diffstat_t 
*data, struct diff_options *option
  				options->output_prefix_data);
  		fprintf(options->file, "%s", msg->buf);
  	}
-	fprintf(options->file, " %d files changed, %d insertions(+), %d 
deletions(-)\n",
-	       total_files, adds, dels);
+	fprintf(options->file, " %d file%s changed, %d insertion%s(+), %d 
deletion%s(-)\n",
+	       total_files, total_files == 1 ? "" : "s", adds, adds == 1 ? "" 
: "s", dels,
+		dels == 1 ? "" : "s");
  }

  static void show_numstat(struct diffstat_t *data, struct diff_options 
*options)
-- 1.7.6.351.gb35ac.dirty

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Nguyen Thai Ngoc Duy]

On Mon, Aug 1, 2011 at 11:46 AM, Jon Forrest <nobozo@gmail.com> wrote:
> [I must have accidentally removed the "s" in "deletions" before.
> I just rebuilt everything and remade the patch. All looks well
> this time. This is my first submitted patch to anything
> so my fingers are still learning.]
>
> I got irritated by the
>
>     1 files changed, 0 insertions(+), 1 deletions(-)
>
> lack of pluralization so I fixed it. Now it says
>
>     1 file changed, 0 insertions(+), 1 deletion(-)
>
> and so forth.

Are you sure this does not break any tests? t3508.2 for example
hardcodes "1 insertions" and does textual compare. I have not run the
tests but I suspect it will fail.
-- 
Duy

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jon Forrest]

On 7/31/2011 9:50 PM, Nguyen Thai Ngoc Duy wrote:

> Are you sure this does not break any tests? t3508.2 for example
> hardcodes "1 insertions" and does textual compare. I have not run the
> tests but I suspect it will fail.

I ran the tests. The only result in red that I saw that
didn't appear to be caused "by breakage" was

not ok - 3 mktemp to unwritable directory prints filename

Since I didn't do anything relating to mktemp I thought
this was probably bogus.

I'm new to this so I don't know how to find the t3508.2.
If you can give me a hint I'd be glad to look at this in
more detail.

Jon

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Nguyen Thai Ngoc Duy]

On Mon, Aug 1, 2011 at 11:57 AM, Jon Forrest <nobozo@gmail.com> wrote:
> On 7/31/2011 9:50 PM, Nguyen Thai Ngoc Duy wrote:
>
>> Are you sure this does not break any tests? t3508.2 for example
>> hardcodes "1 insertions" and does textual compare. I have not run the
>> tests but I suspect it will fail.
>
> I ran the tests. The only result in red that I saw that
> didn't appear to be caused "by breakage" was
>
> not ok - 3 mktemp to unwritable directory prints filename
>
> Since I didn't do anything relating to mktemp I thought
> this was probably bogus.
>
> I'm new to this so I don't know how to find the t3508.2.
> If you can give me a hint I'd be glad to look at this in
> more detail.

Your patch is line-wrapped. Please see
Documentation/SubmittingPatches, it may help.

I manually applied your patch and run t3508 alone. t3508.2 is the
second test in t3502, by the way. t/README gives more information on
tests of git.

pclouds@do ~/w/git/t $ ./t3508-cherry-pick-many-commits.sh  -v
Initialized empty Git repository in /home/pclouds/w/git/t/trash
directory.t3508-cherry-pick-many-commits/.git/
expecting success:
	echo first > file1 &&
	git add file1 &&
	test_tick &&
	git commit -m "first" &&
	git tag first &&

	git checkout -b other &&
	for val in second third fourth
	do
		echo $val >> file1 &&
		git add file1 &&
		test_tick &&
		git commit -m "$val" &&
		git tag $val
	done

[master (root-commit) 0c72e4f] first
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
 create mode 100644 file1
[other 453a047] second
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[other e85abe2] third
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[other 94d3184] fourth
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
ok 1 - setup

expecting success:
	cat <<-\EOF >expected &&
	[master OBJID] second
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	[master OBJID] third
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	[master OBJID] fourth
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	EOF

	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git cherry-pick first..fourth >actual &&
	git diff --quiet other &&
	git diff --quiet HEAD other &&

	sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy &&
	test_cmp expected actual.fuzzy &&
	check_head_differs_from fourth

HEAD is now at 0c72e4f first
--- expected	2011-08-01 05:13:03.000000000 +0000
+++ actual.fuzzy	2011-08-01 05:13:04.000000000 +0000
@@ -1,9 +1,9 @@
 [master OBJID] second
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
 [master OBJID] third
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
 [master OBJID] fourth
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
not ok - 2 cherry-pick first..fourth works
#	
#		cat <<-\EOF >expected &&
#		[master OBJID] second
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		[master OBJID] third
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		[master OBJID] fourth
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		EOF
#	
#		git checkout -f master &&
#		git reset --hard first &&
#		test_tick &&
#		git cherry-pick first..fourth >actual &&
#		git diff --quiet other &&
#		git diff --quiet HEAD other &&
#	
#		sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy &&
#		test_cmp expected actual.fuzzy &&
#		check_head_differs_from fourth
#	

expecting success:
	cat <<-\EOF >expected &&
	Trying simple merge.
	[master OBJID] second
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	Trying simple merge.
	[master OBJID] third
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	Trying simple merge.
	[master OBJID] fourth
	 Author: A U Thor <author@example.com>
	 1 files changed, 1 insertions(+), 0 deletions(-)
	EOF

	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git cherry-pick --strategy resolve first..fourth >actual &&
	git diff --quiet other &&
	git diff --quiet HEAD other &&
	sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy &&
	test_cmp expected actual.fuzzy &&
	check_head_differs_from fourth

HEAD is now at 0c72e4f first
--- expected	2011-08-01 05:13:04.000000000 +0000
+++ actual.fuzzy	2011-08-01 05:13:04.000000000 +0000
@@ -1,12 +1,12 @@
 Trying simple merge.
 [master OBJID] second
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
 Trying simple merge.
 [master OBJID] third
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
 Trying simple merge.
 [master OBJID] fourth
  Author: A U Thor <author@example.com>
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+), 0 deletions(-)
not ok - 3 cherry-pick --strategy resolve first..fourth works
#	
#		cat <<-\EOF >expected &&
#		Trying simple merge.
#		[master OBJID] second
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		Trying simple merge.
#		[master OBJID] third
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		Trying simple merge.
#		[master OBJID] fourth
#		 Author: A U Thor <author@example.com>
#		 1 files changed, 1 insertions(+), 0 deletions(-)
#		EOF
#	
#		git checkout -f master &&
#		git reset --hard first &&
#		test_tick &&
#		git cherry-pick --strategy resolve first..fourth >actual &&
#		git diff --quiet other &&
#		git diff --quiet HEAD other &&
#		sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy &&
#		test_cmp expected actual.fuzzy &&
#		check_head_differs_from fourth
#	

expecting success:
	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git cherry-pick --ff first..fourth &&
	git diff --quiet other &&
	git diff --quiet HEAD other &&
	check_head_equals fourth

HEAD is now at 0c72e4f first
ok 4 - cherry-pick --ff first..fourth works

expecting success:
	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git cherry-pick -n first..fourth &&
	git diff --quiet other &&
	git diff --cached --quiet other &&
	git diff --quiet HEAD first

HEAD is now at 0c72e4f first
ok 5 - cherry-pick -n first..fourth works

expecting success:
	git checkout -f master &&
	git reset --hard fourth &&
	test_tick &&
	git revert first..fourth &&
	git diff --quiet first &&
	git diff --cached --quiet first &&
	git diff --quiet HEAD first

HEAD is now at 94d3184 fourth
[master c20509a] Revert "fourth"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 5c15a57] Revert "third"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 76d1a2e] Revert "second"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
ok 6 - revert first..fourth works

expecting success:
	git checkout -f master &&
	git reset --hard fourth &&
	test_tick &&
	git revert ^first fourth &&
	git diff --quiet first &&
	git diff --cached --quiet first &&
	git diff --quiet HEAD first

HEAD is now at 94d3184 fourth
[master aa52aa6] Revert "fourth"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 7128690] Revert "third"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 2b0bc01] Revert "second"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
ok 7 - revert ^first fourth works

expecting success:
	git checkout -f master &&
	git reset --hard fourth &&
	test_tick &&
	git revert fourth fourth~1 fourth~2 &&
	git diff --quiet first &&
	git diff --cached --quiet first &&
	git diff --quiet HEAD first

HEAD is now at 94d3184 fourth
[master e7406a3] Revert "fourth"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 0039f72] Revert "third"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
[master 6fceae4] Revert "second"
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 1 deletion(-)
ok 8 - revert fourth fourth~1 fourth~2 works

expecting success:
	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git cherry-pick -3 fourth &&
	git diff --quiet other &&
	git diff --quiet HEAD other &&
	check_head_differs_from fourth

HEAD is now at 0c72e4f first
[master de54420] second
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[master 5fbc3d8] third
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[master 0f9850f] fourth
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
ok 9 - cherry-pick -3 fourth works

expecting success:
	git checkout -f master &&
	git reset --hard first &&
	test_tick &&
	git rev-list --reverse first..fourth | git cherry-pick --stdin &&
	git diff --quiet other &&
	git diff --quiet HEAD other &&
	check_head_differs_from fourth

HEAD is now at 0c72e4f first
[master 058f589] second
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[master 013cb83] third
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
[master 79f9008] fourth
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 0 deletions(-)
ok 10 - cherry-pick --stdin works

# failed 2 among 10 test(s)
1..10

-- 
Duy

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Sverre Rabbelier]

Heya,

On Mon, Aug 1, 2011 at 06:46, Jon Forrest <nobozo@gmail.com> wrote:
>        fprintf(options->file, "%s", line_prefix);
>        fprintf(options->file,
> -              " %d files changed, %d insertions(+), %d deletions(-)\n",
> -              total_files, adds, dels);
> +              " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
> +              total_files, total_files == 1 ? "" : "s", adds, adds == 1 ?
> "" : "s", dels,
> +               dels == 1 ? "" : "s");
>  }

Also, this is rather detrimental to the i18n effort methinks?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jon Forrest]

On 8/1/2011 2:58 AM, Sverre Rabbelier wrote:
> Heya,
>
> On Mon, Aug 1, 2011 at 06:46, Jon Forrest<nobozo@gmail.com>  wrote:
>>         fprintf(options->file, "%s", line_prefix);
>>         fprintf(options->file,
>> -              " %d files changed, %d insertions(+), %d deletions(-)\n",
>> -              total_files, adds, dels);
>> +              " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
>> +              total_files, total_files == 1 ? "" : "s", adds, adds == 1 ?
>> "" : "s", dels,
>> +               dels == 1 ? "" : "s");
>>   }
>
> Also, this is rather detrimental to the i18n effort methinks?

If the goal if the i18n effort is also to produce grammatically
correct output in all the supported languages then the
tests that my patch would break would have to be rewritten
anyway.

Jon

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jeff King]

On Mon, Aug 01, 2011 at 07:32:04AM -0700, Jon Forrest wrote:

> On 8/1/2011 2:58 AM, Sverre Rabbelier wrote:
> >Heya,
> >
> >On Mon, Aug 1, 2011 at 06:46, Jon Forrest<nobozo@gmail.com>  wrote:
> >>        fprintf(options->file, "%s", line_prefix);
> >>        fprintf(options->file,
> >>-              " %d files changed, %d insertions(+), %d deletions(-)\n",
> >>-              total_files, adds, dels);
> >>+              " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
> >>+              total_files, total_files == 1 ? "" : "s", adds, adds == 1 ?
> >>"" : "s", dels,
> >>+               dels == 1 ? "" : "s");
> >>  }
> >
> >Also, this is rather detrimental to the i18n effort methinks?
> 
> If the goal if the i18n effort is also to produce grammatically
> correct output in all the supported languages then the
> tests that my patch would break would have to be rewritten
> anyway.

I think he means that auto-pluralization like this cannot be done in an
i18n world, as many languages do not simply add "s". Your patch would
have to use ngettext, something like this (totally untested and just
copying a similar spot in suggest_reattach, as I have never done any
i18n myself):

  fprintf(options->file,
    Q_(" %d file changed,",
       " %d files changed",
       total_files),
    Q_(" %d insertion(+)",
       " %d insertions(+)",
       adds),
    Q_(" %d deletion(-)",
       " %d deletions(-)",
       dels),
    "\n",
    total_files, adds, dels);

And that gives translators a chance to specify the entire singular and
plural versions separately.

-Peff

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jon Forrest]

On 8/1/2011 11:06 AM, Jeff King wrote:
>
> I think he means that auto-pluralization like this cannot be done in an
> i18n world, as many languages do not simply add "s". Your patch would
> have to use ngettext, something like this (totally untested and just
> copying a similar spot in suggest_reattach, as I have never done any
> i18n myself):

[snip]

> And that gives translators a chance to specify the entire singular and
> plural versions separately.

I entirely agree. My point is only that the various tests
that expect the current behavior will have to be changed
whether the implementation of correct plurals uses my
inferior method or the way more correct i18n method.

Jon

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Sverre Rabbelier]

Heya,

On Mon, Aug 1, 2011 at 20:27, Jon Forrest <nobozo@gmail.com> wrote:
> I entirely agree. My point is only that the various tests
> that expect the current behavior will have to be changed
> whether the implementation of correct plurals uses my
> inferior method or the way more correct i18n method.

Wouldn't it be a nice hack if we just solved problem through i18n
then? Have all the plumbing see the current wording, but through i18n
change it to something grammatically correct for the porcelain.
Probably not possible, but a nice daydream :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jeff King]

On Mon, Aug 01, 2011 at 08:32:51PM +0200, Sverre Rabbelier wrote:

> On Mon, Aug 1, 2011 at 20:27, Jon Forrest <nobozo@gmail.com> wrote:
> > I entirely agree. My point is only that the various tests
> > that expect the current behavior will have to be changed
> > whether the implementation of correct plurals uses my
> > inferior method or the way more correct i18n method.
> 
> Wouldn't it be a nice hack if we just solved problem through i18n
> then? Have all the plumbing see the current wording, but through i18n
> change it to something grammatically correct for the porcelain.
> Probably not possible, but a nice daydream :).

I thought there was still some question of whether this text was
something that should be script-parseable. If it is, then it shouldn't
be i18n'd at all, nor should we lightly change the format with
pluralization magic. And if it isn't, then we should definitely go the
full i18n route. So in either case, the original patch isn't
appropriate.

I don't have a strong opinion myself. I tend to lean towards i18n-ing
it, because any scripts should be using --numstat to parse, anyway.
OTOH, as Junio pointed out, we are matching the output of much older
tools, so pre-git scripts might be written to read the --stat format.
I've never seen such a script, and I have no idea how many there really
are.

-Peff

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jakub Narebski]

Jon Forrest <nobozo@gmail.com> writes:
> On 8/1/2011 2:58 AM, Sverre Rabbelier wrote:
>> On Mon, Aug 1, 2011 at 06:46, Jon Forrest<nobozo@gmail.com>  wrote:

>>>         fprintf(options->file, "%s", line_prefix);
>>>         fprintf(options->file,
>>> -              " %d files changed, %d insertions(+), %d deletions(-)\n",
>>> -              total_files, adds, dels);
>>> +              " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
>>> +              total_files, total_files == 1 ? "" : "s", adds, adds == 1 ?
>>> "" : "s", dels,
>>> +               dels == 1 ? "" : "s");
>>>   }
>>
>> Also, this is rather detrimental to the i18n effort methinks?

Besides, as it was already said, this is an API.
 
> If the goal if the i18n effort is also to produce grammatically
> correct output in all the supported languages then the
> tests that my patch would break would have to be rewritten
> anyway.

That's not it.

The problem is that above code assumes that plural form can be formed
by adding suffix, and it assumes that is only one plural form... both
assumptions does not hold for non-English.

C.f. "Additional functions for plural forms" chapter in gettext info
page: http://www.gnu.org/s/hello/manual/gettext/Plural-forms.html

-- 
Jakub Narębski

Re: [PATCH] For Real - Fixed pluralization in diff reports

[Jon Forrest]

On 8/3/2011 6:38 AM, Jakub Narebski wrote:
> Jon Forrest<nobozo@gmail.com>  writes:

> Besides, as it was already said, this is an API.

I pretty much only looked at that one file and it
didn't look at quick glance like an API was being
used to internationalize git.

>> If the goal if the i18n effort is also to produce grammatically
>> correct output in all the supported languages then the
>> tests that my patch would break would have to be rewritten
>> anyway.
>
> That's not it.

We can discuss the correct way to implement a change like this
but the fact remains that whatever the implementation, the issue
that Junio raised will remain. That is, what to do about all
the places that presume the old incorrect output.

This problem isn't specific to git. I can easily imagine
other open source projects that face, or will face, this
problem.

Jon