Re: [ANNOUNCE] Git v2.12.0

[Willy Tarreau]

On Sat, Feb 25, 2017 at 12:31:21AM -0800, Junio C Hamano wrote:
> Willy Tarreau  writes:
> 
> > Hi Junio,
> >
> > On Fri, Feb 24, 2017 at 11:28:58AM -0800, Junio C Hamano wrote:
> >>  * Use of an empty string that is used for 'everything matches' is
> >>still warned and Git asks users to use a more explicit '.' for that
> >>instead.  The hope is that existing users will not mind this
> >>change, and eventually the warning can be turned into a hard error,
> >>upgrading the deprecation into removal of this (mis)feature.  That
> >>is not scheduled to happen in the upcoming release (yet).
> >
> > FWIW '.' is not equivalent to '' when it comes to grep or such commands,
> 
> I am amused and amazed ;-).  
> 
> The above is not about "grep" but was meant to describe "pathspec".
> We used to take "" as a pathspec element that means "every path
> matches", but recently started deprecating it and ask users to be
> more explicit by using "." (as a directory as a pathspec element
> matches everything inside the directory).  We are not changing the
> pattern matching done by "git grep" or "log --grep".  What is
> changing is that between the two that means the same thing:
> 
>   cd t/ && git log ""
>   cd t/ && git log .
> 
> the former is deprecated.

Ah that's fun indeed I never used it like this :-)

> I find it amusing that I have been writing the above in the draft
> release notes without realizing that I failed to say that it is
> about pathspec for quite some time, and without realizing that the
> above can be misinterpreted as if it is talking about grep patterns.
> 
> And I find it amazing that it took this long for somebody to spot
> that misleading vagueness in this description and point it out.
> 
> It should probably be updated to start like so:
> 
>   Use of an empty string as a pathspec element that is used
>   for 'everything matches' is ...

Hey it's the usual matter of perspective and context. When you know
what you do it's always obvious when you explain it while for others
something different is obvious :-)

Thanks for your clarification!
Willy

[PATCH 1/2] t: add an interoperability test harness

[Jeff King]

The current test suite is good at letting you test a
particular version of Git. But it's not very good at letting
you test _two_ versions and seeing how they interact (e.g.,
one cloning from the other).

This commit adds a test harness that will build two
arbitrary versions of git and make it easy to call them from
inside your tests. See the README and the example script for
details.

Signed-off-by: Jeff King 
---
 Makefile |  3 ++
 t/interop/.gitignore |  4 +++
 t/interop/Makefile   | 16 +
 t/interop/README | 85 
 t/interop/i-basic.sh | 27 ++
 t/interop/interop-lib.sh | 92 
 6 files changed, 227 insertions(+)
 create mode 100644 t/interop/.gitignore
 create mode 100644 t/interop/Makefile
 create mode 100644 t/interop/README
 create mode 100755 t/interop/i-basic.sh
 create mode 100644 t/interop/interop-lib.sh

diff --git a/Makefile b/Makefile
index 8e4081e06..7156b051e 100644
--- a/Makefile
+++ b/Makefile
@@ -2254,6 +2254,9 @@ endif
 ifdef GIT_PERF_MAKE_OPTS
@echo GIT_PERF_MAKE_OPTS=\''$(subst ','\'',$(subst 
','\'',$(GIT_PERF_MAKE_OPTS)))'\' >>$@+
 endif
+ifdef GIT_INTEROP_MAKE_OPTS
+   @echo GIT_INTEROP_MAKE_OPTS=\''$(subst ','\'',$(subst 
','\'',$(GIT_INTEROP_MAKE_OPTS)))'\' >>$@+
+endif
 ifdef TEST_GIT_INDEX_VERSION
@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst 
','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
 endif
diff --git a/t/interop/.gitignore b/t/interop/.gitignore
new file mode 100644
index 0..49c78d3db
--- /dev/null
+++ b/t/interop/.gitignore
@@ -0,0 +1,4 @@
+/trash directory*/
+/test-results/
+/.prove/
+/build/
diff --git a/t/interop/Makefile b/t/interop/Makefile
new file mode 100644
index 0..31a4bbc71
--- /dev/null
+++ b/t/interop/Makefile
@@ -0,0 +1,16 @@
+-include ../../config.mak
+export GIT_TEST_OPTIONS
+
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(sort $(wildcard i[0-9][0-9][0-9][0-9]-*.sh))
+
+all: $(T)
+
+$(T):
+   @echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+   rm -rf build "trash directory".* test-results
+
+.PHONY: all clean $(T)
diff --git a/t/interop/README b/t/interop/README
new file mode 100644
index 0..72d42bd85
--- /dev/null
+++ b/t/interop/README
@@ -0,0 +1,85 @@
+Git version interoperability tests
+==
+
+This directory has interoperability tests for git. Each script is
+similar to the normal test scripts found in t/, but with the added twist
+that two special versions of git, "git.a" and "git.b", are available in
+the PATH. Individual tests can then check the interaction between the
+two versions.
+
+When you add a feature that handles backwards compatibility between git
+versions, it's encouraged to add a test here to make sure it behaves as
+you expect.
+
+
+Running Tests
+-
+
+The easiest way to run tests is to say "make".  This runs all
+the tests against their default versions.
+
+You can run a single test like:
+
+$ ./i-basic.sh
+ok 1 - bare git is forbidden
+ok 2 - git.a version (v1.6.6.3)
+ok 3 - git.b version (v2.11.1)
+# passed all 3 test(s)
+1..3
+
+Each test contains default versions to run against. You may override
+these by setting `GIT_TEST_VERSION_A` and `GIT_TEST_VERSION_B` in the
+environment. Note that not all combinations will give sensible outcomes
+for all tests (e.g., a test checking for a specific old/new interaction
+may want something "old" enough" and something "new" enough; see
+individual tests for details).
+
+Version names should be resolvable as revisions in the current
+repository. They will be exported and built as needed using the
+config.mak files found at the root of your working tree.
+
+The exception is the special version "." which uses the currently-built
+contents of your working tree.
+
+You can set the following variables (in the environment or in your config.mak):
+
+GIT_INTEROP_MAKE_OPTS
+   Options to pass to `make` when building a git version (e.g.,
+   `-j8`).
+
+You can also pass any command-line options taken by ordinary git tests (e.g.,
+"-v").
+
+
+Naming Tests
+
+
+The interop test files are named like:
+
+   i-short-description.sh
+
+where N is a decimal digit.  The same conventions for choosing  as
+for normal tests apply.
+
+
+Writing Tests
+-
+
+An interop test script starts like a normal script, declaring a few
+variables and then including interop-lib.sh (which includes test-lib.sh).
+Besides test_description, you should also set the $VERSION_A and $VERSION_B
+variables to give the default versions to test against. See t-basic.sh for
+an example.
+
+You can then use test_expect_success as usual, with a few differences:
+
+  1. The special commands "git.a" and "git.b" correspond to the
+ two versions.
+
+  2. 

[PATCH] sha1_file: release fallback base's memory in unpack_entry()

[René Scharfe]

If a pack entry that's used as a delta base is corrupt, unpack_entry()
marks it as unusable and then searches the object again in the hope that
it can be found in another pack or in a loose file.  The memory for this
external base object is never released.  Free it after use.

Signed-off-by: Rene Scharfe 
---
 sha1_file.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index ec957db5e1..8ce80d4481 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2532,6 +2532,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
while (delta_stack_nr) {
void *delta_data;
void *base = data;
+   void *external_base = NULL;
unsigned long delta_size, base_size = size;
int i;
 
@@ -2558,6 +2559,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  p->pack_name);
mark_bad_packed_object(p, base_sha1);
base = read_object(base_sha1, , 
_size);
+   external_base = base;
}
}
 
@@ -2576,6 +2578,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  "at offset %"PRIuMAX" from %s",
  (uintmax_t)curpos, p->pack_name);
data = NULL;
+   free(external_base);
continue;
}
 
@@ -2595,6 +2598,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
error("failed to apply delta");
 
free(delta_data);
+   free(external_base);
}
 
*final_type = type;
-- 
2.12.0

[PATCH 2/2] t/interop: add test of old clients against modern git-daemon

[Jeff King]

This test just checks that old clients can clone and fetch
from a newer git-daemon. The opposite should also be true,
but it's hard to test ancient versions of git-daemon because
they lack basic options like "--listen".

Note that we have to make a slight tweak to the
lib-git-daemon helper from the regular tests, so that it
starts the daemon with our correct git.a version.

Signed-off-by: Jeff King 
---
 t/interop/i5500-git-daemon.sh | 41 +
 t/lib-git-daemon.sh   |  3 ++-
 2 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100755 t/interop/i5500-git-daemon.sh

diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh
new file mode 100755
index 0..1daf69420
--- /dev/null
+++ b/t/interop/i5500-git-daemon.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+VERSION_A=.
+VERSION_B=v1.0.0
+
+: ${LIB_GIT_DAEMON_PORT:=5500}
+LIB_GIT_DAEMON_COMMAND='git.a daemon'
+
+test_description='clone and fetch by older client'
+. ./interop-lib.sh
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+start_git_daemon --export-all
+
+repo=$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo
+
+test_expect_success "create repo served by $VERSION_A" '
+   git.a init "$repo" &&
+   git.a -C "$repo" commit --allow-empty -m one
+'
+
+test_expect_success "clone with $VERSION_B" '
+   git.b clone "$GIT_DAEMON_URL/repo" child &&
+   echo one >expect &&
+   git.a -C child log -1 --format=%s >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success "fetch with $VERSION_B" '
+   git.a -C "$repo" commit --allow-empty -m two &&
+   (
+   cd child &&
+   git.b fetch
+   ) &&
+   echo two >expect &&
+   git.a -C child log -1 --format=%s FETCH_HEAD >actual &&
+   test_cmp expect actual
+'
+
+stop_git_daemon
+test_done
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f9cbd4793..987d40680 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -46,7 +46,8 @@ start_git_daemon() {
 
say >&3 "Starting git daemon ..."
mkfifo git_daemon_output
-   git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+   ${LIB_GIT_DAEMON_COMMAND:-git daemon} \
+   --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
--reuseaddr --verbose \
--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-- 
2.12.0.616.g5f622f3b1

Re: [ANNOUNCE] Git v2.12.0

[Junio C Hamano]

Willy Tarreau  writes:

> Hi Junio,
>
> On Fri, Feb 24, 2017 at 11:28:58AM -0800, Junio C Hamano wrote:
>>  * Use of an empty string that is used for 'everything matches' is
>>still warned and Git asks users to use a more explicit '.' for that
>>instead.  The hope is that existing users will not mind this
>>change, and eventually the warning can be turned into a hard error,
>>upgrading the deprecation into removal of this (mis)feature.  That
>>is not scheduled to happen in the upcoming release (yet).
>
> FWIW '.' is not equivalent to '' when it comes to grep or such commands,

I am amused and amazed ;-).  

The above is not about "grep" but was meant to describe "pathspec".
We used to take "" as a pathspec element that means "every path
matches", but recently started deprecating it and ask users to be
more explicit by using "." (as a directory as a pathspec element
matches everything inside the directory).  We are not changing the
pattern matching done by "git grep" or "log --grep".  What is
changing is that between the two that means the same thing:

cd t/ && git log ""
cd t/ && git log .

the former is deprecated.

I find it amusing that I have been writing the above in the draft
release notes without realizing that I failed to say that it is
about pathspec for quite some time, and without realizing that the
above can be misinterpreted as if it is talking about grep patterns.

And I find it amazing that it took this long for somebody to spot
that misleading vagueness in this description and point it out.

It should probably be updated to start like so:

Use of an empty string as a pathspec element that is used
for 'everything matches' is ...

Thanks.

[PATCH 0/2] interoperability test harness

[Jeff King]

This series adds a small test harness for interoperability tests. The
heavy lifting is done by the normal test-lib.sh; this just makes it easy
for you to have access to two git versions at the same time.

This is something I've wanted a few times in the past when we make a
fix that can only be tested when interacting with a different version of
git.  As we start to work on changes like new protocols or hash
functions, this will hopefully make it easier to demonstrate what
happens when an older version of git encounters our new features.

This doesn't run when the regular test suite runs (it's likely to be a
bit flakier, as it actually has to build the alternative versions
separately). So I don't necessarily expect people to run it all the
time. But it lets us write down in a repeatable way the sorts of testing
that often ends up being done manually (or not at all) today.

This series just adds the harness and a basic test that we can still
clone from modern git using v1.0.0 (yay!). If people are interested, I
suspect there are previous cases that could be backfilled. A few I can
think of are:

  1. How older versions handle repositoryformatversion=2.

  2. Newer clients hitting older servers without various capabilities.
 One example is in:

   
http://public-inbox.org/git/1433961320-1366-1-git-send-email-ad...@google.com/

  3. Vice-versa: older clients without capabilities hitting newer
 servers (especially in exotic situations, like shallow clone).

I don't think there's a huge value in doing that for old changes unless
somebody has actively reported a problem. So only do it if it sounds
like a fun experiment. :)

  [1/2]: t: add an interoperability test harness
  [2/2]: t/interop: add test of old clients against modern git-daemon

 Makefile  |  3 ++
 t/interop/.gitignore  |  4 ++
 t/interop/Makefile| 16 
 t/interop/README  | 84 +++
 t/interop/i-basic.sh  | 27 +
 t/interop/i5500-git-daemon.sh | 41 +++
 t/interop/interop-lib.sh  | 92 +++
 t/lib-git-daemon.sh   |  3 +-
 8 files changed, 269 insertions(+), 1 deletion(-)
 create mode 100644 t/interop/.gitignore
 create mode 100644 t/interop/Makefile
 create mode 100644 t/interop/README
 create mode 100755 t/interop/i-basic.sh
 create mode 100755 t/interop/i5500-git-daemon.sh
 create mode 100644 t/interop/interop-lib.sh

-Peff

[PATCH 1/2] apply: guard against renames of non-existant empty files

[Vegard Nossum]

If we have a patch like the one in the new test-case, then we will
try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).

The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.

Found using AFL.

Signed-off-by: Vegard Nossum 
---
 apply.c |  3 ++-
 t/t4154-apply-git-header.sh | 15 +++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100755 t/t4154-apply-git-header.sh

diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
-   if (!patch->is_delete && !patch->new_name) {
+   if ((!patch->is_delete && !patch->new_name) ||
+   (patch->is_rename && !patch->old_name)) {
error(_("git diff header lacks filename 
information "
 "(line %d)"), state->linenr);
return -128;
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
new file mode 100755
index 0..d651af4a2
--- /dev/null
+++ b/t/t4154-apply-git-header.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='apply with git/--git headers'
+
+. ./test-lib.sh
+
+test_expect_success 'apply old mode / rename new' '
+   test_must_fail git apply << EOF
+diff --git a/1 b/1
+old mode 0
+rename new 0
+EOF
+'
+
+test_done
-- 
2.12.0.rc0

git status --> Out of memory, realloc failed

[Carsten Fuchs]

Dear Git group,

I use Git at a web hosting service, where my user account has a memory 
limit of 768 MB:


(uiserver):p7715773:~$ uname -a
Linux infongp-de15 3.14.0-ui16322-uiabi1-infong-amd64 #1 SMP Debian 
3.14.79-2~ui80+4 (2016-11-17) x86_64 GNU/Linux


(uiserver):p7715773:~$ git --version
git version 2.1.4

The problem is that `git status` fails with an out of memory error:

(uiserver):p7715773:~/cafu$ git status
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler

I talked to their support and their suggestion was to set a couple of 
memory constraints and to run `git gc`. This seemed to work well – but 
`git status` still fails:


(uiserver):p7715773:~/cafu$ cat ~/.gitconfig
[color]
ui = auto
[user]
name = Carsten Fuchs
email = carsten.fu...@cafu.de
[core]
editor = nano
pager = less -M -FRXS
packedgitwindowsize = 30m
packedgitlimit = 40m
[i18n]
commitEncoding = ISO-8859-1
[pack]
threads = 1
windowMemory = 10m
packSizeLimit = 20m
deltaCacheSize = 30m

(uiserver):p7715773:~/cafu$ git gc
Zähle Objekte: 44293, Fertig.
Komprimiere Objekte: 100% (24534/24534), Fertig.
Schreibe Objekte: 100% (44293/44293), Fertig.
Total 44293 (delta 17560), reused 41828 (delta 16708)

(uiserver):p7715773:~/cafu$ git status
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler

The repository is tracking about 19000 files which together take 260 MB.
The git server version is 2.7.4.1.g5468f9e (Bitbucket)

Well, their next response was that they have no solution for me – 
except, unsurprisingly, coaxing me into a more expensive hosting package.


I've read the Git man page about `git config`, but was not able to come 
up with anything to improve the situation.


Any ideas what I could do to reduce the memory consumption of `git status`?

Best regards,
Carsten

PS: Many thanks to Philip Oakley for initial advice at git-users, to 
which I should have properly subscribed in the first place, as the 
Google Groups interface seems to lose messages (mine at least, and 
inadvertently posts them as HTML) and gmane's NNTP interface reports 
that it is unidirectional/read-only.

[PATCH 2/2] apply: handle assertion failure gracefully

[Vegard Nossum]

For the patches in the added testcases, we were crashing with:

git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' 
failed.

As it turns out, check_preimage() is prepared to handle these conditions,
so we can remove the assertion.

Found using AFL.

Signed-off-by: Vegard Nossum 

---

(I'm fully aware of how it looks to just delete an assertion to "fix" a
bug without any other changes to accomodate the condition that was
being tested for. I am definitely not an expert on this code, but as far
as I can tell -- both by reviewing and testing the code -- the function
really is prepared to handle the case where patch->is_new == 1, as it
will always hit another error condition if that is true. I've tried to
add more test cases to show what errors you can expect to see instead of
the assertion failure when trying to apply these nonsensical patches. If
you don't want to remove the assertion for whatever reason, please feel
free to take the testcases and add "# TODO: known breakage" or whatever.)
---
 apply.c |  1 -
 t/t4154-apply-git-header.sh | 36 
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
if (!old_name)
return 0;
 
-   assert(patch->is_new <= 0);
previous = previous_patch(state, patch, );
 
if (status)
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
index d651af4a2..c440c48ad 100755
--- a/t/t4154-apply-git-header.sh
+++ b/t/t4154-apply-git-header.sh
@@ -12,4 +12,40 @@ rename new 0
 EOF
 '
 
+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+deleted file mode 
+new file mode 
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / wrong type' '
+   mkdir x &&
+   chmod 755 x &&
+   test_must_fail git apply << EOF
+diff --git a/x b/x
+deleted file mode 160755
+new file mode 
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / already exists' 
'
+   touch 1 &&
+   chmod 644 1 &&
+   test_must_fail git apply << EOF
+diff --git a/1 b/1
+deleted file mode 100644
+new file mode 
+EOF
+'
+
+test_expect_success 'apply new file mode / copy from / nonexistant file' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+new file mode 
+copy from  
+EOF
+'
+
 test_done
-- 
2.12.0.rc0

[PATCH] cocci: use ALLOC_ARRAY

[René Scharfe]

Add a semantic patch for using ALLOC_ARRAY to allocate arrays and apply
the transformation on the current source tree.  The macro checks for
multiplication overflow and infers the element size automatically; the
result is shorter and safer code.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/array.cocci | 16 
 worktree.c |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 2d7f25d99f..4ba98b7eaf 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -24,3 +24,19 @@ expression n;
 @@
 - memcpy(dst, src, n * sizeof(T));
 + COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(*ptr));
++ ALLOC_ARRAY(ptr, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(T));
++ ALLOC_ARRAY(ptr, n);
diff --git a/worktree.c b/worktree.c
index d633761575..d7b911aac7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -175,7 +175,7 @@ struct worktree **get_worktrees(unsigned flags)
struct dirent *d;
int counter = 0, alloc = 2;
 
-   list = xmalloc(alloc * sizeof(struct worktree *));
+   ALLOC_ARRAY(list, alloc);
 
list[counter++] = get_main_worktree();
 
-- 
2.12.0

Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

[Johannes Schindelin]

Hi,

On Wed, 22 Feb 2017, Jeff King wrote:

> [two beautiful patches]

I applied them and verified that the reported issue is fixed. Thank you!

Hopefully you do not mind that I cherry-picked them in preparation for
Git for Windows v2.12.0?

I added a small fixup (https://github.com/dscho/git/commit/44ae0bcae5):

-- snip --
Subject: [PATCH] fixup! http: add an "auto" mode for http.emptyauth

Note: we keep a "black list" of authentication methods for which we do
not want to enable http.emptyAuth automatically. A white list would be
nicer, but less robust, as we want to support linking to several cURL
versions and the list of authentication methods (as well as their names)
changed over time.

[jes: actually added the "auto" handling, excluded Digest, too]

This fixes https://github.com/git-for-windows/git/issues/1034

Signed-off-by: Johannes Schindelin 
---
 http.c | 55 +--
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/http.c b/http.c
index f8eb0f23d6c..fb94c444c80 100644
--- a/http.c
+++ b/http.c
@@ -334,7 +334,10 @@ static int http_options(const char *var, const char 
*value, void *cb)
return git_config_string(_agent, var, value);
 
if (!strcmp("http.emptyauth", var)) {
-   curl_empty_auth = git_config_bool(var, value);
+   if (value && !strcmp("auto", value))
+   curl_empty_auth = -1;
+   else
+   curl_empty_auth = git_config_bool(var, value);
return 0;
}
 
@@ -385,29 +388,37 @@ static int http_options(const char *var, const char 
*value, void *cb)
 
 static int curl_empty_auth_enabled(void)
 {
-   if (curl_empty_auth < 0) {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-   /*
-* In the automatic case, kick in the empty-auth
-* hack as long as we would potentially try some
-* method more exotic than "Basic".
-*
-* But only do so when this is _not_ our initial
-* request, as we would not then yet know what
-* methods are available.
-*/
-   return http_auth_methods_restricted &&
-  http_auth_methods != CURLAUTH_BASIC;
+   if (curl_empty_auth >= 0)
+   return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+   /*
+* Our libcurl is too old to do AUTH_ANY in the first place;
+* just default to turning the feature off.
+*/
 #else
-   /*
-* Our libcurl is too old to do AUTH_ANY in the first place;
-* just default to turning the feature off.
-*/
-   return 0;
+   /*
+* In the automatic case, kick in the empty-auth
+* hack as long as we would potentially try some
+* method more exotic than "Basic".
+*
+* But only do this when this is our second or
+* subsequent * request, as by then we know what
+* methods are available.
+*/
+   if (http_auth_methods_restricted)
+   switch (http_auth_methods) {
+   case CURLAUTH_BASIC:
+   case CURLAUTH_DIGEST:
+#ifdef CURLAUTH_DIGEST_IE
+   case CURLAUTH_DIGEST_IE:
 #endif
-   }
-
-   return curl_empty_auth;
+   return 0;
+   default:
+   return 1;
+   }
+#endif
+   return 0;
 }
 
 static void init_curl_http_auth(CURL *result)
-- snap --

As you can see, I actually implemented the handling for
http.emptyauth=auto, and I was more comfortable with handling the "easy"
cases first in the curl_empty_auth_enabled function.

I also took Dave's suggestion:

> On Thu, Feb 23, 2017 at 01:16:33AM +, David Turner wrote:
> 
> > > +  * But only do so when this is _not_ our initial
> > > +  * request, as we would not then yet know what
> > > +  * methods are available.
> > > +  */
> > 
> > Eliminate double-negative:
> > 
> > "But only do this when this is our second or subsequent request, 
> > as by then we know what methods are available."
> 
> Yeah, that is clearer.

Thank you all!

Now, how to get this into upstream Git, too? Jeff, do you want to submit a
v2? In that case, would you please consider the fixup! I mentioned above?
Otherwise I'd be happy to take it from here.

Ciao,
Dscho

Re: [PATCH] http(s): automatically try NTLM authentication first

[Johannes Schindelin]

Hi Peff,

On Thu, 23 Feb 2017, Jeff King wrote:

> For Git for Windows, [PATCH 2/2] seems like the auto behavior would be a
> strict improvement over the "true" default they've been shipping.

Absolutely. Thank you for your tremendous help!

Ciao,
Dscho

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Philip Oakley]

From: "Vegard Nossum" 

If we have a patch like the one in the new test-case, then we will


"the one in the new test-case" needs a clearer reference to the particular 
case so that future readers will know what it refers to. Noticed while 
browsing the commit message..


..reads further; Maybe it's "AFL (American fuzzy lop) found a failure. Add a 
new test case and fix the fault"?


[same for patch 2]


try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).

The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.

Found using AFL.

Signed-off-by: Vegard Nossum 
---
apply.c |  3 ++-
t/t4154-apply-git-header.sh | 15 +++
2 files changed, 17 insertions(+), 1 deletion(-)
create mode 100755 t/t4154-apply-git-header.sh

diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
 patch->old_name = xstrdup(patch->def_name);
 patch->new_name = xstrdup(patch->def_name);
 }
- if (!patch->is_delete && !patch->new_name) {
+ if ((!patch->is_delete && !patch->new_name) ||
+ (patch->is_rename && !patch->old_name)) {
 error(_("git diff header lacks filename information "
  "(line %d)"), state->linenr);
 return -128;
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
new file mode 100755
index 0..d651af4a2
--- /dev/null
+++ b/t/t4154-apply-git-header.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='apply with git/--git headers'
+
+. ./test-lib.sh
+
+test_expect_success 'apply old mode / rename new' '
+ test_must_fail git apply << EOF
+diff --git a/1 b/1
+old mode 0
+rename new 0
+EOF
+'
+
+test_done
--
2.12.0.rc0

--
Philip 

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Vegard Nossum]

On 25/02/2017 12:59, Philip Oakley wrote:

From: "Vegard Nossum" 

If we have a patch like the one in the new test-case, then we will


"the one in the new test-case" needs a clearer reference to the
particular case so that future readers will know what it refers to.
Noticed while browsing the commit message..


There is only one testcase added by this patch, so how is it possibly
unclear? In what situation would you read a commit message and not even
think to glance at the patch for more details?


Vegard

Re: fatal error when diffing changed symlinks

[Johannes Schindelin]

Hi Peff & Junio,

On Fri, 24 Feb 2017, Jeff King wrote:

> On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:
> 
> > > A slightly worse is that the upcoming Git will ship with a rewritten
> > > "difftool" that makes the above sequence segfault.
> > 
> > The culprit seems to be these lines in run_dir_diff():
> > 
> > if (S_ISLNK(lmode)) {
> > char *content = read_sha1_file(loid.hash, , );
> > add_left_or_right(, src_path, content, 0);
> > free(content);
> > }
> > 
> > if (S_ISLNK(rmode)) {
> > char *content = read_sha1_file(roid.hash, , );
> > add_left_or_right(, dst_path, content, 1);
> > free(content);
> > }
> > 
> > When viewing a working tree file, oid.hash could be 0{40} and
> > read_sha1_file() is not the right function to use to obtain the
> > contents.
> > 
> > Both of these two need to pay attention to 0{40}, I think, as the
> > user may be running "difftool -R --dir-diff" in which case the
> > working tree would appear in the left hand side instead.
> 
> As a side note, I think even outside of 0{40}, this should be checking
> the return value of read_sha1_file(). A corrupted repo should die(), not
> segfault.

I agree. I am on it.

Ciao,
Dscho

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Philip Oakley]

