threads / patch / 10807

patch, 6 partsadd ref_abbrev_matches_full_with_rules()

Subject: [PATCH 4/6] add ref_abbrev_matches_full_with_rules()

## tl;dr

18 messages between Nov 11, 2007 and Nov 12, 2007. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Steffen Prohaska· Nov 11, 2007, 14:01 UTC · lore

[REPLACEMENT PATCH 0/6] improve refspec handling in push, refactor matching in fetch

This is a replacement for sp/push-refspec. It is rebased to the current master.
 Documentation/git-push.txt      |    4 ++--
 Documentation/git-send-pack.txt |    4 +++-
 builtin-push.c                  |   11 +++++++++++
 cache.h                         |    4 ++++
 refs.c                          |   31 +++++++++++++++++++++++++++++++
 remote.c                        |   28 +++-------------------------
 sha1_name.c                     |   14 ++------------
 t/t5510-fetch.sh                |   25 +++++++++++++++++++++++++
 t/t5516-fetch-push.sh           |   29 ++++++++++++++++++++++++++++-
 transport.c                     |    8 ++++++--
 transport.h                     |    1 +
 11 files changed, 116 insertions(+), 43 deletions(-)
 [PATCH 1/6] push: mention --verbose option in documentation
    Push uses parseopts, which supports '--verbose'.
 [PATCH 2/6] push: teach push to pass --verbose option to transport layer
    Adapted to paresopts changes.
 [PATCH 3/6] push: support pushing HEAD to real branch name
    Same as before.
 [PATCH 4/6] add ref_abbrev_matches_full_with_rules()
 [PATCH 5/6] push: use same rules as git-rev-parse to resolve refspecs
 [PATCH 6/6] refactor fetch's ref matching to use ref_abbrev_matches_full_with_rules()
    Start unification of refspec matching rules.
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

[PATCH 1/6] push: mention --verbose option in documentation

From: Steffen Prohaska <gitster@pobox.com>
Before this commit, only '-v' was documented.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/git-push.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/git-push.txt +2 −2
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index e5dd4c1..4a68aab 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git-push' [--all] [--dry-run] [--tags] [--receive-pack=<git-receive-pack>]
-           [--repo=all] [-f | --force] [-v] [<repository> <refspec>...]
+           [--repo=all] [-f | --force] [-v | --verbose] [<repository> <refspec>...]
 
 DESCRIPTION
 -----------
@@ -95,7 +95,7 @@ the remote repository.
 	transfer spends extra cycles to minimize the number of
 	objects to be sent and meant to be used on slower connection.
 
--v::
+-v, \--verbose::
 	Run verbosely.
 
 include::urls-remotes.txt[]
-- 
1.5.3.5.578.g886d
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

[PATCH 2/6] push: teach push to pass --verbose option to transport layer

From: Steffen Prohaska <gitster@pobox.com>

A --verbose option to push should also be passed to the transport layer, i.e. git-send-pack, git-http-push.

git push is modified to do so.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-push.c |    2 ++
 transport.c    |    8 ++++++--
 transport.h    |    1 +
 3 files changed, 9 insertions(+), 2 deletions(-)
Show changes to 3 files +9 −2

builtin-push.c, transport.c, transport.h

diff --git a/builtin-push.c b/builtin-push.c
index 2c56195..6d1da07 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -115,6 +115,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		flags |= TRANSPORT_PUSH_FORCE;
 	if (dry_run)
 		flags |= TRANSPORT_PUSH_DRY_RUN;
+	if (verbose)
+		flags |= TRANSPORT_PUSH_VERBOSE;
 	if (tags)
 		add_refspec("refs/tags/*");
 	if (all)
diff --git a/transport.c b/transport.c
index fa5cfbb..e8a2608 100644
--- a/transport.c
+++ b/transport.c
@@ -386,7 +386,7 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 	int argc;
 	int err;
 
-	argv = xmalloc((refspec_nr + 11) * sizeof(char *));
+	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
 	argv[0] = "http-push";
 	argc = 1;
 	if (flags & TRANSPORT_PUSH_ALL)
@@ -395,6 +395,8 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 		argv[argc++] = "--force";
 	if (flags & TRANSPORT_PUSH_DRY_RUN)
 		argv[argc++] = "--dry-run";
+	if (flags & TRANSPORT_PUSH_VERBOSE)
+		argv[argc++] = "--verbose";
 	argv[argc++] = transport->url;
 	while (refspec_nr--)
 		argv[argc++] = *refspec++;
@@ -655,7 +657,7 @@ static int git_transport_push(struct transport *transport, int refspec_nr, const
 	int argc;
 	int err;
 
-	argv = xmalloc((refspec_nr + 11) * sizeof(char *));
+	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
 	argv[0] = "send-pack";
 	argc = 1;
 	if (flags & TRANSPORT_PUSH_ALL)
@@ -664,6 +666,8 @@ static int git_transport_push(struct transport *transport, int refspec_nr, const
 		argv[argc++] = "--force";
 	if (flags & TRANSPORT_PUSH_DRY_RUN)
 		argv[argc++] = "--dry-run";
+	if (flags & TRANSPORT_PUSH_VERBOSE)
+		argv[argc++] = "--verbose";
 	if (data->receivepack) {
 		char *rp = xmalloc(strlen(data->receivepack) + 16);
 		sprintf(rp, "--receive-pack=%s", data->receivepack);
diff --git a/transport.h b/transport.h
index df12ea7..2f80ab4 100644
--- a/transport.h
+++ b/transport.h
@@ -30,6 +30,7 @@ struct transport {
 #define TRANSPORT_PUSH_ALL 1
 #define TRANSPORT_PUSH_FORCE 2
 #define TRANSPORT_PUSH_DRY_RUN 4
+#define TRANSPORT_PUSH_VERBOSE 8
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
1.5.3.5.578.g886d
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

[PATCH 3/6] push: support pushing HEAD to real branch name

From: Steffen Prohaska <gitster@pobox.com>

This teaches "push <remote> HEAD" to resolve HEAD on the local side to its real branch name, e.g. master, and then act as if the real branch name was specified. So we have a shorthand for pushing the current branch. Besides HEAD, no other symbolic ref is resolved.

Thanks to Daniel Barkalow <barkalow@iabervon.org> for suggesting this implementation, which is much simpler than the implementation proposed before.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-push.c        |    9 +++++++++
 t/t5516-fetch-push.sh |   17 +++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)
Show changes to 2 files +26 −0

builtin-push.c, t/t5516-fetch-push.sh

diff --git a/builtin-push.c b/builtin-push.c
index 6d1da07..a99ba0c 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -44,6 +44,15 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
+		if (!strcmp("HEAD", ref)) {
+			unsigned char sha1_dummy[20];
+			ref = resolve_ref(ref, sha1_dummy, 1, NULL);
+			if (!ref)
+				die("HEAD cannot be resolved.");
+			if (strncmp(ref, "refs/heads/", 11))
+				die("HEAD cannot be resolved to branch.");
+			ref = xstrdup(ref + 11);
+		}
 		add_refspec(ref);
 	}
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 86f9b53..b0ff488 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,23 @@ test_expect_success 'push with colon-less refspec (4)' '
 
 '
 
+test_expect_success 'push with HEAD' '
+
+	mk_test heads/master &&
+	git checkout master &&
+	git push testrepo HEAD &&
+	check_push_result $the_commit heads/master
+
+'
+
+test_expect_success 'push with HEAD nonexisting at remote' '
+
+	mk_test heads/master &&
+	git checkout -b local master &&
+	git push testrepo HEAD &&
+	check_push_result $the_commit heads/local
+'
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-- 
1.5.3.5.578.g886d
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

We use at least two rulesets for matching abbreviated refnames with full refnames (starting with 'refs/'). git-rev-parse and git-fetch use slightly different rules.

This commit introduces a new function
ref_abbrev_matches_full_with_rules(
    const char *abbrev_name, const char *full_name, const char **rules).
abbrev_name is expanded using the rules and matched against full_name.
If a match is found the function returns true.  rules is a NULL-terminate
list of format patterns with "%.*s", for example
    const char *ref_rev_parse_rules[] = {
               "%.*s",
               "refs/%.*s",
               "refs/tags/%.*s",
               "refs/heads/%.*s",
               "refs/remotes/%.*s",
               "refs/remotes/%.*s/HEAD",
               NULL
    };

Asterisks are included in the format strings because this is the form required in sha1_name.c. Sharing the list with the functions there is a good idea to avoid duplicating the rules. Hopefully this facilitates unified matching rules in the future.

This commit makes the rules used by rev-parse for resolving refs to sha1s available for string comparison. Before this change, the rules were buried in get_sha1*() and dwim_ref().

A follow-up commit will refactor the rules used by fetch.

ref_abbrev_matches_full_with_rules() will be used for matching refspecs in git-send-pack.

Thanks to Daniel Barkalow <barkalow@iabervon.org> for pointing out that ref_matches_abbrev in remote.c solves a similar problem and care should be taken to avoid confusion.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 cache.h     |    3 +++
 refs.c      |   24 ++++++++++++++++++++++++
 sha1_name.c |   14 ++------------
 3 files changed, 29 insertions(+), 12 deletions(-)
Show changes to 3 files +29 −12

cache.h, refs.c, sha1_name.c

diff --git a/cache.h b/cache.h
index f0a25c7..d36b91d 100644
--- a/cache.h
+++ b/cache.h
@@ -409,6 +409,9 @@ extern const char *resolve_ref(const char *path, unsigned char *sha1, int, int *
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 
+extern int ref_abbrev_matches_full_with_rules(const char *abbrev_name, const char *full_name, const char **rules);
+extern const char *ref_rev_parse_rules[];
+
 extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
 extern int validate_headref(const char *ref);
 
diff --git a/refs.c b/refs.c
index aff02cd..4bb16e5 100644
--- a/refs.c
+++ b/refs.c
@@ -655,6 +655,30 @@ int check_ref_format(const char *ref)
 	}
 }
 
+const char *ref_rev_parse_rules[] = {
+	"%.*s",
+	"refs/%.*s",
+	"refs/tags/%.*s",
+	"refs/heads/%.*s",
+	"refs/remotes/%.*s",
+	"refs/remotes/%.*s/HEAD",
+	NULL
+};
+
+int ref_abbrev_matches_full_with_rules(const char *abbrev_name, const char *full_name, const char **rules)
+{
+	const char **p;
+	const int abbrev_name_len = strlen(abbrev_name);
+
+	for (p = rules; *p; p++) {
+		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 static struct ref_lock *verify_lock(struct ref_lock *lock,
 	const unsigned char *old_sha1, int mustexist)
 {
diff --git a/sha1_name.c b/sha1_name.c
index 2d727d5..d364244 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -239,23 +239,13 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
-static const char *ref_fmt[] = {
-	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
-	NULL
-};
-
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	const char **p, *r;
 	int refs_found = 0;
 
 	*ref = NULL;
-	for (p = ref_fmt; *p; p++) {
+	for (p = ref_rev_parse_rules; *p; p++) {
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
 
@@ -277,7 +267,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 	int logs_found = 0;
 
 	*log = NULL;
-	for (p = ref_fmt; *p; p++) {
+	for (p = ref_rev_parse_rules; *p; p++) {
 		struct stat st;
 		unsigned char hash[20];
 		char path[PATH_MAX];
-- 
1.5.3.5.578.g886d
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

[PATCH 5/6] push: use same rules as git-rev-parse to resolve refspecs

This commit changes the rules for resolving refspecs to match the rules for resolving refs in rev-parse. git-rev-parse uses clear rules to resolve a short ref to its full name, which are well documented. The rules for resolving refspecs documented in git-send-pack were less strict and harder to understand. This commit replaces them by the rules of git-rev-parse.

The unified rules are easier to understand and better resolve ambiguous cases. You can now push from a repository containing several branches ending on the same short name.

Note, this may break existing setups. For example "master" will no longer resolve to "origin/master".

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/git-send-pack.txt |    4 +++-
 remote.c                        |    5 +----
 t/t5516-fetch-push.sh           |   12 +++++++++++-
 3 files changed, 15 insertions(+), 6 deletions(-)
Show changes to 3 files +15 −6

Documentation/git-send-pack.txt, remote.c, t/t5516-fetch-push.sh

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2fa01d4..a2d9cb6 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -85,7 +85,9 @@ Each pattern pair consists of the source side (before the colon)
 and the destination side (after the colon).  The ref to be
 pushed is determined by finding a match that matches the source
 side, and where it is pushed is determined by using the
-destination side.
+destination side. The rules used to match a ref are the same
+rules used by gitlink:git-rev-parse[1] to resolve a symbolic ref
+name.
 
  - It is an error if <src> does not match exactly one of the
    local refs.
diff --git a/remote.c b/remote.c
index bec2ba1..28d8eb7 100644
--- a/remote.c
+++ b/remote.c
@@ -519,10 +519,7 @@ static int count_refspec_match(const char *pattern,
 		char *name = refs->name;
 		int namelen = strlen(name);
 
-		if (namelen < patlen ||
-		    memcmp(name + namelen - patlen, pattern, patlen))
-			continue;
-		if (namelen != patlen && name[namelen - patlen - 1] != '/')
+		if (!ref_abbrev_matches_full_with_rules(pattern, name, ref_rev_parse_rules))
 			continue;
 
 		/* A match is "weak" if it is with refs outside
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b0ff488..fd5f284 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -145,11 +145,21 @@ test_expect_success 'push with no ambiguity (1)' '
 test_expect_success 'push with no ambiguity (2)' '
 
 	mk_test remotes/origin/master &&
-	git push testrepo master:master &&
+	git push testrepo master:origin/master &&
 	check_push_result $the_commit remotes/origin/master
 
 '
 
+test_expect_success 'push with colon-less refspec, no ambiguity' '
+
+	mk_test heads/master heads/t/master &&
+	git branch -f t/master master &&
+	git push testrepo master &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_first_commit heads/t/master
+
+'
+
 test_expect_success 'push with weak ambiguity (1)' '
 
 	mk_test heads/master remotes/origin/master &&
-- 
1.5.3.5.578.g886d
Steffen Prohaska· Nov 11, 2007, 14:01 UTC · re: Steffen Prohaska · lore

[PATCH 6/6] refactor fetch's ref matching to use ref_abbrev_matches_full_with_rules()

The old rules used by fetch were coded as a series of ifs.  The old
rules are:
1) match full refname if it starts with "refs/" or matches "HEAD"
2) verify that full refname starts with "refs/"
3) match abbreviated name in "refs/" if it starts with "heads/",
    "tags/", or "remotes/".
4) match abbreviated name in "refs/heads/"

This is replaced by the new rules a) match full refname b) match abbreviated name prefixed with "refs/" c) match abbreviated name prefixed with "refs/heads/"

The details of the new rules are different from the old rules. We no longer verify that the full refname starts with "refs/". The new rule (a) matches any full string. The old rules (1) and (2) were stricter. Now, the caller is responsible for using sensible full refnames. This should be the case for the current code. The new rule (b) is less strict than old rule (3). The new rule accepts abbreviated names that start with a non-standard prefix below "refs/".

Despite this modifications the new rules should handle all cases as expected. Two tests are added to verify that fetch does not resolve short tags or HEAD in remotes.

We may even think about loosening the rules a bit more and unify them with the rev-parse rules. This would be done by replacing ref_ref_fetch_rules with ref_ref_parse_rules. Note, the two new test would break.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 cache.h          |    1 +
 refs.c           |    7 +++++++
 remote.c         |   23 ++---------------------
 t/t5510-fetch.sh |   25 +++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 21 deletions(-)
Show changes to 4 files +35 −21

cache.h, refs.c, remote.c, t/t5510-fetch.sh

diff --git a/cache.h b/cache.h
index d36b91d..1313378 100644
--- a/cache.h
+++ b/cache.h
@@ -411,6 +411,7 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 
 extern int ref_abbrev_matches_full_with_rules(const char *abbrev_name, const char *full_name, const char **rules);
 extern const char *ref_rev_parse_rules[];
+extern const char *ref_fetch_rules[];
 
 extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
 extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 4bb16e5..7db2146 100644
--- a/refs.c
+++ b/refs.c
@@ -665,6 +665,13 @@ const char *ref_rev_parse_rules[] = {
 	NULL
 };
 
+const char *ref_fetch_rules[] = {
+	"%.*s",
+	"refs/%.*s",
+	"refs/heads/%.*s",
+	NULL
+};
+
 int ref_abbrev_matches_full_with_rules(const char *abbrev_name, const char *full_name, const char **rules)
 {
 	const char **p;
diff --git a/remote.c b/remote.c
index 28d8eb7..2dfce70 100644
--- a/remote.c
+++ b/remote.c
@@ -417,25 +417,6 @@ int remote_has_url(struct remote *remote, const char *url)
 	return 0;
 }
 
-/*
- * Returns true if, under the matching rules for fetching, name is the
- * same as the given full name.
- */
-static int ref_matches_abbrev(const char *name, const char *full)
-{
-	if (!prefixcmp(name, "refs/") || !strcmp(name, "HEAD"))
-		return !strcmp(name, full);
-	if (prefixcmp(full, "refs/"))
-		return 0;
-	if (!prefixcmp(name, "heads/") ||
-	    !prefixcmp(name, "tags/") ||
-	    !prefixcmp(name, "remotes/"))
-		return !strcmp(name, full + 5);
-	if (prefixcmp(full + 5, "heads/"))
-		return 0;
-	return !strcmp(full + 11, name);
-}
-
 int remote_find_tracking(struct remote *remote, struct refspec *refspec)
 {
 	int find_src = refspec->src == NULL;
@@ -804,7 +785,7 @@ int branch_merge_matches(struct branch *branch,
 {
 	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	return ref_matches_abbrev(branch->merge[i]->src, refname);
+	return ref_abbrev_matches_full_with_rules(branch->merge[i]->src, refname, ref_fetch_rules);
 }
 
 static struct ref *get_expanded_map(struct ref *remote_refs,
@@ -843,7 +824,7 @@ static struct ref *find_ref_by_name_abbrev(struct ref *refs, const char *name)
 {
 	struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		if (ref_matches_abbrev(name, ref->name))
+		if (ref_abbrev_matches_full_with_rules(name, ref->name, ref_fetch_rules))
 			return ref;
 	}
 	return NULL;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index aad863d..2025742 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -95,6 +95,31 @@ test_expect_success 'fetch following tags' '
 
 '
 
+test_expect_failure 'fetch must not resolve short tag name' '
+
+	cd "$D" &&
+
+	mkdir five &&
+	cd five &&
+	git init &&
+
+	git fetch .. anno:five
+
+'
+
+test_expect_failure 'fetch must not resolve short remote name' '
+
+	cd "$D" &&
+	git-update-ref refs/remotes/six/HEAD HEAD
+
+	mkdir six &&
+	cd six &&
+	git init &&
+
+	git fetch .. six:six
+
+'
+
 test_expect_success 'create bundle 1' '
 	cd "$D" &&
 	echo >file updated again by origin &&
-- 
1.5.3.5.578.g886d
Jakub Narebski· Nov 11, 2007, 14:55 UTC · re: Steffen Prohaska · lore

Re: [PATCH 6/6] refactor fetch's ref matching to use ref_abbrev_matches_full_with_rules()

Steffen Prohaska wrote:
Show 29 quoted lines
> The old rules used by fetch were coded as a series of ifs.  The old
> rules are:
> 1) match full refname if it starts with "refs/" or matches "HEAD"
> 2) verify that full refname starts with "refs/"
> 3) match abbreviated name in "refs/" if it starts with "heads/",
>     "tags/", or "remotes/".
> 4) match abbreviated name in "refs/heads/"
> 
> This is replaced by the new rules
> a) match full refname
> b) match abbreviated name prefixed with "refs/"
> c) match abbreviated name prefixed with "refs/heads/"
> 
> The details of the new rules are different from the old rules.  We no
> longer verify that the full refname starts with "refs/".  The new rule
> (a) matches any full string.  The old rules (1) and (2) were stricter.
> Now, the caller is responsible for using sensible full refnames.  This
> should be the case for the current code.  The new rule (b) is less
> strict than old rule (3).  The new rule accepts abbreviated names that
> start with a non-standard prefix below "refs/".
> 
> Despite this modifications the new rules should handle all cases as
> expected.  Two tests are added to verify that fetch does not resolve
> short tags or HEAD in remotes.
> 
> We may even think about loosening the rules a bit more and unify them
> with the rev-parse rules.  This would be done by replacing
> ref_ref_fetch_rules with ref_ref_parse_rules.  Note, the two new test
> would break.
Does still "origin" matches "origin/HEAD" if we have emote "origin"?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Steffen Prohaska· Nov 11, 2007, 17:31 UTC · re: Jakub Narebski · lore

Re: [PATCH 6/6] refactor fetch's ref matching to use ref_abbrev_matches_full_with_rules()

On Nov 11, 2007, at 3:55 PM, Jakub Narebski wrote:
Show 37 quoted lines
> Steffen Prohaska wrote:
>
>> The old rules used by fetch were coded as a series of ifs.  The old
>> rules are:
>> 1) match full refname if it starts with "refs/" or matches "HEAD"
>> 2) verify that full refname starts with "refs/"
>> 3) match abbreviated name in "refs/" if it starts with "heads/",
>>     "tags/", or "remotes/".
>> 4) match abbreviated name in "refs/heads/"
>>
>> This is replaced by the new rules
>> a) match full refname
>> b) match abbreviated name prefixed with "refs/"
>> c) match abbreviated name prefixed with "refs/heads/"
>>
>> The details of the new rules are different from the old rules.  We no
>> longer verify that the full refname starts with "refs/".  The new  
>> rule
>> (a) matches any full string.  The old rules (1) and (2) were  
>> stricter.
>> Now, the caller is responsible for using sensible full refnames.   
>> This
>> should be the case for the current code.  The new rule (b) is less
>> strict than old rule (3).  The new rule accepts abbreviated names  
>> that
>> start with a non-standard prefix below "refs/".
>>
>> Despite this modifications the new rules should handle all cases as
>> expected.  Two tests are added to verify that fetch does not resolve
>> short tags or HEAD in remotes.
>>
>> We may even think about loosening the rules a bit more and unify them
>> with the rev-parse rules.  This would be done by replacing
>> ref_ref_fetch_rules with ref_ref_parse_rules.  Note, the two new test
>> would break.
>
> Does still "origin" matches "origin/HEAD" if we have emote "origin"?

No, and it did not before. fetch does _not_ match origin as refs/remotes/origin/HEAD. I added a test to confirm the old behaviour. Only the git-rev-parse rules match origin as refs/remotes/origin/HEAD.

	Steffen
Junio C Hamano· Nov 12, 2007, 19:51 UTC · re: Steffen Prohaska · lore

Re: [PATCH 5/6] push: use same rules as git-rev-parse to resolve refspecs

Steffen Prohaska <prohaska@zib.de> writes:
Show 14 quoted lines
> diff --git a/remote.c b/remote.c
> index bec2ba1..28d8eb7 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -519,10 +519,7 @@ static int count_refspec_match(const char *pattern,
>  		char *name = refs->name;
>  		int namelen = strlen(name);
>  
> -		if (namelen < patlen ||
> -		    memcmp(name + namelen - patlen, pattern, patlen))
> -			continue;
> -		if (namelen != patlen && name[namelen - patlen - 1] != '/')
> +		if (!ref_abbrev_matches_full_with_rules(pattern, name, ref_rev_parse_rules))
>  			continue;

I vaguely recall that in the old round this check used to be without negation '!' in the front. I think this version is correct.

Steffen Prohaska· Nov 12, 2007, 20:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/6] push: use same rules as git-rev-parse to resolve refspecs

On Nov 12, 2007, at 8:51 PM, Junio C Hamano wrote:
Show 22 quoted lines
> Steffen Prohaska <prohaska@zib.de> writes:
>
>> diff --git a/remote.c b/remote.c
>> index bec2ba1..28d8eb7 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -519,10 +519,7 @@ static int count_refspec_match(const char  
>> *pattern,
>>  		char *name = refs->name;
>>  		int namelen = strlen(name);
>>
>> -		if (namelen < patlen ||
>> -		    memcmp(name + namelen - patlen, pattern, patlen))
>> -			continue;
>> -		if (namelen != patlen && name[namelen - patlen - 1] != '/')
>> +		if (!ref_abbrev_matches_full_with_rules(pattern, name,  
>> ref_rev_parse_rules))
>>  			continue;
>
> I vaguely recall that in the old round this check used to be
> without negation '!' in the front.  I think this version is
> correct.

Yes. I started with a syntax inspired by strcmp. But later the function got match in its name. I think returning a match with 'true' is more natural; and reserving '-1, 0, 1' for compare (as in strcmp). Therefore I changed the return value.

With '!' is correct now. Without '!' was correct before.
	Steffen
Junio C Hamano· Nov 12, 2007, 19:51 UTC · re: Steffen Prohaska · lore

Re: [PATCH 4/6] add ref_abbrev_matches_full_with_rules()

Steffen Prohaska <prohaska@zib.de> writes:
Show 14 quoted lines
> +int ref_abbrev_matches_full_with_rules(const char *abbrev_name, const char *full_name, const char **rules)
> +{
> +	const char **p;
> +	const int abbrev_name_len = strlen(abbrev_name);
> +
> +	for (p = rules; *p; p++) {
> +		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

How about calling this simply "ref_abbrev_matches()" or "refname_match()" which is even shorter?

Steffen Prohaska· Nov 12, 2007, 20:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/6] add ref_abbrev_matches_full_with_rules()

On Nov 12, 2007, at 8:51 PM, Junio C Hamano wrote:
Show 21 quoted lines
> Steffen Prohaska <prohaska@zib.de> writes:
>
>> +int ref_abbrev_matches_full_with_rules(const char *abbrev_name,  
>> const char *full_name, const char **rules)
>> +{
>> +	const char **p;
>> +	const int abbrev_name_len = strlen(abbrev_name);
>> +
>> +	for (p = rules; *p; p++) {
>> +		if (!strcmp(full_name, mkpath(*p, abbrev_name_len,  
>> abbrev_name))) {
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>
> How about calling this simply "ref_abbrev_matches()" or
> "refname_match()" which is even shorter?
Yes. As you already did on pu. Thanks for cleaning up.
	Steffen
Andreas Ericsson· Nov 11, 2007, 14:17 UTC · re: Steffen Prohaska · lore

Re: [PATCH 3/6] push: support pushing HEAD to real branch name

Steffen Prohaska wrote:
Show 14 quoted lines
> diff --git a/builtin-push.c b/builtin-push.c
> index 6d1da07..a99ba0c 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -44,6 +44,15 @@ static void set_refspecs(const char **refs, int nr)
>  			strcat(tag, refs[i]);
>  			ref = tag;
>  		}
> +		if (!strcmp("HEAD", ref)) {
> +			unsigned char sha1_dummy[20];
> +			ref = resolve_ref(ref, sha1_dummy, 1, NULL);
> +			if (!ref)
> +				die("HEAD cannot be resolved.");
> +			if (strncmp(ref, "refs/heads/", 11))
Why not prefixcmp(ref, "refs/heads/")?
-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Steffen Prohaska· Nov 11, 2007, 14:35 UTC · re: Andreas Ericsson · lore

[REPLACEMENT PATCH 3/6] push: support pushing HEAD to real branch name

This teaches "push <remote> HEAD" to resolve HEAD on the local side to its real branch name, e.g. master, and then act as if the real branch name was specified. So we have a shorthand for pushing the current branch. Besides HEAD, no other symbolic ref is resolved.

Thanks to Daniel Barkalow <barkalow@iabervon.org> for suggesting this implementation, which is much simpler than the implementation proposed before.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 builtin-push.c        |    9 +++++++++
 t/t5516-fetch-push.sh |   17 +++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)
prefixcmp() is simpler.
    Steffen
Show changes to 2 files +26 −0

builtin-push.c, t/t5516-fetch-push.sh

diff --git a/builtin-push.c b/builtin-push.c
index 6d1da07..54fba0e 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -44,6 +44,15 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
+		if (!strcmp("HEAD", ref)) {
+			unsigned char sha1_dummy[20];
+			ref = resolve_ref(ref, sha1_dummy, 1, NULL);
+			if (!ref)
+				die("HEAD cannot be resolved.");
+			if (prefixcmp(ref, "refs/heads/"))
+				die("HEAD cannot be resolved to branch.");
+			ref = xstrdup(ref + 11);
+		}
 		add_refspec(ref);
 	}
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 86f9b53..b0ff488 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,23 @@ test_expect_success 'push with colon-less refspec (4)' '
 
 '
 
+test_expect_success 'push with HEAD' '
+
+	mk_test heads/master &&
+	git checkout master &&
+	git push testrepo HEAD &&
+	check_push_result $the_commit heads/master
+
+'
+
+test_expect_success 'push with HEAD nonexisting at remote' '
+
+	mk_test heads/master &&
+	git checkout -b local master &&
+	git push testrepo HEAD &&
+	check_push_result $the_commit heads/local
+'
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-- 
1.5.3.5.643.g20245
Steffen Prohaska· Nov 11, 2007, 14:10 UTC · re: Steffen Prohaska · lore

Re: [PATCH 1/6] push: mention --verbose option in documentation

On Nov 11, 2007, at 3:01 PM, Steffen Prohaska wrote:
> From: Steffen Prohaska <gitster@pobox.com>

This is obviously wrong. I have no clear idea how this happend. I remember I took the topic branch sp/push-refspec from pu and started to refactor. I used 'rebase' and 'rebase -i' a couple of times and now I have my name with Junios email address.

I'll investigate how this could happen.
	Steffen
Steffen Prohaska· Nov 11, 2007, 14:14 UTC · re: Steffen Prohaska · lore

Re: [PATCH 1/6] push: mention --verbose option in documentation

On Nov 11, 2007, at 3:10 PM, Steffen Prohaska wrote:
Show 11 quoted lines
>
> On Nov 11, 2007, at 3:01 PM, Steffen Prohaska wrote:
>
>> From: Steffen Prohaska <gitster@pobox.com>
>
> This is obviously wrong. I have no clear idea how this happend.
> I remember I took the topic branch sp/push-refspec from pu and
> started to refactor. I used 'rebase' and 'rebase -i' a couple
> of times and now I have my name with Junios email address.
>
> I'll investigate how this could happen.

The Author line was already wrong in Junio's pu branch, commit f31447f5f06200305393ca16e2eb9485e65dcccc, and I carried this over without noticing.

	Steffen
Junio C Hamano· Nov 11, 2007, 19:39 UTC · re: Steffen Prohaska · lore

Re: [PATCH 1/6] push: mention --verbose option in documentation

Steffen Prohaska <prohaska@zib.de> writes:
> The Author line was already wrong in Junio's pu branch, commit
> f31447f5f06200305393ca16e2eb9485e65dcccc,  and I carried this
> over without noticing.

Yes, I know I pushed out a faulty one on 'pu' in the past. I am suspecting that it probably was a misuse of "git commit --author" after _not_ applying a WS mangled patch and instead of typing, or something like that.

← back to recent threads