Git development
 help / color / mirror / Atom feed
* [PATCH v3 2/4] Split GPG interface into its own helper library
From: Junio C Hamano @ 2011-09-09 20:41 UTC (permalink / raw)
  To: git
In-Reply-To: <1315600904-17032-1-git-send-email-gitster@pobox.com>

This mostly moves existing code from builtin/tag.c (for signing)
and builtin/verify-tag.c (for verifying) to a new gpg-interface.c
file to provide a more generic library interface.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile             |    2 +
 builtin/tag.c        |   60 ++++----------------------------
 builtin/verify-tag.c |   35 ++-----------------
 gpg-interface.c      |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gpg-interface.h      |   11 ++++++
 5 files changed, 117 insertions(+), 85 deletions(-)
 create mode 100644 gpg-interface.c
 create mode 100644 gpg-interface.h

diff --git a/Makefile b/Makefile
index 8d6d451..2183223 100644
--- a/Makefile
+++ b/Makefile
@@ -530,6 +530,7 @@ LIB_H += exec_cmd.h
 LIB_H += fsck.h
 LIB_H += gettext.h
 LIB_H += git-compat-util.h
+LIB_H += gpg-interface.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
@@ -620,6 +621,7 @@ LIB_OBJS += entry.o
 LIB_OBJS += environment.o
 LIB_OBJS += exec_cmd.o
 LIB_OBJS += fsck.o
+LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hash.o
diff --git a/builtin/tag.c b/builtin/tag.c
index 667515e..e9d36fa 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -14,6 +14,7 @@
 #include "parse-options.h"
 #include "diff.h"
 #include "revision.h"
+#include "gpg-interface.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
@@ -208,60 +209,13 @@ static int verify_tag(const char *name, const char *ref,
 
 static int do_sign(struct strbuf *buffer)
 {
-	struct child_process gpg;
-	const char *args[4];
-	char *bracket;
-	int len;
-	int i, j;
+	const char *key;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
-			return error(_("committer info too long."));
-		bracket = strchr(signingkey, '>');
-		if (bracket)
-			bracket[1] = '\0';
-	}
-
-	/* When the username signingkey is bad, program could be terminated
-	 * because gpg exits without reading and then write gets SIGPIPE. */
-	signal(SIGPIPE, SIG_IGN);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args;
-	gpg.in = -1;
-	gpg.out = -1;
-	args[0] = "gpg";
-	args[1] = "-bsau";
-	args[2] = signingkey;
-	args[3] = NULL;
-
-	if (start_command(&gpg))
-		return error(_("could not run gpg."));
-
-	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
-		close(gpg.in);
-		close(gpg.out);
-		finish_command(&gpg);
-		return error(_("gpg did not accept the tag data"));
-	}
-	close(gpg.in);
-	len = strbuf_read(buffer, gpg.out, 1024);
-	close(gpg.out);
-
-	if (finish_command(&gpg) || !len || len < 0)
-		return error(_("gpg failed to sign the tag"));
-
-	/* Strip CR from the line endings, in case we are on Windows. */
-	for (i = j = 0; i < buffer->len; i++)
-		if (buffer->buf[i] != '\r') {
-			if (i != j)
-				buffer->buf[j] = buffer->buf[i];
-			j++;
-		}
-	strbuf_setlen(buffer, j);
-
-	return 0;
+	if (*signingkey)
+		key = signingkey;
+	else
+		key = git_committer_info(IDENT_ERROR_ON_NO_NAME|IDENT_NO_DATE);
+	return sign_buffer(buffer, key);
 }
 
 static const char tag_template[] =
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 3134766..8b4f742 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -11,6 +11,7 @@
 #include "run-command.h"
 #include <signal.h>
 #include "parse-options.h"
