Git development
 help / color / mirror / Atom feed
* Re: [PATCH] send-email: Fix %config_path_settings handling
From: Junio C Hamano @ 2011-10-14 23:23 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Michael J Gruber, Git Mailing List, Cord Seele, Cord Seele
In-Reply-To: <201110142253.32695.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> From: Cord Seele <cowose@gmail.com>
> ...
> Signed-off-by: Cord Seele <cowose@gmail.com>
> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Thanks.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 579ddb7..87b4acc 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1168,4 +1168,32 @@ test_expect_success $PREREQ '--force sends cover letter template anyway' '
>  	test -n "$(ls msgtxt*)"
>  '
>  
> +test_expect_success $PREREQ 'sendemail.aliasfiletype=mailrc' '
> +	clean_fake_sendmail &&
> +	echo "alias sbd  somebody@example.org" >.mailrc &&
> +	git config --replace-all sendemail.aliasesfile "$(pwd)/.mailrc" &&
> +	git config sendemail.aliasfiletype mailrc &&
> +	git send-email \
> +	  --from="Example <nobody@example.com>" \
> +	  --to=sbd \
> +	  --smtp-server="$(pwd)/fake.sendmail" \
> +	  outdir/0001-*.patch \
> +	  2>errors >out &&
> +	grep "^!somebody@example\.org!$" commandline1
> +'
> +
> +test_expect_success $PREREQ 'sendemail.aliasfile=~/.mailrc' '
> +	clean_fake_sendmail &&
> +	echo "alias sbd  someone@example.org" >~/.mailrc &&
> +	git config --replace-all sendemail.aliasesfile "~/.mailrc" &&
> +	git config sendemail.aliasfiletype mailrc &&
> +	git send-email \
> +	  --from="Example <nobody@example.com>" \
> +	  --to=sbd \
> +	  --smtp-server="$(pwd)/fake.sendmail" \
> +	  outdir/0001-*.patch \
> +	  2>errors >out &&
> +	grep "^!someone@example\.org!$" commandline1
> +'
> +
>  test_done

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Sitaram Chamarty @ 2011-10-14 23:39 UTC (permalink / raw)
  To: Jeff King
  Cc: Jonathan Nieder, Junio C Hamano, Nguyen Thai Ngoc Duy, git,
	Ilari Liusvaara, Johannes Sixt
In-Reply-To: <20111014211921.GB16429@sigill.intra.peff.net>

On Sat, Oct 15, 2011 at 2:49 AM, Jeff King <peff@peff.net> wrote:
> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.
>
> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).
>
> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Sites which don't have hidden repositories,

I suggest that even the "secure" version of the message say something
like "access denied or repository not exported".  You're still not
leaking anything, but it reduces confusion to the user, who otherwise
may not realise it *could be* the latter.

regards

sitaram