From: "Vegard Nossum" 

On 25/02/2017 12:59, Philip Oakley wrote:

From: "Vegard Nossum" 

If we have a patch like the one in the new test-case, then we will


"the one in the new test-case" needs a clearer reference to the
particular case so that future readers will know what it refers to.
Noticed while browsing the commit message..


There is only one testcase added by this patch, so how is it possibly
unclear? In what situation would you read a commit message and not even
think to glance at the patch for more details?

On initial reading of a commit message, the expectation is that the commit 
will be about a change from some previous state, so I immediately asked 
myself, where is that new (recent) test case from.


You could say "This patch presents a new test case" which would straight 
away set the expectation that one should read on to see what its about. It 
was just that as a reader of the log message I didn't pick up the sense you 
wanted to convey. It's easy to see with hindsight or fore-knowledge.


I, personally, think that bringing the AFL discovery to the fore would help 
in explaining why/how the patch appeared in the first place.


Hope that helps explain why I responded.

regards

Philip 

FROM AISHA GADDAFI

[aishagadaf...@ono.com]

for more details,contact me at my private email : agaddafi752(at)gmail.
com
Dr. Aisha Gaddafi, the daughter of late Libyan president, I need a 
partner or an investor that will help me in investing the sum of $87.5 
million USD in his or her country, the funds is deposited here in 
Burkina Faso where I am staying for the moment with my family, I would 
like to know your mind tows the investment  of the total sum mentioned 
above, note you will received  30% of the total sum after the transfer 
is completed into your account, 
for more details,contact me at my private email : agaddafi752(at)gmail.
com
Yours friend Dr. Aisha Gaddafi
 
FROM AISHA GADDAFI

Selamlarım sana

[Rachel Washburn]

Selamlarım sana

Ben çavuşum. Rachel Washburn, umarım hepiniz iyi olur. Birleşmiş Milletler 
barış muhafızları Afganistan'da terörle savaş için çalışmadan önce kadın bir 
askerim ancak geçen yıl Eylül 2016'da askeri yönetim devrilmesine karşı özel 
görev yapmak üzere Burkina Faso'ya taşındı. Afganistan 2015'te orada 
yaptırdığım toplam 3.5 milyon dolarlık mülkümde, bu parayı Kızıl Haç ajanıyla 
yatırıyordum. Yararlanıcı olarak durup fonu almanı ve güvenliğini sağlamanı 
istiyorum, Burkina Faso'daki göreve gelince en kısa zamanda iyi bir Karlı 
Girişim'e yatırım yapmama yardım edeceksin ya da benim için bunu benim için 
tutarsın Ülkenize geliyorum, parayı aldıktan sonra size yardım için toplam 
paranın% 40'ını vereceğim. Benimle çalışmaya istekli iseniz, parayı 
yatırdığınız yerden bilgi gönderebilmem için lütfen cevap verin, acil yanıtınız 
gerekiyor.

Re: [PATCH v6 0/6] stash: support pathspec argument

[Thomas Gummerer]

On 02/20, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > @@ -55,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] 
> > [-u|--include-untracked] [-a|--all] [-q
> >  
> > Save your local modifications to a new 'stash', and run `git reset
> > --hard` to revert them.  The  part is optional and gives
> 
> I didn't notice this during v5 review, but the above seems to be
> based on the codebase before your documentation update (which used
> to be part of the series in older iteration).  I had to tweak the
> series to apply on top of your 20a7e06172 ("Documentation/stash:
> remove mention of git reset --hard", 2017-02-12) while queuing, so
> please double check the result when it is pushed out to 'pu'.

Sorry about that.  What you queued looks good to me, I will however
send a re-roll based on your comments on 4/6.  (And I'll make sure to
base it on 20a7e06172)

[PATCH] strbuf: add strbuf_add_real_path()

[René Scharfe]

Add a function for appending the canonized absolute pathname of a given
path to a strbuf.  It keeps the existing contents intact, as expected of
a function of the strbuf_add() family, while avoiding copying the result
if the given strbuf is empty.  It's more consistent with the rest of the
strbuf API than strbuf_realpath(), which it's wrapping.

Also add a semantic patch demonstrating its intended usage and apply it
to the current tree.  Using strbuf_add_real_path() instead of calling
strbuf_addstr() and real_path() avoids an extra copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/strbuf.cocci |  6 ++
 setup.c |  2 +-
 strbuf.c| 11 +++
 strbuf.h| 14 ++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 63995f22ff..1d580e49b0 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -38,3 +38,9 @@ expression E1, E2, E3;
 @@
 - strbuf_addstr(E1, find_unique_abbrev(E2, E3));
 + strbuf_add_unique_abbrev(E1, E2, E3);
+
+@@
+expression E1, E2;
+@@
+- strbuf_addstr(E1, real_path(E2));
++ strbuf_add_real_path(E1, E2);
diff --git a/setup.c b/setup.c
index 967f289f1e..f14cbcd338 100644
--- a/setup.c
+++ b/setup.c
@@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char 
*gitdir)
if (!is_absolute_path(data.buf))
strbuf_addf(, "%s/", gitdir);
strbuf_addbuf(, );
-   strbuf_addstr(sb, real_path(path.buf));
+   strbuf_add_real_path(sb, path.buf);
ret = 1;
} else {
strbuf_addstr(sb, gitdir);
diff --git a/strbuf.c b/strbuf.cq
index 8fec6579f7..ace58e7367 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const 
char *path)
strbuf_addstr(sb, path);
 }
 
+void strbuf_add_real_path(struct strbuf *sb, const char *path)
+{
+   if (sb->len) {
+   struct strbuf resolved = STRBUF_INIT;
+   strbuf_realpath(, path, 1);
+   strbuf_addbuf(sb, );
+   strbuf_release();
+   } else
+   strbuf_realpath(sb, path, 1);
+}
+
 int printf_ln(const char *fmt, ...)
 {
int ret;
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e7..cf8e4bf532 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);
  */
 extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
 
+/**
+ * Canonize `path` (make it absolute, resolve symlinks, remove extra
+ * slashes) and append it to `sb`.  Die with an informative error
+ * message if there is a problem.
+ *
+ * The directory part of `path` (i.e., everything up to the last
+ * dir_sep) must denote a valid, existing directory, but the last
+ * component need not exist.
+ *
+ * Callers that don't mind links should use the more lightweight
+ * strbuf_add_absolute_path() instead.
+ */
+extern void strbuf_add_real_path(struct strbuf *sb, const char *path);
+
 
 /**
  * Normalize in-place the path contained in the strbuf. See
-- 
2.12.0

Re: git diff --quiet exits with 1 on clean tree with CRLF conversions

[Mike Crowe]

On Monday 20 February 2017 at 13:25:01 -0800, Junio C Hamano wrote:
> This almost makes me suspect that the place that checks lengths of
> one and two in order to refrain from running more expensive content
> comparison you found earlier need to ask would_convert_to_git()
> before taking the short-cut, something along the lines of this (for
> illustration purposes only, not even compile-tested).  The "almost"
> comes to me because I do not offhand know the performance implications
> of making calls to would_convert_to_git() here.
> 
>  diff.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 051761be40..094d5913da 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct 
> diff_filepair *p)
>*differences.
>*
>* 2. At this point, the file is known to be modified,
> -  *with the same mode and size, and the object
> -  *name of one side is unknown.  Need to inspect
> -  *the identical contents.
> +  *with the same mode and size, the object
> +  *name of one side is unknown, or size comparison
> +  *cannot be depended upon.  Need to inspect the 
> +  *contents.
>*/
>   if (!DIFF_FILE_VALID(p->one) || /* (1) */
>   !DIFF_FILE_VALID(p->two) ||
> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct 
> diff_filepair *p)
>   (p->one->mode != p->two->mode) ||
>   diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
>   diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> - (p->one->size != p->two->size) ||
> +
> + /* 
> +  * only if eol and other conversions are not involved,
> +  * we can say that two contents of different sizes
> +  * cannot be the same without checking their contents.
> +  */
> + (!would_convert_to_git(p->one->path) &&
> +  !would_convert_to_git(p->two->path) &&
> +  (p->one->size != p->two->size)) ||
> +
>   !diff_filespec_is_identical(p->one, p->two)) /* (2) */
>   p->skip_stat_unmatch_result = 1;
>   return p->skip_stat_unmatch_result;
> 
> 

Thanks for investigating this. I think you are correct that I was misguided
in my previous "fix". However, your change above does fix the problem for
me.

It looks like the main cost of convert_to_git is in convert_attrs which
ends up doing various path operations in attr.c. After that, both
apply_filter and crlf_to_git return straight away if there's nothing to do.

I experimented several times with running "git diff -quiet" after touching
every file in Git's own worktree and any difference in total time was lost
in the noise.