+#include "gpg-interface.h"
 
 static const char * const verify_tag_usage[] = {
 		"git verify-tag [-v|--verbose] <tag>...",
@@ -19,42 +20,12 @@ static const char * const verify_tag_usage[] = {
 
 static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 {
-	struct child_process gpg;
-	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX];
-	size_t len;
-	int fd, ret;
+	int len;
 
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
-	if (fd < 0)
-		return error("could not create temporary file '%s': %s",
-						path, strerror(errno));
-	if (write_in_full(fd, buf, size) < 0)
-		return error("failed writing temporary file '%s': %s",
-						path, strerror(errno));
-	close(fd);
-
-	/* find the length without signature */
 	len = parse_signature(buf, size);
 	if (verbose)
 		write_in_full(1, buf, len);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args_gpg;
-	gpg.in = -1;
-	args_gpg[2] = path;
-	if (start_command(&gpg)) {
-		unlink(path);
-		return error("could not run gpg.");
-	}
-
-	write_in_full(gpg.in, buf, len);
-	close(gpg.in);
-	ret = finish_command(&gpg);
-
-	unlink_or_warn(path);
-
-	return ret;
+	return verify_signed_buffer(buf, size, len);
 }
 
 static int verify_tag(const char *name, int verbose)
diff --git a/gpg-interface.c b/gpg-interface.c
new file mode 100644
index 0000000..b83cca1
--- /dev/null
+++ b/gpg-interface.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2011, Google Inc.
+ */
+#include "cache.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "gpg-interface.h"
+#include "sigchain.h"
+
+int sign_buffer(struct strbuf *buffer, const char *signing_key)
+{
+	struct child_process gpg;
+	const char *args[4];
+	ssize_t len;
+	int i, j;
+
+	memset(&gpg, 0, sizeof(gpg));
+	gpg.argv = args;
+	gpg.in = -1;
+	gpg.out = -1;
+	args[0] = "gpg";
+	args[1] = "-bsau";
+	args[2] = signing_key;
+	args[3] = NULL;
+
+	if (start_command(&gpg))
+		return error(_("could not run gpg."));
+
+	/*
+	 * When the username signingkey is bad, program could be terminated
+	 * because gpg exits without reading and then write gets SIGPIPE.
+	 */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
+		close(gpg.in);
+		close(gpg.out);
+		finish_command(&gpg);
+		return error(_("gpg did not accept the data"));
+	}
+	close(gpg.in);
+	len = strbuf_read(buffer, gpg.out, 1024);
+	close(gpg.out);
+
+	sigchain_pop(SIGPIPE);
+
+	if (finish_command(&gpg) || !len || len < 0)
+		return error(_("gpg failed to sign the data"));
+
+	/* Strip CR from the line endings, in case we are on Windows. */
+	for (i = j = 0; i < buffer->len; i++)
+		if (buffer->buf[i] != '\r') {
+			if (i != j)
+				buffer->buf[j] = buffer->buf[i];
+			j++;
+		}
+	strbuf_setlen(buffer, j);
+
+	return 0;
+}
+
+int verify_signed_buffer(const char *buf, size_t total, size_t payload)
+{
+	struct child_process gpg;
+	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+	char path[PATH_MAX];
+	int fd, ret;
+
+	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+	if (fd < 0)
+		return error("could not create temporary file '%s': %s",
+			     path, strerror(errno));
+	if (write_in_full(fd, buf, total) < 0)
+		return error("failed writing temporary file '%s': %s",
+			     path, strerror(errno));
+	close(fd);
+
+	memset(&gpg, 0, sizeof(gpg));
+	gpg.argv = args_gpg;
+	gpg.in = -1;
+	args_gpg[2] = path;
+	if (start_command(&gpg)) {
+		unlink(path);
+		return error("could not run gpg.");
+	}
+
+	write_in_full(gpg.in, buf, payload);
+	close(gpg.in);
+	ret = finish_command(&gpg);
+
+	unlink_or_warn(path);
+
+	return ret;
+}
diff --git a/gpg-interface.h b/gpg-interface.h
new file mode 100644
index 0000000..7689357
--- /dev/null
+++ b/gpg-interface.h
@@ -0,0 +1,11 @@
+#ifndef GPG_INTERFACE_H
+#define GPG_INTERFACE_H
+
+/*
+ * Copyright (c) 2011, Google Inc.
+ */
+
+extern int sign_buffer(struct strbuf *buffer, const char *signing_key);
+extern int verify_signed_buffer(const char *buffer, size_t total, size_t payload);
+
+#endif
-- 
1.7.7.rc0.188.g3793ac

^ permalink raw reply related

* [PATCH v3 3/4] rename "match_refs()" to "match_push_refs()"
From: Junio C Hamano @ 2011-09-09 20:41 UTC (permalink / raw)
  To: git
In-Reply-To: <1315600904-17032-1-git-send-email-gitster@pobox.com>

Yes, there is a warning that says the function is only used by push in big
red letters in front of this function, but it didn't say a more important
thing it should have said: what the function is for and what it does.