> or don't care, can pass a flag to turn on more specific
> messages.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Minor tweaks to the documentation and code style to make Jonathan happy.
> :)
>
> Note: I labeled this "v3", as it is the third one posted, but the prior
> ones were not labeled with versions at all.
>
>  Documentation/git-daemon.txt |   10 ++++++++++
>  daemon.c                     |   25 +++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 69a1e4a..31b28fc 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning
>        repository configuration.  By default, all the services
>        are overridable.
>
> +--informative-errors::
> +--no-informative-errors::
> +       When informative errors are turned on, git-daemon will report
> +       more verbose errors to the client, differentiating conditions
> +       like "no such repository" from "repository not exported". This
> +       is more convenient for clients, but may leak information about
> +       the existence of unexported repositories.  When informative
> +       errors are not enabled, all errors report "access denied" to the
> +       client. The default is --no-informative-errors.
> +
>  <directory>::
>        A directory to add to the whitelist of allowed directories. Unless
>        --strict-paths is specified this will also include subdirectories
> diff --git a/daemon.c b/daemon.c
> index 4c8346d..6f111af 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -20,6 +20,7 @@
>  static int log_syslog;
>  static int verbose;
>  static int reuseaddr;
> +static int informative_errors;
>
>  static const char daemon_usage[] =
>  "git daemon [--verbose] [--syslog] [--export-all]\n"
> @@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
>        return 0;
>  }
>
> +static int daemon_error(const char *dir, const char *msg)
> +{
> +       if (!informative_errors)
> +               msg = "access denied";
> +       packet_write(1, "ERR %s: %s", msg, dir);
> +       return -1;
> +}
> +
>  static int run_service(char *dir, struct daemon_service *service)
>  {
>        const char *path;
> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!enabled && !service->overridable) {
>                logerror("'%s': service not enabled.", service->name);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }
>
>        if (!(path = path_ok(dir)))
> -               return -1;
> +               return daemon_error(dir, "no such repository");
>
>        /*
>         * Security on the cheap.
> @@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
>                logerror("'%s': repository not exported.", path);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "repository not exported");
>        }
>
>        if (service->overridable) {
> @@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
>                logerror("'%s': service not enabled for '%s'",
>                         service->name, path);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }
>
>        /*
> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
>                        make_service_overridable(arg + 18, 0);
>                        continue;
>                }
> +               if (!prefixcmp(arg, "--informative-errors")) {
> +                       informative_errors = 1;
> +                       continue;
> +               }
> +               if (!prefixcmp(arg, "--no-informative-errors")) {
> +                       informative_errors = 0;
> +                       continue;
> +               }
>                if (!strcmp(arg, "--")) {
>                        ok_paths = &argv[i+1];
>                        break;
> --
> 1.7.6.4.37.g43b58b
>
> --
> 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
>



-- 
Sitaram

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Nguyen Thai Ngoc Duy @ 2011-10-15  0:51 UTC (permalink / raw)
  To: Jeff King
  Cc: Jonathan Nieder, Junio C Hamano, git, Ilari Liusvaara,
	Johannes Sixt
In-Reply-To: <20111014211921.GB16429@sigill.intra.peff.net>

On Sat, Oct 15, 2011 at 8:19 AM, Jeff King <peff@peff.net> wrote:
> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!enabled && !service->overridable) {
>                logerror("'%s': service not enabled.", service->name);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }

Nit picking. In this case the service is disabled entirely regardless
dir and it uses the same message with the case where service is
disabled per repo later on. Maybe we could reword it a little bit to
differentiate the two cases? Say the first case "service disabled",
and the second one "service not enabled"?
-- 
Duy

^ permalink raw reply

* [PATCHv4 0/5] Be more careful when prunning
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf

Hello,

This version covers Junio's concerns about code and functionality
duplication with the third patch. The order of arguments for
get_stale_heads is not changed anymore in the fourth patch.

The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.

The second patch introduces expected failures for the features that
this series fixes.

The third patch is new and moves the logic out of remote_find_tracking
into a query_refspecs function. remote_find_tracking and
apply_refspecs are both changed to be wrappers around this
function. This way, the logic is in one function instead of three
different ones.

The fourth patch changes prune_refs and get_stale_heads so the caller
has to decide which refspecs are the appropriate ones to use. For
example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.

What is probably the most usual case is covered by the fifth patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line. That fixes the

    git fetch --prune --tags origin

case. The non-tag refs are kept now.

Cheers,
   cmn

Carlos Martín Nieto (5):
  fetch: free all the additional refspecs
  t5510: add tests for fetch --prune
  remote: separate out the remote_find_tracking logic into
    query_refspecs
  fetch: honor the user-provided refspecs when pruning refs
  fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

 builtin/fetch.c  |   33 ++++++++++++++---
 builtin/remote.c |    3 +-
 remote.c         |  106 +++++++++++++++++++++++++++++------------------------
 remote.h         |    2 +-
 t/t5510-fetch.sh |   50 +++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 55 deletions(-)

-- 
1.7.5.2.354.g349bf

^ permalink raw reply

* [PATCH 4/5] fetch: honor the user-provided refspecs when pruning refs
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
In-Reply-To: <1318655066-29001-1-git-send-email-cmn@elego.de>

If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other ref under the origin namespace because we
were using the refspec to filter the available refs but using the
configured refspec to figure out if a ref had been deleted on the
remote. This is clearly the wrong thing to do.

Change prune_refs and get_stale_heads to simply accept a list of
references and a list of refspecs. The caller of either function needs
to decide what refspecs should be used to decide whether a ref is
stale.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c  |   12 ++++++++----
 builtin/remote.c |    3 ++-
 remote.c         |   36 ++++++++++++++++++++++++------------
 remote.h         |    2 +-
 t/t5510-fetch.sh |    4 ++--
 5 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 605d1bf..e295d97 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -540,10 +540,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct transport *transport, struct ref *ref_map)
+static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 {
 	int result = 0;
-	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
+	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)\n")
 		: _("   (%s has become dangling)\n");
@@ -734,8 +734,12 @@ static int do_fetch(struct transport *transport,
 		free_refs(ref_map);
 		return 1;
 	}
-	if (prune)
-		prune_refs(transport, ref_map);
+	if (prune) {
+		if (ref_count)
+			prune_refs(refs, ref_count, ref_map);
+		else
+			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
+	}
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/builtin/remote.c b/builtin/remote.c
index f2a9c26..de52367 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 		else
 			string_list_append(&states->tracked, abbrev_branch(ref->name));
 	}
-	stale_refs = get_stale_heads(states->remote, fetch_map);
+	stale_refs = get_stale_heads(states->remote->fetch,
+				     states->remote->fetch_refspec_nr, fetch_map);
 	for (ref = stale_refs; ref; ref = ref->next) {
 		struct string_list_item *item =
 			string_list_append(&states->stale, abbrev_branch(ref->name));
diff --git a/remote.c b/remote.c
index e94c6d2..069b1ff 100644
--- a/remote.c
+++ b/remote.c
@@ -1679,36 +1679,48 @@ struct ref *guess_remote_head(const struct ref *head,
 }
 
 struct stale_heads_info {
-	struct remote *remote;
 	struct string_list *ref_names;
 	struct ref **stale_refs_tail;
+	struct refspec *refs;
+	int ref_count;
 };
 
 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
-	struct refspec refspec;
-	memset(&refspec, 0, sizeof(refspec));
-	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+	struct refspec query;
+	memset(&query, 0, sizeof(struct refspec));
+	query.dst = (char *)refname;
+
+	if (query_refspecs(info->refs, info->ref_count, &query))
+	    return 0; /* No matches */
+
+
+	/*
+	 * If we did find a suitable refspec and it's not a symref and
+	 * it's not in the list of refs that currently exist in that
+	 * remote we consider it to be stale.
+	 */
+	if (!((flags & REF_ISSYMREF) ||
+	      string_list_has_string(info->ref_names, query.src))) {
+		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+		hashcpy(ref->new_sha1, sha1);
 	}
+
+	free(query.src);
 	return 0;
 }
 
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
 	struct stale_heads_info info;
-	info.remote = remote;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
+	info.refs = refs;
+	info.ref_count = ref_count;
 	for (ref = fetch_map; ref; ref = ref->next)
 		string_list_append(&ref_names, ref->name);
 	sort_string_list(&ref_names);
diff --git a/remote.h b/remote.h
index 9a30a9d..f2541b5 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,6 @@ struct ref *guess_remote_head(const struct ref *head,
 			      int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);
+struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map);
 
 #endif
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 8b5e925..581049b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -86,7 +86,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
 	test_must_fail git rev-parse origin/extrabranch
 '
 
-test_expect_failure 'fetch --prune with a branch name keeps branches' '
+test_expect_success 'fetch --prune with a branch name keeps branches' '
 	cd "$D" &&
 	git clone . prune-branch &&
 	cd prune-branch &&
@@ -96,7 +96,7 @@ test_expect_failure 'fetch --prune with a branch name keeps branches' '
 	git rev-parse origin/extrabranch
 '
 
