Git development
 help / color / mirror / Atom feed
* Re: [PATCH] write first for-merge ref to FETCH_HEAD first
From: Junio C Hamano @ 2012-01-04  0:12 UTC (permalink / raw)
  To: Joey Hess; +Cc: Junio C Hamano, git
In-Reply-To: <20120104000339.GA22662@gnu.kitenet.net>

Joey Hess <joey@kitenet.net> writes:

> Junio C Hamano wrote:
>> Ping? I think this is a good change to go in before 1.7.9.
>> 
>> The change broke quite a lot of tests, and I think I've fixed them all.
>
> Signed-off-by: Joey Hess <joey@kitenet.net>
>
> (Hope that's enough!)

Yes. Thanks.

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Junio C Hamano @ 2012-01-04  0:12 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski
In-Reply-To: <4F038E49.9080809@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> git-svn reads usernames and other user queries from an interactive
> terminal. This cause GUIs (w/o STDIN connected) to hang waiting forever
> for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
> git-core already asks for username using *_ASKPASS tools, this commit
> also enables git-svn to do so.
>
> This change extends the Git::prompt method, so that it can also be used
> for non password queries (e.g. usernames), and makes use of it instead
> of using hand-rolled prompt-response code that only works with the
> interactive terminal.

Now "prompt" is no longer a method but is merely a helper function, so
I've queued this (and 1/2 rewrite we discussed in a separate thread) to
'pu' after rewording the commit log message.

Thanks.

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Junio C Hamano @ 2012-01-04  0:10 UTC (permalink / raw)
  To: Sven Strickroth
  Cc: Ævar Arnfjörð Bjarmason, git, Jakub Narebski,
	Jeff King
In-Reply-To: <4F038EC8.505@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> Am 03.01.2012 23:51 schrieb Junio C Hamano:
>> Sven, does it look agreeable? And more importantly, does it still work? ;-)
>
> Works for me :)
>
> I also updated my second patch minutes ago to fit onto the new patch
> (w/o the filename stuff).

Thanks.

For the second patch, I have a feeling that Peff's earlier suggestion to
give precedence to the terminal interaction over SSH_ASKPASS iff we can
open terminal, but I think the first one is OK for 1.7.9.

I'll queue both of them in 'pu' for now just in case others spot silly
mistakes I made while rewriting the first one, though.

^ permalink raw reply

* Re: [PATCH] write first for-merge ref to FETCH_HEAD first
From: Joey Hess @ 2012-01-04  0:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h18s5kg.fsf@alter.siamese.dyndns.org>

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

Junio C Hamano wrote:
> Ping? I think this is a good change to go in before 1.7.9.
> 
> The change broke quite a lot of tests, and I think I've fixed them all.

Signed-off-by: Joey Hess <joey@kitenet.net>

(Hope that's enough!)

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply

* Re: [PATCH] write first for-merge ref to FETCH_HEAD first
From: Junio C Hamano @ 2012-01-03 23:57 UTC (permalink / raw)
  To: Joey Hess; +Cc: git
In-Reply-To: <7v1urp97mp.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Joey Hess <joey@kitenet.net> writes:
>
>> The FETCH_HEAD refname is supposed to refer to the ref that was fetched
>> and should be merged. However all fetched refs are written to
>> .git/FETCH_HEAD in an arbitrary order, and resolve_ref_unsafe simply
>> takes the first ref as the FETCH_HEAD, which is often the wrong one,
>> when other branches were also fetched.
>>
>> The solution is to write the for-merge ref(s) to FETCH_HEAD first.
>> ...
>
> Sign-off?

Ping? I think this is a good change to go in before 1.7.9.

The change broke quite a lot of tests, and I think I've fixed them all.

-- >8 --
From: Joey Hess <joey@kitenet.net>
Date: Mon, 26 Dec 2011 12:16:56 -0400
Subject: [PATCH] write first for-merge ref to FETCH_HEAD first

The FETCH_HEAD refname is supposed to refer to the ref that was fetched
and should be merged. However all fetched refs are written to
.git/FETCH_HEAD in an arbitrary order, and resolve_ref_unsafe simply
takes the first ref as the FETCH_HEAD, which is often the wrong one,
when other branches were also fetched.

The solution is to write the for-merge ref(s) to FETCH_HEAD first.
Then, unless --append is used, the FETCH_HEAD refname behaves as intended.
If the user uses --append, they presumably are doing so in order to
preserve the old FETCH_HEAD.

While we are at it, update an old example in the read-tree documentation
that implied that each entry in FETCH_HEAD only has the object name, which
is not true for quite a while.
---
 Documentation/git-read-tree.txt                    |    2 +-
 builtin/fetch.c                                    |  160 +++++++++++---------
 t/t5510-fetch.sh                                   |    2 +-
 t/t5515/fetch.br-branches-default-merge            |    2 +-
 ...etch.br-branches-default-merge_branches-default |    2 +-
 t/t5515/fetch.br-branches-default-octopus          |    2 +-
 ...ch.br-branches-default-octopus_branches-default |    2 +-
 t/t5515/fetch.br-branches-one-merge                |    2 +-
 t/t5515/fetch.br-branches-one-merge_branches-one   |    2 +-
 t/t5515/fetch.br-config-explicit-merge             |    2 +-
 .../fetch.br-config-explicit-merge_config-explicit |    2 +-
 t/t5515/fetch.br-config-explicit-octopus           |    2 +-
 ...etch.br-config-explicit-octopus_config-explicit |    2 +-
 t/t5515/fetch.br-config-glob-merge                 |    2 +-
 t/t5515/fetch.br-config-glob-merge_config-glob     |    2 +-
 t/t5515/fetch.br-config-glob-octopus               |    4 +-
 t/t5515/fetch.br-config-glob-octopus_config-glob   |    4 +-
 t/t5515/fetch.br-remote-explicit-merge             |    2 +-
 .../fetch.br-remote-explicit-merge_remote-explicit |    2 +-
 t/t5515/fetch.br-remote-explicit-octopus           |    2 +-
 ...etch.br-remote-explicit-octopus_remote-explicit |    2 +-
 t/t5515/fetch.br-remote-glob-merge                 |    2 +-
 t/t5515/fetch.br-remote-glob-merge_remote-glob     |    2 +-
 t/t5515/fetch.br-remote-glob-octopus               |    4 +-
 t/t5515/fetch.br-remote-glob-octopus_remote-glob   |    4 +-
 25 files changed, 114 insertions(+), 102 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 5375549..2d3ff23 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -342,7 +342,7 @@ since you pulled from him:
 
 ----------------
 $ git fetch git://.... linus
-$ LT=`cat .git/FETCH_HEAD`
+$ LT=`git rev-parse FETCH_HEAD`
 ----------------
 
 Your work tree is still based on your HEAD ($JC), but you have
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 33ad3aa..0481c16 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -377,6 +377,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	const char *what, *kind;
 	struct ref *rm;
 	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	int want_merge;
 
 	fp = fopen(filename, "a");
 	if (!fp)
@@ -393,84 +394,95 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		goto abort;
 	}
 