Rename it and document it to avoid future confusion.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/remote.c    |    4 ++--
 builtin/send-pack.c |    2 +-
 http-push.c         |    4 ++--
 remote.c            |   13 ++++++++-----
 remote.h            |    4 ++--
 transport.c         |    4 ++--
 6 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index f2a9c26..f16b544 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -389,8 +389,8 @@ static int get_push_ref_states(const struct ref *remote_refs,
 	local_refs = get_local_heads();
 	push_map = copy_ref_list(remote_refs);
 
-	match_refs(local_refs, &push_map, remote->push_refspec_nr,
-		   remote->push_refspec, MATCH_REFS_NONE);
+	match_push_refs(local_refs, &push_map, remote->push_refspec_nr,
+			remote->push_refspec, MATCH_REFS_NONE);
 
 	states->push.strdup_strings = 1;
 	for (ref = push_map; ref; ref = ref->next) {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 87833f4..e0b8030 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -509,7 +509,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		flags |= MATCH_REFS_MIRROR;
 
 	/* match them up */
-	if (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
+	if (match_push_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;
 
 	set_ref_status_for_push(remote_refs, args.send_mirror,
diff --git a/http-push.c b/http-push.c
index 376331a..02f46a5 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1877,8 +1877,8 @@ int main(int argc, char **argv)
 	}
 
 	/* match them up */
-	if (match_refs(local_refs, &remote_refs,
-		       nr_refspec, (const char **) refspec, push_all)) {
+	if (match_push_refs(local_refs, &remote_refs,
+			    nr_refspec, (const char **) refspec, push_all)) {
 		rc = -1;
 		goto cleanup;
 	}
diff --git a/remote.c b/remote.c
index b8ecfa5..536ffa3 100644
--- a/remote.c
+++ b/remote.c
@@ -1170,12 +1170,15 @@ static struct ref **tail_ref(struct ref **head)
 }
 
 /*
- * Note. This is used only by "push"; refspec matching rules for
- * push and fetch are subtly different, so do not try to reuse it
- * without thinking.
+ * Given the set of refs the local repository has, the set of refs the
+ * remote repository has, and the refspec used for push, determine
+ * what remote refs we will update and with what value by setting
+ * peer_ref (which object is being pushed) and force (if the push is
+ * forced) in elements of "dst". The function may add new elements to
+ * dst (e.g. pushing to a new branch, done in match_explicit_refs).
  */
-int match_refs(struct ref *src, struct ref **dst,
-	       int nr_refspec, const char **refspec, int flags)
+int match_push_refs(struct ref *src, struct ref **dst,
+		    int nr_refspec, const char **refspec, int flags)
 {
 	struct refspec *rs;
 	int send_all = flags & MATCH_REFS_ALL;
diff --git a/remote.h b/remote.h
index 9a30a9d..6729477 100644
--- a/remote.h
+++ b/remote.h
@@ -96,8 +96,8 @@ void free_refspec(int nr_refspec, struct refspec *refspec);
 char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
 		     const char *name);
 
-int match_refs(struct ref *src, struct ref **dst,
-	       int nr_refspec, const char **refspec, int all);
+int match_push_refs(struct ref *src, struct ref **dst,
+		    int nr_refspec, const char **refspec, int all);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	int force_update);
 
diff --git a/transport.c b/transport.c
index fa279d5..740a739 100644
--- a/transport.c
+++ b/transport.c
@@ -1033,8 +1033,8 @@ int transport_push(struct transport *transport,
 		if (flags & TRANSPORT_PUSH_MIRROR)
 			match_flags |= MATCH_REFS_MIRROR;
 
-		if (match_refs(local_refs, &remote_refs,
-			       refspec_nr, refspec, match_flags)) {
+		if (match_push_refs(local_refs, &remote_refs,
+				    refspec_nr, refspec, match_flags)) {
 			return -1;
 		}
 
-- 
1.7.7.rc0.188.g3793ac

^ permalink raw reply related

* [PATCH v3 0/4] Signed push
From: Junio C Hamano @ 2011-09-09 20:41 UTC (permalink / raw)
  To: git
In-Reply-To: <1315512102-19022-1-git-send-email-gitster@pobox.com>

This is based an alternative design we discussed on the list to prepare
and push the notes tree on the sending side.

Junio C Hamano (4):
  send-pack: typofix error message
  Split GPG interface into its own helper library
  rename "match_refs()" to "match_push_refs()"
  push -s: signed push

 Makefile             |    2 +
 builtin/push.c       |    1 +
 builtin/remote.c     |    4 +-
 builtin/send-pack.c  |    4 +-
 builtin/tag.c        |   60 +++---------------------
 builtin/verify-tag.c |   35 +------------
 gpg-interface.c      |   94 ++++++++++++++++++++++++++++++++++++
 gpg-interface.h      |   11 ++++
 http-push.c          |    4 +-
 notes.c              |   16 ++++++
 notes.h              |    2 +
 remote.c             |   13 +++--
 remote.h             |    4 +-
 transport.c          |  128 +++++++++++++++++++++++++++++++++++++++++++++++++-
 transport.h          |    5 ++
 15 files changed, 283 insertions(+), 100 deletions(-)
 create mode 100644 gpg-interface.c
 create mode 100644 gpg-interface.h

-- 
1.7.7.rc0.188.g3793ac

^ permalink raw reply

* [PATCH v3 1/4] send-pack: typofix error message
From: Junio C Hamano @ 2011-09-09 20:41 UTC (permalink / raw)
  To: git
In-Reply-To: <1315600904-17032-1-git-send-email-gitster@pobox.com>

The message identifies the process as receive-pack when it cannot fork the
sideband demultiplexer. We are actually a send-pack.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/send-pack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c1f6ddd..87833f4 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -334,7 +334,7 @@ int send_pack(struct send_pack_args *args,
 		demux.data = fd;
 		demux.out = -1;
 		if (start_async(&demux))
-			die("receive-pack: unable to fork off sideband demultiplexer");
+			die("send-pack: unable to fork off sideband demultiplexer");
 		in = demux.out;
 	}
 
-- 
1.7.7.rc0.188.g3793ac

^ permalink raw reply related

* git checkout under 1.7.6 does not properly list untracked files and aborts
From: Joshua Jensen @ 2011-09-09 20:04 UTC (permalink / raw)
  To: git@vger.kernel.org

This may be an msysGit 1.7.6 issue, as that is what I am using.  It also 
occurs in msysGit 1.7.5, but I am almost certain it did not happen in 
msysGit 1.7.2.

Given an untracked file in the working directory that has been added to 
an alternate branch, when switching to that alternate branch, 'git 
checkout' exits with an error code but does not print anything to the 
console.  It should print an untracked file error.

I have been trying to track this down in code.  The point where the 
error messages are printed, display_error_msgs, is never hit.

I don't have a debugger to trace this on Windows, so I've just been 
adding lots of debug output.  So far, I have not found the catalyst.

I am out on vacation now for the next week and a half.  However, I 
thought I would send this along to see if someone else can confirm the 
the problem.  I'll continue investigation when I'm back.

Thanks.

Josh

^ permalink raw reply

* Re: [PATCH] t3200: test branch creation with -v
From: Jeff King @ 2011-09-09 19:45 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <20110909194357.GA31446@sigill.intra.peff.net>

On Fri, Sep 09, 2011 at 03:43:57PM -0400, Jeff King wrote:

> On Fri, Sep 09, 2011 at 09:40:59PM +0200, Michael J Gruber wrote:
> 
> > +test_expect_success 'git branch -v t should work' '
> > +	git branch -v t &&
> > +	test .git/refs/heads/t &&
> 
> test -f ?

Hmm, this also seems to be a problem in the other tests, too.

-Peff

^ permalink raw reply

* Re: [PATCH] t3200: test branch creation with -v
From: Jeff King @ 2011-09-09 19:43 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <49578782dd114220aa2562b5bd29755fc2bdd0fa.1315597137.git.git@drmicha.warpmail.net>

On Fri, Sep 09, 2011 at 09:40:59PM +0200, Michael J Gruber wrote:

> +test_expect_success 'git branch -v t should work' '
> +	git branch -v t &&
> +	test .git/refs/heads/t &&

test -f ?

Also, don't we have test_path_is_file which yields slightly prettier
output (and maybe some portability benefits; I don't remember)?

> +	git branch -d t &&
> +	test ! -f .git/refs/heads/t

Ditto for 'test_path_is_missing' here.

-Peff

^ permalink raw reply

* [PATCH] t3200: test branch creation with -v
From: Michael J Gruber @ 2011-09-09 19:40 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20110909193033.GA31184@sigill.intra.peff.net>

Make sure that "git branch -v t" creates branch "t".

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t3200-branch.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index c466b20..8381f0c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -105,6 +105,13 @@ test_expect_success 'git branch -v -d t should work' '
 	test ! -f .git/refs/heads/t
 '
 
+test_expect_success 'git branch -v t should work' '
+	git branch -v t &&
+	test .git/refs/heads/t &&
+	git branch -d t &&
+	test ! -f .git/refs/heads/t
+'
+
 test_expect_success 'git branch -v -m t s should work' '
 	git branch t &&
 	test .git/refs/heads/t &&
-- 
1.7.7.rc0.469.g9eb94

^ permalink raw reply related

* Re: Rebase & Trailing Whitespace
From: Jeff King @ 2011-09-09 19:38 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Michael J Gruber, Philip Oakley, Hilco Wijbenga, Git Users
In-Reply-To: <20110903201832.GB8362@ecki>

On Sat, Sep 03, 2011 at 10:18:32PM +0200, Clemens Buchacher wrote:

> I am looking at the output of:
> 
>  git diff --check 4b825 -- $(git ls-files '*.[ch]'|grep -v ^compat)
> 
> Most of those are false positives from multiline function argument
> lists, which are aligned with the first line. So the above command
> would break more than it would fix.
> 
> I don't care either way, but for future reference: Do we or do we
> not want indentation with spaces in this case?

I thought our policy was that a tab is 8 spaces, and that indentations
of size N should be floor(N/8) tabs and (N%8) spaces. I know other
projects disagree (e.g., they want tabs for structural indentation and
spaces for all other alignment), but that seems to be the general use in
git.

-Peff

^ permalink raw reply

* Re: [PATCHv2 4/5] branch: introduce --list option
From: Jeff King @ 2011-09-09 19:30 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Junio C Hamano, git
In-Reply-To: <4E6A691C.2060803@drmicha.warpmail.net>

On Fri, Sep 09, 2011 at 09:29:32PM +0200, Michael J Gruber wrote:

> > I recall Peff had some comments on your new tests last night, by the way.
> 
> Yes, we have tests (now) for "-v -d", "-v -m", "-v branch*" and
> combinations with --list. Testing "-v foo" for creation would be good
> (and belong into t3200). Is there anything to amend?

I think my comment was just to add the "-v foo" test.

-Peff

^ permalink raw reply

* Re: [PATCHv2 4/5] branch: introduce --list option
From: Michael J Gruber @ 2011-09-09 19:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vlitxvh45.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 09.09.2011 18:02:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 08.09.2011 23:17:
>>> Ah, nevermind.
>>>
>>> As the series is already in 'next', here is what I came up with.
>>
>> I thought you'll rebuild next anyways after 1.7.7, but either way it's
>> fine. Thanks for holding this series in next long enough to really cook
>> it (and Jeff for revisiting it), it's much better now, keeping the
>> (undocumented, but expected) behavior of "git branch -v foo".
> 
> Thank *you* for all the work.
> 
> I recall Peff had some comments on your new tests last night, by the way.
> 

Yes, we have tests (now) for "-v -d", "-v -m", "-v branch*" and
combinations with --list. Testing "-v foo" for creation would be good
(and belong into t3200). Is there anything to amend?

Michael

^ permalink raw reply

* Re: can Git encrypt/decrypt .gpg on push/fetch?
From: Jeff King @ 2011-09-09 19:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael J Gruber, Aneesh Bhasin, tzz, git
In-Reply-To: <7vvct1tu3n.fsf@alter.siamese.dyndns.org>

On Fri, Sep 09, 2011 at 12:05:00PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> B) Keep blobs encrypted, checkout decrypted
> >> - Use Use "*.gpg filter=gpg" in your attributes and
> >> [filter "gpg"]
> >> 	smudge = gpg -d
> >> 	clean = gpg -e -r yourgpgkey
> >>   in your config.
> >> 
> >> I use A on a regular basis. B is untested (but patterned after a similar
> >> gzip filter I use). You may or may not have better results with "gpg -ea".
> >
> > Yeah, I think that would work but have never tried it either.
> 
> Unless "gpg -e" encrypts the same cleartext into the same cyphertext every
> time, the above "clean" filter probably wouldn't be very useful.

Ah, right, I remember now running into that at some point. You could get
around that by using a symmetric cipher in block mode, or with a
non-random IV, but then you're opening yourself up to some cryptanalytic
attacks.

-Peff

^ permalink raw reply

* Re: can Git encrypt/decrypt .gpg on push/fetch?
From: Michael J Gruber @ 2011-09-09 19:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Aneesh Bhasin, tzz, git
In-Reply-To: <7vvct1tu3n.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 09.09.2011 21:05:
> Jeff King <peff@peff.net> writes:
> 
>>> B) Keep blobs encrypted, checkout decrypted
>>> - Use Use "*.gpg filter=gpg" in your attributes and
>>> [filter "gpg"]
>>> 	smudge = gpg -d
>>> 	clean = gpg -e -r yourgpgkey
>>>   in your config.
>>>
>>> I use A on a regular basis. B is untested (but patterned after a similar
>>> gzip filter I use). You may or may not have better results with "gpg -ea".
>>
>> Yeah, I think that would work but have never tried it either.
> 
> Unless "gpg -e" encrypts the same cleartext into the same cyphertext every
> time, the above "clean" filter probably wouldn't be very useful.
> 

Uh, right, this would only make sense with specific versions of debian's
openssl then. Only that gpg does not use that ;)

I'm not sure whether "gpg --symmetric" has the same issue, but version
A) seemed better before that already.

Michael

^ permalink raw reply

* Re: [PATCH 2/2] push -s: skeleton
From: Jeff King @ 2011-09-09 19:12 UTC (permalink / raw)
  To: Joey Hess; +Cc: Git Mailing List
In-Reply-To: <20110909160301.GA9707@gnu.kitenet.net>

On Fri, Sep 09, 2011 at 12:03:01PM -0400, Joey Hess wrote:

> The most credible attack I have so far does not involve binary files in
> tree. Someone pointed out that git log, git show, etc stop printing
> commit messages at NULL.

It was me.

> It might be worth ameloriating that attack by making git log always
> show the full buffer. Or it would be easy to write a tool that finds
> any commits that have a NULL in their message.

Unfortunately, that is going to involve a pretty huge code audit of git,
as the "tack a \0 to the end of an object just in case" code dates back
quite a while (e871b64, unpack_sha1_file: zero-pad the unpacked
object, 2005-05-25). So I suspect there is a lot of code built on
top of the assumption that commit messages are NUL-terminated strings.

Besides which, that is only one form of hiding. If collision attacks
against sha1 become a possibility, I think we are better to talk about
moving to a new hash. Even sha-256 truncated to 160 bits would be better
than sha-1 (AFAIK, that family of SHA does not suffer from the same
attacks, so we would still be in the 2^80 range for collision attacks).

-Peff

^ permalink raw reply

* Re: RFD: leveraging GitHub's asciidoc rendering for our Documentation/
From: Jeff King @ 2011-09-09 19:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael J Gruber, Git Mailing List
In-Reply-To: <7v4o0lvcq3.fsf@alter.siamese.dyndns.org>

On Fri, Sep 09, 2011 at 10:37:24AM -0700, Junio C Hamano wrote:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> > Our own customisation is not loaded (of course) so that, e.g., the
> > linkgit macro does not work; and the include statement makes GitHub's
> > parser unhappy and choke.
> >
> > Does anybody feel this is worth pursuing?
> >
> > + Nicer blob view
> > + Simpler way to judge documentation changes
> > - Need to get our asciidoc config in there
> > - GitHub's parser neeeds to learn include
> 
> Personally I am not very interested. Couldn't you just visit the html
> branch instead for viewing?

I agree that it's not very nice for release-quality documentation. But
if I understand correctly, Michael means that you can go straight to any
blob at any commit of any fork on github, and get the rendered (or raw)
version. Which is neat.

How frequently do you build the html branch? I always assumed it was
once in a while (every push?), not for every commit.

We could help that along by giving our documentation a more descriptive
filename. But it would be cool if GitHub respected gitattributes for
this, too.

-Peff

^ permalink raw reply

* Re: RFD: leveraging GitHub's asciidoc rendering for our Documentation/
From: Michael J Gruber @ 2011-09-09 19:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7v4o0lvcq3.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 09.09.2011 19:37:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Our own customisation is not loaded (of course) so that, e.g., the
>> linkgit macro does not work; and the include statement makes GitHub's
>> parser unhappy and choke.
>>
>> Does anybody feel this is worth pursuing?
>>
>> + Nicer blob view
>> + Simpler way to judge documentation changes
>> - Need to get our asciidoc config in there
>> - GitHub's parser neeeds to learn include
> 
> Personally I am not very interested. Couldn't you just visit the html
> branch instead for viewing?

For viewing what you generated yes, of course. Though there's some
fiddling if you want to view a specific version. And even for the latest
version it is more direct to click on the file in the tree and see it
rendered.

But what I had in mind was being able to view anyone's branches like
that, to aid review of a patch which is on list and in a branch on
GitHub, say for restructuring layout or presentation of some man pages.
The linkgit and include issues are a bit of a showstopper (looks like
lots of digging in foreign code).

Michael

^ permalink raw reply

* Re: can Git encrypt/decrypt .gpg on push/fetch?
From: Junio C Hamano @ 2011-09-09 19:05 UTC (permalink / raw)
  To: Jeff King; +Cc: Michael J Gruber, Aneesh Bhasin, tzz, git
In-Reply-To: <20110909184229.GE28480@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> B) Keep blobs encrypted, checkout decrypted
>> - Use Use "*.gpg filter=gpg" in your attributes and
>> [filter "gpg"]
>> 	smudge = gpg -d
>> 	clean = gpg -e -r yourgpgkey
>>   in your config.
>> 
>> I use A on a regular basis. B is untested (but patterned after a similar
>> gzip filter I use). You may or may not have better results with "gpg -ea".
>
> Yeah, I think that would work but have never tried it either.

Unless "gpg -e" encrypts the same cleartext into the same cyphertext every
time, the above "clean" filter probably wouldn't be very useful.

^ permalink raw reply

* Re: can Git encrypt/decrypt .gpg on push/fetch?
From: Jeff King @ 2011-09-09 18:42 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Aneesh Bhasin, tzz, git
In-Reply-To: <4E6A165D.5010703@drmicha.warpmail.net>

On Fri, Sep 09, 2011 at 03:36:29PM +0200, Michael J Gruber wrote:

> A) Keep blobs and checkout encrypted
> - Use an editor which can encrypt/decrypt on the fly (e.g. vim)
> - Use "*.gpg diff=gpg" in your attributes and
> [diff "gpg"]
>         textconv = gpg -d
>   in your config to have cleartext diffs. Use cachetextconv with caution ;)