-test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '
+test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	cd "$D" &&
 	git clone . prune-namespace &&
 	cd prune-namespace &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related

* [PATCH 5/5] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
In-Reply-To: <1318655066-29001-1-git-send-email-cmn@elego.de>

If --tags is specified, add that refspec to the list given to
prune_refs so it knows to treat it as a filter on what refs to
should consider for prunning. This way

    git fetch --prune --tags origin

only prunes tags and doesn't delete the branch refs.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c  |   23 +++++++++++++++++++++--
 t/t5510-fetch.sh |    4 ++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e295d97..9d481f8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -735,10 +735,29 @@ static int do_fetch(struct transport *transport,
 		return 1;
 	}
 	if (prune) {
-		if (ref_count)
+		/* If --tags was specified, pretend the user gave us the canonical tags refspec */
+		if (tags == TAGS_SET) {
+			const char *tags_str = "refs/tags/*:refs/tags/*";
+			struct refspec *tags_refspec, *refspec;
+
+			/* Copy the refspec and add the tags to it */
+			refspec = xcalloc(ref_count + 1, sizeof(struct refspec));
+			tags_refspec = parse_fetch_refspec(1, &tags_str);
+			memcpy(refspec, refs, ref_count * sizeof(struct refspec));
+			memcpy(&refspec[ref_count], tags_refspec, sizeof(struct refspec));
+			ref_count++;
+
+			prune_refs(refspec, ref_count, ref_map);
+
+			ref_count--;
+			/* The rest of the strings belong to fetch_one */
+			free_refspec(1, tags_refspec);
+			free(refspec);
+		} else if (ref_count) {
 			prune_refs(refs, ref_count, ref_map);
-		else
+		} else {
 			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
+		}
 	}
 	free_refs(ref_map);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 581049b..e0af4c4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -105,7 +105,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	git rev-parse origin/master
 '
 
-test_expect_failure 'fetch --prune --tags does not delete the remote-tracking branches' '
+test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
@@ -116,7 +116,7 @@ test_expect_failure 'fetch --prune --tags does not delete the remote-tracking br
 	test_must_fail git rev-parse somebranch
 '
 
-test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related

* [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
In-Reply-To: <1318655066-29001-1-git-send-email-cmn@elego.de>

Move the body of remote_find_tracking to a new function query_refspecs
which does the same (find a refspec that matches and apply the
transformation) but explicitely wants the list of refspecs.

Make remote_find_tracking and apply_refspecs use query_refspecs.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/remote.c b/remote.c
index b8ecfa5..e94c6d2 100644
--- a/remote.c
+++ b/remote.c
@@ -828,59 +828,57 @@ static int match_name_with_pattern(const char *key, const char *name,
 	return ret;
 }
 
-char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
-		     const char *name)
+static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)
 {
 	int i;
-	char *ret = NULL;
-	for (i = 0; i < nr_refspec; i++) {
-		struct refspec *refspec = refspecs + i;
-		if (refspec->pattern) {
-			if (match_name_with_pattern(refspec->src, name,
-						    refspec->dst, &ret))
-				return ret;
-		} else if (!strcmp(refspec->src, name))
-			return strdup(refspec->dst);
-	}
-	return NULL;
-}
+	int find_src = query->src == NULL;
 
