date:20181022

Matthew DeVore  writes:

>  t/t4202-log.sh | 4 
>  t/t8002-blame.sh   | 4 
>  7 files changed, 14 insertions(+), 1 deletion(-)
> ...
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 153a506151..819c24d10e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric 
> ranges' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success '--exclude-promisor-objects does not BUG-crash' '
> + test_must_fail git log --exclude-promisor-objects source-a
> +'
> +
>  test_done
> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index 380e1c1054..eea048e52c 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '
>   check_abbrev 40 --no-abbrev
>  '
>  
> +test_expect_success '--exclude-promisor-objects does not BUG-crash' '
> + test_must_fail git blame --exclude-promisor-objects one
> +'
> +
>  test_done

OK.  We used to be hitting BUG() which is an abort() in disguise, so
must-fail would have caught it without the fix in this patch.  Now
we would see a more controlled failure.

... goes and makes sure that is the case ...

Not really.  We were already doing a controlled failure via die(),
so these two tests would not have caught the problem in the code
before the fix in this patch.

But nevertheless this is a good change; I do not think it is worth
grepping for "unrecognized option" to differentiate the two cases.

Thanks.

Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects

Matthew DeVore  writes:

> This patch set fixes incorrect parsing of the --exclude-promisor-objects
> option that I found while working on:
>
>   https://public-inbox.org/git/cover.1539298957.git.matv...@google.com/
>

Thanks; both patches make sense.  

As the problematic feature appeared in 2.17.x track, I'll see if I
can easily make it ready to be merged down to maint-2.17 track later
when somebody wants to.

> Matthew DeVore (2):
>   Documentation/git-log.txt: do not show --exclude-promisor-objects
>   exclude-promisor-objects: declare when option is allowed
>
>  Documentation/rev-list-options.txt | 2 +-
>  builtin/pack-objects.c | 1 +
>  builtin/prune.c| 1 +
>  builtin/rev-list.c | 1 +
>  revision.c | 3 ++-
>  revision.h | 1 +
>  t/t4202-log.sh | 4 
>  t/t8002-blame.sh   | 4 
>  8 files changed, 15 insertions(+), 2 deletions(-)

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

Stefan Beller  writes:

> Am I overestimating or misunderstanding rerere here?

Yes.

> Would it be realistic for next and master branch instead of pu?
>
> I'd be wary for the master branch, as we may not want to rely on
> spatch without review. (It can produce funny white space issues,
> but seems to produce working/correct code)

Yes, that is why I think it is a bit too late to do the "fixup with
spatch" after merging to 'next' and 'master'.

Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

SZEDER Gábor  writes:

>> To prevent that from happening, let's append `^0` after the stash hash,
>> to make sure that it is interpreted as an OID rather than as a number.
>
> Oh, this is clever.

Yeah, we can do this as we know we'd be dealing with a commit-ish.
If we made a mistake to use a tree object to represent a stash, this
trick wouldn't have been possible ;-)

> FWIW, all patches look good to me, barring the typo below.
> ...
>> +/* Ensure that the hash is not mistake for a number */
>
> s/mistake/mistaken/

Will squash this in and add your reviewed-by before queuing.

Thanks, both.

Re: [PATCH v3] archive: initialize archivers earlier

stead...@google.com writes:

> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0a..cfd5ca492f 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -39,6 +39,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '
>  
>  test_lazy_prereq GZIP 'gzip --version'
>  
> +test_lazy_prereq ZIP 'zip --version'
> +

There are a handful of zip implementations; Info-ZIP found on many
Linux distros does support 'zip --version', but we may want to make
sure this test covers different implementations of zip sufficiently.

Queuing this patch (or an update of it) on 'pu' and hoping those
with zip from different origins to try it would not help very much,
either, as zip implementations that do not react to "zip --version"
would silently turn the prereq off without breaking anything.

In any case, please refrain from adding any ZIP prerequiste to t5000
which is about tar; t5003-archive-zip may be a much better fit.  It
has an already working machinery that validates the generated zip
archive under UNZIP prerequisite, so we may not even have to invent
our own ZIP prereq if we did so.

> @@ -206,6 +208,19 @@ test_expect_success 'git archive with --output, override 
> inferred format' '
>   test_cmp_bin b.tar d4.zip
>  '
>  
> +test_expect_success GZIP 'git archive with --output and --remote creates 
> .tgz' '
> + git archive --output=d5.tgz --remote=. HEAD &&
> + gzip -d -c < d5.tgz > d5.tar &&
> + test_cmp_bin b.tar d5.tar
> +'

We try to write redirections without SP between redirection operator
and target filename, i.e. "gzip -d -c d5.tar".

Thanks.

Re: [PATCH 5/5] t7501: rename commit test to comply with naming convention

On Mon, Oct 22, 2018 at 11:54 PM Stephen P. Smith  wrote:
> The naming convention was documented [1] but this script was not
> renamed.
>
> The original commit message indicates the script tests basic commit
> functionality. Clean up the test name by changing the file name to
> specify the intent as documented in the initial commit.
>
> Signed-off-by: Stephen P. Smith 
> ---
> diff --git a/t/t7501-commit.sh b/t/t7501-commit-basic-funtionality.sh
> rename from t/t7501-commit.sh
> rename to t/t7501-commit-basic-funtionality.sh

s/funtionality/functionality/

Re: [PATCH 4/5] t7500: rename commit tests script to comply with naming convention

On Mon, Oct 22, 2018 at 11:53 PM Stephen P. Smith  wrote:
> When the test naming convention was documented[1] the commit script
> was not renamed.
>
> Update the test description to note that the tests fall into for
> general categories: template, sign-off, -F and squash tests.

s/for/four/

> Chose to not add "File" to the new script name as that did not seem to
> convey the current test contents for that switch.
>
> Signed-off-by: Stephen P. Smith

Re: [PATCH 2/5] t7509: cleanup description and filename

On Mon, Oct 22, 2018 at 11:53 PM Stephen P. Smith  wrote:>
> Rename test and update the test description to explicitly state that
> included tests all relate to commit authorship. The t7509-commit.sh
> file was not rnemamed when other scripts were updated in compliance

s/rnemamed/renamed/

> with the test naming convention.
>
> Signed-off-by: Stephen P. Smith

[PATCH 4/5] t7500: rename commit tests script to comply with naming convention

When the test naming convention was documented[1] the commit script
was not renamed.

Update the test description to note that the tests fall into for
general categories: template, sign-off, -F and squash tests.

Chose to not add "File" to the new script name as that did not seem to
convey the current test contents for that switch.

[1] f50c9f76c ("Rename some test scripts and describe the naming convention", 
2005-05-15)

Signed-off-by: Stephen P. Smith 
---
 t/{t7500-commit.sh => t7500-commit-template-squash-signoff.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t7500-commit.sh => t7500-commit-template-squash-signoff.sh} (99%)

diff --git a/t/t7500-commit.sh b/t/t7500-commit-template-squash-signoff.sh
similarity index 99%
rename from t/t7500-commit.sh
rename to t/t7500-commit-template-squash-signoff.sh
index 31ab608b67..46a5cd4b73 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -5,7 +5,7 @@
 
 test_description='git commit
 
-Tests for selected commit options.'
+Tests for template, signoff, squash and -F functions.'
 
 . ./test-lib.sh
 
-- 
2.19.0

[PATCH 0/5] Commit test name clean-up

Several tests did not comply with the documented test naming standard.

The patch series was held off until updates to t7501 was merged to the
master branch.

Stephen P. Smith (5):
  t2000: rename and combine checkout clash tests
  t7509: cleanup description and filename
  t7502: rename commit test script to comply with naming convention
  t7500: rename commit tests script to comply with naming convention
  t7501: rename commit test to comply with naming convention

 t/t2000-checkout-cache-clash.sh   |  60 
 t/t2000-conflict-when-checking-files-out.sh   | 135 ++
 t/t2001-checkout-cache-clash.sh   |  85 ---
 ...> t7500-commit-template-squash-signoff.sh} |   2 +-
 sh => t7501-commit-basic-funtionality.sh} |   0
 ...02-commit.sh => t7502-commit-porcelain.sh} |   0
 ...9-commit.sh => t7509-commit-authorship.sh} |   2 +-
 7 files changed, 137 insertions(+), 147 deletions(-)
 delete mode 100755 t/t2000-checkout-cache-clash.sh
 create mode 100755 t/t2000-conflict-when-checking-files-out.sh
 delete mode 100755 t/t2001-checkout-cache-clash.sh
 rename t/{t7500-commit.sh => t7500-commit-template-squash-signoff.sh} (99%)
 rename t/{t7501-commit.sh => t7501-commit-basic-funtionality.sh} (100%)
 rename t/{t7502-commit.sh => t7502-commit-porcelain.sh} (100%)
 rename t/{t7509-commit.sh => t7509-commit-authorship.sh} (98%)

-- 
2.19.0

[PATCH 1/5] t2000: rename and combine checkout clash tests

In an earlier patch some tests scripts were renamed and a naming
convention was documented. [1]

Merge t2000-checkout-cache-clash.sh and t2001-checkout-cache-clash.sh into
t2000-conflict-when-checking-files-out.sh.

[1] f50c9f76c ("Rename some test scripts and describe the naming convention", 
2005-05-15)

Signed-off-by: Stephen P. Smith 
---
 t/t2000-checkout-cache-clash.sh |  60 -
 t/t2000-conflict-when-checking-files-out.sh | 135 
 t/t2001-checkout-cache-clash.sh |  85 
 3 files changed, 135 insertions(+), 145 deletions(-)
 delete mode 100755 t/t2000-checkout-cache-clash.sh
 create mode 100755 t/t2000-conflict-when-checking-files-out.sh
 delete mode 100755 t/t2001-checkout-cache-clash.sh

diff --git a/t/t2000-checkout-cache-clash.sh b/t/t2000-checkout-cache-clash.sh
deleted file mode 100755
index de3edb5d57..00
--- a/t/t2000-checkout-cache-clash.sh
+++ /dev/null
@@ -1,60 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Junio C Hamano
-#
-
-test_description='git checkout-index test.
-
-This test registers the following filesystem structure in the
-cache:
-
-path0   - a file
-path1/file1 - a file in a directory
-
-And then tries to checkout in a work tree that has the following:
-
-path0/file0 - a file in a directory
-path1   - a file
-
-The git checkout-index command should fail when attempting to checkout
-path0, finding it is occupied by a directory, and path1/file1, finding
-path1 is occupied by a non-directory.  With "-f" flag, it should remove
-the conflicting paths and succeed.
-'
-. ./test-lib.sh
-
-date >path0
-mkdir path1
-date >path1/file1
-
-test_expect_success \
-'git update-index --add various paths.' \
-'git update-index --add path0 path1/file1'
-
-rm -fr path0 path1
-mkdir path0
-date >path0/file0
-date >path1
-
-test_expect_success \
-'git checkout-index without -f should fail on conflicting work tree.' \
-'test_must_fail git checkout-index -a'
-
-test_expect_success \
-'git checkout-index with -f should succeed.' \
-'git checkout-index -f -a'
-
-test_expect_success \
-'git checkout-index conflicting paths.' \
-'test -f path0 && test -d path1 && test -f path1/file1'
-
-test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
-   mkdir -p tar/get &&
-   ln -s tar/get there &&
-   echo first &&
-   git checkout-index -a -f --prefix=there/ &&
-   echo second &&
-   git checkout-index -a -f --prefix=there/
-'
-
-test_done
diff --git a/t/t2000-conflict-when-checking-files-out.sh 
b/t/t2000-conflict-when-checking-files-out.sh
new file mode 100755
index 00..f18616ad2b
--- /dev/null
+++ b/t/t2000-conflict-when-checking-files-out.sh
@@ -0,0 +1,135 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='git conflicts when checking files out test.'
+
+# The first test registers the following filesystem structure in the
+# cache:
+#
+# path0   - a file
+# path1/file1 - a file in a directory
+#
+# And then tries to checkout in a work tree that has the following:
+#
+# path0/file0 - a file in a directory
+# path1   - a file
+#
+# The git checkout-index command should fail when attempting to checkout
+# path0, finding it is occupied by a directory, and path1/file1, finding
+# path1 is occupied by a non-directory.  With "-f" flag, it should remove
+# the conflicting paths and succeed.
+
+. ./test-lib.sh
+
+show_files() {
+   # show filesystem files, just [-dl] for type and name
+   find path? -ls |
+   sed -e 's/^[0-9]* * [0-9]* * \([-bcdl]\)[^ ]* *[0-9]* *[^ ]* *[^ ]* 
*[0-9]* [A-Z][a-z][a-z] [0-9][0-9] [^ ]* /fs: \1 /'
+   # what's in the cache, just mode and name
+   git ls-files --stage |
+   sed -e 's/^\([0-9]*\) [0-9a-f]* [0-3] /ca: \1 /'
+   # what's in the tree, just mode and name.
+   git ls-tree -r "$1" |
+   sed -e 's/^\([0-9]*\)   [^ ]*   [0-9a-f]*   /tr: \1 /'
+}
+
+date >path0
+mkdir path1
+date >path1/file1
+
+test_expect_success \
+'git update-index --add various paths.' \
+'git update-index --add path0 path1/file1'
+
+rm -fr path0 path1
+mkdir path0
+date >path0/file0
+date >path1
+
+test_expect_success \
+'git checkout-index without -f should fail on conflicting work tree.' \
+'test_must_fail git checkout-index -a'
+
+test_expect_success \
+'git checkout-index with -f should succeed.' \
+'git checkout-index -f -a'
+
+test_expect_success \
+'git checkout-index conflicting paths.' \
+'test -f path0 && test -d path1 && test -f path1/file1'
+
+test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
+   mkdir -p tar/get &&
+   ln -s tar/get there &&
+   echo first &&
+   git checkout-index -a -f --prefix=there/ &&
+   echo second &&
+   git checkout-index -a -f --prefix=there/
+'
+
+# The second test registers the following filesystem structure in the cache:
+#
+#

[PATCH 2/5] t7509: cleanup description and filename

Rename test and update the test description to explicitly state that
included tests all relate to commit authorship. The t7509-commit.sh
file was not rnemamed when other scripts were updated in compliance
with the test naming convention.

[1] f50c9f76c ("Rename some test scripts and describe the naming convention", 
2005-05-15)

Signed-off-by: Stephen P. Smith 
---
 t/{t7509-commit.sh => t7509-commit-authorship.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t7509-commit.sh => t7509-commit-authorship.sh} (98%)

diff --git a/t/t7509-commit.sh b/t/t7509-commit-authorship.sh
similarity index 98%
rename from t/t7509-commit.sh
rename to t/t7509-commit-authorship.sh
index ddef7ea6b0..500ab2fe72 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit-authorship.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2009 Erick Mattos
 #
 
-test_description='git commit --reset-author'
+test_description='commit tests of various authorhip options. '
 
 . ./test-lib.sh
 
-- 
2.19.0

[PATCH 3/5] t7502: rename commit test script to comply with naming convention

When the test naming convention was documented[1] the commit script
was not renamed.

The test description for t7502 indicates that the test file is to
contain porcelain type options for the commit command.

The tests don't fall into a single category.  There are tests for
cleanup, sign-off, multiple message options, etc.

Rename the t7502-commit.sh to t7502-commit-porcelain.sh which reflects
the high level nature and usage of the options to commit.

[1] f50c9f76c ("Rename some test scripts and describe the naming convention", 
2005-05-15)

Signed-off-by: Stephen P. Smith 
---
 t/{t7502-commit.sh => t7502-commit-porcelain.sh} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename t/{t7502-commit.sh => t7502-commit-porcelain.sh} (100%)

diff --git a/t/t7502-commit.sh b/t/t7502-commit-porcelain.sh
similarity index 100%
rename from t/t7502-commit.sh
rename to t/t7502-commit-porcelain.sh
-- 
2.19.0

[PATCH 5/5] t7501: rename commit test to comply with naming convention

The naming convention was documented [1] but this script was not
renamed.

The original commit message indicates the script tests basic commit
functionality. Clean up the test name by changing the file name to
specify the intent as documented in the initial commit.

[1] f50c9f76c ("Rename some test scripts and describe the naming convention", 
2005-05-15)

Signed-off-by: Stephen P. Smith 
---
 t/{t7501-commit.sh => t7501-commit-basic-funtionality.sh} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename t/{t7501-commit.sh => t7501-commit-basic-funtionality.sh} (100%)

diff --git a/t/t7501-commit.sh b/t/t7501-commit-basic-funtionality.sh
similarity index 100%
rename from t/t7501-commit.sh
rename to t/t7501-commit-basic-funtionality.sh
-- 
2.19.0

[PATCH v3] send-email: explicitly disable authentication

It can be necessary to disable SMTP authentication by a mechanism other
than sendemail.smtpuser being undefined. For example, if the user has
sendemail.smtpuser set globally but wants to disable authentication
locally in one repository.

--smtp-auth and sendemail.smtpauth now understand the value 'none' which
means to disable authentication completely, even if an authentication
user is specified.

The value 'none' is lower case to avoid conflicts with any RFC 4422
authentication mechanisms.

The user may also specify the command line argument --no-smtp-auth as a
shorthand for --smtp-auth=none

Signed-off-by: Joshua Watt 
---
 Documentation/git-send-email.txt | 7 ++-
 git-send-email.perl  | 8 ++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 465a4ecbe..17993e3c9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -190,7 +190,9 @@ $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ...
 If at least one of the specified mechanisms matches the ones advertised by the
 SMTP server and if it is supported by the utilized SASL library, the mechanism
 is used for authentication. If neither 'sendemail.smtpAuth' nor `--smtp-auth`
-is specified, all mechanisms supported by the SASL library can be used.
+is specified, all mechanisms supported by the SASL library can be used. The
+special value 'none' maybe specified to completely disable authentication
+independently of `--smtp-user`
 
 --smtp-pass[=]::
