[PATCH v2 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned other patch
to add `--color` (in the non-traditional sense) via `--type=color`,
instead of `--color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 66 +++-
 builtin/config.c | 23 +
 t/t1300-repo-config.sh   | 18 ++
 3 files changed, 75 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..9956b03f7 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type] [--show-origin] [-z|--null] name [value 
[value_regex]]
+'git config' [] [--type] --add name value
+'git config' [] [--type] --replace-all name value [value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] --get name 
[value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] --get-all 
name [value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations 
are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type [type]::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
+  form.
++
+Valid `[type]`'s include:
++
+- 'bool': canonicalize  values as either "true" or "false".
+- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative 
date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-   'git config' will ensure that the output is a simple
-  

[PATCH v2 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Where--although the legacy and modern type specifier (`--int`, and
`--type`, respectively) do not conflict--we would store the value of
`--type=` in a `char *` and the `--int` as a bit in the `int` bitset.

In the above, when error-checking `if (types != && type_str)`, we do not
check additionally whether or not these types are the same, and simply
complain immediately. This change makes `--int` and `--type=int` a true
synonym of each other, and removes the need for additional complexity,
as above in the conditional.

Signed-off-by: Taylor Blau 
---
 builtin/config.c   | 39 +--
 t/t1300-repo-config.sh | 11 +++
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..fd7b36c41 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-color", , N_("find the color configured: slot 
[default]"), ACTION_GET_COLOR),
OPT_BIT(0, "get-colorbool", , N_("find the color setting: slot 
[stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
-   OPT_BIT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
-   OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
-   OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
-   OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
-   OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
+   OPT_SET_INT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
+   OPT_SET_INT(0, "int", , N_("value is decimal number"), TYPE_INT),
+   OPT_SET_INT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
+   OPT_SET_INT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_SET_INT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (show_keys)
strbuf_addch(buf, key_delim);
 
-   if (types == TYPE_INT)
+   if (type == TYPE_INT)
strbuf_addf(buf, "%"PRId64,
git_config_int64(key_, value_ ? value_ : 
""));
-   else if (types == TYPE_BOOL)
+   else if (type == TYPE_BOOL)
strbuf_addstr(buf, git_config_bool(key_, value_) ?
  "true" : "false");
-   else if (types == TYPE_BOOL_OR_INT) {
+   else if (type == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, _bool);
if (is_bool)
strbuf_addstr(buf, v ? "true" : "false");
else
strbuf_addf(buf, "%d", v);
-   } else if (types == TYPE_PATH) {
+   } else if (type == TYPE_PATH) {
const char *v;
if (git_config_pathname(, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
free((char *)v);
-   } else if (types == TYPE_EXPIRY_DATE) {

Re: [PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

On Thu, Mar 29, 2018 at 06:11:22PM -0400, Jeff King wrote:
> > +Valid `[type]`'s include:
> > ++
> > +- 'bool': canonicalize  values as either "true" or "false".
> > +- 'int': canonicalize  values as simple decimla numbers. An optional 
> > suffix of
> > +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, 
> > or
> > +  1073741824 prior to output.
> > +- 'bool-or-int': canonicalize according to either 'bool' or 'int', as 
> > described
> > +  above.
> > +- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
> > +  `~user` to the home directory for the specified user. This specifier has 
> > no
> > +  effect when setting the value (but you can use `git config 
> > section.variable
> > +  ~/` from the command line to let your shell do the expansion.)
> > +- 'expiry-date': canonicalize by converting from a fixed or relative 
> > ate-string
> > +  to a timestamp. This specifier has no effect when setting the value.
> > ++
>
> Yay. It's nice to have this in only one place now.

Thanks! Agreed :-).

> s/ate-string/d&/ :)

Ack.

My excuse for this is that I have started using iTerm2.app with Vim in
my terminal using the new graphics acceleration options, and have had
trouble getting Vim to render the underline for misspelled words
consistently.

> > +static int type_name_to_specifier(char *name)
> > +{
> > +   if (!(strcmp(name, "bool")))
> > +   return TYPE_BOOL;
>
> We'd usually drop the extra level of parentheses, and just write:
>
>   if (!strcmp(name, "bool"))

Sounds good; I have adjusted this in the appropriate location in v2.

> > @@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char 
> > *prefix)
> > usage_with_options(builtin_config_usage, 
> > builtin_config_options);
> > }
> >
> > +   if (type) {
> > +   if (types != 0) {
> > +   error("usage of --type is ambiguous");
> > +   usage_with_options(builtin_config_usage, 
> > builtin_config_options);
> > +   }
> > +   types = type_name_to_specifier(type);
> > +   }
>
> This error message left me scratching my head for a minute. Ambiguous
> how? I think this is covering the case of:
>
>   git config --int --type=bool
>
> So maybe "--type cannot be used with other type options" or something?
>
> Let's take a step back, though. As part of this, should we convert the
> parsing of type options to last-one-wins? The fact that they are all
> OPT_BIT() is quite silly, since you cannot have more than one bit set.
> So if you do:
>
>   git config --int --bool
>
> you get an error. Whereas normal behavior for most options would be for
> --bool to override --int. And that is what happens with:
>
>   git config --type=int --type=bool
>
> I don't think there are any backwards compatibility issues to deal with
> here; we'd be changing a case which is now always an error.

Agreed.

> And then after that, you truly can make (and document, if we want) that
> "--int" is a true synonym for "--type=int".

Great point; for me this is the primary motivating factor for making
this change. In addition to simplifying our work when we check:

  if (types != 0 && type_str) { ... }

it would be nice not to have to compare the two in order to ensure that
they are the same, continuing on if so, and causing an error if not.

> I think it would be pretty simple. One of:
>
>   - convert OPT_BIT("bool") into OPT_CALLBACK("bool") and just assign
> "bool" to the "type" string, which will then later get parsed into
> TYPE_BOOL.
>
> or
>
>   - convert OPT_BIT("bool") into OPT_SET_INT("bool") to set TYPE_BOOL
>
> directly. Convert OPT_STRING("type") into OPT_CALLBACK(), and have
> it assign the result of type_name_to_specifier() directly.
>
> I'd probably do the latter, but would be fine with either (and I'd make
> the OPT_SET_INT thing its own preparatory patch).

I adopted your advice and used the later, converting each `OPT_BIT` into
an `OPT_SET_INT`, and adding a callback for `--type`, which does _not_
complain if `type` is already non-zero.

> If you really want to go all-out, I think the ACTION flags could use the
> same cleanup. We treat them as bitflags, and then issue an error when
> you set more than one, which is just silly.

Agreed, and I think that this is a good candidate for a future patch.
Thoughts? :-).


Thanks,
Taylor

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> I think I'd agree on it being a release blocker. Given that your fix is
>> really a one-liner (3 of the lines are just changing the variable name,
>> which I agree with), I'd be fine with applying it on top rather than
>> reverting the original, even if it means delaying the release slightly.
>> It seems like about the same amount of risk to me.
>
> Yeah, I would say we should just apply the rfc/patch as-is directly
> on 'master'.

... which just has happened.

Thanks.

Re: [PATCH v6 0/6] ref-filter: remove die() calls from formatting logic

[Eric Sunshine]

On Thu, Mar 29, 2018 at 10:41 AM, Christian Couder
 wrote:
> On Thu, Mar 29, 2018 at 2:52 PM, Оля Тележная  
> wrote:
>> Move helper function from strbuf to ref-filter.
>> Get rid of some memory leaks.
>
> The above seems to be the changes since v5. Usually in a cover letter
> (patch 0/X) there is both information about the goal of the patch
> series and the changes since last version.
>
> Repeating the goal in each version is useful for reviewers who might
> not have time to look at the patch series before, or who might have
> forgotten about it.

Another important way to help both returning and new reviewers is to
provide a link to the previous iteration (or iterations), like
this[1].

Thanks.

[1]: 
https://public-inbox.org/git/0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000...@eu-west-1.amazonses.com/

Hello Dear.

[Mrs Roseline Jacque]

Hello Dear

I am Mrs Roseline Jacque 52 years old, from America (California), i am
suffering from long time illness cancer for years, I and my husband
was dealing on Gold Exportation  in Burkina Faso till his sudden death
the year 2008 then I took over the business till date. In fact, at
this moment I have a deposit sum of four million five hundred thousand
US dollars [$4,500,000.00] with one of the leading bank here in
Burkina Faso but unfortunately I cannot visit the bank since I’m
critically sick and powerless to do anything myself but my bank
account officer advised me to assign any of my trustworthy relative,
friends or partner with authorization letter to stand as the recipient
of my money but sorrowfully I don’t have any reliable relative and no
child.

Therefore, I want you to receive the money and take 50% to take care
of yourself and family while 50% should be use basically on
humanitarian purposes if you are interested please reply through my
private email address for more details, ( mrs.roseline...@hotmail.com

hope to hear from you.
Mrs Roseline

Re: [RFC PATCH v2] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

[Eddy Petrișor]

(+list)

2018-03-30 1:55 GMT+03:00  :
> From: Eddy Petrișor 
>
> There are projects such as llvm/clang which use several repositories, and they
> might be forked for providing support for various features such as adding 
> Redox
> awareness to the toolchain. This typically means the superproject will use
> another branch than master, occasionally even use an old commit from that
> non-master branch.
>
> Combined with the fact that when incorporating such a hierachy of repositories
> usually the user is interested in just the exact commit specified in the
> submodule info, it follows that a desireable usecase is to be also able to
> provide '--depth 1' or at least have a shallow clone to avoid waiting for ages
> for the clone operation to finish.
>
> In theory, this should be straightforward since the git protocol allows
> fetching an arbitary commit, but, in practice, some servers do not permit
> fetch-by-sha1.
>
> Git submodule seems to be very stubborn and cloning master, although the
> wrapper script and the gitmodules-helper could work together to clone directly
> the branch specified in the .gitmodules file, if specified.
>
> Signed-off-by: Eddy Petrișor 
> ---
>  git-submodule.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 24914963c..65e3af08b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -589,8 +589,10 @@ cmd_update()
> branch=$(git submodule--helper remote-branch 
> "$sm_path")
> if test -z "$nofetch"
> then
> +   # non-default branch refspec
> +   br_refspec=$(git submodule-helper 
> remote-branch $sm_path)
> # Fetch remote before determining tracking 
> $sha1
> -   fetch_in_submodule "$sm_path" $depth ||
> +   fetch_in_submodule "$sm_path" $depth 
> $br_refspec ||
> die "$(eval_gettext "Unable to fetch in 
> submodule path '\$sm_path'")"
> fi
> remote_name=$(sanitize_submodule_env; cd "$sm_path" 
> && get_default_remote)
> --
> 2.16.2
>

I am planning to write a test case in the next few days, between the
few drops of free time I have.
I am still trying to figure out my way through the test code.

-- 
Eddy Petrișor

Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers

[Junio C Hamano]

Jeff King  writes:

> On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:
>
>> The credential subsystem can trigger SIGPIPE when writing to an
>> external helper if that helper closes its stdin before reading the
>> whole input. Normally this is rare, since helpers would need to read
>> that input to make a decision about how to respond, but:
>> 
>> 1. It's reasonable to configure a helper which only handles "get"
>>while ignoring "store".  Such a handler might not read stdin
>>for "store", thereby rapidly closing stdin upon helper exit.
>> 
>> 2. A broken or misbehaving helper might exit immediately. That's an
>>error, but it's not reasonable for it to take down the parent Git
>>process with SIGPIPE.
>> 
>> Even with such a helper, seeing this problem should be rare. Getting
>> SIGPIPE requires the helper racily exiting before we've written the
>> fairly small credential output.
>> 
>> Signed-off-by: Erik E Brady 
>> ---
>>  credential.c | 3 +++
>>  1 file changed, 3 insertions(+)
>
> This version looks good to me. Thanks!

Yup, looks good.  Thanks, both.

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Jeff King  writes:

> I think I'd agree on it being a release blocker. Given that your fix is
> really a one-liner (3 of the lines are just changing the variable name,
> which I agree with), I'd be fine with applying it on top rather than
> reverting the original, even if it means delaying the release slightly.
> It seems like about the same amount of risk to me.

Yeah, I would say we should just apply the rfc/patch as-is directly
on 'master'.

Thanks.

Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers

[Jeff King]

On Thu, Mar 29, 2018 at 10:20:40PM +, Erik Brady -X (brady - ROBERT HALF 
INTERNATIONAL INC at Cisco) wrote:

> I appreciate your time.  Quick Q... is there a way to track the patch
> through to release?  If not I can just scan release notes/etc so no
> worries.

When the maintainer picks up the patch, he usually says something like
"thanks, will queue". Then you can track its progress either by:

 - fetching from https://github.com/gitster/git, which has all of the
   topic branches. Yours will be "eb/something", depending what Junio
   names it. And then you can periodically "git branch -a --contains
   eb/something" to see it progress through the various integration
   branches ('pu', 'next', 'master'). The branches are described in the
   "note from the maintainer" that's sent to the list periodically.
   E.g.:

 https://public-inbox.org/git/xmqqy3nt40pq@gitster.mtv.corp.google.com/

 - alternatively, you can read the "What's cooking in git.git" messages
   sent out by the maintainer a few times a week. E.g.:

 https://public-inbox.org/git/xmqqefkm6s06@gitster-ct.c.googlers.com/

   There your eb/something topic will be mentioned, along with the
   status (where it is in the graduation cycle, what's coming next, and
   if it's stalled, why).

We're in a release freeze right for v2.17 right now, so I'd expect your
patch to probably go to the 'maint' track and end up in v2.17.1.

Of course somebody else may see something wrong I didn't and ask you to
correct it, which would be in a reply to this thread. :)

-Peff

from Aziz Dake

[Aziz Dake]

Attn: Sir/Madam,

I am Mr. Aziz Dake,  a Minister confide on me to look for foreign
partner who will assist him to invest the sum of  Thirty  Million
Dollars  ($30,000,000) in your country.

He has investment interest in mining, exotic properties for commercial
resident, development properties, hotels and any other viable
investment opportunities in your country based on your recommendation
will be highly welcomed.

Hence your co -operation is highly needed to actualize this investment project.

Sincerely yours

Mr Aziz Dake.

Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Thanks Jeff.

I appreciate your time.  Quick Q... is there a way to track the patch through 
to release?  If not I can just scan release notes/etc so no worries.

Cheers,
Erik

On 3/29/18, 2:51 PM, "Jeff King"  wrote:

On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:

> The credential subsystem can trigger SIGPIPE when writing to an
> external helper if that helper closes its stdin before reading the
> whole input. Normally this is rare, since helpers would need to read
> that input to make a decision about how to respond, but:
> 
> 1. It's reasonable to configure a helper which only handles "get"
>while ignoring "store".  Such a handler might not read stdin
>for "store", thereby rapidly closing stdin upon helper exit.
> 
> 2. A broken or misbehaving helper might exit immediately. That's an
>error, but it's not reasonable for it to take down the parent Git
>process with SIGPIPE.
> 
> Even with such a helper, seeing this problem should be rare. Getting
> SIGPIPE requires the helper racily exiting before we've written the
> fairly small credential output.
> 
> Signed-off-by: Erik E Brady 
> ---
>  credential.c | 3 +++
>  1 file changed, 3 insertions(+)

This version looks good to me. Thanks!

-Peff

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff King]

On Thu, Mar 29, 2018 at 03:03:54PM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:
> 
> >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
> >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item
> >> to allow server operators to control when they start supporting it.
> >>
> >> That config item didn't go far enough, though: it controls whether the
> >> 'filter' capability is advertised, but if a (custom) client ignores
> >> the capability advertisement and passes a filter specification anyway,
> >> the server would handle that despite allowFilter being false.
> [...]
> > Great catch. If I understand correctly, this is an issue in the 2.17.0
> > release candidates. Is this worth delaying the release?
> 
> Yes, IMHO this is a release blocker.  (To put it another way, if this is
> going to delay the release, then we need to revert that patch.)

I think I'd agree on it being a release blocker. Given that your fix is
really a one-liner (3 of the lines are just changing the variable name,
which I agree with), I'd be fine with applying it on top rather than
reverting the original, even if it means delaying the release slightly.
It seems like about the same amount of risk to me.

(Sometimes it's actually _less_ risky to apply the one-liner fix,
because reverting a large feature can have unintended interactions with
other features that were built in the meantime.  Looking at the original
patch, though, I doubt that is the case here, hence my "about the same
amount of risk").

-Peff

Re: [PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Jeff King]

On Wed, Mar 28, 2018 at 04:47:19PM -0700, Taylor Blau wrote:

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
> 
> In another series, we propose to add extend this functionality with
> `--color` and `--default` to replace `--get-color`.
> 
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
> 
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.
> 
> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned other patch
> to add `--color` (in the non-traditional sense) via `--type=color`,
> instead of `--color`.

Makes sense. I agree with promoting --type as the correct way going
forward, since it will grow new types, whereas we can stop adding
"--foo" aliases for "--type=foo".

> +Valid `[type]`'s include:
> ++
> +- 'bool': canonicalize  values as either "true" or "false".
> +- 'int': canonicalize  values as simple decimla numbers. An optional suffix 
> of
> +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
> +  1073741824 prior to output.
> +- 'bool-or-int': canonicalize according to either 'bool' or 'int', as 
> described
> +  above.
> +- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
> +  `~user` to the home directory for the specified user. This specifier has no
> +  effect when setting the value (but you can use `git config section.variable
> +  ~/` from the command line to let your shell do the expansion.)
> +- 'expiry-date': canonicalize by converting from a fixed or relative 
> ate-string
> +  to a timestamp. This specifier has no effect when setting the value.
> ++

Yay. It's nice to have this in only one place now.

s/ate-string/d&/ :)

> +static int type_name_to_specifier(char *name)
> +{
> + if (!(strcmp(name, "bool")))
> + return TYPE_BOOL;

We'd usually drop the extra level of parentheses, and just write:

  if (!strcmp(name, "bool"))

> @@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
>   usage_with_options(builtin_config_usage, 
> builtin_config_options);
>   }
>  
> + if (type) {
> + if (types != 0) {
> + error("usage of --type is ambiguous");
> + usage_with_options(builtin_config_usage, 
> builtin_config_options);
> + }
> + types = type_name_to_specifier(type);
> + }

This error message left me scratching my head for a minute. Ambiguous
how? I think this is covering the case of:

  git config --int --type=bool

So maybe "--type cannot be used with other type options" or something?

Let's take a step back, though. As part of this, should we convert the
parsing of type options to last-one-wins? The fact that they are all
OPT_BIT() is quite silly, since you cannot have more than one bit set.
So if you do:

  git config --int --bool

you get an error. Whereas normal behavior for most options would be for
--bool to override --int. And that is what happens with:

  git config --type=int --type=bool

I don't think there are any backwards compatibility issues to deal with
here; we'd be changing a case which is now always an error.

And then after that, you truly can make (and document, if we want) that
"--int" is a true synonym for "--type=int".

I think it would be pretty simple. One of:

  - convert OPT_BIT("bool") into OPT_CALLBACK("bool") and just assign
"bool" to the "type" string, which will then later get parsed into
TYPE_BOOL.

or

  - convert OPT_BIT("bool") into OPT_SET_INT("bool") to set TYPE_BOOL
directly. Convert OPT_STRING("type") into OPT_CALLBACK(), and have
it assign the result of type_name_to_specifier() directly.

I'd probably do the latter, but would be fine with either (and I'd make
the OPT_SET_INT thing its own preparatory patch).

If you really want to go all-out, I think the ACTION flags could use the
same cleanup. We treat them as bitflags, and then issue an error when
you set more than one, which is just silly.

-Peff

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:

>> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
>> 2017-12-08), it was guarded by the uploadpack.allowFilter config item
>> to allow server operators to control when they start supporting it.
>>
>> That config item didn't go far enough, though: it controls whether the
>> 'filter' capability is advertised, but if a (custom) client ignores
>> the capability advertisement and passes a filter specification anyway,
>> the server would handle that despite allowFilter being false.
[...]
> Great catch. If I understand correctly, this is an issue in the 2.17.0
> release candidates. Is this worth delaying the release?

Yes, IMHO this is a release blocker.  (To put it another way, if this is
going to delay the release, then we need to revert that patch.)

Thanks,
Jonathan

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff King]

On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:

> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
> 2017-12-08), it was guarded by the uploadpack.allowFilter config item
> to allow server operators to control when they start supporting it.
> 
> That config item didn't go far enough, though: it controls whether the
> 'filter' capability is advertised, but if a (custom) client ignores
> the capability advertisement and passes a filter specification anyway,
> the server would handle that despite allowFilter being false.
> 
> This is particularly significant if a security bug is discovered in
> this new experimental partial clone code.  Installations without
> uploadpack.allowFilter ought not to be affected since they don't
> intend to support partial clone, but they would be swept up into being
> vulnerable.
> 
> Simplify and limit the attack surface by making uploadpack.allowFilter
> disable the feature, not just the advertisement of it.

Great catch. If I understand correctly, this is an issue in the 2.17.0
release candidates. Is this worth delaying the release?

It's a funny situation. The presence of a security bug is theoretical
here, so nobody _should_ need to care, and this is just hardening. But
it does make me a little nervous, given the experimental-ness of the new
feature.

-Peff

Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers

[Jeff King]

On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:

> The credential subsystem can trigger SIGPIPE when writing to an
> external helper if that helper closes its stdin before reading the
> whole input. Normally this is rare, since helpers would need to read
> that input to make a decision about how to respond, but:
> 
> 1. It's reasonable to configure a helper which only handles "get"
>while ignoring "store".  Such a handler might not read stdin
>for "store", thereby rapidly closing stdin upon helper exit.
> 
> 2. A broken or misbehaving helper might exit immediately. That's an
>error, but it's not reasonable for it to take down the parent Git
>process with SIGPIPE.
> 
> Even with such a helper, seeing this problem should be rare. Getting
> SIGPIPE requires the helper racily exiting before we've written the
> fairly small credential output.
> 
> Signed-off-by: Erik E Brady 
> ---
>  credential.c | 3 +++
>  1 file changed, 3 insertions(+)

This version looks good to me. Thanks!

-Peff

Re: [PATCH 9/9] git_config_set: reuse empty sections

[Jeff King]

On Thu, Mar 29, 2018 at 05:19:09PM +0200, Johannes Schindelin wrote:

> It can happen quite easily that the last setting in a config section is
> removed, and to avoid confusion when there are comments in the config
> about that section, we keep a lone section header, i.e. an empty
> section.
> 
> The code to add new entries in the config tries to be cute by reusing
> the parsing code that is used to retrieve config settings, but that
> poses the problem that the latter use case does *not* care about empty
> sections, therefore even the former user case won't see them.
> 
> Fix this by introducing a mode where the parser reports also empty
> sections (with a trailing '.' as tell-tale), and then using that when
> adding new config entries.

Heh, so it seems we are partway to the "event-stream" suggestion I made
earlier. I agree this is the right way to approach this problem.

I wondered if we allow keys to end in ".", but it seems that we don't.

> diff --git a/config.c b/config.c
> index eb1e0d335fc..b04c40f76bc 100644
> --- a/config.c
> +++ b/config.c
> @@ -653,13 +653,15 @@ static int get_base_var(struct strbuf *name)
>   }
>  }
>  
> -static int git_parse_source(config_fn_t fn, void *data)
> +static int git_parse_source(config_fn_t fn, void *data,
> + int include_section_headers)

We already have a "struct config_options", but we do a terrible job of
passing it around (since it only impacts the include stuff right now,
and that all gets handled at a very outer level).

Rather than plumb this one int through everywhere, should we add it to
that struct and plumb the struct through?

-Peff

Re: [PATCH 8/9] git_config_set: use do_config_from_file() directly

[Jeff King]

On Thu, Mar 29, 2018 at 05:19:04PM +0200, Johannes Schindelin wrote:

> Technically, it is the git_config_set_multivar_in_file_gently()
> function that we modify here (but the oneline would get too long if we
> were that precise).
> 
> This change prepares the git_config_set machinery to allow reusing empty
> sections, by using the file-local function do_config_from_file()
> directly (whose signature can then be changed without any effect outside
> of config.c).
> 
> An incidental benefit is that we avoid a level of indirection, and we
> also avoid calling flockfile()/funlockfile() when we already know that
> we are not operating on stdin/stdout here.

I'm not sure I understand that last paragraph. What does flockfile() have
to do with stdin/stdout?

The point of those calls is that we're locking the FILE handle, so that
it's safe for the lower-level config code to run getc_unlocked(), which
is faster.

So without those, we're calling getc_unlocked() without holding the
lock. I think it probably works in practice because we know that we're
single-threaded, but it seems a bit sketchy.

-Peff

Re: [PATCH 7/9] git config --unset: remove empty sections (in normal situations)

[Jeff King]

On Thu, Mar 29, 2018 at 05:19:00PM +0200, Johannes Schindelin wrote:

> Let's generalize this observation to this conservative strategy: if we
> are removing the last entry from a section, and there are no comments
> inside that section nor surrounding it, then remove the entire section.
> Otherwise behave as before: leave the now-empty section (including those
> comments, even the one about the now-deleted entry).

Yep, as I said earlier, this makes a ton of sense to me.

> +/*
> + * This function determines whether the offset is in a line that starts with 
> a
> + * comment character.
> + *
> + * Note: it does *not* report when a regular line (section header, config
> + * setting) *ends* in a comment.
> + */
> +static int is_in_comment_line(const char *contents, size_t offset)
> +{
> + int comment = 0;
> +
> + while (offset > 0)
> + switch (contents[--offset]) {
> + case ';':
> + case '#':
> + comment = 1;
> + break;
> + case '\n':
> + break;
> + case ' ':
> + case '\t':
> + continue;
> + default:
> + comment = 0;
> + }
> +
> + return comment;
> +}

This doesn't pay any attention to quoting, so I wondered if it would get
fooled by a line like:

  key = "this content has a # comment in it"

or even:

  [section "this section has a # comment in it"]

but those don't count because the line doesn't _start_ with the comment
character. Could we design one that does? This isn't valid:

  [section]
  key = multiline \
# with comment

But I think this is:

  [section]
  key = "multiline \
# with comment"

So let's see if we can fool it:

-- >8 --

cat >file <<-\EOF
[one]
key = "multiline \
  # with comment"
[two]
key = true
EOF

# should produce "multiline   # with comment"
./git config --file=file one.key

# this should ideally remove the section
./git config --file=file --unset two.key
cat file

-- 8< --

That seems to work as expected. I'm not 100% sure why, though, since I
thought we'd hit the "seen_section && !is_in_comment_line" bit of the
look_before loop. Running it through gdb, I'm not convinced that
is_in_comment_line is working correctly, though. Shouldn't it stop when
it sees the newline, and return "comment"? There's a "break" there, but
it doesn't break out of the loop due to the switch statement.

So we'll _always_ walk back to the beginning of file. So I suspect your
test passes because it does:

  # this is the start of the file
  [section]
  key = true

but:

  [anotherSection]
  key = true
  # a comment not at the start
  [section]
  key = true

does the wrong thing, and removes [section]. If we fix that bug like
this:

diff --git a/config.c b/config.c
index b04c40f76b..3b2c7e9387 100644
--- a/config.c
+++ b/config.c
@@ -2461,7 +2461,7 @@ static int is_in_comment_line(const char *contents, 
size_t offset)
comment = 1;
break;
case '\n':
-   break;
+   return comment;
case ' ':
case '\t':
continue;

then it keeps "[section]" correctly. But now if we go back to our funny
multiline example, it does the wrong thing (it keeps [two], even though
that's not _really_ a comment).

To be honest, I could live with that as an open bug. It's a pretty
ridiculous situation, and the worst case is that we err on the side of
caution and don't remove the section. And I think it would be hard to
fix. We could look for the continuation backslash when we find the
newline, but that gets fooled by:

  # a comment \
  # with a pointless backslash

You can't just notice the quote and say "oh, I'm in a quoted section"
because that gets fooled by:

  # a pointless "quote

To know whether that quote is valid or not, you have to find the other
quote. But doing that backwards is hard (if not impossible).

> +static void maybe_remove_section(const char *contents, size_t size,
> +  const char *section_name,
> +  size_t section_name_len,
> +  size_t *begin, int *i_ptr, int *new_line)
> +{
> + size_t begin2, end2;
> + int seen_section = 0, dummy, i = *i_ptr;
> +
> + /*
> +  * First, make sure that this is the last key in the section, and that
> +  * there are no comments that are possibly about the current section.
> +  */
> +next_entry:
> + for (end2 = store.offset[i]; end2 < size; end2++) {
> + switch (contents[end2]) {
> + case ' ':
> + case '\t':
> + case '\n':
> + continue;
> + case '\r':
> + if (++end2 < size && contents[end2] == '\n')
> + continue;
> + break;
> + case '[':
> +   

Fwd: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

[Eddy Petrișor]

mar., 27 mar. 2018, 02:07 Stefan Beller  a scris:
>
> [snipped the cc list as well]
>
> On Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor 
> wrote:
>
> > Signed-off-by: Eddy Petrișor 
> > ---
>
> Did this go anywhere?
> (I just came back from a longer vacation, sorry for the delay on my site)


Not really. I am still unsure how is best to proceed. Details below.

> > There are projects such as llvm/clang which use several repositories, and
> they
> > might be forked for providing support for various features such as adding
> Redox
> > awareness to the toolchain. This typically means the superproject will use
> > another branch than master, occasionally even use an old commit from that
> > non-master branch.
>
> > Combined with the fact that when incorporating such a hierachy of
> repositories
> > usually the user is interested in just the exact commit specified in the
> > submodule info, it follows that a desireable usecase is to be also able to
> > provide '--depth 1' to avoid waiting for ages for the clone operation to
> > finish.
>
> Very sensible.


The only change is that I realized that hard coding the depth is not
necessary because the client can fetch more and more from the branch
until the commit hash is found or the entire history was fetched and
it wasn't found.

This is more robust but has a variable performance penalty and is
probably slower than single branch fetching from the start.

> > Git submodule seems to be very stubborn and cloning master, although the
> > wrapper script and the gitmodules-helper could work together to clone
> directly
> > the branch specified in the .gitmodules file, if specified.
>
> Also very sensible.
>
> So far so good, could you move these paragraphs before the triple dashed
> line
> and sign off so we record it as the commit message?


Sure, as long as the implementation and design makes sense.

> > Another wrinkle is that when the commit is not the tip of the branch, the
> depth
> > parameter should somehow be stored in the .gitmodules info, but any
> change in
> > the submodule will break the supermodule submodule depth info sooner or
> later,
> > which is definitly frigile.
>
> ... which is why I would not include that.
>
> git-fetch knows about --shallow-since or even better
> shallow-exclude which could be set to the (depth+1)-th commit
> (the boundary commit) recorded in the shallow information.


I am unsure what that means. Without yet looking in the docs, would
this --shallow-since be better than the try-until-found algorithm
explained above?

> > I tried digging into this section of the code and debugging with bashdb
> to see
> > where --depth might fit, but I got stuck on the shell-to-helper
> interaction and
> > the details of the submodule implementation, so I want to lay out this
> first
> > patch as starting point for the discussion in the hope somebody else
> picks it
> > up or can provide some inputs. I have the feeling there are multiple code
> paths
> > that are being ran, depending on the moment (initial clone, submodule
> > recursive, post-clone update etc.) and I have a gut feeling there
> shouldn't be
> > any code duplication just because the operation is different.
>
> > This first patch is only trying to use a non-master branch, I have some
> changes
> > for the --depth part, but I stopped working on it due to the "default
> depth"
> > issue above.
>
> > Does any of this sound reasonable?
> > Is this patch idea usable or did I managed to touch the part of the code
> that
> > should not be touched?
>
> This sounds reasonable. Thanks for writing the patch!


OK. Now I need to make it good, which is the hard part :)

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 2491496..370f19e 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -589,8 +589,11 @@ cmd_update()
> >  branch=$(git submodule--helper remote-branch
> "$sm_path")
> >  if test -z "$nofetch"
> >  then
> > +   # non-default branch
> > +   rbranch=$(git config -f .gitmodules
> submodule.$sm_path.branch)
> > +
> br_refspec=${rbanch:+"refs/heads/$rbranch:refs/heads/$rbranch"}
>
> Wouldn't we want to fetch into a remote tracking branch instead?
> Instead of computing all this by yourself, these two lines could be
>
>  br_refspec=$(git submodule--helper remote-branch $sm_path)
>
> I would think.


I wasn't aware of this, will implement I  the next version and see what happens.
>
>
> >  # Fetch remote before determining
> tracking $sha1
> > -   fetch_in_submodule "$sm_path" $depth ||
> > +   fetch_in_submodule "$sm_path" $depth
> $br_refspec ||
> >  die "$(eval_gettext "Unable to fetch in
> submodule path '\$sm_path'")"
> > 

Re: [PATCH 4/9] t1300: remove unreasonable expectation from TODO

[Junio C Hamano]

Jeff King  writes:

> An obvious question is whether we should preserve the original
> unrealistic parts by splitting it: the realistic parts into one
> expect_failure (that we'd switch to expect_success by the end of this
> series), and then an unrealistic one to serve as a documentation of the
> ideal, with a comment explaining why it's unrealistic.
>
> I doubt the "unrealistic" half would be serving much purpose though, so
> I'm OK to see it get eliminated here.

Likewise.  The series looks good so far.

Re: [PATCH v4] Allow use of TLS 1.3

[Junio C Hamano]

Loganaden Velvindron  writes:

> diff --git a/http.c b/http.c
> index a5bd5d62c..f84b18551 100644
> --- a/http.c
> +++ b/http.c
> @@ -62,6 +62,9 @@ static struct {
>   { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
>   { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>  #endif
> +#if LIBCURL_VERSION_NUM >= 0x073400
> + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 }

Looks OK to me, except one minor nit.

I'll add a trailing comma for this entry while queuing, so that a
future patch to add tlsv1.4 or whatever won't have to worry about
it.

Thanks.

> +#endif
>  };
>  #if LIBCURL_VERSION_NUM >= 0x070903
>  static const char *ssl_key;

Re: [PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Junio C Hamano]

Taylor Blau  writes:

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
>
> In another series, we propose to add extend this functionality with
> `--color` and `--default` to replace `--get-color`.
>
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
>
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.

OK.

> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned other patch
> to add `--color` (in the non-traditional sense) via `--type=color`,
> instead of `--color`.

OK, it's not just "we prefer" but we add this new "--type" thing, so
that we do not have to worry about having to make --color and other
"types" into double-dash options.  Makes good sense, I would guess.

Re: [PATCH v4 2/5] stash: convert apply to builtin

[Junio C Hamano]

Joel Teichroeb  writes:

> +static int get_stash_info(struct stash_info *info, int argc, const char 
> **argv)
> +{

So, this roughly corresponds to parse_flags_and_rev function, it seems.

> + struct strbuf w_commit_rev = STRBUF_INIT;
> + struct strbuf b_commit_rev = STRBUF_INIT;
> + struct strbuf w_tree_rev = STRBUF_INIT;
> + struct strbuf b_tree_rev = STRBUF_INIT;
> + struct strbuf i_tree_rev = STRBUF_INIT;
> + struct strbuf u_tree_rev = STRBUF_INIT;
> + struct strbuf symbolic = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> + const char *revision;
> + const char *commit = NULL;
> + char *end_of_rev;
> + info->is_stash_ref = 0;
> +
> + if (argc > 1) {
> + int i;
> + struct strbuf refs_msg = STRBUF_INIT;
> + for (i = 0; i < argc; ++i)
> + strbuf_addf(_msg, " '%s'", argv[i]);
> +
> + fprintf_ln(stderr, _("Too many revisions specified:%s"), 
> refs_msg.buf);
> + strbuf_release(_msg);
> +
> + return -1;
> + }
> +
> + if (argc == 1)
> + commit = argv[0];
> +
> + strbuf_init(>revision, 0);
> + if (commit == NULL) {
> + if (have_stash()) {
> + free_stash_info(info);
> + return error(_("No stash entries found."));
> + }
> +
> + strbuf_addf(>revision, "%s@{0}", ref_stash);
> + } else if (strspn(commit, "0123456789") == strlen(commit)) {
> + strbuf_addf(>revision, "%s@{%s}", ref_stash, commit);
> + } else {
> + strbuf_addstr(>revision, commit);
> + }
> +
> + revision = info->revision.buf;
> + strbuf_addstr(_commit_rev, revision);
> + ret = !get_oid(w_commit_rev.buf, >w_commit);
> + strbuf_release(_commit_rev);

Use of strbuf w_commit_rev looks completely pointless here.  Am I
mistaken to say that the above three lines are equivalent to:

ret = !get_oid(revision, >w_commit);

> +
> + if (!ret) {
> + error(_("%s is not a valid reference"), revision);
> + free_stash_info(info);
> + return -1;
> + }
> +
> + strbuf_addf(_commit_rev, "%s^1", revision);
> + strbuf_addf(_tree_rev, "%s:", revision);
> + strbuf_addf(_tree_rev, "%s^1:", revision);
> + strbuf_addf(_tree_rev, "%s^2:", revision);
> +
> + ret = !get_oid(b_commit_rev.buf, >b_commit) &&
> + !get_oid(w_tree_rev.buf, >w_tree) &&
> + !get_oid(b_tree_rev.buf, >b_tree) &&
> + !get_oid(i_tree_rev.buf, >i_tree);
> +
> + strbuf_release(_commit_rev);
> + strbuf_release(_tree_rev);
> + strbuf_release(_tree_rev);
> + strbuf_release(_tree_rev);

For the same reason, these strbuf's look pretty much pointless.  I
wonder if a private helper

static int grab_oid(struct oid *oid, const char *fmt, const char *rev)
{
struct strbuf buf = STRBUF_INIT;
int ret;

strbuf_addf(, fmt, rev);
ret = get_oid(buf, oid);
strbuf_release();
return ret;
}

would help here?  Then you wouldn't be writing something like the
above, and instead you'd grab four object names like so:

if (grab_oid(>b_commit, "%s^1", revision) ||
grab_oid(>w_tree, "%s:", revision) ||
grab_oid(>b_tree, "%s&1:", revision) ||
grab_oid(>i_tree, "%s&2:", revision)) {
... we found an error ...
return -1;
}

which would be a lot easier to follow, no?

> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +{
> + int result = 0;
> + pid_t pid = getpid();
> + const char *index_file;
> +
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + git_config(git_default_config, NULL);
> +
> + argc = parse_options(argc, argv, prefix, options, 
> git_stash_helper_usage,
> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> +
> + index_file = get_index_file();
> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%"PRIuMAX, index_file, 
> (uintmax_t)pid);

Wouldn't it make more sense to get rid of PATH_MAX and hold it in a
strbuf instead?  I.e.

static struct strbuf stash_index_path = STRBUF_INIT;
...
strbuf_addf(_index_path, "%s.stash.%" PRIuMAX, index_file, 
(uintmax_t)pid);

> + cd "$START_DIR"
> + git stash--helper apply "$@"
> + res=$?
> + cd_to_toplevel
> + return $res
>  }

Re: [PATCH 5/9] t1300: `--unset-all` can leave an empty section behind (bug)

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:53PM +0200, Johannes Schindelin wrote:

> We already have a test demonstrating that removing the last entry from a
> config section fails to remove the section header of the now-empty
> section.
> 
> The same can happen, of course, if we remove the last entries in one fell
> swoop. This is *also* a bug, and should be fixed at the same time.

Yep, makes sense, and the diff is obviously correct.

-Peff

Re: [PATCH 4/9] t1300: remove unreasonable expectation from TODO

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:50PM +0200, Johannes Schindelin wrote:

> In https://public-inbox.org/git/7vvc8alzat@alter.siamese.dyndns.org/
> a reasonable patch was made quite a bit less so by changing a test case
> demonstrating a bug to a test case that demonstrates that we ask for too
> much: the test case 'unsetting the last key in a section removes header'
> now expects a future bug fix to be able to determine whether a free-form
> comment above a section header refers to said section or not.
> 
> Rather than shooting for the stars (and not even getting off the
> ground), let's start shooting for something obtainable and be reasonably
> confident that we *can* get it.

As I said before, I'm fine with turning this test into something more
realistic.

An obvious question is whether we should preserve the original
unrealistic parts by splitting it: the realistic parts into one
expect_failure (that we'd switch to expect_success by the end of this
series), and then an unrealistic one to serve as a documentation of the
ideal, with a comment explaining why it's unrealistic.

I doubt the "unrealistic" half would be serving much purpose though, so
I'm OK to see it get eliminated here.

-Peff

Re: [PATCH 3/9] t1300: avoid relying on a bug

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:45PM +0200, Johannes Schindelin wrote:

> The test case 'unset with cont. lines' relied on a bug that is about to
> be fixed: it tests *explicitly* that removing the last entry from a
> config section leaves an *empty* section behind.
> 
> Let's fix this test case not to rely on that behavior, simply by
> preventing the section from becoming empty.

Seems like a good solution. I don't think we care in particular about
testing a multi-line value at the end of the file.

-Peff

Re: [PATCH 2/9] t1300: rename it to reflect that `repo-config` was deprecated

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:40PM +0200, Johannes Schindelin wrote:

> Signed-off-by: Johannes Schindelin 
> ---
>  t/{t1300-repo-config.sh => t1300-config.sh} | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename t/{t1300-repo-config.sh => t1300-config.sh} (100%)

This has only been bugging me for oh, about 10 years. Thanks.

-Peff

Re: [PATCH 1/9] git_config_set: fix off-by-two

[Jeff King]

On Thu, Mar 29, 2018 at 11:15:33AM -0700, Stefan Beller wrote:

> > When calling `git config --unset abc.a` on this file, it leaves this
> > (invalid) config behind:
> >
> > [
> > [xyz]
> > key = value
> >
> > The reason is that we try to search for the beginning of the line (or
> > for the end of the preceding section header on the same line) that
> > defines abc.a, but as an optimization, we subtract 2 from the offset
> > pointing just after the definition before we call
> > find_beginning_of_line(). That function, however, *also* performs that
> > optimization and promptly fails to find the section header correctly.
> 
> This commit message would be more convincing if we had it in test form.

I agree a test might be nice. But I don't find the commit message
unconvincing at all. It explains pretty clearly why the bug occurs, and
you can verify it by looking at find_beginning_of_line.

> [abc]a
> 
> is not written by Git, but would be written from an outside tool or person
> and we barely cope with it?

Yes, I don't think git would ever write onto the same line. But clearly
we should handle anything that's syntactically valid.

-Peff

Re: [PATCH v4 1/5] stash: improve option parsing test coverage

[Junio C Hamano]

Joel Teichroeb  writes:

> In preparation for converting the stash command incrementally to
> a builtin command, this patch improves test coverage of the option
> parsing. Both for having too many paramerters, or too few.
>
> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aefde7b17..8a666c60c 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -84,6 +84,17 @@ test_expect_success 'apply stashed changes (including 
> index)' '
>   test 1 = $(git show HEAD:file)
>  '
>  
> +test_expect_success 'giving too many ref agruments does nothing' '
> +
> + for type in apply drop pop show "branch stash-branch"
> + do
> + test-chmtime =123456789 file &&
> + test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
> + test_i18ngrep "Too many" err &&
> + test 123456789 = $(test-chmtime -v +0 file | sed 
> 's/[^0-9].*$//') || return 1
> + done
> +'

This is done with "file" whose contents are all different in HEAD,
the index and the working tree.  If the command tries to "push" by
mistake, it will touch the timestamp of "file" and fail the test.
That is a reasonable thing to check.

What do stash@{0} and stash{1} record at this point?  Would they
touch that "file" if the command tries to "apply" or "pop" by
mistake?  Perhaps it deserves a bit of in-code comment here.

As "drop" or "show" would not touch the working tree anyway, the
test for timestamp seems pointless, even though grepping for "Too
many" would be a reasonable test.

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:30PM +0200, Johannes Schindelin wrote:

> Little did I know that this would turn not only into a full patch to fix this
> issue, but into a full-blown series of nine patches.

It's amazing how often that happens. :)

> The first patch is somewhat of a "while at it" bug fix that I first thought
> would be a lot more critical than it actually is: It really only affects 
> config
> files that start with a section followed immediately (i.e. without a newline)
> by a one-letter boolean setting (i.e. without a `= ` part). So while it
> is a real bug fix, I doubt anybody ever got bitten by it.

That makes me wonder if somebody could craft a malicious config to do
something bad. But I don't think so. Config is trusted already, and it
looks like this bug is both hard to trigger and doesn't result in any
kind of memory funniness, just a bogus output.

> Now, to the really important part: why does this patch series not conflict 
> with
> my very early statements that we cannot simply remove empty sections because 
> we
> may end up with stale comments?
> 
> Well, the patch in question takes pains to determine *iff* there are any
> comments surrounding, or included in, the section. If any are found: previous
> behavior. Under the assumption that the user edited the file, we keep it as
> intact as possible (see below for some argument against this). If no comments
> are found, and let's face it, this is probably *the* common case, as few 
> people
> edit their config files by hand these days (neither should they because it is
> too easy to end up with an unparseable one), the now-empty section *is*
> removed.

I'm not against people editing their config files by hand. But I think
what you propose here makes a lot of sense, because it works as long as
you don't intermingle hand- and auto-editing in the same section (and it
even works if you do intermingle, as long as you don't use comments,
which are probably even more rare).

So it seems like quite a sensible compromise, and I think should make
most people happy.

-Peff

Re: [PATCH v12 00/10] convert: add support for different encodings

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> From: Lars Schneider 
>
> Patches 1-6,9 are preparation and helper functions. Patch 4 is new.
> Patch 7,8,10 are the actual change.
>
> This series depends on Torsten's 8462ff43e4 (convert_to_git():
> safe_crlf/checksafe becomes int conv_flags, 2018-01-13) which is
> already in master.

I didn't see any further review comments on this round, and as far
as I can tell from my reading, these patches looked more-or-less
ready.  

Except for 04/10 that had a few messages around "who should be
responsible for handling the 'NULL is for the default UTF-8'?", that
is.

So, what's the doneness of this thing?

Re: [PATCH v3 0/3] add -p: select individual hunk lines

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Since v2 I've updated the patches to use '-' instead of '^' to invert
> the selection to match the rest of add -i and clean -i.
>
> These patches build on top of the recount fixes in [1]. The commit
> message for the first patch describes the motivation:
>
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."

I haven't seen any review comments on this round, and as I am not a
heavy user of "add -i" interface (even though I admit that I
originally wrote it), I haven't had a chance to exercise the code
myself in the two weeks since the patches have been queued in my
tree.

I am inclihned to merge them to 'next' soonish, but please stop me
if anybody (including the original author) has further comments.

Thanks.

Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

[Junio C Hamano]

Johannes Schindelin  writes:

>> But I wonder if this is even easier to follow.  It makes it even
>> more clear that patchsame commits that are not empty are discarded
>> unconditionally.
>> 
>>  while ((commit = get_revision())) {
>>  int is_empty  = is_original_commit_empty(commit);
>>  if (!is_empty && (commit->object.flags & PATCHSAME))
>>  continue;
>>  strbuf_reset();
>>  if (!keep_empty && is_empty)
>>  strbuf_addf(, "%c ", comment_line_char);
>>  strbuf_addf(, "%s %s ", insn,
>>  oid_to_hex(>object.oid));
>>  pretty_print_commit(, commit, );
>>  strbuf_addch(, '\n');
>>  fputs(buf.buf, out);
>>  }
>> 
>> Or did I screw up the rewrite?
>
> This looks correct. And the postimage is easier to follow than the one of
> my suggested change.

OK, let's squash this in and rebuild both pw/rebase-keep-empty-fixes
and also pw/rebase-signoff that builds on this topic, so that they
can be advanced to 'next'.

Re: [PATCH 1/9] git_config_set: fix off-by-two

[Stefan Beller]

On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
 wrote:
> Currently, we are slightly overzealous When removing an entry from a
> config file of this form:
>
> [abc]a
> [xyz]
> key = value
>
> When calling `git config --unset abc.a` on this file, it leaves this
> (invalid) config behind:
>
> [
> [xyz]
> key = value
>
> The reason is that we try to search for the beginning of the line (or
> for the end of the preceding section header on the same line) that
> defines abc.a, but as an optimization, we subtract 2 from the offset
> pointing just after the definition before we call
> find_beginning_of_line(). That function, however, *also* performs that
> optimization and promptly fails to find the section header correctly.

This commit message would be more convincing if we had it in test form.

[abc]a

is not written by Git, but would be written from an outside tool or person
and we barely cope with it?

Thanks,
Stefan

>
> Signed-off-by: Johannes Schindelin 
> ---
>  config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index b0c20e6cb8a..5cc049aaef0 100644
> --- a/config.c
> +++ b/config.c
> @@ -2632,7 +2632,7 @@ int git_config_set_multivar_in_file_gently(const char 
> *config_filename,
> } else
> copy_end = find_beginning_of_line(
> contents, contents_sz,
> -   store.offset[i]-2, _line);
> +   store.offset[i], _line);
>
> if (copy_end > 0 && contents[copy_end-1] != '\n')
> new_line = 1;
> --
> 2.16.2.windows.1.26.g2cc3565eb4b
>
>

Re: [PATCH 2/4] add chdir-notify API

[Duy Nguyen]

On Thu, Mar 29, 2018 at 7:48 PM, Jeff King  wrote:
> On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote:
>
>> On Wed, Mar 28, 2018 at 7:40 PM, Jeff King  wrote:
>> > +static void reparent_cb(const char *old_cwd,
>> > +   const char *new_cwd,
>> > +   void *data)
>> > +{
>> > +   char **path = data;
>>
>> Maybe check data == NULL and return early. This is just for
>> convenience, e.g. instead of doing
>>
>> path = getenv("foo");
>> if (path)
>> chdir_notify_reparent();
>>
>> I could do
>>
>> path = getenv("foo");
>> chdir_notify_reparent();
>
> You'd need to xstrdup(path) there anyway. I guess you could use
> xstrdup_or_null(), but it seems somewhat unlikely (unless your work on
> top really does add such a callsite?).

It actually exists in 'next', repository.c where we have this line

repo->alternate_db = xstrdup_or_null(o->alternate_db);

> I guess it's not that much code to be careful, though.

>> > +void chdir_notify_reparent(char **path)
>> > +{
>> > +   if (!is_absolute_path(*path))
>>
>> I think this check is a bit early. There could be a big gap between
>> chdir_notify_reparent() and reparent_cb(). During that time, *path may
>> be updated and become a relative path. We can check for absolute path
>> inside reparent_cb() instead.
>
> My thinking was that we'd be effectively zero-cost for an absolute path,
> though really adding an element to the notification list is probably not
> a big deal.

I think we could leave such optimization to the caller. I'm ok with
keeping this "if" too, but you may need to clarify it in the big
comment block in chdir-notify.h because this behavior to me is
surprising.

> I also assumed that whoever changed it to a relative path would then
> install the notification handler. One thing that my series kind of
> glosses over is whether somebody might call any of these functions
> twice, which would involve setting up the handler twice. So e.g. if you
> did:
>
>   set_git_dir("foo");
>   set_git_dir("bar");
>
> we'd have two handlers, which would do the wrong thing when the second
> one triggered. I hoped we could eventually add a BUG() if that happens,
> but maybe we should simply do a:
>
>   static int registered_chdir;
>
>   if (!registered_chdir) {
> chdir_notify_reparent();
> registered_chdir = 1;
>   }
>
> at each call-site. Another alternative would be to make it a noop to
> feed the same path twice. That requires a linear walk through the list,
> but it's a pretty small list.

Well, given the number of call sites is rather small at the moment, I
think chdir-notify can just stay dumb and let the caller deal with
duplicate registration. One thing I like about your linked list design
though, is it makes it quite easy to remove (or even update) a
callback. You can simply return the (opaque) pointer to the entire
chdir_notify_entry as a handle and we can free/unlink the entry based
on it. It's kinda hard to me to do it with array-based design.
-- 
Duy

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

On Thu, Mar 29, 2018 at 7:50 PM, Jeff King  wrote:
> On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote:
>
>> On Wed, Mar 28, 2018 at 7:42 PM, Jeff King  wrote:
>> > When we change to the top of the working tree, we manually
>> > re-adjust $GIT_DIR and call set_git_dir() again, in order to
>> > update any relative git-dir we'd compute earlier.
>>
>> Another way to approach this problem is not delaying chdir() at all.
>> We have to delay calling setup_work_tree() and not do it in
>> setup_git_directory() because it can die() when chdir() fails. But in
>> many cases, the command does not actually need the worktree and does
>> not deserve to die. But what if we make setup_work_tree be gentle?
>>
>> If it successfully chdir() at the end of setup_git_directory() (and
>> perhaps before the first set_git_dir call), great! The problem we're
>> dealing here vanishes. If it fails, don't die, just set a flag. Later
>> on when a command requests a worktree, we can check this flag and now
>> can die().
>>
>> It's less code this way, but it uses up more of your (or my) time
>> because even though the first set_git_dir() call actually happens at 8
>> places. Is it worth trying ?
>
> I do kind of like that. I'm reasonably happy with the chdir_notify()
> interface, but it would be nicer still if we could get rid of it in the
> first place. It's true that we _could_ chdir from other places, but

There's another place we do, that I should mention and keep
forgetting. Our run-command.c code allows to switch cwd, and if
$GIT_DIR and stuff is relative then we should reparent them too just
like we do with setup_work_tree(). Your chdir-notify makes it super
easy to support that, we just need to move the prep_childenv() down
below chdir(). But since nobody has complaint, I suppose that feature
is not really popular (or at least not used to launch another git
process anyway)

> realistically speaking, we do our one big chdir as part of the setup
> process.
-- 
Duy

[PATCH] credential: ignore SIGPIPE when writing to credential helpers

[Erik E Brady]

The credential subsystem can trigger SIGPIPE when writing to an
external helper if that helper closes its stdin before reading the
whole input. Normally this is rare, since helpers would need to read
that input to make a decision about how to respond, but:

1. It's reasonable to configure a helper which only handles "get"
   while ignoring "store".  Such a handler might not read stdin
   for "store", thereby rapidly closing stdin upon helper exit.

2. A broken or misbehaving helper might exit immediately. That's an
   error, but it's not reasonable for it to take down the parent Git
   process with SIGPIPE.

Even with such a helper, seeing this problem should be rare. Getting
SIGPIPE requires the helper racily exiting before we've written the
fairly small credential output.

Signed-off-by: Erik E Brady 
---
 credential.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/credential.c b/credential.c
index 9747f47b1..62be651b0 100644
--- a/credential.c
+++ b/credential.c
@@ -5,6 +5,7 @@
 #include "run-command.h"
 #include "url.h"
 #include "prompt.h"
+#include "sigchain.h"
 
 void credential_init(struct credential *c)
 {
@@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,
return -1;
 
fp = xfdopen(helper.in, "w");
+   sigchain_push(SIGPIPE, SIG_IGN);
credential_write(c, fp);
fclose(fp);
+   sigchain_pop(SIGPIPE);
 
if (want_output) {
int r;
-- 
2.16.3.dirty

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Stefan Beller]

On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
 wrote:

> So what is the argument against this extra care to detect comments? Well, if
> you have something like this:
>
> [section]
> ; Here we comment about the variable called snarf
> snarf = froop
>
> and we run `git config --unset section.snarf`, we end up with this config:
>
> [section]
> ; Here we comment about the variable called snarf
>
> which obviously does not make sense. However, that is already established
> behavior for quite a few years, and I do not even try to think of a way how
> this could be solved.

By commenting out the key/value pair instead of deleting it.
It's called --unset, not --delete ;)

Now onto reviewing the patches.

Stefan

Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash

[Jeff King]

On Thu, Mar 29, 2018 at 05:25:04PM +, Erik Brady -X (brady - ROBERT HALF 
INTERNATIONAL INC at Cisco) wrote:

> OK, will retry on the comment.  I guess I misunderstood the guidelines
> a bit on the signoff as well (ie: non-optional), apologies.  Will
> resubmit via 'git send-email' after adjusting the comment and
> recommitting with the -s option.  First time for everything I suppose,
> doh.

The signoff (for our project) is all about the "yes, this contribution
can go under the gpl". So that part is very non-optional. :)

> As to your comment suggestion, appreciated, looks good.  I might
> reword the #1 item you have just a bit (I removed the host specific
> stuff since I think the race can occur regardless of host specific or
> not... but I might be missing something there?).  Anyhow, how about
> something like this:

Yeah, it can definitely occur regardless. It was just the plausible
reason to have a handler which does not bother to look at the incoming
data (since otherwise you are spewing your password to any host you
connect to; maybe that's OK if you configure it only inside a specific
repo and only fetch from one server).

Your update looks fine to me.

Thanks.

-Peff

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

On Thu, Mar 29, 2018 at 07:23:17PM +0200, Duy Nguyen wrote:

> On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen  wrote:
> > ...
> >
> > It's less code this way, but it uses up more of your (or my) time
> > because even though the first set_git_dir() call actually happens at 8
> > places. Is it worth trying ?
> 
> Never mind. I took a stab anyway. The setup code should have much less
> side effect before we can do stuff like this.

Oh, I should have read this before responding. Oh well. :)

-Peff

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote:

> On Wed, Mar 28, 2018 at 7:42 PM, Jeff King  wrote:
> > When we change to the top of the working tree, we manually
> > re-adjust $GIT_DIR and call set_git_dir() again, in order to
> > update any relative git-dir we'd compute earlier.
> 
> Another way to approach this problem is not delaying chdir() at all.
> We have to delay calling setup_work_tree() and not do it in
> setup_git_directory() because it can die() when chdir() fails. But in
> many cases, the command does not actually need the worktree and does
> not deserve to die. But what if we make setup_work_tree be gentle?
> 
> If it successfully chdir() at the end of setup_git_directory() (and
> perhaps before the first set_git_dir call), great! The problem we're
> dealing here vanishes. If it fails, don't die, just set a flag. Later
> on when a command requests a worktree, we can check this flag and now
> can die().
> 
> It's less code this way, but it uses up more of your (or my) time
> because even though the first set_git_dir() call actually happens at 8
> places. Is it worth trying ?

I do kind of like that. I'm reasonably happy with the chdir_notify()
interface, but it would be nicer still if we could get rid of it in the
first place. It's true that we _could_ chdir from other places, but
realistically speaking, we do our one big chdir as part of the setup
process.

-Peff

Re: [PATCH 2/4] add chdir-notify API

[Jeff King]

On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote:

> On Wed, Mar 28, 2018 at 7:40 PM, Jeff King  wrote:
> > +static void reparent_cb(const char *old_cwd,
> > +   const char *new_cwd,
> > +   void *data)
> > +{
> > +   char **path = data;
> 
> Maybe check data == NULL and return early. This is just for
> convenience, e.g. instead of doing
> 
> path = getenv("foo");
> if (path)
> chdir_notify_reparent();
> 
> I could do
> 
> path = getenv("foo");
> chdir_notify_reparent();

You'd need to xstrdup(path) there anyway. I guess you could use
xstrdup_or_null(), but it seems somewhat unlikely (unless your work on
top really does add such a callsite?).

I guess it's not that much code to be careful, though.

> > +   char *tmp = *path;
> > +   *path = reparent_relative_path(old_cwd, new_cwd, tmp);
> > +   free(tmp);
> > +}
> > +
> > +void chdir_notify_reparent(char **path)
> > +{
> > +   if (!is_absolute_path(*path))
> 
> I think this check is a bit early. There could be a big gap between
> chdir_notify_reparent() and reparent_cb(). During that time, *path may
> be updated and become a relative path. We can check for absolute path
> inside reparent_cb() instead.

My thinking was that we'd be effectively zero-cost for an absolute path,
though really adding an element to the notification list is probably not
a big deal.

I also assumed that whoever changed it to a relative path would then
install the notification handler. One thing that my series kind of
glosses over is whether somebody might call any of these functions
twice, which would involve setting up the handler twice. So e.g. if you
did:

  set_git_dir("foo");
  set_git_dir("bar");

we'd have two handlers, which would do the wrong thing when the second
one triggered. I hoped we could eventually add a BUG() if that happens,
but maybe we should simply do a:

  static int registered_chdir;

  if (!registered_chdir) {
chdir_notify_reparent();
registered_chdir = 1;
  }

at each call-site. Another alternative would be to make it a noop to
feed the same path twice. That requires a linear walk through the list,
but it's a pretty small list.

> > +   chdir_notify_register(reparent_cb, path);
> > +}
> 
> Overall, I like this API very much! I will add another one similar to
> chdir_notify_reparent() but it takes an env name instead and the
> callback will setenv() appropriately. The result looks super good.

Ooh, that's clever.

I had also thought of extending it to other notifications besides
chdir(), like one string depends on another. But then we'd basically
re-inventing a cascading data-flow system, which is probably getting a
bit magical. :)

-Peff

Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Thanks Jeff.

OK, will retry on the comment.  I guess I misunderstood the guidelines a bit on 
the signoff as well (ie: non-optional), apologies.  Will resubmit via 'git 
send-email' after adjusting the comment and recommitting with the -s option.  
First time for everything I suppose, doh.

As to your comment suggestion, appreciated, looks good.  I might reword the #1 
item you have just a bit (I removed the host specific stuff since I think the 
race can occur regardless of host specific or not... but I might be missing 
something there?).  Anyhow, how about something like this:

--
Subject: credential: ignore SIGPIPE when writing to credential helpers

The credential subsystem can trigger SIGPIPE when writing to an
external helper if that helper closes its stdin before reading the
whole input. Normally this is rare, since helpers would need to read
that input to make a decision about how to respond, but:

1. It's reasonable to configure a helper which only handles "get"
while ignoring "store".  Such a handler might not read stdin 
for "store", thereby rapidly closing stdin upon helper exit.

2. A broken or misbehaving helper might exit immediately. That's an
 error, but it's not reasonable for it to take down the parent Git
 process with SIGPIPE.

Even with such a helper, seeing this problem should be rare. Getting
SIGPIPE requires the helper racily exiting before we've written the
fairly small credential output.
--

As to testing, yes, that was my thought as well.  Anyhow, I will try the above 
unless you see a problem or would like any further change (?).

Thanks,
Erik

On 3/29/18, 4:19 AM, "Jeff King"  wrote:

On Wed, Mar 28, 2018 at 03:20:51PM -0700, Erik E Brady wrote:

> Subject: Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, 
crash

Thanks for sending this. The patch itself looks good to me, but I have a
few nits with your commit message.

We usually write commit messages in the imperative, with the subject
summarizing the change. So:

  Subject: credential: ignore SIGPIPE when writing to credential helpers

or similar.

> credential.c, run_credential_helper(): now ignores SIGPIPE
> when writing to credential helper.  Avoids problem with race
> where cred helper exits very quickly and, after, git tries
> to write to it, generating SIGPIPE and crashing git.  To
> reproduce this the cred helper must not read from STDIN.

We can stop being terse outside of the subject line. :) I'd probably
write something like:

  The credential subsystem can trigger SIGPIPE when writing to an
  external helper if that helper closes its stdin before reading the
  whole input. Normally this is rare, since helpers would need to read
  that input to make a decision about how to respond, but:

1. It's reasonable to configure a helper which blindly a "get"
   answer, and trigger it only for certain hosts via config like:

 [credential "https://example.com;]
 helper = "!get-example-password"

2. A broken or misbehaving helper might exit immediately. That's an
   error, but it's not reasonable for it to take down the parent Git
   process with SIGPIPE.

  Even with such a helper, seeing this problem should be rare. Getting
  SIGPIPE requires the helper racily exiting before we've written the
  fairly small credential output.

Feel free to steal or adapt any of that as you see fit.

> This was seen with a custom credential helper, written in
> Go, which ignored the store command (STDIN not read) and
> then did a quick exit.  Even with this fast helper the race
> was pretty rare, ie: was only seen on some of our older VM's
> running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On
> these VM's it occurred only once every few hundred git cmds.
> ---

Missing signoff. See Documentation/SubmittingPatches, especially the
'sign-off' and 'dco' sections.

>  credential.c | 3 +++
>  1 file changed, 3 insertions(+)

No test, but I think that's fine here. Any such test would be inherently
racy.

> @@ -227,8 +228,10 @@ static int run_credential_helper(struct credential 
*c,
>   return -1;
>  
>   fp = xfdopen(helper.in, "w");
> + sigchain_push(SIGPIPE, SIG_IGN);
>   credential_write(c, fp);
>   fclose(fp);
> + sigchain_pop(SIGPIPE);

This looks like the right place to put the push/pop (as you noted
before, we may not write until fclose flushes, so it definitely has to
go after that).

Thanks again for digging this up. It's pretty subtle. :)

-Peff

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen  wrote:
> ...
>
> It's less code this way, but it uses up more of your (or my) time
> because even though the first set_git_dir() call actually happens at 8
> places. Is it worth trying ?

Never mind. I took a stab anyway. The setup code should have much less
side effect before we can do stuff like this.
-- 
Duy

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

On Wed, Mar 28, 2018 at 7:42 PM, Jeff King  wrote:
> When we change to the top of the working tree, we manually
> re-adjust $GIT_DIR and call set_git_dir() again, in order to
> update any relative git-dir we'd compute earlier.

Another way to approach this problem is not delaying chdir() at all.
We have to delay calling setup_work_tree() and not do it in
setup_git_directory() because it can die() when chdir() fails. But in
many cases, the command does not actually need the worktree and does
not deserve to die. But what if we make setup_work_tree be gentle?

If it successfully chdir() at the end of setup_git_directory() (and
perhaps before the first set_git_dir call), great! The problem we're
dealing here vanishes. If it fails, don't die, just set a flag. Later
on when a command requests a worktree, we can check this flag and now
can die().

It's less code this way, but it uses up more of your (or my) time
because even though the first set_git_dir() call actually happens at 8
places. Is it worth trying ?
-- 
Duy

[PATCH 7/9] git config --unset: remove empty sections (in normal situations)

[Johannes Schindelin]

The original reasoning for not removing section headers upon removal of
the last entry went like this: the user could have added comments about
the section, or about the entries therein, and if there were other
comments there, we would not know whether we should remove them.

In particular, a concocted example was presented that looked like this
(and was added to t1300):

# some generic comment on the configuration file itself
# a comment specific to this "section" section.
[section]
# some intervening lines
# that should also be dropped

key = value
# please be careful when you update the above variable

The ideal thing for `git config --unset section.key` in this case would
be to leave only the first line behind, because all the other comments
are now obsolete.

However, this is unfeasible, short of adding a complete Natural Language
Processing module to Git, which seems not only a lot of work, but a
totally unreasonable feature (for little benefit to most users).

Now, the real kicker about this problem is: most users do not edit their
config files at all! In their use case, the config looks like this
instead:

[section]
key = value

... and it is totally obvious what should happen if the entry is
removed.

Let's generalize this observation to this conservative strategy: if we
are removing the last entry from a section, and there are no comments
inside that section nor surrounding it, then remove the entire section.
Otherwise behave as before: leave the now-empty section (including those
comments, even the one about the now-deleted entry).

We have to be careful, though, to handle also the case where there are
*multiple* entries that are removed: any subset of them might be the last
entries of their respective sections.

Signed-off-by: Johannes Schindelin 
---
 config.c  | 181 +-
 t/t1300-config.sh |   4 +-
 2 files changed, 182 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index d35dffa50de..503aef4b318 100644
--- a/config.c
+++ b/config.c
@@ -2429,6 +2429,177 @@ static ssize_t find_beginning_of_line(const char 
*contents, size_t size,
return offset;
 }
 
+/*
+ * This function determines whether the offset is in a line that starts with a
+ * comment character.
+ *
+ * Note: it does *not* report when a regular line (section header, config
+ * setting) *ends* in a comment.
+ */
+static int is_in_comment_line(const char *contents, size_t offset)
+{
+   int comment = 0;
+
+   while (offset > 0)
+   switch (contents[--offset]) {
+   case ';':
+   case '#':
+   comment = 1;
+   break;
+   case '\n':
+   break;
+   case ' ':
+   case '\t':
+   continue;
+   default:
+   comment = 0;
+   }
+
+   return comment;
+}
+
+/*
+ * If we are about to unset the last key(s) in a section, and if there are
+ * no comments surrounding (or included in) the section, we will want to
+ * extend begin/end to remove the entire section.
+ *
+ * Note: the parameter `i_ptr` points to the index into the store.offset
+ * array, reflecting the end offset of the respective entry to be deleted.
+ * This index may be incremented if a section has more than one entry (which
+ * all are to be removed).
+ */
+static void maybe_remove_section(const char *contents, size_t size,
+const char *section_name,
+size_t section_name_len,
+size_t *begin, int *i_ptr, int *new_line)
+{
+   size_t begin2, end2;
+   int seen_section = 0, dummy, i = *i_ptr;
+
+   /*
+* First, make sure that this is the last key in the section, and that
+* there are no comments that are possibly about the current section.
+*/
+next_entry:
+   for (end2 = store.offset[i]; end2 < size; end2++) {
+   switch (contents[end2]) {
+   case ' ':
+   case '\t':
+   case '\n':
+   continue;
+   case '\r':
+   if (++end2 < size && contents[end2] == '\n')
+   continue;
+   break;
+   case '[':
+   /* If the section name is repeated, continue */
+   if (end2 + 1 + section_name_len < size &&
+   contents[end2 + section_name_len] == ']' &&
+   !memcmp(contents + end2 + 1, section_name,
+   section_name_len)) {
+   end2 += section_name_len;
+   continue;
+   }
+   goto look_before;
+

[PATCH 3/9] t1300: avoid relying on a bug

[Johannes Schindelin]

The test case 'unset with cont. lines' relied on a bug that is about to
be fixed: it tests *explicitly* that removing the last entry from a
config section leaves an *empty* section behind.

Let's fix this test case not to rely on that behavior, simply by
preventing the section from becoming empty.

Signed-off-by: Johannes Schindelin 
---
 t/t1300-config.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 4f8e6f5fde3..1ece7bad05f 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -108,6 +108,7 @@ bar = foo
 [beta]
 baz = multiple \
 lines
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines' '
@@ -118,6 +119,7 @@ cat > expect <<\EOF
 [alpha]
 bar = foo
 [beta]
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines is correct' 'test_cmp expect 
.git/config'
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 5/9] t1300: `--unset-all` can leave an empty section behind (bug)

[Johannes Schindelin]

We already have a test demonstrating that removing the last entry from a
config section fails to remove the section header of the now-empty
section.

The same can happen, of course, if we remove the last entries in one fell
swoop. This is *also* a bug, and should be fixed at the same time.

Signed-off-by: Johannes Schindelin 
---
 t/t1300-config.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 3ad3df0c83e..ff79a213567 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1452,6 +1452,17 @@ test_expect_failure '--unset last key removes section 
(except if commented)' '
test_cmp expect .git/config
 '
 
+test_expect_failure '--unset-all removes section if empty & uncommented' '
+   cat >.git/config <<-\EOF &&
+   [section]
+   key = value1
+   key = value2
+   EOF
+
+   git config --unset-all section.key &&
+   test_line_count = 0 .git/config
+'
+
 test_expect_failure 'adding a key into an empty section reuses header' '
cat >.git/config <<-\EOF &&
[section]
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 6/9] git_config_set: simplify the way the section name is remembered

[Johannes Schindelin]

This not only reduces the number of lines, but also opens the door for
reusing the section name later (which the upcoming patch to remove
now-empty sections will do).

Signed-off-by: Johannes Schindelin 
---
 config.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/config.c b/config.c
index 5cc049aaef0..d35dffa50de 100644
--- a/config.c
+++ b/config.c
@@ -2486,12 +2486,14 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
struct lock_file lock = LOCK_INIT;
char *filename_buf = NULL;
char *contents = NULL;
+   char *section_name = NULL;
size_t contents_sz;
 
/* parse-key returns negative; flip the sign to feed exit(3) */
-   ret = 0 - git_config_parse_key(key, , );
+   ret = 0 - git_config_parse_key(key, _name, );
if (ret)
goto out_free;
+   store.key = section_name;
 
store.multi_replace = multi_replace;
 
@@ -2505,7 +2507,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
fd = hold_lock_file_for_update(, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
-   free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
}
@@ -2515,8 +2516,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
in_fd = open(config_filename, O_RDONLY);
if ( in_fd < 0 ) {
-   free(store.key);
-
if ( ENOENT != errno ) {
error_errno("opening %s", config_filename);
ret = CONFIG_INVALID_FILE; /* same as "invalid config 
file" */
@@ -2571,7 +2570,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
if (git_config_from_file(store_aux, config_filename, NULL)) {
error("invalid config file %s", config_filename);
-   free(store.key);
if (store.value_regex != NULL &&
store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
@@ -2581,7 +2579,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
goto out_free;
}
 
-   free(store.key);
if (store.value_regex != NULL &&
store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
@@ -2682,6 +2679,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 
 out_free:
rollback_lock_file();
+   free(section_name);
free(filename_buf);
if (contents)
munmap(contents, contents_sz);
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 4/9] t1300: remove unreasonable expectation from TODO

[Johannes Schindelin]

In https://public-inbox.org/git/7vvc8alzat@alter.siamese.dyndns.org/
a reasonable patch was made quite a bit less so by changing a test case
demonstrating a bug to a test case that demonstrates that we ask for too
much: the test case 'unsetting the last key in a section removes header'
now expects a future bug fix to be able to determine whether a free-form
comment above a section header refers to said section or not.

Rather than shooting for the stars (and not even getting off the
ground), let's start shooting for something obtainable and be reasonably
confident that we *can* get it.

Signed-off-by: Johannes Schindelin 
---
 t/t1300-config.sh | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 1ece7bad05f..3ad3df0c83e 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1413,7 +1413,7 @@ test_expect_success 'urlmatch with wildcard' '
 '
 
 # good section hygiene
-test_expect_failure 'unsetting the last key in a section removes header' '
+test_expect_failure '--unset last key removes section (except if commented)' '
cat >.git/config <<-\EOF &&
# some generic comment on the configuration file itself
# a comment specific to this "section" section.
@@ -1427,6 +1427,25 @@ test_expect_failure 'unsetting the last key in a section 
removes header' '
 
cat >expect <<-\EOF &&
# some generic comment on the configuration file itself
+   # a comment specific to this "section" section.
+   [section]
+   # some intervening lines
+   # that should also be dropped
+
+   # please be careful when you update the above variable
+   EOF
+
+   git config --unset section.key &&
+   test_cmp expect .git/config &&
+
+   cat >.git/config <<-\EOF &&
+   [section]
+   key = value
+   [next-section]
+   EOF
+
+   cat >expect <<-\EOF &&
+   [next-section]
EOF
 
git config --unset section.key &&
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 8/9] git_config_set: use do_config_from_file() directly

[Johannes Schindelin]

Technically, it is the git_config_set_multivar_in_file_gently()
function that we modify here (but the oneline would get too long if we
were that precise).

This change prepares the git_config_set machinery to allow reusing empty
sections, by using the file-local function do_config_from_file()
directly (whose signature can then be changed without any effect outside
of config.c).

An incidental benefit is that we avoid a level of indirection, and we
also avoid calling flockfile()/funlockfile() when we already know that
we are not operating on stdin/stdout here.

Signed-off-by: Johannes Schindelin 
---
 config.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 503aef4b318..eb1e0d335fc 100644
--- a/config.c
+++ b/config.c
@@ -2706,6 +2706,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
struct stat st;
size_t copy_begin, copy_end;
int i, new_line = 0;
+   FILE *f;
 
if (value_regex == NULL)
store.value_regex = NULL;
@@ -2739,7 +2740,10 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 * As a side effect, we make sure to transform only a valid
 * existing config file.
 */
-   if (git_config_from_file(store_aux, config_filename, NULL)) {
+   f = fopen_or_warn(config_filename, "r");
+   if (!f || do_config_from_file(store_aux, CONFIG_ORIGIN_FILE,
+ config_filename, config_filename,
+ f, NULL)) {
error("invalid config file %s", config_filename);
if (store.value_regex != NULL &&
store.value_regex != CONFIG_REGEX_NONE) {
@@ -2747,8 +2751,11 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
free(store.value_regex);
}
ret = CONFIG_INVALID_FILE;
+   if (f)
+   fclose(f);
goto out_free;
-   }
+   } else
+   fclose(f);
 
if (store.value_regex != NULL &&
store.value_regex != CONFIG_REGEX_NONE) {
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 9/9] git_config_set: reuse empty sections

[Johannes Schindelin]

It can happen quite easily that the last setting in a config section is
removed, and to avoid confusion when there are comments in the config
about that section, we keep a lone section header, i.e. an empty
section.

The code to add new entries in the config tries to be cute by reusing
the parsing code that is used to retrieve config settings, but that
poses the problem that the latter use case does *not* care about empty
sections, therefore even the former user case won't see them.

Fix this by introducing a mode where the parser reports also empty
sections (with a trailing '.' as tell-tale), and then using that when
adding new config entries.

Signed-off-by: Johannes Schindelin 
---
 config.c  | 32 +++-
 t/t1300-config.sh |  2 +-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/config.c b/config.c
index eb1e0d335fc..b04c40f76bc 100644
--- a/config.c
+++ b/config.c
@@ -653,13 +653,15 @@ static int get_base_var(struct strbuf *name)
}
 }
 
-static int git_parse_source(config_fn_t fn, void *data)
+static int git_parse_source(config_fn_t fn, void *data,
+   int include_section_headers)
 {
int comment = 0;
int baselen = 0;
struct strbuf *var = >var;
int error_return = 0;
char *error_msg = NULL;
+   int saw_section_header = 0;
 
/* U+FEFF Byte Order Mark in UTF8 */
const char *bomptr = utf8_bom;
@@ -685,6 +687,16 @@ static int git_parse_source(config_fn_t fn, void *data)
if (cf->eof)
return 0;
comment = 0;
+   if (saw_section_header) {
+   if (include_section_headers) {
+   cf->linenr--;
+   error_return = fn(var->buf, NULL, data);
+   if (error_return < 0)
+   break;
+   cf->linenr++;
+   }
+   saw_section_header = 0;
+   }
continue;
}
if (comment || isspace(c))
@@ -700,6 +712,7 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
strbuf_addch(var, '.');
baselen = var->len;
+   saw_section_header = 1;
continue;
}
if (!isalpha(c))
@@ -1398,7 +1411,8 @@ int git_default_config(const char *var, const char 
*value, void *dummy)
  * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
-static int do_config_from(struct config_source *top, config_fn_t fn, void 
*data)
+static int do_config_from(struct config_source *top, config_fn_t fn, void 
*data,
+ int include_section_headers)
 {
int ret;
 
@@ -1410,7 +1424,7 @@ static int do_config_from(struct config_source *top, 
config_fn_t fn, void *data)
strbuf_init(>var, 1024);
cf = top;
 
-   ret = git_parse_source(fn, data);
+   ret = git_parse_source(fn, data, include_section_headers);
 
/* pop config-file parsing state stack */
strbuf_release(>value);
@@ -1423,7 +1437,7 @@ static int do_config_from(struct config_source *top, 
config_fn_t fn, void *data)
 static int do_config_from_file(config_fn_t fn,
const enum config_origin_type origin_type,
const char *name, const char *path, FILE *f,
-   void *data)
+   void *data, int include_section_headers)
 {
struct config_source top;
 
@@ -1436,12 +1450,12 @@ static int do_config_from_file(config_fn_t fn,
top.do_ungetc = config_file_ungetc;
top.do_ftell = config_file_ftell;
 
-   return do_config_from(, fn, data);
+   return do_config_from(, fn, data, include_section_headers);
 }
 
 static int git_config_from_stdin(config_fn_t fn, void *data)
 {
-   return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin, 
data);
+   return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin, 
data, 0);
 }
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
@@ -1452,7 +1466,7 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
f = fopen_or_warn(filename, "r");
if (f) {
flockfile(f);
-   ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, 
filename, f, data);
+   ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, 
filename, f, data, 0);
funlockfile(f);
fclose(f);
}
@@ -1475,7 +1489,7 @@ int git_config_from_mem(config_fn_t fn, const enum 
config_origin_type origin_typ
top.do_ungetc 

[PATCH 2/9] t1300: rename it to reflect that `repo-config` was deprecated

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 t/{t1300-repo-config.sh => t1300-config.sh} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename t/{t1300-repo-config.sh => t1300-config.sh} (100%)

diff --git a/t/t1300-repo-config.sh b/t/t1300-config.sh
similarity index 100%
rename from t/t1300-repo-config.sh
rename to t/t1300-config.sh
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 1/9] git_config_set: fix off-by-two

[Johannes Schindelin]

Currently, we are slightly overzealous When removing an entry from a
config file of this form:

[abc]a
[xyz]
key = value

When calling `git config --unset abc.a` on this file, it leaves this
(invalid) config behind:

[
[xyz]
key = value

The reason is that we try to search for the beginning of the line (or
for the end of the preceding section header on the same line) that
defines abc.a, but as an optimization, we subtract 2 from the offset
pointing just after the definition before we call
find_beginning_of_line(). That function, however, *also* performs that
optimization and promptly fails to find the section header correctly.

Signed-off-by: Johannes Schindelin 
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index b0c20e6cb8a..5cc049aaef0 100644
--- a/config.c
+++ b/config.c
@@ -2632,7 +2632,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
} else
copy_end = find_beginning_of_line(
contents, contents_sz,
-   store.offset[i]-2, _line);
+   store.offset[i], _line);
 
if (copy_end > 0 && contents[copy_end-1] != '\n')
new_line = 1;
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

This patch series started out as a single patch trying to figure out what it
takes to fix that annoying bug that has been reported several times over the
years, where `git config --unset` would leave empty sections behind, and `git
config --add` would not reuse them.

Little did I know that this would turn not only into a full patch to fix this
issue, but into a full-blown series of nine patches.

The first patch is somewhat of a "while at it" bug fix that I first thought
would be a lot more critical than it actually is: It really only affects config
files that start with a section followed immediately (i.e. without a newline)
by a one-letter boolean setting (i.e. without a `= ` part). So while it
is a real bug fix, I doubt anybody ever got bitten by it.

Nonetheless, I would be confortable with this patch going into v2.17.0, even at
this late stage. The final verdict is Junio's, of course.

The next swath of patches add some tests, and adjust one test about which I
already complained at length yesterday, so I will spare you the same ordeal
today. These fixes are pretty straight-forward, and I always try to keep my
added tests as concise as possible, so please tell me if you find a way to make
them smaller (without giving up readability and debuggability).

Finally, the interesting part, where I do two things, essentially (with
preparatory steps for each thing):

1. I add the ability for `git config --unset/--unset-all` to detect that it
   can remove a section that has just become empty (see below for some more
   discussion of what I consider "become empty"), and

2. I add the ability for `git config [--add] key value` to re-use empty
   sections.

Note that the --unset/--unset-all part is the hairy one, and I would love it if
people could concentrate on wrapping their heads around that function, and
obviously tell me how I can change it to make it more readable (or even point
out incorrect behavior).

Now, to the really important part: why does this patch series not conflict with
my very early statements that we cannot simply remove empty sections because we
may end up with stale comments?

Well, the patch in question takes pains to determine *iff* there are any
comments surrounding, or included in, the section. If any are found: previous
behavior. Under the assumption that the user edited the file, we keep it as
intact as possible (see below for some argument against this). If no comments
are found, and let's face it, this is probably *the* common case, as few people
edit their config files by hand these days (neither should they because it is
too easy to end up with an unparseable one), the now-empty section *is*
removed.

So what is the argument against this extra care to detect comments? Well, if
you have something like this:

[section]
; Here we comment about the variable called snarf
snarf = froop

and we run `git config --unset section.snarf`, we end up with this config:

[section]
; Here we comment about the variable called snarf

which obviously does not make sense. However, that is already established
behavior for quite a few years, and I do not even try to think of a way how
this could be solved.


Johannes Schindelin (9):
  git_config_set: fix off-by-two
  t1300: rename it to reflect that `repo-config` was deprecated
  t1300: avoid relying on a bug
  t1300: remove unreasonable expectation from TODO
  t1300: `--unset-all` can leave an empty section behind (bug)
  git_config_set: simplify the way the section name is remembered
  git config --unset: remove empty sections (in normal situations)
  git_config_set: use do_config_from_file() directly
  git_config_set: reuse empty sections

 config.c| 234 +---
 t/{t1300-repo-config.sh => t1300-config.sh} |  36 -
 2 files changed, 250 insertions(+), 20 deletions(-)
 rename t/{t1300-repo-config.sh => t1300-config.sh} (98%)


base-commit: 03df4959472e7d4b5117bb72ac86e1e2bcf21723
Published-As: https://github.com/dscho/git/releases/tag/empty-config-section-v1
Fetch-It-Via: git fetch https://github.com/dscho/git empty-config-section-v1
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH v3 0/3] Enable more compiler warnings for devs

[Nguyễn Thái Ngọc Duy]

v3 fixes the fallthru warnings in connect.c, and with json-writer
series rerolled, 'pu' should build cleanly now.
-Wno-maybe-uninitialized is removed, thanks to Ramsay.

v3 also adds an experimental patch that adds EAGER_DEVELOPER=1. These
developers will have all warnings enabled (nothing is suppressed) but
they are not fatal. They could go through them and perhaps clean up
the code base a bit more.

Interdiff

diff --git a/Makefile b/Makefile
index e6680a8977..e4f04ce1cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,7 +434,9 @@ all::
 #
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif
diff --git a/config.mak.dev b/config.mak.dev
index d8beaf9347..13883410b3 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -18,13 +20,23 @@ endif
 
 ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter 
clang4,$(COMPILER_FEATURES))),)
 CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
+# These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
 CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
-ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
-CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
 endif
 endif
diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
if (unexpected)
die(_("The remote end hung up upon initial contact"));


Nguyễn Thái Ngọc Duy (3):
  connect.c: mark die_initial_contact() NORETURN
  Makefile: detect compiler and enable more warnings in DEVELOPER=1
  Makefile: add EAGER_DEVELOPER mode

 Makefile| 20 +--
 config.mak.dev  | 42 +++
 connect.c   |  2 +-
 detect-compiler | 53 +
 4 files changed, 106 insertions(+), 11 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

-- 
2.17.0.rc1.439.gca064e2955

[PATCH v3 2/3] Makefile: detect compiler and enable more warnings in DEVELOPER=1

[Nguyễn Thái Ngọc Duy]

The set of extra warnings we enable when DEVELOPER has to be
conservative because we can't assume any compiler version the
developer may use. Detect the compiler version so we know when it's
safe to enable -Wextra and maybe more.

These warning settings are mostly from my custom config.mak a long
time ago when I tried to enable as many warnings as possible that can
still build without showing warnings. Some of those warnings are
probably worth fixing instead of just suppressing in future.

Helped-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile| 15 +-
 config.mak.dev  | 38 +++
 detect-compiler | 53 +
 3 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

diff --git a/Makefile b/Makefile
index 7f40f76739..e6680a8977 100644
--- a/Makefile
+++ b/Makefile
@@ -431,6 +431,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and faimily are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -439,15 +443,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-   -Wdeclaration-after-statement \
-   -Wno-format-zero-length \
-   -Wold-style-definition \
-   -Woverflow \
-   -Wpointer-arith \
-   -Wstrict-prototypes \
-   -Wunused \
-   -Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -1047,7 +1042,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,
diff --git a/config.mak.dev b/config.mak.dev
new file mode 100644
index 00..716a14ecc7
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,38 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter 
clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
+CFLAGS += -Wmissing-prototypes
+# These are disabled because we have these all over the place.
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 00..70b754481c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+   $CC -v 2>&1 | grep ' version '
+}
+
+get_family() {
+   get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+   get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+   family=$1
+   version=$(get_version | cut -f 1 -d .)
+
+   # Print a feature flag not only for the current version, but also
+   # for any prior versions we encompass. This avoids needing to do
+   # numeric comparisons in make, which are awkward.
+   while test "$version" -gt 0
+   do
+   echo $family$version
+   version=$((version - 1))
+   done
+}
+
+case "$(get_family)" in
+gcc)
+   print_flags gcc
+   ;;
+clang)
+   print_flags clang
+   ;;
+"FreeBSD clang")
+   print_flags clang
+   ;;
+"Apple LLVM")
+   print_flags clang
+   ;;
+*)
+   : unknown compiler family
+   ;;
+esac
-- 
2.17.0.rc1.439.gca064e2955

[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER mode

[Nguyễn Thái Ngọc Duy]

This mode is for developers who want to keep the code base
clean. There are warning classes that are currently suppressed because
we have too many of them in the code base. This mode will stop the
suppression, let the developer see and decide how to fix them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile   | 7 ++-
 config.mak.dev | 4 
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e6680a8977..e4f04ce1cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,7 +434,9 @@ all::
 #
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif
diff --git a/config.mak.dev b/config.mak.dev
index 716a14ecc7..13883410b3 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
-- 
2.17.0.rc1.439.gca064e2955

Re: [PATCH v4] Allow use of TLS 1.3

[Johannes Schindelin]

Hi Logan,

On Thu, 29 Mar 2018, Loganaden Velvindron wrote:

> Add a tlsv1.3 option to http.sslVersion in addition to the existing
> tlsv1.[012] options. libcurl has supported this since 7.52.0.
> 
> This requires OpenSSL 1.1.1 with TLS 1.3 enabled or curl built with
> recent versions of NSS or BoringSSL as the TLS backend.

Thank you,
Johannes

[PATCH v3 1/3] connect.c: mark die_initial_contact() NORETURN

[Nguyễn Thái Ngọc Duy]

There is a series running in parallel with this one that adds code
like this

switch (...) {
case ...:
die_initial_contact();
case ...:

There is nothing wrong with this. There is no actual falling
through. But since gcc is not that smart and gcc 7.x introduces
-Wimplicit-fallthrough, it raises a false alarm in this case.

This class of warnings may be useful elsewhere, so instead of
suppressing the whole class, let's try to fix just this code. gcc is
smart enough to realize that no execution can continue after a
NORETURN function call and no longer raises the warning.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
if (unexpected)
die(_("The remote end hung up upon initial contact"));
-- 
2.17.0.rc1.439.gca064e2955

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

On Wed, Mar 28, 2018 at 02:19:32PM -0400, Jeff King wrote:
> 
> > I will probably rework on top of your chdir-notify instead (and let
> > yours to be merged earlier)
> 
> Thanks. I like some of the related changes you made, like including this
> in the tracing output. That should be easy to do on top of mine, I
> think.

Yeah. But is it possible to sneak something like this in your series
(I assume you will reroll anyway)? I could do it separately, but it
looks nicer if it's split out and merged in individual patches that
add new chdir-notify call site.

diff --git a/chdir-notify.c b/chdir-notify.c
index 7034e98d71..a2a33443f8 100644
--- a/chdir-notify.c
+++ b/chdir-notify.c
@@ -4,21 +4,26 @@
 #include "strbuf.h"
 
 struct chdir_notify_entry {
+   const char *name;
chdir_notify_callback cb;
void *data;
struct list_head list;
 };
 static LIST_HEAD(chdir_notify_entries);
 
-void chdir_notify_register(chdir_notify_callback cb, void *data)
+void chdir_notify_register(const char *name,
+  chdir_notify_callback cb,
+  void *data)
 {
struct chdir_notify_entry *e = xmalloc(sizeof(*e));
e->cb = cb;
e->data = data;
+   e->name = name;
list_add_tail(>list, _notify_entries);
 }
 
-static void reparent_cb(const char *old_cwd,
+static void reparent_cb(const char *name,
+   const char *old_cwd,
const char *new_cwd,
void *data)
 {
@@ -28,12 +33,16 @@ static void reparent_cb(const char *old_cwd,
if (!tmp || !is_absolute_path(tmp))
return;
*path = reparent_relative_path(old_cwd, new_cwd, tmp);
+   if (name)
+   trace_printf_key(_setup_key,
+"setup: reparent %s to '%s'",
+name, *path);
free(tmp);
 }
 
-void chdir_notify_reparent(char **path)
+void chdir_notify_reparent(const char *name, char **path)
 {
-   chdir_notify_register(reparent_cb, path);
+   chdir_notify_register(name, reparent_cb, path);
 }
 
 int chdir_notify(const char *new_cwd)
@@ -45,11 +54,12 @@ int chdir_notify(const char *new_cwd)
return -1;
if (chdir(new_cwd) < 0)
return -1;
+   trace_printf_key(_setup_key, "setup: chdir to '%s'", new_cwd);
 
list_for_each(pos, _notify_entries) {
struct chdir_notify_entry *e =
list_entry(pos, struct chdir_notify_entry, list);
-   e->cb(old_cwd.buf, new_cwd, e->data);
+   e->cb(e->name, old_cwd.buf, new_cwd, e->data);
}
 
strbuf_release(_cwd);
diff --git a/chdir-notify.h b/chdir-notify.h
index c3bd818a85..b9be1b54ac 100644
--- a/chdir-notify.h
+++ b/chdir-notify.h
@@ -16,23 +16,29 @@
  * warning("switched from %s to %s!", old_path, new_path);
  *   }
  *   ...
- *   chdir_notify_register(foo, data);
+ *   chdir_notify_register("description", foo, data);
  *
  * In practice most callers will want to move a relative path to the new root;
  * they can use the reparent_relative_path() helper for that. If that's all
  * you're doing, you can also use the convenience function:
  *
- *   chdir_notify_reparent(_path);
+ *   chdir_notify_reparent("description", _path);
  *
  * Registered functions are called in the order in which they were added. Note
  * that there's currently no way to remove a function, so make sure that the
  * data parameter remains valid for the rest of the program.
+ *
+ * The first argument is used for tracing purpose only. when my_path is updated
+ * by chdir_notify_reparent() it will print a message if $GIT_TRACE_SETUP is 
set.
+ * This argument could be NULL.
  */
-typedef void (*chdir_notify_callback)(const char *old_cwd,
+typedef void (*chdir_notify_callback)(const char *name,
+ const char *old_cwd,
  const char *new_cwd,
  void *data);
-void chdir_notify_register(chdir_notify_callback cb, void *data);
-void chdir_notify_reparent(char **path);
+void chdir_notify_register(const char *name, chdir_notify_callback cb,
+  void *data);
+void chdir_notify_reparent(const char *name, char **path);
 
 /*
  *
diff --git a/environment.c b/environment.c
index 794a75a717..92701cbc0c 100644
--- a/environment.c
+++ b/environment.c
@@ -330,11 +330,15 @@ static void set_git_dir_1(const char *path)
setup_git_env(path);
 }
 
-static void update_relative_gitdir(const char *old_cwd,
+static void update_relative_gitdir(const char *name,
+  const char *old_cwd,
   const char *new_cwd,
   void *data)
 {
char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());
+   trace_printf_key(_setup_key,
+"setup: move 

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Junio,

On Wed, 28 Mar 2018, Junio C Hamano wrote:

> Daniel Jacques  writes:
> 
> > A simple grep suggests that the current test suite doesn't seem to have any
> > RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> > been doing it with a "config.mak" file that explicitly enables
> > RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> > Git testing suites.
> >
> > From a Git maintainer's perspective, would such a test be a prerequisite
> > for landing this patch series, or is this a good candidate for follow-up
> > work to improve our testing coverage?
> 
> It would be a nice-to-have follow-up, I would say, but as you two
> seem to be working well together and it shouldn't be too involved to
> have the minimum test that makes sure the version of "git" being
> tested thinks things should be where we think they should be, with
> something like...
> 
>   test_expect_success RUNTIME_PREFIX 'runtime-prefix basics' '
>   (
>   # maybe others
>   safe_unset GIT_EXEC_PATH &&
>   git --exec-path >actual

That will only work when the directory into which git (or git.exe) was
compiled is called "bin" or "git" (or "git-core" in a "libexec"
directory), because this is the sanity check we have to determine that Git
is installed into a sensible location where we *can* assume that
libexec/git-core/ is the corresponding location of the support
executables/scripts.

I initially thought that we could somehow do this:

-- snip --
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 2b3c5092a19..3040f0dae49 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "string-list.h"
+#include "exec_cmd.h"
 
 /*
  * A "string_list_each_func_t" function that normalizes an entry from
@@ -270,6 +271,25 @@ int cmd_main(int argc, const char **argv)
if (argc == 2 && !strcmp(argv[1], "dirname"))
return test_function(dirname_data, posix_dirname,
argv[1]);
 
+   if (argc == 3 && !strcmp(argv[1], "runtime-prefix")) {
+#ifndef RUNTIME_PREFIX
+   warning("RUNTIME_PREFIX support not compiled in;
skipping");
+   return 0;
+#else
+   char *path;
+
+   git_resolve_executable_dir(argv[2]);
+   path = system_path("");
+
+   if (!starts_with(argv[2], path))
+   return error("unexpected prefix: '%s'", path);
+
+   puts(path);
+
+   return 0;
+#endif
+   }
+
fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
argv[1] ? argv[1] : "(there was none)");
return 1;
```

but this simply won't work, as the main idea of
`git_resolve_executable_dir()` is to use the executable path whenever
possible, instead of the passed-in parameter.

And since we usually work via the bin-wrappers, we cannot even add a
sanity check that Git was cloned into a directory called "git"...

So... I think we have to leave this out of the patch series, unless
somebody comes up with an idea neither Dan nor I has thought about to test
this reliably *without* copying the Git executable (which would, as I
mentioned, break testing when .dll files need to be present in the same
directory as git.exe).

Ciao,
Dscho

Re: [PATCH 2/4] add chdir-notify API

[Duy Nguyen]

On Wed, Mar 28, 2018 at 7:40 PM, Jeff King  wrote:
> +static void reparent_cb(const char *old_cwd,
> +   const char *new_cwd,
> +   void *data)
> +{
> +   char **path = data;

Maybe check data == NULL and return early. This is just for
convenience, e.g. instead of doing

path = getenv("foo");
if (path)
chdir_notify_reparent();

I could do

path = getenv("foo");
chdir_notify_reparent();

> +   char *tmp = *path;
> +   *path = reparent_relative_path(old_cwd, new_cwd, tmp);
> +   free(tmp);
> +}
> +
> +void chdir_notify_reparent(char **path)
> +{
> +   if (!is_absolute_path(*path))

I think this check is a bit early. There could be a big gap between
chdir_notify_reparent() and reparent_cb(). During that time, *path may
be updated and become a relative path. We can check for absolute path
inside reparent_cb() instead.

> +   chdir_notify_register(reparent_cb, path);
> +}

Overall, I like this API very much! I will add another one similar to
chdir_notify_reparent() but it takes an env name instead and the
callback will setenv() appropriately. The result looks super good.
-- 
Duy

Re: [PATCH v6 0/6] ref-filter: remove die() calls from formatting logic

[Christian Couder]

On Thu, Mar 29, 2018 at 2:52 PM, Оля Тележная  wrote:
> Move helper function from strbuf to ref-filter.
> Get rid of some memory leaks.

The above seems to be the changes since v5. Usually in a cover letter
(patch 0/X) there is both information about the goal of the patch
series and the changes since last version.

Repeating the goal in each version is useful for reviewers who might
not have time to look at the patch series before, or who might have
forgotten about it.

Thanks,
Christian.

Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own file

[Jameson Miller]

Thank you for the thorough and detailed review - I very much appreciate it.

> I
> am OK if the structure of this series is to make that change after
> these three steps we see here. 

I will add (back) the accounting of large memory blocks in a future patch
series, taking into account your existing feedback. As this patch series did 
not have
any consumers that "discarded" a memory pool, I decided to wait until I had 
a consumer that used this functionality.

> Will queue; the proposed log for step 2/3 may want to be a bit
> polished, though.

I am away the rest of this week and next week, but will update this patch
series when I get back (Week of April 9th).

Thank you
Jameson


From: Junio C Hamano  on behalf of Junio C Hamano 

Sent: Tuesday, March 27, 2018 12:43 PM
To: Jameson Miller
Cc: git@vger.kernel.org; p...@peff.net
Subject: Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own 
file 
 
Jameson Miller  writes:

> This moves the reusable parts of the memory pool logic used by
> fast-import.c into its own file for use by other components.
>
> Signed-off-by: Jameson Miller 
> ---
>  Makefile  |  1 +
>  fast-import.c | 70 
>+--
>  mem-pool.c    | 55 ++
>  mem-pool.h    | 34 +
>  4 files changed, 91 insertions(+), 69 deletions(-)
>  create mode 100644 mem-pool.c
>  create mode 100644 mem-pool.h

OK.  This is indeed straight-forward line movements and nothing else,
other than obviously a few static helpers are now extern.

I said I'd anticipate that the allocation that bypasses the pool
subsystem would want to become traceable by the pool subsystem,
which would allow us to free() the pieces of memory allocated
directly with xmalloc() in mem_pool_alloc() instead of leaking.  I
am OK if the structure of this series is to make that change after
these three steps we see here.  When that happens, it will start to
make sense to bill the "this is too big so do not attempt to carve
it out from block, and do not overallocate and make the remainder
available for later requests" to the pool instance mem_pool_alloc()
is working on, as that piece of memory is also known to a specific
pool instance.

After I wrote review for 2/3, I found out that you changed the
meaning of total_allocd (which should probably be described in its
log message).  Unlike the original that counted "total", it now is
used only for memory that is allocated directly by fast-import.c and
does not account for memory obtained by calling mem-pool.

The output routine is changed in 2/3 to add fi_mem_pool's pool_alloc
to it, so billing oversized allocation that does *not* belong to any
specific pool to _some_ pool and ignoring total_allocd would cancel
things out.  It still feels a bit fishy, but I think it is OK.

So all in all, I think we are in no worse shape than the original
(we call it "bug-to-bug compatible" ;-)), and successfully extracted
a reusable piece in a separate file in a clean way so that we can
refine and extend it further.  Nicely done.

Will queue; the proposed log for step 2/3 may want to be a bit
polished, though.

Thanks.

Re: [PATCH 00/10] Hash-independent tests (part 1)

[Johannes Schindelin]

Hi Junio,

On Wed, 28 Mar 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> Ideally, the existing one be annotated with prereq SHA1, and also
> >> duplicated with a tweak to cause the same kind of (half-)collision
> >> under the NewHash and be annotated with prereq NewHash.
> >
> > That's a good idea. I wonder whether we want to be a bit more specific,
> > though, so that we have something grep'able for the future? Something like
> > SHA1_SHORT_COLLISION or some such?
> 
> Sorry, you lost me.  
> 
> What I meant was that a test, for example, that expects the object
> name for an empty blob begins with e69de29 is valid ONLY when Git is
> using SHA-1 to name objects, so such a test should be run only when
> Git is using SHA-1 and no other hash.  All tests in t1512 that
> expects the sequence of events in it would yield blobs and trees
> whose names have many leading "0"s are the same way.
> 
> I think it would do to have a single prerequisite to cover all such
> tests: "Run this piece of test only when Git is using SHA-1 hash".
> I do not think you need a set of prerequisites to say "Run this with
> SHA-1 because we are testing X" where X may be "disambiguation",
> "unique-abbrev", "loose-refs", or whatever.

