[PATCH v2] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

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

tree_entry_interesting() is used for matching pathspec on a tree. The
interesting thing about this function is that, because the tree
entries are known to be sorted, this function can return more than
just "yes, matched" and "no, not matched". It can also say "yes, this
entry is matched and so is the remaining entries in the tree".

This is where I made a mistake when matching exclude pathspec. For
exclude pathspec, we do matching twice, one with positive patterns and
one with negative ones, then a rule table is applied to determine the
final "include or exclude" result. Note that "matched" does not
necessarily mean include. For negative patterns, "matched" means
exclude.

This particular rule is too eager to include everything. Rule 8 says
that "if all entries are positively matched" and the current entry is
not negatively matched (i.e. not excluded), then all entries are
positively matched and therefore included. But this is not true. If
the _current_ entry is not negatively matched, it does not mean the
next one will not be and we cannot conclude right away that all
remaining entries are positively matched and can be included.

Rules 8 and 18 are now updated to be less eager. We conclude that the
current entry is positively matched and included. But we say nothing
about remaining entries. tree_entry_interesting() will be called again
for those entries where we will determine entries individually.

Reported-by: Christophe Bliard 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 v2 fixes the too broad "git add ." in the test

 t/t6132-pathspec-exclude.sh | 17 +
 tree-walk.c | 11 ---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index eb829fce97..2462b19ddd 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' '
test_cmp expect actual
 '
 
+test_expect_success 't_e_i() exclude case #8' '
+   git init case8 &&
+   (
+   cd case8 &&
+   echo file >file1 &&
+   echo file >file2 &&
+   git add file1 file2 &&
+   git commit -m twofiles &&
+   git grep -l file HEAD :^file2 >actual &&
+   echo HEAD:file1 >expected &&
+   test_cmp expected actual &&
+   git grep -l file HEAD :^file1 >actual &&
+   echo HEAD:file2 >expected &&
+   test_cmp expected actual
+   )
+'
+
 test_done
diff --git a/tree-walk.c b/tree-walk.c
index 77b37f36fa..79bafbd1a2 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1107,7 +1107,7 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 *   5  |  file |1 |1 |   0
 *   6  |  file |1 |2 |   0
 *   7  |  file |2 |   -1 |   2
-*   8  |  file |2 |0 |   2
+*   8  |  file |2 |0 |   1
 *   9  |  file |2 |1 |   0
 *  10  |  file |2 |2 |  -1
 * -+---+--+--+---
@@ -1118,7 +1118,7 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 *  15  |  dir  |1 |1 |   1 (*)
 *  16  |  dir  |1 |2 |   0
 *  17  |  dir  |2 |   -1 |   2
-*  18  |  dir  |2 |0 |   2
+*  18  |  dir  |2 |0 |   1
 *  19  |  dir  |2 |1 |   1 (*)
 *  20  |  dir  |2 |2 |  -1
 *
@@ -1134,7 +1134,12 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 
negative = do_match(entry, base, base_offset, ps, 1);
 
-   /* #3, #4, #7, #8, #13, #14, #17, #18 */
+   /* #8, #18 */
+   if (positive == all_entries_interesting &&
+   negative == entry_not_interesting)
+   return entry_interesting;
+
+   /* #3, #4, #7, #13, #14, #17 */
if (negative <= entry_not_interesting)
return positive;
 
-- 
2.19.1.1005.gac84295441

Re: [PATCH v4 2/5] am: improve author-script error reporting

[Eric Sunshine]

On Wed, Oct 31, 2018 at 6:16 AM Phillip Wood  wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)
> +   int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
> @@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)
> +   for (i = 0; i < kv.nr; i++) {
> +   if (!strcmp(kv.items[i].string, "GIT_AUTHOR_NAME")) {
> +   if (name_i >= 0)
> +   name_i = error(_("'GIT_AUTHOR_NAME' already 
> given"));
> +   else
> +   name_i = i;
> +   }
> +   ...
> +   }
> +   if (name_i == -2)
> +   error(_("missing 'GIT_AUTHOR_NAME'"));
> +   ...
> +   if (date_i < 0 || email_i < 0 || date_i < 0 || err)
> goto finish;
> +   state->author_name = kv.items[name_i].util;
> +   ...
> retval = 0;
>  finish:
> string_list_clear(, !!retval);

Junio labeled the "-2" trick "cute", and while it is optimal in that
it only traverses the key/value list once, it also increases cognitive
load since the reader has to spend a good deal more brain cycles
understanding what is going on than would be the case with simpler
(and less noisily repetitive) code.

An alternative would be to make the code trivially easy to understand,
though a bit more costly, but that expense should be negligible since
this file should always be tiny, containing very few key/value pairs,
and it's not timing critical code anyhow. For instance:

static char *find(struct string_list *kv, const char *key)
{
const char *val = NULL;
int i;
for (i = 0; i < kv.nr; i++) {
if (!strcmp(kv.items[i].string, key)) {
if (val) {
error(_("duplicate %s"), key);
return NULL;
}
val = kv.items[i].util;
}
}
if (!val)
error(_("missing %s"), key);
return val;
}

static int read_author_script(struct am_state *state)
{
...
char *name, *email, *date;
...
name = find(, "GIT_AUTHOR_NAME");
email = find(, "GIT_AUTHOR_EMAIL");
date = find(, "GIT_AUTHOR_DATE");
if (name && email && date) {
state->author_name = name;
state->author_email = email;
state->author_date = date;
retval = 0;
}
string_list_clear, !!retval);
...

Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean

[Jeff King]

On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:

> introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
> but included on all previous versions as well.
> 
> midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
> 
> likely an oversight as the information needed to iterate over is
> embedded in nr_large_offset

I've been preparing a series to make the whole code base compile with
-Wunused-parameter, and I handled this case a bit differently.

-- >8 --
Subject: [PATCH] midx: double-check large object write loop

The write_midx_large_offsets() function takes an array of object
entries, the number of entries in the array (nr_objects), and the number
of entries with large offsets (nr_large_offset). But we never actually
use nr_objects; instead we keep walking down the array and counting down
nr_large_offset until we've seen all of the large entries.

This is correct, but we can be a bit more defensive. If there were ever
a mismatch between nr_large_offset and the actual set of large-offset
objects, we'd walk off the end of the array.

Since we know the size of the array, we can use nr_objects to make sure
we don't walk too far.

Signed-off-by: Jeff King 
---
 midx.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/midx.c b/midx.c
index 4fac0cd08a..ecd583666a 100644
--- a/midx.c
+++ b/midx.c
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile 
*f, int large_offset_nee
 static size_t write_midx_large_offsets(struct hashfile *f, uint32_t 
nr_large_offset,
   struct pack_midx_entry *objects, 
uint32_t nr_objects)
 {
-   struct pack_midx_entry *list = objects;
+   struct pack_midx_entry *list = objects, *end = objects + nr_objects;
size_t written = 0;
 
while (nr_large_offset) {
-   struct pack_midx_entry *obj = list++;
-   uint64_t offset = obj->offset;
+   struct pack_midx_entry *obj;
+   uint64_t offset;
+
+   if (list >= end)
+   BUG("too many large-offset objects");
+
+   obj = list++;
+   offset = obj->offset;
 
if (!(offset >> 31))
continue;
-- 
2.19.1.1352.g60f3b1a4c2

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

[Eric Sunshine]

'sb/filenames-with-dashes'On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð
Bjarmason  wrote:
> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
>
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  install_programs | 89 

Pure nitpick: Earlier this year, Stefan made an effort[1] to eradicate
filenames with underscores and replace them with hyphenated filenames.
Perhaps name this "install-programs", instead.

[1]: sb/filenames-with-dashes

> diff --git a/install_programs b/install_programs
> @@ -0,0 +1,89 @@
> +while test $# != 0
> +do
> +   case "$1" in
> +   --X=*)
> +   X="${1#--X=}"
> +   ;;
> +   --RM=*)
> +   RM="${1#--RM=}"
> +   ;;
> +   --bindir=*)
> +   bindir="${1#--bindir=}"
> +   ;;

Is the intention that the user might have X, RM, 'bindir', etc.
already in the environment, and the switches in this script merely
override those values? Or is the intention that X, RM, 'bindir, etc.
should all start out unset? If the latter, perhaps start the script
with an initialization block which clears all these variables first:

X=
RM=
bindir=
...

Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag

[Eric Sunshine]

On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason  wrote:
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Makefile b/Makefile
> @@ -346,6 +346,13 @@ all::
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to

s/returns/return/
s/something/&,/

Although, it's not clear what "return something" means. Perhaps rephrase it as:

   ...git-init is expected to exist, set this.

> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#

Re: [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch

[Eric Sunshine]

On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason  wrote:
> Add a switch for use in conjunction with the INSTALL_SYMLINKS flag
> added in ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13).
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Makefile b/Makefile
> @@ -342,6 +342,10 @@ all::
> +# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with

s/if in/in/

> +# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> +# fallack on hardlinking or copying if "ln -s" fails.
> +#

Hello

[Joshua Mene]

Good day I am Dr. Joshua Mene a banker here in Cotonu Benin Republic, I have a 
lucrative business transaction for you and it is profitable and risk free. 
kindly reach me via my private email: me...@rediffmail.com for more details.

Thanks
Dr. Joshua Mene

[PATCH] multi-pack-index: make code -Wunused-parameter clean

[Carlo Marcelo Arenas Belón]

introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
but included on all previous versions as well.

midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]

likely an oversight as the information needed to iterate over is
embedded in nr_large_offset

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 midx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/midx.c b/midx.c
index 4fac0cd08a..a2c17e3108 100644
--- a/midx.c
+++ b/midx.c
@@ -710,7 +710,7 @@ static size_t write_midx_object_offsets(struct hashfile *f, 
int large_offset_nee
 }
 
 static size_t write_midx_large_offsets(struct hashfile *f, uint32_t 
nr_large_offset,
-  struct pack_midx_entry *objects, 
uint32_t nr_objects)
+  struct pack_midx_entry *objects)
 {
struct pack_midx_entry *list = objects;
size_t written = 0;
@@ -880,7 +880,7 @@ int write_midx_file(const char *object_dir)
break;
 
case MIDX_CHUNKID_LARGEOFFSETS:
-   written += write_midx_large_offsets(f, 
num_large_offsets, entries, nr_entries);
+   written += write_midx_large_offsets(f, 
num_large_offsets, entries);
break;
 
default:
-- 
2.19.1.816.gcd69ec8cd

Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

[Eric Sunshine]

On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy  wrote:
> Rules 8 and 18 are now updated to be less eager. We conclude that the
> current entry is positively matched and included. But we say nothing
> about remaining entries. tree_entry_interesting() will be called again
> for those entries where we will determine entries individually.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> @@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' '
> +test_expect_success 't_e_i() exclude case #8' '
> +   git init case8 &&
> +   (
> +   cd case8 &&
> +   echo file >file1 &&
> +   echo file >file2 &&
> +   git add . &&

Won't this loose git-add invocation end up adding all the junk files
from earlier tests? One might have expected to see the more restricted
invocation:

git add file1 file2 &&

to make it easier to reason about the test and not worry about someone
later inserting tests above this one which might interfere with it.

> +   git commit -m twofiles &&
> +   git grep -l file HEAD :^file2 >actual &&
> +   echo HEAD:file1 >expected &&
> +   test_cmp expected actual &&
> +   git grep -l file HEAD :^file1 >actual &&
> +   echo HEAD:file2 >expected &&
> +   test_cmp expected actual
> +   )
> +'

Re: Git Slowness on Windows w/o Internet

[Philip Oakley]

On 03/11/2018 16:44, brian m. carlson wrote:

On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote:

Wanted to bring to your attention an issue that we discovered on our
Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
servers don't have Internet access. It appears that git.exe is trying
to connect to various Cloudflare and Akamai CDN instances over the
Internet when it first runs and it keeps trying to connect to these
CDNs every git.exe execution until it makes a successful attempt. See
the screenshot attached with the details.

Enabling Internet access via proxy fixes the issue and git.exe
continues to work fast on the next attempts to run git.exe

Is there any configuration setting that can disable this git's
behavior or is there any other workaround without allowing Internet
access? Otherwise, every git command run on a server without the
Internet takes about 30 seconds to complete.


Git itself doesn't make any attempt to access those systems unless it's
configured to do so (e.g. a remote is set up to talk to those systems
and fetch or pull is used).

It's possible that you're using a distribution package that performs
this behavior, say, to check for updates.  I'd recommend that you
contact the distributor, which in this case might be Git for Windows,
and see if they can tell you more about what's going on.  The URL for
that project is at https://github.com/git-for-windows/git.



The normal Git for Windows install includes an option to check for 
updates at a suitable rate. Maybe you are hitting that. It can be 
switched off.


--
Philip

[RFC PATCH] checkout: add synonym of -b

[Mikkel Kjeldsen]

Add --new-branch as a long-form synonym of -b. I occasionally encounter
some confusion in new users having interpreted "checkout -b" to mean
"checkout branch", or internalized it as "the way to create a new
branch" rather than merely a convenience for "branch && checkout". I
think an explicit long-form can help alleviate that.

Signed-off-by: Mikkel Kjeldsen 
---

Notes:
This makes the synopsis and description lines look a little clumsy (and
I think incorrect...?) so if this proposal is accepted perhaps those
parts are better left out. It is meant more for training and
documentation than regular usage, anyway.

I thought I had seen something like "--create-branch" in use by another
command and had intended to use that but I can no longer find that and
so went with "--new-branch" named after the option's argument.

There does not seem to be a practice for testing short- versus long-form
arguments so I did not include one, but I'd be happy to.

 Documentation/git-checkout.txt | 5 +++--
 builtin/checkout.c | 2 +-
 t/t9902-completion.sh  | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 801de2f764..7651d8b83d 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] []
 'git checkout' [-q] [-f] [-m] --detach []
 'git checkout' [-q] [-f] [-m] [--detach] 
-'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] ] []
+'git checkout' [-q] [-f] [-m] [[(-b|--new-branch)|-B|--orphan] ] 
[]
 'git checkout' 

Re: [PATCH v3] commit: add a commit.allowEmpty config variable

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 03 2018, tanushree27 wrote:

> +commit.allowEmpty::
> + A boolean to specify whether empty commits are allowed with `git
> + commit`. See linkgit:git-commit[1].
> + Defaults to false.
> +

Good.

> + if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in 
> config*/
> + allow_empty = config_commit_allow_empty;
> +

This works, but != -1 is our usual idiom for this as you initialize it
to -1. I think that comment can also go then, since it's clear what's
going on.

> +# Tests for commit.allowEmpty config
> +
> +test_expect_success "no commit.allowEmpty and no --allow-empty" "
> + test_must_fail git commit -m 'test'
> +"
> +
> +test_expect_success "no commit.allowEmpty and --allow-empty" "
> + git commit --allow-empty -m 'test'
> +"
> +
> +for i in true 1
> +do
> + test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
> + git -c commit.allowEmpty=$i commit -m 'test'
> + "
> +
> + test_expect_success "commit.allowEmpty=$i and --allow-empty" "
> + git -c commit.allowEmpty=$i commit --allow-empty -m 'test'
> + "
> +done
> +
> +for i in false 0
> +do
> + test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
> + test_must_fail git -C commit.allowEmpty=$i commit -m 'test'
> + "
> +
> + test_expect_success "commit.allowEmpty=$i and --allow-empty" "
> + test_must_fail git -c commit.allowEmpty=$i commit --allow-empty 
> -m 'test'
> + "
> +done

Testing both 1 and "true" can be dropped here. Things that use
git_config_bool() can just assume it works, we test it more exhaustively
elsewhere.

Your patch has whitespace errors. Try with "git show --check" or apply
it with git-am, it also doesn't apply cleanly on the latest master.

But on this patch in general: I don't mind making this configurable, but
neither your commit message nor these tests make it clear what the
actual motivation is, which can be seen on the upstream GitHub bug
report.

I.e. you seemingly have no interest in using "git commit" to produce
empty commits, but are just trying to cherry-pick something and it's
failing because it (presumably, or am I missing something) cherry picks
an existing commit content ends up not changing anything.

I.e. you'd like to make the logic 37f7a85793 ("Teach commit about
CHERRY_PICK_HEAD", 2011-02-19) added a message for the default.

So let's talk about that use case, and for those of us less familiar
with this explain why it is that this needs to still be optional at
all. I.e. aren't we just exposing an implementation detail here where
cherry-pick uses the commit machinery? Should we maybe just always pass
--allow-empty on cherry-pick, if not why not?

I can think of some reasons, but the above is a hint that both this
patch + the current documentation which talks about "foreign SCM
scripts" have drifted very far from what this is actually being used
for, so let's update that.

Re: git appears to ignore GIT_CONFIG environment variable

[Sirio Balmelli]

Thank you very much, I appreciate the answer :)

best,

Sirio

> On Nov 2, 2018, at 04:07, Junio C Hamano  wrote:
> 
> Sirio Balmelli  writes:
> 
>> It appears that git ignores the GIT_CONFIG environment variable,
>> while git-config *does* consider it.
> 
> Yup, that is exactly how it is designed and documented.  These
> dasys, with "git config" taking "--file" to work on any arbitrary
> filename, you do not necessarily need GIT_CONFIG enviornment.
> 



signature.asc
Description: Message signed with OpenPGP

Re: [RFC] Generation Number v2

[Jakub Narebski]

Jakub Narebski  writes:
> Jakub Narebski  writes:
>> Stefan Beller  writes:
> [...]
>>> How would this impact creation of a commit?
>>>
>>> The current generation numbers can be lazily updated or not
>>> updated at all. In my understanding of the maximum generation
>>> numbers, a new commit would make these maximum generation
>>> numbers invalid (i.e. they have to be recomputed).
> [...]
>>> For the V2 maximum generation numbers, would we need to
>>> rewrite the numbers for all commits once we recompute them?
>>> Assuming that is true, it sounds like the benchmark doesn't
>>> cover the whole costs associated with V2, which is why the
>>> exceptional performance can be explained.
>>
>> Let's check it using a simple example
>>
>> First, (minimum) parent-based generation numbers before and after
>> extending the commit graph:
>>
>>   1   2 3   4 5   6   7new
>>   1   2 3   4 5   -   -old
>>   .---.-.---.-.---*---*
>>\
>> \   3   4 5   6new
>>  \  3   4 5   6old
>>   \-.---.-.---.
>>  \
>>   \   5new
>>\  -old
>> \-*
>
> Let me check yet another idea, using (minimum) parent-based V0 generation
> numbers (counting distance from the sink / root) as a starting number
> for source / heads commits.
[...]

> [...] but let's check another example
>
>1   2 3   4   5   6 7   8   9   new
>1   2 3   4   5   6 7   8   -   old
>.---.-.---.---.---.-.---.---*
> \ /
>  \   3   4   / 5   6   7   8   new
>   \  5   6  /  -   -   -   -   old
>\-.---.-/---*---*---*---*

But let's do this correctly.


   1   2 3   4  5   6 7   8   9  new
   1   2 3   4  5   6 7   8   -  old
   .---.-.---.--.---.-.---.---*
\/
 \   3   4  /new
  \  5   6 / old
   \-.---./
  \
   \5   6 7   8  new
\   -   - -   -  old
 \--*---*-*---*

Well, it looks as if I draw it incorrectly, but performed calculations
right.  You may need to modify / change some data, but it looks as if it
is not that much of a problem.

The new version of the maximum generation numbers looks like it gives
the same results as generation numbers for the "longest" path, and
update may affect only the side-branches that were added to.  All
branches merged into the trunk, and not added to should be safe with
respect to updating.

Can anyone here prove a thing about update of those modified maximum
generation numbers?  Thanks in advance.

Best,
-- 
Jakub Narębski

Re: Git Slowness on Windows w/o Internet

[brian m. carlson]

On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote:
> Wanted to bring to your attention an issue that we discovered on our
> Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
> servers don't have Internet access. It appears that git.exe is trying
> to connect to various Cloudflare and Akamai CDN instances over the
> Internet when it first runs and it keeps trying to connect to these
> CDNs every git.exe execution until it makes a successful attempt. See
> the screenshot attached with the details.
> 
> Enabling Internet access via proxy fixes the issue and git.exe
> continues to work fast on the next attempts to run git.exe
> 
> Is there any configuration setting that can disable this git's
> behavior or is there any other workaround without allowing Internet
> access? Otherwise, every git command run on a server without the
> Internet takes about 30 seconds to complete.

Git itself doesn't make any attempt to access those systems unless it's
configured to do so (e.g. a remote is set up to talk to those systems
and fetch or pull is used).

It's possible that you're using a distribution package that performs
this behavior, say, to check for updates.  I'd recommend that you
contact the distributor, which in this case might be Git for Windows,
and see if they can tell you more about what's going on.  The URL for
that project is at https://github.com/git-for-windows/git.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Git Test Coverage Report (Friday, Nov 2)

[Michał Górny]

On Sat, 2018-11-03 at 19:03 +0900, Junio C Hamano wrote:
> Michał Górny  writes:
> 
> > As for how involved... we'd just have to use a key that has split
> > signing subkey.  Would it be fine to add the subkey to the existing key?
> >  It would imply updating keyids/fingerprints everywhere.
> 
> Yes, that "everywhere" is exactly what I meant by "how involved",
> and your suggestion answers "very much involved".
> 
> If we can easily add _another_ key with a subkey that is not the
> primary one we use for other tests, without touching the existing
> key and the existing tests that use it (including the one I touched
> below--- we'd want to see a sig with a key that is not split is
> shown with the same %GF and %GP), while adding a handful of new
> tests that create signed objects under the new & split key and 
> view them with %GF and %GP, then the end result would be that we
> managed to add a new test case where %GF/%GP are different without
> making very much involved changes.  I guess that was what I was
> getting at.
> 

I've just did a little research and came to the following results:

1. modifying the 'C. O. Mitter' key would require changes to 4 tests,

2. modifying the 'Eris Discordia' key would require changes to 2 tests
   (both in 7510).

Do you think 2. would be an acceptable option?  I think changing 2 tests
would be preferable to proliferating a third key for one test case. 
Also, given that both failing tests are specifically format string
tests, one of them would serve additional purpose of testing %GP!=%GF.

-- 
Best regards,
Michał Górny


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

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

[Michał Górny]

On Sat, 2018-11-03 at 16:36 +0100, Duy Nguyen wrote:
> On Sat, Nov 3, 2018 at 4:32 PM Michał Górny  wrote:
> > > Perhaps my gpg is too old?
> > > 
> > > $ gpg --version
> > > gpg (GnuPG) 2.1.15
> > > libgcrypt 1.7.3
> > > Copyright (C) 2016 Free Software Foundation, Inc.
> > > License GPLv3+: GNU GPL version 3 or later 
> > > 
> > > This is free software: you are free to change and redistribute it.
> > > There is NO WARRANTY, to the extent permitted by law.
> > > 
> > > Home: /home/pclouds/.gnupg
> > > Supported algorithms:
> > > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
> > > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> > > CAMELLIA128, CAMELLIA192, CAMELLIA256
> > > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> > > Compression: Uncompressed, ZIP, ZLIB, BZIP2
> > 
> > Perhaps this is indeed specific to this version of GnuPG.  The tests
> > pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
> > anymore.
> 
> Yeah I have not really used gpg and neglected updating it. Will try it
> now. The question remains though whether we need to support 2.1* (I
> don't know at all about gnupg status, maybe 2.1* is indeed too
> old/buggy that nobody should use it and so we don't need to support
> it).

GnuPG upstream considers 2.2 as continuation/mature version of 2.1
branch.  They currently support running either newest version of 1.4
(legacy) or newest version of 2.2 [1].  In other words, this might have
been a bug that was fixed in newer release (possibly 2.2.x).

[1]:https://gnupg.org/download/index.html#text-end-of-life

-- 
Best regards,
Michał Górny


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

Change your password callgsm01 immediately. Your account has been hacked.

[git]

I greet you!

I have bad news for you.
27/08/2018 - on this day I hacked your operating system and got full access to 
your account git@vger.kernel.org
On that day your account (git@vger.kernel.org) password was: callgsm01

It is useless to change the password, my malware intercepts it every time.

How it was:
In the software of the router to which you were connected that day, there was a 
vulnerability.
I first hacked this router and placed my malicious code on it.
When you entered in the Internet, my trojan was installed on the operating 
system of your device.

After that, I made a full dump of your disk (I have all your address book, 
history of viewing sites, all files, phone numbers and addresses of all your 
contacts).

A month ago, I wanted to lock your device and ask for a small amount of money 
to unlock.
But I looked at the sites that you regularly visit, and came to the big delight 
of your favorite resources.
I'm talking about sites for adults.

I want to say - you are a big pervert. You have unbridled fantasy!

After that, an idea came to my mind.
I made a screenshot of the intimate website where you have fun (you know what 
it is about, right?).
After that, I took off your joys (using the camera of your device). It turned 
out beautifully, do not hesitate.

I am strongly belive that you would not like to show these pictures to your 
relatives, friends or colleagues.
I think $956 is a very small amount for my silence.
Besides, I spent a lot of time on you!

I accept money only in Bitcoins.
My BTC wallet: 1LwibmKAKu4kt4SvRLYdUP3aW7vL3Y78zL

You do not know how to replenish a Bitcoin wallet?
In any search engine write "how to send money to btc wallet".
It's easier than send money to a credit card!

For payment you have a little more than two days (exactly 50 hours).
Do not worry, the timer will start at the moment when you open this letter. 
Yes, yes .. it has already started!

After payment, my virus and dirty photos with you self-destruct automatically.
Narrative, if I do not receive the specified amount from you, then your device will be 
blocked, and all your contacts will receive a photos with your "joys".

I want you to be prudent.
- Do not try to find and destroy my virus! (All your data is already uploaded 
to a remote server)
- Do not try to contact me (this is not feasible, I sent you an email from your 
account)
- Various security services will not help you; formatting a disk or destroying 
a device will not help either, since your data is already on a remote server.

P.S. I guarantee you that I will not disturb you again after payment, as you 
are not my single victim.
This is a hacker code of honor.


From now on, I advise you to use good antiviruses and update them regularly 
(several times a day)!


Don't be mad at me, everyone has their own work.
Farewell.

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

[Duy Nguyen]

On Sat, Nov 3, 2018 at 4:32 PM Michał Górny  wrote:
> Perhaps this is indeed specific to this version of GnuPG.  The tests
> pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
> anymore.

Updated to 2.2.8 and the test is passed.
-- 
Duy

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

[Duy Nguyen]

On Sat, Nov 3, 2018 at 4:32 PM Michał Górny  wrote:
> > Perhaps my gpg is too old?
> >
> > $ gpg --version
> > gpg (GnuPG) 2.1.15
> > libgcrypt 1.7.3
> > Copyright (C) 2016 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later 
> > 
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.
> >
> > Home: /home/pclouds/.gnupg
> > Supported algorithms:
> > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
> > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> > CAMELLIA128, CAMELLIA192, CAMELLIA256
> > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> > Compression: Uncompressed, ZIP, ZLIB, BZIP2
>
> Perhaps this is indeed specific to this version of GnuPG.  The tests
> pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
> anymore.

Yeah I have not really used gpg and neglected updating it. Will try it
now. The question remains though whether we need to support 2.1* (I
don't know at all about gnupg status, maybe 2.1* is indeed too
old/buggy that nobody should use it and so we don't need to support
it).
-- 
Duy

Re: Failed stash caused untracked changes to be lost

[Thomas Gummerer]

On 10/23, Quinn, David wrote:
> 
> Issue: While running a git stash command including the '-u' flag to include 
> untracked files, the command failed due to arguments in the incorrect order. 
> After this untracked files the were present had been removed and permanently 
> lost.

Thanks for your report (and sorry for the late reply)!

I believe this (somewhat) fixed in 833622a945 ("stash push: avoid
printing errors", 2018-03-19), which was first included in Git 2.18.
Your message doesn't state which version of Git you encountered the
bug, but I'm going to assume with something below 2.18 (For future
reference, please include the version of Git in bug reports, or even
better test with the latest version of Git, as the bug may have been
fixed in the meantime).

Now I'm saying somewhat fixed above, because we still create an stash
if a pathspec that doesn't match any files is passed to the command,
but then don't remove anything from the working tree, which is a bit
confusing.

I think the right solution here would be to error out early if we were
given a pathspec that doesn't match anything.  I'll look into that,
unless you're interested in giving it a try? :)

> Environment: Windows 10, Powershell w/ PoshGit
> 
> 
> State before running command: 9 Modified files, 2 (new) untracked files
> 
> Note: I only wanted to commit some of the modified files (essentially all the 
> files/changes I wanted to commit were in one directory)
> 
> Actual command run:  git stash push -u -- Directory/To/Files/* -m "My Message"
> 
> Returned:
> 
> Saved working directory and index state WIP on [BranchName]: [Commit 
> hash] [Commit Message]
> fatal: pathspec '-m' did not match any files
> error: unrecognized input
> 
> State after Command ran: 9 Modifed files, 0 untracked files
> 
> 
> The command I should have ran should have been
> 
> git stash push -u -m "My Message"? -- Directory/To/Files/*
> 
> 
> I have found the stash that was created by running this command:
> 
> gitk --all $(git fsck --no-reflog | Select-String "(dangling commit 
> )(.*)" | %{ $_.Line.Split(' ')[2] })
> ?
> and searching for the commit number that was returned from the original 
> (paritally failed??) stash command. However there is nothing in that stash. 
> It is empty.
> 
> 
> 
> I think that the fact my untracked files were lost is not correct behaviour 
> and hence why I'm filing this bug report
> 
> 
> 
> 
> 
> NOTICE: This message, and any attachments, are for the intended recipient(s) 
> only, may contain information that is privileged, confidential and/or 
> proprietary and subject to important terms and conditions available at 
> E-Communication 
> Disclaimer.
>  If you are not the intended recipient, please delete this message. CME Group 
> and its subsidiaries reserve the right to monitor all email communications 
> that occur on CME Group information systems.

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

[Michał Górny]

On Sat, 2018-11-03 at 16:17 +0100, Duy Nguyen wrote:
> On Sat, Oct 20, 2018 at 9:31 PM Michał Górny  wrote:
> > +test_expect_success GPG 'detect fudged commit with double signature' '
> > +   sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
> > +   sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
> > +   sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor 
> > >double-sig1.sig &&
> > +   gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&
> > +   cat double-sig1.sig double-sig2.sig | gpg --enarmor 
> > >double-combined.asc &&
> > +   sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ 
> > /" \
> > +   double-combined.asc > double-gpgsig &&
> > +   sed -e "/committer/r double-gpgsig" double-base >double-commit &&
> > +   git hash-object -w -t commit double-commit >double-commit.commit &&
> > +   test_must_fail git verify-commit $(cat double-commit.commit) &&
> > +   git show --pretty=short --show-signature $(cat 
> > double-commit.commit) >double-actual &&
> > +   grep "BAD signature from" double-actual &&
> > +   grep "Good signature from" double-actual
> > +'
> 
> This test fails on 'master' today for me
> 
> gpg: WARNING: multiple signatures detected.  Only the first will be checked.
> gpg: Signature made Sat Nov  3 15:13:28 2018 UTC
> gpg:using DSA key 13B6F51ECDDE430D
> gpg:issuer "commit...@example.com"
> gpg: BAD signature from "C O Mitter " [ultimate]
> gpg: BAD signature from "C O Mitter " [ultimate]
> not ok 16 - detect fudged commit with double signature
> 
> Perhaps my gpg is too old?
> 
> $ gpg --version
> gpg (GnuPG) 2.1.15
> libgcrypt 1.7.3
> Copyright (C) 2016 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
> 
> Home: /home/pclouds/.gnupg
> Supported algorithms:
> Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
> Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> CAMELLIA128, CAMELLIA192, CAMELLIA256
> Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> Compression: Uncompressed, ZIP, ZLIB, BZIP2

Perhaps this is indeed specific to this version of GnuPG.  The tests
pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
anymore.

-- 
Best regards,
Michał Górny


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

[PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

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

tree_entry_interesting() is used for matching pathspec on a tree. The
interesting thing about this function is that, because the tree
entries are known to be sorted, this function can return more than
just "yes, matched" and "no, not matched". It can also say "yes, this
entry is matched and so is the remaining entries in the tree".

This is where I made a mistake when matching exclude pathspec. For
exclude pathspec, we do matching twice, one with positive patterns and
one with negative ones, then a rule table is applied to determine the
final "include or exclude" result. Note that "matched" does not
necessarily mean include. For negative patterns, "matched" means
exclude.

This particular rule is too eager to include everything. Rule 8 says
that "if all entries are positively matched" and the current entry is
not negatively matched (i.e. not excluded), then all entries are
positively matched and therefore included. But this is not true. If
the _current_ entry is not negatively matched, it does not mean the
next one will not be and we cannot conclude right away that all
remaining entries are positively matched and can be included.

Rules 8 and 18 are now updated to be less eager. We conclude that the
current entry is positively matched and included. But we say nothing
about remaining entries. tree_entry_interesting() will be called again
for those entries where we will determine entries individually.

Reported-by: Christophe Bliard 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t6132-pathspec-exclude.sh | 17 +
 tree-walk.c | 11 ---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index eb829fce97..393b29f205 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' '
test_cmp expect actual
 '
 
+test_expect_success 't_e_i() exclude case #8' '
+   git init case8 &&
+   (
+   cd case8 &&
+   echo file >file1 &&
+   echo file >file2 &&
+   git add . &&
+   git commit -m twofiles &&
+   git grep -l file HEAD :^file2 >actual &&
+   echo HEAD:file1 >expected &&
+   test_cmp expected actual &&
+   git grep -l file HEAD :^file1 >actual &&
+   echo HEAD:file2 >expected &&
+   test_cmp expected actual
+   )
+'
+
 test_done
diff --git a/tree-walk.c b/tree-walk.c
index 77b37f36fa..79bafbd1a2 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1107,7 +1107,7 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 *   5  |  file |1 |1 |   0
 *   6  |  file |1 |2 |   0
 *   7  |  file |2 |   -1 |   2
-*   8  |  file |2 |0 |   2
+*   8  |  file |2 |0 |   1
 *   9  |  file |2 |1 |   0
 *  10  |  file |2 |2 |  -1
 * -+---+--+--+---
@@ -1118,7 +1118,7 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 *  15  |  dir  |1 |1 |   1 (*)
 *  16  |  dir  |1 |2 |   0
 *  17  |  dir  |2 |   -1 |   2
-*  18  |  dir  |2 |0 |   2
+*  18  |  dir  |2 |0 |   1
 *  19  |  dir  |2 |1 |   1 (*)
 *  20  |  dir  |2 |2 |  -1
 *
@@ -1134,7 +1134,12 @@ enum interesting tree_entry_interesting(const struct 
name_entry *entry,
 
negative = do_match(entry, base, base_offset, ps, 1);
 
-   /* #3, #4, #7, #8, #13, #14, #17, #18 */
+   /* #8, #18 */
+   if (positive == all_entries_interesting &&
+   negative == entry_not_interesting)
+   return entry_interesting;
+
+   /* #3, #4, #7, #13, #14, #17 */
if (negative <= entry_not_interesting)
return positive;
 
-- 
2.19.1.1005.gac84295441

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

[Duy Nguyen]

On Sat, Oct 20, 2018 at 9:31 PM Michał Górny  wrote:
> +test_expect_success GPG 'detect fudged commit with double signature' '
> +   sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
> +   sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
> +   sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor >double-sig1.sig 
> &&
> +   gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&
> +   cat double-sig1.sig double-sig2.sig | gpg --enarmor 
> >double-combined.asc &&
> +   sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ /" 
> \
> +   double-combined.asc > double-gpgsig &&
> +   sed -e "/committer/r double-gpgsig" double-base >double-commit &&
> +   git hash-object -w -t commit double-commit >double-commit.commit &&
> +   test_must_fail git verify-commit $(cat double-commit.commit) &&
> +   git show --pretty=short --show-signature $(cat double-commit.commit) 
> >double-actual &&
> +   grep "BAD signature from" double-actual &&
> +   grep "Good signature from" double-actual
> +'

This test fails on 'master' today for me

gpg: WARNING: multiple signatures detected.  Only the first will be checked.
gpg: Signature made Sat Nov  3 15:13:28 2018 UTC
gpg:using DSA key 13B6F51ECDDE430D
gpg:issuer "commit...@example.com"
gpg: BAD signature from "C O Mitter " [ultimate]
gpg: BAD signature from "C O Mitter " [ultimate]
not ok 16 - detect fudged commit with double signature

Perhaps my gpg is too old?

$ gpg --version
gpg (GnuPG) 2.1.15
libgcrypt 1.7.3
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Home: /home/pclouds/.gnupg
Supported algorithms:
Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
CAMELLIA128, CAMELLIA192, CAMELLIA256
Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
Compression: Uncompressed, ZIP, ZLIB, BZIP2
-- 
Duy

[PATCH v3] commit: add a commit.allowEmpty config variable

[tanushree27]

Add commit.allowEmpty configuration variable as a convenience for those
who always prefer --allow-empty.

Add tests to check the behavior introduced by this commit.

This closes https://github.com/git-for-windows/git/issues/1854

Signed-off-by: tanushree27 
---
 Documentation/config.txt |  5 +
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c |  8 
 t/t7500-commit.sh| 32 
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0727b7866..f3828518a5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1467,6 +1467,11 @@ commit.verbose::
A boolean or int to specify the level of verbose with `git commit`.
See linkgit:git-commit[1].
 
+commit.allowEmpty::
+   A boolean to specify whether empty commits are allowed with `git
+   commit`. See linkgit:git-commit[1]. 
+   Defaults to false.
+
 credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a43422..5d3bbf017a 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and 
`-F`.
Usually recording a commit that has the exact same tree as its
sole parent commit is a mistake, and the command prevents you
from making such a commit.  This option bypasses the safety, and
-   is primarily for use by foreign SCM interface scripts.
+   is primarily for use by foreign SCM interface scripts. See
+   `commit.allowEmpty` in linkgit:git-config[1].
 
 --allow-empty-message::