-int remote_find_tracking(struct remote *remote, struct refspec *refspec)
-{
-	int find_src = refspec->src == NULL;
-	char *needle, **result;
-	int i;
+	if (find_src && !query->dst)
+		return error("query_refspecs: need either src or dst");
 
-	if (find_src) {
-		if (!refspec->dst)
-			return error("find_tracking: need either src or dst");
-		needle = refspec->dst;
-		result = &refspec->src;
-	} else {
-		needle = refspec->src;
-		result = &refspec->dst;
-	}
+	for (i = 0; i < ref_count; i++) {
+		struct refspec *refspec = &refs[i];
+		const char *key = find_src ? refspec->dst : refspec->src;
+		const char *value = find_src ? refspec->src : refspec->dst;
+		const char *needle = find_src ? query->dst : query->src;
+		char **result = find_src ? &query->src : &query->dst;
 
-	for (i = 0; i < remote->fetch_refspec_nr; i++) {
-		struct refspec *fetch = &remote->fetch[i];
-		const char *key = find_src ? fetch->dst : fetch->src;
-		const char *value = find_src ? fetch->src : fetch->dst;
-		if (!fetch->dst)
+		if (!refspec->dst)
 			continue;
-		if (fetch->pattern) {
+		if (refspec->pattern) {
 			if (match_name_with_pattern(key, needle, value, result)) {
-				refspec->force = fetch->force;
+				query->force = refspec->force;
 				return 0;
 			}
 		} else if (!strcmp(needle, key)) {
 			*result = xstrdup(value);
-			refspec->force = fetch->force;
+			query->force = refspec->force;
 			return 0;
 		}
 	}
+
 	return -1;
 }
 
+char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
+		     const char *name)
+{
+	struct refspec query;
+
+	memset(&query, 0x0, sizeof(struct refspec));
+	query.src = (char *) name;
+
+	if (query_refspecs(refspecs, nr_refspec, &query))
+		return NULL;
+
+	return query.dst;
+}
+
+int remote_find_tracking(struct remote *remote, struct refspec *refspec)
+{
+	return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
+}
+
 static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
 		const char *name)
 {
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related

* [PATCH 1/5] fetch: free all the additional refspecs
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
In-Reply-To: <1318655066-29001-1-git-send-email-cmn@elego.de>

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e422ced..605d1bf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -918,7 +918,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 	atexit(unlock_pack);
 	refspec = parse_fetch_refspec(ref_nr, refs);
 	exit_code = do_fetch(transport, refspec, ref_nr);
-	free(refspec);
+	free_refspec(ref_nr, refspec);
 	transport_disconnect(transport);
 	transport = NULL;
 	return exit_code;
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related

* [PATCH 2/5] t5510: add tests for fetch --prune
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
In-Reply-To: <1318655066-29001-1-git-send-email-cmn@elego.de>

The failures will be fixed in later commits.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5510-fetch.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 7e433b1..8b5e925 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -76,6 +76,56 @@ test_expect_success "fetch test for-merge" '
 	cut -f -2 .git/FETCH_HEAD >actual &&
 	test_cmp expected actual'
 
+test_expect_success 'fetch --prune on its own works as expected' '
+	cd "$D" &&
+	git clone . prune &&
+	cd prune &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune origin &&
+	test_must_fail git rev-parse origin/extrabranch
+'
+
+test_expect_failure 'fetch --prune with a branch name keeps branches' '
+	cd "$D" &&
+	git clone . prune-branch &&
+	cd prune-branch &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune origin master &&
+	git rev-parse origin/extrabranch
+'
+
+test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '
+	cd "$D" &&
+	git clone . prune-namespace &&
+	cd prune-namespace &&
+
+	git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&
+	git rev-parse origin/master
+'
+
+test_expect_failure 'fetch --prune --tags does not delete the remote-tracking branches' '
+	cd "$D" &&
+	git clone . prune-tags &&
+	cd prune-tags &&
+	git fetch origin refs/heads/master:refs/tags/sometag &&
+
+	git fetch --prune --tags origin &&
+	git rev-parse origin/master &&
+	test_must_fail git rev-parse somebranch
+'
+
+test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+	cd "$D" &&
+	git clone . prune-tags-branch &&
+	cd prune-tags-branch &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune --tags origin master &&
+	git rev-parse origin/extrabranch
+'
+
 test_expect_success 'fetch tags when there is no tags' '
 
     cd "$D" &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related

* Re: [PATCH 2/8] t1402: Ignore a few cases that must fail due to DOS path expansion
From: Junio C Hamano @ 2011-10-15  5:36 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, msysGit, Johannes Schindelin
In-Reply-To: <1318632815-29945-3-git-send-email-patthoyts@users.sourceforge.net>

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

>  t/t1402-check-ref-format.sh |   15 +++++++++------

Didn't we see a different patch that attempts to address the same issue
recently on the list from J6t, or is this a fix for a different problem?

>  1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
> index 710fcca..1a5e343 100755
> --- a/t/t1402-check-ref-format.sh
> +++ b/t/t1402-check-ref-format.sh
> @@ -36,7 +36,7 @@ invalid_ref 'refs///heads/foo'
>  valid_ref 'refs///heads/foo' --normalize
>  invalid_ref 'heads/foo/'
>  invalid_ref '/heads/foo'
> -valid_ref '/heads/foo' --normalize
> +test_have_prereq MINGW || valid_ref '/heads/foo' --normalize
>  invalid_ref '///heads/foo'
>  valid_ref '///heads/foo' --normalize
>  invalid_ref './foo'
> @@ -120,9 +120,12 @@ invalid_ref "$ref" --allow-onelevel
>  invalid_ref "$ref" --refspec-pattern
>  invalid_ref "$ref" '--refspec-pattern --allow-onelevel'
>  invalid_ref "$ref" --normalize
> -valid_ref "$ref" '--allow-onelevel --normalize'
> -invalid_ref "$ref" '--refspec-pattern --normalize'
> -valid_ref "$ref" '--refspec-pattern --allow-onelevel --normalize'
> +if test_have_prereq NOT_MINGW
> +then
> +	valid_ref "$ref" '--allow-onelevel --normalize'
> +	invalid_ref "$ref" '--refspec-pattern --normalize'
> +	valid_ref "$ref" '--refspec-pattern --allow-onelevel --normalize'
> +fi
>  
>  test_expect_success "check-ref-format --branch @{-1}" '
>  	T=$(git write-tree) &&
> @@ -166,10 +169,10 @@ invalid_ref_normalized() {
>  
>  valid_ref_normalized 'heads/foo' 'heads/foo'
>  valid_ref_normalized 'refs///heads/foo' 'refs/heads/foo'
> -valid_ref_normalized '/heads/foo' 'heads/foo'
> +test_have_prereq MINGW || valid_ref_normalized '/heads/foo' 'heads/foo'
>  valid_ref_normalized '///heads/foo' 'heads/foo'
>  invalid_ref_normalized 'foo'
> -invalid_ref_normalized '/foo'
> +test_have_prereq MINGW || invalid_ref_normalized '/foo'
>  invalid_ref_normalized 'heads/foo/../bar'
>  invalid_ref_normalized 'heads/./foo'
>  invalid_ref_normalized 'heads\foo'

^ permalink raw reply

* Re: [PATCH 5/8] t9901: fix line-ending dependency on windows
From: Junio C Hamano @ 2011-10-15  5:44 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, msysGit
In-Reply-To: <1318632815-29945-6-git-send-email-patthoyts@users.sourceforge.net>

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
>  t/t9901-git-web--browse.sh |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
> index 7906e5d..1185b42 100755
> --- a/t/t9901-git-web--browse.sh
> +++ b/t/t9901-git-web--browse.sh
> @@ -12,7 +12,7 @@ test_expect_success \
>  	echo http://example.com/foo\&bar >expect &&
>  	git config browser.custom.cmd echo &&
>  	git web--browse --browser=custom \
> -		http://example.com/foo\&bar >actual &&
> +		http://example.com/foo\&bar | tr -d "\r" >actual &&
>  	test_cmp expect actual
>  '

This is unnice for two reasons. We have web--browse five times in this
test script, and you add tr exactly the same way to each and every one of
them. And you are losing the error condition from each and every one of
these web--browse invocations.

Having to do the same change to all invocations of the same command
suggests that perhaps you can and should wrap that pattern into a single
helper, perhaps like:

test_web_browse () {
	# browser=$1 url=$2
	git web--browse --browser="$1" "$2" >actual &&
        tr -d '\015' <actual >text &&
        test_cmp expect text
}

or something?

^ permalink raw reply

* Re: A note from the maintainer
From: Martin von Zweigbergk @ 2011-10-15  5:47 UTC (permalink / raw)
  To: Junio C Hamano, Paul Mackerras; +Cc: git
In-Reply-To: <7vr52s9ny5.fsf@alter.siamese.dyndns.org>

On Tue, Oct 4, 2011 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  - gitk-git/ comes from Paul Mackerras's gitk project:
>
>        git://git.kernel.org/pub/scm/gitk/gitk.git

I don't seem to be able to fetch from there. Is it still supposed to be there?

Martin

^ permalink raw reply

* Re: [PATCH 7/8] mergetools: use the correct tool for Beyond Compare 3 on Windows
From: Junio C Hamano @ 2011-10-15  5:50 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, msysGit
In-Reply-To: <1318632815-29945-8-git-send-email-patthoyts@users.sourceforge.net>

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> On Windows the bcompare tool launches a graphical program and does
> not wait for it to terminate. A separate 'bcomp' tool is provided which
> will wait for the view to exit so we use this instead.

Hmm, does this only apply to Windows, or are there other platforms on
which BC3 supplies bcomp for the exact same reason? What I am trying to
get at is that it might be nicer if we do not have to check uname, e.g.

	if type bcomp >/dev/null 2>/dev/null
        then
        	echo bcomp
	else
        	echo bcompare
	fi

> Reported-by: Werner BEROUX <werner@beroux.com>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
>  mergetools/bc3 |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/mergetools/bc3 b/mergetools/bc3
> index 27b3dd4..b642bf2 100644
> --- a/mergetools/bc3
> +++ b/mergetools/bc3
> @@ -16,5 +16,12 @@ merge_cmd () {
>  }
>  
>  translate_merge_tool_path() {
> -	echo bcompare
> +	case $(uname -s) in
> +	*MINGW*)
> +		echo bcomp
> +		;;
> +	*)
> +		echo bcompare
> +		;;
> +	esac
>  }

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Junio C Hamano @ 2011-10-15  5:55 UTC (permalink / raw)
  To: Sitaram Chamarty
  Cc: Jeff King, Jonathan Nieder, Nguyen Thai Ngoc Duy, git,
	Ilari Liusvaara, Johannes Sixt
