Re: Re* [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 22:36 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  writes:
> 
>> Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
>> patch of mine and replace it with something René came up with (I have
>> not yet read his 1-6 patches submitted on this topic, so maybe they're
>> not mutually exclusive).
> 
> I think the 6-patch series supersedes this one.  Thanks anyway.

Actually no, I specifically avoided touching builtin/push.c because this
patch already handled it; it should still be applied before patch 6 from
my series.

René

Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output

[Jonathan Nieder]

Hi,

Han-Wen Nienhuys wrote:

> The colorization is controlled with the config setting "color.remote".
>
> Supported keywords are "error", "warning", "hint" and "success". They
> are highlighted if they appear at the start of the line, which is
> common in error messages, eg.
>
>ERROR: commit is missing Change-Id

A few questions:

- should this be documented somewhere in Documentation/technical/*protocol*?
  That way, server implementors can know to take advantage of it.

- how does this interact with (future) internationalization of server
  messages?  If my server knows that the client speaks French, should
  they send "ERROR:" anyway and rely on the client to translate it?
  Or are we deferring that question to a later day?

[...]
> The Git push process itself prints lots of non-actionable messages
> (eg. bandwidth statistics, object counters for different phases of the
> process), and my hypothesis is that these obscure the actionable error
> messages that our server sends back. Highlighting keywords in the
> draws more attention to actionable messages.

I'd be interested in ways to minimize Git's contribution to this as
well.  E.g. can we make more use of \r to make client-produced progress
messages take less screen real estate?  Should some of the servers
involved (e.g., Gerrit) do so as well?

> The highlighting is done on the client rather than server side, so
> servers don't have to grow capabilities to understand terminal escape
> codes and terminal state. It also consistent with the current state
> where Git is control of the local display (eg. prefixing messages with
> "remote: ").

As a strawman idea, what would you think of a way to allow the server
to do some coloration by using color tags like

 Erreur: ...

?

As an advantage, this would give the server more control.  As a
disadvantage, it is not so useful as "semantic markup", meaning it is
harder to figure out how to present to accessibility tools in a
meaningful way.  Plus, there's the issue of usefulness with existing
servers you mentioned:

> Finally, this solution is backwards compatible: many servers already
> prefix their messages with "error", and they will benefit from this
> change without requiring a server update.

Yes, this seems like a compelling reason to follow this approach.

[...]
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
>  color.push.error::
>   Use customized color for push errors.
>  
> +color.remote::
> + A boolean to enable/disable colored remote output. If unset,
> + then the value of `color.ui` is used (`auto` by default).
> +
> +color.remote.::
> + Use customized color for each remote keywords. `` may be
> + `hint`, `warning`, `success` or `error` which match the
> + corresponding keyword.

What positions in a remote message are matched?  If a server prints a
message about something being discouraged because it is error-prone,
would the "error" in error-prone turn red?

[...]
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,6 +1,103 @@
>  #include "cache.h"
> +#include "color.h"
> +#include "config.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "help.h"
> +
> +struct kwtable {

nit: perhaps kwtable_entry?

> + /*
> +  * We use keyword as config key so it can't contain funny chars like
> +  * spaces. When we do that, we need a separate field for slot name in
> +  * load_sideband_colors().
> +  */
> + const char *keyword;
> + char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable keywords[] = {
> + { "hint",   GIT_COLOR_YELLOW },
[...]
> +// Returns a color setting (GIT_COLOR_NEVER, etc).

nit: Git uses C89-style /* */ comments.

> +static int use_sideband_colors(void)

Makes sense.

[...]
> +void list_config_color_sideband_slots(struct string_list *list, const char 
> *prefix)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(keywords); i++)
> + list_config_item(list, prefix, keywords[i].keyword);
> +}

Not about this patch: I wonder if we can eliminate this boilerplate some
time in the future.

[...]
> +/*
> + * Optionally highlight some keywords in remote output if they appear at the
> + * start of the line. This should be called for a single line only.
> + */
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

Can this be static?

What does 'n' represent?  Can the comment say?  (Or if it's the length
of src, can it be renamed to srclen?)

Super-pedantic nit: even if there are multiple special words at the
start, this will only highlight one. :)  So it could say something
like "Optionally check if the first word on the line is a keyword and
highlight it if so."

> +{
> + int i;
> + if (!want_color_stderr(use_sideband_colors())) {

nit: whitespace damage (you can check for this with "git show --check").
There's a bit more elsewhere.

> + strbuf_add(dest, src, n);
> + return;
> + }
> +

Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output

[Eric Sunshine]

On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys  wrote:
> [PATCH 2/2] sideband: highlight keywords in remote sideband output

The -v option of git-format-patch makes it easy to let reviewers know
that this is a new version of a previously-submitted patch series.
This (I think) is the second attempt; if you need to send again, you
could use "git format-patch -v3 ...", which would result in "[PATCH v3
2/2] ...".

> The colorization is controlled with the config setting "color.remote".
>
> Supported keywords are "error", "warning", "hint" and "success". They
> are highlighted if they appear at the start of the line, which is
> common in error messages, eg.
>
>ERROR: commit is missing Change-Id
>
> The rationale for this change is that Gerrit does server-side
> processing to create or update code reviews, and actionable error
> messages (eg. missing Change-Id) must be communicated back to the user
> during the push. User research has shown that new users have trouble
> seeing these messages.
> [...snip...]

Thanks, this commit message is much more helpful than the previous one.

If you end up re-rolling, you might consider swapping the order of
these paragraphs around a bit to first explain the problem the patch
is solving (i.e. the justification), then the solution and relevant
details. Documentation/SubmittingPatches has good advice about this.

Using Gerrit as the sole rationale is underselling this otherwise
general improvement. Instead, the commit message could sell the change
on its own merits and can then use Gerrit as an example.

> The Git push process itself prints lots of non-actionable messages
> (eg. bandwidth statistics, object counters for different phases of the
> process), and my hypothesis is that these obscure the actionable error
> messages that our server sends back. Highlighting keywords in the
> draws more attention to actionable messages.

So, for instance, you might want to use a rewrite of this paragraph to
open the commit message. Something like this, perhaps:

A remote Git operation can print many non-actionable messages
(e.g. bandwidth statistics, object counters for different phases
of the process, etc.) and such noise can obscure genuine
actionable messages, such as warnings and outright errors.

As an example, Gerrit does server-side processing to create or
update code reviews, and actionable error messages (such as
"ERROR: commit is missing Change-Id") must be communicated back to
the user during a push operation, but new users often have trouble
spotting these error messages amid the noise.

Address this problem by upgrading 'sideband' to draw attention to
these messages by highlighting them with color.

[...and so forth...]

> Signed-off-by: Han-Wen Nienhuys 
> ---
> diff --git a/sideband.c b/sideband.c
> @@ -1,6 +1,103 @@
> +struct kwtable {
> +   /*
> +* We use keyword as config key so it can't contain funny chars like
> +* spaces. When we do that, we need a separate field for slot name in
> +* load_sideband_colors().
> +*/

This comment is outdated; load_sideband_colors() does not exist in this patch.

I get what the first sentence of this comment is saying, but I'm
having trouble understanding what the second sentence is all about.

> +   const char *keyword;
> +   char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable keywords[] = {
> +   { "hint",   GIT_COLOR_YELLOW },
> +   { "warning",GIT_COLOR_BOLD_YELLOW },
> +   { "success",GIT_COLOR_BOLD_GREEN },
> +   { "error",  GIT_COLOR_BOLD_RED },
> +};
> +
> +// Returns a color setting (GIT_COLOR_NEVER, etc).

Use /* old-style C comments */ in this codebase, not // C++ or new-style C.

> +static int use_sideband_colors(void)
> +{
> +   static int use_sideband_colors_cached = -1;
> +   const char *key = "color.remote";
> +
> +   if (use_sideband_colors_cached >= 0)
> +   return use_sideband_colors_cached;
> +
> +   if (!git_config_get_string(key, ))
> +   use_sideband_colors_cached = git_config_colorbool(key, value);

So, if "color.remote" exists, then 'use_sideband_colors_cached' is
assigned. However, if "color.remote" does not exist, then it's never
assigned. Is this intended behavior? It seems like you'd want to fall
back to some other value rather than leaving it unassigned.

Which leads to the next question: The documentation says that this
falls back to "color.ui" if "color.remote" does not exist, however,
where is this fallback happening? Am I overlooking something obvious?

> +   for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> +   strbuf_reset();
> +   strbuf_addf(, "%s.%s", key, keywords[i].keyword);
> +   if (git_config_get_string(sb.buf, ))
> +   continue;
> +   if (color_parse(value, keywords[i].color))
> +   die(_("expecting a color: %s"), value);

Re: [PATCH 1/2] config: document git config getter return value.

[Jonathan Nieder]

(cc-ing peff, config whiz)
Hi,

Han-Wen Nienhuys wrote:

> Subject: config: document git config getter return value.

micronit: no period at the end of subject line

> ---
>  config.h | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

May we forge your sign-off?  See Documentation/SubmittingPatches section
[[sign-off]] for what this means.

> 
> diff --git a/config.h b/config.h
> index b95bb7649..41700f40b 100644
> --- a/config.h
> +++ b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
>  };
>  
>  extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **value);
> +

nit: this blank line feels out of place (though I don't particularly
mind it and wouldn't reroll just for that)

>  extern const struct string_list *git_configset_get_value_multi(struct 
> config_set *cs, const char *key);
>  extern void git_configset_clear(struct config_set *cs);
> +
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);

This function doesn't take a 'dest' argument.  Is the comment meant to
apply to it?  If not, can the comment go below it?

> +extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **dest);
>  extern int git_configset_get_string_const(struct config_set *cs, const char 
> *key, const char **dest);
>  extern int git_configset_get_string(struct config_set *cs, const char *key, 
> char **dest);
>  extern int git_configset_get_int(struct config_set *cs, const char *key, int 
> *dest);

With a sign-off and whatever subset of the above suggestions makes sense
to you,

Reviewed-by: Jonathan Nieder 

Thanks.

Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Pratik Karki]

Hi Junio,

On Fri, Aug 3, 2018 at 4:47 AM Junio C Hamano  wrote:
>
> * pk/rebase-in-c (2018-07-30) 3 commits
>  - builtin/rebase: support running "git rebase "
>  - rebase: refactor common shell functions into their own file
>  - rebase: start implementing it as a builtin
>
>  Rewrite of the "rebase" machinery in C.
>
>  Will merge to 'next'.

On a recent mail[1], I had suggested stalling this iteration in `pu`
and wait for another iteration to merge in `next`.

[1]: 
https://public-inbox.org/git/CAOZc8M_FJmMCEB1MkJrBRpLiFjy8OTEg_MxoNQTh5-brHxR-=a...@mail.gmail.com/

Cheers,
Pratik

Re: Hash algorithm analysis

[Jonathan Nieder]

Hi Dan,

Dan Shumow wrote:

[replying out of order for convenience]
> However, I agree with Adam Langley that basically all of the
> finalists for a hash function replacement are about the same for the
> security needs of Git.  I think that, for this community, other
> software engineering considerations should be more important to the
> selection process.

Thanks for this clarification, which provides some useful context to
your opinion that was previously relayed by Dscho.

[...]
> So, as one of the coauthors of the SHA-1 collision detection code, I
> just wanted to chime in and say I'm glad to see the move to a longer
> hash function.  Though, as a cryptographer, I have a few thoughts on
> the matter that I thought I would share.
>
> I think that moving to SHA256 is a fine change, and I support it.

More generally, thanks for weighing in and for explaining your
rationale.  Even (especially) having already made the decision, it's
comforting to hear a qualified person endorsing that choice.

Sincerely,
Jonathan

Re: Question regarding quarantine environments

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Thu, Aug 02, 2018 at 12:58:52PM -0500, Liam Decker wrote:

>> The solution that I implemented was to check the objects directory for the
>> object, and if it was not there, to look for a quarantine directory and try
>> there. However, that feels fairly inefficient.
>
> That's more or less what Git will do under the hood (though in the
> opposite order).
>
>> For the curious, the library and solution I attempted are both here [5]
>
> Just skimming, but it sounds like go-git does not support the
> GIT_OBJECT_DIRECTORY environment variable.

To be clear: we don't guarantee that the quarantine directory in the
future will be where it is today.

So as Peff hinted, supporting GIT_OBJECT_DIRECTORY in go-git is likely
to be the best way forward for your tool.

Thanks and hope that helps,
Jonathan

[PATCH 0/3] Reroll of sb/config-write-fix

[Stefan Beller]

Only fix was in the commit message of the second patch:

2:  c667e555066 ! 1749:  38e5f6f335d config: fix case sensitive subsection 
names on writing
@@ -2,8 +2,8 @@
 
 config: fix case sensitive subsection names on writing
 
-A use reported a submodule issue regarding strange case indentation
-issues, but it could be boiled down to the following test case:
+A user reported a submodule issue regarding a section mix-up,
+but it could be boiled down to the following test case:

previous version at
https://public-inbox.org/git/20180801193413.146994-1-sbel...@google.com/

Stefan Beller (3):
  t1300: document current behavior of setting options
  config: fix case sensitive subsection names on writing
  git-config: document accidental multi-line setting in deprecated
syntax

 Documentation/git-config.txt | 21 +
 config.c | 12 -
 t/t1300-config.sh| 87 
 3 files changed, 119 insertions(+), 1 deletion(-)

-- 
2.18.0.132.g195c49a2227

[PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax

[Stefan Beller]

The bug was noticed when writing the previous patch; a fix for this bug
is not easy though: If we choose to ignore the case of the subsection
(and revert most of the code of the previous patch, just keeping
s/strncasecmp/strcmp/), then we'd introduce new sections using the
new syntax, such that

 
   [section.subsection]
 key = value1
 

  git config section.Subsection.key value2

would result in

 
   [section.subsection]
 key = value1
   [section.Subsection]
 key = value2
 

which is even more confusing. A proper fix would replace the first
occurrence of 'key'. As the syntax is deprecated, let's prefer to not
spend time on fixing the behavior and just document it instead.

Signed-off-by: Stefan Beller 
---
 Documentation/git-config.txt | 21 +
 1 file changed, 21 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 18ddc78f42d..8e240435bee 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -453,6 +453,27 @@ http.sslverify false
 
 include::config.txt[]
 
+BUGS
+
+When using the deprecated `[section.subsection]` syntax, changing a value
+will result in adding a multi-line key instead of a change, if the subsection
+is given with at least one uppercase character. For example when the config
+looks like
+
+
+  [section.subsection]
+key = value1
+
+
+and running `git config section.Subsection.key value2` will result in
+
+
+  [section.subsection]
+key = value1
+key = value2
+
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.18.0.132.g195c49a2227

[PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

A user reported a submodule issue regarding a section mix-up,
but it could be boiled down to the following test case:

  $ git init test  && cd test
  $ git config foo."Bar".key test
  $ git config foo."bar".key test
  $ tail -n 3 .git/config
  [foo "Bar"]
key = test
key = test

Sub sections are case sensitive and we have a test for correctly reading
them. However we do not have a test for writing out config correctly with
case sensitive subsection names, which is why this went unnoticed in
6ae996f2acf (git_config_set: make use of the config parser's event
stream, 2018-04-09)

Unfortunately we have to make a distinction between old style configuration
that looks like

  [foo.Bar]
key = test

and the new quoted style as seen above. The old style is documented as
case-agnostic, hence we need to keep 'strncasecmp'; although the
resulting setting for the old style config differs from the configuration.
That will be fixed in a follow up patch.

Reported-by: JP Sugarbroad 
Signed-off-by: Stefan Beller 
---
 config.c  | 12 +++-
 t/t1300-config.sh |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 66645047eb3..ffc2ffeafeb 100644
--- a/config.c
+++ b/config.c
@@ -37,6 +37,7 @@ struct config_source {
int eof;
struct strbuf value;
struct strbuf var;
+   unsigned section_name_old_dot_style : 1;
 
int (*do_fgetc)(struct config_source *c);
int (*do_ungetc)(int c, struct config_source *conf);
@@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct 
strbuf *name)
 
 static int get_extended_base_var(struct strbuf *name, int c)
 {
+   cf->section_name_old_dot_style = 0;
do {
if (c == '\n')
goto error_incomplete_line;
@@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c)
 
 static int get_base_var(struct strbuf *name)
 {
+   cf->section_name_old_dot_style = 1;
for (;;) {
int c = get_next_char();
if (cf->eof)
@@ -2364,14 +2367,21 @@ static int store_aux_event(enum config_event_t type,
store->parsed[store->parsed_nr].type = type;
 
if (type == CONFIG_EVENT_SECTION) {
+   int (*cmpfn)(const char *, const char *, size_t);
+
if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')
return error("invalid section name '%s'", cf->var.buf);
 
+   if (cf->section_name_old_dot_style)
+   cmpfn = strncasecmp;
+   else
+   cmpfn = strncmp;
+
/* Is this the section we were looking for? */
store->is_keys_section =
store->parsed[store->parsed_nr].is_keys_section =
cf->var.len - 1 == store->baselen &&
-   !strncasecmp(cf->var.buf, store->key, store->baselen);
+   !cmpfn(cf->var.buf, store->key, store->baselen);
if (store->is_keys_section) {
store->section_seen = 1;
ALLOC_GROW(store->seen, store->seen_nr + 1,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e87cfc1804f..4976e2fcd3f 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1290,6 +1290,7 @@ test_expect_success 'setting different case sensitive 
subsections ' '
Qc = v2
[d "e"]
f = v1
+   [d "E"]
Qf = v2
EOF
# exact match
-- 
2.18.0.132.g195c49a2227

[PATCH 1/3] t1300: document current behavior of setting options

[Stefan Beller]

This documents current behavior of the config machinery, when changing
the value of some settings. This patch just serves to provide a baseline
for the follow up that will fix some issues with the current behavior.

Signed-off-by: Stefan Beller 
---
 t/t1300-config.sh | 86 +++
 1 file changed, 86 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 24706ba4125..e87cfc1804f 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
 '
 
+test_expect_success 'old-fashioned settings are case insensitive' '
+   test_when_finished "rm -f testConfig testConfig_expect 
testConfig_actual" &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "v.a.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   QR = value2
+   EOF
+   git config -f testConfig_actual "V.a.R" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   r = value1
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "V.A.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   r = value1
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "v.A.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual
+'
+
+test_expect_success 'setting different case sensitive subsections ' '
+   test_when_finished "rm -f testConfig testConfig_expect 
testConfig_actual" &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V "A"]
+   R = v1
+   [K "E"]
+   Y = v1
+   [a "b"]
+   c = v1
+   [d "e"]
+   f = v1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V "A"]
+   Qr = v2
+   [K "E"]
+   Qy = v2
+   [a "b"]
+   Qc = v2
+   [d "e"]
+   f = v1
+   Qf = v2
+   EOF
+   # exact match
+   git config -f testConfig_actual a.b.c v2 &&
+   # match section and subsection, key is cased differently.
+   git config -f testConfig_actual K.E.y v2 &&
+   # section and key are matched case insensitive, but subsection needs
+   # to match; When writing out new values only the key is adjusted
+   git config -f testConfig_actual v.A.r v2 &&
+   # subsection is not matched:
+   git config -f testConfig_actual d.E.f v2 &&
+   test_cmp testConfig_expect testConfig_actual
+'
+
 for VAR in a .a a. a.0b a."b c". a."b c".0d
 do
test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
-- 
2.18.0.132.g195c49a2227

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

On Wed, Aug 1, 2018 at 3:51 PM Junio C Hamano  wrote:
>
> Stefan Beller  writes:
>
> > A use reported a submodule issue regarding strange case indentation
> > issues, but it could be boiled down to the following test case:
>
> Perhaps
>
> s/use/user/
> s/case indentation issues/section mix-up/

will be fixed in a reroll

>
> > ... However we do not have a test for writing out config correctly with
> > case sensitive subsection names, which is why this went unnoticed in
> > 6ae996f2acf (git_config_set: make use of the config parser's event
> > stream, 2018-04-09)
>
> s/unnoticed in \(.*04-09)\)/unnoticed when \1 broke it./
>
> This is why I asked if the patch is a "FIX" for an issue introduced
> by the cited commit.

I did not check further down the history if it was a recent brakage.

> >  static int get_base_var(struct strbuf *name)
> >  {
> > + cf->section_name_old_dot_style = 1;
> >   for (;;) {
> >   int c = get_next_char();
> >   if (cf->eof)
>
> OK, let me rephrase.  The basic parse structure is that
>
>  * upon seeing '[', we call get_base_var(), which stuffs the
>"section" (including subsection, if exists) in the strbuf.
>
>  * get_base_var() upon seeing a space after "[section ", calls
>get_extended_base_var().  This space can never exist in an
>old-style three-level names, where it is spelled as
>"[section.subsection]".  This space cannot exist in two-level
>names, either.  The closing ']' is eaten by this function before
>it returns.
>
>  * get_extended_base_var() grabs the "double quoted" subsection name
>and eats the closing ']' before it returns.
>
> So you set the new bit (section_name_old_dot_style) at the beginning
> of get_base_var(), i.e. declare that you assume we are reading old
> style, but upon entering get_extended_base_var(), unset it, because
> now you know we are parsing a modern style three-level name(s).
>
> Feels quite sensible way to keep track of old/new styles.
>
> When parsing two-level names, old-style bit is set, which we may
> need to be careful, thoguh.

I considered setting it only when seeing the dot, but then we'd have
to make sure it is properly initialized.

And *technically* the two level is old style, so I figured it's ok.

> > - !strncasecmp(cf->var.buf, store->key, store->baselen);
> > + !cmpfn(cf->var.buf, store->key, store->baselen);
>
> OK.  Section names should still be case insensitive (only the case
> sensitivity of subsection names is special), but presumably that's
> already normalized by the caller so we do not have to worry when we
> use strncmp()?  Can we add a test to demonstrate that it works
> correctly?

That was already demonstrated (but not tested) in
https://public-inbox.org/git/20180730230443.74416-4-sbel...@google.com/

Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Stefan Beller]

On Thu, Aug 2, 2018 at 4:02 PM Junio C Hamano  wrote:

> * sb/config-write-fix (2018-08-01) 3 commits
>  - git-config: document accidental multi-line setting in deprecated syntax
>  - config: fix case sensitive subsection names on writing
>  - t1300: document current behavior of setting options
>
>  Recent update to "git config" broke updating variable in a
>  subsection, which has been corrected.
>
>  Not quite?
>  cf. 

I'd rather point to
https://public-inbox.org/git/xmqqftzx67vo@gitster-ct.c.googlers.com/
https://public-inbox.org/git/xmqqva8t4s63@gitster-ct.c.googlers.com/
instead (reason: shoddiness),

as the message you refer to points out *another*
bug, using the old notation, that was there before that
series and still is there after the series.

Personally I do not want to care about the old notation
and by implementing it the way the series is, the
old notation doesn't see any *changes*.

>
> * ds/commit-graph-with-grafts (2018-07-19) 8 commits
>   (merged to 'next' on 2018-08-02 at 0ee624e329)
>  + commit-graph: close_commit_graph before shallow walk
>  + commit-graph: not compatible with uninitialized repo
>  + commit-graph: not compatible with grafts
>  + commit-graph: not compatible with replace objects
>  + test-repository: properly init repo
>  + commit-graph: update design document
>  + refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
>  + refs.c: migrate internal ref iteration to pass thru repository argument
>
>  The recently introduced commit-graph auxiliary data is incompatible
>  with mechanisms such as replace & grafts that "breaks" immutable
>  nature of the object reference relationship.  Disable optimizations
>  based on its use (and updating existing commit-graph) when these
>  incompatible features are in use in the repository.

Makes sense as a whole, but I dislike the first 2 patches
(they were my suggestion) for the refs API. I plan to re send patches
https://public-inbox.org/git/20180730194731.220191-1-sbel...@google.com/
but fixed for real.

(do not let this stop you from merging down this series)

> * sb/histogram-less-memory (2018-07-23) 4 commits
>   (merged to 'next' on 2018-08-02 at cfb02aa3b5)
>  + xdiff/histogram: remove tail recursion
>  + xdiff/xhistogram: move index allocation into find_lcs
>  + xdiff/xhistogram: factor out memory cleanup into free_index()
>  + xdiff/xhistogram: pass arguments directly to fall_back_to_classic_diff
>
>  "git diff --histogram" had a bad memory usage pattern, which has
>  been rearranged to reduce the peak usage.
>

Reminder to self: I need to work on the documentation patches
for diffing, too.

>
> * sb/submodule-update-in-c (2018-07-18) 6 commits
>  - submodule--helper: introduce new update-module-mode helper
>  - builtin/submodule--helper: factor out method to update a single submodule
>  - builtin/submodule--helper: store update_clone information in a struct
>  - builtin/submodule--helper: factor out submodule updating
>  - git-submodule.sh: rename unused variables
>  - git-submodule.sh: align error reporting for update mode to use path
>
>  "git submodule update" is getting rewritten piece-by-piece into C.
>
>  Will merge to 'next'.

Please do not, AFAICT this is still breaking in combination with the
series merged at 7e25437d35a (Merge branch 'sb/submodule-core-worktree',
2018-07-18) and I do not recall fixing the interaction between those two.

Stefan

What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Many topics have moved to 'master' and 'next' from 'next' to 'pu'
respectively.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/checkout-default-remote (2018-06-11) 8 commits
  (merged to 'next' on 2018-07-24 at 6ef645f485)
 + checkout & worktree: introduce checkout.defaultRemote
 + checkout: add advice for ambiguous "checkout "
 + builtin/checkout.c: use "ret" variable for return
 + checkout: pass the "num_matches" up to callers
 + checkout.c: change "unique" member to "num_matches"
 + checkout.c: introduce an *_INIT macro
 + checkout.h: wrap the arguments to unique_tracking_name()
 + checkout tests: index should be clean after dwim checkout
 (this branch is used by ab/test-must-be-empty.)

 "git checkout" and "git worktree add" learned to honor
 checkout.defaultRemote when auto-vivifying a local branch out of a
 remote tracking branch in a repository with multiple remotes that
 have tracking branches that share the same names.


* bc/object-id (2018-07-16) 16 commits
  (merged to 'next' on 2018-07-24 at 23680778a9)
 + pretty: switch hard-coded constants to the_hash_algo
 + sha1-file: convert constants to uses of the_hash_algo
 + log-tree: switch GIT_SHA1_HEXSZ to the_hash_algo->hexsz
 + diff: switch GIT_SHA1_HEXSZ to use the_hash_algo
 + builtin/merge-recursive: make hash independent
 + builtin/merge: switch to use the_hash_algo
 + builtin/fmt-merge-msg: make hash independent
 + builtin/update-index: simplify parsing of cacheinfo
 + builtin/update-index: convert to using the_hash_algo
 + refs/files-backend: use the_hash_algo for writing refs
 + sha1-name: use the_hash_algo when parsing object names
 + strbuf: allocate space with GIT_MAX_HEXSZ
 + commit: express tree entry constants in terms of the_hash_algo
 + hex: switch to using the_hash_algo
 + tree-walk: replace hard-coded constants with the_hash_algo
 + cache: update object ID functions for the_hash_algo

 Conversion from uchar[40] to struct object_id continues.


* bc/sequencer-export-work-tree-as-well (2018-07-16) 1 commit
  (merged to 'next' on 2018-07-24 at 0b83ade721)
 + sequencer: pass absolute GIT_WORK_TREE to exec commands

 "git rebase" started exporting GIT_DIR environment variable and
 exposing it to hook scripts when part of it got rewritten in C.
 Instead of matching the old scripted Porcelains' behaviour,
 compensate by also exporting GIT_WORK_TREE environment as well to
 lessen the damage.  This can harm existing hooks that want to
 operate on different repository, but the current behaviour is
 already broken for them anyway.


* bp/test-drop-caches-for-windows (2018-07-12) 1 commit
  (merged to 'next' on 2018-07-24 at 257bb336c6)
 + handle lower case drive letters on Windows

 A test helper update for Windows.


* ds/commit-graph-fsck (2018-07-16) 23 commits
  (merged to 'next' on 2018-07-24 at 6a802adc7a)
 + coccinelle: update commit.cocci
 + commit-graph: update design document
 + gc: automatically write commit-graph files
 + commit-graph: add '--reachable' option
 + commit-graph: use string-list API for input
 + fsck: verify commit-graph
 + commit-graph: verify contents match checksum
 + commit-graph: test for corrupted octopus edge
 + commit-graph: verify commit date
 + commit-graph: verify generation number
 + commit-graph: verify parent list
 + commit-graph: verify root tree OIDs
 + commit-graph: verify objects exist
 + commit-graph: verify corrupt OID fanout and lookup
 + commit-graph: verify required chunks are present
 + commit-graph: verify catches corrupt signature
 + commit-graph: add 'verify' subcommand
 + commit-graph: load a root tree from specific graph
 + commit: force commit to parse from object database
 + commit-graph: parse commit from chosen graph
 + commit-graph: fix GRAPH_MIN_SIZE
 + commit-graph: UNLEAK before die()
 + t5318-commit-graph.sh: use core.commitGraph
 (this branch is used by ds/commit-graph-with-grafts, ds/reachable and 
jt/commit-graph-per-object-store.)

 "git fsck" learns to make sure the optional commit-graph file is in
 a sane state.


* en/dirty-merge-fixes (2018-07-11) 9 commits
  (merged to 'next' on 2018-07-24 at 7b6ca3507c)
 + merge: fix misleading pre-merge check documentation
 + merge-recursive: enforce rule that index matches head before merging
 + t6044: add more testcases with staged changes before a merge is invoked
 + merge-recursive: fix assumption that head tree being merged is HEAD
 + merge-recursive: make sure when we say we abort that we actually abort
 + t6044: add a testcase for index matching head, when head doesn't 

Re: [PATCH v4 9/9] Documentation/config: add odb..promisorRemote

[Stefan Beller]

On Wed, Aug 1, 2018 at 11:16 PM Christian Couder
 wrote:
>
> From: Christian Couder 
>
> Signed-off-by: Junio C Hamano 
> ---
>  Documentation/config.txt | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 43b2de7b5f..2d048d47f2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2513,6 +2513,11 @@ This setting can be overridden with the 
> `GIT_NOTES_REWRITE_REF`
>  environment variable, which must be a colon separated list of refs or
>  globs.
>
> +odb..promisorRemote::
> +   The name of a promisor remote. For now promisor remotes are
> +   the only kind of remote object database (odb) that is
> +   supported.
> +

Can you explain the end goal for this? (I did not find it in the cover letter,
nor do I make sense of this documentation)

So from what I understand, this series relates to partialClone, which
has the remote name of the "promisor" in extensions.partialclone.
That is the remote to contact for any needs w.r.t. partial clone and
fetching on demand.

This key "odb..promisorRemote = " introduces
2 new names, where do each of these two names hook in?
name2 is a remote, such as "origin" from what I can tell, but
which naming scheme does name1 follow here?

What makes the odb key different, in that the partial clone
feature only handles objects as well?

>  pack.window::
> The size of the window used by linkgit:git-pack-objects[1] when no
> window size is given on the command line. Defaults to 10.
> --
> 2.18.0.330.g17eb9fed90
>

Re: [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 22:01 schrieb Junio C Hamano:
> René Scharfe  writes:
> 
>> Am 02.08.2018 um 18:54 schrieb Jeff King:
>>> PS I actually would have made the rule simply "does it begin with a
>>>  '<'", which seems simpler still. If people accidentally write ">>  forgetting to close their brackets, that is a bug under both the
>>>  old and new behavior (just with slightly different outcomes).
>>
>> Good point.
> 
> Straying sideways into a tangent, but do we know if any locale wants
> to use something other than "<>" as an enclosing braket around a
> placeholder?

Bulgarian seems to use capitals instead; here are some examples found
with git grep -A1 'msgid "<' po/:

po/bg.po:msgid ""
po/bg.po-msgstr "ОТДАЛЕЧЕНО_ХРАНИЛИЩЕ"
--
po/bg.po:msgid ""
po/bg.po-msgstr "КЛОН"
--
po/bg.po:msgid "/"
po/bg.po-msgstr "ПОДДИРЕКТОРИЯ/"
--
po/bg.po:msgid "[,]"
po/bg.po-msgstr "БРОЙ[,БАЗА]"

>  This arg-help text, for example,
> 
>   N_("refspec")   without LIT-ARG-HELP
> 
> would be irritating for such a locale's translator, who cannot
> defeat the "<>" that is hardcoded and not inside _()
> 
>   s = literal ? "%s" : "<%s>";
>  
> that appear in parse-options.c::usage_argh().
> 
> Perhaps we should do _("<%s>") here?  That way, the result would
> hopefully be made consistent with
> 
>   N_("[:]")  with LIT-ARG-HELP
> 
> which allows translator to use the bracket of the locale's choice.

@Alexander: Would that help you?

René

[PATCH v3] remote: make refspec follow the same disambiguation rule as local refs

[Junio C Hamano]

When matching a non-wildcard LHS of a refspec against a list of
refs, find_ref_by_name_abbrev() returns the first ref that matches
using any DWIM rules used by refname_match() in refs.c, even if a
better match occurs later in the list of refs.

This causes unexpected behavior when (for example) fetching using
the refspec "refs/heads/s:" from a remote with both
"refs/heads/refs/heads/s" and "refs/heads/s"; even if the former was
inadvertently created, one would still expect the latter to be
fetched.  Similarly, when both a tag T and a branch T exist,
fetching T should favor the tag, just like how local refname
disambiguation rule works.  But because the code walks over
ls-remote output from the remote, which happens to be sorted in
alphabetical order and has refs/heads/T before refs/tags/T, a
request to fetch T is (mis)interpreted as fetching refs/heads/T.

Update refname_match(), all of whose current callers care only if it
returns non-zero (i.e. matches) to see if an abbreviated name can
mean the full name being tested, so that it returns a positive
integer whose magnitude can be used to tell the precedence, and fix
the find_ref_by_name_abbrev() function not to stop at the first
match but find the match with the highest precedence.

This is based on an earlier work, which special cased only the exact
matches, by Jonathan Tan.

Helped-by: Jonathan Tan 
Helped-by: Jonathan Nieder 
Signed-off-by: Junio C Hamano 
---

 * OK, with the NUM_REV_PARSE_RULES constant this time.

 refs.c   | 18 +-
 remote.c | 13 ++---
 t/t5510-fetch.sh | 35 +++
 3 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 20ba82b434..2ca715d099 100644
--- a/refs.c
+++ b/refs.c
@@ -487,16 +487,24 @@ static const char *ref_rev_parse_rules[] = {
NULL
 };
 
+#define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)
+
+/*
+ * Is it possible that the caller meant full_name with abbrev_name?
+ * If so return a non-zero value to signal "yes"; the magnitude of
+ * the returned value gives the precedence used for disambiguation.
+ *
+ * If abbrev_name cannot mean full_name, return 0.
+ */
 int refname_match(const char *abbrev_name, const char *full_name)
 {
const char **p;
const int abbrev_name_len = strlen(abbrev_name);
+   const int num_rules = NUM_REV_PARSE_RULES;
 
-   for (p = ref_rev_parse_rules; *p; p++) {
-   if (!strcmp(full_name, mkpath(*p, abbrev_name_len, 
abbrev_name))) {
-   return 1;
-   }
-   }
+   for (p = ref_rev_parse_rules; *p; p++)
+   if (!strcmp(full_name, mkpath(*p, abbrev_name_len, 
abbrev_name)))
+   return _rev_parse_rules[num_rules] - p;
 
return 0;
 }
diff --git a/remote.c b/remote.c
index c10d87c246..4a3e7ba136 100644
--- a/remote.c
+++ b/remote.c
@@ -1880,11 +1880,18 @@ static struct ref *get_expanded_map(const struct ref 
*remote_refs,
 static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const 
char *name)
 {
const struct ref *ref;
+   const struct ref *best_match = NULL;
+   int best_score = 0;
+
for (ref = refs; ref; ref = ref->next) {
-   if (refname_match(name, ref->name))
-   return ref;
+   int score = refname_match(name, ref->name);
+
+   if (best_score < score) {
+   best_match = ref;
+   best_score = score;
+   }
}
-   return NULL;
+   return best_match;
 }
 
 struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index da9ac00557..858381a788 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -535,6 +535,41 @@ test_expect_success "should be able to fetch with 
duplicate refspecs" '
)
 '
 
+test_expect_success 'LHS of refspec follows ref disambiguation rules' '
+   mkdir lhs-ambiguous &&
+   (
+   cd lhs-ambiguous &&
+   git init server &&
+   test_commit -C server unwanted &&
+   test_commit -C server wanted &&
+
+   git init client &&
+
+   # Check a name coming after "refs" alphabetically ...
+   git -C server update-ref refs/heads/s wanted &&
+   git -C server update-ref refs/heads/refs/heads/s unwanted &&
+   git -C client fetch ../server 
+refs/heads/s:refs/heads/checkthis &&
+   git -C server rev-parse wanted >expect &&
+   git -C client rev-parse checkthis >actual &&
+   test_cmp expect actual &&
+
+   # ... and one before.
+   git -C server update-ref refs/heads/q wanted &&
+   git -C server update-ref refs/heads/refs/heads/q unwanted &&
+   git -C client fetch ../server 
+refs/heads/q:refs/heads/checkthis &&
+   

Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, Jeff King wrote:

> On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:
>
>> Let's add a bit of Makefile metaprogramming to generate finer-grained
>> make targets applying one semantic patch to only a single source file,
>> and specify these as dependencies of the targets applying one semantic
>> patch to all source files.  This way that shell loop can be avoided,
>> semantic patches will only be applied to changed source files, and the
>> same semantic patch can be applied in parallel to multiple source
>> files.  The only remaining sequential part is aggregating the
>> suggested transformations from the individual targets into a single
>> patch file, which is comparatively cheap (especially since ideally
>> there aren't any suggestions).
>>
>> This change brings spectacular speedup in the scenario described in
>> point (1) above.  When the results of a previous 'make coccicheck' are
>> there, the time needed to run
>>
>>   touch apply.c ; time make -j4 coccicheck
>>
>> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and
>> 'apply.c' is the second longest source file in our codebase.  In the
>> scenario in point (2), running
>>
>>   touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck
>>
>> went from 56.364s to 35.883s, which is ~36% speedup.
>
> I really like this direction. The slowness of coccicheck is one of the
> things that has prevented me from running it more frequently. And I'm a
> big fan of breaking steps down as much as possible into make targets. It
> lets make do its job (avoiding repeated work and parallelizing).

Yeah, this is great. Also, CC-ing some of the recent contributors to
linux.git's coccinelle, perhaps they're interested / have comments.

>> All the above timings are best-of-five on a machine with 2 physical
>> and 2 logical cores.  I don't have the hardware to bring any numbers
>> for point (3).  The time needed to run 'make -j4 coccicheck' in a
>> clean state didn't change, it's ~3m42s both with and without this
>> patch.
>
> On a 40-core (20+20) machine, doing "make -j40 coccicheck" from scratch
> went from:
>
>   real1m25.520s
>   user5m41.492s
>   sys 0m26.776s
>
> to:
>
>   real0m24.300s
>   user14m35.084s
>   sys 0m50.108s
>
> I was surprised by the jump in CPU times. Doing "make -j1 coccicheck"
> with your patch results in:
>
>   real5m34.887s
>   user5m5.620s
>   sys 0m19.908s
>
> so it's really the parallelizing that seems to be to blame (possibly
> because this CPU boosts from 2.3Ghz to 3.0Ghz, and we're only using 8
> threads in the first example).
>
>>   - [RFC]
>> With this patch 'make coccicheck's output will look like this
>> ('make' apparently doesn't iterate over semantic patches in
>> lexicographical order):
>>
>>   SPATCH commit.cocci  abspath.c
>>   SPATCH commit.cocci  advice.c
>>   <... lots of lines ...>
>>   SPATCH array.cocci   http-walker.c
>>   SPATCH array.cocci   remote-curl.c
>>
>> which means that the number of lines in the output grows from
>> "Nr-of-semantic-patches" to "Nr-of-semantic-patches *
>> Nr-of-source-files".
>
> IMHO this is not really that big a deal. We are doing useful work for
> each line, so to me it's just more eye candy (and it's really satisfying
> to watch it zip by on the 40-core machine ;) ).

FWIW on the 8-cpu box I usually test on this went from 2m30s to 1m50s,
so not a huge improvement in time, but nice to have the per-file
progress.

> What if we inverted the current loop? That is, right now we iterate over
> the cocci patches at the Makefile level, and then for each target we
> iterate over the giant list of source files. Instead, we could teach the
> Makefile to iterate over the source files, with a target for each, and
> then hit each cocci patch inside there.
>
> That would give roughly the same output as a normal build. But moreover,
> I wonder if we could make things faster by actually combining the cocci
> files into a single set of rules to be applied to each source file. That
> would mean invoking spatch 1/8 as much. It does give fewer opportunities
> for "make" to reuse work, but that only matters when the cocci files
> change (which is much less frequent than source files changing).
>
> Doing:
>
>   cat contrib/coccinelle/*.cocci >mega.cocci
>   make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci
>
> gives me:
>
>   real0m17.573s
>   user10m56.724s
>   sys 0m11.548s
>
> And that should show an improvement on more normal-sized systems, too,
> because we really are eliminating some of the startup overhead.
>
> The other obvious downside is that you don't get individual patches for
> each class of transformation. Do we care? Certainly for a routine "make
> coccicheck" I primarily want to know:
>
>   - is there something that needs fixing?
>
>   - give me the patch for all fixes
>
> So I wonder if we'd 

Re: [PATCH] sha1dc: update from upstream

[Stefan Beller]

On Thu, Aug 2, 2018 at 1:51 PM Ævar Arnfjörð Bjarmason  wrote:

> diff --git a/sha1collisiondetection b/sha1collisiondetection
> index 19d97bf5af..232357eb2e 16
> --- a/sha1collisiondetection
> +++ b/sha1collisiondetection
> @@ -1 +1 @@
> -Subproject commit 19d97bf5af05312267c2e874ee6bcf584d9e9681
> +Subproject commit 232357eb2ea0397388254a4b188333a227bf5b10

Offtopic:
I wonder if we want to extend diffs a little here and similar to a diffstat
that is not technically needed, but rather aiding the reviewers eyes.
So maybe we could add a concise shortlog, e.g. after this line:

19d97bf5..232357eb (2 dots indicating it is a fast forward instead of 3 dots)
3 commits from "Merge pull request #37 from avar/fixup-pull-request-34"
to "Merge pull request #45 from avar/aix-big-endian-detection"

Re: [PATCH] sha1dc: update from upstream

[Michael Felt (aixtools)]

Sent from my iPhone

> On 2 Aug 2018, at 22:50, Ævar Arnfjörð Bjarmason  wrote:
> 
> Update sha1dc from the latest version by the upstream
> maintainer[1]. See 2db87328ef ("Merge branch 'ab/sha1dc'", 2017-07-10)
> for the last update.
> 
> This fixes an issue where AIX was wrongly detected as a Little-endian
> instead of a Big-endian system. See [2][3][4].
> 
> 1. 
> https://github.com/cr-marcstevens/sha1collisiondetection/commit/232357eb2ea0397388254a4b188333a227bf5b10
> 2. https://github.com/cr-marcstevens/sha1collisiondetection/pull/45
> 3. https://github.com/cr-marcstevens/sha1collisiondetection/pull/42
> 4. 
> https://public-inbox.org/git/20180729200623.gf945...@genre.crustytoothpaste.net/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> 
>> On Wed, Aug 01 2018, Ævar Arnfjörð Bjarmason wrote:
>> https://github.com/cr-marcstevens/sha1collisiondetection/pull/45
>> [...]
>> It should work, but as noted in the MR please test it so we can make
>> sure, and then (if you have a GitHub account) comment on the MR saying
>> it works for you.
> 
> This got merged upstream, and as noted in that upstream PR I've
> personally tested this on AIX under both GCC and IBM's xlc on the GCC
> compile farm, it works.
> 
Thanks. I already have git 2.18 in use with the manual patch. 
> sha1collisiondetection |  2 +-
> sha1dc/sha1.c  | 12 +++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/sha1collisiondetection b/sha1collisiondetection
> index 19d97bf5af..232357eb2e 16
> --- a/sha1collisiondetection
> +++ b/sha1collisiondetection
> @@ -1 +1 @@
> -Subproject commit 19d97bf5af05312267c2e874ee6bcf584d9e9681
> +Subproject commit 232357eb2ea0397388254a4b188333a227bf5b10
> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index 25eded1399..df0630bc6d 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -93,13 +93,23 @@
> #define SHA1DC_BIGENDIAN
> 
> /* Not under GCC-alike or glibc or *BSD or newlib or  */
> +#elif (defined(_AIX))
> +
> +/*
> + * Defines Big Endian on a whitelist of OSs that are known to be Big
> + * Endian-only. See
> + * 
> https://public-inbox.org/git/93056823-2740-d072-1ebd-46b440b33...@felt.demon.nl/
> + */
> +#define SHA1DC_BIGENDIAN
> +
> +/* Not under GCC-alike or glibc or *BSD or newlib or  
> or  */
> #elif defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
> /*
>  * As a last resort before we do anything else we're not 100% sure
>  * about below, we blacklist specific processors here. We could add
>  * more, see e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
>  */
> -#else /* Not under GCC-alike or glibc or *BSD or newlib or  whitelist>  or  */
> +#else /* Not under GCC-alike or glibc or *BSD or newlib or  whitelist> or  or  */
> 
> /* We do nothing more here for now */
> /*#error "Uncomment this to see if you fall through all the detection"*/
> -- 
> 2.18.0.345.g5c9ce644c3
> 

Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained

[Jeff King]

On Thu, Aug 02, 2018 at 03:46:08PM -0400, Eric Sunshine wrote:

> On Thu, Aug 2, 2018 at 2:02 PM Jeff King  wrote:
> > On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:
> > > This approach uses $(eval), which we haven't used in any of our
> > > Makefiles yet.  I wonder whether it's portable enough.  And it's
> > > ugly and complicated.
> >
> > I looked into this a long time ago for another set of Makefile patches I
> > was considering. $(eval) was added to GNU make in 3.80, released in
> > 2002. Which is probably fine by now.
> 
> For the record, MacOS developer tools are stuck at GNU make 3.81 (from 2006).

Thanks, that's interesting to know. Fortunately from my past research
that means that it should have both $(eval) and $(call).

If we follow my suggestions here, we don't need to care for now, but I
won't be surprised if it comes up again at some point.

-Peff

Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems

[Jeff King]

On Thu, Aug 02, 2018 at 02:14:30PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I also wonder if Windows could return some other file-unique identifier
> > that would work in place of an inode here. That would be pretty easy to
> > swap in via an #ifdef's helper function. I'd be OK shipping without that
> > and letting Windows folks fill it in later (as long as we do not do
> > anything too stupid until then, like claim all of the inode==0 files are
> > the same).
> 
> Yeah, but such a useful file-unique identifier would probably be
> used in place of inum in their (l)stat emulation already, if exists,
> no?

Maybe. It might not work as ino_t. Or it might be expensive to get.  Or
maybe it's simply impossible. I don't know much about Windows. Some
searching implies that NTFS does have a "file index" concept which is
supposed to be unique.

At any rate, until we have an actual plan for Windows, I think it would
make sense only to split the cases into "has working inodes" and
"other", and make sure "other" does something sensible in the meantime
(like mention the conflict, but skip trying to list duplicates).

When somebody wants to work on Windows support, then we can figure out
if it just needs to wrap the "get unique identifier" operation, or if it
would use a totally different algorithm.

-Peff

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Junio C Hamano]

Ramsay Jones  writes:

>>  OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
>> missing - files are ignored in dry run")),
>> -OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
>> executable bit of the listed files")),
>> +{ OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
>
> Am I alone in thinking that "(+x|-x)" is more readable?

I think I am guilty of that, and I agree yours is much easier to
read.

It can of course come on top of the series.

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Junio C Hamano]

Andrei Rybak  writes:

> On 2018-08-02 21:17, René Scharfe wrote:
>> Don't translate the argument specification for --chmod; "+x" and "-x"
>> are the literal strings that the commands accept.
>> > [...]
>> 
>> -OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
>> executable bit of the listed files")),
>> +{ OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
>> +  N_("override the executable bit of the listed files"),
>> +  PARSE_OPT_LITERAL_ARGHELP },
>
> Would it make sense to drop the localizations in po/* as well?
> Or should such things be handled with l10n rounds?

It should happen "automatically" (eh, rather, thanks to hard work by
Jiang); for the rest of us, when the l10n coordinator updates the
master .*pot file next time.

Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> In particular this makes it possible to really clean things up when
> removing the last submodule with "git rm".

This sentence is a continuation of the subject line, and I had to reread
it to follow along.

>
> The rationale is that if git creates .gitmodules when adding the first
> submodule it should also remove it when removing the last submodule.

I agree with this sentiment. It seems slightly odd to me to have this tied
in the same patch series that changes .gitmodules reading behavior
as I could think of this feature as orthogonal to what this series achieved
up to patch 10.

> -   test_cmp expect actual &&
> +   test_cmp expect.both_deleted actual &&

This seems to be the re-occuring pattern in t3600, and given that
we have

  cat >expect  diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 48fd14fae6..2bb42a4c8f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -99,6 +99,17 @@ inspect() {
> )
>  }
>
> +test_expect_success 'removal of last submodule also removes empty 
> .gitmodules' '
> +   (
> +   cd addtest &&
> +   test ! -d .git/modules &&

test_path_is_missing ?

> +   git submodule add -q "$submodurl" first_submod &&
> +   test -e .gitmodules &&

test_path_is_file

> +   git rm -f first_submod &&

Do we need to force it here?

> +   test ! -e .gitmodules

test_path_is_missing

Thanks for this series!
Overall it was a pleasant read, though I had some comments.

Thanks,
Stefan

Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems

[Junio C Hamano]

Jeff King  writes:

> I also wonder if Windows could return some other file-unique identifier
> that would work in place of an inode here. That would be pretty easy to
> swap in via an #ifdef's helper function. I'd be OK shipping without that
> and letting Windows folks fill it in later (as long as we do not do
> anything too stupid until then, like claim all of the inode==0 files are
> the same).

Yeah, but such a useful file-unique identifier would probably be
used in place of inum in their (l)stat emulation already, if exists,
no?

> PS It occurs to me that doing this naively (re-scan the entries already
>checked out when we see a collision) ends up quadratic over the
>number of entries in the worst case. That may not matter. You'd only
>have a handful of collisions normally, and anybody malicious can
>already git-bomb your checkout anyway. If we care, an alternative
>would be to set a flag for "I saw some collisions", and then follow
>up with a single pass putting entries into a hashmap of
>inode->filename.

Yeah, that makes sense.

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Andrei Rybak]

On 2018-08-02 21:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
> > [...]
> 
> - OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
> +   N_("override the executable bit of the listed files"),
> +   PARSE_OPT_LITERAL_ARGHELP },

Would it make sense to drop the localizations in po/* as well?
Or should such things be handled with l10n rounds?

Can be found using:

grep '(+/-)x' po/*

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Ramsay Jones]

On 02/08/18 20:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
> 
> Separate alternatives using a pipe character instead of a slash, for
> consistency.
> 
> Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
> pair of angular brackets around the argument help string, as that would
> wrongly indicate that users need to replace the literal strings with
> some kind of value.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  builtin/add.c  | 4 +++-
>  builtin/update-index.c | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 8a155dd41e..84bfec9b73 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
>   OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
> index")),
>   OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
> which cannot be added because of errors")),
>   OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
> missing - files are ignored in dry run")),
> - OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",

Am I alone in thinking that "(+x|-x)" is more readable?

ATB,
Ramsay Jones

> +   N_("override the executable bit of the listed files"),
> +   PARSE_OPT_LITERAL_ARGHELP },
>   OPT_HIDDEN_BOOL(0, "warn-embedded-repo", _on_embedded_repo,
>   N_("warn when adding an embedded repository")),
>   OPT_END(),
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..7feda6e271 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const 
> char *prefix)
>   PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
>   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>   (parse_opt_cb *) cacheinfo_callback},
> - {OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
> + {OPTION_CALLBACK, 0, "chmod", _executable_bit, "(+|-)x",
>   N_("override the executable bit of the listed files"),
>   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>   chmod_callback},
> 

[PATCH] sha1dc: update from upstream

[Ævar Arnfjörð Bjarmason]

Update sha1dc from the latest version by the upstream
maintainer[1]. See 2db87328ef ("Merge branch 'ab/sha1dc'", 2017-07-10)
for the last update.

This fixes an issue where AIX was wrongly detected as a Little-endian
instead of a Big-endian system. See [2][3][4].

1. 
https://github.com/cr-marcstevens/sha1collisiondetection/commit/232357eb2ea0397388254a4b188333a227bf5b10
2. https://github.com/cr-marcstevens/sha1collisiondetection/pull/45
3. https://github.com/cr-marcstevens/sha1collisiondetection/pull/42
4. 
https://public-inbox.org/git/20180729200623.gf945...@genre.crustytoothpaste.net/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Wed, Aug 01 2018, Ævar Arnfjörð Bjarmason wrote:
> https://github.com/cr-marcstevens/sha1collisiondetection/pull/45
>[...]
> It should work, but as noted in the MR please test it so we can make
> sure, and then (if you have a GitHub account) comment on the MR saying
> it works for you.

This got merged upstream, and as noted in that upstream PR I've
personally tested this on AIX under both GCC and IBM's xlc on the GCC
compile farm, it works.

 sha1collisiondetection |  2 +-
 sha1dc/sha1.c  | 12 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/sha1collisiondetection b/sha1collisiondetection
index 19d97bf5af..232357eb2e 16
--- a/sha1collisiondetection
+++ b/sha1collisiondetection
@@ -1 +1 @@
-Subproject commit 19d97bf5af05312267c2e874ee6bcf584d9e9681
+Subproject commit 232357eb2ea0397388254a4b188333a227bf5b10
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 25eded1399..df0630bc6d 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -93,13 +93,23 @@
 #define SHA1DC_BIGENDIAN
 
 /* Not under GCC-alike or glibc or *BSD or newlib or  */
+#elif (defined(_AIX))
+
+/*
+ * Defines Big Endian on a whitelist of OSs that are known to be Big
+ * Endian-only. See
+ * 
https://public-inbox.org/git/93056823-2740-d072-1ebd-46b440b33...@felt.demon.nl/
+ */
+#define SHA1DC_BIGENDIAN
+
+/* Not under GCC-alike or glibc or *BSD or newlib or  or 
 */
 #elif defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
 /*
  * As a last resort before we do anything else we're not 100% sure
  * about below, we blacklist specific processors here. We could add
  * more, see e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
  */
-#else /* Not under GCC-alike or glibc or *BSD or newlib or   or  */
+#else /* Not under GCC-alike or glibc or *BSD or newlib or  or  or  */
 
 /* We do nothing more here for now */
 /*#error "Uncomment this to see if you fall through all the detection"*/
-- 
2.18.0.345.g5c9ce644c3

Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> The is_empty_file() function can be generally useful, move it to dir.c
> and make it public.
>
> Signed-off-by: Antonio Ospite 

Makes sense,

Thanks,
Stefan

> +++ b/dir.c
> @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
> return ret;
>  }
>
> +/**
> + * Returns 1 if the file is empty or does not exist, 0 otherwise.
> + */

Please move the comment to the header instead.
/* possibly as a oneliner ? */

Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> git submodule commands can now access .gitmodules from the current
> branch even when it's not in the working tree, add some tests for that
> scenario.
>
> Signed-off-by: Antonio Ospite 
> ---
>
> For the test files I used the most used style in other tests, Stefan suggested
> to avoid subshells and use "git -C" but subshells make the test look cleaner
> IMHO.

Oh well. Let's not dive into a style argument, so let me focus on the tests.

IMHO it would be nice if (at least partially) these tests are in the same patch
that added the reading from HEAD:.gitmodules  (although I like short
patches, too).

>
>  t/t7416-submodule-sparse-gitmodules.sh | 112 +
>  1 file changed, 112 insertions(+)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
>
> diff --git a/t/t7416-submodule-sparse-gitmodules.sh 
> b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 00..3c7a53316b
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,112 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018  Antonio Ospite 
> +#
> +
> +test_description=' Test reading/writing .gitmodules is not in the working 
> tree
> +
> +This test verifies that, when .gitmodules is in the current branch but is not
> +in the working tree reading from it still works but writing to it does not.
> +
> +The test setup uses a sparse checkout, but the same scenario can be set up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +
> +NOTE: "git mv" and "git rm" are still supposed to work even without
> +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.

"supposed to work" != "tested that it works" ?
I am not sure what the NOTE wants to tell me? (Should I review those
tests to double check them now? or do we just want to tell future readers
of this test there are other tangent tests to this?)


> +test_expect_success 'initialising submodule when the gitmodules config is 
> not checked out' '
> +   (cd super &&
> +   git submodule init
> +   )
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is 
> not checked out' '
> +   (cd super &&
> +   git submodule summary
> +   )
> +'
> +
> +test_expect_success 'updating submodule when the gitmodules config is not 
> checked out' '
> +   (cd submodule &&
> +   echo file2 >file2 &&
> +   git add file2 &&
> +   git commit -m "add file2 to submodule"
> +   ) &&
> +   (cd super &&
> +   git submodule update
> +   )
> +'
> +
> +test_expect_success 'not adding submodules when the gitmodules config is not 
> checked out' '
> +   (cd super &&
> +   test_must_fail git submodule add ../new_submodule
> +   )
> +'
> +
> +# "git add" in the test above fails as expected, however it still leaves the
> +# cloned tree in there and adds a config entry to .git/config. This is 
> because
> +# no cleanup is done by cmd_add in git-submodule.sh when "git
> +# submodule--helper config" fails to add a new config setting.
> +#
> +# If we added the following commands to the test above:
> +#
> +#   rm -rf .git/modules/new_submodule &&
> +#   git reset HEAD new_submodule &&
> +#   rm -rf new_submodule

Alternatively we could check for the existence of .gitmodules
before starting all these things?

I think it is okay to not clean up if we check all "regular" or rather expected
things such as a non-writable .gitmodules file before actually doing it.
(This is similar to 'checkout' that walks the whole tree and checks if the
checkout is possible given the dirtyness of the tree, to either abort early
or pull through completely. In catastrophic problems such as a full disk
we'd still die in the middle of work)

> +#
> +# then the repository would be in a clean state and the test below would 
> pass.
> +#
> +# Maybe cmd_add should do the cleanup from above itself when failing to add
> +# a submodule.
> +test_expect_failure 'init submodule after adding failed when the gitmodules 
> config is not checked out' '

So this comment and test is about explaining why we can fail mid way through,
which we could not before unless we had the catastrophic event.

I think we should check for a "writable" .gitmodules file at the beginning,
which is if (G || (!G && !H)) [using the notation from the cover letter]?

> +   (cd super &&
> +   git submodule init
> +   )
> +'
> +
> +test_done
> --
> 2.18.0
>

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, René Scharfe wrote:

> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
>
> Separate alternatives using a pipe character instead of a slash, for
> consistency.
>
> Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
> pair of angular brackets around the argument help string, as that would
> wrongly indicate that users need to replace the literal strings with
> some kind of value.

Good change.

Let's mention in the commit message thath we ended up with this because
of 4a4838b46a ("i18n: update-index: mark parseopt strings for
translation", 2012-08-20) and 4e55ed32db ("add: add --chmod=+x /
--chmod=-x options", 2016-05-31) , both of which obviously didn't intend
for this to happen, but were either mass-replacing "..." with N_("..."),
or blindly copy/pasting an existing option whose argument should have
been translated.

> Signed-off-by: Rene Scharfe 
> ---
>  builtin/add.c  | 4 +++-
>  builtin/update-index.c | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 8a155dd41e..84bfec9b73 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
>   OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
> index")),
>   OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
> which cannot be added because of errors")),
>   OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
> missing - files are ignored in dry run")),
> - OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
> +   N_("override the executable bit of the listed files"),
> +   PARSE_OPT_LITERAL_ARGHELP },
>   OPT_HIDDEN_BOOL(0, "warn-embedded-repo", _on_embedded_repo,
>   N_("warn when adding an embedded repository")),
>   OPT_END(),
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..7feda6e271 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const 
> char *prefix)
>   PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
>   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>   (parse_opt_cb *) cacheinfo_callback},
> - {OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
> + {OPTION_CALLBACK, 0, "chmod", _executable_bit, "(+|-)x",
>   N_("override the executable bit of the listed files"),
>   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>   chmod_callback},

Re: Re* [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
> patch of mine and replace it with something René came up with (I have
> not yet read his 1-6 patches submitted on this topic, so maybe they're
> not mutually exclusive).

I think the 6-patch series supersedes this one.  Thanks anyway.

Re: Re* [PATCH] push: comment on a funny unbalanced option help

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, Junio C Hamano wrote:

> René Scharfe  writes:
>
>> Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
>>> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced 
>>> brackets
>>> From: Ævar Arnfjörð Bjarmason 
>>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>>> ...
>>> official escape hatch instead.
>>>
>>> Helped-by: René Scharfe 
>>
>> I didn't do anything for this particular patch so far?  But...
>>
>>> Signed-off-by: Junio C Hamano 
>
> Yeah, I realized it after I sent it out.  The line has been replaced
> with a forged sign-off from Ævar.

Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
patch of mine and replace it with something René came up with (I have
not yet read his 1-6 patches submitted on this topic, so maybe they're
not mutually exclusive).

>>> ---
>>>   builtin/push.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/builtin/push.c b/builtin/push.c
>>> index 1c28427d82..ef4032a9ef 100644
>>> --- a/builtin/push.c
>>> +++ b/builtin/push.c
>>> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char 
>>> *prefix)
>>> OPT_BIT( 0,  "porcelain", , N_("machine-readable 
>>> output"), TRANSPORT_PUSH_PORCELAIN),
>>> OPT_BIT('f', "force", , N_("force updates"), 
>>> TRANSPORT_PUSH_FORCE),
>>> { OPTION_CALLBACK,
>>> - 0, CAS_OPT_NAME, , N_("refname>:>> + 0, CAS_OPT_NAME, , N_(":"),
>>
>> ... shouldn't we use this opportunity to document that "expect" is
>> optional?
>
> I consider that it is a separate topic.
>
> I thought that we achieved a consensus that making the code guess
> missing ":" is a misfeature that should be deprecated (in
> which case we would not want to "s|:|[&]|"), but I may be
> misremembering it.

Re: [RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> When the .gitmodules file is not available in the working tree, try
> using HEAD:.gitmodules from the current branch. This covers the case
> when the file is part of the repository but for some reason it is not
> checked out, for example because of a sparse checkout.
>
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the working tree.

The reading part shows how nice our internal config system is, once
everything is put in place.

> Writing to .gitmodules will still require that the file is checked out,
> so check for that before calling config_set_in_gitmodules_file_gently.

Re: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[Junio C Hamano]

René Scharfe  writes:

> diff --git a/parse-options.c b/parse-options.c
> index 7db84227ab..3b874a83a0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char 
> *prefix,
>  static int usage_argh(const struct option *opts, FILE *outfile)
>  {
>   const char *s;
> - int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> + int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
> + !opts->argh || !!strpbrk(opts->argh, "()<>[]|");

Good that you did not include '-' in there, as that would have
broken a multi-word-placeholder.

All other changes in this patch looked sensible, too.

Re: [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

René Scharfe  writes:

> Am 02.08.2018 um 18:54 schrieb Jeff King:
>> PS I actually would have made the rule simply "does it begin with a
>> '<'", which seems simpler still. If people accidentally write "> forgetting to close their brackets, that is a bug under both the
>> old and new behavior (just with slightly different outcomes).
>
> Good point.

Straying sideways into a tangent, but do we know if any locale wants
to use something other than "<>" as an enclosing braket around a
placeholder?  This arg-help text, for example,

N_("refspec")   without LIT-ARG-HELP

would be irritating for such a locale's translator, who cannot
defeat the "<>" that is hardcoded and not inside _()

s = literal ? "%s" : "<%s>";

that appear in parse-options.c::usage_argh().

Perhaps we should do _("<%s>") here?  That way, the result would
hopefully be made consistent with

N_("[:]")  with LIT-ARG-HELP

which allows translator to use the bracket of the locale's choice.

Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter

[Junio C Hamano]

Stefan Beller  writes:

>> Am I correct to understand that this patch is a "FIX" for breakage
>> introduced by that commit?  The phrasing is not helping me to pick
>> a good base to queue these patches on.
>
> Please pick 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 
> 2018-05-08)
> as the base of this new series (am needs -3 to apply), although I developed 
> this
> series on origin/master.
>
>> Even though I hate to rob Peter to pay Paul (or vice versa)
>
> Yeah me, too. Here is a proper fix (i.e. only pay Paul, without the robbery),
> and a documentation of the second bug that we discovered.

Thanks.

I started with this in 'config'

[V.A]   r = one
[v.a]   r = two

and did "git config --replace-all v.a.r 1", which resulted in

[V.A]
[v.a]
r = 1

which is "correct", but defeats the "empty section is irritating"
tweak that introduced the bug these patches try to fix, which is
unfortunate.

But then "git config v.A.r 2" would give us

[V.A]
[v.a]
r = 1
r = 2

So this seems still broken, somewhat.

Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained

[Eric Sunshine]

On Thu, Aug 2, 2018 at 2:02 PM Jeff King  wrote:
> On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:
> > This approach uses $(eval), which we haven't used in any of our
> > Makefiles yet.  I wonder whether it's portable enough.  And it's
> > ugly and complicated.
>
> I looked into this a long time ago for another set of Makefile patches I
> was considering. $(eval) was added to GNU make in 3.80, released in
> 2002. Which is probably fine by now.

For the record, MacOS developer tools are stuck at GNU make 3.81 (from 2006).

Re: [PATCH] pack-objects: document about thread synchronization

[Jeff King]

On Sun, Jul 29, 2018 at 05:46:17PM +0200, Duy Nguyen wrote:

> tOn Sun, Jul 29, 2018 at 5:36 PM Nguyễn Thái Ngọc Duy  
> wrote:
> >
> > These extra comments should be make it easier to understand how to use
> > locks in pack-objects delta search code. For reference, see
> 
> Side note. I wonder if we could take progress_lock() less often in
> find_deltas() to reduce contention. Right now we take the lock every
> time we check out one object_entry but we could pull like 16 items out
> of the list per lock. I don't know how much actual contention on this
> lock though so maybe not worth doing.

I doubt it really matters that much, since we hold it for such a small
amount of time (basically just a few pointer/integer tweaks). Compared
to the heavy lifting of actually loading objects, you're not likely to
see a huge amount of contention, since at any given moment most threads
will be doing that work (or actually computing deltas).

Of course I could be wrong. If you hit a point where many threads are
skipping work (e.g., because they are considering deltas from objects in
the same pack, and skip forward without actually doing any work), then
they'd be ripping through the find_deltas() loop pretty quickly.

OTOH, in cases like that (and the ultimate case would just be running
"git repack -ad" twice in a row), the compression phase seems to go
quite quickly, implying we're not spending a lot of time there.

-Peff

Re: [PATCH] pack-objects: document about thread synchronization

[Jeff King]

On Sun, Jul 29, 2018 at 05:36:05PM +0200, Nguyễn Thái Ngọc Duy wrote:

> These extra comments should be make it easier to understand how to use
> locks in pack-objects delta search code. For reference, see
> 
> 8ecce684a3 (basic threaded delta search - 2007-09-06)
> 384b32c09b (pack-objects: fix threaded load balancing - 2007-12-08)
> 50f22ada52 (threaded pack-objects: Use condition... - 2007-12-16)

Thanks, I think this is an improvement.

-Peff

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Jeff King]

On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano  wrote:
> >> Johannes Schindelin  writes:
> >> > ACK!
> >>
> >> Did you write a buggy caller that would have been caught or helped
> >> with this change?  You did not write the callee that is made more
> >> defensive with this patch, so I am being curious as to where that
> >> Ack is coming from (I wouldn't have felt curious if this were
> >> a reviewed-by instead).
> >
> > The code being made more defensive with this patch was authored by Dscho[1].
> >
> > [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)
> 
> Ah, OK.  The original by Peff done long time ago didn't check three
> fds separately, but just did a single check_auto_color() implicitly
> only for the standard output.

Right. Technically Eric's check could go into the "if (...AUTO)"
conditional, since that was what was touched in 295d949cfa. But I doubt
it matters for performance (and if it did, we should probably be
coalescing all of these conditionals into a single cached int flag).

> Come to think of it, would want_color_fd(0, var) ever make sense?

No, it's just there because of 0-indexing. The BUG() check could
actually be "if (fd < 1 || ...)" to cover that (it "works", but it is
nonsensical).

Or we could even use "fd - 1" as the index. But it is probably not worth
the effort.

-Peff

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Junio C Hamano]

Eric Sunshine  writes:

> On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>> > ACK!
>>
>> Did you write a buggy caller that would have been caught or helped
>> with this change?  You did not write the callee that is made more
>> defensive with this patch, so I am being curious as to where that
>> Ack is coming from (I wouldn't have felt curious if this were
>> a reviewed-by instead).
>
> The code being made more defensive with this patch was authored by Dscho[1].
>
> [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)

Ah, OK.  The original by Peff done long time ago didn't check three
fds separately, but just did a single check_auto_color() implicitly
only for the standard output.

Come to think of it, would want_color_fd(0, var) ever make sense?

In any case, thanks for unconfusing me.

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Jeff King]

On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:

> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +   if (argc < 2 || argc > 3)
> > +   die("submodule--helper config takes 1 or 2 arguments: name 
> > [value]");
> > +
> > +   /* Equivalent to ACTION_GET in builtin/config.c */
> > +   if (argc == 2)
> > +   return print_config_from_gitmodules(argv[1]);
> > +
> > +   /* Equivalent to ACTION_SET in builtin/config.c */
> > +   if (argc == 3)
> > +   return config_set_in_gitmodules_file_gently(argv[1], 
> > argv[2]);
> > +
> > +   return 0;
> 
> Technically we cannot reach this point here?
> Maybe it would be more defensive to
> 
> BUG("How did we get here?");
> 
> or at least return something !=0 ?

When I find myself reaching for a BUG(), it is often a good time to see
if we can restructure the code so that the logic more naturally falls
out.  Here the issue is that the first if conditional repeats the "else"
for the other two. So I think we could just write:

  if (argc == 2)
return ...
  if (argc == 3)
return ...

  die("need 1 or 2 arguments");

-Peff

[PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[René Scharfe]

Parseopt wraps argument help strings in a pair of angular brackets by
default, to tell users that they need to replace it with an actual
value.  This is useful in most cases, because most option arguments
are indeed single values of a certain type.  The option
PARSE_OPT_LITERAL_ARGHELP needs to be used in option definitions with
arguments that have multiple parts or are literal strings.

Stop adding these angular brackets if special characters are present,
as they indicate that we don't deal with a simple placeholder.  This
simplifies the code a bit and makes defining special options slightly
easier.

Remove the flag PARSE_OPT_LITERAL_ARGHELP in the cases where the new
and more cautious handling suffices.

Signed-off-by: Rene Scharfe 
---
The patch to add PARSE_OPT_LITERAL_ARGHELP for push --force-with-lease
should be applied before this one.

 builtin/add.c  | 5 ++---
 builtin/pack-objects.c | 2 +-
 builtin/read-tree.c| 2 +-
 builtin/send-pack.c| 3 +--
 builtin/shortlog.c | 3 +--
 builtin/show-branch.c  | 2 +-
 builtin/update-index.c | 2 +-
 builtin/write-tree.c   | 5 ++---
 parse-options.c| 3 ++-
 9 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 84bfec9b73..ba1ff5689d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,9 +304,8 @@ static struct option builtin_add_options[] = {
OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
index")),
OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
which cannot be added because of errors")),
OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
missing - files are ignored in dry run")),
-   { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
- N_("override the executable bit of the listed files"),
- PARSE_OPT_LITERAL_ARGHELP },
+   OPT_STRING(0, "chmod", _arg, "(+|-)x",
+  N_("override the executable bit of the listed files")),
OPT_HIDDEN_BOOL(0, "warn-embedded-repo", _on_embedded_repo,
N_("warn when adding an embedded repository")),
OPT_END(),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3a5d1fa317..b2323613bc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3112,7 +3112,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
 N_("similar to --all-progress when progress meter is 
shown")),
{ OPTION_CALLBACK, 0, "index-version", NULL, 
N_("[,]"),
  N_("write the pack index file in the specified idx format 
version"),
- PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
+ 0, option_parse_index_version },
OPT_MAGNITUDE(0, "max-pack-size", _size_limit,
  N_("maximum size of each output pack file")),
OPT_BOOL(0, "local", ,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ebc43eb805..fbbc98e516 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
 N_("same as -m, but discard unmerged entries")),
{ OPTION_STRING, 0, "prefix", , 
N_("/"),
  N_("read the tree into the index under /"),
- PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+ PARSE_OPT_NONEG },
OPT_BOOL('u', NULL, ,
 N_("update working tree with merge result")),
{ OPTION_CALLBACK, 0, "exclude-per-directory", ,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 0b517c9368..724b484850 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -180,8 +180,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
{ OPTION_CALLBACK,
  0, CAS_OPT_NAME, , N_(":"),
  N_("require old value of ref to be at this value"),
- PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
- parseopt_push_cas_option },
+ PARSE_OPT_OPTARG, parseopt_push_cas_option },
OPT_END()
};
 
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index f555cf9e4f..3898a2c9c4 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,8 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('e', "email", ,
 N_("Show the email address of each author")),
{ OPTION_CALLBACK, 'w', NULL, , N_("[,[,]]"),
-   N_("Linewrap output"),
-   PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+   N_("Linewrap output"), PARSE_OPT_OPTARG,
_wrap_args },
OPT_END(),
};
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2e985c00a..9106da1985 100644
--- 

[PATCH 5/6] shortlog: correct option help for -w

[René Scharfe]

Wrap the placeholders in the option help string for -w in pairs of
angular brackets to document that users need to replace them with actual
numbers.  Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt
from adding another pair.

Signed-off-by: Rene Scharfe 
---
 builtin/shortlog.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 608d6ba77b..f555cf9e4f 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -268,8 +268,10 @@ int cmd_shortlog(int argc, const char **argv, const char 
*prefix)
 N_("Suppress commit descriptions, only provides commit 
count")),
OPT_BOOL('e', "email", ,
 N_("Show the email address of each author")),
-   { OPTION_CALLBACK, 'w', NULL, , N_("w[,i1[,i2]]"),
-   N_("Linewrap output"), PARSE_OPT_OPTARG, 
_wrap_args },
+   { OPTION_CALLBACK, 'w', NULL, , N_("[,[,]]"),
+   N_("Linewrap output"),
+   PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+   _wrap_args },
OPT_END(),
};
 
-- 
2.18.0

[PATCH 4/6] send-pack: specify --force-with-lease argument help explicitly

[René Scharfe]

Wrap each part of the argument help string in angular brackets to show
that users need to replace them with actual values.  Do that explicitly
to balance the pairs nicely in the code and avoid confusing casual
readers.  Add the flag PARSE_OPT_LITERAL_ARGHELP to keep parseopt from
adding another pair.

Signed-off-by: Rene Scharfe 
---
 builtin/send-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 42fd8d1a35..0b517c9368 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -178,9 +178,10 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "stdin", _stdin, N_("read refs from stdin")),
OPT_BOOL(0, "helper-status", _status, N_("print status 
from remote helper")),
{ OPTION_CALLBACK,
- 0, CAS_OPT_NAME, , N_("refname>::"),
  N_("require old value of ref to be at this value"),
- PARSE_OPT_OPTARG, parseopt_push_cas_option },
+ PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+ parseopt_push_cas_option },
OPT_END()
};
 
-- 
2.18.0

[PATCH 3/6] pack-objects: specify --index-version argument help explicitly

[René Scharfe]

Wrap both placeholders in the argument help string in angular brackets
to signal that users needs replace them with some actual value.  Use the
flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding another
pair.

Signed-off-by: Rene Scharfe 
---
 builtin/pack-objects.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ebc8cefb53..3a5d1fa317 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3110,9 +3110,9 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "all-progress-implied",
 _progress_implied,
 N_("similar to --all-progress when progress meter is 
shown")),
-   { OPTION_CALLBACK, 0, "index-version", NULL, 
N_("version[,offset]"),
+   { OPTION_CALLBACK, 0, "index-version", NULL, 
N_("[,]"),
  N_("write the pack index file in the specified idx format 
version"),
- 0, option_parse_index_version },
+ PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
OPT_MAGNITUDE(0, "max-pack-size", _size_limit,
  N_("maximum size of each output pack file")),
OPT_BOOL(0, "local", ,
-- 
2.18.0

[PATCH 2/6] difftool: remove angular brackets from argument help

[René Scharfe]

Parseopt wraps arguments in a pair of angular brackets by default,
signifying that the user needs to replace it with a value of the
documented type.  Remove the pairs from the option definitions to
duplication and confusion.

Signed-off-by: Rene Scharfe 
---
 builtin/difftool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 51f6c9cdb4..172af0448b 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -703,7 +703,7 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
OPT_BOOL(0, "symlinks", ,
 N_("use symlinks in dir-diff mode")),
-   OPT_STRING('t', "tool", _cmd, N_(""),
+   OPT_STRING('t', "tool", _cmd, N_("tool"),
   N_("use the specified diff tool")),
OPT_BOOL(0, "tool-help", _help,
 N_("print a list of diff tools that may be used with "
@@ -711,7 +711,7 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "trust-exit-code", _exit_code,
 N_("make 'git-difftool' exit when an invoked diff "
"tool returns a non - zero exit code")),
-   OPT_STRING('x', "extcmd", , N_(""),
+   OPT_STRING('x', "extcmd", , N_("command"),
   N_("specify a custom command for viewing diffs")),
OPT_END()
};
-- 
2.18.0

[PATCH 1/6] add, update-index: fix --chmod argument help

[René Scharfe]

Don't translate the argument specification for --chmod; "+x" and "-x"
are the literal strings that the commands accept.

Separate alternatives using a pipe character instead of a slash, for
consistency.

Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
pair of angular brackets around the argument help string, as that would
wrongly indicate that users need to replace the literal strings with
some kind of value.

Signed-off-by: Rene Scharfe 
---
 builtin/add.c  | 4 +++-
 builtin/update-index.c | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 8a155dd41e..84bfec9b73 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
index")),
OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
which cannot be added because of errors")),
OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
missing - files are ignored in dry run")),
-   OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
executable bit of the listed files")),
+   { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
+ N_("override the executable bit of the listed files"),
+ PARSE_OPT_LITERAL_ARGHELP },
OPT_HIDDEN_BOOL(0, "warn-embedded-repo", _on_embedded_repo,
N_("warn when adding an embedded repository")),
OPT_END(),
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a8709a26ec..7feda6e271 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
(parse_opt_cb *) cacheinfo_callback},
-   {OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
+   {OPTION_CALLBACK, 0, "chmod", _executable_bit, "(+|-)x",
N_("override the executable bit of the listed files"),
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
chmod_callback},
-- 
2.18.0

Re: [RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> In t/t7506-status-submodule.sh at some point a new scenario is set up to
> test different things, in particular new submodules are added which are
> meant to completely replace the previous ones.
>
> However before calling the "git submodule add" commands for the new
> layout, the .gitmodules file is removed only from the working tree still
> leaving the previous content in current branch.
>
> This can break if, in the future, "git submodule add" starts
> differentiating between the following two cases:
>
>   - .gitmodules is not in the working tree but it is in the current
> branch (it may not be safe to add new submodules in this case);
>
>   - .gitmodules is neither in the working tree nor anywhere in the
> current branch (it is safe to add new submodules).
>
> Since the test means to get rid of .gitmodules anyways, let's completely
> remove it from the current branch, to actually start afresh in the new
> scenario.
>
> This is more future-proof and does not break current tests.

Makes sense.

Thanks,
Stefan

>
> Signed-off-by: Antonio Ospite 
> ---
>  t/t7506-status-submodule.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> index b4b74dbe29..af91ba92ff 100755
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh
> @@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked 
> file in nested submodule'
> (
> cd super &&
> git clean -dfx &&
> -   rm .gitmodules &&
> +   git rm .gitmodules &&
> +   git commit -m "remove .gitmodules" &&
> git submodule add -f ./sub1 &&
> git submodule add -f ./sub2 &&
> git submodule add -f ./sub1 sub3 &&
> --
> 2.18.0
>

Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems

[Jeff King]

On Thu, Aug 02, 2018 at 09:27:31AM -0700, Junio C Hamano wrote:

> > OK so we're going back to the original way of checking that we check
> > out the different files on the same place (because fs is icase) and
> > try to collect all paths for reporting, yes? I can give it another go
> > (but of course if anybody else steps up, I'd very gladly hand this
> > over)
> 
> Detect and report, definitely yes; I am not sure about collect all
> (personally I am OK if we stopped at reporting "I tried to check out
> X but your project tree has something else that is turned to X by
> your pathname-smashing filesystem" without making it a requirement
> to report what the other one that conflict with X is.  Of course,
> reporting the other side _is_ nicer and I'd be happier if we can do
> so without too much ugly code, but I do not think it is a hard
> requirement.

Yeah, I think it would be OK to issue the warning for the conflicted
path, and then if we _can_ produce the secondary list of colliding
paths, do so. Even on a system with working inodes, we may not come up
with a match (e.g., if it wasn't us who wrote the file, but rather we
raced with some other process).

I also wonder if Windows could return some other file-unique identifier
that would work in place of an inode here. That would be pretty easy to
swap in via an #ifdef's helper function. I'd be OK shipping without that
and letting Windows folks fill it in later (as long as we do not do
anything too stupid until then, like claim all of the inode==0 files are
the same).

-Peff

PS It occurs to me that doing this naively (re-scan the entries already
   checked out when we see a collision) ends up quadratic over the
   number of entries in the worst case. That may not matter. You'd only
   have a handful of collisions normally, and anybody malicious can
   already git-bomb your checkout anyway. If we care, an alternative
   would be to set a flag for "I saw some collisions", and then follow
   up with a single pass putting entries into a hashmap of
   inode->filename.

Re: [RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> Use the 'submodule--helper config' command in git-submodules.sh to avoid
> referring explicitly to .gitmodules by the hardcoded file path.

ccol! This is the corner stone to the work of the previous patches. Nicely done!

> This makes it possible to access the submodules configuration in a more
> controlled way.
>
> Signed-off-by: Antonio Ospite 
> ---
>
> Note that there are other instances of "git config -f .gitmodules" in test
> files, but I am not touching those for now.

I just checked and this converts all of git-submodule.sh which would
have been my expectation as that is the real product.

The tests are fine to use "git config -f .gitmodules" as there we want to
setup a specific environment to be tested? So I would think even future
patches in this series will not touch test files for such a conversion
as in here.

Stefan

Re: git merge -s subtree seems to be broken.

[Jeff King]

On Wed, Aug 01, 2018 at 02:58:50AM +0200, René Scharfe wrote:

> Am 31.07.2018 um 23:06 schrieb Junio C Hamano:
> > Jeff King  writes:
> > 
> >> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:
> >> ...
> >> So here it is fixed, and with a commit message. I'm not happy to omit a
> >> regression test, but I actually couldn't come up with a minimal one that
> >> tickled the problem, because we're playing around with heuristics.
> How about something like this? (squashable)

Thanks. This is quite similar to what I tried, but I had started a new
script, and I suspect that what is in t6029's master branch tickles the
heuristic in a more interesting way. Or possibly I just botched my
attempt, though I did spend quite a bit of time fiddling with it. The
master branch seems to only contain one file, "hello". Hmph.

At any rate, it does trigger the bug and demonstrate the fix, so let's
go with it. I see Junio already squashed the tests into
jk/merge-subtree-heuristics, but I think we can cut down the commit
message a bit (and stop claiming that we don't have a test ;) ).  Like
so (and yes, this version omits the extra scissors):

-- >8 --
Subject: score_trees(): fix iteration over trees with missing entries

In score_trees(), we walk over two sorted trees to find
which entries are missing or have different content between
the two.  So if we have two trees with these entries:

  one   two
  ---   ---
  a a
  b c
  c d

we'd expect the loop to:

  - compare "a" to "a"

  - compare "b" to "c"; because these are sorted lists, we
know that the second tree does not have "b"

  - compare "c" to "c"

  - compare "d" to end-of-list; we know that the first tree
does not have "d"

And prior to d8febde370 (match-trees: simplify score_trees()
using tree_entry(), 2013-03-24) that worked. But after that
commit, we mistakenly increment the tree pointers for every
loop iteration, even when we've processed the entry for only
one side. As a result, we end up doing this:

  - compare "a" to "a"

  - compare "b" to "c"; we know that we do not have "b", but
we still increment both tree pointers; at this point
we're out of sync and all further comparisons are wrong

  - compare "c" to "d" and mistakenly claim that the second
tree does not have "c"

  - exit the loop, mistakenly not realizing that the first
tree does not have "d"

So contrary to the claim in d8febde370, we really do need to
manually use update_tree_entry(), because advancing the tree
pointer depends on the entry comparison.

That means we must stop using tree_entry() to access each
entry, since it auto-advances the pointer. Instead:

  - we'll use tree_desc.size directly to know if there's
anything left to look at (which is what tree_entry() was
doing under the hood)

  - rather than do an extra struct assignment to "e1" and
"e2", we can just access the "entry" field of tree_desc
directly

That makes us a little more intimate with the tree_desc
code, but that's not uncommon for its callers.

The included test shows off the bug by adding a new entry
"bar.t", which sorts early in the tree and de-syncs the
comparison for "foo.t", which comes after.

Reported-by: George Shammas 
Helped-by: René Scharfe 
Signed-off-by: Jeff King 
---
 match-trees.c| 43 
 t/t6029-merge-subtree.sh | 28 ++
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 4cdeff53e1..37653308d3 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1, const 
struct object_id *ha
int score = 0;
 
for (;;) {
-   struct name_entry e1, e2;
-   int got_entry_from_one = tree_entry(, );
-   int got_entry_from_two = tree_entry(, );
int cmp;
 
-   if (got_entry_from_one && got_entry_from_two)
-   cmp = base_name_entries_compare(, );
-   else if (got_entry_from_one)
+   if (one.size && two.size)
+   cmp = base_name_entries_compare(, );
+   else if (one.size)
/* two lacks this entry */
cmp = -1;
-   else if (got_entry_from_two)
+   else if (two.size)
/* two has more entries */
cmp = 1;
else
break;
 
-   if (cmp < 0)
+   if (cmp < 0) {
/* path1 does not appear in two */
-   score += score_missing(e1.mode, e1.path);
-   else if (cmp > 0)
+   score += score_missing(one.entry.mode, one.entry.path);
+   update_tree_entry();
+   } else if (cmp > 0) {
/* path2 does not appear in one */
-   score += 

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Junio C Hamano]

Antonio Ospite  writes:

> Add a --stage option to the 'submodule--helper config' command so that
> the .gitmodules file can be staged without referring to it explicitly by
> its file path.

Sorry, but I have no clue what the above is trying to say.

The original 's--h config  []' is quite understandable.  It
is like "git config  []", i.e. either get the current value
for the name, or  set a new value to the name.

What does this 's--h config --stage' that does not take any option
do?  "git add .gitmodules"?  Something else?  In what meaning is the
word "stage" used?  Is it used as a verb "to stage"?

In a series that lets us use the data in the .gitmodules file without
materializing the file in the working tree, I would have expected
that you would want an option to specify which .gitmodules among
(1) the one in the working tree (i.e. the only option we currently
have), (2) the one in the index, and (3) the one in the HEAD, and
when I saw the title, I would have expected that

git submodule--helper config --stage name

may be a way to read from the .gitmodules in the index to find the
value for name (however, we we follow the option naming convention
in gitcli.txt, it should be called --cached, I would think).

> In practice the config will still be written to .gitmodules, there are
> no plans to change the file path, but having this level of indirection
> makes it possible to perform additional checks before staging the file.

Again, a claim without explanation or justification.

If you are planning to something like

 - prepare trial contents in .gitmodules.new file
 - do whatever "additional checks" on .gitmodules.new
 - add the contents to it to the index as a new .gitmodules blob

Then you do not need such an option.  "submodule--helper" is purely
a helper for scripts, and not for human consumption, so scripts can
just hash-object the blob contents out and update-index --cacheinfo
to register the blob at any location of choice.

But as I said, this step is way under-explained, so my guess above
may not match what you really wanted to do.

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite 
> ---
>
> Note that the tests follow the predominant style in the file: subshell and cd
> right at the start of the sub-shell opening.
>
>  builtin/submodule--helper.c | 17 +
>  t/t7411-submodule-config.sh | 26 ++
>  2 files changed, 43 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..14f0845d30 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const 
> char **argv, const char *p
> return 0;
>  }
>
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +   if (argc < 2 || argc > 3)
> +   die("submodule--helper config takes 1 or 2 arguments: name 
> [value]");
> +
> +   /* Equivalent to ACTION_GET in builtin/config.c */
> +   if (argc == 2)
> +   return print_config_from_gitmodules(argv[1]);
> +
> +   /* Equivalent to ACTION_SET in builtin/config.c */
> +   if (argc == 3)
> +   return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +
> +   return 0;

Technically we cannot reach this point here?
Maybe it would be more defensive to

BUG("How did we get here?");

or at least return something !=0 ?

>
> +test_expect_success 'reading submodules config with "submodule--helper 
> config"' '

I shortly wondered if it would make sense to put these tests at the
beginning of either
this or a new file, as the functionality is rather fundamental. (I
never thought about
it, but actually it is better to go from common basic to more exotic
tests as you scroll
down the file), but this place is ok, if you choose to do so.

Thanks,
Stefan

Re: [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 18:54 schrieb Jeff King:
> PS I actually would have made the rule simply "does it begin with a
> '<'", which seems simpler still. If people accidentally write " forgetting to close their brackets, that is a bug under both the
> old and new behavior (just with slightly different outcomes).

Good point.  We could also extend it further and check if it contains
any special character, which would allow us to convert the remaining
user of the flag as well:

{OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
N_("override the executable bit of the listed files"),
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
chmod_callback},

Special characters are (, ), <, >, [, ], and |.

The idea is that we shouldn't automatically treat a string as a
simple replacement specifier if it looks like it has some structure
to it.

Side note: "(+/-)x" is marked for translation above.  Any translation
that is not identical would be wrong, though, because the command only
accepts a literal "+x" or "-x" in any locale.  So the N_ wrapper is
bogus, right?

Checked the output with that extended check by generating all help
pages with:

for cmd in $(git --list-cmds=parseopt)
do git-$cmd -h
done

... and found a few differences:

git add:
---chmod <(+/-)x>  override the executable bit of the listed files
+--chmod (+/-)xoverride the executable bit of the listed files

Good change.  We also should change the slash to a pipe.

git checkout-index:
---stage <1-3|all> copy out the files from named stage
+--stage 1-3|all   copy out the files from named stage

Good change, but perhaps mention number two explicitly?

git difftool:
--t, --tool <>   use the specified diff tool
+-t, --tool  use the specified diff tool
--x, --extcmd <>
+-x, --extcmd 

Aha, double angle brackets in the wild!  Good change.  We could
also remove the explicit pairs from the option definitions.

git pack-objects:
---index-version 
+--index-version version[,offset]

Not good before, worse after. Should be to "[,]".

git pull:
--r, --rebase[=]
+-r, --rebase[=false|true|merges|preserve|interactive]

Good change, but wouldn't we want to add a pair of parentheses around
the list of alternatives?

git push:
---force-with-lease[=:]
+--force-with-lease[=refname>:]
+--recurse-submodules[=check|on-demand|no]
---signed[=]
+--signed[=yes|no|if-asked]

git send-pack:
---signed[=]
+--signed[=yes|no|if-asked]

Good changes all three, but need parentheses..

---force-with-lease[=:]
+--force-with-lease[=refname>:] Linewrap output
+-w[w[,i1[,i2]]]   Linewrap output

Not good before, worse after.  Should be "[[,[,]]]".

git update-index:
---cacheinfo ,,
-  add the specified entry to the index
+--cacheinfo   add the specified entry to the index

Eh, what?  Ah, that option is defined with PARSE_OPT_NOARG, and we only
show argument help because PARSE_OPT_LITERAL_ARGHELP is also given, so
we need to keep that flag for this special option.

René

Re: git merge -s subtree seems to be broken.

[Jeff King]

On Tue, Jul 31, 2018 at 02:06:12PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:
> > ...
> > So here it is fixed, and with a commit message. I'm not happy to omit a
> > regression test, but I actually couldn't come up with a minimal one that
> > tickled the problem, because we're playing around with heuristics. So I
> > compensated by probably over-explaining in the commit message. But
> 
> Have you tried to apply the message yourself?  I'll fix it up but
> the hint to answer that question is in two extra pair of scissors.

Heh, thank you for noticing. I actually wondered about that while
writing it and meant to test, but then got distracted.

I wonder "am --scissors" should actually look for the _first_ scissors.
I guess that has the opposite problem, which is that we might include
too much cruft in an email that uses scissors. Perhaps "too much" is a
better failure mode than "too little", though.

-Peff

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[Junio C Hamano]

SZEDER Gábor  writes:

>> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
>> invalid lines in .gitmodules but then only the second commit is removed.
>> 
>> This may affect future subsequent tests if they assume that the
>> .gitmodules file has no errors.
>> 
>> Since those commits are not needed anymore remove both of them.
>> 
>> The modified line is in the last test of the file, so this does not
>> change the current behavior, it only affects future tests.
>> 
>> Signed-off-by: Antonio Ospite 
>> ---
>> 
>> Note that test_when_finished is not used here, both to keep the current style
>> and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Assuming that the operations to create these two extra commits
always succeed, yes, that would be a more robust and preferrable
option.

I don't know if that assumption hold true offhand, though.  Don't we
have a more stable point to anchor that going-back-to commit to?

Re: What's cooking in git.git (Jul 2018, #03; Wed, 25)

[Junio C Hamano]

Christian Couder  writes:

> Looking at the code of the patches again, I can't see any simple way
> to improve on the way it is done.

Thanks.

Re: Question regarding quarantine environments

[Jeff King]

On Thu, Aug 02, 2018 at 12:58:52PM -0500, Liam Decker wrote:

> I've been working on a git hook in golang recently. However, the library I
> was using did not support a possible quarantine directory, which I would
> use for my hook.
> 
> I have been trying to find out how git finds this incoming directory in the
> objects folder, as their code simply assumed it resided in
> .git/objects/<1st byte>/

When you're running a hook inside the quarantine environment, then
$GIT_OBJECT_DIRECTORY in the environment will be set to the quarantine
directory, and $GIT_ALTERNATE_OBJECT_DIRECTORIES will point to the main
repository object directory (possibly alongside other alternates, if
there were any already set).

Any Git commands you run should therefore find objects from either
location, but any writes would go to the quarantine (most notably, Git's
own index-pack/unpack-objects processes, which is the point of the
quarantine in the first place).

> The solution that I implemented was to check the objects directory for the
> object, and if it was not there, to look for a quarantine directory and try
> there. However, that feels fairly inefficient.

That's more or less what Git will do under the hood (though in the
opposite order).

> For the curious, the library and solution I attempted are both here [5]

Just skimming, but it sounds like go-git does not support the
GIT_OBJECT_DIRECTORY environment variable.

-Peff

Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained

[Jeff King]

On Thu, Aug 02, 2018 at 02:01:55PM -0400, Jeff King wrote:

> I suspect if we go with the one-spatch-per-source route, though, that we
> could do this just with regular make rules.

Yeah, it's pretty straightforward:

diff --git a/Makefile b/Makefile
index d616c0412..86fdcf567 100644
--- a/Makefile
+++ b/Makefile
@@ -2674,15 +2674,17 @@ COCCI_SOURCES = $(filter-out 
sha1collisiondetection/%,$(C_SOURCES))
 else
 COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
 endif
+COCCI_COMBINED = contrib/coccinelle/combined.cocci
+COCCI_SEM_PATCHES = $(filter-out $(COCCI_COMBINED), $(wildcard 
contrib/coccinelle/*.cocci))
 
-%.cocci.patch: %.cocci $(COCCI_SOURCES)
+$(COCCI_COMBINED): $(COCCI_SEM_PATCHES)
+   cat $^ >$@+
+   mv $@+ $@
+
+$(COCCI_COMBINED).patches/%.patch: % $(COCCI_COMBINED)
@echo '' SPATCH $<; \
-   ret=0; \
-   for f in $(COCCI_SOURCES); do \
-   $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
-   { ret=$$?; break; }; \
-   done >$@+ 2>$@.log; \
-   if test $$ret != 0; \
+   mkdir -p $(dir $@) || exit 1; \
+   if ! $(SPATCH) --sp-file $(COCCI_COMBINED) $< $(SPATCH_FLAGS) >$@+ 
2>$@.log; \
then \
cat $@.log; \
exit 1; \
@@ -2692,7 +2694,8 @@ endif
then \
echo '' SPATCH result: $@; \
fi
-coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))
+
+coccicheck: $(patsubst %, $(COCCI_COMBINED).patches/%.patch, $(COCCI_SOURCES))
 
 .PHONY: coccicheck
 
@@ -2907,7 +2910,7 @@ profile-clean:
$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
 
 cocciclean:
-   $(RM) contrib/coccinelle/*.cocci.patch*
+   $(RM) -r contrib/coccinelle/*.cocci.patches
 
 clean: profile-clean coverage-clean cocciclean
$(RM) *.res

I guess you could even replace "COCCI_COMBINED" with "COCCI_PATCH" in
most of the targets, and that would let people do individual:

  make COCCI_PATCH=contrib/coccinelle/foo.cocci coccicheck

The default would just be the concatenated version.

-Peff

Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Han-Wen Nienhuys  writes:

> The colorization is controlled with the config setting "color.remote".
> ...
> Finally, this solution is backwards compatible: many servers already
> prefix their messages with "error", and they will benefit from this
> change without requiring a server update. By contrast, a server-side
> solution would likely require plumbing the TERM variable through the
> git protocol, so it would require changes to both server and client.

Thanks; quite readable.

>
> Helped-by: Duy Nguyen 
> Signed-off-by: Han-Wen Nienhuys 
> ---
>  Documentation/config.txt|   9 +++
>  help.c  |   1 +
>  help.h  |   1 +
>  sideband.c  | 119 +---
>  t/t5409-colorize-remote-messages.sh |  47 +++
>  5 files changed, 168 insertions(+), 9 deletions(-)
>  create mode 100644 t/t5409-colorize-remote-messages.sh

I'll "chmod +x" while queuing.

Side note: When all problems pointed out are "I'll fix it
this way while queuing"-kind, and if you agree to the way I
plan to fix them up, then just saying so is sufficient and
you do not have to send a new version of the patch(es).

If your "make test" did not catch this as an error, then we may need
to fix t/Makefile, as it is supposed to run test-lint.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 43b2de7b5..0783323be 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
>  color.push.error::
>   Use customized color for push errors.
>  
> +color.remote::
> + A boolean to enable/disable colored remote output. If unset,
> + then the value of `color.ui` is used (`auto` by default).

Nobody tells the end-users what "colored remote output" does;
arguably they can find it out themselves by enabling the feature and
observing remote messages, but that is not user friendly.

When running commands like `git fetch` and `git push` that
interact with a remote repository, certain keywords (see
`color.remote.`) that appear at the beginning of a
line of message from the remote end can be painted in color.

This configuration variable is a boolean to enable/disable
the feature.  If unset...

or something like that, perhaps?

You use `always` in your tests to a good effect, but what the value
means is not described here.

> +color.remote.::
> + Use customized color for each remote keywords. `` may be

Isn't 'each' a singular, i.e. "for each remote keyword"?  If so I do
not mind dropping 's' myself while queuing.

> + `hint`, `warning`, `success` or `error` which match the
> + corresponding keyword.

We need to say that keywords are painted case insensitively
somewhere in the doc.  Either do that here, or in the updated
description of `color.remote`---I am not sure which one results in
more readable text offhand.

> diff --git a/sideband.c b/sideband.c
> index 325bf0e97..5c72db83c 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,6 +1,103 @@
>  #include "cache.h"
> +#include "color.h"
> +#include "config.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "help.h"
> +
> +struct kwtable {
> + /*
> +  * We use keyword as config key so it can't contain funny chars like
> +  * spaces. When we do that, we need a separate field for slot name in
> +  * load_sideband_colors().
> +  */
> + const char *keyword;
> + char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable keywords[] = {
> + { "hint",   GIT_COLOR_YELLOW },
> + { "warning",GIT_COLOR_BOLD_YELLOW },
> + { "success",GIT_COLOR_BOLD_GREEN },
> + { "error",  GIT_COLOR_BOLD_RED },
> +};
> +
> +// Returns a color setting (GIT_COLOR_NEVER, etc).
> +static int use_sideband_colors(void)
> +{
> + static int use_sideband_colors_cached = -1;
> +
> + const char *key = "color.remote";
> + struct strbuf sb = STRBUF_INIT;
> + char *value;
> + int i;
> +
> + if (use_sideband_colors_cached >= 0)
> + return use_sideband_colors_cached;
> +
> + if (!git_config_get_string(key, ))
> + use_sideband_colors_cached = git_config_colorbool(key, value);
> +
> + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> + strbuf_reset();
> + strbuf_addf(, "%s.%s", key, keywords[i].keyword);
> + if (git_config_get_string(sb.buf, ))
> + continue;
> + if (color_parse(value, keywords[i].color))
> + die(_("expecting a color: %s"), value);
> + }
> + strbuf_release();
> + return use_sideband_colors_cached;
> +}
> +
> +void list_config_color_sideband_slots(struct string_list *list, const char 
> *prefix)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(keywords); i++)
> + list_config_item(list, prefix, 

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[Stefan Beller]

On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor  wrote:
>
> > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> > invalid lines in .gitmodules but then only the second commit is removed.
> >
> > This may affect future subsequent tests if they assume that the
> > .gitmodules file has no errors.
> >
> > Since those commits are not needed anymore remove both of them.
> >
> > The modified line is in the last test of the file, so this does not
> > change the current behavior, it only affects future tests.
> >
> > Signed-off-by: Antonio Ospite 
> > ---
> >
> > Note that test_when_finished is not used here, both to keep the current 
> > style
> > and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Yeah that is a better way to do it.
Even better would be to have 2 of these for both tests 5 and 8,
such that each of them could be skipped individually and any following
tests still work fine.

> >  t/t7411-submodule-config.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index 0bde5850ac..248da0bc4f 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -135,7 +135,9 @@ test_expect_success 'error in history in 
> > fetchrecursesubmodule lets continue' '
> >   HEAD submodule \
> >   >actual &&
> >   test_cmp expect_error actual  &&
> > - git reset --hard HEAD^
> > + # Remove both the commits which add errors to .gitmodules,
> > + # the one from this test and the one from a previous test.
> > + git reset --hard HEAD~2

I am a bit hesitant to removing the commits though, as it is expected to have
potentially broken history and submodules still working.

The config --unset already fixes the gitmodules file,
so I think we can rather do

git commit -a -m 'now the .gitmodules file is fixed at HEAD \
but has a messy history'

But as I have only read up to here, not knowing what the future tests will
bring this is all speculation at this point.

Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite  wrote:
>
> This will be used to print values just like "git config -f .gitmodules"
> would.
>
> Signed-off-by: Antonio Ospite 
> ---
>  submodule-config.c | 25 +
>  submodule-config.h |  2 ++
>  2 files changed, 27 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 2a7259ba8b..6f6f5f9960 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
> submodule_cache_clear(r->submodule_cache);
>  }
>
> +static int config_print_callback(const char *key_, const char *value_, void 
> *cb_data)
> +{
> +   char *key = cb_data;
> +
> +   if (!strcmp(key, key_))
> +   printf("%s\n", value_);
> +
> +   return 0;
> +}
> +
> +int print_config_from_gitmodules(const char *key)
> +{
> +   int ret;
> +   char *store_key;
> +
> +   ret = git_config_parse_key(key, _key, NULL);
> +   if (ret < 0)
> +   return CONFIG_INVALID_KEY;
> +
> +   config_from_gitmodules(config_print_callback, the_repository, 
> store_key);
> +
> +   free(store_key);
> +   return 0;
> +}
> +
>  struct fetch_config {
> int *max_children;
> int *recurse_submodules;
> diff --git a/submodule-config.h b/submodule-config.h
> index dc7278eea4..6fec3caadd 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
>   */
>  int check_submodule_name(const char *name);
>
> +extern int print_config_from_gitmodules(const char *key);

The only real pushback for this patch I'd have is lack of documentation
in public functions, though this is pretty self explanatory; so I'd be fine
for lacking the docs here.

In case a resend is needed, please drop the extern keyword here.

Thanks,
Stefan

Re: [PATCH v2] checkout: optimize "git checkout -b "

[Ben Peart]

On 8/1/2018 11:10 AM, Duy Nguyen wrote:

On Tue, Jul 31, 2018 at 7:03 PM Ben Peart  wrote:


From: Ben Peart 

Skip merging the commit, updating the index and working directory if and
only if we are creating a new branch via "git checkout -b ."
Any other checkout options will still go through the former code path.


I'd like to see this giant list of checks broken down and pushed down
to smaller areas so that chances of new things being added but checks
not updated become much smaller. And ideally there should just be no
behavior change (I think with your change, "checkout -b" will not
report local changes, but it's not mentioned in the config document;
more things like that can easily slip).



One trade off of pushing these optimizations down into the lower-level 
functions is that they have a greater potential to break other command 
if our assumptions are wrong.  Changing these low level functions is a 
much more invasive set of patches.


I didn't feel confident enough to pursue this path and instead, decided 
to do the single, high level optimization around the specific scenario. 
While it has its own drawbacks (the nasty set of conditions we're 
testing), the potential for breaking other commands is much smaller.


That said, I'm willing to look into the model of pushing the 
checks/optimizations down to smaller areas if we can 1) ensure we aren't 
breaking other commands and 2) we can get similar performance.



So. I assume this reason for this patch is because on super large worktrees

  - 2-way merge is too slow
  - applying spare checkout patterns on a huge worktree is also slow
  - writing index is, again, slow
  - show_local_changes() slow



That is pretty close but here is some real data on a large repo.

"git checkout -b " with this patch takes 0.32 seconds.
"git checkout -b " without this patch takes 14.6 seconds.

Note, all numbers are with a hot disk cache - real world numbers for the 
unpatched case can be much worse as it has to do a lot of disk IO to 
read/write the 267 MB index, load 500K+ tree objects, etc:


NameInc % Inc
 ||+ git!mainCRTStartup  89.2  13,380
 || + git!__tmainCRTStartup  89.2  13,380
 ||  + git!cmd_main  89.2  13,380
 ||   + git!handle_builtin   89.2  13,380
 ||+ git!cmd_checkout89.2  13,380
 || + git!unpack_trees   71.5  10,725
 || |+ git!traverse_trees39.7   5,956
 || |+ git!cache_tree_update 16.1   2,408
 || |+ git!??unpack_callback 11.0   1,649
 || |+ git!discard_index  2.8 423
 || + git!write_locked_index  8.4   1,257
 || + git!??cache_tree_invalidate_path5.1 767
 || + git!read_index_preload  3.4 514


For 2-way merge, I believe we can detect inside unpack_trees() that
it's a 2-way merge (fn == twoway_merge), from HEAD to HEAD (simple
enough check), then from the 2-way merge table we know for sure
nothing is going to change and we can just skip traverse_trees() call
in unpack_trees().



If we can skip the call to traverse_trees(), that will give us the bulk 
of the savings (39.7% + 11% = 50.7% if my math is correct).



On the sparse checkout application. This only needs to be done when
there are new files added, or the spare-checkout file has been updated
since the last time it's been used. We can keep track of these things
(sparse-checkout file change could be kept track with just stat info
maybe as an index extension) then we can skip applying sparse checkout
not for this particular case for but general checkouts as well. Spare
checkout file rarely changes. Big win overall.



With the current patch, we don't need to load or update the index at 
all.  Without the patch, we've already replaced the standard 
sparse-checkout logic with something significantly faster so in our 
particular case, I think it's safe to skip the additional complexity of 
keeping track of changes to the sparse-checkout file.



And if all go according to plan, there will be no changes made in the
index (by either 2-way merge or sparse checkout stuff) we should be
able to just skip writing down the index, if we haven't done that
already.



That would be great as writing the index is 8.4% of the time spent.


show_local_changes() should be sped up significantly with the new
cache-tree optimization I'm working on in another thread.



As you can see, updating the cache_tree is relatively expensive (16.1% + 
5.1%) so we would definitely benefit from any improvements there.



If I have not made any mistake in my analysis so far, we achieve a big
speedup without adding a new config knob and can still fall back to
slower-but-same-behavior when things are not in the right condition. I
know 

Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained

[Jeff King]

On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:

> Let's add a bit of Makefile metaprogramming to generate finer-grained
> make targets applying one semantic patch to only a single source file,
> and specify these as dependencies of the targets applying one semantic
> patch to all source files.  This way that shell loop can be avoided,
> semantic patches will only be applied to changed source files, and the
> same semantic patch can be applied in parallel to multiple source
> files.  The only remaining sequential part is aggregating the
> suggested transformations from the individual targets into a single
> patch file, which is comparatively cheap (especially since ideally
> there aren't any suggestions).
> 
> This change brings spectacular speedup in the scenario described in
> point (1) above.  When the results of a previous 'make coccicheck' are
> there, the time needed to run
> 
>   touch apply.c ; time make -j4 coccicheck
> 
> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and
> 'apply.c' is the second longest source file in our codebase.  In the
> scenario in point (2), running
> 
>   touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck
> 
> went from 56.364s to 35.883s, which is ~36% speedup.

I really like this direction. The slowness of coccicheck is one of the
things that has prevented me from running it more frequently. And I'm a
big fan of breaking steps down as much as possible into make targets. It
lets make do its job (avoiding repeated work and parallelizing).

> All the above timings are best-of-five on a machine with 2 physical
> and 2 logical cores.  I don't have the hardware to bring any numbers
> for point (3).  The time needed to run 'make -j4 coccicheck' in a
> clean state didn't change, it's ~3m42s both with and without this
> patch.

On a 40-core (20+20) machine, doing "make -j40 coccicheck" from scratch
went from:

  real  1m25.520s
  user  5m41.492s
  sys   0m26.776s

to:

  real  0m24.300s
  user  14m35.084s
  sys   0m50.108s

I was surprised by the jump in CPU times. Doing "make -j1 coccicheck"
with your patch results in:

  real  5m34.887s
  user  5m5.620s
  sys   0m19.908s

so it's really the parallelizing that seems to be to blame (possibly
because this CPU boosts from 2.3Ghz to 3.0Ghz, and we're only using 8
threads in the first example).

>   - [RFC]
> With this patch 'make coccicheck's output will look like this
> ('make' apparently doesn't iterate over semantic patches in
> lexicographical order):
> 
>   SPATCH commit.cocci  abspath.c
>   SPATCH commit.cocci  advice.c
>   <... lots of lines ...>
>   SPATCH array.cocci   http-walker.c
>   SPATCH array.cocci   remote-curl.c
> 
> which means that the number of lines in the output grows from
> "Nr-of-semantic-patches" to "Nr-of-semantic-patches *
> Nr-of-source-files".

IMHO this is not really that big a deal. We are doing useful work for
each line, so to me it's just more eye candy (and it's really satisfying
to watch it zip by on the 40-core machine ;) ).

What if we inverted the current loop? That is, right now we iterate over
the cocci patches at the Makefile level, and then for each target we
iterate over the giant list of source files. Instead, we could teach the
Makefile to iterate over the source files, with a target for each, and
then hit each cocci patch inside there.

That would give roughly the same output as a normal build. But moreover,
I wonder if we could make things faster by actually combining the cocci
files into a single set of rules to be applied to each source file. That
would mean invoking spatch 1/8 as much. It does give fewer opportunities
for "make" to reuse work, but that only matters when the cocci files
change (which is much less frequent than source files changing).

Doing:

  cat contrib/coccinelle/*.cocci >mega.cocci
  make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci

gives me:

  real  0m17.573s
  user  10m56.724s
  sys   0m11.548s

And that should show an improvement on more normal-sized systems, too,
because we really are eliminating some of the startup overhead.

The other obvious downside is that you don't get individual patches for
each class of transformation. Do we care? Certainly for a routine "make
coccicheck" I primarily want to know:

  - is there something that needs fixing?

  - give me the patch for all fixes

So I wonder if we'd want to have that be the default, and then perhaps
optionally give some targets to let people run single scripts (or not;
they could probably just run spatch themselves).

>   - [RFC]
> The overhead of applying a semantic patch to all source files
> became larger.  'make coccicheck' currently runs only one shell
> process and creates two output files for each semantic patch.
> With this patch it will run approx.  "Nr-of-semantic-patches *
> Nr-of-source-files" shell processes and create twice as 

Question regarding quarantine environments

[Liam Decker]

Hi all,

I've been working on a git hook in golang recently. However, the library I
was using did not support a possible quarantine directory, which I would
use for my hook.

I have been trying to find out how git finds this incoming directory in the
objects folder, as their code simply assumed it resided in
.git/objects/<1st byte>/
I read the documentation describing the git repository layout here [1] as
well as the objects documentation here [2] and the git hooks documentation
here [3] directed me to the receive-pack documentation here [4]
I have also tried googling, but this is a pretty specific question

The solution that I implemented was to check the objects directory for the
object, and if it was not there, to look for a quarantine directory and try
there. However, that feels fairly inefficient.
For the curious, the library and solution I attempted are both here [5]

If anyone could help direct me to find specifically how git looks for
objects in the repository, I would be very grateful

[1] https://git-scm.com/docs/gitrepository-layout
[2] https://git-scm.com/book/en/v2/Git-Internals-Git-Objects
[3] https://git-scm.com/docs/githooks
[4] https://git-scm.com/docs/git-receive-pack
[5] https://github.com/src-d/go-git/pull/887

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Eric Sunshine]

On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano  wrote:
> Johannes Schindelin  writes:
> > ACK!
>
> Did you write a buggy caller that would have been caught or helped
> with this change?  You did not write the callee that is made more
> defensive with this patch, so I am being curious as to where that
> Ack is coming from (I wouldn't have felt curious if this were
> a reviewed-by instead).

The code being made more defensive with this patch was authored by Dscho[1].

[1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)

Re: [PATCH 2/2] Highlight keywords in remote sideband output.

[Stefan Beller]

On Thu, Aug 2, 2018 at 10:37 AM Eric Sunshine  wrote:
>
> On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys  wrote:
> > Sure. My doubt is that it's hard to tell what the state of my patch is
> > at any given time.
>
> Understandable. Junio sends out a periodic "What's cooking" email
> summarizing the state of patches sent to the list. The most recent one
> is here [1].

Please contrast that with [2], specifically:

  The discussion thread in the list archive is
  the authoritative record of the discussion; treat "What's cooking"
  as my personal note, nothing more.

[2] https://public-inbox.org/git/xmqqsh3x69ap@gitster-ct.c.googlers.com/

Re: [PATCH] color: protect against out-of-bounds array access/assignment

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Eric,
>
> On Thu, 2 Aug 2018, Eric Sunshine wrote:
>
>> want_color_fd() is designed to work only with standard input, output,
>> and error file descriptors, and stores information about each descriptor
>> in an array. However, it doesn't verify that the passed-in descriptor
>> lives within that set, which, with a buggy caller, could lead to
>> access/assignment outside the array bounds.
>
> ACK!
>
> Thanks,
> Dscho

Did you write a buggy caller that would have been caught or helped
with this change?  You did not write the callee that is made more
defensive with this patch, so I am being curious as to where that
Ack is coming from (I wouldn't have felt curious if this were
a reviewed-by instead).

In any case, this looks like a good defensive measure.

Re: [PATCH 2/2] Highlight keywords in remote sideband output.

[Eric Sunshine]

On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys  wrote:
> Sure. My doubt is that it's hard to tell what the state of my patch is
> at any given time.

Understandable. Junio sends out a periodic "What's cooking" email
summarizing the state of patches sent to the list. The most recent one
is here [1].

A patch series may be annotated with "will merge to next" (meaning he
thinks it's stable and probably ready to expose to a wider audience),
"waiting for review" (meaning not enough people have shown interest to
look it over), "waiting for a re-roll" (meaning he expects a new
version). Other annotations are possible. If a patch series isn't
annotated, it means he hasn't made any decision about it yet.

Also, if your patch series is not in "What's cooking", it may mean
that Junio just hasn't gotten around to your submission yet, and there
is a good chance it will be in the following "What's cooking". If,
however, a couple weeks or more elapse and your submission doesn't
show up, then it may have gotten lost in the noise, in which case it's
a good idea to send a "ping" as a reminder.

[1]: https://public-inbox.org/git/xmqqd0vbt14e@gitster-ct.c.googlers.com/

Re: [PATCH 1/2] config: document git config getter return value.

[Junio C Hamano]

Han-Wen Nienhuys  writes:

> ---
>  config.h | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

Should I forge your sign-off on this patch?

>
> diff --git a/config.h b/config.h
> index b95bb7649..41700f40b 100644
> --- a/config.h
> +++ b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
>  };
>  
>  extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **value);
> +
>  extern const struct string_list *git_configset_get_value_multi(struct 
> config_set *cs, const char *key);
>  extern void git_configset_clear(struct config_set *cs);
> +
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);
> +extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **dest);
>  extern int git_configset_get_string_const(struct config_set *cs, const char 
> *key, const char **dest);
>  extern int git_configset_get_string(struct config_set *cs, const char *key, 
> char **dest);
>  extern int git_configset_get_int(struct config_set *cs, const char *key, int 
> *dest);

Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Single quotes should be escaped as \' not \\'. The bad quoting breaks
> the interactive version of 'rebase --root' (which is used when there is
> no '--onto' even if the user does not specify --interactive) for authors
> that contain "'" as sq_dequote() called read_author_ident() errors out
> on the bad quoting.
>
> For other interactive rebases this only affects external scripts that
> read the author script and users whose git is upgraded from the shell
> version of rebase -i while rebase was stopped when the author contains
> "'". This is because the parsing in read_env_script() expected the
> broken quoting.

I wasn't following the discussion, but is it the general consensus
that reading the broken a-i file is a requirement for the new code?
Not an objection phrased as a question.

I do not think it is worth worrying about the "upgrade while rebase
was in progress" case, if it involves much more code than necessary
without its support, especially if the only thing the user needs to
do recover from such a situation is to say "rebase --abort" and then
to retry the same rebase with the fixed version that was installed
in the meantime.  Let's see how much we need to bend over backwards
to do this "transition" thing.

> Ideally rebase and am would share the same code for reading and
> writing the author script, but this commit just fixes the immediate
> bug.

OK.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 06a7b79307..c1e3f947a5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -880,7 +880,7 @@ init_basic_state () {
>   mkdir -p "$state_dir" || die "$(eval_gettext "Could not create 
> temporary \$state_dir")"
>   rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>  
> - : > "$state_dir"/interactive || die "$(gettext "Could not mark as 
> interactive")"
> + echo 1 > "$state_dir"/interactive || die "$(gettext "Could not mark as 
> interactive")"

This impacts the work Alban is doing, which at the end removes this
script altogether.

> +/*
> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE line 
> with
> + * a "'" and also escaped "'" incorrectly as "'''" rather than "'\\''". 
> Fix

I think the comment here (for both the wrong and the right versions)
is easier to read if you wrote the string as literal without C, i.e.
The string is "'\\''" but as a string literal in C it is expressed
as "'''".

> +static int fix_bad_author_script(struct strbuf *script)
> +{
> + const char *next;
> + size_t off = 0;
> +
> + while ((next = strstr(script->buf + off, "'''"))) {

This looks brittle.

We need assurance that the first "'\\''" we see on the line came
from the attempt by the broken writer to write out a single "'", and
not from anything else.  The broken writer places its own "'"
immediately after GIT_AUTHOR_NAME= (just like the corrected one
does) before moving on to the end-user payload.  Can the single
quote at the beginning of the substring you are looking for be that
one?  If the end user's payload began with two backslashes, that
would have produced a result that matches the first three bytes of
the substring you are looking for.  But there is no way for the
end-user payload to make the next two bytes "''"---any byte other
than a sq would result in a sq added to the result, and a byte that
is a sq would give one sq followed by a bs.

OK, so this is probably doing the right thing, as long as we know we
are reading from the old and broken writer.  It still does look
unnecessarily ugly and over-engineered to have this (and the
"version" reading code), though, at least to me, but perhaps it is
just me.

Thanks.

Re: [PATCH 1/2] config: document git config getter return value.

[Eric Sunshine]

On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys  wrote:
> diff --git a/config.h b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
>  extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **value);
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char 
> *filename);
> +extern int git_configset_get_value(struct config_set *cs, const char *key, 
> const char **dest);
>  extern int git_configset_get_string_const(struct config_set *cs, const char 
> *key, const char **dest);
>  extern int git_configset_get_string(struct config_set *cs, const char *key, 
> char **dest);
>  extern int git_configset_get_int(struct config_set *cs, const char *key, int 
> *dest);

It doesn't seem like git_configset_add_file() fits in this group. It's
neither searching for something (thus won't return "found" / "not
found"), nor is it returning 0 or 1. (It returns 0 on success and -1
on failure.)

Re: [PATCH] push: comment on a funny unbalanced option help

[Jeff King]

On Thu, Aug 02, 2018 at 08:31:57AM -0700, Junio C Hamano wrote:

> > diff --git a/parse-options.c b/parse-options.c
> > index 7db84227ab..fadfc6a833 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const 
> > char *prefix,
> >  static int usage_argh(const struct option *opts, FILE *outfile)
> >  {
> > const char *s;
> > -   int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> > +   int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh 
> > ||
> > +   (opts->argh[0] == '<' && strchr(opts->argh, '>'));
> 
> You are greedier than what I thought ;-) I would have limited this
> magic to the case where argh is surrounded by "<>", but you allow
> one closing ">" anywhere, which allows us to grab more complex cases.
> 
> The lack of escape hatch to decline this auto-literal magic makes me
> somewhat nervous, but I do not offhand think of a reason why I do
> want an arg-help string that _begins_ with '<' to be enclosed by
> another set of "<>", so perhaps this is a good kind of magic.

I think that case is actually easy; once the caller provides a "<>",
they are in "literal" mode, so they are free to add extra brackets if
you want. I.e., any "bar" that you do want enclosed could be
spelled "". It's the opposite that becomes impossible: you do
not have an opening bracket and nor do you want one.  But as long as we
retain LITERAL_ARGHELP for that case, we have an escape hatch.

I was going to suggest that this conversion has the downside of being
somewhat one-way. That is, it is easy for us to find affected sites now
since they contain the string LITERAL_ARGHELP. Whereas if we wanted to
back it out in the future, it is hard to know which sites are depending
on this new behavior.

But I am having trouble thinking of a reason we would want to back it
out. This makes most cases easier, and has a good escape hatch.

-Peff

PS I actually would have made the rule simply "does it begin with a
   '<'", which seems simpler still. If people accidentally write "

Re: [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 17:31 schrieb Junio C Hamano:
> René Scharfe  writes:
>> diff --git a/parse-options.c b/parse-options.c
>> index 7db84227ab..fadfc6a833 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const 
>> char *prefix,
>>   static int usage_argh(const struct option *opts, FILE *outfile)
>>   {
>>  const char *s;
>> -int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
>> +int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh 
>> ||
>> +(opts->argh[0] == '<' && strchr(opts->argh, '>'));
> 
> You are greedier than what I thought ;-) I would have limited this
> magic to the case where argh is surrounded by "<>", but you allow
> one closing ">" anywhere, which allows us to grab more complex cases.

That's what I had initially, but only one of the current cases would
have benefited from that strict behavior, so it's not useful enough.

> The lack of escape hatch to decline this auto-literal magic makes me
> somewhat nervous, but I do not offhand think of a reason why I do
> want an arg-help string that _begins_ with '<' to be enclosed by
> another set of "<>", so perhaps this is a good kind of magic.

The escape hatch is to add the extra pair manually to the argh string.

René

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[SZEDER Gábor]

> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
> 
> This may affect future subsequent tests if they assume that the
> .gitmodules file has no errors.
> 
> Since those commits are not needed anymore remove both of them.
> 
> The modified line is in the last test of the file, so this does not
> change the current behavior, it only affects future tests.
> 
> Signed-off-by: Antonio Ospite 
> ---
> 
> Note that test_when_finished is not used here, both to keep the current style
> and also because it does not work in sub-shells.

That's true, but I think that this:

  test_when_finished git -C super reset --hard HEAD~2

at the very beginning of the test should work.

>  t/t7411-submodule-config.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..248da0bc4f 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,9 @@ test_expect_success 'error in history in 
> fetchrecursesubmodule lets continue' '
>   HEAD submodule \
>   >actual &&
>   test_cmp expect_error actual  &&
> - git reset --hard HEAD^
> + # Remove both the commits which add errors to .gitmodules,
> + # the one from this test and the one from a previous test.
> + git reset --hard HEAD~2
>   )
>  '
>  
> -- 
> 2.18.0
> 
> 

Re: [PATCH] fetch-pack: unify ref in and out param

[Jeff King]

On Wed, Aug 01, 2018 at 01:13:20PM -0700, Jonathan Tan wrote:

> When a user fetches:
>  - at least one up-to-date ref and at least one non-up-to-date ref,
>  - using HTTP with protocol v0 (or something else that uses the fetch
>command of a remote helper)
> some refs might not be updated after the fetch.
> 
> This bug was introduced in commit 989b8c4452 ("fetch-pack: put shallow
> info in output parameter", 2018-06-28) which allowed transports to
> report the refs that they have fetched in a new out-parameter
> "fetched_refs". If they do so, transport_fetch_refs() makes this
> information available to its caller.
> 
> Users of "fetched_refs" rely on the following 3 properties:
>  (1) it is the complete list of refs that was passed to
>  transport_fetch_refs(),
>  (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if
>  relevant), and
>  (3) it has updated OIDs if ref-in-want was used (introduced after
>  989b8c4452).
> [...]

Thanks, this is a very clear and well-organized commit message. It
answers my questions, and I agree with the general notion of "we can
figure out the right API for ref patterns later" approach.

>  builtin/clone.c |  4 ++--
>  builtin/fetch.c | 28 
>  fetch-object.c  |  2 +-
>  fetch-pack.c| 30 +++---
>  t/t5551-http-fetch-smart.sh | 18 ++
>  transport-helper.c  |  6 ++
>  transport-internal.h|  9 +
>  transport.c | 34 ++
>  transport.h |  3 +--

The patch itself looks sane to me, and obviously fixes the problem. I
cannot offhand think of any reason that munging the existing list would
be a problem (though it has been a while since I have dealt with this
code, so take that with the appropriate grain of salt).

-Peff

Re: [PATCH] transport: report refs only if transport does

[Jeff King]

On Tue, Jul 31, 2018 at 04:23:43PM -0700, Jonathan Tan wrote:

> > > Because transport_fetch_refs() filters the refs sent to the transport,
> > > it cannot just report the transport's result directly, but first needs
> > > to readd the excluded refs, pretending that they are fetched. However,
> > > this results in a wrong result if the transport did not report the refs
> > > that they have fetched in "fetched_refs" - the excluded refs would be
> > > added and reported, presenting an incomplete picture to the caller.
> > 
> > This part leaves me confused. If we are not fetching them, then why do
> > we need to pretend that they are fetched?
> 
> The short answer is that we need:
>  (1) the complete list of refs that was passed to
>  transport_fetch_refs(),
>  (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if
>  relevant), and
>  (3) with updated OIDs if ref-in-want was used.
> 
> The fetched_refs out param already fulfils (2) and (3), and this patch
> makes it fulfil (1). As for calling them fetched_refs, perhaps that is a
> misnomer, but they do appear in FETCH_HEAD even though they are not
> truly fetched.

Thanks for this explanation. It does make more sense to me now, and I
agree that a lot of my confusion was from calling it "fetched_refs" (and
the comment saying "reported as fetched, but not actually fetched").

> Which raises the question...if completeness is so important, why not
> reuse the input list of refs and document that transport_fetch_refs()
> can mutate the input list? You ask the same question below, so I'll put
> the answer after quoting your paragraph.
> [...]

Thanks, the answer here was enlightening as well.

I see you posted a patch to go back to mutating the list, and that seems
reasonable to me. I'm fine with a separate "out" list, too. Its purpose
and expectations just need to be reflected in the name (and possibly in
a comment).

-Peff

Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems

[Junio C Hamano]

Duy Nguyen  writes:

>> But even if inum is unreliable, we should be able to use other
>> clues, perhaps the same set of fields we use for cached stat
>> matching optimization we use for "diff" plumbing commands, to
>> implement the error report.  The more important part of the idea is
>> that we already need to notice that we need to remove a path that is
>> in the working tree while doing the checkout, so the alternative
>> approach won't incur any extra cost for normal cases where the
>> project being checked out does not have two files whose pathnames
>> are only different in case (or checking out such an offending
>> project to a case sensitive filesytem, of course).
>>
>> So I guess it still _is_ workable.  Any takers?
>
> OK so we're going back to the original way of checking that we check
> out the different files on the same place (because fs is icase) and
> try to collect all paths for reporting, yes? I can give it another go
> (but of course if anybody else steps up, I'd very gladly hand this
> over)

Detect and report, definitely yes; I am not sure about collect all
(personally I am OK if we stopped at reporting "I tried to check out
X but your project tree has something else that is turned to X by
your pathname-smashing filesystem" without making it a requirement
to report what the other one that conflict with X is.  Of course,
reporting the other side _is_ nicer and I'd be happier if we can do
so without too much ugly code, but I do not think it is a hard
requirement.

Thanks.

Re: Re* [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

René Scharfe  writes:

> Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
>> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced 
>> brackets
>> From: Ævar Arnfjörð Bjarmason 
>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>> ...
>> official escape hatch instead.
>> 
>> Helped-by: René Scharfe 
>
> I didn't do anything for this particular patch so far?  But...
>
>> Signed-off-by: Junio C Hamano 

Yeah, I realized it after I sent it out.  The line has been replaced
with a forged sign-off from Ævar.

>> ---
>>   builtin/push.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 1c28427d82..ef4032a9ef 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char 
>> *prefix)
>>  OPT_BIT( 0,  "porcelain", , N_("machine-readable 
>> output"), TRANSPORT_PUSH_PORCELAIN),
>>  OPT_BIT('f', "force", , N_("force updates"), 
>> TRANSPORT_PUSH_FORCE),
>>  { OPTION_CALLBACK,
>> -  0, CAS_OPT_NAME, , N_("refname>:> +  0, CAS_OPT_NAME, , N_(":"),
>
> ... shouldn't we use this opportunity to document that "expect" is
> optional?

I consider that it is a separate topic.

I thought that we achieved a consensus that making the code guess
missing ":" is a misfeature that should be deprecated (in
which case we would not want to "s|:|[&]|"), but I may be
misremembering it.

Re: Re* [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced 
> brackets
> From: Ævar Arnfjörð Bjarmason 
> Date: Thu, 02 Aug 2018 00:31:33 +0200
> 
> The option help text for the force-with-lease option to "git push"
> reads like this:
> 
>  $ git push -h 2>&1 | grep -e force-with-lease
> --force-with-lease[=:]
> 
> which comes from having N_("refname>: text in the source code, with an aparent lack of "<" and ">" at both
> ends.
> 
> It turns out that parse-options machinery takes the whole string and
> encloses it inside a pair of "<>", to make it easier for majority
> cases that uses a single token placeholder.
> 
> The help string was written in a funnily unbalanced way knowing that
> the end result would balance out, by somebody who forgot the
> presence of PARSE_OPT_LITERAL_ARGHELP, which is the escape hatch
> mechanism designed to help such a case.  We just should use the
> official escape hatch instead.
> 
> Helped-by: René Scharfe 

I didn't do anything for this particular patch so far?  But...

> Signed-off-by: Junio C Hamano 
> ---
>   builtin/push.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/push.c b/builtin/push.c
> index 1c28427d82..ef4032a9ef 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char 
> *prefix)
>   OPT_BIT( 0,  "porcelain", , N_("machine-readable 
> output"), TRANSPORT_PUSH_PORCELAIN),
>   OPT_BIT('f', "force", , N_("force updates"), 
> TRANSPORT_PUSH_FORCE),
>   { OPTION_CALLBACK,
> -   0, CAS_OPT_NAME, , N_("refname>: +   0, CAS_OPT_NAME, , N_(":"),

... shouldn't we use this opportunity to document that "expect" is
optional?

+ 0, CAS_OPT_NAME, , N_("[:]"),

> N_("require old value of ref to be at this value"),
> -   PARSE_OPT_OPTARG, parseopt_push_cas_option },
> +   PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, 
> parseopt_push_cas_option },
>   { OPTION_CALLBACK, 0, "recurse-submodules", 
> _submodules, "check|on-demand|no",
>   N_("control recursive pushing of submodules"),
>   PARSE_OPT_OPTARG, option_parse_recurse_submodules },
> 

Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Aug 1, 2018 at 12:13 PM Junio C Hamano  wrote:
>>
>> Stefan Beller  writes:
>>
>> > Stefan Beller (8):
>> >   test_decode_color: understand FAINT and ITALIC
>> >   t3206: add color test for range-diff --dual-color
>> >   diff.c: simplify caller of emit_line_0
>> >   diff.c: reorder arguments for emit_line_ws_markup
>> >   diff.c: add set_sign to emit_line_0
>> >   diff: use emit_line_0 once per line
>> >   diff.c: compute reverse locally in emit_line_0
>> >   diff.c: rewrite emit_line_0 more understandably
>> >
>> >  diff.c  | 94 +++--
>> >  t/t3206-range-diff.sh   | 39 +
>> >  t/test-lib-functions.sh |  2 +
>> >  3 files changed, 93 insertions(+), 42 deletions(-)
>>
>> As I cannot merge this as is to 'pu' and keep going, I'll queue the
>> following to the tip.  I think it can be squashed to "t3206: add
>> color test" but for now they will remain separate patches.
>>
>> Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color
>
> Thanks for dealing with my stubbornness here.

I didn't know you were stubborn---I just thought you were busy in
other things.

> I assumed that the contribution would be a one time hassle
> during git-am, not an ongoing problem during the whole time
> the patch flows through pu/next/master, which is why I punted
> on doing this change and resending in a timely manner.

It is a bit worse, in that it won't be at pu/next/master boundary
but each and every time the integration branches are rebuilt, which
happens a few times a day.  Whitespace breakage after a merge is
detected and my automation stops to ask for manual inspection.

Re* [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

Junio C Hamano  writes:

> Ævar Arnfjörð Bjarmason  writes:
>
>>> + /* N_() will get "<>" around, resulting in 
>>> ":" */
>>
>> ...but this comment isn't accurate at all, N_() doesn't wrap the string
>> with <>'s, as can be seen by applying this patch:
>
> I know.  It is a short-hand for "what's inside N_() we see here".
> Try to come up with an equivalent that fits on a 80-char line.

OK, let's scrap the comment patch but do this instead.

If we decide to take René's "auto-literal" change, the update to the
arg help string in this patch will become a necessary preparatory
step, even though "auto-literal" will make the addition of the
PARSE_OPT_LITERAL_ARGHELP bit redundant.

-- >8 --
Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced 
brackets
From: Ævar Arnfjörð Bjarmason 
Date: Thu, 02 Aug 2018 00:31:33 +0200

The option help text for the force-with-lease option to "git push"
reads like this:

$ git push -h 2>&1 | grep -e force-with-lease
   --force-with-lease[=:]

which comes from having N_("refname>:" at both
ends.

It turns out that parse-options machinery takes the whole string and
encloses it inside a pair of "<>", to make it easier for majority
cases that uses a single token placeholder.  

The help string was written in a funnily unbalanced way knowing that
the end result would balance out, by somebody who forgot the
presence of PARSE_OPT_LITERAL_ARGHELP, which is the escape hatch
mechanism designed to help such a case.  We just should use the
official escape hatch instead.

Helped-by: René Scharfe 
Signed-off-by: Junio C Hamano 
---
 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 1c28427d82..ef4032a9ef 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
OPT_BIT( 0,  "porcelain", , N_("machine-readable 
output"), TRANSPORT_PUSH_PORCELAIN),
OPT_BIT('f', "force", , N_("force updates"), 
TRANSPORT_PUSH_FORCE),
{ OPTION_CALLBACK,
- 0, CAS_OPT_NAME, , N_("refname>::"),
  N_("require old value of ref to be at this value"),
- PARSE_OPT_OPTARG, parseopt_push_cas_option },
+ PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, 
parseopt_push_cas_option },
{ OPTION_CALLBACK, 0, "recurse-submodules", 
_submodules, "check|on-demand|no",
N_("control recursive pushing of submodules"),
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
-- 
2.18.0-321-gffc6fa0e39

Re: [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

René Scharfe  writes:

> We could check if argh comes with its own angle brackets already and
> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
> redundant in most cases, including the one above.  Any downsides?
> Too magical?

Hmph.

> -- >8 --
> Subject: [PATCH] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP
>
> Avoid adding an extra pair of angular brackets if the argh string
> already contains one.  Remove the flag PARSE_OPT_LITERAL_ARGHELP in the
> cases where the new automatic handling suffices.  This shortens and
> simplifies option definitions with special argument help strings a bit.
>
> Signed-off-by: Rene Scharfe 
> ---

>   { OPTION_STRING, 0, "prefix", , 
> N_("/"),
>   { OPTION_CALLBACK, 'g', "reflog", _base, 
> N_("[,]"),
>   N_(",,"),
> + OPT_STRING(0, "prefix", , N_("/"),

Hmph.

> diff --git a/parse-options.c b/parse-options.c
> index 7db84227ab..fadfc6a833 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char 
> *prefix,
>  static int usage_argh(const struct option *opts, FILE *outfile)
>  {
>   const char *s;
> - int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> + int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh 
> ||
> + (opts->argh[0] == '<' && strchr(opts->argh, '>'));

You are greedier than what I thought ;-) I would have limited this
magic to the case where argh is surrounded by "<>", but you allow
one closing ">" anywhere, which allows us to grab more complex cases.

The lack of escape hatch to decline this auto-literal magic makes me
somewhat nervous, but I do not offhand think of a reason why I do
want an arg-help string that _begins_ with '<' to be enclosed by
another set of "<>", so perhaps this is a good kind of magic.

Re: [PATCH] push: comment on a funny unbalanced option help

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, Aug 02 2018, René Scharfe wrote:
>
>> Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
>>> But looking at this again it looks like this whole thing should just be
>>> replaced by:
>>>
>>>  diff --git a/builtin/push.c b/builtin/push.c
>>>  index 9cd8e8cd56..b8fa15c101 100644
>>>  --- a/builtin/push.c
>>>  +++ b/builtin/push.c
>>>  @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const 
>>> char *prefix)
>>>  OPT_BIT( 0,  "porcelain", , N_("machine-readable 
>>> output"), TRANSPORT_PUSH_PORCELAIN),
>>>  OPT_BIT('f', "force", , N_("force updates"), 
>>> TRANSPORT_PUSH_FORCE),
>>>  { OPTION_CALLBACK,
>>>  - 0, CAS_OPT_NAME, , N_("refname>:>>  + 0, CAS_OPT_NAME, , N_(":"),
>>>N_("require old value of ref to be at this value"),
>>>  - PARSE_OPT_OPTARG, parseopt_push_cas_option },
>>>  + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>>  + parseopt_push_cas_option },
>>>  { OPTION_CALLBACK, 0, "recurse-submodules", 
>>> _submodules, "check|on-demand|no",
>>>  N_("control recursive pushing of submodules"),
>>>  PARSE_OPT_OPTARG, 
>>> option_parse_recurse_submodules },
>>>
>>> I.e. the reason this is confusing is because the code originally added
>>> in 28f5d17611 ("remote.c: add command line option parser for
>>> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
>>> which I also see is what read-tree etc. use already to not end up with
>>> these double <>'s, see also 29f25d493c ("parse-options: add
>>> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).

Yup.  It shows that I did not know (or remember) about LIT-ARGH when
I wrote it (the line stayed in the same shape since its introduction
to the codebase), and I did not know (or remember) when I sent this
patch.  The above is the best solution to my puzzlement within the
framework of the current codebase.

>> We could check if argh comes with its own angle brackets already and
>> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
>> redundant in most cases, including the one above.  Any downsides?
>> Too magical?
>
> I'm more inclined to say that we should stop using
> PARSE_OPT_LITERAL_ARGHELP in some of these cases, and change
> "refname>::" in push.c, so that the help
> we emit is --force-with-lease[=<:>].

I fail to see why the outermost <> pair could be a good idea.
Without them, i.e. in what the current output shows, I can see
 and  are something that I should supply real
values (i.e. placeholders) and I should have a colon (literal) in
between them.  It is an established convention that a token enclosed
in a <> pair is a placeholder.

But I am not sure what you mean by <:>.

> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<> into --refspec=, but to do stuff
> like --option=[,] for options that take comma-delimited
> options.

There is no --refspec=<> to begin with.

A single placeholder can be written in the source as "refspec" and
shown as "--refspec=" because you get the surrounding <>
pair for free by default.  Nobody would want to write "" in
the arg help, as most of the option arguments are a single value
placeholder.  But if you want "[,]" in the final output,
you do *not* want surrounding <> pair, so you use the option and
write everythnig manually in the source without magic.

Or are you saying that we should consistently write surrounding "<>"
to all placeholders, and stop special casing single-token ones?
IOW, get rid of literal-arghelp and instead make that the default?

> If we're magically removing <>'s we have no consistent convention to
> tell apart --opt= meaning "one of a, b or c", --refspec=
> meaning "the literal string 'refspec'" and --refspec=<> meaning
> add a  string, i.e. fill in your refspec here.

Ah, this is where you are off-by-one.  "--refspec=" means
"give your refspec as its value".

Re: [RFC] push: add documentation on push v2

[Duy Nguyen]

On Wed, Jul 25, 2018 at 7:46 PM Brandon Williams  wrote:
> > Could you send a v2 that covers all the push features in pack version
> > 1? I see some are discussed but it's probably good to summarize in
> > this document too.
>
> I can mention the ones we want to implement, but I expect that a push v2
> would not require that all features in the current push are supported
> out of the box.  Some servers may not want to support signed-push, etc.
> Also I don't want to have to implement every single feature that exists
> before getting something merged.  This way follow on series can be
> written to implement those as new features to push v2.

I thought I wrote this mail but apparently I did not for some unknown
reason. My concern here is painting ourselves in the corner and having
a rough sketch of how current features will be implemented or replaced
in v2 would help reduce that risk.
-- 
Duy

Re: [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 17:06 schrieb René Scharfe:
> According to its manpage the option should rather be shown like this:
> 
>   --force-with-lease[=[:]]
> 
> ... to indicate that all three forms are valid:
> 
>   --force-with-lease
>   --force-with-lease=some_ref
>   --force-with-lease=some_ref:but_not_this
> 
> The current code doesn't allow that to be expressed, while it's possible
> with my patch.
Err, anything is possible with PARSE_OPT_LITERAL_ARGHELP, of course, but
with my patch that flag is automatically inferred if you use the argh
string "[:]".

René

Re: [PATCH] push: comment on a funny unbalanced option help

[René Scharfe]

Am 02.08.2018 um 16:21 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Thu, Aug 02 2018, René Scharfe wrote:
> 
>> Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
>>> But looking at this again it looks like this whole thing should just be
>>> replaced by:
>>>
>>>   diff --git a/builtin/push.c b/builtin/push.c
>>>   index 9cd8e8cd56..b8fa15c101 100644
>>>   --- a/builtin/push.c
>>>   +++ b/builtin/push.c
>>>   @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const 
>>> char *prefix)
>>>   OPT_BIT( 0,  "porcelain", , 
>>> N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>>   OPT_BIT('f', "force", , N_("force updates"), 
>>> TRANSPORT_PUSH_FORCE),
>>>   { OPTION_CALLBACK,
>>>   - 0, CAS_OPT_NAME, , N_("refname>:>>   + 0, CAS_OPT_NAME, , N_(":"),
>>> N_("require old value of ref to be at this value"),
>>>   - PARSE_OPT_OPTARG, parseopt_push_cas_option },
>>>   + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>>   + parseopt_push_cas_option },
>>>   { OPTION_CALLBACK, 0, "recurse-submodules", 
>>> _submodules, "check|on-demand|no",
>>>   N_("control recursive pushing of submodules"),
>>>   PARSE_OPT_OPTARG, 
>>> option_parse_recurse_submodules },
>>>
>>> I.e. the reason this is confusing is because the code originally added
>>> in 28f5d17611 ("remote.c: add command line option parser for
>>> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
>>> which I also see is what read-tree etc. use already to not end up with
>>> these double <>'s, see also 29f25d493c ("parse-options: add
>>> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).
>>
>> We could check if argh comes with its own angle brackets already and
>> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
>> redundant in most cases, including the one above.  Any downsides?
>> Too magical?
> 
> I'm more inclined to say that we should stop using
> PARSE_OPT_LITERAL_ARGHELP in some of these cases, and change
> "refname>::" in push.c, so that the help
> we emit is --force-with-lease[=<:>].
> 
> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<> into --refspec=, but to do stuff
> like --option=[,] for options that take comma-delimited
> options.
> 
> If we're magically removing <>'s we have no consistent convention to
> tell apart --opt= meaning "one of a, b or c", --refspec=
> meaning "the literal string 'refspec'" and --refspec=<> meaning
> add a  string, i.e. fill in your refspec here.

The notation for requiring a literal string is to use no special markers:

--option=literal_string

Alternatives can be grouped with parentheses:

--option=(either|or)

In both cases you'd need PARSE_OPT_LITERAL_ARGHELP.

I haven't seen double angle brackets before in command line help strings.
The commit message of 29f25d493c doesn't mention them either.  A single
pair is used to indicate that users need to fill in a value of a certain
type:

--refspec=

Multi-part options aren't special in this syntax:

--force-with-lease=:

NB: The "--refspec=" in the example before that is a literal string, so
this is also already a multi-part option if you will.

According to its manpage the option should rather be shown like this:

--force-with-lease[=[:]]

... to indicate that all three forms are valid:

--force-with-lease
--force-with-lease=some_ref
--force-with-lease=some_ref:but_not_this

The current code doesn't allow that to be expressed, while it's possible
with my patch.  And nothing is removed -- you can specify as many angle
brackets as you like, if that turns out to be useful; parseopt just won't
add any more on top automatically anymore if you do that.

Side note: The remaining user of PARSE_OPT_LITERAL_ARGHELP in
builtin/update-index.c uses a slash for alternatives; we should probably
use pipe instead:

{OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
N_("override the executable bit of the listed files"),
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
chmod_callback},

René

Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems

[Duy Nguyen]

On Wed, Aug 1, 2018 at 11:20 PM Junio C Hamano  wrote:
>
> Junio C Hamano  writes:
>
> > Jeff King  writes:
> >
> >>> Presumably we are already in an error codepath, so if it is
> >>> absolutely necessary, then we can issue a lstat() to grab the inum
> >>> for the path we are about to create, iterate over the previously
> >>> checked out paths issuing lstat() and see which one yields the same
> >>> inum, to find the one who is the culprit.
> >>
> >> Yes, this is the cleverness I was missing in my earlier response.
> >>
> >> So it seems do-able, and I like that this incurs no cost in the
> >> non-error case.
> >
> > Not so fast, unfortunately.
> >
> > I suspect that some filesystems do not give us inum that we can use
> > for that "identity" purpose, and they tend to be the ones with the
> > case smashing characteristics where we need this code in the error
> > path the most X-<.
>
> But even if inum is unreliable, we should be able to use other
> clues, perhaps the same set of fields we use for cached stat
> matching optimization we use for "diff" plumbing commands, to
> implement the error report.  The more important part of the idea is
> that we already need to notice that we need to remove a path that is
> in the working tree while doing the checkout, so the alternative
> approach won't incur any extra cost for normal cases where the
> project being checked out does not have two files whose pathnames
> are only different in case (or checking out such an offending
> project to a case sensitive filesytem, of course).
>
> So I guess it still _is_ workable.  Any takers?

OK so we're going back to the original way of checking that we check
out the different files on the same place (because fs is icase) and
try to collect all paths for reporting, yes? I can give it another go
(but of course if anybody else steps up, I'd very gladly hand this
over)
-- 
Duy

[RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

Add a new 'config' subcommand to 'submodule--helper', this extra level
of indirection makes it possible to add some flexibility to how the
submodules configuration is handled.

Signed-off-by: Antonio Ospite 
---

Note that the tests follow the predominant style in the file: subshell and cd
right at the start of the sub-shell opening.

 builtin/submodule--helper.c | 17 +
 t/t7411-submodule-config.sh | 26 ++
 2 files changed, 43 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..14f0845d30 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const 
char **argv, const char *p
return 0;
 }
 
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+   if (argc < 2 || argc > 3)
+   die("submodule--helper config takes 1 or 2 arguments: name 
[value]");
+
+   /* Equivalent to ACTION_GET in builtin/config.c */
+   if (argc == 2)
+   return print_config_from_gitmodules(argv[1]);
+
+   /* Equivalent to ACTION_SET in builtin/config.c */
+   if (argc == 3)
+   return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+   return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2057,6 +2073,7 @@ static struct cmd_struct commands[] = {
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
{"is-active", is_active, 0},
{"check-name", check_name, 0},
+   {"config", module_config, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 248da0bc4f..bbca4b421d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -141,4 +141,30 @@ test_expect_success 'error in history in 
fetchrecursesubmodule lets continue' '
)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   echo "../submodule" >expected &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   echo "new_url" >expected &&
+   git submodule--helper config submodule.submodule.url "new_url" 
&&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'overwriting unstaged submodules config with 
"submodule--helper config"' '
+   (cd super &&
+   echo "newer_url" >expected &&
+   git submodule--helper config submodule.submodule.url 
"newer_url" &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
 test_done
-- 
2.18.0

[RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

[Antonio Ospite]

git submodule commands can now access .gitmodules from the current
branch even when it's not in the working tree, add some tests for that
scenario.

Signed-off-by: Antonio Ospite 
---

For the test files I used the most used style in other tests, Stefan suggested
to avoid subshells and use "git -C" but subshells make the test look cleaner
IMHO.

 t/t7416-submodule-sparse-gitmodules.sh | 112 +
 1 file changed, 112 insertions(+)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh

diff --git a/t/t7416-submodule-sparse-gitmodules.sh 
b/t/t7416-submodule-sparse-gitmodules.sh
new file mode 100755
index 00..3c7a53316b
--- /dev/null
+++ b/t/t7416-submodule-sparse-gitmodules.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite 
+#
+
+test_description=' Test reading/writing .gitmodules is not in the working tree
+
+This test verifies that, when .gitmodules is in the current branch but is not
+in the working tree reading from it still works but writing to it does not.
+
+The test setup uses a sparse checkout, but the same scenario can be set up
+also by committing .gitmodules and then just removing it from the filesystem.
+
+NOTE: "git mv" and "git rm" are still supposed to work even without
+a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sparse checkout setup which hides .gitmodules' '
+   echo file > file &&
+   git add file &&
+   test_tick &&
+   git commit -m upstream &&
+   git clone . super &&
+   git clone super submodule &&
+   git clone super new_submodule &&
+   (cd super &&
+   git submodule add ../submodule
+   test_tick &&
+   git commit -m submodule &&
+   cat >.git/info/sparse-checkout <<\EOF &&
+/*
+!/.gitmodules
+EOF
+   git config core.sparsecheckout true &&
+   git read-tree -m -u HEAD &&
+   test ! -e .gitmodules
+   )
+'
+
+test_expect_success 'reading gitmodules config file when it is not checked 
out' '
+   (cd super &&
+   echo "../submodule" >expected &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked 
out' '
+   (cd super &&
+   test_must_fail git submodule--helper config 
submodule.submodule.url newurl
+   )
+'
+
+test_expect_success 'not staging gitmodules config when it is not checked out' 
'
+   (cd super &&
+   test_must_fail git submodule--helper config --stage
+   )
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not 
checked out' '
+   (cd super &&
+   git submodule init
+   )
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is 
not checked out' '
+   (cd super &&
+   git submodule summary
+   )
+'
+
+test_expect_success 'updating submodule when the gitmodules config is not 
checked out' '
+   (cd submodule &&
+   echo file2 >file2 &&
+   git add file2 &&
+   git commit -m "add file2 to submodule"
+   ) &&
+   (cd super &&
+   git submodule update
+   )
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not 
checked out' '
+   (cd super &&
+   test_must_fail git submodule add ../new_submodule
+   )
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned tree in there and adds a config entry to .git/config. This is because
+# no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule
+#
+# then the repository would be in a clean state and the test below would pass.
+#
+# Maybe cmd_add should do the cleanup from above itself when failing to add
+# a submodule.
+test_expect_failure 'init submodule after adding failed when the gitmodules 
config is not checked out' '
+   (cd super &&
+   git submodule init
+   )
+'
+
+test_done
-- 
2.18.0

[RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario

[Antonio Ospite]

In t/t7506-status-submodule.sh at some point a new scenario is set up to
test different things, in particular new submodules are added which are
meant to completely replace the previous ones.

However before calling the "git submodule add" commands for the new
layout, the .gitmodules file is removed only from the working tree still
leaving the previous content in current branch.

This can break if, in the future, "git submodule add" starts
differentiating between the following two cases:

  - .gitmodules is not in the working tree but it is in the current
branch (it may not be safe to add new submodules in this case);

  - .gitmodules is neither in the working tree nor anywhere in the
current branch (it is safe to add new submodules).

Since the test means to get rid of .gitmodules anyways, let's completely
remove it from the current branch, to actually start afresh in the new
scenario.

This is more future-proof and does not break current tests.

Signed-off-by: Antonio Ospite 
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index b4b74dbe29..af91ba92ff 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file 
in nested submodule'
(
cd super &&
git clean -dfx &&
-   rm .gitmodules &&
+   git rm .gitmodules &&
+   git commit -m "remove .gitmodules" &&
git submodule add -f ./sub1 &&
git submodule add -f ./sub2 &&
git submodule add -f ./sub1 sub3 &&
-- 
2.18.0

[RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

[Antonio Ospite]

The is_empty_file() function can be generally useful, move it to dir.c
and make it public.

Signed-off-by: Antonio Ospite 
---
 builtin/am.c | 15 ---
 dir.c| 16 
 dir.h|  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6273ea5195..0c04312a50 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -33,21 +33,6 @@
 #include "string-list.h"
 #include "packfile.h"
 
-/**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-   struct stat st;
-
-   if (stat(filename, ) < 0) {
-   if (errno == ENOENT)
-   return 1;
-   die_errno(_("could not stat %s"), filename);
-   }
-
-   return !st.st_size;
-}
 
 /**
  * Returns the length of the first line of msg.
diff --git a/dir.c b/dir.c
index 21e6f2520a..c1f7b90256 100644
--- a/dir.c
+++ b/dir.c
@@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
return ret;
 }
 
+/**
+ * Returns 1 if the file is empty or does not exist, 0 otherwise.
+ */
+int is_empty_file(const char *filename)
+{
+   struct stat st;
+
+   if (stat(filename, ) < 0) {
+   if (errno == ENOENT)
+   return 1;
+   die_errno(_("could not stat %s"), filename);
+   }
+
+   return !st.st_size;
+}
+
 static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 {
DIR *dir;
diff --git a/dir.h b/dir.h
index f5fdedbab2..e45aa3f459 100644
--- a/dir.h
+++ b/dir.h
@@ -281,6 +281,7 @@ static inline int is_dot_or_dotdot(const char *name)
 }
 
 extern int is_empty_dir(const char *dir);
+extern int is_empty_file(const char *filename);
 
 extern void setup_standard_excludes(struct dir_struct *dir);
 
-- 
2.18.0

[RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

Add a --stage option to the 'submodule--helper config' command so that
the .gitmodules file can be staged without referring to it explicitly by
its file path.

In practice the config will still be written to .gitmodules, there are
no plans to change the file path, but having this level of indirection
makes it possible to perform additional checks before staging the file.

Note that "git submodule--helper config --stage" will fail when
.gitmodules does not exist, this is to be consistent with how "git add"
and "git rm" work: if the file to be staged does not exist they will
complain.

Signed-off-by: Antonio Ospite 
---
 builtin/submodule--helper.c | 54 +++--
 t/t7411-submodule-config.sh | 35 
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 14f0845d30..c388f4ee6f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "quote.h"
 #include "pathspec.h"
+#include "lockfile.h"
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
@@ -2031,8 +2032,57 @@ static int connect_gitdir_workingtree(int argc, const 
char **argv, const char *p
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
-   if (argc < 2 || argc > 3)
-   die("submodule--helper config takes 1 or 2 arguments: name 
[value]");
+   int stage_gitmodules = 0;
+
+   struct option module_config_options[] = {
+   OPT_BOOL(0, "stage", _gitmodules,
+   N_("stage the .gitmodules file content")),
+   OPT_END()
+   };
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper config name [value]"),
+   N_("git submodule--helper config --stage"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+   if ((stage_gitmodules && argc > 1) ||
+   (!stage_gitmodules && (argc < 2 || argc > 3)))
+   usage_with_options(git_submodule_helper_usage, 
module_config_options);
+
+   /*
+* Equivalent to "git add --force .gitmodules".
+*
+* Silently override already staged changes, to support consecutive
+* invocations of "git submodule add".
+*/
+   if (stage_gitmodules) {
+   static struct lock_file lock_file;
+
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
+
+   if (read_cache() < 0)
+   die(_("index file corrupt"));
+
+   /*
+* Do not use is_staging_gitmodules_ok() here to make it
+* possible to stage multiple changes before committing them,
+* in particular this covers the case of consecutive calls of
+* "git submodule add".
+*/
+   if (!file_exists(GITMODULES_FILE))
+   die(_("%s does not exists"), GITMODULES_FILE);
+
+   stage_updated_gitmodules(_index);
+
+   if (write_locked_index(_index, _file,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("Unable to write new index file"));
+
+   return 0;
+   }
 
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index bbca4b421d..8ff6ed444a 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -167,4 +167,39 @@ test_expect_success 'overwriting unstaged submodules 
config with "submodule--hel
)
 '
 
+test_expect_success 'staging submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   : >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual &&
+   git submodule--helper config --stage &&
+   echo ".gitmodules" >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'overwriting staged submodules config with 
"submodule--helper config"' '
+   (cd super &&
+   echo "even_newer_url" >expected &&
+   git submodule--helper config submodule.submodule.url 
"even_newer_url" &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 're-staging overridden submodule config with 
"submodule--helper config"' '
+   (cd super &&
+   echo ".gitmodules" >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual &&
+   echo "bogus config" >.gitmodules &&
+ 

[RFC PATCH v2 07/12] submodule: use 'submodule--helper config --stage' to stage .gitmodules

[Antonio Ospite]

Use 'git submodule--helper config --stage' in git-submodule.sh to remove
the last place where the .gitmodules file is mentioned explicitly by its
file path.

Signed-off-by: Antonio Ospite 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..f20c37d92d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -289,7 +289,7 @@ or you are unsure what this means choose another name with 
the '--name' option."
then
git submodule--helper config submodule."$sm_name".branch 
"$branch"
fi &&
-   git add --force .gitmodules ||
+   git submodule--helper config --stage ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
# NEEDSWORK: In a multi-working-tree world, this needs to be
-- 
2.18.0

[RFC PATCH v2 02/12] submodule: factor out a config_set_in_gitmodules_file_gently function

[Antonio Ospite]

Introduce a new config_set_in_gitmodules_file_gently() function to write
config values to the .gitmodules file.

This is in preparation for a future change which will use the function
to write to the .gitmodules file in a more controlled way instead of
using "git config -f .gitmodules".

The purpose of the change is mainly to centralize the code that writes
to the .gitmodules file to avoid some duplication.

The naming follows git_config_set_in_file_gently() but the git_ prefix
is removed to communicate that this is not a generic git-config API.

Signed-off-by: Antonio Ospite 
---
 submodule-config.c | 12 
 submodule-config.h |  1 +
 submodule.c| 10 +++---
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6f6f5f9960..702d40dd6b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -707,6 +707,18 @@ int print_config_from_gitmodules(const char *key)
return 0;
 }
 
+int config_set_in_gitmodules_file_gently(const char *key, const char *value)
+{
+   int ret;
+
+   ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+   if (ret < 0)
+   /* Maybe the user already did that, don't error out here */
+   warning(_("Could not update .gitmodules entry %s"), key);
+
+   return ret;
+}
+
 struct fetch_config {
int *max_children;
int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index 6fec3caadd..074e74a01c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,6 +57,7 @@ void submodule_free(struct repository *r);
 int check_submodule_name(const char *name);
 
 extern int print_config_from_gitmodules(const char *key);
+extern int config_set_in_gitmodules_file_gently(const char *key, const char 
*value);
 
 /*
  * Note: these helper functions exist solely to maintain backward
diff --git a/submodule.c b/submodule.c
index 2a6381864e..2af09068d7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -89,6 +89,7 @@ int update_path_in_gitmodules(const char *oldpath, const char 
*newpath)
 {
struct strbuf entry = STRBUF_INIT;
const struct submodule *submodule;
+   int ret;
 
if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
return -1;
@@ -104,14 +105,9 @@ int update_path_in_gitmodules(const char *oldpath, const 
char *newpath)
strbuf_addstr(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".path");
-   if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) 
< 0) {
-   /* Maybe the user already did that, don't error out here */
-   warning(_("Could not update .gitmodules entry %s"), entry.buf);
-   strbuf_release();
-   return -1;
-   }
+   ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
-   return 0;
+   return ret;
 }
 
 /*
-- 
2.18.0

[RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

[Antonio Ospite]

This will be used to print values just like "git config -f .gitmodules"
would.

Signed-off-by: Antonio Ospite 
---
 submodule-config.c | 25 +
 submodule-config.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index 2a7259ba8b..6f6f5f9960 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
 }
 
+static int config_print_callback(const char *key_, const char *value_, void 
*cb_data)
+{
+   char *key = cb_data;
+
+   if (!strcmp(key, key_))
+   printf("%s\n", value_);
+
+   return 0;
+}
+
+int print_config_from_gitmodules(const char *key)
+{
+   int ret;
+   char *store_key;
+
+   ret = git_config_parse_key(key, _key, NULL);
+   if (ret < 0)
+   return CONFIG_INVALID_KEY;
+
+   config_from_gitmodules(config_print_callback, the_repository, 
store_key);
+
+   free(store_key);
+   return 0;
+}
+
 struct fetch_config {
int *max_children;
int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index dc7278eea4..6fec3caadd 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
  */
 int check_submodule_name(const char *name);
 
+extern int print_config_from_gitmodules(const char *key);
+
 /*
  * Note: these helper functions exist solely to maintain backward
  * compatibility with 'fetch' and 'update_clone' storing configuration in
-- 
2.18.0

[RFC PATCH v2 00/12] Make submodules work if .gitmodules is not checked out

[Antonio Ospite]

Hi,

this is a second version of the RFC from
https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/

Please refer to the cover letter of the previous series for the
motivation and background of the changes.

Patches from 01 to 08 should be rather uncontroversial, as they do not
introduce any change of behavior but just prepare the ground for patches
09 and 10.

The last two patches, 11 and  12 are not strictly related to the topic,
it's just an idea I came up with while reading the code for the series;
I could send them separately if you think they are valid, or drop them.

The behavior of accessing HEAD:.gitmodules have been analyzed with the
following test matrix:

Predicates:
  G = .gitmodules exists
  H = HEAD:.gitmodules exists

Operations:
  read = git submodule--helper config name
  write = git submodule--helper config name value
  stage = git submodule--helper config --stage

!G and !H:
  - read: nop
  - write: set value to .gitmodules (we are creating a brand new
.gitmodules)
  - stage: exit with error, staging requires .gitmodules (like git
add/rm)

G and !H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules

!G and H:
  - read: get the value from HEAD:.gitmodules
  - write: exit with error, setting a value requires .gitmodules
  - stage: exit with error, staging requires .gitmodules
  
G and H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules


Note that "git rm" and "git mv" will keep working when !G just as
t3600-rm.sh and t7001-mv.sh mandate, I am not sure if they should fail
if "!G and H".

In my previous attempt I also tried to check for the skip-worktree bit,
but I am not sure if this is needed.

My intuition suggested the following behavior to cover the case of
a manually crafted .gitmodules which could be committed and override the
hidden one:

S = .gitmodules has the skip-worktree bit set

S:
  - read: get value from .gitmodules if G or from HEAD:.gitmodules if H
  - write: exit with error, .gitmodules is not supposed to be changed
  - stage: exit with error, .gitmodules is not supposed to be changed

However it looks like git gives the user quite some freedom to add,
stage, commit, remove files even when they have the skip-worktree bit
set (which was a little surprising to me TBH) so I am leaving this out
for now.


Changes since v1:

  * Added a helper to print config values from the gitmodules
configuration, the code is now in submoudle-config.c instead of
buildtin/submodule--helper.c

  * Renamed config_gitmodules_set to
config_set_in_gitmodules_file_gently and put it in
submodule-config.c instead of submodule.c

The naming follows the style of git config functions, and also takes
into account Stefan Beller's comment about the final purpose of the
function: we only write gitmodules config to the actual .gitmodules
file, so we may as well reflect that in the function name,

  * Started using "working tree" instead of "work tree" as the former is
the official term for the set of checked out files.

  * Start referring to HEAD as the "current branch" instead of "the
index", the former should be more accurate.

  * Renamed GITMODULES_BLOB to GITMODULES_HEAD because GITMODULES_BLOB
was added in commit 59e7b080 ("fsck: detect gitmodules files",
2018-05-02)

  * Do not check for the skip-worktree bit on .gitmodules anymore, just
check for file existence when appropriate.

  * Renamed t7415-submodule-sparse-gitmodules.sh to
t7416-submodule-sparse-gitmodules.sh because t7415 was taken by
t7415-submodule-names.sh

  * Made "git mv" and "git rm" work again when .gitmodules does not
exist.

  * Removed checks about the  skip-worktree bit.


Thanks,
   Antonio

Antonio Ospite (12):
  submodule: add a print_config_from_gitmodules() helper
  submodule: factor out a config_set_in_gitmodules_file_gently function
  t7411: be nicer to future tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  submodule--helper: add a '--stage' option to the 'config' sub command
  submodule: use 'submodule--helper config --stage' to stage .gitmodules
  t7506: cleanup .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out
  t7416: add new test about HEAD:.gitmodules and not existing
.gitmodules
  dir: move is_empty_file() from builtin/am.c to dir.c and make it
public
  submodule: remove the .gitmodules file when it is empty

 builtin/am.c   |  15 
 builtin/submodule--helper.c|  82 ++
 cache.h|   1 +
 dir.c  |  16 
 dir.h  |   1 +
 git-submodule.sh   |  10 +--
 submodule-config.c