[PATCH] compat: make sure git_mmap is not expected to write
in f48000fc ("Yank writing-back support from gitfakemmap.", 2005-10-08) support for writting back changes was removed but the specific prot flag that would be used was not checked for) Signed-off-by: Carlo Marcelo Arenas Belón --- compat/mmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mmap.c b/compat/mmap.c index 7f662fef7b..14d31010df 100644 --- a/compat/mmap.c +++ b/compat/mmap.c @@ -4,7 +4,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of { size_t n = 0; - if (start != NULL || !(flags & MAP_PRIVATE)) + if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ) die("Invalid usage of mmap when built with NO_MMAP"); start = xmalloc(length); -- 2.19.1
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
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
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
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
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
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
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()
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
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
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
> -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
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
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
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
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 builtin/prune.c | 2 +- builtin/repack.c | 6 ++ commit.h | 2 +- shallow.c| 22 +- t/t5537-fetch-shallow.sh | 27 +++ 5 files changed, 52 insertions(+), 7 deletions(-) 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
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
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)
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
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
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
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
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
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
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
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
On Mon, Oct 22, 2018 at 12:17 PM SZEDER Gábor wrote: > > On Sun, Oct 21, 2018 at 10:41:02AM +0200, Nguyễn Thái Ngọc Duy wrote: > > This helps format-patch gain completion for a couple new options, > > notably --range-diff. > > > > Signed-off-by: Nguyễn Thái Ngọc Duy > > --- > > Of course it will be even better if I could complete the ref for > > --range-diff=, but maybe another day. > > > > contrib/completion/git-completion.bash | 10 +++--- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/contrib/completion/git-completion.bash > > b/contrib/completion/git-completion.bash > > index c8fdcf8644..065b922777 100644 > > --- a/contrib/completion/git-completion.bash > > +++ b/contrib/completion/git-completion.bash > > @@ -1533,12 +1533,8 @@ _git_fetch () > > } > > > > __git_format_patch_options=" > > - --stdout --attach --no-attach --thread --thread= --no-thread > > - --numbered --start-number --numbered-files --keep-subject --signoff > > - --signature --no-signature --in-reply-to= --cc= --full-index --binary > > - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > > - --inline --suffix= --ignore-if-in-upstream --subject-prefix= > > - --output-directory --reroll-count --to= --quiet --notes > > + --full-index --not --all --no-prefix --src-prefix= > > + --dst-prefix= --notes > > " > > $__git_format_patch_options is also used when completing 'git > send-email's options, thus removing all these options will badly > affect that, and in fact makes 't9902-completion.sh' fail with: Oops. I guess I was excited about the other fix and forgot to test this patch. Junio please kick it out of 'pu'. I'll need to think if I could somehow make send-email work without hardcoding a bunch of options in it.,, -- Duy