In-Reply-To: <CAMK1S_g0aKUa=+ndAm7rqeoPAobjVb6oJ1Z4DqSeNrdauXNH3w@mail.gmail.com>

Sitaram Chamarty <sitaramc@gmail.com> writes:

>> Because there is a risk of leaking information about
>> non-exported repositories, by default all errors simply say
>> "access denied". Sites which don't have hidden repositories,
>
> I suggest that even the "secure" version of the message say something
> like "access denied or repository not exported".  You're still not
> leaking anything, but it reduces confusion to the user, who otherwise
> may not realise it *could be* the latter.

I kind of like the suggestion, but I am afraid that "access denied,
repository nonexistent or not exported" can soon easily get long enough to
be unmanageable.

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Sitaram Chamarty @ 2011-10-15  7:09 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Jonathan Nieder, Nguyen Thai Ngoc Duy, git,
	Ilari Liusvaara, Johannes Sixt
In-Reply-To: <7vk486x0hq.fsf@alter.siamese.dyndns.org>

On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>> Because there is a risk of leaking information about
>>> non-exported repositories, by default all errors simply say
>>> "access denied". Sites which don't have hidden repositories,
>>
>> I suggest that even the "secure" version of the message say something
>> like "access denied or repository not exported".  You're still not
>> leaking anything, but it reduces confusion to the user, who otherwise
>> may not realise it *could be* the latter.
>
> I kind of like the suggestion, but I am afraid that "access denied,
> repository nonexistent or not exported" can soon easily get long enough to
> be unmanageable.

When someone who *does* have access makes a typo, "access denied"
makes it harder to realise it, because it subtly implies the repo
*does* exist and it's an ACL issue.  I've seen lots of frustrating
back-and-forth between admin and user before someone eventually
noticed the typo.

"Access denied or no such repo" is much better.  (The "not exported"
nuance is not relevant in this context; you can safely ignore it.)

regards

-- 
Sitaram

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Jakub Narebski @ 2011-10-15  8:16 UTC (permalink / raw)
  To: Sitaram Chamarty
  Cc: Junio C Hamano, Jeff King, Jonathan Nieder, Nguyen Thai Ngoc Duy,
	git, Ilari Liusvaara, Johannes Sixt
In-Reply-To: <CAMK1S_gkB49qhnt8U=3G3UPnjo2vzFx5mL4cOM1Ubu68ySJrDA@mail.gmail.com>

Sitaram Chamarty <sitaramc@gmail.com> writes:
> On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Sitaram Chamarty <sitaramc@gmail.com> writes:
>>
>>>> Because there is a risk of leaking information about
>>>> non-exported repositories, by default all errors simply say
>>>> "access denied". Sites which don't have hidden repositories,
>>>
>>> I suggest that even the "secure" version of the message say something
>>> like "access denied or repository not exported".  You're still not
>>> leaking anything, but it reduces confusion to the user, who otherwise
>>> may not realise it *could be* the latter.
>>
>> I kind of like the suggestion, but I am afraid that "access denied,
>> repository nonexistent or not exported" can soon easily get long enough to
>> be unmanageable.
> 
> When someone who *does* have access makes a typo, "access denied"
> makes it harder to realise it, because it subtly implies the repo
> *does* exist and it's an ACL issue.  I've seen lots of frustrating
> back-and-forth between admin and user before someone eventually
> noticed the typo.
> 
> "Access denied or no such repo" is much better.  (The "not exported"
> nuance is not relevant in this context; you can safely ignore it.)

To join this bike-shedding:

  "Access denied or repository not available"

or just

  "Repository not available"

-- 
Jakub Narębski

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Jonathan Nieder @ 2011-10-15  8:26 UTC (permalink / raw)
  To: Jakub Narebski
  Cc: Sitaram Chamarty, Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy,
	git, Ilari Liusvaara, Johannes Sixt
In-Reply-To: <m3r52e7js7.fsf@localhost.localdomain>

Jakub Narebski wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:

>> "Access denied or no such repo" is much better.  (The "not exported"
>> nuance is not relevant in this context; you can safely ignore it.)
>
> To join this bike-shedding:
>
>   "Access denied or repository not available"
>
> or just
>
>   "Repository not available"

If such details about the message matter, then I have to say that
Sitaram's "access denied or no such repository" is as close to perfect
as I can imagine.  The admin who is eventually forwarded this message
will be reminded to check two things:

 - that access is not denied, neither globally, using a whitelist,
   using filesystem permissions, nor by leaving out
   git-daemon-export-ok, and

 - that such a repo exists at all, and there was not a typo in the
   address.

^ permalink raw reply

* [PATCH] send-email: Honour SMTP domain when using TLS
From: Matthew Daley @ 2011-10-15  8:44 UTC (permalink / raw)
  To: git; +Cc: Matthew Daley

git-send-email sends two SMTP EHLOs when using TLS encryption, however
only the first, unencrypted EHLO uses the SMTP domain that can be
optionally specified by the user (--smtp-domain).  This is because the
call to hello() that produces the second, encrypted EHLO does not pass
the SMTP domain as an argument, and hence a default of
'localhost.localdomain' is used instead.

Fix by passing in the SMTP domain in this call.

Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 6885dfa..d491db9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1098,7 +1098,7 @@ X-Mailer: git-send-email $gitversion
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
 					# supported commands
-					$smtp->hello();
+					$smtp->hello($smtp_domain);
 				} else {
 					die "Server does not support STARTTLS! ".$smtp->message;
 				}
-- 
1.7.2.5

^ permalink raw reply related

* Re: [PATCH 7/8] mergetools: use the correct tool for Beyond Compare 3 on Windows
From: Sebastian Schuberth @ 2011-10-15 10:27 UTC (permalink / raw)
  To: git; +Cc: msysgit
In-Reply-To: <7vobxix0pk.fsf@alter.siamese.dyndns.org>

On 15.10.2011 07:50, Junio C Hamano wrote:

> Hmm, does this only apply to Windows, or are there other platforms on
> which BC3 supplies bcomp for the exact same reason? What I am trying to

BC3 is only available for Linux and Windows, so it only applies to 
Windows currently.

-- 
Sebastian Schuberth

^ permalink raw reply

* Re: [PATCH 5/8] t9901: fix line-ending dependency on windows
From: Pat Thoyts @ 2011-10-15 12:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git, msysGit
In-Reply-To: <7vsjmux0z3.fsf@alter.siamese.dyndns.org>

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

>Pat Thoyts <patthoyts@users.sourceforge.net> writes:
>
>> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
>> ---
>>  t/t9901-git-web--browse.sh |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
>> index 7906e5d..1185b42 100755
>> --- a/t/t9901-git-web--browse.sh
>> +++ b/t/t9901-git-web--browse.sh
>> @@ -12,7 +12,7 @@ test_expect_success \
>>  	echo http://example.com/foo\&bar >expect &&
>>  	git config browser.custom.cmd echo &&
>>  	git web--browse --browser=custom \
>> -		http://example.com/foo\&bar >actual &&
>> +		http://example.com/foo\&bar | tr -d "\r" >actual &&
>>  	test_cmp expect actual
>>  '
>
>This is unnice for two reasons. We have web--browse five times in this
>test script, and you add tr exactly the same way to each and every one of
>them. And you are losing the error condition from each and every one of
>these web--browse invocations.
>
>Having to do the same change to all invocations of the same command
>suggests that perhaps you can and should wrap that pattern into a single
>helper, perhaps like:
>
>test_web_browse () {
>	# browser=$1 url=$2
>	git web--browse --browser="$1" "$2" >actual &&
>        tr -d '\015' <actual >text &&
>        test_cmp expect text
>}
>
>or something?

OK - The suggested code works fine so I shall re-roll this one shortly.
-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [PATCH 7/8] mergetools: use the correct tool for Beyond Compare 3 on Windows
From: Pat Thoyts @ 2011-10-15 12:39 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: Junio C Hamano, Git, msysGit
In-Reply-To: <4E996012.8090002@gmail.com>

Sebastian Schuberth <sschuberth@gmail.com> writes:

>On 15.10.2011 07:50, Junio C Hamano wrote:
>
>> Hmm, does this only apply to Windows, or are there other platforms on
>> which BC3 supplies bcomp for the exact same reason? What I am trying to
>
>BC3 is only available for Linux and Windows, so it only applies to
>Windows currently.

And checking the linux distribution shows only a bcompare executable so
testing for the presence of 'bcomp' will be fine.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* [PATCH 2/9 v2] completion: optimize refs completion
From: SZEDER Gábor @ 2011-10-15 12:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Jonathan Nieder, SZEDER Gábor
In-Reply-To: <20111014121609.GB2208@goldbirke>

After a unique command or option is completed, in most cases it is a
good thing to add a trailing a space, but sometimes it doesn't make
sense, e.g. when the completed word is an option taking an argument
('--option=') or a configuration section ('core.').  Therefore the
completion script uses the '-o nospace' option to prevent bash from
automatically appending a space to unique completions, and it has the
__gitcomp() function to add that trailing space only when necessary.
See 72e5e989 (bash: Add space after unique command name is completed.,
2007-02-04), 78d4d6a2 (bash: Support unique completion on git-config.,
2007-02-04), and b3391775 (bash: Support unique completion when
possible., 2007-02-04).