I've further improved my test case. Tests 3 and 4 fail without the above
change but pass with it. Unfortunately I'm still unable to get those tests
to fail without the above fix unless the sleeps are present. I've tried
using the "touch -r .datetime" technique from racy-git.txt but it doesn't
help. It seems that I'm unable to stop Git from using its cache without
sleeping. :(

diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh
new file mode 100755
index 000..31a730d
--- /dev/null
+++ b/t/t4063-diff-converted.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+# These tests ensure that files changing line endings in the presence
+# of .gitattributes to indicate that line endings should be ignored
+# don't cause 'git diff' or 'git diff --quiet' to think that they have
+# been changed.
+#
+# The sleeps are necessary to reproduce the problem for reasons that I
+# don't understand.
+
+test_description='git diff with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   echo "* text=auto" > .gitattributes &&
+   printf "Hello\r\nWorld\r\n" > crlf.txt &&
+   printf "Hello\nWorld\n" > lf.txt &&
+   git add .gitattributes crlf.txt lf.txt &&
+   git commit -m "initial" && echo three
+'
+test_expect_success 'noisy diff works on file that requires CRLF conversion' '
+   git status >/dev/null &&
+   git diff >/dev/null &&
+   sleep 1 &&
+   touch crlf.txt lf.txt &&
+   git diff >/dev/null
+'
+test_expect_success 'quiet diff works on file that requires CRLF conversion 
with no changes' '
+   git status &&
+   git diff --quiet &&
+   sleep 1 &&
+   touch crlf.txt lf.txt &&
+   git diff --quiet
+'
+
+test_expect_success 'quiet diff works on file with line-ending change that has 
no effect on repository' '
+   printf "Hello\nWorld\n" > crlf.txt &&
+   printf "Hello\r\nWorld\r\n" > lf.txt &&
+   git diff --quiet
+'
+test_done


Mike.

[ANNOUNCE] Git for Windows 2.12.0

[Johannes Schindelin]

MIME-Version: 1.0

Fcc: Sent

Dear Git users,

It is my pleasure to announce that Git for Windows 2.12.0 is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.11.1 (February 3rd 2017)

New Features

  • Comes with Git v2.12.0.
  • The builtin difftool is no longer opt-in, as it graduated to be
officially adopted by the Git project.
  • Comes with v2.7.0 of the POSIX emulation layer based on the Cygwin
runtime.
  • Includes cURL 7.53.1.
  • The Portable Git now defaults to using the included Git Credential
Manager.

Bug Fixes

  • The stderr output is unbuffered again, i.e. errors are displayed
immediately (this was reported on the Git mailing list as well as
issues #1064, #1064, #1068).
  • Git can clone again from paths containing non-ASCII characters.
  • We no longer ship two different versions of curl.exe.
  • Hitting Ctrl+T in Git GUI even after all files have been (un)staged
no longer throws an exception.
  • A couple of Git GUI bugs regarding the list of recent repositories
have been fixed.
  • The git-bash.exe helper now waits again for the terminal to be
closed before returning.
  • Git for Windows no longer attempts to send empty credentials to
HTTP(S) servers that handle only Basic and/or Digest authentication
.
Filename | SHA-256
 | ---
Git-2.12.0-64-bit.exe | 
0224c1cf4ff48535fdfc2555175be9a06c6d8b67fbf208b1c524f01252f8b13b
Git-2.12.0-32-bit.exe | 
de7f69bd6313bf7b427b02687a0ee930012a32d40aed2bb96f428699c936180d
PortableGit-2.12.0-64-bit.7z.exe | 
5bebd0ee21e5cf3976bc71826a28b2663c7a0c9b5c98f4ab46ff03c3c0d3556f
PortableGit-2.12.0-32-bit.7z.exe | 
0375ba0a05f9cd501cc8089b9af6f2adf8904a5efb1e5b9421e6561bd9f8c817
MinGit-2.12.0-64-bit.zip | 
6238f65c4d8412b993cb092efde4954f8cb7da4def54d0c1533835f00e83fdad
MinGit-2.12.0-32-bit.zip | 
5a118ff8a8f859866d6874261fc8ec685848a2ccf9fa0858417c98e21f5c0ec3
Git-2.12.0-64-bit.tar.bz2 | 
b512fb28ceeddb6a6cdf15e6c936aea15fd2b1b3c8154f72101f8c9060549f90
Git-2.12.0-32-bit.tar.bz2 | 
a6c0b5b36c19a70f2c9ffd8fbfeb57bedbb7a9a2207672ac38c5bc5a38ae320a
Ciao,
Johannes

Re: SHA1 collisions found

[brian m. carlson]

On Fri, Feb 24, 2017 at 09:32:13AM -0800, Junio C Hamano wrote:
> Ian Jackson  writes:
> 
> > I have been thinking about how to do a transition from SHA1 to another
> > hash function.
> 
> Good.  I think many of us have also been, too, not necessarily just
> in the past few days in response to shattered, but over the last 10
> years, yet without coming to a consensus design ;-)
> 
> > I have concluded that:
> >
> >  * We can should avoid expecting everyone to rewrite all their
> >history.
> 
> Yes.

There are security implications for old objects if we mix hashes, but I
suppose people who want better security will just rewrite history
anyway.

> As long as the reader can tell from the format of object names
> stored in the "new object format" object from what era is being
> referred to in some way [*1*], we can name new objects with only new
> hash, I would think.  "new refers only to new" that stratifies
> objects into older and newer may make things simpler, but I am not
> convinced yet that it would give our users a smooth enough
> transition path (but I am open to be educated and pursuaded the
> other way).

I would simply use multihash[0] for this purpose.  New-style objects
serialize data in multihash format, so it's immediately obvious what
hash we're referring to.  That makes future transitions less
problematic.

[0] https://github.com/multiformats/multihash
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: SHA1 collisions found

[brian m. carlson]

On Fri, Feb 24, 2017 at 04:42:38PM +0700, Duy Nguyen wrote:
> On Thu, Feb 23, 2017 at 11:43 PM, Joey Hess  wrote:
> > IIRC someone has been working on parameterizing git's SHA1 assumptions
> > so a repository could eventually use a more secure hash. How far has
> > that gotten? There are still many "40" constants in git.git HEAD.
> 
> Michael asked Brian (that "someone") the other day and he replied [1]
> 
> >> I'm curious; what fraction of the overall convert-to-object_id campaign
> >> do you estimate is done so far? Are you getting close to the promised
> >> land yet?
> >
> > So I think that the current scope left is best estimated by the
> > following command:
> >
> >   git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'
> >
> > So there are approximately 1200 call sites left, which is quite a bit of
> > work.  I estimate between the work I've done and other people's
> > refactoring work (such as the refs backend refactor), we're about 40%
> > done.

As a note, I've been working on this pretty much nonstop since the
collision announcement was made.  After another 27 commits, I've got it
down from 1244 to 1119.

I plan to send another series out sometime after the existing series has
hit next.  People who are interested can follow the object-id-part*
branches at https://github.com/bk2204/git.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

git-clone --config order & fetching extra refs during initial clone

[Robin H. Johnson]

TL;DR: git-clone ignores any fetch specs passed via --config.

The documentation for git-clone --config says:
| Set a configuration variable in the newly-created repository; this takes
| effect immediately __AFTER__ the repository is initialized, but __BEFORE__
| the remote history is fetched or any files checked out. [...]
(emphasis added)

However, this doesn't seem be be true, right after the clone, the refs are NOT
present, and the next fetch seems to pull the extra refs. This seems to be
because the refspec building for the initial clone doesn't take into account
any fetch lines added to the config.

Testcase to reproduce (confirmed in v2.11.1, not tested 2.12.0 quite yet):
# export REPOURI=https://github.com/openstack-dev/sandbox.git DIR=test
# git clone \
-c remote.origin.fetch=+refs/notes/*:refs/notes/* \
-c remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/* \
$REPOURI $DIR \
  && cd $DIR \
  && git fetch

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Trustee & Treasurer
E-Mail   : robb...@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136


signature.asc
Description: Digital signature

Re: SHA1 collisions found

[Jeff King]

On Sat, Feb 25, 2017 at 06:50:50PM +, brian m. carlson wrote:

> > As long as the reader can tell from the format of object names
> > stored in the "new object format" object from what era is being
> > referred to in some way [*1*], we can name new objects with only new
> > hash, I would think.  "new refers only to new" that stratifies
> > objects into older and newer may make things simpler, but I am not
> > convinced yet that it would give our users a smooth enough
> > transition path (but I am open to be educated and pursuaded the
> > other way).
> 
> I would simply use multihash[0] for this purpose.  New-style objects
> serialize data in multihash format, so it's immediately obvious what
> hash we're referring to.  That makes future transitions less
> problematic.
> 
> [0] https://github.com/multiformats/multihash

I looked at that earlier, because I think it's a reasonable idea for
future-proofing. The first byte is a "varint", but I couldn't find where
they defined that format.

The closest I could find is:

  https://github.com/multiformats/unsigned-varint

whose README says:

  This unsigned varint (VARiable INTeger) format is for the use in all
  the multiformats.

- We have not yet decided on a format yet. When we do, this readme
  will be updated.

- We have time. All multiformats are far from requiring this varint.

which is not exactly confidence inspiring. They also put the length at
the front of the hash. That's probably convenient if you're parsing an
unknown set of hashes, but I'm not sure it's helpful inside Git objects.
And there's an incentive to minimize header data at the front of a hash,
because every byte is one more byte that every single hash will collide
over, and people will have to type when passing hashes to "git show",
etc.

I'd almost rather use something _really_ verbose like

  sha256:1234abcd...

in all of the objects. And then when we get an unadorned hash from the
user, we guess it's sha256 (or whatever), and fallback to treating it as
a sha1.

Using a syntactically-obvious name like that also solves one other
problem: there are sha1 hashes whose first bytes will encode as a "this
is sha256" multihash, creating some ambiguity.

-Peff

Re: [PATCH 1/2] commit: be more precise when searching for headers

[Jeff King]

On Sat, Feb 25, 2017 at 08:21:52PM +0100, René Scharfe wrote:

> Search for a space character only within the current line in
> read_commit_extra_header_lines() instead of searching in the whole
> buffer (and possibly beyond, if it's not NUL-terminated) and then
> discarding any results after the end of the current line.
> [...]
> - eof = strchr(line, ' ');
> - if (next <= eof)
> + eof = memchr(line, ' ', next - line);
> + if (!eof)
>   eof = next;

Nice. More efficient, and I think the intent is more clear.

-Peff

Re: git-clone --config order & fetching extra refs during initial clone

[Jeff King]

On Sat, Feb 25, 2017 at 07:12:38PM +, Robin H. Johnson wrote:

> TL;DR: git-clone ignores any fetch specs passed via --config.

I agree that this is a bug. There's some previous discussion and an RFC
patch from lat March (author cc'd):

  
http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/

That discussion veered off into alternatives, but I think the v2 posted
in that thread is taking a sane approach.

-Peff

[PATCH v7 1/6] stash: introduce push verb

[Thomas Gummerer]

Introduce a new git stash push verb in addition to git stash save.  The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  3 +++
 git-stash.sh| 46 ++---
 t/t3903-stash.sh|  9 +
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..d240df4af7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,6 +15,8 @@ SYNOPSIS
 'git stash' branch  []
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] []]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] [-m|--message ]]
 'git stash' clear
 'git stash' create []
 'git stash' store [-m|--message ] [-q|--quiet] 
@@ -46,6 +48,7 @@ OPTIONS
 ---
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ]::
 
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list []
or: $dashless branch  []
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
   [-u|--include-untracked] [-a|--all] []]
+   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m ]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
return $ret
 }
 
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+   stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
-a|--all)
untracked=all
;;
+   -m|--message)
+   shift
+   test -z ${1+x} && usage
+   stash_msg=$1
+   ;;
--help)
show_help
;;
@@ -251,8 +259,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or 
--all at the same time")"
fi
 
-   stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +297,36 @@ save_stash () {
fi
 }
 
+save_stash () {
+   push_options=
+   while test $# != 0
+   do
+   case "$1" in
+   --)
+   shift
+   break
+   ;;
+   -*)
+   # pass all options through to push_stash
+   push_options="$push_options $1"
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   stash_msg="$*"
+
+   if test -z "$stash_msg"
+   then
+   push_stash $push_options
+   else
+   push_stash $push_options -m "$stash_msg"
+   fi
+}
+
 have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +653,10 @@ save)
shift
save_stash "$@"
;;
+push)
+   shift
+   push_stash "$@"
+   ;;
 apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial 
renames' '
test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+   >foo &&
+   git add foo &&
+   git stash push -m "test message" &&
+   echo "stash@{0}: On master: test message" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g275aeb250c.dirty

[PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec

[Thomas Gummerer]

While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone.  Unfortunately
git currently offers no such option.  git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.

Allow 'git stash push' to take pathspec to specify which paths to stash.

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt|  9 -
 git-stash.sh   | 37 +--
 t/t3903-stash.sh   | 76 ++
 t/t3905-stash-include-untracked.sh | 26 +
 4 files changed, 136 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index d240df4af7..88369ed8b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [-u|--include-untracked] [-a|--all] []]
 'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [-m|--message ]]
+[--] [...]
 'git stash' clear
 'git stash' create []
 'git stash' store [-m|--message ] [-q|--quiet] 
@@ -48,7 +49,7 @@ OPTIONS
 ---
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
-push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
@@ -58,6 +59,12 @@ push [-p|--patch] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q
only  does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
 +
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
 +
diff --git a/git-stash.sh b/git-stash.sh
index ef5d1b45be..57828f926d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list []
   [-u|--include-untracked] [-a|--all] []]
or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
  [-u|--include-untracked] [-a|--all] [-m ]
+ [-- ...]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -35,15 +36,15 @@ else
 fi
 
 no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-   git diff-files --quiet --ignore-submodules &&
+   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+   git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
 untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
-   git ls-files -o -z $excl_opt
+   git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -71,12 +72,16 @@ create_stash () {
shift
untracked=${1?"BUG: create_stash () -u requires an 
argument"}
;;
+   --)
+   shift
+   break
+   ;;
esac
shift
done
 
git update-index -q --refresh
-   if no_changes
+   if no_changes "$@"
then
exit 0
fi
@@ -108,7 +113,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless 
commit, for
# ease of unpacking later.
u_commit=$(
-   untracked_files | (
+   untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -131,7 +136,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
-   git diff-index --name-only -z HEAD -- 
>"$TMP-stagenames" &&
+   git diff-index --name-only -z HEAD -- "$@" 
>"$TMP-stagenames" &&
git update-index -z --add --remove --stdin 
<"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -145,7 +150,7 @@ create_stash () {
 
# find out what 

[PATCH v7 5/6] stash: use stash_push for no verb form

[Thomas Gummerer]

Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.

Previously we allowed git stash -- -message, which is no longer allowed
after this patch.  Messages starting with a hyphen was allowed since
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options").  However it was never the intent to allow that, but rather it
was allowed accidentally.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  8 
 git-stash.sh| 16 
 t/t3903-stash.sh|  4 +---
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 88369ed8b6..4d8d30f179 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-[-u|--include-untracked] [-a|--all] []]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] []
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [-m|--message ]]
-[--] [...]
+[--] [...]]
 'git stash' clear
 'git stash' create []
 'git stash' store [-m|--message ] [-q|--quiet] 
diff --git a/git-stash.sh b/git-stash.sh
index 57828f926d..2d7b30ec5e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list []
or: $dashless drop [-q|--quiet] []
or: $dashless ( pop | apply ) [--index] [-q|--quiet] []
or: $dashless branch  []
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-  [-u|--include-untracked] [-a|--all] []]
-   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [-m ]
- [-- ...]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] []
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+  [-u|--include-untracked] [-a|--all] [-m ]
+  [-- ...]]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -656,7 +656,7 @@ apply_to_branch () {
 }
 
 PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
@@ -666,7 +666,7 @@ do
esac
 done
 
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
 
 # Main command set
 case "$1" in
@@ -717,7 +717,7 @@ branch)
 *)
case $# in
0)
-   save_stash &&
+   push_stash &&
say "$(gettext "(To restore them type \"git stash apply\")")"
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4d8a096773..2f5888df0d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
git add file2 &&
test_must_fail git stash --invalid-option &&
test_must_fail git stash save --invalid-option &&
-   test bar5,bar6 = $(cat file),$(cat file2) &&
-   git stash -- -message-starting-with-dash &&
-   test bar,bar2 = $(cat file),$(cat file2)
+   test bar5,bar6 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash an added file' '
-- 
2.11.0.301.g275aeb250c.dirty

Re: [PATCH] sha1_file: release fallback base's memory in unpack_entry()

[Jeff King]

On Sat, Feb 25, 2017 at 11:02:28AM +0100, René Scharfe wrote:

> If a pack entry that's used as a delta base is corrupt, unpack_entry()
> marks it as unusable and then searches the object again in the hope that
> it can be found in another pack or in a loose file.  The memory for this
> external base object is never released.  Free it after use.

This looks good. I wondered if there were any tricks with passing the
resulting base to the delta-base cache. But I don't think so. We add to
the cache right _before_ the fallback check we're looking at here. And
the "base" variable does not persist to the next loop.

Arguably, one could do the fallback first and then add the result to the
delta base cache, but it probably isn't worth the trouble. However we
read the object via read_object() would hopefully have done so already.

-Peff

[PATCH] http: add an "auto" mode for http.emptyauth

[Jeff King]

This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.

However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.

Instead, let's introduce an automatic mode, that works like
this:

  1. We won't try to send the bogus credential on the first
 request. We'll wait to get an HTTP 401, as usual.

  2. After seeing an HTTP 401, the empty-auth hack will kick
 in only when we know there is an auth method available
 that might make use of it (i.e., something besides
 "Basic" or "Digest").

That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.

This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).

The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.

Helped-by: Johannes Schindelin 
Signed-off-by: Jeff King 
---
And here's the full patch. It is meant to go on top of the
already-queued 1/2, though I suspect it could apply separately.

Test reports welcome from people who actually have NTLM or Kerberos
servers. The changes from the previous are fairly minimal, but this kind
of bit-mangling is exactly the kind of thing where I tend to
accidentally invert the logic. ;)

 http.c | 50 +-
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index a05609766..dd637d031 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
@@ -125,6 +125,14 @@ static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+   CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+   | CURLAUTH_DIGEST_IE
+#endif
+   | CURLAUTH_DIGEST;
 #endif
 
 static struct curl_slist *pragma_header;
@@ -333,7 +341,10 @@ static int http_options(const char *var, const char 
*value, void *cb)
return git_config_string(_agent, var, value);
 
if (!strcmp("http.emptyauth", var)) {
-   curl_empty_auth = git_config_bool(var, value);
+   if (value && !strcmp("auto", value))
+   curl_empty_auth = -1;
+   else
+   curl_empty_auth = git_config_bool(var, value);
return 0;
}
 
@@ -382,10 +393,37 @@ static int http_options(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
+static int curl_empty_auth_enabled(void)
+{
+   if (curl_empty_auth >= 0)
+   return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+   /*
+* Our libcurl is too old to do AUTH_ANY in the first place;
+* just default to turning the feature off.
+*/
+#else
+   /*
+* In the automatic case, kick in the empty-auth
+* hack as long as we would potentially try some
+* method more exotic than "Basic" or "Digest".
+*
+* But only do this when this is our second or
+* subsequent * request, as by then we know what
+* methods are available.
+*/
+   if (http_auth_methods_restricted &&
+   (http_auth_methods & ~empty_auth_useless))
+   return 1;
+#endif
+   return 0;
+}
+
 static void init_curl_http_auth(CURL *result)
 {
if (!http_auth.username || !*http_auth.username) {
-   if (curl_empty_auth)
+   if (curl_empty_auth_enabled())
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
}
@@ -1079,7 +1117,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-   if (http_auth.password || curl_empty_auth)
+   if (http_auth.password || curl_empty_auth_enabled())
init_curl_http_auth(slot->curl);
 
return slot;
@@ -1347,8 +1385,10 @@ static int handle_curl_result(struct slot_results 
*results)
} else {
 #ifdef 

Re: [PATCH v6 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec

[Thomas Gummerer]

On 02/21, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > -   git reset --hard ${GIT_QUIET:+-q}
> 
> This hunk is probably the most important one to review in the whole
> series, in the sense that these are entirely new code that didn't
> exist in the original.
> 
> > +   if test $# != 0
> > +   then
> > +   saved_untracked=
> > +   if test -n "$(git ls-files --others -- "$@")"
> 
> I notice that "ls-files -o" used in the code before this series are
> almost always used with --exclude-standard but we do not set up the
> standard exclude pattern here.  Is there a good reason to use (or
> not to use) it here as well?

We probably should use it, not adding it was an oversight.

> > +   then
> > +   saved_untracked=$(
> > +   git ls-files -z --others -- "$@" |
> > +   xargs -0 git stash create -u all --)
> > +   fi
> 
> Running the same ls-files twice look somewhat wasteful.
> 
> I suspect that we avoid "xargs -0" in our code from portability
> concern (isn't it a GNU invention?)
> 
> > +   git ls-files -z -- "$@" | xargs -0 git reset 
> > ${GIT_QUIET:+-q} --
> 
> Hmm, am I being naive to suspect that the above is a roundabout way
> to say:
> 
>   git reset ${GIT_QUIET:+-q} -- "$@"
> 
> or is an effect quite different from that intended here?
> 
> > +   git ls-files -z --modified -- "$@" | xargs -0 git 
> > checkout ${GIT_QUIET:+-q} HEAD --
> 
> Likewise.  Wouldn't the above be equivalent to:
> 
>   git checkout ${GIT_QUIET:+-q} HEAD -- "$@"
> 
> Or is this trying to preserve paths modified in the working tree and
> fully added to the index?.

No, it's not trying to do that (all the paths we're touching here
would have been "reset" earlier, so it wouldn't change anything.
However what this code tried to do is to allow "stash push -- path
pathspec-not-in-repo", where "pathspec-not-in-repo" would end up
tripping up "checkout" which does not accept pathspecs that are not in
the index.

I think we should disallow such pathspecs in stash as well, except in
the --include-untracked case, where it still makes sense.

This means we can't get rid of the "ls-files --modified" here, but we
can in all other places, and get rid of the "stash_create"
"stash_apply" dance to store the untracked files, simplifying this
part quite a bit.

Will re-roll.

> 
> > +   if test -n "$(git ls-files -z --others -- "$@")"
> > +   then
> > +   git ls-files -z --others -- "$@" | xargs -0 git 
> > clean --force -d ${GIT_QUIET:+-q} --
> 
> Likewise.  "ls-files --others" being the major part of what "clean"
> is about, I suspect the "ls-files piped to clean" is redundant.  Do
> you even need a test?  IOW, doesn't "git clean" with a pathspec that
> does not match anything silently succeed without doing anything
> harmful?
>
> > +   fi
> > +   if test -n "$saved_untracked"
> > +   then
> > +   git stash pop -q $saved_untracked
> 
> I see this thing was "created", and the whole point of "create" is
> to be different from "save/push" that automatically adds the result
> to the stash reflog.  Should we be "pop"ing it, or did you mean to
> just call apply_stash on it?
>
> > +   fi
> > +   else
> > +   git reset --hard ${GIT_QUIET:+-q}
> > +   fi
> 

Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

[Jeff King]

On Sat, Feb 25, 2017 at 12:48:54PM +0100, Johannes Schindelin wrote:

> Hi,
> 
> On Wed, 22 Feb 2017, Jeff King wrote:
> 
> > [two beautiful patches]
> 
> I applied them and verified that the reported issue is fixed. Thank you!
> 
> Hopefully you do not mind that I cherry-picked them in preparation for
> Git for Windows v2.12.0?

No, I don't mind. I'm happy that more people with a non-Basic setup are
verifying that they work. :)

Of the changes:

> diff --git a/http.c b/http.c
> index f8eb0f23d6c..fb94c444c80 100644
> --- a/http.c
> +++ b/http.c
> @@ -334,7 +334,10 @@ static int http_options(const char *var, const char 
> *value, void *cb)
>   return git_config_string(_agent, var, value);
>  
>   if (!strcmp("http.emptyauth", var)) {
> - curl_empty_auth = git_config_bool(var, value);
> + if (value && !strcmp("auto", value))
> + curl_empty_auth = -1;
> + else
> + curl_empty_auth = git_config_bool(var, value);
>   return 0;
>   }

Obviously good, I should have included this in the original.

> +#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
> + /*
> +  * Our libcurl is too old to do AUTH_ANY in the first place;
> +  * just default to turning the feature off.
> +  */
>  #else
> - /*
> -  * Our libcurl is too old to do AUTH_ANY in the first place;
> -  * just default to turning the feature off.
> -  */

The ifdef reordering here is good.

> + /*
> +  * In the automatic case, kick in the empty-auth
> +  * hack as long as we would potentially try some
> +  * method more exotic than "Basic".
> +  *
> +  * But only do this when this is our second or
> +  * subsequent * request, as by then we know what
> +  * methods are available.
> +  */
> + if (http_auth_methods_restricted)
> + switch (http_auth_methods) {
> + case CURLAUTH_BASIC:
> + case CURLAUTH_DIGEST:
> +#ifdef CURLAUTH_DIGEST_IE
> + case CURLAUTH_DIGEST_IE:
>  #endif
> [...]
> + return 0;
> + default:
> + return 1;
> + }

This is an improvement over my basic-only, but I think you actually want
to bitmask here. A server which advertises only BASIC|DIGEST should not
do empty-auth, but wouldn't match your switch statement.

Patch below.

> Now, how to get this into upstream Git, too? Jeff, do you want to submit a
> v2? In that case, would you please consider the fixup! I mentioned above?
> Otherwise I'd be happy to take it from here.

I don't mind doing a v2. I'm unsure of whether we want to default to
"auto" or not upstream. It seems from your releases that you think it is
safe enough to do in Windows. And I guess nobody outside of that is
really doing NTLM. So it's OK, I guess?

 I don't have enough information to make an intelligent opinion,
so I'm happy to defer.

I'll send my v2 in a minute. Here's the interdiff/fixup if you need to
apply it separately:

diff --git a/http.c b/http.c
index 523c43cf9..dd637d031 100644
--- a/http.c
+++ b/http.c
@@ -126,6 +126,13 @@ static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
 static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+   CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+   | CURLAUTH_DIGEST_IE
+#endif
+   | CURLAUTH_DIGEST;
 #endif
 
 static struct curl_slist *pragma_header;
@@ -400,23 +407,15 @@ static int curl_empty_auth_enabled(void)
/*
 * In the automatic case, kick in the empty-auth
 * hack as long as we would potentially try some
-* method more exotic than "Basic".
+* method more exotic than "Basic" or "Digest".
 *
 * But only do this when this is our second or
 * subsequent * request, as by then we know what
 * methods are available.
 */
-   if (http_auth_methods_restricted)
-   switch (http_auth_methods) {
-   case CURLAUTH_BASIC:
-   case CURLAUTH_DIGEST:
-#ifdef CURLAUTH_DIGEST_IE
-   case CURLAUTH_DIGEST_IE:
-#endif
-   return 0;
-   default:
-   return 1;
-   }
+   if (http_auth_methods_restricted &&
+   (http_auth_methods & ~empty_auth_useless))
+   return 1;
 #endif
return 0;
 }

[PATCH 1/2] commit: be more precise when searching for headers

[René Scharfe]

Search for a space character only within the current line in
read_commit_extra_header_lines() instead of searching in the whole
buffer (and possibly beyond, if it's not NUL-terminated) and then
discarding any results after the end of the current line.

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

diff --git a/commit.c b/commit.c
index 2cf85158b4..173c6d3818 100644
--- a/commit.c
+++ b/commit.c
@@ -1354,8 +1354,8 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(
strbuf_reset();
it = NULL;
 
-   eof = strchr(line, ' ');
-   if (next <= eof)
+   eof = memchr(line, ' ', next - line);
+   if (!eof)
eof = next;
 
if (standard_header_field(line, eof - line) ||
-- 
2.12.0

[PATCH 2/2] commit: don't check for space twice when looking for header

[René Scharfe]

Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them.  We already
check in the caller if that space is present.  Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.

Signed-off-by: Rene Scharfe 
---
 commit.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/commit.c b/commit.c
index 173c6d3818..fab8269731 100644
--- a/commit.c
+++ b/commit.c
@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct 
commit *commit, void *data)
 
 static inline int standard_header_field(const char *field, size_t len)
 {
-   return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-   (len == 6 && !memcmp(field, "parent ", 7)) ||
-   (len == 6 && !memcmp(field, "author ", 7)) ||
-   (len == 9 && !memcmp(field, "committer ", 10)) ||
-   (len == 8 && !memcmp(field, "encoding ", 9)));
+   return ((len == 4 && !memcmp(field, "tree", 4)) ||
+   (len == 6 && !memcmp(field, "parent", 6)) ||
+   (len == 6 && !memcmp(field, "author", 6)) ||
+   (len == 9 && !memcmp(field, "committer", 9)) ||
+   (len == 8 && !memcmp(field, "encoding", 8)));
 }
 
 static int excluded_header_field(const char *field, size_t len, const char 
**exclude)
@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, 
size_t len, const char **exc
 
while (*exclude) {
size_t xlen = strlen(*exclude);
-   if (len == xlen &&
-   !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
+   if (len == xlen && !memcmp(field, *exclude, xlen))
return 1;
exclude++;
}
@@ -1357,9 +1356,8 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(
eof = memchr(line, ' ', next - line);
if (!eof)
eof = next;
-
-   if (standard_header_field(line, eof - line) ||
-   excluded_header_field(line, eof - line, exclude))
+   else if (standard_header_field(line, eof - line) ||
+excluded_header_field(line, eof - line, exclude))
continue;
 
it = xcalloc(1, sizeof(*it));
-- 
2.12.0

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

[Jeff King]

On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:

> Both standard_header_field() and excluded_header_field() check if
> there's a space after the buffer that's handed to them.  We already
> check in the caller if that space is present.  Don't bother calling
> the functions if it's missing, as they are guaranteed to return 0 in
> that case, and remove the now redundant checks from them.

Makes sense, and I couldn't spot any errors in your logic or in the
code.

>  static inline int standard_header_field(const char *field, size_t len)
>  {
> - return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> - (len == 6 && !memcmp(field, "parent ", 7)) ||
> - (len == 6 && !memcmp(field, "author ", 7)) ||
> - (len == 9 && !memcmp(field, "committer ", 10)) ||
> - (len == 8 && !memcmp(field, "encoding ", 9)));
> + return ((len == 4 && !memcmp(field, "tree", 4)) ||
> + (len == 6 && !memcmp(field, "parent", 6)) ||
> + (len == 6 && !memcmp(field, "author", 6)) ||
> + (len == 9 && !memcmp(field, "committer", 9)) ||
> + (len == 8 && !memcmp(field, "encoding", 8)));

Unrelated, but this could probably be spelled with a macro and strlen()
to avoid the magic numbers. It would probably be measurably slower for a
compiler which doesn't pre-compute strlen() on a string literal, though.

-Peff

[PATCH v7 6/6] stash: allow pathspecs in the no verb form

[Thomas Gummerer]

Now that stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well.  Always use -- to
disambiguate pathspecs from other non-option arguments.

Also make git stash -p an alias for git stash push -p.  This allows
users to use git stash -p .

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt | 11 +++
 git-stash.sh|  3 +++
 t/t3903-stash.sh| 15 +++
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 4d8d30f179..70191d06b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -54,10 +54,13 @@ push [-p|--patch] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
The  part is optional and gives
-   the description along with the stashed state.  For quickly making
-   a snapshot, you can omit _both_ "save" and , but giving
-   only  does not trigger this action to prevent a misspelled
-   subcommand from making an unwanted stash.
+   the description along with the stashed state.
++
+For quickly making a snapshot, you can omit "push".  In this mode,
+non-option arguments are not allowed to prevent a misspelled
+subcommand from making an unwanted stash.  The two exceptions to this
+are `stash -p` which acts as alias for `stash push -p` and pathspecs,
+which are allowed after a double hyphen `--` for disambiguation.
 +
 When pathspec is given to 'git stash push', the new stash records the
 modified states only for the files that match the pathspec.  The index
diff --git a/git-stash.sh b/git-stash.sh
index 2d7b30ec5e..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -655,12 +655,15 @@ apply_to_branch () {
}
 }
 
+test "$1" = "-p" && set "push" "$@"
+
 PARSE_CACHE='--not-parsed'
 # The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
case "$opt" in
+   --) break ;;
-*) ;;
*) seen_non_option=t; break ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2f5888df0d..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -876,4 +876,19 @@ test_expect_success 'untracked files are left in place 
when -u is not given' '
test_path_is_file untracked
 '
 