Password for SMTP-AUTH. The argument is optional: If no
@@ -204,6 +206,9 @@ or on the command line. If a username has been specified 
(with
 specified (with `--smtp-pass` or `sendemail.smtpPass`), then
 a password is obtained using 'git-credential'.
 
+--no-smtp-auth::
+   Disable SMTP authentication. Short hand for `--smtp-auth=none`
+
 --smtp-server=::
If set, specifies the outgoing SMTP server to use (e.g.
`smtp.example.com` or a raw IP address).  Alternatively it can
diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac33..a70679484 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -82,8 +82,11 @@ sub usage {
  Pass an empty string to disable 
certificate
  verification.
 --smtp-domain * The domain name sent to HELO/EHLO 
handshake
---smtp-auth   * Space-separated list of allowed AUTH 
mechanisms.
+--smtp-auth   * Space-separated list of allowed AUTH 
mechanisms, or
+ "none" to disable authentication.
  This setting forces to use one of the 
listed mechanisms.
+--no-smtp-auth   Disable SMTP authentication. Shorthand for
+ `--smtp-auth=none`
 --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
 
 --batch-size  * send max  message per connection.
@@ -341,6 +344,7 @@ sub signal_handler {
"smtp-debug:i" => \$debug_net_smtp,
"smtp-domain:s" => \$smtp_domain,
"smtp-auth=s" => \$smtp_auth,
+   "no-smtp-auth" => sub {$smtp_auth = 'none'},
"identity=s" => \$identity,
"annotate!" => \$annotate,
"no-annotate" => sub {$annotate = 0},
@@ -1241,7 +1245,7 @@ sub smtp_host_string {
 # (smtp_user was not specified), and 0 otherwise.
 
 sub smtp_auth_maybe {
-   if (!defined $smtp_authuser || $auth) {
+   if (!defined $smtp_authuser || $auth || (defined $smtp_auth && 
$smtp_auth eq "none")) {
return 1;
}
 
-- 
2.19.1.543.g838de3e23.dirty

Re: [PATCH v2] send-email: explicitly disable authentication

On Mon, Oct 22, 2018 at 7:32 PM Junio C Hamano  wrote:
>
> Joshua Watt  writes:
>
> > It can be necessary to disable SMTP authentication by a mechanism other
> > than sendemail.smtpuser being undefined. For example, if the user has
> > sendemail.smtpuser set globally but wants to disable authentication
> > locally in one repository.
>
> I wonder if it would be more productive to introduce a mechanism
> that can be used to address that use case more directly.  For
> example, would it help to teach "git send-email" that
> sendemail.smtpuser set to a particular value (say '!', or empty
> string if you prefer) is equivalent to the variable unset at all?
>

I'm a little worried that is more likely to break someone's workflow
(although, I'm not sure why someone would have such simple username).
Using sendemail.smtpauth = "none" is pretty much guaranteed to not
break an existing setup because git send-email would previously reject
any value that wasn't upper case. I suppose the one disadvantage is
that it isn't backward compatible, since setting sendemail.smtpauth to
"none" wouldn't work with older versions of git (due to it not being
upper case), but I'm not sure how much of a concern that is.

IMHO, setting ""  or "!" for sendemail.smtpuser probably isn't any
more clear or direct for the end user than my solution.

[PATCH] Documentation/config.txt: fix typo in core.alternateRefsCommand

2018-10-22 Thread Taylor Blau

In [1] Git learned about 'core.alternateRefsCommand', and with it, the
accompanying documentation. However, this documentation included a typo
involving the verb tense of "produced".

Match the tense of the surrounding bits by correcting this typo.

[1]: 89284c1d6c (transport.c: introduce core.alternateRefsCommand,
 2018-10-08)

Signed-off-by: Taylor Blau 
---

Note: this can be applied as a fixup to the commit [1] mentioned above.
I'm sending it as a separate series, since the patches have already
landed on 'next'.

They were originally introduced in:

  7451b4872a0fd66d84cbe492fdfe7a9a8e81eab7.1539021825.git...@ttaylorr.com

I think that these will ultimately conflict with Duy's patches to move
Documentation/config.txt out to separate files. It seems, however, that
he may re-roll the series beforehand, so I can either:

  1. Resend this once he has landed his changes, or,

  2. He can re-roll with this version of Documentation/config.txt.

 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 552827935a..09e95e9e98 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -620,7 +620,7 @@ core.alternateRefsCommand::
When advertising tips of available history from an alternate, use the 
shell to
execute the specified command instead of linkgit:git-for-each-ref[1]. 
The
first argument is the absolute path of the alternate. Output must 
contain one
-   hex object id per line (i.e., the same as produce by `git for-each-ref
+   hex object id per line (i.e., the same as produced by `git for-each-ref
--format='%(objectname)'`).
 +
 Note that you cannot generally put `git for-each-ref` directly into the config
--
2.19.0.221.g150f307af

Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects





On Mon, 22 Oct 2018, Matthew DeVore wrote:


This patch set fixes incorrect parsing of the --exclude-promisor-objects
option that I found while working on:



Somehow I sent two copies of every message in the patchset. I'm sorry
for the mess.

[RFC 0/2] explicitly support or not support --exclude-promisor-objects

This patch set fixes incorrect parsing of the --exclude-promisor-objects
option that I found while working on:

  https://public-inbox.org/git/cover.1539298957.git.matv...@google.com/

Thank you,

Matthew DeVore (2):
  Documentation/git-log.txt: do not show --exclude-promisor-objects
  exclude-promisor-objects: declare when option is allowed

 Documentation/rev-list-options.txt | 2 +-
 builtin/pack-objects.c | 1 +
 builtin/prune.c| 1 +
 builtin/rev-list.c | 1 +
 revision.c | 3 ++-
 revision.h | 1 +
 t/t4202-log.sh | 4 
 t/t8002-blame.sh   | 4 
 8 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.19.1.568.g152ad8e336-goog

[RFC 2/2] exclude-promisor-objects: declare when option is allowed

The --exclude-promisor-objects option causes some funny behavior in at
least two commands: log and blame. It causes a BUG crash:

$ git log --exclude-promisor-objects
BUG: revision.c:2143: exclude_promisor_objects can only be used
when fetch_if_missing is 0
Aborted
[134]

Fix this such that the option is treated like any other unknown option.
The commands that must support it are limited, so declare in those
commands that the flag is supported. In particular:

pack-objects
prune
rev-list

The commands were found by searching for logic which parses
--exclude-promisor-objects outside of revision.c. Extra logic outside of
revision.c is needed because fetch_if_missing must be turned on before
revision.c sees the option or it will BUG-crash. The above list is
supported by the fact that no other command is introspectively invoked
by another command passing --exclude-promisor-object.

Signed-off-by: Matthew DeVore 
---
 builtin/pack-objects.c | 1 +
 builtin/prune.c| 1 +
 builtin/rev-list.c | 1 +
 revision.c | 3 ++-
 revision.h | 1 +
 t/t4202-log.sh | 4 
 t/t8002-blame.sh   | 4 
 7 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..c409fa25d6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3108,6 +3108,7 @@ static void get_object_list(int ac, const char **av)
 
repo_init_revisions(the_repository, , NULL);
save_commit_buffer = 0;
+   revs.allow_exclude_promisor_objects_opt = 1;
setup_revisions(ac, av, , NULL);
 
/* make sure shallows are read */
diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f8215..11284d0bf3 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
save_commit_buffer = 0;
read_replace_refs = 0;
ref_paranoia = 1;
+   revs.allow_exclude_promisor_objects_opt = 1;
repo_init_revisions(the_repository, , prefix);
 
argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5064d08e1b..2880ed37e3 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -374,6 +374,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
git_config(git_default_config, NULL);
repo_init_revisions(the_repository, , prefix);
revs.abbrev = DEFAULT_ABBREV;
+   revs.allow_exclude_promisor_objects_opt = 1;
revs.commit_format = CMIT_FMT_UNSPECIFIED;
revs.do_not_die_on_missing_tree = 1;
 
diff --git a/revision.c b/revision.c
index a1ddb9e11c..28fb2a70cd 100644
--- a/revision.c
+++ b/revision.c
@@ -2138,7 +2138,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
-   } else if (!strcmp(arg, "--exclude-promisor-objects")) {
+   } else if (revs->allow_exclude_promisor_objects_opt &&
+  !strcmp(arg, "--exclude-promisor-objects")) {
if (fetch_if_missing)
BUG("exclude_promisor_objects can only be used when 
fetch_if_missing is 0");
revs->exclude_promisor_objects = 1;
diff --git a/revision.h b/revision.h
index 1cd0c4b200..0d2abc2d36 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,7 @@ struct rev_info {
do_not_die_on_missing_tree:1,
 
/* for internal use only */
+   allow_exclude_promisor_objects_opt:1,
exclude_promisor_objects:1;
 
/* Diff flags */
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 153a506151..819c24d10e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric 
ranges' '
test_cmp expect actual
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+   test_must_fail git log --exclude-promisor-objects source-a
+'
+
 test_done
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index 380e1c1054..eea048e52c 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '
check_abbrev 40 --no-abbrev
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+   test_must_fail git blame --exclude-promisor-objects one
+'
+
 test_done
-- 
2.19.1.568.g152ad8e336-goog

[RFC 1/2] Documentation/git-log.txt: do not show --exclude-promisor-objects

Do not suggest that --exclude-promisor-objects is supported by git-log,
since it currently BUG-crashes and it's not necessary to support it.
Options that control behavior for promisor objects should be limited to
a small number of commands.

Signed-off-by: Matthew DeVore 
---
 Documentation/rev-list-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 5f1672913b..bab5f50b17 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -761,7 +761,6 @@ Unexpected missing objects will raise an error.
 +
 The form '--missing=print' is like 'allow-any', but will also print a
 list of the missing objects.  Object IDs are prefixed with a ``?'' character.
-endif::git-rev-list[]
 
 --exclude-promisor-objects::
(For internal use only.)  Prefilter object traversal at
@@ -769,6 +768,7 @@ endif::git-rev-list[]
stronger than `--missing=allow-promisor` because it limits the
traversal, rather than just silencing errors about missing
objects.
+endif::git-rev-list[]
 
 --no-walk[=(sorted|unsorted)]::
Only show the given commits, but do not traverse their ancestors.
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH v3 10/12] Add a base implementation of SHA-256 support

2018-10-22 Thread brian m. carlson

On Mon, Oct 22, 2018 at 11:44:40AM +0200, SZEDER Gábor wrote:
> To protect us from potential "macro redefinition" errors, these
> #undefs should come before the #defines above.  Note also that BLKSIZE
> is not #undef-ed.

Ah, okay.  I think I misread your suggestion.  I'll see if anyone has
more comments, and then reroll with that change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 1/1] protocol: advertise multiple supported versions

> > similar to argv_array_pushl  or would that be overengineered?

> Am I missing some other way to do this cleanly? I'll admit I'm not very
> familiar with va_lists.

Ah, you're right. By not passing pointers (and I am unfamiliar
with va_lists, too), this was a moot suggestion.
>
> Yeah, my understanding is that we don't want to assume that version
> identifiers will always be simple integers. We could get away with
> sprintf() for now, but I figured I didn't want to cause future pain
> if/when the version identifiers become complex.

Makes sense.

Stefan

Re: [PATCH v2] send-email: explicitly disable authentication

Joshua Watt  writes:

> It can be necessary to disable SMTP authentication by a mechanism other
> than sendemail.smtpuser being undefined. For example, if the user has
> sendemail.smtpuser set globally but wants to disable authentication
> locally in one repository.

I wonder if it would be more productive to introduce a mechanism
that can be used to address that use case more directly.  For
example, would it help to teach "git send-email" that
sendemail.smtpuser set to a particular value (say '!', or empty
string if you prefer) is equivalent to the variable unset at all?

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

> Stepping back a bit, I'd imagine in an ideal world where "make
> coccicheck" can be done instantaneously _and_ the spatch machinery
> is just as reliable as C compilers.
> [...]
> Now we do not live in that ideal world and [...]
>  such a series will have zero
> chance of landing in 'pu', unless we freeze the world.

I wonder if we could approximate the ideal world with
rerere+spatch a bit more:

1) I resend the series that includes "apply cocci patches"
as the last patch and you queue it as usual

2) The first time such a series is merged, you'd merge
HEAD^ (i.e. excluding the "apply the semantic patch)
to pu instead. I view this as a yet-to-be invented mode
'--theirs-is-stale-use-tree-instead=THEIRS~1^{tree}',
then run spatch to reproduce the last patch into the
dirty merge (which has pu and the last patch as parent)

This step is done to 'pre-heat' the rerere cache.

3) Any further integration (e.g. rebuilding pu) would
benefit from the hot rerere cache and very little work
is actually required as the conflicts are resolved by rerere.

Am I overestimating or misunderstanding rerere here?

> What I _could_ do (and what I did do only for one round of pushing
> out 'pu') is to queue a coccinelle transformation at the tip of
> integration branches.  If "make coccicheck" ran in subsecond, I
> could even automate it in the script that is used to rebuild 'pu'
> every day, so that after merging each and every topic, do the "make
> coccicheck" and apply resulting .cocci.patch files and squash that
> into the merge commit.
>
> But with the current "make coccicheck" performance, that is not
> realistic.

Would it be realistic for next and master branch instead of pu?

I'd be wary for the master branch, as we may not want to rely on
spatch without review. (It can produce funny white space issues,
but seems to produce working/correct code)

> I am wondering if it is feasible to do it at the tip of 'pu' (which
> is rebuilt two to three times a day), 'next' (which is updated once
> or twice a week) and 'master'.

We could even optimize that, by checking if contrib/cocci/ has
changes for the new tip of next/master respectively.

Another thing I wonder is if we care about the distinction between
the (a) pending changes as described by SZEDER, as we introduce
these deliberately, whereas (b) undesirable code patterns
(e.g. free and null instead of FREE_AND_NULL macro) might be
caught and reported in pu/next and then someone learns from it.
Automatic rewriting the (b) cases seems not just as desirable as
(a), where we do it purely to avoid resolving merge conflicts by
hand.

> I find that your "pending" idea may be nicer, as it distributes the
> load.  Whoever wants to change the world order by updating the .cocci
> rules is primarily responsible for making it happen without breaking
> the world during the transition.  That's more scalable.

... and I think SZEDER considers the current world broken as
'make coccicheck' returns non-empty, so it sounds to me as if
the current transition is thought less-than-optimal.

Re: [PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds

Ben Peart  writes:

> From: Ben Peart 
>
> refresh_index() is done after a reset command as an optimization.  Because
> it can be an expensive call, warn the user if it takes more than 2 seconds
> and tell them how to avoid it using the --quiet command line option or
> reset.quiet config setting.

I am moderately negative on this step.  It will irritate users who
know about and still choose not to use the "--quiet" option, because
they want to gain performance in later real work and/or they want to
know what paths are now dirty.  A working tree that needs long time
to refresh will take long time to instead do "cached stat info says
it may be modified so let's run 'diff' for real---we may discover
that there wasn't any change after all" when a "git diff" is run
after a "reset --quiet" that does not refresh; i.e. there would be
valid reasons to run "reset" without "--quiet".

It feels a bit irresponsible to throw an ad without informing
pros-and-cons and to pretend that we are advising on BCP.  In
general, we do *not* advertise new features randomly like this.

Thanks.  The previous two steps looks quite sensible.

Re: [PATCH v2] archive: initialize archivers earlier

On 2018.10.22 20:06, Jeff King wrote:
> On Mon, Oct 22, 2018 at 04:51:27PM -0700, Josh Steadmon wrote:
> 
> > > > +test_expect_success GZIP 'git archive with --output and --remote uses 
> > > > expected format' '
> > > > +   git archive --output=d5.tgz --remote=. HEAD &&
> > > > +   gzip -d -c < d5.tgz > d5.tar &&
> > > > +   test_cmp_bin b.tar d5.tar
> > > > +'
> > > 
> > > This nicely tests the more-interesting tgz case. But unfortunately it
> > > won't run on machines without the GZIP prerequisite. I'd think that
> > > would really be _most_ machines, but is it worth having a separate zip
> > > test to cover machines without gzip? I guess that just creates the
> > > opposite problem: not everybody has ZIP.
> > 
> > Added a test to compare the file lists from the .zip file to the
> > reference .tar file. I'm not sure if this is the best way to do things,
> > but it at least verifies that a .zip is produced. However, it's brittle
> > if the output of "zip -sf" changes. Let me know if you have a better
> > idea.
> 
> I wonder if we could do something more black-box. What we really care
> about here is not the exact output, but rather that "-o foo.zip"
> produces the same output as "--format zip". Could we do that without
> even relying on ZIP?
> 
> I think it should follow even for tgz, because we use "-n" for a
> repeatable output. But there we are relying on an external gzip just to
> _create_ the file, so we'd still need the GZIP prereq.
> 
> Hmm. Looks like we already have a similar test in t5003. So maybe just:
> 
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 55c7870997..cf19f56924 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with 
> --output' \
>  'git archive --format=zip --output=d2.zip HEAD &&
>  test_cmp_bin d.zip d2.zip'
>  
> -test_expect_success 'git archive with --output, inferring format' '
> +test_expect_success 'git archive with --output, inferring format (local)' '
>   git archive --output=d3.zip HEAD &&
>   test_cmp_bin d.zip d3.zip
>  '
>  
> +test_expect_success 'git archive with --output, ferring format (remote)' '
> + git archive --remote=. --output=d4.zip HEAD &&
> + test_cmp_bin d.zip d4.zip
> +'
> +
>  test_expect_success \
>  'git archive --format=zip with prefix' \
>  'git archive --format=zip --prefix=prefix/ HEAD >e.zip'
> 
> which I think exposes the bug and can run everywhere?

Makes sense, thanks!

[PATCH v4] archive: initialize archivers earlier

2018-10-22 Thread steadmon

Initialize archivers as soon as possible when running git-archive.
Various non-obvious behavior depends on having the archivers
initialized, such as determining the desired archival format from the
provided filename.

Since 08716b3c11 ("archive: refactor file extension format-guessing",
2011-06-21), archive_format_from_filename() has used the registered
archivers to match filenames (provided via --output) to archival
formats. However, when git-archive is executed with --remote, format
detection happens before the archivers have been registered. This causes
archives from remotes to always be generated as TAR files, regardless of
the actual filename (unless an explicit --format is provided).

This patch fixes that behavior; archival format is determined properly
from the output filename, even when --remote is used.

Signed-off-by: Josh Steadmon 
Helped-by: Jeff King 
---
Range-diff against v3:
1:  39a4e7bf8f ! 1:  1d7b070928 archive: initialize archivers earlier
@@ -91,15 +91,6 @@
  --- a/t/t5000-tar-tree.sh
  +++ b/t/t5000-tar-tree.sh
 @@
- 
- test_lazy_prereq GZIP 'gzip --version'
- 
-+test_lazy_prereq ZIP 'zip --version'
-+
- get_pax_header() {
-   file=$1
-   header=$2=
-@@
test_cmp_bin b.tar d4.zip
  '
  
@@ -108,14 +99,29 @@
 +  gzip -d -c < d5.tgz > d5.tar &&
 +  test_cmp_bin b.tar d5.tar
 +'
-+
-+test_expect_success ZIP 'git archive with --output and --remote creates 
.zip' '
-+  git archive --output=d6.zip --remote=. HEAD &&
-+  zip -sf d6.zip | sed "/^[^ ]/d" | sed "s/^  //" | sort > zip_manifest &&
-+  "$TAR" tf b.tar | sort > tar_manifest &&
-+  test_cmp zip_manifest tar_manifest
-+'
 +
  test_expect_success 'git archive --list outside of a git repo' '
nongit git archive --list
  '
+
+ diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
+ --- a/t/t5003-archive-zip.sh
+ +++ b/t/t5003-archive-zip.sh
+@@
+ 'git archive --format=zip --output=d2.zip HEAD &&
+ test_cmp_bin d.zip d2.zip'
+ 
+-test_expect_success 'git archive with --output, inferring format' '
++test_expect_success 'git archive with --output, inferring format (local)' 
'
+   git archive --output=d3.zip HEAD &&
+   test_cmp_bin d.zip d3.zip
+ '
+ 
++test_expect_success 'git archive with --output, inferring format 
(remote)' '
++  git archive --remote=. --output=d4.zip HEAD &&
++  test_cmp_bin d.zip d4.zip
++'
++
+ test_expect_success \
+ 'git archive --format=zip with prefix' \
+ 'git archive --format=zip --prefix=prefix/ HEAD >e.zip'

 archive.c| 9 ++---
 archive.h| 1 +
 builtin/archive.c| 2 ++
 builtin/upload-archive.c | 2 ++
 t/t5000-tar-tree.sh  | 6 ++
 t/t5003-archive-zip.sh   | 7 ++-
 6 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/archive.c b/archive.c
index c1870105eb..ce0f8a0362 100644
--- a/archive.c
+++ b/archive.c
@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)
archivers[nr_archivers++] = ar;
 }
 
+void init_archivers(void)
+{
+   init_tar_archiver();
+   init_zip_archiver();
+}
+
 static void format_subst(const struct commit *commit,
  const char *src, size_t len,
  struct strbuf *buf)
@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
git_config_get_bool("uploadarchive.allowunreachable", 
_allow_unreachable);
git_config(git_default_config, NULL);
 
-   init_tar_archiver();
-   init_zip_archiver();
-
args.repo = repo;
argc = parse_archive_args(argc, argv, , , name_hint, remote);
if (!startup_info->have_repository) {
diff --git a/archive.h b/archive.h
index d4f97a00f5..21ac010699 100644
--- a/archive.h
+++ b/archive.h
@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);
 
 extern void init_tar_archiver(void);
 extern void init_zip_archiver(void);
+extern void init_archivers(void);
 
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
const struct object_id *oid,
diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..d2455237ce 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, local_opts, NULL,
 PARSE_OPT_KEEP_ALL);
 
+   init_archivers();
+
if (output)
create_output_file(output);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 25d9116356..018879737a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
if (!enter_repo(argv[1], 0))

Re: [PATCH v2] archive: initialize archivers earlier

On Mon, Oct 22, 2018 at 04:51:27PM -0700, Josh Steadmon wrote:

> > > +test_expect_success GZIP 'git archive with --output and --remote uses 
> > > expected format' '
> > > + git archive --output=d5.tgz --remote=. HEAD &&
> > > + gzip -d -c < d5.tgz > d5.tar &&
> > > + test_cmp_bin b.tar d5.tar
> > > +'
> > 
> > This nicely tests the more-interesting tgz case. But unfortunately it
> > won't run on machines without the GZIP prerequisite. I'd think that
> > would really be _most_ machines, but is it worth having a separate zip
> > test to cover machines without gzip? I guess that just creates the
> > opposite problem: not everybody has ZIP.
> 
> Added a test to compare the file lists from the .zip file to the
> reference .tar file. I'm not sure if this is the best way to do things,
> but it at least verifies that a .zip is produced. However, it's brittle
> if the output of "zip -sf" changes. Let me know if you have a better
> idea.

I wonder if we could do something more black-box. What we really care
about here is not the exact output, but rather that "-o foo.zip"
produces the same output as "--format zip". Could we do that without
even relying on ZIP?

I think it should follow even for tgz, because we use "-n" for a
repeatable output. But there we are relying on an external gzip just to
_create_ the file, so we'd still need the GZIP prereq.

Hmm. Looks like we already have a similar test in t5003. So maybe just:

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 55c7870997..cf19f56924 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with 
--output' \
 'git archive --format=zip --output=d2.zip HEAD &&
 test_cmp_bin d.zip d2.zip'
 
-test_expect_success 'git archive with --output, inferring format' '
+test_expect_success 'git archive with --output, inferring format (local)' '
git archive --output=d3.zip HEAD &&
test_cmp_bin d.zip d3.zip
 '
 
+test_expect_success 'git archive with --output, ferring format (remote)' '
+   git archive --remote=. --output=d4.zip HEAD &&
+   test_cmp_bin d.zip d4.zip
+'
+
 test_expect_success \
 'git archive --format=zip with prefix' \
 'git archive --format=zip --prefix=prefix/ HEAD >e.zip'

which I think exposes the bug and can run everywhere?

-Peff

[PATCH v3] archive: initialize archivers earlier

2018-10-22 Thread steadmon

Initialize archivers as soon as possible when running git-archive.
Various non-obvious behavior depends on having the archivers
initialized, such as determining the desired archival format from the
provided filename.

Since 08716b3c11 ("archive: refactor file extension format-guessing",
2011-06-21), archive_format_from_filename() has used the registered
archivers to match filenames (provided via --output) to archival
formats. However, when git-archive is executed with --remote, format
detection happens before the archivers have been registered. This causes
archives from remotes to always be generated as TAR files, regardless of
the actual filename (unless an explicit --format is provided).

This patch fixes that behavior; archival format is determined properly
from the output filename, even when --remote is used.

Signed-off-by: Josh Steadmon 
Helped-by: Jeff King 
---
Range-diff against v2:
1:  bc6f20274d ! 1:  39a4e7bf8f archive: initialize archivers earlier
@@ -78,26 +78,43 @@
  --- a/builtin/upload-archive.c
  +++ b/builtin/upload-archive.c
 @@
-   }
+   if (!enter_repo(argv[1], 0))
+   die("'%s' does not appear to be a git repository", argv[1]);
  
-   /* parse all options sent by the client */
 +  init_archivers();
-   return write_archive(sent_argv.argc, sent_argv.argv, prefix,
-the_repository, NULL, 1);
- }
++
+   /* put received options in sent_argv[] */
+   argv_array_push(_argv, "git-upload-archive");
+   for (;;) {
 
  diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
  --- a/t/t5000-tar-tree.sh
  +++ b/t/t5000-tar-tree.sh
 @@
+ 
+ test_lazy_prereq GZIP 'gzip --version'
+ 
++test_lazy_prereq ZIP 'zip --version'
++
+ get_pax_header() {
+   file=$1
+   header=$2=
+@@
test_cmp_bin b.tar d4.zip
  '
  
-+test_expect_success GZIP 'git archive with --output and --remote uses 
expected format' '
++test_expect_success GZIP 'git archive with --output and --remote creates 
.tgz' '
 +  git archive --output=d5.tgz --remote=. HEAD &&
 +  gzip -d -c < d5.tgz > d5.tar &&
 +  test_cmp_bin b.tar d5.tar
 +'
++
++test_expect_success ZIP 'git archive with --output and --remote creates 
.zip' '
++  git archive --output=d6.zip --remote=. HEAD &&
++  zip -sf d6.zip | sed "/^[^ ]/d" | sed "s/^  //" | sort > zip_manifest &&
++  "$TAR" tf b.tar | sort > tar_manifest &&
++  test_cmp zip_manifest tar_manifest
++'
 +
  test_expect_success 'git archive --list outside of a git repo' '
nongit git archive --list

 archive.c|  9 ++---
 archive.h|  1 +
 builtin/archive.c|  2 ++
 builtin/upload-archive.c |  2 ++
 t/t5000-tar-tree.sh  | 15 +++
 5 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/archive.c b/archive.c
index c1870105eb..ce0f8a0362 100644
--- a/archive.c
+++ b/archive.c
@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)
archivers[nr_archivers++] = ar;
 }
 
+void init_archivers(void)
+{
+   init_tar_archiver();
+   init_zip_archiver();
+}
+
 static void format_subst(const struct commit *commit,
  const char *src, size_t len,
  struct strbuf *buf)
@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
git_config_get_bool("uploadarchive.allowunreachable", 
_allow_unreachable);
git_config(git_default_config, NULL);
 
-   init_tar_archiver();
-   init_zip_archiver();
-
args.repo = repo;
argc = parse_archive_args(argc, argv, , , name_hint, remote);
if (!startup_info->have_repository) {
diff --git a/archive.h b/archive.h
index d4f97a00f5..21ac010699 100644
--- a/archive.h
+++ b/archive.h
@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);
 
 extern void init_tar_archiver(void);
 extern void init_zip_archiver(void);
+extern void init_archivers(void);
 
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
const struct object_id *oid,
diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..d2455237ce 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, local_opts, NULL,
 PARSE_OPT_KEEP_ALL);
 
+   init_archivers();
+
if (output)
create_output_file(output);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 25d9116356..018879737a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
if (!enter_repo(argv[1], 0))
die("'%s' does not appear

Re: [PATCH v2] archive: initialize archivers earlier

On 2018.10.22 18:35, Jeff King wrote:
> On Mon, Oct 22, 2018 at 02:48:11PM -0700, stead...@google.com wrote:
> 
> > Initialize archivers as soon as possible when running git-archive and
> > git-upload-archive. Various non-obvious behavior depends on having the
> > archivers initialized, such as determining the desired archival format
> > from the provided filename.
> > 
> > Since 08716b3c11 ("archive: refactor file extension format-guessing",
> > 2011-06-21), archive_format_from_filename() has used the registered
> > archivers to match filenames (provided via --output) to archival
> > formats. However, when git-archive is executed with --remote, format
> > detection happens before the archivers have been registered. This causes
> > archives from remotes to always be generated as TAR files, regardless of
> > the actual filename (unless an explicit --format is provided).
> > 
> > This patch fixes that behavior; archival format is determined properly
> > from the output filename, even when --remote is used.
> > 
> > Signed-off-by: Josh Steadmon 
> > Helped-by: Jeff King 
> 
> Thanks, this looks good overall.
> 
> A few minor comments (that I'm not even sure are worth re-rolling for):
> 
> > diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> > index 25d9116356..3f35ebcfe8 100644
> > --- a/builtin/upload-archive.c
> > +++ b/builtin/upload-archive.c
> > @@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char 
> > **argv, const char *prefix)
> > }
> >  
> > /* parse all options sent by the client */
> > +   init_archivers();
> > return write_archive(sent_argv.argc, sent_argv.argv, prefix,
> >  the_repository, NULL, 1);
> >  }
> 
> This seems to separate the comment from what it describes. Any reason
> not to just init_archivers() closer to the top of the function here
> (probably after the enter_repo() call)?

Ack, fixed.


> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> > index 2a97b27b0a..3e95fdf660 100755
> > --- a/t/t5000-tar-tree.sh
> > +++ b/t/t5000-tar-tree.sh
> > @@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, 
> > override inferred format' '
> > test_cmp_bin b.tar d4.zip
> >  '
> >  
> > +test_expect_success GZIP 'git archive with --output and --remote uses 
> > expected format' '
> > +   git archive --output=d5.tgz --remote=. HEAD &&
> > +   gzip -d -c < d5.tgz > d5.tar &&
> > +   test_cmp_bin b.tar d5.tar
> > +'
> 
> This nicely tests the more-interesting tgz case. But unfortunately it
> won't run on machines without the GZIP prerequisite. I'd think that
> would really be _most_ machines, but is it worth having a separate zip
> test to cover machines without gzip? I guess that just creates the
> opposite problem: not everybody has ZIP.

Added a test to compare the file lists from the .zip file to the
reference .tar file. I'm not sure if this is the best way to do things,
but it at least verifies that a .zip is produced. However, it's brittle
if the output of "zip -sf" changes. Let me know if you have a better
idea.

Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

Jeff King  writes:

> If nobody uses it, should we drop the return value, too? Like:

Yup.

>
> diff --git a/read-cache.c b/read-cache.c
> index 78c9516eb7..4b44a2eae5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)
>   return NULL;
>  }
>  
> -static unsigned long load_cache_entries_threaded(struct index_state *istate, 
> const char *mmap, size_t mmap_size,
> -  int nr_threads, struct 
> index_entry_offset_table *ieot)
> +static void load_cache_entries_threaded(struct index_state *istate, const 
> char *mmap, size_t mmap_size,
> + int nr_threads, struct 
> index_entry_offset_table *ieot)
>  {
>   int i, offset, ieot_blocks, ieot_start, err;
>   struct load_cache_entries_thread_data *data;
> - unsigned long consumed = 0;
>  
>   /* a little sanity checking */
>   if (istate->name_hash_initialized)
> @@ -2115,12 +2114,9 @@ static unsigned long 
> load_cache_entries_threaded(struct index_state *istate, con
>   if (err)
>   die(_("unable to join load_cache_entries thread: %s"), 
> strerror(err));
>   mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
> - consumed += p->consumed;
>   }
>  
>   free(data);
> -
> - return consumed;
>  }
>  #endif
>  
>
> -Peff

Re: [PATCH] Poison gettext with the Ook language

Ævar Arnfjörð Bjarmason  writes:

> So I think the only reason to keep it compile-time is performance, but I
> don't think that matters. It's not like we're printing gigabytes of _()
> formatted output. Everything where formatting matters is plumbing which
> doesn't use this API. These messages are always for human consumption.
>
> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time
> entirely, and make GIT_GETTEXT_POISON= a test flag that all
> builds of git are aware of? I think so.

Haven't thought things through, but that sounds like a good thing to
aim for.  Keep the ideas coming...

Re: [PATCH v2 1/1] protocol: advertise multiple supported versions

On 2018.10.12 15:30, Stefan Beller wrote:
> On Thu, Oct 11, 2018 at 6:02 PM  wrote:
> >
> > From: Josh Steadmon 
> >
> > Currently the client advertises that it supports the wire protocol
> > version set in the protocol.version config. However, not all services
> > support the same set of protocol versions. When connecting to
> > git-receive-pack, the client automatically downgrades to v0 if
> > config.protocol is set to v2, but this check is not performed for other
> > services.
> >
> > This patch creates a protocol version registry. Individual commands
> > register all the protocol versions they support prior to communicating
> > with a server. Versions should be listed in preference order; the
> > version specified in protocol.version will automatically be moved to the
> > front of the registry.
> >
> > The protocol version registry is passed to remote helpers via the
> > GIT_PROTOCOL environment variable.
> >
> > Clients now advertise the full list of registered versions. Servers
> > select the first recognized version from this advertisement.
> 
> I like this patch from the users point of view (i.e. inside fetch/push etc),
> and I need to through the details in connect/protocol as that seems
> to be a lot of new code, but hides the complexity very well.
> 
> > +   register_allowed_protocol_version(protocol_v2);
> > +   register_allowed_protocol_version(protocol_v1);
> > +   register_allowed_protocol_version(protocol_v0);
> 
> Would it make sense to have a
> 
> register_allowed_protocol_versions(protocol_v2, protocol_v1,
> protocol_v0, NULL);
> 
> similar to argv_array_pushl  or would that be overengineered?

Hmm, well it wouldn't be quite as clean as the argv_* versions, since we
can't take the pointer of an enum value, so we don't have a good
stop-value for the va_list. I suppose we could use
protocol_unknown_version, but that seems unintuitive to me. We could
also pass in an explicit argument count, but... ugh.

Am I missing some other way to do this cleanly? I'll admit I'm not very
familiar with va_lists.

> > @@ -1085,10 +1085,9 @@ static struct child_process *git_connect_git(int 
> > fd[2], char *hostandport,
> > target_host, 0);
> >
> > /* If using a new version put that stuff here after a second null 
> > byte */
> > -   if (version > 0) {
> > +   if (strcmp(version_advert->buf, "version=0")) {
> > strbuf_addch(, '\0');
> > -   strbuf_addf(, "version=%d%c",
> > -   version, '\0');
> > +   strbuf_addf(, "%s%c", version_advert->buf, '\0');
> 
> It's a bit unfortunate to pass around the string, but reading through
> the following
> lines seems like it is easiest.
> 
> 
> > @@ -1226,16 +1226,10 @@ struct child_process *git_connect(int fd[2], const 
> > char *url,
> >  {
> > char *hostandport, *path;
> > struct child_process *conn;
> > +   struct strbuf version_advert = STRBUF_INIT;
> > enum protocol protocol;
> > -   enum protocol_version version = get_protocol_version_config();
> >
> > -   /*
> > -* NEEDSWORK: If we are trying to use protocol v2 and we are 
> > planning
> > -* to perform a push, then fallback to v0 since the client doesn't 
> > know
> > -* how to push yet using v2.
> > -*/
> > -   if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
> > -   version = protocol_v0;
> > +   get_client_protocol_version_advertisement(_advert);
> 
> Nice to have this special casing gone!
> 
> > diff --git a/protocol.c b/protocol.c
> > index 5e636785d1..f788269c47 100644
> > +
> > +static const char protocol_v0_string[] = "0";
> > +static const char protocol_v1_string[] = "1";
> > +static const char protocol_v2_string[] = "2";
> > +
> ...
> > +/* Return the text representation of a wire protocol version. */
> > +static const char *format_protocol_version(enum protocol_version version)
> > +{
> > +   switch (version) {
> > +   case protocol_v0:
> > +   return protocol_v0_string;
> > +   case protocol_v1:
> > +   return protocol_v1_string;
> > +   case protocol_v2:
> > +   return protocol_v2_string;
> > +   case protocol_unknown_version:
> > +   die(_("Unrecognized protocol version"));
> > +   }
> > +   die(_("Unrecognized protocol_version"));
> > +}
> 
> Up to now we have the textual representation that can easily
> be constructed from protocol_version by using e.g.
> sprintf(buf, "%d", version);
> which is why I initially thought this could be worded way
> shorter, but I guess this is more future proof?

Yeah, my understanding is that we don't want to assume that version
identifiers will always be simple integers. We could get away with
sprintf() for now, but I figured I didn't want to cause future pain
if/when the version identifiers become complex.

> > +
> >  enum protocol_version

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

SZEDER Gábor  writes:

> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
> ...
> How about introducing the concept of "pending" semantic patches,
> stored in 'contrib/coccinelle/.pending.cocci' files, modifying
> 'make coccicheck' to skip them, and adding the new 'make
> coccicheck-pending' target to make it convenient to apply them, e.g.
> something like the simple patch at the end.
>
> So the process would go something like this:
>
>   - A new semantic patch should be added as "pending", e.g. to the
> file 'the_repository.pending.cocci', together with the resulting
> transformations in the same commit.
>
> This way neither 'make coccicheck' nor the static analysis build
> job would complain in the topic branch or in the two integration
> branches.  And if they do complain, then we would know right away
> that they complain because of a well-established semantic patch.
> Yet, anyone interested could run 'make coccicheck-pending' to see
> where are we heading.
>
>   - The author of the "pending" semanting patch should then keep an
> eye on already cooking topics: whether any of them contain new
> code that should be transformed, and how they progress to
> 'master', and sending followup patch(es) with the remaining
> transformations when applicable.
> 
> Futhermore, the author should also pay attention to any new topics
> that branch off after the "pending" semantic patch, and whether
> any of them introduce code to be transformed, warning their
> authors as necessary.
>
>   - Finally, after all the dust settled, the dev should follow up with
> a patch to:
> 
>   - promote the "penging" patch to '.cocci', if its purpose
> is to avoid undesirable code patterns in the future, or
> 
>   - remove the semantic patch, if it was used in a one-off
> transformation.
> ...
> Thoughts?

I actually think this round does a far nicer job playing well with
other topics than any earlier series.  The pain you are observing I
think come primarily from my not making the best use of these
patches.

Steppng back a bit, I'd imagine in an ideal world where "make
coccicheck" can be done instantaneously _and_ the spatch machinery
is just as reliable as C compilers.  In such a world, I may rename
all the *.c files in my tree to *.c.in, make the very first step of
the "make all" to run coccicheck and transform *.c.in to *.c before
starting the compilation.  There is no need to have changes to
*.c.in that spatch would fix.  Such an idealized set-up has a few
nice property.

 - Mechanical conflict resolutions between topics in flight and a
   series that modifies or adds new .cocci rules would greatly be
   reduced.

 - Still, *.c files that are "compiled" from *.c.in file used by
   each build will by definition cocci-clean.

 - Bugs like the one that required 6afedba8 ("object_id.cocci: match
   only expressions of type 'struct object_id'", 2018-10-15) are
   still possible, but some of them may be caught by C compilers
   that inspect the result of spatch compilation from *.c.in to *.c

Now we do not live in that ideal world and there is no separate
"turn *.c.in into *.c" step in our build procedure.  In such a
"real" world, if we had a rule like "each individual commit must
pass 'make coccicheck' cleanly", anybody who does such a merge and
resolve huge conflics would essentially be wasting time on something
that the tool could do, and then the result of the merge would
further need to be amended for semantic conflicts (i.e. the other
branch may have introduced new instances of old pattern .cocci
patches in our branch would want to transform)).  By not insisting
on cocci-cleanness at each commit level, we can gain (or at least
sumulate gaining) some benefit that we would reap in the ideal
world, and Stefan's latest series already does so for the former.
If we insisted that these patches must be accompanied with the
result of the cocci transformations, such a series will have zero
chance of landing in 'pu', unless we freeze the world.

What I _could_ do (and what I did do only for one round of pushing
out 'pu') is to queue a coccinelle transformation at the tip of
integration branches.  If "make coccicheck" ran in subsecond, I
could even automate it in the script that is used to rebuild 'pu'
every day, so that after merging each and every topic, do the "make
coccicheck" and apply resulting .cocci.patch files and squash that
into the merge commit.

But with the current "make coccicheck" performance, that is not
realistic.

I am wondering if it is feasible to do it at the tip of 'pu' (which
is rebuilt two to three times a day), 'next' (which is updated once
or twice a week) and 'master'.

I find that your "pending" idea may be nicer, as it distributes the
load.  Whoever wants to change the world order

Re: [PATCH v2] archive: initialize archivers earlier

On Mon, Oct 22, 2018 at 02:48:11PM -0700, stead...@google.com wrote:

> Initialize archivers as soon as possible when running git-archive and
> git-upload-archive. Various non-obvious behavior depends on having the
> archivers initialized, such as determining the desired archival format
> from the provided filename.
> 
> Since 08716b3c11 ("archive: refactor file extension format-guessing",
> 2011-06-21), archive_format_from_filename() has used the registered
> archivers to match filenames (provided via --output) to archival
> formats. However, when git-archive is executed with --remote, format
> detection happens before the archivers have been registered. This causes
> archives from remotes to always be generated as TAR files, regardless of
> the actual filename (unless an explicit --format is provided).
> 
> This patch fixes that behavior; archival format is determined properly
> from the output filename, even when --remote is used.
> 
> Signed-off-by: Josh Steadmon 
> Helped-by: Jeff King 

Thanks, this looks good overall.

A few minor comments (that I'm not even sure are worth re-rolling for):

> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 25d9116356..3f35ebcfe8 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
> const char *prefix)
>   }
>  
>   /* parse all options sent by the client */
> + init_archivers();
>   return write_archive(sent_argv.argc, sent_argv.argv, prefix,
>the_repository, NULL, 1);
>  }

This seems to separate the comment from what it describes. Any reason
not to just init_archivers() closer to the top of the function here
(probably after the enter_repo() call)?

> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0a..3e95fdf660 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override 
> inferred format' '
>   test_cmp_bin b.tar d4.zip
>  '
>  
> +test_expect_success GZIP 'git archive with --output and --remote uses 
> expected format' '
> + git archive --output=d5.tgz --remote=. HEAD &&
> + gzip -d -c < d5.tgz > d5.tar &&
> + test_cmp_bin b.tar d5.tar
> +'

This nicely tests the more-interesting tgz case. But unfortunately it
won't run on machines without the GZIP prerequisite. I'd think that
would really be _most_ machines, but is it worth having a separate zip
test to cover machines without gzip? I guess that just creates the
opposite problem: not everybody has ZIP.

-Peff

Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

On Mon, Oct 22, 2018 at 03:15:05PM -0700, Johannes Schindelin via GitGitGadget 
wrote:
> From: Johannes Schindelin 
> 
> When `git stash apply ` sees an argument that consists only of
> digits, it tries to be smart and interpret it as `stash@{}`.
> 
> Unfortunately, an all-digit hash (which is unlikely but still possible)
> is therefore misinterpreted as `stash@{}` reflog.
> 
> To prevent that from happening, let's append `^0` after the stash hash,
> to make sure that it is interpreted as an OID rather than as a number.

Oh, this is clever.

FWIW, all patches look good to me, barring the typo below.

> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 418624837..30d58118c 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)
>  
>   if (read_one(path, ))
>   return error(_("Could not read '%s'"), path);
> + /* Ensure that the hash is not mistake for a number */

s/mistake/mistaken/

> + strbuf_addstr(, "^0");
>   argv_array_pushl(_apply.args,
>"stash", "apply", autostash.buf, NULL);
>   stash_apply.git_cmd = 1;
> -- 
> gitgitgadget

Re: [PATCH 1/1] archive: init archivers before determining format

On Mon, Oct 22, 2018 at 02:47:56PM -0700, Josh Steadmon wrote:

> > Does this work with configured archiver extensions, too? I think so,
> > because we load them via init_tar_archiver().
> 
> If you mean things like .tgz and .tar.gz, then yes, they are affected by
> the bug as well, and this patch fixes them. The test included in v2 uses
> a .tgz file.

Yes, but also ones that are provided by the user. E.g., does:

  git config tar.foo.command "foo"
  git archive -o out.tar.foo HEAD

work? (I think the answer is yes, because we read git_tar_config in the
same place).

-Peff

Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

On Mon, Oct 22, 2018 at 6:15 PM Johannes Schindelin via GitGitGadget
 wrote:
> When `git stash apply ` sees an argument that consists only of
> digits, it tries to be smart and interpret it as `stash@{}`.
>
> Unfortunately, an all-digit hash (which is unlikely but still possible)
> is therefore misinterpreted as `stash@{}` reflog.
>
> To prevent that from happening, let's append `^0` after the stash hash,
> to make sure that it is interpreted as an OID rather than as a number.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> @@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)
>
> if (read_one(path, ))
> return error(_("Could not read '%s'"), path);
> +   /* Ensure that the hash is not mistake for a number */

s/mistake/mistaken/

[PATCH v2 2/3] rebase (autostash): store the full OID in /autostash

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

It was reported by Gábor Szeder and analyzed by Alban Gruin that the
built-in rebase stores only abbreviated stash hashes in the `autostash`
file.

This is problematic e.g. in t5520-pull.sh, where the abbreviated hash is
so short that it sometimes consists only of digits, which are
subsequently mistaken ("DWIMmed") for numbers by `git stash apply`.

Let's align the behavior of the built-in rebase with the scripted rebase
and store the full stash hash instead. That makes it a lot less likely
that it consists only of digits.

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d21504d9f..418624837 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1375,7 +1375,7 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
if (safe_create_leading_directories_const(autostash))
die(_("Could not create directory for '%s'"),
options.state_dir);
-   write_file(autostash, "%s", buf.buf);
+   write_file(autostash, "%s", oid_to_hex());
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(>object.oid, "reset --hard",
   NULL, 0, NULL, NULL) < 0)
-- 
gitgitgadget

[PATCH v2 1/3] rebase (autostash): avoid duplicate call to state_dir_path()

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

We already called that function at this point, and stored the result in
the `path` variable. We might just as well use it ;-)

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 313a8263d..d21504d9f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -251,7 +251,7 @@ static int apply_autostash(struct rebase_options *opts)
if (!file_exists(path))
return 0;
 
-   if (read_one(state_dir_path("autostash", opts), ))
+   if (read_one(path, ))
return error(_("Could not read '%s'"), path);
argv_array_pushl(_apply.args,
 "stash", "apply", autostash.buf, NULL);
-- 
gitgitgadget

[PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

When `git stash apply ` sees an argument that consists only of
digits, it tries to be smart and interpret it as `stash@{}`.

Unfortunately, an all-digit hash (which is unlikely but still possible)
is therefore misinterpreted as `stash@{}` reflog.

To prevent that from happening, let's append `^0` after the stash hash,
to make sure that it is interpreted as an OID rather than as a number.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 418624837..30d58118c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)
 
if (read_one(path, ))
return error(_("Could not read '%s'"), path);
+   /* Ensure that the hash is not mistake for a number */
+   strbuf_addstr(, "^0");
argv_array_pushl(_apply.args,
 "stash", "apply", autostash.buf, NULL);
stash_apply.git_cmd = 1;
-- 
gitgitgadget

[PATCH v2 0/3] Fix rebase autostash

2018-10-22 Thread Johannes Schindelin via GitGitGadget

Gábor reported in 
https://public-inbox.org/git/20181019124625.gb30...@szeder.dev/ that 
t5520-pull.sh fails from time to time, and Alban root-caused this to a bug
in the built-in rebase.

This patch series fixes that, and while at it also fixes an oversight of
yours truly when helping Pratik with his GSoC project, and it also adds a
change on top that makes really, really certain that git stash apply 
interprets the OID passed to it correctly (as opposed to an insanely large
number for the stash reflog).

Please note that I based these patches on top of next (they might be most
appropriately applied on top of rebase-in-c-6-final, though).

(Sorry for the v2, v1 did not send due to an UTF-8 character in the Cc:
list, a bug that still needs to be fixed in GitGitGadget.)

Johannes Schindelin (3):
  rebase (autostash): avoid duplicate call to state_dir_path()
  rebase (autostash): store the full OID in /autostash
  rebase (autostash): use an explicit OID to apply the stash

 builtin/rebase.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: 500967bb5e73b67708a64a4360867cf2407ba880
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-52%2Fdscho%2Ffix-rebase-autostash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-52/dscho/fix-rebase-autostash-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/52

Range-diff vs v1:

 1:  88241ad32 = 1:  88241ad32 rebase (autostash): avoid duplicate call to 
state_dir_path()
 2:  86107a6d0 = 2:  86107a6d0 rebase (autostash): store the full OID in 
/autostash
 3:  d3b47a4c1 = 3:  07140a71d rebase (autostash): use an explicit OID to apply 
the stash

-- 
gitgitgadget

RE: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

2018-10-22 Thread Ben Peart

> -Original Message-
> From: Johannes Schindelin 
> Sent: Monday, October 22, 2018 4:45 PM
> To: Ben Peart 
> Cc: git@vger.kernel.org; gits...@pobox.com; Ben Peart
> ; p...@peff.net; sunsh...@sunshineco.com
> Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> reset when --quiet
> 
> Hi Ben,
> 
> On Mon, 22 Oct 2018, Ben Peart wrote:
> 
> > From: Ben Peart 
> >
> > When git reset is run with the --quiet flag, don't bother finding any
> > additional unstaged changes as they won't be output anyway.  This speeds
> up
> > the git reset command by avoiding having to lstat() every file looking for
> > changes that aren't going to be reported anyway.
> >
> > The savings can be significant.  In a repo with 200K files "git reset"
> > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> 
> That's very nice!
> 
> Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> 

It's safe to assume all my numbers are on Windows. :-)

> Ciao,
> Dscho

[PATCH v3 3/3] repack -ad: prune the list of shallow commits

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

`git repack` can drop unreachable commits without further warning,
making the corresponding entries in `.git/shallow` invalid, which causes
serious problems when deepening the branches.

One scenario where unreachable commits are dropped by `git repack` is
when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place, which is an assumption at least some of Git's
code seems to make).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

fatal: error in object: unshallow 

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, we also need to take `--keep-reachable`
and `--unpack-unreachable=` into account.

Note: an alternative solution discussed during the review of this patch
was to teach `git fetch` to simply ignore entries in .git/shallow if the
corresponding commits do not exist locally. A quick test, however,
revealed that the .git/shallow file is written during a shallow *clone*,
in which case the commits do not exist, either, but the "shallow" line
*does* need to be sent. Therefore, this approach would be a lot more
finicky than the approach presented by the this patch.

Signed-off-by: Johannes Schindelin 
---
 builtin/repack.c | 6 ++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5..9217fc832 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
if (!po_args.quiet && isatty(2))
opts |= PRUNE_PACKED_VERBOSE;
prune_packed_objects(opts);
+
+   if (!keep_unreachable &&
+   (!(pack_everything & LOOSEN_UNREACHABLE) ||
+unpack_unreachable) &&
+   is_repository_shallow(the_repository))
+   prune_shallow(0, 1);
}
 
if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 2d0031703..777c9d1dc 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
git init shallow-server &&
test_commit -C shallow-server A &&
test_commit -C shallow-server B &&
-- 
gitgitgadget

[PATCH v3 1/3] repack: point out a bug handling stale shallow info

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

A `git fetch --prune` can turn previously-reachable objects unreachable,
even commits that are in the `shallow` list. A subsequent `git repack
-ad` will then unceremoniously drop those unreachable commits, and the
`shallow` list will become stale. This means that when we try to fetch
with a larger `--depth` the next time, we may end up with:

fatal: error in object: unshallow 

Reported by Alejandro Pauly.

Signed-off-by: Johannes Schindelin 
---
 t/t5537-fetch-shallow.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 7045685e2..2d0031703 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,6 +186,33 @@ EOF
test_cmp expect actual
 '
 
+test_expect_failure '.git/shallow is edited by repack' '
+   git init shallow-server &&
+   test_commit -C shallow-server A &&
+   test_commit -C shallow-server B &&
+   git -C shallow-server checkout -b branch &&
+   test_commit -C shallow-server C &&
+   test_commit -C shallow-server E &&
+   test_commit -C shallow-server D &&
+   d="$(git -C shallow-server rev-parse --verify D)" &&
+   git -C shallow-server checkout master &&
+
+   git clone --depth=1 --no-tags --no-single-branch \
+   "file://$PWD/shallow-server" shallow-client &&
+
+   : now remove the branch and fetch with prune &&
+   git -C shallow-server branch -D branch &&
+   git -C shallow-client fetch --prune --depth=1 \
+   origin "+refs/heads/*:refs/remotes/origin/*" &&
+   git -C shallow-client repack -adfl &&
+   test_must_fail git -C shallow-client rev-parse --verify $d^0 &&
+   ! grep $d shallow-client/.git/shallow &&
+
+   git -C shallow-server branch branch-orig D^0 &&
+   git -C shallow-client fetch --prune --depth=2 \
+   origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
gitgitgadget

[PATCH v3 2/3] shallow: offer to prune only non-existing entries

2018-10-22 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

The `prune_shallow()` function wants a full reachability check to be
completed before it goes to work, to ensure that all unreachable entries
are removed from the shallow file.

However, in the upcoming patch we do not even want to go that far. We
really only need to remove entries corresponding to pruned commits, i.e.
to commits that no longer exist.

Let's support that use case.

Signed-off-by: Johannes Schindelin 
---
 builtin/prune.c |  2 +-
 commit.h|  2 +-
 shallow.c   | 22 +-
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f821..6d6ab6cf1 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
free(s);
 
if (is_repository_shallow(the_repository))
-   prune_shallow(show_only);
+   prune_shallow(show_only, 0);
 
return 0;
 }
diff --git a/commit.h b/commit.h
index 1d260d62f..ff34447ab 100644
--- a/commit.h
+++ b/commit.h
@@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct 
shallow_info *info,
   uint32_t **used,
   int *ref_status);
 extern int delayed_reachability_test(struct shallow_info *si, int c);
-extern void prune_shallow(int show_only);
+extern void prune_shallow(int show_only, int quick_prune);
 extern struct trace_key trace_shallow;
 
 extern int interactive_add(int argc, const char **argv, const char *prefix, 
int patch);
diff --git a/shallow.c b/shallow.c
index 732e18d54..0a2671bc2 100644
--- a/shallow.c
+++ b/shallow.c
@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository 
*r)
 
 #define SEEN_ONLY 1
 #define VERBOSE   2
+#define QUICK_PRUNE 4
 
 struct write_shallow_data {
struct strbuf *out;
@@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft 
*graft, void *cb_data)
const char *hex = oid_to_hex(>oid);
if (graft->nr_parent != -1)
return 0;
-   if (data->flags & SEEN_ONLY) {
+   if (data->flags & QUICK_PRUNE) {
+   struct commit *c = lookup_commit(the_repository, >oid);
+   if (!c || parse_commit(c))
+   return 0;
+   } else if (data->flags & SEEN_ONLY) {
struct commit *c = lookup_commit(the_repository, >oid);
if (!c || !(c->object.flags & SEEN)) {
if (data->flags & VERBOSE)
@@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
 
 /*
  * mark_reachable_objects() should have been run prior to this and all
- * reachable commits marked as "SEEN".
+ * reachable commits marked as "SEEN", except when quick_prune is non-zero,
+ * in which case lines are excised from the shallow file if they refer to
+ * commits that do not exist (any longer).
  */
-void prune_shallow(int show_only)
+void prune_shallow(int show_only, int quick_prune)
 {
struct lock_file shallow_lock = LOCK_INIT;
struct strbuf sb = STRBUF_INIT;
+   unsigned flags = SEEN_ONLY;
int fd;
 
+   if (quick_prune)
+   flags |= QUICK_PRUNE;
+
if (show_only) {
-   write_shallow_commits_1(, 0, NULL, SEEN_ONLY | VERBOSE);
+   flags |= VERBOSE;
+   write_shallow_commits_1(, 0, NULL, flags);
strbuf_release();
return;
}
@@ -388,7 +400,7 @@ void prune_shallow(int show_only)
   git_path_shallow(the_repository),
   LOCK_DIE_ON_ERROR);
check_shallow_file_for_update(the_repository);
-   if (write_shallow_commits_1(, 0, NULL, SEEN_ONLY)) {
+   if (write_shallow_commits_1(, 0, NULL, flags)) {
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
  get_lock_file_path(_lock));
-- 
gitgitgadget

[PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository

2018-10-22 Thread Johannes Schindelin via GitGitGadget

Under certain circumstances, commits that were reachable can be made
unreachable, e.g. via git fetch --prune. These commits might have been
packed already, in which case git repack -adlf will just drop them without
giving them the usual grace period before an eventual git prune (via git gc)
prunes them.

This is a problem when the shallow file still lists them, which is the
reason why git prune edits that file. And with the proposed changes, git
repack -ad will now do the same.

Reported by Alejandro Pauly.

Changes since v2:

* Fixed a typo in the last commit message.
* Added an explanation to the last commit message why we do not simply skip
non-existing shallow commits at fetch time.
* Introduced a new, "quick prune" mode where prune_shallow() does not try
to drop unreachable commits, but only non-existing ones.
* Rebased to current master because there were too many merge conflicts for
my liking otherwise.

Changes since v1:

* Also trigger prune_shallow() when --unpack-unreachable= was
passed to git repack.
* No need to trigger prune_shallow() when git repack was called with -k.

Johannes Schindelin (3):
repack: point out a bug handling stale shallow info
shallow: offer to prune only non-existing entries
repack -ad: prune the list of shallow commits

base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-9/dscho/shallow-and-fetch-prune-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/9

Range-diff vs v2:

1: d2be40131 ! 1: ed8559b91 repack: point out a bug handling stale shallow
info
@@ -48,4 +48,6 @@
+ origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
- test_done
+ . "$TEST_DIRECTORY"/lib-httpd.sh
+ start_httpd
+
-: - > 2: f085eb4f7 shallow: offer to prune only non-existing entries
2: c7ee6e008 ! 3: 1f9ff57d5 repack -ad: prune the list of shallow commits
@@ -2,15 +2,19 @@

repack -ad: prune the list of shallow commits

-While it is true that we never add unreachable commits into pack files
-intentionally (as `git repack`'s documentation states), we must not
-forget that a `git fetch --prune` (or even a `git fetch` when a ref
was
+`git repack` can drop unreachable commits without further warning,
+making the corresponding entries in `.git/shallow` invalid, which
causes
+serious problems when deepening the branches.
+
+One scenario where unreachable commits are dropped by `git repack` is
+when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
-packed in the first place).
+packed in the first place, which is an assumption at least some of
Git's
+code seems to make).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
@@ -23,8 +27,16 @@
To avoid this problem, let's prune the shallow list in `git repack`
when
the `-d` option is passed, unless `-A` is passed, too (which would
force
the now-unreachable objects to be turned into loose objects instead of
-being deleted). Additionally, e also need to take `--keep-reachable`
and
-`--unpack-unreachable=` into account.
+being deleted). Additionally, we also need to take `--keep-reachable`
+and `--unpack-unreachable=` into account.
+
+Note: an alternative solution discussed during the review of this
patch
+was to teach `git fetch` to simply ignore entries in .git/shallow if
the
+corresponding commits do not exist locally. A quick test, however,
+revealed that the .git/shallow file is written during a shallow
*clone*,
+in which case the commits do not exist, either, but the "shallow" line
+*does* need to be sent. Therefore, this approach would be a lot more
+finicky than the approach presented by the this patch.

Signed-off-by: Johannes Schindelin

@@ -32,15 +44,15 @@
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@
- if (!quiet && isatty(2))
+ if (!po_args.quiet && isatty(2))

Re: [PATCH] commit-reach: fix sorting commits by generation

2018-10-22 Thread René Scharfe

Am 22.10.2018 um 23:10 schrieb Thomas Gummerer:
> compare_commit_by_gen is used to sort a list of pointers to 'struct
> commit'.  The comparison function for qsort is called with pointers to
> the objects it needs to compare, so when sorting a list of 'struct
> commit *', the arguments are of type 'struct commit **'.  However,
> currently the comparison function casts it's arguments to 'struct
> commit *' and uses those, leading to out of bounds memory access and
> potentially to wrong results.  Fix that.
> 
> Signed-off-by: Thomas Gummerer 
> ---
> 
> I noticed this by running the test suite through valgrind.  I'm not
> familiar with this code, so I'm not sure why this didn't cause any
> issues or how they would manifest, but this seems like the right fix
> for this function either way.

Right; I sent a similar patch a while ago, but it seems to have fallen
through the cracks:

https://public-inbox.org/git/d1b58614-989f-5998-6c53-c19eee409...@web.de/

Anyway, your implied question was discussed back then.  Derrick wrote:

   The reason to sort is to hopefully minimize the amount we walk by 
   exploring the "lower" commits first. This is a performance-only thing, 
   not a correctness issue (which is why the bug exists). Even then, it is 
   just a heuristic.

Does b6723e4671 in pu (commit-reach: fix first-parent heuristic) change
that picture?  Did a quick test and found no performance difference with
and without the fix on top, i.e. proper sorting didn't seem to matter.

>  commit-reach.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/commit-reach.c b/commit-reach.c
> index bc522d6840..9efddfd7a0 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -516,8 +516,8 @@ int commit_contains(struct ref_filter *filter, struct 
> commit *commit,
>  
>  static int compare_commits_by_gen(const void *_a, const void *_b)
>  {
> - const struct commit *a = (const struct commit *)_a;
> - const struct commit *b = (const struct commit *)_b;
> + const struct commit *a = *(const struct commit **)_a;
> + const struct commit *b = *(const struct commit **)_b;
>  
>   if (a->generation < b->generation)
>   return -1;
> 

Looks good to me.

René

Bug with "git mv" and submodules, and with "git submodule add something --force"

On Sun, Oct 21, 2018 at 7:52 PM Junio C Hamano  wrote:
>
> Jonathan Nieder  writes:
>
> > Stefan Beller wrote:
> >
> >> Maybe for now we can do with just an update of the documentation/bugs
> >> section and say we cannot move files in and out of submodules?
> >
> > I think we have some existing logic to prevent "git add"-ing a file
> > within a submodule to the superproject, for example.
>
> There is die_path_inside_submodule() that sanity-checks the pathspec
> and rejects.  But I think that is done primarily to give an error
> message and not strictly necesary for correctness.

c08397e3aa (pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
flag, 2017-05-11) seems to be relevant here, as that factors out the
warning.

> The real safety of "git add" is its call to dir.c::fill_directory();
> it collects untracked paths that match the pathspec so that they can
> be added as new paths, but because it won't cross the module
> boundary, you won't get such a path in the index to begin with.

It would not cross the boundary and fail silently as it would
treat a path into the submodule as a no-op.

> > So "git mv" should learn the same trick.  And perhaps the trick needs
> > to be moved down a layer (e.g. into the index API).  Hints?

Yeah, I think we'd want to teach git-mv about that trick.

Unfortunately git-mv is one of the last remainders of not
properly using pathspecs, and die_path_inside_submodule
expects a pathspec. :-/

> You would want to be able to remove a submodule and replace it with
> a directory, but you can probably do it in two steps, i.e.
>
> git reset --hard
> git rm --cached sha1collisiondetection
> echo Now a regular dir >sha1collisiondetection/READ.ME
> find sha1collisiondetection ! -type d -print0 |
> git update-index --add --stdin -z

"Ignoring path sha1collisiondetection/.git"

Nice!

>
> So from that point of view, forbidding (starting from the same state
> of our project) this sequence:
>
> git reset --hard
> echo Now a regular dir >sha1collisiondetection/READ.ME
> find sha1collisiondetection ! -type d -print0 |
> git update-index --add --remove --stdin -z
>
> that would nuke the submodule and replace it with a directory within
> which there are files would be OK.  Making the latter's default
> rejection overridable with ADD_CACHE_OK_TO_REPLACE would also be
> fine.

I am having trouble of relating these commands to the original git-mv
across submodule boundaries.

Moving files from the submodule out to the superproject, seems
to fail properly, though having a less-than-optimal error message:

$ git mv sha1collisiondetection/LICENSE.txt .
fatal: not under version control, source=sha1collisiondetection/LICENSE.txt, 
destination=LICENSE.txt

and moving things inside was the original report, below is a proof of concept
that would yield

./git mv -v cache.h sha1collisiondetection/
fatal: moving across submodule boundaries not supported, source=cache.h, 
destination=sha1collisiondetection/cache.h

--8<--
Subject: [WIP/PATCH] builtin/mv.c: disallow moving across submodule boundaries

Signed-off-by: Stefan Beller 
---
 builtin/mv.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 80bb967a63..9ec4b2f0a3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
/* Checking */
for (i = 0; i < argc; i++) {
const char *src = source[i], *dst = destination[i];
-   int length, src_is_dir;
+   int length, src_is_dir, pos;
const char *bad = NULL;
 
if (show_only)
@@ -243,6 +243,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
else
string_list_insert(_for_dst, dst);
 
+   pos = cache_name_pos(dst, strlen(dst));
+   if (pos < 0) {
+   pos = -(pos + 1);
+   if (!S_ISGITLINK(active_cache[pos]->ce_mode))
+   bad = _("moving across submodule boundaries not 
supported");
+   }
+
if (!bad)
continue;
if (!ignore_errors)
-- 
2.19.0

[PATCH v2] archive: initialize archivers earlier

2018-10-22 Thread steadmon

Initialize archivers as soon as possible when running git-archive and
git-upload-archive. Various non-obvious behavior depends on having the
archivers initialized, such as determining the desired archival format
from the provided filename.

Since 08716b3c11 ("archive: refactor file extension format-guessing",
2011-06-21), archive_format_from_filename() has used the registered
archivers to match filenames (provided via --output) to archival
formats. However, when git-archive is executed with --remote, format
detection happens before the archivers have been registered. This causes
archives from remotes to always be generated as TAR files, regardless of
the actual filename (unless an explicit --format is provided).

This patch fixes that behavior; archival format is determined properly
from the output filename, even when --remote is used.

Signed-off-by: Josh Steadmon 
Helped-by: Jeff King 
---
 archive.c| 9 ++---
 archive.h| 1 +
 builtin/archive.c| 2 ++
 builtin/upload-archive.c | 1 +
 t/t5000-tar-tree.sh  | 6 ++
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/archive.c b/archive.c
index c1870105eb..ce0f8a0362 100644
--- a/archive.c
+++ b/archive.c
@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)
archivers[nr_archivers++] = ar;
 }
 
+void init_archivers(void)
+{
+   init_tar_archiver();
+   init_zip_archiver();
+}
+
 static void format_subst(const struct commit *commit,
  const char *src, size_t len,
  struct strbuf *buf)
@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
git_config_get_bool("uploadarchive.allowunreachable", 
_allow_unreachable);
git_config(git_default_config, NULL);
 
-   init_tar_archiver();
-   init_zip_archiver();
-
args.repo = repo;
argc = parse_archive_args(argc, argv, , , name_hint, remote);
if (!startup_info->have_repository) {
diff --git a/archive.h b/archive.h
index d4f97a00f5..21ac010699 100644
--- a/archive.h
+++ b/archive.h
@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);
 
 extern void init_tar_archiver(void);
 extern void init_zip_archiver(void);
+extern void init_archivers(void);
 
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
const struct object_id *oid,
diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..d2455237ce 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, local_opts, NULL,
 PARSE_OPT_KEEP_ALL);
 
+   init_archivers();
+
if (output)
create_output_file(output);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 25d9116356..3f35ebcfe8 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
}
 
/* parse all options sent by the client */
+   init_archivers();
return write_archive(sent_argv.argc, sent_argv.argv, prefix,
 the_repository, NULL, 1);
 }
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 2a97b27b0a..3e95fdf660 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override 
inferred format' '
test_cmp_bin b.tar d4.zip
 '
 
+test_expect_success GZIP 'git archive with --output and --remote uses expected 
format' '
+   git archive --output=d5.tgz --remote=. HEAD &&
+   gzip -d -c < d5.tgz > d5.tar &&
+   test_cmp_bin b.tar d5.tar
+'
+
 test_expect_success 'git archive --list outside of a git repo' '
nongit git archive --list
 '
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 1/1] archive: init archivers before determining format

On 2018.10.19 19:59, Jeff King wrote:
> On Fri, Oct 19, 2018 at 04:19:28PM -0700, stead...@google.com wrote:
> 
> > diff --git a/builtin/archive.c b/builtin/archive.c
> > index e74f675390..dd3283a247 100644
> > --- a/builtin/archive.c
> > +++ b/builtin/archive.c
> > @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char 
> > **argv,
> >  * it.
> >  */
> > if (name_hint) {
> > -   const char *format = archive_format_from_filename(name_hint);
> > +   const char *format;
> > +   init_tar_archiver();
> > +   init_zip_archiver();
> > +   format = archive_format_from_filename(name_hint);
> > if (format)
> > packet_write_fmt(fd[1], "argument --format=%s\n", 
> > format);
> 
> Hrm. This code was added back in 56baa61d01 (archive: move file
> extension format-guessing lower, 2011-06-21), and your example
> invocation worked back then!
> 
> Unfortunately it was broken by the very next patch in the series,
> 08716b3c11 (archive: refactor file extension format-guessing,
> 2011-06-21). I guess that's what I get for not adding regression tests.
> 
> It's probably worth mentioning those points in the commit message.

Done.

> Does this work with configured archiver extensions, too? I think so,
> because we load them via init_tar_archiver().

If you mean things like .tgz and .tar.gz, then yes, they are affected by
the bug as well, and this patch fixes them. The test included in v2 uses
a .tgz file.

> Can we avoid repeating the list of archivers here? This needs to stay in
> sync with the list in write_archive(). I know there are only two, but
> can we factor out an init_archivers() call or something?

Done.

> We also should probably just call it unconditionally when we start the
> archiver command (I don't think there are any other bugs like this
> lurking, but it doesn't cost very much to initialize these; it makes
> sense to just do it early).

Done.

> Other than those minor points (and the lack of test), your fix looks
> good to me.

Thanks for the review!

Re: [PATCH 0/1] Fix format detection when archiving remotely

On 2018.10.19 19:41, Jeff King wrote:
> On Fri, Oct 19, 2018 at 04:19:27PM -0700, stead...@google.com wrote:
> 
> > Currently, git-archive does not properly determine the desired archive
> > format when both --output and --remote are provided, because
> > run_remote_archiver() does not initialize the archivers prior to calling
> > archive_format_from_filename(). This results in the remote archiver
> > always returning a TAR file, regardless of the requested format.
> > 
> > This patch initializes the TAR and ZIP archivers before calling
> > archive_format_from_filename(), which fixes format detection.
> 
> It seems like some of this content could be in the commit message of the
> actual patch.

Ack. I'll be sending v2 shortly, please let me know if I've missed
anything that should be included.

> > Steps to reproduce:
> > 
> > ∫ git version
> > git version 2.19.1.568.g152ad8e336-goog
> > ∫ cd ~/src/git
> > ∫ git archive --output ~/good.zip HEAD
> > ∫ file ~/good.zip
> > /home/steadmon/good.zip: Zip archive data, at least v1.0 to extract
> > ∫ git archive --output ~/bad.zip --remote=. HEAD
> > ∫ file ~/bad.zip
> > /home/steadmon/bad.zip: POSIX tar archive
> 
> And this could be in a test script in the actual patch. :)

Done.

Re: [PATCH] Poison gettext with the Ook language



On Mon, Oct 22 2018, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Oct 22 2018, Nguyễn Thái Ngọc Duy wrote:
>
>> The current gettext() function just replaces all strings with
>> '# GETTEXT POISON #' including format strings and hides the things
>> that we should be allowed to grep (like branch names, or some other
>> codes) even when gettext is poisoned.
>>
>> This patch implements the poisoned _() with a universal and totally
>> legit language called Ook [1]. We could actually grep stuff even in
>> with this because format strings are preserved.
>>
>> Long term, we could implement an "ook translator" for test_i18ngrep
>> and friends so that they translate English to Ook, allowing us to
>> match full text while making sure the text in the code is still marked
>> for translation.
>
> Replying to both this patch, and SZEDER's series for brevity. Thanks
> both for working on this. It's obvious that this stuff needs some
> polishing, and it's great that's being done, and sorry for my part of
> having left this in the current state.
>
> a)
>
> Both this patch and SZEDER's 7/8 are addressing the issue that the
> poison mode doesn't preserve format specifiers.
>
> I haven't tried to dig this up in the list archive, and maybe it only
> exists in my mind, but I seem to remember that this was explicitly
> discussed as a goal at the time.
>
> I.e. we were expecting the lack of this to break tests, and that was
> considered a feature as it would spot plumbing messages we shouldn't be
> translating.
>
> However, it's my opinion that this was just a bad idea to begin with,
> I've CC'd a couple of prolific markers of i18n from my log grepping (and
> feel free to add more) to see if they disagre.
>
> I think it probably helped me a bit in the beginning when i18n was
> bootstrapping and there was a *lot* to mark for translation, but we've
> long since stabilized and aren't doing that anymore, so it's much easier
> to just review the patches to see if they translate plumbing.
>
> All of which is to say that I think something like your patch here is
> fine to just accept, and the only improvement I'd suggest is some note
> in the commit message saying that this was always intentional, but
> nobody can name a case where it helped, so let's just drop it.
>
> In SZEDER's case that we shouldn't have GIT_GETTEXT_POISON=scrambled,
> let's just make it boolean and make "scrambled" (i.e. preserving format
> specifiecs) the default.

[...]

> b)
>
> SZEDER's series, and your patch (although it's smaller) still preserve
> the notion that there's such a thing as a GETTEXT_POISON *build* and you
> actually need to compile git with that flag to make use of it.
>
> This, as I recall, was just paranoia on my part back in 2010/2011. I
> wanted to be able to present a version of this i18n stuff where people
> who didn't like it could be assured that if they didn't opt-in to it
> they wouldn't get any of the code (sans a few trivial and obviously
> correct wrapper functions).
>
> So I think the only reason to keep it compile-time is performance, but I
> don't think that matters. It's not like we're printing gigabytes of _()
> formatted output. Everything where formatting matters is plumbing which
> doesn't use this API. These messages are always for human consumption.
>
> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time
> entirely, and make GIT_GETTEXT_POISON= a test flag that all
> builds of git are aware of? I think so.

Something like this, untested except I compiled it and ran one test
with/without GIT_GETTEXT_POISON, so it may have bugs:

 Makefile| 10 +-
 gettext.c   |  4 +---
 gettext.h   |  4 
 po/README   | 13 -
 t/README|  5 +
 t/helper/test-tool.c|  1 +
 t/helper/test-tool.h|  1 +
 t/test-lib-functions.sh |  8 
 t/test-lib.sh   | 12 +++-
 9 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/Makefile b/Makefile
index d18ab0fe78..78190ee9d9 100644
--- a/Makefile
+++ b/Makefile
@@ -362,11 +362,6 @@ all::
 # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
 # user.
 #
-# Define GETTEXT_POISON if you are debugging the choice of strings marked
-# for translation.  In a GETTEXT_POISON build, you can turn all strings marked
-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable
-# (to any value) in your environment.
-#
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have gitweb.js minified.
 #
@@ -714,6 +709,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-gettext-poison.o
 TEST_BUILTINS_OBJS += test-hashmap.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
@@ -1439,9 +1435,6 @@ endif
 ifdef

Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)

2018-10-22 Thread Johannes Schindelin

Hi Junio,

On Sun, 21 Oct 2018, Junio C Hamano wrote:

> Alban Gruin  writes:
> 
> > The error comes from the call to `git stash apply $stash_id' in
> > builtin/rebase.c:261.  When $stash_id only contains decimals and no
> > letters, git-stash tries to apply stash@{$stash_id}[0][1].  Thas was not
> > a real problem with the shell script, because it did not abbreviate the
> > object id of the stashed commit, so it was very unlikely that the oid
> > would contain only digits.  builtin/rebase.c shortens the oid[2], making
> > this problem more likely to occur.
> 
> OK, so make "rebase in C" a faithful conversion of the original, the
> code that lead to builtin/rebase.c:261 must be fixed not to pass a
> shortened oid.  I suspect that internally it has a full object name,
> so not shortening would be the right thing anyway, so regaredless of
> this bug, it probably makes sense to do the fix.
> 
> But as you said, even the scripted version that passed the full
> object name had a (10/16^40) chance of using a 40-hex that only has
> [0-9] and caused the same breakage, so such a change to "rebase in
> C" is sweeping the age old bug under the same rug, in the context of
> discussing this particular bug.  
> 
> I agree with you that it is a better fix to the problem to allow the
> caller to say "I am giving an oid---it may (or may not---I do not
> even bother to check) consist of only digits, but do not treat it as
> an index to the stash reflog."

We already have a way to say "I am giving you an oid": append `^0` to the
hash.

I implemented a fix, pushed it up to GitGitGadget, and will submit it
tomorrow (after a fresh look, and after the build finished):
https://github.com/gitgitgadget/git/pull/52

Ciao,
Dscho

Re: [PATCH] alias: detect loops in mixed execution mode

On Mon, Oct 22 2018, Jeff King wrote:

> On Sat, Oct 20, 2018 at 09:18:21PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> We didn't support chained aliases at all before, so I think the odds
>> that people will run into this now will increase as they add "!" to
>> existing aliases, and I'd like to have git's UI friendly enough to tell
>> users what went wrong by default, and not have to resort to the likes of
>> GIT_TRACE=1 which really should be left to powerusers.
>
> It's true that non-! aliases couldn't recurse before, but couldn't "!"
> ones always do so?

Yes. I meant that maybe now it's a feature that works for that people
will start using it, and then convert some of that to !-aliases they
wouldn't otherwise have written. Just idle speculation...

Re: [PATCH] alias: detect loops in mixed execution mode

On Sat, Oct 20, 2018 at 09:18:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

> We didn't support chained aliases at all before, so I think the odds
> that people will run into this now will increase as they add "!" to
> existing aliases, and I'd like to have git's UI friendly enough to tell
> users what went wrong by default, and not have to resort to the likes of
> GIT_TRACE=1 which really should be left to powerusers.

It's true that non-! aliases couldn't recurse before, but couldn't "!"
ones always do so?

-Peff

Re: [PATCH 1/6] doc: clarify boundaries of 'git worktree list --porcelain'

On Mon, Oct 22, 2018 at 4:46 PM Andreas Heiduk  wrote:
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -270,8 +270,8 @@ Porcelain Format
>  The porcelain format has a line per attribute.  Attributes are listed with a
>  label and value separated by a single space.  Boolean attributes (like 'bare'
>  and 'detached') are listed as a label only, and are only present if and only
> -if the value is true.  An empty line indicates the end of a worktree.  For
> -example:
> +if the value is true.  The first attribute of a worktree is always 
> `worktree`,
> +an empty line indicates the end of the record.  For example:

When I suggested the --porcelain option for "git worktree list" and
provided an example of its proposed output, the idea all along was
that the "worktree" line itself would indicate start-of-stanza. A
blank line between records is superfluous and unnecessary.
Unfortunately, by the time the implementation was posted with blank
line as stanza separator, I was not around to contest it, and it ended
up in a release, after which point it was too late to change it.

So, the tl;dr is that this documentation update agrees with the
original intention as I envisioned it (although I wouldn't be sad to
see the mention of "blank line" dropped altogether, but that's perhaps
a separate battle).

[PATCH] commit-reach: fix sorting commits by generation

2018-10-22 Thread Thomas Gummerer

compare_commit_by_gen is used to sort a list of pointers to 'struct
commit'.  The comparison function for qsort is called with pointers to
the objects it needs to compare, so when sorting a list of 'struct
commit *', the arguments are of type 'struct commit **'.  However,
currently the comparison function casts it's arguments to 'struct
commit *' and uses those, leading to out of bounds memory access and
potentially to wrong results.  Fix that.

Signed-off-by: Thomas Gummerer 
---

I noticed this by running the test suite through valgrind.  I'm not
familiar with this code, so I'm not sure why this didn't cause any
issues or how they would manifest, but this seems like the right fix
for this function either way.

 commit-reach.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index bc522d6840..9efddfd7a0 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -516,8 +516,8 @@ int commit_contains(struct ref_filter *filter, struct 
commit *commit,
 
 static int compare_commits_by_gen(const void *_a, const void *_b)
 {
-   const struct commit *a = (const struct commit *)_a;
-   const struct commit *b = (const struct commit *)_b;
+   const struct commit *a = *(const struct commit **)_a;
+   const struct commit *b = *(const struct commit **)_b;
 
if (a->generation < b->generation)
return -1;
-- 
2.19.1.759.g500967bb5e

Re: [PATCH] Poison gettext with the Ook language

On Mon, Oct 22 2018, Nguyễn Thái Ngọc Duy wrote:

> The current gettext() function just replaces all strings with
> '# GETTEXT POISON #' including format strings and hides the things
> that we should be allowed to grep (like branch names, or some other
> codes) even when gettext is poisoned.
>
> This patch implements the poisoned _() with a universal and totally
> legit language called Ook [1]. We could actually grep stuff even in
> with this because format strings are preserved.
>
> Long term, we could implement an "ook translator" for test_i18ngrep
> and friends so that they translate English to Ook, allowing us to
> match full text while making sure the text in the code is still marked
> for translation.

Replying to both this patch, and SZEDER's series for brevity. Thanks
both for working on this. It's obvious that this stuff needs some
polishing, and it's great that's being done, and sorry for my part of
having left this in the current state.

a)

Both this patch and SZEDER's 7/8 are addressing the issue that the
poison mode doesn't preserve format specifiers.

I haven't tried to dig this up in the list archive, and maybe it only
exists in my mind, but I seem to remember that this was explicitly
discussed as a goal at the time.

I.e. we were expecting the lack of this to break tests, and that was
considered a feature as it would spot plumbing messages we shouldn't be
translating.

However, it's my opinion that this was just a bad idea to begin with,
I've CC'd a couple of prolific markers of i18n from my log grepping (and
feel free to add more) to see if they disagre.

I think it probably helped me a bit in the beginning when i18n was
bootstrapping and there was a *lot* to mark for translation, but we've
long since stabilized and aren't doing that anymore, so it's much easier
to just review the patches to see if they translate plumbing.

All of which is to say that I think something like your patch here is
fine to just accept, and the only improvement I'd suggest is some note
in the commit message saying that this was always intentional, but
nobody can name a case where it helped, so let's just drop it.

In SZEDER's case that we shouldn't have GIT_GETTEXT_POISON=scrambled,
let's just make it boolean and make "scrambled" (i.e. preserving format
specifiecs) the default.

b)

SZEDER's series, and your patch (although it's smaller) still preserve
the notion that there's such a thing as a GETTEXT_POISON *build* and you
actually need to compile git with that flag to make use of it.

This, as I recall, was just paranoia on my part back in 2010/2011. I
wanted to be able to present a version of this i18n stuff where people
who didn't like it could be assured that if they didn't opt-in to it
they wouldn't get any of the code (sans a few trivial and obviously
correct wrapper functions).

So I think the only reason to keep it compile-time is performance, but I
don't think that matters. It's not like we're printing gigabytes of _()
formatted output. Everything where formatting matters is plumbing which
doesn't use this API. These messages are always for human consumption.

So shouldn't we just drop this notion of GETTEXT_POISON at compile-time
entirely, and make GIT_GETTEXT_POISON= a test flag that all
builds of git are aware of? I think so.

Re: [PATCH] Poison gettext with the Ook language

2018-10-22 Thread Johannes Schindelin

Hi Duy,

On Mon, 22 Oct 2018, Nguyễn Thái Ngọc Duy wrote:

> The current gettext() function just replaces all strings with
> '# GETTEXT POISON #' including format strings and hides the things
> that we should be allowed to grep (like branch names, or some other
> codes) even when gettext is poisoned.
> 
> This patch implements the poisoned _() with a universal and totally
> legit language called Ook [1]. We could actually grep stuff even in
> with this because format strings are preserved.
> 
> Long term, we could implement an "ook translator" for test_i18ngrep
> and friends so that they translate English to Ook, allowing us to
> match full text while making sure the text in the code is still marked
> for translation.
> 
> [1] https://en.wikipedia.org/wiki/Unseen_University#Librarian
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  This started out as something fun to do while running the test suite
>  last weekend. But it turns out actually working! If this patch ends
>  up in git.git, the Librarian would be so proud!

Cute.
Dscho

> 
>  gettext.c   | 54 +
>  gettext.h   |  7 ---
>  t/lib-rebase.sh |  2 +-
>  3 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/gettext.c b/gettext.c
> index 7272771c8e..29901e1ddd 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -56,6 +56,60 @@ int use_gettext_poison(void)
>  }
>  #endif
>  
> +const char *gettext_poison(const char *msgid)
> +{
> + /*
> +  * gettext() returns a string that is always valid. We would
> +  * need a hash map for that but let's stay simple and keep the
> +  * last 64 gettext() results. Should be more than enough.
> +  */
> + static char *bufs[64];
> + static int i;
> + struct strbuf sb = STRBUF_INIT;
> + char *buf;
> + const char *p;
> + const char *type_specifiers = "diouxXeEfFgGaAcsCSpnm%";
> +
> + if (!strchr(msgid, '%'))
> + return "Eek!";
> +
> + p = msgid;
> + while (*p) {
> + const char *type;
> + switch (*p) {
> + case '%':
> + /*
> +  * No strict parsing. We simply look for the end of a
> +  * format string
> +  */
> + type = p + 1;
> + while (*type && !strchr(type_specifiers, *type))
> + type++;
> + if (*type)
> + type++;
> + strbuf_add(, p, (int)(type - p));
> + p = type;
> + break;
> + default:
> + if (!isalpha(*p)) {
> + strbuf_addch(, *p);
> + p++;
> + break;
> + }
> + if (isupper(*p))
> + strbuf_addstr(, "Ook");
> + else
> + strbuf_addstr(, "ook");
> + while (isalpha(*p))
> + p++;
> + }
> + }
> + buf = bufs[(i++) % ARRAY_SIZE(bufs)];
> + free(buf);
> + buf = strbuf_detach(, NULL);
> + return buf;
> +}
> +
>  #ifndef NO_GETTEXT
>  static int test_vsnprintf(const char *fmt, ...)
>  {
> diff --git a/gettext.h b/gettext.h
> index 7eee64a34f..dc9851a06a 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -41,8 +41,9 @@ static inline int gettext_width(const char *s)
>  }
>  #endif
>  
> +const char *gettext_poison(const char *);
>  #ifdef GETTEXT_POISON
> -extern int use_gettext_poison(void);
> +int use_gettext_poison(void);
>  #else
>  #define use_gettext_poison() 0
>  #endif
> @@ -51,14 +52,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const 
> char *msgid)
>  {
>   if (!*msgid)
>   return "";
> - return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
> + return use_gettext_poison() ? gettext_poison(msgid) : gettext(msgid);
>  }
>  
>  static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
>  const char *Q_(const char *msgid, const char *plu, unsigned long n)
>  {
>   if (use_gettext_poison())
> - return "# GETTEXT POISON #";
> + return gettext_poison(msgid);
>   return ngettext(msgid, plu, n);
>  }
>  
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 2ca9fb69d6..1e8440e935 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -29,7 +29,7 @@ set_fake_editor () {
>   */COMMIT_EDITMSG)
>   test -z "$EXPECT_HEADER_COUNT" ||
>   test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is 
> a combination of \(.*\) commits\./\1/p' < "$1")" ||
> - test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
> + test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# Ook ook 
> ook ook ook \(.*\) ook\./\1/p' < "$1")" ||
>   exit
>

[PATCH 6/6] doc: fix formatting in git-update-ref

Remove the parapgraph numbers from lines explaining the reflog format
and typeset these lines in monospace.

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-update-ref.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index fda8516677..9671423117 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -129,8 +129,8 @@ a line to the log file "$GIT_DIR/logs/" (dereferencing 
all
 symbolic refs before creating the log name) describing the change
 in ref value.  Log lines are formatted as:
 
-. oldsha1 SP newsha1 SP committer LF
-+
+oldsha1 SP newsha1 SP committer LF
+
 Where "oldsha1" is the 40 character hexadecimal value previously
 stored in , "newsha1" is the 40 character hexadecimal value of
  and "committer" is the committer's name, email address
@@ -138,8 +138,8 @@ and date in the standard Git committer ident format.
 
 Optionally with -m:
 
-. oldsha1 SP newsha1 SP committer TAB message LF
-+
+oldsha1 SP newsha1 SP committer TAB message LF
+
 Where all fields are as described above and "message" is the
 value supplied to the -m option.
 
-- 
2.19.1

[PATCH 5/6] doc: fix indentation of listing blocks in gitweb.conf.txt

'gitweb.conf.txt' uses inconsistent indentation in listing blocks and a mix
of listing blocks and literal paragraphs. Both didn't look pretty in the
rendered HTML page.

Signed-off-by: Andreas Heiduk 
---
 Documentation/gitweb.conf.txt | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index 9c8982ec98..c0a326e388 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -19,10 +19,12 @@ end of a line is ignored.  See *perlsyn*(1) for details.
 
 An example:
 
-# gitweb configuration file for http://git.example.org
-#
-our $projectroot = "/srv/git"; # FHS recommendation
-our $site_name = 'Example.org >> Repos';
+
+# gitweb configuration file for http://git.example.org
+#
+our $projectroot = "/srv/git"; # FHS recommendation
+our $site_name = 'Example.org >> Repos';
+
 
 
 The configuration file is used to override the default settings that
@@ -357,6 +359,7 @@ $home_link_str::
 +
 For example, the following setting produces a breadcrumb trail like
 "home / dev / projects / ..." where "projects" is the home link.
++
 
 our @extra_breadcrumbs = (
   [ 'home' => 'https://www.example.org/' ],
@@ -901,14 +904,16 @@ To enable blame, pickaxe search, and snapshot support 
(allowing "tar.gz" and
 "zip" snapshots), while allowing individual projects to turn them off, put
 the following in your GITWEB_CONFIG file:
 
-   $feature{'blame'}{'default'} = [1];
-   $feature{'blame'}{'override'} = 1;
+
+$feature{'blame'}{'default'} = [1];
+$feature{'blame'}{'override'} = 1;
 
-   $feature{'pickaxe'}{'default'} = [1];
-   $feature{'pickaxe'}{'override'} = 1;
+$feature{'pickaxe'}{'default'} = [1];
+$feature{'pickaxe'}{'override'} = 1;
 
-   $feature{'snapshot'}{'default'} = ['zip', 'tgz'];
-   $feature{'snapshot'}{'override'} = 1;
+$feature{'snapshot'}{'default'} = ['zip', 'tgz'];
+$feature{'snapshot'}{'override'} = 1;
+
 
 If you allow overriding for the snapshot feature, you can specify which
 snapshot formats are globally disabled. You can also add any command-line
-- 
2.19.1

[PATCH 3/6] doc: fix inappropriate monospace formatting

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-upload-pack.txt |  1 +
 Documentation/git.txt | 10 +-
 Documentation/gitattributes.txt   | 30 +++---
 Documentation/gitmodules.txt  | 17 ++---
 Documentation/gitsubmodules.txt   | 14 +-
 5 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-upload-pack.txt 
b/Documentation/git-upload-pack.txt
index 822ad593af..998f52d3df 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git-upload-pack' [--[no-]strict] [--timeout=] [--stateless-rpc]
  [--advertise-refs] 
+
 DESCRIPTION
 ---
 Invoked by 'git fetch-pack', learns what
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2ac9b1c7fe..00156d64aa 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -402,11 +402,11 @@ Git so take care if using a foreign front-end.
of Git object directories which can be used to search for Git
objects. New objects will not be written to these directories.
 +
-   Entries that begin with `"` (double-quote) will be interpreted
-   as C-style quoted paths, removing leading and trailing
-   double-quotes and respecting backslash escapes. E.g., the value
-   `"path-with-\"-and-:-in-it":vanilla-path` has two paths:
-   `path-with-"-and-:-in-it` and `vanilla-path`.
+Entries that begin with `"` (double-quote) will be interpreted
+as C-style quoted paths, removing leading and trailing
+double-quotes and respecting backslash escapes. E.g., the value
+`"path-with-\"-and-:-in-it":vanilla-path` has two paths:
+`path-with-"-and-:-in-it` and `vanilla-path`.
 
 `GIT_DIR`::
If the `GIT_DIR` environment variable is set then it
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 92010b062e..b8392fc330 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -303,21 +303,21 @@ number of pitfalls:
   attribute. If you decide to use the `working-tree-encoding` attribute
   in your repository, then it is strongly recommended to ensure that all
   clients working with the repository support it.
-
-  For example, Microsoft Visual Studio resources files (`*.rc`) or
-  PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16.
-  If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with
-  a `working-tree-encoding` enabled Git client, then `foo.ps1` will be
-  stored as UTF-8 internally. A client without `working-tree-encoding`
-  support will checkout `foo.ps1` as UTF-8 encoded file. This will
-  typically cause trouble for the users of this file.
-
-  If a Git client, that does not support the `working-tree-encoding`
-  attribute, adds a new file `bar.ps1`, then `bar.ps1` will be
-  stored "as-is" internally (in this example probably as UTF-16).
-  A client with `working-tree-encoding` support will interpret the
-  internal contents as UTF-8 and try to convert it to UTF-16 on checkout.
-  That operation will fail and cause an error.
++
+For example, Microsoft Visual Studio resources files (`*.rc`) or
+PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16.
+If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with
+a `working-tree-encoding` enabled Git client, then `foo.ps1` will be
+stored as UTF-8 internally. A client without `working-tree-encoding`
+support will checkout `foo.ps1` as UTF-8 encoded file. This will
+typically cause trouble for the users of this file.
++
+If a Git client, that does not support the `working-tree-encoding`
+attribute, adds a new file `bar.ps1`, then `bar.ps1` will be
+stored "as-is" internally (in this example probably as UTF-16).
+A client with `working-tree-encoding` support will interpret the
+internal contents as UTF-8 and try to convert it to UTF-16 on checkout.
+That operation will fail and cause an error.
 
 - Reencoding content to non-UTF encodings can cause errors as the
   conversion might not be UTF-8 round trip safe. If you suspect your
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index 4d63def206..312b6f9259 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -67,7 +67,8 @@ submodule..fetchRecurseSubmodules::
 submodule..ignore::
Defines under what circumstances "git status" and the diff family show
a submodule as modified. The following values are supported:
-
++
+--
all;; The submodule will never be considered modified (but will
nonetheless show up in the output of status and commit when it has
been staged).
@@ -84,12 +85,14 @@ submodule..ignore::
differences, and modifications to tracked and untracked files are
shown. This is the default option.
 
-   If this option is also present in the submodules entry in .git/config
-   of the superproject, the setting

[PATCH 4/6] doc: fix descripion for 'git tag --format'

The '--format=' is now listed in the 'OPTIONS' section, not only
the '' string itself. The description moved up a few paragraphs
because '' is not a standalone paramater but a parameter for the
option '--format'.

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-tag.txt | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 92f9c12b87..f2d644e3af 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -187,6 +187,12 @@ This option is only applicable when listing tags without 
annotation lines.
`--create-reflog`, but currently does not negate the setting of
`core.logAllRefUpdates`.
 
+--format=::
+   A string that interpolates `%(fieldname)` from a tag ref being shown
+   and the object it points at.  The format is the same as
+   that of linkgit:git-for-each-ref[1].  When unspecified,
+   defaults to `%(refname:strip=2)`.
+
 ::
The name of the tag to create, delete, or describe.
The new tag name must pass all checks defined by
@@ -198,12 +204,6 @@ This option is only applicable when listing tags without 
annotation lines.
The object that the new tag will refer to, usually a commit.
Defaults to HEAD.
 
-::
-   A string that interpolates `%(fieldname)` from a tag ref being shown
-   and the object it points at.  The format is the same as
-   that of linkgit:git-for-each-ref[1].  When unspecified,
-   defaults to `%(refname:strip=2)`.
-
 CONFIGURATION
 -
 By default, 'git tag' in sign-with-default mode (-s) will use your
-- 
2.19.1

[PATCH 1/6] doc: clarify boundaries of 'git worktree list --porcelain'

Defined delimiters for 'git worktree list --porcelain' make the format
easier to parse in scripts. For example

sed -n '/^worktree ID$/,/^$/p'

extracts only the information for the worktree 'ID'.

The format did not changed since [1], only the guaranty is added.

[1] bb9c03b82a (worktree: add 'list' command, 2015-10-08)

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-worktree.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e2ee9fc21b..73520434f6 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -270,8 +270,8 @@ Porcelain Format
 The porcelain format has a line per attribute.  Attributes are listed with a
 label and value separated by a single space.  Boolean attributes (like 'bare'
 and 'detached') are listed as a label only, and are only present if and only
-if the value is true.  An empty line indicates the end of a worktree.  For
-example:
+if the value is true.  The first attribute of a worktree is always `worktree`,
+an empty line indicates the end of the record.  For example:
 
 
 $ git worktree list --porcelain
-- 
2.19.1

[PATCH 0/6] various fixes for docs

A small batch of fixes for the docs. All but the very first fixes
formatting and similar stuff. The first one makes parsing 
'git worktree list' more future-proof.

Andreas Heiduk (6):
  doc: clarify boundaries of 'git worktree list --porcelain'
  doc: fix ASCII art tab spacing
  doc: fix inappropriate monospace formatting
  doc: fix descripion for 'git tag --format'
  doc: fix indentation of listing blocks in gitweb.conf.txt
  doc: fix formatting in git-update-ref

 Documentation/git-bisect-lk2009.txt | 30 ++---
 Documentation/git-checkout.txt  | 14 +++---
 Documentation/git-merge-base.txt|  6 +++---
 Documentation/git-tag.txt   | 12 ++--
 Documentation/git-update-ref.txt|  8 
 Documentation/git-upload-pack.txt   |  1 +
 Documentation/git-worktree.txt  |  4 ++--
 Documentation/git.txt   | 10 +-
 Documentation/gitattributes.txt | 30 ++---
 Documentation/gitmodules.txt| 17 +---
 Documentation/gitsubmodules.txt | 14 +-
 Documentation/gitweb.conf.txt   | 25 ++--
 12 files changed, 92 insertions(+), 79 deletions(-)

-- 
2.19.1

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

2018-10-22 Thread Johannes Schindelin

Hi Ben,

On Mon, 22 Oct 2018, Ben Peart wrote:

> From: Ben Peart 
> 
> When git reset is run with the --quiet flag, don't bother finding any
> additional unstaged changes as they won't be output anyway.  This speeds up
> the git reset command by avoiding having to lstat() every file looking for
> changes that aren't going to be reported anyway.
> 
> The savings can be significant.  In a repo with 200K files "git reset"
> drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

That's very nice!

Those numbers, just out of curiosity, are they on Windows? Or on Linux?

Ciao,
Dscho

Re: [PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty



On Mon, Oct 22 2018, SZEDER Gábor wrote:

> This allows us to run test with non-GETTEXT POISON-ed behavior even in
> a GETTEXT POISON build by running:
>
>   GIT_GETTEXT_POISON= ./t1234-foo.sh
>
> Signed-off-by: SZEDER Gábor 
> ---
>  Makefile  | 2 +-
>  gettext.c | 9 +++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ad880d1fc5..7a165445cd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -365,7 +365,7 @@ all::
>  # Define GETTEXT_POISON if you are debugging the choice of strings marked
>  # for translation.  In a GETTEXT_POISON build, you can turn all strings 
> marked
>  # for translation into gibberish by setting the GIT_GETTEXT_POISON variable
> -# (to any value) in your environment.
> +# to a non-empty value in your environment.
>  #
>  # Define JSMIN to point to JavaScript minifier that functions as
>  # a filter to have gitweb.js minified.
> diff --git a/gettext.c b/gettext.c
> index 7272771c8e..a9509a5df3 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -50,8 +50,13 @@ const char *get_preferred_languages(void)
>  int use_gettext_poison(void)
>  {
>   static int poison_requested = -1;
> - if (poison_requested == -1)
> - poison_requested = getenv("GIT_GETTEXT_POISON") ? 1 : 0;
> + if (poison_requested == -1) {
> + const char *v = getenv("GIT_GETTEXT_POISON");
> + if (v && *v)
> + poison_requested = 1;
> + else
> + poison_requested = 0;
> + }
>   return poison_requested;
>  }
>  #endif

Fixing this is good. But when the initial support for conditional runs
was added in 309552295a ("i18n: do not poison translations unless
GIT_GETTEXT_POISON envvar is set", 2011-02-22) we didn't have the
convention of using git_env_bool() for these, which a few recent
follow-up patches have also done.

So let's just use git_env_bool() here. It's less code, and consistent
with the rest. See 4c2db93807 ("read-cache.c: make $GIT_TEST_SPLIT_INDEX
boolean", 2018-04-14). Also makes sense to document this in the "Running
tests with special setups" setup that patch added.

[PATCH 6/8] gettext: use an enum for the mode of GETTEXT POISONing

The next patch will add a different mode of GETTEXT POISON-ing,
therefore named constants will be better than magic numbers.

Signed-off-by: SZEDER Gábor 
---
 gettext.c | 12 ++--
 gettext.h | 12 +---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/gettext.c b/gettext.c
index a9509a5df3..c50d1e0377 100644
--- a/gettext.c
+++ b/gettext.c
@@ -47,17 +47,17 @@ const char *get_preferred_languages(void)
 }
 
 #ifdef GETTEXT_POISON
-int use_gettext_poison(void)
+enum poison_mode use_gettext_poison(void)
 {
-   static int poison_requested = -1;
-   if (poison_requested == -1) {
+   static enum poison_mode poison_mode = poison_mode_uninitialized;
+   if (poison_mode == poison_mode_uninitialized) {
const char *v = getenv("GIT_GETTEXT_POISON");
if (v && *v)
-   poison_requested = 1;
+   poison_mode = poison_mode_default;
else
-   poison_requested = 0;
+   poison_mode = poison_mode_none;
}
-   return poison_requested;
+   return poison_mode;
 }
 #endif
 
diff --git a/gettext.h b/gettext.h
index 8e279622f6..fcb6bfaa2c 100644
--- a/gettext.h
+++ b/gettext.h
@@ -42,7 +42,13 @@ static inline int gettext_width(const char *s)
 #endif
 
 #ifdef GETTEXT_POISON
-extern int use_gettext_poison(void);
+enum poison_mode {
+   poison_mode_uninitialized = -1,
+   poison_mode_none = 0,
+   poison_mode_default
+};
+
+extern enum poison_mode use_gettext_poison(void);
 
 #define GETTEXT_POISON_MAGIC "# GETTEXT POISON #"
 #endif
@@ -52,7 +58,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char 
*msgid)
if (!*msgid)
return "";
 #ifdef GETTEXT_POISON
-   if (use_gettext_poison())
+   if (use_gettext_poison() == poison_mode_default)
return GETTEXT_POISON_MAGIC;
 #endif
return gettext(msgid);
@@ -62,7 +68,7 @@ static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
 const char *Q_(const char *msgid, const char *plu, unsigned long n)
 {
 #ifdef GETTEXT_POISON
-   if (use_gettext_poison())
+   if (use_gettext_poison() == poison_mode_default)
return GETTEXT_POISON_MAGIC;
 #endif
return ngettext(msgid, plu, n);
-- 
2.19.1.681.g6bd79da3f5

[PATCH 4/8] gettext: #ifdef away GETTEXT POISON-related code from _() and Q_()

The gettext wrapper functions _() and Q_() contain a GETTEXT
POISON-related conditional construct even in non-GETTEXT POISON
builds, though both of those conditions are #define-d to be false
already at compile time.  Both constructs will grow in a later patch,
using a GETTEXT POISON-specific enum type and calling another GETTEXT
POISON-specific function.

Prepare for those future changes and hide the GETTEXT POISON-related
parts of those functions behind an #ifdef.

Signed-off-by: SZEDER Gábor 
---
 gettext.h | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/gettext.h b/gettext.h
index 7eee64a34f..c658942f7d 100644
--- a/gettext.h
+++ b/gettext.h
@@ -43,22 +43,26 @@ static inline int gettext_width(const char *s)
 
 #ifdef GETTEXT_POISON
 extern int use_gettext_poison(void);
-#else
-#define use_gettext_poison() 0
 #endif
 
 static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 {
if (!*msgid)
return "";
-   return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+#ifdef GETTEXT_POISON
+   if (use_gettext_poison())
+   return "# GETTEXT POISON #";
+#endif
+   return gettext(msgid);
 }
 
 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
 const char *Q_(const char *msgid, const char *plu, unsigned long n)
 {
+#ifdef GETTEXT_POISON
if (use_gettext_poison())
return "# GETTEXT POISON #";
+#endif
return ngettext(msgid, plu, n);
 }
 
-- 
2.19.1.681.g6bd79da3f5

[PATCH 5/8] gettext: put "# GETTEXT POISON #" string literal into a macro

The "# GETTEXT POISON #" string literal is currently used in two
functions in 'gettext.h'.  A later patch will add a function to
'gettext.c' using this string literal as well.

Avoid this duplication and put that string literal into a macro which
is only available in GETTEXT POISON builds.

Signed-off-by: SZEDER Gábor 
---
 gettext.h | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gettext.h b/gettext.h
index c658942f7d..8e279622f6 100644
--- a/gettext.h
+++ b/gettext.h
@@ -43,6 +43,8 @@ static inline int gettext_width(const char *s)
 
 #ifdef GETTEXT_POISON
 extern int use_gettext_poison(void);
+
+#define GETTEXT_POISON_MAGIC "# GETTEXT POISON #"
 #endif
 
 static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
@@ -51,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char 
*msgid)
return "";
 #ifdef GETTEXT_POISON
if (use_gettext_poison())
-   return "# GETTEXT POISON #";
+   return GETTEXT_POISON_MAGIC;
 #endif
return gettext(msgid);
 }
@@ -61,7 +63,7 @@ const char *Q_(const char *msgid, const char *plu, unsigned 
long n)
 {
 #ifdef GETTEXT_POISON
if (use_gettext_poison())
-   return "# GETTEXT POISON #";
+   return GETTEXT_POISON_MAGIC;
 #endif
return ngettext(msgid, plu, n);
 }
-- 
2.19.1.681.g6bd79da3f5

[PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled

Sometimes tests run with GETTEXT POISON fail because of a reason other
than a translated string that should not have been translated.  In
such a case an error message from a git command in the test's verbose
output is usually, well, less than helpful:

  error: # GETTEXT POISON #

or

  fatal: # GETTEXT POISON #: No such file or directory

It's especially annoying on those rare occasions when a heisenbug
decides it's a good time to suddenly reveal its presence during a
GETTEXT POISON test run, and all we get is an error message like these
(yes, I did actually see both of the above error messages only once).

Make builtin commands' GETTEXT POISON-ed error messages more useful
for debugging failures by introducing a new mode of poisoning: if
$GIT_GETTEXT_POISON is set to 'scrambled', then include the original
untranslated message after that "# GETTEXT_POISON #" string in a
scrambled form, interspersing a '.' after each character.  This way
the messages will remain gibberish enough for machine consumption as
they were before, but at the same time they will be relatively easily
legible for humans.  Take extra care to preserve printf() format
conversion specifiers unaltered when inserting those dots.

Leave 'git-sh-i18n.sh' unchanged, because translatable messages in
scripts often include shell variables, and they could (though
currently they don't) include printf format specifiers, parameter
expansions, command substitutions and whatnot, too.  Dealing with
those in a shell script would be too much hassle without its worth.

There is an additional benefit: as this change considerably increases
the size of translated messages, it could detect cases when we try to
format a translated string into a too small buffer.  E.g. this change
applied on old versions causes test failures because of the bug that
was fixed in 2cfa83574c (bisect_next_all: convert xsnprintf to
xstrfmt, 2017-02-16).

[TODO: Fallout?
   A 'printf(_("foo: %s"), var);' call includes the contents of
   'var' unscrambled in the output.  Could that hide the
   translation of a string that should not have been translated?
   I'm afraid yes: to check the output of that printf() a sloppy
   test could do:

 git plumbing-cmd >out && grep "var's content" out

   which would fail in a regular GETTEXT_POISON test run, but
   would succeed in a scrambled test run.  Does this matter in
   practice, do we care at all?

   Does gettext_scramble() need a FORMAT_PRESERVING annotation?
   Seems to work fine without it so far...]

Signed-off-by: SZEDER Gábor 
---
 gettext.c | 54 +++---
 gettext.h | 11 +--
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/gettext.c b/gettext.c
index c50d1e0377..8ba7fd0bea 100644
--- a/gettext.c
+++ b/gettext.c
@@ -52,13 +52,61 @@ enum poison_mode use_gettext_poison(void)
static enum poison_mode poison_mode = poison_mode_uninitialized;
if (poison_mode == poison_mode_uninitialized) {
const char *v = getenv("GIT_GETTEXT_POISON");
-   if (v && *v)
-   poison_mode = poison_mode_default;
-   else
+   if (v && *v) {
+   if (!strcmp(v, "scrambled"))
+   poison_mode = poison_mode_scrambled;
+   else
+   poison_mode = poison_mode_default;
+   } else
poison_mode = poison_mode_none;
}
return poison_mode;
 }
+
+static int conversion_specifier_len(const char *s)
+{
+   const char printf_conversion_specifiers[] = "diouxXeEfFgGaAcsCSpnm%";
+   const char *format_end;
+
+   if (*s != '%')
+   return 0;
+
+   format_end = strpbrk(s + 1, printf_conversion_specifiers);
+   if (format_end)
+   return format_end - s;
+   else
+   return 0;
+}
+
+const char *gettext_scramble(const char *msg)
+{
+   struct strbuf sb;
+
+   strbuf_init(,
+   /* "# GETTEXT_POISON #" + ' ' + "m.e.s.s.a.g.e." + '\0' */
+   strlen(GETTEXT_POISON_MAGIC) + 1 + 2 * strlen(msg) + 1);
+
+   strbuf_addch(, ' ');
+   while (*msg) {
+   if (*msg == '\n') {
+   strbuf_addch(, *(msg++));
+   continue;
+   } else if (*msg == '%') {
+   int spec_len = conversion_specifier_len(msg);
+   if (spec_len) {
+   strbuf_add(, msg, spec_len);
+   msg += spec_len;
+   continue;
+   }
+   }
+
+   strbuf_addch(, *(msg++));
+   strbuf_addch(, '.');
+   }
+
+   /* This will be leaked... */
+   return strbuf_detach(, NULL);
+}
 #endif
 
 #ifndef NO_GETTEXT
diff --git a/gettext.h b/gettext.h
index

[PATCH 8/8] travis-ci: run GETTEXT POISON build job in scrambled mode, too

Run the test suite twice in the GETTEXT POISON build: first with
GIT_GETTEXT_POISON=scrambled and then with "regular" poisoning, to see
whether the scrambled mode hid any mis-translations.

Signed-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh|  1 +
 ci/run-build-and-tests.sh | 10 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 109ef280da..fdfa4e035b 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -122,5 +122,6 @@ osx-clang|osx-gcc)
;;
 GETTEXT_POISON)
export GETTEXT_POISON=YesPlease
+   export GIT_GETTEXT_POISON=scrambled
;;
 esac
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 3735ce413f..74ba05e152 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -9,10 +9,14 @@ ln -s "$cache_dir/.prove" t/.prove
 
 make --jobs=2
 make --quiet test
-if test "$jobname" = "linux-gcc"
-then
+case "$jobname" in
+linux-gcc)
GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test
-fi
+   ;;
+GETTEXT_POISON)
+   GIT_GETTEXT_POISON=YesPlease make --quiet test
+   ;;
+esac
 
 check_unignored_build_artifacts
 
-- 
2.19.1.681.g6bd79da3f5

[PATCH 3/8] lib-rebase: loosen GETTEXT_POISON check in fake editor

The fake editor script created by 't/lib-rebase.sh' recognizes GETTEXT
POSION output when the first line of the file to be edited consists
solely of the GETTEXT POISON magic string as a comment.  However, a
later patch will include additional text after that magic string, so
that check won't work anymore.

So instead of expecting an exact match in the first line, check
whether there are any lines starting with the commented out magic
string.

Signed-off-by: SZEDER Gábor 
---
 t/lib-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 25a77ee5cb..530f8ec0a8 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -29,7 +29,7 @@ set_fake_editor () {
*/COMMIT_EDITMSG)
test -z "$EXPECT_HEADER_COUNT" ||
test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is 
a combination of \(.*\) commits\./\1/p' < "$1")" ||
-   test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
+   ! grep -q "^# # GETTEXT POISON #" ||
exit
test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > 
"$1"
test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> 
"$1"
-- 
2.19.1.681.g6bd79da3f5

[PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty

This allows us to run test with non-GETTEXT POISON-ed behavior even in
a GETTEXT POISON build by running:

  GIT_GETTEXT_POISON= ./t1234-foo.sh

Signed-off-by: SZEDER Gábor 
---
 Makefile  | 2 +-
 gettext.c | 9 +++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index ad880d1fc5..7a165445cd 100644
--- a/Makefile
+++ b/Makefile
@@ -365,7 +365,7 @@ all::
 # Define GETTEXT_POISON if you are debugging the choice of strings marked
 # for translation.  In a GETTEXT_POISON build, you can turn all strings marked
 # for translation into gibberish by setting the GIT_GETTEXT_POISON variable
-# (to any value) in your environment.
+# to a non-empty value in your environment.
 #
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have gitweb.js minified.
diff --git a/gettext.c b/gettext.c
index 7272771c8e..a9509a5df3 100644
--- a/gettext.c
+++ b/gettext.c
@@ -50,8 +50,13 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
static int poison_requested = -1;
-   if (poison_requested == -1)
-   poison_requested = getenv("GIT_GETTEXT_POISON") ? 1 : 0;
+   if (poison_requested == -1) {
+   const char *v = getenv("GIT_GETTEXT_POISON");
+   if (v && *v)
+   poison_requested = 1;
+   else
+   poison_requested = 0;
+   }
return poison_requested;
 }
 #endif
-- 
2.19.1.681.g6bd79da3f5

Re: [PATCH] Poison gettext with the Ook language

On Mon, Oct 22, 2018 at 05:36:33PM +0200, Nguyễn Thái Ngọc Duy wrote:
> The current gettext() function just replaces all strings with
> '# GETTEXT POISON #' including format strings and hides the things
> that we should be allowed to grep (like branch names, or some other
> codes) even when gettext is poisoned.
> 
> This patch implements the poisoned _() with a universal and totally
> legit language called Ook [1]. We could actually grep stuff even in
> with this because format strings are preserved.

Once upon a time a GETTEXT_POISON build job failed on me, and the
error message:

  error: # GETTEXT POISON #

was not particularly useful.  Ook wouldn't help with that...

So I came up with the following couple of patches that implement a
"scrambled" format that makes the poisoned output legible for humans
but still gibberish for machine consumption (i.e. grep-ing the text
part would still fail):

  error:  U.n.a.b.l.e. .t.o. .c.r.e.a.t.e. .'./home/szeder/src/git/t/trash 
directory.t1404-update-ref-errors/.git/packed-refs...l.o.c.k.'.:. .File 
exists...

I have been running GETTEXT_POISON builds with this series for some
months now, but haven't submitted it yet, because I haven't decided
yet whether including strings (paths, refs, etc.) in the output as
they are is a feature or a flaw.  And because it embarrassingly leaks
every single translated string... :)

SZEDER Gábor (8):
  test-lib.sh: preserve GIT_GETTEXT_POISON from the environment
  gettext: don't poison if GIT_GETTEXT_POISON is set but empty
  lib-rebase: loosen GETTEXT_POISON check in fake editor
  gettext: #ifdef away GETTEXT POISON-related code from _() and Q_()
  gettext: put "# GETTEXT POISON #" string literal into a macro
  gettext: use an enum for the mode of GETTEXT POISONing
  gettext: introduce GIT_GETTEXT_POISON=scrambled
  travis-ci: run GETTEXT POISON build job in scrambled mode, too

 Makefile  |  2 +-
 ci/lib-travisci.sh|  1 +
 ci/run-build-and-tests.sh | 10 +--
 gettext.c | 63 +++
 gettext.h | 33 +++-
 t/lib-rebase.sh   |  2 +-
 t/test-lib.sh | 17 ++-
 7 files changed, 110 insertions(+), 18 deletions(-)

-- 
2.19.1.681.g6bd79da3f5

[PATCH 1/8] test-lib.sh: preserve GIT_GETTEXT_POISON from the environment

Setting GIT_GETTEXT_POISON can be useful when testing translated
builds.

However, preserving its value from the environment is not as simple as
adding it to the list of GIT_* variables that should not be scrubbed
from the environment:

  - GIT_GETTEXT_POISON should not influence git commands executed