__gitcomp() therefore iterates over all possible completion words it
got as argument, and checks each word whether a trailing space is
necessary or not.  This is ok for commands, options, etc., i.e. when
the number of words is relatively small, but can be noticeably slow
for large number of refs.  However, while options might or might not
need that trailing space, refs are always handled uniformly and always
get that trailing space (or a trailing '.' for 'git config
branch.<head>.').  Since refs listed by __git_refs() & co. are
separated by newline, this allows us some optimizations with
'compgen'.

So, add a specialized variant of __gitcomp() that only deals with
possible completion words separated by a newline and uniformly appends
the trailing space to all words using 'compgen -S " "' (or any other
suffix, if specified), so no iteration over all words is needed.  But
we need to fiddle with IFS, because the default IFS containing a space
would cause the added space suffix to be stripped off when compgen's
output is stored in the COMPREPLY array.  Therefore we use only
newline as IFS, hence the requirement for the newline-separated
possible completion words.

Convert all callsites of __gitcomp() where it's called with refs, i.e.
when it gets the output of either __git_refs(), __git_heads(),
__git_tags(), __git_refs2(), __git_refs_remotes(), or the odd 'git
for-each-ref' somewhere in _git_config().  Also convert callsites
where it gets other uniformly handled newline separated word lists,
i.e. either remotes from __git_remotes(), names of set configuration
variables from __git_config_get_set_variables(), stashes, or commands.

Here are some timing results for dealing with 10000 refs.
Before:

  $ refs="$(__git_refs ~/tmp/git/repo-with-10k-refs/)"
  $ time __gitcomp "$refs"

  real	0m1.134s
  user	0m1.060s
  sys	0m0.130s

After:

  $ time __gitcomp_nl "$refs"

  real	0m0.373s
  user	0m0.360s
  sys	0m0.020s

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

On Fri, Oct 14, 2011 at 02:16:09PM +0200, SZEDER Gábor wrote:
> Oops, this last hunk is wrong.

Here's the update with that buggy hunk removed.  I also updated the
comments before __gitcomp_nl() to be more explicit, and the commit
message with the IFS fiddling and the grammar errors you pointed out
earlier.

These changes don't conflict with later patches, so I resend only this
patch but not the whole series.

 contrib/completion/git-completion.bash |  115 +++++++++++++++++++------------
 1 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c0fb6e15..daabf827 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -512,6 +512,31 @@ __gitcomp ()
 	esac
 }
 
+# Generates completion reply with compgen from newline-separated possible
+# completion words by appending a space to all of them.
+# It accepts 1 to 4 arguments:
+# 1: List of possible completion words, separated by a single newline.
+# 2: A prefix to be added to each possible completion word (optional).
+# 3: Generate possible completion matches for this word (optional).
+# 4: A suffix to be appended to each possible completion word instead of
+#    the default space (optional).  If specified but empty, nothing is
+#    appended.
+__gitcomp_nl ()
+{
+	local s=$'\n' IFS=' '$'\t'$'\n'
+	local cur_="$cur" suffix=" "
+
+	if [ $# -gt 2 ]; then
+		cur_="$3"
+		if [ $# -gt 3 ]; then
+			suffix="$4"
+		fi
+	fi
+
+	IFS=$s
+	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
+}
+
 # __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
@@ -716,15 +741,15 @@ __git_complete_revlist_file ()
 	*...*)
 		pfx="${cur_%...*}..."
 		cur_="${cur_#*...}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*..*)
 		pfx="${cur_%..*}.."
 		cur_="${cur_#*..}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -764,7 +789,7 @@ __git_complete_remote_or_refspec ()
 		c=$((++c))
 	done
 	if [ -z "$remote" ]; then
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
@@ -789,23 +814,23 @@ __git_complete_remote_or_refspec ()
 	case "$cmd" in
 	fetch)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs2 "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs2 "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	pull)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	push)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		fi
 		;;
 	esac
@@ -1084,7 +1109,7 @@ _git_archive ()
 		return
 		;;
 	--remote=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--remote=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--remote=}"
 		return
 		;;
 	--*)
@@ -1115,7 +1140,7 @@ _git_bisect ()
 
 	case "$subcommand" in
 	bad|good|reset|skip|start)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
 		COMPREPLY=()
@@ -1146,9 +1171,9 @@ _git_branch ()
 		;;
 	*)
 		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
-			__gitcomp "$(__git_heads)"
+			__gitcomp_nl "$(__git_heads)"
 		else
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 		fi
 		;;
 	esac
@@ -1195,7 +1220,7 @@ _git_checkout ()
 		if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
 			track=''
 		fi
-		__gitcomp "$(__git_refs '' $track)"
+		__gitcomp_nl "$(__git_refs '' $track)"
 		;;
 	esac
 }
@@ -1212,7 +1237,7 @@ _git_cherry_pick ()
 		__gitcomp "--edit --no-commit"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -1266,7 +1291,7 @@ _git_commit ()
 		;;
 	--reuse-message=*|--reedit-message=*|\
 	--fixup=*|--squash=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		return
 		;;
 	--untracked-files=*)
@@ -1297,7 +1322,7 @@ _git_describe ()
 			"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 __git_diff_common_options="--stat --numstat --shortstat --summary
@@ -1456,7 +1481,7 @@ _git_grep ()
 		;;
 	esac
 
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_help ()
@@ -1514,7 +1539,7 @@ _git_ls_files ()
 
 _git_ls_remote ()
 {
-	__gitcomp "$(__git_remotes)"
+	__gitcomp_nl "$(__git_remotes)"
 }
 
 _git_ls_tree ()
@@ -1610,7 +1635,7 @@ _git_merge ()
 		__gitcomp "$__git_merge_options"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mergetool ()
@@ -1630,7 +1655,7 @@ _git_mergetool ()
 
 _git_merge_base ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mv ()
@@ -1661,7 +1686,7 @@ _git_notes ()
 	,*)
 		case "${words[cword-1]}" in
 		--ref)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		*)
 			__gitcomp "$subcommands --ref"
@@ -1670,7 +1695,7 @@ _git_notes ()
 		;;
 	add,--reuse-message=*|append,--reuse-message=*|\
 	add,--reedit-message=*|append,--reedit-message=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		;;
 	add,--*|append,--*)
 		__gitcomp '--file= --message= --reedit-message=
