Re: [BUG] Infinite make recursion when configure.ac changes
Jeff King writes: > Anyway, here is the patch to fix the loop. > > -- >8 -- > Subject: [PATCH] Makefile: avoid infinite loop on configure.ac change > > If you are using autoconf and change the configure.ac, the > Makefile will notice that config.status is older than > configure.ac, and will attempt to rebuild and re-run the > configure script to pick up your changes. The first step in > doing so is to run "make configure". Unfortunately, this > tries to include config.mak.autogen, which depends on > config.status, which depends on configure.ac; so we must > rebuild config.status. Which leads to us running "make > configure", and so on. > > It's easy to demonstrate with: > > make configure > ./configure > touch configure.ac > make > > We can break this cycle by not re-invoking make to build > "configure", and instead just putting its rules inline into > our config.status rebuild procedure. We can avoid a copy by > factoring the rules into a make variable. > > Signed-off-by: Jeff King > --- Thanks. This needs to go to both v1.8.1.x and v1.8.2. Will apply. > Makefile | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 3b2c92c..ee1c0b0 100644 > --- a/Makefile > +++ b/Makefile > @@ -1871,12 +1871,14 @@ configure: configure.ac GIT-VERSION-FILE > mv $@+ $@ > endif # NO_PYTHON > > +CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \ > +sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ > + configure.ac >configure.ac+ && \ > +autoconf -o configure configure.ac+ && \ > +$(RM) configure.ac+ > + > configure: configure.ac GIT-VERSION-FILE > - $(QUIET_GEN)$(RM) $@ $<+ && \ > - sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ > - $< > $<+ && \ > - autoconf -o $@ $<+ && \ > - $(RM) $<+ > + $(QUIET_GEN)$(CONFIGURE_RECIPE) > > ifdef AUTOCONFIGURED > # We avoid depending on 'configure' here, because it gets rebuilt > @@ -1885,7 +1887,7 @@ config.status: configure.ac > # do want to recheck when the platform/environment detection logic > # changes, hence this depends on configure.ac. > config.status: configure.ac > - $(QUIET_GEN)$(MAKE) configure && \ > + $(QUIET_GEN)$(CONFIGURE_RECIPE) && \ > if test -f config.status; then \ > ./config.status --recheck; \ > else \ -- 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
products from links of london skull bracelet combine style
Jewellery has been worn by mankind for generations, and meant different things to different people and cultures. Throughout history human beings have sought out items with which to adorn themselves, sometimes for ornament, sometimes for a deeper meaning. Today people tend to have certain expectations of their jewellery,http://www.cheapcheaplinksoflondonbracelets.co.uk/ that it should be high quality, fashionable and attractive. These are criteria which links of london sale aim to meet every time with their beautiful modern collections. Whether you are looking for an amazing gift for someone special, or looking to treat yourself, there may well be an item which is just right for you. The history of the company is interesting, and goes to show that even internationally recognised brands start out small to begin with! If you are considering purchasing their products, some company history and additional information may help to finalise a decision one way or the other. http://www.cheapcheaplinksoflondonbracelets.co.uk/ -- View this message in context: http://git.661346.n2.nabble.com/products-from-links-of-london-skull-bracelet-combine-style-tp7578291.html Sent from the git mailing list archive at Nabble.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: [BUG] Infinite make recursion when configure.ac changes
On Wed, Feb 20, 2013 at 10:10:42PM -0800, Junio C Hamano wrote: > I noticed this while looking at the other autoconf patch yesterday, > but I was otherwise occupied in the evening and did not pursue it > further. Thanks for looking into it. Here's the patch with a commit message. I'm pretty sure this is the right thing to do. I was tempted to find a way of telling make "no, don't include config.mak.autogen, but that actually _isn't_ right. It might be defining $(RM) or some other variable that we use in the recipe. By including it inline, we will use whatever is in the current config.mak.autogen (that we read when we started this make invocation), which is better than nothing. > This may be an unrelated issue, but I've always thought that it was > strange and extremely unintuitive that running "make configure" once > only creates config.mak.autogen, while running it once again after > that (i.e. while having config.mak.autogen in the tree) seems to run > the resulting "./configure" as well. Maybe it is just me. It's not the "configure" target that causes it to run. It's the fact that we include config.mak.autogen, which we then tell make can be rebuilt by running ./configure. _But_ we only tell it so if one already exists (since otherwise everybody would get automake cruft). This is the "ifdef AUTOCONFIGURED" you see. So I think the rule makes sense. Once you have told the Makefile that you are interested in autoconf (by running configure once), then it rebuilds it as necessary, and it should be consistent. You can opt back out of it be removing config.mak.autogen. Anyway, here is the patch to fix the loop. -- >8 -- Subject: [PATCH] Makefile: avoid infinite loop on configure.ac change If you are using autoconf and change the configure.ac, the Makefile will notice that config.status is older than configure.ac, and will attempt to rebuild and re-run the configure script to pick up your changes. The first step in doing so is to run "make configure". Unfortunately, this tries to include config.mak.autogen, which depends on config.status, which depends on configure.ac; so we must rebuild config.status. Which leads to us running "make configure", and so on. It's easy to demonstrate with: make configure ./configure touch configure.ac make We can break this cycle by not re-invoking make to build "configure", and instead just putting its rules inline into our config.status rebuild procedure. We can avoid a copy by factoring the rules into a make variable. Signed-off-by: Jeff King --- Makefile | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 3b2c92c..ee1c0b0 100644 --- a/Makefile +++ b/Makefile @@ -1871,12 +1871,14 @@ configure: configure.ac GIT-VERSION-FILE mv $@+ $@ endif # NO_PYTHON +CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \ + sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ + configure.ac >configure.ac+ && \ + autoconf -o configure configure.ac+ && \ + $(RM) configure.ac+ + configure: configure.ac GIT-VERSION-FILE - $(QUIET_GEN)$(RM) $@ $<+ && \ - sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ - $< > $<+ && \ - autoconf -o $@ $<+ && \ - $(RM) $<+ + $(QUIET_GEN)$(CONFIGURE_RECIPE) ifdef AUTOCONFIGURED # We avoid depending on 'configure' here, because it gets rebuilt @@ -1885,7 +1887,7 @@ config.status: configure.ac # do want to recheck when the platform/environment detection logic # changes, hence this depends on configure.ac. config.status: configure.ac - $(QUIET_GEN)$(MAKE) configure && \ + $(QUIET_GEN)$(CONFIGURE_RECIPE) && \ if test -f config.status; then \ ./config.status --recheck; \ else \ -- 1.8.1.4.4.g265d2fa -- 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: [BUG] Infinite make recursion when configure.ac changes
Jeff King writes: > I can easily replicate it here. > >> This seems due to the change in commit v1.7.12.4-1-g1226504: the >> issue is still present there, but no longer is in the preceding >> commit 7e201053 (a.k.a. v1.7.12.4). >> >> I haven't investigated this any further for the moment. > > Hmm. It looks like config.status depends on configure.ac. So make > rebuilds config.status, which runs "make configure", which includes > "config.mak.autogen", leading "make" to think that it should rebuild the > include file to make sure it is up to date. The include file depends on > "config.status", which needs to be rebuilt due to configure.ac, which > entails running "make configure", and so on. I noticed this while looking at the other autoconf patch yesterday, but I was otherwise occupied in the evening and did not pursue it further. Thanks for looking into it. This may be an unrelated issue, but I've always thought that it was strange and extremely unintuitive that running "make configure" once only creates config.mak.autogen, while running it once again after that (i.e. while having config.mak.autogen in the tree) seems to run the resulting "./configure" as well. Maybe it is just me. -- 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: [BUG] Infinite make recursion when configure.ac changes
On Wed, Feb 20, 2013 at 09:44:58AM +0100, Stefano Lattarini wrote: > From a pristine master checkout: > > $ make configure && ./configure make > ... # All successfull > $ touch configure.ac > $ make > GEN config.status > make[1]: Entering directory `/storage/home/stefano/git/src' > GEN config.status > make[2]: Entering directory `/storage/home/stefano/git/src' > GEN config.status > make[3]: Entering directory `/storage/home/stefano/git/src' > GEN config.status > make[4]: Entering directory `/storage/home/stefano/git/src' > GEN config.status > make[5]: Entering directory `/storage/home/stefano/git/src' > GEN config.status > ... > > and I have to hit ^C to interrupt that recursion. I can easily replicate it here. > This seems due to the change in commit v1.7.12.4-1-g1226504: the > issue is still present there, but no longer is in the preceding > commit 7e201053 (a.k.a. v1.7.12.4). > > I haven't investigated this any further for the moment. Hmm. It looks like config.status depends on configure.ac. So make rebuilds config.status, which runs "make configure", which includes "config.mak.autogen", leading "make" to think that it should rebuild the include file to make sure it is up to date. The include file depends on "config.status", which needs to be rebuilt due to configure.ac, which entails running "make configure", and so on. So the real problem is that make things that "make configure" depends on "config.mak.autogen" being up to date, which isn't true at all; the whole point is to make a new configure script so we can write a new config.mak.autogen. The simplest thing is to just avoid re-running "make" to get the configure script; the only thing we are gaining is that we don't have to repeat the build recipe, but we can sneak around that by sticking it in a variable. Something like this seems to work: diff --git a/Makefile b/Makefile index 3b2c92c..ee1c0b0 100644 --- a/Makefile +++ b/Makefile @@ -1871,12 +1871,14 @@ configure: configure.ac GIT-VERSION-FILE mv $@+ $@ endif # NO_PYTHON +CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \ + sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ + configure.ac >configure.ac+ && \ + autoconf -o configure configure.ac+ && \ + $(RM) configure.ac+ + configure: configure.ac GIT-VERSION-FILE - $(QUIET_GEN)$(RM) $@ $<+ && \ - sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ - $< > $<+ && \ - autoconf -o $@ $<+ && \ - $(RM) $<+ + $(QUIET_GEN)$(CONFIGURE_RECIPE) ifdef AUTOCONFIGURED # We avoid depending on 'configure' here, because it gets rebuilt @@ -1885,7 +1887,7 @@ config.status: configure.ac # do want to recheck when the platform/environment detection logic # changes, hence this depends on configure.ac. config.status: configure.ac - $(QUIET_GEN)$(MAKE) configure && \ + $(QUIET_GEN)$(CONFIGURE_RECIPE) && \ if test -f config.status; then \ ./config.status --recheck; \ else \ -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: [RFH/PATCH] imap-send: support SNI (RFC4366)
On Wed, Feb 20, 2013 at 09:35:16PM -0800, Junio C Hamano wrote:
>> (2) I do not know if everybody has SSL_set_tslext_host_name() macro
>> defined, so this patch may be breaking build for people with
>> different versions of OpenSSL.
> [...]
>
> +#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
> + /*
> + * SNI (RFC4366)
> + * OpenSSL does not document this function, but the implementation
> + * returns 1 on success, 0 on failure after calling SSLerr().
> + */
> + ret = SSL_set_tlsext_host_name(sock->ssl, server.host);
> + if (ret != 1)
> + warning("SSL_set_tslext_host_name(%s) failed.\n", server.host);
> +#endif
Yes, I think this is the right macro to check. According to OpenSSL's
CHANGES file, it was introduced between 0.9.8n and 1.0.0 (Mar 2010). But
I note that the use of the same macro in libcurl dates to 2008. Curious.
Note that you have a typo in your warning text (tslext) and an
extra newline.
As far as testing goes, I don't have an SNI IMAP server handy, but I
think you can simulate one with "openssl s_server". It may be a good
long-term goal to test any ssl-specific code against that in our test
suite (on the other hand, most of the interesting stuff is https, where
the details are all handled by curl).
-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: [RFH/PATCH] imap-send: support SNI (RFC4366)
Junio C Hamano writes:
> To talk to a site that serves multiple names on a single IP address,
> the client needs to ask for the specific hostname it wants to talk
> to. Otherwise, the default certificate returned from the IP address
> may not match that of the host we wanted to talk to.
>
> Signed-off-by: Junio C Hamano
> ---
>
> * I need help from people on this patch in two areas:
>
> (1) I only tested this patch by connecting to https://googlemail.com/
> with
> ...
> I would appreciate it if somebody knows an imap server that
> needs SNI and runs an end-to-end test against that server.
>
> (2) I do not know if everybody has SSL_set_tslext_host_name() macro
> defined, so this patch may be breaking build for people with
> different versions of OpenSSL.
What I queued for tonight replaces the posted patch with this
version in order to address (2) above.
-- >8 --
Subject: [PATCH] imap-send: support SNI (RFC4366)
To talk with some sites that serve multiple names on a single IP
address, the client needs to ask for the specific host it wants to
talk to.
Signed-off-by: Junio C Hamano
---
imap-send.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/imap-send.c b/imap-send.c
index 171c887..ab2098a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -370,6 +370,17 @@ static int ssl_socket_connect(struct imap_socket *sock,
int use_tls_only, int ve
return -1;
}
+#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
+ /*
+* SNI (RFC4366)
+* OpenSSL does not document this function, but the implementation
+* returns 1 on success, 0 on failure after calling SSLerr().
+*/
+ ret = SSL_set_tlsext_host_name(sock->ssl, server.host);
+ if (ret != 1)
+ warning("SSL_set_tslext_host_name(%s) failed.\n", server.host);
+#endif
+
ret = SSL_connect(sock->ssl);
if (ret <= 0) {
socket_perror("SSL_connect", sock, ret);
--
1.8.2.rc0.127.g4d5d7da
--
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 v3 4/4] t7800: "defaults" is no longer a builtin tool name
David Aguilar writes: > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index fb00273..21fbba9 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' ' > ' > > test_expect_success PERL 'custom tool commands override built-ins' ' > - test_config difftool.defaults.cmd "cat \"\$REMOTE\"" && > + test_config difftool.vimdiff "cat \"\$REMOTE\"" && > echo master >expect && > - git difftool --tool defaults --no-prompt branch >actual && > + git difftool --tool vimdiff --no-prompt branch >actual && > test_cmp expect actual > ' Eek. $ sh t7800-difftool.sh -i ok 1 - setup ok 2 - custom commands not ok 3 - custom tool commands override built-ins # # test_config difftool.vimdiff "cat \"\$REMOTE\"" && # echo master >expect && # git difftool --tool vimdiff --no-prompt branch >actual && # test_cmp expect actual # Running the same test with "-v" seems to get stuck with this forever: expecting success: test_config difftool.vimdiff "cat \"\$REMOTE\"" && echo master >expect && git difftool --tool vimdiff --no-prompt branch >actual && test_cmp expect actual Vim: Warning: Output is not to a terminal Vim: Warning: Input is not from a terminal -- 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 v3 4/4] t7800: "defaults" is no longer a builtin tool name
Thanks; will replace and queue. -- 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 3/4] t7800: modernize tests
Eliminate a lot of redundant work by using test_config().
Catch more return codes by more use of temporary files
and test_cmp.
The original tests relied upon restore_test_defaults()
from the previous test to provide the next test with a sane
environment. Make the tests do their own setup so that they
are not dependent on the success of the previous test.
The end result is shorter tests and better test isolation.
Signed-off-by: David Aguilar
---
v3 includes Junio's review notes to avoid cat with stdin_contains
and to use DQ around $LOCAL.
Another difference from v2 is that it tweaks the function
declarations to keep a SP between the name and parens to better
follow the standard git style.
t/t7800-difftool.sh | 366
1 file changed, 168 insertions(+), 198 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 5b5939b..fb00273 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,43 +10,25 @@ Testing basic diff tool invocation
. ./test-lib.sh
-remove_config_vars()
+difftool_test_setup ()
{
- # Unset all config variables used by git-difftool
- git config --unset diff.tool
- git config --unset diff.guitool
- git config --unset difftool.test-tool.cmd
- git config --unset difftool.prompt
- git config --unset merge.tool
- git config --unset mergetool.test-tool.cmd
- git config --unset mergetool.prompt
- return 0
+ test_config diff.tool test-tool &&
+ test_config difftool.test-tool.cmd 'cat "$LOCAL"' &&
+ test_config difftool.bogus-tool.cmd false
}
-restore_test_defaults()
-{
- # Restores the test defaults used by several tests
- remove_config_vars
- unset GIT_DIFF_TOOL
- unset GIT_DIFFTOOL_PROMPT
- unset GIT_DIFFTOOL_NO_PROMPT
- git config diff.tool test-tool &&
- git config difftool.test-tool.cmd 'cat $LOCAL'
- git config difftool.bogus-tool.cmd false
-}
-
-prompt_given()
+prompt_given ()
{
prompt="$1"
test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
}
-stdin_contains()
+stdin_contains ()
{
grep >/dev/null "$1"
}
-stdin_doesnot_contain()
+stdin_doesnot_contain ()
{
! stdin_contains "$1"
}
@@ -65,249 +47,237 @@ test_expect_success PERL 'setup' '
# Configure a custom difftool..cmd and use it
test_expect_success PERL 'custom commands' '
- restore_test_defaults &&
- git config difftool.test-tool.cmd "cat \$REMOTE" &&
+ difftool_test_setup &&
+ test_config difftool.test-tool.cmd "cat \"\$REMOTE\"" &&
+ echo master >expect &&
+ git difftool --no-prompt branch >actual &&
+ test_cmp expect actual &&
- diff=$(git difftool --no-prompt branch) &&
- test "$diff" = "master" &&
-
- restore_test_defaults &&
- diff=$(git difftool --no-prompt branch) &&
- test "$diff" = "branch"
+ test_config difftool.test-tool.cmd "cat \"\$LOCAL\"" &&
+ echo branch >expect &&
+ git difftool --no-prompt branch >actual &&
+ test_cmp expect actual
'
-# Ensures that a custom difftool..cmd overrides built-ins
-test_expect_success PERL 'custom commands override built-ins' '
- restore_test_defaults &&
- git config difftool.defaults.cmd "cat \$REMOTE" &&
-
- diff=$(git difftool --tool defaults --no-prompt branch) &&
- test "$diff" = "master" &&
-
- git config --unset difftool.defaults.cmd
+test_expect_success PERL 'custom tool commands override built-ins' '
+ test_config difftool.defaults.cmd "cat \"\$REMOTE\"" &&
+ echo master >expect &&
+ git difftool --tool defaults --no-prompt branch >actual &&
+ test_cmp expect actual
'
-# Ensures that git-difftool ignores bogus --tool values
test_expect_success PERL 'difftool ignores bad --tool values' '
- diff=$(git difftool --no-prompt --tool=bad-tool branch)
- test "$?" = 1 &&
- test "$diff" = ""
+ : >expect &&
+ test_expect_code 1 \
+ git difftool --no-prompt --tool=bad-tool branch >actual &&
+ test_cmp expect actual
'
test_expect_success PERL 'difftool forwards arguments to diff' '
+ difftool_test_setup &&
>for-diff &&
git add for-diff &&
echo changes>for-diff &&
git add for-diff &&
- diff=$(git difftool --cached --no-prompt -- for-diff) &&
- test "$diff" = "" &&
+ : >expect &&
+ git difftool --cached --no-prompt -- for-diff >actual &&
+ test_cmp expect actual &&
git reset -- for-diff &&
rm for-diff
'
test_expect_success PERL 'difftool honors --gui' '
- git config merge.tool bogus-tool &&
- git config diff.tool bogus-tool &&
- git config diff.guitool test-tool &&
-
- diff=$(git difftool --no-prompt --gui branch) &&
- test "$diff" = "branch" &&
+ difftool_test_setup &&
+ test_config merge.tool bogus-tool &&
+
[PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name
073678b8e6324a155fa99f40eee0637941a70a34 reworked the mergetools/ directory so that every file corresponds to a difftool-supported tool. When this happened the "defaults" file went away as it was no longer needed by mergetool--lib. t7800 tests that configured commands can override builtins, but this test was not adjusted when the "defaults" file was removed because the test continued to pass. Adjust the test to use the everlasting "vimdiff" tool name instead of "defaults" so that it correctly tests against a tool that is known by mergetool--lib. Signed-off-by: David Aguilar --- Rebased against PATCH v3 3/4. t/t7800-difftool.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index fb00273..21fbba9 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' ' ' test_expect_success PERL 'custom tool commands override built-ins' ' - test_config difftool.defaults.cmd "cat \"\$REMOTE\"" && + test_config difftool.vimdiff "cat \"\$REMOTE\"" && echo master >expect && - git difftool --tool defaults --no-prompt branch >actual && + git difftool --tool vimdiff --no-prompt branch >actual && test_cmp expect actual ' -- 1.8.2.rc0.20.gf548dd7 -- 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 2/4] t7800: update copyright notice
Signed-off-by: David Aguilar --- Unchanged since v2. t/t7800-difftool.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index eb1d3f8..5b5939b 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -1,6 +1,6 @@ #!/bin/sh # -# Copyright (c) 2009, 2010 David Aguilar +# Copyright (c) 2009, 2010, 2012, 2013 David Aguilar # test_description='git-difftool -- 1.8.2.rc0.20.gf548dd7 -- 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/4] difftool: silence uninitialized variable warning
Git::config() returns `undef` when given keys that do not exist.
Check that the $guitool value is defined to prevent a noisy
"Use of uninitialized variable $guitool in length" warning.
Signed-off-by: David Aguilar
---
Unchanged since v1.
git-difftool.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..12231fb 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -336,7 +336,7 @@ sub main
}
if ($opts{gui}) {
my $guitool = Git::config('diff.guitool');
- if (length($guitool) > 0) {
+ if (defined($guitool) && length($guitool) > 0) {
$ENV{GIT_DIFF_TOOL} = $guitool;
}
}
--
1.8.2.rc0.20.gf548dd7
--
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
[RFH/PATCH] imap-send: support SNI (RFC4366)
To talk to a site that serves multiple names on a single IP address,
the client needs to ask for the specific hostname it wants to talk
to. Otherwise, the default certificate returned from the IP address
may not match that of the host we wanted to talk to.
Signed-off-by: Junio C Hamano
---
* I need help from people on this patch in two areas:
(1) I only tested this patch by connecting to https://googlemail.com/
with
$ git -c imap.host=imaps://googlemail.com -c imap.port=443 imap-send
ssl, server.host);
+ if (ret != 1)
+ warning("SSL_set_tslext_host_name(%s) failed.\n", server.host);
+
ret = SSL_connect(sock->ssl);
if (ret <= 0) {
socket_perror("SSL_connect", sock, ret);
--
1.8.2.rc0.106.ga6e4a61
--
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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King writes:
> I am more concerned that the assertion is not "oops, another thread is
> doing something crazy, and it is a bug", but rather that there is some
> weird platform where SIG_DFL does not kill the program under SIGPIPE.
> That seems pretty crazy, though. I think I'd squash in something like
> this:
>
> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..abb64db 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -5,7 +5,9 @@ static void check_pipe(int err)
> if (err == EPIPE) {
> signal(SIGPIPE, SIG_DFL);
> raise(SIGPIPE);
> +
> /* Should never happen, but just in case... */
> + error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
> exit(141);
> }
> }
>
> which more directly reports the assertion that failed, and degrades
> reasonably gracefully. Yeah, it's probably overengineering, but it's
> easy enough to do.
Yeah, that sounds like a sensible thing to do, as it is cheap even
though we do not expect it to trigger.
--
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: Merge with staged and unstaged changes
Edward Thomson writes: > I also appreciate your explanation of the affect of the workdir, > and that makes sense. I would have expected that the default was > to presume the workdir files were existent, rather than the > other way around, but we can agree that is an implementation detail. > > My biggest concern, of course, was having the unstaged files in my > workdir overwritten or deleted. Oh, no question about that part. You concluded your original message with: >> I trust the last two cases, where data is lost, are bugs to >> report, but could I get clarification on the other situations? and I was responding to the part after the "but could I get...". I am fairly familiar with the "read-tree -m -u O A B" three-way merge codepath (after all I designed that with Linus in the very early days of Git), but I am not as familar with the merge-recursive backend as merge-resolve, and I was hoping to see the "bug" part triaged by other people. -- 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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 02:06:37PM -0800, Jonathan Nieder wrote:
> > I don't mind adding a "BUG: " message like you described, but we should
> > still try to exit(141) as the backup, since that is the shell-equivalent
> > code to the SIGPIPE signal death.
>
> If you want. :)
>
> I think caring about graceful degradation of behavior in the case of
> an assertion failure is overengineering, but it's mostly harmless.
I am more concerned that the assertion is not "oops, another thread is
doing something crazy, and it is a bug", but rather that there is some
weird platform where SIG_DFL does not kill the program under SIGPIPE.
That seems pretty crazy, though. I think I'd squash in something like
this:
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..abb64db 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -5,7 +5,9 @@ static void check_pipe(int err)
if (err == EPIPE) {
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
+
/* Should never happen, but just in case... */
+ error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
exit(141);
}
}
which more directly reports the assertion that failed, and degrades
reasonably gracefully. Yeah, it's probably overengineering, but it's
easy enough to do.
-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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
> On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:
How about
die("BUG: another thread changed SIGPIPE handling behind my
back!");
to make it easier to find and fix such problems?
>>>
>>> You mean for the "should never happen" bit, not the first part, right? I
>>> actually wonder if we should simply exit(141) in the first place. That
>>> is shell exit-code for SIGPIPE death already (so it's what our
>>> run_command would show us, and what anybody running us through shell
>>> would see).
>>
>> Yes, for the "should never happen" part.
[...]
> I don't mind adding a "BUG: " message like you described, but we should
> still try to exit(141) as the backup, since that is the shell-equivalent
> code to the SIGPIPE signal death.
If you want. :)
I think caring about graceful degradation of behavior in the case of
an assertion failure is overengineering, but it's mostly harmless.
--
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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:
> >> How about
> >>
> >>die("BUG: another thread changed SIGPIPE handling behind my
> >> back!");
> >>
> >> to make it easier to find and fix such problems?
> >
> > You mean for the "should never happen" bit, not the first part, right? I
> > actually wonder if we should simply exit(141) in the first place. That
> > is shell exit-code for SIGPIPE death already (so it's what our
> > run_command would show us, and what anybody running us through shell
> > would see).
>
> Yes, for the "should never happen" part. Raising a signal is nice
> because it means the wait()-ing process can see what happened by
> checking WIFSIGNALED(status).
Right. My point is that only happens if there's no shell in the way. But
I guess it doesn't hurt to make the attempt to help the people using
wait() directly.
I don't mind adding a "BUG: " message like you described, but we should
still try to exit(141) as the backup, since that is the shell-equivalent
code to the SIGPIPE signal death.
-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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
> On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
>>> + if (err == EPIPE) {
>>> + signal(SIGPIPE, SIG_DFL);
>>> + raise(SIGPIPE);
>>> + /* Should never happen, but just in case... */
>>> + exit(141);
>>
>> How about
>>
>> die("BUG: another thread changed SIGPIPE handling behind my
>> back!");
>>
>> to make it easier to find and fix such problems?
>
> You mean for the "should never happen" bit, not the first part, right? I
> actually wonder if we should simply exit(141) in the first place. That
> is shell exit-code for SIGPIPE death already (so it's what our
> run_command would show us, and what anybody running us through shell
> would see).
Yes, for the "should never happen" part. Raising a signal is nice
because it means the wait()-ing process can see what happened by
checking WIFSIGNALED(status).
Jonathan
--
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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
> > This distinction doesn't typically matter in git, because we
> > do not ignore SIGPIPE in the first place. Which means that
> > we will not get EPIPE, but instead will just die when we get
> > a SIGPIPE. But it's possible for a default handler to be set
> > by a parent process,
>
> Not so much "default" as "insane inherited", as in the example
> of old versions of Python's subprocess.Popen.
It's possible that somebody could have a legitimate reason for doing so.
I just can't think of one. :)
> I suspect this used exit(0) instead of raise(SIGPIPE) in the first
> place to work around a bash bug (too much verbosity about SIGPIPE).
> If any programs still have that kind of bug, I'd rather put pressure
> on them to fix it by *not* working around it. So the basic idea here
> looks good to me.
Yeah, if you look for old discussions on SIGPIPE in the git list, it is
mostly Linus complaining about the bash behavior, and this code does
date back to that era. The bash bug is long since fixed.
> > + if (err == EPIPE) {
> > + signal(SIGPIPE, SIG_DFL);
> > + raise(SIGPIPE);
> > + /* Should never happen, but just in case... */
> > + exit(141);
>
> How about
>
> die("BUG: another thread changed SIGPIPE handling behind my
> back!");
>
> to make it easier to find and fix such problems?
You mean for the "should never happen" bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).
-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 v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
> The write_or_die function will always die on an error,
> including EPIPE. However, it currently treats EPIPE
> specially by suppressing any error message, and by exiting
> with exit code 0.
>
> Suppressing the error message makes some sense; a pipe death
> may just be a sign that the other side is not interested in
> what we have to say. However, exiting with a successful
> error code is not a good idea, as write_or_die is frequently
> used in cases where we want to be careful about having
> written all of the output, and we may need to signal to our
> caller that we have done so (e.g., you would not want a push
> whose other end has hung up to report success).
>
> This distinction doesn't typically matter in git, because we
> do not ignore SIGPIPE in the first place. Which means that
> we will not get EPIPE, but instead will just die when we get
> a SIGPIPE. But it's possible for a default handler to be set
> by a parent process,
Not so much "default" as "insane inherited", as in the example
of old versions of Python's subprocess.Popen.
I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it. So the basic idea here
looks good to me.
[...]
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -1,5 +1,15 @@
> #include "cache.h"
>
> +static void check_pipe(int err)
> +{
> + if (err == EPIPE) {
> + signal(SIGPIPE, SIG_DFL);
> + raise(SIGPIPE);
> + /* Should never happen, but just in case... */
> + exit(141);
How about
die("BUG: another thread changed SIGPIPE handling behind my
back!");
to make it easier to find and fix such problems?
Thanks,
Jonathan
--
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: Merge with staged and unstaged changes
On 2/20/13 2:21 PM, "Junio C Hamano" wrote: >Both are very much on purpose. The integrator may have seen the >patch on the list, ran "git apply [--index]" on it to contemplate on >it, and before commiting the result, saw a pull request for a branch >that contains the change. The above two allow the pull from such a >state to succeed without losing any information. >I think we have a similar table in Documentation/technical area that >explains these things, by the way. I believe you are referring to trivial-merge.txt which has been exceptionally helpful in understanding "what" unpack trees does. I appreciate this detailed explanation in providing the "why". I also appreciate your explanation of the affect of the workdir, and that makes sense. I would have expected that the default was to presume the workdir files were existent, rather than the other way around, but we can agree that is an implementation detail. My biggest concern, of course, was having the unstaged files in my workdir overwritten or deleted. Thanks again- -ed -- 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: Merge with staged and unstaged changes
Edward Thomson writes:
> What was surprising to me was that my merge can proceed if I stage a change
> that is identical to the merge result. That is, if my merge result would
> be to take the contents from "theirs", then my merge can proceed if I've
> already staged the same contents:
>
>input result
>anc ours theirs idx wd merge result idx wd
> 4 A AB B B take BB B
> 5 A AB B C take BB C
>
> This seems unexpected - is there a use-case that this enables or is
> this accidental?
Both are very much on purpose. The integrator may have seen the
patch on the list, ran "git apply [--index]" on it to contemplate on
it, and before commiting the result, saw a pull request for a branch
that contains the change. The above two allow the pull from such a
state to succeed without losing any information.
I think we have a similar table in Documentation/technical area that
explains these things, by the way.
> Another surprising result was that if I have deleted a file (and staged
> the deletion or not) then the merge will proceed and the file in question
> will be recreated. Consider "X" to be a missing file:
>
>input result
>anc ours theirs idx wd merge result idx wd
> 6 A AB A X take BB B
> 7 A AB X X take BB B
I am not sure about #7, but #6 is done very much on purpose. The
lower level merge machinery deliberately equates "in index but not
checked out to the working tree" state and "in index and not
modified in the working tree" state; this is to support a merge done
in a temporary working area that starts out empty.
This was designed really long time ago (read: during the first two
weeks of Git development, back when there were no "stash" nor
"remote tracking branches"), with a vision to make this scenario
work nicely:
You have checked out 'master' branch and you are working on it.
You see a more urgent pull request on 'maint' branch. But your
working tree for 'master' is no shape to commit, yet.
Without disturbing the working area you are using to advance the
'master' branch, you could do the merge "only in the index" by doing:
* Fetch the requested commit:
$ git fetch $over_there refs/heads/master
* Populate a temporary index with the contents of your 'maint':
$ GIT_INDEX_FILE=,,temp-index GIT_DIR=$(pwd)/.git
$ export GIT_INDEX_FILE GIT_DIR
$ git read-tree refs/heads/maint
* Create an empty temporary working area and go there:
$ mkdir ,,temp-merge
$ cd ,,temp-merge
* Run the three-way merge between your 'maint' and the requested
commit:
$ MB=$(git merge-base FETCH_HEAD maint)
$ git read-tree -m -u $MB maint FETCH_HEAD
Notice that we start without _any_ file in that temporary working
area (,,temp-merge directory). In the last step, because the merge
machinery (read-tree -m -u) treats missing files as "they are
unmodified by us; we didn't even bother checking them out of the
index", we will see _only_ the files that are different from our
'maint' and files that needs our help to get conflicts resolved
in the temporary working area after the command finishes. You
inspect them, resolve conflicts and run "git update-index" on them
(remember, there weren't "git add" or "git commit -a"), write the
resulting index as a tree with "git write-tree" and record the tree
with "git commit-tree -p maint -p FETCH_HEAD" (actually, back then
"commit-tree -p" insisted on getting raw tree object names, so you
had to do the equivalent of "git rev-parse maint^{tree}" there) and
then update your 'maint' branch with the resulting commit.
If there is no conflict to be resolved, then you essentially can run
a script that does all of the above (and cleans up the temporary
directory ,,temp-merge and the temporary index) to implement "Can
perform a merge into a branch that is not currently checked out,
without disturbing my current work" feature. And this "missing
files are unmodified---I didn't even bother to check them out of the
index" was done as an integral part of it.
Back when we (IIRC, mostly between Linus and I) were discussing this
design, it did come up that in the normal case of "I am merging in
my working tree", this behaviour would lose information, but "I'm
planning to remove this" is only a single bit and lossage of that
was judged to be trivially small and easily recoverable compared to
the benefit of being able to do a merge in an empty working tree,
and that is where this behaviour comes from.
We could certainly revisit this design and make the behaviour
optional. When somebody wants to do the "Can perform a merge into a
branch that is not currently checked out, without disturbing my
current work" feature, its implementation needs to be able to turn
it back on, but for doing everything else we do not have to treat a
missing file
[PATCH v3 04/19] fetch-pack: fix out-of-bounds buffer offset in get_ack
When we read acks from the remote, we expect either:
ACK
or
ACK
We parse the "ACK " bit from the line, and then start
looking for the flag strings at "line+45"; if we don't have
them, we assume it's of the first type. But if we do have
the first type, then line+45 is not necessarily inside our
string at all!
It turns out that this works most of the time due to the way
we parse the packets. They should come in with a newline,
and packet_read puts an extra NUL into the buffer, so we end
up with:
ACK \n\0
with the newline at offset 44 and the NUL at offset 45. We
then strip the newline, putting a NUL at offset 44. So
when we look at "line+45", we are looking past the end of
our string; but it's OK, because we hit the terminator from
the original string.
This breaks down, however, if the other side does not
terminate their packets with a newline. In that case, our
packet is one character shorter, and we start looking
through uninitialized memory for the flag. No known
implementation sends such a packet, so it has never come up
in practice.
This patch tightens the check by looking for a short,
flagless ACK before trying to parse the flag.
Signed-off-by: Jeff King
---
This is the absolute minimal fix, which just checks for the no-flag case
early; we still treat arbitrary crud in the flag field as just an ACK.
>From my understanding of the protocol, a saner parsing scheme would be:
const char *flag = line + 44; /* we already parsed "ACK " */
if (!*flag)
return ACK;
if (!strcmp(flag, " continue"))
return ACK_continue;
if (!strcmp(flag, " common"))
return ACK_continue;
if (!strcmp(flag, " ready"))
return ACK_ready;
die("fetch-pack expected multi-ack flag, got: %s", line);
But that is much tighter, and I wasn't sure if the looseness was there
to facilitate future expansion or something (though I'd think we would
need a new capability for that).
fetch-pack.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fetch-pack.c b/fetch-pack.c
index 6d8926a..27a3e80 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -226,6 +226,8 @@ static enum ack_type get_ack(int fd, unsigned char
*result_sha1)
return NAK;
if (!prefixcmp(line, "ACK ")) {
if (!get_sha1_hex(line+4, result_sha1)) {
+ if (len < 45)
+ return ACK;
if (strstr(line+45, "continue"))
return ACK_continue;
if (strstr(line+45, "common"))
--
1.8.2.rc0.9.g352092c
--
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 02/19] upload-pack: do not add duplicate objects to shallow list
When the client tells us it has a shallow object via
"shallow ", we make sure we have the object, mark it
with a flag, then add it to a dynamic array of shallow
objects. This means that a client can get us to allocate
arbitrary amounts of memory just by flooding us with shallow
lines (whether they have the objects or not). You can
demonstrate it easily with:
yes '0035shallow e83c5163316f89bfbde7d9ab23ca2e25604af290' |
git-upload-pack git.git
We already protect against duplicates in want lines by
checking if our flag is already set; let's do the same thing
here. Note that a client can still get us to allocate some
amount of memory by marking every object in the repo as
"shallow" (or "want"). But this at least bounds it with the
number of objects in the repository, which is not under the
control of an upload-pack client.
Signed-off-by: Jeff King
---
Looking over upload-pack, I think this is the only "consume arbitrary
memory" spot. Since you can convince git to go to quite a bit of work
just processing a big repo, the distinction may not be important, but
drawing the line between "large" and "arbitrarily large" seemed
reasonable to me (and it's a trivial fix).
upload-pack.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index b058e8d..1aee407 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -603,8 +603,10 @@ static void receive_needs(void)
die("did not find object for %s", line);
if (object->type != OBJ_COMMIT)
die("invalid shallow object %s",
sha1_to_hex(sha1));
- object->flags |= CLIENT_SHALLOW;
- add_object_array(object, NULL, &shallows);
+ if (!(object->flags & CLIENT_SHALLOW)) {
+ object->flags |= CLIENT_SHALLOW;
+ add_object_array(object, NULL, &shallows);
+ }
continue;
}
if (!prefixcmp(line, "deepen ")) {
--
1.8.2.rc0.9.g352092c
--
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 03/19] upload-pack: remove packet debugging harness
If you set the GIT_DEBUG_SEND_PACK environment variable, upload-pack will dump lines it receives in the receive_needs phase to a descriptor. This debugging harness is a strict subset of what GIT_TRACE_PACKET can do. Let's just drop it in favor of that. A few tests used GIT_DEBUG_SEND_PACK to confirm which objects get sent; we have to adapt them to the new output format. Signed-off-by: Jeff King --- t/t5503-tagfollow.sh | 38 +- t/t5700-clone-reference.sh | 10 +- upload-pack.c | 9 - 3 files changed, 22 insertions(+), 35 deletions(-) diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh index 60de2d6..d181c96 100755 --- a/t/t5503-tagfollow.sh +++ b/t/t5503-tagfollow.sh @@ -5,7 +5,7 @@ if ! test_have_prereq NOT_MINGW; then . ./test-lib.sh if ! test_have_prereq NOT_MINGW; then - say "GIT_DEBUG_SEND_PACK not supported - skipping tests" + say "GIT_TRACE_PACKET not supported - skipping tests" fi # End state of the repository: @@ -42,21 +42,26 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' ' test_expect_success NOT_MINGW 'setup expect' ' cat -../$U && + GIT_TRACE_PACKET=3 git fetch 3>../$U && test $A = $(git rev-parse --verify origin/master) ) && - test -s $U && - cut -d" " -f1,2 $U >actual && + get_needs $U >actual && test_cmp expect actual ' @@ -74,10 +79,8 @@ want $T test_expect_success NOT_MINGW 'setup expect' ' cat - ../$U && + GIT_TRACE_PACKET=3 git fetch 3>../$U && test $C = $(git rev-parse --verify origin/cat) && test $T = $(git rev-parse --verify tag1) && test $A = $(git rev-parse --verify tag1^0) ) && - test -s $U && - cut -d" " -f1,2 $U >actual && + get_needs $U >actual && test_cmp expect actual ' @@ -113,10 +115,8 @@ want $S test_expect_success NOT_MINGW 'setup expect' ' cat - ../$U && + GIT_TRACE_PACKET=3 git fetch 3>../$U && test $B = $(git rev-parse --verify origin/master) && test $B = $(git rev-parse --verify tag2^0) && test $S = $(git rev-parse --verify tag2) ) && - test -s $U && - cut -d" " -f1,2 $U >actual && + get_needs $U >actual && test_cmp expect actual ' test_expect_success NOT_MINGW 'setup expect' ' cat - ../$U && + GIT_TRACE_PACKET=3 git fetch 3>../$U && test $B = $(git rev-parse --verify origin/master) && test $S = $(git rev-parse --verify tag2) && test $B = $(git rev-parse --verify tag2^0) && test $T = $(git rev-parse --verify tag1) && test $A = $(git rev-parse --verify tag1^0) ) && - test -s $U && - cut -d" " -f1,2 $U >actual && + get_needs $U >actual && test_cmp expect actual ' diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index c47d450..9cd3b4d 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -55,10 +55,10 @@ test_expect_success 'fetched no objects' \ rm -f "$U.D" test_expect_success 'cloning with reference (no -l -s)' \ -'GIT_DEBUG_SEND_PACK=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"' +'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"' test_expect_success 'fetched no objects' \ -'! grep "^want" "$U.D"' +'! grep " want" "$U.D"' cd "$base_dir" @@ -173,12 +173,12 @@ test_expect_success 'fetch with incomplete alternates' ' ( cd K && git remote add J "file://$base_dir/J" && - GIT_DEBUG_SEND_PACK=3 git fetch J 3>"$U.K" + GIT_TRACE_PACKET=3 git fetch J 3>"$U.K" ) && master_object=
[PATCH v3 19/19] remote-curl: always parse incoming refs
When remote-curl receives a list of refs from a server, it
keeps the whole buffer intact. When we get a "list" command,
we feed the result to get_remote_heads, and when we get a
"fetch" or "push" command, we feed it to fetch-pack or
send-pack, respectively.
If the HTTP response from the server is truncated for any
reason, we will get an incomplete ref advertisement. If we
then feed this incomplete list to fetch-pack, one of a few
things may happen:
1. If the truncation is in a packet header, fetch-pack
will notice the bogus line and complain.
2. If the truncation is inside a packet, fetch-pack will
keep waiting for us to send the rest of the packet,
which we never will.
3. If the truncation is at a packet boundary, fetch-pack
will keep waiting for us to send the next packet, which
we never will.
As a result, fetch-pack hangs, waiting for input. However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.
We do notice the broken ref list if we feed it to
get_remote_heads. So if git asks the helper to do a "list"
followed by a "fetch", we are safe; we'll abort during the
list operation, which parses the refs.
This patch teaches remote-curl to always parse and save the
incoming ref list when we read the ref advertisement from a
server. That means that we will always verify and abort
before even running fetch-pack (or send-pack) when reading a
corrupted list, even if we do not run the "list" command
explicitly.
Since we save the result, in the common case of running
"list" then "fetch", we do not do any extra parsing at all.
In the case of just a "fetch", we do an extra round of
parsing, but only once.
Note also that the "fetch" case will now also initialize
server_capabilities from the remote (in remote-curl; we
already would do so inside fetch-pack). Doing "list+fetch"
already does this. It doesn't actually matter now, but the
new behavior is arguably more correct, should remote-curl
ever start caring about the server's capability list.
Signed-off-by: Jeff King
---
remote-curl.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 856decc..3d2b194 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -76,6 +76,7 @@ struct discovery {
char *buf_alloc;
char *buf;
size_t len;
+ struct ref *refs;
unsigned proto_git : 1;
};
static struct discovery *last_discovery;
@@ -145,11 +146,12 @@ static void free_discovery(struct discovery *d)
if (d == last_discovery)
last_discovery = NULL;
free(d->buf_alloc);
+ free_refs(d->refs);
free(d);
}
}
-static struct discovery* discover_refs(const char *service)
+static struct discovery* discover_refs(const char *service, int for_push)
{
struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
@@ -221,6 +223,11 @@ static struct discovery* discover_refs(const char *service)
last->proto_git = 1;
}
+ if (last->proto_git)
+ last->refs = parse_git_refs(last, for_push);
+ else
+ last->refs = parse_info_refs(last);
+
free(refs_url);
strbuf_release(&exp);
strbuf_release(&type);
@@ -234,13 +241,11 @@ static struct ref *get_refs(int for_push)
struct discovery *heads;
if (for_push)
- heads = discover_refs("git-receive-pack");
+ heads = discover_refs("git-receive-pack", for_push);
else
- heads = discover_refs("git-upload-pack");
+ heads = discover_refs("git-upload-pack", for_push);
- if (heads->proto_git)
- return parse_git_refs(heads, for_push);
- return parse_info_refs(heads);
+ return heads->refs;
}
static void output_refs(struct ref *refs)
@@ -254,7 +259,6 @@ static void output_refs(struct ref *refs)
}
printf("\n");
fflush(stdout);
- free_refs(refs);
}
struct rpc_state {
@@ -670,7 +674,7 @@ static int fetch(int nr_heads, struct ref **to_fetch)
static int fetch(int nr_heads, struct ref **to_fetch)
{
- struct discovery *d = discover_refs("git-upload-pack");
+ struct discovery *d = discover_refs("git-upload-pack", 0);
if (d->proto_git)
return fetch_git(d, nr_heads, to_fetch);
else
@@ -789,7 +793,7 @@ static int push(int nr_spec, char **specs)
static int push(int nr_spec, char **specs)
{
- struct discovery *heads = discover_refs("git-receive-pack");
+ struct discovery *heads = discover_refs("git-receive-pack", 1);
int ret;
if (heads->proto_git)
--
1.8.2.rc0.9.g352092c
--
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
[PATCH v3 18/19] remote-curl: move ref-parsing code up in file
The ref-parsing functions are static. Let's move them up in
the file to be available to more functions, which will help
us with later refactoring.
Signed-off-by: Jeff King
---
remote-curl.c | 118 +-
1 file changed, 59 insertions(+), 59 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index e07f654..856decc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -80,6 +80,65 @@ static struct discovery *last_discovery;
};
static struct discovery *last_discovery;
+static struct ref *parse_git_refs(struct discovery *heads, int for_push)
+{
+ struct ref *list = NULL;
+ get_remote_heads(-1, heads->buf, heads->len, &list,
+for_push ? REF_NORMAL : 0, NULL);
+ return list;
+}
+
+static struct ref *parse_info_refs(struct discovery *heads)
+{
+ char *data, *start, *mid;
+ char *ref_name;
+ int i = 0;
+
+ struct ref *refs = NULL;
+ struct ref *ref = NULL;
+ struct ref *last_ref = NULL;
+
+ data = heads->buf;
+ start = NULL;
+ mid = data;
+ while (i < heads->len) {
+ if (!start) {
+ start = &data[i];
+ }
+ if (data[i] == '\t')
+ mid = &data[i];
+ if (data[i] == '\n') {
+ if (mid - start != 40)
+ die("%sinfo/refs not valid: is this a git
repository?", url);
+ data[i] = 0;
+ ref_name = mid + 1;
+ ref = xmalloc(sizeof(struct ref) +
+ strlen(ref_name) + 1);
+ memset(ref, 0, sizeof(struct ref));
+ strcpy(ref->name, ref_name);
+ get_sha1_hex(start, ref->old_sha1);
+ if (!refs)
+ refs = ref;
+ if (last_ref)
+ last_ref->next = ref;
+ last_ref = ref;
+ start = NULL;
+ }
+ i++;
+ }
+
+ ref = alloc_ref("HEAD");
+ if (!http_fetch_ref(url, ref) &&
+ !resolve_remote_symref(ref, refs)) {
+ ref->next = refs;
+ refs = ref;
+ } else {
+ free(ref);
+ }
+
+ return refs;
+}
+
static void free_discovery(struct discovery *d)
{
if (d) {
@@ -170,65 +229,6 @@ static struct discovery* discover_refs(const char *service)
return last;
}
-static struct ref *parse_git_refs(struct discovery *heads, int for_push)
-{
- struct ref *list = NULL;
- get_remote_heads(-1, heads->buf, heads->len, &list,
-for_push ? REF_NORMAL : 0, NULL);
- return list;
-}
-
-static struct ref *parse_info_refs(struct discovery *heads)
-{
- char *data, *start, *mid;
- char *ref_name;
- int i = 0;
-
- struct ref *refs = NULL;
- struct ref *ref = NULL;
- struct ref *last_ref = NULL;
-
- data = heads->buf;
- start = NULL;
- mid = data;
- while (i < heads->len) {
- if (!start) {
- start = &data[i];
- }
- if (data[i] == '\t')
- mid = &data[i];
- if (data[i] == '\n') {
- if (mid - start != 40)
- die("%sinfo/refs not valid: is this a git
repository?", url);
- data[i] = 0;
- ref_name = mid + 1;
- ref = xmalloc(sizeof(struct ref) +
- strlen(ref_name) + 1);
- memset(ref, 0, sizeof(struct ref));
- strcpy(ref->name, ref_name);
- get_sha1_hex(start, ref->old_sha1);
- if (!refs)
- refs = ref;
- if (last_ref)
- last_ref->next = ref;
- last_ref = ref;
- start = NULL;
- }
- i++;
- }
-
- ref = alloc_ref("HEAD");
- if (!http_fetch_ref(url, ref) &&
- !resolve_remote_symref(ref, refs)) {
- ref->next = refs;
- refs = ref;
- } else {
- free(ref);
- }
-
- return refs;
-}
-
static struct ref *get_refs(int for_push)
{
struct discovery *heads;
--
1.8.2.rc0.9.g352092c
--
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 17/19] remote-curl: pass buffer straight to get_remote_heads
Until recently, get_remote_heads only knew how to read refs
from a file descriptor. To hack around this, we spawned a
thread (or forked a process) to write the buffer back to us.
Now that we can just pass it our buffer directly, we don't
have to use this hack anymore.
Signed-off-by: Jeff King
---
remote-curl.c | 26 ++
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 3bc6cb5..e07f654 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -170,33 +170,11 @@ static struct ref *parse_git_refs(struct discovery
*heads, int for_push)
return last;
}
-static int write_discovery(int in, int out, void *data)
-{
- struct discovery *heads = data;
- int err = 0;
- if (write_in_full(out, heads->buf, heads->len) != heads->len)
- err = 1;
- close(out);
- return err;
-}
-
static struct ref *parse_git_refs(struct discovery *heads, int for_push)
{
struct ref *list = NULL;
- struct async async;
-
- memset(&async, 0, sizeof(async));
- async.proc = write_discovery;
- async.data = heads;
- async.out = -1;
-
- if (start_async(&async))
- die("cannot start thread to parse advertised refs");
- get_remote_heads(async.out, NULL, 0, &list,
- for_push ? REF_NORMAL : 0, NULL);
- close(async.out);
- if (finish_async(&async))
- die("ref parsing thread failed");
+ get_remote_heads(-1, heads->buf, heads->len, &list,
+for_push ? REF_NORMAL : 0, NULL);
return list;
}
--
1.8.2.rc0.9.g352092c
--
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 16/19] teach get_remote_heads to read from a memory buffer
Now that we can read packet data from memory as easily as a
descriptor, get_remote_heads can take either one as a
source. This will allow further refactoring in remote-curl.
Signed-off-by: Jeff King
---
Another "wrapper vs NULL argument" opportunity. I could go either way if
we feel strongly in one direction. A third option is:
struct packet_source {
/* Choose one. */
int fd;
char *buf;
int len;
};
but then each caller has to be bothered to define and fill in the
struct, which ends up even uglier.
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
cache.h | 4 +++-
connect.c| 6 +++---
remote-curl.c| 2 +-
transport.c | 6 +++---
6 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c21cc2c..03ed2ca 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -125,7 +125,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
- get_remote_heads(fd[0], &ref, 0, NULL);
+ get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);
ref = fetch_pack(&args, fd, conn, ref, dest,
&sought, pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8778519..152c4ea 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char
*prefix)
memset(&extra_have, 0, sizeof(extra_have));
- get_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have);
+ get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL, &extra_have);
transport_verify_remote_names(nr_refspecs, refspecs);
diff --git a/cache.h b/cache.h
index e493563..db646a2 100644
--- a/cache.h
+++ b/cache.h
@@ -1049,7 +1049,9 @@ struct extra_have_objects {
int nr, alloc;
unsigned char (*array)[20];
};
-extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int
flags, struct extra_have_objects *);
+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **list, unsigned int flags,
+struct extra_have_objects *);
extern int server_supports(const char *feature);
extern int parse_feature_request(const char *features, const char *feature);
extern const char *server_feature_value(const char *feature, int *len_ret);
diff --git a/connect.c b/connect.c
index 061aa5b..f57efd0 100644
--- a/connect.c
+++ b/connect.c
@@ -62,8 +62,8 @@ static void die_initial_contact(int got_at_least_one_head)
/*
* Read all the refs from the other end
*/
-struct ref **get_remote_heads(int in, struct ref **list,
- unsigned int flags,
+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+ struct ref **list, unsigned int flags,
struct extra_have_objects *extra_have)
{
int got_at_least_one_head = 0;
@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
char *buffer = packet_buffer;
- len = packet_read(in, NULL, 0,
+ len = packet_read(in, &src_buf, &src_len,
packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
diff --git a/remote-curl.c b/remote-curl.c
index c0edd4c..3bc6cb5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -192,7 +192,7 @@ static struct ref *parse_git_refs(struct discovery *heads,
int for_push)
if (start_async(&async))
die("cannot start thread to parse advertised refs");
- get_remote_heads(async.out, &list,
+ get_remote_heads(async.out, NULL, 0, &list,
for_push ? REF_NORMAL : 0, NULL);
close(async.out);
if (finish_async(&async))
diff --git a/transport.c b/transport.c
index 886ffd8..62df466 100644
--- a/transport.c
+++ b/transport.c
@@ -507,7 +507,7 @@ static struct ref *get_refs_via_connect(struct transport
*transport, int for_pus
struct ref *refs;
connect_setup(transport, for_push, 0);
- get_remote_heads(data->fd[0], &refs,
+ get_remote_heads(data->fd[0], NULL, 0, &refs,
for_push ? REF_NORMAL : 0, &data->extra_have);
data->got_remote_heads = 1;
@@ -541,7 +541,7 @@ static int fetch_refs_via_pack(struct transport *transport,
if (!data->got_remote_heads) {
connect_setup(transport, 0, 0);
- get_remote_heads(data->fd[0], &refs_tmp, 0, NULL);
+ get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL);
data->got_remote_heads = 1;
}
@@ -799,7 +799,
[PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation
The packet_read function reads from a descriptor. The
packet_get_line function is similar, but reads from an
in-memory buffer, and uses a completely separate
implementation. This patch teaches the generic packet_read
function to accept either source, and we can do away with
packet_get_line's implementation.
There are two other differences to account for between the
old and new functions. The first is that we used to read
into a strbuf, but now read into a fixed size buffer. The
only two callers are fine with that, and in fact it
simplifies their code, since they can use the same
static-buffer interface as the rest of the packet_read_line
callers (and we provide a similar convenience wrapper for
reading from a buffer rather than a descriptor).
This is technically an externally-visible behavior change in
that we used to accept arbitrary sized packets up to 65532
bytes, and now cap out at LARGE_PACKET_MAX, 65520. In
practice this doesn't matter, as we use it only for parsing
smart-http headers (of which there is exactly one defined,
and it is small and fixed-size). And any extension headers
would be breaking the protocol to go over LARGE_PACKET_MAX
anyway.
The other difference is that packet_get_line would return
on error rather than dying. However, both callers of
strbuf_get_line are actually improved by dying.
The first caller does its own error checking, but we can
drop that; as a result, we'll actually get more specific
reporting about protocol breakage when packet_read dies
internally. The only downside is that packet_read will not
print the smart-http URL that failed, but that's not a big
deal; anybody not debugging can already see the remote's URL
already, and anybody debugging would want to run with
GIT_CURL_VERBOSE anyway to see way more information.
The second caller, which is just trying to skip past any
extra smart-http headers (of which there are none defined,
but which we allow to keep room for future expansion), did
not error check at all. As a result, it would treat an error
just like a flush packet. The resulting mess would generally
cause an error later in get_remote_heads, but now we get
error reporting much closer to the source of the problem.
Signed-off-by: Jeff King
---
This adds two options to the generic packet_read interface for which
many callers will just pass (NULL, 0). We can hide that behind a
wrapper, but I was annoyed with the proliferation of wrappers from the
last round. Pick your poison.
connect.c | 3 ++-
daemon.c | 2 +-
pkt-line.c| 77 ++-
pkt-line.h| 23 +-
remote-curl.c | 22 -
sideband.c| 2 +-
6 files changed, 70 insertions(+), 59 deletions(-)
diff --git a/connect.c b/connect.c
index 611ffb4..061aa5b 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
char *buffer = packet_buffer;
- len = packet_read(in, packet_buffer, sizeof(packet_buffer),
+ len = packet_read(in, NULL, 0,
+ packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
diff --git a/daemon.c b/daemon.c
index 3f70e79..9a241d9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
+ pktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer),
0);
alarm(0);
len = strlen(line);
diff --git a/pkt-line.c b/pkt-line.c
index 55fb688..2c47052 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -104,12 +104,29 @@ static int safe_read(int fd, void *buffer, unsigned size,
int options)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int options)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+ void *dst, unsigned size, int options)
{
- ssize_t ret = read_in_full(fd, buffer, size);
- if (ret < 0)
- die_errno("read error");
- else if (ret < size) {
+ ssize_t ret;
+
+ if (fd >= 0 && src_buf && *src_buf)
+ die("BUG: multiple sources given to packet_read");
+
+ /* Read up to "size" bytes from our source, whatever it is. */
+ if (src_buf && *src_buf) {
+ ret = size < *src_size ? size : *src_size;
+ memcpy(dst, *src_buf, ret);
+ *src_buf += size;
+ *src_size -= size;
+ }
+ else {
+ ret = read_in_full(fd, dst, size);
+ if (ret < 0)
+ die_errno("read error");
+ }
+
+
Re: Google Summer of Code 2013 (GSoC13)
On 02/18/2013 06:42 PM, Jeff King wrote: > > I will do it again, if people feel strongly about Git being a part of > it. However, I have gotten a little soured on the GSoC experience. Not > because of anything Google has done; it's a good idea, and I think they > do a fine of administering the program. But I have noticed that the work > that comes out of GSoC the last few years has quite often not been > merged, or not made a big impact in the codebase, and nor have the > participants necessarily stuck around. > > And I do not want to blame the students here (some of whom are on the cc > list :) ). They are certainly under no obligation to stick around after > GSoC ends, and I know they have many demands on their time. But I am > also thinking about what Git wants to get out of GSoC (and to my mind, > the most important thing is contributors). Speaking of libgit2: Git provided the libgit2 project with a slot each of the last three GSOC. The contributions made by the former students (Disclaimer: one of them speaking) have been quite important for libgit2 and all three students are still involved. Each project was an important push towards building a new, feature complete Git library. Thank you! http://libgit2.github.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
[PATCH v3 14/19] pkt-line: provide a LARGE_PACKET_MAX static buffer
Most of the callers of packet_read_line just read into a
static 1000-byte buffer (callers which handle arbitrary
binary data already use LARGE_PACKET_MAX). This works fine
in practice, because:
1. The only variable-sized data in these lines is a ref
name, and refs tend to be a lot shorter than 1000
characters.
2. When sending ref lines, git-core always limits itself
to 1000 byte packets.
However, the only limit given in the protocol specification
in Documentation/technical/protocol-common.txt is
LARGE_PACKET_MAX; the 1000 byte limit is mentioned only in
pack-protocol.txt, and then only describing what we write,
not as a specific limit for readers.
This patch lets us bump the 1000-byte limit to
LARGE_PACKET_MAX. Even though git-core will never write a
packet where this makes a difference, there are two good
reasons to do this:
1. Other git implementations may have followed
protocol-common.txt and used a larger maximum size. We
don't bump into it in practice because it would involve
very long ref names.
2. We may want to increase the 1000-byte limit one day.
Since packets are transferred before any capabilities,
it's difficult to do this in a backwards-compatible
way. But if we bump the size of buffer the readers can
handle, eventually older versions of git will be
obsolete enough that we can justify bumping the
writers, as well. We don't have plans to do this
anytime soon, but there is no reason not to start the
clock ticking now.
Just bumping all of the reading bufs to LARGE_PACKET_MAX
would waste memory. Instead, since most readers just read
into a temporary buffer anyway, let's provide a single
static buffer that all callers can use. We can further wrap
this detail away by having the packet_read_line wrapper just
use the buffer transparently and return a pointer to the
static storage. That covers most of the cases, and the
remaining ones already read into their own LARGE_PACKET_MAX
buffers.
Signed-off-by: Jeff King
---
builtin/archive.c| 15 +++
builtin/fetch-pack.c | 7 +++
builtin/receive-pack.c | 6 +++---
builtin/upload-archive.c | 7 ++-
connect.c| 4 ++--
daemon.c | 4 ++--
fetch-pack.c | 12 ++--
pkt-line.c | 9 +++--
pkt-line.h | 9 +++--
send-pack.c | 7 +++
upload-pack.c| 12 +---
11 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index d381ac4..49178f1 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -27,8 +27,8 @@ static int run_remote_archiver(int argc, const char **argv,
const char *remote, const char *exec,
const char *name_hint)
{
- char buf[LARGE_PACKET_MAX];
- int fd[2], i, len, rv;
+ char *buf;
+ int fd[2], i, rv;
struct transport *transport;
struct remote *_remote;
@@ -53,19 +53,18 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- len = packet_read_line(fd[0], buf, sizeof(buf));
- if (!len)
+ buf = packet_read_line(fd[0], NULL);
+ if (!buf)
die(_("git archive: expected ACK/NAK, got EOF"));
if (strcmp(buf, "ACK")) {
- if (len > 5 && !prefixcmp(buf, "NACK "))
+ if (!prefixcmp(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
- if (len > 4 && !prefixcmp(buf, "ERR "))
+ if (!prefixcmp(buf, "ERR "))
die(_("remote error: %s"), buf + 4);
die(_("git archive: protocol error"));
}
- len = packet_read_line(fd[0], buf, sizeof(buf));
- if (len)
+ if (packet_read_line(fd[0], NULL))
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f73664f..c21cc2c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -100,12 +100,11 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
/* in stateless RPC mode we use pkt-line to read
* from stdin, until we get a flush packet
*/
- static char line[1000];
for (;;) {
- int n = packet_read_line(0, line, sizeof(line));
- if (!n)
+ char *line = packet_read_line(0, NULL);
+ if (!line)
break;
- string_list_append(&sought, xmemdupz(line, n));
+ string_li
[PATCH v3 13/19] pkt-line: move LARGE_PACKET_MAX definition from sideband
Having the packet sizes defined near the packet read/write functions makes more sense. Signed-off-by: Jeff King --- http.c | 1 + pkt-line.h | 3 +++ sideband.h | 3 --- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index d9d1aad..8803c70 100644 --- a/http.c +++ b/http.c @@ -5,6 +5,7 @@ #include "url.h" #include "credential.h" #include "version.h" +#include "pkt-line.h" int active_requests; int http_is_verbose; diff --git a/pkt-line.h b/pkt-line.h index 5d2fb42..6927ea5 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -58,6 +58,9 @@ int packet_read_line(int fd, char *buffer, unsigned size); */ int packet_read_line(int fd, char *buffer, unsigned size); +#define DEFAULT_PACKET_MAX 1000 +#define LARGE_PACKET_MAX 65520 + int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len); #endif diff --git a/sideband.h b/sideband.h index d72db35..e46bed0 100644 --- a/sideband.h +++ b/sideband.h @@ -4,9 +4,6 @@ #define SIDEBAND_PROTOCOL_ERROR -2 #define SIDEBAND_REMOTE_ERROR -1 -#define DEFAULT_PACKET_MAX 1000 -#define LARGE_PACKET_MAX 65520 - int recv_sideband(const char *me, int in_stream, int out); ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max); -- 1.8.2.rc0.9.g352092c -- 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 12/19] pkt-line: teach packet_read_line to chomp newlines
The packets sent during ref negotiation are all terminated
by newline; even though the code to chomp these newlines is
short, we end up doing it in a lot of places.
This patch teaches packet_read_line to auto-chomp the
trailing newline; this lets us get rid of a lot of inline
chomping code.
As a result, some call-sites which are not reading
line-oriented data (e.g., when reading chunks of packfiles
alongside sideband) transition away from packet_read_line to
the generic packet_read interface. This patch converts all
of the existing callsites.
Since the function signature of packet_read_line does not
change (but its behavior does), there is a possibility of
new callsites being introduced in later commits, silently
introducing an incompatibility. However, since a later
patch in this series will change the signature, such a
commit would have to be merged directly into this commit,
not to the tip of the series; we can therefore ignore the
issue.
This is an internal cleanup and should produce no change of
behavior in the normal case. However, there is one corner
case to note. Callers of packet_read_line have never been
able to tell the difference between a flush packet ("")
and an empty packet ("0004"), as both cause packet_read_line
to return a length of 0. Readers treat them identically,
even though Documentation/technical/protocol-common.txt says
we must not; it also says that implementations should not
send an empty pkt-line.
By stripping out the newline before the result gets to the
caller, we will now treat the newline-only packet ("0005\n")
the same as an empty packet, which in turn gets treated like
a flush packet. In practice this doesn't matter, as neither
empty nor newline-only packets are part of git's protocols
(at least not for the line-oriented bits, and readers who
are not expecting line-oriented packets will be calling
packet_read directly, anyway). But even if we do decide to
care about the distinction later, it is orthogonal to this
patch. The right place to tighten would be to stop treating
empty packets as flush packets, and this change does not
make doing so any harder.
Signed-off-by: Jeff King
---
builtin/archive.c| 2 --
builtin/fetch-pack.c | 2 --
builtin/receive-pack.c | 2 --
builtin/upload-archive.c | 4
connect.c| 5 ++---
daemon.c | 2 +-
fetch-pack.c | 2 --
pkt-line.c | 7 ++-
pkt-line.h | 9 -
remote-curl.c| 6 +++---
send-pack.c | 6 +-
sideband.c | 2 +-
upload-pack.c| 8
13 files changed, 22 insertions(+), 35 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index 9a1cfd3..d381ac4 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -56,8 +56,6 @@ static int run_remote_archiver(int argc, const char **argv,
len = packet_read_line(fd[0], buf, sizeof(buf));
if (!len)
die(_("git archive: expected ACK/NAK, got EOF"));
- if (buf[len-1] == '\n')
- buf[--len] = 0;
if (strcmp(buf, "ACK")) {
if (len > 5 && !prefixcmp(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 940ae35..f73664f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -105,8 +105,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
int n = packet_read_line(0, line, sizeof(line));
if (!n)
break;
- if (line[n-1] == '\n')
- n--;
string_list_append(&sought, xmemdupz(line, n));
}
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9129563..6679e63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -763,8 +763,6 @@ static struct command *read_head_info(void)
len = packet_read_line(0, line, sizeof(line));
if (!len)
break;
- if (line[len-1] == '\n')
- line[--len] = 0;
if (len < 83 ||
line[40] != ' ' ||
line[81] != ' ' ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 3393cef..7d367b5 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -40,10 +40,6 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
if (sent_argv.argc > MAX_ARGS)
die("Too many options (>%d)", MAX_ARGS - 1);
- if (buf[len-1] == '\n') {
- buf[--len] = 0;
- }
-
if (prefixcmp(buf, arg_cmd))
die("'argument' token or flush expected");
argv
[PATCH v3 11/19] pkt-line: provide a generic reading function with options
Originally we had a single function for reading packetized
data: packet_read_line. Commit 46284dd grew a more "gentle"
form, packet_read, that returns an error instead of dying
upon reading a truncated input stream. However, it is not
clear from the names which should be called, or what the
difference is.
Let's instead make packet_read be a generic public interface
that can take option flags, and update the single callsite
that uses it. This is less code, more clear, and paves the
way for introducing more options into the generic interface
later. The function signature is changed, so there should be
no hidden conflicts with topics in flight.
While we're at it, we'll document how error conditions are
handled based on the options, and rename the confusing
"return_line_fail" option to "gentle_on_eof". While we are
cleaning up the names, we can drop the "return_line_fail"
checks in packet_read_internal entirely. They look like
this:
ret = safe_read(..., return_line_fail);
if (return_line_fail && ret < 0)
...
The check for return_line_fail is a no-op; safe_read will
only ever return an error value if return_line_fail was true
in the first place.
Signed-off-by: Jeff King
---
connect.c | 3 ++-
pkt-line.c | 21 -
pkt-line.h | 27 ++-
3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/connect.c b/connect.c
index 49e56ba..0aa202f 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
char *name;
int len, name_len;
- len = packet_read(in, buffer, sizeof(buffer));
+ len = packet_read(in, buffer, sizeof(buffer),
+ PACKET_READ_GENTLE_ON_EOF);
if (len < 0)
die_initial_contact(got_at_least_one_head);
diff --git a/pkt-line.c b/pkt-line.c
index 699c2dd..8700cf8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -103,13 +103,13 @@ static int safe_read(int fd, void *buffer, unsigned size,
int return_line_fail)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
+static int safe_read(int fd, void *buffer, unsigned size, int options)
{
ssize_t ret = read_in_full(fd, buffer, size);
if (ret < 0)
die_errno("read error");
else if (ret < size) {
- if (return_line_fail)
+ if (options & PACKET_READ_GENTLE_ON_EOF)
return -1;
die("The remote end hung up unexpectedly");
@@ -143,13 +143,13 @@ static int packet_read_internal(int fd, char *buffer,
unsigned size, int return_
return len;
}
-static int packet_read_internal(int fd, char *buffer, unsigned size, int
return_line_fail)
+int packet_read(int fd, char *buffer, unsigned size, int options)
{
int len, ret;
char linelen[4];
- ret = safe_read(fd, linelen, 4, return_line_fail);
- if (return_line_fail && ret < 0)
+ ret = safe_read(fd, linelen, 4, options);
+ if (ret < 0)
return ret;
len = packet_length(linelen);
if (len < 0)
@@ -161,22 +161,17 @@ int packet_read_line(int fd, char *buffer, unsigned size)
len -= 4;
if (len >= size)
die("protocol error: bad line length %d", len);
- ret = safe_read(fd, buffer, len, return_line_fail);
- if (return_line_fail && ret < 0)
+ ret = safe_read(fd, buffer, len, options);
+ if (ret < 0)
return ret;
buffer[len] = 0;
packet_trace(buffer, len, 0);
return len;
}
-int packet_read(int fd, char *buffer, unsigned size)
-{
- return packet_read_internal(fd, buffer, size, 1);
-}
-
int packet_read_line(int fd, char *buffer, unsigned size)
{
- return packet_read_internal(fd, buffer, size, 0);
+ return packet_read(fd, buffer, size, 0);
}
int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 3b6c19c..8cd326c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -24,8 +24,33 @@ int packet_read_line(int fd, char *buffer, unsigned size);
void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
+/*
+ * Read a packetized line from the descriptor into the buffer, which must be at
+ * least size bytes long. The return value specifies the number of bytes read
+ * into the buffer.
+ *
+ * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
+ * of the following conditions:
+ *
+ * 1. Read error from descriptor.
+ *
+ * 2. Protocol error from the remote (e.g., bogus length characters).
+ *
+ * 3. Receiving a packet larger than "size" bytes.
+ *
+ * 4. Truncated output from the remote (e.g., we expected a packet but got
+ * EOF, or we got a partial packet followed by EOF).
+ *
+ *
[PATCH v3 10/19] pkt-line: drop safe_write function
This is just write_or_die by another name. The one
distinction is that write_or_die will treat EPIPE specially
by suppressing error messages. That's fine, as we die by
SIGPIPE anyway (and in the off chance that it is disabled,
write_or_die will simulate it).
Signed-off-by: Jeff King
---
builtin/receive-pack.c | 2 +-
builtin/send-pack.c| 2 +-
fetch-pack.c | 2 +-
http-backend.c | 8
pkt-line.c | 21 ++---
pkt-line.h | 1 -
remote-curl.c | 4 ++--
send-pack.c| 2 +-
sideband.c | 9 +
upload-pack.c | 3 ++-
10 files changed, 19 insertions(+), 35 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 62ba6e7..9129563 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -932,7 +932,7 @@ static void report(struct command *commands, const char
*unpack_status)
if (use_sideband)
send_sideband(1, 1, buf.buf, buf.len, use_sideband);
else
- safe_write(1, buf.buf, buf.len);
+ write_or_die(1, buf.buf, buf.len);
strbuf_release(&buf);
}
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 57a46b2..8778519 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)
}
strbuf_addch(&buf, '\n');
- safe_write(1, buf.buf, buf.len);
+ write_or_die(1, buf.buf, buf.len);
}
strbuf_release(&buf);
}
diff --git a/fetch-pack.c b/fetch-pack.c
index 27a3e80..b53a18f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -247,7 +247,7 @@ static void send_request(struct fetch_pack_args *args,
send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
packet_flush(fd);
} else
- safe_write(fd, buf->buf, buf->len);
+ write_or_die(fd, buf->buf, buf->len);
}
static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index f50e77f..8144f3a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)
if (n >= sizeof(buffer))
die("protocol error: impossibly long line");
- safe_write(fd, buffer, n);
+ write_or_die(fd, buffer, n);
}
static void http_status(unsigned code, const char *msg)
@@ -111,7 +111,7 @@ static void end_headers(void)
static void end_headers(void)
{
- safe_write(1, "\r\n", 2);
+ write_or_die(1, "\r\n", 2);
}
__attribute__((format (printf, 1, 2)))
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf
*buf)
hdr_int(content_length, buf->len);
hdr_str(content_type, type);
end_headers();
- safe_write(1, buf->buf, buf->len);
+ write_or_die(1, buf->buf, buf->len);
}
static void send_local_file(const char *the_type, const char *name)
@@ -185,7 +185,7 @@ static void send_local_file(const char *the_type, const
char *name)
die_errno("Cannot read '%s'", p);
if (!n)
break;
- safe_write(1, buf, n);
+ write_or_die(1, buf, n);
}
close(fd);
free(buf);
diff --git a/pkt-line.c b/pkt-line.c
index 5138f47..699c2dd 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,23 +46,6 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
strbuf_release(&out);
}
-ssize_t safe_write(int fd, const void *buf, ssize_t n)
-{
- ssize_t nn = n;
- while (n) {
- int ret = xwrite(fd, buf, n);
- if (ret > 0) {
- buf = (char *) buf + ret;
- n -= ret;
- continue;
- }
- if (!ret)
- die("write error (disk full?)");
- die_errno("write error");
- }
- return nn;
-}
-
/*
* If we buffered things up above (we don't, but we should),
* we'd flush it here
@@ -70,7 +53,7 @@ void packet_flush(int fd)
void packet_flush(int fd)
{
packet_trace("", 4, 1);
- safe_write(fd, "", 4);
+ write_or_die(fd, "", 4);
}
void packet_buf_flush(struct strbuf *buf)
@@ -106,7 +89,7 @@ void packet_write(int fd, const char *fmt, ...)
va_start(args, fmt);
n = format_packet(fmt, args);
va_end(args);
- safe_write(fd, buffer, n);
+ write_or_die(fd, buffer, n);
}
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
diff --git a/pkt-line.h b/pkt-line.h
index 7a67e9c..3b6c19c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -27,6 +27,5 @@ int packet_get_line(struct strbuf *out, char **src_buf,
size_t *src_len);
int packet_read_line(int fd, char *buffer, unsigned size);
int packet_read(int fd, char *buffer,
[PATCH v3 09/19] pkt-line: move a misplaced comment
The comment describing the packet writing interface was
originally written above packet_write, but migrated to be
above safe_write in f3a3214, probably because it is meant to
generally describe the packet writing interface and not a
single function. Let's move it into the header file, where
users of the interface are more likely to see it.
Signed-off-by: Jeff King
---
pkt-line.c | 15 ---
pkt-line.h | 14 +-
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..5138f47 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,21 +46,6 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
strbuf_release(&out);
}
-/*
- * Write a packetized stream, where each line is preceded by
- * its length (including the header) as a 4-byte hex number.
- * A length of 'zero' means end of stream (and a length of 1-3
- * would be an error).
- *
- * This is all pretty stupid, but we use this packetized line
- * format to make a streaming format possible without ever
- * over-running the read buffers. That way we'll never read
- * into what might be the pack data (which should go to another
- * process entirely).
- *
- * The writing side could use stdio, but since the reading
- * side can't, we stay with pure read/write interfaces.
- */
ssize_t safe_write(int fd, const void *buf, ssize_t n)
{
ssize_t nn = n;
diff --git a/pkt-line.h b/pkt-line.h
index 8cfeb0c..7a67e9c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -5,7 +5,19 @@
#include "strbuf.h"
/*
- * Silly packetized line writing interface
+ * Write a packetized stream, where each line is preceded by
+ * its length (including the header) as a 4-byte hex number.
+ * A length of 'zero' means end of stream (and a length of 1-3
+ * would be an error).
+ *
+ * This is all pretty stupid, but we use this packetized line
+ * format to make a streaming format possible without ever
+ * over-running the read buffers. That way we'll never read
+ * into what might be the pack data (which should go to another
+ * process entirely).
+ *
+ * The writing side could use stdio, but since the reading
+ * side can't, we stay with pure read/write interfaces.
*/
void packet_flush(int fd);
void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf,
2, 3)));
--
1.8.2.rc0.9.g352092c
--
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 08/19] write_or_die: raise SIGPIPE when we get EPIPE
The write_or_die function will always die on an error,
including EPIPE. However, it currently treats EPIPE
specially by suppressing any error message, and by exiting
with exit code 0.
Suppressing the error message makes some sense; a pipe death
may just be a sign that the other side is not interested in
what we have to say. However, exiting with a successful
error code is not a good idea, as write_or_die is frequently
used in cases where we want to be careful about having
written all of the output, and we may need to signal to our
caller that we have done so (e.g., you would not want a push
whose other end has hung up to report success).
This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process, or for us to add a callsite inside one
of our few SIGPIPE-ignoring blocks of code.
This patch converts write_or_die to actually raise SIGPIPE
when we see EPIPE, rather than exiting with zero. This
brings the behavior in line with the "normal" case that we
die from SIGPIPE (and any callers who want to check why we
died will see the same thing). We also give the same
treatment to other related functions, including
write_or_whine_pipe and maybe_flush_or_die.
Signed-off-by: Jeff King
---
write_or_die.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/write_or_die.c b/write_or_die.c
index 960f448..b50f99a 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,5 +1,15 @@
#include "cache.h"
+static void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
/*
* Some cases use stdio, but want to flush after the write
* to get error handling (and to get better interactive
@@ -34,8 +44,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
return;
}
if (fflush(f)) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
die_errno("write failure on '%s'", desc);
}
}
@@ -50,8 +59,7 @@ void write_or_die(int fd, const void *buf, size_t count)
void write_or_die(int fd, const void *buf, size_t count)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
die_errno("write error");
}
}
@@ -59,8 +67,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t
count, const char *msg)
int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
fprintf(stderr, "%s: write error (%s)\n",
msg, strerror(errno));
return 0;
--
1.8.2.rc0.9.g352092c
--
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 07/19] upload-archive: use argv_array to store client arguments
The current parsing scheme for upload-archive is to pack
arguments into a fixed-size buffer, separated by NULs, and
put a pointer to each argument in the buffer into a
fixed-size argv array.
This works fine, and the limits are high enough that nobody
reasonable is going to hit them, but it makes the code hard
to follow. Instead, let's just stuff the arguments into an
argv_array, which is much simpler. That lifts the "all
arguments must fit inside 4K together" limit.
We could also trivially lift the MAX_ARGS limitation (in
fact, we have to keep extra code to enforce it). But that
would mean a client could force us to allocate an arbitrary
amount of memory simply by sending us "argument" lines. By
limiting the MAX_ARGS, we limit an attacker to about 4
megabytes (64 times a maximum 64K packet buffer). That may
sound like a lot compared to the 4K limit, but it's not a
big deal compared to what git-archive will actually allocate
while working (e.g., to load blobs into memory). The
important thing is that it is bounded.
Signed-off-by: Jeff King
---
builtin/upload-archive.c | 35 ++-
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index c3d134e..3393cef 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -7,6 +7,7 @@
#include "pkt-line.h"
#include "sideband.h"
#include "run-command.h"
+#include "argv-array.h"
static const char upload_archive_usage[] =
"git upload-archive ";
@@ -18,10 +19,9 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
{
- const char *sent_argv[MAX_ARGS];
+ struct argv_array sent_argv = ARGV_ARRAY_INIT;
const char *arg_cmd = "argument ";
- char *p, buf[4096];
- int sent_argc;
+ char buf[4096];
int len;
if (argc != 2)
@@ -31,33 +31,26 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
- sent_argc = 1;
- sent_argv[0] = "git-upload-archive";
- for (p = buf;;) {
+ argv_array_push(&sent_argv, "git-upload-archive");
+ for (;;) {
/* This will die if not enough free space in buf */
- len = packet_read_line(0, p, (buf + sizeof buf) - p);
+ len = packet_read_line(0, buf, sizeof(buf));
if (len == 0)
break; /* got a flush */
- if (sent_argc > MAX_ARGS - 2)
- die("Too many options (>%d)", MAX_ARGS - 2);
+ if (sent_argv.argc > MAX_ARGS)
+ die("Too many options (>%d)", MAX_ARGS - 1);
- if (p[len-1] == '\n') {
- p[--len] = 0;
+ if (buf[len-1] == '\n') {
+ buf[--len] = 0;
}
- if (len < strlen(arg_cmd) ||
- strncmp(arg_cmd, p, strlen(arg_cmd)))
- die("'argument' token or flush expected");
- len -= strlen(arg_cmd);
- memmove(p, p + strlen(arg_cmd), len);
- sent_argv[sent_argc++] = p;
- p += len;
- *p++ = 0;
+ if (prefixcmp(buf, arg_cmd))
+ die("'argument' token or flush expected");
+ argv_array_push(&sent_argv, buf + strlen(arg_cmd));
}
- sent_argv[sent_argc] = NULL;
/* parse all options sent by the client */
- return write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);
+ return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL,
1);
}
__attribute__((format (printf, 1, 2)))
--
1.8.2.rc0.9.g352092c
--
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 06/19] upload-archive: do not copy repo name
According to the comment, enter_repo will modify its input.
However, this has not been the case since 1c64b48
(enter_repo: do not modify input, 2011-10-04). Drop the
now-useless copy.
Signed-off-by: Jeff King
---
builtin/upload-archive.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..c3d134e 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,13 +27,8 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
if (argc != 2)
usage(upload_archive_usage);
- if (strlen(argv[1]) + 1 > sizeof(buf))
- die("insanely long repository name");
-
- strcpy(buf, argv[1]); /* enter-repo smudges its argument */
-
- if (!enter_repo(buf, 0))
- die("'%s' does not appear to be a git repository", buf);
+ if (!enter_repo(argv[1], 0))
+ die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
sent_argc = 1;
--
1.8.2.rc0.9.g352092c
--
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 05/19] send-pack: prefer prefixcmp over memcmp in receive_status
This code predates prefixcmp, so it used memcmp along with
static sizes. Replacing these memcmps with prefixcmp makes
the code much more readable, and the lack of static sizes
will make refactoring it in future patches simpler.
Note that we used to be unnecessarily liberal in parsing the
"unpack" status line, and would accept "unpack ok\njunk". No
version of git has ever produced that, and it violates the
BNF in Documentation/technical/pack-protocol.txt. Let's take
this opportunity to tighten the check by converting the
prefix comparison into a strcmp.
While we're in the area, let's also fix a vague error
message that does not follow our usual conventions (it
writes directly to stderr and does not use the "error:"
prefix).
Signed-off-by: Jeff King
---
send-pack.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 97ab336..e91cbe2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -109,9 +109,9 @@ static int receive_status(int in, struct ref *refs)
char line[1000];
int ret = 0;
int len = packet_read_line(in, line, sizeof(line));
- if (len < 10 || memcmp(line, "unpack ", 7))
+ if (prefixcmp(line, "unpack "))
return error("did not receive remote status");
- if (memcmp(line, "unpack ok\n", 10)) {
+ if (strcmp(line, "unpack ok\n")) {
char *p = line + strlen(line) - 1;
if (*p == '\n')
*p = '\0';
@@ -125,9 +125,8 @@ static int receive_status(int in, struct ref *refs)
len = packet_read_line(in, line, sizeof(line));
if (!len)
break;
- if (len < 3 ||
- (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3))) {
- fprintf(stderr, "protocol error: %s\n", line);
+ if (prefixcmp(line, "ok ") && prefixcmp(line, "ng ")) {
+ error("invalid ref status from remote: %s", line);
ret = -1;
break;
}
--
1.8.2.rc0.9.g352092c
--
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 01/19] upload-pack: use get_sha1_hex to parse "shallow" lines
When we receive a line like "shallow " from the
client, we feed the part to get_sha1. This is a
mistake, as the argument on a shallow line is defined by
Documentation/technical/pack-protocol.txt to contain an
"obj-id". This is never defined in the BNF, but it is clear
from the text and from the other uses that it is meant to be
a hex sha1, not an arbitrary identifier (and that is what
fetch-pack has always sent).
We should be using get_sha1_hex instead, which doesn't allow
the client to request arbitrary junk like "HEAD@{yesterday}".
Because this is just marking shallow objects, the client
couldn't actually do anything interesting (like fetching
objects from unreachable reflog entries), but we should keep
our parsing tight to be on the safe side.
Because get_sha1 is for the most part a superset of
get_sha1_hex, in theory the only behavior change should be
disallowing non-hex object references. However, there is
one interesting exception: get_sha1 will only parse
a 40-character hex sha1 if the string has exactly 40
characters, whereas get_sha1_hex will just eat the first 40
characters, leaving the rest. That means that current
versions of git-upload-pack will not accept a "shallow"
packet that has a trailing newline, even though the protocol
documentation is clear that newlines are allowed (even
encouraged) in non-binary parts of the protocol.
This never mattered in practice, though, because fetch-pack,
contrary to the protocol documentation, does not include a
newline in its shallow lines. JGit follows its lead (though
it correctly is strict on the parsing end about wanting a
hex object id).
We do not adjust fetch-pack to send newlines here, as it
would break communication with older versions of git (and
there is no actual benefit to doing so, except for
consistency with other parts of the protocol).
Signed-off-by: Jeff King
---
I couldn't trigger anything interestingly malicious from this, but I
didn't look very hard. Maybe somebody who knows the shallow protocol
better could think of something clever (not that it matters much).
upload-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 30146a0..b058e8d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -596,7 +596,7 @@ static void receive_needs(void)
if (!prefixcmp(line, "shallow ")) {
unsigned char sha1[20];
struct object *object;
- if (get_sha1(line + 8, sha1))
+ if (get_sha1_hex(line + 8, sha1))
die("invalid shallow line: %s", line);
object = parse_object(sha1);
if (!object)
--
1.8.2.rc0.9.g352092c
--
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
[PATCHv3 0/19] pkt-line cleanups and fixes
Here's another round of my pkt-line fixes. The more I dug, the more interesting corners I found. :) There are really several potentially independent topics rolled together here. There are dependencies between some of them, so I tried to float the most independent and non-controversial bits to the beginning. We may want those as a separate topic to merge sooner, and have the rest as a topic build on top. Overall, the diffstat shows a reduction in lines (and I even added a few dozen lines of comments), which is nice. The intent was to fix some bugs and corner cases, but I found a lot of cleanup opportunities in the middle. builtin/archive.c | 17 ++-- builtin/fetch-pack.c | 11 +- builtin/receive-pack.c | 10 +- builtin/send-pack.c| 4 +- builtin/upload-archive.c | 45 +++-- cache.h| 4 +- connect.c | 13 +-- daemon.c | 4 +- fetch-pack.c | 18 ++-- http-backend.c | 8 +- http.c | 1 + pkt-line.c | 126 ++- pkt-line.h | 72 +- remote-curl.c | 188 --- send-pack.c| 22 ++-- sideband.c | 11 +- sideband.h | 3 - t/t5503-tagfollow.sh | 38 --- t/t5700-clone-reference.sh | 10 +- transport.c| 6 +- upload-pack.c | 40 +++- write_or_die.c | 19 ++-- 22 files changed, 321 insertions(+), 349 deletions(-) The patches are: [01/19]: upload-pack: use get_sha1_hex to parse "shallow" lines New in this round; fixes a potential interoperability problem. [02/19]: upload-pack: do not add duplicate objects to shallow list New. Fixes a potential memory-consumption denial-of-service. [03/19]: upload-pack: remove packet debugging harness New. Optional cleanup, but later patches textually depend on it. [04/19]: fetch-pack: fix out-of-bounds buffer offset in get_ack New. Fixes a potential interoperability problem. [05/19]: send-pack: prefer prefixcmp over memcmp in receive_status New. Optional cleanup. [06/19]: upload-archive: do not copy repo name [07/19]: upload-archive: use argv_array to store client arguments New. Optional cleanup. [08/19]: write_or_die: raise SIGPIPE when we get EPIPE [09/19]: pkt-line: move a misplaced comment [10/19]: pkt-line: drop safe_write function The latter two were in the last round; but it's 08/19 that makes doing 10/19 safe. I think it's also a sane thing to be doing in general for existing callers of write_or_die. These can really be pulled into a separate topic if we want, as there isn't even a lot of textual dependency. [11/19]: pkt-line: provide a generic reading function with options This is an alternative to the proliferation of different reading functions that round 2 had. I think it ends up cleaner. It also addresses Jonathan's function-signature concerns. [12/19]: pkt-line: teach packet_read_line to chomp newlines New. A convenience cleanup that drops a lot of lines. Technically optional, but later patches depend heavily on it (textually, and for splitting line-readers from binary-readers). [13/19]: pkt-line: move LARGE_PACKET_MAX definition from sideband [14/19]: pkt-line: provide a LARGE_PACKET_MAX static buffer New. Another cleanup that makes packet_read_line callers a bit simpler, and bumps the packet size limits throughout git, as we discussed. [15/19]: pkt-line: share buffer/descriptor reading implementation [16/19]: teach get_remote_heads to read from a memory buffer [17/19]: remote-curl: pass buffer straight to get_remote_heads These are more or less ported from v2's patches 6-8, except that the earlier pkt-line changes make the first one way more pleasant. [18/19]: remote-curl: move ref-parsing code up in file [19/19]: remote-curl: always parse incoming refs ...and the yak is shaved. More or less a straight rebase of their v2 counterparts, and the thing that actually started me on this topic. I know it's a big series, but I tried hard to break it down into bite-sized chunks. Thanks for your reviewing patience. -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
Merge with staged and unstaged changes
Hi- I've been investigating the cases where merge is allowed to proceed when there are staged changes in the index or unstaged files in the working directory. There are cases where I find the behavior surprising and I hope I can get clarification. There are also two cases that I will report as bugs, where it appears that the unstaged file contents are deleted. For these cases below, please consider the contents of a single path. In the tables below, we will show the contents of a file across each input and output of the merge - consider that we're merging a single file in some branch "theirs" into the current branch "ours" and that these two branches have a common ancestor "anc". The state of that file in our index and workdir are represented by "idx" and "wd", respectively. Unless otherwise noted, these cases are true for both git-merge-resolve and git-merge-recursive. For completeness and illustration purposes, I'll included the cases where there are no changes staged or unstaged. These succeed, as expected: input result anc ours theirs idx wd merge result idx wd 1 A AB A A take BB B 2 A BA B B take AA A Merge is also expected to proceed if the contents of our branch are the merge result, and there are unstaged changes for that file in the workdir. In this case, the file remains unstaged: input result anc ours theirs idx wd merge result idx wd 3 A BA B C take BB C What was surprising to me was that my merge can proceed if I stage a change that is identical to the merge result. That is, if my merge result would be to take the contents from "theirs", then my merge can proceed if I've already staged the same contents: input result anc ours theirs idx wd merge result idx wd 4 A AB B B take BB B 5 A AB B C take BB C This seems unexpected - is there a use-case that this enables or is this accidental? Another surprising result was that if I have deleted a file (and staged the deletion or not) then the merge will proceed and the file in question will be recreated. Consider "X" to be a missing file: input result anc ours theirs idx wd merge result idx wd 6 A AB A X take BB B 7 A AB X X take BB B I wouldn't have expected a file I deleted to be recreated with the other branch's contents. Is this behavior also intentional? Finally, there are cases when you have staged a deletion of the file and you have unstaged changes in your workdir where the merge will silently delete the unstaged data. If there is a conflict, the xdiff output will overwrite the unstaged file: input result anc ours theirs idx wd merge result idx wd 8 A BC X D conflict X diff3_file And similarly, while git-merge-recursive (only) will also remove my untracked file when there are no changes in our branch but the file was deleted in their branch: input result anc ours theirs idx wd merge result idx wd 9 A AX X B delete file X X I trust the last two cases, where data is lost, are bugs to report, but could I get clarification on the other situations? Thanks- -ed -- 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] Provide a mechanism to turn off symlink resolution in ceiling paths
Michael Haggerty writes: > This is a possible implementation (untested!) of Junio's suggestion, > with the slight twist that the empty entry can appear anywhere in > GIT_CEILING_DIRECTORIES and only turns off symlink expansion for > subsequent entries. Sounds like a good way to go, I think. Originally I thought that checking with the elements literally and stop when we have a match, and then falling back to call realpath() on them to compare, might be a solution, but I do not think it is. I haven't thought things through to convince myself that is the best approach to require the users to explicitly tell us to omit calls to realpath(). Perhaps it is the best we could do, but there might be even better ways somebody can think up. -- 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] Documentation/githooks: Explain pre-rebase parameters
"W. Trevor King" writes:
> Since $upstream_arg will always be set, would it make sense to change
> the `${1+"$@"}` syntax in run_pre_rebase_hook() to a plain "$@"?
I suspect that there no longer is a need for ${1+"$@"} in today's
world even when you do not have arguments, and it certainly is fine
if you want to update that particular instance in the function with
a single caller that calls it with 1 or 2 arguments, especially if
you are updating the code in the vicinity.
I however do not think it is worth blindly replacing them tree-wide
just for the sake of changing them. The upside of helping beginning
shell programers by possibly better readability does not look great,
compared to the downside of possibly breaking somebody who is still
on a broken shell that the old idiom is still helping.
--
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: Credentials and the Secrets API...
On Sat, 9 Feb 2013 05:58:47 -0500 John Szakmeister wrote: JS> On Thu, Feb 7, 2013 at 9:46 AM, Ted Zlatanov wrote: >> On Thu, 27 Oct 2011 12:05:03 -0400 John Szakmeister >> wrote: >> JS> Just wanted to keep folks in the loop. It turns out that the Secrets JS> API is still to young. I asked about the format to store credentials JS> in (as far as attributes), and got a response from a KDE developer JS> that says it's still to young on their front. They hope to have JS> support in the next release of KDE. But there's still the issue of JS> what attributes to use. >> Do you think the Secrets API has matured enough? KDE has had a new >> release since your post... JS> Yes, I think it has. Several other applications appear to be using JS> it, including some things considered "core" in Fedora--which is a good JS> sign. Wonderful. Do you still have interest in working on this credential? Ted -- 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] Documentation/githooks: Explain pre-rebase parameters
On Tue, Feb 19, 2013 at 11:08:29AM -0800, Junio C Hamano wrote:
> "W. Trevor King" writes:
> > Also, it appears that the `git-rebase--*.sh` handlers don't use the
> > pre-rebase hook. Is this intentional?
>
> The codeflow of git-rebase front-end, when you start rebasing, will
> call run_pre_rebase_hook before calling run_specific_rebase. It
> will be redundant for handlers to then call it again, no?
>
> In "rebase --continue" and later steps, you would not want to see
> the hook trigger.
Ah, that makes sense.
> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> > index b9003fe..bc837c6 100644
> > --- a/Documentation/githooks.txt
> > +++ b/Documentation/githooks.txt
> > @@ -140,9 +140,10 @@ the outcome of 'git commit'.
> > pre-rebase
> > ~~
> >
> > -This hook is called by 'git rebase' and can be used to prevent a branch
> > -from getting rebased.
> > -
> > +This hook is called by 'git rebase' and can be used to prevent a
> > +branch from getting rebased. The hook takes two parameters: the
> > +upstream the series was forked from and the branch being rebased. The
> > +second parameter will be empty when rebasing the current branch.
>
> Technically this is incorrect.
>
> We call it with one or two parameters, and sometimes the second
> parameter is _missing_, which is different from calling with an
> empty string. For a script written in some scripting languages like
> shell and perl, the distinction may not matter (i.e. $2 and $ARGV[1]
> will be an empty string when stringified) but not all (accessing
> sys.argv[2] may give you an IndexError in Python).
Will fix in v2.
Since $upstream_arg will always be set, would it make sense to change
the `${1+"$@"}` syntax in run_pre_rebase_hook() to a plain "$@"?
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root
On Tue, Feb 19, 2013 at 06:47:23PM -0800, Junio C Hamano wrote: > Adam Spiers writes: > >> Remove a sweep-the-issue-under-the-rug conditional in check-ignore > >> that avoided to pass an empty string to the callchain while at it. > >> It is a valid question to ask for check-ignore if the top-level is > >> set to be ignored by default, even though the answer is most likely > >> no, if only because there is currently no way to specify such an > > > > Hmm, I see very little difference between the use of "most likely" and > > the use of the words "much" and "typically" which you previously > > considered "a sure sign that the design of the fix is iffy". > > Your patch were "The reason why feeding empty string upsets ^^ "patches were", or "patch was"? It's not clear which patch(es) you're referring to. > hash_name() were not investigated; by punting the '.' as input, and > ignoring the possibility that such a question might make sense, I > can work around the segfault. I don't see how explicitly referring to the possibility can be counted as ignoring it. > I do not even question if hash_name() > that misbehaves on an empty string is a bug. Just make sure we do > not tickle the function with a problematic input". Presumably the "I" here refers to anthropomorphized commit message rather than to me personally, since I did question hash_name()'s behaviour several times already. > The patch you are responding to declares that hash_name() should > work sensibly on an empty string, and that is the _only_ necessary > change for the fix. We could keep "&& path[0]", but even without > it, by fixing the hash_name(), we will no longer segfault. Yes, and as already stated, I agree that is a good thing. > My "most likely" is about "the special case '&& path[0]' produces > correct result, Sorry, I can't understand this. You are paraphrasing something and placing it inside "" quotes, but I can't find the corresponding source. I presumed it refers to this extract of your proposed patch's commit message: "Remove a sweep-the-issue-under-the-rug conditional in check-ignore that avoided to pass an empty string to the callchain while at it. It is a valid question to ask for check-ignore if the top-level is set to be ignored by default, even though the answer is most likely no" but I can't reconcile this extract with the paraphrase "the special case '&& path[0]' produces correct result". > and it is likely to stay so in the near future until > we update .gitignore format to allow users to say 'ignore the top by > default', which is not likely to happen soon". It is not about the > nature of the fix at all. > > Still do not see the difference? I think I *might* be beginning to see you were getting at, although my understanding is still clouded by the ambiguities detailed above. Is your point that the use of words like 'much' and 'typically' are a "sure sign" of "iffy design" _when_used_to_talk_about_fixes_ but not necessarily in other contexts? If so then it makes a bit more sense to me, even though I tend to disagree with such broadly sweeping generalizations, especially when the qualifying context is missing. That aside, your idea of looking out for "bad smells" not only in code but also in the spoken language contained by commit messages and design discussions is an interesting one. I will try to bear that technique in mind more consciously in the future, and see how well it serves me. > The removal of the "&& path[0]" is about allowing such a question > whose likeliness may be remote. In the current .gitignore format, > you may not be able to say "ignore the whole thing by default", so > in that sense, the answer to the question this part of the code is > asking when given "." may always be "no". Keeping the "&& path[0]" > will optimize for that case. > > And "unusual thing to ask" below is to judge if answering such a > question is worth optimizing for (the verdict is "no, it is not a > common thing to do"). Yes, I understand and agree with these paragraphs. > >> entry in the .gitignore file. But it is an unusual thing to ask and > >> it is not worth optimizing for it by special casing at the top level > >> of the call chain. > > > > Although I agree with your proposed patch's sentiment of avoiding > > sweeping this corner case under the rug, 'check-ignore .' still > > wouldn't match anything if for example './' was a supported mechanism > > for ignoring the top level. > > It indicates that there may be more bugs (that may not result in > segv) somewhere in check-ignore codepath, if (1) > > echo ./ >.gitignore > > were to say "ignore everything in the tree by default.", and (2) the > real ignore check does work that way, but (3) > > git check-ignore . > > says "we do not ignore that one". Yes, I think we are saying exactly the same thing here, although if "It indicates that [...]" refers to your proposed patch's commit message then I don't think it in
Re: [PATCH] Bugfix: undefined htmldir in config.mak.autogen
On 02/20/2013 11:42 AM, Jiang Xin wrote:
> 2013/2/20 Stefano Lattarini :
>> On 02/20/2013 02:39 AM, Jiang Xin wrote:
>>>
>>> [SNIP]
>>>
>>> I am not familiar with autoconf. After clone autoconf and check,
>>> I cannot find a neat way to change "htmldir" default location to
>>> use ${datarootdir} (just like mandir).
>>>
>> This one-line change should be enough to do what you want:
>>
>> diff --git a/configure.ac b/configure.ac
>> index 1991258..2bfbec9 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -149,6 +149,8 @@ AC_CONFIG_SRCDIR([git.c])
>>config_file=config.mak.autogen
>>config_in=config.mak.in
>>
>> +AC_SUBST([htmldir], ['${datarootdir}'])
>> +
>>GIT_CONF_SUBST([AUTOCONFIGURED], [YesPlease])
>>
>
> If changed like that, set:
>
> AC_SUBST([htmldir], ['${datarootdir}/doc/git-doc'])
>
> In the generated "configure" file, this instruction will be inserted
> after the option_parse block (not before), and will override what
> the user provided by running "./configure --htmldir=DOCDIR".
>
Yikes, you're right. Scratch my suggestion then; the issue should
probably be brought up on the autoconf mailing list. Albeit I think
it is by design that autoconf doesn't let a package to override the
defaults for installation directory: this way, the end users can
expect consistent, well-documented defaults for all autoconf-based
packages.
> BTW, add "docdir = @docdir@" to "config.mak.in", also let
> "./configure --docdir=DIR" works properly.
>
Thanks, and sorry for the noise,
Stefano
--
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] git-multimail: a replacement for post-receive-email
Michael Haggerty writes: > A while ago, I submitted an RFC for adding a new email notification > script to "contrib" [1]. We've discussed offline with Michael, a few patches have been merged, and there are still a few pending pull requests. I liked the script already, but it's getting even cooler ;-). A few more random thoughts (not on my personal todo-list): * It may make sense to add the short sha1 of the new reference in email titles (branch foo updated -> branch foo updated to $sha1), so that gmail users do not get a single huge thread "branch foo updated". (Yes, I do know about the Reference field, but gmail uses Subject: for threading). * Perhaps we should allow a per-branch configuration, like [multimailhook] mailingList = s...@list.com [multimailhook "refs/heads/my-branch"] mailingList = some-ot...@list.com = Branch specific would override value for Config.get(), and Config.get_all() should probably list both the branch-specific and the other keys. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: Google Summer of Code 2013 (GSoC13)
Christian Couder writes:
> - yes, we could improve mentoring by providing better projects and
> insisting even more on submitting earlier
A few words about my experience, not with GSoC, but with school projects
(I've been proposing a few students in Ensimag to contribute to Git each
year since 2010).
Last year, we've been using Scrum, and the "definition of done" was a
very helpful tool. In Scrum, nothing is ever "half done", it is either
"done" or "not done". Out of a 3 weeks project, the definition of done
was initially "ready to be sent to the list", then "sent to the list, no
major criticism in reviews" the second week, and "sent to the list, no
more objections in reviews" the last week. At the beginning of each week
("sprint" in Scrum), students were commiting to a list of tasks, and at
the end of each week, we evaluated how many were done. This encouraged
students to avoid overcommiting and send patches early. Some of them
validated nothing at all the first week: they hadn't realized the
distance between their notion of clean working code and the one on this
list, but at least they realized it early enough.
Of course, even with that, I had to continue the work to push it to
master for some patch series, and discard some series that were
basically not there.
Having several small projects instead of one big was very important. I'm
not sure how the GSoC would feel about a list of small tasks instead of
one ambitious project however.
My main disappointment is that I never managed to keep students in the
community past the end of the project.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: Google Summer of Code 2013 (GSoC13)
Hi, On Wed, Feb 20, 2013 at 7:50 AM, Shawn Pearce wrote: > On Mon, Feb 18, 2013 at 9:42 AM, Jeff King wrote: >> On Mon, Feb 18, 2013 at 06:23:01PM +0100, Thomas Rast wrote: >> >>> * We need an org admin. AFAIK this was done by Peff and Shawn in >>> tandem last year. Would you do it again? >> >> I will do it again, if people feel strongly about Git being a part of >> it. However, I have gotten a little soured on the GSoC experience. Not >> because of anything Google has done; it's a good idea, and I think they >> do a fine of administering the program. But I have noticed that the work >> that comes out of GSoC the last few years has quite often not been >> merged, or not made a big impact in the codebase, and nor have the >> participants necessarily stuck around. > > This. I think it is ok if the code doesn't make a big impact in the code base and it is ok too if the participants don't stuck around. Of course I would love both of these things to happen, but we have to be realistic and just stop expecting it. > I actually think Git should take a year off from GSoC and not > participate. Consequently I will not be volunteering as backup org > admin. > > Git has been involved since 2007. In all of that time we have had very > few student projects merge successfully into their upstream project > (e.g. git.git, JGit or libgit2) before the end of GSoC. Even fewer > students have stuck around and remained active contributors. When I > look at the amount of effort we contributors put into GSoC, I think we > are misusing our limited time and resources. I don't think so, at least not for me. I feel happy to mentor or co-mentor GSoC student and I don't think I would work much more on git these days if git was not participating to the GSoC. > The intention of the GSoC > program is to grow new open source developers, and increase our > community of contributors. Somehow I think Git is falling well short > of its potential here. This is especially true if you compare Git's > GSoC program to some other equally long-running GSoC programs. > >> And I do not want to blame the students here (some of whom are on the cc >> list :) ). They are certainly under no obligation to stick around after >> GSoC ends, and I know they have many demands on their time. But I am >> also thinking about what Git wants to get out of GSoC (and to my mind, >> the most important thing is contributors). > > I agree, our students have been pretty terrific. I think the > shortcomings in our GSoC program are on the mentoring side. Our > program has not really had much success with keeping students active > and engaged post GSoC. I see that primarily as a mentoring failure. > And its one we keep repeating each year. I don't quite agree with this. My experience has been the following: - 2008: the student I co-mentored did pretty well though he didn't send to the list his patch series early enough. So there was some mentoring failure, but anyway the student stuck around for 9 months and managed to get 53 commits merged. - 2009: if I remember well, it was decided to have only 2 GSoC student that year, and that 5 people would co-mentor both of them together. One of the student did nearly nothing. The other one sent his patch series too late to the list. My opinion is that he relied too much on the people mentoring him and he worked on something that was difficult to merge. - 2010: the student I co-mentored stopped working 3 weeks before the mid-term evaluation despite some warnings from me and Peff, and he had not been doing much a few weeks before that, so we decided to fail him at the mid term evaluation. - 2011: I was lucky to mentor Ram who did well and is still around. So my opinion is that we have some students who are just not doing enough (2 out of 5). Then we have some good students, 2 out of 5 who could sometimes do better if we insisted more on submitting earlier to the mailing list. And we have a few students (1 out of 5) who work difficult to merge projects and who could do better if we insisted more on submitting earlier to the mailing list. So my conclusions are: - it's quite often going well or well enough - when it's not going well often the student is responsible - yes, we could improve mentoring by providing better projects and insisting even more on submitting earlier [...] >> - There is also the angle that even if _Git_ doesn't benefit directly >> from people sticking around, those people may float into other open >> source projects and work on them. Which makes the world a better >> place on the whole. > > Yes, sure, OK. But if Git doesn't participate in GSoC this year > another org will, and this same benefit will still be had by the > greater open source community. The greater open source community benefits a lot these days when Git is improved and get new contributors, as git is now by far the most widely used version control system in the open source community. So my opinion is that we should have
Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root
On Tue, Feb 19, 2013 at 06:53:07PM -0800, Junio C Hamano wrote: > Adam Spiers writes: > > > OK, thanks for the information. IMHO it would be nice if 'git > > format-patch' and 'git am' supported this style of inline patch > > inclusion, but maybe there are good reasons to discourage it? > > "git am --scissors" is a way to process such e-mail where the patch > submitter continues discussion in the top part of a message, > concludes the message with: > > A patch to do so is attached. > -- >8 -- > > and then tells the MUA to read in an output from format-patch into > the e-mail buffer. Ah, nice! I didn't know about that. > You still need to strip out unneeded headers > like the "From ", "From: " and "Date: " lines when you add the > scissors anyway, and this is applicable only for a single-patch > series, so the "feature" does not fit well as a format-patch option. Rather than requiring the user to manually strip out unneeded headers, wouldn't it be friendlier and less error-prone to add a new --inline option to format-patch which omitted them in the first place? It should be easy to make it bail with an error when multiple revisions are requested. -- 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] Bugfix: undefined htmldir in config.mak.autogen
2013/2/20 Stefano Lattarini :
> On 02/20/2013 02:39 AM, Jiang Xin wrote:
>>
>> [SNIP]
>>
>> I am not familiar with autoconf. After clone autoconf and check,
>> I cannot find a neat way to change "htmldir" default location to
>> use ${datarootdir} (just like mandir).
>>
> This one-line change should be enough to do what you want:
>
> diff --git a/configure.ac b/configure.ac
> index 1991258..2bfbec9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -149,6 +149,8 @@ AC_CONFIG_SRCDIR([git.c])
>config_file=config.mak.autogen
>config_in=config.mak.in
>
> +AC_SUBST([htmldir], ['${datarootdir}'])
> +
>GIT_CONF_SUBST([AUTOCONFIGURED], [YesPlease])
>
If changed like that, set:
AC_SUBST([htmldir], ['${datarootdir}/doc/git-doc'])
In the generated "configure" file, this instruction will be inserted
after the option_parse block (not before), and will override what
the user provided by running "./configure --htmldir=DOCDIR".
BTW, add "docdir = @docdir@" to "config.mak.in", also let
"./configure --docdir=DIR" works properly.
--
Jiang Xin
--
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 v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks
On Tue, 19 Feb 2013, Junio C Hamano wrote: > Assuming that this says "yes": > > D=/afs/athena.mit.edu/user/a/n/andersk/my/dir > cd "$D" > test "$(/bin/pwd)" = "$D" && echo yes Correct. > Perhaps existing of an empty element in the list would do? E.g. > > GIT_CEILING_DIRECTORIES=:/afs/athena.mit.edu/users/a/n/andesk > > or something like that. And in such a case, we do not run realpath on > the elements on the list before comparing them with what we get from > getcwd(3). That seems reasonable, and has the advantage of backwards compatibility with versions before 1.8.1.2, I guess. Anders -- 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] Bugfix: undefined htmldir in config.mak.autogen
On Tue, Feb 19, 2013 at 03:40:16PM -0800, Junio C Hamano wrote: > I am not sure if such a layout can be actually used for installing, > though. Didn't we see some issues around the relativeness of > htmldir and mandir vs passing them down to Documentation/Makefile, > or is it not an issue when ./configure and config.mak.autogen is > used? If these variables are set explicitly in config.mak.autogen (or indeed config.mak) then these values should override the ones calculated in the Makefiles so that we avoid that problem - the problem occurs if the relative paths from the top-level Makefile are exported to Documentation/Makefile. John -- 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] Provide a mechanism to turn off symlink resolution in ceiling paths
Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks in
ceiling paths' changed the setup code to resolve symlinks in the
entries in GIT_CEILING_DIRECTORIES. Because those entries are
compared textually to the symlink-resolved current directory, an entry
in GIT_CEILING_DIRECTORIES that contained a symlink would have no
effect. It was known that this could cause performance problems if
the symlink resolution *itself* touched slow filesystems, but it was
thought that such use cases would be unlikely.
After this change was released, Anders Kaseorg
reported:
> [...] my computer has been acting so slow when I’m not connected to
> the network. I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as
> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
> /afs/athena.mit.edu/user/a/n which all live in different AFS
> volumes). Now when I’m not connected to the network, every
> invocation of Git, including the __git_ps1 in my shell prompt, waits
> for AFS to timeout.
So provide the following mechanism to turn off symlink resolution in
GIT_CEILING_DIRECTORIES entries: if that environment variable contains
an empty entry (e.g., GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy), then do not resolve
symlinks in paths that follow the empty entry.
---
This is a possible implementation (untested!) of Junio's suggestion,
with the slight twist that the empty entry can appear anywhere in
GIT_CEILING_DIRECTORIES and only turns off symlink expansion for
subsequent entries. (The original suggestion would be similarly easy
to implement if it is preferred.)
Unfortunately I am swamped with other work right now so I don't have
time to test the code and might not be able to respond promptly to
feedback.
Another alternative (not implemented here) would be to support a
second environment variable with an ugly name like
GIT_CEILING_DIRECTORIES_NO_SYMLINKS which, when set, tells Git not to
resolve symlinks in GIT_CEILING_DIRECTORIES. Hopefully the variable
name itself would warn the user that symlinks are an issue.
The ugliness of the situation unfortunately seems to preclude a
non-ugly solution of one form or another.
Documentation/git.txt | 18 --
setup.c | 32 ++--
2 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index da0115f..35c0517 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -674,12 +674,18 @@ git so take care if using Cogito etc.
The '--namespace' command-line option also sets this value.
'GIT_CEILING_DIRECTORIES'::
- This should be a colon-separated list of absolute paths.
- If set, it is a list of directories that git should not chdir
- up into while looking for a repository directory.
- It will not exclude the current working directory or
- a GIT_DIR set on the command line or in the environment.
- (Useful for excluding slow-loading network directories.)
+ This should be a colon-separated list of absolute paths. If
+ set, it is a list of directories that git should not chdir up
+ into while looking for a repository directory (useful for
+ excluding slow-loading network directories). It will not
+ exclude the current working directory or a GIT_DIR set on the
+ command line or in the environment. Normally, Git has to read
+ the entries in this list are read to resolve any symlinks that
+ might be present. However, if even this access is slow, you
+ can add an empty entry to the list to tell Git that the
+ subsequent entries are not symlinks and needn't be resolved;
+ e.g.,
+ 'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
When run in a directory that does not have ".git" repository
diff --git a/setup.c b/setup.c
index f108c4b..1b12017 100644
--- a/setup.c
+++ b/setup.c
@@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const
char *prefix, int prefix_
/*
* A "string_list_each_func_t" function that canonicalizes an entry
* from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
- * discards it if unusable.
+ * discards it if unusable. The presence of an empty entry in
+ * GIT_CEILING_DIRECTORIES turns off canonicalization for all
+ * subsequent entries.
*/
static int canonicalize_ceiling_entry(struct string_list_item *item,
- void *unused)
+ void *cb_data)
{
+ int *empty_entry_found = cb_data;
char *ceil = item->string;
- const char *real_path;
- if (!*ceil || !is_absolute_path(ceil))
+ if (!*ceil) {
+ *empty_entry_found = 1;
return 0;
- real_path = real_path_if_valid(ceil);
-
Re: Git Merge 2013 Conference, Berlin
On Wed, Feb 20, 2013 at 09:05:46AM +0100, Michael J Gruber wrote: > Maybe, we can - for the next time - try to coordinate the date with the > various international IT conferences which take place here, like > Linux-Tag in Berlin (just a few weeks apart), CEBIT in Hannover or the > smaller Chemnitzer Linux-Tage (or coordinate with events somewhere else > in Europe). That would give contributors not only more incentive to come > to the Git event, but also a better chance for successful negotiations > with their employers. Extending a business trip by a few days is a minor > issue, granting one in the first place not always. I actually have the opposite preference; I hate when conferences are back-to-back, because by the fourth day I'm pretty tired of conferencing and ready to sit in my hotel room. Of course, my employer is rather generous with giving time for Git conferences, so I do not feel the time crunch the way other people do, and can optimize for relaxation. But I wonder where other people come down in that tradeoff. -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: [PATCHv2 04/10] pkt-line: change error message for oversized packet
Jeff King wrote: > >In the meantime, please hold off on what I've posted so far (that >includes the jk/smart-http-robustify topic). Surely. I'm done for the night already. Looking forward to see the reroll tomorrow. Thanks. -- Pardon terseness, typo and HTML from a tablet. -- 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: What's cooking in git.git (Feb 2013, #08; Tue, 19)
On Tue, Feb 19, 2013 at 11:44:48PM -0800, Junio C Hamano wrote: > * jk/smart-http-robustify (2013-02-17) 3 commits > - remote-curl: sanity check ref advertisement from server > - remote-curl: verify smart-http metadata lines > - pkt-line: teach packet_get_line a no-op mode > > Parse the HTTP exchange that implements the native Git protocol as > a series of stateless RPC more carefully to diagnose protocol > breakage better. > > Will merge to and hold in 'next'. I just posted elsewhere, but to make sure I get your attention: please hold off on this. The sanity check in the top patch is not right (because of the ERR/flush thing that Jonathan brought up). I'll have an updated series tomorrow. -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: [PATCHv2 04/10] pkt-line: change error message for oversized packet
On Mon, Feb 18, 2013 at 04:33:31PM -0500, Jeff King wrote: > On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote: > > > Jeff King writes: > > > > > But it's easy to do (1), and it starts the clock ticking for > > > the 1000-byte readers to become obsolete. > > > > Yup, I agree with that goal. > > Having just looked at the pkt-line callers a lot, I think most of them > could go for something like: > [...] > > That would actually simplify the callers a bit, and would harmonize the > buffer sizes at the same time. I'll look into doing a series tonight. Just a quick update on this series. It ended up taking more nights than I thought. :) The result looks much better than what I posted before, and I found several other corner cases and bugs in packet parsing, too. I'm going to hold off on posting it tonight, as I'm now up to 19 patches, and after several rounds of "rebase -i", I need to give it a final read-through myself before inflicting it on anyone else. I'll do that and post it tomorrow. In the meantime, please hold off on what I've posted so far (that includes the jk/smart-http-robustify topic). -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
[BUG] Infinite make recursion when configure.ac changes
>From a pristine master checkout: $ make configure && ./configure make ... # All successfull $ touch configure.ac $ make GEN config.status make[1]: Entering directory `/storage/home/stefano/git/src' GEN config.status make[2]: Entering directory `/storage/home/stefano/git/src' GEN config.status make[3]: Entering directory `/storage/home/stefano/git/src' GEN config.status make[4]: Entering directory `/storage/home/stefano/git/src' GEN config.status make[5]: Entering directory `/storage/home/stefano/git/src' GEN config.status ... and I have to hit ^C to interrupt that recursion. This seems due to the change in commit v1.7.12.4-1-g1226504: the issue is still present there, but no longer is in the preceding commit 7e201053 (a.k.a. v1.7.12.4). I haven't investigated this any further for the moment. Regards, Stefano -- 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] Bugfix: undefined htmldir in config.mak.autogen
On 02/20/2013 02:39 AM, Jiang Xin wrote:
>
> [SNIP]
>
> I am not familiar with autoconf. After clone autoconf and check,
> I cannot find a neat way to change "htmldir" default location to
> use ${datarootdir} (just like mandir).
>
This one-line change should be enough to do what you want:
diff --git a/configure.ac b/configure.ac
index 1991258..2bfbec9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -149,6 +149,8 @@ AC_CONFIG_SRCDIR([git.c])
config_file=config.mak.autogen
config_in=config.mak.in
+AC_SUBST([htmldir], ['${datarootdir}'])
+
GIT_CONF_SUBST([AUTOCONFIGURED], [YesPlease])
# Directories holding "saner" versions of common or POSIX binaries.
Not sure whether this a good idea though (I haven't really followed the
discussion); but it is easily doable.
HTH,
Stefano
--
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 Merge 2013 Conference, Berlin
Junio C Hamano venit, vidit, dixit 20.02.2013 00:47: > Scott Chacon writes: > >> Junio, are you interested in attending? > > I am interested in meeting our European contributors, but Berlin is > kind of very far, so give me a few days to think about it. > > Thanks. > Maybe, we can - for the next time - try to coordinate the date with the various international IT conferences which take place here, like Linux-Tag in Berlin (just a few weeks apart), CEBIT in Hannover or the smaller Chemnitzer Linux-Tage (or coordinate with events somewhere else in Europe). That would give contributors not only more incentive to come to the Git event, but also a better chance for successful negotiations with their employers. Extending a business trip by a few days is a minor issue, granting one in the first place not always. Michael -- 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