Like --allow-empty this command is primarily for use by foreign
diff --git a/builtin/commit.c b/builtin/commit.c
index 67fa949204..4516309ac2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -101,6 +101,7 @@ static int all, also, interactive, patch_interactive, only, 
amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
+static int config_commit_allow_empty = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
 static char *sign_commit;
@@ -1435,6 +1436,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
config_commit_verbose = git_config_bool_or_int(k, v, _bool);
return 0;
}
+   if (!strcmp(k, "commit.allowempty")) {
+   config_commit_allow_empty = git_config_bool(k, v);
+   return 0;
+   }
 
status = git_gpg_config(k, v, NULL);
if (status)
@@ -1556,6 +1561,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
if (verbose == -1)
verbose = (config_commit_verbose < 0) ? 0 : 
config_commit_verbose;
 
+   if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in 
config*/
+   allow_empty = config_commit_allow_empty;
+   
if (dry_run)
return dry_run_commit(argc, argv, prefix, current_head, );
index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..25a7facd53 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -359,4 +359,36 @@ test_expect_success 'new line found before status message 
in commit template' '
test_i18ncmp expected-template editor-input
 '
 
+# Tests for commit.allowEmpty config
+
+test_expect_success "no commit.allowEmpty and no --allow-empty" "
+   test_must_fail git commit -m 'test'
+"
+
+test_expect_success "no commit.allowEmpty and --allow-empty" "
+   git commit --allow-empty -m 'test'
+"
+
+for i in true 1
+do
+   test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
+   git -c commit.allowEmpty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowEmpty=$i and --allow-empty" "
+   git -c commit.allowEmpty=$i commit --allow-empty -m 'test'
+   "
+done
+
+for i in false 0
+do
+   test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
+   test_must_fail git -C commit.allowEmpty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowEmpty=$i and --allow-empty" "
+   test_must_fail git -c commit.allowEmpty=$i commit --allow-empty 
-m 'test'
+   "
+done
+
 test_done
-- 
2.19.1.windows.1.495.g9597888df3.dirty

Re: [[PATCH v2]] commit: add a commit.allowempty config variable

[Duy Nguyen]

On Sat, Nov 3, 2018 at 12:55 PM tanushree27  wrote:
>
> Add commit.allowempty configuration variable as a convenience for those
> who always prefer --allow-empty.
>
> Add tests to check the behavior introduced by this commit.
>
> This closes https://github.com/git-for-windows/git/issues/1854
>
> Signed-off-by: tanushree27 
> ---
>  Documentation/config.txt |  5 +
>  Documentation/git-commit.txt |  3 ++-
>  builtin/commit.c |  8 
>  t/t7500-commit.sh| 32 
>  4 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c0727b7866..ac63b12ab3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1467,6 +1467,11 @@ commit.verbose::
> A boolean or int to specify the level of verbose with `git commit`.
> See linkgit:git-commit[1].
>
> +commit.allowempty::

The current naming convention is camelCase. So this should be commit.allowEmpty.

> +   A boolean to specify whether empty commits are allowed with `git
> +   commit`. See linkgit:git-commit[1].
> +   Defaults to false.
> +
>  credential.helper::
> Specify an external helper to be called when a username or
> password credential is needed; the helper may consult external
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index f970a43422..07a5b60ab9 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, 
> and `-F`.
> Usually recording a commit that has the exact same tree as its
> sole parent commit is a mistake, and the command prevents you
> from making such a commit.  This option bypasses the safety, and
> -   is primarily for use by foreign SCM interface scripts.
> +   is primarily for use by foreign SCM interface scripts. See
> +   `commit.allowempty` in linkgit:git-config[1].

Same.

>
>  --allow-empty-message::
> Like --allow-empty this command is primarily for use by foreign

-- 
Duy

[PATCH] sequencer.c: remove a stray semicolon

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 On top of ag/rebase-i-in-c

 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..92dca06462 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4846,7 +4846,7 @@ int complete_action(struct replay_opts *opts, unsigned 
flags,
 
if (checkout_onto(opts, onto_name, oid_to_hex(), orig_head))
return -1;
-;
+
if (require_clean_work_tree("rebase", "", 1, 1))
return -1;
 
-- 
2.19.1.1005.gac84295441

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 03.11.18 um 09:14 schrieb Carlo Arenas:

On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt  wrote:


+  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);


nitpick: cast to DWORD instead of int


No; timeout is of type int; after an explicit type cast we don't want to 
have another implicit conversion.


-- Hannes

Re: [RFC] Generation Number v2

[Jakub Narebski]

Derrick Stolee  writes:
> On 11/1/2018 8:27 AM, Jakub Narebski wrote:
>> Derrick Stolee  writes:
>>
>>> Please also let me know about any additional tests that I could
>>> run. Now that I've got a lot of test scripts built up, I can re-run
>>> the test suite pretty quickly.
>>
>> I would add straighforward 1-to-N and N-to-1 reachability tests in the
>> form of `git branch / tag --contains` and `git branch / tag --merged`,
>> and perhaps also ahead-behind calculations used by `git status`, and
>> N-to-M reachability tests used by tag following code in push and fetch.

1-to-N and N-to-1 can be done with `git branch --merged` / `--contains`.

>> Possibly also A...B walk, if it is not implemented via calculating
>> merge-base.
>
> I believe this uses paint_down_to_common(), but looks at the PARENT1 and
> PARENT2 flags afterward to determine the left/right/both relationships.

Right, so I guess this is the same internal mechanism that `git
merge-base` utilizes, just used a bit differently.  Thus benchmarking
`git merge-base` should cover also A...B performance, I guess.

>> Maybe even --ancestry-path walk, though I am not sure how important best
>> performance for rhis case is (we would want good performance, but
>> perhaps best is not needed for rarely used command).
>
> Currently, the implementation of --ancestry-path does not use a
> reachability index.

Well, using reachability index would certainly give it a boost.

[...]
>>> Generation Number Performance Test
>>> ==
>>>
>>> Git uses the commit history to answer many basic questions, such as
>>> computing a merge-base. Some of these algorithms can benefit from a
>>> _reachability index_, which is a condition that allows us to answer
>>> "commit A cannot reach commit B" in some cases. These require pre-
>>> computing some values that we can load and use at run-time. Git
>>> already has a notion of _generation number_, stores it in the commit-
>>> graph file, and uses it in several reachability algorithms.
>>
>> Note that there are other kinds of reachability indices.
>>
>> First, there are reachability indices that can answer the full
>> reachability query (if commit A can reach commit B, or if commit A
>> cannot reach commit B) directly, without walking the commit graph at
>> all: so called label-only approach.  For example one could store for
>> each commit the compressed list of all commits reachable from it
>> (transitive closure compression).
>>
>> Those, I think (but I have not checked), would be of not much use for
>> Git, as the size of the index grows stronger than linear with the number
>> of commits, as grows the time to compute such index.  So probably of no
>> use to Git, at least not directly (Git uses so called "bitmap index",
>> see e.g. [1], which stores reachability bit-vector as compressed
>> bitmap... but not for all commits, only for a small subset).
>>
>>
>> Second, beside negative-cut reachability indexes, that can answer with
>> certainity that "commit A cannot reach commit B", like the generation
>> numbers (also known as level, or topological level), there also
>> positive-cut indexes (usually if not always based on the spanning tree
>> or trees for the DAG), that can answer when "commit A can reach commit
>> B".
>>
>> I think that those can lead to significant speedups for at least some
>> types of commit traversal and reachability queries that Git needs to
>> answer.  But currently no algorithms has a provision for using
>> positive-cut filter index.  Anyway, such index would be auxiliary thing,
>> see the next point.
>>
>>
>> Third, we require more than having reachability index in the sense of
>> having some kind of _labelling_, often composite, that can answer either
>> "commit A cannot reach commit B" or "commit A can reach commit B",
>> depending on the type.  Git for most operations needs more, namely an
>> actual _ordering_, that is a weak order (or to be more exact a total
>> preorder, i.e. there can be two different commits with the same
>> "generation number" or index, but always either idx(A) ≲ idx(B) or
>> idx(B) ≲ idx(A)) and not only partial ordering (where there can be
>> incomparable elements, i.e neither idx(A) ≼ idx(B) nor idx(B) ≼ idx(A)).
>> This is because it needs to be used in priority queue to decide which
>> commit to travel next; more on that later.  This is also why there can
>> be only one such "main" reachability _index_.
>>
>> [1]: https://githubengineering.com/counting-objects/
>
> Thanks for the details. At the moment, I'm only interested in improving our
> negative-cut reachability index, as we have algorithms that can take
> advantage of them (and can compare their performance directly).

Right, I can agree with that.  Positive-cut reachability index use would
need to be added separately.

What I wanted to emphasize is the need for a _number_ (or a total
preorder), not simply an _index_ (a label, perhaps a composite one).
This means, as I wrote, that there would 

Update

[Bruce Blake]

Hello Dear

how are you doing today? my name is Bruce Blake, the manager foreign affairs in 
City Finance Bank, we have a customer here in bank that has not accessed his 
account for the past 18 years, after some research made about him we found out 
he was a victim of the crashed mining company in mexico.

 he died a single man no wife One son from his first marriage that he divorced, 
his next of kin is that his only son which we can arrange and make your name 
his next of kin as his brother, and the mandatory rule of the bank is that 
after 25 years of a dormant account, the fund should be moved to Government 
Treasury account.

This Customer was my friend because i have known him since he opened an account 
with us, i can't claim his fund because i am a staff in this bank, that is why 
i want you to come in and claim this fund as my late clients Relative.

if you are interested get back to me so we could discuss and make arrangements 
for the paper work because everything has to be legit and Legal.

Regards

Bruce Blake.

[[PATCH v2]] commit: add a commit.allowempty config variable

[tanushree27]

Add commit.allowempty configuration variable as a convenience for those
who always prefer --allow-empty.

Add tests to check the behavior introduced by this commit.

This closes https://github.com/git-for-windows/git/issues/1854

Signed-off-by: tanushree27 
---
 Documentation/config.txt |  5 +
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c |  8 
 t/t7500-commit.sh| 32 
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0727b7866..ac63b12ab3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1467,6 +1467,11 @@ commit.verbose::
A boolean or int to specify the level of verbose with `git commit`.
See linkgit:git-commit[1].
 
+commit.allowempty::
+   A boolean to specify whether empty commits are allowed with `git
+   commit`. See linkgit:git-commit[1]. 
+   Defaults to false.
+
 credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a43422..07a5b60ab9 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and 
`-F`.
Usually recording a commit that has the exact same tree as its
sole parent commit is a mistake, and the command prevents you
from making such a commit.  This option bypasses the safety, and
-   is primarily for use by foreign SCM interface scripts.
+   is primarily for use by foreign SCM interface scripts. See
+   `commit.allowempty` in linkgit:git-config[1].
 
 --allow-empty-message::
Like --allow-empty this command is primarily for use by foreign
diff --git a/builtin/commit.c b/builtin/commit.c
index 67fa949204..4516309ac2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -101,6 +101,7 @@ static int all, also, interactive, patch_interactive, only, 
amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
+static int config_commit_allow_empty = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
 static char *sign_commit;
@@ -1435,6 +1436,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
config_commit_verbose = git_config_bool_or_int(k, v, _bool);
return 0;
}
+   if (!strcmp(k, "commit.allowempty")) {
+   config_commit_allow_empty = git_config_bool(k, v);
+   return 0;
+   }
 
status = git_gpg_config(k, v, NULL);
if (status)
@@ -1556,6 +1561,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
if (verbose == -1)
verbose = (config_commit_verbose < 0) ? 0 : 
config_commit_verbose;
 
+   if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in 
config*/
+   allow_empty = config_commit_allow_empty;
+   
if (dry_run)
return dry_run_commit(argc, argv, prefix, current_head, );
index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..fb9bfbfb03 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -359,4 +359,36 @@ test_expect_success 'new line found before status message 
in commit template' '
test_i18ncmp expected-template editor-input
 '
 
+# Tests for commit.allowempty config
+
+test_expect_success "no commit.allowempty and no --allow-empty" "
+   test_must_fail git commit -m 'test'
+"
+
+test_expect_success "no commit.allowempty and --allow-empty" "
+   git commit --allow-empty -m 'test'
+"
+
+for i in true 1
+do
+   test_expect_success "commit.allowempty=$i and no --allow-empty" "
+   git -c commit.allowempty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowempty=$i and --allow-empty" "
+   git -c commit.allowempty=$i commit --allow-empty -m 'test'
+   "
+done
+
+for i in false 0
+do
+   test_expect_success "commit.allowempty=$i and no --allow-empty" "
+   test_must_fail git -c commit.allowempty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowempty=$i and --allow-empty" "
+   test_must_fail git -c commit.allowempty=$i commit --allow-empty 
-m 'test'
+   "
+done
+
 test_done