during initialization of test-lib and the test repo.

Save its value before it gets scrubbed from the environment, so it
will be unset for the duration of the initialization, and restore
its original value after initialization is finished.

  - When testing a GETTEXT_POISON build, 'test-lib.sh' always sets
GIT_GETTEXT_POISON to 'YesPlease'.  This was fine while all that
mattered was whether it's set or not.  However, the following
patches will introduce meaningful values (e.g. "set but empty" to
disable poisoning even in a GETTEXT_POISON build), so only set it
like this if GIT_GETTEXT_POISON was not set in the environment.

Signed-off-by: SZEDER Gábor 
---
 t/test-lib.sh | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ea2bbaaa7a..282c05110d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -95,6 +95,16 @@ PAGER=cat
 TZ=UTC
 export LANG LC_ALL PAGER TZ
 EDITOR=:
+
+# GIT_GETTEXT_POISON should not influence git commands executed during
+# initialization of test-lib and the test repo.
+# Back it up, unset and then restore after initialization is finished.
+if test -n "${GIT_GETTEXT_POISON-set}"
+then
+   git_gettext_poison_backup=$GIT_GETTEXT_POISON
+   unset GIT_GETTEXT_POISON
+fi
+
 # A call to "unset" with no arguments causes at least Solaris 10
 # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
 # deriving from the command substitution clustered with the other
@@ -1073,7 +1083,12 @@ test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 # Can we rely on git's output in the C locale?
 if test -n "$GETTEXT_POISON"
 then
-   GIT_GETTEXT_POISON=YesPlease
+   if test -n "${git_gettext_poison_backup-set}"
+   then
+   GIT_GETTEXT_POISON=$git_gettext_poison_backup
+   else
+   GIT_GETTEXT_POISON=YesPlease
+   fi
export GIT_GETTEXT_POISON
test_set_prereq GETTEXT_POISON
 else
-- 
2.19.1.681.g6bd79da3f5

Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote:

> From: Ben Peart 
> 
> Remove the src_offset parameter which is unused as a result of switching
> to the IEOT table of offsets.  Also stop incrementing src_offset in the
> multi-threaded codepath as it is no longer used and could cause confusion.

Hmm, OK. We only do threads if we have ieot:

>   if (ieot) {
> - src_offset += load_cache_entries_threaded(istate, mmap, 
> mmap_size, src_offset, nr_threads, ieot);
> + load_cache_entries_threaded(istate, mmap, mmap_size, 
> nr_threads, ieot);
>   free(ieot);
>   } else {
>   src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
> src_offset);

And we only have ieot if we had an extension_offset:

  if (extension_offset && nr_threads > 1)
  ieot = read_ieot_extension(mmap, mmap_size, extension_offset);

So later, when we _do_ use src_offset, we know that this code should
never trigger in the threaded case:

  if (!extension_offset) {
  p.src_offset = src_offset;
  load_index_extensions();
  }

So I think it's right, but it's rather subtle. I wonder if we could do
it like this:

unsigned long entry_offset;
  [...]
  #ifndef NO_PTHREADS
if (ieot)
load_cache_entries_threaded(...);
else
entry_offset = load_all_cache_entries(...);
  #else
entry_offset = load_all_cache_entries(...);
  [...]

  p.src_offset = src_offset + entry_offset;

and then the compiler could warn us that entry_offset is used
uninitialized (though I would not be surprised if the compiler gets
confused in this case).

Not sure if it is worth the trouble or not.


>  static unsigned long load_cache_entries_threaded(struct index_state *istate, 
> const char *mmap, size_t mmap_size,
> - unsigned long src_offset, int nr_threads, struct 
> index_entry_offset_table *ieot)
> + int nr_threads, struct index_entry_offset_table *ieot)

If nobody uses it, should we drop the return value, too? Like:

diff --git a/read-cache.c b/read-cache.c
index 78c9516eb7..4b44a2eae5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)
return NULL;
 }
 
-static unsigned long load_cache_entries_threaded(struct index_state *istate, 
const char *mmap, size_t mmap_size,
-int nr_threads, struct 
index_entry_offset_table *ieot)
+static void load_cache_entries_threaded(struct index_state *istate, const char 
*mmap, size_t mmap_size,
+   int nr_threads, struct 
index_entry_offset_table *ieot)
 {
int i, offset, ieot_blocks, ieot_start, err;
struct load_cache_entries_thread_data *data;
-   unsigned long consumed = 0;
 
/* a little sanity checking */
if (istate->name_hash_initialized)
@@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct 
index_state *istate, con
if (err)
die(_("unable to join load_cache_entries thread: %s"), 
strerror(err));
mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
-   consumed += p->consumed;
}
 
free(data);
-
-   return consumed;
 }
 #endif
 

-Peff

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:

> >  -q::
> >  --quiet::
> > -   Be quiet, only report errors.
> > +--no-quiet::
> > +   Be quiet, only report errors. The default behavior respects the
> > +   `reset.quiet` config option, or `--no-quiet` if that is not set.
> 
> Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
> command line (should) trump whatever rest.quiet is set to in the
> configuration. Is that not the case?

That is the case, and what was meant by "the default behavior" (i.e.,
the behavior when none of these is used). Maybe there's a more clear way
of saying that.

-Peff

Re: [PATCH 00/59] Split config.txt

On Sat, Oct 20, 2018 at 5:39 AM Nguyễn Thái Ngọc Duy  wrote:
>
> I started this a couple months back, moving a couple big config
> sections out of config.txt to make it more manageable. This series
> almost completes that. It moves all configs (except http.* which have
> changes on 'pu') out of config.txt. config.txt is now about the
> syntax, and a list of config sections. http section can be moved out
> later.
>
> I did a doc-diff on this series and the only change is ssh.variant is
> moved down a bit to keep it in order, which is intended.
>
> I thought of grouping all these config files in a separate directory
> Documentation/config to avoid cluttering Documentation/ too much but
> let's wait and see if people really find Documentation growing too
> large first.

Have you considered to use config as a prefix, i.e.
have config-add.txt instead of add-config.txt ?

currently any command is documented in a file
that is prefixed with "git-". There are others such as
gitcli or gitsubmodules, but as they lack the '-' they
surely are not about a command.

With the config- prefix it would be easier to sort and
cope with the individual files, and it would allow
grepping through Documentation/conf* (which may
be easier than Documentation/*conf* or similar)

I have no strong objection to the series as is
(I have not looked at any patch), but I would think
either a config directory or prefix may help further.

Stefan

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

2018-10-22 Thread Ramsay Jones




On 22/10/2018 14:18, Ben Peart wrote:
> From: Ben Peart 
> 
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
> 
> Signed-off-by: Ben Peart 
> ---
>  Documentation/config.txt| 3 +++
>  Documentation/git-reset.txt | 4 +++-
>  builtin/reset.c | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
>   `$GIT_DIR`, e.g. if "rerere" was previously used in the
>   repository.
>  
> +reset.quiet::
> + When set to true, 'git reset' will default to the '--quiet' option.
> +
>  include::sendemail-config.txt[]
>  
>  sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..51a427a34a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,9 @@ OPTIONS
>  
>  -q::
>  --quiet::
> - Be quiet, only report errors.
> +--no-quiet::
> + Be quiet, only report errors. The default behavior respects the
> + `reset.quiet` config option, or `--no-quiet` if that is not set.

Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
command line (should) trump whatever rest.quiet is set to in the
configuration. Is that not the case?

ATB,
Ramsay Jones

>  
>  
>  EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char 
> *prefix)
>   };
>  
>   git_config(git_reset_config, NULL);
> + git_config_get_bool("reset.quiet", );
>  
>   argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>   PARSE_OPT_KEEP_DASHDASH);
>

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

On Mon, Oct 22, 2018 at 10:39 AM SZEDER Gábor  wrote:
>
> On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:
> > the last patch (applying the semantic patches) has been omitted as that
> > would produce a lot of merge conflicts. Without that patch, this merges
> > cleanly to next.
> >
> > As for when to apply the semantic patches, I wondered if we would prefer a 
> > dirty merge
> > (created via "make coccicheck && git apply 
> > contrib/coccinelle/the_repository.cocci.patch")
> > of the semantic patches, or if we'd actually trickle in the changes over 
> > some time?
>
> > This series takes another approach as it doesn't change the signature of
> > functions, but introduces new functions that can deal with arbitrary
> > repositories, keeping the old function signature around using a shallow 
> > wrapper.
> >
> > Additionally each patch adds a semantic patch, that would port from the 
> > old to
> > the new function. These semantic patches are all applied in the very 
> > last patch,
> > but we could omit applying the last patch if it causes too many merge 
> > conflicts
> > and trickl in the semantic patches over time when there are no merge 
> > conflicts.
>
> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
>
>   - Applying the transformations from several semantic patches in one
> single patch makes it harder to review it, because we won't know
> which change came from which semantic patch.

Good point, so to improve the series sb/more-repo-in-api, I could
send the application of the semantic patch just after each patch
that adds another semantic patching rule? I personally dislike
applying the semantic patch in the same patch as where the
semantic rule was introduced, as then the mechanical conversions
(from the semantic patch) drown out reviewers attention to the
manual changes.

> For comparison, see the patch series adding hasheq()/oideq(),
> merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in
> particular the four "convert  to " patches.

The four patches "convert  to  only contain the semantic
patch as well as its effects, the manual changes are separated out
to later patches, which is quite nice.

>   - 'make coccicheck' won't run clean (and the static analysis build
> job on Travis CI will fail) for all commits following adding the
> new semantic patches but before applying the resulting
> transformations.
>
>   - These semantic patches interact badly with 'pu' and 'next',
> because those integration branches can contain topic branches
> adding new code that should be transformed by these semanic
> patches.

And I thought of this as a feature. There is no merge conflict and it
still compiles, which makes Junios work easy.

Of course it put the load elsewhere. :/

For the sake of a good history, I would think running 'make coccicheck'
and applying the resulting patches would be best as part of the (dirty)
merge of any topic that proposes new semantic patches, but that would
add load to Junio as it would be an extra step during the merge.

One could argue that the step of applying such transformations into
the dirty merge is cheaper than resolving merge conflicts that are
had when the topic includes the transformation.

> Consequently, 'make coccicheck' won't run clean and the
> static analysis build job will fail until all those topics reach
> 'master', and the remaining transformations are applied on top.
>
> This was (and still is!) an issue with the hasheq()/oideq() series
> as well: that series was added on 2018-08-28, and the static
> analysis build job is red on 'pu' ever since.  See the follow-up
> patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and
> one more follow-up will be necessary after the builtin stash topic
> is merged to 'master'.

In my understanding this follow up is a feature, as it helps to avoid
merge conflicts with other topics in flight.

> This makes it harder to review other patch series.

as 'make coccicheck' is an integral part of your review?

>   - Is it really necessary to carry these semantic patches in-tree?
> Let me ellaborate.  There are basically two main use cases for
> semantic patches:
>
>   - To avoid undesirable code patterns, e.g. we should not use
> sha1_to_hex(oid.hash) or strbuf_addf(, "fixed string"), but
> use oid_to_hex() or strbuf_addstr(, "fixed string")
> instead.  Note that in these cases we don't remove the
> functions sha1_to_hex() or strbuf_addf(), because there are
> good reasons to use them in other scenarios.
>
> Our semantic patches under 'contrib/coccinelle/' fall into
> this category, and we have 'make coccicheck' and the static
> analysis build job on Travis CI to catch these

Re: [PATCH v2] send-email: explicitly disable authentication

On Mon, Oct 22, 2018 at 12:52 PM Joshua Watt  wrote:
>
> It can be necessary to disable SMTP authentication by a mechanism other
> than sendemail.smtpuser being undefined. For example, if the user has
> sendemail.smtpuser set globally but wants to disable authentication
> locally in one repository.
>
> --smtp-auth and sendemail.smtpauth now understand the value 'none' which
> means to disable authentication completely, even if an authentication
> user is specified.
>
> The value 'none' is lower case to avoid conflicts with any RFC 4422
> authentication mechanisms.
>
> The user may also specify the command line argument --no-smtp-auth as a
> shorthand for --smtp-auth=none
>
> Signed-off-by: Joshua Watt 
> ---
>  Documentation/git-send-email.txt | 7 ++-
>  git-send-email.perl  | 8 ++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt 
> b/Documentation/git-send-email.txt
> index 465a4ecbe..17993e3c9 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -190,7 +190,9 @@ $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ...
>  If at least one of the specified mechanisms matches the ones advertised by 
> the
>  SMTP server and if it is supported by the utilized SASL library, the 
> mechanism
>  is used for authentication. If neither 'sendemail.smtpAuth' nor `--smtp-auth`
> -is specified, all mechanisms supported by the SASL library can be used.
> +is specified, all mechanisms supported by the SASL library can be used. The
> +special value 'none' maybe specified to completely disable authentication
> +independently of `--smtp-user`
>
>  --smtp-pass[=]::
> Password for SMTP-AUTH. The argument is optional: If no
> @@ -204,6 +206,9 @@ or on the command line. If a username has been specified 
> (with
>  specified (with `--smtp-pass` or `sendemail.smtpPass`), then
>  a password is obtained using 'git-credential'.
>
> +--no-smtp-auth::
> +   Disable SMTP authentication. Short hand for `--smtp-auth=none`
> +
>  --smtp-server=::
> If set, specifies the outgoing SMTP server to use (e.g.
> `smtp.example.com` or a raw IP address).  Alternatively it can
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2be5dac33..7d7e69581 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -82,8 +82,11 @@ sub usage {
>   Pass an empty string to disable 
> certificate
>   verification.
>  --smtp-domain * The domain name sent to HELO/EHLO 
> handshake
> ---smtp-auth   * Space-separated list of allowed AUTH 
> mechanisms.
> +--smtp-auth   * Space-separated list of allowed AUTH 
> mechanisms, or
> + "none" to disable authentication.
>   This setting forces to use one of the 
> listed mechanisms.
> +--no-smtp-auth   Disable SMTP authentication. Shorthand 
> for
> + `--smtp-auth=none`
>  --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
>
>  --batch-size  * send max  message per connection.
> @@ -341,6 +344,7 @@ sub signal_handler {
> "smtp-debug:i" => \$debug_net_smtp,
> "smtp-domain:s" => \$smtp_domain,
> "smtp-auth=s" => \$smtp_auth,
> +   "no-smtp-auth" => sub {$smtp_auth = 'none'},
> "identity=s" => \$identity,
> "annotate!" => \$annotate,
> "no-annotate" => sub {$annotate = 0},
> @@ -1241,7 +1245,7 @@ sub smtp_host_string {
>  # (smtp_user was not specified), and 0 otherwise.
>
>  sub smtp_auth_maybe {
> -   if (!defined $smtp_authuser || $auth) {
> +   if (!defined $smtp_authuser || $auth || $smtp_auth eq "none") {

Oops, this generates a warning when no smtp auth argument is supplied
(comparison to undefined value). Version 3 will be along shortly.

> return 1;
> }
>
> --
> 2.19.1.543.g99a77c85e.dirty
>

[PATCH v2] send-email: explicitly disable authentication

It can be necessary to disable SMTP authentication by a mechanism other
than sendemail.smtpuser being undefined. For example, if the user has
sendemail.smtpuser set globally but wants to disable authentication
locally in one repository.

--smtp-auth and sendemail.smtpauth now understand the value 'none' which
means to disable authentication completely, even if an authentication
user is specified.

The value 'none' is lower case to avoid conflicts with any RFC 4422
authentication mechanisms.

The user may also specify the command line argument --no-smtp-auth as a
shorthand for --smtp-auth=none

Signed-off-by: Joshua Watt 
---
 Documentation/git-send-email.txt | 7 ++-
 git-send-email.perl  | 8 ++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 465a4ecbe..17993e3c9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -190,7 +190,9 @@ $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ...
 If at least one of the specified mechanisms matches the ones advertised by the
 SMTP server and if it is supported by the utilized SASL library, the mechanism
 is used for authentication. If neither 'sendemail.smtpAuth' nor `--smtp-auth`
-is specified, all mechanisms supported by the SASL library can be used.
+is specified, all mechanisms supported by the SASL library can be used. The
+special value 'none' maybe specified to completely disable authentication
+independently of `--smtp-user`
 
 --smtp-pass[=]::
Password for SMTP-AUTH. The argument is optional: If no
@@ -204,6 +206,9 @@ or on the command line. If a username has been specified 
(with
 specified (with `--smtp-pass` or `sendemail.smtpPass`), then
 a password is obtained using 'git-credential'.
 
+--no-smtp-auth::
+   Disable SMTP authentication. Short hand for `--smtp-auth=none`
+
 --smtp-server=::
If set, specifies the outgoing SMTP server to use (e.g.
`smtp.example.com` or a raw IP address).  Alternatively it can
diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac33..7d7e69581 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -82,8 +82,11 @@ sub usage {
  Pass an empty string to disable 
certificate
  verification.
 --smtp-domain * The domain name sent to HELO/EHLO 
handshake
---smtp-auth   * Space-separated list of allowed AUTH 
mechanisms.
+--smtp-auth   * Space-separated list of allowed AUTH 
mechanisms, or
+ "none" to disable authentication.
  This setting forces to use one of the 
listed mechanisms.
+--no-smtp-auth   Disable SMTP authentication. Shorthand for
+ `--smtp-auth=none`
 --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
 
 --batch-size  * send max  message per connection.
@@ -341,6 +344,7 @@ sub signal_handler {
"smtp-debug:i" => \$debug_net_smtp,
"smtp-domain:s" => \$smtp_domain,
"smtp-auth=s" => \$smtp_auth,
+   "no-smtp-auth" => sub {$smtp_auth = 'none'},
"identity=s" => \$identity,
"annotate!" => \$annotate,
"no-annotate" => sub {$annotate = 0},
@@ -1241,7 +1245,7 @@ sub smtp_host_string {
 # (smtp_user was not specified), and 0 otherwise.
 
 sub smtp_auth_maybe {
-   if (!defined $smtp_authuser || $auth) {
+   if (!defined $smtp_authuser || $auth || $smtp_auth eq "none") {
return 1;
}
 
-- 
2.19.1.543.g99a77c85e.dirty

Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options

On Mon, Oct 22, 2018 at 04:34:16PM +0200, Duy Nguyen wrote:
> On Mon, Oct 22, 2018 at 5:51 AM Junio C Hamano  wrote:
> >
> > Nguyễn Thái Ngọc Duy   writes:
> >
> > > __gitcomp_builtin() has the main completion list provided by
> > >
> > > git xxx --git-completion-helper
> > >
> > > but the caller can also add extra options that is not provided by
> > > --git-completion-helper. The only call site that does this is "git
> > > difftool" completion.
> > >
> > > This support is broken by b221b5ab9b (completion: collapse extra
> > > --no-.. options - 2018-06-06), which adds a special value "--" to mark
> > > that the rest of the options can be hidden by default. The commit
> > > forgets the fact that extra options are appended after
> > > "$(git xxx --git-completion-helper)", i.e. after this "--", and will
> > > be incorrectly hidden as well.
> > >
> > > Prepend the extra options before "$(git xxx --git-completion-helper)"
> > > to avoid this.
> >
> > Thanks for a clear analysis.  How did you find it?  Got annoyed that
> > completion of difftool got broken, or discovered while trying to
> > apply the same trick as difftool completion uses to another one and
> > seeing that the technique does not work?
> 
> I was fixing format-patch completion and was surprised it did not work
> as expected. Never really used difftool myself :P I only found out
> about it when I asked myself "why wasn't this breakage found earlier?"

Erm...  maybe because there are no tests covering the combination of
extra options and '--no-..' options in 't9902-completion.sh'?

(Hint, hint... :)

New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:
> the last patch (applying the semantic patches) has been omitted as that
> would produce a lot of merge conflicts. Without that patch, this merges
> cleanly to next.
> 
> As for when to apply the semantic patches, I wondered if we would prefer a 
> dirty merge
> (created via "make coccicheck && git apply 
> contrib/coccinelle/the_repository.cocci.patch")
> of the semantic patches, or if we'd actually trickle in the changes over some 
> time?

> This series takes another approach as it doesn't change the signature of
> functions, but introduces new functions that can deal with arbitrary 
> repositories, keeping the old function signature around using a shallow 
> wrapper.
> 
> Additionally each patch adds a semantic patch, that would port from the 
> old to
> the new function. These semantic patches are all applied in the very last 
> patch,
> but we could omit applying the last patch if it causes too many merge 
> conflicts
> and trickl in the semantic patches over time when there are no merge 
> conflicts.

I don't really like how this or the previous RFC patch series deal
with semantic patches (or how some past patch series dealt with them,
for that matter), for various reasons:

  - Applying the transformations from several semantic patches in one
single patch makes it harder to review it, because we won't know
which change came from which semantic patch.

For comparison, see the patch series adding hasheq()/oideq(),
merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in
particular the four "convert  to " patches.

  - 'make coccicheck' won't run clean (and the static analysis build
job on Travis CI will fail) for all commits following adding the
new semantic patches but before applying the resulting
transformations.

  - These semantic patches interact badly with 'pu' and 'next',
because those integration branches can contain topic branches
adding new code that should be transformed by these semanic
patches.  Consequently, 'make coccicheck' won't run clean and the
static analysis build job will fail until all those topics reach
'master', and the remaining transformations are applied on top.

This was (and still is!) an issue with the hasheq()/oideq() series
as well: that series was added on 2018-08-28, and the static
analysis build job is red on 'pu' ever since.  See the follow-up
patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and
one more follow-up will be necessary after the builtin stash topic
is merged to 'master'.

This makes it harder to review other patch series.

  - Is it really necessary to carry these semantic patches in-tree?
Let me ellaborate.  There are basically two main use cases for
semantic patches:

  - To avoid undesirable code patterns, e.g. we should not use
sha1_to_hex(oid.hash) or strbuf_addf(, "fixed string"), but
use oid_to_hex() or strbuf_addstr(, "fixed string")
instead.  Note that in these cases we don't remove the
functions sha1_to_hex() or strbuf_addf(), because there are
good reasons to use them in other scenarios.

Our semantic patches under 'contrib/coccinelle/' fall into
this category, and we have 'make coccicheck' and the static
analysis build job on Travis CI to catch these undesirable
code patterns preferably early, and to prevent them from
entering our codebase.

  - To perform one-off code transformations, e.g. to modify a
function's name and/or signature and convert all its
callsites; see e.g. commits abef9020e3 (sha1_file: convert
sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e
(sha1_file: convert read_sha1_file to struct object_id,
2018-03-12).

As far as I understand this patch series falls into this
category: once the conversion is complete the old functions
will be removed.  After that there will be no use for these
semanic patches.

Having said that, it's certainly easier to double-check the
resulting transformations when one can apply the semantic
patches locally, and doing so is easier when the semantic
patches are in tree than when they must be copy-pasted from a
commit message.

OK, that was already long.  Now, can we do better?

How about introducing the concept of "pending" semantic patches,
stored in 'contrib/coccinelle/.pending.cocci' files, modifying
'make coccicheck' to skip them, and adding the new 'make
coccicheck-pending' target to make it convenient to apply them, e.g.
something like the simple patch at the end.

So the process would go something like this:

  - A new semantic patch should be added as "pending", e.g. to the
file 'the_repository.pending.cocci', together with the resulting
transformations in the same

[PATCH 2/3] gpg-interface.c: Support getting key fingerprint via %GF format

Support processing VALIDSIG status that provides additional information
for valid signatures.  Use this information to propagate signing key
fingerprint and expose it via %GF pretty format.  This format can be
used to build safer key verification systems that verify the key via
complete fingerprint rather than short/long identifier provided by %GK.

Signed-off-by: Michał Górny 
---
 Documentation/pretty-formats.txt |  1 +
 gpg-interface.c  | 14 +-
 gpg-interface.h  |  1 +
 pretty.c |  4 
 t/t7510-signed-commit.sh | 18 --
 5 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6109ef09a..8ab7d6dd1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -153,6 +153,7 @@ endif::git-rev-list[]
   and "N" for no signature
 - '%GS': show the name of the signer for a signed commit
 - '%GK': show the key used to sign a signed commit
+- '%GF': show the fingerprint of the key used to sign a signed commit
 - '%gD': reflog selector, e.g., `refs/stash@{1}` or
   `refs/stash@{2 minutes ago`}; the format follows the rules described
   for the `-g` option. The portion before the `@` is the refname as
diff --git a/gpg-interface.c b/gpg-interface.c
index c7cd24ec0..a406484e4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -73,6 +73,7 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->gpg_status);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
+   FREE_AND_NULL(sigc->fingerprint);
 }
 
 /* An exclusive status -- only one of them can appear in output */
@@ -81,6 +82,8 @@ void signature_check_clear(struct signature_check *sigc)
 #define GPG_STATUS_KEYID   (1<<1)
 /* The status includes user identifier */
 #define GPG_STATUS_UID (1<<2)
+/* The status includes key fingerprints */
+#define GPG_STATUS_FINGERPRINT (1<<3)
 
 /* Short-hand for standard exclusive *SIG status with keyid & UID */
 #define GPG_STATUS_STDSIG  
(GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID|GPG_STATUS_UID)
@@ -98,6 +101,7 @@ static struct {
{ 'X', "EXPSIG ", GPG_STATUS_STDSIG },
{ 'Y', "EXPKEYSIG ", GPG_STATUS_STDSIG },
{ 'R', "REVKEYSIG ", GPG_STATUS_STDSIG },
+   { 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
 };
 
 static void parse_gpg_output(struct signature_check *sigc)
@@ -123,7 +127,8 @@ static void parse_gpg_output(struct signature_check *sigc)
goto found_duplicate_status;
}
 
-   sigc->result = sigcheck_gpg_status[i].result;
+   if (sigcheck_gpg_status[i].result)
+   sigc->result = 
sigcheck_gpg_status[i].result;
/* Do we have key information? */
if (sigcheck_gpg_status[i].flags & 
GPG_STATUS_KEYID) {
next = strchrnul(line, ' ');
@@ -137,6 +142,12 @@ static void parse_gpg_output(struct signature_check *sigc)
sigc->signer = xmemdupz(line, 
next - line);
}
}
+   /* Do we have fingerprint? */
+   if (sigcheck_gpg_status[i].flags & 
GPG_STATUS_FINGERPRINT) {
+   next = strchrnul(line, ' ');
+   free(sigc->fingerprint);
+   sigc->fingerprint = xmemdupz(line, next 
- line);
+   }
 