-	for (rm = ref_map; rm; rm = rm->next) {
-		struct ref *ref = NULL;
-
-		if (rm->peer_ref) {
-			ref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);
-			strcpy(ref->name, rm->peer_ref->name);
-			hashcpy(ref->old_sha1, rm->peer_ref->old_sha1);
-			hashcpy(ref->new_sha1, rm->old_sha1);
-			ref->force = rm->peer_ref->force;
-		}
+	/*
+	 * The first pass writes objects to be merged and then the
+	 * second pass writes the rest, in order to allow using
+	 * FETCH_HEAD as a refname to refer to the ref to be merged.
+	 */
+	for (want_merge = 1; 0 <= want_merge; want_merge--) {
+		for (rm = ref_map; rm; rm = rm->next) {
+			struct ref *ref = NULL;
+
+			commit = lookup_commit_reference_gently(rm->old_sha1, 1);
+			if (!commit)
+				rm->merge = 0;
+
+			if (rm->merge != want_merge)
+				continue;
+
+			if (rm->peer_ref) {
+				ref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);
+				strcpy(ref->name, rm->peer_ref->name);
+				hashcpy(ref->old_sha1, rm->peer_ref->old_sha1);
+				hashcpy(ref->new_sha1, rm->old_sha1);
+				ref->force = rm->peer_ref->force;
+			}
 
-		commit = lookup_commit_reference_gently(rm->old_sha1, 1);
-		if (!commit)
-			rm->merge = 0;
 
-		if (!strcmp(rm->name, "HEAD")) {
-			kind = "";
-			what = "";
-		}
-		else if (!prefixcmp(rm->name, "refs/heads/")) {
-			kind = "branch";
-			what = rm->name + 11;
-		}
-		else if (!prefixcmp(rm->name, "refs/tags/")) {
-			kind = "tag";
-			what = rm->name + 10;
-		}
-		else if (!prefixcmp(rm->name, "refs/remotes/")) {
-			kind = "remote-tracking branch";
-			what = rm->name + 13;
-		}
-		else {
-			kind = "";
-			what = rm->name;
-		}
+			if (!strcmp(rm->name, "HEAD")) {
+				kind = "";
+				what = "";
+			}
+			else if (!prefixcmp(rm->name, "refs/heads/")) {
+				kind = "branch";
+				what = rm->name + 11;
+			}
+			else if (!prefixcmp(rm->name, "refs/tags/")) {
+				kind = "tag";
+				what = rm->name + 10;
+			}
+			else if (!prefixcmp(rm->name, "refs/remotes/")) {
+				kind = "remote-tracking branch";
+				what = rm->name + 13;
+			}
+			else {
+				kind = "";
+				what = rm->name;
+			}
 
-		url_len = strlen(url);
-		for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-			;
-		url_len = i + 1;
-		if (4 < i && !strncmp(".git", url + i - 3, 4))
-			url_len = i - 3;
-
-		strbuf_reset(&note);
-		if (*what) {
-			if (*kind)
-				strbuf_addf(&note, "%s ", kind);
-			strbuf_addf(&note, "'%s' of ", what);
-		}
-		fprintf(fp, "%s\t%s\t%s",
-			sha1_to_hex(rm->old_sha1),
-			rm->merge ? "" : "not-for-merge",
-			note.buf);
-		for (i = 0; i < url_len; ++i)
-			if ('\n' == url[i])
-				fputs("\\n", fp);
-			else
-				fputc(url[i], fp);
-		fputc('\n', fp);
-
-		strbuf_reset(&note);
-		if (ref) {
-			rc |= update_local_ref(ref, what, &note);
-			free(ref);
-		} else
-			strbuf_addf(&note, "* %-*s %-*s -> FETCH_HEAD",
-				    TRANSPORT_SUMMARY_WIDTH,
-				    *kind ? kind : "branch",
-				    REFCOL_WIDTH,
-				    *what ? what : "HEAD");
-		if (note.len) {
-			if (verbosity >= 0 && !shown_url) {
-				fprintf(stderr, _("From %.*s\n"),
-						url_len, url);
-				shown_url = 1;
+			url_len = strlen(url);
+			for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+				;
+			url_len = i + 1;
+			if (4 < i && !strncmp(".git", url + i - 3, 4))
+				url_len = i - 3;
+
+			strbuf_reset(&note);
+			if (*what) {
+				if (*kind)
+					strbuf_addf(&note, "%s ", kind);
+				strbuf_addf(&note, "'%s' of ", what);
+			}
+			fprintf(fp, "%s\t%s\t%s",
+				sha1_to_hex(rm->old_sha1),
+				rm->merge ? "" : "not-for-merge",
+				note.buf);
+			for (i = 0; i < url_len; ++i)
+				if ('\n' == url[i])
+					fputs("\\n", fp);
+				else
+					fputc(url[i], fp);
+			fputc('\n', fp);
+
+			strbuf_reset(&note);
+			if (ref) {
+				rc |= update_local_ref(ref, what, &note);
+				free(ref);
+			} else
+				strbuf_addf(&note, "* %-*s %-*s -> FETCH_HEAD",
+					    TRANSPORT_SUMMARY_WIDTH,
+					    *kind ? kind : "branch",
+					    REFCOL_WIDTH,
+					    *what ? what : "HEAD");
+			if (note.len) {
+				if (verbosity >= 0 && !shown_url) {
+					fprintf(stderr, _("From %.*s\n"),
+							url_len, url);
+					shown_url = 1;
+				}
+				if (verbosity >= 0)
+					fprintf(stderr, " %s\n", note.buf);
 			}
-			if (verbosity >= 0)
-				fprintf(stderr, " %s\n", note.buf);
 		}
 	}
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e88dbd5..79ee913 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -70,8 +70,8 @@ test_expect_success "fetch test for-merge" '
 	master_in_two=`cd ../two && git rev-parse master` &&
 	one_in_two=`cd ../two && git rev-parse one` &&
 	{
-		echo "$master_in_two	not-for-merge"
 		echo "$one_in_two	"
+		echo "$master_in_two	not-for-merge"
 	} >expected &&
 	cut -f -2 .git/FETCH_HEAD >actual &&
 	test_cmp expected actual'
diff --git a/t/t5515/fetch.br-branches-default-merge b/t/t5515/fetch.br-branches-default-merge
index e3a41ae..12ab08e 100644
--- a/t/t5515/fetch.br-branches-default-merge
+++ b/t/t5515/fetch.br-branches-default-merge
@@ -1,6 +1,6 @@
 # br-branches-default-merge
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-merge_branches-default b/t/t5515/fetch.br-branches-default-merge_branches-default
index 1f60561..5442752 100644
--- a/t/t5515/fetch.br-branches-default-merge_branches-default
+++ b/t/t5515/fetch.br-branches-default-merge_branches-default
@@ -1,6 +1,6 @@
 # br-branches-default-merge branches-default
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-octopus b/t/t5515/fetch.br-branches-default-octopus
index f31e1b3..498a761 100644
--- a/t/t5515/fetch.br-branches-default-octopus
+++ b/t/t5515/fetch.br-branches-default-octopus
@@ -1,7 +1,7 @@
 # br-branches-default-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-octopus_branches-default b/t/t5515/fetch.br-branches-default-octopus_branches-default
index 7060bd9..0857f13 100644
--- a/t/t5515/fetch.br-branches-default-octopus_branches-default
+++ b/t/t5515/fetch.br-branches-default-octopus_branches-default
@@ -1,7 +1,7 @@
 # br-branches-default-octopus branches-default
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-merge b/t/t5515/fetch.br-branches-one-merge
index aa1c8a9..54a7742 100644
--- a/t/t5515/fetch.br-branches-one-merge
+++ b/t/t5515/fetch.br-branches-one-merge
@@ -1,6 +1,6 @@
 # br-branches-one-merge
-8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
+8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-merge_branches-one b/t/t5515/fetch.br-branches-one-merge_branches-one
index c93310a..b4d1bb0 100644
--- a/t/t5515/fetch.br-branches-one-merge_branches-one
+++ b/t/t5515/fetch.br-branches-one-merge_branches-one
@@ -1,6 +1,6 @@
 # br-branches-one-merge branches-one
-8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
+8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-config-explicit-merge b/t/t5515/fetch.br-config-explicit-merge
index f6475b7..5ce764a 100644
--- a/t/t5515/fetch.br-config-explicit-merge
+++ b/t/t5515/fetch.br-config-explicit-merge
@@ -1,8 +1,8 @@
 # br-config-explicit-merge
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-config-explicit-merge_config-explicit b/t/t5515/fetch.br-config-explicit-merge_config-explicit
index 018bdd7..b1152b7 100644
--- a/t/t5515/fetch.br-config-explicit-merge_config-explicit
+++ b/t/t5515/fetch.br-config-explicit-merge_config-explicit
@@ -1,8 +1,8 @@
 # br-config-explicit-merge config-explicit
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-config-explicit-octopus b/t/t5515/fetch.br-config-explicit-octopus
index 36d0270..110577b 100644
--- a/t/t5515/fetch.br-config-explicit-octopus
+++ b/t/t5515/fetch.br-config-explicit-octopus
@@ -1,7 +1,7 @@
 # br-config-explicit-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-config-explicit-octopus_config-explicit b/t/t5515/fetch.br-config-explicit-octopus_config-explicit
index 6654ad0..a29dd8b 100644
--- a/t/t5515/fetch.br-config-explicit-octopus_config-explicit
+++ b/t/t5515/fetch.br-config-explicit-octopus_config-explicit
@@ -1,7 +1,7 @@
 # br-config-explicit-octopus config-explicit
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-config-glob-merge b/t/t5515/fetch.br-config-glob-merge
index 8bb5e8b..89f2596 100644
--- a/t/t5515/fetch.br-config-glob-merge
+++ b/t/t5515/fetch.br-config-glob-merge
@@ -1,7 +1,7 @@
 # br-config-glob-merge
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-config-glob-merge_config-glob b/t/t5515/fetch.br-config-glob-merge_config-glob
index 113c08d..2ba4832 100644
--- a/t/t5515/fetch.br-config-glob-merge_config-glob
+++ b/t/t5515/fetch.br-config-glob-merge_config-glob
@@ -1,7 +1,7 @@
 # br-config-glob-merge config-glob
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-config-glob-octopus b/t/t5515/fetch.br-config-glob-octopus
index 9bbd537..64994df 100644
--- a/t/t5515/fetch.br-config-glob-octopus
+++ b/t/t5515/fetch.br-config-glob-octopus
@@ -1,8 +1,8 @@
 # br-config-glob-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-config-glob-octopus_config-glob b/t/t5515/fetch.br-config-glob-octopus_config-glob
index 4e51043..681a725 100644
--- a/t/t5515/fetch.br-config-glob-octopus_config-glob
+++ b/t/t5515/fetch.br-config-glob-octopus_config-glob
@@ -1,8 +1,8 @@
 # br-config-glob-octopus config-glob
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-remote-explicit-merge b/t/t5515/fetch.br-remote-explicit-merge
index 7421b2c..d018b35 100644
--- a/t/t5515/fetch.br-remote-explicit-merge
+++ b/t/t5515/fetch.br-remote-explicit-merge
@@ -1,8 +1,8 @@
 # br-remote-explicit-merge
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-remote-explicit-merge_remote-explicit b/t/t5515/fetch.br-remote-explicit-merge_remote-explicit
index b6975d3..0d3d780 100644
--- a/t/t5515/fetch.br-remote-explicit-merge_remote-explicit
+++ b/t/t5515/fetch.br-remote-explicit-merge_remote-explicit
@@ -1,8 +1,8 @@
 # br-remote-explicit-merge remote-explicit
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-remote-explicit-octopus b/t/t5515/fetch.br-remote-explicit-octopus
index 7681281..6f84304 100644
--- a/t/t5515/fetch.br-remote-explicit-octopus
+++ b/t/t5515/fetch.br-remote-explicit-octopus
@@ -1,7 +1,7 @@
 # br-remote-explicit-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-remote-explicit-octopus_remote-explicit b/t/t5515/fetch.br-remote-explicit-octopus_remote-explicit
index 4c896cf..3546a83 100644
--- a/t/t5515/fetch.br-remote-explicit-octopus_remote-explicit
+++ b/t/t5515/fetch.br-remote-explicit-octopus_remote-explicit
@@ -1,7 +1,7 @@
 # br-remote-explicit-octopus remote-explicit
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-remote-glob-merge b/t/t5515/fetch.br-remote-glob-merge
index 4b62b01..7e1a433 100644
--- a/t/t5515/fetch.br-remote-glob-merge
+++ b/t/t5515/fetch.br-remote-glob-merge
@@ -1,7 +1,7 @@
 # br-remote-glob-merge
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-remote-glob-merge_remote-glob b/t/t5515/fetch.br-remote-glob-merge_remote-glob
index 7478f1f..53571bb 100644
--- a/t/t5515/fetch.br-remote-glob-merge_remote-glob
+++ b/t/t5515/fetch.br-remote-glob-merge_remote-glob
@@ -1,7 +1,7 @@
 # br-remote-glob-merge remote-glob
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge	branch 'two' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
diff --git a/t/t5515/fetch.br-remote-glob-octopus b/t/t5515/fetch.br-remote-glob-octopus
index 2543420..c7c8b6d 100644
--- a/t/t5515/fetch.br-remote-glob-octopus
+++ b/t/t5515/fetch.br-remote-glob-octopus
@@ -1,8 +1,8 @@
 # br-remote-glob-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-remote-glob-octopus_remote-glob b/t/t5515/fetch.br-remote-glob-octopus_remote-glob
index 5ffde9c..36076fb 100644
--- a/t/t5515/fetch.br-remote-glob-octopus_remote-glob
+++ b/t/t5515/fetch.br-remote-glob-octopus_remote-glob
@@ -1,8 +1,8 @@
 # br-remote-glob-octopus remote-glob
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
-0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge	branch 'three' of ../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
-- 
1.7.8.2.334.ge177b0f

^ permalink raw reply related

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2012-01-03 23:27 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, Jakub Narebski,
	Jeff King
In-Reply-To: <7vboqks8la.fsf@alter.siamese.dyndns.org>

Am 03.01.2012 23:51 schrieb Junio C Hamano:
> Sven, does it look agreeable? And more importantly, does it still work? ;-)

