Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix

[Duy Nguyen]

On Thu, Oct 6, 2016 at 7:40 AM, Jacob Keller  wrote:
> On Wed, Oct 5, 2016 at 3:15 PM, Junio C Hamano  wrote:
>> SZEDER Gábor  writes:
>>
>>> Gut feeling tells me that I should take this as a subtle
>>> encouragement to look into adding 'versionsort.postreleasesuffix',
>>> shouldn't I ;)
>>
>> It is more like "this made me realize that these are merely 'suffix'
>> after the real release name, no pre- or post- about them", also
>> known as "I think PREreleasesuffix was a mistake and we weren't
>> thinking clearly enough when we added it."
>>
>> To me, this looks like a list of possible suffixes that can include
>> an empty suffix to denote "the real thing", e.g.
>>
>> versionsort.suffix = "-alpha" "-beta" "" "-gamma" "-delta"
>>
>> and that position in the list determines the order of things inside
>> the same family of versions that share the same "non-suffix" part.
>
> This is what makes sense to me. Perhaps migrade to "releasesuffix" and
> deprecate "prereleasesuffix"?

Even though this is mostly used with tag listing, it can be used with
branch listing and general refs as well, where "release" does not
really make sense. So I prefer the name versionsort.suffix to
versionsort.releasesuffix.

> I don't think that's too painful
> especially if git accepts the old value and warns about it's
> deprecation? That or we can just stick with calling it pre-release and
> they sort based on order with '' being the empty value?

Yeah deprecation is good, we don't want to support to config keys with
different algorithms forever.
-- 
Duy

Re: [PATCH] run-command: fix build on cygwin (stdin is a macro)

[Torsten Bögershausen]

>I am not suggesting that you apply this exact patch (stdin_ is not a good
choice

How about fd_stdin ?

Re: Setting pager.add=true breaks add --patch

[Kevin Daudt]

On Thu, Oct 06, 2016 at 10:55:11AM +0700, Tom Hale wrote:
> On 2016-10-03 00:00, Anatoly Borodin wrote:
> > I've reported this one bug recently:
> > 
> > https://public-inbox.org/git/nrmbrl$hsk$1...@blaine.gmane.org/
> > 
> > The developers know about it, but it will require some deeper refactoring.
> 
> Thanks Anatoly for reporting this.
> 
> [Meta] All: For updates, is there an issue I can watch, or a way to
> subscribe/monitor only this one thread?
> 

public-inbox.org provides an atom feed[0] which you could add to a
reader.

I don't think you can just subscribe to a single thread in the mailing
list, but you can ask to be cc-ed in that thread.

[0]:https://public-inbox.org/git/nrmbrl$hsk$1...@blaine.gmane.org/t.atom

Re: Setting pager.add=true breaks add --patch

[Tom Hale]

On 2016-10-03 00:00, Anatoly Borodin wrote:

I've reported this one bug recently:

https://public-inbox.org/git/nrmbrl$hsk$1...@blaine.gmane.org/

The developers know about it, but it will require some deeper refactoring.


Thanks Anatoly for reporting this.

[Meta] All: For updates, is there an issue I can watch, or a way to 
subscribe/monitor only this one thread?


--
Cheers,
Tom

Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix

[Jacob Keller]

On Wed, Oct 5, 2016 at 3:15 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> Gut feeling tells me that I should take this as a subtle
>> encouragement to look into adding 'versionsort.postreleasesuffix',
>> shouldn't I ;)
>
> It is more like "this made me realize that these are merely 'suffix'
> after the real release name, no pre- or post- about them", also
> known as "I think PREreleasesuffix was a mistake and we weren't
> thinking clearly enough when we added it."
>
> To me, this looks like a list of possible suffixes that can include
> an empty suffix to denote "the real thing", e.g.
>
> versionsort.suffix = "-alpha" "-beta" "" "-gamma" "-delta"
>
> and that position in the list determines the order of things inside
> the same family of versions that share the same "non-suffix" part.

This is what makes sense to me. Perhaps migrade to "releasesuffix" and
deprecate "prereleasesuffix"? I don't think that's too painful
especially if git accepts the old value and warns about it's
deprecation? That or we can just stick with calling it pre-release and
they sort based on order with '' being the empty value?

Thanks,
Jake

Re: [PATCH v3 05/14] i18n: add--interactive: mark plural strings

[Junio C Hamano]

Vasco Almeida  writes:

> @@ -70,6 +72,8 @@ Git::I18N - Perl interface to Git's Gettext localizations
>  
>   printf __("The following error occurred: %s\n"), $error;
>  
> + printf __n("commited %d file", "commited %d files", $files), $files;
> +

A micronit: the existing example above prints a whole line,
i.e. terminated with a LF.  The new one probably should match.

Re: [PATCH v9 00/14] Git filter protocol

[Jakub Narębski]

W dniu 04.10.2016 o 14:59, larsxschnei...@gmail.com pisze:
> From: Lars Schneider 
> 
> The goal of this series is to avoid launching a new clean/smudge filter
> process for each file that is filtered.
> 
> A short summary about v1 to v5 can be found here:
> https://git.github.io/rev_news/2016/08/17/edition-18/
> 
> This series is also published on web:
> https://github.com/larsxschneider/git/pull/13
> 
> Patches 1 and 2 are cleanups and not strictly necessary for the series.
> Patches 3 to 12 are required preparation. Patch 13 is the main patch.
> Patch 14 adds an example how to use the Git filter protocol in contrib.
> 
> Thanks a lot to
>   Ramsay, Jakub, Junio, Johannes, Torsten, and Peff
> for very helpful reviews,
> Lars

I'll try to review it before the end of the week.

-- 
Jakub Narębski

Re: [PATCH v3 03/14] i18n: add--interactive: mark strings with interpolation for translation

[Junio C Hamano]

Vasco Almeida  writes:

>   if (!defined $bottom) {
> - error_msg "Huh ($choice)?\n";
> + error_msg sprintf(__("Huh (%s)?\n"), 
> $choice);
>   next TOPLOOP;
>   }
>   }
>   if ($opts->{SINGLETON} && $bottom != $top) {
> - error_msg "Huh ($choice)?\n";
> + error_msg sprintf(__("Huh (%s)?"), $choice);
>   next TOPLOOP;

Doesn't this want "\n" just like the other one in this hunk?

Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix

[Junio C Hamano]

SZEDER Gábor  writes:

> Gut feeling tells me that I should take this as a subtle
> encouragement to look into adding 'versionsort.postreleasesuffix',
> shouldn't I ;)

It is more like "this made me realize that these are merely 'suffix'
after the real release name, no pre- or post- about them", also
known as "I think PREreleasesuffix was a mistake and we weren't
thinking clearly enough when we added it."

To me, this looks like a list of possible suffixes that can include
an empty suffix to denote "the real thing", e.g.

versionsort.suffix = "-alpha" "-beta" "" "-gamma" "-delta"

and that position in the list determines the order of things inside
the same family of versions that share the same "non-suffix" part.

Re: [RFC/PATCH] attr: Document a new possible thread safe API

[Junio C Hamano]

Stefan Beller  writes:

> I think so, instead of resending the documentation, maybe the header
> file shows that we're on the same page, I converted everything except
> attr.c to follow this header attr.h:

OK.  The function signature of git_check_attr() looks suspect (it
does not match the "typical case" illustration in the message you
are responding to), but other than that I think this matches my
expectation.

Thanks for taking this over.

Re: [PATCH v3 00/14] Mark strings in Perl scripts for translation

[Jakub Narębski]

W dniu 05.10.2016 o 19:20, Vasco Almeida pisze:
> Mark messages in some perl scripts for translation.
> 
> Thanks for the reviews of Junio Hamano and Jakub Narębski.  Although I think
> Jakub Narębski's suggestions are overall good, I am not willing to go that 
> path
> because I cannot see huge benefits from them given what we already have and
> also I lack Perl skills.

All right.  While I think that Locale::TextDomain-like interpolation
in translated strings (__x, __xn / __nx, etc.), which is labelled as
'perl-brace-format' by gettext, is more Perl-ish and better for unbiased
translators, the printf based interpolation, labelled as ‘perl-format’
(and identical to 'c-format', I think), may be preferred in this case.

The 'perl-brace-format' doesn't need TRANSLATOR comments to explain
what placeholders are, and placeholders are easier to reorder.  On
the other hand translator needs to know to not translate contents
of placeholders.

  "This is the {color} {thing}.\n"

With 'perl-format' / 'c-format' the translator might need to know
how to change order of placeholders, but he or she should know
how to do it translating strings from C code.

  "This is the %s %s.\n"

Also, if Perl code shares translation strings with C code, as in
most cases here, then printf format is needed to do translation
only once.

tldr; I am reversing my opinion, and agree with your solution.

> 
> Interdiff bellow.

One thing I have noticed in the interdiff is using *translated*
strings in hash content (translation takes time, and might not
be necessary), that is:

   $hashref = {
KEY => __('value'),
   }

   ... $hashref->{KEY} ...

instead of marking value for translation, and doing translation
only on print, when it is necessary

   $hashref = {
KEY => N__('value'),
   }

   ... __($hashref->{KEY}) ...

> 
> Vasco Almeida (14):
[...]

I'll try to review those later.

Thank you for your work,
-- 
Jakub Narębski

Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull

[Junio C Hamano]

Sergey Organov  writes:

> So I believe this change is inline with the rest of the patch. The
> reference to git-pull (if it remains) should be a side-note, not part of
> explanation of operation.

Not really.  The thing is, "This is the most common" needs to be
close to "Often...".  "git merge" directly invoked by the end user
is much less likely to encounter a fast forward situation; getting
invoked indirectly by "git pull" makes it common.

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Junio C Hamano]

Sergey Organov  writes:

> OK, I see. So, what is the best way to handle this? Immediately follow
> content change patch with another patch that only re-flows?

Or no reflowing at all.

>> the parents".  I do not know if the updated phrasing is better.  The
>> "name" in the original was meant to be a short-hand for "object name",
>> and I would support a change to spell it out to clarify; "reference"
>> can be a vague word that can mean different things in Git, and when
>> the word is given without context, most Git people would think that
>> the word refers to "refs", but that is definitely not what the new
>> commit records, so...
>
> I won't insist on the change, but "name" sounded wrong to me, and
> "reference" was most general term I was able to come up with in this
> context.
> ...
> Last, if "reference" is not good enough and we get to internals anyway,
> why not say SHA1 then?

Because that is still colloquial?  I think s/name/object name/ is
a sensible change, but not s/name/reference/.

Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull

[Sergey Organov]

Junio C Hamano  writes:

> sorga...@gmail.com writes:

[...]

>> @@ -138,14 +133,15 @@ will exit early with the message "Already up-to-date."
>>  FAST-FORWARD MERGE
>>  --
>>  
>> -Often the current branch head is an ancestor of the named commit.
>> +Often the current branch head is an ancestor of the named commit.  In
>> +this case, a new commit is not needed to store the combined history;
>> +instead, the `HEAD` (along with the index) is updated to point at the
>> +named commit, without creating an extra merge commit.
>> +
>>  This is the most common case especially when invoked from 'git
>>  pull': you are tracking an upstream repository, you have committed
>>  no local changes, and now you want to update to a newer upstream
>> -revision.  In this case, a new commit is not needed to store the
>> -combined history; instead, the `HEAD` (along with the index) is
>> -updated to point at the named commit, without creating an extra
>> -merge commit.
>> +revision.
>
> I am not sure if the post-image of this hunk is better than the
> original.

That's what I've tried to explain in the description of the patch:

"No awareness of git-pull is required to understand git-merge operation,
so leave reference to git-pull only where it actually makes sense, in
the description of fast-forward merges, and only as clarification of
when this merging behaviour is mostly useful."

So I believe this change is inline with the rest of the patch. The
reference to git-pull (if it remains) should be a side-note, not part of
explanation of operation.

-- Sergey

Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix

[SZEDER Gábor]

Quoting Junio C Hamano :


SZEDER Gábor  writes:


And a final sidenote: sorting based on the longest matching suffix
also allows us to (ab)use version sort with prerelease suffixes to
sort postrelease tags as we please, too:

 $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \
 -c versionsort.prereleasesuffix=-beta \
 -c versionsort.prereleasesuffix= \
 -c versionsort.prereleasesuffix=-gamma \
 -c versionsort.prereleasesuffix=-delta \
 tag -l --sort=version:refname 'v3.0*'
 v3.0-alpha1
 v3.0-beta1
 v3.0
 v3.0-gamma1
 v3.0-delta1


Assuming that gamma and delta are meant to indicate "these are
post-release updates",


Indeed they were meant as post-release suffixes.  Naturally following
alpha and beta, they were the first to spring to mind that should be
sorted in non-lexicographical order, so I could show of postrelease
reordering.  It's just that we don't have a config like
'versionsort.postreleasesuffix', which is half the abuse.  The other
half of the abuse is that I had to explicitly indicate the position
of suffixless versions with an empty suffix between pre- and
postrelease suffixes.  The empty suffix matches on every tag, but
then it's overridden by all configured suffixes, so such a version
just stays in the middle.


I think a mechanism that can yield the above
result is fantastic ;-)


Heh.

Gut feeling tells me that I should take this as a subtle
encouragement to look into adding 'versionsort.postreleasesuffix',
shouldn't I ;)

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Sergey Organov]

Jakub Narębski  writes:

> W dniu 05.10.2016 o 16:46, sorga...@gmail.com pisze:
>> From: Sergey Organov 
>> 
>> Old description had a few problems:
>> 
>> - sounded as if commits have changes
>> 
>> - stated that changes are taken since some "divergence point"
>>   that was not defined.
>> 
>> New description rather uses "common ancestor" and "merge base",
>> definitions of which are easily discoverable in the rest of GIT
>> documentation.
>
> This is a step in a good direction, but it has a few issues.

Thanks a lot for reviewing! I'll need time to read your reply carefully.

-- Sergey

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Sergey Organov]

Junio C Hamano  writes:

> sorga...@gmail.com writes:
>
>> From: Sergey Organov 
>>
>> Old description had a few problems:
>>
>> - sounded as if commits have changes
>>
>> - stated that changes are taken since some "divergence point"
>>   that was not defined.
>>
>> New description rather uses "common ancestor" and "merge base",
>> definitions of which are easily discoverable in the rest of GIT
>> documentation.
>>
>> Signed-off-by: Sergey Organov 
>> ---
>>  Documentation/git-merge.txt | 25 +++--
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index cc0329d..351b8fc 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -16,11 +16,16 @@ SYNOPSIS
>>  
>>  DESCRIPTION
>>  ---
>> -Incorporates changes from the named commits (since the time their
>> -histories diverged from the current branch) into the current
>> -branch.  This command is used by 'git pull' to incorporate changes
>> -from another repository and can be used by hand to merge changes
>> -from one branch into another.
>> +
>> +Incorporates changes that lead to the named commits into the current
>> +branch, and joins corresponding histories. The best common ancestor of
>> +named commits and the current branch, called "merge base", is
>> +calculated, and then net changes taken from the merge base to
>> +the named commits are applied.
>> +
>> +This command is used by 'git pull' to incorporate changes from another
>> +repository, and can be used by hand to merge changes from one branch
>> +into another.
>
> Content change together with re-flowing the text makes it more
> costly than necessary to review a change like this.  Please avoid
> doing so in your future patches.

OK, I see. So, what is the best way to handle this? Immediately follow
content change patch with another patch that only re-flows?

> I like what the updated description says very much.  I however
> wonder if "and can be used by hand..." is still appropriate, or
> needs a bit of modernizing.  It feels a bit awkward by making it
> sound as if 'git merge' is primarily an implementation detail of
> 'git pull' but it can also be used as the first-class command, which
> used to be the case in the old days back when "git pull . other" was
> also perfectly good way to merge the 'other' branch from your own
> repository, but I think your update is meant to clarify that we no
> longer live in that old world ;-)

Yes, exactly, but 6/6 removes most of the mentions of git-pull from the
manual anyway, so I felt it better belongs there.

>
>> @@ -31,11 +36,11 @@ Assume the following history exists and the current 
>> branch is
>>  D---E---F---G master
>>  
>>  
>> -Then "`git merge topic`" will replay the changes made on the
>> -`topic` branch since it diverged from `master` (i.e., `E`) until
>> -its current commit (`C`) on top of `master`, and record the result
>> -in a new commit along with the names of the two parent commits and
>> -a log message from the user describing the changes.
>
>> -Then "`git merge topic`" will replay the changes made on the `topic`
>> -branch since it diverged from `master` (i.e., `E`) until its current
>> -commit (`C`) on top of `master`, and record the result in a new commit
>> -along with the names of the two parent commits and a log message from
>> -the user describing the changes.
>
>> +Then "`git merge topic`" will replay the changes made on the `topic`
>> +branch since it diverged from `master` (i.e., `E`) until its current
>> +commit (`C`) on top of `master`, and record the result in a new commit
>> +along with references to the two parent commits and a log message from
>> +the user describing the changes.
>
> Content change together with re-flowing the text makes it more
> costly than necessary to review a change like this.  Please avoid
> doing so in your future patches.

Yeah, got it.

> I had to re-flow the original you removed to match how you flowed in
> the updated one and stare at it for a while to spot that the only
> change was to rephrase "the names of the parents" to "references to
> the parents".  I do not know if the updated phrasing is better.  The
> "name" in the original was meant to be a short-hand for "object name",
> and I would support a change to spell it out to clarify; "reference"
> can be a vague word that can mean different things in Git, and when
> the word is given without context, most Git people would think that
> the word refers to "refs", but that is definitely not what the new
> commit records, so...

I won't insist on the change, but "name" sounded wrong to me, and
"reference" was most general term I was able to come up with in this
context.

First, "name" somehow suggested that it could be the case that

$ git branch
* master
$ git merge topic

will store strings "master" and "topic" in the resulting commit.

Re: [PATCH v9 04/14] run-command: add wait_on_exit

[Junio C Hamano]

Lars Schneider  writes:

> OK. Something like the patch below would work nicely.

Yeah, something along that line; it would eliminate the need to
worry about a field named "stdin" ;-)

But ...

>   while (children_to_clean) {
>   struct child_to_clean *p = children_to_clean;
>   children_to_clean = p->next;
> +
> + if (p->clean_on_exit_handler) {
> + p->clean_on_exit_handler(p->pid, in_signal);
> + }

... the application that used run_command() API would want to be
able to pass extra piece of data that is appliation-specific for the
child being killed, so it may make sense to extend the function
signature to take a pointer to "struct child_process" for the child
process being killed, together with a new field added to "struct
child_process" that is "void *exit_handler_cbdata;", perhaps?

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Johannes Sixt]

Am 05.10.2016 um 09:47 schrieb Josef Ridky:

Add support for user defined suffix part of name of temporary files
created by git mergetool


Do I understand correctly that your users have problems to identify 
which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they 
must edit? I agree that there is some room for improvement.


The goal is that you want the user to see the label, e.g., "_EDIT_THIS_" 
instead of "_LOCAL_". Now you have to teach your users that they have to 
pass --local=_EDIT_THIS_. Why don't you just teach your users to edit 
the file labeled "_LOCAL_"?


Therefore, I think that your patch as written does not help to reduce 
the confusion. It may be a building block for further improvement, but 
if you stop here, it is pointless.



 SYNOPSIS
 
 [verse]
-'git mergetool' [--tool=] [-y | --[no-]prompt] [...]
+'git mergetool' [--tool=] [-y | --[no-]prompt] [--local=] [--remote=] 
[--backup=] [--base=] [...]

 DESCRIPTION
 ---
@@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.

+--local=::
+   Use string from  as part of suffix of name of temporary
+   file (local) for merging. If not used or is equal with any
+   other (remote|backup|base), default value is used.
+   Default suffix is LOCAL.
+
+--remote=::
+   Use string from  as part of suffix of name of temporary
+   file (remote) for merging. If not used or is equal with any
+   other (local|backup|base), default value is used.
+   Default suffix is REMOTE.
+
+--backup=::
+   Use string from  as part of suffix of name of temporary
+   file (backup) for merging. If not used or is equal with any
+   other (local|remote|base), default value is used.
+   Default suffix is BACKUP.
+
+--base=::
+   Use string from  as part of suffix of name of temporary
+   file (base) for merging. If not used or is equal with any
+   other (local|remote|backup), default value is used.
+   Default suffix is BASE.

Re: [PATCH 2/6] Documentation/git-merge.txt: remove list of options from SYNOPSIS

[Sergey Organov]

Junio C Hamano  writes:

