[PATCH 2/2] Makefile: allow static linking against libcurl
Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d6330bc..3c151d3 100644 --- a/Makefile +++ b/Makefile @@ -37,6 +37,9 @@ all:: # Define CURL_CONFIG to the path to a curl-config binary other than the # default 'curl-config'. # +# Define CURL_STATIC to statically link libcurl. Only applies if +# CURL_CONFIG is used. +# # Define CURLDIR=/foo/bar if your curl header and library files are in # /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, # but is less robust. @@ -1139,7 +1142,11 @@ else else CURL_CONFIG ?= curl-config BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) - CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifdef CURL_STATIC + CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs) + else + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + endif ifeq $(CURL_LIBCURL) $(error curl not detected; try setting CURLDIR) endif -- 1.9.1.423.g4596e3a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Makefile: use curl-config to determine curl flags
curl-config should always be installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. Allow overriding CURL_CONFIG to a custom path to curl-config, to compile against a curl installation other than the first in PATH. Use this only when CURLDIR is not explicitly specified, to continue supporting older builds. Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 35 +++ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 2128ce3..d6330bc 100644 --- a/Makefile +++ b/Makefile @@ -34,8 +34,12 @@ all:: # git-http-push are not built, and you cannot use http:// and https:// # transports (neither smart nor dumb). # +# Define CURL_CONFIG to the path to a curl-config binary other than the +# default 'curl-config'. +# # Define CURLDIR=/foo/bar if your curl header and library files are in -# /foo/bar/include and /foo/bar/lib directories. +# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, +# but is less robust. # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports (dumb). @@ -143,9 +147,11 @@ all:: # # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin). # -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). Only used +# if CURLDIR is set. # -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). Only +# used if CURLDIR is set. # # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin). # @@ -1121,18 +1127,23 @@ else # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. BASIC_CFLAGS += -I$(CURLDIR)/include CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + ifdef NEEDS_SSL_WITH_CURL + CURL_LIBCURL += -lssl + ifdef NEEDS_CRYPTO_WITH_SSL + CURL_LIBCURL += -lcrypto + endif + endif + ifdef NEEDS_IDN_WITH_CURL + CURL_LIBCURL += -lidn + endif else - CURL_LIBCURL = -lcurl - endif - ifdef NEEDS_SSL_WITH_CURL - CURL_LIBCURL += -lssl - ifdef NEEDS_CRYPTO_WITH_SSL - CURL_LIBCURL += -lcrypto + CURL_CONFIG ?= curl-config + BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifeq $(CURL_LIBCURL) + $(error curl not detected; try setting CURLDIR) endif endif - ifdef NEEDS_IDN_WITH_CURL - CURL_LIBCURL += -lidn - endif REMOTE_CURL_PRIMARY = git-remote-http$X REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X -- 1.9.1.423.g4596e3a -- 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 1/2] Makefile: use curl-config to determine curl flags
On Mon, Apr 14, 2014 at 4:22 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: curl-config should always be installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. Allow overriding CURL_CONFIG to a custom path to curl-config, to compile against a curl installation other than the first in PATH. Use this only when CURLDIR is not explicitly specified, to continue supporting older builds. Signed-off-by: Dave Borowitz dborow...@google.com Sounds logically the right thing to do. Was there a particular platform that needed this (i.e. cannot be made to work with the existing CURLDIR and guessing flags and dependeent libraries) that may be worth mentioning in the log message? My end goal is to statically link git on Mac OS X (10.9) against a newer version of libcurl than ships with the OS. The normal CURLDIR approach should work with system libcurl: $ /usr/bin/curl-config --libs -lcurl But it gets a bit more complicated with a recent curl version. This likely has to do with the set of enabled options; I passed flags to ./configure based on the build script MacOSX-Framework included in the curl distribution: $ ~/d/curl-out-7.36.0/bin/curl-config --libs -L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv -lldap -lz And with --static-libs there's just that much more: $ ~/d/curl-out-7.36.0/bin/curl-config --static-libs /Users/dborowitz/d/curl-out-7.36.0/lib/libcurl.a -Wl,-syslibroot,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk -arch x86_64 -Wl,-headerpad_max_install_names -framework CoreFoundation -framework Security -lgssapi_krb5 -lresolv -lldap -lz So I don't think specifying NEEDS_*_WITH_CURL scales to this use case. While writing this up I also noticed an issue with the second patch, namely that `curl-config --static-libs` is empty when curl is not built for static linking. I will reroll with a more detailed commit message and a fix to the second patch. --- Makefile | 35 +++ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 2128ce3..d6330bc 100644 --- a/Makefile +++ b/Makefile @@ -34,8 +34,12 @@ all:: # git-http-push are not built, and you cannot use http:// and https:// # transports (neither smart nor dumb). # +# Define CURL_CONFIG to the path to a curl-config binary other than the +# default 'curl-config'. +# # Define CURLDIR=/foo/bar if your curl header and library files are in -# /foo/bar/include and /foo/bar/lib directories. +# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, +# but is less robust. # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports (dumb). @@ -143,9 +147,11 @@ all:: # # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin). # -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). Only used +# if CURLDIR is set. # -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). Only +# used if CURLDIR is set. # # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin). # @@ -1121,18 +1127,23 @@ else # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. BASIC_CFLAGS += -I$(CURLDIR)/include CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + ifdef NEEDS_SSL_WITH_CURL + CURL_LIBCURL += -lssl + ifdef NEEDS_CRYPTO_WITH_SSL + CURL_LIBCURL += -lcrypto + endif + endif + ifdef NEEDS_IDN_WITH_CURL + CURL_LIBCURL += -lidn + endif else - CURL_LIBCURL = -lcurl - endif - ifdef NEEDS_SSL_WITH_CURL - CURL_LIBCURL += -lssl - ifdef NEEDS_CRYPTO_WITH_SSL - CURL_LIBCURL += -lcrypto + CURL_CONFIG ?= curl-config + BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifeq $(CURL_LIBCURL) + $(error curl not detected; try setting CURLDIR) endif endif - ifdef NEEDS_IDN_WITH_CURL - CURL_LIBCURL += -lidn - endif REMOTE_CURL_PRIMARY = git-remote-http$X REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org
[PATCH v2 2/2] Makefile: allow static linking against libcurl
This requires more flags than can be guessed with the old-style CURLDIR and related options, so is only supported when curl-config is present. Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index d6330bc..e4c93f6 100644 --- a/Makefile +++ b/Makefile @@ -37,6 +37,9 @@ all:: # Define CURL_CONFIG to the path to a curl-config binary other than the # default 'curl-config'. # +# Define CURL_STATIC to statically link libcurl. Only applies if +# CURL_CONFIG is used. +# # Define CURLDIR=/foo/bar if your curl header and library files are in # /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, # but is less robust. @@ -1139,9 +1142,16 @@ else else CURL_CONFIG ?= curl-config BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) - CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) - ifeq $(CURL_LIBCURL) - $(error curl not detected; try setting CURLDIR) + ifdef CURL_STATIC + CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs) + ifeq $(CURL_LIBCURL) + $(error libcurl not detected or not compiled with static support) + endif + else + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifeq $(CURL_LIBCURL) + $(error libcurl not detected; try setting CURLDIR) + endif endif endif -- 1.9.1.423.g4596e3a -- 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 v2 1/2] Makefile: use curl-config to determine curl flags
curl-config should always be installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. Allow overriding CURL_CONFIG to a custom path to curl-config, to compile against a curl installation other than the first in PATH. Depending on the set of features curl is compiled with, there may be more libraries required than the previous two options of -lssl and -lidn. For example, with a vanilla build of libcurl-7.36.0 on Mac OS X 10.9: $ ~/d/curl-out-7.36.0/lib/curl-config --libs -L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv -lldap -lz Use this only when CURLDIR is not explicitly specified, to continue supporting older builds. Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 35 +++ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 2128ce3..d6330bc 100644 --- a/Makefile +++ b/Makefile @@ -34,8 +34,12 @@ all:: # git-http-push are not built, and you cannot use http:// and https:// # transports (neither smart nor dumb). # +# Define CURL_CONFIG to the path to a curl-config binary other than the +# default 'curl-config'. +# # Define CURLDIR=/foo/bar if your curl header and library files are in -# /foo/bar/include and /foo/bar/lib directories. +# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, +# but is less robust. # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports (dumb). @@ -143,9 +147,11 @@ all:: # # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin). # -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). Only used +# if CURLDIR is set. # -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). Only +# used if CURLDIR is set. # # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin). # @@ -1121,18 +1127,23 @@ else # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. BASIC_CFLAGS += -I$(CURLDIR)/include CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + ifdef NEEDS_SSL_WITH_CURL + CURL_LIBCURL += -lssl + ifdef NEEDS_CRYPTO_WITH_SSL + CURL_LIBCURL += -lcrypto + endif + endif + ifdef NEEDS_IDN_WITH_CURL + CURL_LIBCURL += -lidn + endif else - CURL_LIBCURL = -lcurl - endif - ifdef NEEDS_SSL_WITH_CURL - CURL_LIBCURL += -lssl - ifdef NEEDS_CRYPTO_WITH_SSL - CURL_LIBCURL += -lcrypto + CURL_CONFIG ?= curl-config + BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifeq $(CURL_LIBCURL) + $(error curl not detected; try setting CURLDIR) endif endif - ifdef NEEDS_IDN_WITH_CURL - CURL_LIBCURL += -lidn - endif REMOTE_CURL_PRIMARY = git-remote-http$X REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X -- 1.9.1.423.g4596e3a -- 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 version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 11:31 AM, Junio C Hamano gits...@pobox.com wrote: Erik Faye-Lund kusmab...@gmail.com writes: We're using Curl 7.30.0 in msysGit (and thus also Git for Windows), so we should be able to include it. However, we do not have curl-config installed. Hmmm, between 2.0-rc0 and 2.0-rc1 there is 61a64fff (Makefile: use curl-config to determine curl flags, 2014-04-15) merged already, which makes this assumption and a claim based on that assumption: curl-config should always be installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. which may make things worse for you guys, unless you are explicitly setting CURLDIR and other Makefile variables. Yes, I can see that assumption may not always hold. But I'm slightly surprised this worked in the first place; are you saying this is a system where: -curl-config is not installed -but -lcurl still works -without setting CURLDIR ? I think I should probably re-roll the patch to default to the old behavior (blind -lcurl) if curl-config returns the empty string, which I believe is also the case when the binary is not found. -- 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 2/2] Makefile: allow static linking against libcurl
This requires more flags than can be guessed with the old-style CURLDIR and related options, so is only supported when curl-config is present. Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 12 1 file changed, 12 insertions(+) diff --git a/Makefile b/Makefile index cb4ee37..360d427 100644 --- a/Makefile +++ b/Makefile @@ -39,6 +39,9 @@ all:: # is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set, # uses -lcurl with no additional library detection. # +# Define CURL_STATIC to statically link libcurl. Only applies if +# CURL_CONFIG is used. +# # Define CURLDIR=/foo/bar if your curl header and library files are in # /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, # but is less robust. @@ -1137,6 +1140,9 @@ else endif ifeq $(CURL_LIBCURL) + ifdef CURL_STATIC +$(error CURL_STATIC must be used with CURL_CONFIG) + endif ifdef CURLDIR # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. BASIC_CFLAGS += -I$(CURLDIR)/include @@ -1155,6 +1161,12 @@ else endif else BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) + ifdef CURL_STATIC + CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs) + ifeq $(CURL_LIBCURL) +$(error libcurl not detected or not compiled with static support) + endif + endif endif REMOTE_CURL_PRIMARY = git-remote-http$X -- 1.9.1.423.g4596e3a -- 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 version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 12:40 PM, Junio C Hamano gits...@pobox.com wrote: 'Dave Borowitz dborow...@google.com' via msysGit msys...@googlegroups.com writes: I think I should probably re-roll the patch to default to the old behavior (blind -lcurl) if curl-config returns the empty string, which I believe is also the case when the binary is not found. Thanks for a prompt response; as this may indicate a possible regression on what is already in 2.0-rc1, I am tempted to say that we should revert the merge in the meantime. Unless such an update can be done in a fairly trivial way, that is. I already sent the update. There's a bit more (unavoidable) complexity in the conditionals but it should still be fairly straightforward. -- 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] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
The original implementation of CURL_CONFIG support did not match the original behavior of using -lcurl when CURLDIR was not set. This broke implementations that were lacking curl-config but did have libcurl installed along system libraries, such as MSysGit. In other words, the assumption that curl-config is always installed was incorrect. Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due to curl-config being missing), use the old behavior of falling back to -lcurl. --- Makefile | 36 +--- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index 74a929b..79b7442 100644 --- a/Makefile +++ b/Makefile @@ -35,7 +35,9 @@ all:: # transports (neither smart nor dumb). # # Define CURL_CONFIG to the path to a curl-config binary other than the -# default 'curl-config'. +# default 'curl-config'. If CURL_CONFIG is unset or points to a binary that +# is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set, +# uses -lcurl with no additional library detection. # # Define CURL_STATIC to statically link libcurl. Only applies if # CURL_CONFIG is used. @@ -1127,9 +1129,27 @@ ifdef NO_CURL REMOTE_CURL_NAMES = else ifdef CURLDIR - # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. - BASIC_CFLAGS += -I$(CURLDIR)/include - CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + CURL_LIBCURL= + else + CURL_CONFIG ?= curl-config + ifeq $(CURL_CONFIG) + CURL_LIBCURL = + else + CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs) + endif + endif + + ifeq $(CURL_LIBCURL) + ifdef CURL_STATIC +$(error CURL_STATIC must be used with CURL_CONFIG) + endif + ifdef CURLDIR + # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. + BASIC_CFLAGS += -I$(CURLDIR)/include + CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + else + CURL_LIBCURL = -lcurl + endif ifdef NEEDS_SSL_WITH_CURL CURL_LIBCURL += -lssl ifdef NEEDS_CRYPTO_WITH_SSL @@ -1140,17 +1160,11 @@ else CURL_LIBCURL += -lidn endif else - CURL_CONFIG ?= curl-config BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) ifdef CURL_STATIC CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs) ifeq $(CURL_LIBCURL) - $(error libcurl not detected or not compiled with static support) - endif - else - CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) - ifeq $(CURL_LIBCURL) - $(error libcurl not detected; try setting CURLDIR) +$(error libcurl not detected or not compiled with static support) endif endif endif -- 1.9.1.423.g4596e3a -- 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 1/2] Makefile: use curl-config to determine curl flags
On Mon, Apr 28, 2014 at 12:44 PM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Dave Borowitz wrote: curl-config is usually installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. The previous version of these two patches is already part of master. Could you make an incremental patch? Done. Thanks for pointing that out, and sorry for the noise. Sorry for the fuss, 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] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Mon, Apr 28, 2014 at 1:46 PM, Dave Borowitz dborow...@google.com wrote: How about: If CURL_CONFIG is unset or points to a binary that is not found, defaults to the CURLDIR behavior. If CURLDIR is not set, this means using -lcurl with no additional library detection (other than NEEDS_*_WITH_CURL). Even better, I'll move the blurb about CURLDIR behavior to CURLDIR. -- 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] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Mon, Apr 28, 2014 at 1:50 PM, Jonathan Nieder jrnie...@gmail.com wrote: $ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk1 make -f mk1 foo mk1:2: *** commands commence before first target. Stop. $ echo -e 'ifndef FOO\n $(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk2 make -f mk2 foo mk2:2: *** bad things. Stop. Gah. Maybe it should be left-justified to avoid accentally breaking it again. I am ok with that on the basis that it is better to be ugly but obvious. I suspect if the Makefile were more littered with conditional $(error)s we would reconsider using tabs as indentation. 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: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder jrnie...@gmail.com wrote: Dave Borowitz wrote: Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due to curl-config being missing), use the old behavior of falling back to -lcurl. --- Makefile | 36 +--- 1 file changed, 25 insertions(+), 11 deletions(-) Sign-off? Oops. [...] +++ b/Makefile @@ -35,7 +35,9 @@ all:: # transports (neither smart nor dumb). # # Define CURL_CONFIG to the path to a curl-config binary other than the -# default 'curl-config'. +# default 'curl-config'. If CURL_CONFIG is unset or points to a binary that +# is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set, +# uses -lcurl with no additional library detection. I'm having a little trouble parsing this but don't have any better suggestion. How about: If CURL_CONFIG is unset or points to a binary that is not found, defaults to the CURLDIR behavior. If CURLDIR is not set, this means using -lcurl with no additional library detection (other than NEEDS_*_WITH_CURL). [...] - $(error libcurl not detected; try setting CURLDIR) +$(error libcurl not detected or not compiled with static support) Whitespace damage. Yes, but intentional, because Makefile parsing is weird. $ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk1 make -f mk1 foo mk1:2: *** commands commence before first target. Stop. $ echo -e 'ifndef FOO\n $(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk2 make -f mk2 foo mk2:2: *** bad things. Stop. See also: http://stackoverflow.com/questions/4713663/gnu-make-yields-commands-commence-before-first-target-error Except for the whitespace issues, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Mon, Apr 28, 2014 at 1:45 PM, Junio C Hamano gits...@pobox.com wrote: I still think the implementation of If CURL_CONFIG is unset bit is a bit redundant, though. If CURL_CONFIG is unset, then $(shell $(CURL_CONFIG) --libs) produces make: --libs: Command not found, which may be confusing. -- 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 v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
The original implementation of CURL_CONFIG support did not match the original behavior of using -lcurl when CURLDIR was not set. This broke implementations that were lacking curl-config but did have libcurl installed along system libraries, such as MSysGit. In other words, the assumption that curl-config is always installed was incorrect. Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due to curl-config being missing), use the old behavior of falling back to -lcurl. Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 41 - 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 74a929b..81e8214 100644 --- a/Makefile +++ b/Makefile @@ -35,14 +35,17 @@ all:: # transports (neither smart nor dumb). # # Define CURL_CONFIG to the path to a curl-config binary other than the -# default 'curl-config'. +# default 'curl-config'. If CURL_CONFIG is unset or points to a binary that +# is not found, defaults to the CURLDIR behavior. # # Define CURL_STATIC to statically link libcurl. Only applies if # CURL_CONFIG is used. # # Define CURLDIR=/foo/bar if your curl header and library files are in -# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, -# but is less robust. +# /foo/bar/include and /foo/bar/lib directories. This overrides +# CURL_CONFIG, but is less robust. If not set, and CURL_CONFIG is not set, +# uses -lcurl with no additional library detection (other than +# NEEDS_*_WITH_CURL). # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports (dumb). @@ -1127,9 +1130,27 @@ ifdef NO_CURL REMOTE_CURL_NAMES = else ifdef CURLDIR - # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. - BASIC_CFLAGS += -I$(CURLDIR)/include - CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + CURL_LIBCURL = + else + CURL_CONFIG = curl-config + ifeq $(CURL_CONFIG) + CURL_LIBCURL = + else + CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs) + endif + endif + + ifeq $(CURL_LIBCURL) + ifdef CURL_STATIC +$(error CURL_STATIC must be used with CURL_CONFIG) + endif + ifdef CURLDIR + # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. + BASIC_CFLAGS += -I$(CURLDIR)/include + CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + else + CURL_LIBCURL = -lcurl + endif ifdef NEEDS_SSL_WITH_CURL CURL_LIBCURL += -lssl ifdef NEEDS_CRYPTO_WITH_SSL @@ -1140,17 +1161,11 @@ else CURL_LIBCURL += -lidn endif else - CURL_CONFIG ?= curl-config BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) ifdef CURL_STATIC CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs) ifeq $(CURL_LIBCURL) - $(error libcurl not detected or not compiled with static support) - endif - else - CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) - ifeq $(CURL_LIBCURL) - $(error libcurl not detected; try setting CURLDIR) +$(error libcurl not detected or not compiled with static support) endif endif endif -- 1.9.1.423.g4596e3a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Makefile: do not depend on curl-config
On Wed, Apr 30, 2014 at 8:27 AM, Junio C Hamano gits...@pobox.com wrote: How old/battle tested is this change? My inclination at this point is to revert the merge of Dave's series from 2.0 (yes, I know we have been looking at fixing it and I _think_ the issue of unpleasant error message you reported can be fixed, but at this rate we would not know if we eradicated all the issues for platforms and distros with confidence by the final), kick it back to 'next'/'pu', and queue this change also in 'next'/'pu' and cook them so that we can merge them early in the 2.1 cycle. This is fine by me FWIW. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] jk/version-string and google code
On Fri, Aug 10, 2012 at 8:37 AM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: Ugh, the jk/version-string topic breaks fetching from Google Code. With my patch, the client unconditionally sends an agent=foo capability, but the server does not like seeing the unknown capability and ends the connection (I'm guessing with some kind of internal exception, since it spews Internal server error over the protocol channel). I asked the folks who run code.google.com and they are indeed seeing something like these in their logs: Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised. FWIW, this error comes from Dulwich: https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196 So any servers running Dulwich would be affected by this...though I'm not aware of any large-scale Dulwich installations other than Google Code. So please consider your conjecture confirmed, and thanks for a prompt fix. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git + SFC Status Update
On Tue, Apr 14, 2015 at 4:54 PM, Jeff King p...@peff.net wrote: On Tue, Apr 14, 2015 at 12:30:23PM -0700, Junio C Hamano wrote: If I recall correctly, Scott said onstage that some/all of the conference proceeds would be going directly into this fund. So this might need to be revised upward by 50-100% sometime soon :) I think you misheard it. The above is money earmarked for Git at SFC; the admission for GitMerge were going to SFC general fund without earmarked for us, IIUC. Yeah, that's my understanding as well. Ah, thanks for the clarification. -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: Git + SFC Status Update
On Mon, Apr 13, 2015 at 7:56 AM, Jeff King p...@peff.net wrote: # Money: How much do we have? - $19,059.25 (USD) // Disclaimer: this is not necessarily up-to-the-minute, as // SFC's reports to us sometimes lag a bit. And also because // I am fairly inexperienced using the `ledger` program, so // it's possible I've misinterpreted the results. However, we // shouldn't have any serious outstanding expenses, so this // is close to correct. If I recall correctly, Scott said onstage that some/all of the conference proceeds would be going directly into this fund. So this might need to be revised upward by 50-100% sometime soon :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] pack-protocol.txt: Be more precise about pusher-key relationship
The use of must (albeit not in all caps) suggests that this is actually a requirement of the protocol. As no implementation exists that actually does this verification, this is misleading at best. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/technical/pack-protocol.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index de3c72c..f37dcf1 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -564,7 +564,8 @@ Currently, the following header fields are defined: The GPG signature lines are a detached signature for the contents recorded in the push certificate before the signature block begins. The detached signature is used to certify that the commands were -given by the pusher, who must be the signer. +given by the pusher, which verifier code SHOULD enforce is a valid User +ID associated with the signer. Report Status - -- 2.4.3.573.g4eafbef -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] pack-protocol.txt: Mark pushee field as optional
send-pack.c omits this field when args-url is null or empty. Fix the protocol specification to match reality. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/technical/pack-protocol.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index f37dcf1..98e512d 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -495,7 +495,7 @@ references. push-cert = PKT-LINE(push-cert NUL capability-list LF) PKT-LINE(certificate version 0.1 LF) PKT-LINE(pusher SP push-cert-ident LF) - PKT-LINE(pushee SP url LF) + [PKT-LINE(pushee SP url LF)] PKT-LINE(nonce SP nonce LF) PKT-LINE(LF) *PKT-LINE(command LF) @@ -554,7 +554,8 @@ Currently, the following header fields are defined: `pushee` url:: The repository URL (anonymized, if the URL contains authentication material) the user who ran `git push` - intended to push into. + intended to push into. This field is optional, and included at + the client's discretion. `nonce` nonce:: The 'nonce' string the receiving repository asked the -- 2.4.3.573.g4eafbef -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies
We want to fix such inaccuracies, but there are a lot and there is no guarantee at any particular point in time that this document will be error-free. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/technical/pack-protocol.txt | 11 +++ 1 file changed, 11 insertions(+) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 4064fc7..66d2d95 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -14,6 +14,17 @@ data. The protocol functions to have a server tell a client what is currently on the server, then for the two to negotiate the smallest amount of data to send in order to fully update one or the other. +Notes to Implementors +- + +WARNING: This document is a work in progress. Some of the protocol +specifications below may be incomplete or inaccurate. When in doubt, +refer to the C code. + +One particular example is that many of the LFs referenced in the +specifications are optional, but may (yet) not be marked as such. If not +explicitly marked one way or the other, double-check with the C code. + Transports -- There are three transports over which the packfile protocol is -- 2.4.3.573.g4eafbef -- 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/7] pack-protocol.txt: Mark LF in command-list as optional
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/technical/pack-protocol.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 66d2d95..1386840 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -481,7 +481,7 @@ references. shallow = PKT-LINE(shallow SP obj-id LF) command-list = PKT-LINE(command NUL capability-list LF) - *PKT-LINE(command LF) + *PKT-LINE(command LF?) flush-pkt command = create / delete / update -- 2.4.3.573.g4eafbef -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] Clarify signed push protocol documentation
The signed push protocol documentation did not really match the reality of what send-pack.c and receive-pack.c do, much to my chagrin as I attempted to implement this protocol in JGit. This series covers most of the issues I found on a first pass. Dave Borowitz (7): pack-protocol.txt: Add warning about protocol inaccuracies pack-protocol.txt: Mark LF in command-list as optional pack-protocol.txt: Mark all LFs in push-cert as required pack-protocol.txt: Elaborate on pusher identity pack-protocol.txt: Be more precise about pusher-key relationship pack-protocol.txt: Mark pushee field as optional send-pack.c: Die if the nonce is empty Documentation/technical/pack-protocol.txt | 38 +-- send-pack.c | 2 ++ 2 files changed, 33 insertions(+), 7 deletions(-) -- 2.4.3.573.g4eafbef -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] pack-protocol.txt: Mark LF in command-list as optional
On Wed, Jul 1, 2015 at 11:21 AM, Stefan Beller sbel...@google.com wrote: I think the problem with this part of the documentation is its ambiguity on what exactly we want to document. The sender SHOULD put an LF, while the receiver MUST NOT assume the LF is there always, so I guess it's best to mark it optional from a receivers point of view. To be clear, this patch is a partial fix to one particular spec in the file. See 1/7 for the general warning not to trust these. Auditing the file completely was not the goal of this series. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] send-pack.c: Die if the nonce is empty
pack-protocol.txt does not list the nonce as optional. Fortunately, it should be impossible to not have a nonce by this point in the code, as the caller should have died on line 380 prior to generating a push certificate with an empty nonce. Nonetheless, having this explicit error handling in the code reduces confusion for implementors trying to understand the signed push protocol by looking at the reference implementation. Signed-off-by: Dave Borowitz dborow...@google.com --- send-pack.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/send-pack.c b/send-pack.c index 2a64fec..77e2131 100644 --- a/send-pack.c +++ b/send-pack.c @@ -254,6 +254,8 @@ static int generate_push_cert(struct strbuf *req_buf, } if (push_cert_nonce[0]) strbuf_addf(cert, nonce %s\n, push_cert_nonce); + else + die(_(server did not provide a nonce)); strbuf_addstr(cert, \n); for (ref = remote_refs; ref; ref = ref-next) { -- 2.4.3.573.g4eafbef -- 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 1/7] pack-protocol.txt: Add warning about protocol inaccuracies
On Wed, Jul 1, 2015 at 12:39 PM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Dave Borowitz wrote: --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -14,6 +14,17 @@ data. The protocol functions to have a server tell a client what is currently on the server, then for the two to negotiate the smallest amount of data to send in order to fully update one or the other. +Notes to Implementors +- + +WARNING: This document is a work in progress. Some of the protocol +specifications below may be incomplete or inaccurate. When in doubt, +refer to the C code. If we include this warning, can it also say to contact git@vger.kernel.org for inaccuracies? Otherwise it is easy to misread as Some of this document may be inaccurate, and we're working on fixing that. Don't bug me --- I already know about the problems --- just be patient! I would rather that it would say something like Caveat -- This document contains some inaccuracies. Do not forget to also check against the C code, and please contact git@vger.kernel.org if you run into any unclear or inaccurate passages in this spec. Agreed with your rationale and suggested wording, thanks. + +One particular example is that many of the LFs referenced in the +specifications are optional, but may (yet) not be marked as such. If not +explicitly marked one way or the other, double-check with the C code. The 'Reference Discovery' section says: Server SHOULD terminate each non-flush line using LF (\n) terminator; client MUST NOT complain if there is no terminator. Unfortunately that's not explained in a section with broader scope. Isn't that actually the rule everywhere except for in push certs? It's certainly the rule in more places. I personally have partially audited send-pack.c, but there are many places that are not yet audited. I don't feel comfortable making a broader claim without having done so, and I do not have the time to do so at the moment. The documentation will be easier to use if it says so instead of asking implementers to check the source of all implementations (since interoperability with only one isn't enough). Unfortunately, the trust I have in this document at this point is less than zero. A handful of spot fixes, while useful, does not serve as any sort of assurance that we've gotten all or even most of the problems. Until such time as this document is actually reliable, implementors must check the source of all implementations if they want to be accurate. That sucks for implementors (believe me, I know), but it's the truth. 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: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional
On Wed, Jul 1, 2015 at 12:07 PM, Junio C Hamano gits...@pobox.com wrote: Answering myself, the most trivial example is git send-pack ;-) It passes args that has a NULL in the .url field. Well, the example I have involves an actual git push command. The fact that .url is NULL in that case may be a separate bug. -- 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 6/7] pack-protocol.txt: Mark pushee field as optional
On Wed, Jul 1, 2015 at 11:56 AM, Junio C Hamano gits...@pobox.com wrote: Do some clients omit this in the real world? I just sent you privately a trace where this happens using a recent git client. With that in the wild, I think our server is going to have to handle these even if there is a bug and it is fixed promptly. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Wed, Jul 1, 2015 at 1:00 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/technical/pack-protocol.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 1386840..2d8b1a1 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -534,6 +534,9 @@ A push certificate begins with a set of header lines. After the header and an empty line, the protocol commands follow, one per line. +Note that (unlike other portions of the protocol), all LFs in the +`push-cert` specification above MUST be present. + Currently, the following header fields are defined: `pusher` ident:: I am moderately negative about this; wouldn't it make the end result cleaner to fix the implementation? I'm not sure I understand your suggestion. Are you saying, you would prefer to make LFs optional in the push cert, for consistency with LFs being optional elsewhere? C git servers in the wild already require LFs when extracting the nonce value from the certificate (see find_header). If we make the LFs optional, then a conforming client may not send LFs, which will cause nonce verification to fail when pushing to an unfixed server. That is why I think we are stuck with this. (Also, this is probably not insurmountable, but the cert processing code in receive-pack.c would have to be substantially rewritten if we loosened this requirement. Currently it concatenates the cert contents without pkt-line framing into a buffer, and searches around for \n and \n\n. If LF is optional, then with that approach you might end up with a section of that buffer like: nonce 1234-abcd deadbeefdeadbeefdeadbeefdeadbeefdeadbeef refs/heads/master where it is impossible to distinguish between the end of the nonce and the start of the first command.) -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz dborow...@google.com wrote: The alternatives for someone writing a parser are: a. Store the original pkt-line framing. Or obviously, a2. Frame in some other way, e.g. JSON array of strings (complete straw man, not seriously proposing this). -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 2:06 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Mon, Jul 6, 2015 at 1:34 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Another way of looking at the problem with my assumptions is, I was assuming pkt-line framing was the same thing as pkt-line header. You seem to be saying the definition of pkt-line framing is header, and optional trailing newline. Yes. I thought that was what Server SHOULD terminate with LF; client MUST NOT require it in the existing text meant. Unfortunately, the existing text is littered with examples of PKT-LINE(foo SP bar LF). If we assume PKT-LINE(...) means apply pkt-line framing to the [...], then this strongly implies that pkt-line framing does _not_ include the trailing LF. (Or the logical but bizarre alternative reading that such an example might have _two_ trailing LFs :) Yes, But I never viewed PKT-LINE() as an element that strictly defines the grammar of the packet protocol ;-) By clarifying that sender SHOULD terminate with LF, receiver MUST NOT require it is the rule (and fixing the existing implementations at places where they violate the MUST NOT part, which I think are very small number of places), I think we can drop these LF (or LF? for that matter) from all of the PKT-LINE() in the construction in the pack-protocol.txt, which would be a very good thing to do. Completely agree, and that is what I meant when I said The additional upside [to explicitly defining pkt-line framing in this way] is that we could then potentially remove all or almost all LFs from this document. The example in your sentence will become PKT-LINE(foo SP bar) and the there may be an LF at the end would only be at one place, as a part of the definition of PKT-LINE(). -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 12:57 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: The server can munge pkt-lines and reinsert LFs, but it _must_ have some way of reconstructing the payload that the client signed in order to verify the signature. If we just naively insert LFs where missing, we lose the ability to verify the signature. I still do not understand this part. There is no way to naively insert, is there? You have an array of lines (each of which you have already stripped its terminating LF at its end). How else other than adding one LF at the end of each element do you reconstruct the original multi-line string the client signed? Are there other ways that makes the result ambiguous?? I think I understand the confusion now. I think you and I are working from different assumptions about the client behavior. My assumption was: the intended behavior for the client is to sign the exact payload that was sent over the wire, minus pkt-line headers. For example, under my assumption, if the client sent: 0008foo\n 0007bar 0008baz\n then this indicates the client signed: foo\nbarbaz\n Under this assumption, naively inserting LF means inserting an LF after bar. Thus the server would record the following in a persistent store: foo\nbar\nbaz\n If we only store this string, and do not remember the fact that the client originally omitted one of those LFs, then when we go back to verify that signature later, it will fail. That was my assumption. Your assumption, IIUC, is that the payload the client signed MUST have contained LFs in between each line. When framing the content for the wire, the client MUST send one logical line, which has no trailing LF, per pkt-line, and furthermore the pkt-line content MAY contain an additional trailing LF. Under your assumption, the server always has enough information to reconstruct the original signed payload. The problem with the documentation, then, is that the documentation does not say anything to indicate that the signed payload is anything other than what is on the wire. So maybe this series should include an explicit description of the singed payload outside of the context of a push. Then, in the push section, we can describe the set of transformations that the client MUST perform (splitting on LF; adding pkt-line headers) and MAY perform (adding LFs). If we say the payload the client signs MUST have LFs only in certain places, then that gives the server enough information to reconstruct the payload and verify the signature. But if we say the signed payload MUST have LFs and the wire payload MAY have LFs, then now we have two completely different formats, only one of which is documented. I thought that was what I was saying. The wire protocol sends the contents of each line (both what is signed and the signature) on a separate packet. When I say contents of a line, I do not include the terminating LF as part of the line (iow, LF is not even optional; the terminating LF is not considered as part of the contents of a line). It becomes irrelevant that a pkt-line may or may not have a trailing optional LF. If there is LF at the end of a packet between push-cert and push-cert-end packets, that LF by definition cannot be part of the contents of a line in a certificate. It is just a pkt-line framing artifact you can and should remove if you are doing a split to array, join with LF implementation to recover the original string. And that is very much consistent with the way we send other things with pkt-line protocol. Each packet up to the first flush is expected to have object name and refname as ref advertisement. The pkt-line framing may or may not add a trailing LF, but LF is not part of refname. It is not even part of the payload; merely an artifact of pkt-line framing. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 1:30 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: I think I understand the confusion now. I think you and I are working from different assumptions about the client behavior. I agree that we now both understand where we come from ;-) And sorry for not being clear when I did the push-cert originally in the documentation. As I already said, packets between push-cert and push-cert-end are contents of individual lines of the GPG signed push certificate This sentence makes sense to me now, but only because we now agree that contents does not include the LF. Different people may have different initial assumptions about whether the contents of a line includes the trailing newline or not. was the design meant from day one, and a85b377d (push: the beginning of git push --signed, 2014-09-12) could have made it clearer. The problem with the documentation, then, is that the documentation does not say anything to indicate that the signed payload is anything other than what is on the wire. Yeah, that was untold assumption, as I considered what is on the wire to include pkt-line framing when I wrote a85b377d (push: the beginning of git push --signed, 2014-09-12). So maybe this series should include an explicit description of the singed payload outside of the context of a push. Then, in the push section, we can describe the set of transformations that the client MUST perform (splitting on LF; adding pkt-line headers) and MAY perform (adding LFs). Yes, and the latter is not limited to push-cert but anything sent on pkt-line. That incidentally is the only point I deeply care about. I just want to minimize the protocol is this way in general, but only for this one you must do it differently. Understood, and I'm glad we have finally come to an arrangement that is both consistent and easy to implement on the server side. One example of only for this one you must do it differently is another caveat for protocol implementors for the sending side (again not limited to push cert). That existing implementations of the receivers treat an empty packet (i.e. 0004) or 0005\n ;) as if it is the same as a flush packet (i.e. ), so even if the sending side chooses to ignore the SHOULD terminate each non-flush line using LF, it is strongly advised not to do so when it wants to send an empty payload. This needs to be documented. The receiving end SHOULD NOT treat 0004 the same way as . I think that must be documented and implementations (including our own) should be fixed. Agreed. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 1:11 PM, Dave Borowitz dborow...@google.com wrote: On Mon, Jul 6, 2015 at 12:57 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: The server can munge pkt-lines and reinsert LFs, but it _must_ have some way of reconstructing the payload that the client signed in order to verify the signature. If we just naively insert LFs where missing, we lose the ability to verify the signature. I still do not understand this part. There is no way to naively insert, is there? You have an array of lines (each of which you have already stripped its terminating LF at its end). How else other than adding one LF at the end of each element do you reconstruct the original multi-line string the client signed? Are there other ways that makes the result ambiguous?? I think I understand the confusion now. I think you and I are working from different assumptions about the client behavior. My assumption was: the intended behavior for the client is to sign the exact payload that was sent over the wire, minus pkt-line headers. For example, under my assumption, if the client sent: 0008foo\n 0007bar 0008baz\n then this indicates the client signed: foo\nbarbaz\n Under this assumption, naively inserting LF means inserting an LF after bar. Thus the server would record the following in a persistent store: foo\nbar\nbaz\n If we only store this string, and do not remember the fact that the client originally omitted one of those LFs, then when we go back to verify that signature later, it will fail. That was my assumption. Your assumption, IIUC, is that the payload the client signed MUST have contained LFs in between each line. When framing the content for the wire, the client MUST send one logical line, which has no trailing LF, per pkt-line, and furthermore the pkt-line content MAY contain an additional trailing LF. Under your assumption, the server always has enough information to reconstruct the original signed payload. The problem with the documentation, then, is that the documentation does not say anything to indicate that the signed payload is anything other than what is on the wire. Another way of looking at the problem with my assumptions is, I was assuming pkt-line framing was the same thing as pkt-line header. You seem to be saying the definition of pkt-line framing is header, and optional trailing newline. A quick scan of pack-protocol.txt did not turn up anything one way or the other on this issue, so perhaps we could make it more explicit. The additional upside here is that we could then potentially remove all or almost all LFs from this document. So maybe this series should include an explicit description of the singed payload outside of the context of a push. Then, in the push section, we can describe the set of transformations that the client MUST perform (splitting on LF; adding pkt-line headers) and MAY perform (adding LFs). If we say the payload the client signs MUST have LFs only in certain places, then that gives the server enough information to reconstruct the payload and verify the signature. But if we say the signed payload MUST have LFs and the wire payload MAY have LFs, then now we have two completely different formats, only one of which is documented. I thought that was what I was saying. The wire protocol sends the contents of each line (both what is signed and the signature) on a separate packet. When I say contents of a line, I do not include the terminating LF as part of the line (iow, LF is not even optional; the terminating LF is not considered as part of the contents of a line). It becomes irrelevant that a pkt-line may or may not have a trailing optional LF. If there is LF at the end of a packet between push-cert and push-cert-end packets, that LF by definition cannot be part of the contents of a line in a certificate. It is just a pkt-line framing artifact you can and should remove if you are doing a split to array, join with LF implementation to recover the original string. And that is very much consistent with the way we send other things with pkt-line protocol. Each packet up to the first flush is expected to have object name and refname as ref advertisement. The pkt-line framing may or may not add a trailing LF, but LF is not part of refname. It is not even part of the payload; merely an artifact of pkt-line framing. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 1:34 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Another way of looking at the problem with my assumptions is, I was assuming pkt-line framing was the same thing as pkt-line header. You seem to be saying the definition of pkt-line framing is header, and optional trailing newline. Yes. I thought that was what Server SHOULD terminate with LF; client MUST NOT require it in the existing text meant. Unfortunately, the existing text is littered with examples of PKT-LINE(foo SP bar LF). If we assume PKT-LINE(...) means apply pkt-line framing to the [...], then this strongly implies that pkt-line framing does _not_ include the trailing LF. (Or the logical but bizarre alternative reading that such an example might have _two_ trailing LFs :) Ah, that reminds me of one thing I already said elsewhere. We need to correct the above with s/Server/Sender/; s/Client/Receiver/; I think. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 12:28 PM, Junio C Hamano gits...@pobox.com wrote: Shawn Pearce spea...@spearce.org writes: push cert format is like commit or tag format. You need those LFs. We can't just go declare them optional because of the way pkt-line read function is implemented in git-core. As I said, I view each of the packets between push-cert and push-cert-end packets representing the meat of each line in the cert. The sending end takes a cert as a long multi-line string, splits them into an array, each of whose element represents a line in it (iow certlines = certstring.split('\n')), and sends them packetised. Right now the sending end sends newlines. The receiver receives a sequence of packets, notices push-cert packet, collects packets until it sees push-cert-end packet and treats them as elements of this array. pkt-line deframing process would have to strip optional LFs to reconstruct the original array the sender had (i.e. the above certlines array). The problem is the signature. Today, the client computes the signature over the payload it actually sends (minus pkt-line headers) The server can munge pkt-lines and reinsert LFs, but it _must_ have some way of reconstructing the payload that the client signed in order to verify the signature. If we just naively insert LFs where missing, we lose the ability to verify the signature. If we say the payload the client signs MUST have LFs only in certain places, then that gives the server enough information to reconstruct the payload and verify the signature. But if we say the signed payload MUST have LFs and the wire payload MAY have LFs, then now we have two completely different formats, only one of which is documented. The receiver needs to join the array with LF to recover the long multi-line string once it received the array. But this LF does not have anything to do with the optional trailing LF in pkt-line. If you sent the original certlines array via different RPC mechanism, you need to join them together with your own LF to reconstruct the multi-line string. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: I am moderately negative about this; wouldn't it make the end result cleaner to fix the implementation? I'm not sure I understand your suggestion. Are you saying, you would prefer to make LFs optional in the push cert, for consistency with LFs being optional elsewhere? Absolutely. It is not make it optional, but even though it is optional, the receiver has not been following the spec, and it is not too late to fix it. The earliest these documentation updates can hit the public is 2.6; by that time I'd expect the deployed receivers would be fixed with 2.5.1 and 2.4.7 maintenance releases. If some third-party reimplemented their client not to terminate with LF, they wouldn't be working correctly with the deployed servers right now *anyway*. And with the more lenient receive-pack in 2.5.1 or 2.4.7, they will start working. And we will not change our client to drop LF termination. So overall I do not see that it is too much a price to pay for consistency across the protocol. Ok, I understand your proposal now, thank you. I will drop this documentation patch from this series, and abandon https://git.eclipse.org/r/51071 in JGit. I am not volunteering to rewrite push cert handling in git-core though ;) If LF is optional, then with that approach you might end up with a section of that buffer like: I think I touched on this in my previous message. You cannot send an empty line anywhere, and this is not limited to push-cert section of the protocol. Strictly speaking, the wire level allows it, but I do not think the deployed client APIs easily lets you deal with it. So you must follow the SHOULD terminate with LF for an empty line, even when you choose to ignore the SHOULD in most other places. I do not think it is such a big loss, as long as it is properly documented. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz dborow...@google.com wrote: b. Write a parser in some other clever way, e.g. parsing the entire cert in reverse might work. ...as long as is illegal in nonce and pushee values, which it may be but is not explicitly documented. I still have no desire to write such a parser. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 11:27 AM, Dave Borowitz dborow...@google.com wrote: On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz dborow...@google.com wrote: b. Write a parser in some other clever way, e.g. parsing the entire cert in reverse might work. ...as long as is illegal in nonce and pushee values, which it may be but is not explicitly documented. I still have no desire to write such a parser. TBQH at this point I would prefer, as a protocol implementor, to restore the original proposal of this patch, which is to require \n in push certificates. -- 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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required
On Mon, Jul 6, 2015 at 10:46 AM, Dave Borowitz dborow...@google.com wrote: On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: I am moderately negative about this; wouldn't it make the end result cleaner to fix the implementation? I'm not sure I understand your suggestion. Are you saying, you would prefer to make LFs optional in the push cert, for consistency with LFs being optional elsewhere? Absolutely. It is not make it optional, but even though it is optional, the receiver has not been following the spec, and it is not too late to fix it. The earliest these documentation updates can hit the public is 2.6; by that time I'd expect the deployed receivers would be fixed with 2.5.1 and 2.4.7 maintenance releases. If some third-party reimplemented their client not to terminate with LF, they wouldn't be working correctly with the deployed servers right now *anyway*. And with the more lenient receive-pack in 2.5.1 or 2.4.7, they will start working. And we will not change our client to drop LF termination. So overall I do not see that it is too much a price to pay for consistency across the protocol. Ok, I understand your proposal now, thank you. I will drop this documentation patch from this series, and abandon https://git.eclipse.org/r/51071 in JGit. I am not volunteering to rewrite push cert handling in git-core though ;) Unfortunately, optional LFs still make the stored certs for later auditing and parsing a bit illegible. This is one way in which push certs are fundamentally different from the rest of the wire protocol, which is not intended to be persisted. The corner case I pointed out before where nonce runs into commands is not the only one. Consider the following cert fragment: 001fpushee git://localhost/repo 0029nonce 1433954361-bde756572d665bba81d8 A naive cert storage/auditing implementation would store the raw payload that needs to be verified, without the pkt-line framing. In this case: pushee git://localhost/repononce 1433954361-bde756572d665bba81d8 A naive parser that wants to find the pushee would look for pushee urlish, which would be wrong in this case. (To say nothing of the fact that pushee might actually be -0700pushee.) The alternatives for someone writing a parser are: a. Store the original pkt-line framing. b. Write a parser in some other clever way, e.g. parsing the entire cert in reverse might work. Neither of these is very satisfying, and both reduce human legibility of the stored payload. If LF is optional, then with that approach you might end up with a section of that buffer like: I think I touched on this in my previous message. You cannot send an empty line anywhere, and this is not limited to push-cert section of the protocol. Strictly speaking, the wire level allows it, but I do not think the deployed client APIs easily lets you deal with it. So you must follow the SHOULD terminate with LF for an empty line, even when you choose to ignore the SHOULD in most other places. I do not think it is such a big loss, as long as it is properly documented. -- 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 1/7] Documentation/git-push.txt: Document when --signed may fail
On Fri, Aug 14, 2015 at 7:10 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Like --atomic, --signed will fail if the server does not advertise the necessary capability. In addition, it requires gpg on the client side. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-push.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 135d810..f8b8b8b 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -137,7 +137,9 @@ already exists on the remote side. GPG-sign the push request to update refs on the receiving side, to allow it to be checked by the hooks and/or be logged. See linkgit:git-receive-pack[1] for the details - on the receiving end. + on the receiving end. If the `gpg` executable is not available, + or if the server does not support signed pushes, the push will + fail. Looks good. I am wondering if another mode of failure is worth mentioning: `gpg` available, you have _some_ keys, but signingkey configured does not match any of the keys. Note that I said am wondering, which is very different from I think we should also describe. I think we don't need to go down the path of enumerating all possible ways the operation can fail. There is probably a reasonably concise way to include more possibilities. How about: If the attempt to sign with `gpg` fails, or if the server does not support signed pushes, the push will fail. This should cover gpg not being found, gpg being fatally misconfigured, crazy unexpected pipe closures, etc. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/9] builtin/send-pack.c: Use option parsing API
The old option parsing code in this plumbing command predates this API, so option parsing was done more manually. Using the new API brings send-pack more in line with push, and accepts new variants like --no-* for negating options. Signed-off-by: Dave Borowitz dborow...@google.com --- builtin/send-pack.c | 163 +++- 1 file changed, 59 insertions(+), 104 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 23b2962..5f2c744 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -12,10 +12,15 @@ #include version.h #include sha1-array.h #include gpg-interface.h +#include gettext.h -static const char send_pack_usage[] = -git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...]\n - --all and explicit ref specification are mutually exclusive.; +static const char * const send_pack_usage[] = { + N_(git send-pack [--all | --mirror] [--dry-run] [--force] + [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] + [host:]directory [ref...]\n + --all and explicit ref specification are mutually exclusive.), + NULL, +}; static struct send_pack_args args; @@ -107,116 +112,66 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int ret; int helper_status = 0; int send_all = 0; + int verbose = 0; const char *receivepack = git-receive-pack; + unsigned dry_run = 0; + unsigned send_mirror = 0; + unsigned force_update = 0; + unsigned quiet = 0; + unsigned push_cert = 0; + unsigned use_thin_pack = 0; + unsigned atomic = 0; + unsigned stateless_rpc = 0; int flags; unsigned int reject_reasons; int progress = -1; int from_stdin = 0; struct push_cas_option cas = {0}; - git_config(git_gpg_config, NULL); + struct option options[] = { + OPT__VERBOSITY(verbose), + OPT_STRING(0, receive-pack, receivepack, receive-pack, N_(receive pack program)), + OPT_STRING(0, exec, receivepack, receive-pack, N_(receive pack program)), + OPT_STRING(0, remote, remote_name, remote, N_(remote name)), + OPT_BOOL(0, all, send_all, N_(push all refs)), + OPT_BOOL('n' , dry-run, dry_run, N_(dry run)), + OPT_BOOL(0, mirror, send_mirror, N_(mirror all refs)), + OPT_BOOL('f', force, force_update, N_(force updates)), + OPT_BOOL(0, signed, push_cert, N_(GPG sign the push)), + OPT_BOOL(0, progress, progress, N_(force progress reporting)), + OPT_BOOL(0, thin, use_thin_pack, N_(use thin pack)), + OPT_BOOL(0, atomic, atomic, N_(request atomic transaction on remote side)), + OPT_BOOL(0, stateless-rpc, stateless_rpc, N_(use stateless RPC protocol)), + OPT_BOOL(0, stdin, from_stdin, N_(read refs from stdin)), + OPT_BOOL(0, helper-status, helper_status, N_(print status from remote helper)), + { OPTION_CALLBACK, + 0, CAS_OPT_NAME, cas, N_(refname:expect), + N_(require old value of ref to be at this value), + PARSE_OPT_OPTARG, parseopt_push_cas_option }, + OPT_END() + }; - argv++; - for (i = 1; i argc; i++, argv++) { - const char *arg = *argv; - - if (*arg == '-') { - if (starts_with(arg, --receive-pack=)) { - receivepack = arg + 15; - continue; - } - if (starts_with(arg, --exec=)) { - receivepack = arg + 7; - continue; - } - if (starts_with(arg, --remote=)) { - remote_name = arg + 9; - continue; - } - if (!strcmp(arg, --all)) { - send_all = 1; - continue; - } - if (!strcmp(arg, --dry-run)) { - args.dry_run = 1; - continue; - } - if (!strcmp(arg, --mirror)) { - args.send_mirror = 1; - continue; - } - if (!strcmp(arg, --force)) { - args.force_update = 1; - continue; - } - if (!strcmp(arg, --quiet)) { - args.quiet = 1; - continue
[PATCH v2 5/9] transport: Remove git_transport_options.push_cert
This field was set in transport_set_option, but never read in the push code. The push code basically ignores the smart_options field entirely, and derives its options from the flags arguments to the push* callbacks. Note that in git_transport_push there are already several args set from flags that have no corresponding field in git_transport_options; after this change, push_cert is just like those. Signed-off-by: Dave Borowitz dborow...@google.com --- transport.c | 3 --- transport.h | 1 - 2 files changed, 4 deletions(-) diff --git a/transport.c b/transport.c index 40692f8..3dd6e30 100644 --- a/transport.c +++ b/transport.c @@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts, die(transport: invalid depth option '%s', value); } return 0; - } else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) { - opts-push_cert = !!value; - return 0; } return 1; } diff --git a/transport.h b/transport.h index 18d2cf8..79190df 100644 --- a/transport.h +++ b/transport.h @@ -12,7 +12,6 @@ struct git_transport_options { unsigned check_self_contained_and_connected : 1; unsigned self_contained_and_connected : 1; unsigned update_shallow : 1; - unsigned push_cert : 1; int depth; const char *uploadpack; const char *receivepack; -- 2.5.0.276.gf5e568e -- 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 v2 6/9] config.c: Expose git_parse_maybe_bool
Signed-off-by: Dave Borowitz dborow...@google.com --- cache.h | 1 + config.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 6bb7119..95d9594 100644 --- a/cache.h +++ b/cache.h @@ -1392,6 +1392,7 @@ extern int git_config_with_options(config_fn_t fn, void *, int respect_includes); extern int git_config_early(config_fn_t fn, void *, const char *repo_config); extern int git_parse_ulong(const char *, unsigned long *); +extern int git_parse_maybe_bool(const char *); extern int git_config_int(const char *, const char *); extern int64_t git_config_int64(const char *, const char *); extern unsigned long git_config_ulong(const char *, const char *); diff --git a/config.c b/config.c index 9fd275f..e5d7959 100644 --- a/config.c +++ b/config.c @@ -618,7 +618,7 @@ unsigned long git_config_ulong(const char *name, const char *value) return ret; } -static int git_config_maybe_bool_text(const char *name, const char *value) +int git_parse_maybe_bool(const char *value) { if (!value) return 1; @@ -637,7 +637,7 @@ static int git_config_maybe_bool_text(const char *name, const char *value) int git_config_maybe_bool(const char *name, const char *value) { - int v = git_config_maybe_bool_text(name, value); + int v = git_parse_maybe_bool(value); if (0 = v) return v; if (git_parse_int(value, v)) @@ -647,7 +647,7 @@ int git_config_maybe_bool(const char *name, const char *value) int git_config_bool_or_int(const char *name, const char *value, int *is_bool) { - int v = git_config_maybe_bool_text(name, value); + int v = git_parse_maybe_bool(value); if (0 = v) { *is_bool = 1; return v; -- 2.5.0.276.gf5e568e -- 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 v2 3/9] Documentation/git-send-pack.txt: Document --signed
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-send-pack.txt | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index 6a6..0a0a3fb 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -10,7 +10,8 @@ SYNOPSIS [verse] 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] - [--verbose] [--thin] [--atomic] [host:]directory [ref...] + [--verbose] [--thin] [--atomic] [--signed] + [host:]directory [ref...] DESCRIPTION --- @@ -68,6 +69,14 @@ be in a separate packet, and the list must end with a flush packet. fails to update then the entire push will fail without changing any refs. +--signed:: + GPG-sign the push request to update refs on the receiving + side, to allow it to be checked by the hooks and/or be + logged. See linkgit:git-receive-pack[1] for the details + on the receiving end. If the attempt to sign with `gpg` fails, + or if the server does not support signed pushes, the push will + fail. + host:: A remote host to house the repository. When this part is specified, 'git-receive-pack' is invoked via -- 2.5.0.276.gf5e568e -- 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 v2 9/9] Add a config option push.gpgSign for default signed pushes
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/config.txt | 8 builtin/push.c | 50 ++-- builtin/send-pack.c | 27 +- 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 016f6e9..4ba0e4b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2178,6 +2178,14 @@ push.followTags:: may override this configuration at time of push by specifying '--no-follow-tags'. +push.gpgSign:: + May be set to a boolean value, or the string 'if-asked'. A true + value causes all pushes to be GPG signed, as if '--signed' is + passed to linkgit:git-push[1]. The string 'if-asked' causes + pushes to be signed if the server supports it, as if + '--signed=if-asked' is passed to 'git push'. A false value may + override a value from a lower-priority config file. An explicit + command-line flag always overrides this config option. rebase.stat:: Whether to show a diffstat of what changed upstream since the last diff --git a/builtin/push.c b/builtin/push.c index 85a82cd..3bda430 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -472,6 +472,24 @@ static int option_parse_recurse_submodules(const struct option *opt, return 0; } +static void set_push_cert_flags(int *flags, int v) +{ + switch (v) { + case SEND_PACK_PUSH_CERT_NEVER: + *flags = ~(TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_ASKED); + break; + case SEND_PACK_PUSH_CERT_ALWAYS: + *flags |= TRANSPORT_PUSH_CERT_ALWAYS; + *flags = ~TRANSPORT_PUSH_CERT_IF_ASKED; + break; + case SEND_PACK_PUSH_CERT_IF_ASKED: + *flags |= TRANSPORT_PUSH_CERT_IF_ASKED; + *flags = ~TRANSPORT_PUSH_CERT_ALWAYS; + break; + } +} + + static int git_push_config(const char *k, const char *v, void *cb) { int *flags = cb; @@ -487,6 +505,23 @@ static int git_push_config(const char *k, const char *v, void *cb) else *flags = ~TRANSPORT_PUSH_FOLLOW_TAGS; return 0; + } else if (!strcmp(k, push.gpgsign)) { + const char *value; + if (!git_config_get_value(push.gpgsign, value)) { + switch (git_config_maybe_bool(push.gpgsign, value)) { + case 0: + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER); + break; + case 1: + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS); + break; + default: + if (value !strcasecmp(value, if-asked)) + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED); + else + return error(Invalid value for '%s', k); + } + } } return git_default_config(k, v, NULL); @@ -538,6 +573,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) packet_trace_identity(push); git_config(git_push_config, flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); + set_push_cert_flags(flags, push_cert); if (deleterefs (tags || (flags (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR die(_(--delete is incompatible with --all, --mirror and --tags)); @@ -552,20 +588,6 @@ int cmd_push(int argc, const char **argv, const char *prefix) set_refspecs(argv + 1, argc - 1, repo); } - switch (push_cert) { - case SEND_PACK_PUSH_CERT_NEVER: - flags = ~(TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_ASKED); - break; - case SEND_PACK_PUSH_CERT_ALWAYS: - flags |= TRANSPORT_PUSH_CERT_ALWAYS; - flags = ~TRANSPORT_PUSH_CERT_IF_ASKED; - break; - case SEND_PACK_PUSH_CERT_IF_ASKED: - flags |= TRANSPORT_PUSH_CERT_IF_ASKED; - flags = ~TRANSPORT_PUSH_CERT_ALWAYS; - break; - } - rc = do_push(repo, flags); if (rc == -1) usage_with_options(push_usage, options); diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 0ce3bc8..f6e5d64 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -97,6 +97,31 @@ static void print_helper_status(struct ref *ref) strbuf_release(buf); } +static int send_pack_config(const char *k, const char *v, void *cb) +{ + git_gpg_config(k, v, NULL); + + if (!strcmp(k, push.gpgsign)) { + const char *value; + if (!git_config_get_value
Re: [PATCH 6/7] Support signing pushes iff the server supports it
On Fri, Aug 14, 2015 at 7:22 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: diff --git a/send-pack.c b/send-pack.c index 2a64fec..6ae9f45 100644 --- a/send-pack.c +++ b/send-pack.c @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args, args-use_thin_pack = 0; if (server_supports(atomic)) atomic_supported = 1; - if (args-push_cert) { + if (args-push_cert == SEND_PACK_PUSH_CERT_ALWAYS) { int len; push_cert_nonce = server_feature_value(push-cert, len); @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args, reject_invalid_nonce(push_cert_nonce, len); push_cert_nonce = xmemdupz(push_cert_nonce, len); } + if (args-push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) { + int len; + + push_cert_nonce = server_feature_value(push-cert, len); + if (push_cert_nonce) { + reject_invalid_nonce(push_cert_nonce, len); + push_cert_nonce = xmemdupz(push_cert_nonce, len); + } else + warning(_(not sending a push certificate since the +receiving end does not support --signed +push)); + } I wonder if the bodies of these two if statements can be a bit better organized to avoid duplication (I suspect you have tried and you may already know that the above is the most readable version, but I haven't tried to do so myself, so...). Found a slightly less repetitious way. -- 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 v2 8/9] Support signing pushes iff the server supports it
Add a new flag --signed-if-possible to push and send-pack that sends a push certificate if and only if the server advertised a push cert nonce. If not, at least warn the user that their push may not be as secure as they thought. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-push.txt | 17 +--- Documentation/git-send-pack.txt | 16 +-- builtin/push.c | 20 ++- builtin/send-pack.c | 6 -- remote-curl.c | 16 ++- send-pack.c | 43 ++--- send-pack.h | 12 +++- transport-helper.c | 34 transport.c | 8 +++- transport.h | 5 +++-- 10 files changed, 128 insertions(+), 49 deletions(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index da0a98d..1495e34 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -11,7 +11,8 @@ SYNOPSIS [verse] 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=git-receive-pack] [--repo=repository] [-f | --force] [--prune] [-v | --verbose] - [-u | --set-upstream] [--signed] + [-u | --set-upstream] + [--[no-]signed|--sign=(true|false|if-asked)] [--force-with-lease[=refname[:expect]]] [--no-verify] [repository [refspec...]] @@ -132,14 +133,16 @@ already exists on the remote side. with configuration variable 'push.followTags'. For more information, see 'push.followTags' in linkgit:git-config[1]. - ---signed:: +--[no-]signed:: +--sign=(true|false|if-asked):: GPG-sign the push request to update refs on the receiving side, to allow it to be checked by the hooks and/or be - logged. See linkgit:git-receive-pack[1] for the details - on the receiving end. If the attempt to sign with `gpg` fails, - or if the server does not support signed pushes, the push will - fail. + logged. If `false` or `--no-signed`, no signing will be + attempted. If `true` or `--signed`, the push will fail if the + server does not support signed pushes. If set to `if-asked`, + sign if and only if the server supports signed pushes. The push + will also fail if the actual call to `gpg --sign` fails. See + linkgit:git-receive-pack[1] for the details on the receiving end. --[no-]atomic:: Use an atomic transaction on the remote side if available. diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index 0a0a3fb..6aa91e8 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -10,7 +10,8 @@ SYNOPSIS [verse] 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] - [--verbose] [--thin] [--atomic] [--signed] + [--verbose] [--thin] [--atomic] + [--[no-]signed|--sign=(true|false|if-asked)] [host:]directory [ref...] DESCRIPTION @@ -69,13 +70,16 @@ be in a separate packet, and the list must end with a flush packet. fails to update then the entire push will fail without changing any refs. ---signed:: +--[no-]signed:: +--sign=(true|false|if-asked):: GPG-sign the push request to update refs on the receiving side, to allow it to be checked by the hooks and/or be - logged. See linkgit:git-receive-pack[1] for the details - on the receiving end. If the attempt to sign with `gpg` fails, - or if the server does not support signed pushes, the push will - fail. + logged. If `false` or `--no-signed`, no signing will be + attempted. If `true` or `--signed`, the push will fail if the + server does not support signed pushes. If set to `if-asked`, + sign if and only if the server supports signed pushes. The push + will also fail if the actual call to `gpg --sign` fails. See + linkgit:git-receive-pack[1] for the details on the receiving end. host:: A remote host to house the repository. When this diff --git a/builtin/push.c b/builtin/push.c index 57c138b..85a82cd 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -9,6 +9,7 @@ #include transport.h #include parse-options.h #include submodule.h +#include send-pack.h static const char * const push_usage[] = { N_(git push [options] [repository [refspec...]]), @@ -495,6 +496,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) { int flags = 0; int tags = 0; + int push_cert = -1; int rc; const char *repo = NULL;/* default repository */ struct option options[] = { @@ -526,7 +528,9 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BIT(0, no-verify, flags
[PATCH v2 4/9] gitremote-helpers.txt: Document pushcert option
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/gitremote-helpers.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 82e2d15..78e0b27 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -448,6 +448,9 @@ set by Git if the remote helper has the 'option' capability. 'option update-shallow {'true'|'false'}:: Allow to extend .git/shallow if the new refs require it. +'option pushcert {'true'|'false'}:: + GPG sign pushes. + SEE ALSO linkgit:git-remote[1] -- 2.5.0.276.gf5e568e -- 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 v2 1/9] Documentation/git-push.txt: Document when --signed may fail
Like --atomic, --signed will fail if the server does not advertise the necessary capability. In addition, it requires gpg on the client side. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-push.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 135d810..da0a98d 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -137,7 +137,9 @@ already exists on the remote side. GPG-sign the push request to update refs on the receiving side, to allow it to be checked by the hooks and/or be logged. See linkgit:git-receive-pack[1] for the details - on the receiving end. + on the receiving end. If the attempt to sign with `gpg` fails, + or if the server does not support signed pushes, the push will + fail. --[no-]atomic:: Use an atomic transaction on the remote side if available. -- 2.5.0.276.gf5e568e -- 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 v2 2/9] Documentation/git-send-pack.txt: Flow long synopsis line
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-send-pack.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index b5d09f7..6a6 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -9,7 +9,8 @@ git-send-pack - Push objects over Git protocol to another repository SYNOPSIS [verse] -'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...] +'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] + [--verbose] [--thin] [--atomic] [host:]directory [ref...] DESCRIPTION --- -- 2.5.0.276.gf5e568e -- 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 v2 0/9] Flags and config to sign pushes by default
Changes since v1: - Rebased on 44e02239 - Use options --[no-]signed|--signed=(yes|no|if-asked) - Support general yes/true/1/etc. option parsing. - Convert builtin/send-pack.c to use option parsing API for better code reuse. - Various cleanups as suggested by Junio. v1 can be found at: http://thread.gmane.org/gmane.comp.version-control.git/275881 Dave Borowitz (9): Documentation/git-push.txt: Document when --signed may fail Documentation/git-send-pack.txt: Flow long synopsis line Documentation/git-send-pack.txt: Document --signed gitremote-helpers.txt: Document pushcert option transport: Remove git_transport_options.push_cert config.c: Expose git_parse_maybe_bool builtin/send-pack.c: Use option parsing API Support signing pushes iff the server supports it Add a config option push.gpgSign for default signed pushes Documentation/config.txt| 8 ++ Documentation/git-push.txt | 15 ++- Documentation/git-send-pack.txt | 16 ++- Documentation/gitremote-helpers.txt | 3 + builtin/push.c | 42 +++- builtin/send-pack.c | 192 cache.h | 1 + config.c| 6 +- remote-curl.c | 16 ++- send-pack.c | 43 ++-- send-pack.h | 12 ++- transport-helper.c | 34 +++ transport.c | 11 ++- transport.h | 6 +- 14 files changed, 253 insertions(+), 152 deletions(-) -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] builtin/send-pack.c: Use option parsing API
On Wed, Aug 19, 2015 at 2:00 PM, Stefan Beller sbel...@google.com wrote: On Wed, Aug 19, 2015 at 8:26 AM, Dave Borowitz dborow...@google.com wrote: The old option parsing code in this plumbing command predates this API, so option parsing was done more manually. Using the new API brings send-pack more in line with push, and accepts new variants like --no-* for negating options. Signed-off-by: Dave Borowitz dborow...@google.com --- builtin/send-pack.c | 163 +++- 1 file changed, 59 insertions(+), 104 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 23b2962..5f2c744 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -12,10 +12,15 @@ #include version.h #include sha1-array.h #include gpg-interface.h +#include gettext.h -static const char send_pack_usage[] = -git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...]\n - --all and explicit ref specification are mutually exclusive.; +static const char * const send_pack_usage[] = { + N_(git send-pack [--all | --mirror] [--dry-run] [--force] + [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] + [host:]directory [ref...]\n + --all and explicit ref specification are mutually exclusive.), + NULL, +}; static struct send_pack_args args; @@ -107,116 +112,66 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int ret; int helper_status = 0; int send_all = 0; + int verbose = 0; const char *receivepack = git-receive-pack; + unsigned dry_run = 0; + unsigned send_mirror = 0; + unsigned force_update = 0; + unsigned quiet = 0; + unsigned push_cert = 0; + unsigned use_thin_pack = 0; + unsigned atomic = 0; + unsigned stateless_rpc = 0; First I thought: You could write to the args flags directly from the options. No need to have (most of) the variables around here and copy over the values. You'd need to use OPT_BIT instead for setting a specific bit though but then I realized we do not have a direct bit field in args, which would make it a bit unreadable. Right, and args-push_cert etc. is invalid, and I didn't know if it was ok to expand the args struct to be several words longer. But I'm not a C programmer so I'm happy to take suggestions how to make this more idiomatic. int flags; unsigned int reject_reasons; int progress = -1; int from_stdin = 0; struct push_cas_option cas = {0}; - git_config(git_gpg_config, NULL); + struct option options[] = { + OPT__VERBOSITY(verbose), + OPT_STRING(0, receive-pack, receivepack, receive-pack, N_(receive pack program)), + OPT_STRING(0, exec, receivepack, receive-pack, N_(receive pack program)), + OPT_STRING(0, remote, remote_name, remote, N_(remote name)), + OPT_BOOL(0, all, send_all, N_(push all refs)), + OPT_BOOL('n' , dry-run, dry_run, N_(dry run)), + OPT_BOOL(0, mirror, send_mirror, N_(mirror all refs)), + OPT_BOOL('f', force, force_update, N_(force updates)), -f and -n are new here now? Yeah, I was going for consistency with push.c (and also just copy/pasted ;) + OPT_BOOL(0, signed, push_cert, N_(GPG sign the push)), + OPT_BOOL(0, progress, progress, N_(force progress reporting)), + OPT_BOOL(0, thin, use_thin_pack, N_(use thin pack)), + OPT_BOOL(0, atomic, atomic, N_(request atomic transaction on remote side)), + OPT_BOOL(0, stateless-rpc, stateless_rpc, N_(use stateless RPC protocol)), + OPT_BOOL(0, stdin, from_stdin, N_(read refs from stdin)), + OPT_BOOL(0, helper-status, helper_status, N_(print status from remote helper)), + { OPTION_CALLBACK, + 0, CAS_OPT_NAME, cas, N_(refname:expect), + N_(require old value of ref to be at this value), + PARSE_OPT_OPTARG, parseopt_push_cas_option }, + OPT_END() + }; - argv++; - for (i = 1; i argc; i++, argv++) { - const char *arg = *argv; - - if (*arg == '-') { - if (starts_with(arg, --receive-pack=)) { - receivepack = arg + 15; - continue; - } - if (starts_with(arg, --exec=)) { - receivepack = arg + 7; - continue; - } - if (starts_with(arg, --remote=)) { - remote_name = arg + 9
Re: [PATCH v2 2/9] Documentation/git-send-pack.txt: Flow long synopsis line
On Wed, Aug 19, 2015 at 3:56 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-send-pack.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index b5d09f7..6a6 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -9,7 +9,8 @@ git-send-pack - Push objects over Git protocol to another repository SYNOPSIS [verse] -'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...] +'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] + [--verbose] [--thin] [--atomic] [host:]directory [ref...] DESCRIPTION --- As can be expected from the Subject: line, this patch is line-wrapped and does not apply ;-) I produced the patch with git format-patch --subject-prefix='PATCH v2' --cover-letter @{u}.. and mailed with git send-email --to=git@vger.kernel.org,gits...@pobox.com 0*.patch; is there a way that would have preserved whitespace better? I've done a trivial fix-up and took the liberty of making the result of this step into three lines, not two. That would make 3/9 look more trivial. Ok by me. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] Support signing pushes iff the server supports it
On Wed, Aug 19, 2015 at 3:58 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Add a new flag --signed-if-possible to push and send-pack that sends a push certificate if and only if the server advertised a push cert nonce. If not, at least warn the user that their push may not be as secure as they thought. Signed-off-by: Dave Borowitz dborow...@google.com --- Obviously, the above description needs updating. Here is what I've queued tentatively. Sound good. Thanks. commit 32d273dfabb0a70b2839971f5afff7fa86a8f4c2 Author: Dave Borowitz dborow...@google.com Date: Wed Aug 19 11:26:46 2015 -0400 push: support signing pushes iff the server supports it Add a new flag --sign=true (or --sign=false), which means the same thing as the original --signed (or --no-signed). Give it a third value --sign=if-asked to tell push and send-pack to send a push certificate if and only if the server advertised a push cert nonce. If not, warn the user that their push may not be as secure as they thought. Signed-off-by: Dave Borowitz dborow...@google.com Signed-off-by: Junio C Hamano gits...@pobox.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes
On Mon, Aug 17, 2015 at 1:13 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: --- Does the lack of sign-off indicate something (like this is just a 'what do people think?' weatherbaloon not yet a serious submission)? +push.gpgSign:: + May be set to a boolean value, or the string 'if-possible'. A + true value causes all pushes to be GPG signed, as if '--signed' + is passed to linkgit:git-push[1]. The string 'if-possible' + causes pushes to be signed if the server supports it, as if + '--signed-if-possible' is passed to 'git push'. A false value + may override a value from a lower-priority config file. An + explicit command-line flag always overrides this config option. diff --git a/builtin/push.c b/builtin/push.c index 95a67c5..8972193 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb) return git_default_config(k, v, NULL); } +static void set_push_cert_flags_from_config(int *flags) +{ + const char *value; + /* Ignore config if flags were set from command line. */ + if (*flags (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE)) + return; This looks somewhat strange. Usually we read from config first and then from options, so a git_config() callback shouldn't have to worry about what command line option parser did (because it hasn't happened yet). Why isn't the addition to support this new variable done inside existing git_push_config() callback function? The issue is that if both _ALWAYS and _IF_POSSIBLE are set, git_transport_push interprets it as _ALWAYS. But, we are also supposed to prefer explicit command-line options to config values. Suppose we parsed config first, then options. If the user has push.signed = always and and passes --signed-if-possible, then the end result is (_ALWAYS | _IF_POSSIBLE), aka always, and we've violated prefer command line options to config values. I guess the alternative is to have --signed just clear the _IF_POSSIBLE bit in addition to setting the _ALWAYS bit, and vice versa for --signed-if-possible. I am not sure what the end result would be if the user passed a combination of various --signed and --signed-if-possible flags on the command line; maybe that's not worth worrying about. + if (!git_config_get_value(push.gpgsign, value)) { + switch (git_config_maybe_bool(push.gpgsign, value)) { + case 1: + *flags |= TRANSPORT_PUSH_CERT_ALWAYS; + break; + default: + if (value !strcmp(value, if-possible)) + *flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE; + else + die(_(Invalid value for 'push.gpgsign')); + } + } +} + maybe_bool() returns 0 for false (and its various spellings), 1 for true (and its various spellings) and -1 for that's not a bool. For A false value may override a value to be true, we'd need case 0: *flags = ~TRANSPORT_PUSH_CERT_ALWAYS; break; or something? Yes, except unsetting both flags? ~(TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_CERT_IF_POSSIBLE) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: Ok, so let us bikeshed a bit further. Bikeshed 1. Option A: --signed/--no-signed--signed-if-possible Option B: --signed=true|false|if-possible, --signed alone implies =true. Bikeshed 2. Option A: if-possible The possibly confusing thing is one might interpret missing gpg to mean impossible, i.e. if gpg is not installed don't attempt to sign, which is not the behavior we want. I don't have another succinct way of saying this. if-server-supported is a mouthful. I think Jonathan mentioned opportunistic, which is fairly opaque. By strange, I was referring to the possible perception issue on having a choice other than yes/no for a configuration that allows you to express your security preference. My preference on Bikeshed 1. would probably be to add --sign=yes/no/if-asked and to keep --[no-]signed for no and yes for existing users. Incidentally, I just looked up incidence of true/false vs. yes/no in command line options, and the results are decidedly undecided: $ grep -e '--[^ ]*=[^ ]*true' Documentation/*.txt Documentation/git-init.txt:--shared[=(false|true|umask|group|all|world|everybody|0xxx)]:: Documentation/git-pull.txt:--rebase[=false|true|preserve]:: Documentation/git-svn.txt:--shared[=(false|true|umask|group|all|world|everybody)]:: $ grep -e '--[^ ]*=[^ ]*yes' Documentation/*.txt Documentation/fetch-options.txt:--recurse-submodules[=yes|on-demand|no]:: Documentation/fetch-options.txt:--recurse-submodules-default=[yes|on-demand]:: Documentation/git-pull.txt:--[no-]recurse-submodules[=yes|on-demand|no]:: Consistency is hard. I am inclined to stick with yes/no in this case because --recurse-submodules at least feels like a more modern option that we should emulate, but don't feel strongly either way. Regarding Bikeshed 2., I do not have a strong opinion myself. Although it sounds like you already expressed an opinion for if-asked if-possible, which is stronger than my own :) Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Mon, Aug 17, 2015 at 2:47 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano gits...@pobox.com wrote: My preference on Bikeshed 1. would probably be to add --sign=yes/no/if-asked and to keep --[no-]signed for no and yes for existing users. Incidentally, I just looked up incidence of true/false vs. yes/no in command line options,... My yes/no was a short-hand for yes (and various other ways to spell true) and no (and various other ways to spell false). I was NOT bikeshedding to say I do not like true/false but favor yes/no. I actually was expecting a short discussion on sign vs signed, though. As tag --sign is not tag --signed even though we call the resulting object a signed tag, push --sign may be a good enough way to spell signed push. I _think_ signed pushes are recent enough that we still have time to deprecate --signed form, but I do not think it is worth it. I agree that push --sign would be better than push --signed for consistency with tag, but will defer to you as to whether it's worth it to do the deprecation. So an updated suggestion would be that we'd take (this is a pretty much exhaustive enumeration) these: --no-signed --signed --signed=if-asked --signed=yes/true/on/1/2... --signed=no/false/off/0 Fine by me. Would you expect those to all be documented, or just --[no-]signed|--signed=(yes|no|if-asked) and silently accept the rest? Is there a common utility function that does what we want? Basically git_config_maybe_bool but not specifically about configs. We might want to throw in 'always' and 'never' as synonyms for 'true' and 'false', but again I do not think it is worth the confusion factor, as 'always' and 'true' already mean different things in some other contexts. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Mon, Aug 17, 2015 at 3:54 PM, Junio C Hamano gits...@pobox.com wrote: In the shorter term, at least we should be able to introduce git_parse_maybe_bool() that does not take name, use that as a helper to implement git_config_maybe_bool(), so that the existing callers of git_config_maybe_bool() does not have to change. And that new helper can be used as your Basically it, but not specifically about configs. Will do, thanks for the suggestion. Slight digression for a question that came up during reworking the series: would it be reasonable to rewrite option parsing in builtin/send-pack.c to use the options API? That way we can easily reuse the option callback from builtin/push.c. (It would have some side effects like making --no-* variants work where they did not before; I assume that's a good thing, but it's marginally inconsistent with some other plumbing commands like receive-pack.) -- 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 7/7] Add a config option push.gpgSign for default signed pushes
On Mon, Aug 17, 2015 at 3:42 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: The issue is that if both _ALWAYS and _IF_POSSIBLE are set, git_transport_push interprets it as _ALWAYS. But, we are also supposed to prefer explicit command-line options to config values. Suppose we parsed config first, then options. If the user has push.signed = always and and passes --signed-if-possible, then the end result is (_ALWAYS | _IF_POSSIBLE), aka always,... Doesn't that merely suggest that the option parsing is implemented incorrectly? Why is --signed-if-possible just ORing its bits into the flag, instead of clearing and setting? Yes, that would be incorrect, but the actual implementation in my patch is not incorrect, it just solves the problem a different way. The alternative I suggested is another correct implementation, and it sounds like it's more in line with the convention you expect. I'll go with that. -- 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? git config and --unset are not inverses
It looks like git config --unset may leave an orphaned section, but a subsequent set adds a new section: $ git config --file=myconfig section.foo true $ git config --file=myconfig --unset section.foo $ cat myconfig [section] $ git config --file=myconfig section.foo true $ cat myconfig [section] [section] foo = true $ git --version git version 2.5.0.rc2.392.g76e840b -- 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: Thoughts on refactoring the transport (+helper) code
On Thu, Aug 13, 2015 at 12:45 PM, Ilari Liusvaara ilari.liusva...@elisanet.fi wrote: On Thu, Aug 13, 2015 at 11:42:50AM -0400, Dave Borowitz wrote: In my ideal world: -smart_options would never be NULL, and would instead be called options with a smart bit which is unset for dumb protocols. -Command line option processing code in {fetch,clone,push}.c would set fields in options (formerly known as smart_options) rather than passing around string constants. -TRANS_OPT_* string constants would only be used for remote helper protocol option names, and no more hard-coding these names. -The flags arg to the push* callbacks would go away, and callbacks would respect options instead. -The helper code would not send options immediately, but instead send just the relevant options immediately before a particular command requires them. Hopefully we could then eliminate the set_option callback entirely. (Two things I ran into that complicated this: 1) backfill_tags mutates just a couple of options before reusing the transport, and 2) the handling of push_cas_option is very special-cased.) AFAIK, here are what one can encounter with remote helpers: Some remote helpers are always smart, some are always dumb, and some may be smart or dumb, depending on the URL. I don't know how useful the last one is (smart or dumb depending on URL) is. I have never seen such remote helper (HTTP doesn't count, from Git PoV it is always dumb). All smart helpers take common options associated with git smart transport (e.g. thin, follow tags, push certs, etc..). Dumb transports may take some of these kind of of options (esp. smart HTTP), but it is highly dependent on the helper. Then transports can have connection-level options (e.g. HTTPS cert options). These can be present regardless of wheither transport is smart or dumb. The receive-pack / send-pack paths fall into connection-level options, even if those are presently in smart transport common options. Since those things make sense for some smart transports (e.g. ssh://), but not others (e.g. git://). Yeah, thanks for summarizing some of the differences between options. The really confusing thing when looking at the code, though, is that the various different ways of specifying options in the code don't actually align with those distinctions. Maybe they once did, but they certainly don't today. I wouldn't be opposed to splitting up git_transport_options into different structs for connection options, smart fetch options, smart push options, etc., rather than putting everything in one kitchen-sink struct. -Ilari -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] transport: Remove git_transport_options.push_cert
This field was set in transport_set_option, but never read in the push code. The push code basically ignores the smart_options field entirely, and derives its options from the flags arguments to the push* callbacks. Note that in git_transport_push there are already several args set from flags that have no corresponding field in git_transport_options; after this change, push_cert is just like those. Signed-off-by: Dave Borowitz dborow...@google.com --- transport.c | 3 --- transport.h | 1 - 2 files changed, 4 deletions(-) diff --git a/transport.c b/transport.c index 40692f8..3dd6e30 100644 --- a/transport.c +++ b/transport.c @@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts, die(transport: invalid depth option '%s', value); } return 0; - } else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) { - opts-push_cert = !!value; - return 0; } return 1; } diff --git a/transport.h b/transport.h index 18d2cf8..79190df 100644 --- a/transport.h +++ b/transport.h @@ -12,7 +12,6 @@ struct git_transport_options { unsigned check_self_contained_and_connected : 1; unsigned self_contained_and_connected : 1; unsigned update_shallow : 1; - unsigned push_cert : 1; int depth; const char *uploadpack; const char *receivepack; -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] Add a config option push.gpgSign for default signed pushes
--- Documentation/config.txt | 8 builtin/push.c | 22 ++ builtin/send-pack.c | 27 ++- 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 016f6e9..6804f5b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2178,6 +2178,14 @@ push.followTags:: may override this configuration at time of push by specifying '--no-follow-tags'. +push.gpgSign:: + May be set to a boolean value, or the string 'if-possible'. A + true value causes all pushes to be GPG signed, as if '--signed' + is passed to linkgit:git-push[1]. The string 'if-possible' + causes pushes to be signed if the server supports it, as if + '--signed-if-possible' is passed to 'git push'. A false value + may override a value from a lower-priority config file. An + explicit command-line flag always overrides this config option. rebase.stat:: Whether to show a diffstat of what changed upstream since the last diff --git a/builtin/push.c b/builtin/push.c index 95a67c5..8972193 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb) return git_default_config(k, v, NULL); } +static void set_push_cert_flags_from_config(int *flags) +{ + const char *value; + /* Ignore config if flags were set from command line. */ + if (*flags (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE)) + return; + if (!git_config_get_value(push.gpgsign, value)) { + switch (git_config_maybe_bool(push.gpgsign, value)) { + case 1: + *flags |= TRANSPORT_PUSH_CERT_ALWAYS; + break; + default: + if (value !strcmp(value, if-possible)) + *flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE; + else + die(_(Invalid value for 'push.gpgsign')); + } + } +} + int cmd_push(int argc, const char **argv, const char *prefix) { int flags = 0; @@ -537,6 +557,8 @@ int cmd_push(int argc, const char **argv, const char *prefix) git_config(git_push_config, flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); + set_push_cert_flags_from_config(flags); + if (deleterefs (tags || (flags (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR die(_(--delete is incompatible with --all, --mirror and --tags)); if (deleterefs argc 2) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 8eebbf4..9c8b7de 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -92,6 +92,31 @@ static void print_helper_status(struct ref *ref) strbuf_release(buf); } +static int send_pack_config(const char *k, const char *v, void *cb) +{ + git_gpg_config(k, v, NULL); + + if (!strcmp(k, push.gpgsign)) { + const char *value; + if (!git_config_get_value(push.gpgsign, value)) { + switch (git_config_maybe_bool(push.gpgsign, value)) { + case 0: + args.push_cert = SEND_PACK_PUSH_CERT_NEVER; + break; + case 1: + args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS; + break; + default: + if (value !strcasecmp(value, if-possible)) + args.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE; + else + return error(Invalid value for '%s', k); + } + } + } + return 0; +} + int cmd_send_pack(int argc, const char **argv, const char *prefix) { int i, nr_refspecs = 0; @@ -114,7 +139,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int from_stdin = 0; struct push_cas_option cas = {0}; - git_config(git_gpg_config, NULL); + git_config(send_pack_config, NULL); argv++; for (i = 1; i argc; i++, argv++) { -- 2.5.0.276.gf5e568e -- 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/7] Documentation/git-send-pack.txt: Flow long synopsis line
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-send-pack.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index b5d09f7..6a6 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -9,7 +9,8 @@ git-send-pack - Push objects over Git protocol to another repository SYNOPSIS [verse] -'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...] +'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] + [--verbose] [--thin] [--atomic] [host:]directory [ref...] DESCRIPTION --- -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] Flags and config to sign pushes by default
Remembering to pass --signed to git push on every push is extra typing that is easy to forget, and just leads to annoyance if the remote has a hook that makes signed pushes required. Add a config option push.gpgSign, analogous to commit.gpgSign, allowing users to set this flag by default. Since --signed push will simply fail on any remote that does not advertise a push cert nonce, actually setting this to true is not very useful (except for the super-paranoid who would never want to push to a server that does not support signed pushes). So, add a third state to this boolean, if-possible, to sign the push if and only if supported by the server. To keep parity between the config and command line options, add a --signed-if-possible flag to git push as well. The if-possible name and weird tri-state boolean is basically a straw man, and I am happy to change if someone has a clearer suggestion. Dave Borowitz (7): Documentation/git-push.txt: Document when --signed may fail Documentation/git-send-pack.txt: Flow long synopsis line Documentation/git-send-pack.txt: Document --signed gitremote-helpers.txt: Document pushcert option transport: Remove git_transport_options.push_cert Support signing pushes iff the server supports it Add a config option push.gpgSign for default signed pushes Documentation/config.txt| 8 Documentation/git-push.txt | 11 +-- Documentation/git-send-pack.txt | 17 - Documentation/gitremote-helpers.txt | 3 +++ builtin/push.c | 26 +- builtin/send-pack.c | 33 +++-- remote-curl.c | 14 ++ send-pack.c | 18 +++--- send-pack.h | 8 +++- transport-helper.c | 34 +- transport.c | 11 +++ transport.h | 6 +++--- 12 files changed, 151 insertions(+), 38 deletions(-) -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] Support signing pushes iff the server supports it
Add a new flag --signed-if-possible to push and send-pack that sends a push certificate if and only if the server advertised a push cert nonce. If not, at least warn the user that their push may not be as secure as they thought. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-push.txt | 9 +++-- Documentation/git-send-pack.txt | 9 +++-- builtin/push.c | 4 +++- builtin/send-pack.c | 6 +- remote-curl.c | 14 ++ send-pack.c | 18 +++--- send-pack.h | 8 +++- transport-helper.c | 34 +- transport.c | 8 +++- transport.h | 5 +++-- 10 files changed, 81 insertions(+), 34 deletions(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index f8b8b8b..fcfdf73 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=git-receive-pack] [--repo=repository] [-f | --force] [--prune] [-v | --verbose] - [-u | --set-upstream] [--signed] + [-u | --set-upstream] [--signed] [--signed-if-possible] [--force-with-lease[=refname[:expect]]] [--no-verify] [repository [refspec...]] @@ -139,7 +139,12 @@ already exists on the remote side. logged. See linkgit:git-receive-pack[1] for the details on the receiving end. If the `gpg` executable is not available, or if the server does not support signed pushes, the push will - fail. + fail. Takes precedence over --signed-if-possible. + +--signed-if-possible:: + Like --signed, but only if the server supports signed pushes. If + the server supports signed pushes but the `gpg` is not available, + the push will fail. --[no-]atomic:: Use an atomic transaction on the remote side if available. diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index dde13b0..5789208 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] - [--verbose] [--thin] [--atomic] [--signed] + [--verbose] [--thin] [--atomic] [--signed] [--signed-if-possible] [host:]directory [ref...] DESCRIPTION @@ -75,7 +75,12 @@ be in a separate packet, and the list must end with a flush packet. logged. See linkgit:git-receive-pack[1] for the details on the receiving end. If the `gpg` executable is not available, or if the server does not support signed pushes, the push will - fail. + fail. Takes precedence over --signed-if-possible. + +--signed-if-possible:: + Like --signed, but only if the server supports signed pushes. If + the server supports signed pushes but the `gpg` is not available, + the push will fail. host:: A remote host to house the repository. When this diff --git a/builtin/push.c b/builtin/push.c index 57c138b..95a67c5 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -526,7 +526,9 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BIT(0, no-verify, flags, N_(bypass pre-push hook), TRANSPORT_PUSH_NO_HOOK), OPT_BIT(0, follow-tags, flags, N_(push missing but relevant tags), TRANSPORT_PUSH_FOLLOW_TAGS), - OPT_BIT(0, signed, flags, N_(GPG sign the push), TRANSPORT_PUSH_CERT), + OPT_BIT(0, signed, flags, N_(GPG sign the push), TRANSPORT_PUSH_CERT_ALWAYS), + OPT_BIT(0, signed-if-possible, flags, N_(GPG sign the push, if supported by the server), + TRANSPORT_PUSH_CERT_IF_POSSIBLE), OPT_BIT(0, atomic, flags, N_(request atomic transaction on remote side), TRANSPORT_PUSH_ATOMIC), OPT_END() }; diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 23b2962..8eebbf4 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -158,7 +158,11 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) continue; } if (!strcmp(arg, --signed)) { - args.push_cert = 1; + args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS; + continue; + } + if (!strcmp(arg, --signed-if-possible)) { + args.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE; continue; } if (!strcmp(arg, --progress)) { diff --git a/remote
[PATCH 3/7] Documentation/git-send-pack.txt: Document --signed
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-send-pack.txt | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index 6a6..dde13b0 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -10,7 +10,8 @@ SYNOPSIS [verse] 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=git-receive-pack] - [--verbose] [--thin] [--atomic] [host:]directory [ref...] + [--verbose] [--thin] [--atomic] [--signed] + [host:]directory [ref...] DESCRIPTION --- @@ -68,6 +69,14 @@ be in a separate packet, and the list must end with a flush packet. fails to update then the entire push will fail without changing any refs. +--signed:: + GPG-sign the push request to update refs on the receiving + side, to allow it to be checked by the hooks and/or be + logged. See linkgit:git-receive-pack[1] for the details + on the receiving end. If the `gpg` executable is not available, + or if the server does not support signed pushes, the push will + fail. + host:: A remote host to house the repository. When this part is specified, 'git-receive-pack' is invoked via -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail
Like --atomic, --signed will fail if the server does not advertise the necessary capability. In addition, it requires gpg on the client side. Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/git-push.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 135d810..f8b8b8b 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -137,7 +137,9 @@ already exists on the remote side. GPG-sign the push request to update refs on the receiving side, to allow it to be checked by the hooks and/or be logged. See linkgit:git-receive-pack[1] for the details - on the receiving end. + on the receiving end. If the `gpg` executable is not available, + or if the server does not support signed pushes, the push will + fail. --[no-]atomic:: Use an atomic transaction on the remote side if available. -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] gitremote-helpers.txt: Document pushcert option
Signed-off-by: Dave Borowitz dborow...@google.com --- Documentation/gitremote-helpers.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 82e2d15..78e0b27 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -448,6 +448,9 @@ set by Git if the remote helper has the 'option' capability. 'option update-shallow {'true'|'false'}:: Allow to extend .git/shallow if the new refs require it. +'option pushcert {'true'|'false'}:: + GPG sign pushes. + SEE ALSO linkgit:git-remote[1] -- 2.5.0.276.gf5e568e -- 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: An option to sign the push by default
On Sun, Aug 9, 2015 at 12:57 PM, Agostino Sarubbo a...@gentoo.org wrote: Hello folks, during the configuration of git, client side, to sign all commit I used: git config --global commit.gpgsign 1 Since at push time I use: git push --signed I'm wondering if there is a git config option which put something in the config file and avoid to type --signed. I agree this would be useful, and that's why I just implemented it today :) If there isn't this feature, I'd like to know if it is a reasonable feature request. Thanks in advance. -- Agostino Sarubbo Gentoo Linux Developer -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: config options for automatic signed tags and signed pushes
On Tue, Aug 11, 2015 at 3:06 PM, Matthew Thode mth...@mthode.org wrote: If it doesn't already exist (not that I can find). I'd like to see config options analogous to commit.gpgsign for both tagging and pushing. I agree this would be useful, and that's why I just implemented it today :) Not sure where else to send this request though, let me know if there's a better place. Thanks, -- Matthew Thode -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano gits...@pobox.com wrote: The if-possible name and weird tri-state boolean is basically a straw man, and I am happy to change if someone has a clearer suggestion. Yes, it looks somewhat strange. Let me go on a slight tangent to explain why I think it is OK for push --signed. I think we agree that there are three possible behaviors for push and we should allow the user to specify any of the three. The straw-man strangeness is that two of them are the traditional boolean values true/false and the third is file not found^W^W^Wif-possible :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano gits...@pobox.com wrote: So I am fine as long as if-possible turns a failure to make signed push into a success _only_ when the reason of the failure is because we did not see the capability supported by the receiving end. If the reason why you cannot do a signed push is because you cannot sign push certificate, if-possible should still fail. I completely agree with your reasoning, and that's exactly how I implemented and documented --signed-if-possible. From patch 6/7: +--signed-if-possible:: + Like --signed, but only if the server supports signed pushes. If + the server supports signed pushes but the `gpg` is not available, + the push will fail. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Flags and config to sign pushes by default
On Fri, Aug 14, 2015 at 4:45 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano gits...@pobox.com wrote: Yes, it looks somewhat strange. ... The straw-man strangeness is that two of them are the traditional boolean values true/false and the third is file not found^W^W^Wif-possible :) It actually is not uncommon for a Git configuration variable to start its life as a boolean and then later become tristate (or more) as we gain experience with the system, so don't worry about it being strange. A tristate, among whose choices two of them are true and false, is not strange around here. Ok, so let us bikeshed a bit further. Bikeshed 1. Option A: --signed/--no-signed--signed-if-possible Option B: --signed=true|false|if-possible, --signed alone implies =true. Bikeshed 2. Option A: if-possible The possibly confusing thing is one might interpret missing gpg to mean impossible, i.e. if gpg is not installed don't attempt to sign, which is not the behavior we want. I don't have another succinct way of saying this. if-server-supported is a mouthful. I think Jonathan mentioned opportunistic, which is fairly opaque. By strange, I was referring to the possible perception issue on having a choice other than yes/no for a configuration that allows you to express your security preference. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thoughts on refactoring the transport (+helper) code
I spent a day trying to understand what the heck is going on in the transport code, with the intent of adding an option like --sign-if-possible to git push. This has come up twice on the mailing list in the past couple weeks, and I also think it's important for $DAY_JOB. I'm giving up in despair, but I decided to leave some comments that will hopefully help a future, more enterprising refactorer. Please don't read this (entirely) as a rant; I understand that the transport code is old and hairy and grew organically to get to this point. I really do just hope this makes the next guy's job easier in understanding the code. (But then again, this hope may be in vain: it's not like I searched the mailing list myself before diving in :) One of the biggest issues IMO is that the idea of options for transports is grossly overloaded: -struct git_transport_options contains mostly boolean options that are read in fetch.c to enable certain options. -In push.c, we for the most part ignore the smart_options field in transport, and instead rely on the flags bitfield. However, some (not all) flags in this bitfield have seemingly-equivalent fields in git_transport_options. In some cases (particularly push_cert), the field in git_transport_options is ignored. -Similarly, one might think the executable arg to the connect callback might bear some relation to the uploadpack/receivepack field in smart_options. It does not. -Some (but by no means all) options are set via transport_set_option by passing in one of the TRANS_OPT_* string constants. This a) switches on these values and populates the appropriate smart_options field, and b) calls the set_option callback. -The end result is that the TRANS_OPT_* constants are a mixture of things that can be set on the command line and things that are documented in the remote helper protocol. But there are also options used in the remote helper protocol that are hard-coded and have no constant, or can't be set on the command line, etc. -The helper protocol's set_helper_option synchronously sends the option to the helper process and reads the response. Naturally, the return code from transport_set_option is almost always ignored. In my ideal world: -smart_options would never be NULL, and would instead be called options with a smart bit which is unset for dumb protocols. -Command line option processing code in {fetch,clone,push}.c would set fields in options (formerly known as smart_options) rather than passing around string constants. -TRANS_OPT_* string constants would only be used for remote helper protocol option names, and no more hard-coding these names. -The flags arg to the push* callbacks would go away, and callbacks would respect options instead. -The helper code would not send options immediately, but instead send just the relevant options immediately before a particular command requires them. Hopefully we could then eliminate the set_option callback entirely. (Two things I ran into that complicated this: 1) backfill_tags mutates just a couple of options before reusing the transport, and 2) the handling of push_cas_option is very special-cased.) There are other confusing things but I think that would make option handling in particular less of a head-scratcher. -- 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: send-pack does not respect http.signingkey
On Thu, Jul 16, 2015 at 3:08 PM, Dave Borowitz dborow...@google.com wrote: On Thu, Jul 16, 2015 at 2:10 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano gits...@pobox.com wrote: Perhaps something like this? Seems like it should work. Jonathan had suggested there might be some principled reason why send-pack does not respect config options, and suggested passing it in as a flag. But that would be more work, certainly, as it would also have to get passed through git-remote-http somehow. I actually was wondering about exactly the same thing as Jonathan, and that is where my Perhaps came from. I will say, though, as the maintainer of a handful of custom remote helpers, I would prefer a solution that does not involve changing the implementation of those just to pass this configuration through. That is not a controversial part ;-) So my vote would be for send-pack to respect the normal config options. The thing is what should be included in the normal config options. The something like this? patch was deliberately narrow, including only the GPG thing and nothing else. But anticipating that the ref backend would be per repo configuration, and send-pack would want to read from refs (and possibly write back tracking?), we may want to prepare ourselves by reading a bit wider than GPG thing and nothing else, e.g. git_default_config() or something like that. Ah, now I understand the question. I have no opinion other than that we shouldn't let discussion about future features prevent us from fixing this obvious signed push bug :) Should I formally send a patch with your configuration one-liner? -- 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] builtin/send-pack.c: Respect http.signingkey
From: Junio C Hamano gits...@pobox.com Prior to this patch, when git-send-pack was exec'ed, as is done by git-remote-http, it did not reread the config, so it did not respect the configured value of http.signingkey. Thus it was impossible to specify a signing key over HTTP, other than the default key in the keyring having a User ID matching the Name email format. This patch at least partially fixes the problem by reading in the GPG config from within send-pack. It does not address the related problem of plumbing a value for this configuration option using `git -c http.signingkey push ...`. Signed-off-by: Dave Borowitz dborow...@google.com --- builtin/send-pack.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index b961e5a..23b2962 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -11,6 +11,7 @@ #include transport.h #include version.h #include sha1-array.h +#include gpg-interface.h static const char send_pack_usage[] = git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic] [host:]directory [ref...]\n @@ -113,6 +114,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int from_stdin = 0; struct push_cas_option cas = {0}; + git_config(git_gpg_config, NULL); + argv++; for (i = 1; i argc; i++, argv++) { const char *arg = *argv; -- 2.4.3.573.g4eafbef -- 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: send-pack does not respect http.signingkey
When git-send-pack is exec'ed, as is done by git-remote-http, it does not reread the config, so it does not respect the configured http.signingkey, either from the config file or -c on the command line. Thus it is currently impossible to specify a signing key over HTTP, other than the default one matching the Name email format in the keyring. This is not an issue for git:// as send-pack is executed directly in the same process that reads the config. --- gpg-interface.c | 1 + run-command.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index 68b0c81..e0ffcb0 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -87,6 +87,7 @@ void set_signing_key(const char *key) int git_gpg_config(const char *var, const char *value, void *cb) { if (!strcmp(var, user.signingkey)) { + fprintf(stderr, setting %s\n, value); set_signing_key(value); } if (!strcmp(var, gpg.program)) { diff --git a/run-command.c b/run-command.c index 4d73e90..39ae8d5 100644 --- a/run-command.c +++ b/run-command.c @@ -1,5 +1,6 @@ #include cache.h #include run-command.h +#include gpg-interface.h #include exec_cmd.h #include sigchain.h #include argv-array.h @@ -344,7 +345,7 @@ fail_pipe: cmd-err = fderr[0]; } - trace_argv_printf(cmd-argv, trace: run_command:); + trace_argv_printf(cmd-argv, trace: run_command (%s):, get_signing_key()); fflush(NULL); #ifndef GIT_WINDOWS_NATIVE -- 2.4.3.573.g4eafbef -- 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: send-pack does not respect http.signingkey
On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano gits...@pobox.com wrote: Perhaps something like this? Seems like it should work. Jonathan had suggested there might be some principled reason why send-pack does not respect config options, and suggested passing it in as a flag. But that would be more work, certainly, as it would also have to get passed through git-remote-http somehow. I actually was wondering about exactly the same thing as Jonathan, and that is where my Perhaps came from. I will say, though, as the maintainer of a handful of custom remote helpers, I would prefer a solution that does not involve changing the implementation of those just to pass this configuration through. So my vote would be for send-pack to respect the normal config options. Not sure what that would mean for -c on the command line, though. -- 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: send-pack does not respect http.signingkey
On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: When git-send-pack is exec'ed, as is done by git-remote-http, it does not reread the config, so it does not respect the configured http.signingkey, either from the config file or -c on the command line. Thus it is currently impossible to specify a signing key over HTTP, other than the default one matching the Name email format in the keyring. This is not an issue for git:// as send-pack is executed directly in the same process that reads the config. Interesting. I agree that it would be a problem not to be able to specify which signing key to use. Perhaps something like this? Seems like it should work. Jonathan had suggested there might be some principled reason why send-pack does not respect config options, and suggested passing it in as a flag. But that would be more work, certainly, as it would also have to get passed through git-remote-http somehow. diff --git a/builtin/send-pack.c b/builtin/send-pack.c index b564a77..57c3a9c 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -11,6 +11,7 @@ #include transport.h #include version.h #include sha1-array.h +#include gpg-interface.h static const char send_pack_usage[] = git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory [ref...]\n @@ -113,6 +114,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int from_stdin = 0; struct push_cas_option cas = {0}; + git_config(git_gpg_config, NULL); + argv++; for (i = 1; i argc; i++, argv++) { const char *arg = *argv; -- 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: send-pack does not respect http.signingkey
On Thu, Jul 16, 2015 at 2:10 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano gits...@pobox.com wrote: Dave Borowitz dborow...@google.com writes: On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano gits...@pobox.com wrote: Perhaps something like this? Seems like it should work. Jonathan had suggested there might be some principled reason why send-pack does not respect config options, and suggested passing it in as a flag. But that would be more work, certainly, as it would also have to get passed through git-remote-http somehow. I actually was wondering about exactly the same thing as Jonathan, and that is where my Perhaps came from. I will say, though, as the maintainer of a handful of custom remote helpers, I would prefer a solution that does not involve changing the implementation of those just to pass this configuration through. That is not a controversial part ;-) So my vote would be for send-pack to respect the normal config options. The thing is what should be included in the normal config options. The something like this? patch was deliberately narrow, including only the GPG thing and nothing else. But anticipating that the ref backend would be per repo configuration, and send-pack would want to read from refs (and possibly write back tracking?), we may want to prepare ourselves by reading a bit wider than GPG thing and nothing else, e.g. git_default_config() or something like that. Ah, now I understand the question. I have no opinion other than that we shouldn't let discussion about future features prevent us from fixing this obvious signed push bug :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-candidate: git based patch tracking and review
On Tue, Dec 1, 2015 at 3:55 PM, Jonathan Niederwrote: > Cc-ing dborowitz, who has been working on storing Gerrit's code review > information in Git instead of a separate database (e.g., see [1]). Thanks, we actually already had a thread going that I realize only in retrospect did not include the git mailing list. -- 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: RefTree: Alternate ref backend
On Tue, Dec 22, 2015 at 8:11 AM, Shawn Pearcewrote: > Yup, and Gerrit Code Review servers often have 100k+ refs per > repository. We can't rewrite the entire store every time either. So > its not a flat list. Its a directory structure using the / separators > in the ref namespace. I wonder if this might be insufficient in some cases, and additional sharding might be required (though I haven't thought about how to do that). Chromium, for example, has upwards of 10k tags: $ git ls-remote https://chromium.googlesource.com/chromium/src refs/tags/\* | wc -l 8733 And I doubt it's unique in this regard. -- 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: Building Git with HTTPS support: avoiding libcurl?
On Tue, Dec 22, 2015 at 7:39 AM, Paul Smithwrote: > I'm trying to build Git (2.6.4) on GNU/Linux, but without any > requirements (other than basic libc etc.) on the local system. This > works fine except for one thing: git-remote-https. > > In order to build this I need to have libcurl, but libcurl is a MONSTER > library with an enormous number of prerequisites (see below). Well, IIUC one of the reasons for Git's fork-everything strategy is to avoid having to dynamically link the core git binary against large libraries like libcurl, so the dependency size has been taken into account at least in that sense. > Just wondering if anyone has considered an alternative to libcurl; maybe > I'm wrong but it seems to me that HTTPS support for Git would require > only a tiny fraction of the libcurl features and maybe there's an > alternative available which would be more targeted? > > I realize this is not a short-term thing in that there won't be an API > compatible library that can just be dropped in. This is more a forward > -looking question. For now I'm looking to see if I can rebuild libcurl > myself without most of these dependencies such as Kerberos, LDAP, etc. > > > $ ldd /usr/lib/x86_64-linux-gnu/libcurl.so.4 > linux-vdso.so.1 => (0x7fff37d81000) > libidn.so.11 => /usr/lib/x86_64-linux-gnu/libidn.so.11 > (0x7f682b921000) > librtmp.so.1 => /usr/lib/x86_64-linux-gnu/librtmp.so.1 > (0x7f682b704000) > libssl.so.1.0.0 => /lib/x86_64-linux-gnu/libssl.so.1.0.0 > (0x7f682b49a000) > libcrypto.so.1.0.0 => /lib/x86_64-linux-gnu/libcrypto.so.1.0.0 > (0x7f682b058000) > libgssapi_krb5.so.2 => /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2 > (0x7f682ae0e000) > liblber-2.4.so.2 => /usr/lib/x86_64-linux-gnu/liblber-2.4.so.2 > (0x7f682abfe000) > libldap_r-2.4.so.2 => /usr/lib/x86_64-linux-gnu/libldap_r-2.4.so.2 > (0x7f682a9ac000) > libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x7f682a792000) > libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 > (0x7f682a573000) > libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x7f682a1a9000) > libgnutls-deb0.so.28 => > /usr/lib/x86_64-linux-gnu/libgnutls-deb0.so.28 (0x7f6829e8d000) > libhogweed.so.4 => /usr/lib/x86_64-linux-gnu/libhogweed.so.4 > (0x7f6829c59000) > libnettle.so.6 => /usr/lib/x86_64-linux-gnu/libnettle.so.6 > (0x7f6829a23000) > libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 > (0x7f68297a3000) > libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x7f682959e000) > libkrb5.so.3 => /usr/lib/x86_64-linux-gnu/libkrb5.so.3 > (0x7f68292cc000) > libk5crypto.so.3 => /usr/lib/x86_64-linux-gnu/libk5crypto.so.3 > (0x7f682909d000) > libcom_err.so.2 => /lib/x86_64-linux-gnu/libcom_err.so.2 > (0x7f6828e98000) > libkrb5support.so.0 => /usr/lib/x86_64-linux-gnu/libkrb5support.so.0 > (0x7f6828c8d000) > libresolv.so.2 => /lib/x86_64-linux-gnu/libresolv.so.2 > (0x7f6828a71000) > libsasl2.so.2 => /usr/lib/x86_64-linux-gnu/libsasl2.so.2 > (0x7f6828855000) > libgssapi.so.3 => /usr/lib/x86_64-linux-gnu/libgssapi.so.3 > (0x7f6828615000) > /lib64/ld-linux-x86-64.so.2 (0x559b03259000) > libp11-kit.so.0 => /usr/lib/x86_64-linux-gnu/libp11-kit.so.0 > (0x7f68283b) > libtasn1.so.6 => /usr/lib/x86_64-linux-gnu/libtasn1.so.6 > (0x7f682819c000) > libkeyutils.so.1 => /lib/x86_64-linux-gnu/libkeyutils.so.1 > (0x7f6827f98000) > libheimntlm.so.0 => /usr/lib/x86_64-linux-gnu/libheimntlm.so.0 > (0x7f6827d8e000) > libkrb5.so.26 => /usr/lib/x86_64-linux-gnu/libkrb5.so.26 > (0x7f6827b04000) > libasn1.so.8 => /usr/lib/x86_64-linux-gnu/libasn1.so.8 > (0x7f6827861000) > libhcrypto.so.4 => /usr/lib/x86_64-linux-gnu/libhcrypto.so.4 > (0x7f682762d000) > libroken.so.18 => /usr/lib/x86_64-linux-gnu/libroken.so.18 > (0x7f6827418000) > libffi.so.6 => /usr/lib/x86_64-linux-gnu/libffi.so.6 > (0x7f682721) > libwind.so.0 => /usr/lib/x86_64-linux-gnu/libwind.so.0 > (0x7f6826fe6000) > libheimbase.so.1 => /usr/lib/x86_64-linux-gnu/libheimbase.so.1 > (0x7f6826dd7000) > libhx509.so.5 => /usr/lib/x86_64-linux-gnu/libhx509.so.5 > (0x7f6826b8c000) > libsqlite3.so.0 => /usr/lib/x86_64-linux-gnu/libsqlite3.so.0 > (0x7f68268be000) > libcrypt.so.1 => /lib/x86_64-linux-gnu/libcrypt.so.1 > (0x7f6826686000) Maybe half of those dependencies are crypto libraries, so even if you managed to eliminate libcurl, you'd have a hard time supporting HTTPS without them. Or maybe use something like boringssl instead? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to
Re: reftable [v4]: new ref storage format
On Sun, Jul 30, 2017 at 11:51 PM, Shawn Pearcewrote: > - Near constant time verification a SHA-1 is referred to by at least > one reference (for allow-tip-sha1-in-want). I think I understated the importance of this when I originally brought up allow-tip-sha1-in-want. This is an important optimization for *any* HTTP server, even without allow-tip-sha1-in-want, in order to validate the SHA-1s sent in the upload-pack request, which doesn't share memory state with the /info/refs request processing.
Re: reftable [v4]: new ref storage format
On Tue, Aug 1, 2017 at 10:38 PM, Shawn Pearcewrote: >> Peff and I discussed off-list whether the lookup-by-SHA-1 feature is >> so important in the first place. Currently, all references must be >> scanned for the advertisement anyway, > > Not really. You can hide refs and allow-tip-sha1 so clients can fetch > a ref even if it wasn't in the advertisement. We really want to use > that wire protocol capability with Gerrit Code Review to hide the > refs/changes/ namespace from the advertisement, but allow clients to > fetch any of those refs if they send its current SHA-1 in a want line > anyway. > > So a server could scan only the refs/{heads,tags}/ prefixes for the > advertisement, and then leverage the lookup-by-SHA1 to verify other > SHA-1s sent by the client. > >> so avoiding a second scan to vet >> SHA-1s received from the client is at best going to reduce the effort >> by a constant factor. Do you have numbers showing that this >> optimization is worth it? > > No, but I don't think I need to do much to prove it. My 866k ref > example advertisement right now is >62 MiB. If we do what I'm > suggesting in the paragraphs above, the advertisement is ~51 KiB. That being said, our bias towards minimizing the number of ref scans is rooted in our experience where scanning 866k refs takes 5 seconds to get the response from the storage backend into the git server. Cutting ref scans from 2 to 1 (or 1 to 0) is a big deal in that case. But that 5s number is based on our current, slow storage, not on reftable. If migrating to reftable turns each 5s scan into a 400ms scan, we might be able to live with that, even if we don't have fast lookup by SHA-1. >> OTOH a mythical protocol v2 might reduce the need to scan the >> references for advertisement, so maybe this optimization will be more >> helpful in the future? I haven't been following the status of the proposal, but I was assuming a client-speaks-first protocol would also imply the client asking for refnames, not SHA-1s, in which case lookup by SHA-1 is no longer relevant.
Re: reftable: new ref storage format
On Thu, Jul 13, 2017 at 8:11 PM, Shawn Pearcewrote: > In another (Gerrit Code Review), we disable reflogs for > the insane refs/changes/ namespace, as nearly every reference is > created once, and never modified. Apologies for the tangent, but this is not true in the most recent Gerrit implementation. We update refs/changes/CD/ABCD/1 and refs/changes/CD/ABCD/meta in a single BatchRefUpdate, and we set a reflog message on the BatchRefUpdate instance, which updates the reflog for all refs in the batch. The reflog message on /meta is important, and arguably it's useful to be able to correlate that with the reflog on /1. If you think storing reflogs on patch set refs is going to be a problem wrt on-disk storage, we should discuss this offline :)
Re: reftable: new ref storage format
On Sun, Jul 16, 2017 at 3:43 PM, Shawn Pearcewrote: > True... but... in my "android" example repository we have 866,456 live > refs. A block size of 64k needs only 443 blocks, and a 12k index, to > get the file to compress to 28M (vs. 62M packed-refs). > > Index records are averaging 28 bytes per block. That gives us room for > about 1955 blocks, or 4,574,700 refs before the index block exceeds > 64k. That's only a 5x increase over the current number of refs in this android repo. I would not be so sure this repo doesn't grow another 5x in the next few years. Especially as the other optimizations for working with large repos start to be applied, so it won't be prohibitively painful to work with such a repo. Are we ok with increasing the block size when this eventually happens? (At least I think that's what we would have to do, I haven't been following closely the discussion on scaling limits.)
Re: reftable [v4]: new ref storage format
On Sun, Jul 30, 2017 at 11:51 PM, Shawn Pearcewrote: > - Ref-like files (FETCH_HEAD, MERGE_HEAD) also use type 0x3. > - Combine reflog storage with ref storage for small transactions. > - Separate reflog storage for base refs and historical logs. How is the stash implemented in reftable? In particular, "git stash drop" needs to be able to remove an arbitrary single entry from a reflog. I don't think the current proposal supports writing tombstones for reflog entries, so this maybe implies that "stash drop" would have to be implemented by rewriting the whole reflog for the stash ref during compaction. Can the current compaction algorithm support this? I suppose there are more exotic alternatives: * Go back to the normal ref(log) format for refs/stash. I figure you probably don't want to do this, given that you already moved other ref-like files into the reftable in a later revision of this proposal. * Implement the whole stash storage/command in some other way that doesn't depend on reflog.
Re: [PATCH v2] completion: reduce overhead of clearing cached --options
On Thu, Jun 7, 2018 at 1:48 AM Jonathan Nieder wrote: > Whatever > I try, I end up with one of two results: either it uses zsh's standard > completion, or it spews a stream of errors about missing functions > like compgen. What am I doing wrong? Try adding to the top of your .zshrc: autoload -U compinit; compinit autoload -U bashcompinit; bashcompinit Those are both in my .zshrc, and I think they are sufficient magic to define compgen.
[PATCH] config.txt: Document behavior of backslashes in subsections
Unrecognized escape sequences are invalid in values: $ git config -f - --list < --- Documentation/config.txt | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index b18c0f97fe..f772186c44 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -41,11 +41,13 @@ in the section header, like in the example below: Subsection names are case sensitive and can contain any characters except -newline (doublequote `"` and backslash can be included by escaping them -as `\"` and `\\`, respectively). Section headers cannot span multiple -lines. Variables may belong directly to a section or to a given subsection. -You can have `[section]` if you have `[section "subsection"]`, but you -don't need to. +newline and the null byte. Doublequote `"` and backslash can be included +by escaping them as `\"` and `\\`, respectively. Backslashes preceding +other characters are dropped when reading; for example, `\t` is read as +`t` and `\0` is read as `0` Section headers cannot span multiple lines. +Variables may belong directly to a section or to a given subsection. You +can have `[section]` if you have `[section "subsection"]`, but you don't +need to. There is also a deprecated `[section.subsection]` syntax. With this syntax, the subsection name is converted to lower-case and is also -- 2.15.1.620.gb9897f4670-goog