@@ -1689,7 +1714,7 @@ _git_notes ()
 		-m|-F)
 			;;
 		*)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		esac
 		;;
@@ -1717,12 +1742,12 @@ _git_push ()
 {
 	case "$prev" in
 	--repo)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	esac
 	case "$cur" in
 	--repo=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--repo=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
 		return
 		;;
 	--*)
@@ -1760,7 +1785,7 @@ _git_rebase ()
 
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reflog ()
@@ -1771,7 +1796,7 @@ _git_reflog ()
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 	else
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 	fi
 }
 
@@ -1853,23 +1878,23 @@ _git_config ()
 {
 	case "$prev" in
 	branch.*.remote)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 		;;
 	branch.*.merge)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		return
 		;;
 	remote.*.fetch)
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
-		__gitcomp "$(__git_refs_remotes "$remote")"
+		__gitcomp_nl "$(__git_refs_remotes "$remote")"
 		return
 		;;
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp "$(git --git-dir="$(__gitdir)" \
+		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
 			for-each-ref --format='%(refname):%(refname)' \
 			refs/heads)"
 		return
@@ -1916,7 +1941,7 @@ _git_config ()
 		return
 		;;
 	--get|--get-all|--unset|--unset-all)
-		__gitcomp "$(__git_config_get_set_variables)"
+		__gitcomp_nl "$(__git_config_get_set_variables)"
 		return
 		;;
 	*.*)
@@ -1942,7 +1967,7 @@ _git_config ()
 		;;
 	branch.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_heads)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
 		return
 		;;
 	guitool.*.*)
@@ -1971,7 +1996,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
 		__git_compute_all_commands
-		__gitcomp "$__git_all_commands" "$pfx" "$cur_"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_"
 		return
 		;;
 	remote.*.*)
@@ -1984,7 +2009,7 @@ _git_config ()
 		;;
 	remote.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_remotes)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
 		return
 		;;
 	url.*.*)
@@ -2285,7 +2310,7 @@ _git_remote ()
 
 	case "$subcommand" in
 	rename|rm|show|prune)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		;;
 	update)
 		local i c='' IFS=$'\n'
@@ -2303,7 +2328,7 @@ _git_remote ()
 
 _git_replace ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reset ()
@@ -2316,7 +2341,7 @@ _git_reset ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_revert ()
@@ -2327,7 +2352,7 @@ _git_revert ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_rm ()
@@ -2426,7 +2451,7 @@ _git_stash ()
 			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
-			__gitcomp "$(git --git-dir="$(__gitdir)" stash list \
+			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
@@ -2560,7 +2585,7 @@ _git_tag ()
 		i="${words[c]}"
 		case "$i" in
 		-d|-v)
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 			return
 			;;
 		-f)
@@ -2576,13 +2601,13 @@ _git_tag ()
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 		else
 			COMPREPLY=()
 		fi
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
-- 
1.7.7.197.g04a3e

^ permalink raw reply related

* Re: [PATCH 2/8] t1402: Ignore a few cases that must fail due to DOS path expansion
From: Pat Thoyts @ 2011-10-15 13:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git, msysGit, Johannes Schindelin
In-Reply-To: <7vwrc6x1cp.fsf@alter.siamese.dyndns.org>

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

>Pat Thoyts <patthoyts@users.sourceforge.net> writes:
>
>>  t/t1402-check-ref-format.sh |   15 +++++++++------
>
>Didn't we see a different patch that attempts to address the same issue
>recently on the list from J6t, or is this a fix for a different problem?
>

You are correct - I'll leave this out of this series then. j6t's patch
is an alternative fix for the same problem.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* [PATCH 0/7] Some patches from msysGit (round 2)
From: Pat Thoyts @ 2011-10-15 14:05 UTC (permalink / raw)
  To: Git; +Cc: Junio C Hamano, msysGit, Pat Thoyts

This collects some recent patches from the msysGit tree that clear up
test issues on Windows.

This second version incorporates suggestions received from round 1 to:
  avoid duplicating code in t9901 web-browse tests
  drop the t1402 changes in favour of another change from J6t (on pu)
  test for the presence of 'bcomp' in bc3 mergetool

Johannes Schindelin (3):
  t1020: disable the pwd test on MinGW
  t9001: do not fail only due to CR/LF issues
  t9300: do not run --cat-blob-fd related tests on MinGW

Pat Thoyts (3):
  t9901: fix line-ending dependency on windows
  mergetools: use the correct tool for Beyond Compare 3 on Windows
  mingw: ensure sockets are initialized before calling gethostname

Sebastian Schuberth (1):
  git-svn: On MSYS, escape and quote SVN_SSH also if set by the user

 compat/mingw.c             |    7 +++++++
 compat/mingw.h             |    3 +++
 git-svn.perl               |   15 +++++++--------
 mergetools/bc3             |    7 ++++++-
 t/t1020-subdirectory.sh    |    2 +-
 t/t9001-send-email.sh      |    1 +
 t/t9300-fast-import.sh     |    8 ++++----
 t/t9901-git-web--browse.sh |   32 +++++++++++++++++---------------
 8 files changed, 46 insertions(+), 29 deletions(-)

-- 
1.7.7.1.gbba15

^ permalink raw reply

* [PATCH 1/7] t1020: disable the pwd test on MinGW
From: Pat Thoyts @ 2011-10-15 14:05 UTC (permalink / raw)
  To: Git; +Cc: Junio C Hamano, msysGit, Johannes Schindelin
In-Reply-To: <1318687520-19522-1-git-send-email-patthoyts@users.sourceforge.net>

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It fails both for line ending and for DOS path reasons.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1020-subdirectory.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 3b1b985..e23ac0e 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -118,7 +118,7 @@ test_expect_success 'alias expansion' '
 	)
 '
 
-test_expect_success '!alias expansion' '
+test_expect_success NOT_MINGW '!alias expansion' '
 	pwd >expect &&
 	(
 		git config alias.test !pwd &&
-- 
1.7.7.1.gbba15

^ permalink raw reply related


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