* CVSImport - spaces in CVS path
From: Yojoa @ 2016-11-30 20:56 UTC (permalink / raw)
To: git
I'm in the process of moving an entire collection of cvs modules into git.
I'm working in Mac Yosemite. Everything is working fine except for one
thing. A couple of the CVS modules have spaces in the paths. Below is what
my command line looks like. When the path has spaces I've tried putting it
in single and double quotes and using escape characters. None of that
matters. I always get a message that it can't read dir/dir/path and then
messages that it can't find the modules "with" or "spaces".
git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/
dir/dir/PathWithNoSpaces/dir
git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/ dir/dir/path with
spaces/dir
--
View this message in context: http://git.661346.n2.nabble.com/CVSImport-spaces-in-CVS-path-tp7657459.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCHv2 4/4] submodule: add embed-git-dir function
From: Stefan Beller @ 2016-11-30 21:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Brandon Williams, Git Mailing List, Jonathan Nieder,
Jens Lehmann, Heiko Voigt
In-Reply-To: <xmqqfum8d4w3.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 30, 2016 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> git relocate-git-dir (--into-workingtree|--into-gitdir) \
>
> I am not sure if you meant this as a submodule-specific subcommand
> or more general helper. "into-workingtree" suggests to me that it
> is submodule specific, so I'll base my response on that assumption.
>
> Would there ever be a situation where you already have submodule
> repositories in the right place (according to the more modern
> practice, to keep them in .git/modules/ of superproject) and want to
> move them to embed them in worktrees of submodules? I do not think
> of any.
"Hi, I made a mistake by using submodules. I don't want to use
them any more, I rather want to:
A) make it a separate git repo again and I'll keep them in sync myself
B) ... "
"I abuse submodules for what git-LFS was designed for, and the
submodule is on a different mount point, please keep the git directory
also at that mount point".
Not sure I agree these problems and the proposed solutions are beautiful,
but that is what people may think of as a fast hack?
>
> If there is no such situation, I do not think we want a verb that is
> direction-neutral (e.g. "move" or "relocate") with two options.
> Rather we would want "git submodule unembed-git-dir" or something
> like that.
So when we want to have a generic function in C ("relocate_gitdir")
for both worktree and submodules, the recursive flag is not supposed
to invoke a submodule specific helper, but a generic helper.
Alternatively we make the function not as generic and claim the
recursive part is submodule specific and we can happily call
"git submodule [un]embed-git-dir" recursively.
^ permalink raw reply
* [RFC/PATCH v3 09/16] Add GIT_NO_EXTERNAL_ODB env variable
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
cache.h | 9 +++++++++
environment.c | 4 ++++
external-odb.c | 6 ++++++
sha1_file.c | 3 +++
4 files changed, 22 insertions(+)
diff --git a/cache.h b/cache.h
index b419b9b7ce..503b618a1f 100644
--- a/cache.h
+++ b/cache.h
@@ -422,6 +422,7 @@ static inline enum object_type object_type(unsigned int mode)
#define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
#define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
#define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
+#define NO_EXTERNAL_ODB_ENVIRONMENT "GIT_NO_EXTERNAL_ODB"
#define GITATTRIBUTES_FILE ".gitattributes"
#define INFOATTRIBUTES_FILE "info/attributes"
#define ATTRIBUTE_MACRO_PREFIX "[attr]"
@@ -698,6 +699,14 @@ void reset_shared_repository(void);
extern int check_replace_refs;
extern char *git_replace_ref_base;
+/*
+ * Do external odbs need to be used this run? This variable is
+ * initialized to true unless $GIT_NO_EXTERNAL_ODB is set, but it
+ * maybe set to false by some commands that do not want external
+ * odbs to be active.
+ */
+extern int use_external_odb;
+
extern int fsync_object_files;
extern int core_preload_index;
extern int core_apply_sparse_checkout;
diff --git a/environment.c b/environment.c
index 0935ec696e..8aecdd0544 100644
--- a/environment.c
+++ b/environment.c
@@ -47,6 +47,7 @@ const char *excludes_file;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
int check_replace_refs = 1;
char *git_replace_ref_base;
+int use_external_odb = 1;
enum eol core_eol = EOL_UNSET;
enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
@@ -120,6 +121,7 @@ const char * const local_repo_env[] = {
INDEX_ENVIRONMENT,
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
+ NO_EXTERNAL_ODB_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
GIT_SUPER_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
@@ -185,6 +187,8 @@ static void setup_git_env(void)
replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
: "refs/replace/");
+ if (getenv(NO_EXTERNAL_ODB_ENVIRONMENT))
+ use_external_odb = 0;
namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
namespace_len = strlen(namespace);
shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
diff --git a/external-odb.c b/external-odb.c
index 6dd7b2548b..a980fbfbf2 100644
--- a/external-odb.c
+++ b/external-odb.c
@@ -63,6 +63,9 @@ int external_odb_has_object(const unsigned char *sha1)
{
struct odb_helper *o;
+ if (!use_external_odb)
+ return 0;
+
external_odb_init();
for (o = helpers; o; o = o->next)
@@ -133,6 +136,9 @@ int external_odb_write_object(const void *buf, unsigned long len,
{
struct odb_helper *o;
+ if (!use_external_odb)
+ return 1;
+
/* For now accept only blobs */
if (strcmp(type, "blob"))
return 1;
diff --git a/sha1_file.c b/sha1_file.c
index 3532c1c598..92f1244205 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -556,6 +556,9 @@ void prepare_external_alt_odb(void)
static int linked_external;
const char *path;
+ if (!use_external_odb)
+ return;
+
if (linked_external)
return;
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 10/16] Add t0410 to test external ODB transfer
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t0410-transfer-e-odb.sh | 136 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
create mode 100755 t/t0410-transfer-e-odb.sh
diff --git a/t/t0410-transfer-e-odb.sh b/t/t0410-transfer-e-odb.sh
new file mode 100755
index 0000000000..868b55db94
--- /dev/null
+++ b/t/t0410-transfer-e-odb.sh
@@ -0,0 +1,136 @@
+#!/bin/sh
+
+test_description='basic tests for transfering external ODBs'
+
+. ./test-lib.sh
+
+ORIG_SOURCE="$PWD/.git"
+export ORIG_SOURCE
+
+ALT_SOURCE1="$PWD/alt-repo1/.git"
+export ALT_SOURCE1
+write_script odb-helper1 <<\EOF
+die() {
+ printf >&2 "%s\n" "$@"
+ exit 1
+}
+GIT_DIR=$ALT_SOURCE1; export GIT_DIR
+case "$1" in
+have)
+ git cat-file --batch-check --batch-all-objects |
+ awk '{print $1 " " $3 " " $2}'
+ ;;
+get)
+ cat "$GIT_DIR"/objects/$(echo $2 | sed 's#..#&/#')
+ ;;
+put)
+ sha1="$2"
+ size="$3"
+ kind="$4"
+ writen=$(git hash-object -w -t "$kind" --stdin)
+ test "$writen" = "$sha1" || die "bad sha1 passed '$sha1' vs writen '$writen'"
+ ref_hash=$(echo "$sha1 $size $kind" | GIT_DIR=$ORIG_SOURCE GIT_NO_EXTERNAL_ODB=1 git hash-object -w -t blob --stdin) || exit
+ GIT_DIR=$ORIG_SOURCE git update-ref refs/odbs/magic/"$sha1" "$ref_hash"
+ ;;
+*)
+ die "unknown command '$1'"
+ ;;
+esac
+EOF
+HELPER1="\"$PWD\"/odb-helper1"
+
+OTHER_SOURCE="$PWD/.git"
+export OTHER_SOURCE
+
+ALT_SOURCE2="$PWD/alt-repo2/.git"
+export ALT_SOURCE2
+write_script odb-helper2 <<\EOF
+die() {
+ printf >&2 "%s\n" "$@"
+ exit 1
+}
+GIT_DIR=$ALT_SOURCE2; export GIT_DIR
+case "$1" in
+have)
+ GIT_DIR=$OTHER_SOURCE git for-each-ref --format='%(objectname)' refs/odbs/magic/ | GIT_DIR=$OTHER_SOURCE xargs git show
+ ;;
+get)
+ OBJ_FILE="$GIT_DIR"/objects/$(echo $2 | sed 's#..#&/#')
+ if ! test -f "$OBJ_FILE"
+ then
+ # "Download" the missing object by copying it from alt-repo1
+ OBJ_DIR=$(echo $2 | sed 's/\(..\).*/\1/')
+ OBJ_BASE=$(basename "$OBJ_FILE")
+ ALT_OBJ_DIR1="$ALT_SOURCE1/objects/$OBJ_DIR"
+ ALT_OBJ_DIR2="$ALT_SOURCE2/objects/$OBJ_DIR"
+ mkdir -p "$ALT_OBJ_DIR2" || die "Could not mkdir '$ALT_OBJ_DIR2'"
+ OBJ_SRC="$ALT_OBJ_DIR1/$OBJ_BASE"
+ cp "$OBJ_SRC" "$ALT_OBJ_DIR2" ||
+ die "Could not cp '$OBJ_SRC' into '$ALT_OBJ_DIR2'"
+ fi
+ cat "$OBJ_FILE" || die "Could not cat '$OBJ_FILE'"
+ ;;
+put)
+ sha1="$2"
+ size="$3"
+ kind="$4"
+ writen=$(git hash-object -w -t "$kind" --stdin)
+ test "$writen" = "$sha1" || die "bad sha1 passed '$sha1' vs writen '$writen'"
+ ref_hash=$(echo "$sha1 $size $kind" | GIT_DIR=$OTHER_SOURCE GIT_NO_EXTERNAL_ODB=1 git hash-object -w -t blob --stdin) || exit
+ GIT_DIR=$OTHER_SOURCE git update-ref refs/odbs/magic/"$sha1" "$ref_hash"
+ ;;
+*)
+ die "unknown command '$1'"
+ ;;
+esac
+EOF
+HELPER2="\"$PWD\"/odb-helper2"
+
+test_expect_success 'setup first alternate repo' '
+ git init alt-repo1 &&
+ test_commit zero &&
+ git config odb.magic.command "$HELPER1"
+'
+
+test_expect_success 'setup other repo and its alternate repo' '
+ git init other-repo &&
+ git init alt-repo2 &&
+ (cd other-repo &&
+ git remote add origin .. &&
+ git pull origin master &&
+ git checkout master &&
+ git log)
+'
+
+test_expect_success 'new blobs are put in first object store' '
+ test_commit one &&
+ hash1=$(git ls-tree HEAD | grep one.t | cut -f1 | cut -d\ -f3) &&
+ content=$(cd alt-repo1 && git show "$hash1") &&
+ test "$content" = "one" &&
+ test_commit two &&
+ hash2=$(git ls-tree HEAD | grep two.t | cut -f1 | cut -d\ -f3) &&
+ content=$(cd alt-repo1 && git show "$hash2") &&
+ test "$content" = "two"
+'
+
+test_expect_success 'other repo gets the blobs from object store' '
+ (cd other-repo &&
+ git fetch origin "refs/odbs/magic/*:refs/odbs/magic/*" &&
+ test_must_fail git cat-file blob "$hash1" &&
+ test_must_fail git cat-file blob "$hash2" &&
+ git config odb.magic.command "$HELPER2" &&
+ git cat-file blob "$hash1" &&
+ git cat-file blob "$hash2"
+ )
+'
+
+test_expect_success 'other repo gets everything else' '
+ (cd other-repo &&
+ git fetch origin &&
+ content=$(git show "$hash1") &&
+ test "$content" = "one" &&
+ content=$(git show "$hash2") &&
+ test "$content" = "two")
+'
+
+test_done
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 13/16] lib-httpd: add list.sh
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
This cgi script can list Git objects that have been uploaded as
files to an apache web server. This script can also retrieve
the content of each of these files.
This will help make apache work as an external object database.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/list.sh | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
create mode 100644 t/lib-httpd/list.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index d80b004549..f31ea261f5 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -133,6 +133,7 @@ prepare_httpd() {
install_script broken-smart-http.sh
install_script error.sh
install_script upload.sh
+ install_script list.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/list.sh b/t/lib-httpd/list.sh
new file mode 100644
index 0000000000..a54402558f
--- /dev/null
+++ b/t/lib-httpd/list.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+FILES_DIR="www/files"
+
+OLDIFS="$IFS"
+IFS='&'
+set -- $QUERY_STRING
+IFS="$OLDIFS"
+
+while test $# -gt 0
+do
+ key=${1%=*}
+ val=${1#*=}
+
+ case "$key" in
+ "sha1") sha1="$val" ;;
+ *) echo >&2 "unknown key '$key'" ;;
+ esac
+
+ shift
+done
+
+echo 'Status: 200 OK'
+echo
+
+if test -d "$FILES_DIR"
+then
+ if test -n "$sha1"
+ then
+ cat "$FILES_DIR/$sha1"-*
+ else
+ ls "$FILES_DIR" | tr '-' ' '
+ fi
+fi
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 11/16] lib-httpd: pass config file to start_httpd()
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
This makes it possible to start an apache web server with different
config files.
This will be used in a later patch to pass a config file that makes
apache store external objects.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/lib-httpd.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465a..2e659a8ee2 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -171,12 +171,14 @@ prepare_httpd() {
}
start_httpd() {
+ APACHE_CONF_FILE=${1-apache.conf}
+
prepare_httpd >&3 2>&4
trap 'code=$?; stop_httpd; (exit $code); die' EXIT
"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
- -f "$TEST_PATH/apache.conf" $HTTPD_PARA \
+ -f "$TEST_PATH/$APACHE_CONF_FILE" $HTTPD_PARA \
-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start \
>&3 2>&4
if test $? -ne 0
@@ -191,7 +193,7 @@ stop_httpd() {
trap 'die' EXIT
"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
- -f "$TEST_PATH/apache.conf" $HTTPD_PARA -k stop
+ -f "$TEST_PATH/$APACHE_CONF_FILE" $HTTPD_PARA -k stop
}
test_http_push_nonff () {
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 07/16] external-odb: accept only blobs for now
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
external-odb.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/external-odb.c b/external-odb.c
index bb70fe3298..6dd7b2548b 100644
--- a/external-odb.c
+++ b/external-odb.c
@@ -133,6 +133,10 @@ int external_odb_write_object(const void *buf, unsigned long len,
{
struct odb_helper *o;
+ /* For now accept only blobs */
+ if (strcmp(type, "blob"))
+ return 1;
+
external_odb_init();
for (o = helpers; o; o = o->next) {
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 03/16] t0400: use --batch-all-objects to get all objects
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t0400-external-odb.sh | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/t/t0400-external-odb.sh b/t/t0400-external-odb.sh
index 2b016173a0..fe85413725 100755
--- a/t/t0400-external-odb.sh
+++ b/t/t0400-external-odb.sh
@@ -10,9 +10,7 @@ write_script odb-helper <<\EOF
GIT_DIR=$ALT_SOURCE; export GIT_DIR
case "$1" in
have)
- git rev-list --all --objects |
- cut -d' ' -f1 |
- git cat-file --batch-check |
+ git cat-file --batch-check --batch-all-objects |
awk '{print $1 " " $3 " " $2}'
;;
get)
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* Re: "git add -p ." raises an unexpected "warning: empty strings as pathspecs will be made invalid in upcoming releases. please use . instead if you meant to match all paths"
From: Junio C Hamano @ 2016-11-30 22:04 UTC (permalink / raw)
To: git, Nguyễn Thái Ngọc Duy; +Cc: Kevin Daudt, Peter Urda
In-Reply-To: <20161130211100.GA18680@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> On Wed, Nov 30, 2016 at 12:31:49PM -0800, Peter Urda wrote:
>> After upgrading to version 2.11.0 I am getting a warning about empty
>> strings as pathspecs while using 'patch'
>>
>> - Ran 'git add -p .' from the root of my git repository.
>>
>> - I was able to normally stage my changes, but was presented with a
>> "warning: empty strings as pathspecs will be made invalid in upcoming
>> releases. please use . instead if you meant to match all paths"
>> message.
>>
>> - I expected no warning message since I included a "." with my original command.
>>
>> I believe that I should not be seeing this warning message as I
>> included the requested "." pathspec.
Yes, this seems to be caused by pathspec.c::prefix_pathspec()
overwriting the original pathspec "." into "". The callchain
looks like this:
builtin/add.c::interactive_add()
-> parse_pathspec()
passes argv[] that has "." to the caller,
receives pathspec whose pathspec->items[].original
is supposed to point at the unmolested original,
but prefix_pathspec() munges "." into ""
-> run_add_interactive()
which runs "git add--interactive" with
pathspec->items[].original as pathspecs
Perhaps this would work it around, but there should be a better way
to fix it (like, making sure that what we call "original" indeed
stays "original").
builtin/add.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index e8fb80b36e..137097192d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -167,9 +167,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
if (revision)
argv_array_push(&argv, revision);
argv_array_push(&argv, "--");
- for (i = 0; i < pathspec->nr; i++)
+ for (i = 0; i < pathspec->nr; i++) {
/* pass original pathspec, to be re-parsed */
+ if (!*pathspec->items[i].original) {
+ /*
+ * work around a misfeature in parse_pathspecs()
+ * that munges "." into "".
+ */
+ argv_array_push(&argv, ".");
+ continue;
+ }
argv_array_push(&argv, pathspec->items[i].original);
+ }
status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
argv_array_clear(&argv);
@@ -180,7 +189,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
{
struct pathspec pathspec;
- parse_pathspec(&pathspec, 0,
+ parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH |
PATHSPEC_PREFIX_ORIGIN,
^ permalink raw reply related
* [RFC/PATCH v3 02/16] external odb foreach
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
From: Jeff King <peff@peff.net>
---
external-odb.c | 14 ++++++++++++++
external-odb.h | 6 ++++++
odb-helper.c | 15 +++++++++++++++
odb-helper.h | 4 ++++
4 files changed, 39 insertions(+)
diff --git a/external-odb.c b/external-odb.c
index 1ccfa99a01..42978a3298 100644
--- a/external-odb.c
+++ b/external-odb.c
@@ -113,3 +113,17 @@ int external_odb_fetch_object(const unsigned char *sha1)
return -1;
}
+
+int external_odb_for_each_object(each_external_object_fn fn, void *data)
+{
+ struct odb_helper *o;
+
+ external_odb_init();
+
+ for (o = helpers; o; o = o->next) {
+ int r = odb_helper_for_each_object(o, fn, data);
+ if (r)
+ return r;
+ }
+ return 0;
+}
diff --git a/external-odb.h b/external-odb.h
index 2397477684..cea8570a49 100644
--- a/external-odb.h
+++ b/external-odb.h
@@ -5,4 +5,10 @@ const char *external_odb_root(void);
int external_odb_has_object(const unsigned char *sha1);
int external_odb_fetch_object(const unsigned char *sha1);
+typedef int (*each_external_object_fn)(const unsigned char *sha1,
+ enum object_type type,
+ unsigned long size,
+ void *data);
+int external_odb_for_each_object(each_external_object_fn, void *);
+
#endif /* EXTERNAL_ODB_H */
diff --git a/odb-helper.c b/odb-helper.c
index 244bc86792..2db59caa53 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -237,3 +237,18 @@ int odb_helper_fetch_object(struct odb_helper *o, const unsigned char *sha1,
return 0;
}
+
+int odb_helper_for_each_object(struct odb_helper *o,
+ each_external_object_fn fn,
+ void *data)
+{
+ int i;
+ for (i = 0; i < o->have_nr; i++) {
+ struct odb_helper_object *obj = &o->have[i];
+ int r = fn(obj->sha1, obj->type, obj->size, data);
+ if (r)
+ return r;
+ }
+
+ return 0;
+}
diff --git a/odb-helper.h b/odb-helper.h
index 0f704f9452..8c3916d215 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -1,6 +1,8 @@
#ifndef ODB_HELPER_H
#define ODB_HELPER_H
+#include "external-odb.h"
+
struct odb_helper {
const char *name;
const char *cmd;
@@ -21,5 +23,7 @@ struct odb_helper *odb_helper_new(const char *name, int namelen);
int odb_helper_has_object(struct odb_helper *o, const unsigned char *sha1);
int odb_helper_fetch_object(struct odb_helper *o, const unsigned char *sha1,
int fd);
+int odb_helper_for_each_object(struct odb_helper *o,
+ each_external_object_fn, void *);
#endif /* ODB_HELPER_H */
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 12/16] lib-httpd: add upload.sh
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
This cgi will be used to upload objects to, or to delete
objects from, an apache web server.
This way the apache server can work as an external object
database.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/upload.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
create mode 100644 t/lib-httpd/upload.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2e659a8ee2..d80b004549 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script upload.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/upload.sh b/t/lib-httpd/upload.sh
new file mode 100644
index 0000000000..172be0f73f
--- /dev/null
+++ b/t/lib-httpd/upload.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+# In part from http://codereview.stackexchange.com/questions/79549/bash-cgi-upload-file
+
+FILES_DIR="www/files"
+
+OLDIFS="$IFS"
+IFS='&'
+set -- $QUERY_STRING
+IFS="$OLDIFS"
+
+while test $# -gt 0
+do
+ key=${1%=*}
+ val=${1#*=}
+
+ case "$key" in
+ "sha1") sha1="$val" ;;
+ "type") type="$val" ;;
+ "size") size="$val" ;;
+ "delete") delete=1 ;;
+ *) echo >&2 "unknown key '$key'" ;;
+ esac
+
+ shift
+done
+
+case "$REQUEST_METHOD" in
+ POST)
+ if test "$delete" = "1"
+ then
+ rm -f "$FILES_DIR/$sha1-$size-$type"
+ else
+ mkdir -p "$FILES_DIR"
+ cat >"$FILES_DIR/$sha1-$size-$type"
+ fi
+
+ echo 'Status: 204 No Content'
+ echo
+ ;;
+
+ *)
+ echo 'Status: 405 Method Not Allowed'
+ echo
+esac
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* Re: [PATCHv2 4/4] submodule: add embed-git-dir function
From: Junio C Hamano @ 2016-11-30 22:18 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, Brandon Williams, Git Mailing List, Jonathan Nieder,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79kZSAJauwBwrxf+QAhQgyu4ACn+8LrwjpFGVaUQfSzHEAg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Nov 30, 2016 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> git relocate-git-dir (--into-workingtree|--into-gitdir) \
>>
>> I am not sure if you meant this as a submodule-specific subcommand
>> or more general helper. "into-workingtree" suggests to me that it
>> is submodule specific, so I'll base my response on that assumption.
>>
>> Would there ever be a situation where you already have submodule
>> repositories in the right place (according to the more modern
>> practice, to keep them in .git/modules/ of superproject) and want to
>> move them to embed them in worktrees of submodules? I do not think
>> of any.
>
> "Hi, I made a mistake by using submodules. I don't want to use
> them any more, I rather want to:
> A) make it a separate git repo again and I'll keep them in sync myself
> B) ... "
OK, I can buy that. Thanks.
^ permalink raw reply
* Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
From: Junio C Hamano @ 2016-11-30 22:36 UTC (permalink / raw)
To: Christian Couder
Cc: git, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey, Lars Schneider,
Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> For now there should be one odb ref per blob. Each ref name should be
> refs/odbs/<odbname>/<sha1> where <sha1> is the sha1 of the blob stored
> in the external odb named <odbname>.
>
> These odb refs should all point to a blob that should be stored in the
> Git repository and contain information about the blob stored in the
> external odb. This information can be specific to the external odb.
> The repos can then share this information using commands like:
>
> `git fetch origin "refs/odbs/<odbname>/*:refs/odbs/<odbname>/*"`
Unless this is designed to serve only a handful of blobs, I cannot
see how this design would scale successfully. I notice you wrote
"For now" at the beginning, but what is the envisioned way this will
evolve in the future?
^ permalink raw reply
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Ramsay Jones @ 2016-11-30 22:37 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Johannes Schindelin, GIT Mailing-list
In-Reply-To: <20161130212510.ihcmvig7jq44p3nx@sigill.intra.peff.net>
On 30/11/16 21:25, Jeff King wrote:
> On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:
>
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>
>>> [I have fixed my config.mak file now, so I don't see the warning
>>> anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or
>>> not, is a separate matter.]
>>
>> I suspect that 658df95a4a ("add DEVELOPER makefile knob to check for
>> acknowledged warnings", 2016-02-25) took it from me (namely, Make
>> script in my 'todo' branch). In turn, I added it to my set of flags
>> in order to squelch this exact warning, so...
>
> For anybody interested in the history, we started using this when
> status_printf() got the format attribute. Relevant patch and discussion:
>
> http://public-inbox.org/git/20130710002328.GC19423@sigill.intra.peff.net/T/#u
Ah, thank you! I've been trying to remember this for the last hour or so ...
(I misremembered something about gettext, and went looking in the wrong
direction ;-) ).
> We went with disabling the warning because it really is wrong. It makes
> an assumption that calling a format function with an empty string
> doesn't do anything, but that's only true of the stock printf functions.
> Our custom functions _do_ have a side effect in this case.
Yes, I agree, gcc is making an unwarranted assumption.
> The other options are to have a special function for "print a warning
> (or status) line with no content". Or to teach those functions to handle
> newlines specially. We've often discussed that you should be able to do:
>
> warning("foo\nbar");
>
> and have it print:
>
> warning: foo
> warning: bar
>
> That's useful in itself, and would probably make cases like this easier
> to handle, too. But it's a pretty big change. Another option would be to
> just teach formatting functions to handle a single "\n" as a synonym for
> the empty string (or even detect trailing newlines and avoid appending
> our own in that case). That would mean you could do:
>
> warning("\n");
>
> to print a blank line. That's arguably more obvious about the intent to
> a reader (I say arguably because the new behavior _is_ subtle if you
> happen to know that warning() usually appends a newline).
Yes, I remember the discussion now and agree that this could
cause problems.
> Anyway. Those are all options, but I don't think there is any problem
> with sticking with warning("") for now. It is not the first part of the
> code that tickles the format-zero-length warning.
Hmm, well I have been building git for some time with the warning
enabled without problem.
[I can see a few examples of 'status_printf_ln(s, c, "%s", "");' in
git-grep output, so ...]
I am not suggesting any change here, and was just curious why it
seemed to be unnecessary now (I knew it was necessary at one time).
ATB,
Ramsay Jones
^ permalink raw reply
* Re: "git add -p ." raises an unexpected "warning: empty strings as pathspecs will be made invalid in upcoming releases. please use . instead if you meant to match all paths"
From: Junio C Hamano @ 2016-11-30 22:40 UTC (permalink / raw)
To: git; +Cc: Emily Xie
In-Reply-To: <xmqq7f7kd3pj.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> forgot to Cc: the author of the
most relevant change to the issue, d426430e6e ("pathspec: warn on
empty strings as pathspec", 2016-06-22).
> Kevin Daudt <me@ikke.info> writes:
>
>> On Wed, Nov 30, 2016 at 12:31:49PM -0800, Peter Urda wrote:
>>> After upgrading to version 2.11.0 I am getting a warning about empty
>>> strings as pathspecs while using 'patch'
>>>
>>> - Ran 'git add -p .' from the root of my git repository.
>>>
>>> - I was able to normally stage my changes, but was presented with a
>>> "warning: empty strings as pathspecs will be made invalid in upcoming
>>> releases. please use . instead if you meant to match all paths"
>>> message.
>>>
>>> - I expected no warning message since I included a "." with my original command.
>>>
>>> I believe that I should not be seeing this warning message as I
>>> included the requested "." pathspec.
>
> Yes, this seems to be caused by pathspec.c::prefix_pathspec()
> overwriting the original pathspec "." into "". The callchain
> looks like this:
>
> builtin/add.c::interactive_add()
> -> parse_pathspec()
> passes argv[] that has "." to the caller,
> receives pathspec whose pathspec->items[].original
> is supposed to point at the unmolested original,
> but prefix_pathspec() munges "." into ""
> -> run_add_interactive()
> which runs "git add--interactive" with
> pathspec->items[].original as pathspecs
>
>
> Perhaps this would work it around, but there should be a better way
> to fix it (like, making sure that what we call "original" indeed
> stays "original").
>
> builtin/add.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index e8fb80b36e..137097192d 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -167,9 +167,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> if (revision)
> argv_array_push(&argv, revision);
> argv_array_push(&argv, "--");
> - for (i = 0; i < pathspec->nr; i++)
> + for (i = 0; i < pathspec->nr; i++) {
> /* pass original pathspec, to be re-parsed */
> + if (!*pathspec->items[i].original) {
> + /*
> + * work around a misfeature in parse_pathspecs()
> + * that munges "." into "".
> + */
> + argv_array_push(&argv, ".");
> + continue;
> + }
> argv_array_push(&argv, pathspec->items[i].original);
> + }
>
> status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> argv_array_clear(&argv);
> @@ -180,7 +189,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
> {
> struct pathspec pathspec;
>
> - parse_pathspec(&pathspec, 0,
> + parse_pathspec(&pathspec, 0,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH |
> PATHSPEC_PREFIX_ORIGIN,
^ permalink raw reply
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2016-11-30 23:18 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <3e6a6685-19ec-4536-4a5f-3a56e30fb530@ramsayjones.plus.com>
On Wed, Nov 30, 2016 at 10:37:40PM +0000, Ramsay Jones wrote:
> > Anyway. Those are all options, but I don't think there is any problem
> > with sticking with warning("") for now. It is not the first part of the
> > code that tickles the format-zero-length warning.
>
> Hmm, well I have been building git for some time with the warning
> enabled without problem.
>
> [I can see a few examples of 'status_printf_ln(s, c, "%s", "");' in
> git-grep output, so ...]
>
> I am not suggesting any change here, and was just curious why it
> seemed to be unnecessary now (I knew it was necessary at one time).
I forgot, we ended up reversing course later and silencing them:
http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
By the rationale of that conversation, we should be doing:
warning("%s", "");
here.
-Peff
^ permalink raw reply
* [RFC/PATCH v3 04/16] t0400: add 'put' command to odb-helper script
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t0400-external-odb.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/t/t0400-external-odb.sh b/t/t0400-external-odb.sh
index fe85413725..0f1bb97f38 100755
--- a/t/t0400-external-odb.sh
+++ b/t/t0400-external-odb.sh
@@ -7,6 +7,10 @@ test_description='basic tests for external object databases'
ALT_SOURCE="$PWD/alt-repo/.git"
export ALT_SOURCE
write_script odb-helper <<\EOF
+die() {
+ printf >&2 "%s\n" "$@"
+ exit 1
+}
GIT_DIR=$ALT_SOURCE; export GIT_DIR
case "$1" in
have)
@@ -16,6 +20,16 @@ have)
get)
cat "$GIT_DIR"/objects/$(echo $2 | sed 's#..#&/#')
;;
+put)
+ sha1="$2"
+ size="$3"
+ kind="$4"
+ writen=$(git hash-object -w -t "$kind" --stdin)
+ test "$writen" = "$sha1" || die "bad sha1 passed '$sha1' vs writen '$writen'"
+ ;;
+*)
+ die "unknown command '$1'"
+ ;;
esac
EOF
HELPER="\"$PWD\"/odb-helper"
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* [RFC/PATCH v3 08/16] t0400: add test for external odb write support
From: Christian Couder @ 2016-11-30 21:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-1-chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t0400-external-odb.sh | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/t/t0400-external-odb.sh b/t/t0400-external-odb.sh
index 6c6da5cf4f..3c868cad4c 100755
--- a/t/t0400-external-odb.sh
+++ b/t/t0400-external-odb.sh
@@ -66,4 +66,12 @@ test_expect_success 'helper can add objects to alt repo' '
test "$size" -eq "$alt_size"
'
+test_expect_success 'commit adds objects to alt repo' '
+ test_config odb.magic.command "$HELPER" &&
+ test_commit three &&
+ hash3=$(git ls-tree HEAD | grep three.t | cut -f1 | cut -d\ -f3) &&
+ content=$(cd alt-repo && git show "$hash3") &&
+ test "$content" = "three"
+'
+
test_done
--
2.11.0.rc2.37.geb49ca6
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Brandon Williams @ 2016-11-30 23:28 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20161130195427.GA166433@google.com>
On 11/30, Brandon Williams wrote:
> On 11/29, Jeff King wrote:
> > On Tue, Nov 29, 2016 at 01:37:59AM -0500, Jeff King wrote:
> >
> > > 2. Grep threads doing more complicated stuff that needs to take a
> > > lock. You might try building with -fsanitize=thread to see if it
> > > turns up anything.
> >
> > I tried this and it didn't find anything useful. It complains about
> > multiple threads calling want_color() at the same time, which you can
> > silence with something like:
> >
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 2c727ef49..d48846f40 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -207,6 +207,12 @@ static void start_threads(struct grep_opt *opt)
> > {
> > int i;
> >
> > + /*
> > + * trigger want_color() for its side effect of caching the result;
> > + * otherwise the threads will fight over setting the cache
> > + */
> > + want_color(GIT_COLOR_AUTO);
> > +
> > pthread_mutex_init(&grep_mutex, NULL);
> > pthread_mutex_init(&grep_read_mutex, NULL);
> > pthread_mutex_init(&grep_attr_mutex, NULL);
> >
> > But the problem persists even with that patch, so it is something else.
> > It may still be a threading problem; -fsanitize=thread isn't perfect. I
> > also couldn't get the stress-test to fail when compiled with it. But
> > that may simply mean that the timing of the resulting binary is changed
> > enough not to trigger the issue.
> >
> > -Peff
>
> With you're stress script I'm able to see the failures. The interesting
> thing is that the entry missing is always from the non-submodule file.
So I couldn't find a race condition in the code. I tracked the problem
to grep_source_load_file which attempts to run lstat on the file so that
it can read it into a buffer. The lstat call fails with ENOENT (which
conveniently is skipped by the if statement which calls error_errno). So
for some reason the file cannot be found and read into memory resulting
in nothing being grep'ed for that particular file (since the buffer is
NULL).
Maybe there is something wrong with the way I wrote the tests
themselves? So I could try rewriting them. Any other ideas?
--
Brandon Williams
^ permalink raw reply
* Re: [RFC/PATCH v3 01/16] Add initial external odb support
From: Junio C Hamano @ 2016-11-30 23:30 UTC (permalink / raw)
To: Christian Couder
Cc: git, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey, Lars Schneider,
Eric Wong, Christian Couder
In-Reply-To: <20161130210420.15982-2-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> From: Jeff King <peff@peff.net>
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
By the time the series loses RFC prefix, we'd need to see the above
three lines straightened out a bit more, e.g. a real message and a
more proper sign-off sequence.
> +static struct odb_helper *find_or_create_helper(const char *name, int len)
> +{
> + struct odb_helper *o;
> +
> + for (o = helpers; o; o = o->next)
> + if (!strncmp(o->name, name, len) && !o->name[len])
> + return o;
> +
> + o = odb_helper_new(name, len);
> + *helpers_tail = o;
> + helpers_tail = &o->next;
> +
> + return o;
> +}
> +
> +static int external_odb_config(const char *var, const char *value, void *data)
> +{
> + struct odb_helper *o;
> + const char *key, *dot;
> +
> + if (!skip_prefix(var, "odb.", &key))
> + return 0;
> + dot = strrchr(key, '.');
> + if (!dot)
> + return 0;
Is this something Peff wrote long time ago? I find it surprising
that he would write this without using parse_config_key().
> +struct odb_helper_cmd {
> + struct argv_array argv;
> + struct child_process child;
> +};
> +
> +static void prepare_helper_command(struct argv_array *argv, const char *cmd,
> + const char *fmt, va_list ap)
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, cmd);
> + strbuf_addch(&buf, ' ');
> + strbuf_vaddf(&buf, fmt, ap);
> +
> + argv_array_push(argv, buf.buf);
> + strbuf_release(&buf);
This concatenates the cmdname (like "get") and its parameters into a
single string (e.g. "get 454cb6bd52a4de614a3633e4f547af03d5c3b640")
and then places the resulting string as the sole element in argv[]
array, which I find somewhat unusual. It at least deserves a
comment in front of the function that the callers are responsible to
ensure that the result of vaddf(fmt, ap) is properly shell-quoted.
Otherwise it is inviting quoting errors in the future (even if there
is no such errors in the current code and command set, i.e. a 40-hex
object name that "get" subcommand takes happens to require no
special shell-quoting consideration).
> +int odb_helper_fetch_object(struct odb_helper *o, const unsigned char *sha1,
> + int fd)
> +{
> + struct odb_helper_object *obj;
> + struct odb_helper_cmd cmd;
> + unsigned long total_got;
> + git_zstream stream;
> + int zret = Z_STREAM_END;
> + git_SHA_CTX hash;
> + unsigned char real_sha1[20];
> +
> + obj = odb_helper_lookup(o, sha1);
> + if (!obj)
> + return -1;
> +
> + if (odb_helper_start(o, &cmd, "get %s", sha1_to_hex(sha1)) < 0)
> + return -1;
> +
> + memset(&stream, 0, sizeof(stream));
> + git_inflate_init(&stream);
> + git_SHA1_Init(&hash);
> + total_got = 0;
> +
> + for (;;) {
> + unsigned char buf[4096];
> + int r;
> +
> + r = xread(cmd.child.out, buf, sizeof(buf));
> + if (r < 0) {
> + error("unable to read from odb helper '%s': %s",
> + o->name, strerror(errno));
> + close(cmd.child.out);
> + odb_helper_finish(o, &cmd);
> + git_inflate_end(&stream);
> + return -1;
> + }
> + if (r == 0)
> + break;
> +
> + write_or_die(fd, buf, r);
> +
> + stream.next_in = buf;
> + stream.avail_in = r;
> + do {
> + unsigned char inflated[4096];
> + unsigned long got;
> +
> + stream.next_out = inflated;
> + stream.avail_out = sizeof(inflated);
> + zret = git_inflate(&stream, Z_SYNC_FLUSH);
> + got = sizeof(inflated) - stream.avail_out;
> +
> + git_SHA1_Update(&hash, inflated, got);
> + /* skip header when counting size */
> + if (!total_got) {
> + const unsigned char *p = memchr(inflated, '\0', got);
Does this assume that a single xread() that can result in a
short-read would not return in the middle of "header", and if so, is
that a safe assumption to make?
> + if (p)
> + got -= p - inflated + 1;
> + else
> + got = 0;
> + }
> + total_got += got;
> + } while (stream.avail_in && zret == Z_OK);
> + }
> +
> + close(cmd.child.out);
> + git_inflate_end(&stream);
> + git_SHA1_Final(real_sha1, &hash);
> + if (odb_helper_finish(o, &cmd))
> + return -1;
> + if (zret != Z_STREAM_END) {
> + warning("bad zlib data from odb helper '%s' for %s",
> + o->name, sha1_to_hex(sha1));
> + return -1;
> + }
> + if (total_got != obj->size) {
> + warning("size mismatch from odb helper '%s' for %s (%lu != %lu)",
> + o->name, sha1_to_hex(sha1), total_got, obj->size);
> + return -1;
> + }
> + if (hashcmp(real_sha1, sha1)) {
> + warning("sha1 mismatch from odb helper '%s' for %s (got %s)",
> + o->name, sha1_to_hex(sha1), sha1_to_hex(real_sha1));
> + return -1;
> + }
> +
> + return 0;
> +}
OK. So we call the external helper with "get" command, expecting
that the helper returns the data as a zlib deflated stream, and
validate that the data matches the expected hash and the expected
size, while also saving the contents of the deflated stream to fd.
Presumably the caller opened a fd to write into a loose object? I
do not see this code actually validating that the loose object
header, i.e. "<type> <len>\0", matches what the caller wanted to see
in obj->size and obj->type. Shouldn't there be a check for that,
too?
I am tempted to debate myself if it is a sensible design to require
"get" to return a loose object representation, but cannot decide
without seeing the remainder of the series. An obvious alternative
is to define the "get" request to interface with us via the raw
contents (not even deflated) and leave the deflating to us, i.e. Git
sitting on the receiving end, which would allow us to choose to
store it differently (e.g. we may want to try streaming it into its
own pack using the streaming.h API, for example).
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Jeff King @ 2016-11-30 23:32 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git
In-Reply-To: <20161130232823.GA192901@google.com>
On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:
> So I couldn't find a race condition in the code. I tracked the problem
> to grep_source_load_file which attempts to run lstat on the file so that
> it can read it into a buffer. The lstat call fails with ENOENT (which
> conveniently is skipped by the if statement which calls error_errno). So
> for some reason the file cannot be found and read into memory resulting
> in nothing being grep'ed for that particular file (since the buffer is
> NULL).
That's definitely weird. Is it possible that any of the underlying calls
from another thread are using chdir()? I think realpath() make do that
behind the scenes, and there may be others.
A full strace from a failing case would be interesting reading. In
theory we should be able to get that by running the stress script for
long enough. :)
-Peff
^ permalink raw reply
* Re: [RFC/PATCH v3 01/16] Add initial external odb support
From: Jeff King @ 2016-11-30 23:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, git, Nguyen Thai Ngoc Duy, Mike Hommey,
Lars Schneider, Eric Wong, Christian Couder
In-Reply-To: <xmqqmvggbl6m.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 30, 2016 at 03:30:09PM -0800, Junio C Hamano wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
> > From: Jeff King <peff@peff.net>
> >
> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> > ---
>
> By the time the series loses RFC prefix, we'd need to see the above
> three lines straightened out a bit more, e.g. a real message and a
> more proper sign-off sequence.
Actually, I would assume that this patch would go away altogether and
get folded into commits that get us closer to the end state. But I
haven't read the series carefully yet, so maybe it really does work
better with this as a base.
I am perfectly OK dropping my commit count by one and getting "based on
a patch by Peff" in a cover letter or elsewhere.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Jeff King @ 2016-11-30 23:40 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git
In-Reply-To: <20161130233204.ihbrjwwu3yiv4ugq@sigill.intra.peff.net>
On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:
> On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:
>
> > So I couldn't find a race condition in the code. I tracked the problem
> > to grep_source_load_file which attempts to run lstat on the file so that
> > it can read it into a buffer. The lstat call fails with ENOENT (which
> > conveniently is skipped by the if statement which calls error_errno). So
> > for some reason the file cannot be found and read into memory resulting
> > in nothing being grep'ed for that particular file (since the buffer is
> > NULL).
>
> That's definitely weird. Is it possible that any of the underlying calls
> from another thread are using chdir()? I think realpath() make do that
> behind the scenes, and there may be others.
>
> A full strace from a failing case would be interesting reading. In
> theory we should be able to get that by running the stress script for
> long enough. :)
Actually, it failed pretty much immediately. I guess the extra stracing
changes the timing to make the problem _more_ likely.
And indeed, I see:
20867 lstat("fi:le", <unfinished ...>
20813 <... read resumed> "", 232) = 0
20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL <unfinished ...>
20813 close(7 <unfinished ...>
20870 <... futex resumed> ) = 0
20869 lstat(".gitmodules", <unfinished ...>
20813 <... close resumed> ) = 0
20865 set_robust_list(0x7f1df92579e0, 24 <unfinished ...>
20813 lstat("su:b/../.git/modules/su:b/commondir", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)
20865 <... set_robust_list resumed> ) = 0
20868 set_robust_list(0x7f1df7a549e0, 24 <unfinished ...>
20813 access("su:b/../.git/modules/su:b/objects", X_OK) = 0
20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
20813 stat("su:b/../.git/modules/su:b", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0
20813 getcwd("/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent", 129) = 80
20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0
20813 chdir("su:b/../.git/modules/su:b") = 0
20869 open(".gitmodules", O_RDONLY <unfinished ...>
20813 getcwd( <unfinished ...>
20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)
where 20813 and 20867 are two threads of the main process. One is doing
the lstat and the other calls chdir at the same moment.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Brandon Williams @ 2016-11-30 23:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20161130234056.iltitkszvccbjivp@sigill.intra.peff.net>
On 11/30, Jeff King wrote:
> On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:
>
> > On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:
> >
> > > So I couldn't find a race condition in the code. I tracked the problem
> > > to grep_source_load_file which attempts to run lstat on the file so that
> > > it can read it into a buffer. The lstat call fails with ENOENT (which
> > > conveniently is skipped by the if statement which calls error_errno). So
> > > for some reason the file cannot be found and read into memory resulting
> > > in nothing being grep'ed for that particular file (since the buffer is
> > > NULL).
> >
> > That's definitely weird. Is it possible that any of the underlying calls
> > from another thread are using chdir()? I think realpath() make do that
> > behind the scenes, and there may be others.
> >
> > A full strace from a failing case would be interesting reading. In
> > theory we should be able to get that by running the stress script for
> > long enough. :)
>
> Actually, it failed pretty much immediately. I guess the extra stracing
> changes the timing to make the problem _more_ likely.
>
> And indeed, I see:
>
> 20867 lstat("fi:le", <unfinished ...>
> 20813 <... read resumed> "", 232) = 0
> 20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL <unfinished ...>
> 20813 close(7 <unfinished ...>
> 20870 <... futex resumed> ) = 0
> 20869 lstat(".gitmodules", <unfinished ...>
> 20813 <... close resumed> ) = 0
> 20865 set_robust_list(0x7f1df92579e0, 24 <unfinished ...>
> 20813 lstat("su:b/../.git/modules/su:b/commondir", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)
> 20865 <... set_robust_list resumed> ) = 0
> 20868 set_robust_list(0x7f1df7a549e0, 24 <unfinished ...>
> 20813 access("su:b/../.git/modules/su:b/objects", X_OK) = 0
> 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
> 20813 stat("su:b/../.git/modules/su:b", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0
> 20813 getcwd("/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent", 129) = 80
> 20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0
> 20813 chdir("su:b/../.git/modules/su:b") = 0
> 20869 open(".gitmodules", O_RDONLY <unfinished ...>
> 20813 getcwd( <unfinished ...>
> 20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)
>
> where 20813 and 20867 are two threads of the main process. One is doing
> the lstat and the other calls chdir at the same moment.
>
> -Peff
Yeah so it looks like the start_command function calls chdir. Which
means any uses of the run-command interface are not thread safe....
For now the work around could be to just pass "-C <dir>" to the child
process instead of relying on run-command to chdir.
--
Brandon Williams
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Stefan Beller @ 2016-11-30 23:43 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Williams, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20161130234056.iltitkszvccbjivp@sigill.intra.peff.net>
On Wed, Nov 30, 2016 at 3:40 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:
>
>> On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:
>>
>> > So I couldn't find a race condition in the code. I tracked the problem
>> > to grep_source_load_file which attempts to run lstat on the file so that
>> > it can read it into a buffer. The lstat call fails with ENOENT (which
>> > conveniently is skipped by the if statement which calls error_errno). So
>> > for some reason the file cannot be found and read into memory resulting
>> > in nothing being grep'ed for that particular file (since the buffer is
>> > NULL).
>>
>> That's definitely weird. Is it possible that any of the underlying calls
>> from another thread are using chdir()? I think realpath() make do that
>> behind the scenes, and there may be others.
>>
>> A full strace from a failing case would be interesting reading. In
>> theory we should be able to get that by running the stress script for
>> long enough. :)
>
> Actually, it failed pretty much immediately. I guess the extra stracing
> changes the timing to make the problem _more_ likely.
>
> And indeed, I see:
>
> 20867 lstat("fi:le", <unfinished ...>
> 20813 <... read resumed> "", 232) = 0
> 20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL <unfinished ...>
> 20813 close(7 <unfinished ...>
> 20870 <... futex resumed> ) = 0
> 20869 lstat(".gitmodules", <unfinished ...>
> 20813 <... close resumed> ) = 0
> 20865 set_robust_list(0x7f1df92579e0, 24 <unfinished ...>
> 20813 lstat("su:b/../.git/modules/su:b/commondir", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)
> 20865 <... set_robust_list resumed> ) = 0
> 20868 set_robust_list(0x7f1df7a549e0, 24 <unfinished ...>
> 20813 access("su:b/../.git/modules/su:b/objects", X_OK) = 0
> 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
> 20813 stat("su:b/../.git/modules/su:b", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0
> 20813 getcwd("/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent", 129) = 80
> 20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0
> 20813 chdir("su:b/../.git/modules/su:b") = 0
> 20869 open(".gitmodules", O_RDONLY <unfinished ...>
> 20813 getcwd( <unfinished ...>
> 20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)
>
> where 20813 and 20867 are two threads of the main process. One is doing
> the lstat and the other calls chdir at the same moment.
>
Lessons learned here:
The run-command API is not thread safe when used with setting the directory.
If you need to run a thing in a threaded environment run
git -C <dir> ... such that the child chdirs.
Are there any other threaded environments that run things with .dir set?
^ 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