+test_expect_success 'stash without verb with pathspec' '
+   >"foo bar" &&
+   >foo &&
+   >bar &&
+   git add foo* &&
+   git stash -- "foo b*" &&
+   test_path_is_missing "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar &&
+   git stash pop &&
+   test_path_is_file "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar
+'
+
 test_done
-- 
2.11.0.301.g275aeb250c.dirty

[PATCH v7 0/6] stash: support pathspec argument

[Thomas Gummerer]

Thanks Junio for more comments on the last round, and Peff for reading
through it as well.

Changes since v6:

- If no --include-untracked option is given to git stash push, and a
  pathspec is not in the repository, error out instead of ignoring
  it.  This brings it in line with things like checkout, and also
  allows us to simplify the code internally.
- Simplify the code for rolling back the changes from the working
  tree.  This is enabled by the changes to the pathspec handling.
- There's no more "xargs -0", so there should be no more portability
  concerns.
- Adjust the tests and improve some of the titles a bit

Interdiff:
diff --git a/git-stash.sh b/git-stash.sh
index 18aba1346f..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -278,12 +278,15 @@ push_stash () {
die "$(gettext "Can't use --patch and --include-untracked or 
--all at the same time")"
fi
 
+   test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null 
|| exit 1
+
git update-index -q --refresh
if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
fi
+
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
 
@@ -296,23 +299,9 @@ push_stash () {
then
if test $# != 0
then
-   saved_untracked=
-   if test -n "$(git ls-files --others -- "$@")"
-   then
-   saved_untracked=$(
-   git ls-files -z --others -- "$@" |
-   xargs -0 git stash create -u all --)
-   fi
-   git ls-files -z -- "$@" | xargs -0 git reset 
${GIT_QUIET:+-q} --
-   git ls-files -z --modified -- "$@" | xargs -0 git 
checkout ${GIT_QUIET:+-q} HEAD --
-   if test -n "$(git ls-files -z --others -- "$@")"
-   then
-   git ls-files -z --others -- "$@" | xargs -0 git 
clean --force -d ${GIT_QUIET:+-q} --
-   fi
-   if test -n "$saved_untracked"
-   then
-   git stash pop -q $saved_untracked
-   fi
+   git reset ${GIT_QUIET:+-q} -- "$@"
+   git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z 
--modified "$@")
+   git clean --force ${GIT_QUIET:+-q} -d -- "$@"
else
git reset --hard ${GIT_QUIET:+-q}
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index c0ae41e724..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -800,7 +800,7 @@ test_expect_success 'create with multiple arguments for the 
message' '
test_cmp expect actual
 '
 
-test_expect_success 'stash --  stashes and restores the file' '
+test_expect_success 'stash --  stashes and restores the file' '
>foo &&
>bar &&
git add foo bar &&
@@ -812,7 +812,7 @@ test_expect_success 'stash --  stashes and 
restores the file' '
test_path_is_file bar
 '
 
-test_expect_success 'stash with multiple filename arguments' '
+test_expect_success 'stash with multiple pathspec arguments' '
>foo &&
>bar &&
>extra &&
@@ -842,25 +842,29 @@ test_expect_success 'stash with file including $IFS 
character' '
test_path_is_file bar
 '
 
-test_expect_success 'stash push -p with pathspec shows no changes only onece' '
-   >file &&
-   git add file &&
-   git stash push -p not-file >actual &&
+test_expect_success 'stash push -p with pathspec shows no changes only once' '
+   >foo &&
+   git add foo &&
+   git commit -m "tmp" &&
+   git stash push -p foo >actual &&
echo "No local changes to save" >expect &&
+   git reset --hard HEAD~ &&
test_cmp expect actual
 '
 
 test_expect_success 'stash push with pathspec shows no changes when there are 
none' '
-   >file &&
-   git add file &&
-   git stash push not-file >actual &&
+   >foo &&
+   git add foo &&
+   git commit -m "tmp" &&
+   git stash push foo >actual &&
echo "No local changes to save" >expect &&
+   git reset --hard HEAD~ &&
test_cmp expect actual
 '
 
-test_expect_success 'untracked file is not removed when using pathspecs' '
+test_expect_success 'stash push with pathspec not in the repository errors 
out' '
>untracked &&
-   git stash push untracked &&
+   test_must_fail git stash push untracked &&
test_path_is_file untracked
 '
 

Thomas Gummerer (6):
  stash: introduce push verb
  stash: add test for the create command line arguments
  stash: refactor stash_create
  stash: teach 'push' (and 'create_stash') to honor pathspec
  stash: use stash_push 

[PATCH v7 3/6] stash: refactor stash_create

[Thomas Gummerer]

Refactor the internal stash_create function to use a -m flag for
specifying the message and -u flag to indicate whether untracked files
should be added to the stash.

This makes it easier to pass a pathspec argument to stash_create in the
next patch.

The user interface for git stash create stays the same.

Signed-off-by: Thomas Gummerer 
---
 git-stash.sh | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..ef5d1b45be 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,22 @@ clear_stash () {
 }
 
 create_stash () {
-   stash_msg="$1"
-   untracked="$2"
+   stash_msg=
+   untracked=
+   while test $# != 0
+   do
+   case "$1" in
+   -m|--message)
+   shift
+   stash_msg=${1?"BUG: create_stash () -m requires an 
argument"}
+   ;;
+   -u|--include-untracked)
+   shift
+   untracked=${1?"BUG: create_stash () -u requires an 
argument"}
+   ;;
+   esac
+   shift
+   done
 
git update-index -q --refresh
if no_changes
@@ -268,7 +282,7 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
 
-   create_stash "$stash_msg" $untracked
+   create_stash -m "$stash_msg" -u "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state 
\$stash_msg")"
@@ -667,7 +681,7 @@ clear)
;;
 create)
shift
-   create_stash "$*" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift
-- 
2.11.0.301.g275aeb250c.dirty

[PATCH v7 2/6] stash: add test for the create command line arguments

[Thomas Gummerer]

Currently there is no test showing the expected behaviour of git stash
create's command line arguments.  Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.

Signed-off-by: Thomas Gummerer 
---
 t/t3903-stash.sh | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3577115807..ffe3549ea5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
 '
 
+test_expect_success 'create stores correct message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create "create test message") &&
+   echo "On master: create test message" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create test untracked) &&
+   echo "On master: test untracked" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g275aeb250c.dirty

Re: git-clone --config order & fetching extra refs during initial clone

[Jeff King]

[Re-sending, as I used an old address for Gábor on the original]

On Sat, Feb 25, 2017 at 07:12:38PM +, Robin H. Johnson wrote:

> TL;DR: git-clone ignores any fetch specs passed via --config.

I agree that this is a bug. There's some previous discussion and an RFC
patch from lat March (author cc'd):

  
http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/

That discussion veered off into alternatives, but I think the v2 posted
in that thread is taking a sane approach.

-Peff

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:

If we have a patch like the one in the new test-case, then we will
try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).

The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.

Found using AFL.

Signed-off-by: Vegard Nossum 
---
 apply.c |  3 ++-
 t/t4154-apply-git-header.sh | 15 +++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100755 t/t4154-apply-git-header.sh

diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
-   if (!patch->is_delete && !patch->new_name) {
+   if ((!patch->is_delete && !patch->new_name) ||
+   (patch->is_rename && !patch->old_name)) {


Would it make sense to mirror the previously existing condition and 
check for is_new instead?  I.e.:


if ((!patch->is_delete && !patch->new_name) ||
(!patch->is_new&& !patch->old_name)) {

or

if (!(patch->is_delete || patch->new_name) ||
!(patch->is_new|| patch->old_name)) {

René

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:

For the patches in the added testcases, we were crashing with:

git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' 
failed.

As it turns out, check_preimage() is prepared to handle these conditions,
so we can remove the assertion.

Found using AFL.

Signed-off-by: Vegard Nossum 

---

(I'm fully aware of how it looks to just delete an assertion to "fix" a
bug without any other changes to accomodate the condition that was
being tested for. I am definitely not an expert on this code, but as far
as I can tell -- both by reviewing and testing the code -- the function
really is prepared to handle the case where patch->is_new == 1, as it
will always hit another error condition if that is true. I've tried to
add more test cases to show what errors you can expect to see instead of
the assertion failure when trying to apply these nonsensical patches. If
you don't want to remove the assertion for whatever reason, please feel
free to take the testcases and add "# TODO: known breakage" or whatever.)
---
 apply.c |  1 -
 t/t4154-apply-git-header.sh | 36 
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
if (!old_name)
return 0;

-   assert(patch->is_new <= 0);


5c47f4c6 (builtin-apply: accept patch to an empty file) added that line. 
 Its intent was to handle diffs that contain an old name even for a 
file that's created.  Citing from its commit message: "When we cannot be 
sure by parsing the patch that it is not a creation patch, we shouldn't 
complain when if there is no such a file."  Why not stop complaining 
also in case we happen to know for sure that it's a creation patch? 
I.e., why not replace the assert() with:


if (patch->is_new == 1)
goto is_new;


previous = previous_patch(state, patch, );

if (status)
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
index d651af4a2..c440c48ad 100755
--- a/t/t4154-apply-git-header.sh
+++ b/t/t4154-apply-git-header.sh
@@ -12,4 +12,40 @@ rename new 0
 EOF
 '

+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+deleted file mode
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / wrong type' '
+   mkdir x &&
+   chmod 755 x &&
+   test_must_fail git apply << EOF
+diff --git a/x b/x
+deleted file mode 160755
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / already exists' 
'
+   touch 1 &&
+   chmod 644 1 &&
+   test_must_fail git apply << EOF
+diff --git a/1 b/1
+deleted file mode 100644
+new file mode
+EOF
+'
+
+test_expect_success 'apply new file mode / copy from / nonexistant file' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+new file mode
+copy from
+EOF
+'
+
 test_done

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

[René Scharfe]

Am 25.02.2017 um 21:15 schrieb Jeff King:

On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:


Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them.  We already
check in the caller if that space is present.  Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.


Makes sense, and I couldn't spot any errors in your logic or in the
code.


Thanks for checking!


 static inline int standard_header_field(const char *field, size_t len)
 {
-   return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-   (len == 6 && !memcmp(field, "parent ", 7)) ||
-   (len == 6 && !memcmp(field, "author ", 7)) ||
-   (len == 9 && !memcmp(field, "committer ", 10)) ||
-   (len == 8 && !memcmp(field, "encoding ", 9)));
+   return ((len == 4 && !memcmp(field, "tree", 4)) ||
+   (len == 6 && !memcmp(field, "parent", 6)) ||
+   (len == 6 && !memcmp(field, "author", 6)) ||
+   (len == 9 && !memcmp(field, "committer", 9)) ||
+   (len == 8 && !memcmp(field, "encoding", 8)));


Unrelated, but this could probably be spelled with a macro and strlen()
to avoid the magic numbers. It would probably be measurably slower for a
compiler which doesn't pre-compute strlen() on a string literal, though.


sizeof(string_constant) - 1 might be a better choice here than strlen().

René

Re: [PATCH] travis-ci: run scan-build every time

[Lars Schneider]

> On 24 Feb 2017, at 18:29, Samuel Lijin  wrote:
> 
> Introduces the scan-build static code analysis tool from the Clang
> project to all Travis CI builds. Installs clang (since scan-build
> needs clang as a dependency) to make this possible (on macOS, also
> updates PATH to allow scan-build to be invoked without referencing the
> full path).

This is a pretty neat idea. However, I think this should become a
dedicated job in a TravisCI build (similar to the Documentation job [1])
because:
 a) We don't want to build and test a scan-build version of Git (AFAIK
scan-build kind of proxies the compiler to do its job - I don't if
this has any side effects)
 b) We don't want to slow down the other builds
 c) It should be enough to run scan-build once on Linux per build

I ran scan-build on the current master and it detected 72 potential bugs [2]. 
I looked through a few of them and they seem to be legitimate. If the list 
agrees
that running scan-build is a useful thing and that these problems should be 
fixed
then we could:

(1) Add scan-build check to Travis CI but only print errors as warning
(2) Fix the 72 existing bugs over time
(3) Turn scan-build warnings into errors


[1] 
https://github.com/git/git/blob/e7e07d5a4fcc2a203d9873968ad3e6bd4d7419d7/.travis.yml#L42-L53
[2] https://larsxschneider.github.io/git-scan/


> ---
> 
> A build with this patch can be found at [1]. Note that if reports *are*
> generated, this doesn't allow us to access them, and if we dumped
> the reports as build artifacts, I'm not sure where we would want to
> dump them to.

We could upload the results to a Git repo and then use GitHub pages to serve
it. I did that with my run here: https://larsxschneider.github.io/git-scan/


> It's worth noting that there seems to be a weird issue with scan-build
> where it *will* generate a report for something locally, but won't do it
> on Travis. See [2] for an example where I have a C program with a
> very obvious memory leak but scan-build on Travis doesn't generate
> a report (despite complaining about it in stdout), even though it does
> on my local machine.
> 
> [1] https://travis-ci.org/sxlijin/git/builds/204853233
> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342

Scan-build stores the report in some temp folder. I assume you can't access
this folder on TravisCI. Try the scan-build option "-o scan-build-results"
to store the report in the local directory. 


> 
> .travis.yml | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 9c63c8c3f..1038b1b3d 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -20,6 +20,7 @@ addons:
> - language-pack-is
> - git-svn
> - apache2
> +- clang
> 
> env:
>   global:
> @@ -78,9 +79,8 @@ before_install:
>   brew update --quiet
>   # Uncomment this if you want to run perf tests:
>   # brew install gnu-time
> -  brew install git-lfs gettext
> -  brew link --force gettext
> -  brew install caskroom/cask/perforce
> +  brew install git-lfs gettext caskroom/cask/perforce llvm
> +  brew link --force gettext llvm

This wouldn't be necessary if we only scan on Linux.


>   ;;
> esac;
> echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)";
> @@ -92,9 +92,9 @@ before_install:
> mkdir -p $HOME/travis-cache;
> ln -s $HOME/travis-cache/.prove t/.prove;
> 
> -before_script: make --jobs=2
> +before_script: scan-build make --jobs=2

I think we should run it like this:

scan-build -analyze-headers --status-bugs --keep-going 
--force-analyze-debug-code make --jobs=2

This way TravisCI would be notified via the return code if scan-build detected
errors I think.


> -script: make --quiet test
> +script: scan-build make --quiet test

Why do you want to scan the tests?


Cheers,
Lars

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

[Jeff King]

On Sat, Feb 25, 2017 at 10:39:29PM +0100, René Scharfe wrote:

> > > + (len == 8 && !memcmp(field, "encoding", 8)));
> > 
> > Unrelated, but this could probably be spelled with a macro and strlen()
> > to avoid the magic numbers. It would probably be measurably slower for a
> > compiler which doesn't pre-compute strlen() on a string literal, though.
> 
> sizeof(string_constant) - 1 might be a better choice here than strlen().

Yeah. If you use a macro, that works. If it's an inline function you'd
need strlen(). That's a tradeoff we've already made in skip_prefix_mem()
and strip_suffix(), but it's not like we expect this list to grow much,
so it may not be worth fussing with.

-Peff

Re: SHA1 collisions found

[Mike Hommey]

On Sat, Feb 25, 2017 at 02:26:56PM -0500, Jeff King wrote:
> On Sat, Feb 25, 2017 at 06:50:50PM +, brian m. carlson wrote:
> 
> > > As long as the reader can tell from the format of object names
> > > stored in the "new object format" object from what era is being
> > > referred to in some way [*1*], we can name new objects with only new
> > > hash, I would think.  "new refers only to new" that stratifies
> > > objects into older and newer may make things simpler, but I am not
> > > convinced yet that it would give our users a smooth enough
> > > transition path (but I am open to be educated and pursuaded the
> > > other way).
> > 
> > I would simply use multihash[0] for this purpose.  New-style objects
> > serialize data in multihash format, so it's immediately obvious what
> > hash we're referring to.  That makes future transitions less
> > problematic.
> > 
> > [0] https://github.com/multiformats/multihash
> 
> I looked at that earlier, because I think it's a reasonable idea for
> future-proofing. The first byte is a "varint", but I couldn't find where
> they defined that format.
> 
> The closest I could find is:
> 
>   https://github.com/multiformats/unsigned-varint
> 
> whose README says:
> 
>   This unsigned varint (VARiable INTeger) format is for the use in all
>   the multiformats.
> 
> - We have not yet decided on a format yet. When we do, this readme
>   will be updated.
> 
> - We have time. All multiformats are far from requiring this varint.
> 
> which is not exactly confidence inspiring. They also put the length at
> the front of the hash. That's probably convenient if you're parsing an
> unknown set of hashes, but I'm not sure it's helpful inside Git objects.
> And there's an incentive to minimize header data at the front of a hash,
> because every byte is one more byte that every single hash will collide
> over, and people will have to type when passing hashes to "git show",
> etc.
> 
> I'd almost rather use something _really_ verbose like
> 
>   sha256:1234abcd...
> 
> in all of the objects. And then when we get an unadorned hash from the
> user, we guess it's sha256 (or whatever), and fallback to treating it as
> a sha1.
> 
> Using a syntactically-obvious name like that also solves one other
> problem: there are sha1 hashes whose first bytes will encode as a "this
> is sha256" multihash, creating some ambiguity.

Indeed, multihash only really is interesting when *all* hashes use it.
And obviously, git can't change the existing sha1s.

Mike

Re: [PATCH] travis-ci: run scan-build every time

[Jeff King]

On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:

> 
> > On 24 Feb 2017, at 18:29, Samuel Lijin  wrote:
> > 
> > Introduces the scan-build static code analysis tool from the Clang
> > project to all Travis CI builds. Installs clang (since scan-build
> > needs clang as a dependency) to make this possible (on macOS, also
> > updates PATH to allow scan-build to be invoked without referencing the
> > full path).
> 
> This is a pretty neat idea. However, I think this should become a
> dedicated job in a TravisCI build (similar to the Documentation job [1])
> because:
>  a) We don't want to build and test a scan-build version of Git (AFAIK
> scan-build kind of proxies the compiler to do its job - I don't if
> this has any side effects)
>  b) We don't want to slow down the other builds
>  c) It should be enough to run scan-build once on Linux per build

Yeah. I am all for static analysis, but I agree it should be its own
job. Especially as it can be quite noisy with false positives (and I
really think before any static analysis is useful we need to figure out
a way to suppress the false positives, so that we can see the signal in
the noise).

Fully a third of the problem cases found are dead assignments or
increments. I looked at a few, and I think the right strategy is to tell
the tool "no really, our code is fine". For instance, it complains
about:

  argc = parse_options(argc, argv, ...);

when argc is not used again later. Sure, that assignment is doing
nothing. But from a maintainability perspective, I'd much rather have a
dead assignment (that the compiler is free to remove) then for somebody
to later add a loop like:

  for (i = 0; i < argc; i++)
  something(argv[i]);

which will read past the end of the rearranged argv (and probably
_wouldn't_ be caught by static analysis, because the hidden dependency
between argc and argv is buried inside the parse_options() call).

So there is definitely some bug-fixing to be done, but I think there is
also some work in figuring out how to suppress these useless reports.
Turning off the dead-assignment checker is one option, but I actually
think it _could_ produce useful results. It just isn't in these cases.
So I'd much rather if we can somehow suppress the specific callsites.

> I ran scan-build on the current master and it detected 72 potential bugs [2]. 
> I looked through a few of them and they seem to be legitimate. If the list 
> agrees
> that running scan-build is a useful thing and that these problems should be 
> fixed
> then we could:
> 
> (1) Add scan-build check to Travis CI but only print errors as warning
> (2) Fix the 72 existing bugs over time
> (3) Turn scan-build warnings into errors

If they are warnings socked away in a Travis CI job that nobody looks
out, then I doubt anybody is going to bother fixing them.

Not that step (1) hurts necessarily, but I don't think it's really doing
anything until step (2) is finished.

I took a look at a few of the non-dead-assignment ones and some of them
are obviously false positives. E.g., in check_pbase_path(), it claims
that done_pbase_paths might be NULL. But that value just went through
ALLOC_GROW() with a non-zero value, which would either have allocated or
died.

There are other cases where it complains that a strbuf's "buf" parameter
might be NULL. That _shouldn't_ be the case, as it is an invariant of
strbuf. It might be a bug, but it is certainly not a bug where the
analyzer is pointing.

I won't be surprised at all if there are a bunch of real bugs in that
list. But I think the interesting work at this point is not a CI build,
but somebody locally slogging through scan-build and categorizing each
one.

-Peff

Re: SHA1 collisions found

[Lars Schneider]

> On 25 Feb 2017, at 00:06, Jeff King  wrote:
> 
> On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
> 
>> I have just read on ArsTechnica[1] that while Git repository could be
>> corrupted (though this would require attackers to spend great amount
>> of resources creating their own collision, while as said elsewhere
>> in this thread allegedly easy to detect), putting two proof-of-concept
>> different PDFs with same size and SHA-1 actually *breaks* Subversion.
>> Repository can become corrupt, and stop accepting new commits.  
>> 
>> From what I understand people tried this, and Git doesn't exhibit
>> such problem.  I wonder what assumptions SVN made that were broken...
> 
> To be clear, nobody has generated a sha1 collision in Git yet, and you
> cannot blindly use the shattered PDFs to do so. Git's notion of the
> SHA-1 of an object include the header, so somebody would have to do a
> shattered-level collision search for something that starts with the
> correct "blob 1234\0" header.
> 
> So we don't actually know how Git would behave in the face of a SHA-1
> collision. It would be pretty easy to simulate it with something like:
> 
> ---
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index 22b125cf8..1be5b5ba3 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -231,6 +231,16 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, 
> unsigned long len)
>   memcpy(ctx->W, data, len);
> }
> 
> +/* sha1 of blobs containing "foo\n" and "bar\n" */
> +static const unsigned char foo_sha1[] = {
> + 0x25, 0x7c, 0xc5, 0x64, 0x2c, 0xb1, 0xa0, 0x54, 0xf0, 0x8c,
> + 0xc8, 0x3f, 0x2d, 0x94, 0x3e, 0x56, 0xfd, 0x3e, 0xbe, 0x99
> +};
> +static const unsigned char bar_sha1[] = {
> + 0x57, 0x16, 0xca, 0x59, 0x87, 0xcb, 0xf9, 0x7d, 0x6b, 0xb5,
> + 0x49, 0x20, 0xbe, 0xa6, 0xad, 0xde, 0x24, 0x2d, 0x87, 0xe6
> +};
> +
> void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
> {
>   static const unsigned char pad[64] = { 0x80 };
> @@ -248,4 +258,8 @@ void blk_SHA1_Final(unsigned char hashout[20], 
> blk_SHA_CTX *ctx)
>   /* Output hash */
>   for (i = 0; i < 5; i++)
>   put_be32(hashout + i * 4, ctx->H[i]);
> +
> + /* pretend "foo" and "bar" collide */
> + if (!memcmp(hashout, bar_sha1, 20))
> + memcpy(hashout, foo_sha1, 20);
> }

That's a good idea! I wonder if it would make sense to setup an 
additional job in TravisCI that patches every Git version with some hash 
collisions and then runs special tests. This way we could ensure Git 
behaves reasonable in case of a collision. E.g. by printing errors and 
not crashing or corrupting the repo. Do you think that would be worth 
the effort?

- Lars

Re: [PATCH] travis-ci: run scan-build every time

[Lars Schneider]

> On 25 Feb 2017, at 23:31, Jeff King  wrote:
> 
> On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:
> 
>> 
>>> On 24 Feb 2017, at 18:29, Samuel Lijin  wrote:
>>> 
>>> Introduces the scan-build static code analysis tool from the Clang
>>> project to all Travis CI builds. Installs clang (since scan-build
>>> needs clang as a dependency) to make this possible (on macOS, also
>>> updates PATH to allow scan-build to be invoked without referencing the
>>> full path).
>> 
>> This is a pretty neat idea. However, I think this should become a
>> dedicated job in a TravisCI build (similar to the Documentation job [1])
>> because:
>> a) We don't want to build and test a scan-build version of Git (AFAIK
>>scan-build kind of proxies the compiler to do its job - I don't if
>>this has any side effects)
>> b) We don't want to slow down the other builds
>> c) It should be enough to run scan-build once on Linux per build
> 
> Yeah. I am all for static analysis, but I agree it should be its own
> job. Especially as it can be quite noisy with false positives (and I
> really think before any static analysis is useful we need to figure out
> a way to suppress the false positives, so that we can see the signal in
> the noise).
> 
> Fully a third of the problem cases found are dead assignments or
> increments. I looked at a few, and I think the right strategy is to tell
> the tool "no really, our code is fine". For instance, it complains
> about:
> 
>  argc = parse_options(argc, argv, ...);
> 
> when argc is not used again later. Sure, that assignment is doing
> nothing. But from a maintainability perspective, I'd much rather have a
> dead assignment (that the compiler is free to remove) then for somebody
> to later add a loop like:
> 
>  for (i = 0; i < argc; i++)
>  something(argv[i]);
> 
> which will read past the end of the rearranged argv (and probably
> _wouldn't_ be caught by static analysis, because the hidden dependency
> between argc and argv is buried inside the parse_options() call).
> 
> So there is definitely some bug-fixing to be done, but I think there is
> also some work in figuring out how to suppress these useless reports.

That makes sense. I suspected that this assignment was intentional
but I wasn't sure why. I didn't know about the rearrangement of argv.

Apparently an "(void)argc;" silences this warning. Would that be too
ugly to bear? :-)


