* [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Jeff King @ 2016-10-03 20:34 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
When we add a new alternate to the list, we try to normalize
out any redundant "..", etc. However, we do not look at the
return value of normalize_path_copy(), and will happily
continue with a path that could not be normalized. Worse,
the normalizing process is done in-place, so we are left
with whatever half-finished working state the normalizing
function was in.
Fortunately, this cannot cause us to read past the end of
our buffer, as that working state will always leave the
NUL from the original path in place. And we do tend to
notice problems when we check is_directory() on the path.
But you can see the nonsense that we feed to is_directory
with an entry like:
this/../../is/../../way/../../too/../../deep/../../to/../../resolve
in your objects/info/alternates, which yields:
error: object directory
/to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve
does not exist; check .git/objects/info/alternates.
We can easily fix this just by checking the return value.
But that makes it hard to generate a good error message,
since we're normalizing in-place and our input value has
been overwritten by cruft.
Instead, let's provide a strbuf helper that does an in-place
normalize, but restores the original contents on error. This
uses a second buffer under the hood, which is slightly less
efficient, but this is not a performance-critical code path.
The strbuf helper can also properly set the "len" parameter
of the strbuf before returning. Just doing:
normalize_path_copy(buf.buf, buf.buf);
will shorten the string, but leave buf.len at the original
length. That may be confusing to later code which uses the
strbuf.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_file.c | 11 +++++++++--
strbuf.c | 20 ++++++++++++++++++++
strbuf.h | 8 ++++++++
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..68571bd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
}
strbuf_addstr(&pathbuf, entry);
- normalize_path_copy(pathbuf.buf, pathbuf.buf);
+ if (strbuf_normalize_path(&pathbuf) < 0) {
+ error("unable to normalize alternate object path: %s",
+ pathbuf.buf);
+ strbuf_release(&pathbuf);
+ return -1;
+ }
pfxlen = strlen(pathbuf.buf);
@@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
}
strbuf_add_absolute_path(&objdirbuf, get_object_directory());
- normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
+ if (strbuf_normalize_path(&objdirbuf) < 0)
+ die("unable to normalize object directory: %s",
+ objdirbuf.buf);
alt_copy = xmemdupz(alt, len);
string_list_split_in_place(&entries, alt_copy, sep, -1);
diff --git a/strbuf.c b/strbuf.c
index b839be4..8fec657 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
strbuf_setlen(sb, j);
}
+
+int strbuf_normalize_path(struct strbuf *src)
+{
+ struct strbuf dst = STRBUF_INIT;
+
+ strbuf_grow(&dst, src->len);
+ if (normalize_path_copy(dst.buf, src->buf) < 0) {
+ strbuf_release(&dst);
+ return -1;
+ }
+
+ /*
+ * normalize_path does not tell us the new length, so we have to
+ * compute it by looking for the new NUL it placed
+ */
+ strbuf_setlen(&dst, strlen(dst.buf));
+ strbuf_swap(src, &dst);
+ strbuf_release(&dst);
+ return 0;
+}
diff --git a/strbuf.h b/strbuf.h
index ba8d5f1..2262b12 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);
*/
extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
+/**
+ * Normalize in-place the path contained in the strbuf. See
+ * normalize_path_copy() for details. If an error occurs, the contents of "sb"
+ * are left untouched, and -1 is returned.
+ */
+extern int strbuf_normalize_path(struct strbuf *sb);
+
/**
* Strip whitespace from a buffer. The second parameter controls if
* comments are considered contents to be removed or not.
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-03 20:34 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
These tests are just trying to show that we allow recursion
up to a certain depth, but not past it. But the counting is
a bit non-intuitive, and rather than test at the edge of the
breakage, we test "OK" cases in the middle of the chain.
Let's explain what's going on, and explicitly test the
switch between "OK" and "too deep".
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 7bc1c3c..b393613 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
)
'
+# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
+# the depth at 0 and count links, not repositories, so in a chain like:
+#
+# A -> B -> C -> D -> E -> F -> G -> H
+# 0 1 2 3 4 5 6
+#
+# we are OK at "G", but break at "H".
+#
+# Note also that we must use "--bare -l" to make the link to H. The "-l"
+# ensures we do not do a connectivity check, and the "--bare" makes sure
+# we do not try to checkout the result (which needs objects), either of
+# which would cause the clone to fail.
test_expect_success 'creating too deep nesting' '
git clone -l -s C D &&
git clone -l -s D E &&
@@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '
git clone --bare -l -s G H
'
-test_expect_success 'invalidity of deepest repository' '
- test_must_fail git -C H fsck
-'
-
-test_expect_success 'validity of third repository' '
- git -C C fsck
+test_expect_success 'validity of fifth-deep repository' '
+ git -C G fsck
'
-test_expect_success 'validity of fourth repository' '
- git -C D fsck
+test_expect_success 'invalidity of sixth-deep repository' '
+ test_must_fail git -C H fsck
'
test_expect_success 'breaking of loops' '
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 04/18] t5613: whitespace/style cleanups
From: Jeff King @ 2016-10-03 20:34 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
Our normal test style these days puts the opening quote of
the body on the description line, and indents the body with
a single tab. This ancient test did not follow this.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 114 +++++++++++++++++++++++++---------------------
1 file changed, 62 insertions(+), 52 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 65074dd..1f283a5 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -8,88 +8,98 @@ test_description='test transitive info/alternate entries'
base_dir=$(pwd)
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo "Hello World" > file1 &&
-git add file1 &&
-git commit -m "Initial commit" file1 &&
-git repack -a -d &&
-git prune'
+test_expect_success 'preparing first repository' '
+ test_create_repo A &&
+ cd A &&
+ echo "Hello World" > file1 &&
+ git add file1 &&
+ git commit -m "Initial commit" file1 &&
+ git repack -a -d &&
+ git prune
+'
cd "$base_dir"
-test_expect_success 'preparing second repository' \
-'git clone -l -s A B && cd B &&
-echo "foo bar" > file2 &&
-git add file2 &&
-git commit -m "next commit" file2 &&
-git repack -a -d -l &&
-git prune'
+test_expect_success 'preparing second repository' '
+ git clone -l -s A B &&
+ cd B &&
+ echo "foo bar" > file2 &&
+ git add file2 &&
+ git commit -m "next commit" file2 &&
+ git repack -a -d -l &&
+ git prune
+'
cd "$base_dir"
-test_expect_success 'preparing third repository' \
-'git clone -l -s B C && cd C &&
-echo "Goodbye, cruel world" > file3 &&
-git add file3 &&
-git commit -m "one more" file3 &&
-git repack -a -d -l &&
-git prune'
+test_expect_success 'preparing third repository' '
+ git clone -l -s B C &&
+ cd C &&
+ echo "Goodbye, cruel world" > file3 &&
+ git add file3 &&
+ git commit -m "one more" file3 &&
+ git repack -a -d -l &&
+ git prune
+'
cd "$base_dir"
-test_expect_success 'creating too deep nesting' \
-'git clone -l -s C D &&
-git clone -l -s D E &&
-git clone -l -s E F &&
-git clone -l -s F G &&
-git clone --bare -l -s G H'
+test_expect_success 'creating too deep nesting' '
+ git clone -l -s C D &&
+ git clone -l -s D E &&
+ git clone -l -s E F &&
+ git clone -l -s F G &&
+ git clone --bare -l -s G H
+'
-test_expect_success 'invalidity of deepest repository' \
-'cd H &&
-test_must_fail git fsck
+test_expect_success 'invalidity of deepest repository' '
+ cd H &&
+ test_must_fail git fsck
'
cd "$base_dir"
-test_expect_success 'validity of third repository' \
-'cd C &&
-git fsck'
+test_expect_success 'validity of third repository' '
+ cd C &&
+ git fsck
+'
cd "$base_dir"
-test_expect_success 'validity of fourth repository' \
-'cd D &&
-git fsck'
+test_expect_success 'validity of fourth repository' '
+ cd D &&
+ git fsck
+'
cd "$base_dir"
-test_expect_success 'breaking of loops' \
-'echo "$base_dir"/B/.git/objects >> "$base_dir"/A/.git/objects/info/alternates&&
-cd C &&
-git fsck'
+test_expect_success 'breaking of loops' '
+ echo "$base_dir"/B/.git/objects >>"$base_dir"/A/.git/objects/info/alternatesi &&
+ cd C &&
+ git fsck
+'
cd "$base_dir"
-test_expect_success 'that info/alternates is necessary' \
-'cd C &&
-rm -f .git/objects/info/alternates &&
-test_must_fail git fsck
+test_expect_success 'that info/alternates is necessary' '
+ cd C &&
+ rm -f .git/objects/info/alternates &&
+ test_must_fail git fsck
'
cd "$base_dir"
-test_expect_success 'that relative alternate is possible for current dir' \
-'cd C &&
-echo "../../../B/.git/objects" > .git/objects/info/alternates &&
-git fsck'
+test_expect_success 'that relative alternate is possible for current dir' '
+ cd C &&
+ echo "../../../B/.git/objects" > .git/objects/info/alternates &&
+ git fsck
+'
cd "$base_dir"
-test_expect_success \
- 'that relative alternate is only possible for current dir' '
- cd D &&
- test_must_fail git fsck
+test_expect_success 'that relative alternate is only possible for current dir' '
+ cd D &&
+ test_must_fail git fsck
'
cd "$base_dir"
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 05/18] t5613: do not chdir in main process
From: Jeff King @ 2016-10-03 20:34 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
Our usual style when working with subdirectories is to chdir
inside a subshell or to use "git -C", which means we do not
have to constantly return to the main test directory. Let's
convert this old test, which does not follow that style.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 92 +++++++++++++++++------------------------------
1 file changed, 33 insertions(+), 59 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 1f283a5..7bc1c3c 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -6,44 +6,39 @@
test_description='test transitive info/alternate entries'
. ./test-lib.sh
-base_dir=$(pwd)
-
test_expect_success 'preparing first repository' '
- test_create_repo A &&
- cd A &&
- echo "Hello World" > file1 &&
- git add file1 &&
- git commit -m "Initial commit" file1 &&
- git repack -a -d &&
- git prune
+ test_create_repo A && (
+ cd A &&
+ echo "Hello World" > file1 &&
+ git add file1 &&
+ git commit -m "Initial commit" file1 &&
+ git repack -a -d &&
+ git prune
+ )
'
-cd "$base_dir"
-
test_expect_success 'preparing second repository' '
- git clone -l -s A B &&
- cd B &&
- echo "foo bar" > file2 &&
- git add file2 &&
- git commit -m "next commit" file2 &&
- git repack -a -d -l &&
- git prune
+ git clone -l -s A B && (
+ cd B &&
+ echo "foo bar" > file2 &&
+ git add file2 &&
+ git commit -m "next commit" file2 &&
+ git repack -a -d -l &&
+ git prune
+ )
'
-cd "$base_dir"
-
test_expect_success 'preparing third repository' '
- git clone -l -s B C &&
- cd C &&
- echo "Goodbye, cruel world" > file3 &&
- git add file3 &&
- git commit -m "one more" file3 &&
- git repack -a -d -l &&
- git prune
+ git clone -l -s B C && (
+ cd C &&
+ echo "Goodbye, cruel world" > file3 &&
+ git add file3 &&
+ git commit -m "one more" file3 &&
+ git repack -a -d -l &&
+ git prune
+ )
'
-cd "$base_dir"
-
test_expect_success 'creating too deep nesting' '
git clone -l -s C D &&
git clone -l -s D E &&
@@ -53,55 +48,34 @@ test_expect_success 'creating too deep nesting' '
'
test_expect_success 'invalidity of deepest repository' '
- cd H &&
- test_must_fail git fsck
+ test_must_fail git -C H fsck
'
-cd "$base_dir"
-
test_expect_success 'validity of third repository' '
- cd C &&
- git fsck
+ git -C C fsck
'
-cd "$base_dir"
-
test_expect_success 'validity of fourth repository' '
- cd D &&
- git fsck
+ git -C D fsck
'
-cd "$base_dir"
-
test_expect_success 'breaking of loops' '
- echo "$base_dir"/B/.git/objects >>"$base_dir"/A/.git/objects/info/alternatesi &&
- cd C &&
- git fsck
+ echo "$(pwd)"/B/.git/objects >>A/.git/objects/info/alternates &&
+ git -C C fsck
'
-cd "$base_dir"
-
test_expect_success 'that info/alternates is necessary' '
- cd C &&
- rm -f .git/objects/info/alternates &&
- test_must_fail git fsck
+ rm -f C/.git/objects/info/alternates &&
+ test_must_fail git -C C fsck
'
-cd "$base_dir"
-
test_expect_success 'that relative alternate is possible for current dir' '
- cd C &&
- echo "../../../B/.git/objects" > .git/objects/info/alternates &&
+ echo "../../../B/.git/objects" >C/.git/objects/info/alternates &&
git fsck
'
-cd "$base_dir"
-
test_expect_success 'that relative alternate is only possible for current dir' '
- cd D &&
- test_must_fail git fsck
+ test_must_fail git -C D fsck
'
-cd "$base_dir"
-
test_done
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 03/18] t5613: use test_must_fail
From: Jeff King @ 2016-10-03 20:34 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
Besides being our normal style, this correctly checks for an
error exit() versus signal death.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 4548fb0..65074dd 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -46,10 +46,9 @@ git clone -l -s F G &&
git clone --bare -l -s G H'
test_expect_success 'invalidity of deepest repository' \
-'cd H && {
- git fsck
- test $? -ne 0
-}'
+'cd H &&
+test_must_fail git fsck
+'
cd "$base_dir"
@@ -75,7 +74,8 @@ cd "$base_dir"
test_expect_success 'that info/alternates is necessary' \
'cd C &&
rm -f .git/objects/info/alternates &&
-! (git fsck)'
+test_must_fail git fsck
+'
cd "$base_dir"
@@ -89,7 +89,7 @@ cd "$base_dir"
test_expect_success \
'that relative alternate is only possible for current dir' '
cd D &&
- ! (git fsck)
+ test_must_fail git fsck
'
cd "$base_dir"
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 02/18] t5613: drop test_valid_repo function
From: Jeff King @ 2016-10-03 20:33 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
This function makes sure that "git fsck" does not report any
errors. But "--full" has been the default since f29cd39
(fsck: default to "git fsck --full", 2009-10-20), and we can
use the exit code (instead of counting the lines) since
e2b4f63 (fsck: exit with non-zero status upon errors,
2007-03-05).
So we can just use "git fsck", which is shorter and more
flexible (e.g., we can use "git -C").
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index e13f57d..4548fb0 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -6,11 +6,6 @@
test_description='test transitive info/alternate entries'
. ./test-lib.sh
-test_valid_repo() {
- git fsck --full > fsck.log &&
- test_line_count = 0 fsck.log
-}
-
base_dir=$(pwd)
test_expect_success 'preparing first repository' \
@@ -52,7 +47,7 @@ git clone --bare -l -s G H'
test_expect_success 'invalidity of deepest repository' \
'cd H && {
- test_valid_repo
+ git fsck
test $? -ne 0
}'
@@ -60,41 +55,41 @@ cd "$base_dir"
test_expect_success 'validity of third repository' \
'cd C &&
-test_valid_repo'
+git fsck'
cd "$base_dir"
test_expect_success 'validity of fourth repository' \
'cd D &&
-test_valid_repo'
+git fsck'
cd "$base_dir"
test_expect_success 'breaking of loops' \
'echo "$base_dir"/B/.git/objects >> "$base_dir"/A/.git/objects/info/alternates&&
cd C &&
-test_valid_repo'
+git fsck'
cd "$base_dir"
test_expect_success 'that info/alternates is necessary' \
'cd C &&
rm -f .git/objects/info/alternates &&
-! (test_valid_repo)'
+! (git fsck)'
cd "$base_dir"
test_expect_success 'that relative alternate is possible for current dir' \
'cd C &&
echo "../../../B/.git/objects" > .git/objects/info/alternates &&
-test_valid_repo'
+git fsck'
cd "$base_dir"
test_expect_success \
'that relative alternate is only possible for current dir' '
cd D &&
- ! (test_valid_repo)
+ ! (git fsck)
'
cd "$base_dir"
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 01/18] t5613: drop reachable_via function
From: Jeff King @ 2016-10-03 20:33 UTC (permalink / raw)
To: git; +Cc: René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
This function was never used since its inception in dd05ea1
(test case for transitive info/alternates, 2006-05-07).
Which is just as well, since it mutates the repo state in a
way that would invalidate further tests, without cleaning up
after itself. Let's get rid of it so that nobody is tempted
to use it.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5613-info-alternate.sh | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 9cd2626..e13f57d 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -6,16 +6,6 @@
test_description='test transitive info/alternate entries'
. ./test-lib.sh
-# test that a file is not reachable in the current repository
-# but that it is after creating a info/alternate entry
-reachable_via() {
- alternate="$1"
- file="$2"
- if git cat-file -e "HEAD:$file"; then return 1; fi
- echo "$alternate" >> .git/objects/info/alternate
- git cat-file -e "HEAD:$file"
-}
-
test_valid_repo() {
git fsck --full > fsck.log &&
test_line_count = 0 fsck.log
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 0/18] alternate object database cleanups
From: Jeff King @ 2016-10-03 20:33 UTC (permalink / raw)
To: git; +Cc: René Scharfe
This series is the result of René nerd-sniping me with the claim that we
could "easily" teach count-objects to print out the list of alternates
in:
http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/
My real goal is just patch 17, which is needed for the quarantine series
in that thread. But along the way there were quite a few opportunities
for cleanups along with a few minor bugfixes (in patches 7 and 18), and
I think the count-objects change in patch 16 is a nice general debugging
tool.
The rest of it is "just" cleanup, but I'll note that it clears up some
hairy allocation code. These were bits that I noticed in my big
allocation-cleanup series last year, but were too nasty to fit any of
the more general fixes. I think the end result is much better.
The number of patches is a little intimidating, but I tried hard to
break the refactoring down into a sequence of obviously-correct steps.
You can be the judge of my success.
[01/18]: t5613: drop reachable_via function
[02/18]: t5613: drop test_valid_repo function
[03/18]: t5613: use test_must_fail
[04/18]: t5613: whitespace/style cleanups
[05/18]: t5613: do not chdir in main process
[06/18]: t5613: clarify "too deep" recursion tests
[07/18]: link_alt_odb_entry: handle normalize_path errors
[08/18]: link_alt_odb_entry: refactor string handling
[09/18]: alternates: provide helper for adding to alternates list
[10/18]: alternates: provide helper for allocating alternate
[11/18]: alternates: encapsulate alt->base munging
[12/18]: alternates: use a separate scratch space
[13/18]: fill_sha1_file: write "boring" characters
[14/18]: alternates: store scratch buffer as strbuf
[15/18]: fill_sha1_file: write into a strbuf
[16/18]: count-objects: report alternates via verbose mode
[17/18]: sha1_file: always allow relative paths to alternates
[18/18]: alternates: use fspathcmp to detect duplicates
Documentation/git-count-objects.txt | 5 +
builtin/count-objects.c | 12 +++
builtin/fsck.c | 10 +-
builtin/submodule--helper.c | 11 +-
cache.h | 36 ++++++-
sha1_file.c | 179 ++++++++++++++++++--------------
sha1_name.c | 17 +--
strbuf.c | 20 ++++
strbuf.h | 8 ++
submodule.c | 23 +---
t/t5613-info-alternate.sh | 202 ++++++++++++++++++++----------------
transport.c | 4 +-
12 files changed, 305 insertions(+), 222 deletions(-)
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Junio C Hamano @ 2016-10-03 20:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79ka8dO1AHJftKAqD6LvxJSP+8yGGa7Citcdxxrnc5DMeYg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> The minimum that would future-proof us, that is still missing from
>> the series, would probably be to separate the query parameter
>> "struct git_attr_check" and the return values from git_check_attr().
>
> Not sure what you mean here with separating as a preparation for
> the thread safety. As I understand it we can still keep the thread local
> states in git_attr_check, we'd just have to route each thread to its
> own part of the memory in there?
For example, look at what you did in your pathspec-label topic.
static int match_attrs(const char *name, int namelen,
const struct pathspec_item *item)
{
int i;
git_check_attr_counted(name, namelen, item->attr_check);
for (i = 0; i < item->attr_match_nr; i++) {
const char *value;
int matched;
enum attr_match_mode match_mode;
value = item->attr_check->check[i].value;
match_mode = item->attr_match[i].match_mode;
Each pathspec item has an attr_check member that wants to see a
specific set of attributes for a path being matched. Each element
of the item->attr_check->check[] array is <attr, value> pair, where
<attr> is a constant for the purpose of the codepath (i.e. no matter
which thread is asking, and no matter for which path the question is
being asked, it asks for a fixed attribute that was computed when
the pathspec was parsed). But <value> is a slot to return the
finding back to the caller.
So you can never keep this code structure and have this function
called more than once, specifically, you cannot make
git_check_attr_counted() call from multiple threads, at one time.
Instead the calling convention needs to be updated to allow this
caller of git_check_attr_counted() to pass a separate array that is
on its stack, e.g.
const char *v[... some size ...];
git_check_attr_counted(name, namelen, item->attr_check, v);
for (i = 0; i < item->attr_match_nr; i++) {
const char *value;
value = v[i];
match_mode = item->attr_match[i].match_mode;
We could do that API update way before we make the attribute
subsystem's implementation thread-safe, and if we did so now,
then the caller will not have to change.
That is what I meant as "future-proofing", i.e. future-proofing the
callers.
And from that point of view, I think 0a5aadcce4 is not an ideal
place to stop. We'd want at least up to 079186123a but probably
even more, e.g. to 48d93f7f42, I would think.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Stefan Beller @ 2016-10-03 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqk2dp71d2.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 3, 2016 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> * jc/attr (2016-05-25) 18 commits
>>> ...
>>> The attributes API has been updated so that it can later be
>>> optimized using the knowledge of which attributes are queried.
>>>
>>> I wanted to polish this topic further to make the attribute
>>> subsystem thread-ready, but because other topics depend on this
>>> topic and they do not (yet) need it to be thread-ready.
>>>
>>> As the authors of topics that depend on this seem not in a hurry,
>>> let's discard this and dependent topics and restart them some other
>>> day.
>>>
>>> Will discard.
>>
>> So I just realized this is a big hint for me to pick up that topic; I assumed
>> you'd want to tackle the attr subsystem eventually, so all I was doing, was
>> waiting for your motivation to look at attr stuff to come back.
>>
>> So what is the actual lacking stuff here?
>
> Quite a bit. Do you want a grand vision, or just the minimum that
> will hopefully futureproof us?
That is a good question. I do want the bare minimum at least; time
permitting I can do more.
I looked through jc/attr and I think all commits up to
0a5aad (2016-05-25, attr.c: plug small leak in parse_attr_line())
are better off if we'd include them today, no matter if we go with the
grand vision or the bare minimum viable, because all commits
up to that one are fixing real issues (memory or readability issues)
Starting with its child b649c7a50c (attr: rename function and struct
related to checking attributes) we start off going on an adventure which
may lead to the grand vision implemented, so these should be held off
until we have implemented a viable way.
>
> The current attribute API is optimized for a very narrow use case
> where a single thread looks up a single set of attributes for
> adjacent paths, without mixing lookups for other attributes of
> distant paths.
>
> Take "conversion" codepath for an example. "git checkout" will
> iterate over the paths in the index and is interested in the eol,
> text and filter attributes (perhaps more, but the details do not
> change the overall picture that much). When it checks dir/fileA, it
> is expected that it would next want to check dir/fileB, before it
> would want to check dir2/fileC and it would move much later to
> otherdir/fileD. It also is expected that it would want to learn the
> same set of attributes, simply because the codepath is doing the
> same operation over these paths (i.e. learn how the Git "clean"
> representation needs to be converted to the external "smudged"
> representation).
>
> Based on the "adjacent paths" assumption, the attribute subsystem
> has a single cache that holds the contents of the .gitattributes
> files that matter to the current query. This has to be split up if
> we ever want to do a parallel checkout with multiple threads. One
> thread may be walking the first half of the index, while another one
> may be walking the second half of the index. They would benefit
> from the same optimization that keeps track of the contents of
> .gitattributes files that matters to each of their traversal. If
> the first thread is responsible for working on dir/fileA, it will
> work on dir/fileB next, before going to dir2/fileC, but it does not
> want to share the cache with the other thread that would be scanning
> entries far-away from what it's scanning, like otherdir/fileD, if it
> evicts the cached information for dir/* that is still useful for the
> first thread.
>
> Another thing we would want to see is to take advantage of the
> "lookup is for single set of attributes from a single codepath" for
> further optimization. The way each of the callers is structured is
> to first declare a set of attributes it is interested in by
> preparing git_attr_check_elem[] array and then make repeated calls
> to git_attr_check() passing it and a path.
>
> The current implementation however does not tie the cache to this
> git_attr_check_elem[] array but has only one single global cache.
> The cache _could_ be used to query any attribute because of it, and
> that leads to inefficiency. It has everything it read from relevant
> .gitattributes files, even the entries that do not affect any
> attributes that a single codepath showed its interest in. I am
> hoping that we can do better by having a per <thread, callsite>
> cache of .gitattributes files, so that a caller in one thread (say,
> "git checkout" that scans the first half of the index) that asks for
> "eol" and "filter" would use a cache that does not have entries
> irrelevant for the attributes the caller is intereseted in, and that
> is tied to the directory hierarchy the caller is asking about
> (i.e. what prepare_attr_stack() does).
>
> Up to 079186123a ("attr: retire git_check_attrs() API", 2016-05-16)
> of the series gives us "struct git_attr_check" to replace the
> git_attr_check_elem[] array. I originally hoped that this struct
> can hold the per-callsite cache itself, before we hit the threading
> issue too early (IIRC, that was preload-index code) and realize that
> the cache needs to be not just per callsite but needs to be per
> <thread, callsite>. This new structure cannot be used to store the
> cache itself, but this change is probably a necessary first step for
> allowing the API in multi-threaded context. git_attr_check_elem[]
> array was static and had slots to receive returned values, which
> would not have worked in threaded environment. We'd further need to
> change it so that the inquiry keys (which are "interned" git_attr
> objects) of "git_attr_check" are initialized just once before
> starting to make repeated calls to git_attr_check(), but the
> mechanism to return values would be thread-safe. git_check_attr()
> call may have to gain an extra/separate variable for the caller to
> specify an array to return values, or something.
I am looking at the tip of jc/attr-more and for a minimum
thread safety we'd want to change the call sites to be aware of the
threads, i.e. instead of doing
if (!check)
check = git_attr_check_initl("crlf", "ident",
"filter", "eol", "text",
NULL);
We'd rather call
struct git_attr_check *check;
check = git_attr_check_lookup_or_initl_threadsafe(
"crlf", "ident", "filter", "eol", "text", NULL);
if (!git_check_attr(path, check)) {
...
So we would make all init functions (git_attr_check_initl,
git_attr_check_alloc) to be aware of the threads and we would
not have a static variable to keep the state as that is global unfortunately,
but rather have a threadsafe lookup function. Though this makes me
wonder how much performance we need to care about here as the version
with the static variable seems to be optimized a lot. So maybe we'd add this
lookup function in attr.h so the respective callers can inline it?
>
> The way "interned" git_attr objects are managed needs to become
> thread-safe by protecting their creation and registering with mutex,
> but that is relatively isolated and straightforward conversion so I
> didn't pay any attention to that in my series. In the final state,
> it of course needs to be taken care of.
>
> So that's the overall "grand vision" picture the series leading up
> to the tip of jc/attr-more was trying to lead us to.
>
> The minimum that would future-proof us, that is still missing from
> the series, would probably be to separate the query parameter
> "struct git_attr_check" and the return values from git_check_attr().
Not sure what you mean here with separating as a preparation for
the thread safety. As I understand it we can still keep the thread local
states in git_attr_check, we'd just have to route each thread to its
own part of the memory in there?
> Once it is done, I think we do not need to update the caller when we
> update the attr.c infrastructure to be thread-safe.
^ permalink raw reply
* Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-10-03 19:31 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Satoshi Yasushima, git, Jakub Narębski
In-Reply-To: <xmqq60p98kym.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Pat Thoyts <patthoyts@users.sourceforge.net> writes:
>
>> I'm just starting to catch up once again. hopefully I can be
>> a bit more reactive than recently. Merging 52285c83 looks fine. I'll
>> stick that onto the 0.20.0 head and see what else I can pick up on top.
>> There are a few from the git for windows set among others.
>
> Nice to hear from you again. I think I have a few topics I merged
> to my tree bypassing you in the meantime. Let me get back to you
> with a list of topic tips to bring your tree in sync with what I
> have later.
I think the following lists everything that has been done bypassing
your tree:
66fe3e061a ("git-gui: l10n: add Portuguese translation", 2016-05-06)
52285c8312 ("git-gui: update Japanese information", 2016-09-07)
2afe6b733e ("git-gui: respect commit.gpgsign again", 2016-09-09)
b5f325cb4a ("git-gui: stop using deprecated merge syntax", 2016-09-24)
52285c8312 and 2afe6b733e are already in my 'master'; the other two
are already cooking in 'next'.
So if you fetch from me and merge the above, you'd be in sync with
me (I won't be in sync with you, as you would have more than I have
from other places like Git for Windows set).
Thanks.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-10-03 19:17 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <e03fdabd-6690-5244-5f79-1715b0364845@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> That sounds reasonable to me. Would a patch set to implement this new
> trailer block heuristic (in both sequencer and trailer) be reasonable?
> And if yes, should trailer know about the "(cherry picked from"
> prefix? (I can see it both ways - knowing about the "(cherry picked
> from" prefix would make it consistent with sequencer, but it seems
> like a detail that it shouldn't know about. Writing
> "Cherry-picked-from:" instead probably wouldn't solve our problem
> because, for backwards compatibility, we would still need to support
> reading the old format.)
If we were to go that route, I'd suggest keeping the historical
practice supported, exactly because you would need to be prepared to
cherry-pick an old commit.
It may be necessary for the code to analize the lines in a block
identified as "likely to be a trailing block" more carefully,
though. The example 59f0aa94 in the message you are responding to
has "Link 1:" that consists of 3 physical lines. An instruction to
interpret-trailers to add a new one _after_ "Link-$n:" may have to
treat these as a single logical line and do the addition after them,
i.e. before "Link 2:" line, for example.
I also saw
Signed-off-by: Some body <some@body.xz> (some comment
that extends to the next line without being indented)
Sined-off-by: Some body Else <some.body@else.xz>
where the only clue that the second line is logically a part of the
first one was the balancing of parentheses (or [brakets]). To
accomodate real-world use cases, you may have to take into account a
lot more than the strict rfc-822 style "line folding".
^ permalink raw reply
* Re: Reference a submodule branch instead of a commit
From: Junio C Hamano @ 2016-10-03 19:00 UTC (permalink / raw)
To: Jeremy Morton; +Cc: git
In-Reply-To: <57F29FEF.30700@game-point.net>
Jeremy Morton <admin@game-point.net> writes:
> At the moment, supermodules must reference a given commit in each of
> its submodules. If one is in control of a submodule and it changes on
> a regular basis, this can cause a lot of overhead with "submodule
> updated" commits in the supermodule. It would be useful of git allows
> the option of referencing a submodule's branch instead of a given
> submodule commit. How about adding this functionality?
When somebody downstream fetches from your superproject and grabs
the set of submodules, how would s/he know what _exact_ state you
meant to record? When s/he says "I have your superproject commit X,
which binds submodule's branch Y at path sub/, and it simply does
not work. Your project is broken", how do you go about reproducing
the exact state s/he had trouble with to help her/him?
The only thing s/he knows is that the commit used from the submodule
must be one of the commits that was on branch Y at some point in
time, hopefully close to the timestamp recorded in the commit in the
superproject. And your record in the history of the superproject
does not tell you more than that, so you wouldn't have any idea
better than what s/he already has to help.
Hence, such a "functionality" will never happen, at least in the
exact form you are describing.
It is conceivable to add some feature that allows you to squelch the
report that the submodule recorded in your superproject is not up to
date from "git status" etc. to help those who thinks it is OK to not
bind the latest submodule commit to the superproject all the time,
though.
^ permalink raw reply
* Reference a submodule branch instead of a commit
From: Jeremy Morton @ 2016-10-03 18:14 UTC (permalink / raw)
To: git
At the moment, supermodules must reference a given commit in each of
its submodules. If one is in control of a submodule and it changes on
a regular basis, this can cause a lot of overhead with "submodule
updated" commits in the supermodule. It would be useful of git allows
the option of referencing a submodule's branch instead of a given
submodule commit. How about adding this functionality?
--
Best regards,
Jeremy Morton (Jez)
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Junio C Hamano @ 2016-10-03 18:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kY6c-vwSP7-1Gz4jwWO-z_yT2oFbG4cgZb-JAae=Sk-cA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> * jc/attr (2016-05-25) 18 commits
>> ...
>> The attributes API has been updated so that it can later be
>> optimized using the knowledge of which attributes are queried.
>>
>> I wanted to polish this topic further to make the attribute
>> subsystem thread-ready, but because other topics depend on this
>> topic and they do not (yet) need it to be thread-ready.
>>
>> As the authors of topics that depend on this seem not in a hurry,
>> let's discard this and dependent topics and restart them some other
>> day.
>>
>> Will discard.
>
> So I just realized this is a big hint for me to pick up that topic; I assumed
> you'd want to tackle the attr subsystem eventually, so all I was doing, was
> waiting for your motivation to look at attr stuff to come back.
>
> So what is the actual lacking stuff here?
Quite a bit. Do you want a grand vision, or just the minimum that
will hopefully futureproof us?
The current attribute API is optimized for a very narrow use case
where a single thread looks up a single set of attributes for
adjacent paths, without mixing lookups for other attributes of
distant paths.
Take "conversion" codepath for an example. "git checkout" will
iterate over the paths in the index and is interested in the eol,
text and filter attributes (perhaps more, but the details do not
change the overall picture that much). When it checks dir/fileA, it
is expected that it would next want to check dir/fileB, before it
would want to check dir2/fileC and it would move much later to
otherdir/fileD. It also is expected that it would want to learn the
same set of attributes, simply because the codepath is doing the
same operation over these paths (i.e. learn how the Git "clean"
representation needs to be converted to the external "smudged"
representation).
Based on the "adjacent paths" assumption, the attribute subsystem
has a single cache that holds the contents of the .gitattributes
files that matter to the current query. This has to be split up if
we ever want to do a parallel checkout with multiple threads. One
thread may be walking the first half of the index, while another one
may be walking the second half of the index. They would benefit
from the same optimization that keeps track of the contents of
.gitattributes files that matters to each of their traversal. If
the first thread is responsible for working on dir/fileA, it will
work on dir/fileB next, before going to dir2/fileC, but it does not
want to share the cache with the other thread that would be scanning
entries far-away from what it's scanning, like otherdir/fileD, if it
evicts the cached information for dir/* that is still useful for the
first thread.
Another thing we would want to see is to take advantage of the
"lookup is for single set of attributes from a single codepath" for
further optimization. The way each of the callers is structured is
to first declare a set of attributes it is interested in by
preparing git_attr_check_elem[] array and then make repeated calls
to git_attr_check() passing it and a path.
The current implementation however does not tie the cache to this
git_attr_check_elem[] array but has only one single global cache.
The cache _could_ be used to query any attribute because of it, and
that leads to inefficiency. It has everything it read from relevant
.gitattributes files, even the entries that do not affect any
attributes that a single codepath showed its interest in. I am
hoping that we can do better by having a per <thread, callsite>
cache of .gitattributes files, so that a caller in one thread (say,
"git checkout" that scans the first half of the index) that asks for
"eol" and "filter" would use a cache that does not have entries
irrelevant for the attributes the caller is intereseted in, and that
is tied to the directory hierarchy the caller is asking about
(i.e. what prepare_attr_stack() does).
Up to 079186123a ("attr: retire git_check_attrs() API", 2016-05-16)
of the series gives us "struct git_attr_check" to replace the
git_attr_check_elem[] array. I originally hoped that this struct
can hold the per-callsite cache itself, before we hit the threading
issue too early (IIRC, that was preload-index code) and realize that
the cache needs to be not just per callsite but needs to be per
<thread, callsite>. This new structure cannot be used to store the
cache itself, but this change is probably a necessary first step for
allowing the API in multi-threaded context. git_attr_check_elem[]
array was static and had slots to receive returned values, which
would not have worked in threaded environment. We'd further need to
change it so that the inquiry keys (which are "interned" git_attr
objects) of "git_attr_check" are initialized just once before
starting to make repeated calls to git_attr_check(), but the
mechanism to return values would be thread-safe. git_check_attr()
call may have to gain an extra/separate variable for the caller to
specify an array to return values, or something.
The way "interned" git_attr objects are managed needs to become
thread-safe by protecting their creation and registering with mutex,
but that is relatively isolated and straightforward conversion so I
didn't pay any attention to that in my series. In the final state,
it of course needs to be taken care of.
So that's the overall "grand vision" picture the series leading up
to the tip of jc/attr-more was trying to lead us to.
The minimum that would future-proof us, that is still missing from
the series, would probably be to separate the query parameter
"struct git_attr_check" and the return values from git_check_attr().
Once it is done, I think we do not need to update the caller when we
update the attr.c infrastructure to be thread-safe.
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: Kevin Bracey @ 2016-10-03 16:46 UTC (permalink / raw)
To: GIT Mailing-list
In-Reply-To: <83398160-555f-adab-6b1e-3283c533b5ff@web.de>
On 01/10/2016 19:19, René Scharfe wrote:
> It's hard to imagine an implementation of qsort(3) that can't handle
> zero elements. QSORT's safety feature is that it prevents the compiler
> from removing NULL checks for the array pointer. E.g. the last two
> lines in the following example could be optimized away:
>
> qsort(ptr, n, sizeof(*ptr), fn);
> if (!ptr)
> do_stuff();
>
> You can see that on https://godbolt.org/g/JwS99b -- an awesome website
> for exploring compilation results for small snippets, by the way.
>
> This optimization is dangerous when combined with the convention of
> using a NULL pointer for empty arrays. Diagnosing an affected NULL
> check is probably quite hard -- it's right there in the code after all
> and not all compilers remove it.
Hang on, hang on. This is either a compiler bug, or you're wrong on your
assumption about the specification of qsort.
Either way, the extra layer of indirection is not proper protection. The
unwanted compiler optimisation you're inadvertently triggering could
still be triggered through the inline.
Now, looking at the C standard, this isn't actually clear to me. The
standard says that if you call qsort with nmemb zero, the pointer still
has to be "valid". Not totally clear to me if NULL is valid, by their
definition in C99 7.1.4. Googling hasn't given me a concrete answer.
The compiler seems to think that NULL wouldn't be valid, so because
you've called qsort on it, you've invoked undefined behaviour if it's
NULL, so it's free to elide the NULL check.
Kevin
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-10-03 17:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqtwcx8669.fsf@gitster.mtv.corp.google.com>
On 09/30/2016 01:49 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>>> I vaguely recall that there were some discussion on the definition
>>>> of "what's a trailer line" with folks from the kernel land, perhaps
>>>> while discussing the interpret-trailers topic. IIRC, when somebody
>>>> passes an improved version along, the resulting message's trailer
>>>> block may look like this:
>>>>
>>>> Signed-off-by: Original Author <original@author.xz>
>>>> [fixed typo in the variable names]
>>>> Signed-off-by: Somebhody Else <somebody@else.xz>
>>>>
>>>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>>>> "a line that begins with token followed by a colon" but also these
>>>> short comments as part of the trailer block. Your original wish in
>>>> [*1*] is to also treat "a line that begin with a whitespace that
>>>> follows a line that begins with token followed by a colon" as part
>>>> of the trailer block and I personally think that is a reasonable
>>>> thing to wish for, too.
>>>
>>> If we allowed arbitrary lines in the trailer block, this would solve
>>> my original problem, yes.
>
> Here is an experiment I ran during my lunch break. The script
> (attached) is meant to run in the kernel repository and
> for each log messages of each non-merge commit:
>
> * find its last paragraph, where the definition of paragraph is
> simply "a blank/empty line";
>
> * inspect if there is at least one RFC2822-header-looking line, or
> a line that begins with "(cherry picked from";
>
> * dump the ones that do not pass the above criteria.
>
> My cursory look of the output did not spot a legitimate trailer
> block that we should have identified. The output lines shown were
> ones that are not signed off at all (e.g. af8c34ce6ae32add that says
> "Linux 4.7-rc2"), ones that has three-dash line "---" in them
> (e.g. 133d558216d9), ones that has diffstat that should have been
> after "---" (e.g. 259307074bfcf1f).
>
> The story is the same if you run it in git.git; the "do we have at
> least one rfc2822-header-looking line or '(cherry picked from' line
> in the last paragraph? if so, then that is an existing trailer
> block" seems to be a good heuristics to cover many cases like
> these:
>
> d0196c8d5d3057c5c21a82f3d0113ca8e501033b
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [tomi.valkeinen@ti.com: resolved conflicts]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> 59f0aa9480cfef9173a648cec4537addc5f3ad94
> Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916
> http://bugzilla.kernel.org/show_bug.cgi?id=10100
> https://lkml.org/lkml/2008/2/25/282
> Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399
> https://bugzilla.kernel.org/show_bug.cgi?id=12461
> https://bugzilla.kernel.org/show_bug.cgi?id=11880
> Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884
> https://bugzilla.kernel.org/show_bug.cgi?id=14081
> https://bugzilla.kernel.org/show_bug.cgi?id=14086
> https://bugzilla.kernel.org/show_bug.cgi?id=14446
> Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
That sounds reasonable to me. Would a patch set to implement this new
trailer block heuristic (in both sequencer and trailer) be reasonable?
And if yes, should trailer know about the "(cherry picked from" prefix?
(I can see it both ways - knowing about the "(cherry picked from" prefix
would make it consistent with sequencer, but it seems like a detail that
it shouldn't know about. Writing "Cherry-picked-from:" instead probably
wouldn't solve our problem because, for backwards compatibility, we
would still need to support reading the old format.)
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Lars Schneider @ 2016-10-03 17:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, git, Jeff King, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <xmqqvax974dl.fsf@gitster.mtv.corp.google.com>
> On 03 Oct 2016, at 19:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> If the filter process refuses to die forever when Git told it to
>>> shutdown (by closing the pipe to it, for example), that filter
>>> process is simply buggy. I think we want users to become aware of
>>> that, instead of Git leaving it behind, which essentially is to
>>> sweep the problem under the rug.
>>>
>>> I agree with what Peff said elsewhere in the thread; if a filter
>>> process wants to take time to clean things up while letting Git
>>> proceed, it can do its own process management, but I think it is
>>> sensible for Git to wait the filter process it directly spawned.
>>
>> To realize the approach above I prototyped the run-command patch below:
>>
>> I added an "exit_timeout" variable to the "child_process" struct.
>> On exit, Git will close the pipe to the process and wait "exit_timeout"
>> seconds until it kills the child process. If "exit_timeout" is negative
>> then Git will wait until the process is done.
>
>> If we use that in the long running filter process, then we could make
>> the timeout even configurable. E.g. with "filter.<driver>.process-timeout".
>>
>> What do you think about this solution?
>
> Is such a configuration (or timeout in general) necessary? I
> suspect that a need for timeout, especially needing timeout and
> needing to get killed that happens so often to require a
> configuration variable, is a sign of something else seriously wrong.
>
> What's the justification for a filter to _require_ getting killed
> all the time when it is spawned? Otherwise you wouldn't configure
> "this driver does not die when told, so we need a timeout" variable.
> Is it a sign of the flaw in the protocol to talk to it? e.g. Git
> has a way to tell it to die, but it somehow is very hard to hear
> from filter's end and honor that request?
>
> I think that we would need some timeout in the mechanism, but not to
> be used for "killing".
>
> You would decide to "kill" an filter process in two cases: the
> filter is buggy and refuses to die when Git tells it to exit, or the
> code in Git waiting for its death is somehow miscounting its
> children, and thought it told to die one process but in fact it
> didn't (perhaps it told somebody else to die), or it thought it
> hasn't seen the child die when in fact it already did.
Agreed.
> Calling kill(2) and exiting would hide these two kind of bugs from
> end users. Not doing so would give the end users a hung Git, which
> is a VERY GOOD thing. Otherwise you would not notice bugs and lose
> the opportunity to diagnose and fix it.
Aha. I assumed that a hung Git because of a buggy filter would be a no-no.
Thanks for this clarification.
> The timeout would be good for you to give a message "filter process
> running the script '%s' is not exiting; I am waiting for it". The
> user is still left with a hung Git, and can then see if that process
> is hanging around. If it is, then we found a buggy filter. Or we
> found a buggy Git. Either needs to be fixed. I do not think it
> would help anybody by doing a kill(2) to sweep possible bugs under
> the rug.
I could achieve that with this run-command patch:
http://public-inbox.org/git/E9946E9F-6EE5-492B-B122-9078CEB88044@gmail.com/
(I'll remove the "timeout after x seconds" parts and keep the "wait until
done" part with stderr output)
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: Kevin Bracey @ 2016-10-03 17:09 UTC (permalink / raw)
To: GIT Mailing-list, René Scharfe
In-Reply-To: <83398160-555f-adab-6b1e-3283c533b5ff@web.de>
On 01/10/2016 19:19, René Scharfe wrote:
>
> It's hard to imagine an implementation of qsort(3) that can't handle
> zero elements. QSORT's safety feature is that it prevents the compiler
> from removing NULL checks for the array pointer. E.g. the last two
> lines in the following example could be optimized away:
>
> qsort(ptr, n, sizeof(*ptr), fn);
> if (!ptr)
> do_stuff();
>
> You can see that on https://godbolt.org/g/JwS99b -- an awesome website
> for exploring compilation results for small snippets, by the way.
>
Ah, second attempt. Originally misread the original code, and didn't
understand what it was doing.
I get it now.
A nasty trap I hadn't been aware of - I was under the impression NULL +
zero length was generally legal, but the C standard does indeed not give
you a specific out for NULL to library functions in that case.
As such, NULL checks can still be elided even with your change. If you
effectively change your example to:
if (nmemb > 1)
qsort(array, nmemb, size, cmp);
if (!array)
printf("array is NULL\n");
array may only be checked for NULL if nmemb <= 1. You can see GCC doing
that in the compiler explorer - it effectively turns that into "else
if". To make that check really work, you have to do:
if (array)
qsort(array, nmemb, size, cmp);
else
printf("array is NULL\n");
So maybe your "sane_qsort" should be checking array, not nmemb.
Kevin
^ permalink raw reply
* [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames
From: David Turner @ 2016-10-03 17:19 UTC (permalink / raw)
To: git, sandals; +Cc: David Turner
When using kerberos authentication, one URL pattern which is
allowed is http://@gitserver.example.com. This leads to a username
of zero-length, rather than a NULL username. But the two cases
should be treated the same by http.emptyauth.
Signed-off-by: David Turner <dturner@twosigma.com>
---
http.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 82ed542..bd0dba2 100644
--- a/http.c
+++ b/http.c
@@ -351,7 +351,7 @@ static int http_options(const char *var, const char *value, void *cb)
static void init_curl_http_auth(CURL *result)
{
- if (!http_auth.username) {
+ if (!http_auth.username || !*http_auth.username) {
if (curl_empty_auth)
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [RFC PATCH] clone: add clone.recursesubmodules config option
From: Stefan Beller @ 2016-10-03 17:18 UTC (permalink / raw)
To: Jeremy Morton
Cc: Chris Packham, git@vger.kernel.org, mara.kim, Junio C Hamano
In-Reply-To: <57F27B02.8080803@game-point.net>
On Mon, Oct 3, 2016 at 8:36 AM, Jeremy Morton <admin@game-point.net> wrote:
> Did this ever get anywhere? Can we recursively update submodules with "git
> pull" in the supermodule now?
I think the idea is sound.
>> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
>> index 7ca10b8..fc2c189 100755
>> --- a/t/t7407-submodule-foreach.sh
>> +++ b/t/t7407-submodule-foreach.sh
Not sure if t7407-submodule-foreach.sh is the best place to put these tests,
as it is not `submodule foreach`, maybe put it into 7400 (though that
is larger already)
>> +test_expect_success 'use "git clone" with clone.recursesubmodules to
>> checkout all submodules' '
>> + git config --local clone.recursesubmodules true&&
Nit of the day:
I think we prefer a single white space between the line and the ending
&&.
No need for --local as that is the default.
However I'd propose to use test_config here,
as then the option is cleaned up after the test
automatically.
>> + git clone super clone7&&
>> + (
>> + cd clone7&&
>> + git rev-parse --resolve-git-dir .git&&
>> + git rev-parse --resolve-git-dir sub1/.git&&
>> + git rev-parse --resolve-git-dir sub2/.git&&
>> + git rev-parse --resolve-git-dir sub3/.git&&
>> + git rev-parse --resolve-git-dir nested1/.git&&
>> + git rev-parse --resolve-git-dir nested1/nested2/.git&&
>> + git rev-parse --resolve-git-dir
>> nested1/nested2/nested3/.git&&
>> + git rev-parse --resolve-git-dir
>> nested1/nested2/nested3/submodule/.git
>> + )&&
>> + git config --local --unset clone.recursesubmodules
No need to unset it here when test_config is used.
We'd maybe would want to also test that
git -c clone.recursesubmodules clone --no-recursive ...
works as expected (the --no-recursive taking precedence
over the config option)
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Lars Schneider @ 2016-10-03 17:13 UTC (permalink / raw)
To: Jakub Narębski
Cc: Junio C Hamano, Torsten Bögershausen, git, Jeff King,
Stefan Beller, Martin-Louis Bright, Ramsay Jones
In-Reply-To: <15ff438f-ec58-e649-b927-b1de4751cc45@gmail.com>
> On 01 Oct 2016, at 22:48, Jakub Narębski <jnareb@gmail.com> wrote:
>
> W dniu 01.10.2016 o 20:59, Lars Schneider pisze:
>> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@pobox.com> wrote:
>>> Lars Schneider <larsxschneider@gmail.com> writes:
>>>
>>> If the filter process refuses to die forever when Git told it to
>>> shutdown (by closing the pipe to it, for example), that filter
>>> process is simply buggy. I think we want users to become aware of
>>> that, instead of Git leaving it behind, which essentially is to
>>> sweep the problem under the rug.
>
> Well, it would be good to tell users _why_ Git is hanging, see below.
Agreed. Do you think it is OK to write the message to stderr?
>>> I agree with what Peff said elsewhere in the thread; if a filter
>>> process wants to take time to clean things up while letting Git
>>> proceed, it can do its own process management, but I think it is
>>> sensible for Git to wait the filter process it directly spawned.
>>
>> To realize the approach above I prototyped the run-command patch below:
>>
>> I added an "exit_timeout" variable to the "child_process" struct.
>> On exit, Git will close the pipe to the process and wait "exit_timeout"
>> seconds until it kills the child process. If "exit_timeout" is negative
>> then Git will wait until the process is done.
>
> That might be good approach. Probably the default would be to wait.
I think I would prefer a 2sec timeout or something as default. This way
we can ensure Git would not wait indefinitely for a buggy filter by default.
>> If we use that in the long running filter process, then we could make
>> the timeout even configurable. E.g. with "filter.<driver>.process-timeout".
>
> Sidenote: we prefer camelCase rather than kebab-case for config
> variables, that is, "filter.<driver>.processTimeout".
Sure!
> Also, how would one set default value of timeout for all process
> based filters?
I think we don't need that because a timeout is always specific
to a filter (if the 2sec default is not sufficient).
>>
>> + while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
>> + ; /* nothing */
>
> Ah, this loop is here because waiting on waitpid() can be interrupted
> by the delivery of a signal to the calling process; though the result
> is -1, not just any < 0.
"< 0" is also used in wait_or_whine()
>> + while (getpgid(p->pid) >= 0 && tv.tv_sec - secs < p->timeout) {
>> + gettimeofday(&tv, NULL);
>> + }
>
> WTF? Busy wait loop???
This was just a quick prototype to show "my thinking direction".
I wasn't expecting a review. Sorry :-)
> Also, if we want to wait for child without blocking, then instead
> of cryptic getpgid(p->pid) maybe use waitpid(p->pid, &status, WNOHANG);
> it is more explicit.
Sure!
> There is also another complication: there can be more than one
> long-running filter driver used. With this implementation we
> wait for each of one in sequence, e.g. 10s + 10s + 10s.
Good idea, I fixed that in the version below!
>>
>> -static void mark_child_for_cleanup(pid_t pid)
>> +static void mark_child_for_cleanup(pid_t pid, int timeout, int stdin)
>
> Hmmmm... three parameters is not too much, but we might want to
> pass "struct child_process *" directly if this number grows.
I used parameters because this function is also used with the async struct...
I've attached a more serious patch for review below.
What do you think?
Thanks,
Lars
diff --git a/run-command.c b/run-command.c
index 3269362..ca0feef 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,9 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
+ char *name;
+ int stdin;
+ int timeout;
struct child_to_clean *next;
};
static struct child_to_clean *children_to_clean;
@@ -28,12 +31,53 @@ static int installed_child_cleanup_handler;
static void cleanup_children(int sig, int in_signal)
{
+ int status;
+ struct timeval tv;
+ time_t secs;
+ struct child_to_clean *p = children_to_clean;
+
+ // Send EOF to children as indicator that Git will exit soon
+ while (p) {
+ if (p->timeout != 0) {
+ if (p->stdin > 0)
+ close(p->stdin);
+ }
+ p = p->next;
+ }
+
while (children_to_clean) {
- struct child_to_clean *p = children_to_clean;
+ p = children_to_clean;
children_to_clean = p->next;
+
+ if (p->timeout != 0) {
+ fprintf(stderr, _("Waiting for '%s' to finish..."), p->name);
+ if (p->timeout < 0) {
+ // No timeout given - wait indefinitely
+ while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
+ ; /* nothing */
+ } else {
+ // Wait until timeout
+ gettimeofday(&tv, NULL);
+ secs = tv.tv_sec;
+ while (!waitpid(p->pid, &status, WNOHANG) &&
+ tv.tv_sec - secs < p->timeout) {
+ fprintf(stderr, _(" \rWaiting %lds for '%s' to finish..."),
+ p->timeout - tv.tv_sec + secs - 1, p->name);
+ gettimeofday(&tv, NULL);
+ sleep_millisec(10);
+ }
+ }
+ if (waitpid(p->pid, &status, WNOHANG))
+ fprintf(stderr, _("done!\n"));
+ else
+ fprintf(stderr, _("timeout. Killing...\n"));
+ }
+
kill(p->pid, sig);
- if (!in_signal)
+ if (!in_signal) {
+ free(p->name);
free(p);
+ }
}
}
@@ -49,10 +93,18 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup_with_timeout(pid_t pid, const char *name, int stdin, int timeout)
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+ p->timeout = timeout;
+ p->stdin = stdin;
+ if (name) {
+ p->name = xmalloc(strlen(name) + 1);
+ strcpy(p->name, name);
+ } else {
+ p->name = "process";
+ }
p->next = children_to_clean;
children_to_clean = p;
@@ -63,6 +115,13 @@ static void mark_child_for_cleanup(pid_t pid)
}
}
+#ifdef NO_PTHREADS
+static void mark_child_for_cleanup(pid_t pid, const char *name, int timeout, int stdin)
+{
+ mark_child_for_cleanup_with_timeout(pid, NULL, 0, 0);
+}
+#endif
+
static void clear_child_for_cleanup(pid_t pid)
{
struct child_to_clean **pp;
@@ -422,7 +481,8 @@ int start_command(struct child_process *cmd)
if (cmd->pid < 0)
error_errno("cannot fork() for %s", cmd->argv[0]);
else if (cmd->clean_on_exit)
- mark_child_for_cleanup(cmd->pid);
+ mark_child_for_cleanup_with_timeout(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
/*
* Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -483,7 +543,8 @@ int start_command(struct child_process *cmd)
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error_errno("cannot spawn %s", cmd->argv[0]);
if (cmd->clean_on_exit && cmd->pid >= 0)
- mark_child_for_cleanup(cmd->pid);
+ mark_child_for_cleanup_with_timeout(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
argv_array_clear(&nargv);
cmd->argv = sargv;
diff --git a/run-command.h b/run-command.h
index cf29a31..4c1c1f4 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,16 @@ struct child_process {
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
unsigned clean_on_exit:1;
+ /*
+ * clean_on_exit_timeout is only considered if clean_on_exit is set.
+ * - Specify 0 to kill the child on Git exit (default)
+ * - Specify a negative value to close the child's stdin on Git exit
+ * and wait indefinitely for the child's termination.
+ * - Specify a positive value to close the child's stdin on Git exit
+ * and wait clean_on_exit_timeout seconds for the child's
+ * termination.
+ */
+ int clean_on_exit_timeout;
};
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
^ permalink raw reply related
* Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation
From: Junio C Hamano @ 2016-10-03 17:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Linus Torvalds
In-Reply-To: <20161001091558.guduirzlkog5fkzd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I guess my point was that the poor name may have contributed to the need
> to explain it.
The comment was not about "it may not be obvious but this tries to
pad and align", but to say "the way this tries to pad and align is
based on an unsaid assumption that leads to this limitation". I do
agree it is a good idea to rename it to a name that has 'pad' or
'align' in addition to 'unique', but I doubt renaming alone would
reduce the need for the new comment.
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Junio C Hamano @ 2016-10-03 17:02 UTC (permalink / raw)
To: Lars Schneider
Cc: Torsten Bögershausen, git, Jeff King, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <C53500E8-7352-4AAC-9F53-40CCFA7F1418@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> If the filter process refuses to die forever when Git told it to
>> shutdown (by closing the pipe to it, for example), that filter
>> process is simply buggy. I think we want users to become aware of
>> that, instead of Git leaving it behind, which essentially is to
>> sweep the problem under the rug.
>>
>> I agree with what Peff said elsewhere in the thread; if a filter
>> process wants to take time to clean things up while letting Git
>> proceed, it can do its own process management, but I think it is
>> sensible for Git to wait the filter process it directly spawned.
>
> To realize the approach above I prototyped the run-command patch below:
>
> I added an "exit_timeout" variable to the "child_process" struct.
> On exit, Git will close the pipe to the process and wait "exit_timeout"
> seconds until it kills the child process. If "exit_timeout" is negative
> then Git will wait until the process is done.
> If we use that in the long running filter process, then we could make
> the timeout even configurable. E.g. with "filter.<driver>.process-timeout".
>
> What do you think about this solution?
Is such a configuration (or timeout in general) necessary? I
suspect that a need for timeout, especially needing timeout and
needing to get killed that happens so often to require a
configuration variable, is a sign of something else seriously wrong.
What's the justification for a filter to _require_ getting killed
all the time when it is spawned? Otherwise you wouldn't configure
"this driver does not die when told, so we need a timeout" variable.
Is it a sign of the flaw in the protocol to talk to it? e.g. Git
has a way to tell it to die, but it somehow is very hard to hear
from filter's end and honor that request?
I think that we would need some timeout in the mechanism, but not to
be used for "killing".
You would decide to "kill" an filter process in two cases: the
filter is buggy and refuses to die when Git tells it to exit, or the
code in Git waiting for its death is somehow miscounting its
children, and thought it told to die one process but in fact it
didn't (perhaps it told somebody else to die), or it thought it
hasn't seen the child die when in fact it already did.
Calling kill(2) and exiting would hide these two kind of bugs from
end users. Not doing so would give the end users a hung Git, which
is a VERY GOOD thing. Otherwise you would not notice bugs and lose
the opportunity to diagnose and fix it.
The timeout would be good for you to give a message "filter process
running the script '%s' is not exiting; I am waiting for it". The
user is still left with a hung Git, and can then see if that process
is hanging around. If it is, then we found a buggy filter. Or we
found a buggy Git. Either needs to be fixed. I do not think it
would help anybody by doing a kill(2) to sweep possible bugs under
the rug.
Thanks.
^ permalink raw reply
* Re: [Q] would it be bad to make /etc/gitconfig runtime configurable?
From: Junio C Hamano @ 2016-10-03 16:24 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20161003112654.3vca4zmctslcudfz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I admit both of those are uses for git _developers_, though, not git
> _users_.
Yes, this is meant for developers and not users.
The initial question probably should have stated more explicitly,
e.g. "I am wondering if it would be helpful to developers if we add
this thing; does anybody think of a reason why exposing it to end
users is a bad idea?"
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox