* Re: [PATCH] Revert "http: don't always prompt for password"
From: Junio C Hamano @ 2011-12-13 21:09 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Naewe, Sebastian Schuberth, Eric, git, msysgit
In-Reply-To: <20111213202508.GA12187@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Doing (3) is obviously the easiest thing. And given the complexity of
> the other two solutions, I think it makes sense to revert 986bbc08
> (i.e., apply this patch), ship a working v1.7.8.1, and then look at
> doing one of the other two solutions for v1.7.9.
Or just let the "dumb HTTP" die.
I thought push over DAV has long been dead; is anybody using it for real?
^ permalink raw reply
* [PATCH] Revert "http: don't always prompt for password"
From: Jeff King @ 2011-12-13 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Naewe, Sebastian Schuberth, Eric, git, msysgit
In-Reply-To: <20111213201704.GA12072@sigill.intra.peff.net>
This reverts commit 986bbc0842334f0e07731fa37f2a55d2930a5b8c.
The rationale for that commit relied on the fact that asking
for the password up-front was merely an optimization,
because git will notice an HTTP 401 and prompt for the
password. However, that is only true for smart-http, and for
dumb fetching. Dumb push over DAV does not have this
feature; as a result, authenticated push-over-DAV does not
work at all, as it never prompts the user for a password.
Signed-off-by: Jeff King <peff@peff.net>
---
We need to deal with this regression for v1.7.8.1, I think.
There are basically three options for fixing it:
1. Teach http-push the same retry-after-401 trick that the rest of the
http code knows.
2. Refactor the retry-after-401 logic from http.c into a common
function that http-push can build on top of.
3. Revert 986bbc08 and leave it alone; it only hurts .netrc users,
there's a reasonable workaround (don't put the user in the URL) and
hopefully those people will convert to using better storage via
credential helper once it is available.
I looked at doing (1), but my first attempt[1] didn't quite work. So
it's not a huge amount of code, but it's annoyingly non-trivial. And as
a long-term solution, it's just making hack-y code hackier.
Doing (2) would be the best solution, but it's going to require some
pretty major surgery to http.c and http-push.c. I'll take a look, but if
it gets too complex, it may simply not be worth it (now that smart-http
is available, I would hope that push-over-DAV is slowly going away).
Doing (3) is obviously the easiest thing. And given the complexity of
the other two solutions, I think it makes sense to revert 986bbc08
(i.e., apply this patch), ship a working v1.7.8.1, and then look at
doing one of the other two solutions for v1.7.9.
[1] http://article.gmane.org/gmane.comp.version-control.msysgit/14153
http.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 008ad72..a4bc770 100644
--- a/http.c
+++ b/http.c
@@ -279,6 +279,8 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
#endif
+ init_curl_http_auth(result);
+
if (ssl_cert != NULL)
curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
if (has_cert_password())
@@ -844,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)
else if (missing_target(&results))
ret = HTTP_MISSING_TARGET;
else if (results.http_code == 401) {
- if (user_name && user_pass) {
+ if (user_name) {
ret = HTTP_NOAUTH;
} else {
/*
@@ -853,8 +855,7 @@ static int http_request(const char *url, void *result, int target, int options)
* but that is non-portable. Using git_getpass() can at least be stubbed
* on other platforms with a different implementation if/when necessary.
*/
- if (!user_name)
- user_name = xstrdup(git_getpass_with_description("Username", description));
+ user_name = xstrdup(git_getpass_with_description("Username", description));
init_curl_http_auth(slot->curl);
ret = HTTP_REAUTH;
}
--
1.7.8.17.gfd3524
^ permalink raw reply related
* [PATCH 1/2] t5540: test DAV push with authentication
From: Jeff King @ 2011-12-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Naewe, Sebastian Schuberth, Eric, git, msysgit
We don't currently test this case at all, and instead just
test the DAV mechanism over an unauthenticated push. That
isn't very realistic, as most people will want to
authenticate pushes.
Two of the tests expect_failure as they reveal bugs:
1. Pushing without a username in the URL fails to ask for
credentials when we get an HTTP 401. This has always
been the case, but it would be nice if it worked like
smart-http.
2. Pushing with a username fails to ask for the password
since 986bbc0 (http: don't always prompt for password,
2011-11-04). This is a severe regression in v1.7.8, as
authenticated push-over-DAV is now totally unusable
unless you have credentials in your .netrc.
Signed-off-by: Jeff King <peff@peff.net>
---
Nobody has mentioned the regression on git@vger yet, but there are two
threads already on msysgit:
http://thread.gmane.org/gmane.comp.version-control.msysgit/14138
http://thread.gmane.org/gmane.comp.version-control.msysgit/14161
t/lib-httpd/apache.conf | 3 +++
t/t5540-http-push.sh | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0a4cdfa..3c12b05 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -92,6 +92,9 @@ SSLEngine On
<Location /dumb/>
Dav on
</Location>
+ <Location /auth/dumb>
+ Dav on
+ </Location>
</IfDefine>
<IfDefine SVN>
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 64767d8..3300227 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -40,6 +40,22 @@ test_expect_success 'setup remote repository' '
mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
'
+test_expect_success 'create password-protected repository' '
+ mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb" &&
+ cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+ "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git"
+'
+
+test_expect_success 'setup askpass helper' '
+ cat >askpass <<-\EOF &&
+ #!/bin/sh
+ echo user@host
+ EOF
+ chmod +x askpass &&
+ GIT_ASKPASS="$PWD/askpass" &&
+ export GIT_ASKPASS
+'
+
test_expect_success 'clone remote repository' '
cd "$ROOT_PATH" &&
git clone $HTTPD_URL/dumb/test_repo.git test_repo_clone
@@ -144,6 +160,24 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
"$ROOT_PATH"/test_repo_clone master
+test_expect_failure 'push to password-protected repository (user in URL)' '
+ test_commit pw-user &&
+ git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
+ git rev-parse --verify HEAD >expect &&
+ git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+ rev-parse --verify HEAD >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'push to password-protected repository (no user in URL)' '
+ test_commit pw-nouser &&
+ git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD &&
+ git rev-parse --verify HEAD >expect &&
+ git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+ rev-parse --verify HEAD >actual &&
+ test_cmp expect actual
+'
+
stop_httpd
test_done
--
1.7.8.17.gfd3524
^ permalink raw reply related
* [PATCH 2/6] add_extra_ref(): remove flag argument
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
The argument was always set to 0 (and other values do not make sense)
so remove the argument.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
builtin/clone.c | 4 ++--
builtin/receive-pack.c | 2 +-
refs.c | 4 ++--
refs.h | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index efe8b6c..5035767 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -252,7 +252,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
transport = transport_get(remote, ref_git);
for (extra = transport_get_remote_refs(transport); extra;
extra = extra->next)
- add_extra_ref(extra->name, extra->old_sha1, 0);
+ add_extra_ref(extra->name, extra->old_sha1);
transport_disconnect(transport);
free(ref_git);
@@ -441,7 +441,7 @@ static void write_remote_refs(const struct ref *local_refs)
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
- add_extra_ref(r->peer_ref->name, r->old_sha1, 0);
+ add_extra_ref(r->peer_ref->name, r->old_sha1);
}
pack_refs(PACK_REFS_ALL);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b6d957c..e3b46ce 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -872,7 +872,7 @@ static int delete_only(struct command *commands)
static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
{
- add_extra_ref(".have", sha1, 0);
+ add_extra_ref(".have", sha1);
}
static void collect_one_alternate_ref(const struct ref *ref, void *data)
diff --git a/refs.c b/refs.c
index f5cb297..6115487 100644
--- a/refs.c
+++ b/refs.c
@@ -248,9 +248,9 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
sort_ref_array(array);
}
-void add_extra_ref(const char *name, const unsigned char *sha1, int flag)
+void add_extra_ref(const char *name, const unsigned char *sha1)
{
- add_ref(name, sha1, flag, 0, &extra_refs, NULL);
+ add_ref(name, sha1, 0, 0, &extra_refs, NULL);
}
void clear_extra_refs(void)
diff --git a/refs.h b/refs.h
index 3fd5536..39bb289 100644
--- a/refs.h
+++ b/refs.h
@@ -56,7 +56,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
* called. Only extra refs added before for_each_ref() is called will
* be listed on a given call of for_each_ref().
*/
-extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
+extern void add_extra_ref(const char *refname, const unsigned char *sha1);
extern void clear_extra_refs(void);
extern int ref_exists(const char *);
--
1.7.8
^ permalink raw reply related
* [PATCH 4/6] Store extra_refs in a separate data structure
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
Extra references really don't have much to do with real references,
and we want to change how real references are handled. So hold extra
references in a separate data structure.
Since extra references cannot be broken, don't store flags with them
and don't pass the flags argument (a different "flags"!) to
do_for_each_extra_ref().
Finally, current_ref is not useful while iterating through the
extra_refs. In fact, peel_ref() doesn't even work for extra refs
because they cannot be peeled themselves, and moreover they might
coincidentally have the same name as a "real" reference. So
explicitly set current_ref to NULL while iterating over extra_refs to
avoid confusion.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Strictly speaking, this patch changes how an extra reference with an
invalid SHA1 would be treated. In the old code, it would be skipped
over (assuming DO_FOR_EACH_INCLUDE_BROKEN is not set). In the new
code the SHA1 is not checked. However, from my understanding of how
extra_refs are used, they should never have invalid SHA1s and so the
old test is superfluous.
refs.c | 38 ++++++++++++++++++++++++++++----------
1 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 268bda9..843c530 100644
--- a/refs.c
+++ b/refs.c
@@ -146,7 +146,11 @@ static struct ref_cache {
static struct ref_entry *current_ref;
-static struct ref_array extra_refs;
+static struct extra_ref {
+ struct extra_ref *next;
+ unsigned char sha1[20];
+ char name[FLEX_ARRAY];
+} *extra_refs;
static void free_ref_array(struct ref_array *array)
{
@@ -250,12 +254,21 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
void add_extra_ref(const char *name, const unsigned char *sha1)
{
- add_ref(name, sha1, 0, 0, &extra_refs, NULL);
+ int len = strlen(name) + 1;
+ struct extra_ref *entry = xmalloc(sizeof(struct extra_ref) + len);
+ hashcpy(entry->sha1, sha1);
+ memcpy(entry->name, name, len);
+ entry->next = extra_refs;
+ extra_refs = entry;
}
void clear_extra_refs(void)
{
- free_ref_array(&extra_refs);
+ while (extra_refs) {
+ struct extra_ref *entry = extra_refs;
+ extra_refs = entry->next;
+ free(entry);
+ }
}
static struct ref_array *get_packed_refs(const char *submodule)
@@ -694,13 +707,18 @@ fallback:
}
static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
- int trim, int flags, void *cb_data)
+ int trim, void *cb_data)
{
- int i;
- for (i = 0; i < extra_refs.nr; i++) {
- int retval = do_one_ref(base, fn, trim, flags, cb_data, extra_refs.refs[i]);
- if (retval)
- return retval;
+ struct extra_ref *extra;
+
+ current_ref = NULL;
+ for (extra = extra_refs; extra; extra = extra->next) {
+ if (!prefixcmp(extra->name, base)) {
+ int retval = fn(extra->name + trim, extra->sha1,
+ 0, cb_data);
+ if (retval)
+ return retval;
+ }
}
return 0;
}
@@ -712,7 +730,7 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
struct ref_array *packed = get_packed_refs(submodule);
struct ref_array *loose = get_loose_refs(submodule);
- retval = do_for_each_extra_ref(base, fn, trim, flags, cb_data);
+ retval = do_for_each_extra_ref(base, fn, trim, cb_data);
if (retval)
goto end_each;
--
1.7.8
^ permalink raw reply related
* [PATCH 6/6] do_for_each_extra_ref(): simplify signature
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
Now that do_for_each_extra_ref() is only called from one place, it can
be less general.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index f52d8b5..4b2ba3f 100644
--- a/refs.c
+++ b/refs.c
@@ -706,19 +706,15 @@ fallback:
return -1;
}
-static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
- int trim, void *cb_data)
+static int do_for_each_extra_ref(each_ref_fn fn, void *cb_data)
{
struct extra_ref *extra;
current_ref = NULL;
for (extra = extra_refs; extra; extra = extra->next) {
- if (!prefixcmp(extra->name, base)) {
- int retval = fn(extra->name + trim, extra->sha1,
- 0, cb_data);
- if (retval)
- return retval;
- }
+ int retval = fn(extra->name, extra->sha1, 0, cb_data);
+ if (retval)
+ return retval;
}
return 0;
}
@@ -794,7 +790,7 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
int for_each_ref(each_ref_fn fn, void *cb_data)
{
- int retval = do_for_each_extra_ref("", fn, 0, cb_data);
+ int retval = do_for_each_extra_ref(fn, cb_data);
if (retval)
return retval;
return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
--
1.7.8
^ permalink raw reply related
* [PATCH 3/6] Extract a function do_for_each_extra_ref()
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
We want to hold the extra refs at arms length.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 6115487..268bda9 100644
--- a/refs.c
+++ b/refs.c
@@ -693,17 +693,28 @@ fallback:
return -1;
}
+static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
+ int trim, int flags, void *cb_data)
+{
+ int i;
+ for (i = 0; i < extra_refs.nr; i++) {
+ int retval = do_one_ref(base, fn, trim, flags, cb_data, extra_refs.refs[i]);
+ if (retval)
+ return retval;
+ }
+ return 0;
+}
+
static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
int trim, int flags, void *cb_data)
{
- int retval = 0, i, p = 0, l = 0;
+ int retval = 0, p = 0, l = 0;
struct ref_array *packed = get_packed_refs(submodule);
struct ref_array *loose = get_loose_refs(submodule);
- struct ref_array *extra = &extra_refs;
-
- for (i = 0; i < extra->nr; i++)
- retval = do_one_ref(base, fn, trim, flags, cb_data, extra->refs[i]);
+ retval = do_for_each_extra_ref(base, fn, trim, flags, cb_data);
+ if (retval)
+ goto end_each;
while (p < packed->nr && l < loose->nr) {
struct ref_entry *entry;
--
1.7.8
^ permalink raw reply related
* [PATCH 5/6] Omit extra_refs except when iterating using for_each_ref()
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
According to Junio, the only reference iteration function that needs
to include the extra refs is for_each_ref(). So call
do_for_each_extra_ref() directly from there instead of from
do_for_each_ref().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 843c530..f52d8b5 100644
--- a/refs.c
+++ b/refs.c
@@ -730,10 +730,6 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
struct ref_array *packed = get_packed_refs(submodule);
struct ref_array *loose = get_loose_refs(submodule);
- retval = do_for_each_extra_ref(base, fn, trim, cb_data);
- if (retval)
- goto end_each;
-
while (p < packed->nr && l < loose->nr) {
struct ref_entry *entry;
int cmp = strcmp(packed->refs[p]->name, loose->refs[l]->name);
@@ -798,6 +794,9 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
int for_each_ref(each_ref_fn fn, void *cb_data)
{
+ int retval = do_for_each_extra_ref("", fn, 0, cb_data);
+ if (retval)
+ return retval;
return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
}
--
1.7.8
^ permalink raw reply related
* [PATCH 0/6] Handle extra_refs separately from ref_caches
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
From: Michael Haggerty <mhagger@alum.mit.edu>
Extra refs don't have much to do with real references, and in fact
they have to be handled differently. For example, they do not support
flags, they might not have unique names (indeed, the names are rather
meaningless), and they are only ever iterated over, never looked up.
So seemingly innocent things that one might want to do with real
references, like check for conflicting duplicates, must not be done
for extra refs.
This patch series creates a new linked-list data structure for the
extra refs, separates iteration over the extra refs into a new
function, and changes a test to actually create multiple extra refs
with the same name.
This patch series applies on top of master. If this approach is
selected, then the ref-api-D series will have to be rebased on top of
it and touched up to avoid the problems that it has with duplicate
extra refs.
By the way, I have been carrying around the CC list of this email for
quite a while. If you are tired of being spammed with my patch
series, send me a private email and I will be happy to remove you from
future mailings.
Michael Haggerty (6):
t5519: push two branches to alternate repo
add_extra_ref(): remove flag argument
Extract a function do_for_each_extra_ref()
Store extra_refs in a separate data structure
Omit extra_refs except when iterating using for_each_ref()
do_for_each_extra_ref(): simplify signature
builtin/clone.c | 4 ++--
builtin/receive-pack.c | 2 +-
refs.c | 44 ++++++++++++++++++++++++++++++++++----------
refs.h | 2 +-
t/t5519-push-alternates.sh | 10 +++++++++-
5 files changed, 47 insertions(+), 15 deletions(-)
--
1.7.8
^ permalink raw reply
* [PATCH 1/6] t5519: push two branches to alternate repo
From: mhagger @ 2011-12-13 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1323806811-5798-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
Since each branch in the alternate repo results in an "extra_ref"
named ".have", pushing two of them results in two extra_refs with the
same name. This change to the test therefore makes sure that we can
handle extra_refs names that are not unique.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
I'm not sure how well this change fits into the other things that the
test wants to do, but it triggers the failure mode in ref-api-D v2
that was predicted by Junio.
t/t5519-push-alternates.sh | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index c00c9b0..315f65d 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -17,7 +17,15 @@ test_expect_success setup '
>file &&
git add . &&
git commit -m initial &&
- git push ../alice-pub master
+ git checkout -b foo &&
+ >file1 &&
+ git add . &&
+ git commit -m file1 &&
+ git checkout master &&
+ >file2 &&
+ git add . &&
+ git commit -m file2 &&
+ git push ../alice-pub master foo
) &&
# Project Bob is a fork of project Alice
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 2/2] Makefile: optionally exclude code that needs Unix sockets
From: Johannes Sixt @ 2011-12-13 20:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vzkexwb7m.fsf@alter.siamese.dyndns.org>
Am 13.12.2011 01:45, schrieb Junio C Hamano:
> I'll queue a single patch that is a squash between 2/2 and Peff's test
> updates between "credentials: add "cache" helper" and "strbuf: add
> strbuf_add*_urlencode" in the series.
Thanks. The resulting series builds fine on Windows and passes/skips the
new tests in the expected manner.
-- Hannes
^ permalink raw reply
* Re: [PATCH/RFC 1/2] wrapper: supply xsetenv
From: Andreas Schwab @ 2011-12-13 19:21 UTC (permalink / raw)
To: kusmabite; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <CABPQNSZ_r4u-yXtEGw8duZyoN3SdF5p+7vabz2qqS161kgkHWQ@mail.gmail.com>
Erik Faye-Lund <kusmabite@gmail.com> writes:
> Older Linux kernels maxed out at 128 kB.
But you wouldn't detect that at setenv time.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v2 28/51] refs.c: rename ref_array -> ref_dir
From: Junio C Hamano @ 2011-12-13 19:17 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
Heiko Voigt, Johan Herland, Julian Phillips
In-Reply-To: <4EE7A387.3070400@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> ... But I leave for vacation on Thursday so it is quite likely that I
> won't be able to get it finished before I return in the new year.
That is perfectly fine. Have fun.
I wasn't expecting the entire series to be ready during this cycle
anyway, and was planning to queue the clean-up series, up to 16/51
or so, for the next release.
^ permalink raw reply
* Re: [PATCH v2 28/51] refs.c: rename ref_array -> ref_dir
From: Michael Haggerty @ 2011-12-13 19:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <7vk461vuy9.fsf@alter.siamese.dyndns.org>
On 12/13/2011 07:37 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Apropos testing, it is unsettling that our test suite doesn't show any
>> failures after my changes. The dir entries in extra_refs are now always
>> sorted and de-duped when do_for_each_ref() is called. Could it be that
>> duplicate ".have" references never come up in the test suite? It sounds
>> like an important code path is not being tested. In particular, I won't
>> be able to test whether my fix works :-/
>
> I doubt anybody thought of using more than one --reference while cloning
> or having more than one entry in $GIT_DIR/objects/info/alternates in a
> repository that is being pushed into, even though we might have tests for
> simpler single --reference and single alternate cases.
Even with a single alternate database, multiple references are
advertised with the same name ".have". But the tests never push from a
repository with more than one reference in its alternates (verified by
instrumenting code). That is why my changes didn't cause test failures.
When I, for example, change the setup function in t5519 to push *two*
references to alice-pub, then master works fine, my ref-api branch
fails, and my fixup patch fixes the failure.
> As to the order of the changes, my gut feeling is that it would make it
> harder to debug your series to delay the removal of "extra_ref" hackery,
> as it would be a corner case that your "hierarchical" structure never has
> to worry about in the end result.
>
> Another possibility is to keep the extra_ref interface in refs.c, without
> any of your changes (i.e. keep it just as a flat list, with the original
> interface to append to it without any dedup and other fancy features) and
> also keep the special casing of it in for_each_ref(). AFAIK, that is the
> only iterator that should care about the extra refs. Thinking about it a
> bit more, removal of add_extra_ref() API may be unnecessarily complex with
> no real gain. For example, extra refs added in builtin/clone.c is used by
> builtin/fetch-pack.c, meaning that the codepath that discovers and
> remembers these extra history anchor points and the codepath that uses
> them while walking the history are not localized and we would need some
> sort of "extra ref API" anyway. I am leaning towards this option.
In a few minutes I will post a series of patches that store extra_refs
in a linked list separate from the reference caches, and iterates over
them only from for_each_ref(). I could rebase my ref-api-D changes on
top of this patch series in such a way that the extra refs are kept in
non-hierarchical storage. But I leave for vacation on Thursday so it is
quite likely that I won't be able to get it finished before I return in
the new year.
An alternative is to use my one-patch fix on top of the ref-api-D
changes (plus another patch to beef up t5519). The fix is isolated and
I'm confident that it is safe (though I would inspect and test it better
before formally submitting it). The advantage is that is less work, so
it can be ready tomorrow instead of in two weeks. The disadvantage is
that there would be an interval of commits on the feature branch (from
the middle of the ref-api-D patch series until the fix) that are
non-functional, albeit in a way that the test suite doesn't detect.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: best way to fastforward all tracking branches after a fetch
From: Hallvard Breien Furuseth @ 2011-12-13 19:05 UTC (permalink / raw)
To: Stefan Haller; +Cc: Gelonida N, git
In-Reply-To: <1kc5m38.m71ik21ytxkhbM%lists@haller-berlin.de>
Stefan Haller writes:
>Hallvard B Furuseth <h.b.furuseth@usit.uio.no> wrote:
>> Local branches can track each other. So the script needs to toposort
>> the branches, or to loop until either nothing was done or an error
>> happened. (The latter to prevent an eternal loop on error.)
>
> Is this just theoretical, or are there real use cases for this? What
> would be a workflow with such a local tracking branch?
Personally I don't care much, I just noted that the script did not
match the question in the subject line.
--
Hallvard
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Junio C Hamano @ 2011-12-13 19:01 UTC (permalink / raw)
To: Frans Klaver; +Cc: git
In-Reply-To: <1323788917-4141-2-git-send-email-fransklaver@gmail.com>
Frans Klaver <fransklaver@gmail.com> writes:
> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> + /*
> + * man 2 execve states that EACCES is returned for:
> + * - Search permission is denied on a component of the path prefix
> + * of cmd or the name of a script interpreter
> + * - The file or script interpreter is not a regular file
> + * - Execute permission is denied for the file, script or ELF
> + * interpreter
> + * - The file system is mounted noexec
> + */
> + struct strbuf sb = STRBUF_INIT;
> + char *path;
> + char *next;
> +
> + if (strchr(cmd, '/')) {
> + if (!have_read_execute_permissions(cmd))
> + error("no read/execute permissions on '%s'\n", cmd);
> + return;
> + }
> +
Three points.
- error() gives you a LF at the end, so you do not have to have your own.
- That "have_..._ions()" is too long and ugly.
- The only thing you care about this callsite is if you have enough
permission to execute the "cmd".
In fact, you should not unconditionally require read permissions here.
$ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
--wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
$ /home/junio/git-active/bin/git --version
git version 1.7.8.249.gb1b73
You may need read permission when the file is a script (i.e. not binary
executable).
> + path = getenv("PATH");
> + while (path) {
> + next = strchrnul(path, ':');
> + if (path < next)
> + strbuf_add(&sb, path, next - path);
> + else
> + strbuf_addch(&sb, '.');
> +
> + if (!*next)
> + path = NULL;
> + else
> + path = next + 1;
> +
> + if (!have_read_execute_permissions(sb.buf)) {
When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/"
do not have to be readable. They only have to have executable bit to allow
descending into them, and typically this is called "searchable" (see man chmod).
$ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
$ chmod 111 /var/tmp/a /var/tmp/a/b
$ /var/tmp/a/b/git --version
git version 1.7.8.249.gb1b73
I'd suggest having two helper functions, instead of the single one with
overlong "have...ions" name.
- can_search_directory() checks with access(X_OK);
- can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
not always needed.
Use the former here where you check the directory that contains the
command, and use the latter up above where you check the command that is
supposed to be executable, and also down below after you checked sb.buf is
a path to a file that may be the command that is supposed to be
executable.
Then patch 2/2 can extend can_execute() to enhance its support for scripts
by reading the hash-bang line and validating it, etc.
^ permalink raw reply
* Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv
From: Erik Faye-Lund @ 2011-12-13 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v1us8wccm.fsf@alter.siamese.dyndns.org>
On Tue, Dec 13, 2011 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:
>>
>>> While reviewing some patches for Git for Windows, I realized that
>>> we almost never check the return-value from setenv. This can lead
>>> to quite surprising errors in unusual sitations. Mostly, an error
>>> would probably be preferred. So here we go.
>>>
>>> However, I'm not at all convinced myself that all of these make
>>> sense; in particular settings like GIT_EDITOR and GIT_PAGER could
>>> perhaps benefit from having a warning printed rather than a hard
>>> error.
>>>
>>> Thoughts?
>>
>> I wrote almost the same patch once[1], but failed to actually push it
>> through to acceptance.
>
> In both cases, the patches are _designed_ to fail to attract any
> attention. Your earlier one had this preamble:
>
> Here is a patch. I still feel a little silly writing this. The chances
> that you will run out of memory doing setenv but _not_ doing any of the
> other git operations seems very low.
>
> which rather *loudly* says "ignore me, please!" ;-)
>
> Erik's patch this round is no better; if its log message said something
> like "On platform X, the environment space is merely nKB and setenv has
> much higher chance of failing than on typical Linux boxes", it would have
> been a no brainer for me to respond with "makes perfect sense but don't we
> also use putenv?" immediately.
>
It could be because I treated this completely like a theoretical
patch; I haven't seen it actually happen.
But you are right, Windows 32 kB environment limit makes this much
more likely than your average Linux box. So perhaps I should add a
notice about that in the next round...
^ permalink raw reply
* Re: [PATCH/RFC 1/2] wrapper: supply xsetenv
From: Erik Faye-Lund @ 2011-12-13 18:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vwra0uxqo.fsf@alter.siamese.dyndns.org>
On Tue, Dec 13, 2011 at 7:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Tue, Dec 13, 2011 at 01:10:26PM +0100, Erik Faye-Lund wrote:
>>
>>> +int xsetenv(const char *name, const char *val, int overwrite)
>>> +{
>>> + if (setenv(name, val, overwrite))
>>> + die_errno("setenv failed");
>>> +}
>>
>> It probably doesn't matter, because the error condition is almost
>> certainly just "oops, we ran out of memory". But you could also print
>> the name of the variable being set, which may give the user a clue to
>> some misconfiguration (e.g., trying to put some extremely long value
>> into the environment).
>
> Do we have enough memory to format that message in that situation ;-)?
We could. Running out of environment space is not the same as running
out of memory. For instance, Windows has a maximum environment size of
32 kB. Older Linux kernels maxed out at 128 kB.
So I think it's a good idea to at least try. The worst that can happen
is another, less descriptive error, no?
^ permalink raw reply
* Re: [PATCH/RFC 1/2] wrapper: supply xsetenv
From: Junio C Hamano @ 2011-12-13 18:34 UTC (permalink / raw)
To: Jeff King; +Cc: Erik Faye-Lund, git
In-Reply-To: <20111213181602.GB1663@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 13, 2011 at 01:10:26PM +0100, Erik Faye-Lund wrote:
>
>> +int xsetenv(const char *name, const char *val, int overwrite)
>> +{
>> + if (setenv(name, val, overwrite))
>> + die_errno("setenv failed");
>> +}
>
> It probably doesn't matter, because the error condition is almost
> certainly just "oops, we ran out of memory". But you could also print
> the name of the variable being set, which may give the user a clue to
> some misconfiguration (e.g., trying to put some extremely long value
> into the environment).
Do we have enough memory to format that message in that situation ;-)?
^ permalink raw reply
* Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv
From: Junio C Hamano @ 2011-12-13 18:33 UTC (permalink / raw)
To: Jeff King; +Cc: Erik Faye-Lund, git
In-Reply-To: <20111213181946.GC1663@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:
>
>> While reviewing some patches for Git for Windows, I realized that
>> we almost never check the return-value from setenv. This can lead
>> to quite surprising errors in unusual sitations. Mostly, an error
>> would probably be preferred. So here we go.
>>
>> However, I'm not at all convinced myself that all of these make
>> sense; in particular settings like GIT_EDITOR and GIT_PAGER could
>> perhaps benefit from having a warning printed rather than a hard
>> error.
>>
>> Thoughts?
>
> I wrote almost the same patch once[1], but failed to actually push it
> through to acceptance.
In both cases, the patches are _designed_ to fail to attract any
attention. Your earlier one had this preamble:
Here is a patch. I still feel a little silly writing this. The chances
that you will run out of memory doing setenv but _not_ doing any of the
other git operations seems very low.
which rather *loudly* says "ignore me, please!" ;-)
Erik's patch this round is no better; if its log message said something
like "On platform X, the environment space is merely nKB and setenv has
much higher chance of failing than on typical Linux boxes", it would have
been a no brainer for me to respond with "makes perfect sense but don't we
also use putenv?" immediately.
^ permalink raw reply
* [PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol
From: iheffner @ 2011-12-13 18:28 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Dave Olszewski, Ivan Heffner
In-Reply-To: <1323800931-37123-1-git-send-email-iheffner@gmail.com>
From: Ivan Heffner <iheffner@gmail.com>
Switched calls to send_sideband() for pack protocol errors to use
SIDEBAND_PROTOCOL_ERROR and SIDEBAND_REMOTE_ERROR in place of the
sideband magic numbers of -2 and -1, respectively.
Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..63e9ac4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -245,7 +245,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
static void send_request(int fd, struct strbuf *buf)
{
if (args.stateless_rpc) {
- send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
+ send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf->buf, buf->len, LARGE_PACKET_MAX);
packet_flush(fd);
} else
safe_write(fd, buf->buf, buf->len);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e0b8030..67c9fe5 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -96,7 +96,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
if (n <= 0)
break;
- send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
+ send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf, n, LARGE_PACKET_MAX);
}
free(buf);
close(po.out);
@@ -320,7 +320,7 @@ int send_pack(struct send_pack_args *args,
if (args->stateless_rpc) {
if (!args->dry_run && cmds_sent) {
packet_buf_flush(&req_buf);
- send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
+ send_sideband(out, SIDEBAND_REMOTE_ERROR, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
}
} else {
safe_write(out, req_buf.buf, req_buf.len);
--
1.7.6.553.g917d7.dirty
^ permalink raw reply related
* [PATCH 2/3] switch sideband communication to use constants
From: iheffner @ 2011-12-13 18:28 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Dave Olszewski, Ivan Heffner
In-Reply-To: <1323800931-37123-1-git-send-email-iheffner@gmail.com>
From: Ivan Heffner <iheffner@gmail.com>
Found all uses of magic numbers for sideband channel indicators and
changed them to use the new SIDEBAND_CHAN_* constants.
Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
builtin/receive-pack.c | 6 +++---
builtin/upload-archive.c | 6 +++---
sideband.c | 6 +++---
upload-pack.c | 22 +++++++++++-----------
4 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..43f7a55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -179,7 +179,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
msg[sz++] = '\n';
if (use_sideband)
- send_sideband(1, 2, msg, sz, use_sideband);
+ send_sideband(1, SIDEBAND_CHAN_PROGRESS, msg, sz, use_sideband);
else
xwrite(2, msg, sz);
}
@@ -207,7 +207,7 @@ static int copy_to_sideband(int in, int out, void *arg)
ssize_t sz = xread(in, data, sizeof(data));
if (sz <= 0)
break;
- send_sideband(1, 2, data, sz, use_sideband);
+ send_sideband(1, SIDEBAND_CHAN_PROGRESS, data, sz, use_sideband);
}
close(in);
return 0;
@@ -851,7 +851,7 @@ static void report(struct command *commands, const char *unpack_status)
packet_buf_flush(&buf);
if (use_sideband)
- send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+ send_sideband(1, SIDEBAND_CHAN_PACK, buf.buf, buf.len, use_sideband);
else
safe_write(1, buf.buf, buf.len);
strbuf_release(&buf);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2d0b383..4ac7984 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -77,7 +77,7 @@ static void error_clnt(const char *fmt, ...)
va_start(params, fmt);
len = vsprintf(buf, fmt, params);
va_end(params);
- send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
+ send_sideband(1, SIDEBAND_CHAN_ERROR, buf, len, LARGE_PACKET_MAX);
die("sent error to the client: %s", buf);
}
@@ -149,11 +149,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
}
if (pfd[1].revents & POLLIN)
/* Status stream ready */
- if (process_input(pfd[1].fd, 2))
+ if (process_input(pfd[1].fd, SIDEBAND_CHAN_PROGRESS))
continue;
if (pfd[0].revents & POLLIN)
/* Data stream ready */
- if (process_input(pfd[0].fd, 1))
+ if (process_input(pfd[0].fd, SIDEBAND_CHAN_PACK))
continue;
if (waitpid(writer, &status, 0) < 0)
diff --git a/sideband.c b/sideband.c
index d5ffa1c..4b4199a 100644
--- a/sideband.c
+++ b/sideband.c
@@ -47,12 +47,12 @@ int recv_sideband(const char *me, int in_stream, int out)
band = buf[pf] & 0xff;
len--;
switch (band) {
- case 3:
+ case SIDEBAND_CHAN_ERROR:
buf[pf] = ' ';
buf[pf+1+len] = '\0';
fprintf(stderr, "%s\n", buf);
return SIDEBAND_REMOTE_ERROR;
- case 2:
+ case SIDEBAND_CHAN_PROGRESS:
buf[pf] = ' ';
do {
char *b = buf;
@@ -107,7 +107,7 @@ int recv_sideband(const char *me, int in_stream, int out)
memmove(buf + pf+1, b + brk, len);
} while (len);
continue;
- case 1:
+ case SIDEBAND_CHAN_PACK:
safe_write(out, buf + pf+1, len);
continue;
default:
diff --git a/upload-pack.c b/upload-pack.c
index 470cffd..98c2542 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -56,19 +56,19 @@ static int strip(char *line, int len)
return len;
}
-static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
+static ssize_t send_client_data(int chan, const char *data, ssize_t sz)
{
if (use_sideband)
- return send_sideband(1, fd, data, sz, use_sideband);
- if (fd == 3)
+ return send_sideband(1, chan, data, sz, use_sideband);
+ if (chan == SIDEBAND_CHAN_ERROR)
/* emergency quit */
- fd = 2;
- if (fd == 2) {
+ chan = SIDEBAND_CHAN_PROGRESS;
+ if (chan == SIDEBAND_CHAN_PROGRESS) {
/* XXX: are we happy to lose stuff here? */
- xwrite(fd, data, sz);
+ xwrite(chan, data, sz);
return sz;
}
- return safe_write(fd, data, sz);
+ return safe_write(chan, data, sz);
}
static FILE *pack_pipe = NULL;
@@ -243,7 +243,7 @@ static void create_pack_file(void)
sz = xread(pack_objects.err, progress,
sizeof(progress));
if (0 < sz)
- send_client_data(2, progress, sz);
+ send_client_data(SIDEBAND_CHAN_PROGRESS, progress, sz);
else if (sz == 0) {
close(pack_objects.err);
pack_objects.err = -1;
@@ -286,7 +286,7 @@ static void create_pack_file(void)
}
else
buffered = -1;
- sz = send_client_data(1, data, sz);
+ sz = send_client_data(SIDEBAND_CHAN_PACK, data, sz);
if (sz < 0)
goto fail;
}
@@ -302,7 +302,7 @@ static void create_pack_file(void)
/* flush the data */
if (0 <= buffered) {
data[0] = buffered;
- sz = send_client_data(1, data, 1);
+ sz = send_client_data(SIDEBAND_CHAN_PACK, data, 1);
if (sz < 0)
goto fail;
fprintf(stderr, "flushed.\n");
@@ -312,7 +312,7 @@ static void create_pack_file(void)
return;
fail:
- send_client_data(3, abort_msg, sizeof(abort_msg));
+ send_client_data(SIDEBAND_CHAN_ERROR, abort_msg, sizeof(abort_msg));
die("git upload-pack: %s", abort_msg);
}
--
1.7.6.553.g917d7.dirty
^ permalink raw reply related
* [PATCH 1/3] add constants for sideband communication channels
From: iheffner @ 2011-12-13 18:28 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Dave Olszewski, Ivan Heffner
In-Reply-To: <1323800931-37123-1-git-send-email-iheffner@gmail.com>
From: Ivan Heffner <iheffner@gmail.com>
Add constants for sideband channel identifiers.
* SIDEBAND_CHAN_PACK => 1
* SIDEBAND_CHAN_PROGRESS => 2
* SIDEBAND_CHAN_ERROR => 3
Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
sideband.h | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/sideband.h b/sideband.h
index d72db35..2954979 100644
--- a/sideband.h
+++ b/sideband.h
@@ -2,7 +2,11 @@
#define SIDEBAND_H
#define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_CHAN_PACK 1
+#define SIDEBAND_CHAN_PROGRESS 2
+#define SIDEBAND_CHAN_ERROR 3
+
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
--
1.7.6.553.g917d7.dirty
^ permalink raw reply related
* [PATCH 0/3] use constants for sideband communication channels
From: iheffner @ 2011-12-13 18:28 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Dave Olszewski
In order to make more clear how the different channels in sidechannel.c
are to be used, I'm proposing some macros/constants which can be used in
place of the "magic numbers" that mean little or nothing to someone not
familiar with the protocol.
In working on a script to handle archiving from a bare repository a
project which contained submodules, I had a difficult time determining
how to talk between my git-upload-archive replacement and git-archive.
It did not occur to me to look at documentation for git-send-pack or
git-receive-pack when trying to understand the communication protocol.
But looking through the code and poking at an implementation, I finally
understood that I needed to send a channel identifier to say what type
of communication I was sending. I determined that there were three
possible channels. I could easily tell that channel three (3) was for
catastrophic errors on the server side. But it was not clear what the
difference between channel 1 and channel 2 were. Because channel 2 was
the one that appeared to be read and parsed in some manner, I assumed
that this was the "important" data channel and tried sending my data on
that channel.
I was confused and frustrated when it appeared that git-archive treated
data coming from my --exec'd git-upload-archive replacement differently
than it did when receiving data from the real git-upload-archive.
Eventually I figured out that channel 1 was for data, channel 2 was for
non-fatal messages, and channel 3 was for fatal error communications.
Having comments in sidechannel.h would have helped. But constants or
macros would have helped as well and makes the code that uses them more
clear.
Ivan
[PATCH 1/3] add constants for sideband communication channels
[PATCH 2/3] switch sideband communication to use constants
[PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol
builtin/fetch-pack.c | 2 +-
builtin/receive-pack.c | 6 +++---
builtin/send-pack.c | 4 ++--
builtin/upload-archive.c | 6 +++---
sideband.c | 6 +++---
sideband.h | 6 +++++-
upload-pack.c | 22 +++++++++++-----------
7 files changed, 28 insertions(+), 24 deletions(-)
^ permalink raw reply
* Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv
From: Jeff King @ 2011-12-13 18:19 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
In-Reply-To: <1323778227-1664-2-git-send-email-kusmabite@gmail.com>
On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:
> While reviewing some patches for Git for Windows, I realized that
> we almost never check the return-value from setenv. This can lead
> to quite surprising errors in unusual sitations. Mostly, an error
> would probably be preferred. So here we go.
>
> However, I'm not at all convinced myself that all of these make
> sense; in particular settings like GIT_EDITOR and GIT_PAGER could
> perhaps benefit from having a warning printed rather than a hard
> error.
>
> Thoughts?
I wrote almost the same patch once[1], but failed to actually push it
through to acceptance. There weren't any objections, just that nobody
really cared. I think it's a reasonable thing to do. The chances of
setenv failing are very low, but the consequences could be quite bad.
There is also a call to putenv in git.c which should be checked (or
could arguably just be converted to setenv).
-Peff
[1] http://thread.gmane.org/gmane.comp.version-control.git/134466
^ 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