break;
}
@@ -154,6 +165,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 */
sigc->result = 'E';
/* Clear partial data to avoid confusion */
+   FREE_AND_NULL(sigc->fingerprint);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
 }
diff --git a/gpg-interface.h b/gpg-interface.h
index acf50c461..8ce614fc9 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,6 +23,7 @@ struct signature_check {
char result;
char *signer;
char *key;
+   char *fingerprint;
 };
 
 void signature_check_clear(struct signature_check *sigc);
diff --git a/pretty.c b/pretty.c
index 8ca29e928..4567b5321 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1256,6 +1256,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
if (c->signature_check.key)
strbuf_addstr(sb, c->signature_check.key);
break;
+   case 'F':
+   if (c->signature_check.fingerprint)
+   strbuf_addstr(sb, 
c->signature_check.fingerprint);
+

[PATCH 1/3] gpg-interface.c: use flags to determine key/signer info presence

Replace the logic used to determine whether key and signer information
is present to use explicit flags in sigcheck_gpg_status[] array.  This
is more future-proof, since it makes it possible to add additional
statuses without having to explicitly update the conditions.

Signed-off-by: Michał Górny 
---
 gpg-interface.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d72a43b77..c7cd24ec0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -77,20 +77,27 @@ void signature_check_clear(struct signature_check *sigc)
 
 /* An exclusive status -- only one of them can appear in output */
 #define GPG_STATUS_EXCLUSIVE   (1<<0)
+/* The status includes key identifier */
+#define GPG_STATUS_KEYID   (1<<1)
+/* The status includes user identifier */
+#define GPG_STATUS_UID (1<<2)
+
+/* Short-hand for standard exclusive *SIG status with keyid & UID */
+#define GPG_STATUS_STDSIG  
(GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID|GPG_STATUS_UID)
 
 static struct {
char result;
const char *check;
unsigned int flags;
 } sigcheck_gpg_status[] = {
-   { 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
-   { 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
+   { 'G', "GOODSIG ", GPG_STATUS_STDSIG },
+   { 'B', "BADSIG ", GPG_STATUS_STDSIG },
{ 'U', "TRUST_NEVER", 0 },
{ 'U', "TRUST_UNDEFINED", 0 },
-   { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
-   { 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
-   { 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
-   { 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
+   { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID },
+   { 'X', "EXPSIG ", GPG_STATUS_STDSIG },
+   { 'Y', "EXPKEYSIG ", GPG_STATUS_STDSIG },
+   { 'R', "REVKEYSIG ", GPG_STATUS_STDSIG },
 };
 
 static void parse_gpg_output(struct signature_check *sigc)
@@ -117,13 +124,13 @@ static void parse_gpg_output(struct signature_check *sigc)
}
 
sigc->result = sigcheck_gpg_status[i].result;
-   /* The trust messages are not followed by 
key/signer information */
-   if (sigc->result != 'U') {
+   /* Do we have key information? */
+   if (sigcheck_gpg_status[i].flags & 
GPG_STATUS_KEYID) {
next = strchrnul(line, ' ');
free(sigc->key);
sigc->key = xmemdupz(line, next - line);
-   /* The ERRSIG message is not followed 
by signer information */
-   if (*next && sigc->result != 'E') {
+   /* Do we have signer information? */
+   if (*next && 
(sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
line = next + 1;
next = strchrnul(line, '\n');
free(sigc->signer);
-- 
2.19.1

[PATCH 3/3] gpg-interface.c: Obtain primary key fingerprint as well

Obtain the primary key fingerprint off VALIDSIG status message,
and expose it via %GP format.

Signed-off-by: Michał Górny 
---
 Documentation/pretty-formats.txt |  2 ++
 gpg-interface.c  | 16 +++-
 gpg-interface.h  |  1 +
 pretty.c |  4 
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8ab7d6dd1..417b638cd 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -154,6 +154,8 @@ endif::git-rev-list[]
 - '%GS': show the name of the signer for a signed commit
 - '%GK': show the key used to sign a signed commit
 - '%GF': show the fingerprint of the key used to sign a signed commit
+- '%GP': show the fingerprint of the primary key whose subkey was used
+  to sign a signed commit
 - '%gD': reflog selector, e.g., `refs/stash@{1}` or
   `refs/stash@{2 minutes ago`}; the format follows the rules described
   for the `-g` option. The portion before the `@` is the refname as
diff --git a/gpg-interface.c b/gpg-interface.c
index a406484e4..8ed274533 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -74,6 +74,7 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
FREE_AND_NULL(sigc->fingerprint);
+   FREE_AND_NULL(sigc->primary_key_fingerprint);
 }
 
 /* An exclusive status -- only one of them can appear in output */
@@ -108,7 +109,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 {
const char *buf = sigc->gpg_status;
const char *line, *next;
-   int i;
+   int i, j;
int seen_exclusive_status = 0;
 
/* Iterate over all lines */
@@ -147,6 +148,18 @@ static void parse_gpg_output(struct signature_check *sigc)
next = strchrnul(line, ' ');
free(sigc->fingerprint);
sigc->fingerprint = xmemdupz(line, next 
- line);
+
+   /* Skip interim fields */
+   for (j = 9; j > 0; j--) {
+   if (!*next)
+   break;
+   line = next + 1;
+   next = strchrnul(line, ' ');
+   }
+
+   next = strchrnul(line, '\n');
+   free(sigc->primary_key_fingerprint);
+   sigc->primary_key_fingerprint = 
xmemdupz(line, next - line);
}
 
break;
@@ -165,6 +178,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 */
sigc->result = 'E';
/* Clear partial data to avoid confusion */
+   FREE_AND_NULL(sigc->primary_key_fingerprint);
FREE_AND_NULL(sigc->fingerprint);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
diff --git a/gpg-interface.h b/gpg-interface.h
index 8ce614fc9..3e624ec28 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -24,6 +24,7 @@ struct signature_check {
char *signer;
char *key;
char *fingerprint;
+   char *primary_key_fingerprint;
 };
 
 void signature_check_clear(struct signature_check *sigc);
diff --git a/pretty.c b/pretty.c
index 4567b5321..b83a3ecd2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1260,6 +1260,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
if (c->signature_check.fingerprint)
strbuf_addstr(sb, 
c->signature_check.fingerprint);
break;
+   case 'P':
+   if (c->signature_check.primary_key_fingerprint)
+   strbuf_addstr(sb, 
c->signature_check.primary_key_fingerprint);
+   break;
default:
return 0;
}
-- 
2.19.1

[PATCH] Poison gettext with the Ook language

2018-10-22 Thread Nguyễn Thái Ngọc Duy

The current gettext() function just replaces all strings with
'# GETTEXT POISON #' including format strings and hides the things
that we should be allowed to grep (like branch names, or some other
codes) even when gettext is poisoned.

This patch implements the poisoned _() with a universal and totally
legit language called Ook [1]. We could actually grep stuff even in
with this because format strings are preserved.

Long term, we could implement an "ook translator" for test_i18ngrep
and friends so that they translate English to Ook, allowing us to
match full text while making sure the text in the code is still marked
for translation.

[1] https://en.wikipedia.org/wiki/Unseen_University#Librarian

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 This started out as something fun to do while running the test suite
 last weekend. But it turns out actually working! If this patch ends
 up in git.git, the Librarian would be so proud!

 gettext.c   | 54 +
 gettext.h   |  7 ---
 t/lib-rebase.sh |  2 +-
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/gettext.c b/gettext.c
index 7272771c8e..29901e1ddd 100644
--- a/gettext.c
+++ b/gettext.c
@@ -56,6 +56,60 @@ int use_gettext_poison(void)
 }
 #endif
 
+const char *gettext_poison(const char *msgid)
+{
+   /*
+* gettext() returns a string that is always valid. We would
+* need a hash map for that but let's stay simple and keep the
+* last 64 gettext() results. Should be more than enough.
+*/
+   static char *bufs[64];
+   static int i;
+   struct strbuf sb = STRBUF_INIT;
+   char *buf;
+   const char *p;
+   const char *type_specifiers = "diouxXeEfFgGaAcsCSpnm%";
+
+   if (!strchr(msgid, '%'))
+   return "Eek!";
+
+   p = msgid;
+   while (*p) {
+   const char *type;
+   switch (*p) {
+   case '%':
+   /*
+* No strict parsing. We simply look for the end of a
+* format string
+*/
+   type = p + 1;
+   while (*type && !strchr(type_specifiers, *type))
+   type++;
+   if (*type)
+   type++;
+   strbuf_add(, p, (int)(type - p));
+   p = type;
+   break;
+   default:
+   if (!isalpha(*p)) {
+   strbuf_addch(, *p);
+   p++;
+   break;
+   }
+   if (isupper(*p))
+   strbuf_addstr(, "Ook");
+   else
+   strbuf_addstr(, "ook");
+   while (isalpha(*p))
+   p++;
+   }
+   }
+   buf = bufs[(i++) % ARRAY_SIZE(bufs)];
+   free(buf);
+   buf = strbuf_detach(, NULL);
+   return buf;
+}
+
 #ifndef NO_GETTEXT
 static int test_vsnprintf(const char *fmt, ...)
 {
diff --git a/gettext.h b/gettext.h
index 7eee64a34f..dc9851a06a 100644
--- a/gettext.h
+++ b/gettext.h
@@ -41,8 +41,9 @@ static inline int gettext_width(const char *s)
 }
 #endif
 
+const char *gettext_poison(const char *);
 #ifdef GETTEXT_POISON
-extern int use_gettext_poison(void);
+int use_gettext_poison(void);
 #else
 #define use_gettext_poison() 0
 #endif
@@ -51,14 +52,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char 
*msgid)
 {
if (!*msgid)
return "";
-   return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+   return use_gettext_poison() ? gettext_poison(msgid) : gettext(msgid);
 }
 
 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
 const char *Q_(const char *msgid, const char *plu, unsigned long n)
 {
if (use_gettext_poison())
-   return "# GETTEXT POISON #";
+   return gettext_poison(msgid);
return ngettext(msgid, plu, n);
 }
 
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 2ca9fb69d6..1e8440e935 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -29,7 +29,7 @@ set_fake_editor () {
*/COMMIT_EDITMSG)
test -z "$EXPECT_HEADER_COUNT" ||
test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is 
a combination of \(.*\) commits\./\1/p' < "$1")" ||
-   test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
+   test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# Ook ook 
ook ook ook \(.*\) ook\./\1/p' < "$1")" ||
exit
test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > 
"$1"
test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> 
"$1"
-- 
2.19.1.647.g708186aaf9

Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits

On Mon, 2018-10-22 at 08:04 +, Michał Górny wrote:
> Dnia October 20, 2018 11:57:36 PM UTC, Junio C Hamano  
> napisał(a):
> > Michał Górny  writes:
> > 
> > > GnuPG supports creating signatures consisting of multiple signature
> > > packets.  If such a signature is verified, it outputs all the status
> > > messages for each signature separately.  However, git currently does
> > 
> > not
> > > account for such scenario and gets terribly confused over getting
> > > multiple *SIG statuses.
> > > 
> > > For example, if a malicious party alters a signed commit and appends
> > > a new untrusted signature, git is going to ignore the original bad
> > > signature and report untrusted commit instead.  However, %GK and %GS
> > > format strings may still expand to the data corresponding
> > > to the original signature, potentially tricking the scripts into
> > > trusting the malicious commit.
> > > 
> > > Given that the use of multiple signatures is quite rare, git does not
> > > support creating them without jumping through a few hoops, and
> > 
> > finally
> > > supporting them properly would require extensive API improvement, it
> > > seems reasonable to just reject them at the moment.
> > > 
> > > Signed-off-by: Michał Górny 
> > > ---
> > >  gpg-interface.c  | 90
> > 
> > +++-
> > >  t/t7510-signed-commit.sh | 26 
> > >  2 files changed, 87 insertions(+), 29 deletions(-)
> > > 
> > > Changes in v4:
> > > * switched to using skip_prefix(),
> > > * renamed the variable to seen_exclusive_status,
> > > * made the loop terminate early on first duplicate status seen.
> > 
> > Thanks for sticking to the topic and polishing it further.  Looks
> > very good.  
> > 
> > Will replace.
> > 
> > > + int seen_exclusive_status = 0;
> > > +
> > > + /* Iterate over all lines */
> > > + for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> > > + while (*line == '\n')
> > > + line++;
> > > + /* Skip lines that don't start with GNUPG status */
> > > + if (!skip_prefix(line, "[GNUPG:] ", ))
> > > + continue;
> > > +
> > > + /* Iterate over all search strings */
> > > + for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > > + if (skip_prefix(line, sigcheck_gpg_status[i].check, 
> > > )) {
> > > + if (sigcheck_gpg_status[i].flags & 
> > > GPG_STATUS_EXCLUSIVE) {
> > > + if (++seen_exclusive_status > 1)
> > > + goto found_duplicate_status;
> > 
> > Very minor point but by not using pre-increment, i.e.
> > 
> > if (seen_exclusive_status++)
> > goto found_duplicate_status;
> > 
> > you can use the expression as a "have we already seen?" boolean,
> > whic may probably be more idiomatic.
> > 
> > The patch is good in the way written as-is, and this is so minor
> > that it is not worth rerolling to only update this part.
> 
> Please don't merge it yet. I gave it some more thought and I think the loop 
> refactoring may cause TRUST_* to override BADSIG (i.e. upgrade from 'bad' to 
> 'untrusted'). I'm going to verify this when I get home.
> 

I was wrong.  I'm sorry about the noise.  I've reverified the logic,
and it correct.  That is:

1) for trusted signature, only GOODSIG is emitted and 'G' is returned
correctly,

2) for untrusted signature, GOODSIG is followed by TRUST_* messages,
so line-wise TRUST_* check replaces the 'G' with 'U',

3) for bad signature, only BADSIG is emitted without TRUST_* messages.

Furthermore, GnuPG documentation confirms that TRUST_* is only emitted
for good signatures [1].

[1]:https://github.com/gpg/gnupg/blob/master/doc/DETAILS#trust_

-- 
Best regards,
Michał Górny


signature.asc
Description: This is a digitally signed message part

[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

2018-10-22 Thread Ben Peart

From: Ben Peart 

Remove the src_offset parameter which is unused as a result of switching
to the IEOT table of offsets.  Also stop incrementing src_offset in the
multi-threaded codepath as it is no longer used and could cause confusion.

Signed-off-by: Ben Peart 
---

Notes:
Base Ref:
Web-Diff: https://github.com/benpeart/git/commit/7360721408
Checkout: git fetch https://github.com/benpeart/git 
read-index-multithread-no-src-offset-v1 && git checkout 7360721408

 read-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f9fa6a7979..6db6f0f220 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2037,7 +2037,7 @@ static void *load_cache_entries_thread(void *_data)
 }
 
 static unsigned long load_cache_entries_threaded(struct index_state *istate, 
const char *mmap, size_t mmap_size,
-   unsigned long src_offset, int nr_threads, struct 
index_entry_offset_table *ieot)
+   int nr_threads, struct index_entry_offset_table *ieot)
 {
int i, offset, ieot_blocks, ieot_start, err;
struct load_cache_entries_thread_data *data;
@@ -2198,7 +2198,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
ieot = read_ieot_extension(mmap, mmap_size, extension_offset);
 
if (ieot) {
-   src_offset += load_cache_entries_threaded(istate, mmap, 
mmap_size, src_offset, nr_threads, ieot);
+   load_cache_entries_threaded(istate, mmap, mmap_size, 
nr_threads, ieot);
free(ieot);
} else {
src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);

base-commit: f58b85df6937e3f3d9ef26bb52a513c8ada17ffc
-- 
2.18.0.windows.1

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

On Mon, Oct 22, 2018 at 3:38 PM Ben Peart  wrote:
>
> From: Ben Peart 
>
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
>
> Signed-off-by: Ben Peart 
> ---
>  Documentation/config.txt| 3 +++
>  Documentation/git-reset.txt | 4 +++-
>  builtin/reset.c | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
>
> +reset.quiet::
> +   When set to true, 'git reset' will default to the '--quiet' option.
> +

With 'nd/config-split' topic moving pretty much all config keys out of
config.txt, you probably want to do the same for this series: add this
in a new file called Documentation/reset-config.txt then include the
file here like the sendemail one below.

>  include::sendemail-config.txt[]
>
>  sequence.editor::
-- 
Duy

Re: [PATCH 00/59] Split config.txt

On Sun, Oct 21, 2018 at 1:29 AM Junio C Hamano  wrote:
>
> Ævar Arnfjörð Bjarmason  writes:
>
> > I had a slight bias against this when you started this, since I'm one of
> > these odd people who don't mind ~20k line files if the line count isn't
> > contributing to inherent complexity, e.g. in the case of config.txt you
> > could just use the search function all in one file.
>
> After typing "less Documentation/config.txt" and realizing that I
> have to open another file (which one?) to find how we described the
> push.default config, I am already experiencing a lot stronger bias
> against this.
>
> But I know it will pass.  Once this ~60 patch series completes, my
> irritation would peak, because at that point I would not be able to
> even do "git grep push.config Documentation/config*", but I would no
> longer be reaching for "less Documentation/config.txt" anymore at
> that point.  Once Documentation/$group-config.txt (which I think is
> a mistake) are renamed to Documentation/$something/$group.txt,

I'll wait for js/mingw-http-ssl to land, move http.* out then rename
them all to Documentation/config/$group.txt then. I'll fix up makefile
dependencies then too.

> then
> I know I can do "less Doc/$some/$gro" to get my ease
> of use back.  There will still be an annoyance caused by having to
> open another file when reading description of branch..merge in
> branch-config.txt and seeing a reference to push.default, though.
>
> And the end result makes it impossible to place a description of a
> new variable in a wrong section.  It still is possible to mistakenly
> insert a variable in a wrong place in the right section that
> requires a fix like 8578037b ("config.txt: reorder blame stuff to
> keep config keys sorted", 2018-08-04), but we do not fix all the
> problems under the sky in one series ;-).
>
> So after saying all of the above, I am moderately supportive of this
> series.
-- 
Duy

Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads

2018-10-22 Thread Ben Peart





On 10/21/2018 10:14 PM, Junio C Hamano wrote:

Jeff King  writes:


On Wed, Oct 10, 2018 at 11:59:38AM -0400, Ben Peart wrote:


+static unsigned long load_cache_entries_threaded(struct index_state *istate, 
const char *mmap, size_t mmap_size,
+   unsigned long src_offset, int nr_threads, struct 
index_entry_offset_table *ieot)


The src_offset parameter isn't used in this function.

In early versions of the series, it was used to feed the p->start_offset
field of each load_cache_entries_thread_data. But after the switch to
ieot, we don't, and instead feed p->ieot_start. But we always begin that
at 0.

Is that right (and we can drop the parameter), or should this logic:


+   offset = ieot_start = 0;
+   ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);
+   for (i = 0; i < nr_threads; i++) {
[...]


be starting at src_offset instead of 0?


I think "offset" has nothing to do with the offset into the mmapped
region of memory.  It is an integer index into a (virtual) array
that is a concatenation of ieot->entries[].entries[], and it is
correct to count from zero.  The value taken from that array using
the index is used to compute the offset into the mmapped region.

Unlike load_all_cache_entries() called from the other side of the
same if() statement in the same caller, this does not depend on the
fact that the first index entry in the mmapped region appears
immediately after the index-file header.  It goes from the offsets
into the file that are recorded in the entry offset table that is an
index extension, so the sizeof(*hdr) that initializes src_offset is
not used by the codepath.

The number of bytes consumed, i.e. its return value from the
function, is not really used, either, as the caller does not use
src_offset for anything other than updating it with "+=" and passing
it to this function (which does not use it) when it calls this
function (i.e. when ieot extension exists--and by definition when
that extension exists extension_offset is not 0, so we do not make
the final load_index_extensions() call in the caller that uses
src_offset).



Thanks for discovering/analyzing this.  You're right, I missed removing 
this when we switched from a single offset to an array of offsets via 
the IEOT.  I'll send a patch to fix both issues shortly.

Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options

On Mon, Oct 22, 2018 at 5:51 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > __gitcomp_builtin() has the main completion list provided by
> >
> > git xxx --git-completion-helper
> >
> > but the caller can also add extra options that is not provided by
> > --git-completion-helper. The only call site that does this is "git
> > difftool" completion.
> >
> > This support is broken by b221b5ab9b (completion: collapse extra
> > --no-.. options - 2018-06-06), which adds a special value "--" to mark
> > that the rest of the options can be hidden by default. The commit
> > forgets the fact that extra options are appended after
> > "$(git xxx --git-completion-helper)", i.e. after this "--", and will
> > be incorrectly hidden as well.
> >
> > Prepend the extra options before "$(git xxx --git-completion-helper)"
> > to avoid this.
>
> Thanks for a clear analysis.  How did you find it?  Got annoyed that
> completion of difftool got broken, or discovered while trying to
> apply the same trick as difftool completion uses to another one and
> seeing that the technique does not work?

I was fixing format-patch completion and was surprised it did not work
as expected. Never really used difftool myself :P I only found out
about it when I asked myself "why wasn't this breakage found earlier?"
-- 
Duy

Re: [PATCH v4 2/2] worktree: add per-worktree config files

On Mon, Oct 22, 2018 at 6:54 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 552827935a..244560a35e 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2,8 +2,9 @@ CONFIGURATION FILE
> >  --
> >
> >  The Git configuration file contains a number of variables that affect
> > -the Git commands' behavior. The `.git/config` file in each repository
> > -is used to store the configuration for that repository, and
> > +the Git commands' behavior. The files `.git/config` and optionally
> > +`config.worktree` (see `extensions.worktreeConfig` below) in each
> > +repository are used to store the configuration for that repository, and
> >  `$HOME/.gitconfig` is used to store a per-user configuration as
> >  fallback values for the `.git/config` file. The file `/etc/gitconfig`
> >  can be used to store a system-wide default configuration.
> > @@ -371,6 +372,13 @@ advice.*::
> >   editor input from the user.
> >  --
> >
> > +extensions.worktreeConfig::
> > + If set, by default "git config" reads from both "config" and
> > + "config.worktree" file from GIT_DIR in that order. In
> > + multiple working directory mode, "config" file is shared while
> > + "config.worktree" is per-working directory (i.e., it's in
> > + GIT_COMMON_DIR/worktrees//config.worktree)
> > +
>
> This obviously conflicts with your 59-patch series, but more
> importantly
>
>  - I notice that this is the only description of extensions.* key in
>the configuration files.  Don't we have any other extension
>defined, and if so shouldn't we be describing them already (not
>as a part of this series, obviously)?

Right. We have two extensions already but it's described in
technical/repository-format.txt. I'll move this extension there
because it's written "This document will serve as the master list for
extensions." in that document.

>  - If we are going to describe other extensions.* keys, do we want
>extensions-config.txt file to split this (and others) out and
>later rename it to config/extensions.txt?  Or do we want to
>collect related things together by logically not by name and have
>this extension described in config/worktree.txt instead, perhaps
>separate from other extensions.* keys?

I think we would go with config/extensions.txt because if grouping
logically, I'm not sure where extensions.preciousObjects and
extensions.partialClone would go.
-- 
Duy

Re: [PATCH] completion: use __gitcomp_builtin for format-patch