> Turning off the dead-assignment checker is one option, but I actually
> think it _could_ produce useful results. It just isn't in these cases.
> So I'd much rather if we can somehow suppress the specific callsites.
> 
>> I ran scan-build on the current master and it detected 72 potential bugs 
>> [2]. 
>> I looked through a few of them and they seem to be legitimate. If the list 
>> agrees
>> that running scan-build is a useful thing and that these problems should be 
>> fixed
>> then we could:
>> 
>> (1) Add scan-build check to Travis CI but only print errors as warning
>> (2) Fix the 72 existing bugs over time
>> (3) Turn scan-build warnings into errors
> 
> If they are warnings socked away in a Travis CI job that nobody looks
> out, then I doubt anybody is going to bother fixing them.
> 
> Not that step (1) hurts necessarily, but I don't think it's really doing
> anything until step (2) is finished.

Agreed.


- Lars

Re: [PATCH v6 1/1] config: add conditional include

[Jeff King]

On Fri, Feb 24, 2017 at 08:14:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> + /* no condition (i.e., "include.path") is always true */
> + if (!cond)
> + return 1;
> +
> + if (skip_prefix_mem(cond, cond_len, "gitdir:", , _len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", , _len))
> + return include_by_gitdir(cond, cond_len, 1);
> +
> + error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> + /* unknown conditionals are always false */
> + return 0;
> +}

This "error()" is a nice warning to people who have misspelled the
condition (or are surprised that an old version does not respect their
condition). But it seems out of place with the rest of the compatibility
strategy, which is to quietly ignore include types we don't understand.

For example, if your patch ships in v2.13 and we add the "foo:"
condition in v2.14, then with config like:

  [includeif "foo:bar"]
  path = whatever

you get:

  v2.12: silently ignore
  v2.13: loudly ignore
  v2.14: works

which seems odd.

If we do keep it, it should probably be warning(). I would expect
"error:" to indicate that some requested operation could not be
completed, but this is just "by the way, I ignored this garbage".

-Peff

PS I know we try to avoid dashes in the section names, but "includeIf"
   and "includeif" just look really ugly to me. Maybe it is just me,
   though.

Re: SHA1 collisions found

[Jason Cooper]

Hi Junio,

On Fri, Feb 24, 2017 at 10:10:01PM -0800, Junio C Hamano wrote:
> I was thinking we would need mixed mode support for smoother
> transition, but it now seems to me that the approach to stratify the
> history into old and new is workable.

As someone looking to deploy (and having previously deployed) git in
unconventional roles, I'd like to add one caveat.  The flag day in the
history is great, but I'd like to be able to confirm the integrity of
the old history.

"Counter-hashing" the blobs is easy enough, but the trees, commits and
tags would need to have, iiuc, some sort of cross-reference.  As in my
previous example, "git tag -v v3.16" also checks the counter hash to
further verify the integrity of the history (yes, it *really* needs to
check all of the old hashes, but I'd like to make sure I can do step one
first).

Would there be opposition to counter-hashing the old commits at the flag
day?

thx,

Jason.

Re: [PATCH] travis-ci: run scan-build every time

[Samuel Lijin]

On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider
 wrote:
>
>> On 24 Feb 2017, at 18:29, Samuel Lijin  wrote:
>>
>> It's worth noting that there seems to be a weird issue with scan-build
>> where it *will* generate a report for something locally, but won't do it
>> on Travis. See [2] for an example where I have a C program with a
>> very obvious memory leak but scan-build on Travis doesn't generate
>> a report (despite complaining about it in stdout), even though it does
>> on my local machine.
>>
>> [1] https://travis-ci.org/sxlijin/git/builds/204853233
>> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
>
> Scan-build stores the report in some temp folder. I assume you can't access
> this folder on TravisCI. Try the scan-build option "-o scan-build-results"
> to store the report in the local directory.

That occurred to me, but I don't quite think that's the issue. I just
noticed that on the repo I use to test build matrices, jobs 1-8 don't
generate a report, but 9-14 and 19-20 do [1]. I don't think it's an
issue with write permissions (scan-build complains much more vocally
if that happens), but it doesn't seem to matter if the output dir is
in the tmpfs [2] or a local directory [3].

[1] https://travis-ci.org/sxlijin/travis-testing/builds/205054253
[2] https://travis-ci.org/sxlijin/git/jobs/205028920#L1000
[2] https://travis-ci.org/sxlijin/git/jobs/205411705#L998

>> @@ -78,9 +79,8 @@ before_install:
>>   brew update --quiet
>>   # Uncomment this if you want to run perf tests:
>>   # brew install gnu-time
>> -  brew install git-lfs gettext
>> -  brew link --force gettext
>> -  brew install caskroom/cask/perforce
>> +  brew install git-lfs gettext caskroom/cask/perforce llvm
>> +  brew link --force gettext llvm
>
> This wouldn't be necessary if we only scan on Linux.

Agreed. I'm not sure if macOS static analysis would bring any specific
benefits; I don't really have much experience with static analysis
tools one way or another, so I'm happy to defer on this decision.


>> -script: make --quiet test
>> +script: scan-build make --quiet test
>
> Why do you want to scan the tests?

Brain fart on my end.

> Cheers,
> Lars

Re: [PATCH v5 1/1] config: add conditional include

[Jeff King]

On Fri, Feb 24, 2017 at 09:46:17AM -0800, Junio C Hamano wrote:

> Duy Nguyen  writes:
> 
> >>> + if (skip_prefix_mem(cond, cond_len, "gitdir:", , _len))
> >>> + return include_by_gitdir(cond, cond_len, 0);
> >>> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", , 
> >>> _len))
> >>> + return include_by_gitdir(cond, cond_len, 1);
> >>
> >> This may be OK for now, but it should be trivial to start from a
> >> table with two entries, i.e.
> >>
> >> struct include_cond {
> >> const char *keyword;
> >> int (*fn)(const char *, size_t);
> >> };
> >>
> >> and will show a better way to do things to those who follow your
> >> footsteps.
> >
> > Yeah I don't see a third include coming soon and did not go with that.
> > Let's way for it and refactor then.
> 
> I would have said exactly that in my message if you already had
> include_by_gitdir() and include_by_gitdir_i() as separate functions.
> 
> But I didn't, because the above code gives an excuse to the person
> who adds the third one to be lazy and add another "else if" for
> expediency, because making it table-driven would require an extra
> work to add two wrapper functions that do not have anything to do
> with the third one being added.

I don't think driving that with a two-entry table is the right thing
here. We are as likely to add another "foobar:" entry as we are to add
another modifier "/i" modifier to "gitdir:", and it is unclear whether
that modifier would be mutually exclusive with "/i".

E.g., imagine we add "/re" for a regex, but allow a case-insensitive
regex with an "i", giving something like "gitdir/i/re:".  Now you would
want to drive it by matching "gitdir" first, and then collecting the
"/i/re" independently, to avoid an explosion of matches.

Driving that with a table is much more complex. I'd just as soon keep
things as simple as possible for now and worry about flagging it in
review when something new gets added.

-Peff

Re: SHA1 collisions found

[Jeff King]

On Sat, Feb 25, 2017 at 11:35:27PM +0100, Lars Schneider wrote:

> > So we don't actually know how Git would behave in the face of a SHA-1
> > collision. It would be pretty easy to simulate it with something like:
> [...]
> 
> That's a good idea! I wonder if it would make sense to setup an 
> additional job in TravisCI that patches every Git version with some hash 
> collisions and then runs special tests. This way we could ensure Git 
> behaves reasonable in case of a collision. E.g. by printing errors and 
> not crashing or corrupting the repo. Do you think that would be worth 
> the effort?

I think it would be interesting to see the results under various
scenarios. I don't know that it would be all that interesting from an
ongoing CI perspective. But we wouldn't know until somebody actually
writes the tests and we see what they do.

-Peff

Re: [PATCH v6 1/1] config: add conditional include

[Duy Nguyen]

On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley  wrote:
>> +Conditional includes
>> +
>> +
>> +You can include one config file from another conditionally by setting
>
>
> On first reading I thought this implied you can only have one `includeIf`
> within the config file.
> I think it is meant to mean that each `includeIf`could include one other
> file, and that users can have multiple `includeIf` lines.

Yes. Not sure how to put it better though (I basically copied the
first paragraph from the unconditional include section above, which
shares the same confusion). Perhaps just write "the variable can be
specified multiple times"? Or "multiple variables include multiple
times, the last variable does not override the previous ones"?
-- 
Duy

Re: SHA1 collisions found

[Jason Cooper]

Hi,

On Sat, Feb 25, 2017 at 01:31:32AM +0100, ankostis wrote:
> That is why I believe that some HASH (e.g. SHA-3) must be the blessed one.
> All git >= 3.x.x must support at least this one (for naming and
> cross-referencing between objects).

I would stress caution here.  SHA3 has survived the NIST competition,
but that's about it.  It has *not* received nearly as much scrutiny as
SHA2.

SHA2 is a similar construction to SHA1 (Merkle–Damgård [1]) so it makes
sense to be leery of it, but I would argue it's seasoning merits serious
consideration.

Ideally, bless SHA2-384 (minimum) as the next hash.  Five or so years
down the road, if SHA3 is still in good standing, bless it as the next
hash.


thx,

Jason.

[1]
https://en.wikipedia.org/wiki/Merkle%E2%80%93Damg%C3%A5rd_construction

Re: SHA1 collisions found

[Jeff King]

On Sun, Feb 26, 2017 at 01:13:59AM +, Jason Cooper wrote:

> On Fri, Feb 24, 2017 at 10:10:01PM -0800, Junio C Hamano wrote:
> > I was thinking we would need mixed mode support for smoother
> > transition, but it now seems to me that the approach to stratify the
> > history into old and new is workable.
> 
> As someone looking to deploy (and having previously deployed) git in
> unconventional roles, I'd like to add one caveat.  The flag day in the
> history is great, but I'd like to be able to confirm the integrity of
> the old history.
> 
> "Counter-hashing" the blobs is easy enough, but the trees, commits and
> tags would need to have, iiuc, some sort of cross-reference.  As in my
> previous example, "git tag -v v3.16" also checks the counter hash to
> further verify the integrity of the history (yes, it *really* needs to
> check all of the old hashes, but I'd like to make sure I can do step one
> first).
> 
> Would there be opposition to counter-hashing the old commits at the flag
> day?

I don't think a counter-hash needs to be embedded into the git objects
themselves. If the "modern" repo format stores everything primarily as
sha-256, say, it will probably need to maintain a (local) mapping table
of sha1/sha256 equivalence. That table can be generated at any time from
the object data (though I suspect we'll keep it up to date as objects
enter the repository).

At the flag day[1], you can make a signed tag with the "correct" mapping
in the tag body (so part of the actual GPG signed data, not referenced
by sha1). Then later you can compare that mapping to the object content
in the repo (or to the local copy of the mapping based on that data).

-Peff

[1] You don't even need to wait until the flag day. You can do it now.
This is conceptually similar to the git-evtag tool, though it just
signs the blob contents of the tag's current tree state. Signing the
whole mapping lets you verify the entirety of history, but of course
that mapping is quite big: 20 + 32 bytes per object for
sha1/sha-256, which is ~250MB for the kernel. So you'd probably not
want to do it more than once.

Re: SHA1 collisions found

[ankostis]

On 24 February 2017 at 18:23, Jason Cooper  wrote:
> Hi Ian,
>
> On Fri, Feb 24, 2017 at 03:13:37PM +, Ian Jackson wrote:
>> Joey Hess writes ("SHA1 collisions found"):
>> > https://shattered.io/static/shattered.pdf
>> > https://freedom-to-tinker.com/2017/02/23/rip-sha-1/
>> >
>> > IIRC someone has been working on parameterizing git's SHA1 assumptions
>> > so a repository could eventually use a more secure hash. How far has
>> > that gotten? There are still many "40" constants in git.git HEAD.
>>
>> I have been thinking about how to do a transition from SHA1 to another
>> hash function.
>>
>> I have concluded that:
>>
>>  * We can should avoid expecting everyone to rewrite all their
>>history.
>
> Agreed.
>
>>  * Unfortunately, because the data formats (particularly, the commit
>>header) are not in practice extensible (because of the way existing
>>code parses them), it is not useful to try generate new data (new
>>commits etc.) containing both new hashes and old hashes: old
>>clients will mishandle the new data.
>
> My thought here is:
>
>  a) re-hash blobs with sha256, hardlink to sha1 objects
>  b) create new tree objects which are mirrors of each sha1 tree object,
> but purely sha256
>  c) mirror commits, but they are also purely sha256
>  d) future PGP signed tags would sign both hashes (or include both?)


IMHO that is a great idea that needs more attention.
You get to keep 2 or more hash-functions for extra security in a PQ world.

And to keep sketches for the future so far,
SHA-3 must be always one of the new hashes.
Actually, you can get rid of SHA-1 completely, and land on Linus's
current sketches for the way ahead.

Thanks,
  Kostis
>
> Which would end up something like:
>
>   .git/
> \... #usual files
> \objects
>   \ef
> \3c39f7522dc55a24f64da9febcfac71e984366
> \objects-sha2_256
>   \72
> \604fd2de5f25c89d692b01081af93bcf00d2af34549d8d1bdeb68bc048932
> \info
>   \...
> \info-sha2_256
>   \refs #uses sha256 commit identifiers
>
> Basically, keep the sha256 stuff out of the way for legacy clients, and
> new clients will still be able to use it.
>
> There shouldn't be a need to re-sign old signed tags if the underlying
> objects are counter-hashed.  There might need to be some transition
> info, though.
>
> Say a new client does 'git tag -v tags/v3.16' in the kernel tree.  I would
> expect it to check the sha1 hashes, verify the PGP signed tag, and then
> also check the sha256 counter-hashes of the relevant objects.
>
> thx,
>
> Jason.