-- 
2.19.1.windows.1.495.gd17cbd8b09

Re: Git Test Coverage Report (Friday, Nov 2)

[SZEDER Gábor]

On Fri, Nov 02, 2018 at 10:16:48PM -0400, Derrick Stolee wrote:
> Here is the coverage report for today. Some builds were timing out, so I
> removed the tests with number 9000 or more from the build [1]. Hopefully
> this is a temporary measure.

I think it's the Azure CI patch series, see:

https://public-inbox.org/git/20181017232952.gt19...@szeder.dev/
https://public-inbox.org/git/20181021112053.gc30...@szeder.dev/

[PATCH] commit: add a commit.allowEmpty config variable

[tanushree27]

Add commit.allowEmpty configuration variable as a convenience for those
who always prefer --allow-empty.

Add tests to check the behavior introduced by this commit.

This closes https://github.com/git-for-windows/git/issues/1854

Signed-off-by: tanushree27 
---
 Documentation/config.txt |  5 +
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c |  8 
 t/t7500-commit.sh| 32 
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0727b7866..ac63b12ab3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1467,6 +1467,11 @@ commit.verbose::
A boolean or int to specify the level of verbose with `git commit`.
See linkgit:git-commit[1].
 
+commit.allowempty::
+   A boolean to specify whether empty commits are allowed with `git
+   commit`. See linkgit:git-commit[1]. 
+   Defaults to false.
+
 credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a43422..07a5b60ab9 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and 
`-F`.
Usually recording a commit that has the exact same tree as its
sole parent commit is a mistake, and the command prevents you
from making such a commit.  This option bypasses the safety, and
-   is primarily for use by foreign SCM interface scripts.
+   is primarily for use by foreign SCM interface scripts. See
+   `commit.allowempty` in linkgit:git-config[1].
 
 --allow-empty-message::
Like --allow-empty this command is primarily for use by foreign
diff --git a/builtin/commit.c b/builtin/commit.c
index 67fa949204..4516309ac2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -101,6 +101,7 @@ static int all, also, interactive, patch_interactive, only, 
amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
+static int config_commit_allow_empty = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
 static char *sign_commit;
@@ -1435,6 +1436,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
config_commit_verbose = git_config_bool_or_int(k, v, _bool);
return 0;
}
+   if (!strcmp(k, "commit.allowempty")) {
+   config_commit_allow_empty = git_config_bool(k, v);
+   return 0;
+   }
 
status = git_gpg_config(k, v, NULL);
if (status)
@@ -1556,6 +1561,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
if (verbose == -1)
verbose = (config_commit_verbose < 0) ? 0 : 
config_commit_verbose;
 
+   if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in 
config*/
+   allow_empty = config_commit_allow_empty;
+   
if (dry_run)
return dry_run_commit(argc, argv, prefix, current_head, );
index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..fb9bfbfb03 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -359,4 +359,36 @@ test_expect_success 'new line found before status message 
in commit template' '
test_i18ncmp expected-template editor-input
 '
 
+# Tests for commit.allowempty config
+
+test_expect_success "no commit.allowempty and no --allow-empty" "
+   test_must_fail git commit -m 'test'
+"
+
+test_expect_success "no commit.allowempty and --allow-empty" "
+   git commit --allow-empty -m 'test'
+"
+
+for i in true 1
+do
+   test_expect_success "commit.allowempty=$i and no --allow-empty" "
+   git -c commit.allowempty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowempty=$i and --allow-empty" "
+   git -c commit.allowempty=$i commit --allow-empty -m 'test'
+   "
+done
+
+for i in false 0
+do
+   test_expect_success "commit.allowempty=$i and no --allow-empty" "
+   test_must_fail git -c commit.allowempty=$i commit -m 'test'
+   "
+
+   test_expect_success "commit.allowempty=$i and --allow-empty" "
+   test_must_fail git -c commit.allowempty=$i commit --allow-empty 
-m 'test'
+   "
+done
+
 test_done
-- 
2.19.1.windows.1.495.gd17cbd8b09

Re: [PATCH v2] completion: use builtin completion for format-patch

[Junio C Hamano]

Duy Nguyen  writes:

>> Would it make sense to make send-email's completion helper print these
>> out directly? That way, if someone were to modify send-email in the
>> future, they'd only have to look through one file instead of both
>> send-email and the completions script.
>
> I did think about that and decided not to do it (in fact the first
> revision of this patch did exactly that).
>
> If we have to maintain this list manually, we might as well leave to
> the place that matters: the completion script. I don't think the
> person who updates send-email.perl would be always interested in
> completion support. And the one that is interested usually only looks
> at the completion script. Putting this list in send-email.perl just
> makes it harder to find.

I do not necessarily disagree with the conclusion, but I am not sure
if I agree with the last paragraph.  If the definition used to list
completable options was in the send-email command, it is more likely
that a patch to send-email.perl that would add/modify an option
without making a matching change to the definition in the same file
gets noticed, whether the developer who is doing the feature is or
is not interested in maintaining the completion script working, no?
Similarly, if one notices that an option the command supports that
ought to get completed does not get completed, and gets motivated
enough to try finding where in the completion script other existing
options that do get completed are handled, wouldn't that lead one to
the right solution, i.e. discovery of the definition in the
send-email script?  

Quite honestly, I would expect that our developers and user base are
much more competent than one who

 - wants to add completion of the option Y to the command A, which
   has known-to-be-working completion of the option X, and yet

 - fails to imagine that it could be a possible good first step to
   figure out how the option X is completed, so that a new support
   for the option Y might be able to emulate it.

Now, once we start going in the direction of having both the
implementation of options *and* the definition of the list of
completable options in send-email.perl script, I would agree with
your initial assessment in a message much earlier in the thread.  It
would be very tempting to use the data we feed Getopt::Long as the
source of the list of completable options to reduce the longer-term
maintenance load, which would mean it will involve more work.  And
in order to avoid having to invest more work upfront (which I do not
think is necessarily a bad thing), having the definition in the
completion script might be easier to manage---it is closer to the
status quo, especially the state before you taught parse-options API
to give the list of completable options.

Thanks.

Re: Git Test Coverage Report (Friday, Nov 2)

[Junio C Hamano]

Michał Górny  writes:

> As for how involved... we'd just have to use a key that has split
> signing subkey.  Would it be fine to add the subkey to the existing key?
>  It would imply updating keyids/fingerprints everywhere.

Yes, that "everywhere" is exactly what I meant by "how involved",
and your suggestion answers "very much involved".

If we can easily add _another_ key with a subkey that is not the
primary one we use for other tests, without touching the existing
key and the existing tests that use it (including the one I touched
below--- we'd want to see a sig with a key that is not split is
shown with the same %GF and %GP), while adding a handful of new
tests that create signed objects under the new & split key and 
view them with %GF and %GP, then the end result would be that we
managed to add a new test case where %GF/%GP are different without
making very much involved changes.  I guess that was what I was
getting at.

Thanks.

>
>> 
>>  t/t7510-signed-commit.sh | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>> index 19ccae2869..9ecafedcc4 100755
>> --- a/t/t7510-signed-commit.sh
>> +++ b/t/t7510-signed-commit.sh
>> @@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom 
>> format' '
>>  13B6F51ECDDE430D
>>  C O Mitter 
>>  73D758744BE721698EC54E8713B6F51ECDDE430D
>> +73D758744BE721698EC54E8713B6F51ECDDE430D
>>  EOF
>> -git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual &&
>> +git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
>>  test_cmp expect actual
>>  '
>>  

[PATCH v3 00/14] Reduce #ifdef NO_PTHREADS

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

Changes since v2

- more cleanups in grep.c, read-cache.c and index-pack.c
- the send-pack.c changes are back, but this time I just add
  async_with_fork() to move NO_PTHREADS back in run-command.c

For grep.c and read-cache.c, changes are split in two patches. The
first one is a dumb, mechanical conversion from #ifdef to if and is
straightforward. The second one makes "no thread" use "one thread"
code path and needs more careful review.

Diff against nd/pthreads

diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-   assert(num_threads);
pthread_mutex_lock(_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-   assert(num_threads);
pthread_mutex_unlock(_mutex);
 }
 
@@ -234,7 +232,7 @@ static int wait_all(void)
int i;
 
if (!HAVE_THREADS)
-   return 0;
+   BUG("Never call this function unless you have started threads");
 
grep_lock();
all_work_added = 1;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
if (num_threads < 0)
die(_("invalid number of threads specified (%d) for 
%s"),
num_threads, var);
-   else if (!HAVE_THREADS && num_threads && num_threads != 1) {
+   else if (!HAVE_THREADS && num_threads > 1) {
/*
 * TRANSLATORS: %s is the configuration
 * variable for tweaking threads, currently
 * grep.threads
 */
warning(_("no threads support, ignoring %s"), var);
-   num_threads = 0;
+   num_threads = 1;
}
}
 
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct 
object_id *oid,
grep_source_init(, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release();
 
-   if (HAVE_THREADS && num_threads) {
+   if (num_threads > 1) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
grep_source_init(, GREP_SOURCE_FILE, buf.buf, filename, filename);
strbuf_release();
 
-   if (HAVE_THREADS && num_threads) {
+   if (num_threads > 1) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
pathspec.recursive = 1;
pathspec.recurse_submodules = !!recurse_submodules;
 
-   if (HAVE_THREADS) {
-   if (list.nr || cached || show_in_pager)
-   num_threads = 0;
-   else if (num_threads == 0)
-   num_threads = GREP_NUM_THREADS_DEFAULT;
-   else if (num_threads < 0)
-   die(_("invalid number of threads specified (%d)"), 
num_threads);
-   if (num_threads == 1)
-   num_threads = 0;
+   if (list.nr || cached || show_in_pager) {
+   if (num_threads > 1)
+   warning(_("invalid option combination, ignoring 
--threads"));
+   num_threads = 1;
+   } else if (!HAVE_THREADS && num_threads > 1) {
+   warning(_("no threads support, ignoring --threads"));
+   num_threads = 1;
+   } else if (num_threads < 0)
+   die(_("invalid number of threads specified (%d)"), num_threads);
+   else if (num_threads == 0)
+   num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;
+
+   if (num_threads > 1) {
+   if (!HAVE_THREADS)
+   BUG("Somebody got num_threads calculation wrong!");
+   if (!(opt.name_only || opt.unmatch_name_only || opt.count)
+   && (opt.pre_context || opt.post_context ||
+   opt.file_break || opt.funcbody))
+   skip_first_line = 1;
+   start_threads();
} else {
-   if (num_threads)
-   warning(_("no threads support, ignoring --threads"));
-   num_threads = 0;
-   }
-
-   if (!num_threads)
/*
 * The compiled patterns on the main path are only
 * used when not using threading. Otherwise
-* start_threads() below calls compile_grep_patterns()
+* start_threads() above calls compile_grep_patterns()
 * for each thread.
 */
compile_grep_patterns();
-
-   

[PATCH v3 14/14] Clean up pthread_create() error handling

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

Normally pthread_create() rarely fails. But with new pthreads wrapper,
pthread_create() will return ENOSYS on a system without thread support.

Threaded code _is_ protected by HAVE_THREADS and pthread_create()
should never run in the first place. But the situation could change in
the future and bugs may sneak in. Make sure that all pthread_create()
reports the error cause.

While at there, mark these strings for translation if they aren't.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 name-hash.c | 16 ++--
 preload-index.c |  8 ++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(
 static void threaded_lazy_init_name_hash(
struct index_state *istate)
 {
+   int err;
int nr_each;
int k_start;
int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(
if (k_start > istate->cache_nr)
k_start = istate->cache_nr;
td_dir_t->k_end = k_start;
-   if (pthread_create(_dir_t->pthread, NULL, 
lazy_dir_thread_proc, td_dir_t))
-   die("unable to create lazy_dir_thread");
+   err = pthread_create(_dir_t->pthread, NULL, 
lazy_dir_thread_proc, td_dir_t);
+   if (err)
+   die(_("unable to create lazy_dir thread: %s"), 
strerror(err));
}
for (t = 0; t < lazy_nr_dir_threads; t++) {
struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(
 */
td_name->istate = istate;
td_name->lazy_entries = lazy_entries;
-   if (pthread_create(_name->pthread, NULL, lazy_name_thread_proc, 
td_name))
-   die("unable to create lazy_name_thread");
+   err = pthread_create(_name->pthread, NULL, lazy_name_thread_proc, 
td_name);
+   if (err)
+   die(_("unable to create lazy_name thread: %s"), strerror(err));
 
lazy_update_dir_ref_counts(istate, lazy_entries);
 
-   if (pthread_join(td_name->pthread, NULL))
-   die("unable to join lazy_name_thread");
+   err = pthread_join(td_name->pthread, NULL);
+   if (err)
+   die(_("unable to join lazy_name thread: %s"), strerror(err));
 
cleanup_dir_mutex();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,
 
for (i = 0; i < threads; i++) {
struct thread_data *p = data+i;
+   int err;
+
p->index = index;
if (pathspec)
copy_pathspec(>pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,
if (pd.progress)
p->progress = 
offset += work;
-   if (pthread_create(>pthread, NULL, preload_thread, p))
-   die("unable to create threaded lstat");
+   err = pthread_create(>pthread, NULL, preload_thread, p);
+
+   if (err)
+   die(_("unable to create threaded lstat: %s"), 
strerror(err));
}
for (i = 0; i < threads; i++) {
struct thread_data *p = data+i;
diff --git a/run-command.c b/run-command.c
index 3c3b8814df..decf3239bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1213,7 +1213,7 @@ int start_async(struct async *async)
{
int err = pthread_create(>tid, NULL, run_thread, async);
if (err) {
-   error_errno("cannot create thread");
+   error(_("cannot create async thread: %s"), 
strerror(err));
goto error;
}
}
-- 
2.19.1.1005.gac84295441

[PATCH v3 05/14] name-hash.c: remove #ifdef NO_PTHREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 name-hash.c | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -7,6 +7,7 @@
  */
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "thread-utils.h"
 
 struct dir_entry {
struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,
 static int lazy_try_threaded = 1;
 static int lazy_nr_dir_threads;
 
-#ifdef NO_PTHREADS
-
-static inline int lookup_lazy_params(struct index_state *istate)
-{
-   return 0;
-}
-
-static inline void threaded_lazy_init_name_hash(
-   struct index_state *istate)
-{
-}
-
-#else
-
-#include "thread-utils.h"
-
 /*
  * Set a minimum number of cache_entries that we will handle per
  * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(
struct lazy_dir_thread_data *td_dir;
struct lazy_name_thread_data *td_name;
 
+   if (!HAVE_THREADS)
+   return;
+
k_start = 0;
nr_each = DIV_ROUND_UP(istate->cache_nr, lazy_nr_dir_threads);
 
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(
free(lazy_entries);
 }
 
-#endif
-
 static void lazy_init_name_hash(struct index_state *istate)
 {
 
-- 
2.19.1.1005.gac84295441

[PATCH v3 03/14] send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c

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

On systems that do not support multithread, start_async() is
implemented with fork(). This implementation details unfortunately
leak out at least in send-pack.c [1].

To keep the code base clean of NO_PTHREADS, move the this #ifdef back
to run-command.c. The new wrapper function async_with_fork() at least
helps suggest that this special "close()" is related to async in fork
mode.

[1] 09c9957cf7 (send-pack: avoid deadlock when pack-object dies early
- 2011-04-25)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 run-command.c | 9 +
 run-command.h | 1 +
 send-pack.c   | 5 ++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 84b883c213..3c3b8814df 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1246,6 +1246,15 @@ int finish_async(struct async *async)
 #endif
 }
 
+int async_with_fork(void)
+{
+#ifdef NO_PTHREADS
+   return 1;
+#else
+   return 0;
+#endif
+}
+
 const char *find_hook(const char *name)
 {
static struct strbuf path = STRBUF_INIT;
diff --git a/run-command.h b/run-command.h
index 9b7f38202c..68f5369fc2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -141,6 +141,7 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 int in_async(void);
+int async_with_fork(void);
 void check_pipe(int err);
 
 /**
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..f692686770 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -203,9 +203,8 @@ static int receive_status(int in, struct ref *refs)
 static int sideband_demux(int in, int out, void *data)
 {
int *fd = data, ret;
-#ifdef NO_PTHREADS
-   close(fd[1]);
-#endif
+   if (async_with_fork())
+   close(fd[1]);
ret = recv_sideband("send-pack", fd[0], out);
close(out);
return ret;
-- 
2.19.1.1005.gac84295441

[PATCH v3 12/14] read-cache.c: reduce branching based on HAVE_THREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 40fc0cb65f..00cd416816 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2172,7 +2172,6 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
 
src_offset = sizeof(*hdr);
 
-   if (HAVE_THREADS) {
nr_threads = git_config_get_index_threads();
 
/* TODO: does creating more threads than cores help? */
@@ -2183,6 +2182,9 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
nr_threads = cpus;
}
 
+   if (!HAVE_THREADS)
+   nr_threads = 1;
+
if (nr_threads > 1) {
extension_offset = read_eoie_extension(mmap, mmap_size);
if (extension_offset) {
@@ -2210,20 +2212,16 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
} else {
src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
}
-   } else {
-   src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
-   }
 
istate->timestamp.sec = st.st_mtime;
istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
/* if we created a thread, join it otherwise load the extensions on the 
primary thread */
-   if (HAVE_THREADS && extension_offset) {
+   if (extension_offset) {
int ret = pthread_join(p.pthread, NULL);
if (ret)
die(_("unable to join load_index_extensions thread: 
%s"), strerror(ret));
-   }
-   if (!extension_offset) {
+   } else {
p.src_offset = src_offset;
load_index_extensions();
}
@@ -2745,8 +2743,10 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
if (ce_write(, newfd, , sizeof(hdr)) < 0)
return -1;
 
-   if (HAVE_THREADS) {
+   if (HAVE_THREADS)
nr_threads = git_config_get_index_threads();
+   else
+   nr_threads = 1;
 
if (nr_threads != 1) {
int ieot_blocks, cpus;
@@ -2777,7 +2777,6 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
}
}
-   }
 
offset = lseek(newfd, 0, SEEK_CUR);
if (offset < 0) {
@@ -2861,7 +2860,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
 * strip_extensions parameter as we need it when loading the shared
 * index.
 */
-   if (HAVE_THREADS && ieot) {
+   if (ieot) {
struct strbuf sb = STRBUF_INIT;
 
write_ieot_extension(, ieot);
-- 
2.19.1.1005.gac84295441

[PATCH v3 13/14] read-cache.c: initialize copy_len to shut up gcc 8

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

It was reported that when building with NO_PTHREADS=1,
-Wmaybe-uninitialized is triggered. Just initialize the variable from
the beginning to shut the compiler up (because this warning is enabled
in config.dev)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 00cd416816..c510f598b1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct 
mem_pool *ce_mem_pool,
size_t len;
const char *name;
unsigned int flags;
-   size_t copy_len;
+   size_t copy_len = 0;
/*
 * Adjacent cache entries tend to share the leading paths, so it makes
 * sense to only store the differences in later entries.  In the v4
@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct 
mem_pool *ce_mem_pool,
die(_("malformed name field in the index, near 
path '%s'"),
previous_ce->name);
copy_len = previous_len - strip_len;
-   } else {
-   copy_len = 0;
}
name = (const char *)cp;
}
-- 
2.19.1.1005.gac84295441

[PATCH v3 10/14] pack-objects: remove #ifdef NO_PTHREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/pack-objects.c | 26 ++
 pack-objects.h |  6 --
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, 
unsigned long trg_size,
return 0;
 }
 
-#ifndef NO_PTHREADS
-
 /* Protect access to object database */
 static pthread_mutex_t read_mutex;
 #define read_lock()pthread_mutex_lock(_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;
  * ahead in the list because they can be stolen and would need
  * progress_mutex for protection.
  */
-#else
-
-#define read_lock()(void)0
-#define read_unlock()  (void)0
-#define cache_lock()   (void)0
-#define cache_unlock() (void)0
-#define progress_lock()(void)0
-#define progress_unlock()  (void)0
-
-#endif
 
 /*
  * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, 
unsigned *list_size,
free(array);
 }
 
-#ifndef NO_PTHREADS
-
 static void try_to_free_from_threads(size_t size)
 {
read_lock();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, 
unsigned list_size,
free(p);
 }
 
-#else
-#define ll_find_deltas(l, s, w, d, p)  find_deltas(l, , w, d, p)
-#endif
-
 static void add_tag_chain(const struct object_id *oid)
 {
struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char 
*v, void *cb)
if (delta_search_threads < 0)
die(_("invalid number of threads specified (%d)"),
delta_search_threads);
-#ifdef NO_PTHREADS
-   if (delta_search_threads != 1) {
+   if (!HAVE_THREADS && delta_search_threads != 1) {
warning(_("no threads support, ignoring %s"), k);
delta_search_threads = 0;
}
-#endif
return 0;
}
if (!strcmp(k, "pack.indexversion")) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
if (!delta_search_threads)  /* --threads=0 means autodetect */
delta_search_threads = online_cpus();
 
-#ifdef NO_PTHREADS
-   if (delta_search_threads != 1)
+   if (!HAVE_THREADS && delta_search_threads != 1)
warning(_("no threads support, ignoring --threads"));
-#endif
if (!pack_to_stdout && !pack_size_limit)
pack_size_limit = pack_size_limit_cfg;
if (pack_to_stdout && pack_size_limit)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,9 +145,7 @@ struct packing_data {
struct packed_git **in_pack_by_idx;
struct packed_git **in_pack;
 
-#ifndef NO_PTHREADS
pthread_mutex_t lock;
-#endif
 
/*
 * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);
 
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
pthread_mutex_lock(>lock);
-#endif
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
pthread_mutex_unlock(>lock);
-#endif
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 
2.19.1.1005.gac84295441

[PATCH v3 07/14] grep: remove #ifdef NO_PTHREADS

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

This is a faithful conversion without attempting to improve
anything. That comes later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/grep.c | 59 +++---
 grep.c |  6 -
 grep.h |  6 -
 3 files changed, 22 insertions(+), 49 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..6dd15dbaa2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
-#ifndef NO_PTHREADS
 static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
@@ -234,6 +233,9 @@ static int wait_all(void)
int hit = 0;
int i;
 
+   if (!HAVE_THREADS)
+   return 0;
+
grep_lock();
all_work_added = 1;
 
@@ -265,13 +267,6 @@ static int wait_all(void)
 
return hit;
 }
-#else /* !NO_PTHREADS */
-
-static int wait_all(void)
-{
-   return 0;
-}
-#endif
 
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
@@ -284,8 +279,7 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
if (num_threads < 0)
die(_("invalid number of threads specified (%d) for 
%s"),
num_threads, var);
-#ifdef NO_PTHREADS
-   else if (num_threads && num_threads != 1) {
+   else if (!HAVE_THREADS && num_threads && num_threads != 1) {
/*
 * TRANSLATORS: %s is the configuration
 * variable for tweaking threads, currently
@@ -294,7 +288,6 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
warning(_("no threads support, ignoring %s"), var);
num_threads = 0;
}
-#endif
}
 
if (!strcmp(var, "submodule.recurse"))
@@ -330,17 +323,14 @@ static int grep_oid(struct grep_opt *opt, const struct 
object_id *oid,
grep_source_init(, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release();
 
-#ifndef NO_PTHREADS
-   if (num_threads) {
+   if (HAVE_THREADS && num_threads) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
 */
add_work(opt, );
return 0;
-   } else
-#endif
-   {
+   } else {
int hit;
 
hit = grep_source(opt, );
@@ -363,17 +353,14 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
grep_source_init(, GREP_SOURCE_FILE, buf.buf, filename, filename);
strbuf_release();
 
-#ifndef NO_PTHREADS
-   if (num_threads) {
+   if (HAVE_THREADS && num_threads) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
 */
add_work(opt, );
return 0;
-   } else
-#endif
-   {
+   } else {
int hit;
 
hit = grep_source(opt, );
@@ -1038,20 +1025,20 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
pathspec.recursive = 1;
pathspec.recurse_submodules = !!recurse_submodules;
 
-#ifndef NO_PTHREADS
-   if (list.nr || cached || show_in_pager)
-   num_threads = 0;
-   else if (num_threads == 0)
-   num_threads = GREP_NUM_THREADS_DEFAULT;
-   else if (num_threads < 0)
-   die(_("invalid number of threads specified (%d)"), num_threads);
-   if (num_threads == 1)
+   if (HAVE_THREADS) {
+   if (list.nr || cached || show_in_pager)
+   num_threads = 0;
+   else if (num_threads == 0)
+   num_threads = GREP_NUM_THREADS_DEFAULT;
+   else if (num_threads < 0)
+   die(_("invalid number of threads specified (%d)"), 
num_threads);
+   if (num_threads == 1)
+   num_threads = 0;
+   } else {
+   if (num_threads)
+   warning(_("no threads support, ignoring --threads"));
num_threads = 0;
-#else
-   if (num_threads)
-   warning(_("no threads support, ignoring --threads"));
-   num_threads = 0;
-#endif
+   }
 
if (!num_threads)
/*
@@ -1062,15 +1049,13 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
 */
compile_grep_patterns();
 
-#ifndef NO_PTHREADS
-   if (num_threads) {
+   if (HAVE_THREADS && num_threads) {
if (!(opt.name_only || opt.unmatch_name_only || opt.count)
&& (opt.pre_context || opt.post_context ||
opt.file_break || opt.funcbody))

[PATCH v3 01/14] thread-utils: macros to unconditionally compile pthreads API

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

When built with NO_PTHREADS, the macros are used make the code build
even though pthreads header and library may be missing. The code can
still have different code paths for no threads support with
HAVE_THREADS variable.

There are of course impacts on no-pthreads builds:

- data structure may get slightly bigger because all the mutexes and
  pthread_t are present (as an int)

- code execution is not impacted much. Locking (in hot path) is
  no-op. Other wrapper function calls really should not matter much.

- the binary size grows bigger because of threaded code. But at least
  on Linux this does not matter, if some code is not executed, it's
  not mapped in memory.

This is a preparation step to remove "#ifdef NO_PTHREADS" in the code
mostly because of maintainability. As Jeff put it

> it's probably OK to stop thinking of it as "non-threaded platforms
> are the default and must pay zero cost" and more as "threaded
> platforms are the default, and non-threaded ones are OK to pay a
> small cost as long as they still work".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile   |  2 +-
 thread-utils.c | 48 
 thread-utils.h | 48 +---
 3 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += thread-utils.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS
 else
BASIC_CFLAGS += $(PTHREAD_CFLAGS)
EXTLIBS += $(PTHREAD_LIBS)
-   LIB_OBJS += thread-utils.o
 endif
 
 ifdef HAVE_PATHS_H
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+   return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
pthread_mutexattr_t a;
int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
pthread_mutexattr_destroy();
}
return ret;
+#else
+   return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+void *(*fn)(void *), void *data)
+{
+   /*
+* Do nothing.
+*
+* The main purpose of this function is to break compiler's
+* flow analysis and avoid -Wunused-variable false warnings.
+*/
+   return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+   /*
+* Do nothing.
+*
+* The main purpose of this function is to break compiler's
+* flow analysis or it may realize that functions like
+* pthread_mutex_init() is no-op, which means the (static)
+* variable is not used/initialized at all and trigger
+* -Wunused-variable
+*/
+   return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+   /*
+* Do nothing.
+*
+* The main purpose of this function is to break compiler's
+* flow analysis and avoid -Wunused-variable false warnings.
+*/
+   return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..4961487ed9 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,54 @@
 #ifndef NO_PTHREADS
 #include 
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 0
+
+/*
+ * macros instead of typedefs because pthread definitions may have
+ * been pulled in by some system dependencies even though the user
+ * wants to disable pthread.
+ */
+#define pthread_t int
+#define pthread_mutex_t int
+#define pthread_cond_t int
+#define pthread_key_t int
+
+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
+#define pthread_mutex_lock(mutex)
+#define pthread_mutex_unlock(mutex)
+#define pthread_mutex_destroy(mutex)
+
+#define pthread_cond_init(cond, attr) dummy_pthread_init(cond)
+#define pthread_cond_wait(cond, mutex)
+#define pthread_cond_signal(cond)
+#define pthread_cond_broadcast(cond)
+#define pthread_cond_destroy(cond)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+   dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+   dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+

[PATCH v3 04/14] index-pack: remove #ifdef NO_PTHREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/index-pack.c | 63 ++--
 1 file changed, 14 insertions(+), 49 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..682042579b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -42,9 +42,7 @@ struct base_data {
 };
 
 struct thread_local {
-#ifndef NO_PTHREADS
pthread_t thread;
-#endif
struct base_data *base_cache;
size_t base_cache_used;
int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;
 static int input_fd, output_fd;
 static const char *curr_pack;
 
-#ifndef NO_PTHREADS
-
 static struct thread_local *thread_data;
 static int nr_dispatched;
 static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)
free(thread_data);
 }
 
-#else
-
-#define read_lock()
-#define read_unlock()
-
-#define counter_lock()
-#define counter_unlock()
-
-#define work_lock()
-#define work_unlock()
-
-#define deepest_delta_lock()
-#define deepest_delta_unlock()
-
-#define type_cas_lock()
-#define type_cas_unlock()
-
-#endif
-
-
 static int mark_link(struct object *obj, int type, void *data, struct 
fsck_options *options)
 {
if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char 
*format, ...)
 
 static inline struct thread_local *get_thread_data(void)
 {
-#ifndef NO_PTHREADS
-   if (threads_active)
-   return pthread_getspecific(key);
-   assert(!threads_active &&
-  "This should only be reached when all threads are gone");
-#endif
+   if (HAVE_THREADS) {
+   if (threads_active)
+   return pthread_getspecific(key);
+   assert(!threads_active &&
+  "This should only be reached when all threads are gone");
+   }
return _data;
 }
 
-#ifndef NO_PTHREADS
 static void set_thread_data(struct thread_local *data)
 {
if (threads_active)
pthread_setspecific(key, data);
 }
-#endif
 
 static struct base_data *alloc_base_data(void)
 {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)
find_unresolved_deltas(base_obj);
 }
 
-#ifndef NO_PTHREADS
 static void *threaded_second_pass(void *data)
 {
set_thread_data(data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data)
}
return NULL;
 }
-#endif
 
 /*
  * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void)
progress = start_progress(_("Resolving deltas"),
  nr_ref_deltas + nr_ofs_deltas);
 
-#ifndef NO_PTHREADS
nr_dispatched = 0;
if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
init_thread();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)
cleanup_thread();
return;
}
-#endif
 
for (i = 0; i < nr_objects; i++) {
struct object_entry *obj = [i];
@@ -1531,11 +1501,10 @@ static int git_index_pack_config(const char *k, const 
char *v, void *cb)
if (nr_threads < 0)
die(_("invalid number of threads specified (%d)"),
nr_threads);
-#ifdef NO_PTHREADS
-   if (nr_threads != 1)
+   if (!HAVE_THREADS && nr_threads != 1) {
warning(_("no threads support, ignoring %s"), k);
-   nr_threads = 1;
-#endif
+   nr_threads = 1;
+   }
return 0;
}
return git_default_config(k, v, cb);
@@ -1723,12 +1692,10 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
nr_threads = strtoul(arg+10, , 0);
if (!arg[10] || *end || nr_threads < 0)
usage(index_pack_usage);
-#ifdef NO_PTHREADS
-   if (nr_threads != 1)
-   warning(_("no threads support, "
- "ignoring %s"), arg);
-   nr_threads = 1;
-#endif
+   if (!HAVE_THREADS && nr_threads != 1) {
+   warning(_("no threads support, ignoring 
%s"), arg);
+   nr_threads = 1;
+   }
} else if (starts_with(arg, "--pack_header=")) {
struct pack_header *hdr;
char *c;
@@ -1791,14 +1758,12 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
if (strict)
opts.flags |= WRITE_IDX_STRICT;
 
-#ifndef NO_PTHREADS
-   if (!nr_threads) {
+   if (HAVE_THREADS && !nr_threads) {
nr_threads = online_cpus();
/* An experiment showed that more threads 

[PATCH v3 11/14] read-cache.c: remove #ifdef NO_PTHREADS

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

This is a faithful conversion with no attempt to clean up whatsoever.
Code indentation is left broken. There will be another commit to clean
it up and un-indent if we just indent now. It's just more code noise.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 34 ++
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index d57958233e..40fc0cb65f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, 
size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct 
index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx 
*eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
pthread_t pthread;
-#endif
struct index_state *istate;
const char *mmap;
size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct 
index_state *istate,
return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct 
index_state *istate, con
 
return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
size_t mmap_size;
struct load_index_extensions p;
size_t extension_offset = 0;
-#ifndef NO_PTHREADS
int nr_threads, cpus;
struct index_entry_offset_table *ieot = NULL;
-#endif
 
if (istate->initialized)
return istate->cache_nr;
@@ -2181,7 +2172,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
 
src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
+   if (HAVE_THREADS) {
nr_threads = git_config_get_index_threads();
 
/* TODO: does creating more threads than cores help? */
@@ -2219,21 +2210,19 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
} else {
src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
}
-#else
-   src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
-#endif
+   } else {
+   src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
+   }
 
istate->timestamp.sec = st.st_mtime;
istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
/* if we created a thread, join it otherwise load the extensions on the 
primary thread */
-#ifndef NO_PTHREADS
-   if (extension_offset) {
+   if (HAVE_THREADS && extension_offset) {
int ret = pthread_join(p.pthread, NULL);
if (ret)
die(_("unable to join load_index_extensions thread: 
%s"), strerror(ret));
}
-#endif
if (!extension_offset) {
p.src_offset = src_offset;
load_index_extensions();
@@ -2756,8 +2745,9 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
if (ce_write(, newfd, , sizeof(hdr)) < 0)
return -1;
 
-#ifndef NO_PTHREADS
-   nr_threads = git_config_get_index_threads();
+   if (HAVE_THREADS) {
+   nr_threads = git_config_get_index_threads();
+
if (nr_threads != 1) {
int ieot_blocks, cpus;
 
@@ -2787,7 +2777,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
}
}
-#endif
+   }
 
offset = lseek(newfd, 0, SEEK_CUR);
if (offset < 0) {
@@ -2871,8 +2861,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
 * strip_extensions parameter as we need it when loading the shared
 * index.
 */
-#ifndef NO_PTHREADS
-   if (ieot) {
+   if (HAVE_THREADS && ieot) {
struct strbuf sb = STRBUF_INIT;
 
write_ieot_extension(, ieot);
@@ -2883,7 +2872,6 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
if (err)
return -1;
}
-#endif
 
if (!strip_extensions && istate->split_index) {
struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3457,6 @@ static void write_eoie_extension(struct strbuf *sb, 
git_hash_ctx *eoie_context,
   

[PATCH v3 09/14] preload-index.c: remove #ifdef NO_PTHREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 preload-index.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
 #include "fsmonitor.h"
 #include "config.h"
 #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
- const struct pathspec *pathspec,
- unsigned int refresh_flags)
-{
-   ; /* nothing */
-}
-#else
-
-#include 
+#include "thread-utils.h"
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
struct thread_data data[MAX_PARALLEL];
struct progress_data pd;
 
-   if (!core_preload_index)
+   if (!HAVE_THREADS || !core_preload_index)
return;
 
threads = index->cache_nr / THREAD_COST;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
 
trace_performance_leave("preload index");
 }
-#endif
 
 int read_index_preload(struct index_state *index,
   const struct pathspec *pathspec,
-- 
2.19.1.1005.gac84295441

[PATCH v3 06/14] attr.c: remove #ifdef NO_PTHREADS

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 attr.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)
 
 struct attr_hashmap {
struct hashmap map;
-#ifndef NO_PTHREADS
pthread_mutex_t mutex;
-#endif
 };
 
 static inline void hashmap_lock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
pthread_mutex_lock(>mutex);
-#endif
 }
 
 static inline void hashmap_unlock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
pthread_mutex_unlock(>mutex);
-#endif
 }
 
 /*
@@ -498,23 +492,17 @@ static struct check_vector {
size_t nr;
size_t alloc;
struct attr_check **checks;
-#ifndef NO_PTHREADS
pthread_mutex_t mutex;
-#endif
 } check_vector;
 
 static inline void vector_lock(void)
 {
-#ifndef NO_PTHREADS
pthread_mutex_lock(_vector.mutex);
-#endif
 }
 
 static inline void vector_unlock(void)
 {
-#ifndef NO_PTHREADS
pthread_mutex_unlock(_vector.mutex);
-#endif
 }
 
 static void check_vector_add(struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,
 
 void attr_start(void)
 {
-#ifndef NO_PTHREADS
pthread_mutex_init(_attr_hashmap.mutex, NULL);
pthread_mutex_init(_vector.mutex, NULL);
-#endif
 }
-- 
2.19.1.1005.gac84295441

[PATCH v3 08/14] grep: clean up num_threads handling

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

When NO_PTHREADS is still used in this file, we have two separate code
paths for thread and no thread support. The latter will always have
num_threads remain zero while the former uses num_threads zero as
"default number of threads".

With recent changes blur the line between thread and no-thread
support, this num_threads handling becomes a bit strange so let's
redefine it like this:

- num_threads == 0 means default number of threads and should become
  positive after all configuration and option parsing is done if
  multithread is supported.

- num_threads <= 1 runs no threads. It does not matter if the platform
  supports threading or not.

- num_threads > 1 will run multiple threads and is invalid if
  HAVE_THREADS is false. pthread API is only used in this case.

PS. a new warning is also added when num_threads is forced back to one
because a thread-incompatible option is specified.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/grep.c | 58 +++---
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-   assert(num_threads);
pthread_mutex_lock(_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-   assert(num_threads);
pthread_mutex_unlock(_mutex);
 }
 
@@ -234,7 +232,7 @@ static int wait_all(void)
int i;
 
if (!HAVE_THREADS)
-   return 0;
+   BUG("Never call this function unless you have started threads");
 
grep_lock();
all_work_added = 1;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
if (num_threads < 0)
die(_("invalid number of threads specified (%d) for 
%s"),
num_threads, var);
-   else if (!HAVE_THREADS && num_threads && num_threads != 1) {
+   else if (!HAVE_THREADS && num_threads > 1) {
/*
 * TRANSLATORS: %s is the configuration
 * variable for tweaking threads, currently
 * grep.threads
 */
warning(_("no threads support, ignoring %s"), var);
-   num_threads = 0;
+   num_threads = 1;
}
}
 
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct 
object_id *oid,
grep_source_init(, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release();
 
-   if (HAVE_THREADS && num_threads) {
+   if (num_threads > 1) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
grep_source_init(, GREP_SOURCE_FILE, buf.buf, filename, filename);
strbuf_release();
 
-   if (HAVE_THREADS && num_threads) {
+   if (num_threads > 1) {
/*
 * add_work() copies gs and thus assumes ownership of
 * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
pathspec.recursive = 1;
pathspec.recurse_submodules = !!recurse_submodules;
 
-   if (HAVE_THREADS) {
-   if (list.nr || cached || show_in_pager)
-   num_threads = 0;
-   else if (num_threads == 0)
-   num_threads = GREP_NUM_THREADS_DEFAULT;
-   else if (num_threads < 0)
-   die(_("invalid number of threads specified (%d)"), 
num_threads);
-   if (num_threads == 1)
-   num_threads = 0;
+   if (list.nr || cached || show_in_pager) {
+   if (num_threads > 1)
+   warning(_("invalid option combination, ignoring 
--threads"));
+   num_threads = 1;
+   } else if (!HAVE_THREADS && num_threads > 1) {
+   warning(_("no threads support, ignoring --threads"));
+   num_threads = 1;
+   } else if (num_threads < 0)
+   die(_("invalid number of threads specified (%d)"), num_threads);
+   else if (num_threads == 0)
+   num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;
+
+   if (num_threads > 1) {
+   if (!HAVE_THREADS)
+   BUG("Somebody got num_threads calculation wrong!");
+   if (!(opt.name_only || opt.unmatch_name_only || opt.count)
+   && (opt.pre_context || opt.post_context ||
+   opt.file_break || opt.funcbody))
+   skip_first_line = 1;
+   

[PATCH v3 02/14] run-command.h: include thread-utils.h instead of pthread.h

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

run-command.c may use threads for its async support. But instead of
including pthread.h directly, let's include thread-utils.h.

run-command.c probably never needs the dummy bits in thread-utils.h
when NO_PTHREADS is defined. But this makes sure we have consistent
HAVE_THREADS behavior everywhere. From now on outside compat/,
thread-utils.h is the only place that includes pthread.h

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 run-command.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/run-command.h b/run-command.h
index 3932420ec8..9b7f38202c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -1,9 +1,7 @@
 #ifndef RUN_COMMAND_H
 #define RUN_COMMAND_H
 
-#ifndef NO_PTHREADS
-#include 
-#endif
+#include "thread-utils.h"
 
 #include "argv-array.h"
 
-- 
2.19.1.1005.gac84295441

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

On Sat, Nov 3, 2018 at 8:59 AM Denton Liu  wrote:
> > @@ -2081,7 +2077,7 @@ _git_send_email ()
> >   return
> >   ;;
> >   --*)
> > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
> > + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd 
> > --chain-reply-to
> >   --compose --confirm= --dry-run --envelope-sender
> >   --from --identity
> >   --in-reply-to --no-chain-reply-to 
> > --no-signed-off-by-cc
>
> Would it make sense to make send-email's completion helper print these
> out directly? That way, if someone were to modify send-email in the
> future, they'd only have to look through one file instead of both
> send-email and the completions script.

I did think about that and decided not to do it (in fact the first
revision of this patch did exactly that).

If we have to maintain this list manually, we might as well leave to
the place that matters: the completion script. I don't think the
person who updates send-email.perl would be always interested in
completion support. And the one that is interested usually only looks
at the completion script. Putting this list in send-email.perl just
makes it harder to find.
-- 
Duy

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Carlo Arenas]

On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt  wrote:
>
> +  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);

nitpick: cast to DWORD instead of int

Carlo

Re: [PATCH v2] completion: use builtin completion for format-patch

[Denton Liu]

On Sat, Nov 03, 2018 at 07:03:18AM +0100, Duy Nguyen wrote:
> Subject: [PATCH] completion: use __gitcomp_builtin for format-patch
> 
> This helps format-patch gain completion for a couple new options,
> notably --range-diff.
> 
> Since send-email completion relies on $__git_format_patch_options
> which is now reduced, we need to do something not to regress
> send-email completion.
> 
> The workaround here is implement --git-completion-helper in
> send-email.perl just as a bridge to "format-patch --git-completion-helper".
> This is enough to use __gitcomp_builtin on send-email (to take
> advantage of caching).
> 
> In the end, send-email.perl can probably reuse the same info it passes
> to GetOptions() to generate full --git-completion-helper output so
> that we don't need to keep track of its options in git-completion.bash
> anymore. But that's something for another boring day.
> 
> Helped-by: Denton Liu 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash | 16 ++--
>  git-send-email.perl|  8 
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index db7fd87b6b..8409978793 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1532,13 +1532,9 @@ _git_fetch ()
>   __git_complete_remote_or_refspec
>  }
>  
> -__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
> +__git_format_patch_extra_options="
> + --full-index --not --all --no-prefix --src-prefix=
> + --dst-prefix= --notes
>  "
>  
>  _git_format_patch ()
> @@ -1551,7 +1547,7 @@ _git_format_patch ()
>   return
>   ;;
>   --*)
> - __gitcomp "$__git_format_patch_options"
> + __gitcomp_builtin format-patch 
> "$__git_format_patch_extra_options"
>   return
>   ;;
>   esac
> @@ -2081,7 +2077,7 @@ _git_send_email ()
>   return
>   ;;
>   --*)
> - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
> + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd 
> --chain-reply-to
>   --compose --confirm= --dry-run --envelope-sender
>   --from --identity
>   --in-reply-to --no-chain-reply-to --no-signed-off-by-cc

Would it make sense to make send-email's completion helper print these
out directly? That way, if someone were to modify send-email in the
future, they'd only have to look through one file instead of both
send-email and the completions script.

> @@ -2090,7 +2086,7 @@ _git_send_email ()
>   --smtp-server-port --smtp-encryption= --smtp-user
>   --subject --suppress-cc= --suppress-from --thread --to
>   --validate --no-validate
> - $__git_format_patch_options"
> + $__git_format_patch_extra_options"
>   return
>   ;;
>   esac
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2be5dac337..ed0714eaaa 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -119,6 +119,11 @@ sub usage {
>   exit(1);
>  }
>  
> +sub completion_helper {
> +print Git::command('format-patch', '--git-completion-helper');
> +exit(0);
> +}
> +
>  # most mail servers generate the Date: header, but not all...
>  sub format_2822_time {
>   my ($time) = @_;
> @@ -311,6 +316,7 @@ sub signal_handler {
>  # needing, first, from the command line:
>  
>  my $help;
> +my $git_completion_helper;
>  my $rc = GetOptions("h" => \$help,
>  "dump-aliases" => \$dump_aliases);
>  usage() unless $rc;
> @@ -373,9 +379,11 @@ sub signal_handler {
>   "no-xmailer" => sub {$use_xmailer = 0},
>   "batch-size=i" => \$batch_size,
>   "relogin-delay=i" => \$relogin_delay,
> + "git-completion-helper" => \$git_completion_helper,
>);
>  
>  usage() if $help;
> +completion_helper() if $git_completion_helper;
>  unless ($rc) {
>  usage();
>  }
> -- 
> 2.19.1.1005.gac84295441
> 
> -- 8< --
> --
> Duy

Aside from that one comment, it looks good to me. Thanks for helping me
clean up my earlier patch!

Re: Git Test Coverage Report (Friday, Nov 2)

[Michał Górny]

On Sat, 2018-11-03 at 12:38 +0900, Junio C Hamano wrote:
> Derrick Stolee  writes:
> 
> > Uncovered code in 'next' not in 'master'
> > 
> > 
> > pretty.c
> > 4de9394dcb 1264) if (c->signature_check.primary_key_fingerprint)
> > 4de9394dcb 1265) strbuf_addstr(sb,
> > c->signature_check.primary_key_fingerprint);
> > 4de9394dcb 1266) break;
> 
> Perhaps a patch along this line can be appended to the
> mg/gpg-fingerprint topic that ends at 4de9394d ("gpg-interface.c:
> obtain primary key fingerprint as well", 2018-10-22) to cover this
> entry in the report.  
> 
> I do not know how involved it would be to set up a new test case
> that demonstrates a case where %GF and %GP are different, but if it
> is very involved perhaps it is not worth adding such a case.

Well, I didn't add a test for %GP primarily because we didn't have a key
with different primary and subkey fingerprints.

As for how involved... we'd just have to use a key that has split
signing subkey.  Would it be fine to add the subkey to the existing key?
 It would imply updating keyids/fingerprints everywhere.

> 
>  t/t7510-signed-commit.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 19ccae2869..9ecafedcc4 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom 
> format' '
>   13B6F51ECDDE430D
>   C O Mitter 
>   73D758744BE721698EC54E8713B6F51ECDDE430D
> + 73D758744BE721698EC54E8713B6F51ECDDE430D
>   EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual &&
> + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
>   test_cmp expect actual
>  '
>  

-- 
Best regards,
Michał Górny


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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

On Fri, Nov 02, 2018 at 08:52:30AM +0900, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> >> > I have no comment about this. In an ideal world, sendemail.perl could
> >> > be taught to support --git-completion-helper but I don't think my
> >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps
> >> > this will do. I don't know.
> >>
> >> So "all", "attach", etc. are added to this list while these similar
> >> options are lost from the other variable?  Is this a good trade-off?
> >
> > Not sure if I understand you correctly, but it looks to me that the
> > options in git-send-email.perl are well organized, so we could...
> 
> Yes, but I wasn't commenting on your "sendemail should also be able
> to help completion by supporting --completion-helper option" (I think
> that is a sensible approach).  My comment was about Denton's patch,
> which reduced the hard-coded list of format-patch options (i.e. the
> first hunk) but had to add back many of them to send-email's
> completion (i.e. the last hunk)---overall, it did not help reducing
> the number of options hardcoded in the script.
> 
> If it makes sense to complete all options to format-patch to
> send-email, then as you outlined, grabbing them out of format-patch
> with the --completion-helper option at runtime, and using them to
> complete both format-patch and send-email would be a good idea.  And
> that should be doable even before send-email learns how to list its
> supported options to help the completion.

OK how about this?

Minimal changes in send-email.perl and no duplication in
_git_send_email(). I could do $(git format-patch
--git-completion-helper) directly from _git_send_email() too but we
lose caching.

-- 8< --
Subject: [PATCH] completion: use __gitcomp_builtin for format-patch

This helps format-patch gain completion for a couple new options,
notably --range-diff.

Since send-email completion relies on $__git_format_patch_options
which is now reduced, we need to do something not to regress
send-email completion.

The workaround here is implement --git-completion-helper in
send-email.perl just as a bridge to "format-patch --git-completion-helper".
This is enough to use __gitcomp_builtin on send-email (to take
advantage of caching).

In the end, send-email.perl can probably reuse the same info it passes
to GetOptions() to generate full --git-completion-helper output so
that we don't need to keep track of its options in git-completion.bash
anymore. But that's something for another boring day.

Helped-by: Denton Liu 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 16 ++--
 git-send-email.perl|  8 
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index db7fd87b6b..8409978793 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1532,13 +1532,9 @@ _git_fetch ()
__git_complete_remote_or_refspec
 }
 
-__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
+__git_format_patch_extra_options="
+   --full-index --not --all --no-prefix --src-prefix=
+   --dst-prefix= --notes
 "
 
 _git_format_patch ()
@@ -1551,7 +1547,7 @@ _git_format_patch ()
return
;;
--*)
-   __gitcomp "$__git_format_patch_options"
+   __gitcomp_builtin format-patch 
"$__git_format_patch_extra_options"
return
;;
esac
@@ -2081,7 +2077,7 @@ _git_send_email ()
return
;;
--*)
-   __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
+   __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd 
--chain-reply-to
--compose --confirm= --dry-run --envelope-sender
--from --identity
--in-reply-to --no-chain-reply-to --no-signed-off-by-cc
@@ -2090,7 +2086,7 @@ _git_send_email ()
--smtp-server-port --smtp-encryption= --smtp-user
--subject --suppress-cc= --suppress-from --thread --to
--validate --no-validate
-   $__git_format_patch_options"
+   $__git_format_patch_extra_options"
return
;;
esac
diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac337..ed0714eaaa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -119,6 +119,11 @@