I meant for the test case to be annotated such that one could find easily
which test cases rely on specially-crafted objects to cause identical hash
prefixes.

But I guess it is not worth the effort (because you'll only find out which
tests miss that annotation when you try to port the test suite to a new
hash).

Ciao,
Dscho

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff Hostetler]

On 3/28/2018 6:12 PM, Junio C Hamano wrote:

Jonathan Nieder  writes:


When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
2017-12-08), it was guarded by the uploadpack.allowFilter config item
to allow server operators to control when they start supporting it.

That config item didn't go far enough, though: it controls whether the
'filter' capability is advertised, but if a (custom) client ignores
the capability advertisement and passes a filter specification anyway,
the server would handle that despite allowFilter being false.

This is particularly significant if a security bug is discovered in
this new experimental partial clone code.  Installations without
uploadpack.allowFilter ought not to be affected since they don't
intend to support partial clone, but they would be swept up into being
vulnerable.

Simplify and limit the attack surface by making uploadpack.allowFilter
disable the feature, not just the advertisement of it.

NEEDSWORK: tests

Signed-off-by: Jonathan Nieder 
---
Noticed while reviewing the corresponding JGit code.

If this change seems like a good idea, I can add tests and re-send it
for real.


Yup.  The names of the static variables tell almost the whole story
to convince me why this is a good change ;-).  It is not about
advertising a feature alone, but if the feature is actually enabled,
so advertisement and handling of the feature should be either both
enabled or disabled.

Thanks.



I agree.  Thanks.

[PATCH v6 0/6] ref-filter: remove die() calls from formatting logic

[Оля Тележная]

Move helper function from strbuf to ref-filter.
Get rid of some memory leaks.

Thanks to everyone!
Olga

[PATCH v6 1/6] ref-filter: add shortcut to work with strbufs

[Olga Telezhnaya]

Add function strbuf_addf_ret() that helps to save a few lines of code.
Function expands fmt with placeholders, append resulting message
to strbuf *sb, and return error code ret.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..0c8d1589cf316 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,6 +101,19 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 
+/*
+ * Expand string, append it to strbuf *sb, then return error code ret.
+ * Allow to save few lines of code.
+ */
+static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
+{
+   va_list ap;
+   va_start(ap, fmt);
+   strbuf_vaddf(sb, fmt, ap);
+   va_end(ap);
+   return ret;
+}
+
 static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
 {
if (!color_value)

--
https://github.com/git/git/pull/466

[PATCH v6 4/6] ref-filter: change parsing function error handling

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
The function returns the position in the used_atom[] array
(as before) for the given atom, or -1 to signal an error.
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index a18c86961f08c..93fa6b4e5e63d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -410,7 +410,8 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
-const char *atom, const char *ep)
+const char *atom, const char *ep,
+struct strbuf *err)
 {
const char *sp;
const char *arg;
@@ -420,7 +421,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
if (*sp == '*' && sp < ep)
sp++; /* deref */
if (ep <= sp)
-   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+   return strbuf_addf_ret(err, -1, _("malformed field name: %.*s"),
+  (int)(ep-atom), atom);
 
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -446,7 +448,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
}
 
if (ARRAY_SIZE(valid_atom) <= i)
-   die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+   return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
+  (int)(ep-atom), atom);
 
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -728,17 +731,21 @@ int verify_ref_format(struct ref_format *format)
 
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+   struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
 
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, sp + 2, ep);
+   at = parse_ref_filter_atom(format, sp + 2, ep, );
+   if (at < 0)
+   die("%s", err.buf);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
format->need_color_reset_at_eol = !!strcmp(color, 
"reset");
+   strbuf_release();
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2157,13 +2164,17 @@ int format_ref_array_item(struct ref_array_item *info,
 
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+   int pos;
 
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, );
-   get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, ep),
-  );
+   pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+   if (pos < 0) {
+   pop_stack_element();
+   return -1;
+   }
+   get_ref_atom_value(info, pos, );
if (atomv->handler(atomv, , error_buf)) {
pop_stack_element();
return -1;
@@ -,7 +2233,12 @@ static int parse_sorting_atom(const char *atom)
 */
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
-   return parse_ref_filter_atom(, atom, end);
+   struct strbuf err = STRBUF_INIT;
+   int res = parse_ref_filter_atom(, atom, end, );
+   if (res < 0)
+   die("%s", err.buf);
+   strbuf_release();
+   return res;
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/466

[PATCH v6 2/6] ref-filter: start adding strbufs with errors

[Olga Telezhnaya]

This is a first step in removing die() calls from ref-filter
formatting logic, so that it could be used by other commands
that do not want to die during formatting process.
die() calls related to bugs in code will not be touched in this patch.

Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.

This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.

Signed-off-by: Olga Telezhnaia 
---
 builtin/branch.c |  7 +--
 ref-filter.c | 17 -
 ref-filter.h |  7 ---
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
-   struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
 
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
ref_array_sort(sorting, );
 
for (i = 0; i < array.nr; i++) {
-   format_ref_array_item(array.items[i], format, );
+   struct strbuf out = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
+   if (format_ref_array_item(array.items[i], format, , ))
+   die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are 
incompatible");
 /* format to a string_list to let print_columns() do 
its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+   strbuf_release();
strbuf_release();
}
 
diff --git a/ref-filter.c b/ref-filter.c
index 0c8d1589cf316..9833709dbefe3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2131,9 +2131,10 @@ static void append_literal(const char *cp, const char 
*ep, struct ref_formatting
}
 }
 
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
   const struct ref_format *format,
-  struct strbuf *final_buf)
+  struct strbuf *final_buf,
+  struct strbuf *error_buf)
 {
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2161,19 +2162,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(, );
}
-   if (state.stack->prev)
-   die(_("format: %%(end) atom missing"));
+   if (state.stack->prev) {
+   pop_stack_element();
+   return strbuf_addf_ret(error_buf, -1, _("format: %%(end) atom 
missing"));
+   }
strbuf_addbuf(final_buf, >output);
pop_stack_element();
+   return 0;
 }
 
 void show_ref_array_item(struct ref_array_item *info,
 const struct ref_format *format)
 {
struct strbuf final_buf = STRBUF_INIT;
+   struct strbuf error_buf = STRBUF_INIT;
 
-   format_ref_array_item(info, format, _buf);
+   if (format_ref_array_item(info, format, _buf, _buf))
+   die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
strbuf_release(_buf);
putchar('\n');
 }
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
-  const struct ref_format *format,
-  struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const struct ref_format 
*format);
 /*  Parse a single sort specifier and add it to 

[PATCH v6 3/6] ref-filter: add return value && strbuf to handlers

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of handlers by adding return value
and strbuf parameter for errors.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 51 +++
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 9833709dbefe3..a18c86961f08c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -400,7 +400,8 @@ struct ref_formatting_state {
 
 struct atom_value {
const char *s;
-   void (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state);
+   int (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state,
+  struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
 };
@@ -494,7 +495,8 @@ static void quote_formatting(struct strbuf *s, const char 
*str, int quote_style)
}
 }
 
-static void append_atom(struct atom_value *v, struct ref_formatting_state 
*state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state 
*state,
+  struct strbuf *unused_err)
 {
/*
 * Quote formatting is only done when the stack has a single
@@ -506,6 +508,7 @@ static void append_atom(struct atom_value *v, struct 
ref_formatting_state *state
quote_formatting(>stack->output, v->s, 
state->quote_style);
else
strbuf_addstr(>stack->output, v->s);
+   return 0;
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -540,7 +543,8 @@ static void end_align_handler(struct ref_formatting_stack 
**stack)
strbuf_release();
 }
 
-static void align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+ struct strbuf *unused_err)
 {
struct ref_formatting_stack *new_stack;
 
@@ -548,6 +552,7 @@ static void align_atom_handler(struct atom_value *atomv, 
struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = >atom->u.align;
+   return 0;
 }
 
 static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -585,7 +590,8 @@ static void if_then_else_handler(struct 
ref_formatting_stack **stack)
free(if_then_else);
 }
 
-static void if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+  struct strbuf *unused_err)
 {
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct 
if_then_else), 1);
@@ -597,6 +603,7 @@ static void if_atom_handler(struct atom_value *atomv, 
struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+   return 0;
 }
 
 static int is_empty(const char *s)
@@ -609,7 +616,8 @@ static int is_empty(const char *s)
return 1;
 }
 
-static void then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+struct strbuf *err)
 {
struct ref_formatting_stack *cur = state->stack;
struct if_then_else *if_then_else = NULL;
@@ -617,11 +625,11 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
if (cur->at_end == if_then_else_handler)
if_then_else = (struct if_then_else *)cur->at_end_data;
if (!if_then_else)
-   die(_("format: %%(then) atom used without an %%(if) atom"));
+   return strbuf_addf_ret(err, -1, _("format: %%(then) atom used 
without an %%(if) atom"));
if (if_then_else->then_atom_seen)
-   die(_("format: %%(then) atom used more than once"));
+   return strbuf_addf_ret(err, -1, _("format: %%(then) atom used 
more than once"));
if (if_then_else->else_atom_seen)
-   die(_("format: %%(then) atom used after %%(else)"));
+   return strbuf_addf_ret(err, -1, _("format: %%(then) atom used 
after %%(else)"));
if_then_else->then_atom_seen = 1;
/*
 * If the 'equals' or 'notequals' attribute is used then
@@ -637,9 +645,11 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(>output);
+   return 0;
 }
 

[PATCH v6 6/6] ref-filter: libify get_ref_atom_value()

[Olga Telezhnaya]

Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 54 ++
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3f85ef64267d9..3bc65e49358ee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1427,28 +1427,30 @@ static const char *get_refname(struct used_atom *atom, 
struct ref_array_item *re
return show_ref(>u.refname, ref->refname);
 }
 
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
-  int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+  int deref, struct object **obj, struct strbuf *err)
 {
int eaten;
+   int ret = 0;
unsigned long size;
void *buf = get_obj(oid, obj, , );
if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(oid), ref->refname);
-   if (!*obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(oid), ref->refname);
-
-   grab_values(ref->value, deref, *obj, buf, size);
+   ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+ oid_to_hex(oid), ref->refname);
+   else if (!*obj)
+   ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on 
%s for %s"),
+ oid_to_hex(oid), ref->refname);
+   else
+   grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+   return ret;
 }
 
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
struct object *obj;
int i;
@@ -1570,16 +1572,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
-   return;
+   return 0;
 
-   get_object(ref, >objectname, 0, );
+   if (get_object(ref, >objectname, 0, , err))
+   return -1;
 
/*
 * If there is no atom that wants to know about tagged
 * object, we are done.
 */
if (!need_tagged || (obj->type != OBJ_TAG))
-   return;
+   return 0;
 
/*
 * If it is a tag object, see if we use a value that derefs
@@ -1593,20 +1596,23 @@ static void populate_value(struct ref_array_item *ref)
 * is not consistent with what deref_tag() does
 * which peels the onion to the core.
 */
-   get_object(ref, tagged, 1, );
+   return get_object(ref, tagged, 1, , err);
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct 
atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
 {
if (!ref->value) {
-   populate_value(ref);
+   if (populate_value(ref, err))
+   return -1;
fill_missing_values(ref->value);
}
*v = >value[atom];
+   return 0;
 }
 
 /*
@@ -2130,9 +2136,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct 
ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+   struct strbuf err = STRBUF_INIT;
 
-   get_ref_atom_value(a, s->atom, );
-   get_ref_atom_value(b, s->atom, );
+   if (get_ref_atom_value(a, s->atom, , ))
+   die("%s", err.buf);
+   if (get_ref_atom_value(b, s->atom, , ))
+   die("%s", err.buf);
+   strbuf_release();
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2210,12 +2220,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, );
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
-   if (pos < 0) {
-   pop_stack_element();
-   return -1;
-   }
-   get_ref_atom_value(info, pos, );
-   if (atomv->handler(atomv, , error_buf)) {
+   if (pos < 0 || 

[PATCH v6 5/6] ref-filter: add return value to parsers

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of parsers by adding return value and
strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 138 ++-
 1 file changed, 89 insertions(+), 49 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 93fa6b4e5e63d..3f85ef64267d9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -114,22 +114,25 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, 
const char *fmt, ...)
return ret;
 }
 
-static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom 
*atom,
+const char *color_value, struct strbuf *err)
 {
if (!color_value)
-   die(_("expected format: %%(color:)"));
+   return strbuf_addf_ret(err, -1, _("expected format: 
%%(color:)"));
if (color_parse(color_value, atom->u.color) < 0)
-   die(_("unrecognized color: %%(color:%s)"), color_value);
+   return strbuf_addf_ret(err, -1, _("unrecognized color: 
%%(color:%s)"),
+  color_value);
/*
 * We check this after we've parsed the color, which lets us complain
 * about syntactically bogus color names even if they won't be used.
 */
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+   return 0;
 }
 
-static void refname_atom_parser_internal(struct refname_atom *atom,
-const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char 
*arg,
+const char *name, struct strbuf *err)
 {
if (!arg)
atom->option = R_NORMAL;
@@ -139,16 +142,18 @@ static void refname_atom_parser_internal(struct 
refname_atom *atom,
 skip_prefix(arg, "strip=", )) {
atom->option = R_LSTRIP;
if (strtol_i(arg, 10, >lstrip))
-   die(_("Integer value expected refname:lstrip=%s"), arg);
+   return strbuf_addf_ret(err, -1, _("Integer value 
expected refname:lstrip=%s"), arg);
} else if (skip_prefix(arg, "rstrip=", )) {
atom->option = R_RSTRIP;
if (strtol_i(arg, 10, >rstrip))
-   die(_("Integer value expected refname:rstrip=%s"), arg);
+   return strbuf_addf_ret(err, -1, _("Integer value 
expected refname:rstrip=%s"), arg);
} else
-   die(_("unrecognized %%(%s) argument: %s"), name, arg);
+   return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) 
argument: %s"), name, arg);
+   return 0;
 }
 
-static void remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom,
+ const char *arg, struct strbuf *err)
 {
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -158,9 +163,8 @@ static void remote_ref_atom_parser(const struct ref_format 
*format, struct used_
 
if (!arg) {
atom->u.remote_ref.option = RR_REF;
-   refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
-   return;
+   return refname_atom_parser_internal(>u.remote_ref.refname,
+   arg, atom->name, err);
}
 
atom->u.remote_ref.nobracket = 0;
@@ -183,29 +187,38 @@ static void remote_ref_atom_parser(const struct 
ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
-   
refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
+   if 
(refname_atom_parser_internal(>u.remote_ref.refname,
+arg, atom->name, err)) 
{
+   string_list_clear(, 0);
+   return -1;
+   }
}
}
 
string_list_clear(, 0);
+   return 0;
 }
 
-static void body_atom_parser(const struct ref_format *format, struct used_atom 
*atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom 
*atom,
+   const char *arg, struct 

Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash

[Jeff King]

On Wed, Mar 28, 2018 at 03:20:51PM -0700, Erik E Brady wrote:

> Subject: Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, 
> crash

Thanks for sending this. The patch itself looks good to me, but I have a
few nits with your commit message.

We usually write commit messages in the imperative, with the subject
summarizing the change. So:

  Subject: credential: ignore SIGPIPE when writing to credential helpers

or similar.

> credential.c, run_credential_helper(): now ignores SIGPIPE
> when writing to credential helper.  Avoids problem with race
> where cred helper exits very quickly and, after, git tries
> to write to it, generating SIGPIPE and crashing git.  To
> reproduce this the cred helper must not read from STDIN.

We can stop being terse outside of the subject line. :) I'd probably
write something like:

  The credential subsystem can trigger SIGPIPE when writing to an
  external helper if that helper closes its stdin before reading the
  whole input. Normally this is rare, since helpers would need to read
  that input to make a decision about how to respond, but:

1. It's reasonable to configure a helper which blindly a "get"
   answer, and trigger it only for certain hosts via config like:

 [credential "https://example.com;]
 helper = "!get-example-password"

2. A broken or misbehaving helper might exit immediately. That's an
   error, but it's not reasonable for it to take down the parent Git
   process with SIGPIPE.

  Even with such a helper, seeing this problem should be rare. Getting
  SIGPIPE requires the helper racily exiting before we've written the
  fairly small credential output.

Feel free to steal or adapt any of that as you see fit.

> This was seen with a custom credential helper, written in
> Go, which ignored the store command (STDIN not read) and
> then did a quick exit.  Even with this fast helper the race
> was pretty rare, ie: was only seen on some of our older VM's
> running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On
> these VM's it occurred only once every few hundred git cmds.
> ---

Missing signoff. See Documentation/SubmittingPatches, especially the
'sign-off' and 'dco' sections.

>  credential.c | 3 +++
>  1 file changed, 3 insertions(+)

No test, but I think that's fine here. Any such test would be inherently
racy.

> @@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,
>   return -1;
>  
>   fp = xfdopen(helper.in, "w");
> + sigchain_push(SIGPIPE, SIG_IGN);
>   credential_write(c, fp);
>   fclose(fp);
> + sigchain_pop(SIGPIPE);

This looks like the right place to put the push/pop (as you noted
before, we may not write until fclose flushes, so it definitely has to
go after that).

Thanks again for digging this up. It's pretty subtle. :)

-Peff

[PATCH v4] Allow use of TLS 1.3

[Loganaden Velvindron]

Add a tlsv1.3 option to http.sslVersion in addition to the existing
tlsv1.[012] options. libcurl has supported this since 7.52.0.

This requires OpenSSL 1.1.1 with TLS 1.3 enabled or curl built with
recent versions of NSS or BoringSSL as the TLS backend.

Signed-off-by: Loganaden Velvindron 
---
 Documentation/config.txt | 1 +
 http.c   | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce9102cea..f31d62772 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1957,6 +1957,7 @@ http.sslVersion::
- tlsv1.0
- tlsv1.1
- tlsv1.2
+   - tlsv1.3
 
 +
 Can be overridden by the `GIT_SSL_VERSION` environment variable.
diff --git a/http.c b/http.c
index a5bd5d62c..f84b18551 100644
--- a/http.c
+++ b/http.c
@@ -62,6 +62,9 @@ static struct {
{ "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
 #endif
+#if LIBCURL_VERSION_NUM >= 0x073400
+   { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 }
+#endif
 };
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
-- 
2.16.2

Re: [PATCH v4 1/5] stash: improve option parsing test coverage

[Eric Sunshine]

On Wed, Mar 28, 2018 at 6:21 PM, Joel Teichroeb  wrote:
> In preparation for converting the stash command incrementally to
> a builtin command, this patch improves test coverage of the option
> parsing. Both for having too many paramerters, or too few.

s/paramerters/parameters/

> Signed-off-by: Joel Teichroeb 

Re: M.Williams Chambers and Associates.

[omar . gamez01]

Good Day and congratulations . My Client Mrs. Marilyn Boldon has made a 
donation 3.5m euros to you, contact her through her private email: 
marilynbol...@gmail.com for more detail on how to receive this donation.

Barrister. Omar Gamez Vargas. Esq.
M.Williams Chambers and Associates.