Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> Subject: Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally 
>> remote ref name
>
> No verb?  s/optionally/report/ perhaps?

I just noticed that I didn't tweak the copy I have in my tree after
sending this message, but now I did s/optionally/& report the/;

I am still puzzled by the hunk below, though.

>> @@ -1262,6 +1265,14 @@ static void fill_remote_ref_details(struct used_atom 
>> *atom, const char *refname,
>>  *s = xstrdup(remote);
>>  else
>>  *s = "";
>> +} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
>> +int explicit, for_push = starts_with(atom->name, "push");
>
> Hmph, the previous step got rid of starts_with() rather nicely by
> introducing atom->u.remote_ref.push bit; can't we do the same in
> this step?
>
> Other than that, looks nicer.

Thanks.

Re: [PATCH] checkout doc: clarify command line args for "checkout paths" mode

[Jonathan Nieder]

Junio C Hamano wrote:

> here is another attempt, this time to avoid "Restore" and ...
>
>  Documentation/git-checkout.txt | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)

Thanks.  I find this one easier to read indeed.

[...]
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -13,7 +13,8 @@ SYNOPSIS
[...]
> @@ -101,6 +95,14 @@ specific side of the merge can be checked out of the 
> index by
>  using `--ours` or `--theirs`.  With `-m`, changes made to the working tree
>  file can be discarded to re-create the original conflicted merge result.
>  
> +'git checkout' (-p|--patch) [] [--] [...]::
> + This is similar to the "check out paths to the working tree
> + from either the index or from a tree-ish" mode described
> + above, but lets you use the interactive interface to show
> + the "diff" output and choose which hunks to use in the
> + result.  See below for the descriptoin of `--patch` option.

nit: s/descriptoin/description/

With that tweak,
Reviewed-by: Jonathan Nieder 

Thanks.

Darlehensangebot

[ICBC FINANCE COMPANY]

Guten Tag Sir / ma,

Wir. ICBC Finance Company hier in der Türkei gewährt Darlehen an 
Kunden mit 3% Zinssatz.
und auch Geld für Investitionen und andere finanzielle Zwecke unabhängig von 
ihrem Standort, Geschlecht, Familienstand, Bildung, Jobstatus, aber müssen eine 
gesetzliche Möglichkeit der Rückzahlung zu schaffen. Unser Darlehen reicht von 
5000 Euro, nok, chf, - 5.000.000,00 Euro, nok , chf, mit 3% Zinssatz.

  
Interessierte Einzelpersonen, Firmen und Unternehmen. Bitte kontaktieren Sie 
uns unter unserer E-Mail:

icbccreditl...@gmail.com

Wenn Sie an einem Darlehen interessiert sind Füllen Sie das Darlehensformular 
aus und senden Sie es umgehend zurück.

INFORMATIONEN ERFORDERLICH

Deine Namen:
Adresse: ...
Telefon: ...
Menge benötigt: ...
Dauer: ...
Beruf: ...
Monatliches Einkommensniveau: ..
Geschlecht: ..
Geburtsdatum: ...
Bundesland: ...
Land: .
Zweck des Darlehens oder Überprüfung des Projekts .

KONTAKTIERE UNS

icbccreditl...@gmail.com


Danke für Ihr Verständnis; wir dienen für Ihren Komfort.


Mit freundlichen Grüßen,
Dr. Helen Mathew

Re: [PATCH] checkout doc: clarify command line args for "checkout paths" mode

[Junio C Hamano]

Jonathan Nieder  writes:

>> +'git checkout' [] [--] ...
>> -'git checkout' [-p|--patch] [] [--] [...]
>> +'git checkout' (-p|--patch) [] [--] [...]
> ...
> Even for this particular form, the current file is inconsistent about
> whether to call the arguments '...' or '...'.
> "..." is even more strange when I think about it for a moment,
> since it is the plural of the plural of path --- i.e., it implies that
> each argument names multiple paths.  Why does this patch use
>  for the --no-patch case and  for the --patch case?

No particular reason, other than "the original had , which
was inherited by the line that was minimally modified, while the
line I typed anew has more preferrable " ;-)

>>  DESCRIPTION
>>  ---
>> @@ -78,20 +79,13 @@ be used to detach HEAD at the tip of the branch (`git 
>> checkout
>>  +
>>  Omitting  detaches HEAD at the tip of the current branch.
>>  
>> +'git checkout' [] [--] ...::
>> +Restore modified or deleted paths in the working tree by
>> +replacing with their original contents from the index, or
>> +the contents from a named  (most often a
>
> This sentence is hard to read:
>
> - "Restore ... in" reads oddly, as though this is an art conservation
>   project that takes place in the working tree.  Is the intent that
>   the command will bring the content of those paths back to the
>   working tree, or something else?

The "restore" was taken from the sentence that was buried in the
paragraph removed by this patch.  I think the original used the
word, fully intending to evoke "an art conservation project"
connotation---after all, the user made a mess in the working tree
and a known-to-be-good copy stored in the index or tree is used to
restore the working tree copy.

> To fix that, it could say
>
>   Replace matching paths in the working tree with content
>   from the index or the named tree (most often a commit).  The
>argument can be used to specify a specific tree,
>   commit, or tag that will be used to update the entries for
>   matching paths in the index before updating the working tree.

It could (and it was what my earlier draft said), but I found it
less clear than necessary.  It does not matter the data goes from
tree-ish to index and then to the working tree, and a more important
thing is to say both are updated from the tree-ish, for example.

> ... aha, now I see the next paragraph where this text came from.  I
> still think I like my example text based on the first paragraph
> better. :)

And I already said the reason why I started from that and improved
it.

>> @@ -101,6 +95,14 @@ specific side of the merge can be checked out of the 
>> index by
>>  using `--ours` or `--theirs`.  With `-m`, changes made to the working tree
>>  file can be discarded to re-create the original conflicted merge result.
>>  
>> +'git checkout' (-p|--patch) [] [--] [...]::
>> +This is similar to the "check out paths to the working tree
>> +from either the index or from a tree-ish" mode described
>> +above, but lets you use the interactive interface to show
>> +the "diff" output and choose which hunks to use in the
>> +result.
>
> s/use/apply/, perhaps.

That might sound technically more accurate, but when we consider
that this application is done in reverse, it is probably not such a
good idea to say "apply" here.  The user chooses which one to use,
and the way it is "used" is by applying the hunk in reverse.
"choose the preimage of which hunks to use in the result" may be
more technically accurate, but it is a mouthful.  I dunno.

The interactive UI explains it by saying

Discard this hunk from worktree?

which is probably the easiest explanation.

> The options section explains --patch again.  Maybe that should point
> back to here to avoid some duplication.

here is another attempt, this time to avoid "Restore" and ...


 Documentation/git-checkout.txt | 30 --
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index d6399c0af8..088266e7e9 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -13,7 +13,8 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [--detach] 
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] ] []
 'git checkout' 

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:01, Jeff King  wrote:
> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
> >> On 9 October 2017 at 23:45, Kevin Daudt  wrote:
> >> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> >> > 2017-08-02), the pager has been enabled by default, exposing this
> >> > problem to more people.
> >>
> >> Oh. :( I didn't know about "column" to be honest.
> >
> > Yeah, I didn't think of that with respect to the pager. This is a
> > regression in v2.14.2, I think.
> 
> Yes.

Though 2.14.2 enabled the pager by default, even before that when
someone would've enabled the pager theirselves (by setting pager.tag for
example), it would also shown just a single column.

I could reproduce it as far as 2.8.3 (I could not test further due to
libssl incompattibility).

Re: [PATCH] checkout doc: clarify command line args for "checkout paths" mode

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

[...]
> The source of the confusion is that -p(atch) is described as if it
> is just another "optional" part and its description is lumped
> together with the non patch mode, even though the actual end user
> experience is vastly different.

Makes sense.  This should have been done as part of b831deda
(2010-06-01), but better late than never.

Let's see how the patch goes...

[...]
>  Documentation/git-checkout.txt | 32 +---
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index d6399c0af8..8e77a9de49 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -13,7 +13,8 @@ SYNOPSIS
>  'git checkout' [-q] [-f] [-m] [--detach] 
>  'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] ] []
>  'git checkout' 

[PATCH] checkout doc: clarify command line args for "checkout paths" mode

[Junio C Hamano]

There are "git checkout [-p][][--][...]" in the
SYNOPSIS section, and "git checkout [-p][][--]..."
as the header for the section that explains the "check out paths
from index/tree-ish" mode.  It is unclear if we require at least one
path, or it is entirely optional.

Actually, both are wrong.  Without the "-p(atch)" option, you must
have  (otherwise, with a commit that is a , you
would be checking out that commit to build a new history on top of
it).  With it, it is already clear that you are checking out paths,
it is optional.  In other words, you cannot omit both.

The source of the confusion is that -p(atch) is described as if it
is just another "optional" part and its description is lumped
together with the non patch mode, even though the actual end user
experience is vastly different.

Let's split the entry into two, and describe the regular mode and
the patch mode separately.  This allows us to make it clear that the
regular mode MUST be given at least one pathspec, that the patch
mode can be invoked with either '-p' or '--patch' but one of these
must be given, and that the pathspec is entirely optional in the
patch mode.

Also, revamp the explanation of "checkout paths" by removing
extraneous description at the beginning, that says "checking out
paths is not checking out a branch".  Explaining what it is for and
when the user wants to use it upfront is the most direct way to help
the readers.

Noticed-by: Robert P J Day
Signed-off-by: Junio C Hamano 
---

 * As people burned braincycles discussing this earlier already,
   let's try to tie the loose end to help future readers of the
   manual.

 Documentation/git-checkout.txt | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index d6399c0af8..8e77a9de49 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -13,7 +13,8 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [--detach] 
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] ] []
 'git checkout' 

Re: [PATCH v2 1/3] for-each-ref: let upstream/push optionally report the remote name

[Junio C Hamano]

Johannes Schindelin  writes:

> The implementation chooses *not* to DWIM the push remote if no explicit
> push remote was configured; The reason is that it is possible to DWIM this
> by using
>
>   %(if)%(push:remotename)%(then)
>   %(push:remotename)
>   %(else)
>   %(upstream:remotename)
>   %(end)
>
> while it would be impossible to "un-DWIM" the information in case the
> caller is really only interested in explicit push remotes.

Good.

> Note: the dashless, non-CamelCased form `:remotename` follows the
> example of the `:trackshort` example.

Good.  Come to think of it, aside from modifiers, the use of
squashedwords like committerdate and objecttype has long been the
norm in the for-each-ref atom names, so "remotename" is much more in
line with other formats.

> @@ -1354,16 +1371,20 @@ static void populate_value(struct ref_array_item *ref)
>   if (refname)
>   fill_remote_ref_details(atom, refname, branch, 
> >s);
>   continue;
> - } else if (starts_with(name, "push")) {
> + } else if (atom->u.remote_ref.push) {
>   const char *branch_name;
>   if (!skip_prefix(ref->refname, "refs/heads/",
>_name))
>   continue;
>   branch = branch_get(branch_name);
>  
> - refname = branch_get_push(branch, NULL);
> - if (!refname)
> - continue;
> + if (atom->u.remote_ref.push_remote)
> + refname = NULL;
> + else {
> + refname = branch_get_push(branch, NULL);
> + if (!refname)
> + continue;
> + }
>   fill_remote_ref_details(atom, refname, branch, >s);

It still feels a bit strange to pass NULL refname to
fill_remote_ref_details() function, but fixing it would require
overhaul of this if/else if cascade, I think, so let's not worry too
much about it at least for now.

Looks much better than the previous one.  Thanks.  Queued.

Re: What happened to "git status --color=(always|auto|never)"?

[Junio C Hamano]

Jeff King  writes:

> On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote:
>
>> Where I worry is about commands where the line between porcelain and
>> plumbing blur, like "git log --format=raw".  I actually still prefer
>> the approach where "color.ui=always" becomes impossible to express in
>> config and each command takes a --color option.
>> 
>> If we want to be extra fancy, we could make git take a --color option
>> instead of requiring each command to do it.
>> 
>> To support existing scripts, we could treat "-c color.ui=always" as a
>> historical synonym for --color=always, either temporarily or
>> indefinitely.  Making it clear that this is only there for historical
>> reasons would make it less likely that other options make the same
>> mistake in the future.
>
> So that's basically my (2), with the twist that we claim it's only
> horrible and inconsistent for historical reasons. :)
>
> Is that the direction we want to go?

Your (2), and Jonathan's "git --color=..."  as an extension to it,
is probably the least risky approach forward, as you said earlier.
And I think that it would get us closest to the ideal world (in
which color=always did not exist in the configuration system), while
breaking least number of various "questionable" scripts in the wild.

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Junio C Hamano]

Jonathan Nieder  writes:

> If we want to make the advertised protocol versions on the server side
> configurable, I think it should be independent from the configuration
> for protocol versions to use on the client side.  Rationale:
>
>  - As a client-side user, I may want to (after reporting the bug, of
>course!) blacklist certain protocol versions to work around server
>bugs.  But this should not affect my behavior as a server --- in
>my role as a server, these server-side bugs have no effect on me.
>
>  - As a server operator, I may want to (after reporting the bug, of
>course!) blacklist certain protocol versions to work around client
>bugs.  But this should not affect my behavior as a client --- in my
>role as a client, these client-side bugs have no effect on me.

Good point.  

> Making the client-side case configurable seems important since Git is
> widely used in environments where it may not be easy to control the
> deployed version (so having configuration as an escape hatch is
> important).
>
> Making the server-side case configurable seems less important since
> Git server operators usually have tight control over the deployed Git
> version and can apply a targetted fix or workaround.

The above also suggests that the configuration variable that lets
you specify the protocol version should hint in its name which
direction of the protocol it controls.  Even if we decide that we'd
add only the variable for the initiator side for now, if it is
possible that we later may want to add another for the acceptor
side, we need to do it right from day one.

Perhaps protocol.connectVersion(s) vs protocol.acceptVersion(s) or
something like that?

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Junio C Hamano]

Junio C Hamano  writes:

> A few questions that come to mind are:
>
>  - Does "git add sub/" have enough information to populate
>.gitmodules?  If we have reasonable "default" values for
>.gitmodules entries (e.g. missing URL means we won't fetch when
>asked to go recursively fetch), perhaps we can leave everything
>other than "submodule.$name.path" undefined.
> ...
>  - ...  IOW, if "git add sub/" can
>add .gitmodules to help people without having to type "git
>submodule add sub/", then we can give exactly the same degree of
>help without even modifying .gitmodules when "git add sub/" is
>run.

Answering my own questions (aka correcting my own stupidity), there
is a big leap/gap between the two that came from my forgetting an
important point: a local repository has a lot richer information
than others that are clones of it.

"git add sub/" could look at sub/.git/config and use that
information when considering what values to populate .gitmodules
with.  It can learn where its origin remote is, for example.

And while this can do that at look-up time locally (i.e. removing
the need to do .gitmodules), those who pull from this local
repository, of those who pull from a shared central repository this
local repository pushes into, will not have the same information
available to them, _unless_ this local repository records it in the
.gitmodules file for them to use.

So, I think "git add sub/" that adds to .gitmodules would work
(unless the sub/ repository originates locally without pushing
out--in which case, submodule.$name.url cannot be populated with a
value suitable for other people, and we should continue warning),
while doing the same at look-up time would not be a good solution.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Stefan Beller]

On Tue, Oct 10, 2017 at 4:31 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So you propose to make git-add behave like "git submodule add"
>> (i.e. also add the .gitmodules entry for name/path/URL), which I
>> like from a submodule perspective.
>>
>> However other users of gitlinks might be confused[1], which is why
>> I refrained from "making every gitlink into a submodule". Specifically
>> the more powerful a submodule operation is (the more fluff adds),
>> the harder it should be for people to mis-use it.
>
> A few questions that come to mind are:
>
>  - Does "git add sub/" have enough information to populate
>.gitmodules?  If we have reasonable "default" values for
>.gitmodules entries (e.g. missing URL means we won't fetch when
>asked to go recursively fetch), perhaps we can leave everything
>other than "submodule.$name.path" undefined.

I think we would want to populate path and URL only.

>
>  - Can't we help those who have gitlinks without .gitmodules entries
>exactly the same way as above, i.e. when we see a gitlink and try
>to treat it as a submodule, we'd first try to look it up from
>.gitmodules (by going from path to name and then to
>submodule.$name.$var); the above "'git add sub/' would add an
>entry for .gitmodules" wish is based on the assumption that there
>are reasonable "default" values for each of these $var--so by
>basing on the same assumption, we can "pretend" as if these
>submodule.$name.$var were in .gitmodules file when we see
>gitlinks without .gitmodules entries.  IOW, if "git add sub/" can
>add .gitmodules to help people without having to type "git
>submodule add sub/", then we can give exactly the same degree of
>help without even modifying .gitmodules when "git add sub/" is
>run.

I do not understand the gist of this paragraph, other then:

  "When git-add  encounters a section submodule..*,
   do not modify it; We can assume it is sane already."

>  - Even if we could solve it with "git add sub/" that adds to
>.gitmodules, is it a good solution, when we can solve the same
>thing without having to do so?

I am confused even more.

So you suggest that "git add [--gitlink=submodule]" taking on the
responsibilities of "git submodule add" is a bad idea?

I thought we had the same transition from "git remote update" to
"git fetch", which eventually superseded the former.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Junio C Hamano]

Stefan Beller  writes:

> So you propose to make git-add behave like "git submodule add"
> (i.e. also add the .gitmodules entry for name/path/URL), which I
> like from a submodule perspective.
>
> However other users of gitlinks might be confused[1], which is why
> I refrained from "making every gitlink into a submodule". Specifically
> the more powerful a submodule operation is (the more fluff adds),
> the harder it should be for people to mis-use it.

A few questions that come to mind are:

 - Does "git add sub/" have enough information to populate
   .gitmodules?  If we have reasonable "default" values for
   .gitmodules entries (e.g. missing URL means we won't fetch when
   asked to go recursively fetch), perhaps we can leave everything
   other than "submodule.$name.path" undefined.

 - Can't we help those who have gitlinks without .gitmodules entries
   exactly the same way as above, i.e. when we see a gitlink and try
   to treat it as a submodule, we'd first try to look it up from
   .gitmodules (by going from path to name and then to
   submodule.$name.$var); the above "'git add sub/' would add an
   entry for .gitmodules" wish is based on the assumption that there
   are reasonable "default" values for each of these $var--so by
   basing on the same assumption, we can "pretend" as if these
   submodule.$name.$var were in .gitmodules file when we see
   gitlinks without .gitmodules entries.  IOW, if "git add sub/" can
   add .gitmodules to help people without having to type "git
   submodule add sub/", then we can give exactly the same degree of
   help without even modifying .gitmodules when "git add sub/" is
   run.

 - Even if we could solve it with "git add sub/" that adds to
   .gitmodules, is it a good solution, when we can solve the same
   thing without having to do so?

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Stefan Beller]

On Tue, Oct 10, 2017 at 2:17 PM, Jonathan Nieder  wrote:
> Hi,
>
> Brandon Williams wrote:
>
>> Given some of this discussion though, maybe we want to change the
>> semantics of 'protocol.version' so that both servers and clients respect
>> it.

I have no preference for this, but I agree with Jonathans reasoning.


>> As of right now, these patches have the server always allow
>> protocol v0 and v1?  Though that doesn't do much right now since v1 is
>> the same as v0.

I would strongly prefer if the user configures "v0 and v1", being explicit,
if the client wants to talk either of them.

(v0 is not any more special than any future protocol after all -- from the
users perspective, configuring protocol.version)

On the wire transfer we may want to omit the v0, but for simplicity we could
just dump the clients config of "v0 and v1" onto the wire, and the server
would either ignore the "v0 and" part (when speaking v1), or ignore it
completely (old server, speaking v0).

Given this model, we have
* a strict whitelist clientside,
* the ordering decision can be deferred until later,
  when we have an actual v2.

>> One other considerations that I should probably handle is that a client
>> doesn't do any verification right now to ensure that the protocol
>> version the server selects was indeed the protocol that the client
>> requested.  Is that something you think we need to check for?

Yes, we would want to see if the protocol version matches the configured
white list. ("Once we have a v2, I would no longer want a server talking
v0,v1 to me, because I consider v0 insecure[1], and I personally want
to rather have no communication than a v0 protocol. After all I configured
'protocol.version = v2' only")

[1] e.g. 
https://public-inbox.org/git/1477690790.2904.22.ca...@mattmccutchen.net/

> Do you mean in tests, or are you referring to something else?

A test would be lovely.

Thanks,
Stefan

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Jonathan Tan]

On Tue,  3 Oct 2017 13:15:00 -0700
Brandon Williams  wrote:

> +enum protocol_version determine_protocol_version_server(void)
> +{
> + const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
> + enum protocol_version version = protocol_v0;
> +
> + /*
> +  * Determine which protocol version the client has requested.  Since
> +  * multiple 'version' keys can be sent by the client, indicating that
> +  * the client is okay to speak any of them, select the greatest version
> +  * that the client has requested.  This is due to the assumption that
> +  * the most recent protocol version will be the most state-of-the-art.
> +  */
> + if (git_protocol) {
> + struct string_list list = STRING_LIST_INIT_DUP;
> + const struct string_list_item *item;
> + string_list_split(, git_protocol, ':', -1);
> +
> + for_each_string_list_item(item, ) {
> + const char *value;
> + enum protocol_version v;
> +
> + if (skip_prefix(item->string, "version=", )) {

After writing some protocol docs [1], I wonder if this is also too
lenient. The code should probably die if a lone "version" (without the
equals sign) is given.

[1] 
https://public-inbox.org/git/20171010193956.168385-1-jonathanta...@google.com/

> + v = parse_protocol_version(value);
> + if (v > version)
> + version = v;
> + }
> + }
> +
> + string_list_clear(, 0);
> + }
> +
> + return version;
> +}

Also, in your commit title, it is "extension", not "extention".

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Given some of this discussion though, maybe we want to change the
> semantics of 'protocol.version' so that both servers and clients respect
> it.  As of right now, these patches have the server always allow
> protocol v0 and v1?  Though that doesn't do much right now since v1 is
> the same as v0.

I strongly prefer not to do that.

If we want to make the advertised protocol versions on the server side
configurable, I think it should be independent from the configuration
for protocol versions to use on the client side.  Rationale:

 - As a client-side user, I may want to (after reporting the bug, of
   course!) blacklist certain protocol versions to work around server
   bugs.  But this should not affect my behavior as a server --- in
   my role as a server, these server-side bugs have no effect on me.

 - As a server operator, I may want to (after reporting the bug, of
   course!) blacklist certain protocol versions to work around client
   bugs.  But this should not affect my behavior as a client --- in my
   role as a client, these client-side bugs have no effect on me.

Making the client-side case configurable seems important since Git is
widely used in environments where it may not be easy to control the
deployed version (so having configuration as an escape hatch is
important).

Making the server-side case configurable seems less important since
Git server operators usually have tight control over the deployed Git
version and can apply a targetted fix or workaround.

> One other considerations that I should probably handle is that a client
> doesn't do any verification right now to ensure that the protocol
> version the server selects was indeed the protocol that the client
> requested.  Is that something you think we need to check for?

Do you mean in tests, or are you referring to something else?

Thanks,
Jonathan

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Brandon Williams]

On 10/06, Martin Ågren wrote:
> On 6 October 2017 at 11:40, Junio C Hamano  wrote:
> > Simon Ruderich  writes:
> >
> >> Did you consider Stefan Beller's suggestion regarding a
> >> (white)list of allowed versions?
> >>
> >> On Mon, Sep 18, 2017 at 01:06:59PM -0700, Stefan Beller wrote:
> >>> Thinking about this, how about:
> >>>
> >>>   If not configured, we do as we want. (i.e. Git has full control over
> >>>   it's decision making process, which for now is "favor v0 over v1 as
> >>>   we are experimenting with v1". This strategy may change in the future
> >>>   to "prefer highest version number that both client and server can 
> >>> speak".)
> >>>
> >>>   If it is configured, "use highest configured number from the given set".
> >>>
> >>> ?
> >>
> >> It would also allow the server operator to configure only a
> >> specific set of versions (to handle the "version x is
> >> insecure/slow"-issue raised by Stefan Beller). The current code
> >> always uses the latest protocol supported by the git binary.
> >
> > If we do anything less trivial than "highest supported by both" (and
> > I suspect we want to in the final production version), I'd prefer
> > the configuration to list versions one side supports in decreasing
> > order of preference (e.g. "v3 v0 v2"), and take the earliest from
> > this list that both sides know how to talk, so that we can skip
> > insecure versions altogether by omitting, and we can express that we
> > would rather avoid talking expensive versions unless there is no
> > other version that is understood by the other side.
> 
> Maybe I'm missing something Git-specific, but isn't the only thing that
> needs to be done now, to document/specify that 1) the client should send
> its list ordered by preference, 2) how preference is signalled, and 3)
> that the server gets to choose?
> 
> Why would a server operator with only v0 and v1 at their disposal want
> to choose v0 instead of v1, considering that -- as far as I understand
> -- they are in fact the same?
> 
> Different server implementations and different server configurations
> will be able to apply whatever rules they want in order to decide which
> version to use. (It's not like the client can verify the choice that the
> server makes.) And Brandon's "pick the highest number" will do for now.
> 
> There are many possible rules that the server could apply and they
> shouldn't affect other servers or what the client does. For example, the
> server can go "You seem to know lots of versions, including X and Y.
> Those are the two that I really prefer, but between those two I'm not
> picky, so I'll use whichever of X and Y that you seem to prefer." Unless
> I've missed something, we'll never need to implement -- nor specify --
> anything like that before we have learned both v2 and v3.
> 
> I guess my thinking is, there's a difference between the protocol and
> the implementation.

I've been busy the last week or so and I probably wont get much more
time to go into more detail on this until the end of the week, so sorry
for not being super active on this thread in the past couple of days.

One of the key things about this transition is ensuring that we get it
right, because if we get it wrong and find out years later we'll have
to live with it.  So I'm excited and happy that people are taking a
close look at this.

That being said I don't think we need to worry too much about how one
version of the protocol is selected over another.  I fully expect this
to change based on many different factors.  Maybe one particular server
implementation favors v4 over v5, or another serve has no preference at
all.  We may learn something later on, based on security or other
reasons, that we want to prefer one version or another.  Because of that
(and because I'm hoping that once we have a v2 built that we don't have
to move to another protocol version any time soon) I think it would be a
mistake to hard-code or design in inherent preferences that a client
expresses that servers are 'required' to respect.

So I agree with Martin here that if more complicated use cases arise we
can design in a preference system for them at a later time.

Given some of this discussion though, maybe we want to change the
semantics of 'protocol.version' so that both servers and clients respect
it.  As of right now, these patches have the server always allow
protocol v0 and v1?  Though that doesn't do much right now since v1 is
the same as v0.

One other considerations that I should probably handle is that a client
doesn't do any verification right now to ensure that the protocol
version the server selects was indeed the protocol that the client
requested.  Is that something you think we need to check for?

-- 
Brandon Williams

Re: "git rm" seems to do recursive removal even without "-r"

[Robert P. J. Day]

On Tue, 10 Oct 2017, Paul Smith wrote:

> On Tue, 2017-10-10 at 04:36 -0400, Robert P. J. Day wrote:
> >   ah, now *that* is a compelling rationale that justifies the
> > underlying weirdness. but it still doesn't explain the different
> > behaviour between:
> >
> >   $ git rm -n 'Makefile*'
> >   $ git rm -n '*Makefile'
>
> I explained that behavior in the email up-thread from this reply:

  yup, sorry i missed it. man, it's been an educational thread.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

[PATCH] Documentation: document Extra Parameters

[Jonathan Tan]

Document the server support for Extra Parameters, additional information
that the client can send in its first message to the server during a
Git client-server interaction.

Signed-off-by: Jonathan Tan 
---
I noticed that Documentation/technical was not updated in this patch
series. It probably should, and here is a suggestion on how to do it.

Also, I'm not sure what to call the extra sendable information. I'm open
to suggestions for a better name than Extra Parameters.
---
 Documentation/technical/http-protocol.txt |  8 ++
 Documentation/technical/pack-protocol.txt | 43 ++-
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/http-protocol.txt 
b/Documentation/technical/http-protocol.txt
index 1c561bdd9..a0e45f288 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -219,6 +219,10 @@ smart server reply:
S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
 
+The client may send Extra Parameters (see
+Documentation/technical/pack-protocol.txt) as a colon-separated string
+in the Git-Protocol HTTP header.
+
 Dumb Server Response
 
 Dumb servers MUST respond with the dumb server reply format.
@@ -269,7 +273,11 @@ the C locale ordering.  The stream SHOULD include the 
default ref
 named `HEAD` as the first ref.  The stream MUST include capability
 declarations behind a NUL on the first ref.
 
+The returned response contains "version 1" if "version=1" was sent as an
+Extra Parameter.
+
   smart_reply =  PKT-LINE("# service=$servicename" LF)
+*1("version 1")
 ref_list
 ""
   ref_list=  empty_list / non_empty_list
diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index ed1eae8b8..f9ebfb23e 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -39,6 +39,19 @@ communicates with that invoked process over the SSH 
connection.
 The file:// transport runs the 'upload-pack' or 'receive-pack'
 process locally and communicates with it over a pipe.
 
+Extra Parameters
+
+
+The protocol provides a mechanism in which clients can send additional
+information in its first message to the server. These are called "Extra
+Parameters", and are supported by the Git, SSH, and HTTP protocols.
+
+Each Extra Parameter takes the form of `=` or ``.
+
+Servers that receive any such Extra Parameters MUST ignore all
+unrecognized keys. Currently, the only Extra Parameter recognized is
+"version=1".
+
 Git Transport
 -
 
@@ -46,18 +59,25 @@ The Git transport starts off by sending the command and 
repository
 on the wire using the pkt-line format, followed by a NUL byte and a
 hostname parameter, terminated by a NUL byte.
 
-   0032git-upload-pack /project.git\0host=myserver.com\0
+   0033git-upload-pack /project.git\0host=myserver.com\0
+
+The transport may send Extra Parameters by adding an additional NUL
+byte, and then adding one or more NUL-terminated strings:
+
+   003egit-upload-pack /project.git\0host=myserver.com\0\0version=1\0
 
 --
-   git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]
+   git-proto-request = request-command SP pathname NUL
+  [ host-parameter NUL [ NUL extra-parameters ] ]
request-command   = "git-upload-pack" / "git-receive-pack" /
   "git-upload-archive"   ; case sensitive
pathname  = *( %x01-ff ) ; exclude NUL
host-parameter= "host=" hostname [ ":" port ]
+   extra-parameters  = 1*extra-parameter
+   extra-parameter   = 1*( %x01-ff ) NUL
 --
 
-Only host-parameter is allowed in the git-proto-request. Clients
-MUST NOT attempt to send additional parameters. It is used for the
+host-parameter is used for the
 git-daemon name based virtual hosting.  See --interpolated-path
 option to git daemon, with the %H/%CH format characters.
 
@@ -117,6 +137,12 @@ we execute it without the leading '/'.
 v
ssh u...@example.com "git-upload-pack '~alice/project.git'"
 
+Depending on the value of the `protocol.version` configuration variable,
+Git may attempt to send Extra Parameters as a colon-separated string in
+the GIT_PROTOCOL environment variable. This is done only if
+the `ssh.variant` configuration variable indicates that the ssh command
+supports passing environment variables as an argument.
+
 A few things to remember here:
 
 - The "command name" is spelled with dash (e.g. git-upload-pack), but
@@ -137,11 +163,13 @@ Reference Discovery
 ---
 
 When the client initially connects the server will immediately respond
-with a listing of each reference it has (all branches and tags) along
+with a version number (if "version=1" is sent as an 

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote:

> > I do wonder if people would end up seeing some corner cases as
> > regressions, though. Right now "diff-tree" _does_ color the output by
> > default, and it would stop doing so under your scheme. That's the right
> > thing to do by the plumbing/porcelain distinction, but users with
> > scripts that use diff-tree (or other plumbing) to generate user-visible
> > output may unexpectedly lose their color, until the calling script is
> > fixed to add back in a --color option[1].
> 
> I think it's better for the calling script to be fixed to use "git
> diff", since it is producing output for the sake of the user instead
> of for machine parsing.  That way, the script gets the benefit of
> other changes like --decorate automatically.
> 
> So I don't see that as a regression.

I agree that may be the best way for those scripts to do it. But it's
still a regression to them, if their script used to do what they wanted
and now it doesn't.

It may be one we don't want to care about because the script is doing
something we don't want to support. But then, think we are still
deciding whether "color.always" in your ~/.gitconfig is in the same
boat.

> Where I worry is about commands where the line between porcelain and
> plumbing blur, like "git log --format=raw".  I actually still prefer
> the approach where "color.ui=always" becomes impossible to express in
> config and each command takes a --color option.
> 
> If we want to be extra fancy, we could make git take a --color option
> instead of requiring each command to do it.
> 
> To support existing scripts, we could treat "-c color.ui=always" as a
> historical synonym for --color=always, either temporarily or
> indefinitely.  Making it clear that this is only there for historical
> reasons would make it less likely that other options make the same
> mistake in the future.

So that's basically my (2), with the twist that we claim it's only
horrible and inconsistent for historical reasons. :)

Is that the direction we want to go?

-Peff

Re: What happened to "git status --color=(always|auto|never)"?

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote:

>> I think the right fix to the original problem (you cannot remove
>> auto-color from the plumbing) is to stop paying attention to color
>> configuration from the default config.  I wonder if something like
>> this would work?
>>
>>  - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;
>>
>>  - When git_color_config() is called, and if git_use_color_default
>>is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless
>>of the variable git_color_config() is called for).
>>
>>  - In color.c::want_color(), when git_use_color_default is used,
>>notice if it is GIT_COLOR_UNKNOWN and behave as if it is
>>GIT_COLOR_NEVER.
>>
>> Then we make sure that git_color_config() is never called by any
>> plumbing command.  The fact it is (ever) called can be taken as a
>> clue that we are running a Porcelain (hence we transition from
>> UNKNOWN to AUTO), so we'd get the desirable "no default color for
>> plumbing, auto color for Porcelain", I would think.
>
> Yes, I think that's the simplest way to implement the "plumbing should
> never do color without a command-line option" scheme.
>
> I do wonder if people would end up seeing some corner cases as
> regressions, though. Right now "diff-tree" _does_ color the output by
> default, and it would stop doing so under your scheme. That's the right
> thing to do by the plumbing/porcelain distinction, but users with
> scripts that use diff-tree (or other plumbing) to generate user-visible
> output may unexpectedly lose their color, until the calling script is
> fixed to add back in a --color option[1].

I think it's better for the calling script to be fixed to use "git
diff", since it is producing output for the sake of the user instead
of for machine parsing.  That way, the script gets the benefit of
other changes like --decorate automatically.

So I don't see that as a regression.

Where I worry is about commands where the line between porcelain and
plumbing blur, like "git log --format=raw".  I actually still prefer
the approach where "color.ui=always" becomes impossible to express in
config and each command takes a --color option.

If we want to be extra fancy, we could make git take a --color option
instead of requiring each command to do it.

To support existing scripts, we could treat "-c color.ui=always" as a
historical synonym for --color=always, either temporarily or
indefinitely.  Making it clear that this is only there for historical
reasons would make it less likely that other options make the same
mistake in the future.

Thanks,
Jonathan

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Stefan Beller]

On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt  wrote:

>> > As mentioned in the cover letter. This seems to be the only test that
>> > ensures that we stay compatible with setups without .gitmodules. Maybe
>> > we should add/revive some?
>>
>> An interesting discussion covering this topic is found at
>> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3...@sigill.intra.peff.net/
>
> Thanks for that pointer. So in that discussion Junio said that the
> recursive operations should succeed if we have everything necessary at
> hand. I kind of agree because why should we limit usage when not
> necessary. On the other hand we want git to be easy to use. And that
> example from Peff is perfect as a demonstration of a incosistency we
> currently have:
>
> git clone git://some.where.git/submodule.git
> git add submodule
>
> is an operation I remember, I did, when first getting in contact with
> submodules (many years back), since that is one intuitive way. And the
> thing is: It works, kind of... Only later I discovered that one actually
> needs to us a special submodule command to get everything approriately
> setup to work together with others.

I agree that we ought to not block off users "because we can", but rather
perform the operation if possible with the data at hand.

Note that the result of the discussion `jk/warn-add-gitlink actually`
warns about adding a raw gitlink now, such that we hint at using
"git submodule add", directly.

So you propose to make git-add behave like "git submodule add"
(i.e. also add the .gitmodules entry for name/path/URL), which I
like from a submodule perspective.

However other users of gitlinks might be confused[1], which is why
I refrained from "making every gitlink into a submodule". Specifically
the more powerful a submodule operation is (the more fluff adds),
the harder it should be for people to mis-use it.

[1] https://github.com/git-series/git-series/blob/master/INTERNALS.md
 "git-series uses gitlinks to store pointer to commits in its own repo."

> If everyone agrees that submodules are the default way of handling
> repositories insided repositories, IMO, 'git add' should also alter
> .gitmodules by default. We could provide a switch to avoid doing that.

I wonder if that switch should be default-on (i.e. not treat a gitlink as
a submodule initially, behavior as-is, and then eventually we will
die() on unconfigured repos, expecting the user to make the decision)

> An intermediate solution would be to warn

That is already implemented by Peff.

> but in the long run my goal
> for submodules is and always was: Make them behave as close to files as
> possible. And why should a 'git add submodule' not magically do
> everything it can to make submodules just work? I can look into a patch
> for that if people agree here...

I'd love to see this implemented. I cc'd Josh (the author of git-series), who
may disagree with this, or has some good input how to go forward without
breaking git-series.

> Regarding handling of gitlinks with or without .gitmodules:
>
> Currently we are actually in some intermediate state:
>
>  * If there is no .gitmodules file: No submodule processing on any
>gitlinks (AFAIK)

AFAIK this is true.

>  * If there is a .gitmodules files with some submodule configured: Do
>recursive fetch and push as far as possible on gitlinks.

* If submodule.recurse is set, then we also treat submodules like files
  for checkout, reset, read-tree.

> So I am not sure whether there are actually many users (knowingly)
> using a mix of some submodules configured and some not and then relying
> on the submodule infrastructure.
>
> I would rather expect two sorts of users:
>
>   1. Those that do use .gitmodules

Those want to reap all benefits of good submodules.

>
>   2. Those that do *not* use .gitmodules

As said above, we don't know if those users are
"holding submodules wrong" or are using gitlinks for
magic tricks (unrelated to submodules).

>
> Users that do not use any .gitmodules file will currently (AFAIK) not
> get any submodule handling. So the question is are there really many
> "mixed users"? My guess would be no.

I hope that there are few (if any) users of these mixed setups.

> Because without those using this mixed we could switch to saying: "You
> need to have a .gitmodules file for submodule handling" without much
> fallout from breaking users use cases.

That seems reasonable to me, actually.

> Maybe we can test this out somehow? My patch series would be ready in
> that case, just had to drop the first patch and adjust the commit
> message of this one.

I wonder how we would test this, though? Do you have any idea
(even vague) how we'd accomplish such a measurement?
I fear we'll have to go this way blindly.

Cheers,
Stefan

>
> Cheers Heiko

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:29, Jeff King  wrote:
> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
> >
> > it will randomly succeed or fail, depending on whether sed manages to
> > read the input before the stdin terminal is closed.
> >
> > I'm not sure of an easy way to fix test-terminal, but we could work
> > around it like this (which also uses "-p" to actually invoke the pager,
> > and uses a pager that makes it clear when it's being run):
> >
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 9441145bf0..d322c3b745 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -180,14 +180,14 @@ EOF
> >
> >  test_expect_success TTY '20 columns, mode auto, pager' '
> > cat >expected <<\EOF &&
> > -oneseven
> > -twoeight
> > -three  nine
> > -four   ten
> > -five   eleven
> > -six
> > +paged:oneseven
> > +paged:twoeight
> > +paged:three  nine
> > +paged:four   ten
> > +paged:five   eleven
> > +paged:six
> >  EOF
> > -   test_terminal env PAGER="cat|cat" git column --mode=auto  > >actual &&
> > +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> > --mode=auto actual &&
> > test_cmp expected actual
> >  '
> >  test_done
> 
> Makes sense. FWIW, I don't see the flakyness with this.
> 
> > All that said, I think I'd just as soon test a real command like "git
> > tag", which doesn't care about reading from stdin.
> 
> For reference for Kevin, in case you consider testing, e.g., git tag,
> the main reason I referred to the test I posted as "hacky", was that I
> just inserted it at a more-or-less random place in t7006. So it had to
> play with `git reset` to avoid upsetting the later tests. It could
> obviously go to the end instead, but I was too lazy to move it and
> define a pager.

Thanks Jeff and Martin, I will use your tips to build a test based on
git tag instead.

Re: [PATCH v3 07/10] connect: tell server that the client understands v1

[Jonathan Tan]

On Tue,  3 Oct 2017 13:15:04 -0700
Brandon Williams  wrote:

> 2. ssh://, file://
>Set 'GIT_PROTOCOL' environment variable with the desired protocol
>version.  With the file:// transport, 'GIT_PROTOCOL' can be set
>explicitly in the locally running git-upload-pack or git-receive-pack
>processes.  With the ssh:// transport and OpenSSH compliant ssh
>programs, 'GIT_PROTOCOL' can be sent across ssh by using '-o
>SendEnv=GIT_PROTOCOL' and having the server whitelist this
>environment variable.

In your commit message, also mention what GIT_PROTOCOL contains
(version=?). (From this commit message alone, I would have expected a
lone integer, but that is not the case.)

Same comment for the commit message of PATCH v3 08/10.

Re: [PATCH v3 05/10] upload-pack, receive-pack: introduce protocol version 1

[Jonathan Tan]

On Tue,  3 Oct 2017 13:15:02 -0700
Brandon Williams  wrote:

> + switch (determine_protocol_version_server()) {
> + case protocol_v1:
> + if (advertise_refs || !stateless_rpc)
> + packet_write_fmt(1, "version 1\n");
> + /*
> +  * v1 is just the original protocol with a version string,
> +  * so just fall through after writing the version string.
> +  */

Peff sent out at least one patch [1] that reformats fallthrough comments
to be understood by GCC. It's probably worth doing here too.

In this case, I would do the 2-comment system that Peff suggested:

/*
 * v1 is just the original protocol with a version string,
 * so just fall through after writing the version string.
 */
if (advertise_refs || !stateless_rpc)
packet_write_fmt(1, "version 1\n");
/* fallthrough */

(I put the first comment before the code, so it doesn't look so weird.)

[1] 
https://public-inbox.org/git/20170921062541.ew67gyvrmb2ot...@sigill.intra.peff.net/

> + case protocol_v0:
> + break;
> + default:
> + BUG("unknown protocol version");
> + }

Re: [PATCH v3 04/10] daemon: recognize hidden request arguments

[Jonathan Tan]

On Tue,  3 Oct 2017 13:15:01 -0700
Brandon Williams  wrote:

>  /*
>   * Read the host as supplied by the client connection.

The return value is probably worth documenting. Something like "Returns
a pointer to the character *after* the NUL byte terminating the host
argument, or extra_args if there is no host argument."

>   */
> -static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
> +static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int 
> buflen)

[snip]

> +static void parse_extra_args(struct hostinfo *hi, struct argv_array *env,
> +  char *extra_args, int buflen)
> +{
> + const char *end = extra_args + buflen;
> + struct strbuf git_protocol = STRBUF_INIT;
> +
> + /* First look for the host argument */
> + extra_args = parse_host_arg(hi, extra_args, buflen);

This works, but is a bit loose in what it accepts. I think it's better
to be tighter - in particular, if there is no host argument, we
shouldn't be looking for extra args.

> +
> + /* Look for additional arguments places after a second NUL byte */
> + for (; extra_args < end; extra_args += strlen(extra_args) + 1) {

Assuming that the host argument exists, this for loop should start at
extra_args + 1 to skip the second NUL byte. This currently works
because this code is lenient towards empty strings.

> + const char *arg = extra_args;
> +
> + /*
> +  * Parse the extra arguments, adding most to 'git_protocol'
> +  * which will be used to set the 'GIT_PROTOCOL' envvar in the
> +  * service that will be run.
> +  *
> +  * If there ends up being a particular arg in the future that
> +  * git-daemon needs to parse specificly (like the 'host' arg)
> +  * then it can be parsed here and not added to 'git_protocol'.
> +  */
> + if (*arg) {
> + if (git_protocol.len > 0)
> + strbuf_addch(_protocol, ':');
> + strbuf_addstr(_protocol, arg);
> + }
> + }

But I think we shouldn't be lenient towards empty strings.

> +
> + if (git_protocol.len > 0)
> + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> +  git_protocol.buf);
> + strbuf_release(_protocol);
>  }

Re: [PATCH v3 02/10] pkt-line: add packet_write function

[Jonathan Tan]

On Tue,  3 Oct 2017 13:14:59 -0700
Brandon Williams  wrote:

> +void packet_write(const int fd_out, const char *buf, size_t size)

No need for "const" in "const int fd_out", I think. Same comment for the
header file.

> +{
> + if (packet_write_gently(fd_out, buf, size))
> + die_errno("packet write failed");
> +}

Re: [PATCH v3 01/10] connect: in ref advertisement, shallows are last

[Jonathan Tan]

On Tue,  3 Oct 2017 13:14:58 -0700
Brandon Williams  wrote:

> +static int process_dummy_ref(void)
> +{
> + struct object_id oid;
> + const char *name;
> +
> + if (parse_oid_hex(packet_buffer, , ))
> + return 0;
> + if (*name != ' ')
> + return 0;
> + name++;
> +
> + return !oidcmp(_oid, ) && !strcmp(name, "capabilities^{}");
> +}

Nit: If you're planning to parse_oid_hex, you can strcmp with
" capabilities^{}" directly (note the space at the start) instead of
first checking for ' ' then "capabilities^{}".

Re: [PATCH] submodule: avoid sentence-lego in translated string

[Martin Ågren]

On 10 October 2017 at 04:35, Changwoo Ryu  wrote:
> 2017-10-10 10:26 GMT+09:00 Junio C Hamano :
>> Jean-Noël AVILA  writes:
>>
>>> On Monday, 9 October 2017, 09:47:26 CEST Stefan Beller wrote:
>>>
 I always assumed that translators are aware of these issues and sort of
 work around this somehow, maybe like this:

   "submodule entry '%s' (%s) is not a commit. It is of type %s"
>>>
>>> Translators can be aware of the issue if the coder commented the
>>> internationalization string with some possible candidates for the 
>>> placeholders
>>> when it is not clear unless you check in the source code. Much effort was
>>> poured into translating the technical terms in other parts of Git; it seems
>>> awkward to just step back in this occurence.
>>
>> I do not see this particular case as "stepping back", though.
>>
>> Our users do not spell "git cat-file -t commit v2.0^{commit}" with
>> 'commit' translated to their language, right?  Shouldn't an error
>> message output use the same phrase the input side requests users to
>> use?

I thought Jean-Noël meant at least partially the translator-experience,
not just the user-experience, but I might be wrong.

I prepared a patch to give a TRANSLATORS:-comment, but then I realized
that we have more instances like this with `typename()`. Actually, quite
often we avoid the issue (intentionally or unintentionally) by writing
"of type %s", but other times, we do the "is a %s". So I don't know,
maybe it all works anyway. The regular translators have now received 10
mails (11 counting this) so might be aware of this particular string by
now. :-/

> Users know the limit of command-line translation. They type "commit"
> to commit but they see translated "commit" in output messages. It is
> actually confusing. But the untranslated English literals in the
> middle of translated sentences does not remove the confusion but
> increase it in a different way.

What you describe seems plausible, but I have to admit that I don't use
i18n-ized software myself.

Martin

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 10 October 2017 at 16:29, Jeff King  wrote:
> On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
>
> it will randomly succeed or fail, depending on whether sed manages to
> read the input before the stdin terminal is closed.
>
> I'm not sure of an easy way to fix test-terminal, but we could work
> around it like this (which also uses "-p" to actually invoke the pager,
> and uses a pager that makes it clear when it's being run):
>
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 9441145bf0..d322c3b745 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -180,14 +180,14 @@ EOF
>
>  test_expect_success TTY '20 columns, mode auto, pager' '
> cat >expected <<\EOF &&
> -oneseven
> -twoeight
> -three  nine
> -four   ten
> -five   eleven
> -six
> +paged:oneseven
> +paged:twoeight
> +paged:three  nine
> +paged:four   ten
> +paged:five   eleven
> +paged:six
>  EOF
> -   test_terminal env PAGER="cat|cat" git column --mode=auto  >actual &&
> +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> --mode=auto actual &&
> test_cmp expected actual
>  '
>  test_done

Makes sense. FWIW, I don't see the flakyness with this.

> All that said, I think I'd just as soon test a real command like "git
> tag", which doesn't care about reading from stdin.

For reference for Kevin, in case you consider testing, e.g., git tag,
the main reason I referred to the test I posted as "hacky", was that I
just inserted it at a more-or-less random place in t7006. So it had to
play with `git reset` to avoid upsetting the later tests. It could
obviously go to the end instead, but I was too lazy to move it and
define a pager.

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 10 October 2017 at 16:01, Jeff King  wrote:
> On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
>> On 9 October 2017 at 23:45, Kevin Daudt  wrote:
>> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
>> > 2017-08-02), the pager has been enabled by default, exposing this
>> > problem to more people.
>>
>> Oh. :( I didn't know about "column" to be honest.
>
> Yeah, I didn't think of that with respect to the pager. This is a
> regression in v2.14.2, I think.

Yes.

>> In any case, it might make sense to test an
>> actual use-case also. Of course, the code should be largely the same,
>> but in builtin/tag.c, it's quite important that `setup_auto_pager()`
>> and `finalize_colopts()` are called in the right order.
>
> I think it might work out either way. If we have started the pager when
> we finalize_colopts(), then the pager_in_use() bit will kick in. If we
> haven't, then either:
>
>   1. stdout is a tty, and we'll kick in the auto behavior for columns,
>  and then later for the pager.
>
>   2. stdout isn't a tty, in which case we also won't kick in the pager.

Right you are.

Re: git download issue

[Jeff King]

On Tue, Oct 10, 2017 at 03:45:38PM +0200, Anthony Chevalet wrote:

> I can't download git for windows from https://git-scm.com/downloads
> 
> https://git-scm.com/download/win redirects to https://git-scm.com/downloads
> 
> Any hint?

Thanks for reporting. This should be fixed now, and is related to some
database maintenance I was doing related to manpage versions[1].

-Peff

[1] https://github.com/git/git-scm.com/issues/1041#issuecomment-335437722
if you're curious.

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:

> That said, I'm still puzzled why it would return zero output. Strace
> shows that the read from stdin is getting no input. I suspect this may
> have to do with how we redirect stdin in test-terminal.perl.
> 
> See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), which claims there's some raciness with closing the
> descriptor.

Ah, yeah, I think that is it. Try:

  echo input | test_terminal sed s/^/foo:/

it will randomly succeed or fail, depending on whether sed manages to
read the input before the stdin terminal is closed.

I'm not sure of an easy way to fix test-terminal, but we could work
around it like this (which also uses "-p" to actually invoke the pager,
and uses a pager that makes it clear when it's being run):

diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 9441145bf0..d322c3b745 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -180,14 +180,14 @@ EOF
 
 test_expect_success TTY '20 columns, mode auto, pager' '
cat >expected <<\EOF &&
-oneseven
-twoeight
-three  nine
-four   ten
-five   eleven
-six
+paged:oneseven
+paged:twoeight
+paged:three  nine
+paged:four   ten
+paged:five   eleven
+paged:six
 EOF
-   test_terminal env PAGER="cat|cat" git column --mode=auto actual 
&&
+   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
--mode=auto actual &&
test_cmp expected actual
 '
 test_done

All that said, I think I'd just as soon test a real command like "git
tag", which doesn't care about reading from stdin.

-Peff

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Mon, Oct 09, 2017 at 11:45:43PM +0200, Kevin Daudt wrote:

> +test_expect_success TTY '20 columns, mode auto, pager' '
> + cat >expected <<\EOF &&
> +oneseven
> +twoeight
> +three  nine
> +four   ten
> +five   eleven
> +six
> +EOF
> + test_terminal env PAGER="cat|cat" git column --mode=auto actual 
> &&
> + test_cmp expected actual
> +'

I don't think "git column" will run the pager by default, will it?
You'd need "git -p" here.

That said, I'm still puzzled why it would return zero output. Strace
shows that the read from stdin is getting no input. I suspect this may
have to do with how we redirect stdin in test-terminal.perl.

See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), which claims there's some raciness with closing the
descriptor.

-Peff

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:

> On 9 October 2017 at 23:45, Kevin Daudt  wrote:
> > When columns are set to automatic for git tag and the output is
> > paginated by git, the output is a single column instead of multiple
> > columns.
> >
> > Standard behaviour in git is to honor auto values when the pager is
> > active, which happens for example with commands like git log showing
> > colors when being paged.
> >
> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> > 2017-08-02), the pager has been enabled by default, exposing this
> > problem to more people.
> 
> Oh. :( I didn't know about "column" to be honest.

Yeah, I didn't think of that with respect to the pager. This is a
regression in v2.14.2, I think.

I agree that anything that is "auto" on stdout probably ought to kick in
when the pager is in effect (since that only kicks in when stdout _was_
a tty before we stuck a pager in front of it).

> I had slightly more success with PAGER="cat >actual", but the test is
> flaky for some reason.

The test in t9002 should be immune to this, but the one you suggest in
t7006 would need to set COLUMNS to get consistent output, I think.

> In any case, it might make sense to test an
> actual use-case also. Of course, the code should be largely the same,
> but in builtin/tag.c, it's quite important that `setup_auto_pager()`
> and `finalize_colopts()` are called in the right order.

I think it might work out either way. If we have started the pager when
we finalize_colopts(), then the pager_in_use() bit will kick in. If we
haven't, then either:

  1. stdout is a tty, and we'll kick in the auto behavior for columns,
 and then later for the pager.

  2. stdout isn't a tty, in which case we also won't kick in the pager.

-Peff

git download issue

[Anthony Chevalet]

Hello,

I can't download git for windows from https://git-scm.com/downloads

https://git-scm.com/download/win redirects to https://git-scm.com/downloads

Any hint?

Thanks,
Anthony

Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine

[SZEDER Gábor]

> The cmd_foo() function is a moral equivalent of 'main' for a Git
> subcommand 'git foo', and as such, it is allowed to do many things
> that make it unsuitable to be called as a subroutine, including
> 
>  - call exit(3) to terminate the process;
> 
>  - allocate resource held and used throughout its lifetime, without
>releasing it upon return/exit;
> 
>  - rely on global variables being initialized at program startup,
>and update them as needed, making another clean invocation of the
>function impossible.
> 
> The call to cmd_diff_index() "git describe" makes has been working
> by accident that the function did not call exit(3); it sets a bad
> precedent for people to cut and paste.

The subject implies to me that this patch eliminates all cmd_*() calls
in builtin/describe.c, but a call to cmd_name_rev() still remains.

However, that call is special in the sense that cmd_describe() returns
immediately thereafter, so none of the above three points are an issue
there.  Someone might argue that it still sets a bad precedent, but I
won't :)  To avoid the direct cmd_name_rev() call we would have to use
run_command(), because there are no libified helper functions
available to do the job, adding the overhead of a fork()+exec(),
though only once or nonce, depending on cmdline options.

Maybe you already considered all this WRT that cmd_name_rev() call, I
don't know.  In any case, I think at least the subject line should
spell out cmd_diff_index().


Gábor


> We could invoke it via the run_command() interface, but the diff
> family of commands have helper functions in diff-lib.c that are
> meant to be usable as subroutines, and using the latter does not
> make the resulting code all that longer.  Use it.
> 
> Signed-off-by: Junio C Hamano 
> ---
>  builtin/describe.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 89ea1cdd60..50263759cb 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -7,12 +7,12 @@
>  #include "builtin.h"
>  #include "exec_cmd.h"
>  #include "parse-options.h"
> +#include "revision.h"
>  #include "diff.h"
>  #include "hashmap.h"
>  #include "argv-array.h"
>  #include "run-command.h"
>  
> -#define SEEN (1u << 0)
>  #define MAX_TAGS (FLAG_BITS - 1)
>  
>  static const char * const describe_usage[] = {
> @@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char 
> *prefix)
>   }
>   } else if (dirty) {
>   static struct lock_file index_lock;
> - int fd;
> + struct rev_info revs;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + int fd, result;
>  
>   read_cache_preload(NULL);
>   refresh_index(_index, 
> REFRESH_QUIET|REFRESH_UNMERGED,
> @@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char 
> *prefix)
>   if (0 <= fd)
>   update_index_if_able(_index, _lock);
>  
> - if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
> - diff_index_args, prefix))
> + init_revisions(, prefix);
> + argv_array_pushv(, diff_index_args);
> + if (setup_revisions(args.argc, args.argv, , NULL) 
> != 1)
> + BUG("malformed internal diff-index command 
> line");
> + result = run_diff_index(, 0);
> +
> + if (!diff_result_code(, result))
>   suffix = NULL;
>   else
>   suffix = dirty;
> -- 
> 2.15.0-rc0-203-g4c8d0e28b1

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Jeff King]

On Tue, Oct 10, 2017 at 09:11:15AM -0400, Derrick Stolee wrote:

> On 10/10/2017 8:56 AM, Junio C Hamano wrote:
> > Jeff King  writes:
> > 
> > > OK, I think that makes more sense. But note the p->num_objects thing I
> > > mentioned. If I do:
> > > 
> > >git pack-objects .git/objects/pack/pack  > > 
> > > then I have a pack with zero objects, which I think we'd similarly want
> > > to return early from. I.e., I think we need:
> > > 
> > >if (p->num_objects)
> > >   return;
> > > 
> > > Technically that also covers open_pack_index() failure, too, but that's
> > > a subtlety I don't think we should rely on.
> > True.  I notice that the early part of the two functions look almost
> > identical.  Do we need error condition handling for the other one,
> > too?
> 
> I prefer to fix the problem in all code clones when they cause review
> friction, so I'll send a fifth commit showing just the diff for these
> packfile issues in sha1_name.c. See patch below.

Ah, that answers my earlier question. Junio mean unique_in_pack(). And
yeah, I think it suffers from the same problem.

> Should open_pack_index() return a non-zero status if the packfile is empty?
> Or, is there a meaningful reason to have empty packfiles?

I can't think of a compelling reason to have an empty packfile. But nor
do I think we should consider them an error when we can handle them
quietly (and returning non-zero status would cause Git to complain on
many operations in a repository that has such a file).

-Peff

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Derrick Stolee]

On 10/10/2017 8:56 AM, Junio C Hamano wrote:

Jeff King  writes:


OK, I think that makes more sense. But note the p->num_objects thing I
mentioned. If I do:

   git pack-objects .git/objects/pack/pack num_objects)
return;

Technically that also covers open_pack_index() failure, too, but that's
a subtlety I don't think we should rely on.

True.  I notice that the early part of the two functions look almost
identical.  Do we need error condition handling for the other one,
too?


I prefer to fix the problem in all code clones when they cause review 
friction, so I'll send a fifth commit showing just the diff for these 
packfile issues in sha1_name.c. See patch below.


Should open_pack_index() return a non-zero status if the packfile is 
empty? Or, is there a meaningful reason to have empty packfiles?


Thanks,
-Stolee


diff --git a/sha1_name.c b/sha1_name.c
index 49ba67955..9f8a33e82 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -153,7 +153,9 @@ static void unique_in_pack(struct packed_git *p,
    uint32_t num, last, i, first = 0;
    const struct object_id *current = NULL;

-   open_pack_index(p);
+   if (open_pack_index(p) || !p->num_objects)
+   return;
+
    num = p->num_objects;
    last = num;
    while (first < last) {
@@ -513,7 +515,9 @@ static void find_abbrev_len_for_pack(struct 
packed_git *p,

    uint32_t num, last, first = 0;
    struct object_id oid;

-   open_pack_index(p);
+   if (open_pack_index(p) || !p->num_objects)
+   return;
+
    num = p->num_objects;
    last = num;
    while (first < last) {

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Jeff King]

On Tue, Oct 10, 2017 at 09:56:38PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > OK, I think that makes more sense. But note the p->num_objects thing I
> > mentioned. If I do:
> >
> >   git pack-objects .git/objects/pack/pack  >
> > then I have a pack with zero objects, which I think we'd similarly want
> > to return early from. I.e., I think we need:
> >
> >   if (p->num_objects)
> > return;
> >
> > Technically that also covers open_pack_index() failure, too, but that's
> > a subtlety I don't think we should rely on.
> 
> True.  I notice that the early part of the two functions look almost
> identical.  Do we need error condition handling for the other one,
> too?

I'm not sure which two you mean. Do you mean find_pack_entry_one() in
packfile.c as the other one? If so, I think it is fine in the
zero-object case, because it does not do the "this is the sha1 at the
position where it _would_ be found" trick, which is what causes us to
potentially dereference nonsense.

-Peff

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > :( I was worried that this might hit some third-party scripts.
> > ...
> > All that said, should we revisit the decision from 6be4595edb? The two
> > code changes we could make are:
> >
> >   1. Adding a "--color" option to "git status". Commit 0c88bf5050
> >  (provide --color option for all ref-filter users, 2017-10-03) from
> >  that same series shows some prior art.
> >
> >  This is a clean solution, but it does mean that scripts have to
> >  adapt (and would potentially need to care about which Git version
> >  they're relying on).
> 
> If we view that "always" issue is a regression, then this is not a
> "solution".  It is a part of an ideal world where we never allowed
> "always" as a value for color.ui, which is not the world we live in.

Right, this doesn't solve any regression. It solves the "there's no way
to do this thing I might want to" that exists in either world (one where
"always" never existed, or one where "always" does not do that anymore
but we accept it as either not a regression or an acceptable
regression).

> >   2. Re-allow "color.always" config from the command-line. It's actually
> >  on-disk config that we want to downgrade, but I wanted to avoid
> >  making complicated rules about how the config would behave in
> >  different scopes. The patch for this would look something like the
> >  one below.
> 
> Yuck, ugly.  The code is simple (thanks to the "who ordered it?"
> thing), but the behaviour is rather embarrassing to explain.

Yes, it's definitely the most ugly of all the options. The reason I
mention it is that it's also the only one that solves the "git -c
color.ui=always" regression (if we consider it one) without making a
huge and risky change.

> >   3. Revert the original series, and revisit the original "respect
> >  color.ui via porcelain" commit which broke add--interactive in
> >  v2.14.2 (136c8c8b8fa).
> 
> Which one do you mean is "the original series"?  The one that made
> plumbing to pay attention to the color config?

No, I meant reverting jk/ui-color-always-to-auto. But if we revert that,
it leaves "add -p" broken when you set color.ui=always. So we must
either accept that, or _also_ revert 136c8c8b8fa and come up with a
different solution.

> I think it would be
> the cleanest "solution" in the world we live in, but the series (and
> the follow-on changes that started assuming that config_default
> reads the color config) have a rather large footprint and it will be
> quite painful to vet the result.

I agree that is a risk.  It might not be _too_ bad, though this is an
area that historically has poor test coverage. I know that for-each-ref
and tag are two that would need touched (and it was them that led me
down to the path to 136c8c8b8fa in the first place).

> I think the right fix to the original problem (you cannot remove
> auto-color from the plumbing) is to stop paying attention to color
> configuration from the default config.  I wonder if something like
> this would work?
> 
>  - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;
> 
>  - When git_color_config() is called, and if git_use_color_default
>is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless
>of the variable git_color_config() is called for).
> 
>  - In color.c::want_color(), when git_use_color_default is used,
>notice if it is GIT_COLOR_UNKNOWN and behave as if it is
>GIT_COLOR_NEVER.
>
> Then we make sure that git_color_config() is never called by any
> plumbing command.  The fact it is (ever) called can be taken as a
> clue that we are running a Porcelain (hence we transition from
> UNKNOWN to AUTO), so we'd get the desirable "no default color for
> plumbing, auto color for Porcelain", I would think.

Yes, I think that's the simplest way to implement the "plumbing should
never do color without a command-line option" scheme.

I do wonder if people would end up seeing some corner cases as
regressions, though. Right now "diff-tree" _does_ color the output by
default, and it would stop doing so under your scheme. That's the right
thing to do by the plumbing/porcelain distinction, but users with
scripts that use diff-tree (or other plumbing) to generate user-visible
output may unexpectedly lose their color, until the calling script is
fixed to add back in a --color option[1].

-Peff

[1] Actually, just saying "--color" isn't enough, since you'd want to
respect the user's color options. add--interactive does this, but
it's a slight pain. It would be nice to have a --color=config
variant that just calls git_color_config(). But if we are talking
regression-fixes before v2.15, I don't think we need to have such
niceties.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

Hi,

On Mon, Oct 09, 2017 at 11:20:51AM -0700, Stefan Beller wrote:
> On Fri, Oct 6, 2017 at 3:32 PM, Heiko Voigt  wrote:
> > NOTE: The argument in this message is not correct, see description in
> > cover letter.
> >
> > The setup of the repositories in this test is using gitlinks without the
> > .gitmodules infrastructure. It is however testing convenience features
> > like --recurse-submodules=on-demand. These features are already not
> > supported by fetch without a .gitmodules file. This leads us to the
> > conclusion that it is not really used here as well.
> >
> > Let's use the usual submodule commands to setup the repository in a
> > typical way. This also has the advantage that we are testing with a
> > repository structure that is more similar to one we could expect on a
> > users setup.
> >
> > Signed-off-by: Heiko Voigt 
> > ---
> >
> > As mentioned in the cover letter. This seems to be the only test that
> > ensures that we stay compatible with setups without .gitmodules. Maybe
> > we should add/revive some?
> 
> An interesting discussion covering this topic is found at
> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3...@sigill.intra.peff.net/

Thanks for that pointer. So in that discussion Junio said that the
recursive operations should succeed if we have everything necessary at
hand. I kind of agree because why should we limit usage when not
necessary. On the other hand we want git to be easy to use. And that
example from Peff is perfect as a demonstration of a incosistency we
currently have:

git clone git://some.where.git/submodule.git
git add submodule

is an operation I remember, I did, when first getting in contact with
submodules (many years back), since that is one intuitive way. And the
thing is: It works, kind of... Only later I discovered that one actually
needs to us a special submodule command to get everything approriately
setup to work together with others.

If everyone agrees that submodules are the default way of handling
repositories insided repositories, IMO, 'git add' should also alter
.gitmodules by default. We could provide a switch to avoid doing that.

An intermediate solution would be to warn but in the long run my goal
for submodules is and always was: Make them behave as close to files as
possible. And why should a 'git add submodule' not magically do
everything it can to make submodules just work? I can look into a patch
for that if people agree here...

Regarding handling of gitlinks with or without .gitmodules:

Currently we are actually in some intermediate state:

 * If there is no .gitmodules file: No submodule processing on any
   gitlinks (AFAIK)
 * If there is a .gitmodules files with some submodule configured: Do
   recursive fetch and push as far as possible on gitlinks.

So I am not sure whether there are actually many users (knowingly)
using a mix of some submodules configured and some not and then relying
on the submodule infrastructure.

I would rather expect two sorts of users:

  1. Those that do use .gitmodules

  2. Those that do *not* use .gitmodules

Users that do not use any .gitmodules file will currently (AFAIK) not
get any submodule handling. So the question is are there really many
"mixed users"? My guess would be no.
Because without those using this mixed we could switch to saying: "You
need to have a .gitmodules file for submodule handling" without much
fallout from breaking users use cases.

Maybe we can test this out somehow? My patch series would be ready in
that case, just had to drop the first patch and adjust the commit
message of this one.

Cheers Heiko

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Junio C Hamano]

Jeff King  writes:

> OK, I think that makes more sense. But note the p->num_objects thing I
> mentioned. If I do:
>
>   git pack-objects .git/objects/pack/pack 
> then I have a pack with zero objects, which I think we'd similarly want
> to return early from. I.e., I think we need:
>
>   if (p->num_objects)
>   return;
>
> Technically that also covers open_pack_index() failure, too, but that's
> a subtlety I don't think we should rely on.

True.  I notice that the early part of the two functions look almost
identical.  Do we need error condition handling for the other one,
too?

Re: What happened to "git status --color=(always|auto|never)"?

[Junio C Hamano]

Jeff King  writes:

> :( I was worried that this might hit some third-party scripts.
> ...
> All that said, should we revisit the decision from 6be4595edb? The two
> code changes we could make are:
>
>   1. Adding a "--color" option to "git status". Commit 0c88bf5050
>  (provide --color option for all ref-filter users, 2017-10-03) from
>  that same series shows some prior art.
>
>  This is a clean solution, but it does mean that scripts have to
>  adapt (and would potentially need to care about which Git version
>  they're relying on).

If we view that "always" issue is a regression, then this is not a
"solution".  It is a part of an ideal world where we never allowed
"always" as a value for color.ui, which is not the world we live in.

>   2. Re-allow "color.always" config from the command-line. It's actually
>  on-disk config that we want to downgrade, but I wanted to avoid
>  making complicated rules about how the config would behave in
>  different scopes. The patch for this would look something like the
>  one below.

Yuck, ugly.  The code is simple (thanks to the "who ordered it?"
thing), but the behaviour is rather embarrassing to explain.

>   3. Revert the original series, and revisit the original "respect
>  color.ui via porcelain" commit which broke add--interactive in
>  v2.14.2 (136c8c8b8fa).

Which one do you mean is "the original series"?  The one that made
plumbing to pay attention to the color config?  I think it would be
the cleanest "solution" in the world we live in, but the series (and
the follow-on changes that started assuming that config_default
reads the color config) have a rather large footprint and it will be
quite painful to vet the result.

I think the right fix to the original problem (you cannot remove
auto-color from the plumbing) is to stop paying attention to color
configuration from the default config.  I wonder if something like
this would work?

 - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;

 - When git_color_config() is called, and if git_use_color_default
   is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless
   of the variable git_color_config() is called for).

 - In color.c::want_color(), when git_use_color_default is used,
   notice if it is GIT_COLOR_UNKNOWN and behave as if it is
   GIT_COLOR_NEVER.

Then we make sure that git_color_config() is never called by any
plumbing command.  The fact it is (ever) called can be taken as a
clue that we are running a Porcelain (hence we transition from
UNKNOWN to AUTO), so we'd get the desirable "no default color for
plumbing, auto color for Porcelain", I would think.

Re: "git rm" seems to do recursive removal even without "-r"

[Paul Smith]

On Tue, 2017-10-10 at 04:36 -0400, Robert P. J. Day wrote:
>   ah, now *that* is a compelling rationale that justifies the
> underlying weirdness. but it still doesn't explain the different
> behaviour between:
> 
>   $ git rm -n 'Makefile*'
>   $ git rm -n '*Makefile'

I explained that behavior in the email up-thread from this reply:

> Globbing in "git rm" matches on the FULL PATH, not just the file name. 
> So, if you have a list of Makefiles in your repository like:
> 
>   Makefile
>   foo/Makefile
>   bar/Makefile
> 
> Then 'Makefile*' only matches the first one, since 'Makefile*' doesn't
> match 'foo/Makefile' or 'bar/Makefile'.
>
> If you you worry that '*Makefile' will match things you don't want to
> match, you'll have to use:
> 
>   git rm -n Makefile '*/Makefile'

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Jeff King]

On Tue, Oct 10, 2017 at 08:16:27AM -0400, Derrick Stolee wrote:

> > > + mad->init_len = 0;
> > > + if (!match) {
> > > + nth_packed_object_oid(, p, first);
> > > + extend_abbrev_len(, mad);
> > If we have zero objects in the pack, what would nth_packed_object_oid()
> > be returning here?
> > 
> > So I actually think we do want an early return, not just when
> > open_packed_index() fails, but also when p->num_objects is zero.
> > 
> > -Peff
> 
> Sorry about this. I caught this while I was writing my cover letter and
> amended my last commit to include the following:
> 
>     if (open_pack_index(p))
>         return;
> 
> After I amended the commit, I forgot to 'format-patch' again. I can send a
> diff between the commits after review has calmed.

OK, I think that makes more sense. But note the p->num_objects thing I
mentioned. If I do:

  git pack-objects .git/objects/pack/pack num_objects)
return;

Technically that also covers open_pack_index() failure, too, but that's
a subtlety I don't think we should rely on.

-Peff

Re: [PATCH v4 4/4] sha1_name: minimize OID comparisons during disambiguation

[Derrick Stolee]

On 10/9/2017 9:49 AM, Jeff King wrote:

On Sun, Oct 08, 2017 at 02:49:42PM -0400, Derrick Stolee wrote:


@@ -505,6 +506,65 @@ static int extend_abbrev_len(const struct object_id *oid, 
void *cb_data)
return 0;
  }
  
+static void find_abbrev_len_for_pack(struct packed_git *p,

+struct min_abbrev_data *mad)
+{
+   int match = 0;
+   uint32_t num, last, first = 0;
+   struct object_id oid;
+
+   open_pack_index(p);
+   num = p->num_objects;
+   last = num;
+   while (first < last) {
[...]

Your cover letter lists:

   * Silently skip packfiles that fail to open with open_pack_index()

as a change from the previous version. But this looks the same as the
last round. I think this _does_ end up skipping such packfiles because
p->num_objects will be zero. Is it worth having a comment to that
effect (or even just an early return) to make it clear that the
situation is intentional?

Although...


+   /*
+* first is now the position in the packfile where we would insert
+* mad->hash if it does not exist (or the position of mad->hash if
+* it does exist). Hence, we consider a maximum of three objects
+* nearby for the abbreviation length.
+*/
+   mad->init_len = 0;
+   if (!match) {
+   nth_packed_object_oid(, p, first);
+   extend_abbrev_len(, mad);

If we have zero objects in the pack, what would nth_packed_object_oid()
be returning here?

So I actually think we do want an early return, not just when
open_packed_index() fails, but also when p->num_objects is zero.

-Peff


Sorry about this. I caught this while I was writing my cover letter and 
amended my last commit to include the following:


    if (open_pack_index(p))
        return;

After I amended the commit, I forgot to 'format-patch' again. I can send 
a diff between the commits after review has calmed.


Thanks,
-Stolee

Re: "git rm" seems to do recursive removal even without "-r"

[Heiko Voigt]

On Sun, Oct 08, 2017 at 07:56:20AM -0400, Robert P. J. Day wrote:
>   but as i asked in my earlier post, if i wanted to remove *all* files
> with names of "Makefile*", why can't i use:
> 
>   $ git rm 'Makefile*'
> 
> just as i used:
> 
>   $ git rm '*.c'
> 
> are those not both acceptable fileglobs? why does the former clearly
> only match the top-level Makefile, and refuse to cross directory
> boundaries?

Maybe think about it this way: The only difference between git's
globbing and the default shell globbing is that the '/' in a path has a
special meaning. The shells expansion stops at a '/' but git does not.

So with *.c the shell matches: blabla.c, blub.c, ...  but not
subdir/bla.c, subdir/blub.c, ... since it only considers files in the
current directory. A little different for Makefile* that will also match
Makefile.bla, Makefile/bla or Makefile_bla/blub in shell but not
subdir/Makefile or bla.Makefile. Basically anything directly in *this*
directory that *starts* with 'Makefile'.

Git on the other hand does not consider '/' to be special. So *.c
matches all of the path above: bla.c, blub.c, subdir/bla.c,
subdir/blub.c. Basically any file below the current directory with a
path that ends in '.c'. With Makefile* it is the opposite: Every file
below the current directory that *starts* with 'Makefile'. So
Makefile.bla, Makefile/bla, ... but also not subdir/Makefile or
bla.Makefile.

Maybe that helps...

Cheers Heiko

My name is Sylvanus Eyadema , i have written you the letter about one a month ago and i don't hear from you therefore i repeated this letter once again to you, i in possession of the leading documents

[Sylvanus Eyadéma]

Wir haben ein $ 6,480,000.00 Paket, um Ihnen zu liefern!

[chetumal . ventanilla]

Wir haben ein Paket für Sie mit einem gewinnenden Scheck lohnt sich die Summe 
von sechs Millionen vierhundert und achtzehn tausend United States Dollars ($ 
6.480.000,00USD) und auch ein Apple MacBook Pro und das neue Apple iPhone (7) 
256GB Handy hinzugefügt zu Ihrem Paket, das Ihnen geliefert wird, nachdem Sie 
alle erforderlichen Informationen an die FedEx Delivery Company geliefert 
haben, bevor das Paket an Ihre eigene Wohnadresse in Ihrem Land versandt werden 
kann.

Liefermanager.
Name: Mrs.Rose Michael.
E-Mail: fed...@qq.com

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 9 October 2017 at 23:45, Kevin Daudt  wrote:
> When columns are set to automatic for git tag and the output is
> paginated by git, the output is a single column instead of multiple
> columns.
>
> Standard behaviour in git is to honor auto values when the pager is
> active, which happens for example with commands like git log showing
> colors when being paged.
>
> Since ff1e72483 (tag: change default of `pager.tag` to "on",
> 2017-08-02), the pager has been enabled by default, exposing this
> problem to more people.

Oh. :( I didn't know about "column" to be honest.

> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to autol. Also check
> if the pager is active.
>
> Helped-by: Rafael Ascensão 
> Signed-off-by: Kevin Daudt 
> ---
> This came to light when someone wondered on irc why
> column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
> found it to work with always, but not with auto.
>
> I could not get the test to work yet, because somehow it's not giving
> any output, so feedback regarding that is welcome.

I had slightly more success with PAGER="cat >actual", but the test is
flaky for some reason. In any case, it might make sense to test an
actual use-case also. Of course, the code should be largely the same,
but in builtin/tag.c, it's quite important that `setup_auto_pager()`
and `finalize_colopts()` are called in the right order. In other words,
there is some regression-potential. This is a whitespace-damaged and
hacky attempt to test. Maybe it helps a little. I hope I'll have more
time later today.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..91f2b5871 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -214,6 +214,19 @@ test_expect_success TTY 'git tag as alias
respects pager.tag with -l' '
! test -e paginated.out
 '

+test_expect_success TTY 'git tag with column.tag=auto' '
+   test_commit second &&
+   test_commit third &&
+   test_commit fourth &&
+   test_when_finished "git reset --hard HEAD~3" &&
+   cat >expected <<\EOF &&
+fourth   initial  second   third
+EOF
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag -c column.tag=auto tag &&
+   test_cmp expected paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

On Tue, Oct 10, 2017 at 12:42:43PM +0800, Nazri Ramliy wrote:

> > commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87
> > Author: Jeff King 
> > Date:   Tue Oct 3 09:46:06 2017 -0400
> >
> > color: make "always" the same as "auto" in config
> >
> > Would you like to take a stab at adding it?  builtin/commit.c and
> > Documentation/git-{commit,status}.txt would be my best guesses at
> > where to start.
> 
> Perhaps, seeing that that commit intentionally "broke" the color
> output of my tool[1], because it parses the output of `git -c
> color.status=always status`, which now no longer work the way it used
> to. I know, I know... shame on me for parsing the output of a
> porcelain command :)
> 
> But this also means that I will have to modify [1] to cope with this,
> given that it may be used with an older version of git (parse
> git-version and shell out to different git command - either `git -c
> color.ui=always status`, or the not-yet-exist `git status
> --color=always`), or make it use the plumbing output of `git status`,
> but that would just add additional work that  I really don't look
> forward to doing at this moment.

:( I was worried that this might hit some third-party scripts.

One workaround you can do that should work with any version of Git is:

  GIT_PAGER_IN_USE=1 git status | your-parser

That tells Git that even though stdout isn't a tty, you're expecting to
present the data to the user and it should be colored appropriately. It
has the additional upside that it doesn't override the user's color
config.

It does have the potential downside that other non-color changes could
kick in (e.g., somebody recently proposed that auto-columns kick in with
GIT_PAGER_IN_USE).

All that said, should we revisit the decision from 6be4595edb? The two
code changes we could make are:

  1. Adding a "--color" option to "git status". Commit 0c88bf5050
 (provide --color option for all ref-filter users, 2017-10-03) from
 that same series shows some prior art.

 This is a clean solution, but it does mean that scripts have to
 adapt (and would potentially need to care about which Git version
 they're relying on).

  2. Re-allow "color.always" config from the command-line. It's actually
 on-disk config that we want to downgrade, but I wanted to avoid
 making complicated rules about how the config would behave in
 different scopes. The patch for this would look something like the
 one below.

  3. Revert the original series, and revisit the original "respect
 color.ui via porcelain" commit which broke add--interactive in
 v2.14.2 (136c8c8b8fa).

I dunno. I think for your use case, PAGER_IN_USE may actually be the
"right" solution, because it most closely expresses what you're doing.
We probably ought to have (1) as a general rule for commands which
handle color.

But (2) and (3) are the only ones that will work seamlessly with
existing scripts. I'm not excited about either of them, though.

-Peff

diff --git a/color.c b/color.c
index 9c0dc82370..3870d3e395 100644
--- a/color.c
+++ b/color.c
@@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char 
*value)
if (value) {
if (!strcasecmp(value, "never"))
return 0;
-   if (!strcasecmp(value, "always"))
-   return var ? GIT_COLOR_AUTO : 1;
+   if (!strcasecmp(value, "always")) {
+   /*
+* Command-line options always respect "always".
+* Likewise for "-c" config on the command-line.
+*/
+   if (!var ||
+   current_config_scope() == CONFIG_SCOPE_CMDLINE)
+   return 1;
+
+   /*
+* Otherwise, we're looking at on-disk config;
+* downgrade to auto.
+*/
+   return GIT_COLOR_AUTO;
+   }
if (!strcasecmp(value, "auto"))
return GIT_COLOR_AUTO;
}

Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter

[Simon Ruderich]

On Tue, Oct 10, 2017 at 05:25:43AM -0400, Jeff King wrote:
> On Tue, Oct 10, 2017 at 11:23:19AM +0200, Simon Ruderich wrote:
>> On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:
 --- a/entry.c
 +++ b/entry.c
 @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,
if (dco && dco->state != CE_NO_DELAY) {
/* Do not send the blob in case of a retry. */
if (dco->state == CE_RETRY) {
 +  free(new);
new = NULL;
size = 0;
}
>>
>> FREE_AND_NULL(new)?
>
> Ah, yeah, I forgot we had that now. It would work here, but note that
> this code ends up going away later in the series.

Ah sorry, missed that. Then never mind.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH v2 1/3] for-each-ref: let upstream/push optionally report the remote name

[Junio C Hamano]

Johannes Schindelin  writes:

> There are times when e.g. scripts want to know not only the name of the
> upstream branch on the remote repository, but also the name of the
> remote.
>
> This patch offers the new suffix :remotename for the upstream and for
> the push atoms, allowing to show exactly that. Example:
> ...
>   $ git for-each-ref \
>   --format='%(upstream) %(upstream:remote) %(push:remote)' \
>   refs/heads/master
>   refs/remotes/origin/master origin hello-world

While doing the regular re-review of the topics in flight, I noticed
that there are two references to :remote here that should be :remotename,
so I tweaked them locally and pushed the result out as part of 'pu'.

Thanks.

Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter

[Jeff King]

On Tue, Oct 10, 2017 at 11:23:19AM +0200, Simon Ruderich wrote:

> On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:
> >> --- a/entry.c
> >> +++ b/entry.c
> >> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,
> >>if (dco && dco->state != CE_NO_DELAY) {
> >>/* Do not send the blob in case of a retry. */
> >>if (dco->state == CE_RETRY) {
> >> +  free(new);
> >>new = NULL;
> >>size = 0;
> >>}
> 
> FREE_AND_NULL(new)?

Ah, yeah, I forgot we had that now. It would work here, but note that
this code ends up going away later in the series.

-Peff

Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter

[Simon Ruderich]

On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,
>>  if (dco && dco->state != CE_NO_DELAY) {
>>  /* Do not send the blob in case of a retry. */
>>  if (dco->state == CE_RETRY) {
>> +free(new);
>>  new = NULL;
>>  size = 0;
>>  }

FREE_AND_NULL(new)?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: "git rm" seems to do recursive removal even without "-r"

[Junio C Hamano]

"Robert P. J. Day"  writes:

> underlying weirdness. but it still doesn't explain the different
> behaviour between:
>
>   $ git rm -n 'Makefile*'
>   $ git rm -n '*Makefile'
>
> in the linux kernel source tree, the first form matches only the
> single, top-level Makefile, while the second form gets *all* of them
> recursively, even though those globs should be equivalent in terms of
> matching all files named "Makefile".
>
>   am i misunderstanding something?

We are matching what Git cares/knows about aka "the paths in the
index" to pathspec patterns.

What are these paths in the index?  In Linux kernel sources, there
are quite a many but here are examples that are enough to explain
the above:

Makefile
COPYING
fs/Makefile
fs/ext4/Makefile

Which one of these four match patterh "Makefile*", which is "the
first letter is 'M', the second letter is 'a', ,, the eighth
letter is 'e', and anything else can follow to the end"?  Yes, only
the first one.

Which one of these four match pattern "*Makefile", then?  "Anything
can appear as leading substring, but then 'M', 'a', 'k', ..., and
finally 'e' must appear at the end"?

Note that these "start from the paths in the index that match the
pathspec patterns" have nothing to do with "recursive".  It happens
way before we decide to go recursive (or not).

We are not going down recursively from the paths in the index that
are matched by pathspec patterns with the above two "git rm"
requests (because there is no "-r" there), but even if we were,
because these three Makefile files are not directories, there is
nothing to remove recursively underneath them.

Re: [PATCH] Documentation/git-config.txt: reword missleading sentence

[Junio C Hamano]

Nathan PAYRE  writes:

> Thanks you for the this complete answer,
> we take note of your comments.
>
> We would like to reword something else in the same line
> and we don't know what is the best way to do that properly.
> Should we do a [PATCH v2] or revert the last commit and
> commit a new one?

I'd imagine that it is in the same spirit of the old one
(i.e. "let's make it less confusing"), so let's have a single patch
that has both changes which is [PATCH v2].

Thanks.

Re: "git rm" seems to do recursive removal even without "-r"

[Robert P. J. Day]

On Mon, 9 Oct 2017, Jeff King wrote:

> On Sun, Oct 08, 2017 at 04:42:27PM -0400, Theodore Ts'o wrote:
>
> > On Sun, Oct 08, 2017 at 03:44:14PM -0400, Robert P. J. Day wrote:
> > > >
> > > > find  | xargs git rm
> > > >
> > > > myself.
> > >
> > >   that's what i would have normally used until i learned about
> > > git's magical globbing capabilities, and i'm going to go back to
> > > using it, because git's magical globbing capabilities now scare
> > > me.
> >
> > Hmm, I wonder if the reason why git's magically globbing
> > capabilities even exist at all is for those poor benighted souls
> > on Windows, for which their shell (and associated utilities)
> > doesn't have advanced tools like "find" and "xargs"
>
> One benefit of globbing with Git is that it restricts the matches
> only to tracked files. That matters a lot when you have a very broad
> glob (e.g., like you might use with "git grep") because it avoids
> looking at cruft like generated files (or even inside .git).

  ah, now *that* is a compelling rationale that justifies the
underlying weirdness. but it still doesn't explain the different
behaviour between:

  $ git rm -n 'Makefile*'
  $ git rm -n '*Makefile'

in the linux kernel source tree, the first form matches only the
single, top-level Makefile, while the second form gets *all* of them
recursively, even though those globs should be equivalent in terms of
matching all files named "Makefile".

  am i misunderstanding something?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH] Documentation/git-config.txt: reword missleading sentence

[Nathan PAYRE]

Hi,

Thanks you for the this complete answer,
we take note of your comments.

We would like to reword something else in the same line
and we don't know what is the best way to do that properly.
Should we do a [PATCH v2] or revert the last commit and
commit a new one?

2017-10-05 12:13 GMT+02:00 Junio C Hamano :
> On Thu, Oct 5, 2017 at 5:17 PM, PAYRE NATHAN p1508475
>  wrote:
>> Change the word "bla" to "section.variable" to make it clear that it's a 
>> placeholder for a variable name.
>
> Please make sure that your log message shows without wrapping and is a
> confortable read on a 80-column terminal by wrapping long lines.
>
>>
>> See discussion at: 
>> https://public-inbox.org/git/20171002061303.horde.sl92grzcqtrv9oqkbfpe...@crashcourse.ca/
>
> I do not think it matters that much in this particular case, but please
> make it a habit to assume that time of people who run "git log" to
> find out why the change was done is 100x more valuable than the
> time you need to leave a good summary of the discussion in the
> log message. A URL at the end _in addition to_ a summary in your
> words is OK; just a URL without any effort to summarize why you
> did this change is not.
>
> I often find myself understanding the issues a lot better _only_
> after I try to summarize the argument for a change in the log
> message--it forces me to _think_. And (this probably does not
> apply to this patch, as it is not a code change) it often results
> in a better code. First I come up with a solution, write a quick
> patch, try to explain the approach in the log message and then
> realize there is a better solution _only_ after doing so. It is a
> good habit to get into to try explaining the thought process in
> the log message.
>
>> Noticed-by: rpj...@crashcourse.ca
>> ---
>
> Here, after "Reported-by:" before the three-dash line, we need
> your "Signed-off-by:" line. See Documentation/SubmittingPatches
> for details. The name and address should match what appears
> on the "From:" field from your e-mail.
>
> Ah, one more thing. Do you want to be known as somebody
> with ALL CAPS first and last name, with student number? ;-)
> If it is cumbersome to convince your MUA to use your real
> name spelled in normal way on the "From:" header, you could
> start the body of your message with
>
> From: Payre Nathan 
>
> followed by an empty line, followed by the body of the log
> message.
>
>
>>  Documentation/git-config.txt | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>> index 83f86b923..f9808d7ad 100644
>> --- a/Documentation/git-config.txt
>> +++ b/Documentation/git-config.txt
>> @@ -177,8 +177,8 @@ See also <>.
>> 'git-config' will expand leading '{tilde}' to the value of
>> '$HOME', and '{tilde}user' to the home directory for the
>> specified user.  This option has no effect when setting the
>> -   value (but you can use 'git config bla {tilde}/' from the
>> -   command line to let your shell do the expansion).
>> +   value (but you can use 'git config section.variable {tilde}/'
>> +   from the command line to let your shell do the expansion).
>>
>
> The new text proposed by the patch looks good.
>
> Note that I am not in front of a real computer, and I cannot
> verify if there is a whitespace breakage in the patch to make
> it unusable. I am guessing this one is OK (it seems there
> is only one SP followed by HT on the context lines).
>
> Thanks, and welcome to Git development community.

Re: new contributors presentation

[Nathan PAYRE]

Hi,

>First things first.  I suspect that you are trying to contribute to
>the Git project (GitHub is totally a different animal, even though
>they benefit greatly from our presence, and we theirs).

You are right excuse us for this.

>And if you are dipping your toes to the Git project's development
>community, then, "Welcome!" ;-).

Thanks we are enthusiastic to begin!


>I do not know about others, but I cannot quite tell what you are
>propsing from that three-line description to tell if it would be
>useful or not.  Let's wait to hear more from you guys.

Last year, another students group (Tom Russelo, Samuel Groot)
supervised by Matthieu Moy were working on git.
There goal was to create the --quote-mail= option for git
send-email, this option permit to quote an email in the same cover letter.
All the metadata ("Cc", "Subject", ...) will be automatically filled.

For now the code contains a syntax error then our obvious first
goal is to fix it.

Discussion of the previous team with more information:
https://public-inbox.org/git/1464031829-6107-1-git-send-email-tom.russe...@grenoble-inp.org/

2017-10-04 6:05 GMT+02:00 Junio C Hamano :
> Nathan PAYRE  writes:
>
>> Hi all,
>> me and my two other partner (Daniel and Timothee) have make the choice
>> to contribute to gitHub for a university project supervised by Mattieu
>> Moy.
>
> First things first.  I suspect that you are trying to contribute to
> the Git project (GitHub is totally a different animal, even though
> they benefit greatly from our presence, and we theirs).
>
> And if you are dipping your toes to the Git project's development
> community, then, "Welcome!" ;-).
>
>> The principal project is to improve the git-send-email function, for
>> example we will try to implement the possibility to answer to a email
>> by keeping the recipient list or quote properly the email body!
>> Do you think that it's will be usefull ?
>
> I do not know about others, but I cannot quite tell what you are
> propsing from that three-line description to tell if it would be
> useful or not.  Let's wait to hear more from you guys.