Merge branch 'js/transport-helper-error-reporting-fix' into fc/makefile
authorJunio C Hamano <gitster@pobox.com>
Fri, 7 Jun 2013 23:15:32 +0000 (16:15 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 7 Jun 2013 23:15:32 +0000 (16:15 -0700)
* js/transport-helper-error-reporting-fix:
git-remote-testgit: build it to run under $SHELL_PATH
git-remote-testgit: further remove some bashisms
git-remote-testgit: avoid process substitution
t5801: "VAR=VAL shell_func args" is forbidden
transport-helper: update remote helper namespace
transport-helper: trivial code shuffle
transport-helper: warn when refspec is not used
transport-helper: clarify pushing without refspecs
transport-helper: update refspec documentation
transport-helper: clarify *:* refspec
transport-helper: improve push messages
transport-helper: mention helper name when it dies
transport-helper: report errors properly

Conflicts:
t/t5801-remote-helpers.sh

.gitignore
Documentation/gitremote-helpers.txt
Makefile
git-remote-testgit [deleted file]
git-remote-testgit.sh [new file with mode: 0755]
t/t5801-remote-helpers.sh
transport-helper.c
index 6669bf0c6c9a0b42f74e2ed189350a6a9f11f17d..10aee94760bca452935ac5c0dfc337a0f6b58a30 100644 (file)
 /git-remote-ftps
 /git-remote-fd
 /git-remote-ext
+/git-remote-testgit
 /git-remote-testpy
 /git-remote-testsvn
 /git-repack
index da746419b355a4b5d49b23ed51e3d4aa89d8c2c1..0827f691396ba698d3976eba1eddedd9e7ff3e9e 100644 (file)
@@ -159,11 +159,11 @@ Miscellaneous capabilities
        carried out.
 
 'refspec' <refspec>::
-       This modifies the 'import' capability, allowing the produced
-       fast-import stream to modify refs in a private namespace
-       instead of writing to refs/heads or refs/remotes directly.
+       For remote helpers that implement 'import' or 'export', this capability
+       allows the refs to be constrained to a private namespace, instead of
+       writing to refs/heads or refs/remotes directly.
        It is recommended that all importers providing the 'import'
-       capability use this.
+       capability use this. It's mandatory for 'export'.
 +
 A helper advertising the capability
 `refspec refs/heads/*:refs/svn/origin/branches/*`
@@ -174,8 +174,8 @@ ref.
 This capability can be advertised multiple times.  The first
 applicable refspec takes precedence.  The left-hand of refspecs
 advertised with this capability must cover all refs reported by
-the list command.  If a helper does not need a specific 'refspec'
-capability then it should advertise `refspec *:*`.
+the list command.  If no 'refspec' capability is advertised,
+there is an implied `refspec *:*`.
 
 'bidi-import'::
        This modifies the 'import' capability.
index ff0f527b52ab84b2581e85e714e021d2c7ec8307..e9ac1229e8ab902785430252ca8f93c758770279 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -460,6 +460,7 @@ SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-repack.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
diff --git a/git-remote-testgit b/git-remote-testgit
deleted file mode 100755 (executable)
index e7ed3a3..0000000
+++ /dev/null
@@ -1,91 +0,0 @@
-#!/usr/bin/env bash
-# Copyright (c) 2012 Felipe Contreras
-
-alias=$1
-url=$2
-
-dir="$GIT_DIR/testgit/$alias"
-prefix="refs/testgit/$alias"
-
-default_refspec="refs/heads/*:${prefix}/heads/*"
-
-refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}"
-
-test -z "$refspec" && prefix="refs"
-
-export GIT_DIR="$url/.git"
-
-mkdir -p "$dir"
-
-if test -z "$GIT_REMOTE_TESTGIT_NO_MARKS"
-then
-       gitmarks="$dir/git.marks"
-       testgitmarks="$dir/testgit.marks"
-       test -e "$gitmarks" || >"$gitmarks"
-       test -e "$testgitmarks" || >"$testgitmarks"
-       testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
-fi
-
-while read line
-do
-       case $line in
-       capabilities)
-               echo 'import'
-               echo 'export'
-               test -n "$refspec" && echo "refspec $refspec"
-               if test -n "$gitmarks"
-               then
-                       echo "*import-marks $gitmarks"
-                       echo "*export-marks $gitmarks"
-               fi
-               test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
-               echo
-               ;;
-       list)
-               git for-each-ref --format='? %(refname)' 'refs/heads/'
-               head=$(git symbolic-ref HEAD)
-               echo "@$head HEAD"
-               echo
-               ;;
-       import*)
-               # read all import lines
-               while true
-               do
-                       ref="${line#* }"
-                       refs="$refs $ref"
-                       read line
-                       test "${line%% *}" != "import" && break
-               done
-
-               if test -n "$gitmarks"
-               then
-                       echo "feature import-marks=$gitmarks"
-                       echo "feature export-marks=$gitmarks"
-               fi
-               echo "feature done"
-               git fast-export "${testgitmarks_args[@]}" $refs |
-               sed -e "s#refs/heads/#${prefix}/heads/#g"
-               echo "done"
-               ;;
-       export)
-               before=$(git for-each-ref --format='%(refname) %(objectname)')
-
-               git fast-import "${testgitmarks_args[@]}" --quiet
-
-               after=$(git for-each-ref --format='%(refname) %(objectname)')
-
-               # figure out which refs were updated
-               join -e 0 -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo "$after") |
-               while read ref a b
-               do
-                       test $a == $b && continue
-                       echo "ok $ref"
-               done
-
-               echo
-               ;;
-       '')
-               exit
-               ;;
-       esac
-done
diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
new file mode 100755 (executable)
index 0000000..e83315f
--- /dev/null
@@ -0,0 +1,116 @@
+#!/bin/sh
+# Copyright (c) 2012 Felipe Contreras
+
+alias=$1
+url=$2
+
+dir="$GIT_DIR/testgit/$alias"
+prefix="refs/testgit/$alias"
+
+default_refspec="refs/heads/*:${prefix}/heads/*"
+
+refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}"
+
+test -z "$refspec" && prefix="refs"
+
+export GIT_DIR="$url/.git"
+
+mkdir -p "$dir"
+
+if test -z "$GIT_REMOTE_TESTGIT_NO_MARKS"
+then
+       gitmarks="$dir/git.marks"
+       testgitmarks="$dir/testgit.marks"
+       test -e "$gitmarks" || >"$gitmarks"
+       test -e "$testgitmarks" || >"$testgitmarks"
+fi
+
+while read line
+do
+       case $line in
+       capabilities)
+               echo 'import'
+               echo 'export'
+               test -n "$refspec" && echo "refspec $refspec"
+               if test -n "$gitmarks"
+               then
+                       echo "*import-marks $gitmarks"
+                       echo "*export-marks $gitmarks"
+               fi
+               test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
+               echo
+               ;;
+       list)
+               git for-each-ref --format='? %(refname)' 'refs/heads/'
+               head=$(git symbolic-ref HEAD)
+               echo "@$head HEAD"
+               echo
+               ;;
+       import*)
+               # read all import lines
+               while true
+               do
+                       ref="${line#* }"
+                       refs="$refs $ref"
+                       read line
+                       test "${line%% *}" != "import" && break
+               done
+
+               if test -n "$gitmarks"
+               then
+                       echo "feature import-marks=$gitmarks"
+                       echo "feature export-marks=$gitmarks"
+               fi
+
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       echo "feature done"
+                       exit 1
+               fi
+
+               echo "feature done"
+               git fast-export \
+                       ${testgitmarks:+"--import-marks=$testgitmarks"} \
+                       ${testgitmarks:+"--export-marks=$testgitmarks"} \
+                       $refs |
+               sed -e "s#refs/heads/#${prefix}/heads/#g"
+               echo "done"
+               ;;
+       export)
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;
+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks
+                       while read line; do
+                               test "done" = "$line" && break
+                       done
+                       exit 1
+               fi
+
+               before=$(git for-each-ref --format=' %(refname) %(objectname) ')
+
+               git fast-import \
+                       ${testgitmarks:+"--import-marks=$testgitmarks"} \
+                       ${testgitmarks:+"--export-marks=$testgitmarks"} \
+                       --quiet
+
+               # figure out which refs were updated
+               git for-each-ref --format='%(refname) %(objectname)' |
+               while read ref a
+               do
+                       case "$before" in
+                       *" $ref $a "*)
+                               continue ;;     # unchanged
+                       esac
+                       echo "ok $ref"
+               done
+
+               echo
+               ;;
+       '')
+               exit
+               ;;
+       esac
+done
index dbb02e2afd16b2ca53f18107f5f07af37f217c9b..61479c3b16841c13798486590c56f265d821176c 100755 (executable)
@@ -8,11 +8,6 @@ test_description='Test remote-helper import and export commands'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-if ! type "${BASH-bash}" >/dev/null 2>&1; then
-       skip_all='skipping remote-testgit tests, bash not available'
-       test_done
-fi
-
 compare_refs() {
        git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
        git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
@@ -101,39 +96,28 @@ test_expect_failure 'push new branch with old:new refspec' '
 
 test_expect_success 'cloning without refspec' '
        GIT_REMOTE_TESTGIT_REFSPEC="" \
-       git clone "testgit::${PWD}/server" local2 &&
+       git clone "testgit::${PWD}/server" local2 2>error &&
+       grep "This remote helper should implement refspec capability" error &&
        compare_refs local2 HEAD server HEAD
 '
 
 test_expect_success 'pulling without refspecs' '
        (cd local2 &&
        git reset --hard &&
-       GIT_REMOTE_TESTGIT_REFSPEC="" git pull) &&
+       GIT_REMOTE_TESTGIT_REFSPEC="" git pull 2>../error) &&
+       grep "This remote helper should implement refspec capability" error &&
        compare_refs local2 HEAD server HEAD
 '
 
-test_expect_failure 'pushing without refspecs' '
+test_expect_success 'pushing without refspecs' '
        test_when_finished "(cd local2 && git reset --hard origin)" &&
        (cd local2 &&
        echo content >>file &&
        git commit -a -m ten &&
-       GIT_REMOTE_TESTGIT_REFSPEC="" git push) &&
-       compare_refs local2 HEAD server HEAD
-'
-
-test_expect_success 'pulling with straight refspec' '
-       (cd local2 &&
-       GIT_REMOTE_TESTGIT_REFSPEC="*:*" git pull) &&
-       compare_refs local2 HEAD server HEAD
-'
-
-test_expect_failure 'pushing with straight refspec' '
-       test_when_finished "(cd local2 && git reset --hard origin)" &&
-       (cd local2 &&
-       echo content >>file &&
-       git commit -a -m eleven &&
-       GIT_REMOTE_TESTGIT_REFSPEC="*:*" git push) &&
-       compare_refs local2 HEAD server HEAD
+       GIT_REMOTE_TESTGIT_REFSPEC="" &&
+       export GIT_REMOTE_TESTGIT_REFSPEC &&
+       test_must_fail git push 2>../error) &&
+       grep "remote-helper doesn.t support push; refspec needed" error
 '
 
 test_expect_success 'pulling without marks' '
@@ -186,6 +170,38 @@ test_expect_success GPG 'push signed tag with signed-tags capability' '
        compare_refs local signed-tag-2 server signed-tag-2
 '
 
+test_expect_success 'push update refs' '
+       (cd local &&
+       git checkout -b update master &&
+       echo update >>file &&
+       git commit -a -m update &&
+       git push origin update
+       git rev-parse --verify remotes/origin/update >expect &&
+       git rev-parse --verify testgit/origin/heads/update >actual &&
+       test_cmp expect actual
+       )
+'
+
+test_expect_success 'proper failure checks for fetching' '
+       (GIT_REMOTE_TESTGIT_FAILURE=1 &&
+       export GIT_REMOTE_TESTGIT_FAILURE &&
+       cd local &&
+       test_must_fail git fetch 2> error &&
+       cat error &&
+       grep -q "Error while running fast-import" error
+       )
+'
+
+test_expect_success 'proper failure checks for pushing' '
+       (GIT_REMOTE_TESTGIT_FAILURE=1 &&
+       export GIT_REMOTE_TESTGIT_FAILURE &&
+       cd local &&
+       test_must_fail git push --all 2> error &&
+       cat error &&
+       grep -q "Reading from helper .git-remote-testgit. failed" error
+       )
+'
+
 test_expect_success 'push messages' '
        (cd local &&
        git checkout -b new_branch master &&
index 522d79178e2cc4909a5df21fe483946fc3d1ad0e..f11d78a55a675ea2ea6dca0818ef796ae7fc74e3 100644 (file)
@@ -11,6 +11,7 @@
 #include "thread-utils.h"
 #include "sigchain.h"
 #include "argv-array.h"
+#include "refs.h"
 
 static int debug;
 
@@ -47,7 +48,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)
@@ -55,7 +56,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 helper 'git-remote-%s' failed", name);
        }
 
        if (debug)
@@ -65,7 +66,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)
@@ -217,6 +218,8 @@ static struct child_process *get_helper(struct transport *transport)
                for (i = 0; i < refspec_nr; i++)
                        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)
@@ -473,7 +476,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
@@ -540,7 +543,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)
@@ -622,7 +625,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)
 {
@@ -688,7 +691,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) {
@@ -697,11 +700,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,
@@ -710,11 +714,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);
 }
@@ -789,6 +806,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");
@@ -799,8 +819,9 @@ static int push_refs_with_export(struct transport *transport,
                char *private;
                unsigned char sha1[20];
 
-               if (!data->refspecs)
-                       continue;
+               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);
@@ -809,13 +830,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))