I use something like this for my password store, though I use:

  textconv = gpg -qd --no-tty

to keep things as clean as possible. Running gpg-agent is a must, of
course.

The wallet itself is just a gpg-encrypted YAML file, with a few scripts
grep within the hierarchy. I'm happy to share the code if anybody is
interested. I've also written firefox hooks to fill website form fields,
but that code is a little gross.

> B) Keep blobs encrypted, checkout decrypted
> - Use Use "*.gpg filter=gpg" in your attributes and
> [filter "gpg"]
> 	smudge = gpg -d
> 	clean = gpg -e -r yourgpgkey
>   in your config.
> 
> I use A on a regular basis. B is untested (but patterned after a similar
> gzip filter I use). You may or may not have better results with "gpg -ea".

Yeah, I think that would work but have never tried it either.

-Peff

^ permalink raw reply

* Re: The imporantance of including http credential caching in 1.7.7
From: Jeff King @ 2011-09-09 18:34 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Kyle Neath, git
In-Reply-To: <4E69C8F0.9070204@drmicha.warpmail.net>

On Fri, Sep 09, 2011 at 10:06:08AM +0200, Michael J Gruber wrote:

> > So I think the wheels have been turning on this for quite a while from
> > GitHub's perspective.
> 
> Thanks for clarifying. While it should make no difference for the
> acceptance of patches, it's great to see GitHub invest into scratching
> their Git itches, and thus contribute back. That's how open source works
> as a business model :)