Works for me :)

I also updated my second patch minutes ago to fit onto the new patch
(w/o the filename stuff).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2012-01-03 23:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski
In-Reply-To: <7vzke4vebl.fsf@alter.siamese.dyndns.org>

git-svn reads usernames and other user queries from an interactive
terminal. This cause GUIs (w/o STDIN connected) to hang waiting forever
for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
git-core already asks for username using *_ASKPASS tools, this commit
also enables git-svn to do so.

This change extends the Git::prompt method, so that it can also be used
for non password queries (e.g. usernames), and makes use of it instead
of using hand-rolled prompt-response code that only works with the
interactive terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   19 ++++++++-----------
 perl/Git.pm  |   27 ++++++++++++++++-----------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index ade88ae..be713f5 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4356,17 +4356,16 @@ sub ssl_server_trust {
 	        map $cert_info->$_, qw(hostname valid_from valid_until
 	                               issuer_dname fingerprint);
 	my $choice;
-prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	if ($choice =~ /^t$/i) {
+prompt:
+	$choice = lc(substr(Git::prompt($options) || 'R', 0, 1));
+	if ($choice eq 't') {
 		$cred->may_save(undef);
-	} elsif ($choice =~ /^r$/i) {
+	} elsif ($choice eq 'r') {
 		return -1;
-	} elsif ($may_save && $choice =~ /^p$/i) {
+	} elsif ($may_save && $choice eq 'p') {
 		$cred->may_save($may_save);
 	} else {
 		goto prompt;
@@ -4404,9 +4403,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git::prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4415,7 +4412,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git::prompt($prompt);
+	my $password = Git::prompt($prompt, 1);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index 46f11a8..33e68c4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -512,18 +512,19 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD )

 Query user C<PROMPT> and return answer from user.

 If an external helper is specified via GIT_ASKPASS or SSH_ASKPASS, it
 is used to interact with the user; otherwise the prompt is given to
 and the answer is read from the terminal.
+If C<ISPASSWORD> is true, the terminal disables echo.

 =cut

 sub prompt {
-	my ($prompt) = @_;
+	my ($prompt, $isPassword) = @_;
 	my $ret;
 	if (!defined $ret) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -532,18 +533,22 @@ sub prompt {
 		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
 	}
 	if (!defined $ret) {
-		$ret = '';
 		print STDERR $prompt;
 		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$ret .= $key;
+		if ($isPassword) {
+			$ret = '';
+			require Term::ReadKey;
+			Term::ReadKey::ReadMode('noecho');
+			while (defined(my $key = Term::ReadKey::ReadKey(0))) {
+				last if $key =~ /[\012\015]/; # \n\r
+				$ret .= $key;
+			}
+			Term::ReadKey::ReadMode('restore');
+			print STDERR "\n";
+			STDERR->flush;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
-- 
1.7.8.msysgit.0

^ permalink raw reply related

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Junio C Hamano @ 2012-01-03 22:51 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Sven Strickroth, git, Jakub Narebski, Jeff King
In-Reply-To: <CACBZZX7P9PEq0wZp0d3dSwDjF6J6Z3cO4VtWc9_frBengtqPLw@mail.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Dec 28, 2011 at 01:11, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>
> Nom nom, some Perl. Thanks for tackling this. Reviewing it as
> requested by Junio.
>

As I'd like to have this in 1.7.9-rc-something, here is my attempt to
rewrite the patch based on your comments, modulo the "chomp()" bit, to
expedite the cycle. Major fixes are:

 * make "prompt" just a helper subroutine. It does not have to be tied to
   any particular repository object anyway.

 * Move the "ah, the environment is there but the value is not set to
   anything sensible" logic from the caller "prompt" to the helper
   "_prompt". For now, forget about an executable whose name is "0" for
   simplicity.

 * Do so using Perl-ish idiom "return unless ($foo)" to avoid potential
   issues with callers who might call it in the list context (I do not
   think it is reasonable to call "prompt" in the list context to begin
   with, though. It is not like the function is to return a list of zero
   or more answers, in which case "@answers = function()" makes
   sense. This is "ask to get a single answer" interface).

 * "open ... or return", not "||".

Sven, does it look agreeable? And more importantly, does it still work? ;-)

Thanks.

 git-svn.perl |   20 +-------------------
 perl/Git.pm  |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index e30df22..6a01176 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,25 +4415,7 @@ sub username {
 
 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = '';
-	if (exists $ENV{GIT_ASKPASS}) {
-		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
-		$password = <PH>;
-		$password =~ s/[\012\015]//; # \n\r
-		close(PH);
-	} else {
-		print STDERR $prompt;
-		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$password .= $key;
-		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
-	}
+	my $password = Git::prompt($prompt);
 	$password;
 }
 
diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..abf9de9 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -58,7 +58,7 @@ require Exporter;
                 command_output_pipe command_input_pipe command_close_pipe
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
-                remote_refs
+                remote_refs prompt
                 temp_acquire temp_release temp_reset temp_path);
 
 
@@ -512,6 +512,55 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }
 
 
+=item prompt ( PROMPT )
+
+Query user C<PROMPT> and return answer from user.
+
+If an external helper is specified via GIT_ASKPASS or SSH_ASKPASS, it
+is used to interact with the user; otherwise the prompt is given to
+and the answer is read from the terminal.
+
+=cut
+
+sub prompt {
+	my ($prompt) = @_;
+	my $ret;
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		$ret = '';
+		print STDERR $prompt;
+		STDERR->flush;
+		require Term::ReadKey;
+		Term::ReadKey::ReadMode('noecho');
+		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
+			last if $key =~ /[\012\015]/; # \n\r
+			$ret .= $key;
+		}
+		Term::ReadKey::ReadMode('restore');
+		print STDERR "\n";
+		STDERR->flush;
+	}
+	return $ret;
+}
+
+sub _prompt {
+	my ($askpass, $prompt) = @_;
+	return unless ($askpass);
+
+	open my $fh, "-|", $askpass, $prompt
+		or return;
+	my $ret = <$fh>;
+	$ret =~ s/[\012\015]//g; # \n\r
+	close ($fh);
+	return $ret;
+}
+
+
 =item repo_path ()
 
 Return path to the git repository. Must be called on a repository instance.