> sorga...@gmail.com writes:
>
>> From: Sergey Organov 
>>
>> This partial list of option is confusing as it lacks a lot of
>> available options. It also clutters the SYNOPSIS making differences
>> between forms of invocation less clear.
>>
>> Signed-off-by: Sergey Organov 
>> ---
>>  Documentation/git-merge.txt | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index b758d55..90342eb 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -9,10 +9,7 @@ git-merge - Join two or more development histories together
>>  SYNOPSIS
>>  
>>  [verse]
>> -'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
>> -[-s ] [-X ] [-S[]]
>> -[--[no-]allow-unrelated-histories]
>> -[--[no-]rerere-autoupdate] [-m ] [...]
>> +'git merge' [options] [-m ] [...]
>>  'git merge'  HEAD ...
>>  'git merge' --abort
>
> Same comment as 1/6; as we'd hopefully be removing the deprecated
> form soonish, it would probably make sense to leave only two, i.e.
>
>   git merge [options] [...]
>   git merge --abort
>
> in synposis.

Same "yes" as in 1/6, obviously.

-- Sergey

Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME

[Sergey Organov]

Junio C Hamano  writes:

> sorga...@gmail.com writes:
>
>> From: Sergey Organov 
>>
>> Old description not only raised the question of why the tool is called
>> git-merge rather than git-join, but "join histories" also sounds like
>> very simple operation, something like what "git-merge -s ours" does.
>>
>> Signed-off-by: Sergey Organov 
>> ---
>>  Documentation/git-merge.txt | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index 216d2f4..cc0329d 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -3,7 +3,8 @@ git-merge(1)
>>  
>>  NAME
>>  
>> -git-merge - Join two or more development histories together
>> +
>> +git-merge - Merge one or more branches to the current branch
>
> This patch, evaluated by itself, looks like a regression in that it
> tries to explain "merge" by using verb "merge", making it fuzzier to
> those who do not yet know what a "merge" is.  That was why it tried
> to explain "merge" as an operation to join histories.

My thought was that "merge", the operation, is so well-known term that
it could well go into the NAME section without explanation.

Besides:

$ man merge
NAME
   merge - three-way file merge
[...]

Uses the same pattern.

>
> However, the next one, 5/6, resurrects the "join history" in the
> description part to help them, so the damage is not so severe when
> we take them together.

Damage? In SCM world we can track the issue back to:

$ man -k rcsmerge
rcsmerge (1) - merge RCS revisions

-- Sergey

Re: [PATCH v9 04/14] run-command: add wait_on_exit

[Lars Schneider]

> On 04 Oct 2016, at 21:30, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> The flag 'clean_on_exit' kills child processes spawned by Git on exit.
>> A hard kill like this might not be desired in all cases.
>> 
>> Add 'wait_on_exit' which closes the child's stdin on Git exit and waits
>> until the child process has terminated.
>> 
>> The flag is used in a subsequent patch.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> ...
>> static void cleanup_children(int sig, int in_signal)
>> {
>> +int status;
>> +struct child_to_clean *p = children_to_clean;
>> +
>> +/* Close the the child's stdin as indicator that Git will exit soon */
>> +while (p) {
>> +if (p->wait)
>> +if (p->stdin > 0)
>> +close(p->stdin);
>> +p = p->next;
>> +}
> 
> This part and the "stdin" field feels a bit too specific to the
> caller you are adding.  Allowing the user of the API to specify what
> clean-up cation needs to be taken in the form of a callback function
> may not be that much more effort and would be more flexible and
> useful, I would imagine?

OK. Something like the patch below would work nicely.
Does this look acceptable?

Thanks,
Lars


diff --git a/run-command.c b/run-command.c
index 3269362..a0256a6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
 
 struct child_to_clean {
pid_t pid;
+   void (*clean_on_exit_handler)(pid_t, int);
struct child_to_clean *next;
 };
 static struct child_to_clean *children_to_clean;
@@ -31,6 +32,11 @@ static void cleanup_children(int sig, int in_signal)
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
+
+   if (p->clean_on_exit_handler) {
+   p->clean_on_exit_handler(p->pid, in_signal);
+   }
+
kill(p->pid, sig);
if (!in_signal)
free(p);
@@ -49,10 +55,11 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
 }
 
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup(pid_t pid, void 
(*clean_on_exit_handler)(pid_t, int))
 {
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+   p->clean_on_exit_handler = clean_on_exit_handler;
p->next = children_to_clean;
children_to_clean = p;
 
@@ -421,8 +428,8 @@ int start_command(struct child_process *cmd)
}
if (cmd->pid < 0)
error_errno("cannot fork() for %s", cmd->argv[0]);
-   else if (cmd->clean_on_exit)
-   mark_child_for_cleanup(cmd->pid);
+   else if (cmd->clean_on_exit || cmd->clean_on_exit_handler)
+   mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
/*
 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -482,8 +489,8 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error_errno("cannot spawn %s", cmd->argv[0]);
-   if (cmd->clean_on_exit && cmd->pid >= 0)
-   mark_child_for_cleanup(cmd->pid);
+   if ((cmd->clean_on_exit || cmd->clean_on_exit_handler) && cmd->pid >= 0)
+   mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
argv_array_clear();
cmd->argv = sargv;
@@ -765,7 +772,7 @@ int start_async(struct async *async)
exit(!!async->proc(proc_in, proc_out, async->data));
}
 
-   mark_child_for_cleanup(async->pid);
+   mark_child_for_cleanup(async->pid, NULL);
 
if (need_in)
close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index cf29a31..3630733 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,7 @@ struct child_process {
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
unsigned clean_on_exit:1;
+   void (*clean_on_exit_handler)(pid_t, int);
 };
 
 #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
 

Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME

[Sergey Organov]

Jeff King  writes:

> On Wed, Oct 05, 2016 at 05:46:22PM +0300, sorga...@gmail.com wrote:
>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index 216d2f4..cc0329d 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -3,7 +3,8 @@ git-merge(1)
>>  
>>  NAME
>>  
>> -git-merge - Join two or more development histories together
>> +
>> +git-merge - Merge one or more branches to the current branch
>
> I wonder if we should be more clear that you don't have to merge a
> branch; you can merge any commit. I do agree that the original was
> unnecessarily general. And I think "the current branch" is accurate
> (technically it can be to a detached HEAD, but that is pedantry that
> doesn't need to make it into the synopsis).
>
> So maybe "Merge one or more commits into the current branch".  I guess
> that is a bit vague, too. It is really "commit tips" or "lines of
> development" that we are merging. Bringing them in of course brings in
> many commits, but the "or more" there is meant to hint at multi-parent
> merges.
>
> So perhaps "one or more branches", while not completely accurate, is the
> best we can do. I dunno.

You've basically repeated my entire line of thinking that lead to the
patch.

-- Sergey.

Re: [PATCH 1/6] git-merge: clarify "usage" by adding "-m "

[Sergey Organov]

Junio C Hamano  writes:

> sorga...@gmail.com writes:
>
>> From: Sergey Organov 
>>
>> "-m " is one of essential distinctions between obsolete
>> invocation form and the recent one. Add it to the "usage" returned by
>> 'git merge -h' for more clarity.
>>
>> Signed-off-by: Sergey Organov 
>> ---
>>  builtin/merge.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index a8b57c7..0e367ba 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -43,7 +43,7 @@ struct strategy {
>>  };
>>  
>>  static const char * const builtin_merge_usage[] = {
>> -N_("git merge [] [...]"),
>> +N_("git merge [] [-m ] [...]"),
>>  N_("git merge []  HEAD "),
>>  N_("git merge --abort"),
>>  NULL
>
> While this is not wrong per-se, as the deprecated form will go away
> soon, I hope you do not mind if I had to drop this one from the
> series to avoid merge conflicts to 'pu' (I do not know how bad the
> conflict would be yet; I am just reviewing in my MUA).

Yeah, sure. I was not aware obsolete form description is to go away
soon.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan  writes:

> Sounds reasonable to me. Would the "[" be a bit of overspecification,
> though, since Git doesn't produce it? Also, identifying it as a
> garbage line probably wouldn't change any behavior - in the Linux
> kernel examples, it is used to show what happened in between
> sign-offs, so there will always be one "Signed-off-by:" at the top.

Good thinking.  As "interpret trailers" cannot locate such a line to
manipulate (as it lacks ), we can simply treat it as a
garbage line.

>> struct {
>>  const char *whole;
>>  const char *end_of_message_proper;
>>  struct {
>>  const char *token;
>>  const char *contents;
>>  } *trailer;
>>  int alloc_trailers, nr_trailers;
>> };
>>
>> where
>>
>>  - whole points at the first byte of the input, i.e. the beginning
>>of the commit message buffer.
>>
>>  - end-of-message-proper points at the first byte of the trailer
>>block into the buffer at "whole".
>>
>>  - token is a canonical header name for easy comparison for
>>interpret-trailers (you can use NULL for garbage lines, and made
>>up token like "[bracket]" and "(cherrypick)" that would not clash
>>with real tokens like "Signed-off-by").
>>
>>  - contents is the bytes on the logical line, including the header
>>part
>>
>> E.g. an element in trailer[] array may say
>>
>> {
>>  .token = "Signed-off-by",
>> .contents = "Signed-Off-By: Some Body \n",
>> }
>
> I get the impression from the rest of your e-mail that no strings are
> meant to be copied - is that true? (That sounds like a good idea to
> me.)

I was envisioning that "whole", "end-of-message" can point into the
input buffer, while trailer[].contents may have to be copied, if
only to make it to a NUL-terminated string.  But I am fine with
 or  pair to avoid copying .contents if that is
desired.  You'd need to worry about differentiating .contents that
need to be freed after "interpret trailers" inserted a new entry or
replaced the contents, though.

[PATCH] run-command: fix build on cygwin (stdin is a macro)

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Lars,

Commit 6007c69e ("run-command: add wait_on_exit", 04-10-2016), which
is part of your 'ls/filter-process' branch, causes the build to fail
on cygwin, since 'stdin' is defined as a macro thus:

#define stdin   (_REENT->_stdin)

(you can probably guess what stdout and stderr look like!) where _REENT
in turn expands to a function call (__getreent()) which returns a pointer
to a 'struct _reent', etc., ...

I am not suggesting that you apply this exact patch (stdin_ is not a good
choice), but I wanted to show the exact patch I used to get the build to
complete on cygwin.

ATB,
Ramsay Jones

 run-command.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 96c54fe..a9dd91a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -22,7 +22,7 @@ void child_process_clear(struct child_process *child)
 struct child_to_clean {
pid_t pid;
char *name;
-   int stdin;
+   int stdin_;
int wait;
struct child_to_clean *next;
 };
@@ -37,8 +37,8 @@ static void cleanup_children(int sig, int in_signal)
/* Close the the child's stdin as indicator that Git will exit soon */
while (p) {
if (p->wait)
-   if (p->stdin > 0)
-   close(p->stdin);
+   if (p->stdin_ > 0)
+   close(p->stdin_);
p = p->next;
}
 
@@ -73,12 +73,12 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
 }
 
-static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int 
wait)
+static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin_, 
int wait)
 {
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
p->wait = wait;
-   p->stdin = stdin;
+   p->stdin_ = stdin_;
if (name)
p->name = xstrdup(name);
else
@@ -94,7 +94,7 @@ static void mark_child_for_cleanup(pid_t pid, const char 
*name, int stdin, int w
 }
 
 #ifdef NO_PTHREADS
-static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int 
timeout, int stdin)
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int 
timeout, int stdin_)
 {
mark_child_for_cleanup(pid, NULL, 0, 0);
 }
-- 
2.10.0

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Junio C Hamano]

Jakub Narębski  writes:

>> +
>> +This command is used by 'git pull' to incorporate changes from another
>> +repository, and can be used by hand to merge changes from one branch
>> +into another.
>
> Rather "can be used by 'git pull'", or "is used by 'git pull' (unless
> configured otherwise)"...

I think you are misreading the original and the update (see my
comments in the other message).

>> @@ -31,11 +36,11 @@ Assume the following history exists and the current 
>> branch is
>>  D---E---F---G master
>>  
>>  
>> -Then "`git merge topic`" will replay the changes made on the
>> -`topic` branch since it diverged from `master` (i.e., `E`) until
>> -its current commit (`C`) on top of `master`, and record the result
>> -in a new commit along with the names of the two parent commits and
>> -a log message from the user describing the changes.
>> +Then "`git merge topic`" will replay the changes made on the `topic`
>> +branch since it diverged from `master` (i.e., `E`) until its current
>> +commit (`C`) on top of `master`, and record the result in a new commit
>> +along with references to the two parent commits and a log message from
>> +the user describing the changes.
>
> What the happened here!?!  Please do not rewrap documentation, especially
> not without changes!

Yes, reflowing is bad but you can spot the change from "along with
the names of the parent commits" to "along with references to the
parent commits" if you stare at it long enough ;-)

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/04/2016 11:28 AM, Junio C Hamano wrote:

An addendum.  We may also want to be prepared to accept an input
that has some garbage lines _after_ the trailer block, if we can
clearly identify them as such.  For example, we could change the
definition of "the last paragraph" as "the block of lines that
do not have any empty (or blank) line, that appear either at the end
of the input, or immediately before three-dash lines", to allow

commit title

explanation of the change

Signed-off-by: Some Body 
[some other things]
Acked-by: Some Other Person 

---
 additional comment

which (unfortunately) is a rather common pattern for people who plan
to send the commit over e-mail.

If we add a new field "const char *beginning_of_tail_garbage" next
to "end_of_message_proper" that points at the blank line before the
three-dash line in the above example, the parser should be able to
break such an input into a parsed form, allow the trailer[] array to
be manipulated and reproduce a commit log message.


How important is this feature? It doesn't seem too difficult to add, 
although it does break compatibility (in particular, "--signoff" must 
now be documented as "after the last trailer" instead of "at the end of 
the commit message").

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/04/2016 10:25 AM, Junio C Hamano wrote:

So I would say it is perfectly OK if your update works only for
cases we can clearly define the semantics for.  For example, we can
even start with something simple like:

 * A RFC822-header like line, together with any number of whitespace
   indented lines that immediately follow it, will be taken as a
   single logical trailer element (with embedded LF in it if it uses
   the "line folding").  For the purpose of "replace", the entire
   single logical trailer element is replaced.

 * A line that begins with "(cherry picked from" and "[" becomes a
   single logical trailer element.  No continuation of anything
   fancy.

 * A line with any other shape is a garbage line in a trailer
   block.  It is kept in its place, but because it does not even
   have  part, it will not participate in locating with
   "trailer.where", "trailer.ifexists", etc.


Sounds reasonable to me. Would the "[" be a bit of overspecification, 
though, since Git doesn't produce it? Also, identifying it as a garbage 
line probably wouldn't change any behavior - in the Linux kernel 
examples, it is used to show what happened in between sign-offs, so 
there will always be one "Signed-off-by:" at the top.  (But I do not 
feel strongly about this.)



A block of lines that appear as the last paragraph in a commit
message is a trailer block if and only if certain number or
percentage of lines are non-garbage lines according to the above
definition.


I think the number should be 1 - that seems like the easiest to explain. 
But I'm OK with other suggestions.



I wonder if we can share a new helper function to do the detection
(and classification) of a trailer block and parsing the logical
lines out of a commit log message.  The function signature could be
as simple as taking a single  (or a strbuf) that holds
a commit log message, and splitting it out into something like:

struct {
const char *whole;
const char *end_of_message_proper;
struct {
const char *token;
const char *contents;
} *trailer;
int alloc_trailers, nr_trailers;
};

where

 - whole points at the first byte of the input, i.e. the beginning
   of the commit message buffer.

 - end-of-message-proper points at the first byte of the trailer
   block into the buffer at "whole".

 - token is a canonical header name for easy comparison for
   interpret-trailers (you can use NULL for garbage lines, and made
   up token like "[bracket]" and "(cherrypick)" that would not clash
   with real tokens like "Signed-off-by").

 - contents is the bytes on the logical line, including the header
   part

E.g. an element in trailer[] array may say

{
.token = "Signed-off-by",
.contents = "Signed-Off-By: Some Body \n",
}


I get the impression from the rest of your e-mail that no strings are 
meant to be copied - is that true? (That sounds like a good idea to me.) 
In which case this might be better:


  struct {
const char *first_trailer; /* = end_of_message_proper */
struct {
  const char *start;
  const char *value;
  const char *end;
} *trailers;
int trailers_nr, trailers_alloc;
  };

start = value for "[", "(cherry picked from" and garbage lines. We also 
need end because there is no \0 there (we didn't copy any strings).


The existing code (in trailer.c) uses a linked list to store trailers, 
but an array (as written in your e-mail) is probably better for us since 
clients would want to access the last element (as also written in your 
e-mail).



With something like that, you can manipulate the "insert at ...",
"replace", etc. in the trailer[] array and then produce an updated
commit message fairly easily (i.e. copy out the bytes beginning at
"whole" up to "end_of_message_proper", then iterate over trailer[]
array and show their contents field).  The codepaths in the core
part only need to know

 - how to check the last item in trailer[] array to see if it ends
   with the same sign-off as they are trying to add.

 - how to append one new element to the trailer[] array.

 - reproduce an updated commit log message after the above.


I don't think we need trailer block struct -> commit message conversion 
- when adding a new trailer or replacing an existing trailer, the client 
code can just remember the index and then modify its behavior 
accordingly when iterating through all trailers. But this conversion can 
be easily added if/when we need it.


> Hmm?

Overall, this seems like a good idea - I'll go ahead and do this if 
there are no other objections.


It just occurred to me that there could be some corner cases when the 
trailer separator is configured to not include ":" - I'll make sure to 
include tests that check those corner cases.

Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Junio C Hamano]

Jacob Keller  writes:

>> The cost of fill function having to do the same thing repeatedly is
>> negligible, so I am OK with the result, but for fairness, this was
>> not "make the callers do this extra thing", but was "the caller can
>> prepare these unchanging parts just once, and the fill function that
>> is repeatedly run does not have to."
>
> Sure, but it's a pretty minor optimization and I think the result is
> easier to understand.

Yes; in case it wasn't clear, my comment was merely for fairness to
the original code.  I do agree that the end result of this series
makes a very pleasant read.

Re: [PATCH v3 6/6] wt-status: begin error messages with lower-case

[Jakub Narębski]

W dniu 04.10.2016 o 15:06, Johannes Schindelin pisze:

> The previous code still followed the old git-pull.sh code which did not
> adhere to our new convention.

Good to know why it used its own convention.
 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/pull.c | 2 +-
>  wt-status.c| 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index c639167..0bf9802 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>  
>   if (!autostash)
>   require_clean_work_tree(N_("pull with rebase"),
> - "Please commit or stash them.", 1, 0);
> + "please commit or stash them.", 1, 0);
>  

Shouldn't those also be marked for translation with N_() or _()?

Best,
-- 
Jakub Narębski

Re: [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions

[Jakub Narębski]

W dniu 04.10.2016 o 15:05, Johannes Schindelin pisze:

> Subject: Export also the has_un{staged,committed}_changed() functions

s/changed/changes/   that is   d -> s

Those are has_unstaged_changes() and has_uncommitted_changes().

Though I wonder if "other has_un*_changes() functions" would be
more readable (while shorter), if less specific...

>
> They will be used in the upcoming rebase helper.
> 
> Signed-off-by: Johannes Schindelin 
> ---
[...]
> -/* The following function expect that the caller took care of reading the 
> index. */
> +/* The following functions expect that the caller took care of reading the 
> index. */
> +int has_unstaged_changes(void);
> +int has_uncommitted_changes(void);
>  int require_clean_work_tree(const char *action, const char *hint, int 
> gently);

Nice to see the fix in comment too.  Good work!

-- 
Jakub Narębski

Re: [PATCH v2 1/5] check_connected: accept an env argument

[Jeff King]

On Wed, Oct 05, 2016 at 09:01:57PM +0200, Jakub Narębski wrote:

> > diff --git a/connected.h b/connected.h
> > index afa48cc..4ca325f 100644
> > --- a/connected.h
> > +++ b/connected.h
> > @@ -33,6 +33,11 @@ struct check_connected_options {
> >  
> > /* If non-zero, show progress as we traverse the objects. */
> > int progress;
> > +
> > +   /*
> > +* Insert these variables into the environment of the child process.
> > +*/
> > +   const char **env;
> >  };
> 
> Just a nitpick, but I wonder why one comment is in single-line form,
> and the other uses block-form with a single line.

I think I wrote something longer originally, and then shortened it
before sending.

I don't generally think it matters much for a case like this (if it were
in the middle of code, I think the shorter form is worth doing, but here
it's basically a header for this variable).

-Peff

Re: [PATCH v9 09/14] pkt-line: add packet_write_gently()

[Lars Schneider]

> On 04 Oct 2016, at 21:33, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> 
>> +static int packet_write_gently(const int fd_out, const char *buf, size_t 
>> size)
>> +{
>> +static char packet_write_buffer[LARGE_PACKET_MAX];
>> +const size_t packet_size = size + 4;
>> +
>> +if (packet_size > sizeof(packet_write_buffer))
>> +return error("packet write failed - data exceeds max packet 
>> size");
> 
> Hmph, in the previous round, this used to be "is the size larger
> than sizeof(..) - 4?", which avoided integer overflow issue rather
> nicely and more idiomatic.  If size is near the size_t's max,
> packet_size may wrap around to become very small, and we won't hit
> this error, will we?

You are right. Would the solution below be acceptable?
I would like to keep the `packet_size` variable as it eases the rest
of the function.

 
const size_t packet_size = size + 4;
 
-   if (packet_size > sizeof(packet_write_buffer))
+   if (size > sizeof(packet_write_buffer) - 4)
return error("packet write failed - data exceeds max packet 
size");

Thanks,
Lars

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[Jeff King]

On Wed, Oct 05, 2016 at 08:47:29PM +0200, René Scharfe wrote:

> > Instead, let's provide a strbuf helper that does an in-place
> > normalize, but restores the original contents on error. This
> > uses a second buffer under the hood, which is slightly less
> > efficient, but this is not a performance-critical code path.
> 
> Hmm, in-place functions are quite rare in the strbuf collection.  It looks
> like a good fit for the two callers and makes sense in general, though.

Yeah, I almost wrote "strbuf_add_normalized_path()" instead. But then
the callers end up having to do the allocate-and-swap thing themselves.
And I think we're still set in the future to add that if somebody wants
it (and we can then implement the in-place version in terms of it).

Another alternative is to observe that the strbuf is generally used in
the first place to make the path absolute. So another interface is
perhaps something like:

  strbuf_add_path(struct strbuf *sb, const char *path,
  const char *relative_base)
  {
struct strbuf scratch = STRBUF_INIT;
int ret;

if (is_absolute_path(path))
strbuf_grow(sb, strlen(path));
else {
if (relative_path)
strbuf_addstr(, path);
else {
if (strbuf_getcwd())
return -1;
}
strbuf_addch(, '/');
strbuf_addstr(, path);

strbuf_grow(sb, scratch.len);
path = scratch.buf;
}

ret = normalize_path_copy(sb.buf + sb.len, path);
strbuf_release();
return ret;
  }

I don't think its worth the complexity of interface for the spots in
this series, but maybe there are other places that could use it. I'll
leave that to somebody else to explore if the ywant to.

-Peff

Re: [PATCH v2 1/5] check_connected: accept an env argument

[Jakub Narębski]

W dniu 03.10.2016 o 22:49, Jeff King pisze:

> diff --git a/connected.h b/connected.h
> index afa48cc..4ca325f 100644
> --- a/connected.h
> +++ b/connected.h
> @@ -33,6 +33,11 @@ struct check_connected_options {
>  
>   /* If non-zero, show progress as we traverse the objects. */
>   int progress;
> +
> + /*
> +  * Insert these variables into the environment of the child process.
> +  */
> + const char **env;
>  };

Just a nitpick, but I wonder why one comment is in single-line form,
and the other uses block-form with a single line.

-- 
Jakub Narębski, bikeshedding

Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull

[Junio C Hamano]

sorga...@gmail.com writes:

> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 351b8fc..ba5fb0a 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -23,10 +23,6 @@ named commits and the current branch, called "merge base", 
> is
>  calculated, and then net changes taken from the merge base to
>  the named commits are applied.
>  
> -This command is used by 'git pull' to incorporate changes from another
> -repository, and can be used by hand to merge changes from one branch
> -into another.
> -

Good.

> @@ -119,18 +115,17 @@ of `git fetch` for merging are merged to the current 
> branch.
>  PRE-MERGE CHECKS
>  
>  
> -Before applying outside changes, you should get your own work in
> -good shape and committed locally, so it will not be clobbered if
> -there are conflicts.  See also linkgit:git-stash[1].
> -'git pull' and 'git merge' will stop without doing anything when
> -local uncommitted changes overlap with files that 'git pull'/'git
> -merge' may need to update.
> +Before applying outside changes, you should get your own work in good
> +shape and committed locally, so it will not be clobbered if there are
> +conflicts. See also linkgit:git-stash[1]. 'git merge' will stop
> +without doing anything when local uncommitted changes overlap with
> +files that 'git merge' may need to update.
>  
> -To avoid recording unrelated changes in the merge commit,
> -'git pull' and 'git merge' will also abort if there are any changes
> -registered in the index relative to the `HEAD` commit.  (One
> -exception is when the changed index entries are in the state that
> -would result from the merge already.)
> +To avoid recording unrelated changes in the merge commit, 'git merge'
> +will also abort if there are any changes registered in the index
> +relative to the `HEAD` commit. (One exception is when the changed
> +index entries are in the state that would result from the merge
> +already.)

OK, so "git pull and git merge" have been updated to say "git
merge" and there is no other change.  Looks good.

Please do not re-flow and change in the same commit, by the way.

> @@ -138,14 +133,15 @@ will exit early with the message "Already up-to-date."
>  FAST-FORWARD MERGE
>  --
>  
> -Often the current branch head is an ancestor of the named commit.
> +Often the current branch head is an ancestor of the named commit.  In
> +this case, a new commit is not needed to store the combined history;
> +instead, the `HEAD` (along with the index) is updated to point at the
> +named commit, without creating an extra merge commit.
> +
>  This is the most common case especially when invoked from 'git
>  pull': you are tracking an upstream repository, you have committed
>  no local changes, and now you want to update to a newer upstream
> -revision.  In this case, a new commit is not needed to store the
> -combined history; instead, the `HEAD` (along with the index) is
> -updated to point at the named commit, without creating an extra
> -merge commit.
> +revision.

I am not sure if the post-image of this hunk is better than the
original.

Re: [PATCH 0/18] alternate object database cleanups

[René Scharfe]

Am 03.10.2016 um 22:33 schrieb Jeff King:

This series is the result of René nerd-sniping me with the claim that we
could "easily" teach count-objects to print out the list of alternates
in:

  http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161...@web.de/


1. Send crappy patch
2. 
3. PROFIT!!!

Sometimes it works. :)

Thank you!
René

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[René Scharfe]

Am 03.10.2016 um 22:34 schrieb Jeff King:

When we add a new alternate to the list, we try to normalize
out any redundant "..", etc. However, we do not look at the
return value of normalize_path_copy(), and will happily
continue with a path that could not be normalized. Worse,
the normalizing process is done in-place, so we are left
with whatever half-finished working state the normalizing
function was in.

Fortunately, this cannot cause us to read past the end of
our buffer, as that working state will always leave the
NUL from the original path in place. And we do tend to
notice problems when we check is_directory() on the path.
But you can see the nonsense that we feed to is_directory
with an entry like:

  this/../../is/../../way/../../too/../../deep/../../to/../../resolve

in your objects/info/alternates, which yields:

  error: object directory
  
/to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve
  does not exist; check .git/objects/info/alternates.

We can easily fix this just by checking the return value.
But that makes it hard to generate a good error message,
since we're normalizing in-place and our input value has
been overwritten by cruft.

Instead, let's provide a strbuf helper that does an in-place
normalize, but restores the original contents on error. This
uses a second buffer under the hood, which is slightly less
efficient, but this is not a performance-critical code path.


Hmm, in-place functions are quite rare in the strbuf collection.  It 
looks like a good fit for the two callers and makes sense in general, 
though.

Re: [PATCH 16/18] count-objects: report alternates via verbose mode

[René Scharfe]

Am 03.10.2016 um 22:36 schrieb Jeff King:

There's no way to get the list of alternates that git
computes internally; our tests only infer it based on which
objects are available. In addition to testing, knowing this
list may be helpful for somebody debugging their alternates
setup.

Let's add it to the "count-objects -v" output. We could give
it a separate flag, but there's not really any need.
"count-objects -v" is already a debugging catch-all for the
object database, its output is easily extensible to new data
items, and printing the alternates is not expensive (we
already had to find them to count the objects).


Good idea.


Signed-off-by: Jeff King 
---
 Documentation/git-count-objects.txt |  5 +
 builtin/count-objects.c | 10 ++
 t/t5613-info-alternate.sh   | 10 ++
 3 files changed, 25 insertions(+)

diff --git a/Documentation/git-count-objects.txt 
b/Documentation/git-count-objects.txt
index 2ff3568..cb9b4d2 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -38,6 +38,11 @@ objects nor valid packs
 +
 size-garbage: disk space consumed by garbage files, in KiB (unless -H is
 specified)
++
+alternate: absolute path of alternate object databases; may appear
+multiple times, one line per path. Note that if the path contains
+non-printable characters, it may be surrounded by double-quotes and
+contain C-style backslashed escape sequences.

 -H::
 --human-readable::
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..a700409 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -8,6 +8,7 @@
 #include "dir.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "quote.h"

 static unsigned long garbage;
 static off_t size_garbage;
@@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char 
*path, void *data)
return 0;
 }

+static int print_alternate(struct alternate_object_database *alt, void *data)
+{
+   printf("alternate: ");
+   quote_c_style(alt->path, NULL, stdout, 0);
+   putchar('\n');
+   return 0;
+}


Yeah, quoting paths makes sense.


+
 static char const * const count_objects_usage[] = {
N_("git count-objects [-v] [-H | --human-readable]"),
NULL
@@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
printf("prune-packable: %lu\n", packed_loose);
printf("garbage: %lu\n", garbage);
printf("size-garbage: %s\n", garbage_buf.buf);
+   foreach_alt_odb(print_alternate, NULL);
strbuf_release(_buf);
strbuf_release(_buf);
strbuf_release(_buf);
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index b393613..74f6770 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '
)
 '

+test_expect_success 'count-objects shows the alternates' '
+   cat >expect <<-EOF &&
+   alternate: $(pwd)/B/.git/objects
+   alternate: $(pwd)/A/.git/objects
+   EOF
+   git -C C count-objects -v >actual &&
+   grep ^alternate: actual >actual.alternates &&
+   test_cmp expect actual.alternates
+'
+
 # Note: These tests depend on the hard-coded value of 5 as "too deep". We start
 # the depth at 0 and count links, not repositories, so in a chain like:
 #

Re: Reference a submodule branch instead of a commit

[Heiko Voigt]

On Wed, Oct 05, 2016 at 09:13:53AM -0700, Junio C Hamano wrote:
> Heiko Voigt  writes:
> 
> >> It IS a hack, but having this information in .git would
> >> mean that it can be forced to be in machine readable form, unlike a
> >> mention in README.  I do not know if the .gitmodules/.gitignore
> >> combination is a sensible thing to use, but it does smell like a
> >> potentially useful hack.
> >
> > IIRC the tree entries are the reference for submodules in the code. We
> > are iterating over the tree entries in many places so that change does
> > not seem so easy to me.
> >
> > But you are right maybe we should stop arguing against this workflow and
> > just let people use it until they find out whats wrong with it ;)
> 
> I didn't say that, though.  I am fairly firm on _not_ changing what
> the superproject records in its tree for the submodule, i.e. it must
> record the exact commit, not "a branch name", for reproducibility. 

I was not talking about changing what the superproject records in its
tree. I was just talking about changing where we look for submodules
(e.g. for updating and such). I.e. in .git* instead of just the tree as
it is at the moment. Thats what I understood from the discussion above.
Sorry that might have been ambiguous.

I agree that there should always be a commit as a reference for a
submodule. But as far as I understand for some projects its to much
overhead to record every change of a submodule but still they want to
use the latest code during development. Those projects might only want
to record the actual commit when they release something. At least thats
what I imagine.

> I am OK if people ignored the unmatch between the recorded commit
> from a submodule and what they had in the submodule directory while
> they developed and tested the superproject commit.  After all, it is
> not an error to make a commit while having a local uncommitted
> changes to tracked files, and it is equally valid to have a commit
> checked out in a submodule directory that is different from what
> goes in the superproject commit.  But we do show "modified but not
> committed" in the status output.  In that light, submodule.*.ignore
> may have been a mistake.

The original intend for submodule.*.ignore was to help people not
showing submodules as dirty when they had untracked files in them. That
was after status learned to look into submodules. 'untracked' to avoid the
performance overhead and 'dirty' for the people that accidentally worked
with dirty submodules. I agree 'all' might have been to much.

For the above workflow what user might actually want is something that
ignores all changes as long as they are part of the remote branch. But I
am just guessing here. My gut feeling is still that most people that
request this feature come from svn. Thats why I asked whether the
options I described provide the behavior that Jeremy wants.

Cheers Heiko

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Junio C Hamano]

sorga...@gmail.com writes:

> From: Sergey Organov 
>
> Old description had a few problems:
>
> - sounded as if commits have changes
>
> - stated that changes are taken since some "divergence point"
>   that was not defined.
>
> New description rather uses "common ancestor" and "merge base",
> definitions of which are easily discoverable in the rest of GIT
> documentation.
>
> Signed-off-by: Sergey Organov 
> ---
>  Documentation/git-merge.txt | 25 +++--
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index cc0329d..351b8fc 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -16,11 +16,16 @@ SYNOPSIS
>  
>  DESCRIPTION
>  ---
> -Incorporates changes from the named commits (since the time their
> -histories diverged from the current branch) into the current
> -branch.  This command is used by 'git pull' to incorporate changes
> -from another repository and can be used by hand to merge changes
> -from one branch into another.
> +
> +Incorporates changes that lead to the named commits into the current
> +branch, and joins corresponding histories. The best common ancestor of
> +named commits and the current branch, called "merge base", is
> +calculated, and then net changes taken from the merge base to
> +the named commits are applied.
> +
> +This command is used by 'git pull' to incorporate changes from another
> +repository, and can be used by hand to merge changes from one branch
> +into another.

Content change together with re-flowing the text makes it more
costly than necessary to review a change like this.  Please avoid
doing so in your future patches.

I like what the updated description says very much.  I however
wonder if "and can be used by hand..." is still appropriate, or
needs a bit of modernizing.  It feels a bit awkward by making it
sound as if 'git merge' is primarily an implementation detail of
'git pull' but it can also be used as the first-class command, which
used to be the case in the old days back when "git pull . other" was
also perfectly good way to merge the 'other' branch from your own
repository, but I think your update is meant to clarify that we no
longer live in that old world ;-)

> @@ -31,11 +36,11 @@ Assume the following history exists and the current 
> branch is
>  D---E---F---G master
>  
>  
> -Then "`git merge topic`" will replay the changes made on the
> -`topic` branch since it diverged from `master` (i.e., `E`) until
> -its current commit (`C`) on top of `master`, and record the result
> -in a new commit along with the names of the two parent commits and
> -a log message from the user describing the changes.

> -Then "`git merge topic`" will replay the changes made on the `topic`
> -branch since it diverged from `master` (i.e., `E`) until its current
> -commit (`C`) on top of `master`, and record the result in a new commit
> -along with the names of the two parent commits and a log message from
> -the user describing the changes.

> +Then "`git merge topic`" will replay the changes made on the `topic`
> +branch since it diverged from `master` (i.e., `E`) until its current
> +commit (`C`) on top of `master`, and record the result in a new commit
> +along with references to the two parent commits and a log message from
> +the user describing the changes.

Content change together with re-flowing the text makes it more
costly than necessary to review a change like this.  Please avoid
doing so in your future patches.

I had to re-flow the original you removed to match how you flowed in
the updated one and stare at it for a while to spot that the only
change was to rephrase "the names of the parents" to "references to
the parents".  I do not know if the updated phrasing is better.  The
"name" in the original was meant to be a short-hand for "object name",
and I would support a change to spell it out to clarify; "reference"
can be a vague word that can mean different things in Git, and when
the word is given without context, most Git people would think that
the word refers to "refs", but that is definitely not what the new
commit records, so...

Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME

[Jeff King]

On Wed, Oct 05, 2016 at 05:46:22PM +0300, sorga...@gmail.com wrote:

> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 216d2f4..cc0329d 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -3,7 +3,8 @@ git-merge(1)
>  
>  NAME
>  
> -git-merge - Join two or more development histories together
> +
> +git-merge - Merge one or more branches to the current branch

I wonder if we should be more clear that you don't have to merge a
branch; you can merge any commit. I do agree that the original was
unnecessarily general. And I think "the current branch" is accurate
(technically it can be to a detached HEAD, but that is pedantry that
doesn't need to make it into the synopsis).

So maybe "Merge one or more commits into the current branch".  I guess
that is a bit vague, too. It is really "commit tips" or "lines of
development" that we are merging. Bringing them in of course brings in
many commits, but the "or more" there is meant to hint at multi-parent
merges.

So perhaps "one or more branches", while not completely accurate, is the
best we can do. I dunno.

-Peff

Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME

[Junio C Hamano]

sorga...@gmail.com writes:

> From: Sergey Organov 
>
> Old description not only raised the question of why the tool is called
> git-merge rather than git-join, but "join histories" also sounds like
> very simple operation, something like what "git-merge -s ours" does.
>
> Signed-off-by: Sergey Organov 
> ---
>  Documentation/git-merge.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 216d2f4..cc0329d 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -3,7 +3,8 @@ git-merge(1)
>  
>  NAME
>  
> -git-merge - Join two or more development histories together
> +
> +git-merge - Merge one or more branches to the current branch

This patch, evaluated by itself, looks like a regression in that it
tries to explain "merge" by using verb "merge", making it fuzzier to
those who do not yet know what a "merge" is.  That was why it tried
to explain "merge" as an operation to join histories.

However, the next one, 5/6, resurrects the "join history" in the
description part to help them, so the damage is not so severe when
we take them together.

I haven't formed firm opinion on this patch yet.

Re: [PATCH 2/6] Documentation/git-merge.txt: remove list of options from SYNOPSIS

[Junio C Hamano]

sorga...@gmail.com writes:

> From: Sergey Organov 
>
> This partial list of option is confusing as it lacks a lot of
> available options. It also clutters the SYNOPSIS making differences
> between forms of invocation less clear.
>
> Signed-off-by: Sergey Organov 
> ---
>  Documentation/git-merge.txt | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index b758d55..90342eb 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -9,10 +9,7 @@ git-merge - Join two or more development histories together
>  SYNOPSIS
>  
>  [verse]
> -'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
> - [-s ] [-X ] [-S[]]
> - [--[no-]allow-unrelated-histories]
> - [--[no-]rerere-autoupdate] [-m ] [...]
> +'git merge' [options] [-m ] [...]
>  'git merge'  HEAD ...
>  'git merge' --abort

Same comment as 1/6; as we'd hopefully be removing the deprecated
form soonish, it would probably make sense to leave only two, i.e.

git merge [options] [...]
git merge --abort

in synposis.

Re: [PATCH 1/6] git-merge: clarify "usage" by adding "-m "

[Junio C Hamano]

sorga...@gmail.com writes:

> From: Sergey Organov 
>
> "-m " is one of essential distinctions between obsolete
> invocation form and the recent one. Add it to the "usage" returned by
> 'git merge -h' for more clarity.
>
> Signed-off-by: Sergey Organov 
> ---
>  builtin/merge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8b57c7..0e367ba 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -43,7 +43,7 @@ struct strategy {
>  };
>  
>  static const char * const builtin_merge_usage[] = {
> - N_("git merge [] [...]"),
> + N_("git merge [] [-m ] [...]"),
>   N_("git merge []  HEAD "),
>   N_("git merge --abort"),
>   NULL

While this is not wrong per-se, as the deprecated form will go away
soon, I hope you do not mind if I had to drop this one from the
series to avoid merge conflicts to 'pu' (I do not know how bad the
conflict would be yet; I am just reviewing in my MUA).

Re: [PATCH v2 19/25] sequencer: remember do_recursive_merge()'s return value

[Junio C Hamano]

Johannes Schindelin  writes:

> Actually, come to think of it, I will change the patch, as it is too
> confusing. What I want is to preserve a positive return value in case of
> merge conflicts, and that is exactly what I should do instead of playing
> games with the Boolean OR operator.

That would be good; that was exactly the confusion I felt that led
to my comments.

Re: [PATCH v2 04/25] sequencer: future-proof remove_sequencer_state()

[Junio C Hamano]

Johannes Schindelin  writes:

> I briefly considered consolidating them and using .git/rebase-merge/ as
> state directory also for cherry-pick/revert, but that would cause
> problems: I am surely not the only user who cherry-picks commits manually
> while running interactive rebases.

Good thinking.

Re: [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams

[Lars Schneider]

> On 04 Oct 2016, at 21:53, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
> 
>> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
>> +{
>> +static char buf[LARGE_PACKET_DATA_MAX];
>> +int err = 0;
>> +size_t bytes_written = 0;
>> +size_t bytes_to_write;
>> +
>> +while (!err) {
>> +if ((len - bytes_written) > sizeof(buf))
>> +bytes_to_write = sizeof(buf);
>> +else
>> +bytes_to_write = len - bytes_written;
>> +if (bytes_to_write == 0)
>> +break;
>> +err = packet_write_gently(fd_out, src_in + bytes_written, 
>> bytes_to_write);
>> +bytes_written += bytes_to_write;
>> +}
>> +if (!err)
>> +err = packet_flush_gently(fd_out);
>> +return err;
>> +}
> 
> Hmph, what is buf[] used for, other than its sizeof() taken to yield
> a constant LARGE_PACKET_DATA_MAX?

Agreed. This is stupid. I will fix it.


>> 
>> +for (;;) {
>> +strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
>> +packet_len = packet_read(fd_in, NULL, NULL,
>> +// TODO: explain + 1
> 
> No // C99 comment please.
> 
> And I agree that the +1 needs to be explained.

Oh. I did not send the very latest version :-(

Is this explanation OK?

+   strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
+   packet_len = packet_read(fd_in, NULL, NULL,
+   /* strbuf_grow() above always allocates one extra byte to
+* store a '\0' at the end of the string. packet_read()
+* writes a '\0' extra byte at the end, too. Let it know
+* that there is already room for the extra byte.
+*/
+   sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
+   PACKET_READ_GENTLE_ON_EOF);


Thanks,
Lars

[PATCH v3 13/14] i18n: send-email: mark string with interpolation for translation

[Vasco Almeida]

Mark warnings, errors and other messages that are interpolated for
translation.

We call sprintf() before calling die() and in few other circumstances in
order to replace the values on the placeholders.

Signed-off-by: Vasco Almeida 
---

I changed (y|N) to [y|N] around line 1750 to match with the others
questions style.

 git-send-email.perl | 90 -
 1 file changed, 48 insertions(+), 42 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 982c6c0..5c01425 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -279,10 +279,13 @@ sub signal_handler {
# tmp files from --compose
if (defined $compose_filename) {
if (-e $compose_filename) {
-   print "'$compose_filename' contains an intermediate 
version of the email you were composing.\n";
+   printf __("'%s' contains an intermediate version ".
+ "of the email you were composing.\n"),
+ $compose_filename;
}
if (-e ($compose_filename . ".final")) {
-   print "'$compose_filename.final' contains the composed 
email.\n"
+   printf __("'%s.final' contains the composed email.\n"),
+ $compose_filename;
}
}
 
@@ -431,7 +434,7 @@ $smtp_encryption = '' unless (defined $smtp_encryption);
 my(%suppress_cc);
 if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
-   die "Unknown --suppress-cc field: '$entry'\n"
+   die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry)
unless $entry =~ 
/^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
$suppress_cc{$entry} = 1;
}
@@ -460,7 +463,7 @@ my $confirm_unconfigured = !defined $confirm;
 if ($confirm_unconfigured) {
$confirm = scalar %suppress_cc ? 'compose' : 'auto';
 };
-die "Unknown --confirm setting: '$confirm'\n"
+die sprintf(__("Unknown --confirm setting: '%s'\n"), $confirm)
unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
 
 # Debugging, print out the suppressions.
@@ -492,16 +495,16 @@ my %aliases;
 sub parse_sendmail_alias {
local $_ = shift;
if (/"/) {
-   print STDERR "warning: sendmail alias with quotes is not 
supported: $_\n";
+   printf STDERR __("warning: sendmail alias with quotes is not 
supported: %s\n"), $_;
} elsif (/:include:/) {
-   print STDERR "warning: `:include:` not supported: $_\n";
+   printf STDERR __("warning: `:include:` not supported: %s\n"), 
$_;
} elsif (/[\/|]/) {
-   print STDERR "warning: `/file` or `|pipe` redirection not 
supported: $_\n";
+   printf STDERR __("warning: `/file` or `|pipe` redirection not 
supported: %s\n"), $_;
} elsif (/^(\S+?)\s*:\s*(.+)$/) {
my ($alias, $addr) = ($1, $2);
$aliases{$alias} = [ split_addrs($addr) ];
} else {
-   print STDERR "warning: sendmail line is not recognized: $_\n";
+   printf STDERR __("warning: sendmail line is not recognized: 
%s\n"), $_;
}
 }
 
@@ -582,13 +585,12 @@ sub is_format_patch_arg {
if (defined($format_patch)) {
return $format_patch;
}
-   die(<

[PATCH v3 09/14] i18n: add--interactive: remove %patch_modes entries

[Vasco Almeida]

Remove unnecessary entries from %patch_modes. After the i18n conversion,
these entries are not used anymore.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 21 -
 1 file changed, 21 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5356d5a..e81939f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -106,9 +106,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stage',
-   TARGET => '',
-   PARTICIPLE => 'staging',
FILTER => 'file-only',
IS_REVERSE => 0,
},
@@ -116,9 +113,6 @@ my %patch_modes = (
DIFF => 'diff-index -p HEAD',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stash',
-   TARGET => '',
-   PARTICIPLE => 'stashing',
FILTER => undef,
IS_REVERSE => 0,
},
@@ -126,9 +120,6 @@ my %patch_modes = (
DIFF => 'diff-index -p --cached',
APPLY => sub { apply_patch 'apply -R --cached', @_; },
APPLY_CHECK => 'apply -R --cached',
-   VERB => 'Unstage',
-   TARGET => '',
-   PARTICIPLE => 'unstaging',
FILTER => 'index-only',
IS_REVERSE => 1,
},
@@ -136,9 +127,6 @@ my %patch_modes = (
DIFF => 'diff-index -R -p --cached',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Apply',
-   TARGET => ' to index',
-   PARTICIPLE => 'applying',
FILTER => 'index-only',
IS_REVERSE => 0,
},
@@ -146,9 +134,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply -R', @_; },
APPLY_CHECK => 'apply -R',
-   VERB => 'Discard',
-   TARGET => ' from worktree',
-   PARTICIPLE => 'discarding',
FILTER => 'file-only',
IS_REVERSE => 1,
},
@@ -156,9 +141,6 @@ my %patch_modes = (
DIFF => 'diff-index -p',
APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
APPLY_CHECK => 'apply -R',
-   VERB => 'Discard',
-   TARGET => ' from index and worktree',
-   PARTICIPLE => 'discarding',
FILTER => undef,
IS_REVERSE => 1,
},
@@ -166,9 +148,6 @@ my %patch_modes = (
DIFF => 'diff-index -R -p',
APPLY => sub { apply_patch_for_checkout_commit '', @_ },
APPLY_CHECK => 'apply',
-   VERB => 'Apply',
-   TARGET => ' to index and worktree',
-   PARTICIPLE => 'applying',
FILTER => undef,
IS_REVERSE => 0,
},
-- 
2.7.4

[PATCH v3 12/14] i18n: send-email: mark warnings and errors for translation

[Vasco Almeida]

Mark warnings, errors and other messages for translation.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 78eb59b..982c6c0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -118,20 +118,20 @@ sub format_2822_time {
my $localmin = $localtm[1] + $localtm[2] * 60;
my $gmtmin = $gmttm[1] + $gmttm[2] * 60;
if ($localtm[0] != $gmttm[0]) {
-   die "local zone differs from GMT by a non-minute interval\n";
+   die __("local zone differs from GMT by a non-minute 
interval\n");
}
if ((($gmttm[6] + 1) % 7) == $localtm[6]) {
$localmin += 1440;
} elsif ((($gmttm[6] - 1) % 7) == $localtm[6]) {
$localmin -= 1440;
} elsif ($gmttm[6] != $localtm[6]) {
-   die "local time offset greater than or equal to 24 hours\n";
+   die __("local time offset greater than or equal to 24 hours\n");
}
my $offset = $localmin - $gmtmin;
my $offhour = $offset / 60;
my $offmin = abs($offset % 60);
if (abs($offhour) >= 24) {
-   die ("local time offset greater than or equal to 24 hours\n");
+   die __("local time offset greater than or equal to 24 hours\n");
}
 
return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
@@ -199,13 +199,13 @@ sub do_edit {
map {
system('sh', '-c', $editor.' "$@"', $editor, $_);
if (($? & 127) || ($? >> 8)) {
-   die("the editor exited uncleanly, aborting 
everything");
+   die(__("the editor exited uncleanly, aborting 
everything"));
}
} @_;
} else {
system('sh', '-c', $editor.' "$@"', $editor, @_);
if (($? & 127) || ($? >> 8)) {
-   die("the editor exited uncleanly, aborting everything");
+   die(__("the editor exited uncleanly, aborting 
everything"));
}
}
 }
@@ -299,7 +299,7 @@ my $help;
 my $rc = GetOptions("h" => \$help,
 "dump-aliases" => \$dump_aliases);
 usage() unless $rc;
-die "--dump-aliases incompatible with other options\n"
+die __("--dump-aliases incompatible with other options\n")
 if !$help and $dump_aliases and @ARGV;
 $rc = GetOptions(
"sender|from=s" => \$sender,
@@ -362,7 +362,7 @@ unless ($rc) {
 usage();
 }
 
-die "Cannot run git format-patch from outside a repository\n"
+die __("Cannot run git format-patch from outside a repository\n")
if $format_patch and not $repo;
 
 # Now, let's fill any that aren't set in with defaults:
@@ -617,7 +617,7 @@ while (defined(my $f = shift @ARGV)) {
 }
 
 if (@rev_list_opts) {
-   die "Cannot run git format-patch from outside a repository\n"
+   die __("Cannot run git format-patch from outside a repository\n")
unless $repo;
push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 
1), @rev_list_opts);
 }
@@ -638,7 +638,7 @@ if (@files) {
print $_,"\n" for (@files);
}
 } else {
-   print STDERR "\nNo patch files specified!\n\n";
+   print STDERR __("\nNo patch files specified!\n\n");
usage();
 }
 
@@ -730,7 +730,7 @@ EOT
$sender = $1;
next;
} elsif (/^(?:To|Cc|Bcc):/i) {
-   print "To/Cc/Bcc fields are not interpreted yet, they 
have been ignored\n";
+   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
next;
}
print $c2 $_;
@@ -739,7 +739,7 @@ EOT
close $c2;
 
if ($summary_empty) {
-   print "Summary email is empty, skipping it\n";
+   print __("Summary email is empty, skipping it\n");
$compose = -1;
}
 } elsif ($annotate) {
@@ -1314,7 +1314,7 @@ Message-Id: $message_id
$_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
 valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
 default => $ask_default);
-   die "Send this email reply required" unless defined $_;
+   die __("Send this email reply required") unless defined $_;
if (/^n/i) {
return 0;
} elsif (/^q/i) {
@@ -1340,7 +1340,7 @@ Message-Id: $message_id
} else {
 
if (!defined $smtp_server) {
-   die "The required SMTP server is not properly defined."
+   die __("The required SMTP server is not properly 
defined.")
}
 

[PATCH v3 10/14] i18n: add--interactive: mark status words for translation

[Vasco Almeida]

Mark words 'nothing', 'unchanged' and 'binary' used to display what has
been staged or not, in "git add -i" status command.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e81939f..0b4a27c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -295,7 +295,7 @@ sub list_modified {
my ($change, $bin);
$file = unquote_path($file);
if ($add eq '-' && $del eq '-') {
-   $change = 'binary';
+   $change = __('binary');
$bin = 1;
}
else {
@@ -304,7 +304,7 @@ sub list_modified {
$data{$file} = {
INDEX => $change,
BINARY => $bin,
-   FILE => 'nothing',
+   FILE => __('nothing'),
}
}
elsif (($adddel, $file) =
@@ -320,7 +320,7 @@ sub list_modified {
$file = unquote_path($file);
my ($change, $bin);
if ($add eq '-' && $del eq '-') {
-   $change = 'binary';
+   $change = __('binary');
$bin = 1;
}
else {
@@ -340,7 +340,7 @@ sub list_modified {
$file = unquote_path($2);
if (!exists $data{$file}) {
$data{$file} = +{
-   INDEX => 'unchanged',
+   INDEX => __('unchanged'),
BINARY => 0,
};
}
@@ -355,10 +355,10 @@ sub list_modified {
 
if ($only) {
if ($only eq 'index-only') {
-   next if ($it->{INDEX} eq 'unchanged');
+   next if ($it->{INDEX} eq __('unchanged'));
}
if ($only eq 'file-only') {
-   next if ($it->{FILE} eq 'nothing');
+   next if ($it->{FILE} eq __('nothing'));
}
}
push @return, +{
-- 
2.7.4

[PATCH v3 11/14] i18n: send-email: mark strings for translation

[Vasco Almeida]

Mark strings often displayed to the user for translation.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 52 ++--
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..78eb59b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
 use Error qw(:try);
 use Git;
+use Git::I18N;
 
 Getopt::Long::Configure qw/ pass_through /;
 
@@ -797,12 +798,12 @@ foreach my $f (@files) {
 }
 
 if (!defined $auto_8bit_encoding && scalar %broken_encoding) {
-   print "The following files are 8bit, but do not declare " .
-   "a Content-Transfer-Encoding.\n";
+   print __("The following files are 8bit, but do not declare " .
+"a Content-Transfer-Encoding.\n");
foreach my $f (sort keys %broken_encoding) {
print "$f\n";
}
-   $auto_8bit_encoding = ask("Which 8bit encoding should I declare 
[UTF-8]? ",
+   $auto_8bit_encoding = ask(__("Which 8bit encoding should I declare 
[UTF-8]? "),
  valid_re => qr/.{4}/, confirm_only => 1,
  default => "UTF-8");
 }
@@ -829,7 +830,7 @@ if (defined $sender) {
 # But it's a no-op to run sanitize_address on an already sanitized address.
 $sender = sanitize_address($sender);
 
-my $to_whom = "To whom should the emails be sent (if anyone)?";
+my $to_whom = __("To whom should the emails be sent (if anyone)?");
 my $prompting = 0;
 if (!@initial_to && !defined $to_cmd) {
my $to = ask("$to_whom ",
@@ -859,7 +860,7 @@ sub expand_one_alias {
 
 if ($thread && !defined $initial_reply_to && $prompting) {
$initial_reply_to = ask(
-   "Message-ID to be used as In-Reply-To for the first email (if 
any)? ",
+   __("Message-ID to be used as In-Reply-To for the first email 
(if any)? "),
default => "",
valid_re => qr/\@.*\./, confirm_only => 1);
 }
@@ -918,7 +919,10 @@ sub validate_address {
my $address = shift;
while (!extract_valid_address($address)) {
print STDERR "error: unable to extract a valid address from: 
$address\n";
-   $_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): 
",
+   # TRANSLATORS: Make sure to include [q] [d] [e] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = ask(__("What to do with this address? 
([q]uit|[d]rop|[e]dit): "),
valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
default => 'q');
if (/^d/i) {
@@ -1293,17 +1297,21 @@ Message-Id: $message_id
if ($needs_confirm eq "inform") {
$confirm_unconfigured = 0; # squelch this message for 
the rest of this run
$ask_default = "y"; # assume yes on EOF since user 
hasn't explicitly asked for confirmation
-   print "The Cc list above has been expanded by 
additional\n";
-   print "addresses found in the patch commit message. 
By default\n";
-   print "send-email prompts before sending whenever 
this occurs.\n";
-   print "This behavior is controlled by the 
sendemail.confirm\n";
-   print "configuration setting.\n";
-   print "\n";
-   print "For additional information, run 'git 
send-email --help'.\n";
-   print "To retain the current behavior, but squelch 
this message,\n";
-   print "run 'git config --global sendemail.confirm 
auto'.\n\n";
+   print __(
+"The Cc list above has been expanded by additional
+addresses found in the patch commit message. By default
+send-email prompts before sending whenever this occurs.
+This behavior is controlled by the sendemail.confirm
+configuration setting.
+
+For additional information, run 'git send-email --help'.
+To retain the current behavior, but squelch this message,
+run 'git config --global sendemail.confirm auto'."), "\n\n";
}
-   $_ = ask("Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
+   # TRANSLATORS: Make sure to include [y] [n] [q] [a] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
 valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
 default => $ask_default);
die "Send this email reply required" unless defined $_;
@@ -1405,7 +1413,7 @@ Message-Id: 

[PATCH v3 14/14] i18n: difftool: mark warnings for translation

[Vasco Almeida]

Signed-off-by: Vasco Almeida 
---
 git-difftool.perl | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d0..8d3632e 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,7 @@ use File::Path qw(mkpath rmtree);
 use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
+use Git::I18N;
 
 sub usage
 {
@@ -122,7 +123,7 @@ sub setup_dir_diff
my $i = 0;
while ($i < $#rawdiff) {
if ($rawdiff[$i] =~ /^::/) {
-   warn << 'EOF';
+   warn __ <<'EOF';
 Combined diff formats ('-c' and '--cc') are not supported in
 directory diff mode ('-d' and '--dir-diff').
 EOF
@@ -338,7 +339,7 @@ sub main
if (length($opts{difftool_cmd}) > 0) {
$ENV{GIT_DIFF_TOOL} = $opts{difftool_cmd};
} else {
-   print "No  given for --tool=\n";
+   print __("No  given for --tool=\n");
usage(1);
}
}
@@ -346,7 +347,7 @@ sub main
if (length($opts{extcmd}) > 0) {
$ENV{GIT_DIFFTOOL_EXTCMD} = $opts{extcmd};
} else {
-   print "No  given for --extcmd=\n";
+   print __("No  given for --extcmd=\n");
usage(1);
}
}
@@ -419,11 +420,11 @@ sub dir_diff
}
 
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) 
{
-   my $errmsg = "warning: Both files modified: ";
-   $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
-   $errmsg .= "warning: Working tree file has been 
left.\n";
-   $errmsg .= "warning:\n";
-   warn $errmsg;
+   warn sprintf(__(
+   "warning: Both files modified:\n" .
+   "'%s/%s' and '%s/%s'.\n" .
+   "warning: Working tree file has been left.\n" .
+   "warning:\n"), $workdir, $file, $b, $file);
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
@@ -435,8 +436,9 @@ sub dir_diff
}
}
if ($error) {
-   warn "warning: Temporary files exist in '$tmpdir'.\n";
-   warn "warning: You may want to cleanup or recover these.\n";
+   warn sprintf(__(
+   "warning: Temporary files exist in '%s'.\n" .
+   "warning: You may want to cleanup or recover 
these.\n"), $tmpdir);
exit(1);
} else {
exit_cleanup($tmpdir, $rc);
-- 
2.7.4

[PATCH v3 03/14] i18n: add--interactive: mark strings with interpolation for translation

[Vasco Almeida]

Since at this point Git::I18N.perl lacks support for Perl i18n
placeholder substitution, use of sprintf following die or error_msg is
necessary for placeholder substitution take place.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3c10ced..50bcf94 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -615,12 +615,12 @@ sub list_and_choose {
else {
$bottom = $top = find_unique($choice, @stuff);
if (!defined $bottom) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?\n"), 
$choice);
next TOPLOOP;
}
}
if ($opts->{SINGLETON} && $bottom != $top) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?"), $choice);
next TOPLOOP;
}
for ($i = $bottom-1; $i <= $top-1; $i++) {
@@ -1056,7 +1056,7 @@ sub edit_hunk_manually {
my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
my $fh;
open $fh, '>', $hunkfile
-   or die "failed to open hunk edit file for writing: " . $!;
+   or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
print $fh @$oldtext;
my $participle = $patch_mode_flavour{PARTICIPLE};
@@ -1083,7 +1083,7 @@ EOF
}
 
open $fh, '<', $hunkfile
-   or die "failed to open hunk edit file for reading: " . $!;
+   or die sprintf(__("failed to open hunk edit file for reading: 
%s"), $!);
my @newtext = grep { !/^#/ } <$fh>;
close $fh;
unlink $hunkfile;
@@ -1236,7 +1236,7 @@ sub apply_patch_for_checkout_commit {
 
 sub patch_update_cmd {
my @all_mods = list_modified($patch_mode_flavour{FILTER});
-   error_msg "ignoring unmerged: $_->{VALUE}\n"
+   error_msg sprintf(__("ignoring unmerged: %s\n"), $_->{VALUE})
for grep { $_->{UNMERGED} } @all_mods;
@all_mods = grep { !$_->{UNMERGED} } @all_mods;
 
@@ -1418,7 +1418,8 @@ sub patch_update_file {
chomp $response;
}
if ($response !~ /^\s*\d+\s*$/) {
-   error_msg "Invalid number: 
'$response'\n";
+   error_msg sprintf(__("Invalid number: 
'%s'\n"),
+$response);
} elsif (0 < $response && $response <= $num) {
$ix = $response - 1;
} else {
@@ -1460,7 +1461,7 @@ sub patch_update_file {
if ($@) {
my ($err,$exp) = ($@, $1);
$err =~ s/ at .*git-add--interactive 
line \d+,  line \d+.*$//;
-   error_msg "Malformed search regexp 
$exp: $err\n";
+   error_msg sprintf(__("Malformed search 
regexp %s: %s\n"), $exp, $err);
next;
}
my $iy = $ix;
@@ -1625,18 +1626,18 @@ sub process_args {
$patch_mode = $1;
$arg = shift @ARGV or die __("missing --");
} else {
-   die "unknown --patch mode: $1";
+   die sprintf(__("unknown --patch mode: %s"), $1);
}
} else {
$patch_mode = 'stage';
$arg = shift @ARGV or die __("missing --");
}
-   die "invalid argument $arg, expecting --"
-   unless $arg eq "--";
+   die sprintf(__("invalid argument %s, expecting --"),
+  $arg) unless $arg eq "--";
%patch_mode_flavour = %{$patch_modes{$patch_mode}};
}
elsif ($arg ne "--") {
-   die "invalid argument $arg, expecting --";
+   die sprintf(__("invalid argument %s, expecting --"), $arg);
}
 }
 
-- 
2.7.4

[PATCH v3 06/14] i18n: add--interactive: mark patch prompt for translation

[Vasco Almeida]

Mark prompt message assembled in place for translation, unfolding each
use case for each entry in the %patch_modes hash table.

Previously, this script relied on whether $patch_mode was set to run the
command patch_update_cmd() or show status and loop the main loop. Now,
it uses $cmd to indicate we must run patch_update_cmd() and $patch_mode
is used to tell which flavor of the %patch_modes are we on.  This is
introduced in order to be able to mark and unfold the message prompt
knowing in which context we are.

The tracking of context was done previously by point %patch_mode_flavour
hash table to the correct entry of %patch_modes, focusing only on value
of %patch_modes. Now, we are also interested in the key ('staged',
'stash', 'checkout_head', ...).

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 54 ---
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 6bbde2d..c8d5093 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -93,6 +93,7 @@ sub colored {
 }
 
 # command line options
+my $cmd;
 my $patch_mode;
 my $patch_mode_revision;
 
@@ -173,7 +174,8 @@ my %patch_modes = (
},
 );
 
-my %patch_mode_flavour = %{$patch_modes{stage}};
+$patch_mode = 'stage';
+my %patch_mode_flavour = %{$patch_modes{$patch_mode}};
 
 sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
@@ -1311,6 +1313,44 @@ sub display_hunks {
return $i;
 }
 
+my %patch_update_prompt_modes = (
+   stage => {
+   mode => __("Stage mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => __("Stage deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => __("Stage this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   stash => {
+   mode => __("Stash mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => __("Stash deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => __("Stash this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   reset_head => {
+   mode => __("Unstage mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => __("Unstage deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => __("Unstage this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   reset_nothead => {
+   mode => __("Apply mode change to index [y,n,q,a,d,/%s,?]? "),
+   deletion => __("Apply deletion to index [y,n,q,a,d,/%s,?]? "),
+   hunk => __("Apply this hunk to index [y,n,q,a,d,/%s,?]? "),
+   },
+   checkout_index => {
+   mode => __("Discard mode change from worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => __("Discard deletion from worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => __("Discard this hunk from worktree [y,n,q,a,d,/%s,?]? 
"),
+   },
+   checkout_head => {
+   mode => __("Discard mode change from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => __("Discard deletion from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => __("Discard this hunk from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   },
+   checkout_nothead => {
+   mode => __("Apply mode change to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => __("Apply deletion to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => __("Apply this hunk to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   },
+);
+
 sub patch_update_file {
my $quit = 0;
my ($ix, $num);
@@ -1383,12 +1423,9 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
-   print colored $prompt_color, $patch_mode_flavour{VERB},
- ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' :
-  $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
-  ' this hunk'),
- $patch_mode_flavour{TARGET},
- " [y,n,q,a,d,/$other,?]? ";
+   print colored $prompt_color,
+   
sprintf($patch_update_prompt_modes{$patch_mode}{$hunk[$ix]{TYPE}}, $other);
+
my $line = prompt_single_character;
last unless defined $line;
if ($line) {
@@ -1644,6 +1681,7 @@ sub process_args {
die sprintf(__("invalid argument %s, expecting --"),
   $arg) unless $arg eq "--";
%patch_mode_flavour = %{$patch_modes{$patch_mode}};
+   $cmd = 1;
}
elsif ($arg ne "--") {
die sprintf(__("invalid argument %s, expecting --"), $arg);
@@ -1680,7 +1718,7 @@ sub main_loop {
 
 process_args();
 refresh();
-if ($patch_mode) {
+if ($cmd) {
patch_update_cmd();
 }
 else {
-- 
2.7.4

[PATCH v3 01/14] i18n: add--interactive: mark strings for translation

[Vasco Almeida]

Mark simple strings (without interpolation) for translation.

Brackets around first parameter of ternary operator is necessary because
otherwise xgettext fails to extract strings marked for translation from
the rest of the file.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 74 ++-
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812..da0255b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -4,6 +4,7 @@ use 5.008;
 use strict;
 use warnings;
 use Git;
+use Git::I18N;
 
 binmode(STDOUT, ":raw");
 
@@ -253,8 +254,9 @@ sub list_untracked {
run_cmd_pipe(qw(git ls-files --others --exclude-standard --), @ARGV);
 }
 
-my $status_fmt = '%12s %12s %s';
-my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
+# TRANSLATORS: you can adjust this to align "git add -i" status menu
+my $status_fmt = __('%12s %12s %s');
+my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), 
__('path'));
 
 {
my $initial;
@@ -680,7 +682,7 @@ sub update_cmd {
my @mods = list_modified('file-only');
return if (!@mods);
 
-   my @update = list_and_choose({ PROMPT => 'Update',
+   my @update = list_and_choose({ PROMPT => __('Update'),
   HEADER => $status_head, },
 @mods);
if (@update) {
@@ -692,7 +694,7 @@ sub update_cmd {
 }
 
 sub revert_cmd {
-   my @update = list_and_choose({ PROMPT => 'Revert',
+   my @update = list_and_choose({ PROMPT => __('Revert'),
   HEADER => $status_head, },
 list_modified());
if (@update) {
@@ -726,13 +728,13 @@ sub revert_cmd {
 }
 
 sub add_untracked_cmd {
-   my @add = list_and_choose({ PROMPT => 'Add untracked' },
+   my @add = list_and_choose({ PROMPT => __('Add untracked') },
  list_untracked());
if (@add) {
system(qw(git update-index --add --), @add);
say_n_paths('added', @add);
} else {
-   print "No untracked files.\n";
+   print __("No untracked files.\n");
}
print "\n";
 }
@@ -1166,8 +1168,14 @@ sub edit_hunk_loop {
}
else {
prompt_yesno(
-   'Your edited hunk does not apply. Edit again '
-   . '(saying "no" discards!) [y/n]? '
+   # TRANSLATORS: do not translate [y/n]
+   # The program will only accept that input
+   # at this point.
+   # Consider translating (saying "no" discards!) 
as
+   # (saying "n" for "no" discards!) if the 
translation
+   # of the word "no" does not start with n.
+   __('Your edited hunk does not apply. Edit again 
'
+  . '(saying "no" discards!) [y/n]? ')
) or return undef;
}
}
@@ -1213,11 +1221,11 @@ sub apply_patch_for_checkout_commit {
run_git_apply 'apply '.$reverse, @_;
return 1;
} elsif (!$applies_index) {
-   print colored $error_color, "The selected hunks do not apply to 
the index!\n";
-   if (prompt_yesno "Apply them to the worktree anyway? ") {
+   print colored $error_color, __("The selected hunks do not apply 
to the index!\n");
+   if (prompt_yesno __("Apply them to the worktree anyway? ")) {
return run_git_apply 'apply '.$reverse, @_;
} else {
-   print colored $error_color, "Nothing was applied.\n";
+   print colored $error_color, __("Nothing was 
applied.\n");
return 0;
}
} else {
@@ -1237,9 +1245,9 @@ sub patch_update_cmd {
 
if (!@mods) {
if (@all_mods) {
-   print STDERR "Only binary files changed.\n";
+   print STDERR __("Only binary files changed.\n");
} else {
-   print STDERR "No changes.\n";
+   print STDERR __("No changes.\n");
}
return 0;
}
@@ -1397,12 +1405,12 @@ sub patch_update_file {
my $response = $1;
my $no = $ix > 10 ? $ix - 10 : 0;
while ($response eq '') {
-   my $extra = "";
$no = display_hunks(\@hunk, $no);
if ($no < $num) {
- 

[PATCH v3 04/14] i18n: clean.c: match string with git-add--interactive.perl

[Vasco Almeida]

Change strings for help to match the ones in git-add--interactive.perl.
The strings now represent one entry to translate each rather then two
entries each different only by an ending newline character.

Signed-off-by: Vasco Almeida 
---
 builtin/clean.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..d6bc3aa 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -287,11 +287,11 @@ static void pretty_print_menus(struct string_list 
*menu_list)
 static void prompt_help_cmd(int singleton)
 {
clean_print_color(CLEAN_COLOR_HELP);
-   printf_ln(singleton ?
+   printf(singleton ?
  _("Prompt help:\n"
"1  - select a numbered item\n"
"foo- select item based on unique prefix\n"
-   "   - (empty) select nothing") :
+   "   - (empty) select nothing\n") :
  _("Prompt help:\n"
"1  - select a single item\n"
"3-5- select a range of items\n"
@@ -299,7 +299,7 @@ static void prompt_help_cmd(int singleton)
"foo- select item based on unique prefix\n"
"-...   - unselect specified items\n"
"*  - choose all items\n"
-   "   - (empty) finish selecting"));
+   "   - (empty) finish selecting\n"));
clean_print_color(CLEAN_COLOR_RESET);
 }
 
@@ -508,7 +508,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > 
top ||
(is_single && bottom != top)) {
clean_print_color(CLEAN_COLOR_ERROR);
-   printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+   printf(_("Huh (%s)?\n"), (*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
continue;
}
@@ -774,7 +774,7 @@ static int ask_each_cmd(void)
 static int quit_cmd(void)
 {
string_list_clear(_list, 0);
-   printf_ln(_("Bye."));
+   printf(_("Bye.\n"));
return MENU_RETURN_NO_LOOP;
 }
 
-- 
2.7.4

[PATCH v3 07/14] i18n: add--interactive: i18n of help_patch_cmd

[Vasco Almeida]

Mark help message of help_patch_cmd for translation.  The message must
be unfolded to be free of variables so we can have high quality
translations.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 54 ---
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index c8d5093..35967fe 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1189,15 +1189,53 @@ sub edit_hunk_loop {
}
 }
 
+my %help_patch_modes = (
+   stage => __(
+"y - stage this hunk
+n - do not stage this hunk
+q - quit; do not stage this hunk or any of the remaining ones
+a - stage this hunk and all later hunks in the file
+d - do not stage this hunk or any of the later hunks in the file"),
+   stash => __(
+"y - stash this hunk
+n - do not stash this hunk
+q - quit; do not stash this hunk or any of the remaining ones
+a - stash this hunk and all later hunks in the file
+d - do not stash this hunk or any of the later hunks in the file"),
+   reset_head => __(
+"y - unstage this hunk
+n - do not unstage this hunk
+q - quit; do not unstage this hunk or any of the remaining ones
+a - unstage this hunk and all later hunks in the file
+d - do not unstage this hunk or any of the later hunks in the file"),
+   reset_nothead => __(
+"y - apply this hunk to index
+n - do not apply this hunk to index
+q - quit; do not apply this hunk or any of the remaining ones
+a - apply this hunk and all later hunks in the file
+d - do not apply this hunk or any of the later hunks in the file"),
+   checkout_index => __(
+"y - discard this hunk from worktree
+n - do not discard this hunk from worktree
+q - quit; do not discard this hunk or any of the remaining ones
+a - discard this hunk and all later hunks in the file
+d - do not discard this hunk or any of the later hunks in the file"),
+   checkout_head => __(
+"y - discard this hunk from index and worktree
+n - do not discard this hunk from index and worktree
+q - quit; do not discard this hunk or any of the remaining ones
+a - discard this hunk and all later hunks in the file
+d - do not discard this hunk or any of the later hunks in the file"),
+   checkout_nothead => __(
+"y - apply this hunk to index and worktree
+n - do not apply this hunk to index and worktree
+q - quit; do not apply this hunk or any of the remaining ones
+a - apply this hunk and all later hunks in the file
+d - do not apply this hunk or any of the later hunks in the file"),
+);
+
 sub help_patch_cmd {
-   my $verb = lc $patch_mode_flavour{VERB};
-   my $target = $patch_mode_flavour{TARGET};
-   print colored $help_color, <

[PATCH v3 02/14] i18n: add--interactive: mark simple here-documents for translation

[Vasco Almeida]

Mark messages in here-document without interpolation for translation.

The here-document delimiter \EOF, which is the same as 'EOF', indicate
that the text is to be treated literally without interpolation of its
content.  Unfortunately xgettext is not able to extract here documents
with delimiter \EOF but it is with delimiter enclosed in single quotes.
Then change \EOF to 'EOF', although in this case does not make
difference what variation of here-document to use since there is nothing
to interpolate.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index da0255b..3c10ced 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -639,7 +639,7 @@ sub list_and_choose {
 }
 
 sub singleton_prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
+   print colored $help_color, __ <<'EOF' ;
 Prompt help:
 1  - select a numbered item
 foo- select item based on unique prefix
@@ -648,7 +648,7 @@ EOF
 }
 
 sub prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
+   print colored $help_color, __ <<'EOF' ;
 Prompt help:
 1  - select a single item
 3-5- select a range of items
@@ -1584,7 +1584,9 @@ sub quit_cmd {
 }
 
 sub help_cmd {
-   print colored $help_color, <<\EOF ;
+# TRANSLATORS: please do not translate the command names
+# 'status', 'update', 'revert', etc.
+   print colored $help_color, __ <<'EOF' ;
 status- show paths with changes
 update- add working tree state to the staged set of changes
 revert- revert staged set of changes back to the HEAD version
-- 
2.7.4

[PATCH v3 05/14] i18n: add--interactive: mark plural strings

[Vasco Almeida]

Mark plural strings for translation.  Unfold each action case in one
entire sentence.

Pass new keyword for xgettext to extract.

Update test to include new subroutine __n() for plural strings handling.

Update documentation to include a description of the new __n() subroutine.

Signed-off-by: Vasco Almeida 
---

On the present re-roll, change name __Q to __n following the
Locale::Message convention.
Add documentation about __n().

 Makefile  |  3 ++-
 git-add--interactive.perl | 27 ++-
 perl/Git/I18N.pm  |  9 -
 t/t0202/test.pl   | 11 ++-
 4 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index 1aad150..4ef0344 100644
--- a/Makefile
+++ b/Makefile
@@ -2111,7 +2111,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
-XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+   --keyword=__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 50bcf94..6bbde2d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -669,12 +669,18 @@ sub status_cmd {
 sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
-   print "$did ";
-   if (1 < $cnt) {
-   print "$cnt paths\n";
-   }
-   else {
-   print "one path\n";
+   if ($did eq 'added') {
+   printf(__n("added %d path\n", "added %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'updated') {
+   printf(__n("updated %d path\n", "updated %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'reverted') {
+   printf(__n("reverted %d path\n", "reverted %d paths\n",
+  $cnt), $cnt);
+   } else {
+   printf(__n("touched %d path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1423,7 +1429,8 @@ sub patch_update_file {
} elsif (0 < $response && $response <= $num) {
$ix = $response - 1;
} else {
-   error_msg "Sorry, only $num hunks 
available.\n";
+   error_msg sprintf(__n("Sorry, only %d 
hunk available.\n",
+ "Sorry, only %d 
hunks available.\n", $num), $num);
}
next;
}
@@ -1518,8 +1525,10 @@ sub patch_update_file {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split = split_hunk($hunk[$ix]{TEXT}, 
$hunk[$ix]{DISPLAY});
if (1 < @split) {
-   print colored $header_color, "Split 
into ",
-   scalar(@split), " hunks.\n";
+   print colored $header_color, sprintf(
+   __n("Split into %d hunk.\n",
+   "Split into %d hunks.\n",
+   scalar(@split)), 
scalar(@split));
}
splice (@hunk, $ix, 1, @split);
$num = scalar @hunk;
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index f889fd6..3f7ac25 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
 }
 
-our @EXPORT = qw(__);
+our @EXPORT = qw(__ __n);
 our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
@@ -44,6 +44,7 @@ BEGIN
eval {
__bootstrap_locale_messages();
*__ = \::Messages::gettext;
+   *__n = \::Messages::ngettext;
1;
} or do {
# Tell test.pl that we couldn't load the gettext library.
@@ -51,6 +52,7 @@ BEGIN
 
# Just a fall-through no-op
*__ = sub ($) { $_[0] };
+   *__n = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
 }
 
@@ -70,6 +72,8 @@ Git::I18N - Perl interface to Git's Gettext localizations
 
printf __("The following error occurred: %s\n"), $error;
 
+   printf __n("commited %d file", "commited %d files", $files), $files;
+
 =head1 DESCRIPTION
 
 Git's internal Perl interface to gettext via L. If
@@ -87,6 +91,9 @@ it.
 L's gettext function if all goes well, otherwise our
 passthrough fallback function.
 

[PATCH v3 08/14] i18n: add--interactive: mark edit_hunk_manually message for translation

[Vasco Almeida]

Mark message of edit_hunk_manually displayed in the editing file when
user chooses 'e' option.  The message had to be unfolded to allow
translation of the $participle verb.

Some messages end up being exactly the same for some uses cases, but
left it for easier change in the future, e.g., wanting to change wording
of one particular use case.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 45 ++---
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35967fe..5356d5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1058,6 +1058,30 @@ sub color_diff {
} @_;
 }
 
+my %edit_hunk_manually_modes = (
+   stage => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for staging."),
+   stash => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for stashing."),
+   reset_head => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for unstaging."),
+   reset_nothead => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for applying."),
+   checkout_index => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for discarding"),
+   checkout_head => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for discarding."),
+   checkout_nothead => __(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for applying."),
+);
+
 sub edit_hunk_manually {
my ($oldtext) = @_;
 
@@ -1065,22 +1089,21 @@ sub edit_hunk_manually {
my $fh;
open $fh, '>', $hunkfile
or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
-   print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+   print $fh __("# Manual hunk edit mode -- see bottom for a quick 
guide\n");
print $fh @$oldtext;
-   my $participle = $patch_mode_flavour{PARTICIPLE};
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
-   print $fh <

[PATCH v3 00/14] Mark strings in Perl scripts for translation

[Vasco Almeida]

Mark messages in some perl scripts for translation.

Thanks for the reviews of Junio Hamano and Jakub Narębski.  Although I think
Jakub Narębski's suggestions are overall good, I am not willing to go that path
because I cannot see huge benefits from them given what we already have and
also I lack Perl skills.

Interdiff bellow.

Vasco Almeida (14):
  i18n: add--interactive: mark strings for translation
  i18n: add--interactive: mark simple here-documents for translation
  i18n: add--interactive: mark strings with interpolation for
translation
  i18n: clean.c: match string with git-add--interactive.perl
  i18n: add--interactive: mark plural strings
  i18n: add--interactive: mark patch prompt for translation
  i18n: add--interactive: i18n of help_patch_cmd
  i18n: add--interactive: mark edit_hunk_manually message for
translation
  i18n: add--interactive: remove %patch_modes entries
  i18n: add--interactive: mark status words for translation
  i18n: send-email: mark strings for translation
  i18n: send-email: mark warnings and errors for translation
  i18n: send-email: mark string with interpolation for translation
  i18n: difftool: mark warnings for translation

 Makefile  |   3 +-
 builtin/clean.c   |  10 +-
 git-add--interactive.perl | 318 ++
 git-difftool.perl |  22 ++--
 git-send-email.perl   | 176 +
 perl/Git/I18N.pm  |   9 +-
 t/t0202/test.pl   |  11 +-
 7 files changed, 340 insertions(+), 209 deletions(-)


diff --git a/Makefile b/Makefile
index fc29d85..4ef0344 100644
--- a/Makefile
+++ b/Makefile
@@ -2112,7 +2112,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
-   --keyword=__ --keyword="Q__:1,2"
+   --keyword=__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..d6bc3aa 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -287,11 +287,11 @@ static void pretty_print_menus(struct string_list 
*menu_list)
 static void prompt_help_cmd(int singleton)
 {
clean_print_color(CLEAN_COLOR_HELP);
-   printf_ln(singleton ?
+   printf(singleton ?
  _("Prompt help:\n"
"1  - select a numbered item\n"
"foo- select item based on unique prefix\n"
-   "   - (empty) select nothing") :
+   "   - (empty) select nothing\n") :
  _("Prompt help:\n"
"1  - select a single item\n"
"3-5- select a range of items\n"
@@ -299,7 +299,7 @@ static void prompt_help_cmd(int singleton)
"foo- select item based on unique prefix\n"
"-...   - unselect specified items\n"
"*  - choose all items\n"
-   "   - (empty) finish selecting"));
+   "   - (empty) finish selecting\n"));
clean_print_color(CLEAN_COLOR_RESET);
 }
 
@@ -508,7 +508,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > 
top ||
(is_single && bottom != top)) {
clean_print_color(CLEAN_COLOR_ERROR);
-   printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+   printf(_("Huh (%s)?\n"), (*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
continue;
}
@@ -774,7 +774,7 @@ static int ask_each_cmd(void)
 static int quit_cmd(void)
 {
string_list_clear(_list, 0);
-   printf_ln(_("Bye."));
+   printf(_("Bye.\n"));
return MENU_RETURN_NO_LOOP;
 }
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b72cc97..0b4a27c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -106,9 +106,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stage',
-   TARGET => '',
-   PARTICIPLE => 'staging',
FILTER => 'file-only',
IS_REVERSE => 0,
},
@@ -116,9 +113,6 @@ my %patch_modes = (
DIFF => 'diff-index -p HEAD',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stash',
-   TARGET => '',
-   PARTICIPLE => 'stashing',
FILTER => undef,
IS_REVERSE => 0,

Re: git commit -p with file arguments

[Junio C Hamano]

Duy Nguyen  writes:

>> At the least I think we should clarify this in the document.
>
> How about something like this? Would it help?
>
> -- 8< --
> Subject: [PATCH] git-commit.txt: clarify --patch mode with pathspec
>
> How pathspec is used, with and without --interactive/--patch, is
> different. But this is not clear from the document. These changes hint
> the user to keep reading (to option #5) instead of stopping at #2 and
> assuming --patch/--interactive behaves the same way.
>
> And since all the options listed here always mention how the index is
> involved (or not) in the final commit, add that bit for #5 as well. This
> "on top of the index" is implied when you head over git-add(1), but if
> you just go straight to the "Interactive mode" and not read what git-add
> is for, you may miss it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Excellent.  

>  Documentation/git-commit.txt | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index b0a294d..f2ab0ee 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -29,7 +29,8 @@ The content to be added can be specified in several ways:
>  2. by using 'git rm' to remove files from the working tree
> and the index, again before using the 'commit' command;
>  
> -3. by listing files as arguments to the 'commit' command, in which
> +3. by listing files as arguments to the 'commit' command
> +   (without --interactive or --patch switch), in which
> case the commit will ignore changes staged in the index, and instead
> record the current content of the listed files (which must already
> be known to Git);
> @@ -41,7 +42,8 @@ The content to be added can be specified in several ways:
> actual commit;
>  
>  5. by using the --interactive or --patch switches with the 'commit' command
> -   to decide one by one which files or hunks should be part of the commit,
> +   to decide one by one which files or hunks should be part of the commit
> +   in addition to contents in the index,
> before finalizing the operation. See the ``Interactive Mode'' section of
> linkgit:git-add[1] to learn how to operate these modes.

When re-reading these 5 bullet points, I feel there may be some
unrelated updates needed to clarify (e.g. it is unclear when reading
the description pretending to be a newbie, if it is the "changes"
that is recorded in the index, or if it is the "state"; the answer
is the latter but if the reader's world model is still the former,
the description can mislead to expect a different behaviour).

But regardless of such remaining potential issues, this is a clearly
good change.  Thanks.

Re: [RFC/PATCH] attr: Document a new possible thread safe API

[Stefan Beller]

On Wed, Oct 5, 2016 at 10:00 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> I thought about that, but as I concluded that the get_all_attrs doesn't need
>> conversion to a threading environment, we can keep it as is.
>
> I agree that it is OK for get_all_attrs() to use its own way to ask
> a question and receive an answer to it, that is different from how
> git_check_attr() asks its question.  The threading-support for it is
> an unrelated issue, though (not that I think it needs to be run from
> a multi-threaded caller).
>
>>> ...  I'd expect the most
>>> typical caller to be
>>>
>>> static struct git_attr_check *check;
>>> struct git_attr_result result[2]; /* we want two */
>>>
>>> git_attr_check_initl(, "crlf", "ident", NULL);
>>> git_check_attr(path, check, result);
>>> /* result[0] has "crlf", result[1] has "ident" */
>>>
>>> or something like that.
>>
>> I see, that seems to be a clean API. So git_attr_check_initl
>> will lock the mutex and once it got the mutex it can either
>> * return early as someone else did the work
>> * needs to do the actual work
>> and then unlock. In any case the work was done.
>>
>> git_check_attr, which runs in all threads points to the same check,
>> but gets the different results.
>
> Yeah, something along that line.  It seems that we are now on the
> same page?
>

I think so, instead of resending the documentation, maybe the header
file shows that we're on the same page, I converted everything except
attr.c to follow this header attr.h:

---8<---
#ifndef ATTR_H
#define ATTR_H

/* An attribute is a pointer to this opaque structure */
struct git_attr;

/*
 * Given a string, return the gitattribute object that
 * corresponds to it.
 */
extern struct git_attr *git_attr(const char *);

/*
 * Return the name of the attribute represented by the argument.  The
 * return value is a pointer to a null-delimited string that is part
 * of the internal data structure; it should not be modified or freed.
 */
extern const char *git_attr_name(const struct git_attr *);

extern int attr_name_valid(const char *name, size_t namelen);
extern void invalid_attr_name_message(struct strbuf *, const char *, int);

/* Internal use */
extern const char git_attr__true[];
extern const char git_attr__false[];

/* For public to check git_attr_check results */
#define ATTR_TRUE(v) ((v) == git_attr__true)
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)

struct git_attr_check {
int finalized;
int check_nr;
int check_alloc;
struct git_attr **attr;
};

struct git_attr_result {
int check_nr;
int check_alloc;
const char **value;
};

/*
 * Initialize the `git_attr_check` via one of the following three functions:
 *
 * git_attr_check_alloc allocates an empty check, add more attributes via
 *  git_attr_check_append.
 * git_all_attrsallocates a check and fills in all attributes that
 *  are set for the given path.
 * git_attr_check_initl takes a pointer to where the check will be initialized,
 *  followed by all attributes that are to be checked.
 *  This makes it potentially thread safe as it could
 *  internally have a mutex for that memory location.
 *  Currently it is not thread safe!
 */
extern struct git_attr_check *git_attr_check_alloc(void);
extern void git_attr_check_append(struct git_attr_check *, const
struct git_attr *);
extern struct git_attr_check *git_all_attrs(const char *path);
extern void git_attr_check_initl(struct git_attr_check **, const char *, ...);

/* Query a path for its attributes */
extern struct git_attr_result *git_check_attr(const char *path,
 struct git_attr_check *);

/* internal? */
extern void git_attr_check_clear(struct git_attr_check *);

/* After use free the check */
extern void git_attr_check_free(struct git_attr_check *);

enum git_attr_direction {
GIT_ATTR_CHECKIN,
GIT_ATTR_CHECKOUT,
GIT_ATTR_INDEX
};
void git_attr_set_direction(enum git_attr_direction, struct index_state *);

#endif /* ATTR_H */

Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix

[Junio C Hamano]

SZEDER Gábor  writes:

> And a final sidenote: sorting based on the longest matching suffix
> also allows us to (ab)use version sort with prerelease suffixes to
> sort postrelease tags as we please, too:
>
>   $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \
>   -c versionsort.prereleasesuffix=-beta \
>   -c versionsort.prereleasesuffix= \
>   -c versionsort.prereleasesuffix=-gamma \
>   -c versionsort.prereleasesuffix=-delta \
>   tag -l --sort=version:refname 'v3.0*'
>   v3.0-alpha1
>   v3.0-beta1
>   v3.0
>   v3.0-gamma1
>   v3.0-delta1

Assuming that gamma and delta are meant to indicate "these are
post-release updates", I think a mechanism that can yield the above
result is fantastic ;-)

Re: [RFC/PATCH] attr: Document a new possible thread safe API

[Junio C Hamano]

Stefan Beller  writes:

> I thought about that, but as I concluded that the get_all_attrs doesn't need
> conversion to a threading environment, we can keep it as is.

I agree that it is OK for get_all_attrs() to use its own way to ask
a question and receive an answer to it, that is different from how
git_check_attr() asks its question.  The threading-support for it is
an unrelated issue, though (not that I think it needs to be run from
a multi-threaded caller).

>> ...  I'd expect the most
>> typical caller to be
>>
>> static struct git_attr_check *check;
>> struct git_attr_result result[2]; /* we want two */
>>
>> git_attr_check_initl(, "crlf", "ident", NULL);
>> git_check_attr(path, check, result);
>> /* result[0] has "crlf", result[1] has "ident" */
>>
>> or something like that.
>
> I see, that seems to be a clean API. So git_attr_check_initl
> will lock the mutex and once it got the mutex it can either
> * return early as someone else did the work
> * needs to do the actual work
> and then unlock. In any case the work was done.
>
> git_check_attr, which runs in all threads points to the same check,
> but gets the different results.

Yeah, something along that line.  It seems that we are now on the
same page?

Thanks.

Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[Jakub Narębski]

W dniu 05.10.2016 o 16:46, sorga...@gmail.com pisze:
> From: Sergey Organov 
> 
> Old description had a few problems:
> 
> - sounded as if commits have changes
> 
> - stated that changes are taken since some "divergence point"
>   that was not defined.
> 
> New description rather uses "common ancestor" and "merge base",
> definitions of which are easily discoverable in the rest of GIT
> documentation.

This is a step in a good direction, but it has a few issues.

> 
> Signed-off-by: Sergey Organov 
> ---
>  Documentation/git-merge.txt | 25 +++--
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index cc0329d..351b8fc 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -16,11 +16,16 @@ SYNOPSIS
>  
>  DESCRIPTION
>  ---
> -Incorporates changes from the named commits (since the time their
> -histories diverged from the current branch) into the current
> -branch.  This command is used by 'git pull' to incorporate changes
> -from another repository and can be used by hand to merge changes
> -from one branch into another.
> +
> +Incorporates changes that lead to the named commits into the current
> +branch, and joins corresponding histories. The best common ancestor of
> +named commits and the current branch, called "merge base", is
> +calculated, and then net changes taken from the merge base to
> +the named commits are applied.

The first sentence is all right; it reads better than the original
without the introduced part in parentheses.  The only minor issue
is with "joins corresponding histories" - it is a good description,
but may imply that the branch we are merging vanishes: it doesn't.
But all in all, it is a good change.

Second sentence has some problems.  First, while it is a good idea
to use well defined term "merge base", I think writing "since the
time their histories diverged" or "(which is the point where histories
diverged)" would be a good plain language description; it was removed
entirely in the proposal.

Second, while "common ancestor" and "least common ancestor" are well
defined in mathematics of graphs, "best common ancestor" isn't...
but this is what git-merge-base(1) documentation uses.

Also, the "best common ancestor" doesn't need to be only one.  There
might be many such ancestors... though Git would generate then a
virtual best common ancestor thanks to recursive merge strategy.
And usually there is only one "best common ancestor", that is a single
merge base.  So this may need clarification, but it is not much of
a problem.

Third, and most important, is that "net changes taken from the merge
base to the named commits are applied" is simply not true.  The
`git merge` command does not reapply changes - that is what rebase
and cherry-pick do.  The merge operation uses 3-way merge strategy
(diff3) between merge-base, current branch, and merged commit.
That is, it finds differences between differences, and "applies"
that.

See "A Formal Investigation of Diff3" paper by Sanjeev Khanna,
Keshav Kunal, and Benjamin C. Pierce:
  http://www.cis.upenn.edu/~bcpierce/papers/diff3-short.pdf

I'm not sure how to explain it succintly.  Perhaps

  net changes between merge base to the current (merged into)
  branch and named commits are integrated

There is description of trivial 3-way merge somewhere in Git docs,
though in very unobvious place; we can link it.

> +
> +This command is used by 'git pull' to incorporate changes from another
> +repository, and can be used by hand to merge changes from one branch
> +into another.

Rather "can be used by 'git pull'", or "is used by 'git pull' (unless
configured otherwise)"...

Separating this information makes a very good sense.  Thanks.

>  
>  Assume the following history exists and the current branch is
>  "`master`":
> @@ -31,11 +36,11 @@ Assume the following history exists and the current 
> branch is
>  D---E---F---G master
>  
>  
> -Then "`git merge topic`" will replay the changes made on the
> -`topic` branch since it diverged from `master` (i.e., `E`) until
> -its current commit (`C`) on top of `master`, and record the result
> -in a new commit along with the names of the two parent commits and
> -a log message from the user describing the changes.
> +Then "`git merge topic`" will replay the changes made on the `topic`
> +branch since it diverged from `master` (i.e., `E`) until its current
> +commit (`C`) on top of `master`, and record the result in a new commit
> +along with references to the two parent commits and a log message from
> +the user describing the changes.

What the happened here!?!  Please do not rewrap documentation, especially
not without changes!

>  
>  
> A---B---C topic
> 
-- 
Jakub Narębski

Re: [PATCH] clone: detect errors in normalize_path_copy

[Stefan Beller]

On Wed, Oct 5, 2016 at 7:29 AM, Jeff King  wrote:
> When we are copying the alternates from the source
> repository, if we find a relative path that is too deep for
> the source (e.g., "../../../objects" from "/repo.git/objects"),
> then normalize_path_copy will report an error and leave
> trash in the buffer, which we will add to our new alternates
> file. Instead, let's detect the error, print a warning, and
> skip copying that alternate.
>
> There's no need to die. The relative path is probably just
> broken cruft in the source repo. If it turns out to have
> been important for accessing some objects, we rely on other
> parts of the clone to detect that, just as they would with a
> missing object in the source repo itself (though note that
> clones with "-s" are inherently local, which may do fewer
> object-quality checks in the first place).

This explanation and the implementation make sense.
Thanks!

>
> Signed-off-by: Jeff King 
> ---
> Noticed by coverity;

I saw them, too and wanted to start preparing a patch,
but I cannot quite compete with your speed here. ;)

> the recent alternates cleanups mean that all of the
> other calls to normalize_path_copy() are now checked, so it realized
> this one was an oddball and probably an error (I actually looked for
> others with `grep` when doing that series, but somehow missed this one;
> hooray for static analysis). The fix is independent of that series.
>
>  builtin/clone.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index fb75f7e..6cf3b54 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -345,8 +345,11 @@ static void copy_alternates(struct strbuf *src, struct 
> strbuf *dst,
> continue;
> }
> abs_path = mkpathdup("%s/objects/%s", src_repo, line.buf);
> -   normalize_path_copy(abs_path, abs_path);
> -   add_to_alternates_file(abs_path);
> +   if (!normalize_path_copy(abs_path, abs_path))
> +   add_to_alternates_file(abs_path);
> +   else
> +   warning("skipping invalid relative alternate: %s/%s",
> +   src_repo, line.buf);
> free(abs_path);
> }
> strbuf_release();
> --
> 2.10.1.506.g904834d

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Wed, Oct 5, 2016 at 7:40 AM, Jeff King  wrote:
> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:
>
>> I would prefer the following:
>>
>> #   A --> B --> C --> D --> E --> F --> G --> H
>> #  0 1 2 3 4 5 6
>
> Yeah, that is also more visually pleasing.
>
> Here's a squashable update that uses that and clarifies the points in
> the discussion with Jacob.
>
> Junio, do you mind squashing this in to jk/alt-odb-cleanup?
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..62170b7 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
> )
>  '
>
> -# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> start
> -# the depth at 0 and count links, not repositories, so in a chain like:
> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth
> +# we will follow recursion. We start the depth at 0 and count links, not
> +# repositories. This means that in a chain like:
>  #
> -#   A -> B -> C -> D -> E -> F -> G -> H
> -#  0123456
> +#   A --> B --> C --> D --> E --> F --> G --> H
> +#  0 1 2 3 4 5 6

Yea this looks much better (when I view it locally, gmail still looks
aweful here but...)

>  #
> -# we are OK at "G", but break at "H".
> +# we are OK at "G", but break at "H", even though "H" is actually the 8th
> +# repository, not the 6th, which you might expect. Counting the links allows
> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
>  #

... This is much more clear wording that helps me understand this a
lot more. Thanks!

Regards,
Jake

>  # Note also that we must use "--bare -l" to make the link to H. The "-l"
>  # ensures we do not do a connectivity check, and the "--bare" makes sure
> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
> git clone --bare -l -s G H
>  '
>
> -test_expect_success 'validity of fifth-deep repository' '
> +test_expect_success 'validity of seventh repository' '
> git -C G fsck
>  '
>
> -test_expect_success 'invalidity of sixth-deep repository' '
> +test_expect_success 'invalidity of eighth repository' '
> test_must_fail git -C H fsck
>  '
>

RE: [musl] Re: Regression: git no longer works with musl libc's regex impl

[writeonce]

Hi Johannes,

> 
> 
>  Original Message 
> Subject: RE: [musl] Re: Regression: git no longer works with musl libc's
> regex impl
> From: Johannes Schindelin 
> Date: Wed, October 05, 2016 3:49 am
> To: writeo...@midipix.org
> Cc: m...@lists.openwall.com, git@vger.kernel.org, Jeff King
> 
> 
> Hi writeonce,
> 
> On Tue, 4 Oct 2016, writeo...@midipix.org wrote:
> 
> > < On Tue, 4 Oct 2016, Rich Felker wrote:
> > < 
> > < > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
> > < > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
> > < > > 
> > < > > > 1. is nonzero mod page size, it just works; the remainder of the
> > < > > > last page reads as zero bytes when mmapped.
> > < > > 
> > < > > Is that a portable assumption?
> > < > 
> > < > Yes.
> > < 
> > < No, it is not. You quote POSIX, but the matter of the fact is that we
> > < use a subset of POSIX in order to be able to keep things running on
> > < Windows.
> > 
> > As far as I can tell (and as the attached program may help demonstrate),
> > the above assumption has been valid on all versions of Windows since at
> > least Windows 2000.
> 
> And since W2K is already past its end of life, it would be safe for
> practical considerations.
> 
> However, I have to add two comments to that:
> 
> - it is *not* guaranteed. The behavior is undefined, even if you see
>  consistent behavior so far. Future Windows versions might break that
>  assumption freely, though.
> 

That is of course the official language, and generally speaking a good
rule of thumb. However... there is enough information to suggest that
when it comes to mapping of file-backed sections, the NT kernel
developers will choose to keep things the way they are. In brief, here's
why:

Given a "gray zone" that spans from EOF to end-of-page, there are in
essence three possible behaviors:

[1] bytes in the gray zone are accessible and are all zero's. This is
the current behavior.
[2] bytes in the gray zone are not accessible; trying to read past EOF
would result in a segfault.
[3] bytes in the gray zone are accessible but might contain random data
or junk.

Assessment:

[1] backward-compatible, POSIX-compliant, single code path for both
WIN32 and LXW.
[2] requires changing memory access granularity from 4096 bytes to a
single byte, and is therefore extremely costly.
[3] introduces a whole new class of security vulnerabilities, and will
thus be a lot of fun to watch:-)

All in all taken, then, I'd argue that relying on the current behavior
is very reasonable. If you, too, find the above assessment valid, and
since you mentioned that you were a Microsoft employee, it would be
great if you could make a good-faith effort to have the current behavior
added to the Driver Documentation and thus guaranteed.

PS. this isn't to say that the regex extension should or should not be
used, only that a decision on the matter should not be based on the
undocumentedness of current behavior.

midipix

> - some implementations of the REG_STARTEND feature have the nice property
>  that they can read past NUL characters. Granted, not all of them do
>  (AFAIU one example is FreeBSD itself, the first platform to sport
>  REG_STARTEND), but we at least reap the benefit whenever using a regex
>  that *can* read past NUL characters.
> 
> > In this context, one thing to remember is that the page-size for the mod
> > operation is 4096, whereas the POSIX page-size (for the purpose of mmap
> > and mremap) is 65536.
> 
> Indeed. A colleague of mine spotted the segfault when diffing a file that
> was *exactly* 4,096 bytes.
> 
> > Note also that in the case of file-backed mapped sections, using
> > kernel32.dll or msvcrt.dll or cygwin/newlib or midipix/musl is of little
> > significance, specifically since all invoke ZwCreateSection and
> > ZwMapViewOfSection under the hood.
> 
> Right. It's all backed by the very same kernel functions.
> 
> Ciao,
> Johannes

Re: [musl] Re: Regression: git no longer works with musl libc's regex impl

[Rich Felker]

On Wed, Oct 05, 2016 at 12:11:58PM -0400, Jeff King wrote:
> On Wed, Oct 05, 2016 at 10:59:34PM +1100, James B wrote:
> 
> > Number downloads does not make first-tier platform. You know that as
> > well as everyone else.
> > 
> > First-tier support is the decision made by the maintainers that the
> > entire features of the software must be available on those first tier
> > platforms. So if Windows is indeed first-tier platform for git, it
> > means any features that don't work on git version of Windows must not
> > be used/developed or even castrated. That's a scary thought.
> 
> Prepare to be scared, then, I guess. Ever since the msysgit project
> started years ago, we have made concessions in the code to work both
> with POSIX-ish systems and with the msys layer. E.g., see how git-daemon
> does not fork(), but actually re-spawns itself to handle connections.
> 
> When possible we try to put our abstractions at a level where they can
> be implemented in a performant way on all platforms (the git-daemon
> things is probably the _most_ ugly in that respect; I think nobody has
> really cared about the performance enough to add back in a forking code
> path for POSIX systems).
> 
> > So this decision that "Windows is now a first-tier platform for git" -
> > is your own opinion, or is this the collective opinion of *all* the
> > git maintainers?
> 
> There is only one maintainer of git: Junio. However, you'll note that I
> also used "we" in the paragraphs above. And that is because the approach
> I am talking about is something that has been done over the course of
> many years by many members of the development community.
> 
> You may disagree with that approach, but it is nothing new. The msysgit
> project started in 2007.

The goal of the midipix project is to make the need for FOSS projects
supporting Windows to do hacks like this obsolete. It still has a
little ways to go to be ready for mainstream use, but it's already
running a lot, and I hope you'll consider it for the future since it
simplifies things A LOT when you can just write to POSIX instead of
having to come up with abstraction layers that cater to Windows'
brokenness.

> > Well thank you for being honest. I can see now why you responded the
> > way you did (and still do). By being employed by Microsoft, and
> > especially paid to work on Git for Windows, you have all the
> > incentives to make it work best on Windows, and to make it as its
> > first-tier platform within the limitation of Windows.
> 
> Please don't insinuate that Johannes is a Microsoft shill. He has been
> working on the Windows port of Git for over 9 years, and was only
> employed by Microsoft this year. Furthermore, his original REG_STARTEND
> patch actually did a run-time fallback of NUL-terminating the input
> buffers. It was _I_ who suggested that we should simply push people
> towards our compat/regex routines instead. So if you want to be mad at
> somebody, be mad at me.

I hope we can get this thread away from accusing and attacking people
and on to doing productive things to make the software better.

Rich

Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable

[Junio C Hamano]

Johannes Schindelin  writes:

>> The function "git pull" uses to stop the user when the working
>> tree has changes is useful in other places.
>
> I stumbled over this sentence. How about
>
>   The function used by "git pull" to stop [...]
>
> instead?

Perfect. Thanks.

gitk and hook script

[GAUTHIER Geoffrey]

Hello,

I want to deploy a hook script to control format of commit message. For this I 
use the prepare-commit-msg script.

I want to check if the commit message starts with a number of 3 digits. If this 
is the case, the script returns 0, otherwise 1.

It works fine for an utilisation with the git command, but it doesn't when used 
with gitk. When script returns 1, the commit is not aborded. I don't manage to 
display message either (an information message explaining why commit will be 
abort).

Is there a way to use hook script with gitk?

Thanks.

PS: I use git version 1.8.5.3

 This electronic message and its attachments are confidential and transmitted 
for the exclusive use of their addressee. Should you receive this message by 
mistake, you are not authorized to use it for any purpose whatsoever; please 
delete it and notify the sender at once. LACROIX reserves the right to initiate 
any legal proceedings against any individual and organization in case of 
unauthorized use, without prejudice to possible criminal sanctions.

 Ce message et ses pièces jointes sont confidentiels et exclusivement transmis 
à l'usage de leur destinataire. Si vous recevez ce message par erreur, vous 
n’êtes pas autorisés à en faire une quelconque utilisation ; merci de le 
détruire et d'en avertir immédiatement l'expéditeur. LACROIX se réserve le 
droit de poursuivre toute entité, personne physique ou morale qui en ferait un 
usage non autorisé, sans préjudice d'éventuelles sanctions pénales.

Re: [musl] Re: Regression: git no longer works with musl libc's regex impl

[Rich Felker]

On Wed, Oct 05, 2016 at 03:11:05PM +0200, Jakub Narębski wrote:
> W dniu 05.10.2016 o 00:33, Rich Felker pisze:
> > On Wed, Oct 05, 2016 at 09:06:25AM +1100, James B wrote:
> >> On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
> >> Johannes Schindelin  wrote:
> >>>
> >>> No, it is not. You quote POSIX, but the matter of the fact is that we use
> >>> a subset of POSIX in order to be able to keep things running on Windows.
> >>>
> >>> And quite honestly, there are lots of reasons to keep things running on
> >>> Windows, and even to favor Windows support over musl support. Over four
> >>> million reasons: the Git for Windows users.
> >>
> >> Wow, I don't know that Windows is a git's first-tier platform now,
> >> and Linux/POSIX second. Are we talking about the same git that was
> >> originally written in Linus Torvalds, and is used to manage Linux
> >> kernel? Are you by any chance employed by Redmond, directly or
> >> indirectly?
> >>
> >> Sorry - can't help it.
> 
> Windows is one of the major platforms, yes.  I think there much, much
> more people using Git on Windows, than using Git with musl.  More
> users = more important.
> 
> Also, working with some inconvenience (requiring compilation with
> NO_REGEX=1) is better than not working at all.
> 
> In CodingGuidelines we say:
> 
>  - Most importantly, we never say "It's in POSIX; we'll happily
>ignore your needs should your system not conform to it."
>We live in the real world.
> 
>  - However, we often say "Let's stay away from that construct,
>it's not even in POSIX".

I agree wholeheartedly with these points.

> 
>  - In spite of the above two rules, we sometimes say "Although
>this is not in POSIX, it (is so convenient | makes the code
>much more readable | has other good characteristics) and
>practically all the platforms we care about support it, so
>let's use it".
> 
> The REG_STARTEND is 3rd point,

To begin with I wasn't clear that REG_STARDEND being nonstandard was
even noticed or compatibility considered when adding the dependency on
it, but it seems such discussion did take place and most targets have
it. Perhaps this means it should be proposed for standardization in
the next issue of POSIX.

> mmap shenningans looks like 1st...
> 
> on the other hand midipix  wrote in
> http://public-inbox.org/git/20161004200057.dc30d64f61e5ec441c34ffd4f788e58e.efa66ead67@email15.godaddy.com/
> that the proposed fix should work on all Windows version we are
> interested in (I think).  Test program included / attached.
> 
> The above-mentioned email also explains that the problem was
> caught on MS Windows; it triggers if file end falls on the mmapped
> page boundary, which is more likely to happen with 4096 mod size
> on Windows rather than 65536 mod size on Linux.

On Linux page-size (mmap granularity) varies by arch but it's 4k on
basically all archs that people care about. I think midipix's author
was talking about real page size on Windows (4k) vs the minimum
logical page size (mmap granularity) that can be used to get
POSIX-matching semantics in midipix (which is 64k due to some
technical reasons I forget, which he could probably remind me of).

> On the other hand, while the proposed solution of "add padding as
> to not end at page boundary, if necessary" doesn't have the
> performance impact of "memcpy into NUL-terminated buffer" that
> was originally proposed in patch series, it is still extra code
> to maintain.

*nod*

Rich

Re: [PATCH 1/3] add QSORT

[Kevin Bracey]

On 04/10/2016 23:31, René Scharfe wrote:



So let's summarize; here's the effect of a raw qsort(3) call:

array == NULL  nmemb  bug  QSORT  following NULL check
-  -  ---  -  
0  0  no   qsort  is skipped
0 >0  no   qsort  is skipped
1  0  no   qsort  is skipped (bad!) **
1 >0  yes  qsort  is skipped **

Right - row 3 may not be a bug from the point of view of your internals, 
but it means you violate the API of qsort.Therefore a fix is required.



With the micro-optimization removed (nmemb > 0) the matrix gets simpler:

array == NULL  nmemb  bug  QSORT  following NULL check
-  -  ---  -  
0  0  no   noop   is executed
0 >0  no   qsort  is skipped
1  0  no   noop   is executed
1 >0  yes  qsort  is skipped **

And with your NULL check (array != NULL) we'd get:

array == NULL  nmemb  bug  QSORT  following NULL check
-  -  ---  -  
0  0  no   qsort  reuses check result
0 >0  no   qsort  reuses check result
1  0  no   noop   reuses check result
1 >0  yes  noop   reuses check result

Did I get it right?  AFAICS all variants (except raw qsort) are safe 
-- no useful NULL checks are removed, and buggy code should be noticed 
by segfaults in code accessing the sorted array.

I think your tables are correct.

But I disagree that you could ever call invoking the "" lines safe. 
Unless you have documentation on what limit GCC (and your other 
compilers) are prepared to put on the undefined behaviour of violating 
that "non-null" constraint.


Up to now dereferencing a null pointer has been implicitly (or 
explicitly?) defined as simply generating SIGSEGV. And that has 
naturally extended into NULL passed to library implementations. But 
that's no longer true - it seems bets are somewhat off.


But, as long as you are confident you never invoke that line without a 
program bug - ie an API precondition of your own QSORT is that NULL is 
legal iff nmemb is zero, then I guess it's fine. Behaviour is defined, 
as long as you don't violate your internal preconditions.


Kevin

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Junio C Hamano]

Jeff King  writes:

> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:
>
>> I would prefer the following:
>> 
>> #   A --> B --> C --> D --> E --> F --> G --> H
>> #  0 1 2 3 4 5 6
>
> Yeah, that is also more visually pleasing.
>
> Here's a squashable update that uses that and clarifies the points in
> the discussion with Jacob.
>
> Junio, do you mind squashing this in to jk/alt-odb-cleanup?

No, I don't.

> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..62170b7 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
>   )
>  '
>  
> -# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> start
> -# the depth at 0 and count links, not repositories, so in a chain like:
> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth
> +# we will follow recursion. We start the depth at 0 and count links, not
> +# repositories. This means that in a chain like:
>  #
> -#   A -> B -> C -> D -> E -> F -> G -> H
> -#  0123456
> +#   A --> B --> C --> D --> E --> F --> G --> H
> +#  0 1 2 3 4 5 6
>  #
> -# we are OK at "G", but break at "H".
> +# we are OK at "G", but break at "H", even though "H" is actually the 8th
> +# repository, not the 6th, which you might expect. Counting the links allows
> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
>  #
>  # Note also that we must use "--bare -l" to make the link to H. The "-l"
>  # ensures we do not do a connectivity check, and the "--bare" makes sure
> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
>   git clone --bare -l -s G H
>  '
>  
> -test_expect_success 'validity of fifth-deep repository' '
> +test_expect_success 'validity of seventh repository' '
>   git -C G fsck
>  '
>  
> -test_expect_success 'invalidity of sixth-deep repository' '
> +test_expect_success 'invalidity of eighth repository' '
>   test_must_fail git -C H fsck
>  '
>  

Re: Reference a submodule branch instead of a commit

[Junio C Hamano]

Heiko Voigt  writes:

>> It IS a hack, but having this information in .git would
>> mean that it can be forced to be in machine readable form, unlike a
>> mention in README.  I do not know if the .gitmodules/.gitignore
>> combination is a sensible thing to use, but it does smell like a
>> potentially useful hack.
>
> IIRC the tree entries are the reference for submodules in the code. We
> are iterating over the tree entries in many places so that change does
> not seem so easy to me.
>
> But you are right maybe we should stop arguing against this workflow and
> just let people use it until they find out whats wrong with it ;)

I didn't say that, though.  I am fairly firm on _not_ changing what
the superproject records in its tree for the submodule, i.e. it must
record the exact commit, not "a branch name", for reproducibility. 

I am OK if people ignored the unmatch between the recorded commit
from a submodule and what they had in the submodule directory while
they developed and tested the superproject commit.  After all, it is
not an error to make a commit while having a local uncommitted
changes to tracked files, and it is equally valid to have a commit
checked out in a submodule directory that is different from what
goes in the superproject commit.  But we do show "modified but not
committed" in the status output.  In that light, submodule.*.ignore
may have been a mistake.

Re: [musl] Re: Regression: git no longer works with musl libc's regex impl

[Jeff King]

On Wed, Oct 05, 2016 at 10:59:34PM +1100, James B wrote:

> Number downloads does not make first-tier platform. You know that as
> well as everyone else.
> 
> First-tier support is the decision made by the maintainers that the
> entire features of the software must be available on those first tier
> platforms. So if Windows is indeed first-tier platform for git, it
> means any features that don't work on git version of Windows must not
> be used/developed or even castrated. That's a scary thought.

Prepare to be scared, then, I guess. Ever since the msysgit project
started years ago, we have made concessions in the code to work both
with POSIX-ish systems and with the msys layer. E.g., see how git-daemon
does not fork(), but actually re-spawns itself to handle connections.

When possible we try to put our abstractions at a level where they can
be implemented in a performant way on all platforms (the git-daemon
things is probably the _most_ ugly in that respect; I think nobody has
really cared about the performance enough to add back in a forking code
path for POSIX systems).

> So this decision that "Windows is now a first-tier platform for git" -
> is your own opinion, or is this the collective opinion of *all* the
> git maintainers?

There is only one maintainer of git: Junio. However, you'll note that I
also used "we" in the paragraphs above. And that is because the approach
I am talking about is something that has been done over the course of
many years by many members of the development community.

You may disagree with that approach, but it is nothing new. The msysgit
project started in 2007.

> Well thank you for being honest. I can see now why you responded the
> way you did (and still do). By being employed by Microsoft, and
> especially paid to work on Git for Windows, you have all the
> incentives to make it work best on Windows, and to make it as its
> first-tier platform within the limitation of Windows.

Please don't insinuate that Johannes is a Microsoft shill. He has been
working on the Windows port of Git for over 9 years, and was only
employed by Microsoft this year. Furthermore, his original REG_STARTEND
patch actually did a run-time fallback of NUL-terminating the input
buffers. It was _I_ who suggested that we should simply push people
towards our compat/regex routines instead. So if you want to be mad at
somebody, be mad at me.

-Peff

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Junio C Hamano]

Josef Ridky  writes:

> Hi, 
>
> I have just realize, that my attachment has been cut off from my previous 
> message.
> Below you can find patch with requested change.
>
> Add support for user defined suffix part of name of temporary files
> created by git mergetool
> ---

The first two paragraphs above do not look like they are meant for
the commit log for this change.

Please sign-off your patch.

> -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
> ...'
> +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] 
> [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
>  SUBDIRECTORY_OK=Yes
>  NONGIT_OK=Yes
>  OPTIONS_SPEC=
>  TOOL_MODE=merge
> +
> +#optional name space convention
> +local_name=""
> +remote_name=""
> +base_name=""
> +backup_name=""

If you initialize these to LOCAL, REMOTE, etc. instead of empty,
wouldn't it make the remainder of the code a lot simpler?  For
example,

> + if [ "$local_name" != "" ]  && [ "$local_name" != "$remote_name" ] && [ 
> "$local_name" != "$backup_name" ] && [ "$local_name" != "$base_name" ]
> + then
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
> + else
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> + fi

This can just be made an unconditional

LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"

without any "if" check in front.  The same for all others.

The conditional you added is doing two unrelated things.  It is
trying to switch between an unset $local_name and default LOCAL,
while it tries to make sure the user did not give the same string
for two different things (which is a nonsense).  It is probably
better to check for nonsense just once just before all these
assuments of LOCAL, REMOTE, etc. begins, not at each point where
they are set like this patch does.

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

On Wed, Oct 5, 2016 at 8:47 AM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
>
>> thanks for the suggestions, both git_path(..) as well as checking the config,
>> this seems quite readable to me:
>>
>>  builtin/push.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> Yeah, this seems like a good compromise to me.
>
> I did have one other thought, but I don't think it's worth pursuing now.
> We care about finding gitlinks in objects we're pushing; is there any
> process which is already looking at those objects? Genreally, the
> "pack-objects" doing the push has to. Not to actually push the objects,
> which often are sent blindly off disk, but to determine the set of
> reachable objects in the first place. So in theory we could prepare the
> list of objects to pack, and as a side effect it could say "and here are
> gitlinks referenced by those objects". But that doesn't work if bitmaps
> are in effect, because then we don't access the objects directly at all.
> I think you could solve that by extending the bitmap format to include a
> bit for gitlinks that are reachable (but not necessarily included in the
> pack).
>
> So I don't think that's worth thinking too much about now, but it might
> be an interesting optimization down the road.
>
> -Peff

That sounds like what we'd want down the road. In my imagination the
submodules of the future may live completely inside the superproject,
i.e. they do not necessarily have their own URL. They can be obtained
via fetching the superproject, that automagically also transmits the objects
of the submodules.

Thanks for the hint w.r.t. the bimaps!
Stefan

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Jeff King]

On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:

> thanks for the suggestions, both git_path(..) as well as checking the config,
> this seems quite readable to me:
> 
>  builtin/push.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Yeah, this seems like a good compromise to me.

I did have one other thought, but I don't think it's worth pursuing now.
We care about finding gitlinks in objects we're pushing; is there any
process which is already looking at those objects? Genreally, the
"pack-objects" doing the push has to. Not to actually push the objects,
which often are sent blindly off disk, but to determine the set of
reachable objects in the first place. So in theory we could prepare the
list of objects to pack, and as a side effect it could say "and here are
gitlinks referenced by those objects". But that doesn't work if bitmaps
are in effect, because then we don't access the objects directly at all.
I think you could solve that by extending the bitmap format to include a
bit for gitlinks that are reachable (but not necessarily included in the
pack).

So I don't think that's worth thinking too much about now, but it might
be an interesting optimization down the road.

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:

> I would prefer the following:
> 
> #   A --> B --> C --> D --> E --> F --> G --> H
> #  0 1 2 3 4 5 6

Yeah, that is also more visually pleasing.

Here's a squashable update that uses that and clarifies the points in
the discussion with Jacob.

Junio, do you mind squashing this in to jk/alt-odb-cleanup?

diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index b393613..62170b7 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
)
 '
 
-# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
-# the depth at 0 and count links, not repositories, so in a chain like:
+# Note: These tests depend on the hard-coded value of 5 as the maximum depth
+# we will follow recursion. We start the depth at 0 and count links, not
+# repositories. This means that in a chain like:
 #
-#   A -> B -> C -> D -> E -> F -> G -> H
-#  0123456
+#   A --> B --> C --> D --> E --> F --> G --> H
+#  0 1 2 3 4 5 6
 #
-# we are OK at "G", but break at "H".
+# we are OK at "G", but break at "H", even though "H" is actually the 8th
+# repository, not the 6th, which you might expect. Counting the links allows
+# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
 #
 # Note also that we must use "--bare -l" to make the link to H. The "-l"
 # ensures we do not do a connectivity check, and the "--bare" makes sure
@@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
git clone --bare -l -s G H
 '
 
-test_expect_success 'validity of fifth-deep repository' '
+test_expect_success 'validity of seventh repository' '
git -C G fsck
 '
 
-test_expect_success 'invalidity of sixth-deep repository' '
+test_expect_success 'invalidity of eighth repository' '
test_must_fail git -C H fsck
 '
 

[PATCH 0/6] git-merge: a few documentation improvements

[sorganov]

From: Sergey Organov 

Sergey Organov (6):
  git-merge: clarify "usage" by adding "-m "
  Documentation/git-merge.txt: remove list of options from SYNOPSIS
  Documentation/git-merge.txt: fix SYNOPSIS of obsolete form to include
options
  Documentation/git-merge.txt: improve short description in NAME
  Documentation/git-merge.txt: improve short description in DESCRIPTION
  Documentation/git-merge.txt: get rid of irrelevant references to
git-pull

 Documentation/git-merge.txt | 63 ++---
 builtin/merge.c |  2 +-
 2 files changed, 32 insertions(+), 33 deletions(-)

-- 
2.10.0.1.g57b01a3

[PATCH 2/6] Documentation/git-merge.txt: remove list of options from SYNOPSIS

[sorganov]

From: Sergey Organov 

This partial list of option is confusing as it lacks a lot of
available options. It also clutters the SYNOPSIS making differences
between forms of invocation less clear.

Signed-off-by: Sergey Organov 
---
 Documentation/git-merge.txt | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b758d55..90342eb 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -9,10 +9,7 @@ git-merge - Join two or more development histories together
 SYNOPSIS
 
 [verse]
-'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-   [-s ] [-X ] [-S[]]
-   [--[no-]allow-unrelated-histories]
-   [--[no-]rerere-autoupdate] [-m ] [...]
+'git merge' [options] [-m ] [...]
 'git merge'  HEAD ...
 'git merge' --abort
 
-- 
2.10.0.1.g57b01a3

[PATCH 1/6] git-merge: clarify "usage" by adding "-m "

[sorganov]

From: Sergey Organov 

"-m " is one of essential distinctions between obsolete
invocation form and the recent one. Add it to the "usage" returned by
'git merge -h' for more clarity.

Signed-off-by: Sergey Organov 
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8b57c7..0e367ba 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -43,7 +43,7 @@ struct strategy {
 };
 
 static const char * const builtin_merge_usage[] = {
-   N_("git merge [] [...]"),
+   N_("git merge [] [-m ] [...]"),
N_("git merge []  HEAD "),
N_("git merge --abort"),
NULL
-- 
2.10.0.1.g57b01a3

[PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME

[sorganov]

From: Sergey Organov 

Old description not only raised the question of why the tool is called
git-merge rather than git-join, but "join histories" also sounds like
very simple operation, something like what "git-merge -s ours" does.

Signed-off-by: Sergey Organov 
---
 Documentation/git-merge.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 216d2f4..cc0329d 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -3,7 +3,8 @@ git-merge(1)
 
 NAME
 
-git-merge - Join two or more development histories together
+
+git-merge - Merge one or more branches to the current branch
 
 
 SYNOPSIS
-- 
2.10.0.1.g57b01a3

[PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull

[sorganov]

From: Sergey Organov 

No awareness of git-pull is required to understand git-merge operation,
so leave reference to git-pull only where it actually makes sense, in
the description of fast-forward merges, and only as clarification of
when this merging behaviour is mostly useful.

Other references to git-pull are likely just a historical leftover
that are now neither required nor clarify anything. Besides, git-pull
may use rebase rather than merge, so it's also technically wrong to
say, unconditionally, that git-pull uses git-merge.

Overall, let git-pull description refer to git-merge where
appropriate, and not vice versa.

Signed-off-by: Sergey Organov 
---
 Documentation/git-merge.txt | 36 
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 351b8fc..ba5fb0a 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -23,10 +23,6 @@ named commits and the current branch, called "merge base", is
 calculated, and then net changes taken from the merge base to
 the named commits are applied.
 
-This command is used by 'git pull' to incorporate changes from another
-repository, and can be used by hand to merge changes from one branch
-into another.
-
 Assume the following history exists and the current branch is
 "`master`":
 
@@ -119,18 +115,17 @@ of `git fetch` for merging are merged to the current 
branch.
 PRE-MERGE CHECKS
 
 
-Before applying outside changes, you should get your own work in
-good shape and committed locally, so it will not be clobbered if
-there are conflicts.  See also linkgit:git-stash[1].
-'git pull' and 'git merge' will stop without doing anything when
-local uncommitted changes overlap with files that 'git pull'/'git
-merge' may need to update.
+Before applying outside changes, you should get your own work in good
+shape and committed locally, so it will not be clobbered if there are
+conflicts. See also linkgit:git-stash[1]. 'git merge' will stop
+without doing anything when local uncommitted changes overlap with
+files that 'git merge' may need to update.
 
-To avoid recording unrelated changes in the merge commit,
-'git pull' and 'git merge' will also abort if there are any changes
-registered in the index relative to the `HEAD` commit.  (One
-exception is when the changed index entries are in the state that
-would result from the merge already.)
+To avoid recording unrelated changes in the merge commit, 'git merge'
+will also abort if there are any changes registered in the index
+relative to the `HEAD` commit. (One exception is when the changed
+index entries are in the state that would result from the merge
+already.)
 
 If all named commits are already ancestors of `HEAD`, 'git merge'
 will exit early with the message "Already up-to-date."
@@ -138,14 +133,15 @@ will exit early with the message "Already up-to-date."
 FAST-FORWARD MERGE
 --
 
-Often the current branch head is an ancestor of the named commit.
+Often the current branch head is an ancestor of the named commit.  In
+this case, a new commit is not needed to store the combined history;
+instead, the `HEAD` (along with the index) is updated to point at the
+named commit, without creating an extra merge commit.
+
 This is the most common case especially when invoked from 'git
 pull': you are tracking an upstream repository, you have committed
 no local changes, and now you want to update to a newer upstream
-revision.  In this case, a new commit is not needed to store the
-combined history; instead, the `HEAD` (along with the index) is
-updated to point at the named commit, without creating an extra
-merge commit.
+revision.
 
 This behavior can be suppressed with the `--no-ff` option.
 
-- 
2.10.0.1.g57b01a3

[PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

[sorganov]

From: Sergey Organov 

Old description had a few problems:

- sounded as if commits have changes

- stated that changes are taken since some "divergence point"
  that was not defined.

New description rather uses "common ancestor" and "merge base",
definitions of which are easily discoverable in the rest of GIT
documentation.

Signed-off-by: Sergey Organov 
---
 Documentation/git-merge.txt | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index cc0329d..351b8fc 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -16,11 +16,16 @@ SYNOPSIS
 
 DESCRIPTION
 ---
-Incorporates changes from the named commits (since the time their
-histories diverged from the current branch) into the current
-branch.  This command is used by 'git pull' to incorporate changes
-from another repository and can be used by hand to merge changes
-from one branch into another.
+
+Incorporates changes that lead to the named commits into the current
+branch, and joins corresponding histories. The best common ancestor of
+named commits and the current branch, called "merge base", is
+calculated, and then net changes taken from the merge base to
+the named commits are applied.
+
+This command is used by 'git pull' to incorporate changes from another
+repository, and can be used by hand to merge changes from one branch
+into another.
 
 Assume the following history exists and the current branch is
 "`master`":
@@ -31,11 +36,11 @@ Assume the following history exists and the current branch 
is
 D---E---F---G master
 
 
-Then "`git merge topic`" will replay the changes made on the
-`topic` branch since it diverged from `master` (i.e., `E`) until
-its current commit (`C`) on top of `master`, and record the result
-in a new commit along with the names of the two parent commits and
-a log message from the user describing the changes.
+Then "`git merge topic`" will replay the changes made on the `topic`
+branch since it diverged from `master` (i.e., `E`) until its current
+commit (`C`) on top of `master`, and record the result in a new commit
+along with references to the two parent commits and a log message from
+the user describing the changes.
 
 
  A---B---C topic
-- 
2.10.0.1.g57b01a3

[PATCH 3/6] Documentation/git-merge.txt: fix SYNOPSIS of obsolete form to include options

[sorganov]

From: Sergey Organov 

Signed-off-by: Sergey Organov 
---
 Documentation/git-merge.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 90342eb..216d2f4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git merge' [options] [-m ] [...]
-'git merge'  HEAD ...
+'git merge' [options]  HEAD ...
 'git merge' --abort
 
 DESCRIPTION
-- 
2.10.0.1.g57b01a3

[PATCH] clone: detect errors in normalize_path_copy

[Jeff King]

When we are copying the alternates from the source
repository, if we find a relative path that is too deep for
the source (e.g., "../../../objects" from "/repo.git/objects"),
then normalize_path_copy will report an error and leave
trash in the buffer, which we will add to our new alternates
file. Instead, let's detect the error, print a warning, and
skip copying that alternate.

There's no need to die. The relative path is probably just
broken cruft in the source repo. If it turns out to have
been important for accessing some objects, we rely on other
parts of the clone to detect that, just as they would with a
missing object in the source repo itself (though note that
clones with "-s" are inherently local, which may do fewer
object-quality checks in the first place).

Signed-off-by: Jeff King 
---
Noticed by coverity; the recent alternates cleanups mean that all of the
other calls to normalize_path_copy() are now checked, so it realized
this one was an oddball and probably an error (I actually looked for
others with `grep` when doing that series, but somehow missed this one;
hooray for static analysis). The fix is independent of that series.

 builtin/clone.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index fb75f7e..6cf3b54 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -345,8 +345,11 @@ static void copy_alternates(struct strbuf *src, struct 
strbuf *dst,
continue;
}
abs_path = mkpathdup("%s/objects/%s", src_repo, line.buf);
-   normalize_path_copy(abs_path, abs_path);
-   add_to_alternates_file(abs_path);
+   if (!normalize_path_copy(abs_path, abs_path))
+   add_to_alternates_file(abs_path);
+   else
+   warning("skipping invalid relative alternate: %s/%s",
+   src_repo, line.buf);
free(abs_path);
}
strbuf_release();
-- 
2.10.1.506.g904834d

Re: [PATCH 16/18] count-objects: report alternates via verbose mode

[Jakub Narębski]

W dniu 03.10.2016 o 22:36, Jeff King pisze:

> +test_expect_success 'count-objects shows the alternates' '
> + cat >expect <<-EOF &&
> + alternate: $(pwd)/B/.git/objects
> + alternate: $(pwd)/A/.git/objects
> + EOF
> + git -C C count-objects -v >actual &&
> + grep ^alternate: actual >actual.alternates &&
> + test_cmp expect actual.alternates
> +'

This was bit hard to grok for me without quotes around
regular expression in grep (should it be sane_grep, BTW?):

  + grep "^alternate:" actual >actual.alternates &&

But it might be just me... It's certainly not necessary.

-- 
Jakub Narębski

Re: Reference a submodule branch instead of a commit

[Heiko Voigt]

On Tue, Oct 04, 2016 at 12:01:13PM -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > Stefan Beller  writes:
> >
> >> I wonder if we could make that convenient for users by not tracking
> >> the submodule,
> >> i.e.
> >> * we have the information in the .gitmodules file
> >> * the path itself is in the .gitignore
> >> * no tree entry
> >>
> >> Then you can update to the remote latest branch, without Git reporting
> >> a dirty submodule locally, in fact it reports nothing for the submodule.
> >>
> >> It sounds like a hack, but maybe it's worth looking into that when
> >> people want to see that workflow.
> >
> > It IS a hack.  
> >
> > But if you do not touch .git file and instead say "clone
> > this other project at that path yourself" in README, that would
> > probably be sufficient.
> 
> eh,... hit send too early.
> 
> It IS a hack, but having this information in .git would
> mean that it can be forced to be in machine readable form, unlike a
> mention in README.  I do not know if the .gitmodules/.gitignore
> combination is a sensible thing to use, but it does smell like a
> potentially useful hack.

IIRC the tree entries are the reference for submodules in the code. We
are iterating over the tree entries in many places so that change does
not seem so easy to me.

But you are right maybe we should stop arguing against this workflow and
just let people use it until they find out whats wrong with it ;)

I have another tip for Jeremy:

git config submodule..ignore all

and you will not see any changes to the submodule. Put that into your
.gitmodules and you do not see any changes to the submodules anymore.

So now the only thing missing for complete convenience is a config
option for the --remote option in 'git submodule update'.

Jeremy, does the ignore option combined with --remote what you want?

Cheers Heiko

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jakub Narębski]

W dniu 04.10.2016 o 22:58, Stefan Beller pisze:
> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King  wrote:
>> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>>
 +# Note: These tests depend on the hard-coded value of 5 as "too 
 deep". We start
 +# the depth at 0 and count links, not repositories, so in a chain 
 like:
 +#
 +#   A -> B -> C -> D -> E -> F -> G -> H
 +#  0123456
 +#

> 
> Input from a self-claimed design expert for ASCII art. ;)
> What about this?
> 
> #   A  -0->  B  -1->  C  -2->  ...

I would prefer the following:

#   A --> B --> C --> D --> E --> F --> G --> H
#  0 1 2 3 4 5 6

that is, the number below the middle of the arrow
(which could have been even longer)

#   A ---> B ---> C ---> D ---> E ---> F ---> G ---> H
#  0  1  2  3  4  5  6


Let's paint this bikeshed _plaid_ ;-
-- 
Jakub Narębski

Re: Bug Report: "git submodule deinit" fails right after a clone

[Heiko Voigt]

Hi,

please do not top-post the conversation will otherwise get hard to
follow. Thank you.

On Tue, Oct 04, 2016 at 05:46:45PM +0200, Thomas Bétous wrote:
> Thank you for your answer and sorry for the delay (I was on vacation...).
> 
> I am using git 2.9.0.windows.1 (run on Windows 7 via git bash).

My initial reaction is that this might be a problem with line endings. Did you
check whether you get any diff when you do a 'git diff' after the clone?

Maybe the variable 'core.autocrlf' is set to 'input' ? Have a look at 'git help
config'

> I tested it on this repo:
> https://github.com/githubtraining/example-dependency.git
> The same problem occurs.
> Here a small script to reproduce the error on my PC:
> #!/bin/bash
> git clone https://github.com/githubtraining/example-dependency.git
> cd example-dependency
> git submodule deinit js
> 
> It ends with this error:
> fatal: Please stage your changes to .gitmodules or stash them to proceed
> Submodule work tree 'js' contains local modifications; use '-f' to discard 
> them

Here I get

$ git submodule deinit js
Cleared directory 'js'

So all seems fine.

> Is the script working on your PC?

Yes. I am on Mac OS X though.

Cheers Heiko

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
> Jeff,
> thanks for the suggestions, both git_path(..) as well as checking the config,
> this seems quite readable to me:

When reading the discussion I thought the same: What about the
"old-style" repositories. I like this one. Checking both locations
is nice.

Cheers Heiko

Re: [musl] Re: Regression: git no longer works with musl libc's regex impl

[Rich Felker]

On Wed, Oct 05, 2016 at 01:17:49PM +0200, Johannes Schindelin wrote:
> Hi Rich,
> 
> On Tue, 4 Oct 2016, Rich Felker wrote:
> 
> > On Tue, Oct 04, 2016 at 06:08:33PM +0200, Johannes Schindelin wrote:
> >
> > > And lastly, the best alternative would be to teach musl about
> > > REG_STARTEND, as it is rather useful a feature.
> > 
> > Maybe, but it seems fundamentally costly to support -- it's extra
> > state in the inner loops that imposes costly spill/reload on archs
> > with too few registers (x86).
> 
> It is true that it could cause that.
> 
> I had a brief look at the source code (you use backtracking...

Where did you get that idea? Backtracking is the most utterly
incompetent way to implement regex -- it throws away the whole
property that makes regex useful, being regular. Unfortunately, POSIX
BRE is not regular, as it contains backreferences, so any
implementation of regcomp/regexec requires at least a minimal
backtracking code path for BREs that contain backreferences.

> hopefully
> nobody uses musl to parse regular expressions from untrusted, or

On the contrary, musl's is the only system reccomp/regexec I'm aware
of that actually attempts to be safe with untrusted input -- when
using REG_EXTENDED (ERE). Other implementations provide backreferences
in ERE as an extension, making ERE unsafe just like BRE. musl
intentionally disallows them as a feature.

At least until recently, glibc also crashed on malloc failures in
regcomp, making it unsafe on untrusted input for that reason too.

Rich

Re: Regression: git no longer works with musl libc's regex impl

[Jakub Narębski]

W dniu 05.10.2016 o 00:33, Rich Felker pisze:
> On Wed, Oct 05, 2016 at 09:06:25AM +1100, James B wrote:
>> On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
>> Johannes Schindelin  wrote:
>>>
>>> No, it is not. You quote POSIX, but the matter of the fact is that we use
>>> a subset of POSIX in order to be able to keep things running on Windows.
>>>
>>> And quite honestly, there are lots of reasons to keep things running on
>>> Windows, and even to favor Windows support over musl support. Over four
>>> million reasons: the Git for Windows users.
>>
>> Wow, I don't know that Windows is a git's first-tier platform now,
>> and Linux/POSIX second. Are we talking about the same git that was
>> originally written in Linus Torvalds, and is used to manage Linux
>> kernel? Are you by any chance employed by Redmond, directly or
>> indirectly?
>>
>> Sorry - can't help it.

Windows is one of the major platforms, yes.  I think there much, much
more people using Git on Windows, than using Git with musl.  More
users = more important.

Also, working with some inconvenience (requiring compilation with
NO_REGEX=1) is better than not working at all.

In CodingGuidelines we say:

 - Most importantly, we never say "It's in POSIX; we'll happily
   ignore your needs should your system not conform to it."
   We live in the real world.

 - However, we often say "Let's stay away from that construct,
   it's not even in POSIX".

 - In spite of the above two rules, we sometimes say "Although
   this is not in POSIX, it (is so convenient | makes the code
   much more readable | has other good characteristics) and
   practically all the platforms we care about support it, so
   let's use it".

The REG_STARTEND is 3rd point, mmap shenningans looks like 1st...

...on the other hand midipix  wrote in
http://public-inbox.org/git/20161004200057.dc30d64f61e5ec441c34ffd4f788e58e.efa66ead67@email15.godaddy.com/
that the proposed fix should work on all Windows version we are
interested in (I think).  Test program included / attached.

The above-mentioned email also explains that the problem was
caught on MS Windows; it triggers if file end falls on the mmapped
page boundary, which is more likely to happen with 4096 mod size
on Windows rather than 65536 mod size on Linux.


On the other hand, while the proposed solution of "add padding as
to not end at page boundary, if necessary" doesn't have the
performance impact of "memcpy into NUL-terminated buffer" that
was originally proposed in patch series, it is still extra code
to maintain.

> 
> I don't think the hostility and sarcasm are really needed here. But
> what this does speak to is that users don't like feeling like their
> platform is being treated as a second-class target, which is what it
> feels like when you have to manually flip a switch to make git build.

You are welcome to send a patch adding to configure.ac detection
of REG_STARTEND support in standard library - setting NO_REGEX if
needed, and/or adding to Makefile uname-based defaults setting
NO_REGEX for compiling with musl.

> This is especially unfriendly when the semantics of the switch come
> across, at least to some users, as "your system regex is incomplete"
> rather than "git can't use it because git depends on nonstandard
> extensions".

Nonstandard but common extension.  As 2f8952250a commit message says
https://github.com/git/git/commit/2f8952250a84313b74f96abb7b035874854cf202

  Happily, there is an extension to regexec() introduced by the NetBSD
  project and present in all major regex implementation including
  Linux', MacOSX' and the one Git includes in compat/regex/: [...]

  [...]

  Since support for REG_STARTEND is so widespread by now, let's just
  introduce a helper function that always uses it, and tell people
  on a platform whose regex library does not support it to use the
  one from our compat/regex/ directory.

Also, as Junio said, the description of NO_REGEX option in the
Makefile now explicitly says:

# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
# feature.

Best,
-- 
Jakub Narębski

Re: [PATCH v2 21/25] sequencer: refactor write_message()

[Johannes Schindelin]

Hi Junio & Hannes,

On Thu, 15 Sep 2016, Junio C Hamano wrote:

> Johannes Sixt  writes:
> 
> > Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:
> >> -static int write_message(struct strbuf *msgbuf, const char *filename)
> >> +static int write_with_lock_file(const char *filename,
> >> +  const void *buf, size_t len, int append_eol)
> >>  {
> >>static struct lock_file msg_file;
> >>
> >>int msg_fd = hold_lock_file_for_update(_file, filename, 0);
> >>if (msg_fd < 0)
> >>return error_errno(_("Could not lock '%s'"), filename);
> >> -  if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> >> -  return error_errno(_("Could not write to %s"), filename);
> >> -  strbuf_release(msgbuf);
> >> +  if (write_in_full(msg_fd, buf, len) < 0)
> >> +  return error_errno(_("Could not write to '%s'"), filename);
> >> +  if (append_eol && write(msg_fd, "\n", 1) < 0)
> >> +  return error_errno(_("Could not write eol to '%s"), filename);
> >>if (commit_lock_file(_file) < 0)
> >>return error(_("Error wrapping up %s."), filename);
> >>
> >>return 0;
> >>  }
> >
> > The two error paths in the added lines should both
> >
> > rollback_lock_file(_file);
> >
> > , I think. But I do notice that this is not exactly new, so...
> 
> It may not be new for this step, but overall the series is aiming to
> libify the stuff, so we should fix fd and lockfile leaks like this
> as we notice them.

Makes sense, even for the final commit_lock_file().

Ciao,
Dscho