Yes. I don't often enough mention how awesome GitHub is for funding me
and giving me a free hand to improve git. They're doing everything
right. So let me mention it here one more time. :)

> > In the meantime, the best thing we can do to push it forward is to write
> > helpers. I implemented some basic ones that should work anywhere, but
> > aren't as nice as integration with existing keychains. Some people are
> > working on Linux ones. The single best thing GitHub can do to push this
> > forward right now is to provide a well-written OS X Keychain helper, and
> > to provide feedback on whether git's end of the API is good enough.
> 
> ... and one for Git on Windows? It seems we're lacking both Win and OS X
> developers here.

I mentioned OS X because of Kyle's mention of the GitHub for Mac client.
But yes, I do think in the long term we want something similar on
Windows. GitHub recently hired a developer with some Windows experience;
I'll try to see if he's interested in writing a credential helper.

-Peff

^ permalink raw reply

* Re: The imporantance of including http credential caching in 1.7.7
From: Jeff King @ 2011-09-09 18:27 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: John Szakmeister, Kyle Neath, git
In-Reply-To: <4E69C8DC.7060008@drmicha.warpmail.net>

On Fri, Sep 09, 2011 at 10:05:48AM +0200, Michael J Gruber wrote:

> > Agreed. Anything harder than ssh keys is right out the window, because
> > they're always the alternative these people could be using (but can't or
> > don't want to).
> 
> Sue, the question was: What is easy enough? I hoped that people would be
> using gpg to check signed tags, and that there might be a simple,
> convenient gnupg installer for Win and Mac which ties into the
> respective wallet systems or provides one they use already.

I suspect most people aren't checking signed tags. And even if they did
have gpg installed, most people aren't going to want a new password
wallet.  They're going to want integration with what they're already
using.

Which isn't to say that a gpg-based wallet is wrong, it's just that I
don't think it's filling the role that really needs filled. If you want
to make such a wallet helper, you're welcome to. But it doesn't
necessarily need to be a part of git core, and if it's not, then maybe
it's worth looking at the zillion other password wallet programs that
exist.

FWIW, I keep my passwords in a gpg-encrypted file and wrote a 10-line
shell script helper to do lookups for git. :)

> > We could make our own gpg-based password wallet system, but I think it's
> > a really bad idea, for two reasons:
> > 
> >   1. It's reinventing the wheel. Which is bad enough as it is, but is
> >      doubly bad with security-related code, because it's very easy to
> >      screw something up when you're writing a lot of new code.
> 
> So please let's not deploy credential-store...

I'm tempted to agree. But I also think it represents a nice lowest
common denominator. No hassle, no setup, but no security either. And
there are situations where that's appropriate (e.g., for unattended
cron operation, it's not much different than an unencrypted ssh key on
disk). My compromise was to put a big warning at the top of the
documentation. Maybe that's not enough, though.

And as far as reinventing the wheel with security code, I don't think
git-credential-store counts. It's not secure at all, so there's very
little to screw up. :)

> On 1.+2.: The idea/hope was to use an existing wallet system which
> people use for gnupg already to store their passphrase. If that is not
> used then my suggestion does not help much (the issue of widespread
> deployment), though it still is a secure version of credential-store for
> those who want a desktop-independent secure credential store.

Yeah, if there is an existing wallet system based around gpg, then
absolutely there should be a helper for it. But I don't know that there
is such a widely deployed system. And the helper for it doesn't need to
ship with git-core; anybody who uses their wallet system is free to
write and distribute the helper.

-Peff

^ permalink raw reply

* [PATCH] fetch: skip on-demand checking when no submodules are configured
From: Jens Lehmann @ 2011-09-09 18:22 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Martin Fick, Heiko Voigt, Michael Haggerty

It makes no sense to do the - possibly very expensive - call to "rev-list
<new-ref-sha1> --not --all" in check_for_new_submodule_commits() when
there aren't any submodules configured.

Leave check_for_new_submodule_commits() early when no name <-> path
mappings for submodules are found in the configuration. To make that work
reading the configuration had to be moved further up in cmd_fetch(), as
doing that after the actual fetch of the superproject was too late.

Reported-by: Martin Fick <mfick@codeaurora.org>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

This achieves the first goal: Don't let people pay a performance penalty
when they don't even use submodules. On Michael's test repo from [1] the
time for a full fetch went down from 142 seconds (current master) to one
second which is - not surprisingly - the same as using current master
with the --no-recurse-submodules option.

Now back to the drawing board to fix the performance regression for those
people who are using submodules ...

[1] http://comments.gmane.org/gmane.comp.version-control.git/177103

 builtin/fetch.c |   15 +++++++++------
 submodule.c     |    4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 93c9938..e422ced 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -941,6 +941,15 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);

