Re: [PATCH] vcs-svn: Fix 'fa/remote-svn' and 'fa/vcs-svn' in pu
Hi! Thanks for your fixups. I'm currently integrating them in a new series. On what platform did you find that problems? Tried to reproduce them on 64bit Linux. Anyways the fixes look very reasonable. Florian On Thursday 23 August 2012 18:55:39 Ramsay Jones wrote: Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Florian, The build on pu is currently broken: CC remote-testsvn.o LINK git-remote-testsvn cc: vcs-svn/lib.a: No such file or directory make: *** [git-remote-testsvn] Error 1 This is caused by a dependency missing from the git-remote-testsvn link rule. The addition of the $(VCSSVN_LIB) dependency, which should be squashed into commit ea1f4afb (Add git-remote-testsvn to Makefile, 20-08-2012), fixes the build. However, this leads to a failure of test t9020.5 and (not unrelated) compiler warnings: CC vcs-svn/svndump.o vcs-svn/svndump.c: In function ‘handle_node’: vcs-svn/svndump.c:246: warning: left shift count = width of type vcs-svn/svndump.c:345: warning: format ‘%lu’ expects type ‘long \ unsigned int’, but argument 3 has type ‘uintmax_t’ The fix for the shift count warning is to cast the lhs of the shift expression to uintmax_t. The format warning is fixed by using the PRIuMAX format macro. These fixes should be squashed into commit 78d9d4138 (vcs-svn/svndump: rewrite handle_node(), begin|end_revision(), 20-08-2012). HTH ATB, Ramsay Jones Makefile | 2 +- vcs-svn/svndump.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 9cede84..761ae05 100644 --- a/Makefile +++ b/Makefile @@ -2356,7 +2356,7 @@ git-http-push$X: revision.o http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ $(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) -git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) +git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \ $(VCSSVN_LIB) diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c index 28ce2aa..eb97e8e 100644 --- a/vcs-svn/svndump.c +++ b/vcs-svn/svndump.c @@ -243,7 +243,7 @@ static void handle_node(struct node_ctx_t *node) const char *old_data = NULL; uint32_t old_mode = REPO_MODE_BLB; struct strbuf sb = STRBUF_INIT; - static uintmax_t blobmark = 1UL (bitsizeof(uintmax_t) - 1); + static uintmax_t blobmark = (uintmax_t) 1UL (bitsizeof(uintmax_t) - 1); if (have_text type == REPO_MODE_DIR) @@ -342,7 +342,7 @@ static void handle_node(struct node_ctx_t *node) node-text_length, input); } - strbuf_addf(sb, :%lu, blobmark); + strbuf_addf(sb, :%PRIuMAX, blobmark); node-dataref = sb.buf; } } -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git no longer prompting for password
On 26/08/12 10:57, Iain Paton wrote:
If %{THE_REQUEST} =~ /git-receive-pack/
I've just discovered that the If .. directive only appears in apache 2.4
so something more generic will probably be a better idea. Not everyone will
be running 2.4.x for a while yet.
Iain
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] Name local variables more consistently
On 08/26/2012 07:39 PM, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
From: Michael Haggerty mhag...@alum.mit.edu
Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).
I think this is fine, although:
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
cutoff)
}
}
-static void filter_refs(struct ref **refs, int nr_match, char **match)
+static void filter_refs(struct ref **refs, int nr_heads, char **heads)
{
struct ref **return_refs;
struct ref *newlist = NULL;
@@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
nr_match, char **match)
struct ref *fastarray[32];
int match_pos;
This match_pos is an index into the match array, which becomes head.
Should it become head_pos?
And then bits like this:
- while (match_pos nr_match) {
- cmp = strcmp(ref-name, match[match_pos]);
+ while (match_pos nr_heads) {
+ cmp = strcmp(ref-name, heads[match_pos]);
Would be:
while (head_pos nr_heads)
which makes more sense to me.
Using one name is better, but I wonder heads is the better one
between the two.
After all, this codepath is not limited to branches these days as
the word head implies. Rather, nr_thing, thing has what we
asked for, and refs has what the other sides have, and we are
trying to make sure we haven't asked what they do not have in this
function.
And the way we do so is to match the things with what are in
refs to find unmatched ones.
So between the two, I would have chosen match instead of heads
to call the thing.
When I decided between heads and match, my main consideration was
that match sounds like something that has already been matched, not
something that is being matched against. The word match also implies
to me that some nontrivial matching process is going on, like glob
expansion.
But I agree with you that heads also has disadvantages.
What about a third option: refnames? This name makes it clear that we
are talking about simple names as opposed to struct ref or some kind
of refname glob patterns and also makes it clear that they are not
necessarily all branches. It would also be distinct from the refs
linked list that is often used in the same functions.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] Name local variables more consistently
On Mon, Aug 27, 2012 at 11:22:36AM +0200, Michael Haggerty wrote: Using one name is better, but I wonder heads is the better one between the two. After all, this codepath is not limited to branches these days as the word head implies. Rather, nr_thing, thing has what we asked for, and refs has what the other sides have, and we are trying to make sure we haven't asked what they do not have in this function. And the way we do so is to match the things with what are in refs to find unmatched ones. So between the two, I would have chosen match instead of heads to call the thing. When I decided between heads and match, my main consideration was that match sounds like something that has already been matched, not something that is being matched against. The word match also implies to me that some nontrivial matching process is going on, like glob expansion. But I agree with you that heads also has disadvantages. What about a third option: refnames? This name makes it clear that we are talking about simple names as opposed to struct ref or some kind of refname glob patterns and also makes it clear that they are not necessarily all branches. It would also be distinct from the refs linked list that is often used in the same functions. Yeah, I agree that refnames would be better. I think something like spec or refspec would indicate better that they are to be matched against, but then you run afoul of confusing that with colon-delimited refspecs (which I do not think fetch-pack understands at all). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v4 02/13] read-cache.c: Re-read index if index file changed
On 08/25, Joachim Schmitz wrote: Thomas Gummerer t.gumme...@gmail.com schrieb im Newsbeitrag news:134529-6925-3-git-send-email-t.gumme...@gmail.com... [...] + usleep(10*1000); usleep() is not available to anybody, e.g. it is not in HP NonStop (not in every case at least) Bye, Jojo Thanks for noticing, will be fixed in the re-roll. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git checkout -t -B
On Mon, Aug 27, 2012 at 1:38 AM, Junio C Hamano gits...@pobox.com wrote: 乙酸鋰 ch3co...@gmail.com writes: git checkout -t -B origin/abcde works but git checkout -B -t origin/abcde does not. Could you document the order of parameters or fix the behaviour? It is crystal clear that -b/-B/--orphan must be followed by the name of the branch you are creating from the SYNOPSIS section of the documentation. Yet it's not very clear from the error message: fatal: git checkout: updating paths is incompatible with switching branches. Did you intend to checkout 'origin/abcde' which can not be resolved as commit? I wonder if we should reject -t as a value of -[Bb] by adding new parseopt flag to reject values starting with '-'. In this case, branch names can't start with '-'. There may be cases where we accept option value starting with '-', but I suspect the other way is dominant. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/8] fix password prompting for half-auth servers
On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote: No problem. I'll probably be a day or two on the patches, as the http tests are in need of some refactoring before adding more tests. But in the meantime, I think your config change is a sane work-around. OK, here is the series. For those just joining us, the problem is that git will not correctly prompt for credentials when pushing to a repository which allows the initial GET of .../info/refs?service=git-receive-pack, but then gives a 401 when we try to POST the pack. This has never worked for a plain URL, but used to work if you put the username in the URL (because we would unconditionally load the credentials before making any requests). That was broken by 986bbc0, which does not do that proactive prompting for smart-http, meaning such repositories cannot be pushed to at all. Such a server-side setup is questionable in my opinion (because the client will actually create the pack before failing), but we have been advertising it for a long time in git-http-backend(1) as the right way to make repositories that are anonymous for fetching but require auth for pushing. The fix is somewhat uglier than I would like, but I think it's practical and the right thing to do (see the final patch for lots of discussion). I built this on the current tip of master. It might make sense to backport it directly on top of 986bbc0 for the maint track. There are conflicts, but they are all textual. Another option would be to revert 986bbc0 for the maint track, as that commit is itself fixing a minor bug that is of decreasing relevance (it fixed extra password prompting when .netrc was in use, but one can work around it by dropping the username from the URL). The patches are: [1/8]: t5550: put auth-required repo in auth/dumb [2/8]: t5550: factor out http auth setup [3/8]: t/lib-httpd: only route auth/dumb to dumb repos [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http [5/8]: t: test basic smart-http authentication These are all refactoring of the test scripts in preparation for 6/8 (and are where all of the conflicts lie). [6/8]: t: test http access to half-auth repositories This demonstrates the bug. [7/8]: http: factor out http error code handling Refactoring to support 8/8. [8/8]: http: prompt for credentials on failed POST And this one is the actual fix. I'd like to have a 9/8 which tweaks the git-http-backend documentation to provide better example apache config, but I haven't yet figured out the right incantation. Suggestions from apache gurus are welcome. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] 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 p...@peff.net
---
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 b06f817..5ad2123 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
'
--
1.7.11.5.10.g3c8125b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] 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 p...@peff.net
---
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 36b1596..d13fe64 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -46,7 +46,7 @@ PassEnv GIT_VALGRIND
PassEnv GIT_VALGRIND_OPTIONS
Alias /dumb/ www/
-Alias /auth/ www/auth/
+Alias /auth/dumb/ www/auth/dumb/
Location /smart/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
--
1.7.11.5.10.g3c8125b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/8] 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 p...@peff.net
---
Another backporting gotcha: the smart_custom_env bits did not exist back
in the v1.7.8 era.
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 d13fe64..c6a1a87 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -48,22 +48,20 @@ PassEnv GIT_VALGRIND_OPTIONS
Alias /dumb/ www/
Alias /auth/dumb/ www/auth/dumb/
-Location /smart/
+LocationMatch /smart/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
-/Location
-Location /smart_noexport/
+/LocationMatch
+LocationMatch /smart_noexport/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
-/Location
-Location /smart_custom_env/
+/LocationMatch
+LocationMatch /smart_custom_env/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
SetEnv GIT_COMMITTER_NAME Custom User
SetEnv GIT_COMMITTER_EMAIL cus...@example.com
-/Location
-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/
+/LocationMatch
+ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
Directory ${GIT_EXEC_PATH}
Options FollowSymlinks
/Directory
--
1.7.11.5.10.g3c8125b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/8] 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 p...@peff.net --- 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 312e484..eeb9932 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 EOF GET /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200 POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200 @@ -266,5 +268,17 @@ test_expect_success 'http push respects GIT_COMMITTER_* in reflog' ' test_cmp expect actual ' +test_expect_success 'push over smart http with auth' ' + cd $ROOT_PATH/test_repo_clone + echo push-auth-test 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 91eaf53..e653ae3 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 EOF 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' ' -- 1.7.11.5.10.g3c8125b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/8] 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 p...@peff.net --- 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 c6a1a87..49d5d87 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -92,6 +92,13 @@ SSLEngine On Require valid-user /Location +LocationMatch ^/auth-push/.*/git-receive-pack$ + AuthType Basic + AuthName git-auth + AuthUserFile passwd + Require valid-user +/LocationMatch + IfDefine DAV 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 eeb9932..9b1cd60 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 e653ae3..2db5c35 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' ' -- 1.7.11.5.10.g3c8125b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] 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 p...@peff.net
---
http.c | 51 ---
http.h | 1 +
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/http.c b/http.c
index b61ac85..6793137 100644
--- a/http.c
+++ b/http.c
@@ -745,6 +745,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_STRBUF0
#define HTTP_REQUEST_FILE 1
@@ -792,26 +819,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;
@@ -820,9 +828,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 915c286..12de255 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);
--
1.7.11.5.10.g3c8125b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/8] 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 p...@peff.net
---
Sorry for the wordy explanation. I really tried to refactor this into a
nice single code path for making both GET and POST requests, but I think
there are just too many corner cases. Suggestions welcome if somebody
has a better idea of how to refactor it (preferably in the form of a
patch).
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 04a9d62..3ec474f 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 9b1cd60..ef6d6b6 100755
--- a/t/t5541-http-push.sh
+++
Re: git no longer prompting for password
On Aug 27, 2012, at 04:28 , Iain Paton wrote:
On 26/08/12 10:57, Iain Paton wrote:
If %{THE_REQUEST} =~ /git-receive-pack/
I've just discovered that the If .. directive only appears in apache 2.4
so something more generic will probably be a better idea. Not everyone will
be running 2.4.x for a while yet.
You could try something like this:
Location /git
# Require authentication for git push
RewriteCond %{QUERY_STRING} service=git-receive-pack
RewriteRule .* - [E=AUTHREQUIRED:yes]
Order Allow,Deny
Deny from env=AUTHREQUIRED
Allow from all
Satisfy Any
# Whatever auth rules you want ...
I haven't tested this specific example but it is based upon similar rules I use
on a 2.0 server to require auth when specific query parameters are present. In
my case, I have the Rewrite rules in the VirtualHost and the other directives
in the Directory being protected.
--
BJ
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
exporting a .git file ?
Hi, Is there anyway to get my git repository as a single file ? Many thanks in advance, Aaron -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
On 08/27/2012 05:32 PM, Aaron Gray wrote: Is there anyway to get my git repository as a single file ? What is the purpose? .git/ is a folder... -- RMA. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
On 27 August 2012 15:35, Mihamina Rakotomandimby miham...@rktmb.org wrote: On 08/27/2012 05:32 PM, Aaron Gray wrote: Is there anyway to get my git repository as a single file ? What is the purpose? .git/ is a folder... I realize that but I wanted to distribute it as a downloadable file without having to use a server. Aaron -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
On 27 August 2012 15:45, Dan Johnson computerdr...@gmail.com wrote: On Mon, Aug 27, 2012 at 10:32 AM, Aaron Gray aaronngray.li...@gmail.com wrote: Hi, Is there anyway to get my git repository as a single file ? You're probably looking for the git bundle command (see git bundle --help), but it's possible you might just want to use tar. Hope that helps, Great thanks Dan, comes in under 2 megs, thats great. Aaron -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
On Mon, 27 Aug 2012 15:32:40 +0100 Aaron Gray aaronngray.li...@gmail.com wrote: Is there anyway to get my git repository as a single file ? Depends on what you really need. If you need to export the repository *history* of one or more references (branches or tags), use `git bundle` to create a specially-formatted file which can be imported using Git on another machine. If you need to just export the snapshot of the files maintained in the repository at the specified commit, use `git export`. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
Aaron Gray aaronngray.li...@gmail.com writes: I realize that but I wanted to distribute it as a downloadable file without having to use a server. git bundle can create a single file from the object database. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exporting a .git file ?
On 08/27/2012 04:45 PM, Dan Johnson wrote: On Mon, Aug 27, 2012 at 10:32 AM, Aaron Gray aaronngray.li...@gmail.com wrote: Hi, Is there anyway to get my git repository as a single file ? You're probably looking for the git bundle command (see git bundle --help), but it's possible you might just want to use tar. Please note that a git bundle will contain the contents of the repository (commits, log messages, etc) but not the local configuration like .git/config, .git/info/*, nor some other information like the reflogs (history of old values of references). This is probably what you want. A tar of the .git directory, by contrast, would include this local information unless you exclude it explicitly. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY
Jiang Xin worldhello@gmail.com writes:
Some testcases will fail if current work directory is on a symlink.
symlink$ sh ./t4035-diff-quiet.sh
$ sh ./t4035-diff-quiet.sh --root=/symlink
$ TEST_OUTPUT_DIRECTORY=/symlink sh ./t4035-diff-quiet.sh
This is because the realpath of .git directory will be returned when
running the command 'git rev-parse --git-dir' in a subdir of the work
tree, and the realpath may not equal to $TRASH_DIRECTORY.
In this fix, $TRASH_DIRECTORY is determined right after the realpath
of CWD is resolved.
Signed-off-by: Jiang Xin worldhello@gmail.com
Reported-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jiang Xin worldhello@gmail.com
---
I think this is in line with what was discussed in the other thread
Michael brought this up. Thanks for following it through.
Michael, this looks good to me; anything I missed?
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 78c42..9a59ca8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -531,17 +531,17 @@ fi
test=trash directory.$(basename $0 .sh)
test -n $root test=$root/$test
case $test in
-/*) TRASH_DIRECTORY=$test ;;
- *) TRASH_DIRECTORY=$TEST_OUTPUT_DIRECTORY/$test ;;
+/*) ;;
+ *) test=$TEST_OUTPUT_DIRECTORY/$test ;;
esac
-test ! -z $debug || remove_trash=$TRASH_DIRECTORY
+test ! -z $debug || remove_trash=$test
rm -fr $test || {
GIT_EXIT_OK=t
echo 5 FATAL: Cannot prepare test area
exit 1
}
-HOME=$TRASH_DIRECTORY
+HOME=$test
export HOME
if test -z $TEST_NO_CREATE_REPO; then
@@ -552,6 +552,7 @@ fi
# Use -P to resolve symlinks in our working directory so that the cwd
# in subprocesses like git equals our $PWD (for pathname comparisons).
cd -P $test || exit 1
+TRASH_DIRECTORY=$(pwd)
this_test=${0##*/}
this_test=${this_test%%-*}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libgit2 status
Junio C Hamano gits...@pobox.com writes: And the last one should really be a longer term item. It is more important for its codebase to get mature and robust, and that can only happen by various projects and products (e.g. GitHub for Mac) using it to improve it. I do not think subtree (or anything in contrib/ for that matter) is part of the core stuff of git, and do not see a problem; such a move may help both subtree and libgit2. Over a much longer timeperiod, I wouldn't be surprised if some core stuff gets reimplemented on top of libgit2 and distributed as part of the git-core. I am hoping to move git-subtree into core once it performs a little better and I've fixed a couple of bugs. Will basing it on libgit2 delay that process significantly? Six months delay is no problem. 2 years would be problematic. I would be happy to be a guinea pig for libgit2 in order to improve it, but I don't want to significantly impact git-subtree's move to core. I'll have to figure out the right balance there given feedback. -Dave -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] Fix tests under GETTEXT_POISON on git-remote
Jiang Xin worldhello@gmail.com writes: Use i18n-specific test functions in test scripts for git-remote. This issue was was introduced in v1.7.10-233-gbb16d5: bb16d5 i18n: remote: mark strings for translation and been broken under GETTEXT_POISON=YesPlease since. Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- ... @@ -77,6 +75,14 @@ test_expect_success 'add another remote' ' ) ' +test_expect_success C_LOCALE_OUTPUT 'add another remote' ' +( + cd test + check_remote_track origin master side + check_remote_track second master side another +) +' This couldn't have possibly passed with the trailing , or am I missing something? There is already add another remote before this test that adds second remote. Is this test about add yet another remote, or is it checking the result of adding second that was done in the previous step? Will queue with an obvious fix-up with retitle, 'check tracking', or something. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] Name local variables more consistently
Michael Haggerty mhag...@alum.mit.edu writes: On 08/26/2012 07:39 PM, Junio C Hamano wrote: ... After all, this codepath is not limited to branches these days as the word head implies. Rather, nr_thing, thing has what we asked for, and refs has what the other sides have, and we are trying to make sure we haven't asked what they do not have in this function. And the way we do so is to match the things with what are in refs to find unmatched ones. So between the two, I would have chosen match instead of heads to call the thing. When I decided between heads and match, my main consideration was that match sounds like something that has already been matched, not something that is being matched against. The word match also implies to me that some nontrivial matching process is going on, like glob expansion. But I agree with you that heads also has disadvantages. What about a third option: refnames? This name makes it clear that we are talking about simple names as opposed to struct ref or some kind of refname glob patterns and also makes it clear that they are not necessarily all branches. It would also be distinct from the refs linked list that is often used in the same functions. refnames has a similar issue as match, as you have pointed out, and more. Was it the remote or us who populated the refnames (answer: these are our refnames, and the other one refs is what they have)? What do we have that refnames for (answer: these specify what we are going to pick among what they have)? Perhaps asked? At the beginning of the processing, we have all that were asked for, and at the end, we leave what were asked for but were not fulfilled. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] Name local variables more consistently
Jeff King p...@peff.net writes: Yeah, I agree that refnames would be better. I think something like spec or refspec would indicate better that they are to be matched against, but then you run afoul of confusing that with colon-delimited refspecs (which I do not think fetch-pack understands at all). Correct. It only takes the LHS of the refspecs, following the one tool does one specific thing philosophy. The arrangement was that the calling script interpreted refspecs, split them into LHS and RHS, fed the LHS of the refspecs to fetch-pack, and then read the output to learn which local refs (i.e. RHS of the refspecs) to update with what value. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git checkout -t -B
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: On Mon, Aug 27, 2012 at 1:38 AM, Junio C Hamano gits...@pobox.com wrote: 乙酸鋰 ch3co...@gmail.com writes: git checkout -t -B origin/abcde works but git checkout -B -t origin/abcde does not. Could you document the order of parameters or fix the behaviour? It is crystal clear that -b/-B/--orphan must be followed by the name of the branch you are creating from the SYNOPSIS section of the documentation. Yet it's not very clear from the error message: fatal: git checkout: updating paths is incompatible with switching branches. Did you intend to checkout 'origin/abcde' which can not be resolved as commit? I wonder if we should reject -t as a value of -[Bb] by adding new parseopt flag to reject values starting with '-'. You should be able to cope with other invalid branch names in the same codepath, but your approach would not help at all if the user said git checkout -B q..f origin/abcde. Futzing with parseopt is not a reasonable answer to this one. Ideally you would want fatal: -t is not an acceptable name for a branch in this case; if it is cumbersome to arrange, at the very least, updating paths is incompatible with checking out the branch -t. would be clearer. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libgit2 status
d...@cray.com writes: I am hoping to move git-subtree into core once it performs a little better and I've fixed a couple of bugs. Will basing it on libgit2 delay that process significantly? Six months delay is no problem. 2 years would be problematic. I would be happy to be a guinea pig for libgit2 in order to improve it, but I don't want to significantly impact git-subtree's move to core. I'll have to figure out the right balance there given feedback. I expect it will take some time for libgit2 to allow our Makefile to start saying LDFLAGS += -libgit2; it will need to become as stable and widespread as other libraries we depend on, e.g. -lz and -lcurl. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/8] fix password prompting for half-auth servers
Jeff King p...@peff.net writes: (+cc: Shawn) On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote: No problem. I'll probably be a day or two on the patches, as the http tests are in need of some refactoring before adding more tests. But in the meantime, I think your config change is a sane work-around. OK, here is the series. For those just joining us, the problem is that git will not correctly prompt for credentials when pushing to a repository which allows the initial GET of .../info/refs?service=git-receive-pack, but then gives a 401 when we try to POST the pack. This has never worked for a plain URL, but used to work if you put the username in the URL (because we would unconditionally load the credentials before making any requests). That was broken by 986bbc0, which does not do that proactive prompting for smart-http, meaning such repositories cannot be pushed to at all. Such a server-side setup is questionable in my opinion (because the client will actually create the pack before failing), but we have been advertising it for a long time in git-http-backend(1) as the right way to make repositories that are anonymous for fetching but require auth for pushing. The fix is somewhat uglier than I would like, but I think it's practical and the right thing to do (see the final patch for lots of discussion). I built this on the current tip of master. It might make sense to backport it directly on top of 986bbc0 for the maint track. There are conflicts, but they are all textual. Another option would be to revert 986bbc0 for the maint track, as that commit is itself fixing a minor bug that is of decreasing relevance (it fixed extra password prompting when .netrc was in use, but one can work around it by dropping the username from the URL). The patches are: [1/8]: t5550: put auth-required repo in auth/dumb [2/8]: t5550: factor out http auth setup [3/8]: t/lib-httpd: only route auth/dumb to dumb repos [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http [5/8]: t: test basic smart-http authentication These are all refactoring of the test scripts in preparation for 6/8 (and are where all of the conflicts lie). [6/8]: t: test http access to half-auth repositories This demonstrates the bug. [7/8]: http: factor out http error code handling Refactoring to support 8/8. [8/8]: http: prompt for credentials on failed POST And this one is the actual fix. I'd like to have a 9/8 which tweaks the git-http-backend documentation to provide better example apache config, but I haven't yet figured out the right incantation. Suggestions from apache gurus are welcome. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] http: prompt for credentials on failed POST
Jeff King p...@peff.net writes:
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.
A silly question. Does the initial GET request when we push look
any different from the initial GET request when we fetch? Can we
make them look different in an updated client, so that the server
side can say this GET is about pushing into us, and we require
authentication?
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.
I suspect that in real life, almost nobody runs smart HTTP server
that allows anonymous push.
How much usability penalty would it be if we always fill credential
before pushing? Alternatively, how much latency penalty would it
incur if we always send a probe request regardless of the request
size when we try to push without having an authentication material?
Signed-off-by: Jeff King p...@peff.net
---
Sorry for the wordy explanation. I really tried to refactor this into a
nice single code path for making both GET and POST requests, but I think
there are just too many corner cases. Suggestions welcome if somebody
has a better idea of how to refactor it (preferably in the form of a
patch).
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 04a9d62..3ec474f 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 9b1cd60..ef6d6b6 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
Re: [PATCH 2/3] branch: add --unset-upstream option
c...@elego.de (Carlos Martín Nieto) writes:
Junio C Hamano gits...@pobox.com writes:
Carlos Martín Nieto c...@elego.de writes:
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..93e5d6e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a
particular branch' \
test $(git config branch.my13.remote) = .
test $(git config branch.my13.merge) = refs/heads/master'
+test_expect_success 'test --unset-upstream on HEAD' \
+'git branch my14
+ test_config branch.master.remote foo
+ test_config branch.master.merge foo
+ git branch --set-upstream-to my14
+ git branch --unset-upstream
+ test_must_fail git config branch.master.remote
+ test_must_fail git config branch.master.merge'
+
+test_expect_success 'test --unset-upstream on a particular branch' \
+'git branch my15
+ git branch --set-upstream-to master my14
+ git branch --unset-upstream my14
+ test_must_fail git config branch.my14.remote
+ test_must_fail git config branch.my14.merge'
+
What should happen when you say --unset-upstream on a branch B
that does not have B@{upstream}? Should it fail? Should it be
silently ignored? Is it undefined that we do not want to test?
I'd say it should be ignored, as the end result we want is for there to
be no upstream information. What we do underneath is remove a couple of
config options, wich doesn't fail if they didn't insist in the first
place.
I am not sure about that reasoning.
$ git config --unset core.nosuchvariable ; echo $?
5
looks like a failure to me.
More importantly, wouldn't we want to catch typo in
git branch --unset-upstream mext
which admittedly should come from a different codepath (I do not
think your patch checks if mext branch exists before going ahead
to the config--unset dance)?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] branch: add --unset-upstream option
Junio C Hamano gits...@pobox.com writes:
c...@elego.de (Carlos Martín Nieto) writes:
Junio C Hamano gits...@pobox.com writes:
Carlos Martín Nieto c...@elego.de writes:
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..93e5d6e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a
particular branch' \
test $(git config branch.my13.remote) = .
test $(git config branch.my13.merge) = refs/heads/master'
+test_expect_success 'test --unset-upstream on HEAD' \
+'git branch my14
+ test_config branch.master.remote foo
+ test_config branch.master.merge foo
+ git branch --set-upstream-to my14
+ git branch --unset-upstream
+ test_must_fail git config branch.master.remote
+ test_must_fail git config branch.master.merge'
+
+test_expect_success 'test --unset-upstream on a particular branch' \
+'git branch my15
+ git branch --set-upstream-to master my14
+ git branch --unset-upstream my14
+ test_must_fail git config branch.my14.remote
+ test_must_fail git config branch.my14.merge'
+
What should happen when you say --unset-upstream on a branch B
that does not have B@{upstream}? Should it fail? Should it be
silently ignored? Is it undefined that we do not want to test?
I'd say it should be ignored, as the end result we want is for there to
be no upstream information. What we do underneath is remove a couple of
config options, wich doesn't fail if they didn't insist in the first
place.
I am not sure about that reasoning.
$ git config --unset core.nosuchvariable ; echo $?
5
looks like a failure to me.
More importantly, wouldn't we want to catch typo in
git branch --unset-upstream mext
which admittedly should come from a different codepath (I do not
think your patch checks if mext branch exists before going ahead
to the config--unset dance)?
Sorry, the last paragraph was incomplete.
In general, we should detect errors and allow the user to choose to
ignore.
A script that wants to make sure that B does not have an upstream,
without knowing if it already has one, can say --unset-upstream B
and choose to ignore the error if B does not have an upstream.
If the script is carefully written, it would try to check if B has
one and call --unset-upstream B only when it doesn't. A casually
written loose script would say --unset-upstream B 2/dev/null
and keep going (it would not notice other kinds of errors, but that
is what makes it casual and loose).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
'git --grep' doesn't grep notes?
I have a note attached to a commit, but the text of the note isn't treated as part of the commit for the purposes of grep. For example: mkdir /tmp/foo cd /tmp/foo git init git commit --allow-empty -m 'Blank commit.' git notes add -m 'Find me!' git log --show-notes --regexp-ignore-case --grep=find This doesn't match anything. Expected behavior is that this will match the commit that contains Find me! in the notes. As an example use case, consider ticket integration, where you may want to add a ticket ID to a commit long after it's been pushed. You don't want to rewind the branch, or push a rebase, so a note seems like a sensible place to store a ticket ID. However, if the notes aren't part of the commit, this will break integration with tools like Pivotal Tracker that search the commits, and it will also require you to use an external grep and some custom parsing to find related commit IDs. I'm sure there are other use cases. This just happens to be mine right now. What is the right way to include notes in log searches, especially if the end goal is to find the related commit ID? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: synchronization with Subversion
Hello Tad, we maintain a central/main git repository which our developers clone from. We want to synchronize this central git repository with Subversion. I know this is not the recommended way to do this, but this was the choice that was made. The central git repository was originally cloned from Subversion as that was our migration path to git. Have you looked at SubGit, http://subgit.com/ ? SubGit is a bi-directional git-svn synchronization tool. It's basically a set of hooks that enable instant conversion of changes between Subversion and Git repositories. So, I think it'd work great for you. SubGit does not use git-svn internally, it's built on home-grown conversion engine. Most probably you have to re-translate your Subversion repository with SubGit in order to keep repositories synced. SubGit is a commercial closed-source product. But we provide free options for open-source and academic projects. It's also free for evaluation period. Semen Vadishev, TMate Software, http://subgit.com/ - git+svn on the server side! On 8/7/12 22:28, tad.man...@sita.aero wrote: Greetings, we maintain a central/main git repository which our developers clone from. We want to synchronize this central git repository with Subversion. I know this is not the recommended way to do this, but this was the choice that was made. The central git repository was originally cloned from Subversion as that was our migration path to git. Currently I can't get the synchronization to work again after another sprint. I get the following error message: Unable to determine upstream SVN information from HEAD history. Perhaps the repository is empty. at /usr/libexec/git-core/git-svn line 525. This synchronization has worked a number of times, but now it always fails with the above error. I have read that it's best to have a linear commit history when synchronizing to Subversion, and I've read that git rebase is the way to accomplish this. I've attempted this, but I run into two problems trying to do this: 1. Any files/directories which get moved/renamed cause the rebase to stop and I have to tell git to skip the commit, though it appears to me that the move/rename actually worked. I am confused by this behavior, and don't understand why it happens at all. 2. There are a number of conflicts which occur during the rebase. This also confuses me. I think I understand why they happen, but I'm not clear about how to handle them. Our code base goes back many years and contains a huge number of commits (originally in CVS, then migrated to Subversion and Git). It isn't obvious what impact the conflict resolution would have. My suspicion, is that it will breed even more conflicts as the rebase continues from that point. As you might have guessed, Subversion is the corporate mandated repository, which is why we are attempting to maintain the synchronization. We have a central git repository as we want to have more control over which changes are accepted. I'm hoping for some suggestions for dealing with this. Any takers? Thnks/Brgds --Tad --- Tad K. Mannes Senior Developer SITA - Societe Internationale de Telecommunications Aeronautiques -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libgit2 status
Junio C Hamano gits...@pobox.com writes: I would be happy to be a guinea pig for libgit2 in order to improve it, but I don't want to significantly impact git-subtree's move to core. I'll have to figure out the right balance there given feedback. I expect it will take some time for libgit2 to allow our Makefile to start saying LDFLAGS += -libgit2; it will need to become as stable and widespread as other libraries we depend on, e.g. -lz and -lcurl. Well that's a chicken-and-egg problem, isn't it. How will a library become widespread unless something uses it? Would it be enough to have libgit2 as an installable package in the major distributions? -Dave -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git --grep' doesn't grep notes?
Todd A. Jacobs nospam+listm...@codegnome.org writes: What is the right way to include notes in log searches, especially if the end goal is to find the related commit ID? You can git grep on refs/notes/commits and s,/,,g on the file name found. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 PATCH] Teach rm to remove submodules unless they contain a git directory
Currently using git rm on a submodule - populated or not - fails with
this error:
fatal: git rm: 'submodule path': Is a directory
This made sense in the past as there was no way to remove a submodule
without possibly removing unpushed parts of the submodule's history
contained in its .git directory too, so erroring out here protected the
user from possible loss of data.
But submodules cloned with a recent git version do not contain the .git
directory anymore, they use a gitfile to point to their git directory
which is safely stored inside the superproject's .git directory. The work
tree of these submodules can safely be removed without loosing history, so
let's teach git to do so.
Using rm on an unpopulated submodule now removes the empty directory from
the work tree and the gitlink from the index. If the submodule's directory
is missing from the work tree, it will still be removed from the index.
Using rm on a populated submodule using a gitfile will apply the usual
checks for work tree modification adapted to submodules (unless forced).
For a submodule that means that the HEAD is the same as recorded in the
index, no tracked files are modified and no untracked files that aren't
ignored are present in the submodules work tree (Ignored files are deemed
expendable and won't stop a submodule's work tree from being removed).
That logic has to be applied in all nested submodules too.
Using rm on a submodule which has its .git directory inside the work trees
top level directory will just error out like it did before, forced or not.
In the future git could either provide a message informing the user to
convert the submodule to use a gitfile or even attempt to do the
conversion itself, but that is not part of this change.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
This is the reroll of the Teach rm to better handle submodules series
($gmane/201015). It does not attempt to convert submodules that still
contain their git directory (by moving their git directory into
.git/modules/name and replacing it with a gitfile pointing there).
That will be subject to a future patch, as I'm not sure yet if git rm
should do that automagically or rather tell the user to use a (still
to be added) git submodule to-gitfile path invocation to achieve
that.
In a follow up patch I'll teach git rm submod/ to not barf about the
trailing '/', as that is added by TAB completion.
Documentation/git-rm.txt | 15
builtin/rm.c | 95 ++---
submodule.c | 80 ++
submodule.h | 2 +
t/t3600-rm.sh| 210 +++
5 files changed, 389 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 5d31860..3c76f9c 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -107,6 +107,21 @@ as well as modifications of existing paths.
Typically you would first remove all tracked files from the working
tree using this command:
+Submodules
+
+Only submodules using a gitfile (which means they were cloned
+with a git version 1.7.8 or newer) will be removed from the work
+tree, as their repository lives inside the .git directory of the
+superproject. If a submodule (or one of those nested inside it)
+still use a .git directory, `git rm` will fail - no matter if forced
+or not - to protect the submodules history.
+
+A submodule is considered up-to-date when the HEAD is the same as
+recorded in the index, no tracked files are modified and no untracked
+files that aren't ignored are present in the submodules work tree.
+Ignored files are deemed expendable and won't stop a submodule's work
+tree from being removed.
+
git ls-files -z | xargs -0 rm -f
diff --git a/builtin/rm.c b/builtin/rm.c
index 90c8a50..cb927a8 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -9,6 +9,7 @@
#include cache-tree.h
#include tree-walk.h
#include parse-options.h
+#include submodule.h
static const char * const builtin_rm_usage[] = {
git rm [options] [--] file...,
@@ -17,9 +18,43 @@ static const char * const builtin_rm_usage[] = {
static struct {
int nr, alloc;
- const char **name;
+ struct {
+ const char *name;
+ char is_submodule;
+ } *entry;
} list;
+static int check_submodules_use_gitfiles()
+{
+ int i;
+ int errs = 0;
+
+ for (i = 0; i list.nr; i++) {
+ const char *name = list.entry[i].name;
+ int pos;
+ struct cache_entry *ce;
+ struct stat st;
+
+ pos = cache_name_pos(name, strlen(name));
+ if (pos 0)
+ continue; /* ignore unmerged entry */
+ ce = active_cache[pos];
+
+ if (!S_ISGITLINK(ce-ce_mode) ||
+ (lstat(ce-name, st) 0) ||
+ is_empty_dir(name))
+
Re: libgit2 status
d...@cray.com writes: Junio C Hamano gits...@pobox.com writes: I would be happy to be a guinea pig for libgit2 in order to improve it, but I don't want to significantly impact git-subtree's move to core. I'll have to figure out the right balance there given feedback. I expect it will take some time for libgit2 to allow our Makefile to start saying LDFLAGS += -libgit2; it will need to become as stable and widespread as other libraries we depend on, e.g. -lz and -lcurl. Well that's a chicken-and-egg problem, isn't it. How will a library become widespread unless something uses it? That something will not be the git core itself. Otherwise we will lose a stable reference implementation to catch its bugs. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 PATCH] Teach rm to remove submodules unless they contain a git directory
Jens Lehmann jens.lehm...@web.de writes:
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 5d31860..3c76f9c 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -107,6 +107,21 @@ as well as modifications of existing paths.
Typically you would first remove all tracked files from the working
tree using this command:
+Submodules
+
You need to match the underline to the text if you want to make this
a heading.
diff --git a/builtin/rm.c b/builtin/rm.c
index 90c8a50..cb927a8 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -9,6 +9,7 @@
#include cache-tree.h
#include tree-walk.h
#include parse-options.h
+#include submodule.h
static const char * const builtin_rm_usage[] = {
git rm [options] [--] file...,
@@ -17,9 +18,43 @@ static const char * const builtin_rm_usage[] = {
static struct {
int nr, alloc;
- const char **name;
+ struct {
+ const char *name;
+ char is_submodule;
+ } *entry;
} list;
+static int check_submodules_use_gitfiles()
static int check_submodules_use_gitfiles(void)
+{
+ int i;
+ int errs = 0;
+
+ for (i = 0; i list.nr; i++) {
+ const char *name = list.entry[i].name;
+ int pos;
+ struct cache_entry *ce;
+ struct stat st;
+
+ pos = cache_name_pos(name, strlen(name));
+ if (pos 0)
+ continue; /* ignore unmerged entry */
Would this cause git rm -f path for an unmerged submodule bypass
the safety check?
With or without this patch, check_local_mod() will allow you to
remove unmerged entry and the file in the working tree, and that is
perfectly fine for a regular file or a symlink (as the path is
involved in a conflicted merge (or other mergy operation), and its
change from the HEAD can only come from that merge, because we would
not let merge touch a path and leave its index entry unmerged if the
path has local changes in the first place). Resolving the merge as
a removal at the index level for a submodule is also fine in such a
case, but don't you want to still keep the submodule working tree if
it has its sole copy of the repository? And as far as I can tell,
this function is the only thing that protects the user in such a
situation.
+ ce = active_cache[pos];
+
+ if (!S_ISGITLINK(ce-ce_mode) ||
+ (lstat(ce-name, st) 0) ||
+ is_empty_dir(name))
+ continue;
+
+ if (!submodule_uses_gitfile(name))
+ errs = error(_(submodule '%s' (or one of its nested
+ submodules) uses a .git directory\n
+ (use 'rm -rf' if you really want to
remove
+ it including all of its history)), name);
+ }
+
+ return errs;
+}
The call to this function comes at the very end and gives us yes/no
for the entire set of paths. After getting this error for one
submodule and bunch of other non-submodule paths, what is the
procedure for the user to remove it that we want to recommend in our
documentation? Would it go like this?
$ git rm path1 path2 sub path3
... get the above error ...
$ git submodule --to-gitfile sub
$ rm -fr sub
$ git rm sub
... then finally ...
$ git rm path1 path2 path3
This is not a complaint about the error message above, by the way.
@@ -37,7 +72,7 @@ static int check_local_mod(unsigned char *head, int
index_only)
struct stat st;
int pos;
struct cache_entry *ce;
- const char *name = list.name[i];
+ const char *name = list.entry[i].name;
unsigned char sha1[20];
unsigned mode;
int local_changes = 0;
@@ -58,9 +93,10 @@ static int check_local_mod(unsigned char *head, int
index_only)
/* if a file was removed and it is now a
* directory, that is the same as ENOENT as
* far as git is concerned; we do not track
- * directories.
+ * directories unless they are submodules.
*/
- continue;
+ if (!S_ISGITLINK(ce-ce_mode))
+ continue;
}
/*
@@ -80,8 +116,11 @@ static int check_local_mod(unsigned char *head, int
index_only)
/*
* Is the index different from the file in the work tree?
+ * If it's a submodule, is its work tree modified?
*/
- if (ce_match_stat(ce, st, 0))
+ if (ce_match_stat(ce, st, 0) ||
+ (S_ISGITLINK(ce-ce_mode)
+ !ok_to_remove_submodule(ce-name)))
Re: libgit2 status
Junio C Hamano gits...@pobox.com writes: Well that's a chicken-and-egg problem, isn't it. How will a library become widespread unless something uses it? That something will not be the git core itself. Otherwise we will lose a stable reference implementation to catch its bugs. Well, the whole question here is whether git-subtree can become part of core if it is based on libgit2. It boils down to what you mean by widespread, I guess. Does widespread mean available as a package in major distributions, installed by default in major distributions or something else? -Dave -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] Fix tests under GETTEXT_POISON on git-remote
2012/8/28 Junio C Hamano gits...@pobox.com:
Jiang Xin worldhello@gmail.com writes:
Use i18n-specific test functions in test scripts for git-remote.
This issue was was introduced in v1.7.10-233-gbb16d5:
bb16d5 i18n: remote: mark strings for translation
and been broken under GETTEXT_POISON=YesPlease since.
Signed-off-by: Jiang Xin worldhello@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
...
@@ -77,6 +75,14 @@ test_expect_success 'add another remote' '
)
'
+test_expect_success C_LOCALE_OUTPUT 'add another remote' '
+(
+ cd test
+ check_remote_track origin master side
+ check_remote_track second master side another
+)
+'
This couldn't have possibly passed with the trailing , or am I
missing something? There is already add another remote before
this test that adds second remote. Is this test about add
yet another remote, or is it checking the result of adding second
that was done in the previous step?
The trailing “ is a copy paste error. I only run my fixup in
GIT_GETTEXT_POISON mode in a harry, not noticed the bypassed
testcase has this serious bug.
I split the original add another remote into two blocks. One is a
normal testcase, and another has a C_LOCALE_OUTPUT prereq
flag. This is because other testcases depend on the operations in
add another remote testcase ('git remote add -f second ../two'),
and these testcases would fail if add C_LOCALE_OUTPUT
prereq to the whole add another remote testcase.
Will queue with an obvious fix-up with retitle, 'check tracking', or
something.
Thanks.
--
蒋鑫
北京群英汇信息技术有限公司
邮件: worldhello@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 010-51262007, 18601196889
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] branch: add --unset-upstream option
Junio C Hamano gits...@pobox.com writes:
Junio C Hamano gits...@pobox.com writes:
c...@elego.de (Carlos Martín Nieto) writes:
Junio C Hamano gits...@pobox.com writes:
Carlos Martín Nieto c...@elego.de writes:
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..93e5d6e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a
particular branch' \
test $(git config branch.my13.remote) = .
test $(git config branch.my13.merge) = refs/heads/master'
+test_expect_success 'test --unset-upstream on HEAD' \
+'git branch my14
+ test_config branch.master.remote foo
+ test_config branch.master.merge foo
+ git branch --set-upstream-to my14
+ git branch --unset-upstream
+ test_must_fail git config branch.master.remote
+ test_must_fail git config branch.master.merge'
+
+test_expect_success 'test --unset-upstream on a particular branch' \
+'git branch my15
+ git branch --set-upstream-to master my14
+ git branch --unset-upstream my14
+ test_must_fail git config branch.my14.remote
+ test_must_fail git config branch.my14.merge'
+
What should happen when you say --unset-upstream on a branch B
that does not have B@{upstream}? Should it fail? Should it be
silently ignored? Is it undefined that we do not want to test?
I'd say it should be ignored, as the end result we want is for there to
be no upstream information. What we do underneath is remove a couple of
config options, wich doesn't fail if they didn't insist in the first
place.
I am not sure about that reasoning.
$ git config --unset core.nosuchvariable ; echo $?
5
looks like a failure to me.
More importantly, wouldn't we want to catch typo in
git branch --unset-upstream mext
which admittedly should come from a different codepath (I do not
think your patch checks if mext branch exists before going ahead
to the config--unset dance)?
Sorry, the last paragraph was incomplete.
In general, we should detect errors and allow the user to choose to
ignore.
A script that wants to make sure that B does not have an upstream,
without knowing if it already has one, can say --unset-upstream B
and choose to ignore the error if B does not have an upstream.
If the script is carefully written, it would try to check if B has
one and call --unset-upstream B only when it doesn't. A casually
written loose script would say --unset-upstream B 2/dev/null
and keep going (it would not notice other kinds of errors, but that
is what makes it casual and loose).
OK, that's a good point, I'll update the patch.
cmn
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libgit2 status
The 27/08/12, Junio C Hamano wrote: d...@cray.com writes: Junio C Hamano gits...@pobox.com writes: I would be happy to be a guinea pig for libgit2 in order to improve it, but I don't want to significantly impact git-subtree's move to core. I'll have to figure out the right balance there given feedback. I expect it will take some time for libgit2 to allow our Makefile to start saying LDFLAGS += -libgit2; it will need to become as stable and widespread as other libraries we depend on, e.g. -lz and -lcurl. Well that's a chicken-and-egg problem, isn't it. How will a library become widespread unless something uses it? That something will not be the git core itself. Otherwise we will lose a stable reference implementation to catch its bugs. This is exactly what I'm most afraid about. I tend to think it's a chicken-and-egg problem, too. Do you expect one big merge of a very stable libgit2 at some point? Because, asking others to implement widely used tools on top of libgit2 in order to have it as stable/tested as curl is not going to happen, IMHO. Otherwise, what about going with this optionnal LDFLAGS += -libgit2 ASAP with good disclaimer that it's only intended for development and testing purpose? Then, git-core could slowly rely on functions of libgit2, one after the other. -- Nicolas Sebrecht -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] http: prompt for credentials on failed POST
On Mon, Aug 27, 2012 at 10:48:28AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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. A silly question. Does the initial GET request when we push look any different from the initial GET request when we fetch? Can we make them look different in an updated client, so that the server side can say this GET is about pushing into us, and we require authentication? Yes, they are already different. A fetch asks for info/refs?service=git-upload-pack and a push asks for info/refs?service-git-receive-pack And I definitely think the optimal server config will authenticate the client at that first GET step, because the client may do a significant amount of work for the POST (due to the probe_rpc, it won't actually _send_ a large pack, but it will do the complete delta-compression phase before generating any output, which can be slow). But doing it this way has been advertised in our manpage for so long, I assume some people are using it. And given that it used to work for older clients (prior to v1.7.8), and that the person who upgraded their client is not always in charge of telling the person running the server to fix their server, I think it's worth un-breaking it. And we should definitely tweak what git-http-backend advertises on top so that eventually this sub-optimal config dies out. 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. I suspect that in real life, almost nobody runs smart HTTP server that allows anonymous push. How much usability penalty would it be if we always fill credential before pushing? It would reintroduce the problem that 986bbc0 was fixing: we would prompt even when curl would end up pulling the credential from .netrc. I find that somewhat less compelling a problem now that we have credential helpers, though. And of course it does not fix (1) or (2) above, either. Alternatively, how much latency penalty would it incur if we always send a probe request regardless of the request size when we try to push without having an authentication material? It would be one http round-trip and no-op invocation of request-pack on the server. If we did it only on push, that would probably not be too bad, as we would hit it only when we were actually pushing something. But that would still suffer from (1) and (2) above, so I don't see it as a real advantage. You _could_ fix both cases by buffering the input data and restarting the request. I just didn't think it was worth doing, since they are unlikely configurations and the code complexity is much higher. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bringing a bit more sanity to $GIT_DIR/objects/info/alternates?
hi, Junio C Hamano gitster at pobox.com writes: The alternates mechanism [...] sorry for the somewhat late response - i found this thread only now. at qt-project.org we have a somewhat peculiar setup: we have the qt4 repository, and a bunch of qt5 repositories which resulted from a split. qt5 is under active development, but qt4 is still maintained. that means that we need to cherry-pick between those repositories quite a lot. for an optimal cherry-picking experience one needs three-way-merging, which means we need shared object stores. which is where the problems start: my first approach was just a common objects/ directory with all repositories symlinking into it. problems: - the object store can never be garbage-collected. with a lot of heavy rebasing and temporarily added remotes, it gets messy after a while. - there is a constant risk of destroying the object store by inadvertently running git gc - which is particularly likely with git-gui, as it seems to be retarded enough to ignore the auto-gc setting. so the second approach is the bare aggregator repo which adds all other repos as remotes, and the other repos link back via alternates. problems: - to actually share objects, one always needs to push to the aggregator - tags having a shared namespace doesn't actually work, because the repos have the same tags on different commits (they are independent repos, after all) - one still cannot safely garbage-collect the aggregator, as the refs don't include the stashes and the index, so rebasing may invalidate these more transient objects. i would re-propose hallvard's volatile alternates (at least i think that's what he was talking about two weeks ago): they can be used to obtain objects, but every object which is in any way referenced from the current clone must be available locally (or from a regular alternate). that means that diffing, etc. would get objects only temporarily, while cherry-picking would actually copy (some of) the objects. this would make it possible to cross-link repositories, safely and without any 3rd parties. thoughts? regards -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/7] Fix tests under GETTEXT_POISON on relative dates
Use a i18n-specific test_i18ncmp in t/t0006-date.sh for relative dates
tests. This issue was was introduced in v1.7.10-230-g7d29a:
7d29a i18n: mark relative dates for translation
and been broken under GETTEXT_POISON=YesPlease since.
Signed-off-by: Jiang Xin worldhello@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
t/t0006-date.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 1d29..e53cf 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -11,7 +11,7 @@ check_show() {
echo $t - $2 expect
test_expect_${3:-success} relative date ($2)
test-date show $t actual
- test_cmp expect actual
+ test_i18ncmp expect actual
}
--
1.7.12.92.g949df84
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/7] Fix tests under GETTEXT_POISON on git-apply
Use i18n-specific test functions in test scripts for git-apply. This issue was was introduced in the following commits: de373 i18n: apply: mark parseopt strings for translation 3638e i18n: apply: mark strings for translation and been broken under GETTEXT_POISON=YesPlease since. Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- t/t4012-diff-binary.sh | 4 ++-- t/t4120-apply-popt.sh | 4 ++-- t/t4133-apply-filenames.sh | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh index ec4de..1215a 100755 --- a/t/t4012-diff-binary.sh +++ b/t/t4012-diff-binary.sh @@ -63,7 +63,7 @@ test_expect_success 'apply --numstat understands diff --binary format' ' # apply needs to be able to skip the binary material correctly # in order to report the line number of a corrupt patch. -test_expect_success 'apply detecting corrupt patch correctly' ' +test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' ' git diff output sed -e s/-CIT/xCIT/ output broken test_must_fail git apply --stat --summary broken 2detected @@ -73,7 +73,7 @@ test_expect_success 'apply detecting corrupt patch correctly' ' test $detected = xCIT ' -test_expect_success 'apply detecting corrupt patch correctly' ' +test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' ' git diff --binary | sed -e s/-CIT/xCIT/ broken test_must_fail git apply --stat --summary broken 2detected detected=`cat detected` diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh index a33d5..c5fec 100755 --- a/t/t4120-apply-popt.sh +++ b/t/t4120-apply-popt.sh @@ -32,7 +32,7 @@ test_expect_success 'apply git diff with -p2' ' test_expect_success 'apply with too large -p' ' cp file1.saved file1 test_must_fail git apply --stat -p3 patch.file 2err - grep removing 3 leading err + test_i18ngrep removing 3 leading err ' test_expect_success 'apply (-p2) traditional diff with funny filenames' ' @@ -54,7 +54,7 @@ test_expect_success 'apply (-p2) traditional diff with funny filenames' ' test_expect_success 'apply with too large -p and fancy filename' ' cp file1.saved file1 test_must_fail git apply --stat -p3 patch.escaped 2err - grep removing 3 leading err + test_i18ngrep removing 3 leading err ' test_expect_success 'apply (-p2) diff, mode change only' ' diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh index 94da9..2ecb4 100755 --- a/t/t4133-apply-filenames.sh +++ b/t/t4133-apply-filenames.sh @@ -30,9 +30,9 @@ EOF test_expect_success 'apply diff with inconsistent filenames in headers' ' test_must_fail git apply bad1.patch 2err - grep inconsistent new filename err + test_i18ngrep inconsistent new filename err test_must_fail git apply bad2.patch 2err - grep inconsistent old filename err + test_i18ngrep inconsistent old filename err ' test_done -- 1.7.12.92.g949df84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/7] Fix tests under GETTEXT_POISON on pack-object
Use i18n-specific test functions in test scripts for pack-object. This issue was was introduced in v1.7.10.2-556-g46140: 46140 index-pack: use streaming interface for collision test on large blobs cf2ba pack-objects: use streaming interface for reading large loose blobs and been broken under GETTEXT_POISON=YesPlease since. Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- t/t5300-pack-object.sh | 4 ++-- t/t5530-upload-pack-error.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 2e52..a07c8 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -416,11 +416,11 @@ test_expect_success \ test_expect_success \ 'make sure index-pack detects the SHA1 collision' \ 'test_must_fail git index-pack -o bad.idx test-3.pack 2msg - grep SHA1 COLLISION FOUND msg' + test_i18ngrep SHA1 COLLISION FOUND msg' test_expect_success \ 'make sure index-pack detects the SHA1 collision (large blobs)' \ 'test_must_fail git -c core.bigfilethreshold=1 index-pack -o bad.idx test-3.pack 2msg - grep SHA1 COLLISION FOUND msg' + test_i18ngrep SHA1 COLLISION FOUND msg' test_done diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh index 6b2a5f..c983d 100755 --- a/t/t5530-upload-pack-error.sh +++ b/t/t5530-upload-pack-error.sh @@ -35,8 +35,8 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' ' printf 0032want %s\n0009done\n \ $(git rev-parse HEAD) input test_must_fail git upload-pack . input /dev/null 2output.err - grep unable to read output.err - grep pack-objects died output.err + test_i18ngrep unable to read output.err + test_i18ngrep pack-objects died output.err ' test_expect_success 'corrupt repo differently' ' -- 1.7.12.92.g949df84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 6/7] Fix tests under GETTEXT_POISON on git-remote
Use i18n-specific test functions in test scripts for git-remote, and bypass some testcases using C_LOCALE_OUTPUT prereq flag. This issue was was introduced in v1.7.10-233-gbb16d5: bb16d5 i18n: remote: mark strings for translation and been broken under GETTEXT_POISON=YesPlease since. Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- t/t5505-remote.sh | 28 +--- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index e8af6..4b720 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -52,7 +52,7 @@ test_expect_success setup ' ' -test_expect_success 'remote information for the origin' ' +test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' ' ( cd test tokens_match origin $(git remote) @@ -66,8 +66,6 @@ test_expect_success 'add another remote' ' cd test git remote add -f second ../two tokens_match origin second $(git remote) - check_remote_track origin master side - check_remote_track second master side another check_tracking_branch second master side another git for-each-ref --format=%(refname) refs/remotes | sed -e /^refs\/remotes\/origin\//d \ @@ -77,6 +75,14 @@ test_expect_success 'add another remote' ' ) ' +test_expect_success C_LOCALE_OUTPUT 'check tracking for add another remote' ' +( + cd test + check_remote_track origin master side + check_remote_track second master side another +) +' + test_expect_success 'remote forces tracking branches' ' ( cd test @@ -95,7 +101,7 @@ test_expect_success 'remove remote' ' ) ' -test_expect_success 'remove remote' ' +test_expect_success C_LOCALE_OUTPUT 'remove remote' ' ( cd test tokens_match origin $(git remote) @@ -131,8 +137,8 @@ EOF git remote rm oops 2actual2 git branch -d foobranch git tag -d footag - test_cmp expect1 actual1 - test_cmp expect2 actual2 + test_i18ncmp expect1 actual1 + test_i18ncmp expect2 actual2 ) ' @@ -192,7 +198,7 @@ test_expect_success 'show' ' git config --add remote.two.push refs/heads/master:refs/heads/another git remote show origin two output git branch -d rebase octopus -test_cmp expect output) +test_i18ncmp expect output) ' cat test/expect EOF @@ -217,7 +223,7 @@ test_expect_success 'show -n' ' cd test git remote show -n origin output mv ../one.unreachable ../one -test_cmp expect output) +test_i18ncmp expect output) ' test_expect_success 'prune' ' @@ -255,7 +261,7 @@ EOF test_expect_success 'set-head --auto fails w/multiple HEADs' ' (cd test test_must_fail git remote set-head --auto two output 21 - test_cmp expect output) + test_i18ncmp expect output) ' cat test/expect EOF @@ -285,7 +291,7 @@ test_expect_success 'prune --dry-run' ' test_must_fail git rev-parse refs/remotes/origin/side (cd ../one git branch -m side side2) -test_cmp expect output) +test_i18ncmp expect output) ' test_expect_success 'add --mirror prune' ' @@ -705,7 +711,7 @@ test_expect_success 'remote prune to cause a dangling symref' ' cd seven git remote prune origin ) err 21 - grep has become dangling err + test_i18ngrep has become dangling err : And the dangling symref will not cause other annoying errors ( -- 1.7.12.92.g949df84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 7/7] Fix tests under GETTEXT_POISON on parseopt
Use i18n-specific test functions in test scripts for parseopt tests.
This issue was was introduced in v1.7.10.1-488-g54e6d:
54e6d i18n: parseopt: lookup help and argument translations when showing
usage
and been broken under GETTEXT_POISON=YesPlease since.
Signed-off-by: Jiang Xin worldhello@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
t/t0040-parse-options.sh| 34 +-
t/t1300-repo-config.sh | 2 +-
t/t1502-rev-parse-parseopt.sh | 2 +-
t/t2006-checkout-index-basic.sh | 4 ++--
t/t2107-update-index-basic.sh | 4 ++--
t/t3004-ls-files-basic.sh | 4 ++--
t/t3200-branch.sh | 4 ++--
t/t3501-revert-cherry-pick.sh | 4 ++--
t/t4200-rerere.sh | 4 ++--
t/t6500-gc.sh | 4 ++--
t/t7600-merge.sh| 2 +-
11 files changed, 46 insertions(+), 22 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index e3f35..244a4 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -51,7 +51,7 @@ EOF
test_expect_success 'test help' '
test_must_fail test-parse-options -h output 2 output.err
test ! -s output.err
- test_cmp expect output
+ test_i18ncmp expect output
'
mv expect expect.err
@@ -79,6 +79,17 @@ check() {
test_cmp expect output
}
+check_i18n() {
+ what=$1
+ shift
+ expect=$1
+ shift
+ sed s/^$what .*/$what $expect/ expect.template expect
+ test-parse-options $* output 2output.err
+ test ! -s output.err
+ test_i18ncmp expect output
+}
+
check_unknown() {
case $1 in
--*)
@@ -92,6 +103,19 @@ check_unknown() {
test_cmp expect output.err
}
+check_unknown_i18n() {
+ case $1 in
+ --*)
+ echo error: unknown option \`${1#--}\' expect ;;
+ -*)
+ echo error: unknown switch \`${1#-}\' expect ;;
+ esac
+ cat expect.err expect
+ test_must_fail test-parse-options $* output 2output.err
+ test ! -s output
+ test_i18ncmp expect output.err
+}
+
test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'
test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-doubt'
test_expect_success 'OPT_BOOL() #3' 'check boolean: 1 -D'
@@ -104,8 +128,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check
boolean: 1 -DB'
test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D
--no-no-doubt'
-test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'
-test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'
+test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n
--no-no-fear'
test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
@@ -310,8 +334,8 @@ EOF
test_expect_success 'OPT_CALLBACK() and callback errors work' '
test_must_fail test-parse-options --no-length output 2 output.err
- test_cmp expect output
- test_cmp expect.err output.err
+ test_i18ncmp expect output
+ test_i18ncmp expect.err output.err
'
cat expect EOF
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a477..e127f 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -391,7 +391,7 @@ test_expect_success 'get bool variable with empty value' \
test_expect_success 'no arguments, but no crash' '
test_must_fail git config output 21
- grep usage output
+ test_i18ngrep usage output
'
cat .git/config EOF
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 1efd7..13c88 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -41,7 +41,7 @@ EOF
test_expect_success 'test --parseopt help output' '
test_expect_code 129 git rev-parse --parseopt -- -h output
optionspec
- test_cmp expect output
+ test_i18ncmp expect output
'
cat expect EOF
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index b855..57cbd 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -7,7 +7,7 @@ test_description='basic checkout-index tests
test_expect_success 'checkout-index --gobbledegook' '
test_expect_code 129 git checkout-index --gobbledegook 2err
- grep [Uu]sage err
+ test_i18ngrep [Uu]sage err
'
test_expect_success 'checkout-index -h in broken repository' '
@@ -18,7 +18,7 @@ test_expect_success 'checkout-index -h in broken repository' '
.git/index
test_expect_code 129 git checkout-index -h usage 21
)
- grep [Uu]sage broken/usage
+ test_i18ngrep [Uu]sage broken/usage
'
test_done
diff --git a/t/t2107-update-index-basic.sh
Re: git checkout -t -B
From: Junio C Hamano gits...@pobox.com Sent: Sunday, August 26, 2012 7:38 PM 乙酸鋰 ch3co...@gmail.com writes: git checkout -t -B origin/abcde works but git checkout -B -t origin/abcde does not. Could you document the order of parameters or fix the behaviour? It is crystal clear that -b/-B/--orphan must be followed by the name of the branch you are creating from the SYNOPSIS section of the documentation. NAME git-checkout - Checkout a branch or paths to the working tree SYNOPSIS [verse] ... 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] new_branch] [st.. ... I didn't find it immediately obvious, but with 6/6 hindsight it is clearly indicated. However the -t option isn't listed within the synopsis, and does immediately follow the -b -B in the options list which could confuse some readers who would stick it after the -b ;-) Should the -t and -l options be shown in the synopsis? However, the option description can use some improvement. It currently reads: -b:: Create a new branch named new_branch and start it at start_point; see linkgit:git-branch[1] for details. as if it and new_branch are freestanding arguments. I think we should describe the option like this: -b new_branch:: Create a new branch named new_branch and start it at start_point; see linkgit:git-branch[1] for details. The description for -B and --orphan options share the same issue. I suspect that documentation for other commands may share this issue. It would be good if somebody can check the option description section and make sure there is no discrepancy like this by comparing it to the SYNOPSIS section (or git cmd -h) for all manual pages. I'll patch only git-checkout.txt myself for now; hint, hint. I searched for all occurrences of '[[' which would indicate a double optional argument within the synopsis and only found git-read-tree. I don't think it has the same problem but I wasn't sure of the priority order between the '[]' and '|' selectors in the multi-choice cases. I haven't seen anything definitive on how one should read the synopsis. I thought the order of some option could be exchanged, as rev-parse says that they can be combined (for the right commands). Thanks. -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] optimize fast-forward checks
This is a follow-up to $gmane/204149; somehow the discussion petered out, but I think this topic (and also tag --contains that was not really discussed) needs to be looked into. Here is a first baby step. - The first one is an obvious simplification; we never supported more than one reference commits and no caller had to invent a loop around it to emulate multi-reference in_merge_base(). - The two patches that follow are the uses of get_merge_bases() where in_merge_bases() is sufficient. These callers are not interested in the merge bases between the two points; they only want to know if one point is an ancestor of the other. - The next one [4/5] should be identical to Thomas's patch. - The last one attempts to reduce the cost of postprocessing from N*(N-1) to N, but it somehow breaks 6010. I haven't looked into why. They say all bugs are shallow given enough eyeballs, so I am sending it out to see if that is true ;-) Junio C Hamano (5): in_merge_bases(): support only one other commit receive-pack: use in_merge_bases() for fast-forward check http-push: use in_merge_bases() for fast-forward check in_merge_bases(): omit unnecessary redundant common ancestor reduction (BROKEN) get_merge_bases_many(): walk from many tips in parallel builtin/branch.c | 4 +-- builtin/fetch.c| 2 +- builtin/receive-pack.c | 8 + commit.c | 60 -- commit.h | 2 +- contrib/examples/builtin-fetch--tool.c | 2 +- fast-import.c | 2 +- http-push.c| 3 +- submodule.c| 12 +++ 9 files changed, 49 insertions(+), 46 deletions(-) -- 1.7.12.116.g31e0100 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] in_merge_bases(): support only one other commit
In early days of its life, I planned to make it possible to compute
is a commit contained in all of these other commits? with this
function, but it turned out that no caller needed it.
Just make it take two commit objects and add a comment to say what
these two functions do.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/branch.c | 4 ++--
builtin/fetch.c| 2 +-
commit.c | 15 +--
commit.h | 2 +-
contrib/examples/builtin-fetch--tool.c | 2 +-
fast-import.c | 2 +-
submodule.c| 12 ++--
7 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..0360774 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -129,7 +129,7 @@ static int branch_merged(int kind, const char *name,
if (!reference_rev)
reference_rev = head_rev;
- merged = in_merge_bases(rev, reference_rev, 1);
+ merged = in_merge_bases(rev, reference_rev);
/*
* After the safety valve is fully redefined to check with
@@ -139,7 +139,7 @@ static int branch_merged(int kind, const char *name,
* a gentle reminder is in order.
*/
if ((head_rev != reference_rev)
- in_merge_bases(rev, head_rev, 1) != merged) {
+ in_merge_bases(rev, head_rev) != merged) {
if (merged)
warning(_(deleting branch '%s' that has been merged
to\n
'%s', but not yet merged to HEAD.),
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..723072f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -323,7 +323,7 @@ static int update_local_ref(struct ref *ref,
return r;
}
- if (in_merge_bases(current, updated, 1)) {
+ if (in_merge_bases(current, updated)) {
char quickref[83];
int r;
strcpy(quickref, find_unique_abbrev(current-object.sha1,
DEFAULT_ABBREV));
diff --git a/commit.c b/commit.c
index 42af4c1..74ec5f1 100644
--- a/commit.c
+++ b/commit.c
@@ -780,6 +780,9 @@ struct commit_list *get_merge_bases(struct commit *one,
struct commit *two,
return get_merge_bases_many(one, 1, two, cleanup);
}
+/*
+ * Is commit a decendant of one of the elements on the with_commit list?
+ */
int is_descendant_of(struct commit *commit, struct commit_list *with_commit)
{
if (!with_commit)
@@ -789,21 +792,21 @@ int is_descendant_of(struct commit *commit, struct
commit_list *with_commit)
other = with_commit-item;
with_commit = with_commit-next;
- if (in_merge_bases(other, commit, 1))
+ if (in_merge_bases(other, commit))
return 1;
}
return 0;
}
-int in_merge_bases(struct commit *commit, struct commit **reference, int num)
+/*
+ * Is commit an ancestor of (i.e. reachable from) the reference?
+ */
+int in_merge_bases(struct commit *commit, struct commit *reference)
{
struct commit_list *bases, *b;
int ret = 0;
- if (num == 1)
- bases = get_merge_bases(commit, *reference, 1);
- else
- die(not yet);
+ bases = get_merge_bases(commit, reference, 1);
for (b = bases; b; b = b-next) {
if (!hashcmp(commit-object.sha1, b-item-object.sha1)) {
ret = 1;
diff --git a/commit.h b/commit.h
index d617fa3..6edce87 100644
--- a/commit.h
+++ b/commit.h
@@ -171,7 +171,7 @@ extern struct commit_list *get_shallow_commits(struct
object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
int is_descendant_of(struct commit *, struct commit_list *);
-int in_merge_bases(struct commit *, struct commit **, int);
+int in_merge_bases(struct commit *, struct commit *);
extern int interactive_add(int argc, const char **argv, const char *prefix,
int patch);
extern int run_add_interactive(const char *revision, const char *patch_mode,
diff --git a/contrib/examples/builtin-fetch--tool.c
b/contrib/examples/builtin-fetch--tool.c
index 0d54aa7..8bc8c75 100644
--- a/contrib/examples/builtin-fetch--tool.c
+++ b/contrib/examples/builtin-fetch--tool.c
@@ -96,7 +96,7 @@ static int update_local_ref(const char *name,
strcpy(oldh, find_unique_abbrev(current-object.sha1, DEFAULT_ABBREV));
strcpy(newh, find_unique_abbrev(sha1_new, DEFAULT_ABBREV));
- if (in_merge_bases(current, updated, 1)) {
+ if (in_merge_bases(current, updated)) {
fprintf(stderr, * %s: fast-forward to %s\n,
name, note);
fprintf(stderr, old..new: %s..%s\n, oldh, newh);
diff --git a/fast-import.c b/fast-import.c
index eed97c8..c2a814e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1691,7 +1691,7
[PATCH 2/5] receive-pack: use in_merge_bases() for fast-forward check
The original computed merge-base between the old commit and the new
commit and checked if the old commit was a merge base between them,
in order to make sure we are fast-forwarding.
Instead, call in_merge_bases(old, new) which does the same.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/receive-pack.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 3f05d97..b1abe7b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -478,7 +478,6 @@ static const char *update(struct command *cmd)
!prefixcmp(name, refs/heads/)) {
struct object *old_object, *new_object;
struct commit *old_commit, *new_commit;
- struct commit_list *bases, *ent;
old_object = parse_object(old_sha1);
new_object = parse_object(new_sha1);
@@ -491,12 +490,7 @@ static const char *update(struct command *cmd)
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
- bases = get_merge_bases(old_commit, new_commit, 1);
- for (ent = bases; ent; ent = ent-next)
- if (!hashcmp(old_sha1, ent-item-object.sha1))
- break;
- free_commit_list(bases);
- if (!ent) {
+ if (!in_merge_bases(old_commit, new_commit)) {
rp_error(denying non-fast-forward %s
(you should pull first), name);
return non-fast-forward;
--
1.7.12.116.g31e0100
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] in_merge_bases(): omit unnecessary redundant common ancestor reduction
The function get_merge_bases() needs to postprocess the result from
merge_bases_many() in order to make sure none of the commit is a
true ancestor of another commit, which is expensive. However, when
checking if a commit is an ancestor of another commit, we only need
to see if the commit is a common ancestor between the two, and do
not have to care if other common ancestors merge_bases_many() finds
are true merge bases or an ancestor of another merge base.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
commit.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 74ec5f1..f5211bd 100644
--- a/commit.c
+++ b/commit.c
@@ -806,7 +806,10 @@ int in_merge_bases(struct commit *commit, struct commit
*reference)
struct commit_list *bases, *b;
int ret = 0;
- bases = get_merge_bases(commit, reference, 1);
+ bases = merge_bases_many(commit, 1, reference);
+ clear_commit_marks(commit, all_flags);
+ clear_commit_marks(reference, all_flags);
+
for (b = bases; b; b = b-next) {
if (!hashcmp(commit-object.sha1, b-item-object.sha1)) {
ret = 1;
--
1.7.12.116.g31e0100
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] http-push: use in_merge_bases() for fast-forward check
The original computed merge-base between HEAD and the remote ref and
checked if the remote ref is a merge base between them, in order to
make sure that we are fast-forwarding.
Instead, call in_merge_bases(remote, HEAD) which does the same.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
http-push.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/http-push.c b/http-push.c
index a832ca7..8701c12 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1610,9 +1610,8 @@ static int verify_merge_base(unsigned char *head_sha1,
struct ref *remote)
{
struct commit *head = lookup_commit_or_die(head_sha1, HEAD);
struct commit *branch = lookup_commit_or_die(remote-old_sha1,
remote-name);
- struct commit_list *merge_bases = get_merge_bases(head, branch, 1);
- return (merge_bases !merge_bases-next merge_bases-item ==
branch);
+ return in_merge_bases(branch, head);
}
static int delete_remote_branch(const char *pattern, int force)
--
1.7.12.116.g31e0100
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] (BROKEN) get_merge_bases_many(): walk from many tips in parallel
After merge_bases_many() paints the graph in two colors to find
common ancestors, get_merge_bases_many() reduces the result by
excluding commits that can be reached from other commits in the
result. We used to do N * (N-1) traversals for this, but we can
check if one commit is reachable from any of the other (N-1) commits
by a single traversal, and repeat it N times to find the answer.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
commit.c | 42 +++---
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/commit.c b/commit.c
index f5211bd..ccf2c5a 100644
--- a/commit.c
+++ b/commit.c
@@ -715,7 +715,7 @@ struct commit_list *get_merge_bases_many(struct commit *one,
int cleanup)
{
struct commit_list *list;
- struct commit **rslt;
+ struct commit **rslt, **others;
struct commit_list *result;
int cnt, i, j;
@@ -744,33 +744,37 @@ struct commit_list *get_merge_bases_many(struct commit
*one,
for (list = result, i = 0; list; list = list-next)
rslt[i++] = list-item;
free_commit_list(result);
+ result = NULL;
clear_commit_marks(one, all_flags);
for (i = 0; i n; i++)
clear_commit_marks(twos[i], all_flags);
- for (i = 0; i cnt - 1; i++) {
- for (j = i+1; j cnt; j++) {
- if (!rslt[i] || !rslt[j])
- continue;
- result = merge_bases_many(rslt[i], 1, rslt[j]);
- clear_commit_marks(rslt[i], all_flags);
- clear_commit_marks(rslt[j], all_flags);
- for (list = result; list; list = list-next) {
- if (rslt[i] == list-item)
- rslt[i] = NULL;
- if (rslt[j] == list-item)
- rslt[j] = NULL;
- }
- }
- }
- /* Surviving ones in rslt[] are the independent results */
- result = NULL;
+ others = xcalloc(cnt - 1, sizeof(*others));
for (i = 0; i cnt; i++) {
- if (rslt[i])
+ /*
+* Is rslt[i] an ancestor of any of the others?
+* then it is not interesting to us.
+*/
+ for (j = 0; j i; j++)
+ others[j] = rslt[j];
+ for (j = 1 + 1; j cnt; j++)
+ others[j - 1] = rslt[j];
+ list = merge_bases_many(rslt[i], cnt - 1, others);
+ clear_commit_marks(rslt[i], all_flags);
+ for (j = 0; j cnt - 1; j++)
+ clear_commit_marks(others[j], all_flags);
+ while (list) {
+ if (rslt[i] == list-item)
+ break;
+ list = list-next;
+ }
+ if (!list)
commit_list_insert_by_date(rslt[i], result);
+ free_commit_list(list);
}
free(rslt);
+ free(others);
return result;
}
--
1.7.12.116.g31e0100
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git checkout -t -B
Philip Oakley philipoak...@iee.org writes: I searched for all occurrences of '[[' which would indicate a double optional argument within the synopsis and only found git-read-tree. Double-optional? That is not an issue. If an option always takes a parameter, we would have git cmd [--option parameter] instead of one of git cmd [--option] git cmd [--option] parameter git cmd [--option] parameter... and if we had --option:: This option distims the parameter ... that needs to be updated to --option parameter:: This option distims the parameter ... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] http: prompt for credentials on failed POST
Jeff King p...@peff.net writes: A silly question. Does the initial GET request when we push look any different from the initial GET request when we fetch? Can we make them look different in an updated client, so that the server side can say this GET is about pushing into us, and we require authentication? Yes, they are already different. A fetch asks for ... But doing it this way has been advertised in our manpage for so long, I assume some people are using it. And given that it used to work for older clients (prior to v1.7.8), and that the person who upgraded their client is not always in charge of telling the person running the server to fix their server, I think it's worth un-breaking it. Oh, I wasn't saying the fix is unnecessary. I was trying to see if there is something people who _care_ about wasted effort on the client side can do to fix their configuration properly (otherwise while we are patching the client, make sure we give them a way). But that would still suffer from (1) and (2) above, so I don't see it as a real advantage. You _could_ fix both cases by buffering the input data and restarting the request. I just didn't think it was worth doing, since they are unlikely configurations and the code complexity is much higher. OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] (BROKEN) get_merge_bases_many(): walk from many tips in parallel
Junio C Hamano gits...@pobox.com writes:
for (i = 0; i cnt; i++) {
- if (rslt[i])
+ /*
+ * Is rslt[i] an ancestor of any of the others?
+ * then it is not interesting to us.
+ */
+ for (j = 0; j i; j++)
+ others[j] = rslt[j];
+ for (j = 1 + 1; j cnt; j++)
s/1 + 1/i + 1/;
With that, all tests seem to pass ;-)
+ others[j - 1] = rslt[j];
+ list = merge_bases_many(rslt[i], cnt - 1, others);
+ clear_commit_marks(rslt[i], all_flags);
+ for (j = 0; j cnt - 1; j++)
+ clear_commit_marks(others[j], all_flags);
+ while (list) {
+ if (rslt[i] == list-item)
+ break;
+ list = list-next;
+ }
+ if (!list)
commit_list_insert_by_date(rslt[i], result);
+ free_commit_list(list);
}
free(rslt);
+ free(others);
return result;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent pu update
Thomas Rast tr...@student.ethz.ch writes:
diff --git i/commit.c w/commit.c
index 65a8485..70427ab 100644
--- i/commit.c
+++ w/commit.c
@@ -837,10 +837,13 @@ int in_merge_bases(struct commit *commit, struct commit
**reference, int num)
struct commit_list *bases, *b;
int ret = 0;
- if (num == 1)
- bases = get_merge_bases(commit, *reference, 1);
- else
+ if (num != 1)
die(not yet);
+
+ bases = merge_bases_many(commit, 1, reference);
+ clear_commit_marks(commit, all_flags);
+ clear_commit_marks(*reference, all_flags);
+
for (b = bases; b; b = b-next) {
if (!hashcmp(commit-object.sha1, b-item-object.sha1)) {
ret = 1;
This ended up being part of the series I sent earlier, and I want to
assign authorship to you. As you did this as part of the discussion,
naturally the patch came without a sign-off. Can we consider it
signed off? Just saying ok is fine.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] checkout: verify new branch name's validity early
If the new branch name to -b/-B happens to be missing, the next
argument may be mistaken as branch name and no longer recognized by
checkout as argument. This may lead to confusing error messages.
By checking branch name early and printing out the invalid name, users
may realize they forgot to specify branch name.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano gits...@pobox.com wrote:
Ideally you would want
fatal: -t is not an acceptable name for a branch
in this case; if it is cumbersome to arrange, at the very least,
updating paths is incompatible with checking out the branch -t.
would be clearer.
Fair enough. It turns out we do check branch name's validity. It's
just too late.
builtin/checkout.c | 20 ++--
t/t2018-checkout-branch.sh | 5 +
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d812219..03b0f25 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1049,6 +1049,16 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
if (opts.track == BRANCH_TRACK_UNSPECIFIED)
opts.track = git_branch_track;
+ if (opts.new_branch) {
+ struct strbuf buf = STRBUF_INIT;
+
+ opts.branch_exists = validate_new_branchname(opts.new_branch,
buf,
+
!!opts.new_branch_force,
+
!!opts.new_branch_force);
+
+ strbuf_release(buf);
+ }
+
if (argc) {
const char **pathspec = get_pathspec(prefix, argv);
@@ -1079,16 +1089,6 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
if (patch_mode)
return interactive_checkout(new.name, NULL, opts);
- if (opts.new_branch) {
- struct strbuf buf = STRBUF_INIT;
-
- opts.branch_exists = validate_new_branchname(opts.new_branch,
buf,
-
!!opts.new_branch_force,
-
!!opts.new_branch_force);
-
- strbuf_release(buf);
- }
-
if (new.name !new.commit) {
die(_(Cannot switch branch to a non-commit.));
}
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 2741262..48ab6a2 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,4 +198,9 @@ test_expect_success 'checkout -B to the current branch
works' '
test_dirty_mergeable
'
+test_expect_success 'checkout -b checks branch validitity early' '
+ test_must_fail git checkout -b -t origin/master 2err
+ grep not a valid branch name err
+'
+
test_done
--
1.7.12.rc2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] checkout: verify new branch name's validity early
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
If the new branch name to -b/-B happens to be missing, the next
argument may be mistaken as branch name and no longer recognized by
checkout as argument. This may lead to confusing error messages.
By checking branch name early and printing out the invalid name, users
may realize they forgot to specify branch name.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano gits...@pobox.com wrote:
Ideally you would want
fatal: -t is not an acceptable name for a branch
in this case; if it is cumbersome to arrange, at the very least,
updating paths is incompatible with checking out the branch -t.
would be clearer.
Fair enough. It turns out we do check branch name's validity. It's
just too late.
The surrounding code is somewhat tricky and the code structure is
brittle; there are places that update the opts.new_branch so the new
location of this check has to be after them, and there is one
codepath that having a bad value in it does not matter.
I had to check the code outside the context of this patch a few
times to convince myself that this patch does not break them. I'll
queue the patch as-is for now, but we probably would want to see how
we can structure it to be less brittle.
Thanks.
builtin/checkout.c | 20 ++--
t/t2018-checkout-branch.sh | 5 +
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d812219..03b0f25 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1049,6 +1049,16 @@ int cmd_checkout(int argc, const char **argv, const
char *prefix)
if (opts.track == BRANCH_TRACK_UNSPECIFIED)
opts.track = git_branch_track;
+ if (opts.new_branch) {
+ struct strbuf buf = STRBUF_INIT;
+
+ opts.branch_exists = validate_new_branchname(opts.new_branch,
buf,
+
!!opts.new_branch_force,
+
!!opts.new_branch_force);
+
+ strbuf_release(buf);
+ }
+
if (argc) {
const char **pathspec = get_pathspec(prefix, argv);
@@ -1079,16 +1089,6 @@ int cmd_checkout(int argc, const char **argv, const
char *prefix)
if (patch_mode)
return interactive_checkout(new.name, NULL, opts);
- if (opts.new_branch) {
- struct strbuf buf = STRBUF_INIT;
-
- opts.branch_exists = validate_new_branchname(opts.new_branch,
buf,
-
!!opts.new_branch_force,
-
!!opts.new_branch_force);
-
- strbuf_release(buf);
- }
-
if (new.name !new.commit) {
die(_(Cannot switch branch to a non-commit.));
}
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 2741262..48ab6a2 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,4 +198,9 @@ test_expect_success 'checkout -B to the current branch
works' '
test_dirty_mergeable
'
+test_expect_success 'checkout -b checks branch validitity early' '
+ test_must_fail git checkout -b -t origin/master 2err
+ grep not a valid branch name err
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
