Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:40:03PM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau  wrote:
> > [...]
> > This commit (and those following it in this series) aim to eventually
> > replace `--get-color` with a consistent alternative. By introducing
> > `--default`, we allow the `--get-color` action to be promoted to a
> > `--type=color` type specifier, retaining the "fallback" behavior via the
> > `--default` flag introduced in this commit.
> > [...]
> >
> > Signed-off-by: Taylor Blau 
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -235,6 +235,10 @@ Valid ``'s include:
> > +--default value::
>
> This should be typeset as: --default 

I see; thank you for letting me know. I have updated this in the
forthcoming re-roll.

> > +  When using `--get`, and the requested slot is not found, behave as if 
> > value
> > +  were the value assigned to the that slot.
>
> And you might use "behaves as if " in the body as well (though
> I think Git documentation isn't terribly consistent about typesetting
> as "" in the body, so I don't insist upon it).

Since I'm here, and re-rolling anyway, I have included this change as
well.


Thanks,
Taylor

Re: [PATCH v5 0/2] *** SUBJECT HERE ***

[Jacob Keller]

On Thu, Apr 5, 2018 at 10:27 PM, Taylor Blau  wrote:
> *** BLURB HERE ***
>

Missing subject and cover letter stuff.. did you submit before you
were ready? or did you not mean to include the cover letter? :)

-Jake

> Taylor Blau (2):
>   builtin/config.c: treat type specifiers singularly
>   builtin/config.c: prefer `--type=bool` over `--bool`, etc.
>
>  Documentation/git-config.txt | 74 +++---
>  builtin/config.c | 77 +++-
>  t/t1300-repo-config.sh   | 29 ++
>  3 files changed, 121 insertions(+), 59 deletions(-)
>
> --
> 2.17.0

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:29:49PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
> > *regex_)
> > config_with_options(collect_config, ,
> > _config_source, _options);
> >
> > +   if (!values.nr && default_value) {
> > +   struct strbuf *item;
> > +   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> > +   item = [values.nr++];
> > +   strbuf_init(item, 0);
> > +   if (format_config(item, key_, default_value) < 0) {
> > +   exit(1);
> > +   }
> > +   }
>
> Calling exit() explicitly is unusual for our code. Usually we would
> either die() or propagate the error. Most of the types in
> format_config() would die on bogus input, but a few code paths will
> return an error.
>
> What happens if a non-default value has a bogus format? E.g.:
>
>   $ git config foo.bar '~NoSuchUser'
>   $ git config --path foo.bar
>   fatal: failed to expand user dir in: '~NoSuchUser'
>
> Oops. Despite us checking for an error return from
> git_config_pathname(), it will always either return 0 or die(). So
> that's not a good example. ;)
>
> Let's try expiry-date:
>
>   $ git config foo.bar 'the first of octember'
>   $ git config --expiry-date foo.bar
>   error: 'the first of octember' for 'foo.bar' is not a valid timestamp
>   fatal: bad config line 7 in file .git/config
>
> OK. So we call format_config() there from the actual collect_config()
> callback, and the error gets propagated back to the config parser, which
> then gives us an informative die(). What happens with your new code:
>
>   $ ./git config --default 'the first of octember' --type=expiry-date 
> no.such.key
>   error: 'the first of octember' for 'no.such.key' is not a valid timestamp
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Aha. Thanks for the explanation :-). I've removed the exit() and changed
it to the die() that you suggested above. The test in t1310 is updated
to grep for the new message, so we know that it is being reported
appropriately.

> > +test_expect_success 'does not allow --default without --get' '
> > +   test_must_fail git config --default quux --unset a >output 2>&1 &&
> > +   test_i18ngrep "\-\-default is only applicable to" output
> > +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

Aha, thanks again. I have updated this in the forthcoming re-roll.


Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 10:29:01PM -0700, Taylor Blau wrote:
> Hi,

Ack. Clearly I am still getting used to things outside of Git LFS. I have sent
this topic in response to my series to add "--default" instead of the
appropriate series.

I'll avoid re-sending it in an effort to reduce further noise. In the meantime,
thanks for understanding my silly mistakes.


Thanks,
Taylor

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

[Taylor Blau]

Hi,

(Apologies for the noise of accidentally sending an empty template cover
letter. Here is the real one :-).)

Attached is my fifth, and anticipated final re-roll of my series to add
"--type=" to "git config" in an effort to replace "--bool" with
"--type=bool".

I have changed the following since v4:

  - Remove extra space when redirecting in t1300 (cc: Peff).

  - Clarify the effect of using "--type" in Documentation (cc: Eric).

  - Document "--no-type" in Documentation (cc: Eric).

  - Fix an outstanding typo in Documentation, "input output" (cc: Peff).

Thanks as always for your feedback -- I look forward to having this queued.


Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 74 +++---
 builtin/config.c | 77 +++-
 t/t1300-repo-config.sh   | 29 ++
 3 files changed, 121 insertions(+), 59 deletions(-)

--
2.17.0

