* (unknown),
From: Ben Walton @ 2011-11-02 16:38 UTC (permalink / raw)
To: normalperson; +Cc: git
Hi Eric,
When running the test suite against subversion 1.7 on solaris, t9134
was failing with an assertion failure in the SVN bindings and a
subsequent core dump from perl.
The behaviour of 1.7 has seemingly changed such that even file paths
must be eascaped/canonicalized now. The core dump behaviour is
something that still needs to be investigated with the subversion
folks. A colleague and I will push that on their side.
See the initial discussion of this issue on the svn list here:
http://article.gmane.org/gmane.comp.version-control.subversion.devel/132227
With the following patch, the test suite once again passes. I don't
know how it will behave against an older client though so maybe a
conditional check for the version of the bindings is still required?
Thanks
-Ben
From: Ben Walton <bwalton@artsci.utoronto.ca>
Subject:
In-Reply-To:
^ permalink raw reply
* [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements
From: Ben Walton @ 2011-11-02 16:38 UTC (permalink / raw)
To: normalperson; +Cc: git, Ben Walton
In-Reply-To: <1320251895-6348-1-git-send-email-bwalton@artsci.utoronto.ca>
Previously only http/https URL's were uri escaped. When building
against subversion 1.7, this was causing a segfault in perl after
an assertion failure in the SVN::Ra bindings during in t9134.
Changing 'trash directory' to 'trash_directory' worked around the
problem.
After a colleague reported this problem to the subversion list, it
was determined that the problem is in git, not svn.[1] The SVN code
expects URL's and paths to be pre-escaped.
We now also escape file:// URL's in the Git::SVN::Ra->escape_url
code path.
[1] http://news.gmane.org/gmane.comp.version-control.subversion.devel
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
git-svn.perl | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 351e743..b00c188 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5366,6 +5366,9 @@ sub escape_url {
if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
$url = "$scheme://$domain$uri";
+ } elsif ($url =~ m#^(file)://(.*)$#) {
+ my ($scheme, $uri) = ($1, escape_uri_only($2));
+ $url = "$scheme://$uri";
}
$url;
}
--
1.7.7.1
^ permalink raw reply related
* Re: git sticker svg
From: Jeff King @ 2011-11-02 16:59 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <4EB1002A.6010602@drmicha.warpmail.net>
On Wed, Nov 02, 2011 at 09:32:42AM +0100, Michael J Gruber wrote:
> Yeah, git has been G-+ long before G+ ;)
>
> I do prefer the rotated, vertically stacked +-G though (with + <-> t, -
> <-> i, G <-> 3/4-circle with arrow). Good thing we don't have to argue
> about an "official" logo...
I actually don't like the stacked +-G (I find it hard to read). But
since I was ordering the stickers, I got to decide. :)
I think it's clear that we don't want to get into an "official" logo
argument, though. I'll let whoever orders stickers next time make the
decision. :)
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] http-push: don't always prompt for password
From: Junio C Hamano @ 2011-11-02 17:13 UTC (permalink / raw)
To: Stefan Näwe; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <4EB104EA.2040001@atlas-elektronik.com>
Stefan Näwe <stefan.naewe@atlas-elektronik.com> writes:
> Am 01.11.2011 19:12, schrieb Junio C Hamano:
>>
>> There are only handful of commits that even remotely touch http related
>> codepath between v1.7.7 and v1.7.8-rc0:
>>
>> * deba493 http_init: accept separate URL parameter
>>
>> This could change the URL string given to http_auth_init().
>> ...
>> Could you try reverting deba493 and retest, and then if the behaviour is
>> the same "need ENTER", further revert 070b4dd and retest?
>
> I did a little more testing.
> This WIP makes it work for me (i.e. "need ENTER" is gone, works with
> and without .netrc, with 'https://host/repo.git' and
> 'https://user@host...' URL). Needs testing, of course.
>
> ---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---
> diff --git a/http.c b/http.c
> index a4bc770..008ad72 100644
> --- a/http.c
> +++ b/http.c
> @@ -279,8 +279,6 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
> #endif
>
> - init_curl_http_auth(result);
> -
> if (ssl_cert != NULL)
> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> if (has_cert_password())
> @@ -846,7 +844,7 @@ static int http_request(const char *url, void *result, int target, int options)
> else if (missing_target(&results))
> ret = HTTP_MISSING_TARGET;
> else if (results.http_code == 401) {
> - if (user_name) {
> + if (user_name && user_pass) {
> ret = HTTP_NOAUTH;
> } else {
> /*
> @@ -855,7 +853,8 @@ static int http_request(const char *url, void *result, int target, int options)
> * but that is non-portable. Using git_getpass() can at least be stubbed
> * on other platforms with a different implementation if/when necessary.
> */
> - user_name = xstrdup(git_getpass_with_description("Username", description));
> + if (!user_name)
> + user_name = xstrdup(git_getpass_with_description("Username", description));
> init_curl_http_auth(slot->curl);
> ret = HTTP_REAUTH;
> }
> ---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---
This defers the calls to git_getpass* until we get 401 from the server
side.
I am guessing the reason why in the current code get_curl_handle() has a
call to init_curl_http_auth() very early, but only when user_name is set,
is because it is likely for a site to require authentication when the user
already has "username@" in its URL, and doing it this way will avoid the
extra round-trip because by the time we make an HTTP request, we have both
name and pass. If we apply this patch, the check in init_curl_http_auth()
that asks for the password only when user_name is set becomes unnecessary.
I think the second hunk at l.846 sort of makes sense, but not quite.
"We got 401 even though we know we have supplied name and pass" is a valid
criterion to decide that the name/pass is an invalid combination. But it
makes me wonder if this code in its early days guaranteed whenever we have
user_name we always have made sure we have user_pass (otherwise by asking
for it with git_getpass) and that is the reason why it had to check only
for user_name, and if that is the case perhaps the real breakage is we are
not keeping that guarantee in the current code?
IOW, when in the current code do we make a new HTTP request while
user_name is set but no user_pass? Perhaps if we fix that codepath we do
not have to have this extra 401 roundtrip this patch is introducing when
the username (but not password) is given?
Peff, what do you think?
^ permalink raw reply
* Re: [RFC/PATCH] http-push: don't always prompt for password
From: Jeff King @ 2011-11-02 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Näwe, git@vger.kernel.org
In-Reply-To: <7vk47ijvlv.fsf@alter.siamese.dyndns.org>
On Wed, Nov 02, 2011 at 10:13:32AM -0700, Junio C Hamano wrote:
> This defers the calls to git_getpass* until we get 401 from the server
> side.
>
> I am guessing the reason why in the current code get_curl_handle() has a
> call to init_curl_http_auth() very early, but only when user_name is set,
> is because it is likely for a site to require authentication when the user
> already has "username@" in its URL, and doing it this way will avoid the
> extra round-trip because by the time we make an HTTP request, we have both
> name and pass. If we apply this patch, the check in init_curl_http_auth()
> that asks for the password only when user_name is set becomes unnecessary.
Yeah, that was my reading, as well. I was tempted to do away with it, as
it makes the code much simpler, at the cost of doing that extra
round-trip. However, most browsers do that round-trip, and I don't think
it's that big a deal.
In the end I decided not to switch it just because I wanted to be as
minimally invasive as possible, just in case somebody did care about the
round trip.
> I think the second hunk at l.846 sort of makes sense, but not quite.
>
> "We got 401 even though we know we have supplied name and pass" is a valid
> criterion to decide that the name/pass is an invalid combination. But it
> makes me wonder if this code in its early days guaranteed whenever we have
> user_name we always have made sure we have user_pass (otherwise by asking
> for it with git_getpass) and that is the reason why it had to check only
> for user_name, and if that is the case perhaps the real breakage is we are
> not keeping that guarantee in the current code?
All of my patches attempted to keep that condition, as well (because
credential_fill tries to do so). But I didn't think about it during the
re-roll of the patches that are in master now, so maybe that wasn't
kept.
It seems to me that Stefan's patch actually causes that (because he
removes the early "set password if we have a username" logic). But I'll
take another look at what's in master.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] http-push: don't always prompt for password
From: Junio C Hamano @ 2011-11-02 17:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Stefan Näwe, git@vger.kernel.org
In-Reply-To: <20111102172310.GA28525@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It seems to me that Stefan's patch actually causes that (because he
> removes the early "set password if we have a username" logic). But I'll
> take another look at what's in master.
Thanks.
^ permalink raw reply
* Re: [ANNOUNCE] Git 1.7.8.rc0
From: Jeff King @ 2011-11-02 18:03 UTC (permalink / raw)
To: Stefan Naewe; +Cc: Junio C Hamano, Michael J Gruber, git
In-Reply-To: <loom.20111101T211624-511@post.gmane.org>
On Tue, Nov 01, 2011 at 08:18:47PM +0000, Stefan Naewe wrote:
> Stefan Naewe <stefan.naewe <at> gmail.com> writes:
>
> > Push with https works, if the URL looks e.g. like this:
> >
> > https://github.com/user/repo.git
> >
> > rather than this
> >
> > https://user <at> github.com/user/repo.git
> >
> > and having a ~/.netrc like this
> >
> > machine github.com login user password YouDontWantToKnow
> >
> > If the URL contains 'user@' I get the 'need ENTER' behaviour.
> >
>
> Another update:
>
> If I revert deba493 the 'need ENTER' is gone and everything works as above.
I think this is a false positive. The problem that deba493 fixes is that
"git push" was not properly looking at the push URL, but rather pulling
the initial auth information from the fetch URL. So I suspect your
finding the bug there or not may have to do with it actually respecting
your config properly.
I think the bug is much older than this, and we are just exposing it by
finally using the correct URL.
Without using a configured remote, try this (with .netrc configured):
git push https://github.com/user/repo.git :refs/heads/nothing
which should work, and then this:
git push https://user@github.com/user/repo.git :refs/heads/nothing
which will do the "must hit enter to accept password" thing.
That fails even with v1.7.7. I didn't bisect, but it has been there
quite a while (v1.6.6 has it, but v1.6.5 has a weird error, so I didn't
bisect further).
-Peff
^ permalink raw reply
* Re: [ANNOUNCE] Git 1.7.8.rc0
From: Jeff King @ 2011-11-02 18:10 UTC (permalink / raw)
To: Stefan Naewe; +Cc: Junio C Hamano, Michael J Gruber, git
In-Reply-To: <20111102180327.GA30668@sigill.intra.peff.net>
On Wed, Nov 02, 2011 at 02:03:27PM -0400, Jeff King wrote:
> Without using a configured remote, try this (with .netrc configured):
>
> git push https://github.com/user/repo.git :refs/heads/nothing
>
> which should work, and then this:
>
> git push https://user@github.com/user/repo.git :refs/heads/nothing
>
> which will do the "must hit enter to accept password" thing.
>
> That fails even with v1.7.7. I didn't bisect, but it has been there
> quite a while (v1.6.6 has it, but v1.6.5 has a weird error, so I didn't
> bisect further).
OK, I see the issue.
The logic is "if we have a username, but not a password, then ask for
the password before trying any http" (this is what avoids the extra
round trip).
But if you are using netrc, we don't parse it ourselves. We just tell
curl "when you are making the request, check netrc, too".
So the ideal logic is:
1. look in netrc
2. If we have a username and no password, ask for password
3. Otherwise, try it and see if we get a 401.
But we can't do that, because (1) and (3) happen atomically inside of
curl.
The simplest thing is to just drop the behavior in (2), and let it drop
to a 401. The extra round trip probably isn't that big a deal.
The other option is to start parsing netrc ourselves, or do the extra
round trip if we detect ~/.netrc or something. But that last one is
getting pretty hackish.
-Peff
^ permalink raw reply
* Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements
From: Jonathan Nieder @ 2011-11-02 18:20 UTC (permalink / raw)
To: Ben Walton; +Cc: normalperson, git
In-Reply-To: <1320251895-6348-2-git-send-email-bwalton@artsci.utoronto.ca>
Hi,
Ben Walton wrote:
> After a colleague reported this problem to the subversion list, it
> was determined that the problem is in git, not svn.[1] The SVN code
> expects URL's and paths to be pre-escaped.
Thanks for your work on this! I'm not really sure how one can decide
that the problem is not in svn --- some existing functions changed ABI
in such a way as to break existing applications and require code
changes and a recompile. It would be better for Subversion to
silently fix up paths provided by bad callers, or at least to return a
sensible error code.
So the problem is that nobody who cared was testing prereleases of
subversion and reporting bugs early enough for it to get this fixed
before the 1.7 release. But yes, that's water under the bridge and
git-svn (and libsvn-perl, and pysvn, and ...) should just adjust to
the new world order.
> [1] http://news.gmane.org/gmane.comp.version-control.subversion.devel
Do you mean
http://thread.gmane.org/gmane.comp.version-control.subversion.devel/132250
?
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
> git-svn.perl | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
Sounds sensible in principle, though I haven't checked the patch in
detail. When I run "make test" with svn 1.7 with this patch applied,
I get the following result, unfortunately:
| expecting success:
| git svn clone -s "$svnrepo" g &&
| (
| cd g &&
| test x`git rev-parse --verify refs/remotes/trunk^0` = \
| x`git rev-parse --verify refs/heads/master^0`
| )
|
| Initialized empty Git repository in /home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/g/.git/
| Using higher level of URL: file:///home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/svnrepo => file:///home/jrn/src/git2/t/trash%20directory.t9145-git-svn-master-branch/svnrepo
| svn-remote.svn: remote ref '///home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/svnrepo/trunk:refs/remotes/trunk' must start with 'refs/'
|
| not ok - 2 git svn clone --stdlayout sets up trunk as master
| #
| # git svn clone -s "$svnrepo" g &&
| # (
| # cd g &&
| # test x`git rev-parse --verify refs/remotes/trunk^0` = \
| # x`git rev-parse --verify refs/heads/master^0`
| # )
| #
|
| # failed 1 among 2 test(s)
| 1..2
| make[3]: *** [t9145-git-svn-master-branch.sh] Error 1
Does it work for you? This is with a merge of git 1.7.8-rc0 and 1.7.7.2.
^ permalink raw reply
* Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Junio C Hamano @ 2011-11-02 18:29 UTC (permalink / raw)
To: Mika Fischer; +Cc: git, Daniel Stenberg
In-Reply-To: <1320230734-5933-1-git-send-email-mika.fischer@zoopnet.de>
Mika Fischer <mika.fischer@zoopnet.de> writes:
> Previously, when nothing could be read from the connections curl had
> open, git would just sleep unconditionally for 50ms. This patch changes
> this behavior and instead obtains the recommended timeout and the actual
> file descriptors from curl. This should eliminate time spent sleeping when
> data could actually be read/written on the socket.
>
> Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>
> ---
Thanks. I added Daniel back to Cc: list as I know he is the area expert
when it comes to the use of libcurl, and also his input helped to polish
this patch during the initial round of discussion.
> http.c | 21 ++++++++++++++++-----
> 1 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/http.c b/http.c
> index a4bc770..12180f3 100644
> --- a/http.c
> +++ b/http.c
> @@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)
> fd_set excfds;
> int max_fd;
> struct timeval select_timeout;
> + long int curl_timeout;
Just a style nit, but we usually spell this "long" not "long int" in our
codebase.
> @@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)
> }
>
> if (slot->in_use && !data_received) {
> - max_fd = 0;
> + curl_multi_timeout(curlm, &curl_timeout);
According to http://curl.haxx.se/libcurl/c/curl_multi_timeout.html
this was added in 7.15.4 which may be much newer than some of the versions
the existing code checks LIBCURL_VERSION_NUM against (grep for it in http.c).
Shouldn't you make this conditional?
> + if (curl_timeout == 0) {
> + continue;
> + } else if (curl_timeout == -1) {
> + select_timeout.tv_sec = 0;
> + select_timeout.tv_usec = 50000;
> + } else {
> + select_timeout.tv_sec = curl_timeout / 1000;
> + select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> + }
> +
> + max_fd = -1;
> FD_ZERO(&readfds);
> FD_ZERO(&writefds);
> FD_ZERO(&excfds);
> - select_timeout.tv_sec = 0;
> - select_timeout.tv_usec = 50000;
> - select(max_fd, &readfds, &writefds,
> - &excfds, &select_timeout);
> + curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
I couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
what the version requirement for using this function is, but the same
comment as above applies here.
By the way, I think I saw Daniel posting a link to a nicely formatted
table that lists each and every functions and CURLOPT_* symbol with
ranges of version it is usable, but I seem to be unable to find it.
> +
> + select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
> }
> }
> #else
^ permalink raw reply
* Re: [git patches] libata updates, GPG signed (but see admin notes)
From: Junio C Hamano @ 2011-11-02 18:58 UTC (permalink / raw)
To: Michael J Gruber
Cc: Linus Torvalds, git, James Bottomley, Jeff Garzik, Andrew Morton,
linux-ide, LKML
In-Reply-To: <4EB12122.7010803@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> The advantage of tags is that they can be added without rewriting the
> commit, of course.
And you did neither think about the downsides of tags, nor read what
others already explained for you?
^ permalink raw reply
* Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements
From: Ben Walton @ 2011-11-02 19:05 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: normalperson, git
In-Reply-To: <20111102182015.GA11401@elie.hsd1.il.comcast.net>
Excerpts from Jonathan Nieder's message of Wed Nov 02 14:20:38 -0400 2011:
> Thanks for your work on this! I'm not really sure how one can
> decide that the problem is not in svn --- some existing functions
> changed ABI in such a way as to break existing applications and
> require code changes and a recompile. It would be better for
> Subversion to silently fix up paths provided by bad callers, or at
> least to return a sensible error code.
Yes, my apologies. I wrote this commit message before fully realizing
just how wrong the response on the svn list was. It core dumps, after
all...If the patch is useful, I'll resubmit with a better commit
message.
> So the problem is that nobody who cared was testing prereleases of
> subversion and reporting bugs early enough for it to get this fixed
> before the 1.7 release. But yes, that's water under the bridge and
> git-svn (and libsvn-perl, and pysvn, and ...) should just adjust to
> the new world order.
>
> > [1] http://news.gmane.org/gmane.comp.version-control.subversion.devel
>
> Do you mean
> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/132250
> ?
No, the link should have been:
http://article.gmane.org/gmane.comp.version-control.subversion.devel/132227
I'm not sure how it got mangled like that.
> | # failed 1 among 2 test(s)
> | 1..2
> | make[3]: *** [t9145-git-svn-master-branch.sh] Error 1
>
> Does it work for you? This is with a merge of git 1.7.8-rc0 and
> 1.7.7.2.
Yikes...I had svn tests turned off in my global build script still and
only validated this against t9134. My apologies. I'll submit
something proper later this evening. (Assuming I get a working patch
that passes the whole suite.)
Sorry for the clumsy patch.
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
^ permalink raw reply
* Re: [ANNOUNCE] Git 1.7.8.rc0
From: Junio C Hamano @ 2011-11-02 19:13 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Naewe, Michael J Gruber, git
In-Reply-To: <20111102181041.GA5366@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So the ideal logic is:
>
> 1. look in netrc
>
> 2. If we have a username and no password, ask for password
>
> 3. Otherwise, try it and see if we get a 401.
>
> But we can't do that, because (1) and (3) happen atomically inside of
> curl.
>
> The simplest thing is to just drop the behavior in (2), and let it drop
> to a 401. The extra round trip probably isn't that big a deal.
That is essentially what Stefan's fix is about.
The cases we have "extra" roundtrip are:
- when you have username@ in URL but no password is stored in .netrc;
- when you have username@ in URL and no $HOME/.netrc file.
and in such a case using URL without username@ in it as a workaround would
save the roundtrip but forces you to type your username@ over and over
again, which is _not_ a real workaround.
A workaround for people who want ultimate convenience is to use .netrc to
have both username:password, but that is at the cost of potentially
reduced security. Having username@ in URL and typing password
interactively, if it worked properly, would have been the best of both
worlds.
> The other option is to start parsing netrc ourselves, or do the extra
> round trip if we detect ~/.netrc or something. But that last one is
> getting pretty hackish.
I tend to agree that we wouldn't want to parse netrc ourselves (that is
what library support e.g. CURLOPT_NETRC is for). The latter is hackish but
on the other hand it is a cheap, simple and useful hack.
How would the upcoming keystore support fit in this picture, by the way?
^ permalink raw reply
* Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Daniel Stenberg @ 2011-11-02 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mika Fischer, git
In-Reply-To: <7v62j2js3p.fsf@alter.siamese.dyndns.org>
On Wed, 2 Nov 2011, Junio C Hamano wrote:
I'm totally fine with the patch's approach with respect to how it uses libcurl
and it should be an improvement compared to the previous way.
>> + curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
>
> I couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
> what the version requirement for using this function is, but the same
> comment as above applies here.
That function has been around for as long as the multi interface has, so it
should be safe to use it just as widely as curl_multi_perform().
> By the way, I think I saw Daniel posting a link to a nicely formatted table
> that lists each and every functions and CURLOPT_* symbol with ranges of
> version it is usable, but I seem to be unable to find it.
Right, the document is a bit hard to find and I should figure out a more
prominent place to link to it. But it can be found here:
https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
--
/ daniel.haxx.se
^ permalink raw reply
* Re: [PATCH] t3200: add test case for 'branch -m'
From: Junio C Hamano @ 2011-11-02 19:43 UTC (permalink / raw)
To: Stefan Naewe; +Cc: git, Tay Ray Chuan
In-Reply-To: <1320246425-2141-1-git-send-email-stefan.naewe@gmail.com>
Thanks, both.
^ permalink raw reply
* Re: [git patches] libata updates, GPG signed (but see admin notes)
From: Linus Torvalds @ 2011-11-02 20:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, James Bottomley, Jeff Garzik, Andrew Morton, linux-ide, LKML
In-Reply-To: <7vk47jld5s.fsf@alter.siamese.dyndns.org>
On Tue, Nov 1, 2011 at 2:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> But on the other hand, in many ways, publishing your commit to the outside
> world, not necessarily for getting pulled into the final destination
> (i.e. your tree) but merely for other people to try it out, is the point
> of no return (aka "don't rewind or rebase once you publish"). "pushing
> out" might be less special than "please pull", but it still is special.
So I really think that signing the top commit itself is fundamentally wrong.
That commit may not even be *yours*. You may have pulled it from a
sub-lieutenant as a fast-forward, or similar. Amending it later would
be actively very very *wrong*.
So quite frankly, I think the stuff in pu (or next?) is completely
mis-designed. Doing it in the commit is wrong for fundamental reasons,
which all boil down to a simple issue:
- you absolutely *need* to add the signature later. You *cannot* do
it at "git commit" time.
That's a fundamental issue both from a "workflow model" issue (ie you
want to sign stuff after it has passed testing etc, but you may need
to commit it in order to *get* testing), as well as from a
"fundamental git datastructures" issue (ie you would want to sign
commits that aren't yours.
"git commit --amend" is not the answer - that destroys the fundamental
concept of history being immutable, and while it works for your local
commits, it doesn't work for anybody elses commits, or for stuff you
already pushed out.
And "add a fake empty commit just for the signature" is not the answer
either - because that is clearly inferior to the tags we already had.
I dunno. Did I miss something? As far as I can tell, the signed tags
that we've had since day one are *clearly* much better in very
fundamental ways.
Linus
^ permalink raw reply
* Re: Q: "git diff" using tag names
From: Alexey Shumkin @ 2011-11-02 20:08 UTC (permalink / raw)
To: Frans Klaver; +Cc: Ulrich Windl, git
In-Reply-To: <CAH6sp9OdjNZ6_j-eSFMpecUcxW_y6fpkDZ0cRds62wOrc12x-Q@mail.gmail.com>
thanks )
> On Wed, Nov 2, 2011 at 10:29 AM, Alexey Shumkin
> <alex.crezoff@gmail.com> wrote:
>
> >> Also it seems that both syntaxes work:
> >> git diff v0.4..v0.5
> >> git diff v0.4 v0.5
> > honestly, I do not know the difference (at the moment :))
> > may be gurus or manual will help to discover it
>
> As per the git-diff documentation, these two versions behave equally
> -- i.e. no differences.
>
> Comparing branches
> $ git diff topic master <1>
> $ git diff topic..master <2>
> $ git diff topic...master <3>
> 1. Changes between the tips of the topic and the master branches.
> 2. Same as above.
> 3. Changes that occurred on the master branch since when the topic
> branch was started off it.
>
> Cheers,
> Frans
^ permalink raw reply
* Re: Q: "git diff" using tag names
From: Alexey Shumkin @ 2011-11-02 20:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ulrich Windl, git
In-Reply-To: <7vty6mkgsg.fsf@alter.siamese.dyndns.org>
Oh, I see
> Alexey Shumkin <alex.crezoff@gmail.com> writes:
>
> >> Also it seems that both syntaxes work:
> >> git diff v0.4..v0.5
> >> git diff v0.4 v0.5
> > honestly, I do not know the difference (at the moment :))
> > may be gurus or manual will help to discover it
>
> The latter is the kosher version, as diff is about two "endpoints"
> and not about "ranges". The only reason the former is parsed without
> erroring out is because too many people are used to type .. between
> two things without thinking, learned the notation from "git log",
> which _is_ about ranges.
^ permalink raw reply
* Re: [ANNOUNCE] Git 1.7.8.rc0
From: Jeff King @ 2011-11-02 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Naewe, Michael J Gruber, git
In-Reply-To: <7vwrbiibgz.fsf@alter.siamese.dyndns.org>
On Wed, Nov 02, 2011 at 12:13:48PM -0700, Junio C Hamano wrote:
> > The simplest thing is to just drop the behavior in (2), and let it drop
> > to a 401. The extra round trip probably isn't that big a deal.
>
> That is essentially what Stefan's fix is about.
Right. I think it may be the sanest thing to do.
> The cases we have "extra" roundtrip are:
>
> - when you have username@ in URL but no password is stored in .netrc;
> - when you have username@ in URL and no $HOME/.netrc file.
>
> and in such a case using URL without username@ in it as a workaround would
> save the roundtrip but forces you to type your username@ over and over
> again, which is _not_ a real workaround.
Yeah. There's no way for us to know before we hand off to curl what you
have in netrc. So these netrc cases will always be at odds with the
no-netrc case.
Normally I would say to implement in favor of the no-netrc case, as it
is probably more common (and will hopefully be more so after the auth
helpers are finished). But the problem is that the penalties are
different. On the one hand, we have the extra http round-trip. Which is
annoying, but mostly invisible to the user. But on the other, we have
git prompting the user unnecessarily, which is just awful.
> > The other option is to start parsing netrc ourselves, or do the extra
> > round trip if we detect ~/.netrc or something. But that last one is
> > getting pretty hackish.
>
> I tend to agree that we wouldn't want to parse netrc ourselves (that is
> what library support e.g. CURLOPT_NETRC is for). The latter is hackish but
> on the other hand it is a cheap, simple and useful hack.
Note that it's not always right, of course. You might have a .netrc but
no entry for that host. But at least it lets the common case people
(i.e., people who never heard of or touched netrc) to avoid the round
trip.
> How would the upcoming keystore support fit in this picture, by the way?
Any time we would call getpass(), we ask the helper for the credential.
So for user@host, we would call out to the helper for the password
proactively, and otherwise wait for a 401.
We _could_ be proactive and actually ask the helpers for a username and
password even for "https://host/repo", which would save a round-trip to
get the 401 in some cases. But that assumes that asking the helper is
cheap. It might actually require the user inputting a password to unlock
the keystore, which would be annoying if the remote doesn't require
auth at all.
We could try to be clever and use a heuristic that fetch probably
doesn't need auth, but push does. Then fetch gets the extra round-trip
but push doesn't. But that just seems needlessly complex to save one
http round-trip on push.
-Peff
^ permalink raw reply
* [PATCHv2] Improve use of select in http backend
From: Mika Fischer @ 2011-11-02 20:21 UTC (permalink / raw)
To: git; +Cc: gitster, daniel
I've split my previous patch into two, since the two parts actually do
different things.
The first patch uses curl_multi_fdset to use the file descriptors of the http
connections in the call to select, in effect waking up immediately when new
data arrives.
The second patch uses curl_multi_timeout (if available) to get a better
recommendation for how long to sleep from curl instead of always sleeping
for 50ms.
^ permalink raw reply
* [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Mika Fischer @ 2011-11-02 20:21 UTC (permalink / raw)
To: git; +Cc: gitster, daniel, Mika Fischer
In-Reply-To: <1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de>
Instead of sleeping unconditionally for a 50ms, when no data can be read
from the http connection(s), use curl_multi_fdset to obtain the actual
file descriptors of the open connections and use them in the select call.
This way, the 50ms sleep is interrupted when new data arrives.
---
http.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index a4bc770..ae92318 100644
--- a/http.c
+++ b/http.c
@@ -664,14 +664,14 @@ void run_active_slot(struct active_request_slot *slot)
}
if (slot->in_use && !data_received) {
- max_fd = 0;
+ max_fd = -1;
FD_ZERO(&readfds);
FD_ZERO(&writefds);
FD_ZERO(&excfds);
+ curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
select_timeout.tv_sec = 0;
select_timeout.tv_usec = 50000;
- select(max_fd, &readfds, &writefds,
- &excfds, &select_timeout);
+ select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
}
}
#else
--
1.7.8.rc0.33.g09c6f.dirty
^ permalink raw reply related
* [PATCH 2/2] http.c: Use timeout suggested by curl instead of fixed 50ms timeout
From: Mika Fischer @ 2011-11-02 20:21 UTC (permalink / raw)
To: git; +Cc: gitster, daniel, Mika Fischer
In-Reply-To: <1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de>
Recent versions of curl can suggest a period of time the library user
should sleep and try again, when curl is blocked on reading or writing
(or connecting). Use this timeout instead of always sleeping for 50ms.
---
http.c | 22 ++++++++++++++++++++--
1 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index ae92318..e91a2ab 100644
--- a/http.c
+++ b/http.c
@@ -649,6 +649,9 @@ void run_active_slot(struct active_request_slot *slot)
fd_set excfds;
int max_fd;
struct timeval select_timeout;
+#if LIBCURL_VERSION_NUM >= 0x070f04
+ long curl_timeout;
+#endif
int finished = 0;
slot->finished = &finished;
@@ -664,13 +667,28 @@ void run_active_slot(struct active_request_slot *slot)
}
if (slot->in_use && !data_received) {
+#if LIBCURL_VERSION_NUM >= 0x070f04
+ curl_multi_timeout(curlm, &curl_timeout);
+ if (curl_timeout == 0) {
+ continue;
+ } else if (curl_timeout == -1) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 50000;
+ } else {
+ select_timeout.tv_sec = curl_timeout / 1000;
+ select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
+ }
+#else
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 50000;
+#endif
+
max_fd = -1;
FD_ZERO(&readfds);
FD_ZERO(&writefds);
FD_ZERO(&excfds);
curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
- select_timeout.tv_sec = 0;
- select_timeout.tv_usec = 50000;
+
select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
}
}
--
1.7.8.rc0.33.g09c6f.dirty
^ permalink raw reply related
* Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Jeff King @ 2011-11-02 20:32 UTC (permalink / raw)
To: Mika Fischer; +Cc: git, gitster, daniel
In-Reply-To: <1320265288-12647-2-git-send-email-mika.fischer@zoopnet.de>
On Wed, Nov 02, 2011 at 09:21:27PM +0100, Mika Fischer wrote:
> Instead of sleeping unconditionally for a 50ms, when no data can be read
> from the http connection(s), use curl_multi_fdset to obtain the actual
> file descriptors of the open connections and use them in the select call.
> This way, the 50ms sleep is interrupted when new data arrives.
> ---
> http.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/http.c b/http.c
> index a4bc770..ae92318 100644
> --- a/http.c
> +++ b/http.c
> @@ -664,14 +664,14 @@ void run_active_slot(struct active_request_slot *slot)
> }
>
> if (slot->in_use && !data_received) {
> - max_fd = 0;
> + max_fd = -1;
> FD_ZERO(&readfds);
> FD_ZERO(&writefds);
> FD_ZERO(&excfds);
> + curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> select_timeout.tv_sec = 0;
> select_timeout.tv_usec = 50000;
> - select(max_fd, &readfds, &writefds,
> - &excfds, &select_timeout);
> + select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
> }
> }
Do we still need to care about data_received?
My understanding was that the code was originally trying to do:
1. Call curl, maybe get some data.
2. If we got data, then ask curl against immediately for some data.
3. Otherwise, sleep 50ms and then ask curl again.
But now that we are actually selecting on the proper descriptors, it
should now be safe to just do:
1. Call curl, maybe get some data.
2. Call select, which will wake immediately if curl is going to get
data.
At least that's my reading. I am working on unrelated patches that clean
up the handling of data_received, but if it could go away altogether,
that would be even simpler.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Jeff King @ 2011-11-02 20:35 UTC (permalink / raw)
To: Mika Fischer; +Cc: git, gitster, daniel
In-Reply-To: <20111102203221.GB5628@sigill.intra.peff.net>
On Wed, Nov 02, 2011 at 04:32:21PM -0400, Jeff King wrote:
> At least that's my reading. I am working on unrelated patches that clean
> up the handling of data_received, but if it could go away altogether,
> that would be even simpler.
That patch, btw, looks like this:
-- >8 --
Subject: [PATCH] http: remove "local" member from slot struct
The curl-multi http code does something like this:
while (!finished) {
try_to_read_from_slots();
if (!data_received)
wait_for_50_ms();
}
Which is horrible enough in itself, because of the
hard-coded 50ms wait.
But there's some additional complexity: the method for
finding whether we received data is to actually run ftell()
on the file we are writing to to see if it got anything. So
the "local" member of the slot struct contains the FILE
pointer. Except that sometimes we don't have a file, because
we're writing to a strbuf. In that case, since curl calls
our custom callback, we just increment the data_received
flag when curl gives data to our callback.
Let's do the same thing for the write-to-file case as we do
for the write-to-strbuf case: use a thin wrapper callback
and increment the received flag. This makes both methods
consistent with each other, and saves us from managing the
"local" struct member at all, reducing the code size.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 23 +++++++----------------
http.h | 1 -
2 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/http.c b/http.c
index a4bc770..99386ef 100644
--- a/http.c
+++ b/http.c
@@ -93,6 +93,12 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
}
#endif
+size_t fwrite_file(char *ptr, size_t eltsize, size_t nmemb, void *handle)
+{
+ data_received++;
+ return fwrite(ptr, eltsize, nmemb, handle);
+}
+
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -538,7 +544,6 @@ struct active_request_slot *get_active_slot(void)
active_requests++;
slot->in_use = 1;
- slot->local = NULL;
slot->results = NULL;
slot->finished = NULL;
slot->callback_data = NULL;
@@ -642,8 +647,6 @@ void step_active_slots(void)
void run_active_slot(struct active_request_slot *slot)
{
#ifdef USE_CURL_MULTI
- long last_pos = 0;
- long current_pos;
fd_set readfds;
fd_set writefds;
fd_set excfds;
@@ -656,13 +659,6 @@ void run_active_slot(struct active_request_slot *slot)
data_received = 0;
step_active_slots();
- if (!data_received && slot->local != NULL) {
- current_pos = ftell(slot->local);
- if (current_pos > last_pos)
- data_received++;
- last_pos = current_pos;
- }
-
if (slot->in_use && !data_received) {
max_fd = 0;
FD_ZERO(&readfds);
@@ -818,13 +814,12 @@ static int http_request(const char *url, void *result, int target, int options)
if (target == HTTP_REQUEST_FILE) {
long posn = ftell(result);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
- fwrite);
+ fwrite_file);
if (posn > 0) {
strbuf_addf(&buf, "Range: bytes=%ld-", posn);
headers = curl_slist_append(headers, buf.buf);
strbuf_reset(&buf);
}
- slot->local = result;
} else
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
fwrite_buffer);
@@ -871,7 +866,6 @@ static int http_request(const char *url, void *result, int target, int options)
ret = HTTP_START_FAILED;
}
- slot->local = NULL;
curl_slist_free_all(headers);
strbuf_release(&buf);
@@ -1066,7 +1060,6 @@ void release_http_pack_request(struct http_pack_request *preq)
if (preq->packfile != NULL) {
fclose(preq->packfile);
preq->packfile = NULL;
- preq->slot->local = NULL;
}
if (preq->range_header != NULL) {
curl_slist_free_all(preq->range_header);
@@ -1088,7 +1081,6 @@ int finish_http_pack_request(struct http_pack_request *preq)
fclose(preq->packfile);
preq->packfile = NULL;
- preq->slot->local = NULL;
lst = preq->lst;
while (*lst != p)
@@ -1157,7 +1149,6 @@ struct http_pack_request *new_http_pack_request(
}
preq->slot = get_active_slot();
- preq->slot->local = preq->packfile;
curl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);
curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
diff --git a/http.h b/http.h
index 3c332a9..7429381 100644
--- a/http.h
+++ b/http.h
@@ -49,7 +49,6 @@ struct slot_results {
struct active_request_slot {
CURL *curl;
- FILE *local;
int in_use;
CURLcode curl_result;
long http_code;
--
1.7.7.rc3.12.g571d67
^ permalink raw reply related
* Re: [git patches] libata updates, GPG signed (but see admin notes)
From: Michael J Gruber @ 2011-11-02 21:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, git, James Bottomley, Jeff Garzik, Andrew Morton,
linux-ide, LKML
In-Reply-To: <7v1utqjqre.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 02.11.2011 19:58:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> The advantage of tags is that they can be added without rewriting the
>> commit, of course.
>
> And you did neither think about the downsides of tags, nor read what
> others already explained for you?
We're just weighing things differently here, and no accusations of
"misinformation" or "not thinking" will change this.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox