From 912072d53a6f257fbbfd8a103e14248ae2382b6d Mon Sep 17 00:00:00 2001 From: Kacper Kornet Date: Tue, 21 Aug 2012 09:46:06 +0200 Subject: t6300: test sort with multiple keys Documentation of git-for-each-ref says that --sort= option can be used multiple times, in which case the last key becomes the primary key. However this functionality was never checked in test suite and is currently broken. This commit adds appropriate test in preparation for fix. Signed-off-by: Kacper Kornet Signed-off-by: Junio C Hamano --- t/t6300-for-each-ref.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 172178490..a0d82d493 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -456,4 +456,14 @@ test_atom refs/tags/signed-long contents "subject line body contents $sig" +cat >expected <<\EOF +408fe76d02a785a006c2e9c669b7be5589ede96d refs/tags/master +90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo +EOF + +test_expect_failure 'Verify sort with multiple keys' ' + git for-each-ref --format="%(objectname) %(taggeremail) %(refname)" --sort=objectname --sort=taggeremail \ + refs/tags/bogo refs/tags/master > actual && + test_cmp expected actual +' test_done -- cgit v1.2.1 From 3b51222ceceed022f45193db19c57cf53f0164df Mon Sep 17 00:00:00 2001 From: Kacper Kornet Date: Tue, 21 Aug 2012 09:47:26 +0200 Subject: for-each-ref: Fix sort with multiple keys The linked list describing sort options was not correctly set up in opt_parse_sort. In the result, contrary to the documentation, only the last of multiple --sort options to git-for-each-ref was taken into account. This commit fixes it. Signed-off-by: Kacper Kornet Signed-off-by: Junio C Hamano --- builtin/for-each-ref.c | 4 +++- t/t6300-for-each-ref.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index b01d76a24..0c5294e5e 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -962,7 +962,9 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset) if (!arg) /* should --no-sort void the list ? */ return -1; - *sort_tail = s = xcalloc(1, sizeof(*s)); + s = xcalloc(1, sizeof(*s)); + s->next = *sort_tail; + *sort_tail = s; if (*arg == '-') { s->reverse = 1; diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index a0d82d493..752f5cb7d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -461,7 +461,7 @@ cat >expected <<\EOF 90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo EOF -test_expect_failure 'Verify sort with multiple keys' ' +test_expect_success 'Verify sort with multiple keys' ' git for-each-ref --format="%(objectname) %(taggeremail) %(refname)" --sort=objectname --sort=taggeremail \ refs/tags/bogo refs/tags/master > actual && test_cmp expected actual -- cgit v1.2.1 From 003c84f6d2b9e9c4d5bbf5262cae994bac7190cb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 2 May 2011 13:39:16 -0700 Subject: specifying ranges: we did not mean to make ".." an empty set Either end of revision range operator can be omitted to default to HEAD, as in "origin.." (what did I do since I forked) or "..origin" (what did they do since I forked). But the current parser interprets ".." as an empty range "HEAD..HEAD", and worse yet, because ".." does exist on the filesystem, we get this annoying output: $ cd Documentation/howto $ git log .. ;# give me recent commits that touch Documentation/ area. fatal: ambiguous argument '..': both revision and filename Use '--' to separate filenames from revisions Surely we could say "git log ../" or even "git log -- .." to disambiguate, but we shouldn't have to. Helped-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/revisions.txt | 7 +++++++ builtin/rev-parse.c | 16 ++++++++++++++-- revision.c | 16 ++++++++++++++-- t/t1506-rev-parse-diagnosis.sh | 14 ++++++++++++++ t/t4202-log.sh | 7 +++++++ 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index dc0070bcb..69d996bc3 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -213,6 +213,13 @@ of 'r1' and 'r2' and is defined as It is the set of commits that are reachable from either one of 'r1' or 'r2' but not from both. +In these two shorthands, you can omit one end and let it default to HEAD. +For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What +did I do since I forked from the origin branch?" Similarly, '..origin' +is a shorthand for 'HEAD..origin' and asks "What did the origin do since +I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an +empty range that is both reachable and unreachable from HEAD. + Two other shorthands for naming a set that is formed by a commit and its parent commits exist. The 'r1{caret}@' notation means all parents of 'r1'. 'r1{caret}!' includes commit 'r1' but excludes diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 13495b88f..47b4e7adb 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -224,6 +224,7 @@ static int try_difference(const char *arg) const char *next; const char *this; int symmetric; + static const char head_by_default[] = "HEAD"; if (!(dotdot = strstr(arg, ".."))) return 0; @@ -235,9 +236,20 @@ static int try_difference(const char *arg) next += symmetric; if (!*next) - next = "HEAD"; + next = head_by_default; if (dotdot == arg) - this = "HEAD"; + this = head_by_default; + + if (this == head_by_default && next == head_by_default && + !symmetric) { + /* + * Just ".."? That is not a range but the + * pathspec for the parent directory. + */ + *dotdot = '.'; + return 0; + } + if (!get_sha1(this, sha1) && !get_sha1(next, end)) { show_rev(NORMAL, end, next); show_rev(symmetric ? NORMAL : REVERSED, sha1, this); diff --git a/revision.c b/revision.c index 5b81a92e3..457868d64 100644 --- a/revision.c +++ b/revision.c @@ -1132,15 +1132,27 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, const char *this = arg; int symmetric = *next == '.'; unsigned int flags_exclude = flags ^ UNINTERESTING; + static const char head_by_default[] = "HEAD"; unsigned int a_flags; *dotdot = 0; next += symmetric; if (!*next) - next = "HEAD"; + next = head_by_default; if (dotdot == arg) - this = "HEAD"; + this = head_by_default; + if (this == head_by_default && next == head_by_default && + !symmetric) { + /* + * Just ".."? That is not a range but the + * pathspec for the parent directory. + */ + if (!cant_be_filename) { + *dotdot = '.'; + return -1; + } + } if (!get_sha1(this, from_sha1) && !get_sha1(next, sha1)) { struct commit *a, *b; diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh index c5cb77a0e..f950c1012 100755 --- a/t/t1506-rev-parse-diagnosis.sh +++ b/t/t1506-rev-parse-diagnosis.sh @@ -182,4 +182,18 @@ test_expect_success ':file correctly diagnosed after a pathname' ' test_cmp expect actual ' +test_expect_success 'dotdot is not an empty set' ' + ( H=$(git rev-parse HEAD) && echo $H && echo ^$H ) >expect && + + git rev-parse HEAD.. >actual && + test_cmp expect actual && + + git rev-parse ..HEAD >actual && + test_cmp expect actual && + + echo .. >expect && + git rev-parse .. >actual && + test_cmp expect actual +' + test_done diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 71be59d44..45058cc8c 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -806,4 +806,11 @@ test_expect_success 'log --graph with diff and stats' ' test_cmp expect actual.sanitized ' +test_expect_success 'dotdot is a parent directory' ' + mkdir -p a/b && + ( echo sixth && echo fifth ) >expect && + ( cd a/b && git log --format=%s .. ) >actual && + test_cmp expect actual +' + test_done -- cgit v1.2.1 From 6a2abdc12516cd3801bceef4ccfba399c962a074 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 24 Aug 2012 22:48:55 -0700 Subject: apply: compute patch->def_name correctly under -p0 Back when "git apply" was written, we made sure that the user can skip more than the default number of path components (i.e. 1) by giving "-p", but the logic for doing so was built around the notion of "we skip N slashes and stop". This obviously does not work well when running under -p0 where we do not want to skip any, but still want to skip SP/HT that separates the pathnames of preimage and postimage and want to reject absolute pathnames. Stop using "stop_at_slash()", and instead introduce a new helper "skip_tree_prefix()" with similar logic but works correctly even for the -p0 case. This is an ancient bug, but has been masked for a long time because most of the patches are text and have other clues to tell us the name of the preimage and the postimage. Noticed by Colin McCabe. Signed-off-by: Junio C Hamano --- builtin/apply.c | 68 +++++++++++++++++++++++++++++++------------------ t/t4103-apply-binary.sh | 54 ++++++++++++++++++++++++--------------- 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index c24dc546d..2ad8c4820 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1022,15 +1022,23 @@ static int gitdiff_unrecognized(const char *line, struct patch *patch) return -1; } -static const char *stop_at_slash(const char *line, int llen) +/* + * Skip p_value leading components from "line"; as we do not accept + * absolute paths, return NULL in that case. + */ +static const char *skip_tree_prefix(const char *line, int llen) { - int nslash = p_value; + int nslash; int i; + if (!p_value) + return (llen && line[0] == '/') ? NULL : line; + + nslash = p_value; for (i = 0; i < llen; i++) { int ch = line[i]; if (ch == '/' && --nslash <= 0) - return &line[i]; + return (i == 0) ? NULL : &line[i + 1]; } return NULL; } @@ -1060,12 +1068,11 @@ static char *git_header_name(char *line, int llen) if (unquote_c_style(&first, line, &second)) goto free_and_fail1; - /* advance to the first slash */ - cp = stop_at_slash(first.buf, first.len); - /* we do not accept absolute paths */ - if (!cp || cp == first.buf) + /* strip the a/b prefix including trailing slash */ + cp = skip_tree_prefix(first.buf, first.len); + if (!cp) goto free_and_fail1; - strbuf_remove(&first, 0, cp + 1 - first.buf); + strbuf_remove(&first, 0, cp - first.buf); /* * second points at one past closing dq of name. @@ -1079,22 +1086,21 @@ static char *git_header_name(char *line, int llen) if (*second == '"') { if (unquote_c_style(&sp, second, NULL)) goto free_and_fail1; - cp = stop_at_slash(sp.buf, sp.len); - if (!cp || cp == sp.buf) + cp = skip_tree_prefix(sp.buf, sp.len); + if (!cp) goto free_and_fail1; /* They must match, otherwise ignore */ - if (strcmp(cp + 1, first.buf)) + if (strcmp(cp, first.buf)) goto free_and_fail1; strbuf_release(&sp); return strbuf_detach(&first, NULL); } /* unquoted second */ - cp = stop_at_slash(second, line + llen - second); - if (!cp || cp == second) + cp = skip_tree_prefix(second, line + llen - second); + if (!cp) goto free_and_fail1; - cp++; - if (line + llen - cp != first.len + 1 || + if (line + llen - cp != first.len || memcmp(first.buf, cp, first.len)) goto free_and_fail1; return strbuf_detach(&first, NULL); @@ -1106,10 +1112,9 @@ static char *git_header_name(char *line, int llen) } /* unquoted first name */ - name = stop_at_slash(line, llen); - if (!name || name == line) + name = skip_tree_prefix(line, llen); + if (!name) return NULL; - name++; /* * since the first name is unquoted, a dq if exists must be @@ -1123,10 +1128,9 @@ static char *git_header_name(char *line, int llen) if (unquote_c_style(&sp, second, NULL)) goto free_and_fail2; - np = stop_at_slash(sp.buf, sp.len); - if (!np || np == sp.buf) + np = skip_tree_prefix(sp.buf, sp.len); + if (!np) goto free_and_fail2; - np++; len = sp.buf + sp.len - np; if (len < second - name && @@ -1158,13 +1162,27 @@ static char *git_header_name(char *line, int llen) case '\n': return NULL; case '\t': case ' ': - second = stop_at_slash(name + len, line_len - len); + /* + * Is this the separator between the preimage + * and the postimage pathname? Again, we are + * only interested in the case where there is + * no rename, as this is only to set def_name + * and a rename patch has the names elsewhere + * in an unambiguous form. + */ + if (!name[len + 1]) + return NULL; /* no postimage name */ + second = skip_tree_prefix(name + len + 1, + line_len - (len + 1)); if (!second) return NULL; - second++; - if (second[len] == '\n' && !strncmp(name, second, len)) { + /* + * Does len bytes starting at "name" and "second" + * (that are separated by one HT or SP we just + * found) exactly match? + */ + if (second[len] == '\n' && !strncmp(name, second, len)) return xmemdupz(name, len); - } } } } diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh index dbbf56cba..1b420e3b5 100755 --- a/t/t4103-apply-binary.sh +++ b/t/t4103-apply-binary.sh @@ -8,30 +8,28 @@ test_description='git apply handling binary patches ' . ./test-lib.sh -# setup - -cat >file1 <file2 -cat file1 >file4 - -test_expect_success 'setup' " +test_expect_success 'setup' ' + cat >file1 <<-\EOF && + A quick brown fox jumps over the lazy dog. + A tiny little penguin runs around in circles. + There is a flag with Linux written on it. + A slow black-and-white panda just sits there, + munching on his bamboo. + EOF + cat file1 >file2 && + cat file1 >file4 && + git update-index --add --remove file1 file2 file4 && - git commit -m 'Initial Version' 2>/dev/null && + git commit -m "Initial Version" 2>/dev/null && git checkout -b binary && - perl -pe 'y/x/\000/' file3 && + perl -pe "y/x/\000/" file3 && cat file3 >file4 && git add file2 && - perl -pe 'y/\000/v/' file1 && + perl -pe "y/\000/v/" file1 && rm -f file2 && git update-index --add --remove file1 file2 file3 file4 && - git commit -m 'Second Version' && + git commit -m "Second Version" && git diff-tree -p master binary >B.diff && git diff-tree -p -C master binary >C.diff && @@ -42,17 +40,25 @@ test_expect_success 'setup' " git diff-tree -p --full-index master binary >B-index.diff && git diff-tree -p -C --full-index master binary >C-index.diff && + git diff-tree -p --binary --no-prefix master binary -- file3 >B0.diff && + git init other-repo && - (cd other-repo && - git fetch .. master && - git reset --hard FETCH_HEAD + ( + cd other-repo && + git fetch .. master && + git reset --hard FETCH_HEAD ) -" +' test_expect_success 'stat binary diff -- should not fail.' \ 'git checkout master && git apply --stat --summary B.diff' +test_expect_success 'stat binary -p0 diff -- should not fail.' ' + git checkout master && + git apply --stat -p0 B0.diff +' + test_expect_success 'stat binary diff (copy) -- should not fail.' \ 'git checkout master && git apply --stat --summary C.diff' @@ -143,4 +149,10 @@ test_expect_success 'apply binary diff (copy).' \ git apply --allow-binary-replacement --index CF.diff && test -z "$(git diff --name-status binary)"' +test_expect_success 'apply binary -p0 diff' ' + do_reset && + git apply -p0 --index B0.diff && + test -z "$(git diff --name-status binary -- file3)" +' + test_done -- cgit v1.2.1 From 45aaf0310f2b85d9c3264d1703c82cd5085aaf24 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 26 Aug 2012 11:40:08 -0700 Subject: doc: "git checkout -b/-B/--orphan" always takes a branch name While the synopsis section makes it clear that the new branch name is the parameter to these flags, the option description did not. Signed-off-by: Junio C Hamano --- Documentation/git-checkout.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index c0a96e6c1..e3270cdbc 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -84,11 +84,11 @@ entries; instead, unmerged entries are ignored. When checking out paths from the index, check out stage #2 ('ours') or #3 ('theirs') for unmerged paths. --b:: +-b :: Create a new branch named and start it at ; see linkgit:git-branch[1] for details. --B:: +-B :: Creates the branch and start it at ; if it already exists, then reset it to . This is equivalent to running "git branch" with "-f"; see @@ -124,7 +124,7 @@ explicitly give a name with '-b' in such a case. is not a branch name. See the "DETACHED HEAD" section below for details. ---orphan:: +--orphan :: Create a new 'orphan' branch, named , started from and switch to it. The first commit made on this new branch will have no parents and it will be the root of a new -- cgit v1.2.1 From 726800a8b3bf3702490a79f0205e33b2a0dbbb64 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:23:37 -0400 Subject: t5550: put auth-required repo in auth/dumb In most of our tests, we put repos to be accessed by dumb protocols in /dumb, and repos to be accessed by smart protocols in /smart. In our test apache setup, the whole /auth hierarchy requires authentication. However, we don't bother to split it by smart and dumb here because we are not currently testing smart-http authentication at all. That will change in future patches, so let's be explicit that we are interested in testing dumb access here. This also happens to match what t5540 does for the push tests. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5550-http-fetch.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index b06f817af..5ad21233e 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -41,9 +41,9 @@ test_expect_success 'clone http repository' ' ' test_expect_success 'create password-protected repository' ' - mkdir "$HTTPD_DOCUMENT_ROOT_PATH/auth/" && + mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" && cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ - "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git" + "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" ' test_expect_success 'setup askpass helpers' ' @@ -81,28 +81,28 @@ expect_askpass() { test_expect_success 'cloning password-protected repository can fail' ' >askpass-query && echo wrong >askpass-response && - test_must_fail git clone "$HTTPD_URL/auth/repo.git" clone-auth-fail && + test_must_fail git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-fail && expect_askpass both wrong ' test_expect_success 'http auth can use user/pass in URL' ' >askpass-query && echo wrong >askpass-response && - git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none && + git clone "$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none && expect_askpass none ' test_expect_success 'http auth can use just user in URL' ' >askpass-query && echo user@host >askpass-response && - git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass && + git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass && expect_askpass pass user@host ' test_expect_success 'http auth can request both user and pass' ' >askpass-query && echo user@host >askpass-response && - git clone "$HTTPD_URL/auth/repo.git" clone-auth-both && + git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both && expect_askpass both user@host ' @@ -114,7 +114,7 @@ test_expect_success 'http auth respects credential helper config' ' }; f" && >askpass-query && echo wrong >askpass-response && - git clone "$HTTPD_URL/auth/repo.git" clone-auth-helper && + git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-helper && expect_askpass none ' @@ -122,7 +122,7 @@ test_expect_success 'http auth can get username from config' ' test_config_global "credential.$HTTPD_URL.username" user@host && >askpass-query && echo user@host >askpass-response && - git clone "$HTTPD_URL/auth/repo.git" clone-auth-user && + git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user && expect_askpass pass user@host ' @@ -130,7 +130,7 @@ test_expect_success 'configured username does not override URL' ' test_config_global "credential.$HTTPD_URL.username" wrong && >askpass-query && echo user@host >askpass-response && - git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-user2 && + git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 && expect_askpass pass user@host ' -- cgit v1.2.1 From e837936c7c08ac5829bc53761ecb57d18d458edd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:24:31 -0400 Subject: t5550: factor out http auth setup The t5550 script sets up a nice askpass helper for simulating user input and checking what git prompted for. Let's make it available to other http scripts by migrating it to lib-httpd. We can use this immediately in t5540 to make our tests more robust (previously, we did not check at all that hitting the password-protected repo actually involved a password). Unfortunately, we end up failing the test because the current code erroneously prompts twice (once for git-remote-http, and then again when the former spawns git-http-push). More importantly, though, it will let us easily add smart-http authentication tests in t5541 and t5551; we currently do not test smart-http authentication at all. As part of making it generic, let's always look for and store auxiliary askpass files at the top-level trash directory; this makes it compatible with t5540, which runs some tests from sub-repositories. We can abstract away the ugliness with a short helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/lib-httpd.sh | 39 +++++++++++++++++++++++++++++++++++++ t/t5540-http-push.sh | 17 ++++++++--------- t/t5550-http-fetch.sh | 53 ++++++++------------------------------------------- 3 files changed, 55 insertions(+), 54 deletions(-) diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 094d49089..0f31ef9be 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -163,3 +163,42 @@ test_http_push_nonff() { test_i18ngrep "Updates were rejected because" output ' } + +setup_askpass_helper() { + test_expect_success 'setup askpass helper' ' + write_script "$TRASH_DIRECTORY/askpass" <<-\EOF && + echo >>"$TRASH_DIRECTORY/askpass-query" "askpass: $*" && + cat "$TRASH_DIRECTORY/askpass-response" + EOF + GIT_ASKPASS="$TRASH_DIRECTORY/askpass" && + export GIT_ASKPASS && + export TRASH_DIRECTORY + ' +} + +set_askpass() { + >"$TRASH_DIRECTORY/askpass-query" && + echo "$*" >"$TRASH_DIRECTORY/askpass-response" +} + +expect_askpass() { + dest=$HTTPD_DEST + { + case "$1" in + none) + ;; + pass) + echo "askpass: Password for 'http://$2@$dest': " + ;; + both) + echo "askpass: Username for 'http://$dest': " + echo "askpass: Password for 'http://$2@$dest': " + ;; + *) + false + ;; + esac + } >"$TRASH_DIRECTORY/askpass-expect" && + test_cmp "$TRASH_DIRECTORY/askpass-expect" \ + "$TRASH_DIRECTORY/askpass-query" +} diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh index 1eea64765..f141f2d1d 100755 --- a/t/t5540-http-push.sh +++ b/t/t5540-http-push.sh @@ -46,15 +46,7 @@ test_expect_success 'create password-protected repository' ' "$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 -' +setup_askpass_helper test_expect_success 'clone remote repository' ' cd "$ROOT_PATH" && @@ -162,6 +154,7 @@ test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \ test_expect_success 'push to password-protected repository (user in URL)' ' test_commit pw-user && + set_askpass user@host && 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" \ @@ -169,9 +162,15 @@ test_expect_success 'push to password-protected repository (user in URL)' ' test_cmp expect actual ' +test_expect_failure 'user was prompted only once for password' ' + expect_askpass pass user@host +' + test_expect_failure 'push to password-protected repository (no user in URL)' ' test_commit pw-nouser && + set_askpass user@host && git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD && + expect_askpass both user@host git rev-parse --verify HEAD >expect && git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \ rev-parse --verify HEAD >actual && diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index 5ad21233e..16ef0419e 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -46,62 +46,28 @@ test_expect_success 'create password-protected repository' ' "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" ' -test_expect_success 'setup askpass helpers' ' - cat >askpass <<-EOF && - #!/bin/sh - echo >>"$PWD/askpass-query" "askpass: \$*" && - cat "$PWD/askpass-response" - EOF - chmod +x askpass && - GIT_ASKPASS="$PWD/askpass" && - export GIT_ASKPASS -' - -expect_askpass() { - dest=$HTTPD_DEST - { - case "$1" in - none) - ;; - pass) - echo "askpass: Password for 'http://$2@$dest': " - ;; - both) - echo "askpass: Username for 'http://$dest': " - echo "askpass: Password for 'http://$2@$dest': " - ;; - *) - false - ;; - esac - } >askpass-expect && - test_cmp askpass-expect askpass-query -} +setup_askpass_helper test_expect_success 'cloning password-protected repository can fail' ' - >askpass-query && - echo wrong >askpass-response && + set_askpass wrong && test_must_fail git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-fail && expect_askpass both wrong ' test_expect_success 'http auth can use user/pass in URL' ' - >askpass-query && - echo wrong >askpass-response && + set_askpass wrong && git clone "$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none && expect_askpass none ' test_expect_success 'http auth can use just user in URL' ' - >askpass-query && - echo user@host >askpass-response && + set_askpass user@host && git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass && expect_askpass pass user@host ' test_expect_success 'http auth can request both user and pass' ' - >askpass-query && - echo user@host >askpass-response && + set_askpass user@host && git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both && expect_askpass both user@host ' @@ -112,24 +78,21 @@ test_expect_success 'http auth respects credential helper config' ' echo username=user@host echo password=user@host }; f" && - >askpass-query && - echo wrong >askpass-response && + set_askpass wrong && git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-helper && expect_askpass none ' test_expect_success 'http auth can get username from config' ' test_config_global "credential.$HTTPD_URL.username" user@host && - >askpass-query && - echo user@host >askpass-response && + set_askpass user@host && git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user && expect_askpass pass user@host ' test_expect_success 'configured username does not override URL' ' test_config_global "credential.$HTTPD_URL.username" wrong && - >askpass-query && - echo user@host >askpass-response && + set_askpass user@host && git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 && expect_askpass pass user@host ' -- cgit v1.2.1 From 05b577107dda131d46f93aa9bb7817c80bc30ee9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:24:42 -0400 Subject: t/lib-httpd: only route auth/dumb to dumb repos Our test apache config points all of auth/ directly to the on-disk repositories via an Alias directive. This works fine because everything authenticated is currently in auth/dumb, which is a subset. However, this would conflict with a ScriptAlias for auth/smart (which will come in future patches), so let's narrow the Alias. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/lib-httpd/apache.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index de3762e24..b183e3513 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -43,7 +43,7 @@ ErrorLog error.log Alias /dumb/ www/ -Alias /auth/ www/auth/ +Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} -- cgit v1.2.1 From 666aae9aed5a29019d2cd696d4258750c0dc96c7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:25:21 -0400 Subject: t/lib-httpd: recognize */smart/* repos as smart-http We do not currently test authentication for smart-http repos at all. Part of the infrastructure to do this is recognizing that auth/smart is indeed a smart-http repo. The current apache config recognizes only "^/smart/*" as smart-http. Let's instead treat anything with /smart/ in the URL as smart-http. This is obviously a stupid thing to do for a real production site, but for our test suite we know that our repositories will not have this magic string in the name. Note that we will route /foo/smart/bar.git directly to git-http-backend/bar.git; in other words, everything before the "/smart/" is irrelevant to finding the repo on disk (but may impact apache config, for example by triggering auth checks). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/lib-httpd/apache.conf | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index b183e3513..616486f9e 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -45,22 +45,20 @@ ErrorLog error.log Alias /dumb/ www/ Alias /auth/dumb/ www/auth/dumb/ - + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL - - + + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} - - + + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_COMMITTER_NAME "Custom User" SetEnv GIT_COMMITTER_EMAIL custom@example.com - -ScriptAlias /smart/ ${GIT_EXEC_PATH}/git-http-backend/ -ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/ -ScriptAlias /smart_custom_env/ ${GIT_EXEC_PATH}/git-http-backend/ + +ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 Options None -- cgit v1.2.1 From 6ac2b3aeb9900a8fb0cd3fd9be0bff00eb3a4b5b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:25:36 -0400 Subject: t: test basic smart-http authentication We do not currently test authentication over smart-http at all. In theory, it should work exactly as it does for dumb http (which we do test). It does indeed work for these simple tests, but this patch lays the groundwork for more complex tests in future patches. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5541-http-push.sh | 14 ++++++++++++++ t/t5551-http-fetch.sh | 11 +++++++++++ 2 files changed, 25 insertions(+) diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh index 312e48409..eeb993203 100755 --- a/t/t5541-http-push.sh +++ b/t/t5541-http-push.sh @@ -36,6 +36,8 @@ test_expect_success 'setup remote repository' ' mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH" ' +setup_askpass_helper + cat >exp <expect && + test_commit push-auth-test && + set_askpass user@host && + git push "$HTTPD_URL"/auth/smart/test_repo.git && + git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \ + log -1 --format=%s >actual && + expect_askpass both user@host && + test_cmp expect actual +' + stop_httpd test_done diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh index be6094be7..342d6af86 100755 --- a/t/t5551-http-fetch.sh +++ b/t/t5551-http-fetch.sh @@ -27,6 +27,8 @@ test_expect_success 'create http-accessible bare repository' ' git push public master:master ' +setup_askpass_helper + cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* @@ -109,6 +111,15 @@ test_expect_success 'follow redirects (302)' ' git clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t ' +test_expect_success 'clone from password-protected repository' ' + echo two >expect && + set_askpass user@host && + git clone --bare "$HTTPD_URL/auth/smart/repo.git" smart-auth && + expect_askpass both user@host && + git --git-dir=smart-auth log -1 --format=%s >actual && + test_cmp expect actual +' + test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE test_expect_success EXPENSIVE 'create 50,000 tags in the repo' ' -- cgit v1.2.1 From 4c71009da60baee436358e84ff1057cd1c80e776 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:25:53 -0400 Subject: t: test http access to "half-auth" repositories Some sites set up http access to repositories such that fetching is anonymous and unauthenticated, but pushing is authenticated. While there are multiple ways to do this, the technique advertised in the git-http-backend manpage is to block access to locations matching "/git-receive-pack$". Let's emulate that advice in our test setup, which makes it clear that this advice does not actually work. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/lib-httpd/apache.conf | 7 +++++++ t/t5541-http-push.sh | 12 ++++++++++++ t/t5551-http-fetch.sh | 9 +++++++++ 3 files changed, 28 insertions(+) diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 616486f9e..ec8618dfd 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -89,6 +89,13 @@ SSLEngine On Require valid-user + + AuthType Basic + AuthName "git-auth" + AuthUserFile passwd + Require valid-user + + LoadModule dav_module modules/mod_dav.so LoadModule dav_fs_module modules/mod_dav_fs.so diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh index eeb993203..9b1cd603c 100755 --- a/t/t5541-http-push.sh +++ b/t/t5541-http-push.sh @@ -280,5 +280,17 @@ test_expect_success 'push over smart http with auth' ' test_cmp expect actual ' +test_expect_failure 'push to auth-only-for-push repo' ' + cd "$ROOT_PATH/test_repo_clone" && + echo push-half-auth >expect && + test_commit push-half-auth && + set_askpass user@host && + git push "$HTTPD_URL"/auth-push/smart/test_repo.git && + git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \ + log -1 --format=%s >actual && + expect_askpass both user@host && + test_cmp expect actual +' + stop_httpd test_done diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh index 342d6af86..7380f2a2d 100755 --- a/t/t5551-http-fetch.sh +++ b/t/t5551-http-fetch.sh @@ -120,6 +120,15 @@ test_expect_success 'clone from password-protected repository' ' test_cmp expect actual ' +test_expect_success 'clone from auth-only-for-push repository' ' + echo two >expect && + set_askpass wrong && + git clone --bare "$HTTPD_URL/auth-push/smart/repo.git" smart-noauth && + expect_askpass none && + git --git-dir=smart-noauth log -1 --format=%s >actual && + test_cmp expect actual +' + test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE test_expect_success EXPENSIVE 'create 50,000 tags in the repo' ' -- cgit v1.2.1 From 88097030725bf68d1801559cfb4785b93a50f5f8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:26:04 -0400 Subject: http: factor out http error code handling Most of our http requests go through the http_request() interface, which does some nice post-processing on the results. In particular, it handles prompting for missing credentials as well as approving and rejecting valid or invalid credentials. Unfortunately, it only handles GET requests. Making it handle POSTs would be quite complex, so let's pull result handling code into its own function so that it can be reused from the POST code paths. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 51 ++++++++++++++++++++++++++++----------------------- http.h | 1 + 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/http.c b/http.c index 2ec37891f..7c4a4072f 100644 --- a/http.c +++ b/http.c @@ -744,6 +744,33 @@ char *get_remote_object_url(const char *url, const char *hex, return strbuf_detach(&buf, NULL); } +int handle_curl_result(struct active_request_slot *slot) +{ + struct slot_results *results = slot->results; + + if (results->curl_result == CURLE_OK) { + credential_approve(&http_auth); + return HTTP_OK; + } else if (missing_target(results)) + return HTTP_MISSING_TARGET; + else if (results->http_code == 401) { + if (http_auth.username && http_auth.password) { + credential_reject(&http_auth); + return HTTP_NOAUTH; + } else { + credential_fill(&http_auth); + init_curl_http_auth(slot->curl); + return HTTP_REAUTH; + } + } else { + if (!curl_errorstr[0]) + strlcpy(curl_errorstr, + curl_easy_strerror(results->curl_result), + sizeof(curl_errorstr)); + return HTTP_ERROR; + } +} + /* http_request() targets */ #define HTTP_REQUEST_STRBUF 0 #define HTTP_REQUEST_FILE 1 @@ -791,26 +818,7 @@ static int http_request(const char *url, void *result, int target, int options) if (start_active_slot(slot)) { run_active_slot(slot); - if (results.curl_result == CURLE_OK) - ret = HTTP_OK; - else if (missing_target(&results)) - ret = HTTP_MISSING_TARGET; - else if (results.http_code == 401) { - if (http_auth.username && http_auth.password) { - credential_reject(&http_auth); - ret = HTTP_NOAUTH; - } else { - credential_fill(&http_auth); - init_curl_http_auth(slot->curl); - ret = HTTP_REAUTH; - } - } else { - if (!curl_errorstr[0]) - strlcpy(curl_errorstr, - curl_easy_strerror(results.curl_result), - sizeof(curl_errorstr)); - ret = HTTP_ERROR; - } + ret = handle_curl_result(slot); } else { error("Unable to start HTTP request for %s", url); ret = HTTP_START_FAILED; @@ -819,9 +827,6 @@ static int http_request(const char *url, void *result, int target, int options) curl_slist_free_all(headers); strbuf_release(&buf); - if (ret == HTTP_OK) - credential_approve(&http_auth); - return ret; } diff --git a/http.h b/http.h index 915c2862a..12de25597 100644 --- a/http.h +++ b/http.h @@ -78,6 +78,7 @@ extern int start_active_slot(struct active_request_slot *slot); extern void run_active_slot(struct active_request_slot *slot); extern void finish_active_slot(struct active_request_slot *slot); extern void finish_all_active_slots(void); +extern int handle_curl_result(struct active_request_slot *slot); #ifdef USE_CURL_MULTI extern void fill_active_slots(void); -- cgit v1.2.1 From b81401c1de0e0fec39f8643ce7a794fda083f7a1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 27 Aug 2012 09:27:15 -0400 Subject: http: prompt for credentials on failed POST All of the smart-http GET requests go through the http_get_* functions, which will prompt for credentials and retry if we see an HTTP 401. POST requests, however, do not go through any central point. Moreover, it is difficult to retry in the general case; we cannot assume the request body fits in memory or is even seekable, and we don't know how much of it was consumed during the attempt. Most of the time, this is not a big deal; for both fetching and pushing, we make a GET request before doing any POSTs, so typically we figure out the credentials during the first request, then reuse them during the POST. However, some servers may allow a client to get the list of refs from receive-pack without authentication, and then require authentication when the client actually tries to POST the pack. This is not ideal, as the client may do a non-trivial amount of work to generate the pack (e.g., delta-compressing objects). However, for a long time it has been the recommended example configuration in git-http-backend(1) for setting up a repository with anonymous fetch and authenticated push. This setup has always been broken without putting a username into the URL. Prior to commit 986bbc0, it did work with a username in the URL, because git would prompt for credentials before making any requests at all. However, post-986bbc0, it is totally broken. Since it has been advertised in the manpage for some time, we should make sure it works. Unfortunately, it is not as easy as simply calling post_rpc again when it fails, due to the input issue mentioned above. However, we can still make this specific case work by retrying in two specific instances: 1. If the request is large (bigger than LARGE_PACKET_MAX), we will first send a probe request with a single flush packet. Since this request is static, we can freely retry it. 2. If the request is small and we are not using gzip, then we have the whole thing in-core, and we can freely retry. That means we will not retry in some instances, including: 1. If we are using gzip. However, we only do so when calling git-upload-pack, so it does not apply to pushes. 2. If we have a large request, the probe succeeds, but then the real POST wants authentication. This is an extremely unlikely configuration and not worth worrying about. While it might be nice to cover those instances, doing so would be significantly more complex for very little real-world gain. In the long run, we will be much better off when curl learns to internally handle authentication as a callback, and we can cleanly handle all cases that way. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 23 +++++++++++++++-------- t/t5541-http-push.sh | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 04a9d6277..3ec474fc6 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -362,16 +362,17 @@ static size_t rpc_in(char *ptr, size_t eltsize, static int run_slot(struct active_request_slot *slot) { - int err = 0; + int err; struct slot_results results; slot->results = &results; slot->curl_result = curl_easy_perform(slot->curl); finish_active_slot(slot); - if (results.curl_result != CURLE_OK) { - err |= error("RPC failed; result=%d, HTTP code = %ld", - results.curl_result, results.http_code); + err = handle_curl_result(slot); + if (err != HTTP_OK && err != HTTP_REAUTH) { + error("RPC failed; result=%d, HTTP code = %ld", + results.curl_result, results.http_code); } return err; @@ -436,9 +437,11 @@ static int post_rpc(struct rpc_state *rpc) } if (large_request) { - err = probe_rpc(rpc); - if (err) - return err; + do { + err = probe_rpc(rpc); + } while (err == HTTP_REAUTH); + if (err != HTTP_OK) + return -1; } slot = get_active_slot(); @@ -525,7 +528,11 @@ static int post_rpc(struct rpc_state *rpc) curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc); - err = run_slot(slot); + do { + err = run_slot(slot); + } while (err == HTTP_REAUTH && !large_request && !use_gzip); + if (err != HTTP_OK) + err = -1; curl_slist_free_all(headers); free(gzip_body); diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh index 9b1cd603c..ef6d6b6e4 100755 --- a/t/t5541-http-push.sh +++ b/t/t5541-http-push.sh @@ -280,7 +280,7 @@ test_expect_success 'push over smart http with auth' ' test_cmp expect actual ' -test_expect_failure 'push to auth-only-for-push repo' ' +test_expect_success 'push to auth-only-for-push repo' ' cd "$ROOT_PATH/test_repo_clone" && echo push-half-auth >expect && test_commit push-half-auth && -- cgit v1.2.1