Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Dave Borowitz]

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

[Dave Borowitz]

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

Re: reftable [v4]: new ref storage format

[Dave Borowitz]

On Tue, Aug 1, 2017 at 10:38 PM, Shawn Pearce  wrote:
>> 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 [v4]: new ref storage format

[Dave Borowitz]

On Sun, Jul 30, 2017 at 11:51 PM, Shawn Pearce  wrote:
> - 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: reftable [v4]: new ref storage format

[Dave Borowitz]

On Sun, Jul 30, 2017 at 11:51 PM, Shawn Pearce  wrote:
> - 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: new ref storage format

[Dave Borowitz]

On Sun, Jul 16, 2017 at 3:43 PM, Shawn Pearce  wrote:
> 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: new ref storage format

[Dave Borowitz]

On Thu, Jul 13, 2017 at 8:11 PM, Shawn Pearce  wrote:
> 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: RefTree: Alternate ref backend

[Dave Borowitz]

On Tue, Dec 22, 2015 at 8:11 AM, Shawn Pearce  wrote:
> 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?

[Dave Borowitz]

On Tue, Dec 22, 2015 at 7:39 AM, Paul Smith  wrote:
> 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: [PATCH 0/2] git-candidate: git based patch tracking and review

[Dave Borowitz]

On Tue, Dec 1, 2015 at 3:55 PM, Jonathan Nieder  wrote:
> 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

[PATCH v2 7/9] builtin/send-pack.c: Use option parsing API

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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 1/7] Documentation/git-push.txt: Document when --signed may fail

[Dave Borowitz]

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

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

Re: Thoughts on refactoring the transport (+helper) code

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

---
 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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

Thoughts on refactoring the transport (+helper) code

[Dave Borowitz]

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

Bug? git config and --unset are not inverses

[Dave Borowitz]

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: Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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 3/7] pack-protocol.txt: Mark all LFs in push-cert as required

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[PATCH 5/7] pack-protocol.txt: Be more precise about pusher-key relationship

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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: Git + SFC Status Update

[Dave Borowitz]

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

[Dave Borowitz]

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

Re: [PATCH/RFC] Makefile: do not depend on curl-config

[Dave Borowitz]

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: git version 1.9.0 missing git-http-push?

[Dave Borowitz]

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

[Dave Borowitz]

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?

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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 1/2] Makefile: use curl-config to determine curl flags

[Dave Borowitz]

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

[Dave Borowitz]

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

[Dave Borowitz]

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

[PATCH 2/2] Makefile: allow static linking against libcurl

[Dave Borowitz]

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

[Dave Borowitz]

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 0/4] jk/version-string and google code

[Dave Borowitz]

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