+	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
+		if (recurse_submodules_default) {
+			int arg = parse_fetch_recurse_submodules_arg("--recurse-submodules-default", recurse_submodules_default);
+			set_config_fetch_recurse_submodules(arg);
+		}
+		gitmodules_config();
+		git_config(submodule_config, NULL);
+	}
+
 	if (all) {
 		if (argc == 1)
 			die(_("fetch --all does not take a repository argument"));
@@ -976,12 +985,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
 		const char *options[10];
 		int num_options = 0;
-		if (recurse_submodules_default) {
-			int arg = parse_fetch_recurse_submodules_arg("--recurse-submodules-default", recurse_submodules_default);
-			set_config_fetch_recurse_submodules(arg);
-		}
-		gitmodules_config();
-		git_config(submodule_config, NULL);
 		add_options_to_argv(&num_options, options);
 		result = fetch_populated_submodules(num_options, options,
 						    submodule_prefix,
diff --git a/submodule.c b/submodule.c
index 7a76edf..ad86534 100644
--- a/submodule.c
+++ b/submodule.c
@@ -481,6 +481,10 @@ void check_for_new_submodule_commits(unsigned char new_sha1[20])
 	const char *argv[] = {NULL, NULL, "--not", "--all", NULL};
 	int argc = ARRAY_SIZE(argv) - 1;

+	/* No need to check if there are no submodules configured */
+	if (!config_name_for_path.nr)
+		return;
+
 	init_revisions(&rev, NULL);
 	argv[1] = xstrdup(sha1_to_hex(new_sha1));
 	setup_revisions(argc, argv, &rev, NULL);
-- 
1.7.7.rc0.189.gf9175

^ permalink raw reply related

* Re: [PATCH 0/6] Improved infrastructure for refname normalization
From: Junio C Hamano @ 2011-09-09 17:57 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: gitzilla, git, cmn
In-Reply-To: <4E6A31D1.5020404@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The library could do the normalization, but
>
> 1. It would probably cost a lot of redundant checks as reference names
> pass in and out of the library and back in again
>
> 2. Normalization requires copying or overwriting the incoming string, so
> each time a refname crosses the library perimeter there might have to be
> an extra memory allocation with the associated headaches of dealing with
> the ownership of the memory.
>
> 3. The library doesn't encapsulate all uses of reference names; for
> example, for_each_ref() invokes a callback function with the refname as
> an argument.  The callback function is free to do a strcmp() of the
> refname (normalized by the library) with some arbitrary string that it
> got from the command line.  Either the caller has to do the
> normalization itself (i.e., outside of the library) or the library has
> to learn how to do every possible filtering operation with refnames.

4. The caller needs to be corrected to pay attention to the normalization
the library did for it. Your code may use a string as a ref and then
create something based on the refname; illustrating with a fictitious
example:

	ref = make_branch_ref("refs/heads/%s", branch_name);
        update_ref(ref, sha1);
        write_log("created branch '%s'", branch_name);

Even though make_branch_ref() may have removed duplicated slashes from the
name in "branch_name" when it computed "ref", the log still will record
unnormalized name.

I think the callers need to be aware of the normalization in practice
anyway for this reason, and a good way forward is to give the callers a
library interface to do so. It might even make sense to make the other
parts of the API _reject_ unnormalized input to catch offending callers.

By the way, does this series introduce new infrastructure features that
can be reused in different areas, such as Hui's "alt_odb path
normalization" patch?

^ permalink raw reply

* Re: git-p4.skipSubmitEdit
From: L. A. Linden Levy @ 2011-09-09 17:52 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Vitor Antunes, git@vger.kernel.org
In-Reply-To: <4E6A514C.5080200@diamand.org>

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

I noticed that it only skipped the edit check. That is why I added the
skipSubmitEdit option. If they are both true then it never opens the
editor and never checks for an edit. Probably they should just be one
option. I think it should probably also be a command line option to skip
the editor.

- Alex

On Fri, 2011-09-09 at 13:47 -0400, Luke Diamand wrote:
> On 09/09/11 11:05, Vitor Antunes wrote:
> > L. A. Linden Levy<alevy<at>  mobitv.com>  writes:
> >
> >>
> >> Hi All,
> >>
> >> I have been using git-p4 for a while and it has allowed me to completely
> >> change the way I develop and still be able to use perforce which my
> >> company has for its main VCS. One thing that was driving me nuts was
> >> that "git p4 submit" cycles through all of my individual commits and
> >> asks me if I want to change them. The way I develop I often am checking
> >> in 20 to 50 different small commits each with a descriptive git comment.
> >> I felt like I was doing double duty by having emacs open on every commit
> >> into perforce. So I modified git-p4 to have an option to skip the
> >> editor. This option coupled with git-p4.skipSubmitEditCheck will make
> >> the submission non-interactive for "git p4 submit".
> >
> > Hi Loren,
> >
> > This option was already included in a recent commit. The name that was
> > used is "skipSubmitEditCheck". Please make sure you are using the most
> > recent version of the script.
> 
> I put that option in - glad it's of use!
> 
> That option actually just skips the check of 'did the user edit the 
> file'. git-p4 will still go ahead and bring up the file in the editor first.
> 
> I get around this myself by setting EDITOR=/bin/true. That works for me 
> because I'm only using it in a script.
> 
> But it's possible that an additional option would actually be useful.
> 
> 
> 
> >
> > But don't let this discourage you from submitting patches. Just makesure
> > you clone git's repository and apply your patch over "maint" or "master"
> > branches. For more details on how to submit patches you can read
> > Documentation/SubmittingPatches.
> >
> > Vitor
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Alex Linden Levy
Senior Software Engineer
MobiTV, Inc.
6425 Christie Avenue, 5th Floor, Emeryville, CA 94608
phone 510.450.5190 mobile 720.352.8394
email alevy@mobitv.com  web www.mobitv.com


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* Re: git repository size / compression
From: Andreas Krey @ 2011-09-09 17:49 UTC (permalink / raw)
  To: John Szakmeister; +Cc: Carlos Martín Nieto, neubyr, git
In-Reply-To: <CAEBDL5U5-1nBGbWtb6+CfBrESoy8+p0Qqw1t1n_5EKFmpq9NhA@mail.gmail.com>

On Fri, 09 Sep 2011 12:05:03 +0000, John Szakmeister wrote:
...
> will at times choose to self-compress the file, instead of doing a
> delta and compressing.  IIRC, there is some heuristics in there for
> determining when to do that, but I forget the exact method.

Don't know about the compression part, but subversion does a delta of the nth
version of a file (not the global revision number n) against the version m, where
m is (n & (n-1)), or the least significant '1' bit flipped to '0'. That way, there
are only O(log(n)) instead of O(n) deltas to apply to get at a specific version.

[Was on the svn users list just then. They described it differently,
 but in essence it's that.]

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

^ permalink raw reply

* Re: git-p4.skipSubmitEdit
From: Luke Diamand @ 2011-09-09 17:47 UTC (permalink / raw)
  To: Vitor Antunes; +Cc: git, alevy
In-Reply-To: <loom.20110909T115356-849@post.gmane.org>

On 09/09/11 11:05, Vitor Antunes wrote:
> L. A. Linden Levy<alevy<at>  mobitv.com>  writes:
>
>>
>> Hi All,
>>
>> I have been using git-p4 for a while and it has allowed me to completely
>> change the way I develop and still be able to use perforce which my
>> company has for its main VCS. One thing that was driving me nuts was
>> that "git p4 submit" cycles through all of my individual commits and
>> asks me if I want to change them. The way I develop I often am checking
>> in 20 to 50 different small commits each with a descriptive git comment.
>> I felt like I was doing double duty by having emacs open on every commit
>> into perforce. So I modified git-p4 to have an option to skip the
>> editor. This option coupled with git-p4.skipSubmitEditCheck will make
>> the submission non-interactive for "git p4 submit".
>
> Hi Loren,
>
> This option was already included in a recent commit. The name that was
> used is "skipSubmitEditCheck". Please make sure you are using the most
> recent version of the script.

I put that option in - glad it's of use!

That option actually just skips the check of 'did the user edit the 
file'. git-p4 will still go ahead and bring up the file in the editor first.

I get around this myself by setting EDITOR=/bin/true. That works for me 
because I'm only using it in a script.

But it's possible that an additional option would actually be useful.



>
> But don't let this discourage you from submitting patches. Just makesure
> you clone git's repository and apply your patch over "maint" or "master"
> branches. For more details on how to submit patches you can read
> Documentation/SubmittingPatches.
>
> Vitor
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox