From 81d340d40af506eda3182190b6132575547fa4c5 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 10 Apr 2013 17:15:52 -0400 Subject: transport-helper: report errors properly If a push fails because the remote-helper died (with fast-export), the user may not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. While we're adding tests, let's also confirm that the remote-helper dying is also detected when importing refs. We currently do so robustly when the helper uses the "done" feature (and that is what we test). We cannot do so reliably when the helper does not use the "done" feature, but it is not even worth testing; the right solution is for the helper to start using "done". Suggested-by: Jeff King Signed-off-by: Felipe Contreras Signed-off-by: Jeff King Acked-by: Sverre Rabbelier Signed-off-by: Junio C Hamano --- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index cb3ef7d38e..96081cc392 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer) if (strbuf_getline(buffer, helper, '\n') == EOF) { if (debug) fprintf(stderr, "Debug: Remote helper quit.\n"); - exit(128); + die("Reading from remote helper failed"); } if (debug) -- cgit v1.2.1 From c096955c5bb8e20186cc7c07d4d12b77ddcd01c6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Apr 2013 17:16:03 -0400 Subject: transport-helper: mention helper name when it dies When we try to read from a remote-helper and get EOF or an error, we print a message indicating that the helper died. However, users may not know that a remote helper was in use (e.g., when using git-over-http), or even what a remote helper is. Let's print the name of the helper (e.g., "git-remote-https"); this makes it more obvious what the program is for, and provides a useful token for reporting bugs or searching for more information (e.g., in manpages). Signed-off-by: Jeff King Acked-by: Sverre Rabbelier Signed-off-by: Junio C Hamano --- transport-helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 96081cc392..3fc43b97f5 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -46,7 +46,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer) die_errno("Full write to remote helper failed"); } -static int recvline_fh(FILE *helper, struct strbuf *buffer) +static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name) { strbuf_reset(buffer); if (debug) @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer) if (strbuf_getline(buffer, helper, '\n') == EOF) { if (debug) fprintf(stderr, "Debug: Remote helper quit.\n"); - die("Reading from remote helper failed"); + die("Reading from helper 'git-remote-%s' failed", name); } if (debug) @@ -64,7 +64,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer) static int recvline(struct helper_data *helper, struct strbuf *buffer) { - return recvline_fh(helper->out, buffer); + return recvline_fh(helper->out, buffer, helper->name); } static void xchgline(struct helper_data *helper, struct strbuf *buffer) @@ -536,7 +536,7 @@ static int process_connect_service(struct transport *transport, goto exit; sendline(data, &cmdbuf); - recvline_fh(input, &cmdbuf); + recvline_fh(input, &cmdbuf, name); if (!strcmp(cmdbuf.buf, "")) { data->no_disconnect_req = 1; if (debug) -- cgit v1.2.1 From 5034fdea3017304fa37e50b3bd40c392503ebfd2 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 10 Apr 2013 19:07:11 -0500 Subject: transport-helper: improve push messages If there's already a remote-helper tracking ref, we can fetch the SHA-1 to report proper push messages (as opposed to always reporting [new branch]). The remote-helper currently can specify the old SHA-1 to avoid this problem, but there's no point in forcing all remote-helpers to be aware of git commit ids; they should be able to be agnostic of them. Signed-off-by: Felipe Contreras Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- transport-helper.c | 1 + 1 file changed, 1 insertion(+) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 3fc43b97f5..56207eae2f 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -801,6 +801,7 @@ static int push_refs_with_export(struct transport *transport, if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); string_list_append(&revlist_args, strbuf_detach(&buf, NULL)); + hashcpy(ref->old_sha1, sha1); } free(private); -- cgit v1.2.1 From 7a43c55415a224039a41684cbadd5b94be6f1a8e Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 17 Apr 2013 23:14:28 -0500 Subject: transport-helper: clarify *:* refspec The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 56207eae2f..018513baa0 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport, * were fetching. * * (If no "refspec" capability was specified, for historical - * reasons we default to *:*.) + * reasons we default to the equivalent of *:*.) * * Store the result in to_fetch[i].old_sha1. Callers such * as "git fetch" can use the value to write feedback to the -- cgit v1.2.1 From 21610d820b97583a8f4e3e7f4a48716c8e32fd92 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 17 Apr 2013 23:14:30 -0500 Subject: transport-helper: clarify pushing without refspecs This has never worked, since it's inception the code simply skips all the refs, essentially telling fast-export to do nothing. Let's at least tell the user what's going on. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 018513baa0..98ef8f6410 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -785,6 +785,9 @@ static int push_refs_with_export(struct transport *transport, struct string_list revlist_args = STRING_LIST_INIT_NODUP; struct strbuf buf = STRBUF_INIT; + if (!data->refspecs) + die("remote-helper doesn't support push; refspec needed"); + helper = get_helper(transport); write_constant(helper->in, "export\n"); @@ -795,8 +798,6 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; - if (!data->refspecs) - continue; private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); -- cgit v1.2.1 From a93b4a09acfed0e2a006770d0196c74968e65c25 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 17 Apr 2013 23:14:31 -0500 Subject: transport-helper: warn when refspec is not used For the modes that need it. In the future we should probably error out, instead of providing half-assed support. The reason we want to do this is because if it's not present, the remote helper might be updating refs/heads/*, or refs/remotes/origin/*, directly, and in the process fetch will get confused trying to update refs that are already updated, or older than what they should be. We shouldn't be messing with the rest of git. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 98ef8f6410..2452b3bb6d 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport *transport) free((char *)refspecs[i]); } free(refspecs); + } else if (data->import || data->bidi_import || data->export) { + warning("This remote helper should implement refspec capability."); } strbuf_release(&buf); if (debug) -- cgit v1.2.1 From 9c51558cfb6ffe104da45d324194d9f1ebf9bc65 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 17 Apr 2013 23:14:32 -0500 Subject: transport-helper: trivial code shuffle Just shuffle the die() part to make it more explicit, and cleanup the code-style. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 2452b3bb6d..d48e00d03c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -800,6 +800,9 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; + if (ref->deletion) + die("remote-helpers do not support ref deletion"); + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); @@ -808,13 +811,8 @@ static int push_refs_with_export(struct transport *transport, } free(private); - if (ref->deletion) { - die("remote-helpers do not support ref deletion"); - } - if (ref->peer_ref) string_list_append(&revlist_args, ref->peer_ref->name); - } if (get_exporter(transport, &exporter, &revlist_args)) -- cgit v1.2.1 From 664059fb624467975da5c9a4bf6053c83126eaa7 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Wed, 17 Apr 2013 23:14:33 -0500 Subject: transport-helper: update remote helper namespace When pushing, the remote namespace is updated correctly (e.g. refs/origin/master), but not the remote helper's (e.g. refs/testgit/origin/master), which currently is only updated while fetching. Since the remote namespace is used to tell fast-export which commits to avoid (because they were already imported/exported), it makes sense to have them in sync so they don't get generated twice. If the remote helper was implemented properly, they would be ignored, if not, they probably would end up repeated. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index d48e00d03c..92174095ed 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -11,6 +11,7 @@ #include "thread-utils.h" #include "sigchain.h" #include "argv-array.h" +#include "refs.h" static int debug; @@ -620,7 +621,7 @@ static int fetch(struct transport *transport, return -1; } -static void push_update_ref_status(struct strbuf *buf, +static int push_update_ref_status(struct strbuf *buf, struct ref **ref, struct ref *remote_refs) { @@ -686,7 +687,7 @@ static void push_update_ref_status(struct strbuf *buf, *ref = find_ref_by_name(remote_refs, refname); if (!*ref) { warning("helper reported unexpected status of %s", refname); - return; + return 1; } if ((*ref)->status != REF_STATUS_NONE) { @@ -695,11 +696,12 @@ static void push_update_ref_status(struct strbuf *buf, * status reported by the remote helper if the latter is 'no match'. */ if (status == REF_STATUS_NONE) - return; + return 1; } (*ref)->status = status; (*ref)->remote_status = msg; + return 0; } static void push_update_refs_status(struct helper_data *data, @@ -708,11 +710,24 @@ static void push_update_refs_status(struct helper_data *data, struct strbuf buf = STRBUF_INIT; struct ref *ref = remote_refs; for (;;) { + char *private; + recvline(data, &buf); if (!buf.len) break; - push_update_ref_status(&buf, &ref, remote_refs); + if (push_update_ref_status(&buf, &ref, remote_refs)) + continue; + + if (!data->refspecs) + continue; + + /* propagate back the update to the remote namespace */ + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); + if (!private) + continue; + update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0); + free(private); } strbuf_release(&buf); } -- cgit v1.2.1 From 126aac5cf3b5696481aafe840f8f596653087d8b Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Fri, 10 May 2013 07:08:30 -0500 Subject: transport-helper: fix remote helper namespace regression Commit 664059f (transport-helper: update remote helper namespace) updates the namespace when the push succeeds or not; we should do it only when it succeeded. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'transport-helper.c') diff --git a/transport-helper.c b/transport-helper.c index 92174095ed..6cd0be90e0 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -701,7 +701,7 @@ static int push_update_ref_status(struct strbuf *buf, (*ref)->status = status; (*ref)->remote_status = msg; - return 0; + return !(status == REF_STATUS_OK); } static void push_update_refs_status(struct helper_data *data, -- cgit v1.2.1