^ permalink raw reply related

* Re: [PATCH] Submodules always use a relative path to gitdir
From: Junio C Hamano @ 2012-01-03 22:17 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Antony Male, git, iveqy
In-Reply-To: <4F037CBF.9010005@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Not if we would implement a "if no worktree is set but we came here via
> a gitfile, then take the directory the gitfile was found in as worktree"
> heuristic. And that heuristic looks quite sane to me, as a gitfile can
> only be found in a work tree, or am I missing something obvious here?

Like it wouldn't work without changes to the core side?

^ permalink raw reply

* Re: [PATCH] Submodules always use a relative path to gitdir
From: Jens Lehmann @ 2012-01-03 22:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Antony Male, git, iveqy
In-Reply-To: <7vsjjwvdyl.fsf@alter.siamese.dyndns.org>

Am 03.01.2012 19:27, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> Am 29.12.2011 23:40, schrieb Junio C Hamano:
>>> I further wonder if we can get away without using separate-git-dir option
>>> in this codepath, though. IOW using
>>>
>>>         git clone $quiet -bare ${reference:+"$reference"} "$url" "$gitdir"
>>>
>>> might be a better solution.
>>
>> A quick test shows that using a bare repo won't fly because without the
>> core.worktree setting commands that operate on the work tree can't be
>> run anymore inside submodules (starting with the initial checkout). 
> 
> Probably the right thing to do would be to restructure the flow as I
> suggested, i.e.
> 
> 	if we do not have it yet
>         then
>         	git clone --bare ...
> 	fi
> 	# now we have it, make sure they are correct
> 	git config core.bare false

Ah, I forgot to set core.bare to false when trying this. But even then
a dozen tests fail, no matter if I set core.worktree or not. A cursory
glance indicates problems with branches ... I'll have to dig deeper
here.

> 	git config core.worktree $there

Please see below.

>         echo "gitdir: $here" >$there/.git
> 
>> Yes, and the core.worktree setting also contains an absolute path. So
>> we must either make that relative too and rewrite it on every "git
>> submodule add" to record the possibly changed path there or make the
>> bare clone work with a work tree (which sounds a bit strange ;-).
> 
> Update of core.worktree has to be done regardless of the absolute/relative
> differences anyway, no?

Not if we would implement a "if no worktree is set but we came here via
a gitfile, then take the directory the gitfile was found in as worktree"
heuristic. And that heuristic looks quite sane to me, as a gitfile can
only be found in a work tree, or am I missing something obvious here?

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Junio C Hamano @ 2012-01-03 21:44 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, Joey Hess, git
In-Reply-To: <20120103200642.GH20926@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> The credential helper code could potentially have the same deadlock.
> Possibly also clean/smudge filters.
>
> Maybe it could even be part of the run-command interface?

Hmm, that smells like the right thing to do.

^ permalink raw reply

* Re: [PATCH] git-svn: enable platform-specific authentication
From: Matthijs Kooijman @ 2012-01-03 20:44 UTC (permalink / raw)
  To: Eric Wong, Gustav Munkby, Edward Rudd, Carsten Bormann; +Cc: git
In-Reply-To: <20110809210638.GK6418@login.drsnuggles.stderr.nl>

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

Hey folks,

I sent the below patch a few months ago, and not having it applied in
git-svn bit me again just now. Did any of you get a chance to have a
look at it?

I'm still not 100% sure if this patch is correct for all the corner
cases, but it works like a charm in the regular case.

Perhaps it should just be included as is?

Gr.

Matthijs

On Tue, Aug 09, 2011 at 11:06:38PM +0200, Matthijs Kooijman wrote:
> Hey folks,
> 
> > > Use the platform-specific authentication providers that are
> > > exposed to subversion bindings starting with subversion 1.6.
> > 
> > This came up several months ago, I understand there were some issues
> > with the SVN Perl bindings.  Cc-ing interested parties.
> I missed the CC, sorry for that.
> 
> > >  sub _auth_providers () {
> > >  	[
> > > +	  $SVN::Core::VERSION lt '1.6' ? () :
> > > +	    @{SVN::Core::auth_get_platform_specific_client_providers(
> > > +	      undef,undef)},
> > 
> > I think it needs to take into account the config from
> > SVN::Core::config_get_config, otherwise people with non-standard SVN
> > configurations could get locked out.  I seem to recall this was the
> > broken part in the SVN Perl bindings, but one of the Cc-ed parties would
> > know for sure.
> 
> Indeed, but a proposed patch by Eric for this did not work. I solved the
> problem quite some time ago, but apparently I never sent out the
> solution (I think I got distracted by trying to get a passphrase prompt
> to unlock locked keychains). I couldn't find my fixes anymore either,
> but I think I've managed to reproduce them just now.
> 
> Some basic testing shows below patch works, but I think it might need
> some more testing and work. At least the below patch allows for example
> to disable the gnome-keyring provider from a different svn config
> directory by passing --config-dir /some/path to git-svn (which is not
> possible using above patch passing undef, which will only read from
> ~/.subversion).
> 
> Using strace, I did notice that git-svn still reads stuff
> from ~/.subversion/auth/svn.ssl.server/ and
> .subversion/auth/svn.simple/, but I couldn't exactly find why this is
> right away. In any case, it also happens without this patch applied, so
> I guess it's a completely separate issue.
> 
> As for the actual patch, notice that config_get_config returns a hash
> that consists again of a "config" and "servers" patch. Previous attempts
> at this patch passed the entire hash to
> auth_get_platform_specific_client_providers, but it only wants the
> "client" part. It's a bit confusing until you realize that the
> config_get_config return value represents your ~/.subversion directory,
> which again contains a "config" and "servers" file.
> 
> I'm not 100% sure this patch is correct as it is now. I hope to get
> another look at my "automatically unlock keychain" work tomorrow,
> in case there are some hints about flaws in this patch there. In the
> meanwhile, feedback on this patch is welcome.
> 
> Gr.
> 
> Matthijs
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index da3fea8..6dc5196 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4916,7 +4916,7 @@ BEGIN {
>  }
>  
>  sub _auth_providers () {
> -       [
> +       my @rv = (
>           SVN::Client::get_simple_provider(),
>           SVN::Client::get_ssl_server_trust_file_provider(),
>           SVN::Client::get_simple_prompt_provider(
> @@ -4932,7 +4932,23 @@ sub _auth_providers () {
>             \&Git::SVN::Prompt::ssl_server_trust),
>           SVN::Client::get_username_prompt_provider(
>             \&Git::SVN::Prompt::username, 2)
> -       ]
> +       );
> +
> +       # earlier 1.6.x versions would segfault, and <= 1.5.x didn't have
> +       # this function
> +       if ($SVN::Core::VERSION gt '1.6.12') {
> +               my $config = SVN::Core::config_get_config($config_dir);
> +               my ($p, @a);
> +              # config_get_config returns all config files from
> +              # ~/.subversion, auth_get_platform_specific_client_providers
> +              # just wants the config "file".
> +               @a = ($config->{'config'}, undef);
> +               $p = SVN::Core::auth_get_platform_specific_client_providers(@a);
> +              # Insert the return value from
> +              # auth_get_platform_specific_providers
> +               unshift @rv, @$p;
> +       }
> +       \@rv;
>  }
>  
>  sub escape_uri_only {
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: Possible submodule or submodule documentation issue
From: Junio C Hamano @ 2012-01-03 20:48 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Bill Zaumen, git
In-Reply-To: <4F00780C.7090801@web.de>

Will queue; thanks.

^ permalink raw reply

* Re: Stashing individual files
From: Junio C Hamano @ 2012-01-03 20:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Chris Leong, git
In-Reply-To: <20120103201323.GA4340@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> We already have partial stashing like this via "git stash -p".  This is
> just a shorthand for "say yes to all of the changes in foo.c, and no to
> everything else". So I don't see it as particularly new or dangerous.

Ok.

^ permalink raw reply

* Re: 1.7.7.3 wishlist: add --verbose option to git-tag
From: jaalto @ 2012-01-03 20:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk458tuzq.fsf@alter.siamese.dyndns.org>

On 2012-01-03 12:02, Junio C Hamano wrote:
| Jari Aalto <jari.aalto@cante.net> writes:
| 
| > In scripts it would be useful if "git tag" would provide option:
| >
| >     --verbose
| >
| > As in script:
| >
| >     git tag --verbose -m "Initial import" upstream/1.0
| 
| What does the proposed "--verbose" produce that makes scripts easier to
| write

Not "easier write", but to display progress to user; show what is
hapening. Similarly to other --verbose options. Messgae could be:

	  "tagged: upstream/1.0 asd1234"

Jari

^ permalink raw reply

* Re: git and github query about maintaining project
From: Seth Robertson @ 2012-01-03 20:21 UTC (permalink / raw)
  To: manoj soni; +Cc: git
In-Reply-To: <CA+KSefW+K1hMiFkrFCP1LAVjfV9hECwFWAHz940fwGJawHuoFQ@mail.gmail.com>


In message <CA+KSefW+K1hMiFkrFCP1LAVjfV9hECwFWAHz940fwGJawHuoFQ@mail.gmail.com>, manoj soni writes:

    Forwarding you below email, which I have sent to wrong email address by mistake.

You might want to ask on the IRC #git channel for tactical support
questions, like this. irc://irc.freenode.net/git

    we forked P (on github),  I got project forked copy C1

    What I want to do that in my project C1, OLD branch should have
    all of my commits and master branch should be same as P.

Tactically, the answer to your question is as follows:
----------------------------------------------------------------------
# Create a new branch OLD from where master is (presumably containing your commits)
git checkout -b OLD master

# Share OLD with your forked C1 (if you want to)
git push origin OLD
git branch --set-upstream OLD origin/OLD

# Get access to the repository you forked from
git remote add P URL-TO-P
git fetch P

# Reset the master branch to the contents of P/master
#  Please note that any uncommitted changes WILL BE LOST
git checkout master
git reset --hard P/master

# Share your rewritten history with origin (C1)
#  Please note, rewriting publicly visible history is a BAD IDEA.
#  Anyone else who might have pulled the old history will have to do
#  special things and may hate you forever.
git push -f origin
----------------------------------------------------------------------

However, interpreting what you are really trying to do (get your
changes and C2's changes put together and uploaded to P), this is what
*I* would do:
----------------------------------------------------------------------
# Get access to the repository you forked from git remote add P
URL-TO-P git fetch P

# Merge your development with the other development that has been happening
git merge origin/P

# Share your rewritten history with origin (C1)
git push

# When you have finished testing (unless you are using the github pull request method)
git push P master
----------------------------------------------------------------------

					-Seth Robertson

^ permalink raw reply

* Re: Stashing individual files
From: Jeff King @ 2012-01-03 20:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Chris Leong, git
In-Reply-To: <7vfwfwtup7.fsf@alter.siamese.dyndns.org>

On Tue, Jan 03, 2012 at 12:08:52PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think that would be OK compromise, though. I'd rather not introduce a
> > whole new "stashfiles" command (or even a new subcommand of stash) if we
> > can avoid it.
> 
> Why wouldn't a simple "git diff -- paths >P.diff" work?

For all the same reasons that "git diff >P.diff" is not as good as a
regular stash.

> What does such a partial stash leave in the working tree, how does the
> user deal with the remaining local changes, what happens after such a
> partial stash is applied/popped?

I would expect it to stash only the changes in the selected files,
restoring them to their state in HEAD, and leave other files untouched.

We already have partial stashing like this via "git stash -p".  This is
just a shorthand for "say yes to all of the changes in foo.c, and no to
everything else". So I don't see it as particularly new or dangerous.

-Peff

^ permalink raw reply

* Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Junio C Hamano @ 2012-01-03 20:13 UTC (permalink / raw)
  To: Brian Harring; +Cc: git
In-Reply-To: <20120103134603.GA31034@localhost>

Brian Harring <ferringb@gmail.com> writes:

> git-repo if interupted at the exact wrong time will generate zero
> length bundles- literal empty files.  git-repo is wrong here, but
> git fetch shouldn't effectively spin loop if pointed at a zero
> length bundle.
>
> Signed-off-by: Brian Harring <ferringb@chromium.org>
> ---

Thanks.

Also thanks to all reviewers.

^ permalink raw reply

* Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Junio C Hamano @ 2012-01-03 20:11 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Brian Harring, git, spearce
In-Reply-To: <CACsJy8B5B2bx7WK7ViziseuWCPaMgcc-avwtsw2DybRme8Vgfg@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Tue, Jan 3, 2012 at 8:13 AM, Brian Harring <ferringb@gmail.com> wrote:
>> @@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)
>>        while (1) {
>>                char ch;
>>                ssize_t len = xread(fd, &ch, 1);
>> -               if (len < 0)
>> +               if (len <= 0)
>>                        return -1;
>>                strbuf_addch(sb, ch);
>>                if (ch == '\n')
>
> I think it should return 0 when len == 0 because strictly speaking eof
> is not a fault.

Even if you do not strictly speak, end of file is a perfectly normal thing
to see, no?  IOW wouldn't the original patch actively _break_ callers that
read the whole file from the file descriptor to the end?

> FWIW I went through all xread call sites. All seem to handle return
> value <= 0 correctly.

Thanks.

^ permalink raw reply

* Re: Stashing individual files
From: Junio C Hamano @ 2012-01-03 20:08 UTC (permalink / raw)
  To: Jeff King; +Cc: Chris Leong, git
In-Reply-To: <20120103190612.GC20926@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> I think that would be OK compromise, though. I'd rather not introduce a
> whole new "stashfiles" command (or even a new subcommand of stash) if we
> can avoid it.

Why wouldn't a simple "git diff -- paths >P.diff" work?

What does such a partial stash leave in the working tree, how does the
user deal with the remaining local changes, what happens after such a
partial stash is applied/popped?

I wouldn't have worried about such a change before e0e2a9c (stash: drop
dirty worktree check on apply, 2011-04-05) but now we allow application of
stashed changes to the dirty working tree (which is a very good thing), I
am not sure how sensibly these changes in different places would interact
if we start supporting partial stashing.

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Jeff King @ 2012-01-03 20:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Joey Hess, git
In-Reply-To: <7vsjjwtvf1.fsf@alter.siamese.dyndns.org>

On Tue, Jan 03, 2012 at 11:53:22AM -0800, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > IMO, as the first step, the user of this infrastructure should only be
> > required to construct the hook input as a strbuf, and receive the hook
> > output, if needed, also as a strbuf.
> 
> Now you brought it up, I think I would agree. The only reason I suggested
> a callback feeder approach was because I somehow was hoping that it may be
> possible to share more code with the codepath for textconv that may not
> want to hold too much buffer in core when we know the data is only used
> sequencially and I wanted to see more things to go through streaming API
> in the future.

Even if we don't make the input streaming, it would be nice to factor
the concept of "feed input to program and read its output without
deadlocking" into something independent of hooks.

The credential helper code could potentially have the same deadlock.
Possibly also clean/smudge filters.

Maybe it could even be part of the run-command interface?

-Peff

^ permalink raw reply

* Re: [PATCH resend] Do not create commits whose message contains NUL
From: Jeff King @ 2012-01-03 20:03 UTC (permalink / raw)
  To: Drew Northup; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <1325435251.4752.104.camel@drew-northup.unet.maine.edu>

On Sun, Jan 01, 2012 at 11:27:31AM -0500, Drew Northup wrote:

> I had already started experimenting with automatically detecting decent
> UTF-16 a long while back so that compatible platforms could handle it
> appropriately in terms of creating diffs and dealing with newline
> munging between platforms. There is no 100% sure-fire check for UTF-16
> if you don't already suspect it is possibly UTF-16. If we really want to
> check for possible UTF-16 specifically I can scrape out the check I
> wrote up and send it along.

I also looked into this recently. You can generally detect UTF-16 by the
BOM at the beginning of the file (which will also tell you the
endian-ness). I did a simple test by integrating it into the check for
binary-ness during diffs. However, as I recall, the result wasn't
particularly useful. Some of the diff code wasn't happy with the
embedded NUL bytes (i.e., there is code that assumes that NUL is the end
of a string). Not to mention that ascii newline (0x0a) can appear as
part of other characters in a wide encoding like utf-16. And since git
outputs straight ascii for all of the diff boilerplate, you end up with
a mish-mash of utf-16 and ascii (this is OK with utf-8, of course,
because utf-8 is a superset of ascii).

If anything, I think you would want to do something like "textconv" to
convert the utf-16 into utf-8, then diff that. Git won't do it
automatically based on encoding, but if you know the filenames of the
utf-16 files in your repository, you can do something like:

  echo 'foo.txt diff=utf16' >.gitattributes
  git config diff.utf16.textconv 'iconv -f utf16 -t utf8'

and get readable diffs. Of course you couldn't use that diff to apply a
patch, though.

I strongly suspect that not many people are really using git for utf-16
files. Git treats them as binary, which makes them unpleasant for
anything except simple storage.

> The is_utf8 check was not written to detect 100% valid UTF-8 per-se. It
> seems to me that it was written as part of the "is this a binary or not"
> check in the add/commit path.

We shouldn't care about binary file content at all in the add or commit
code paths. I would guess we do only if you are using auto-crlf (but
then, I don't think we care about utf8 in that cases, only whether line
endings should be converted or not).

We do check that the commit message itself is utf8, but only to generate
a warning that you should set i81n.commitencoding.

-Peff

^ permalink raw reply

* Re: 1.7.7.3 wishlist: add --verbose option to git-tag
From: Junio C Hamano @ 2012-01-03 20:02 UTC (permalink / raw)
  To: Jari Aalto; +Cc: git
In-Reply-To: <87d3b51vr0.fsf@cante.cante.net>

Jari Aalto <jari.aalto@cante.net> writes:

> In scripts it would be useful if "git tag" would provide option:
>
>     --verbose
>
> As in script:
>
>     git tag --verbose -m "Initial import" upstream/1.0

"In scripts", you are expected to be capable of doing anything fancy with
"git cat-file tag", but we add things that turn out to be commonly needed.

What does the proposed "--verbose" produce that makes scripts easier to
write (i.e. avoids repeated post-processing of "git cat-file tag" output),
and how commonly would what you propose apply to various people's needs
other than yours?

^ permalink raw reply

* Re: [PATCH] Submodules always use a relative path to gitdir
From: Junio C Hamano @ 2012-01-03 19:58 UTC (permalink / raw)
  To: Phil Hord; +Cc: Antony Male, git, iveqy
In-Reply-To: <7vlipovd4n.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> ...
> And the extent of the design of
>
>     echo "gitdir: $there" >.git && git config core.worktree "$(pwd)"
>
> is to work with the locations of these two places as they are set up.
> Moving one or the other or both may or may not work without adjusting to
> what you did. If you "mv $there $newlocation" (the repository) behind
> Git's back, you may need to update .git to point at the new location of
> the repository.  If you move your working tree woth "mv", you may need to
> update core.worktree to point at the new location of the working tree.
> And until you do so things may not work. That is why we do not explicitly
> say "you can move them to arbitrary places without telling git and things
> will work"---because that is not the case.

Just to avoid any misunderstanding, I still agree with the overall goal of
the original patch to allow moving the whole superproject tree, including
its submodule repositories in its .git/modules/, and the working trees of
itself and its submodules. It is a narrow special case with a very well
defined relative relationships between the working tree of submodules and
the repositories that control them, and having them point to each other
with relative paths will make any post-move adjustments unnecessary, unlike
more general unconstrained uses of the "gitdir: $there" mechanism.

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Junio C Hamano @ 2012-01-03 19:53 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Joey Hess, git
In-Reply-To: <4EFD88CB.3050403@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

> IMO, as the first step, the user of this infrastructure should only be
> required to construct the hook input as a strbuf, and receive the hook
> output, if needed, also as a strbuf.

Now you brought it up, I think I would agree. The only reason I suggested
a callback feeder approach was because I somehow was hoping that it may be
possible to share more code with the codepath for textconv that may not
want to hold too much buffer in core when we know the data is only used
sequencially and I wanted to see more things to go through streaming API
in the future.

>> +`run_hook_complex`::

Also, I think the updated interface should become the "run_hook" function;
nothing "complex" about it. The name "run_hook()" was a perfectly fine
abstraction for what it did when it used to be a static helper function
within builtin-commit.c, but its special-casing of GIT_INDEX_FILE
environment is _not_ general enough to deserve it to be called the
"run_hook" in the global scope.

IOW, I am saying that we screwed up at ae98a00 (Move run_hook() from
builtin-commit.c into run-command.c (libgit), 2009-01-16.

The environment tweaking should not take a "index_file" field in the
structure, but an array "environ" that is used to tweak the environment
variables for the hook process.

^ 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