[PATCH v5 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

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

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..92fb8d56b 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;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL  1
+#define TYPE_INT   2
+#define TYPE_BOOL_OR_INT   3
+#define TYPE_PATH  4
+#define TYPE_EXPIRY_DATE   5
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -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 

[PATCH v5 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
`--type=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 `--type=color` (in the traditional sense) in
the future, if that is desired.

In this patch, we prefer `--type=` over
`--int`, `--bool`, and etc. This allows the aforementioned upcoming
patch to support querying a color value with a default via `--type=color
--default=`, without squandering `--color`.

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

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..31931ae00 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,13 @@ 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.
+The `--type` option requests 'git config' to ensure that the configured values
+assosciated with the given variable(s) are of the given type. When given
+`--type`, 'git config' will convert the value to that type's canonical form.
+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. Callers may unset any pre-existing type specifiers with
+`--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +161,39 @@ 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 ::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in ``'s
+  canonical form.
++
+Valid ``'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal 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 

[PATCH v5 0/2] *** SUBJECT HERE ***

[Taylor Blau]

*** BLURB HERE ***

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 74 +++---
 builtin/config.c | 77 +++-
 t/t1300-repo-config.sh   | 29 ++
 3 files changed, 121 insertions(+), 59 deletions(-)

-- 
2.17.0

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:40:51PM -0400, Jeff King wrote:
> On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:
>
> > > +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.
> > > @@ -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 ::
> > > +  'git config' will ensure that any input output is valid under the 
> > > given type
> > > +  constraint(s), and will canonicalize outgoing values in ``'s 
> > > canonical
> > > +  form.
> >
> > In response to my question[2] about whether the typesetting "[type]"
> > meant that it was optional, you responded[1] that it was not, thus
> > correctly changed the typesetting to "". However...
>
> Right, "--type" without a specifier means nothing (also, I missed this
> in my review, but "input output" in the quoted text?)

I missed this too; thanks for pointing it out. I have amended this in
the forthcoming re-roll.

> > > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are 
> > > given precedence' '
> > > +test_expect_success '--no-type unsets type specifiers' '
> > > +   echo "10" > expect &&
> > > +   git config --type=bool --no-type core.number >actual &&
> > > +   test_cmp expect actual
> > > +'
> >
> > What does --no-type mean and why is it being tested? If this is an
> > explicitly supported user-facing option, should it be documented? If
> > it's not meant to be user-facing, then why are we enforcing its
> > presence and behavior via a test?
>
> It would be the same as if no --type option had been given. The current
> documentation says:
>
>   If no type specifier is passed, no checks or transformations are
>   performed on the value.
>
> That's retained in the DESCRIPTION section, but it may be worth a
> mention of the "--no-type" behavior in the OPTIONS section, too. I
> dunno.

Fair; my inclination is to document it, since it is potentially useful
for scripts (as I mentioned in my mail to Eric).

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 10:00 PM, Taylor Blau  wrote:
> > [...]
> > In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> > `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> > patch to support querying a color value with a default via `--type=color
> > --default=`
> >
> > Signed-off-by: Taylor Blau 
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -38,12 +38,10 @@ existing values that match the regexp are updated or 
> > unset.  If
> > +A type specifier may be given as an argument to `--type` to make 'git 
> > config'
>
> In [1], you said that the argument to --type is required, so use of
> "may be given" here is ambiguous; it makes it sound as if the argument
> is optional. Perhaps rewrite something like:
>
> The --type option requests `git config` to ...

Thanks; I agree that this is much clearer.

> Not necessarily worth a re-roll, though. (But if you do need to
> re-roll for some reason, it might make sense to combine this series
> with the --default series to make it slightly easier to review them
> together -- since the one depends upon the other -- and probably ease
> the burden on Junio slightly.)

I am re-rolling for some of the additional feedback, and have included
the above changes in it. That being the case, I think that I would like
to hold off on merging the two together, since they are separate topics
and should be merged as-such.

If this list feels differently, however, I can happily merge the two.

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are 
> > given precedence' '
> > +test_expect_success '--no-type unsets type specifiers' '
> > +   echo "10" > expect &&
> > +   git config --type=bool --no-type core.number >actual &&
> > +   test_cmp expect actual
> > +'
>
> What does --no-type mean and why is it being tested? If this is an
> explicitly supported user-facing option, should it be documented? If
> it's not meant to be user-facing, then why are we enforcing its
> presence and behavior via a test?

I think that --no-type should be documented. It means "unset any
previously set --type= or --". For example, "git config
--type=bool --no-type core.var" is a synonym of "git config core.var".

This option is useful for callers that would like to overwrite scripts
that impose a specific type specifier. (For example, my mail script
invokes "git format-patch --thread $@", but I can disable threading if I
run my script with "--no-thread".)

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:12:02PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:
>
> > +test_expect_success '--no-type unsets type specifiers' '
> > +   echo "10" > expect &&
> > +   git config --type=bool --no-type core.number >actual &&
> > +   test_cmp expect actual
> > +'
>
> Actually, one minor nit (not worth a re-roll, but Junio may want to mark
> it up): drop the space in "> expect".

I'm re-rolling for some of the other review this round picked up, so
I've included this in the forthcoming v5.

Thanks,
Taylor

Re: Self-inflicted "abort" in a newbie attempt at read-only exploration of a cloned repository?

[Thierry Moreau]

On 05/04/18 11:34 PM, Bryan Turner wrote:

On Thu, Apr 5, 2018 at 4:18 PM, Bryan Turner  wrote:


So passing --work-tree tells Git where to store your _files_, but it's
still using the same .git directory.

If your goal is to have worktrees for various versions, that implies
the git worktree [1] command might be more along the lines of what
you're looking for. An invocation based on above might look like this:
$ git -C linux-stable/ worktree add $PWD/tmp/ checkout linux-4.15.y


Apologies, I didn't mean to have the "checkout" in that.
$ git -C linux-stable/ worktree add $PWD/tmp/ linux-4.15.y



That should leave linux-4.16.y checked out in linux-stable, while
creating a full work tree in $PWD/tmp that has 4.15.y checked out.

Note that worktree is a newer git command. 2.17 has it, but old
versions like 2.1 won't.

[1] https://git-scm.com/docs/git-worktree

Hope this helps!


For sure, it helps! Thanks.

- Thierry

Hello

[Ali Mohamed]

Good Day To You,

Am Mr.Ali.Please i need your kind Assistance. I will be very glad if 
you can assist me to receive this sum of ( $22. Million US dollars.) 
into your bank account for the benefit of our both families, reply me 
if you are ready to receive this fund.

Thanks

Re: Errors testing on macOS High Sierra version 10.13.4

[Wink Saville]

On Thu, Apr 5, 2018 at 2:33 PM, Eric Sunshine  wrote:
> On Wed, Apr 4, 2018 at 1:06 PM, Wink Saville  wrote:
>> I built git on a mac osx laptop and got some errors when testing.
>> I ran ./ci/run-build-and-tests.sh and three of the tests had failures
>> that appear to be associated with character encoding:
>> ...
>> Of course on travis-ci there are no failures so I dug deeper and found
>> that travis-ci is running 10.12.6 (I added a call to system_profier in
>> ci/run-build-and-tests.sh) where as I'm running is 10.13.4:
>>
>> Not sure, but maybe I've got something configured incorrectly.
>> Suggestions anyone?
>
> I'm still on 10.12.6 and I don't plan on upgrading, so you may need to
> dig into this yourself.
>
> Try narrowing down the problem to the exact command within the test
> which is failing or giving unexpected results. From there, it may be
> possible to identify some difference between 10.12.6 and 10.13.4 or
> between something in your current configuration and that on Travis or
> elsewhere.

OK

Re: [PATCH v2 2/4] push: colorize errors

[Jacob Keller]

On Thu, Apr 5, 2018 at 3:48 PM, Johannes Schindelin
 wrote:
> From: Ryan Dammrose 
>
> This is an attempt to resolve an issue I experience with people that are
> new to Git -- especially colleagues in a team setting -- where they miss
> that their push to a remote location failed because the failure and
> success both return a block of white text.
>
> An example is if I push something to a remote repository and then a
> colleague attempts to push to the same remote repository and the push
> fails because it requires them to pull first, but they don't notice
> because a success and failure both return a block of white text. They
> then continue about their business, thinking it has been successfully
> pushed.
>
> This patch colorizes the errors and hints (in red and yellow,
> respectively) so whenever there is a failure when pushing to a remote
> repository that fails, it is more noticeable.
>
> [jes: fixed a couple bugs, added the color.{advice,push,transport}
> settings, refactored to use want_color_stderr().]
>
> Signed-off-by: Ryan Dammrose ryandammr...@gmail.com
> Signed-off-by: Johannes Schindelin 
>
> squash! push: colorize errors
>
> Stop talking about localized errors

Guessing you intended to remove this part after squashing?

Didn't see anything else to comment on in the actual code.

Thanks,
Jake

Re: Self-inflicted "abort" in a newbie attempt at read-only exploration of a cloned repository?

[Bryan Turner]

On Thu, Apr 5, 2018 at 4:18 PM, Bryan Turner  wrote:
> On Thu, Apr 5, 2018 at 12:42 PM, Thierry Moreau
>  wrote:
>> Dear GIT enthusiasts!
>>
>> This ends up with a "git checkout" command aborting. A bit frustrating at
>> the early stage of GIT learning curve.
>>
>> My first goal is to clone repositories locally in order to explore the
>> various linux kernel versions, with the rich GIT metadata.
>>
>> Thus, I clone:
>>
>> $  git clone --branch linux-4.16.y
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
>> linux-stable
>> $  git -C linux-stable/ branch
>> * linux-4.16.y
>>
>> So far so good. Then, I want to extract an earlier kernel version into a tmp
>> dir:
>>
>> $  mkdir tmp
>> $  git -C linux-stable/ --work-tree $PWD/tmp/ checkout linux-4.15.y
>> $  git -C linux-stable/ branch
>> * linux-4.15.y
>>   linux-4.16.y
>
> The documentation for --work-tree says:
>
> --work-tree=
>
> Set the path to the working tree. It can be an absolute path or a path
> relative to the current working directory. This can also be controlled
> by setting the GIT_WORK_TREE environment variable and the
> core.worktree configuration variable (see core.worktree in
> git-config(1) for a more detailed discussion).
>
> So passing --work-tree tells Git where to store your _files_, but it's
> still using the same .git directory.
>
> If your goal is to have worktrees for various versions, that implies
> the git worktree [1] command might be more along the lines of what
> you're looking for. An invocation based on above might look like this:
> $ git -C linux-stable/ worktree add $PWD/tmp/ checkout linux-4.15.y

Apologies, I didn't mean to have the "checkout" in that.
$ git -C linux-stable/ worktree add $PWD/tmp/ linux-4.15.y

>
> That should leave linux-4.16.y checked out in linux-stable, while
> creating a full work tree in $PWD/tmp that has 4.15.y checked out.
>
> Note that worktree is a newer git command. 2.17 has it, but old
> versions like 2.1 won't.
>
> [1] https://git-scm.com/docs/git-worktree
>
> Hope this helps!
> Bryan

Re: [RFC PATCH 0/1] bdl-lib.sh: add bash debug logger

[Wink Saville]

On Thu, Apr 5, 2018 at 12:32 PM, Johannes Schindelin
 wrote:
> Hi Wink,

> Note: I have nothing to do with including it. That is the sole discretion
> of Junio (who is offline for a week or two, if I understood correctly).
>
> Ciao,
> Johannes

txs, I understand.

Re: Self-inflicted "abort" in a newbie attempt at read-only exploration of a cloned repository?

[Bryan Turner]

On Thu, Apr 5, 2018 at 12:42 PM, Thierry Moreau
 wrote:
> Dear GIT enthusiasts!
>
> This ends up with a "git checkout" command aborting. A bit frustrating at
> the early stage of GIT learning curve.
>
> My first goal is to clone repositories locally in order to explore the
> various linux kernel versions, with the rich GIT metadata.
>
> Thus, I clone:
>
> $  git clone --branch linux-4.16.y
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
> linux-stable
> $  git -C linux-stable/ branch
> * linux-4.16.y
>
> So far so good. Then, I want to extract an earlier kernel version into a tmp
> dir:
>
> $  mkdir tmp
> $  git -C linux-stable/ --work-tree $PWD/tmp/ checkout linux-4.15.y
> $  git -C linux-stable/ branch
> * linux-4.15.y
>   linux-4.16.y

The documentation for --work-tree says:

--work-tree=

Set the path to the working tree. It can be an absolute path or a path
relative to the current working directory. This can also be controlled
by setting the GIT_WORK_TREE environment variable and the
core.worktree configuration variable (see core.worktree in
git-config(1) for a more detailed discussion).

So passing --work-tree tells Git where to store your _files_, but it's
still using the same .git directory.

If your goal is to have worktrees for various versions, that implies
the git worktree [1] command might be more along the lines of what
you're looking for. An invocation based on above might look like this:
$ git -C linux-stable/ worktree add $PWD/tmp/ checkout linux-4.15.y

That should leave linux-4.16.y checked out in linux-stable, while
creating a full work tree in $PWD/tmp that has 4.15.y checked out.

Note that worktree is a newer git command. 2.17 has it, but old
versions like 2.1 won't.

[1] https://git-scm.com/docs/git-worktree

Hope this helps!
Bryan

Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier

[Jeff King]

On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:

> On Thu, Apr 5, 2018 at 6:36 PM, Jeff King  wrote:
> > On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
> >> + if (type == TYPE_COLOR) {
> >> + char v[COLOR_MAXLEN];
> >> + if (!git_config_color(v, key, value))
> >> + /*
> >> +  * The contents of `v` now contain an ANSI escape
> >> +  * sequence, not suitable for including within a
> >> +  * configuration file. Treat the above as a
> >> +  * "sanity-check", and return the given value, which 
> >> we
> >> +  * know is representable as valid color code.
> >> +  */
> >> + return xstrdup(value);
> >> + die("cannot parse color '%s'", value);
> >> + }
> >
> > And this returns the original. Good.
> 
> It's entirely subjective, but I find this easier to read and
> understand when written like this:
> 
> char v[COLOR_MAXLEN];
> if (git_config_color(v, key, value))
> die("cannot parse color '%s'", value);
> 
> /*
>  * The contents of `v` now contain an ANSI escape
>  * sequence, not suitable for including within a
>  * configuration file. Treat the above as a
>  * "sanity-check", and return the given value, which we
>  * know is representable as valid color code.
> */
> return xstrdup(value);

It may be subjective, but I happen to agree with you.

-Peff

Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier

[Eric Sunshine]

On Thu, Apr 5, 2018 at 6:36 PM, Jeff King  wrote:
> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
>> + if (type == TYPE_COLOR) {
>> + char v[COLOR_MAXLEN];
>> + if (!git_config_color(v, key, value))
>> + /*
>> +  * The contents of `v` now contain an ANSI escape
>> +  * sequence, not suitable for including within a
>> +  * configuration file. Treat the above as a
>> +  * "sanity-check", and return the given value, which we
>> +  * know is representable as valid color code.
>> +  */
>> + return xstrdup(value);
>> + die("cannot parse color '%s'", value);
>> + }
>
> And this returns the original. Good.

It's entirely subjective, but I find this easier to read and
understand when written like this:

char v[COLOR_MAXLEN];
if (git_config_color(v, key, value))
die("cannot parse color '%s'", value);

/*
 * The contents of `v` now contain an ANSI escape
 * sequence, not suitable for including within a
 * configuration file. Treat the above as a
 * "sanity-check", and return the given value, which we
 * know is representable as valid color code.
*/
return xstrdup(value);

[PATCH v2 1/4] color: introduce support for colorizing stderr

[Johannes Schindelin]

So far, we only ever asked whether stdout wants to be colorful. In the
upcoming patches, we will want to make push errors more prominent, which
are printed to stderr, though.

So let's refactor the want_color() function into a want_color_fd()
function (which expects to be called with fd == 1 or fd == 2 for stdout
and stderr, respectively), and then define the macro `want_color()` to
use the want_color_fd() function.

And then also add a macro `want_color_stderr()`, for convenience and
for documentation.

Signed-off-by: Johannes Schindelin 
---
 color.c | 20 +++-
 color.h |  4 +++-
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/color.c b/color.c
index f277e72e4ce..c6c6c4f580f 100644
--- a/color.c
+++ b/color.c
@@ -319,18 +319,20 @@ int git_config_colorbool(const char *var, const char 
*value)
return GIT_COLOR_AUTO;
 }
 
-static int check_auto_color(void)
+static int check_auto_color(int fd)
 {
-   if (color_stdout_is_tty < 0)
-   color_stdout_is_tty = isatty(1);
-   if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
+   static int color_stderr_is_tty = -1;
+   int *is_tty_p = fd == 1 ? _stdout_is_tty : _stderr_is_tty;
+   if (*is_tty_p < 0)
+   *is_tty_p = isatty(fd);
+   if (*is_tty_p || (fd == 1 && pager_in_use() && pager_use_color)) {
if (!is_terminal_dumb())
return 1;
}
return 0;
 }
 
-int want_color(int var)
+int want_color_fd(int fd, int var)
 {
/*
 * NEEDSWORK: This function is sometimes used from multiple threads, and
@@ -339,15 +341,15 @@ int want_color(int var)
 * is listed in .tsan-suppressions for the time being.
 */
 
-   static int want_auto = -1;
+   static int want_auto[3] = { -1, -1, -1 };
 
if (var < 0)
var = git_use_color_default;
 
if (var == GIT_COLOR_AUTO) {
-   if (want_auto < 0)
-   want_auto = check_auto_color();
-   return want_auto;
+   if (want_auto[fd] < 0)
+   want_auto[fd] = check_auto_color(fd);
+   return want_auto[fd];
}
return var;
 }
diff --git a/color.h b/color.h
index cd0bcedd084..5b744e1bc68 100644
--- a/color.h
+++ b/color.h
@@ -88,7 +88,9 @@ int git_config_colorbool(const char *var, const char *value);
  * Return a boolean whether to use color, where the argument 'var' is
  * one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO.
  */
-int want_color(int var);
+int want_color_fd(int fd, int var);
+#define want_color(colorbool) want_color_fd(1, (colorbool))
+#define want_color_stderr(colorbool) want_color_fd(2, (colorbool))
 
 /*
  * Translate a Git color from 'value' into a string that the terminal can
-- 
2.17.0.windows.1.4.g7e4058d72e3

[PATCH v2 2/4] push: colorize errors

[Johannes Schindelin]

From: Ryan Dammrose 

This is an attempt to resolve an issue I experience with people that are
new to Git -- especially colleagues in a team setting -- where they miss
that their push to a remote location failed because the failure and
success both return a block of white text.

An example is if I push something to a remote repository and then a
colleague attempts to push to the same remote repository and the push
fails because it requires them to pull first, but they don't notice
because a success and failure both return a block of white text. They
then continue about their business, thinking it has been successfully
pushed.

This patch colorizes the errors and hints (in red and yellow,
respectively) so whenever there is a failure when pushing to a remote
repository that fails, it is more noticeable.

[jes: fixed a couple bugs, added the color.{advice,push,transport}
settings, refactored to use want_color_stderr().]

Signed-off-by: Ryan Dammrose ryandammr...@gmail.com
Signed-off-by: Johannes Schindelin 

squash! push: colorize errors

Stop talking about localized errors
---
 advice.c   | 49 ++--
 builtin/push.c | 44 -
 config.c   |  2 +-
 transport.c| 67 +-
 4 files changed, 157 insertions(+), 5 deletions(-)

diff --git a/advice.c b/advice.c
index 406efc183ba..2121c1811fd 100644
--- a/advice.c
+++ b/advice.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 
 int advice_push_update_rejected = 1;
 int advice_push_non_ff_current = 1;
@@ -20,6 +21,33 @@ int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
 
+static int advice_use_color = -1;
+static char advice_colors[][COLOR_MAXLEN] = {
+   GIT_COLOR_RESET,
+   GIT_COLOR_YELLOW,   /* HINT */
+};
+
+enum color_advice {
+   ADVICE_COLOR_RESET = 0,
+   ADVICE_COLOR_HINT = 1,
+};
+
+static int parse_advise_color_slot(const char *slot)
+{
+   if (!strcasecmp(slot, "reset"))
+   return ADVICE_COLOR_RESET;
+   if (!strcasecmp(slot, "advice"))
+   return ADVICE_COLOR_HINT;
+   return -1;
+}
+
+static const char *advise_get_color(enum color_advice ix)
+{
+   if (want_color_stderr(advice_use_color))
+   return advice_colors[ix];
+   return "";
+}
+
 static struct {
const char *name;
int *preference;
@@ -59,7 +87,10 @@ void advise(const char *advice, ...)
 
for (cp = buf.buf; *cp; cp = np) {
np = strchrnul(cp, '\n');
-   fprintf(stderr, _("hint: %.*s\n"), (int)(np - cp), cp);
+   fprintf(stderr, _("%shint: %.*s%s\n"),
+   advise_get_color(ADVICE_COLOR_HINT),
+   (int)(np - cp), cp,
+   advise_get_color(ADVICE_COLOR_RESET));
if (*np)
np++;
}
@@ -68,9 +99,23 @@ void advise(const char *advice, ...)
 
 int git_default_advice_config(const char *var, const char *value)
 {
-   const char *k;
+   const char *k, *slot_name;
int i;
 
+   if (!strcmp(var, "color.advice")) {
+   advice_use_color = git_config_colorbool(var, value);
+   return 0;
+   }
+
+   if (skip_prefix(var, "color.advice.", _name)) {
+   int slot = parse_advise_color_slot(slot_name);
+   if (slot < 0)
+   return 0;
+   if (!value)
+   return config_error_nonbool(var);
+   return color_parse(value, advice_colors[slot]);
+   }
+
if (!skip_prefix(var, "advice.", ))
return 0;
 
diff --git a/builtin/push.c b/builtin/push.c
index 013c20d6164..ac3705370e1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -12,12 +12,40 @@
 #include "submodule.h"
 #include "submodule-config.h"
 #include "send-pack.h"
+#include "color.h"
 
 static const char * const push_usage[] = {
N_("git push [] [ [...]]"),
NULL,
 };
 
+static int push_use_color = -1;
+static char push_colors[][COLOR_MAXLEN] = {
+   GIT_COLOR_RESET,
+   GIT_COLOR_RED,  /* ERROR */
+};
+
+enum color_push {
+   PUSH_COLOR_RESET = 0,
+   PUSH_COLOR_ERROR = 1
+};
+
+static int parse_push_color_slot(const char *slot)
+{
+   if (!strcasecmp(slot, "reset"))
+   return PUSH_COLOR_RESET;
+   if (!strcasecmp(slot, "error"))
+   return PUSH_COLOR_ERROR;
+   return -1;
+}
+
+static const char *push_get_color(enum color_push ix)
+{
+   if (want_color_stderr(push_use_color))
+   return push_colors[ix];
+   return "";
+}
+
 static int thin = 1;
 static int deleterefs;
 static const char *receivepack;
@@ -337,8 +365,11 @@ static int push_with_options(struct transport *transport, 
int flags)
fprintf(stderr, _("Pushing to 

[PATCH v2 3/4] Add a test to verify that push errors are colorful

[Johannes Schindelin]

This actually only tests whether the push errors/hints are colored if
the respective color.* config settings are `always`, but in the regular
case they default to `auto` (in which case we color the messages when
stderr is connected to an interactive terminal), therefore these tests
should suffice.

Signed-off-by: Johannes Schindelin 
---
 t/t5541-http-push-smart.sh | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 21340e89c96..1c2be98dc2b 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '
grep "^To $HTTPD_URL/smart/test_repo.git" status
 '
 
+test_expect_success 'colorize errors/hints' '
+   cd "$ROOT_PATH"/test_repo_clone &&
+   cat >exp <<-EOF &&
+   To http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
+! [rejected]origin/master^ -> master 
(non-fast-forward)
+   error: failed to push some refs to 
'\''http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'\''
+   EOF
+
+   test_must_fail git -c color.transport=always -c color.advice=always \
+   -c color.push=always \
+   push origin origin/master^:master 2>act &&
+   test_decode_color decoded &&
+   test_i18ngrep ".*rejected.*" decoded &&
+   test_i18ngrep "error: failed to push some refs" decoded &&
+   test_i18ngrep "hint: " decoded &&
+   test_i18ngrep ! "^hint: " decoded
+'
+
 stop_httpd
 test_done
-- 
2.17.0.windows.1.4.g7e4058d72e3

[PATCH v2 4/4] Document the new color.* settings to colorize push errors/hints

[Johannes Schindelin]

Let's make it easier for users to find out how to customize these colors.

Signed-off-by: Johannes Schindelin 
---
 Documentation/config.txt | 28 
 1 file changed, 28 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e0cff87f62..40e3828b85f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1088,6 +1088,16 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n.   Defaults to true.
 
+color.advice::
+   A boolean to enable/disable color in hints (e.g. when a push
+   failed, see `advice.*` for a list).  May be set to `always`,
+   `false` (or `never`) or `auto` (or `true`), in which case colors
+   are used only when the error output goes to a terminal. If
+   unset, then the value of `color.ui` is used (`auto` by default).
+
+color.advice.advice::
+   Use customized color for hints.
+
 color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
@@ -1190,6 +1200,15 @@ color.pager::
A boolean to enable/disable colored output when the pager is in
use (default is true).
 
+color.push::
+   A boolean to enable/disable color in push errors. May be set to
+   `always`, `false` (or `never`) or `auto` (or `true`), in which
+   case colors are used only when the error output goes to a terminal.
+   If unset, then the value of `color.ui` is used (`auto` by default).
+
+color.push.error::
+   Use customized color for push errors.
+
 color.showBranch::
A boolean to enable/disable color in the output of
linkgit:git-show-branch[1]. May be set to `always`,
@@ -1218,6 +1237,15 @@ color.status.::
status short-format), or
`unmerged` (files which have unmerged changes).
 
+color.transport::
+   A boolean to enable/disable color when pushes are rejected. May be
+   set to `always`, `false` (or `never`) or `auto` (or `true`), in which
+   case colors are used only when the error output goes to a terminal.
+   If unset, then the value of `color.ui` is used (`auto` by default).
+
+color.transport.rejected::
+   Use customized color when a push was rejected.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
-- 
2.17.0.windows.1.4.g7e4058d72e3

[PATCH v2 0/4] Colorize push errors

[Johannes Schindelin]

To help users discern large chunks of white text (when the push
succeeds) from large chunks of white text (when the push fails), let's
add some color to the latter.

The original contribution came in via Pull Request from the Git for
Windows project:

https://github.com/git-for-windows/git/pull/1429

Changes since v1:

- implemented want_color_fd() as suggested by Junio

- added color.{advice,push,transport} to be able to force this thing on

- added a test

- added documentation to `git config`'s man page

- fixed a bug where the push config looked at color.advice.

- fixed a bug where `color.advise.` was not parsed because
  git_default_config() would fail to hand `color.advise.*` settings to
  git_default_advice_config()

- wrapped a couple of too-long lines

- changed the strstr() hack to detect push failures to use push_had_errors()
  instead

- renamed the transport.color.* settings to color.transport.* (to make them
  consistent with all the other color.. settings)


Johannes Schindelin (3):
  color: introduce support for colorizing stderr
  Add a test to verify that push errors are colorful
  Document the new color.* settings to colorize push errors/hints

Ryan Dammrose (1):
  push: colorize errors

 Documentation/config.txt   | 28 
 advice.c   | 49 ++--
 builtin/push.c | 44 -
 color.c| 20 +++-
 color.h|  4 ++-
 config.c   |  2 +-
 t/t5541-http-push-smart.sh | 18 ++
 transport.c| 67 +-
 8 files changed, 217 insertions(+), 15 deletions(-)


base-commit: 468165c1d8a442994a825f3684528361727cd8c0
Published-As: https://github.com/dscho/git/releases/tag/colorize-push-errors-v2
Fetch-It-Via: git fetch https://github.com/dscho/git colorize-push-errors-v2

Interdiff vs v1:
 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 4e0cff87f62..40e3828b85f 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1088,6 +1088,16 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n.   Defaults to true.
  
 +color.advice::
 +  A boolean to enable/disable color in hints (e.g. when a push
 +  failed, see `advice.*` for a list).  May be set to `always`,
 +  `false` (or `never`) or `auto` (or `true`), in which case colors
 +  are used only when the error output goes to a terminal. If
 +  unset, then the value of `color.ui` is used (`auto` by default).
 +
 +color.advice.advice::
 +  Use customized color for hints.
 +
  color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
 @@ -1190,6 +1200,15 @@ color.pager::
A boolean to enable/disable colored output when the pager is in
use (default is true).
  
 +color.push::
 +  A boolean to enable/disable color in push errors. May be set to
 +  `always`, `false` (or `never`) or `auto` (or `true`), in which
 +  case colors are used only when the error output goes to a terminal.
 +  If unset, then the value of `color.ui` is used (`auto` by default).
 +
 +color.push.error::
 +  Use customized color for push errors.
 +
  color.showBranch::
A boolean to enable/disable color in the output of
linkgit:git-show-branch[1]. May be set to `always`,
 @@ -1218,6 +1237,15 @@ color.status.::
status short-format), or
`unmerged` (files which have unmerged changes).
  
 +color.transport::
 +  A boolean to enable/disable color when pushes are rejected. May be
 +  set to `always`, `false` (or `never`) or `auto` (or `true`), in which
 +  case colors are used only when the error output goes to a terminal.
 +  If unset, then the value of `color.ui` is used (`auto` by default).
 +
 +color.transport.rejected::
 +  Use customized color when a push was rejected.
 +
  color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
 diff --git a/advice.c b/advice.c
 index 42460877ef6..2121c1811fd 100644
 --- a/advice.c
 +++ b/advice.c
 @@ -43,7 +43,7 @@ static int parse_advise_color_slot(const char *slot)
  
  static const char *advise_get_color(enum color_advice ix)
  {
 -  if (want_color(advice_use_color))
 +  if (want_color_stderr(advice_use_color))
return advice_colors[ix];
return "";
  }
 @@ -87,8 +87,10 @@ void advise(const char *advice, ...)
  
for (cp = buf.buf; *cp; cp = np) {
np = strchrnul(cp, '\n');
 -  fprintf(stderr, _("%shint: %.*s%s\n"), 
advise_get_color(ADVICE_COLOR_HINT),
 -  (int)(np - cp), cp, 
advise_get_color(ADVICE_COLOR_RESET));
 +  fprintf(stderr, _("%shint: %.*s%s\n"),
 +  

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

[Jeff King]

On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:

> > +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.
> > @@ -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 ::
> > +  'git config' will ensure that any input output is valid under the given 
> > type
> > +  constraint(s), and will canonicalize outgoing values in ``'s 
> > canonical
> > +  form.
> 
> In response to my question[2] about whether the typesetting "[type]"
> meant that it was optional, you responded[1] that it was not, thus
> correctly changed the typesetting to "". However...

Right, "--type" without a specifier means nothing (also, I missed this
in my review, but "input output" in the quoted text?)

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are 
> > given precedence' '
> > +test_expect_success '--no-type unsets type specifiers' '
> > +   echo "10" > expect &&
> > +   git config --type=bool --no-type core.number >actual &&
> > +   test_cmp expect actual
> > +'
> 
> What does --no-type mean and why is it being tested? If this is an
> explicitly supported user-facing option, should it be documented? If
> it's not meant to be user-facing, then why are we enforcing its
> presence and behavior via a test?

It would be the same as if no --type option had been given. The current
documentation says:

  If no type specifier is passed, no checks or transformations are
  performed on the value.

That's retained in the DESCRIPTION section, but it may be worth a
mention of the "--no-type" behavior in the OPTIONS section, too. I
dunno.

-Peff

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau  wrote:
> [...]
> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--type=color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.
> [...]
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -235,6 +235,10 @@ Valid ``'s include:
> +--default value::

This should be typeset as: --default 

> +  When using `--get`, and the requested slot is not found, behave as if value
> +  were the value assigned to the that slot.

And you might use "behaves as if " in the body as well (though
I think Git documentation isn't terribly consistent about typesetting
as "" in the body, so I don't insist upon it).

Re: [PATCH v4 0/3] Teach `--default` to `git-config(1)`

[Jeff King]

On Wed, Apr 04, 2018 at 07:58:45PM -0700, Taylor Blau wrote:

> Hi,
> 
> Attached is a fourth re-roll of my series to add "--default" and
> "--type=color" to "git config" in order to replace:
> 
>   $ git config --get-color  [default]
> 
> with
> 
>   $ git config [--default=] --type=color 

I think this is getting close, but I found a few minor problems to
address.

-Peff

Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier

[Jeff King]

On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:

> As of this commit, the canonical way to retreive an ANSI-compatible
> color escape sequence from a configuration file is with the
> `--get-color` action.
> 
> This is to allow Git to "fall back" on a default value for the color
> should the given section not exist in the specified configuration(s).
> 
> With the addition of `--default`, this is no longer needed since:
> 
>   $ git config --default red --type=color core.section
> 
> will be have exactly as:
> 
>   $ git config --get-color core.section red
> 
> For consistency, let's introduce `--color` and encourage `--type=color`,
> `--default` together over `--get-color` alone.

In this last sentence, did you mean "let's introduce --type=color and
encourage its use with --default over --get-color alone"?

> Signed-off-by: Taylor Blau 
> ---
>  Documentation/git-config.txt | 10 ++
>  builtin/config.c | 21 +
>  t/t1300-repo-config.sh   | 30 ++
>  3 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 620492d1e..bde702d2e 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -38,10 +38,8 @@ 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 <>).
>  
> -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.
> +`color`::
> +The value is taken as an ANSI color escape sequence.

We'd want to keep that introductory paragraph, right? And there is no
`--color`? So I think this hunk can go away (and is presumably a
leftover mistake during rebasing).

>  When reading, the values are read from the system, global and
>  repository local configuration files by default, and options
> @@ -177,6 +175,7 @@ Valid ``'s include:
>~/` 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.
> +- 'color': canonicalize by converting to an ANSI color escape sequence.
>  +

This one is part of the --type list, so that's what we expect.

You may want to also cover the behavior when setting the value (we check
that it's sane, but store the original).

> diff --git a/builtin/config.c b/builtin/config.c
> index 1328b568b..aa3fcabe9 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -61,6 +61,7 @@ static int show_origin;
>  #define TYPE_BOOL_OR_INT 3
>  #define TYPE_PATH4
>  #define TYPE_EXPIRY_DATE 5
> +#define TYPE_COLOR   6

Not strictly necessary for this series, but if this became an enum as
part of the de-bitifying, you wouldn't have to write the numbers
manually. :)

> @@ -203,6 +206,11 @@ static int format_config(struct strbuf *buf, const char 
> *key_, const char *value
>   if (git_config_expiry_date(, key_, value_) < 0)
>   return -1;
>   strbuf_addf(buf, "%"PRItime, t);
> + } else if (type == TYPE_COLOR) {
> + char v[COLOR_MAXLEN];
> + if (git_config_color(v, key_, value_) < 0)
> + return -1;
> + strbuf_addstr(buf, v);
>   } else if (value_) {
>   strbuf_addstr(buf, value_);
>   } else {

OK, formatting shows the converted value. Good.

> @@ -348,6 +356,19 @@ static char *normalize_value(const char *key, const char 
> *value)
>   else
>   return xstrdup(v ? "true" : "false");
>   }
> + if (type == TYPE_COLOR) {
> + char v[COLOR_MAXLEN];
> + if (!git_config_color(v, key, value))
> + /*
> +  * The contents of `v` now contain an ANSI escape
> +  * sequence, not suitable for including within a
> +  * configuration file. Treat the above as a
> +  * "sanity-check", and return the given value, which we
> +  * know is representable as valid color code.
> +  */
> + return xstrdup(value);
> + die("cannot parse color '%s'", value);
> + }

And this returns the original. Good.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index b25ab7b9e..c630bdc77 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh

The tests look good.

-Peff

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Jeff King]

On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:

> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
> *regex_)
>   config_with_options(collect_config, ,
>   _config_source, _options);
>  
> + if (!values.nr && default_value) {
> + struct strbuf *item;
> + ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> + item = [values.nr++];
> + strbuf_init(item, 0);
> + if (format_config(item, key_, default_value) < 0) {
> + exit(1);
> + }
> + }

Calling exit() explicitly is unusual for our code. Usually we would
either die() or propagate the error. Most of the types in
format_config() would die on bogus input, but a few code paths will
return an error.

What happens if a non-default value has a bogus format? E.g.:

  $ git config foo.bar '~NoSuchUser'
  $ git config --path foo.bar
  fatal: failed to expand user dir in: '~NoSuchUser'

Oops. Despite us checking for an error return from
git_config_pathname(), it will always either return 0 or die(). So
that's not a good example. ;)

Let's try expiry-date:

  $ git config foo.bar 'the first of octember'
  $ git config --expiry-date foo.bar
  error: 'the first of octember' for 'foo.bar' is not a valid timestamp
  fatal: bad config line 7 in file .git/config

OK. So we call format_config() there from the actual collect_config()
callback, and the error gets propagated back to the config parser, which
then gives us an informative die(). What happens with your new code:

  $ ./git config --default 'the first of octember' --type=expiry-date 
no.such.key
  error: 'the first of octember' for 'no.such.key' is not a valid timestamp

It's obvious in this toy example, but that config call may be buried
deep in a script. It'd probably be nicer for that exit(1) to be
something like:

  die(_("failed to format default config value"));

> +test_expect_success 'does not allow --default without --get' '
> + test_must_fail git config --default quux --unset a >output 2>&1 &&
> + test_i18ngrep "\-\-default is only applicable to" output
> +'

I think "a" here needs to be "a.section". I get:

  error: key does not contain a section: a

-Peff

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

[Eric Sunshine]

On Wed, Apr 4, 2018 at 10:00 PM, Taylor Blau  wrote:
> [...]
> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> patch to support querying a color value with a default via `--type=color
> --default=`
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -38,12 +38,10 @@ existing values that match the regexp are updated or 
> unset.  If
> +A type specifier may be given as an argument to `--type` to make 'git config'

In [1], you said that the argument to --type is required, so use of
"may be given" here is ambiguous; it makes it sound as if the argument
is optional. Perhaps rewrite something like:

The --type option requests `git config` to ...

Not necessarily worth a re-roll, though. (But if you do need to
re-roll for some reason, it might make sense to combine this series
with the --default series to make it slightly easier to review them
together -- since the one depends upon the other -- and probably ease
the burden on Junio slightly.)

> +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.
> @@ -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 ::
> +  'git config' will ensure that any input output is valid under the given 
> type
> +  constraint(s), and will canonicalize outgoing values in ``'s 
> canonical
> +  form.

In response to my question[2] about whether the typesetting "[type]"
meant that it was optional, you responded[1] that it was not, thus
correctly changed the typesetting to "". However...

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given 
> precedence' '
> +test_expect_success '--no-type unsets type specifiers' '
> +   echo "10" > expect &&
> +   git config --type=bool --no-type core.number >actual &&
> +   test_cmp expect actual
> +'

What does --no-type mean and why is it being tested? If this is an
explicitly supported user-facing option, should it be documented? If
it's not meant to be user-facing, then why are we enforcing its
presence and behavior via a test?

[1]: https://public-inbox.org/git/20180405014758.GA4671@syl.local/
[2]: 
https://public-inbox.org/git/CAPig+cR4uFiC_gFsb2e9JR6SdP-wUQVz-E0MjRHR=vnhd+h...@mail.gmail.com/

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 05:58:00PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:00:34PM -0700, Taylor Blau wrote:
>
> > I have attached a fourth re-roll of my series to introduce
> > "--type=" in "git config", and prefer it to "--".
> >
> > In particular, since the last update, I have changed the following:
> >
> >   - Clearer wording in the second patch per Eric's suggestion.
> >
> >   - Stopped spelling the required argument to "--type=" as "[type]", and
> > instead as "" (cc: Eric).
> >
> >   - Changed "unexpected" to "unrecognized" in the fatal message when we
> > don't know how to interpret the argument to "--type".
>
> This iteration looks good to me, assuming that last-one-wins is still
> the direction we want to go. I'm open to the notion that the cleanup is
> not worth the change in behavior. It is IMHO, but obviously it's
> somewhat subjective.

I am too, unless people on this thread have strong feelings otherwise.

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:12:02PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:
>
> > +test_expect_success '--no-type unsets type specifiers' '
> > +   echo "10" > expect &&
> > +   git config --type=bool --no-type core.number >actual &&
> > +   test_cmp expect actual
> > +'
>
> Actually, one minor nit (not worth a re-roll, but Junio may want to mark
> it up): drop the space in "> expect".

Ack; I thought I picked this one up. I am happy to re-roll it, but maybe
it makes to amend while queueing.

Thanks,
Taylor

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:

> +test_expect_success '--no-type unsets type specifiers' '
> + echo "10" > expect &&
> + git config --type=bool --no-type core.number >actual &&
> + test_cmp expect actual
> +'

Actually, one minor nit (not worth a re-roll, but Junio may want to mark
it up): drop the space in "> expect".

-Peff

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:00:34PM -0700, Taylor Blau wrote:

> I have attached a fourth re-roll of my series to introduce
> "--type=" in "git config", and prefer it to "--".
> 
> In particular, since the last update, I have changed the following:
> 
>   - Clearer wording in the second patch per Eric's suggestion.
> 
>   - Stopped spelling the required argument to "--type=" as "[type]", and
> instead as "" (cc: Eric).
> 
>   - Changed "unexpected" to "unrecognized" in the fatal message when we
> don't know how to interpret the argument to "--type".

This iteration looks good to me, assuming that last-one-wins is still
the direction we want to go. I'm open to the notion that the cleanup is
not worth the change in behavior. It is IMHO, but obviously it's
somewhat subjective.

-Peff

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

[Jeff King]

On Wed, Apr 04, 2018 at 06:53:04PM -0700, Taylor Blau wrote:

> > I understand that you're doing this to avoid complaining about "--int
> > --type=int", but exactly how that case is supported should be an
> > implementation detail; it doesn't need to bleed into the UI as an
> > unnecessary and not-well-justified loosening of an age-old
> > restriction. There are other ways to support "--int --type=int"
> > without "last wins". Also, do we really need to support "--int
> > --type=int"? Is that an expected use-case?
> 
> This is a fair concern, though I view the problem slightly differently.
> I see this change as being motivated by the fact that we are adding
> "--type", not removing an age-old restriction.
> 
> Peff's motivation for this--as I understand it--is that "--type=int"
> should be a _true_ synonym for "--int". Adopting the old-style
> "OPT_SET_BIT" for this purpose makes "--type=int" and "--int" _mostly_ a
> synonym for one another, except that "--type=bool --type=int" will not
> complain, whereas "--bool --int" would.
> 
> Loosening this restriction, in my view, brings us closer (1) to the new
> semantics of multiple "--type"'s, and (2) to the existing semantics of
> "--verbose=1 --verbose=2" as you mentioned above.
> 
> I would like to hear Peff's take on this as well, since he suggested the
> patch originally, and would likely provide the clearest insight into
> this.

I think you've captured it fairly well.  The options _are_ semantically
linked, in that they are all mutually-exclusive types. Obviously we
could continue to flag errors, and even catch "--type=int --type=bool"
in the same way if we really wanted to (by using a custom parse-options
callback).

So I think the primary value here is in the code cleanup. Even without
the new "--type" option, avoiding the bitset makes the code clearer
(IMHO).

I do agree that a user saying "--int --bool" is almost certainly an
error, and they'd be just as happy to see an error message as to get the
last-one-wins behavior. But I also doubt that it comes up very much
either way.

-Peff

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Thu, Apr 5, 2018 at 3:48 PM, Igor Korot  wrote:
> Is support for 10.8 dropped?

Until about a year ago, I was still periodically building Git from
source on MacOS 10.5, and there hasn't been any outright effort to
drop support for older MacOS, so I expect that 10.8 is still supported
by the Git project itself.

However, whether various packagers of (pre-built) Git support 10.8 is
a different matter.

> dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk
>   Referenced from: /usr/local/git/libexec/git-core/git
>   Expected in: /usr/lib/libSystem.B.dylib
> dyld: Symbol not found: ___strlcpy_chk
>   Referenced from: /usr/local/git/libexec/git-core/git
>   Expected in: /usr/lib/libSystem.B.dylib
>
> Now my question is - how I can upgrade the git console client for my
> OSX version?
> It looks like all installers are written for 10.9+ and the only way to
> work it is to update the OS?
>
> Is there a version of the git console app for OSX 10.8?

It's not clear what installer you used? Was it the package from
git-scm? Was it from Homebrew?

I would guess that, even if the git-scm installer no longer supports
10.8, it is likely that Homebrew does. Have you tried it?

If both those options fail, you can always build from source.

Re: Errors testing on macOS High Sierra version 10.13.4

[Eric Sunshine]

On Wed, Apr 4, 2018 at 1:06 PM, Wink Saville  wrote:
> I built git on a mac osx laptop and got some errors when testing.
> I ran ./ci/run-build-and-tests.sh and three of the tests had failures
> that appear to be associated with character encoding:
> ...
> Of course on travis-ci there are no failures so I dug deeper and found
> that travis-ci is running 10.12.6 (I added a call to system_profier in
> ci/run-build-and-tests.sh) where as I'm running is 10.13.4:
>
> Not sure, but maybe I've got something configured incorrectly.
> Suggestions anyone?

I'm still on 10.12.6 and I don't plan on upgrading, so you may need to
dig into this yourself.

Try narrowing down the problem to the exact command within the test
which is failing or giving unexpected results. From there, it may be
possible to identify some difference between 10.12.6 and 10.13.4 or
between something in your current configuration and that on Travis or
elsewhere.

Re: [PATCH] Make running git under other debugger-like programs easy

[Elijah Newren]

Hi Johannes,

On Thu, Apr 5, 2018 at 12:57 PM, Johannes Schindelin
 wrote:

> I wonder whether a better approach would be to add an optional argument to
> that `debug` function (which is intended to have `git` as first argument,
> anyway, or at least a command/function that does not start with a dash):
>
> debug_aux () {
> shift
> "$@" <&6 >&5 2>&7
> }
>
> debug () {
> case "$1" in
> -d)
> shift &&
> GIT_TEST_GDB="$1" debug_aux "$@"
> ;;
> --debugger=*)
> GIT_TEST_GDB="${1#*=}" debug_aux "$@"
> ;;
> *)
> GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
> ;;
> esac
> }
>
> ... and then in wrap-for-bin.sh, we would replace the last lines
>
> if test -n "$GIT_TEST_GDB"
> then
> unset GIT_TEST_GDB
> exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> else
> exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> fi
>
> by
>
> case "$GIT_TEST_GDB" in
> '')
> exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> ;;
> 1)
> unset GIT_TEST_GDB
> exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> ;;
> *)
> GIT_TEST_GDB_$$="$GIT_TEST_GDB"
> unset GIT_TEST_GDB
> exec $GIT_TEST_GDB_$$ "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> ;;
> esac
>
> or some such.

That all looks great to me.  But at this point, it seems like it's a
full rewrite and your patch to submit (which I'd be happy to endorse
in lieu of my own)...or do you want me to submit with you as author
and me as committer?

Also, a side question: if we go this route, do we want to rename
GIT_TEST_GDB to reflect its expanded usage?

> Then your magic "GIT_WRAPPER" invocation would become a bit more explicit:
>
> debug --debugger=nemiver git $ARGS
> debug -d "valgrind --tool=memcheck --track-origins=yes" git $ARGS

No, for most (60-80%?) of my invocations, I wouldn't be able to use
the debug function; only a minority of my uses are from within the
testsuite.  The rest are from the command line (I have
git/bin-wrappers/ in my $PATH), so the above suggestions would mean
that my invocation would become:

GIT_TEST_GDB="nemiver" git $ARGS
GIT_TEST_GDB="valgrind --tool-memcheck --track-origins=yes" git $ARGS

> (In any case, "GIT_WRAPPER" is probably a name in want of being renamed.)

Well, with your suggestion, it'd just be whatever that environment
variable is named.  I'm perfectly happy with something other than
GIT_WRAPPER (or GIT_TEST_GDB).  I'm not so good at coming up with such
myself, but maybe something like GIT_DEBUGGER or GIT_DEBUG_WITH?

Thanks,
Elijah

Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item

[Elijah Newren]

On Thu, Apr 5, 2018 at 12:04 PM, Jeff King  wrote:
> On Thu, Apr 05, 2018 at 11:36:45AM -0700, Elijah Newren wrote:
>
>> > Do we care about matching the name "foo" against the patchspec_item "foo/"?
>> >
>> > That matches now, but wouldn't after your patch.
>>

>> So I should probably make the check handle both cases:
>>
>> @@ -383,8 +383,9 @@ static int match_pathspec_item(const struct
>> pathspec_item *item, int prefix,
>> /* Perform checks to see if "name" is a super set of the pathspec */
>> if (flags & DO_MATCH_LEADING_PATHSPEC) {
>> /* name is a literal prefix of the pathspec */
>> +   int offset = name[namelen-1] == '/' ? 1 : 0;
>> if ((namelen < matchlen) &&
>> -   (match[namelen] == '/') &&
>> +   (match[namelen-offset] == '/') &&
>> !ps_strncmp(item, match, name, namelen))
>> return MATCHED_RECURSIVELY_LEADING_PATHSPEC;
>
> That seems reasonable to me, and your "offset" trick here should prevent
> us from getting confused. Can namelen ever be zero here? I guess
> probably not (I could see an empty pathspec, but an empty path does not
> make sense).

Right, I don't see how an empty path would make sense.

> There are other similar trailing-slash matches in that function, but I'm
> not sure of all the cases in which they're used. I don't know if any of
> those would need similar treatment (sorry for being vague; I expect I'd
> need a few hours to dig into how the pathspec code actually works, and I
> don't have that today).

If it'd only take you a few hours, then you're a lot faster than me.
It took me a while to start wrapping my head around it.

The other trailing-slash matches in the function are all correct,
according to the testsuite.  (I'm not sure I like the
DO_MATCH_DIRECTORY stuff, but it is encoded in tests and backward
compatibility is important.)  In particular, changing the earlier code
to have the same offset trick would make it claim that e.g. either
"a/b" or "a/b/" as names match unconditionally against "a/b/c" as a
pathspec.  We need it to be conditional: we only want that to be
considered a match when checking whether we want to recurse into the
directory for other matches, not when checking whether the directory
itself matches the pathspec.  Thus, it should be behind a separate
flag, in a subsequent check, which is what this series does (namely
with DO_MATCH_LEADING_PATHSPEC).

To be more precise, here is how a matrix of pathnames and pathspecs
would be treated by match_pathspec_item(), where I am abbreviating
names like MATCH_RECURSIVELY_LEADING_PATHSPEC to LEADING):

   Pathspecs
|a/b|a/b/|   a/b/c
  --+---++---
  a/b   |  EXACT| RECURSIVE  |  LEADING[3]
  Names   a/b/  |  EXACT[1] |  EXACT |  LEADING[2]
  a/b/c | RECURSIVE | RECURSIVE  |  EXACT

[1] Only if DO_MATCH_DIRECTORY is passed.  Otherwise,
this is NOT a match at all.
[2] Only if DO_MATCH_LEADING_PATHSPEC is passed,
after applying this series.  Otherwise, not a match
at all.
[3] Without the fix in this thread that you highlighted,
and assuming we apply patch 7, this would actually
mistakenly return RECURSIVE.


Now for a separate question: How much of the above would you like
added to the commit message...or even as a comment in the code to make
it clearer to other folks trying to make sense of it?


Elijah

Re: [PATCH] Make running git under other debugger-like programs easy

[Johannes Schindelin]

Hi Elijah,

On Thu, 5 Apr 2018, Elijah Newren wrote:

> This allows us to run git, when using the script from bin-wrappers, under
> other programs.  A few examples:
>GIT_WRAPPER=nemiver git $ARGS
>GIT_WRAPPER="valgrind --tool=memcheck --track-origins=yes" git $ARGS
> 
> Yes, we already have GIT_TEST_GDB (which could potentially be replaced
> with GIT_WRAPPER="gdb --args"), and a bunch of options for running
> a testcase or multiple testcases under valgrind, but I find the extra
> flexibility useful.

It would be even more useful if it could be made to work interactively,
too, by removing those redirections. The `debug` function does this
thusly:

debug () {
 GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
}

I wonder whether a better approach would be to add an optional argument to
that `debug` function (which is intended to have `git` as first argument,
anyway, or at least a command/function that does not start with a dash):

debug_aux () {
shift
"$@" <&6 >&5 2>&7
}

debug () {
case "$1" in
-d)
shift &&
GIT_TEST_GDB="$1" debug_aux "$@"
;;
--debugger=*)
GIT_TEST_GDB="${1#*=}" debug_aux "$@"
;;
*)
GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
;;
esac
}

... and then in wrap-for-bin.sh, we would replace the last lines

if test -n "$GIT_TEST_GDB"
then
unset GIT_TEST_GDB
exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
else
exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
fi

by

case "$GIT_TEST_GDB" in
'')
exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
;;
1)
unset GIT_TEST_GDB
exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
;;
*)
GIT_TEST_GDB_$$="$GIT_TEST_GDB"
unset GIT_TEST_GDB
exec $GIT_TEST_GDB_$$ "${GIT_EXEC_PATH}/@@PROG@@" "$@"
;;
esac

or some such.

Then your magic "GIT_WRAPPER" invocation would become a bit more explicit:

debug --debugger=nemiver git $ARGS
debug -d "valgrind --tool=memcheck --track-origins=yes" git $ARGS

(In any case, "GIT_WRAPPER" is probably a name in want of being renamed.)

Ciao,
Dscho

Self-inflicted "abort" in a newbie attempt at read-only exploration of a cloned repository?

[Thierry Moreau]

Dear GIT enthusiasts!

This ends up with a "git checkout" command aborting. A bit frustrating 
at the early stage of GIT learning curve.


My first goal is to clone repositories locally in order to explore the 
various linux kernel versions, with the rich GIT metadata.


Thus, I clone:

$  git clone --branch linux-4.16.y 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git 
linux-stable

$  git -C linux-stable/ branch
* linux-4.16.y

So far so good. Then, I want to extract an earlier kernel version into a 
tmp dir:


$  mkdir tmp
$  git -C linux-stable/ --work-tree $PWD/tmp/ checkout linux-4.15.y
$  git -C linux-stable/ branch
* linux-4.15.y
  linux-4.16.y

I got my extracted 4.15 version but the source repository (index? ...?) 
has somehow changed. Let me try something silly:


$  git -C linux-stable/ --work-tree $PWD/tmp/ checkout linux-4.14.y
$  git -C linux-stable/ branch
* linux-4.14.y
  linux-4.15.y
  linux-4.16.y

I indeed switched my extracted version from 4.15 to 4.14, but I am 
puzzled that the local source repository (linux-stable) is modified. 
Then I try to bring it back closer to its original state, just to keep 
things tidy:


$  git -C linux-stable/ checkout linux-4.16.y

And this command aborts, both with Git versions 2.01 and 2.17. Here is 
the truncated command output:


error: Your local changes to the following files would be overwritten by 
checkout:

.gitignore
.mailmap
Documentation/00-INDEX
Documentation/ABI/obsolete/sysfs-gpio
Documentation/ABI/stable/sysfs-bus-vmbus
Documentation/ABI/stable/sysfs-devices
[... ...]
Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
Documentation/devicetree/bindings/arm/omap/crossbar.txt
Documentation/devicetree/bindings/arm/omap/ctrl.txt
Documentation/devicetree/bindings/arm/realtek.txt
Documentation/devicetree/bindings/arm/rock
error: The following untracked working tree files would be overwritten 
by checkout:

Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Diagram.html
Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html
Documentation/RCU/Design/Memory-Ordering/TreeRCU-callback-invocation.svg
Documentation/RCU/Design/Memory-Ordering/TreeRCU-callback-registry.svg
Documentation/RCU/Design/Memory-Ordering/TreeRCU-dyntick.svg
[... ...]
arch/riscv/include/asm/smp.h
arch/riscv/include/asm/spinlock.h
arch/riscv/include/asm/spinlock_types.h
arch/riscv/include/asm/string.h
arch/riscv/include/
Aborting

Questions:
=

Is there a GIT tutorial that begins with my stated goal of an 
extract-only usage of cloned GIT repositories? (Maybe the root cause is 
my reluctance to learn the more involved GIT usages.)


Does the above reproducible abort deserve attention?

Any suggestion for my stated goal?

Thanks in advance,

- Thierry Moreau

Is support for 10.8 dropped?

[Igor Korot]

Hi, ALL,
I am a successful user of git and my project is locad on GitHub (still
in development).

I have console git client installed on  all 3 major platforms -
Windows, Linux and Mac.

Up until recently everything was working fine. However about a month
ago I started experiencing issues with OSX.

I am running OSX 10.8 and initially I was receiving the following:

[quote]
fatal: unable to access
'https://github.com/oneeyeman1/dbhandler.git/': error:1407742E:SSL
routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
[/quote].

After asking on the stackoverflow I received a suggestion of updating
the git application.
I did that successfully) and now am getting the following:

[quote]
MyMac:dbhandler igorkorot$ /usr/local/git/bin/git pull
dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk
  Referenced from: /usr/local/git/libexec/git-core/git
  Expected in: /usr/lib/libSystem.B.dylib
dyld: Symbol not found: ___strlcpy_chk
  Referenced from: /usr/local/git/libexec/git-core/git
  Expected in: /usr/lib/libSystem.B.dylib
error: fetch died of signal 5
[/quote]

Now my question is - how I can upgrade the git console client for my
OSX version?
It looks like all installers are written for 10.9+ and the only way to
work it is to update the OS?

Is there a version of the git console app for OSX 10.8?

Thank you.

Re: [PATCH v3] git-svn: allow empty email-address using authors-prog and authors-file

[Eric Wong]

Andreas Heiduk  wrote:
> Am 05.04.2018 um 09:51 schrieb Eric Wong:
> > Can you confirm it's OK for you?  Thanks.
> 
> Looks good, works for me.
> 
> Do you squash this patch with with my commit or do you need a reroll?

Nope, no need to reroll.  Pushed to my repo for Junio.  Thanks all.

The following changes since commit 468165c1d8a442994a825f3684528361727cd8c0:

  Git 2.17 (2018-04-02 10:13:35 -0700)

are available in the Git repository at:

  git://bogomips.org/git-svn.git svn/authors-prog-2

for you to fetch changes up to cb427e9eb0243fe7a1a22ea3bd0a46b7410c0bf3:

  git-svn: allow empty email-address using authors-prog and authors-file 
(2018-04-05 19:22:06 +)


Andreas Heiduk (2):
  git-svn: search --authors-prog in PATH too
  git-svn: allow empty email-address using authors-prog and authors-file

 Documentation/git-svn.txt   | 13 ++---
 git-svn.perl|  3 ++-
 perl/Git/SVN.pm | 13 ++---
 t/t9130-git-svn-authors-file.sh | 14 ++
 t/t9138-git-svn-authors-prog.sh | 26 +-
 5 files changed, 57 insertions(+), 12 deletions(-)

Re: [RFC PATCH 0/1] bdl-lib.sh: add bash debug logger

[Johannes Schindelin]

Hi Wink,

On Thu, 5 Apr 2018, Wink Saville wrote:

> On Thu, Apr 5, 2018 at 6:37 AM, Johannes Schindelin
>  wrote:
> > After thinking about this more, I am a lot less opposed to including this
> > in Git's source code. However, as it is not necessary for Git's
> > functionality, it should probably go into contrib/, and I would much
> > rather have a more descriptive name such as
> > contrib/bash-debugging-library/...
> 
> I'll move it, thanks for the feed back and considering it for inclusion.

Note: I have nothing to do with including it. That is the sole discretion
of Junio (who is offline for a week or two, if I understood correctly).

Ciao,
Johannes

Re: [RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too

[Jeff King]

On Thu, Apr 05, 2018 at 12:15:59PM -0700, Elijah Newren wrote:

> On Thu, Apr 5, 2018 at 11:58 AM, Jeff King  wrote:
> 
> > It sounds like correct_untracked_entries() is doing the wrong thing, and
> > it should be aware of the pathspec-matching when culling entries. In
> > other words, my understanding was that read_directory() does not
> > necessarily promise to cull fully (which is what led to cf424f5fd in the
> > first place), and callers are forced to apply their own pathspecs.
> >
> > The distinction is academic for this particular bug, but it makes me
> > wonder if there are other cases where "clean" needs to be more careful
> > with what comes out of dir.c.
> 
> Interesting, I read things very differently.  Looking back at commit
> 6b1db43109ab ("clean: teach clean -d to preserve ignored paths",
> 2017-05-23), which introduced correct_untracked_entries(), I thought
> that correct_untracked_entries() wasn't there to correct pathspec
> issues with fill_directory(), but instead to special case the handling
> of files which are both untracked and ignored.  Did I mis-read or were
> there other commits that changed the semantics?
> 
> Also, it would just seem odd to me that fill_directory() requires
> pathspecs, and it uses those pathspecs, but it doesn't guarantee that
> the files it returns matches them.  That seems like an API ripe for
> mis-use, especially since I don't see any comment in the code about
> such an assumption.  Is that really the expectation?

To be honest, I don't know. Most of dir.c predates me, and I've tried to
avoid looking at it too hard. But I had a vague recollection of it being
"best effort", and this bit from cf424f5fd89b reinforces that:

  However, read_directory does not actually check against our pathspec.
  It uses a simplified version that may turn up false positives. As a
  result, we need to check that any hits match our pathspec.

So I don't know that correct_untracked_entries() is there to fix the
pathspec handling. But I think that anybody who looks at the output of
fill_directory() does need to be aware that they may get more entries
than they expected, and has to apply the pathspecs themselves. And
that's what that extra dir_path_match() call in cmd_clean() is
there for (it used to be match_pathspec before some renaming).

I agree it's an error-prone interface. I don't know all the conditions
under which dir.c might return extra entries, but it seems like it might
be sane for it to do a final pathspec-matching pass so that callers
don't have to. That would mean that correct_untracked_entries() sees the
correctly culled list, and the extra check in cmd_clean() could be
dropped.

Ideally, of course, we'd fix those individual cases, since that would be
more efficient. And your patch may be the right first step in that
direction. But since we don't know what all of them are, it seems ripe
for regressions.

-Peff

Re: [RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too

[Elijah Newren]

On Thu, Apr 5, 2018 at 11:58 AM, Jeff King  wrote:

> It sounds like correct_untracked_entries() is doing the wrong thing, and
> it should be aware of the pathspec-matching when culling entries. In
> other words, my understanding was that read_directory() does not
> necessarily promise to cull fully (which is what led to cf424f5fd in the
> first place), and callers are forced to apply their own pathspecs.
>
> The distinction is academic for this particular bug, but it makes me
> wonder if there are other cases where "clean" needs to be more careful
> with what comes out of dir.c.

Interesting, I read things very differently.  Looking back at commit
6b1db43109ab ("clean: teach clean -d to preserve ignored paths",
2017-05-23), which introduced correct_untracked_entries(), I thought
that correct_untracked_entries() wasn't there to correct pathspec
issues with fill_directory(), but instead to special case the handling
of files which are both untracked and ignored.  Did I mis-read or were
there other commits that changed the semantics?

Also, it would just seem odd to me that fill_directory() requires
pathspecs, and it uses those pathspecs, but it doesn't guarantee that
the files it returns matches them.  That seems like an API ripe for
mis-use, especially since I don't see any comment in the code about
such an assumption.  Is that really the expectation?

Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item

[Jeff King]

On Thu, Apr 05, 2018 at 11:36:45AM -0700, Elijah Newren wrote:

> > Do we care about matching the name "foo" against the patchspec_item "foo/"?
> >
> > That matches now, but wouldn't after your patch.
> 
> Technically, the tests pass anyway due to the fallback behavior
> mentioned in the commit message, but this is a really good point.  It
> looks like the call to submodule_path_match() from builtin/grep.c is
> going to be passing name without the trailing '/', which is contrary
> to how read_directory_recursive() in dir.c builds up paths (namely
> with the trailing '/'). If we tried to force consistency (either
> always omit the trailing slash or always include it), then we'd
> probably want to do so for match_pathspec() calls as well, and there
> are lots of those throughout the code and auditing it all looks
> painful.
> 
> So I should probably make the check handle both cases:
> 
> @@ -383,8 +383,9 @@ static int match_pathspec_item(const struct
> pathspec_item *item, int prefix,
> /* Perform checks to see if "name" is a super set of the pathspec */
> if (flags & DO_MATCH_LEADING_PATHSPEC) {
> /* name is a literal prefix of the pathspec */
> +   int offset = name[namelen-1] == '/' ? 1 : 0;
> if ((namelen < matchlen) &&
> -   (match[namelen] == '/') &&
> +   (match[namelen-offset] == '/') &&
> !ps_strncmp(item, match, name, namelen))
> return MATCHED_RECURSIVELY_LEADING_PATHSPEC;

That seems reasonable to me, and your "offset" trick here should prevent
us from getting confused. Can namelen ever be zero here? I guess
probably not (I could see an empty pathspec, but an empty path does not
make sense).

There are other similar trailing-slash matches in that function, but I'm
not sure of all the cases in which they're used. I don't know if any of
those would need similar treatment (sorry for being vague; I expect I'd
need a few hours to dig into how the pathspec code actually works, and I
don't have that today).

-Peff

Re: [RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too

[Jeff King]

On Thu, Apr 05, 2018 at 10:34:43AM -0700, Elijah Newren wrote:

> Even if a directory doesn't match a pathspec, it is possible, depending
> on the precise pathspecs, that some file underneath it might.  So we
> special case and recurse into the directory for such situations.  However,
> we previously always added any untracked directory that we recursed into
> to the list of untracked paths, regardless of whether the directory
> itself matched the pathspec.
> 
> For the case of git-clean and a set of pathspecs of "dir/file" and "more",
> this caused a problem because we'd end up with dir entries for both of
>   "dir"
>   "dir/file"
> Then correct_untracked_entries() would try to helpfully prune duplicates
> for us by removing "dir/file" since it's under "dir", leaving us with
>   "dir"
> Since the original pathspec only had "dir/file", the only entry left
> doesn't match and leaves nothing to be removed.  (Note that if only one
> pathspec was specified, e.g. only "dir/file", then the common_prefix_len
> optimizations in fill_directory would cause us to bypass this problem,
> making it appear in simple tests that we could correctly remove manually
> specified pathspecs.)

It sounds like correct_untracked_entries() is doing the wrong thing, and
it should be aware of the pathspec-matching when culling entries. In
other words, my understanding was that read_directory() does not
necessarily promise to cull fully (which is what led to cf424f5fd in the
first place), and callers are forced to apply their own pathspecs.

The distinction is academic for this particular bug, but it makes me
wonder if there are other cases where "clean" needs to be more careful
with what comes out of dir.c.

-Peff

Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item

[Elijah Newren]

On Thu, Apr 5, 2018 at 10:49 AM, Jeff King  wrote:
>> diff --git a/dir.c b/dir.c
>> index 19212129f0..c915a69385 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -384,7 +384,7 @@ static int match_pathspec_item(const struct 
>> pathspec_item *item, int prefix,
>>   if (flags & DO_MATCH_SUBMODULE) {
>>   /* name is a literal prefix of the pathspec */
>>   if ((namelen < matchlen) &&
>> - (match[namelen] == '/') &&
>> + (match[namelen-1] == '/') &&
>>   !ps_strncmp(item, match, name, namelen))
>>   return MATCHED_RECURSIVELY;
>
> Do we care about matching the name "foo" against the patchspec_item "foo/"?
>
> That matches now, but wouldn't after your patch.

Technically, the tests pass anyway due to the fallback behavior
mentioned in the commit message, but this is a really good point.  It
looks like the call to submodule_path_match() from builtin/grep.c is
going to be passing name without the trailing '/', which is contrary
to how read_directory_recursive() in dir.c builds up paths (namely
with the trailing '/'). If we tried to force consistency (either
always omit the trailing slash or always include it), then we'd
probably want to do so for match_pathspec() calls as well, and there
are lots of those throughout the code and auditing it all looks
painful.

So I should probably make the check handle both cases:

@@ -383,8 +383,9 @@ static int match_pathspec_item(const struct
pathspec_item *item, int prefix,
/* Perform checks to see if "name" is a super set of the pathspec */
if (flags & DO_MATCH_LEADING_PATHSPEC) {
/* name is a literal prefix of the pathspec */
+   int offset = name[namelen-1] == '/' ? 1 : 0;
if ((namelen < matchlen) &&
-   (match[namelen] == '/') &&
+   (match[namelen-offset] == '/') &&
!ps_strncmp(item, match, name, namelen))
return MATCHED_RECURSIVELY_LEADING_PATHSPEC;

Re: [PATCH v3] git-svn: allow empty email-address using authors-prog and authors-file

[Andreas Heiduk]

Am 05.04.2018 um 09:51 schrieb Eric Wong:
> Thanks for the update.  The patch itself looks good, but I
> noticed one --show-item isn't supported on SVN 1.8.10 for me.

--show-item is indeed a 1.9.0 thing:

https://subversion.apache.org/docs/release-notes/1.9.html#svn-info-item

> I've tested the following on both SVN 1.8.10 and 1.9.5:
> 
> --- a/t/t9138-git-svn-authors-prog.sh
> +++ b/t/t9138-git-svn-authors-prog.sh
> @@ -83,7 +83,8 @@ test_expect_success 'authors-prog imported user without 
> email' '
>  test_expect_success 'imported without authors-prog and authors-file' '
>   svn mkdir -m hh --username hh "$svnrepo"/hh &&
>   (
> - uuid=$(svn info --show-item=repos-uuid "$svnrepo") &&
> + uuid=$(svn info "$svnrepo" |
> + sed -n "s/^Repository UUID: //p") &&
>   cd x &&
>   git svn fetch &&
>   git rev-list -1 --pretty=raw refs/remotes/git-svn | \
> 
> Can you confirm it's OK for you?  Thanks.

Looks good, works for me.

Do you squash this patch with with my commit or do you need a reroll?

[PATCH] Make running git under other debugger-like programs easy

[Elijah Newren]

This allows us to run git, when using the script from bin-wrappers, under
other programs.  A few examples:
   GIT_WRAPPER=nemiver git $ARGS
   GIT_WRAPPER="valgrind --tool=memcheck --track-origins=yes" git $ARGS

Yes, we already have GIT_TEST_GDB (which could potentially be replaced
with GIT_WRAPPER="gdb --args"), and a bunch of options for running
a testcase or multiple testcases under valgrind, but I find the extra
flexibility useful.

Signed-off-by: Elijah Newren 
---
 wrap-for-bin.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index 5842408817..1b34d44193 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -25,5 +25,5 @@ then
unset GIT_TEST_GDB
exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
 else
-   exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+   exec ${GIT_WRAPPER} "${GIT_EXEC_PATH}/@@PROG@@" "$@"
 fi
-- 
2.17.0.7.g0b50f94d69

Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item

[Jeff King]

On Thu, Apr 05, 2018 at 10:34:41AM -0700, Elijah Newren wrote:

> For a pathspec like 'foo/bar' comparing against a path named "foo/",
> namelen will be 4, and match[namelen] will be 'b'.  The correct location
> of the directory separator is namelen-1.
> 
> The reason the code worked anyway was that the following code immediately
> checked whether the first matchlen characters matched (which they do) and
> then bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't
> have the ability to check if "name" can be matched as a directory (or
> prefix) against the pathspec.
> 
> Signed-off-by: Elijah Newren 
> ---
>  dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 19212129f0..c915a69385 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -384,7 +384,7 @@ static int match_pathspec_item(const struct pathspec_item 
> *item, int prefix,
>   if (flags & DO_MATCH_SUBMODULE) {
>   /* name is a literal prefix of the pathspec */
>   if ((namelen < matchlen) &&
> - (match[namelen] == '/') &&
> + (match[namelen-1] == '/') &&
>   !ps_strncmp(item, match, name, namelen))
>   return MATCHED_RECURSIVELY;

Do we care about matching the name "foo" against the patchspec_item "foo/"?

That matches now, but wouldn't after your patch.

-Peff

Re: [PATCH 2/2] Documentation: Normalize spelling of 'normalised'

[Stefan Beller]

On Thu, Apr 5, 2018 at 10:20 AM, Elijah Newren  wrote:
> This could be a localization issue, but we had about four dozen
> "normalize"s (or variants, e.g. normalized, renormalize, etc.), and only
> one "normalised" (no other variants), so normalize normalised into
> normalized.

This and the previous patch are
Reviewed-by: Stefan Beller 

> Signed-off-by: Elijah Newren 
> ---
>  Documentation/git-mktree.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
> index c3616e7711..27fe2b32e1 100644
> --- a/Documentation/git-mktree.txt
> +++ b/Documentation/git-mktree.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
>  DESCRIPTION
>  ---
>  Reads standard input in non-recursive `ls-tree` output format, and creates
> -a tree object.  The order of the tree entries is normalised by mktree so
> +a tree object.  The order of the tree entries is normalized by mktree so
>  pre-sorting the input is not required.  The object name of the tree object
>  built is written to the standard output.
>
> --
> 2.17.0.7.g0b50f94d69
>

[RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item

[Elijah Newren]

For a pathspec like 'foo/bar' comparing against a path named "foo/",
namelen will be 4, and match[namelen] will be 'b'.  The correct location
of the directory separator is namelen-1.

The reason the code worked anyway was that the following code immediately
checked whether the first matchlen characters matched (which they do) and
then bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't
have the ability to check if "name" can be matched as a directory (or
prefix) against the pathspec.

Signed-off-by: Elijah Newren 
---
 dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 19212129f0..c915a69385 100644
--- a/dir.c
+++ b/dir.c
@@ -384,7 +384,7 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
if (flags & DO_MATCH_SUBMODULE) {
/* name is a literal prefix of the pathspec */
if ((namelen < matchlen) &&
-   (match[namelen] == '/') &&
+   (match[namelen-1] == '/') &&
!ps_strncmp(item, match, name, namelen))
return MATCHED_RECURSIVELY;
 
-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 0/7] Fix `git clean` with pathspecs

[Elijah Newren]

Sometimes, multiple `git clean $ARGS` invocations (with the exact same
flags and parameters for each invocation) are needed to properly clean
out the desired files.  Sometimes, `git clean $PATHS` just refuses to
clean the files it was explicitly told to clean.  This patch series
aims to address these (very old) problems.

I was made aware of the problems when a user brought to me the
following testcase:
mkdir d{1,2}
touch d{1,2}/ut
touch d1/t
git add d1/t
With this setup, the user would need to run
git clean -ffd */ut
twice to delete both ut files.  Digging further, I found multiple
interesting variants.

However, I am still slightly unsure of what the correct behavior is
supposed to be for one particular case, namely, if the clean command
were instead:
git clean -f '*ut'
(note that the glob is quoted to protect from shell expansion, and
that the -d option was removed), should the files still be cleaned?  I
assumed yes and implemented that in patches 5-6, but the commit message
discusses this case, and patch 7 exists to change the implementation
to answer this question with a 'no'.  Patch 7 should NOT should not be
accepted as-is -- it should either be dropped or squashed into earlier
commits, but which depends on the desired behavior.

Patches 1-2 are almost independent one-line fixes that could be
submitted independently.  However, if we decide to keep the changes
from patch 7, then this series does depend on patch 2 for the tests to
pass.

Patch 3 adds four new testcases covering the variants I noticed.

Patch 4 fixes clean with explicit pathspecs and the -d option.

Patches 5-7 fixes clean with explicit pathspecs without the -d option.

Elijah Newren (7):
  dir.c: Fix typo in comment
  dir.c: fix off-by-one error in match_pathspec_item
  t7300: Add some testcases showing failure to clean specified pathspecs
  dir: Directories should be checked for matching pathspecs too
  dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule
case
  dir: If our pathspec might match files under a dir, recurse into it
  If we do not want globs to recurse into subdirs without -d...

 dir.c| 23 +++
 dir.h|  5 +++--
 t/t7300-clean.sh | 32 
 3 files changed, 50 insertions(+), 10 deletions(-)

-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 1/7] dir.c: Fix typo in comment

[Elijah Newren]

Signed-off-by: Elijah Newren 
---
 dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index dedbf5d476..19212129f0 100644
--- a/dir.c
+++ b/dir.c
@@ -138,7 +138,7 @@ static size_t common_prefix_len(const struct pathspec 
*pathspec)
 * ":(icase)path" is treated as a pathspec full of
 * wildcard. In other words, only prefix is considered common
 * prefix. If the pathspec is abc/foo abc/bar, running in
-* subdir xyz, the common prefix is still xyz, not xuz/abc as
+* subdir xyz, the common prefix is still xyz, not xyz/abc as
 * in non-:(icase).
 */
GUARD_PATHSPEC(pathspec,
-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 5/7] dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule case

[Elijah Newren]

The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE
case are useful for other cases which have nothing to do with submodules.
Rename this constant; a subsequent commit will make use of this change.

Signed-off-by: Elijah Newren 
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index e783431948..b0bca179fd 100644
--- a/dir.c
+++ b/dir.c
@@ -272,7 +272,7 @@ static int do_read_blob(const struct object_id *oid, struct 
oid_stat *oid_stat,
 
 #define DO_MATCH_EXCLUDE   (1<<0)
 #define DO_MATCH_DIRECTORY (1<<1)
-#define DO_MATCH_SUBMODULE (1<<2)
+#define DO_MATCH_LEADING_PATHSPEC (1<<2)
 
 static int match_attrs(const char *name, int namelen,
   const struct pathspec_item *item)
@@ -381,7 +381,7 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
return MATCHED_FNMATCH;
 
/* Perform checks to see if "name" is a super set of the pathspec */
-   if (flags & DO_MATCH_SUBMODULE) {
+   if (flags & DO_MATCH_LEADING_PATHSPEC) {
/* name is a literal prefix of the pathspec */
if ((namelen < matchlen) &&
(match[namelen-1] == '/') &&
@@ -521,7 +521,7 @@ int submodule_path_match(const struct pathspec *ps,
strlen(submodule_name),
0, seen,
DO_MATCH_DIRECTORY |
-   DO_MATCH_SUBMODULE);
+   DO_MATCH_LEADING_PATHSPEC);
return matched;
 }
 
-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too

[Elijah Newren]

Even if a directory doesn't match a pathspec, it is possible, depending
on the precise pathspecs, that some file underneath it might.  So we
special case and recurse into the directory for such situations.  However,
we previously always added any untracked directory that we recursed into
to the list of untracked paths, regardless of whether the directory
itself matched the pathspec.

For the case of git-clean and a set of pathspecs of "dir/file" and "more",
this caused a problem because we'd end up with dir entries for both of
  "dir"
  "dir/file"
Then correct_untracked_entries() would try to helpfully prune duplicates
for us by removing "dir/file" since it's under "dir", leaving us with
  "dir"
Since the original pathspec only had "dir/file", the only entry left
doesn't match and leaves nothing to be removed.  (Note that if only one
pathspec was specified, e.g. only "dir/file", then the common_prefix_len
optimizations in fill_directory would cause us to bypass this problem,
making it appear in simple tests that we could correctly remove manually
specified pathspecs.)

Fix this by actually checking whether the directory we are about to add
to the list of dir entries actually matches the pathspec; only do this
matching check after we have already returned from recursing into the
directory.

Signed-off-by: Elijah Newren 
---
 dir.c| 5 +
 t/t7300-clean.sh | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index c915a69385..e783431948 100644
--- a/dir.c
+++ b/dir.c
@@ -1973,6 +1973,11 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
 check_only, 
stop_at_first_file, pathspec);
if (subdir_state > dir_state)
dir_state = subdir_state;
+
+   if (!match_pathspec(pathspec, path.buf, path.len,
+   0 /* prefix */, NULL,
+   0 /* do NOT special case dirs */))
+   state = path_none;
}
 
if (check_only) {
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 3d260e21ea..31f2d0d8ba 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -677,7 +677,7 @@ test_expect_failure 'git clean handles being told what to 
clean' '
test_path_is_missing d2/ut
 '
 
-test_expect_failure 'git clean handles being told what to clean, with -d' '
+test_expect_success 'git clean handles being told what to clean, with -d' '
mkdir -p d1 d2 &&
touch d1/ut d2/ut &&
git clean -ffd */ut &&
@@ -693,7 +693,7 @@ test_expect_failure 'git clean handles being told a glob to 
clean' '
test_path_is_missing d2/ut
 '
 
-test_expect_failure 'git clean handles being told a glob to clean with -d' '
+test_expect_success 'git clean handles being told a glob to clean with -d' '
mkdir -p d1 d2 &&
touch d1/ut d2/ut &&
git clean -ffd "*ut" &&
-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 6/7] dir: If our pathspec might match files under a dir, recurse into it

[Elijah Newren]

For git clean, if a directory is entirely untracked and the user did not
specify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do
not want to remove that directory and thus do not recurse into it.
However, if the user manually specified specific (or even globbed) paths
somewhere under that directory to remove, then we need to recurse into
the directory to make sure we remove the relevant paths under that
directory as the user requested.

Note that this does not mean that the recursed-into directory will be
added to dir->entries for later removal; as of a few commits earlier in
this series, there is another more strict match check that is run after
returning from a recursed-into directory before deciding to add it to the
list of entries.  Therefore, this will only result in files underneath
the given directory which match one of the pathspecs being added to the
entries list.

Two particular considerations for this patch:

  * If we want to only recurse into a directory when it is specifically
matched rather than matched-via-glob (e.g. '*.c'), then we could do
so via making the final non-zero return in match_pathspec_item be
MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.
(See final patch of this RFC series for details; note that the
relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC and
MATCHED_RECURSIVELY are important for such a change.))

  * There is a growing amount of logic in read_directory_recursive() for
deciding whether to recurse into a subdirectory.  However, there is
a comment immediately preceding this logic that says to recurse if
instructed by treat_path().   It may be better for the logic in
read_directory_recursive() to be moved to treat_path() (or another
function it calls, such as treat_directory()), but I did not feel
strongly about this and just left the logic where it was while
adding to it.  Do others have strong opinions on this?

Signed-off-by: Elijah Newren 
---
 dir.c| 10 ++
 dir.h|  5 +++--
 t/t7300-clean.sh |  4 ++--
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/dir.c b/dir.c
index b0bca179fd..f55e24f149 100644
--- a/dir.c
+++ b/dir.c
@@ -386,7 +386,7 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
if ((namelen < matchlen) &&
(match[namelen-1] == '/') &&
!ps_strncmp(item, match, name, namelen))
-   return MATCHED_RECURSIVELY;
+   return MATCHED_RECURSIVELY_LEADING_PATHSPEC;
 
/* name" doesn't match up to the first wild character */
if (item->nowildcard_len < item->len &&
@@ -403,7 +403,7 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
 * The submodules themselves will be able to perform more
 * accurate matching to determine if the pathspec matches.
 */
-   return MATCHED_RECURSIVELY;
+   return MATCHED_RECURSIVELY_LEADING_PATHSPEC;
}
 
return 0;
@@ -1961,8 +1961,10 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
/* recurse into subdir if instructed by treat_path */
if ((state == path_recurse) ||
((state == path_untracked) &&
-(dir->flags & DIR_SHOW_IGNORED_TOO) &&
-(get_dtype(cdir.de, istate, path.buf, path.len) == 
DT_DIR))) {
+(get_dtype(cdir.de, istate, path.buf, path.len) == 
DT_DIR) &&
+((dir->flags & DIR_SHOW_IGNORED_TOO) ||
+ do_match_pathspec(pathspec, path.buf, path.len,
+   baselen, NULL, 
DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
diff --git a/dir.h b/dir.h
index b0758b82a2..0573f99ae0 100644
--- a/dir.h
+++ b/dir.h
@@ -210,8 +210,9 @@ extern int count_slashes(const char *s);
  * when populating the seen[] array.
  */
 #define MATCHED_RECURSIVELY 1
-#define MATCHED_FNMATCH 2
-#define MATCHED_EXACTLY 3
+#define MATCHED_RECURSIVELY_LEADING_PATHSPEC 2
+#define MATCHED_FNMATCH 3
+#define MATCHED_EXACTLY 4
 extern int simple_length(const char *match);
 extern int no_wildcard(const char *string);
 extern char *common_prefix(const struct pathspec *pathspec);
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 31f2d0d8ba..889b3401e4 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -669,7 +669,7 @@ test_expect_success 'git clean -d skips untracked dirs 
containing ignored files'
test_path_is_missing foo/b/bb
 '
 
-test_expect_failure 'git clean handles being 

[RFC PATCH 7/7] If we do not want globs to recurse into subdirs without -d...

[Elijah Newren]

If folks prefer this behavior, I'll squash this patch into the previous.
Otherwise, I'll just drop this patch from the series.
---
 dir.c| 2 +-
 t/t7300-clean.sh | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index f55e24f149..bad75e9fbd 100644
--- a/dir.c
+++ b/dir.c
@@ -403,7 +403,7 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
 * The submodules themselves will be able to perform more
 * accurate matching to determine if the pathspec matches.
 */
-   return MATCHED_RECURSIVELY_LEADING_PATHSPEC;
+   return MATCHED_RECURSIVELY;
}
 
return 0;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 889b3401e4..913ea6bda3 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -685,12 +685,12 @@ test_expect_success 'git clean handles being told what to 
clean, with -d' '
test_path_is_missing d2/ut
 '
 
-test_expect_success 'git clean handles being told a glob to clean' '
+test_expect_success 'git clean will not recurse with globs without -d' '
mkdir -p d1 d2 &&
touch d1/ut d2/ut &&
git clean -f "*ut" &&
-   test_path_is_missing d1/ut &&
-   test_path_is_missing d2/ut
+   test_path_is_file d1/ut &&
+   test_path_is_file d2/ut
 '
 
 test_expect_success 'git clean handles being told a glob to clean with -d' '
-- 
2.17.0.7.g0b50f94d69

[RFC PATCH 3/7] t7300: Add some testcases showing failure to clean specified pathspecs

[Elijah Newren]

Someone brought me a testcase where multiple git-clean invocations were
required to clean out unwanted files:
  mkdir d{1,2}
  touch d{1,2}/ut
  touch d1/t && git add d1/t
With this setup, the user would need to run
  git clean -ffd */ut
twice to delete both ut files.

A little testing showed some interesting variants:
  * If only one of those two ut files existed (either one), then only one
clean command would be necessary.
  * If both directories had tracked files, then only one git clean would
be necessary to clean both files.
  * If both directories had no tracked files then the clean command above
would never clean either of the untracked files despite the pathspec
explicitly calling both of them out.

A bisect showed that the failure to clean out the files started with
commit cf424f5fd89b ("clean: respect pathspecs with "-d"", 2014-03-10).
However, that pointed to a separate issue: while the "-d" flag was used
by the original user who showed me this problem, that flag should have
been irrelevant to this problem.  Testing again without the "-d" flag
showed that the same buggy behavior exists without using that flag, and
has in fact existed since before cf424f5fd89b.

Add testcases showing that multiple untracked files within entirely
untracked directories cannot be cleaned when specifying these files to
git clean via pathspecs.

Signed-off-by: Elijah Newren 
---
 t/t7300-clean.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 7b36954d63..3d260e21ea 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -669,4 +669,36 @@ test_expect_success 'git clean -d skips untracked dirs 
containing ignored files'
test_path_is_missing foo/b/bb
 '
 
+test_expect_failure 'git clean handles being told what to clean' '
+   mkdir -p d1 d2 &&
+   touch d1/ut d2/ut &&
+   git clean -f */ut &&
+   test_path_is_missing d1/ut &&
+   test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean handles being told what to clean, with -d' '
+   mkdir -p d1 d2 &&
+   touch d1/ut d2/ut &&
+   git clean -ffd */ut &&
+   test_path_is_missing d1/ut &&
+   test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean handles being told a glob to clean' '
+   mkdir -p d1 d2 &&
+   touch d1/ut d2/ut &&
+   git clean -f "*ut" &&
+   test_path_is_missing d1/ut &&
+   test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean handles being told a glob to clean with -d' '
+   mkdir -p d1 d2 &&
+   touch d1/ut d2/ut &&
+   git clean -ffd "*ut" &&
+   test_path_is_missing d1/ut &&
+   test_path_is_missing d2/ut
+'
+
 test_done
-- 
2.17.0.7.g0b50f94d69

[PATCH 2/2] Documentation: Normalize spelling of 'normalised'

[Elijah Newren]

This could be a localization issue, but we had about four dozen
"normalize"s (or variants, e.g. normalized, renormalize, etc.), and only
one "normalised" (no other variants), so normalize normalised into
normalized.

Signed-off-by: Elijah Newren 
---
 Documentation/git-mktree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
index c3616e7711..27fe2b32e1 100644
--- a/Documentation/git-mktree.txt
+++ b/Documentation/git-mktree.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 DESCRIPTION
 ---
 Reads standard input in non-recursive `ls-tree` output format, and creates
-a tree object.  The order of the tree entries is normalised by mktree so
+a tree object.  The order of the tree entries is normalized by mktree so
 pre-sorting the input is not required.  The object name of the tree object
 built is written to the standard output.
 
-- 
2.17.0.7.g0b50f94d69

[PATCH 1/2] Documentation: Fix several one-character-off spelling errors

[Elijah Newren]

Signed-off-by: Elijah Newren 
---
 Documentation/diff-options.txt| 4 ++--
 Documentation/git-fetch-pack.txt  | 2 +-
 Documentation/git-for-each-ref.txt| 2 +-
 Documentation/git-send-email.txt  | 2 +-
 Documentation/git-status.txt  | 2 +-
 Documentation/technical/api-directory-listing.txt | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e3a44f03cd..f466600972 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -568,7 +568,7 @@ the normal order.
 --
 +
 Patterns have the same syntax and semantics as patterns used for
-fnmantch(3) without the FNM_PATHNAME flag, except a pathname also
+fnmatch(3) without the FNM_PATHNAME flag, except a pathname also
 matches a pattern if removing any number of the final pathname
 components matches the pattern.  For example, the pattern "`foo*bar`"
 matches "`fooasdfbar`" and "`foo/bar/baz/asdf`" but not "`foobarx`".
@@ -592,7 +592,7 @@ endif::git-format-patch[]
Treat all files as text.
 
 --ignore-cr-at-eol::
-   Ignore carrige-return at the end of line when doing a comparison.
+   Ignore carriage-return at the end of line when doing a comparison.
 
 --ignore-space-at-eol::
Ignore changes in whitespace at EOL.
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index f7ebe36a7b..c975884793 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -88,7 +88,7 @@ be in a separate packet, and the list must end with a flush 
packet.
infinite even if there is an ancestor-chain that long.
 
 --shallow-since=::
-   Deepen or shorten the history of a shallow'repository to
+   Deepen or shorten the history of a shallow repository to
include all reachable commits after .
 
 --shallow-exclude=::
diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index dffa14a795..085d177d97 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -121,7 +121,7 @@ refname::
stripping with positive , or it becomes the full refname if
stripping with negative .  Neither is an error.
 +
-`strip` can be used as a synomym to `lstrip`.
+`strip` can be used as a synonym to `lstrip`.
 
 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 71ef97ba9b..cd149e7056 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -255,7 +255,7 @@ must be used for each option.
 
 --batch-size=::
Some email servers (e.g. smtp.163.com) limit the number emails to be
-   sent per session (connection) and this will lead to a faliure when
+   sent per session (connection) and this will lead to a failure when
sending many messages.  With this option, send-email will disconnect 
after
sending $ messages and wait for a few seconds (see --relogin-delay)
and reconnect, to work around such a limit.  You may want to
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6c230c0c72..c16e27e63d 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -113,7 +113,7 @@ The possible options are:
- 'matching'- Shows ignored files and directories matching an
  ignore pattern.
 +
-When 'matching' mode is specified, paths that explicity match an
+When 'matching' mode is specified, paths that explicitly match an
 ignored pattern are shown. If a directory matches an ignore pattern,
 then it is shown, but not paths contained in the ignored directory. If
 a directory does not match an ignore pattern, but all contents are
diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 7fae00f44f..4f44ca24f6 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -53,7 +53,7 @@ The notable options are:
not be returned even if all of its contents are ignored. In
this case, the contents are returned as individual entries.
 +
-If this is set, files and directories that explicity match an ignore
+If this is set, files and directories that explicitly match an ignore
 pattern are reported. Implicity ignored directories (directories that
 do not match an ignore pattern, but whose contents are all ignored)
 are not reported, instead all of the contents are reported.
-- 
2.17.0.7.g0b50f94d69

Re: [PATCH] mergetools: add support for guiffy

[David Aguilar]

On Thu, Apr 05, 2018 at 08:55:01AM -0500, Bill Ritcher wrote:
> Add guiffy as difftool and mergetool
> 
> guiffy is available on Windows, Linux, and MacOS
> 
> Signed-off-by: Bill Ritcher 
> ---
>  mergetools/guiffy | 18 ++
>  1 file changed, 18 insertions(+)
>  create mode 100644 mergetools/guiffy
> 
> diff --git a/mergetools/guiffy b/mergetools/guiffy
> new file mode 100644
> index 0..8b23a13c4
> --- /dev/null
> +++ b/mergetools/guiffy
> @@ -0,0 +1,18 @@
> +diff_cmd () {
> + "$merge_tool_path" "$LOCAL" "$REMOTE"
> +}
> +
> +merge_cmd () {
> + if $base_present
> + then
> + "$merge_tool_path" -s "$LOCAL" \
> + "$REMOTE" "$BASE" "$MERGED"
> + else
> + "$merge_tool_path" -m "$LOCAL" \
> + "$REMOTE" "$MERGED"
> + fi
> +}
> +
> +exit_code_trustable () {
> + true
> +}
> -- 
> 2.15.1.windows.2

I tested this on Linux and it works great.  Thanks Bill.

Acked-by: David Aguilar 

cheers,
-- 
David

Re: [PATCH v11 06/10] convert: add 'working-tree-encoding' attribute

[Torsten Bögershausen]

On 01.04.18 15:24, Lars Schneider wrote:
>> TRUE or false are values, but just wrong ones.
>> If this test is removed, the user will see "failed to encode "TRUE" to 
>> "UTF-8",
>> which should give enough information to fix it.
> 
> I see your point. However, I would like to stop the processing right
> there for these invalid values. How about 
> 
>   error(_("true/false are no valid working-tree-encodings"));
> 
> I think that is the most straight forward/helpful error message
> for the enduser (I consider the term "boolean" but dismissed it
> as potentially confusing to folks not familiar with the term).
> 
> OK with you?

Yes.

Another thing that came up recently, independent of your series:

What should happen if a user specifies "UTF-8" and the file
has an UTF-8 encoded BOM ?
I ask because I stumbled over such a file coming from a Windows
which the java compiler under Linux didn't accept.

And because some tools love to put an UTF-8 encoded BOM
into text files.

The clearest thing would be to extend the BOM check in 5/9
to cover UTF-32, UTF-16 and UTF-8.

Are there any plans to do so?

And thanks for the work.

Re: Timing issue in t5570 "daemon log records all attributes"

[Jeff King]

On Wed, Apr 04, 2018 at 11:57:52PM +0200, Jan Palus wrote:

> On 03.04.2018 16:32, Jeff King wrote:
> > I'm tempted to say we should just scrap this test. It was added
> > relatively recently and only shows the fix for a pretty minor bug.
> > It's probably not worth the contortions to make it race-proof.
> 
> Thanks for your reply Jeff.
> 
> It appears race condition in reading/writing daemon.log is not the only issue 
> of
> t5570. On a different machine I've just randomly got:
> 
> t5570-git-daemon.sh[163]: can't create git_daemon_output: Interrupted system 
> call
> 
> which I believe might also be associated with concurrent processing of piped
> data connected with a fact that test restarts daemon few times. I can barely
> wrap my head around it but I guess it's somewhat around "shell still tries to
> read fifo while attempt to create new one is made".

That sounds more like your system doesn't handle EINTR gracefully
(presumably it's getting SIGCLD during the mknod() call). Normally that
would be done by an external program, but is mkfifo perhaps a builtin in
your shell?

If I run t5570 on a loop on a loaded system[1], I can't seem to provoke
any failures, using dash.

-Peff

[1] The script I use is:

  https://github.com/peff/git/blob/meta/stress

which runs the test script over and over in parallel. That's usually
enough to exhibit practical races, since it creates enough load to
show timing effects.

Re: How to undo previously set configuration?

[Jeff King]

On Thu, Apr 05, 2018 at 03:25:25PM +0200, Olaf Hering wrote:

> Am Thu, 05 Apr 2018 13:21:02 +0200
> schrieb Ævar Arnfjörð Bjarmason :
> 
> > I'm assuming you mean something like:
> > [user]
> > # This is an error
> > -email
> 
> Yes. Just some flag to say "whatever value this variable has from
> earlier parsing, forget it in case it really exists". Just like "unset
> PATH" in bash.
> 
> I do not know the git internals, so can not really help with the case.

The general strategy in Git's config is that instead of "unsetting", you
should overwrite with whatever value you _do_ want. So a config option
like sendemail.smtpauth should accept some kind of empty or "none" value
to disable auth.

Most single-value config options should work this way (and if one
doesn't, I'd say that's a bug we should fix).

Multi-valued config (e.g., "remote.*.fetch") is harder, since it's
inherently a list where each new instance adds to the list. We've
discussed there having an empty value reset the list, but it's not
applied consistently.

-Peff

Re: [RFC PATCH 0/1] bdl-lib.sh: add bash debug logger

[Wink Saville]

On Thu, Apr 5, 2018 at 6:37 AM, Johannes Schindelin
 wrote:
> After thinking about this more, I am a lot less opposed to including this
> in Git's source code. However, as it is not necessary for Git's
> functionality, it should probably go into contrib/, and I would much
> rather have a more descriptive name such as
> contrib/bash-debugging-library/...
>
> Ciao,
> Johannes

I'll move it, thanks for the feed back and considering it for inclusion.

-- Wink

[PATCH] mergetools: add support for guiffy

[Bill Ritcher]

Add guiffy as difftool and mergetool

guiffy is available on Windows, Linux, and MacOS

Signed-off-by: Bill Ritcher 
---
 mergetools/guiffy | 18 ++
 1 file changed, 18 insertions(+)
 create mode 100644 mergetools/guiffy

diff --git a/mergetools/guiffy b/mergetools/guiffy
new file mode 100644
index 0..8b23a13c4
--- /dev/null
+++ b/mergetools/guiffy
@@ -0,0 +1,18 @@
+diff_cmd () {
+   "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+   if $base_present
+   then
+   "$merge_tool_path" -s "$LOCAL" \
+   "$REMOTE" "$BASE" "$MERGED"
+   else
+   "$merge_tool_path" -m "$LOCAL" \
+   "$REMOTE" "$MERGED"
+   fi
+}
+
+exit_code_trustable () {
+   true
+}
-- 
2.15.1.windows.2

Re: [RFC PATCH 0/1] bdl-lib.sh: add bash debug logger

[Johannes Schindelin]

Hi Wink,

On Fri, 30 Mar 2018, Wink Saville wrote:

> On Fri, Mar 30, 2018 at 3:34 AM, Johannes Schindelin
>  wrote:
> >
> > On Tue, 27 Mar 2018, Wink Saville wrote:
> >
> >> Add bdl-lib.sh which provides functions to assit in debugging git
> >> shell scripts and tests.
> >
> > Interesting idea. It is Bash-only, though... (and it is not a secret
> > that I want to discourage using Unix shell scripts in Git's production
> > code, they are a decent way to prototype things, but they fall short
> > of being robust and portable in practice, and don't get me started on
> > speed issues).
> >
> > So rather than spending time on making it easier to debug shell
> > scripts, I would love to see us going into the direction of a
> > consistent C source code. I still believe that we can get there, and
> > that the benefits are worth the (huge) effort.
> 
> I totally agree the code base should use primarily one language!

Great!

> But that's not the case now and "bdl" gave me insight into the workings
> of rebase--interactive and I found little guidance on how to debug
> or learn the code base. So I thought I'd see if there was interest
> in what I'd done.

And realistically, requiring Bash is not all that much of a problem when
targeting developers.

> If I were to make it non-bash specific would there be any interest?

After thinking about this more, I am a lot less opposed to including this
in Git's source code. However, as it is not necessary for Git's
functionality, it should probably go into contrib/, and I would much
rather have a more descriptive name such as
contrib/bash-debugging-library/...

Ciao,
Johannes

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

[Johannes Schindelin]

Hi Christian,

[please cull a *lot* more of the quoted mail when you do not reference any
of it... Thanks]

On Thu, 5 Apr 2018, Christian Couder wrote:

> On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb  wrote:
> >
> > [...]
> > +
> > +   revision = info->revision.buf;
> > +
> > +   if (get_oid(revision, >w_commit)) {
> > +   error(_("%s is not a valid reference"), revision);
> > +   free_stash_info(info);
> > +   return -1;
> 
> Maybe:
> 
>free_stash_info(info);
>return error(_("%s is not a valid reference"), revision);
> 
> to save one line and be more consistent with above.

No. The parameter `revision` of the `error()` call is assigned just above
the `if()` block and clearly points into the `info` structure. So you must
not release that `info` before printing the error. The order of statements
is correct.

> > +   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)) {
> > +
> > +   error(_("'%s' is not a stash-like commit"), revision);
> > +   free_stash_info(info);
> > +   return -1;
> 
> Here also.

Yes, here, too, `revision` points at a field inside `info`, so we must not
release it before using it.

> > +   if (info->has_u) {
> > +   if (restore_untracked(>u_tree))
> > +   return error(_("Could not restore untracked files 
> > from stash"));
> > +   }
> 
> Maybe:
> 
>if (info->has_u && restore_untracked(>u_tree))
>return error(_("Could not restore untracked files from 
> stash"));

I agree with this, as it avoids an unncessary indentation level.

> So maybe we can get rid of `result` and have something like:
> 
>if (argc < 1) {
>error(_("at least one argument is required"));
>usage_with_options(git_stash_helper_usage, options);
>}
> 
>if (!strcmp(argv[0], "apply"))
>return apply_stash(argc, argv, prefix);

... except we have to use !!apply_stash() here: apply_stash() probably
returns -1 in case of error (at least that would be consistent with our
coding conventions), and the return value from cmd_*() is handed to exit()
as exit status. The `!!` trick turns any non-zero value into a 1, also
consistent with our coding conventions where we set exit code 1 upon error
in the "business logic".

> 
>error(_("unknown subcommand: %s"), argv[0]);
>usage_with_options(git_stash_helper_usage, options);
> }

Ciao,
Dscho

Re: How to undo previously set configuration?

[Olaf Hering]

Am Thu, 05 Apr 2018 13:21:02 +0200
schrieb Ævar Arnfjörð Bjarmason :

> I'm assuming you mean something like:
> [user]
> # This is an error
> -email

Yes. Just some flag to say "whatever value this variable has from earlier 
parsing, forget it in case it really exists". Just like "unset PATH" in bash.

I do not know the git internals, so can not really help with the case.

Olaf


pgpnLQis2dHKn.pgp
Description: Digitale Signatur von OpenPGP

Great News!!

[Amnesty International]

We have a great news about your E-mail address!!! 

You Won  $950,500.00 USD on Amnesty International 
Kenya online E-mail Promotion. For more details 
about your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Please can i trust you?

[Sgt. Britta Lopez]

Good Day,
How are u doing today ? Apologies! I am a military woman ,seeking
your kind assistance to move the sum of ($7M USD) to you, as far
as i can be assured that my money will be safe in your care until
i complete my service here in Afghanistan and come over next
month.
This is legitimate, and there is no danger involved.If
interested, reply immediately for detailed information.
 
Reply to this email sgt.brittalo...@gmail.com
Regards ,
Sgt. Britta Lopez

RE: How to undo previously set configuration?

[Randall S. Becker]

On April 5, 2018 7:21 AM, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Apr 05 2018, Olaf Hering wrote:
> 
> > Am Thu, 05 Apr 2018 10:42:15 +0200
> > schrieb Ævar Arnfjörð Bjarmason :
> >
> >> I've been meaning to work on this but haven't figured out a good syntax
> for it (suggestions welcome!).
> >
> > Just prefix the knob with something like "no." or "-" or whatever to 
> > indicate
> that it never happened.
> 
> Those wouldn't work, respectively, because:
> 
>  a) For 'no.' there would be no way to override three-level keys,
> because prefixing such a key with "no" would introduce a 4th nesting
> level, which would be incompatible with existing config parsers.
> 
>  b) Similarly a prefix of - dies in git now. Unless I misunderstand
> you. I'm assuming you mean something like:
> 
> [user]
> # This is an error
> -email
> 
> Although I see we don't ignore or error out on:
> 
> [user "-email"]
> foo=bar
> 
>But that's back to problem a), and also looks ugly since you need
>something like the extra foo=bar so we'll pay attention to it.
> 
> Also it's important that the syntax leaves room for item #1 that I mentioned,
> 
> I.e. not just ignore stuff like user.email, but being able to specify where 
> from
> you'd like to ignore that. Sometimes your local sysadmin is overzealous with
> his /etc/gitconfig settings and you'd like to quarantine just that, but pay
> attention to everything else in ~/.gitconfig, or similarly in
> /some/repo/.git/config ignore your usual custom sendemail.* from
> ~/.gitconfig but not /etc/gitconfig, so the semantics can't just be "clear
> existing".
> 
> But of course, you might say that it *should* be a syntax error because if you
> rely on this feature and downgrade, you don't want to suddenly pay
> attention to the sendemail.* config again.
> 
> I think that's an acceptable failure mode, and better than the syntax error,
> because that's exactly what we have now, so this is similar to e.g. the
> conditional include directive which was understood but just copmletely
> ignored by older versions.
> 
> So we're OK with getting different config between versions with new
> releases, but at all costs don't want to have new releases introduce
> constructs that older gits error out on, and let's say hypothetically we
> supported something like:
> 
> [user "-email"]
> [user]
> email = ...
> 
> Even `git config -l` on older version won't show that "user.-email", and it's
> better if older tools at least understand the syntax, even though they don't
> pick up on the magic.

I may be missing something but..

Another completely different approach to "undoing" configurations is to 
consider using git for this. Have a repository set up for your ~ directory, 
ignoring content other than .*, so you would ignore any sub-repositories at 
this level. Then manage your configuration as any other repo.

For configurations that are not user-specific, use in-repository configurations 
instead of system and global, so your undo is also handled by git. However, you 
can version control your /etc directory as well. We do that to detect changes 
(as a practical example, we have /etc/.git with some bits ignored but critical 
things involving rc.d, and the system git configurations are managed content in 
that repository. This implies our Ops team has to use git to make changes - a 
good thing - and 'git status' and 'git log' tells me immediately if someone 
changed something.

Undo becomes a git operation in both situations.

This may be complete OT, but I thought it might help

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Great News!

[Amnesty International]

We have a great news about your E-mail address!!! 

You Won  $950,500.00 USD on Amnesty International 
Kenya online E-mail Promotion. For more details 
about your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Re: How to undo previously set configuration?

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 05 2018, Olaf Hering wrote:

> Am Thu, 05 Apr 2018 10:42:15 +0200
> schrieb Ævar Arnfjörð Bjarmason :
>
>> I've been meaning to work on this but haven't figured out a good syntax for 
>> it (suggestions welcome!).
>
> Just prefix the knob with something like "no." or "-" or whatever to indicate 
> that it never happened.

Those wouldn't work, respectively, because:

 a) For 'no.' there would be no way to override three-level keys,
because prefixing such a key with "no" would introduce a 4th nesting
level, which would be incompatible with existing config parsers.

 b) Similarly a prefix of - dies in git now. Unless I misunderstand
you. I'm assuming you mean something like:

[user]
# This is an error
-email

Although I see we don't ignore or error out on:

[user "-email"]
foo=bar

   But that's back to problem a), and also looks ugly since you need
   something like the extra foo=bar so we'll pay attention to it.

Also it's important that the syntax leaves room for item #1 that I
mentioned,

I.e. not just ignore stuff like user.email, but being able to specify
where from you'd like to ignore that. Sometimes your local sysadmin is
overzealous with his /etc/gitconfig settings and you'd like to
quarantine just that, but pay attention to everything else in
~/.gitconfig, or similarly in /some/repo/.git/config ignore your usual
custom sendemail.* from ~/.gitconfig but not /etc/gitconfig, so the
semantics can't just be "clear existing".

But of course, you might say that it *should* be a syntax error because
if you rely on this feature and downgrade, you don't want to suddenly
pay attention to the sendemail.* config again.

I think that's an acceptable failure mode, and better than the syntax
error, because that's exactly what we have now, so this is similar to
e.g. the conditional include directive which was understood but just
copmletely ignored by older versions.

So we're OK with getting different config between versions with new
releases, but at all costs don't want to have new releases introduce
constructs that older gits error out on, and let's say hypothetically we
supported something like:

[user "-email"]
[user]
email = ...

Even `git config -l` on older version won't show that "user.-email", and
it's better if older tools at least understand the syntax, even though
they don't pick up on the magic.

Re: How to configure sendemail for no-auth?

[astian]

astian:
> Olaf Hering:> My ~/.gitconfig looks like this, because all cloned 
> repositories require these settings:
>> [sendemail]
>> from = Olaf Hering 
>> envelopesender = o...@aepfle.de
>> chainreplyto = false
>> ccover = yes
>> smtpencryption = tls
>> smtpdomain = sender
>> smtppass = smtppass
>> smtpAuth = PLAIN
>> smtpserver = smtp.strato.de
>> smtpuser = smtpuser
>> confirm = always
>> assume8bitEncoding = yes
>> transferEncoding = 8bit
>> 
>> Now there is that one repo that requires this:
>> 
>> [sendemail]
>> from = Olaf Hering 
>> envelopesender = a@b.c
>> smtpserver = otherhost
>> 
>> That "otherhost" does just plain oldstyle unencrypted SMTP.
>> 
>> How do I undo the global sendemail settings for that one repo?
>> Setting the knobs to empty strings does not help:
>> Command unknown: 'AUTH' at /usr/lib/git/git-send-email line 1455.
>> 
>> It seems the global smtpuser is causing the error.
>> 
>> Olaf
> Hm, I remember successfully doing something like this, quite some time ago.
> 
> Couldn't you simply disable smtpEncryption in the .git/config of that one
> repo?  E.g.:
> 
> [sendemail]
>   smtpEncryption = none
> 
> You might also want to take a look at "identities" in the manual.  And related
> to that, there's a patch in this old (never merged) series of mine which might
> be useful since I believe the documentation bug it fixes still exists.
> 
> Cheers.

Err, forgot the link:
https://public-inbox.org/git/xmqqzicc6zbf@gitster.mtv.corp.google.com/T/

Cheers.

Re: How to configure sendemail for no-auth?

[astian]

Olaf Hering:
> My ~/.gitconfig looks like this, because all cloned repositories require 
> these settings:
> [sendemail]
> from = Olaf Hering 
> envelopesender = o...@aepfle.de
> chainreplyto = false
> ccover = yes
> smtpencryption = tls
> smtpdomain = sender
> smtppass = smtppass
> smtpAuth = PLAIN
> smtpserver = smtp.strato.de
> smtpuser = smtpuser
> confirm = always
> assume8bitEncoding = yes
> transferEncoding = 8bit
> 
> Now there is that one repo that requires this:
> 
> [sendemail]
> from = Olaf Hering 
> envelopesender = a@b.c
> smtpserver = otherhost
> 
> That "otherhost" does just plain oldstyle unencrypted SMTP.
> 
> How do I undo the global sendemail settings for that one repo?
> Setting the knobs to empty strings does not help:
> Command unknown: 'AUTH' at /usr/lib/git/git-send-email line 1455.
> 
> It seems the global smtpuser is causing the error.
> 
> Olaf

Hm, I remember successfully doing something like this, quite some time ago.

Couldn't you simply disable smtpEncryption in the .git/config of that one
repo?  E.g.:

[sendemail]
  smtpEncryption = none

You might also want to take a look at "identities" in the manual.  And related
to that, there's a patch in this old (never merged) series of mine which might
be useful since I believe the documentation bug it fixes still exists.

Cheers.

Re: [PATCH] specify encoding for sed command

[SZEDER Gábor]

On Thu, Apr 5, 2018 at 8:53 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, Apr 05 2018, Stephon Harris wrote:
>
>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running 
>> git-completion.bash

Please:
  - prefix the subject line with "completion:".
  - nit: replace "running" with "sourcing".
  - wrap the body of the commit message around 70 characters.
  - sign off your patch; see Documentation/SubmittingPatches and 'git
commit -s'.

>> ---
>>  contrib/completion/git-completion.bash | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index b09c8a23626b4..52a4ab5e2165a 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -282,7 +282,7 @@ __gitcomp ()
>>
>>  # Clear the variables caching builtins' options when (re-)sourcing
>>  # the completion script.
>> -unset $(set |sed -ne 
>> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
>> +unset $(set |LANG=C sed -ne 
>> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

I was wondering how could you see an error message from 'sed', when the
stderr of the whole thing is redirected to /dev/null.  It turns out that
such a redirection doesn't affect the stderr of the commands executed
inside the command substitution:

  $ echo $(echo foo ; echo >&2 error) 2>/dev/null
  error
  foo

Interesting, I didn't know that.

> This is getting closer to the issue than your previous patch, but
> there's still some open questions:
>
> 1) What platform OS / version / sed version is this on?
>
> 2) What's the output from "set" that's causing this error? Do we have an
>isolated test case for that?

A quick internet search suggests that this error message tends to appear
on Macs, when its 'sed' encounters an invalid UTF-8 character in its
input.

> 3) There's other invocations of "sed" in the file, aren't those affected
>as well?

The two 'sed' invocations in _git_stash() are suspect, as they process
the output of 'git stash list', which includes the stashes'
descriptions, which can contain whatever the users wished to store there
(though they would probably get a "Warning: commit message did not
conform to UTF-8" message from 9e83266525 (commit-tree: encourage UTF-8
commit messages., 2006-12-22) when doing so).

The 'sed' invocation in __git_complete_revlist_file() is suspect as
well, as it processes the output of 'git ls-tree'.  Pathnames can
contain anything, and while 'git ls-tree' quotes pathnames with
"unusual" characters, I don't know whether it quotes all characters that
can possibly upset Stephon's 'sed'.

What about 'awk' on Stephon's platform?  We already have one 'awk'
invocation in __git_match_ctag(), which we use for 'git grep ' and
'git log -L:'.  That 'awk' script uses a regexp to match the symbol
name in a ctags file; does any programming language allow such unusual
characters in symbol names?

What about 'sed' and 'awk' scripts that don't use regexps at all?  (I
intend to add such an 'awk' script in a day or two...)

> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
>already do it for our invocation to "git merge".

That would perhaps be the safest thing to do...
But how can we set it for a whole file, when the file is not executed as
a script but sourced into the user's shell environment?

Re: [PATCH] specify encoding for sed command

[Eric Sunshine]

On Thu, Apr 5, 2018 at 2:53 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Thu, Apr 05 2018, Stephon Harris wrote:
>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running 
>> git-completion.bash
>
> This is getting closer to the issue than your previous patch, but
> there's still some open questions:
>
> 1) What platform OS / version / sed version is this on?
>
> 2) What's the output from "set" that's causing this error? Do we have an
>isolated test case for that?
>
> 3) There's other invocations of "sed" in the file, aren't those affected
>as well?
>
> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
>already do it for our invocation to "git merge".

Also, missing Signed-off-by: (see Documentation/SubmittingPatches).

Thanks.

Re: How to undo previously set configuration?

[Olaf Hering]

Am Thu, 05 Apr 2018 10:42:15 +0200
schrieb Ævar Arnfjörð Bjarmason :

> I've been meaning to work on this but haven't figured out a good syntax for 
> it (suggestions welcome!).

Just prefix the knob with something like "no." or "-" or whatever to indicate 
that it never happened.

Olaf


pgpfQgIWe0Wl7.pgp
Description: Digitale Signatur von OpenPGP

From: Mr.Ahmed Owain

[Mr.Ahmed Owain]

Good Day,

Please accept my apologies for writing you a surprise letter.I am
Mr.Ahmed Owain, account Manager with an investment bank here in
Burkina Faso.I have a very important business I want to discuss with
you.There is a draft account opened in my firm by a long-time client
of our bank.I have the opportunity of transferring the left over fund
(15.8 Million UsDollars)Fiftheen Million Eight Hundred Thousand United
States of American Dollars of one of my Bank clients who died at the
collapsing of the world trade center at the United States on September
11th 2001.

I want to invest this funds and introduce you to our bank for this
deal.All I require is your honest co-operation and I guarantee you
that this will be executed under a legitimate arrangement that will
protect us from any breach of the law.I agree that 40% of this money
will be for you as my foreign partner,50% for me while 10% is for
establishing of foundation for the less privilleges in your country.If
you are really interested in my proposal further details of the
Transfer will be forwarded unto you as soon as I receive your
willingness mail for a successful transfer.

Yours Sincerely,
Mr.Ahmed Owain,

How to undo previously set configuration?

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 05 2018, Olaf Hering wrote:

[Changed subject]

> My ~/.gitconfig looks like this, because all cloned repositories require 
> these settings:
> [sendemail]
> from = Olaf Hering 
> envelopesender = o...@aepfle.de
> chainreplyto = false
> ccover = yes
> smtpencryption = tls
> smtpdomain = sender
> smtppass = smtppass
> smtpAuth = PLAIN
> smtpserver = smtp.strato.de
> smtpuser = smtpuser
> confirm = always
> assume8bitEncoding = yes
> transferEncoding = 8bit
>
> Now there is that one repo that requires this:
>
> [sendemail]
> from = Olaf Hering 
> envelopesender = a@b.c
> smtpserver = otherhost
>
> That "otherhost" does just plain oldstyle unencrypted SMTP.
>
> How do I undo the global sendemail settings for that one repo?
> Setting the knobs to empty strings does not help:
> Command unknown: 'AUTH' at /usr/lib/git/git-send-email line 1455.
>
> It seems the global smtpuser is causing the error.

There isn't any way to do this, the only way out is the hack of using
conditional includes and placing this repository in some special
location.

In general it would be very nice if git learned to conditionally pay
attention to config from various places, I've been meaning to work on
this but haven't figured out a good syntax for it (suggestions
welcome!). Things I'd like to do:

1) Set some config in e.g. ~/.gitconfig saying that I want to ignore
   everything from /etc/gitconfig, or in /some/repo/.git/config saying I
   want to ignore ~/.gitconfig but not /etc/gitconfig.

2) Ditto #1 but more granular, e.g. for your use-case saying you're OK
   with grabbing ~/.gitconfig, but you'd like to ignore all sendemail.*
   values from there, or say in your local .git/config that you'd like
   to ignore all previously set sendemail.* no matter where it came
   from.

3) Ability to re-arrange the config priority, right now it's hardcoded
   that we look at /etc/gitconfig, then ~/.gitconfig then your
   .git/config. You can add a config for ~/work with the conditional
   includes, but it would be nice (just as a general thing) to also
   re-arrange things so /etc/gitconfig gets parsed last or whatever.

   I don't really have a use-case for that, but adding such priorities
   would be simple once we had support for #1 and #2, just some
   "priority" integer you could override for each file, and we'd set
   default values for them, e.g. 10 for /etc/gitconfig, 20 for
   ~/.gitconfig, 40 for .git/config etc.

For any of this to work we'd need to re-arrange the config code so that
we'd fully parse all the config files first, and consider any such
"ignore the thing before me" rules in each file, and then make a second
pass over the config data

The ulterior motive I want this for is to eventually support some
facility where we can safely load a .gitconfig from clone repos, since
once we have this for other reasons (and as noted above, it would be
useful for that in its own right) we can load .gitconfig from some
untrusted source, because we're going to be able to say that we only
trust the repo's .gitconfig to set sendemail.to or whatever, but nothing
else.

Previous ramblings from me on this subject:
https://public-inbox.org/git/87zi6eakkt@evledraar.gmail.com/

So maybe something like this in a .git/config

# Reject all previous such [config] overrides, by default we'd add
# them (as default in git config)
[config]
reject = *
[config "system"]
priority = 50
reject = *
accept = sendmail.*
[config "global"]
reject = *
accept = sendmail.*

And eventually have git itself mark up each config option on some scale
of least harmful (sendmail.to & friends) to most harmful (executing
shell aliases), and:

# Remote maintained untrusted config
[config "repo"]
acceptLevel = least-harmful

Or whatever toggle to include some default policy shipped with
git. Actually we could just do that with more generally with config
includes if we learned some syntax for including some templates shipped
with git itself.

Re: [PATCH v7 14/14] commit-graph: implement "--additive" option

[SZEDER Gábor]

On Mon, Apr 2, 2018 at 10:34 PM, Derrick Stolee  wrote:

> +With the `--append` option, include all commits that are present in the

> +$ git rev-parse HEAD | git commit-graph write --stdin-commits --append

> +   N_("git commit-graph write [--object-dir ] [--append] 
> [--stdin-packs|--stdin-commits]"),

> +   N_("git commit-graph write [--object-dir ] [--append] 
> [--stdin-packs|--stdin-commits]"),

> +   OPT_BOOL(0, "append", ,

> +   git rev-parse merge/3 | git commit-graph write --stdin-commits 
> --append &&

I see '--append' everywhere in the code and docs, good.
Please update the subject line as well, it still says '--additive'.

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

[Christian Couder]

On Thu, Apr 5, 2018 at 9:59 AM, Christian Couder
 wrote:
> On Thu, Apr 5, 2018 at 9:50 AM, Christian Couder
>  wrote:
>>
>> So maybe we can get rid of `result` and have something like:
>>
>>if (argc < 1) {
>>error(_("at least one argument is required"));
>>usage_with_options(git_stash_helper_usage, options);
>
> Maybe we could also simplify these 2 lines by using usage_msg_opt().
>
>>}
>>
>>if (!strcmp(argv[0], "apply"))
>>return apply_stash(argc, argv, prefix);
>>
>>error(_("unknown subcommand: %s"), argv[0]);
>>usage_with_options(git_stash_helper_usage, options);

And here actually we could improve the above 2 lines using something like:

usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);

It's better than using `error()` because the printed message will
start with "fatal" instead of "error".

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

[Christian Couder]

On Thu, Apr 5, 2018 at 9:50 AM, Christian Couder
 wrote:
>
> So maybe we can get rid of `result` and have something like:
>
>if (argc < 1) {
>error(_("at least one argument is required"));
>usage_with_options(git_stash_helper_usage, options);

Maybe we could also simplify these 2 lines by using usage_msg_opt().

>}
>
>if (!strcmp(argv[0], "apply"))
>return apply_stash(argc, argv, prefix);
>
>error(_("unknown subcommand: %s"), argv[0]);
>usage_with_options(git_stash_helper_usage, options);
> }

Re: [PATCH v3] git-svn: allow empty email-address using authors-prog and authors-file

[Eric Wong]

Thanks for the update.  The patch itself looks good, but I
noticed one --show-item isn't supported on SVN 1.8.10 for me.

I've tested the following on both SVN 1.8.10 and 1.9.5:

--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -83,7 +83,8 @@ test_expect_success 'authors-prog imported user without 
email' '
 test_expect_success 'imported without authors-prog and authors-file' '
svn mkdir -m hh --username hh "$svnrepo"/hh &&
(
-   uuid=$(svn info --show-item=repos-uuid "$svnrepo") &&
+   uuid=$(svn info "$svnrepo" |
+   sed -n "s/^Repository UUID: //p") &&
cd x &&
git svn fetch &&
git rev-list -1 --pretty=raw refs/remotes/git-svn | \

Can you confirm it's OK for you?  Thanks.

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

[Christian Couder]

On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb  wrote:
> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being
> finished.
>
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
>
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
>
> Signed-off-by: Joel Teichroeb 
> ---
>  .gitignore  |   1 +
>  Makefile|   1 +
>  builtin.h   |   1 +
>  builtin/stash--helper.c | 431 
> 
>  git-stash.sh|  75 +
>  git.c   |   1 +
>  6 files changed, 440 insertions(+), 70 deletions(-)
>  create mode 100644 builtin/stash--helper.c
>
> diff --git a/.gitignore b/.gitignore
> index 833ef3b0b7..296d5f376d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -152,6 +152,7 @@
>  /git-show-ref
>  /git-stage
>  /git-stash
> +/git-stash--helper
>  /git-status
>  /git-stripspace
>  /git-submodule
> diff --git a/Makefile b/Makefile
> index 96f6138f63..6cfdbe9a32 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
>  BUILTIN_OBJS += builtin/shortlog.o
>  BUILTIN_OBJS += builtin/show-branch.o
>  BUILTIN_OBJS += builtin/show-ref.o
> +BUILTIN_OBJS += builtin/stash--helper.o
>  BUILTIN_OBJS += builtin/stripspace.o
>  BUILTIN_OBJS += builtin/submodule--helper.o
>  BUILTIN_OBJS += builtin/symbolic-ref.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa4..a14fd85b0e 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_show(int argc, const char **argv, const char *prefix);
>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
>  extern int cmd_status(int argc, const char **argv, const char *prefix);
> +extern int cmd_stash__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
>  extern int cmd_submodule__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 00..9d00a9547d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,431 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +#include "dir.h"
> +#include "rerere.h"
> +
> +static const char * const git_stash_helper_usage[] = {
> +   N_("git stash--helper apply [--index] [-q|--quiet] []"),
> +   NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> +   N_("git stash--helper apply [--index] [-q|--quiet] []"),
> +   NULL
> +};
> +
> +static const char *ref_stash = "refs/stash";
> +static int quiet;
> +static struct strbuf stash_index_path = STRBUF_INIT;
> +
> +struct stash_info {
> +   struct object_id w_commit;
> +   struct object_id b_commit;
> +   struct object_id i_commit;
> +   struct object_id u_commit;
> +   struct object_id w_tree;
> +   struct object_id b_tree;
> +   struct object_id i_tree;
> +   struct object_id u_tree;
> +   struct strbuf revision;
> +   int is_stash_ref;
> +   int has_u;
> +};
> +
> +static int grab_oid(struct object_id *oid, const char *fmt, const char *rev)
> +{
> +   struct strbuf buf = STRBUF_INIT;
> +   int ret;
> +
> +   strbuf_addf(, fmt, rev);
> +   ret = get_oid(buf.buf, oid);
> +   strbuf_release();
> +   return ret;
> +}
> +
> +static void free_stash_info(struct stash_info *info)
> +{
> +   strbuf_release(>revision);
> +}
> +
> +static int get_stash_info(struct stash_info *info, int argc, const char 
> **argv)
> +{
> +   struct strbuf symbolic = STRBUF_INIT;
> +   int ret;
> +   const char *revision;
> +   const char *commit = NULL;
> +   char *end_of_rev;
> +   char *expanded_ref;
> +   struct object_id discard;
> +
> +   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 

How to configure sendemail for no-auth?

[Olaf Hering]

My ~/.gitconfig looks like this, because all cloned repositories require these 
settings:
[sendemail]
from = Olaf Hering 
envelopesender = o...@aepfle.de
chainreplyto = false
ccover = yes
smtpencryption = tls
smtpdomain = sender
smtppass = smtppass
smtpAuth = PLAIN
smtpserver = smtp.strato.de
smtpuser = smtpuser
confirm = always
assume8bitEncoding = yes
transferEncoding = 8bit

Now there is that one repo that requires this:

[sendemail]
from = Olaf Hering 
envelopesender = a@b.c
smtpserver = otherhost

That "otherhost" does just plain oldstyle unencrypted SMTP.

How do I undo the global sendemail settings for that one repo?
Setting the knobs to empty strings does not help:
Command unknown: 'AUTH' at /usr/lib/git/git-send-email line 1455.

It seems the global smtpuser is causing the error.

Olaf


pgpAXV1diKPY9.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH] specify encoding for sed command

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 05 2018, Stephon Harris wrote:

> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running 
> git-completion.bash
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index b09c8a23626b4..52a4ab5e2165a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -282,7 +282,7 @@ __gitcomp ()
>
>  # Clear the variables caching builtins' options when (re-)sourcing
>  # the completion script.
> -unset $(set |sed -ne 
> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
> +unset $(set |LANG=C sed -ne 
> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

This is getting closer to the issue than your previous patch, but
there's still some open questions:

1) What platform OS / version / sed version is this on?

2) What's the output from "set" that's causing this error? Do we have an
   isolated test case for that?

3) There's other invocations of "sed" in the file, aren't those affected
   as well?

4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
   already do it for our invocation to "git merge".