From 51f8c814d5603a917a91a47019875bbe707675bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 11 May 2013 18:14:03 +0200 Subject: [PATCH 1/5] t5510: start tracking-ref tests from a known state We have three sequential tests for for whether tracking refs are updated by various fetches and pulls; the first two should not update the ref, and the third should. Each test depends on the state left by the test before. This is fragile (a failing early test will confuse later tests), and means we cannot add more "should update" tests after the third one. Let's instead save the initial state before these tests, and then reset to a known state before running each test. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5510-fetch.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index d7a19a182..789c22827 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -370,12 +370,20 @@ test_expect_success 'bundle should record HEAD correctly' ' ' +test_expect_success 'mark initial state of origin/master' ' + ( + cd three && + git tag base-origin-master refs/remotes/origin/master + ) +' + test_expect_success 'explicit fetch should not update tracking' ' cd "$D" && git branch -f side && ( cd three && + git update-ref refs/remotes/origin/master base-origin-master && o=$(git rev-parse --verify refs/remotes/origin/master) && git fetch origin master && n=$(git rev-parse --verify refs/remotes/origin/master) && @@ -390,6 +398,7 @@ test_expect_success 'explicit pull should not update tracking' ' git branch -f side && ( cd three && + git update-ref refs/remotes/origin/master base-origin-master && o=$(git rev-parse --verify refs/remotes/origin/master) && git pull origin master && n=$(git rev-parse --verify refs/remotes/origin/master) && @@ -404,6 +413,7 @@ test_expect_success 'configured fetch updates tracking' ' git branch -f side && ( cd three && + git update-ref refs/remotes/origin/master base-origin-master && o=$(git rev-parse --verify refs/remotes/origin/master) && git fetch origin && n=$(git rev-parse --verify refs/remotes/origin/master) && From 4ab90e7a5c46ce929ae8b3b7bbb5b276f2f60e0d Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Sat, 11 May 2013 18:14:25 +0200 Subject: [PATCH 2/5] fetch/pull doc: untangle meaning of bare The documentation erroneously used the same wording for both fetch and pull, stating that something will be merged even in git-fetch(1). In addition, saying that " is equivalent to :" doesn't really help anyone who still needs to read manpages. Clarify what is actually going on. Signed-off-by: Thomas Rast Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 94a9d32f1..6f5ca2174 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -68,6 +68,11 @@ Some short-cut notations are also supported. + * `tag ` means the same as `refs/tags/:refs/tags/`; it requests fetching everything up to the given tag. -* A parameter without a colon is equivalent to - : when pulling/fetching, so it merges into the current - branch without storing the remote branch anywhere locally +ifndef::git-pull[] +* A parameter without a colon fetches that ref into FETCH_HEAD, +endif::git-pull[] +ifdef::git-pull[] +* A parameter without a colon merges into the current + branch, +endif::git-pull[] + while not storing the branch anywhere locally. From 900f2814b8da951a450f5762dabb1b248dd77abc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 11 May 2013 18:15:59 +0200 Subject: [PATCH 3/5] refactor "ref->merge" flag Each "struct ref" has a boolean flag that is set by the fetch code to determine whether the ref should be marked as "not-for-merge" or not when we write it out to FETCH_HEAD. It would be useful to turn this boolean into a tri-state, with the third state meaning "do not bother writing it out to FETCH_HEAD at all". That would let us add extra refs to the set of refs to be stored (e.g., to store copies of things we fetched) without impacting FETCH_HEAD. This patch turns it into an enum that covers the tri-state case, and hopefully makes the code more explicit and easier to read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 57 ++++++++++++++++++++++++++++++------------------- cache.h | 14 +++++++++++- 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 4b6b1dfe6..287cf4ce8 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -119,7 +119,7 @@ static void add_merge_config(struct ref **head, for (rm = *head; rm; rm = rm->next) { if (branch_merge_matches(branch, i, rm->name)) { - rm->merge = 1; + rm->fetch_head_status = FETCH_HEAD_MERGE; break; } } @@ -140,7 +140,7 @@ static void add_merge_config(struct ref **head, refspec.src = branch->merge[i]->src; get_fetch_map(remote_refs, &refspec, tail, 1); for (rm = *old_tail; rm; rm = rm->next) - rm->merge = 1; + rm->fetch_head_status = FETCH_HEAD_MERGE; } } @@ -167,7 +167,7 @@ static struct ref *get_ref_map(struct transport *transport, } /* Merge everything on the command line, but not --tags */ for (rm = ref_map; rm; rm = rm->next) - rm->merge = 1; + rm->fetch_head_status = FETCH_HEAD_MERGE; if (tags == TAGS_SET) get_fetch_map(remote_refs, tag_refspec, &tail, 0); } else { @@ -186,7 +186,7 @@ static struct ref *get_ref_map(struct transport *transport, *autotags = 1; if (!i && !has_merge && ref_map && !remote->fetch[0].pattern) - ref_map->merge = 1; + ref_map->fetch_head_status = FETCH_HEAD_MERGE; } /* * if the remote we're fetching from is the same @@ -202,7 +202,7 @@ static struct ref *get_ref_map(struct transport *transport, ref_map = get_remote_ref(remote_refs, "HEAD"); if (!ref_map) die(_("Couldn't find remote ref HEAD")); - ref_map->merge = 1; + ref_map->fetch_head_status = FETCH_HEAD_MERGE; tail = &ref_map->next; } } @@ -389,7 +389,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; + int want_status; fp = fopen(filename, "a"); if (!fp) @@ -407,19 +407,22 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, } /* - * 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. + * We do a pass for each fetch_head_status type in their enum order, so + * merged entries are written before not-for-merge. That lets readers + * use FETCH_HEAD as a refname to refer to the ref to be merged. */ - for (want_merge = 1; 0 <= want_merge; want_merge--) { + for (want_status = FETCH_HEAD_MERGE; + want_status <= FETCH_HEAD_IGNORE; + want_status++) { for (rm = ref_map; rm; rm = rm->next) { struct ref *ref = NULL; + const char *merge_status_marker = ""; commit = lookup_commit_reference_gently(rm->old_sha1, 1); if (!commit) - rm->merge = 0; + rm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE; - if (rm->merge != want_merge) + if (rm->fetch_head_status != want_status) continue; if (rm->peer_ref) { @@ -465,16 +468,26 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_addf(¬e, "%s ", kind); strbuf_addf(¬e, "'%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); + switch (rm->fetch_head_status) { + case FETCH_HEAD_NOT_FOR_MERGE: + merge_status_marker = "not-for-merge"; + /* fall-through */ + case FETCH_HEAD_MERGE: + fprintf(fp, "%s\t%s\t%s", + sha1_to_hex(rm->old_sha1), + merge_status_marker, + note.buf); + for (i = 0; i < url_len; ++i) + if ('\n' == url[i]) + fputs("\\n", fp); + else + fputc(url[i], fp); + fputc('\n', fp); + break; + default: + /* do not write anything to FETCH_HEAD */ + break; + } strbuf_reset(¬e); if (ref) { diff --git a/cache.h b/cache.h index 94ca1acf7..9670d99f2 100644 --- a/cache.h +++ b/cache.h @@ -1024,9 +1024,21 @@ struct ref { unsigned int force:1, forced_update:1, - merge:1, deletion:1, matched:1; + + /* + * Order is important here, as we write to FETCH_HEAD + * in numeric order. And the default NOT_FOR_MERGE + * should be 0, so that xcalloc'd structures get it + * by default. + */ + enum { + FETCH_HEAD_MERGE = -1, + FETCH_HEAD_NOT_FOR_MERGE = 0, + FETCH_HEAD_IGNORE = 1 + } fetch_head_status; + enum { REF_STATUS_NONE = 0, REF_STATUS_OK, From f269048754f3b835f4f7287c5a132714a059efce Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 11 May 2013 18:16:52 +0200 Subject: [PATCH 4/5] fetch: opportunistically update tracking refs When we run a regular "git fetch" without arguments, we update the tracking refs according to the configured refspec. However, when we run "git fetch origin master" (or "git pull origin master"), we do not look at the configured refspecs at all, and just update FETCH_HEAD. We miss an opportunity to update "refs/remotes/origin/master" (or whatever the user has configured). Some users find this confusing, because they would want to do further comparisons against the old state of the remote master, like: $ git pull origin master $ git log HEAD...origin/master In the currnet code, they are comparing against whatever commit happened to be in origin/master from the last time they did a complete "git fetch". This patch will update a ref from the RHS of a configured refspec whenever we happen to be fetching its LHS. That makes the case above work. The downside is that any users who really care about whether and when their tracking branches are updated may be surprised. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 2 +- builtin/fetch.c | 16 ++++++++++++++++ t/t5510-fetch.sh | 8 ++++---- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 6f5ca2174..18cffc25b 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -75,4 +75,4 @@ ifdef::git-pull[] * A parameter without a colon merges into the current branch, endif::git-pull[] - while not storing the branch anywhere locally. + and updates the remote-tracking branches (if any). diff --git a/builtin/fetch.c b/builtin/fetch.c index 287cf4ce8..e41cc0d73 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -160,6 +160,8 @@ static struct ref *get_ref_map(struct transport *transport, const struct ref *remote_refs = transport_get_remote_refs(transport); if (ref_count || tags == TAGS_SET) { + struct ref **old_tail; + for (i = 0; i < ref_count; i++) { get_fetch_map(remote_refs, &refs[i], &tail, 0); if (refs[i].dst && refs[i].dst[0]) @@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport, rm->fetch_head_status = FETCH_HEAD_MERGE; if (tags == TAGS_SET) get_fetch_map(remote_refs, tag_refspec, &tail, 0); + + /* + * For any refs that we happen to be fetching via command-line + * arguments, take the opportunity to update their configured + * counterparts. However, we do not want to mention these + * entries in FETCH_HEAD at all, as they would simply be + * duplicates of existing entries. + */ + old_tail = tail; + for (i = 0; i < transport->remote->fetch_refspec_nr; i++) + get_fetch_map(ref_map, &transport->remote->fetch[i], + &tail, 0); + for (rm = *old_tail; rm; rm = rm->next) + rm->fetch_head_status = FETCH_HEAD_IGNORE; } else { /* Use the defaults */ struct remote *remote = transport->remote; diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 789c22827..ff43e0875 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -377,7 +377,7 @@ test_expect_success 'mark initial state of origin/master' ' ) ' -test_expect_success 'explicit fetch should not update tracking' ' +test_expect_success 'explicit fetch should update tracking' ' cd "$D" && git branch -f side && @@ -387,12 +387,12 @@ test_expect_success 'explicit fetch should not update tracking' ' o=$(git rev-parse --verify refs/remotes/origin/master) && git fetch origin master && n=$(git rev-parse --verify refs/remotes/origin/master) && - test "$o" = "$n" && + test "$o" != "$n" && test_must_fail git rev-parse --verify refs/remotes/origin/side ) ' -test_expect_success 'explicit pull should not update tracking' ' +test_expect_success 'explicit pull should update tracking' ' cd "$D" && git branch -f side && @@ -402,7 +402,7 @@ test_expect_success 'explicit pull should not update tracking' ' o=$(git rev-parse --verify refs/remotes/origin/master) && git pull origin master && n=$(git rev-parse --verify refs/remotes/origin/master) && - test "$o" = "$n" && + test "$o" != "$n" && test_must_fail git rev-parse --verify refs/remotes/origin/side ) ' From 823c6d56a83f3eec657a94581c23c5b7682bdaf4 Mon Sep 17 00:00:00 2001 From: John Keeping Date: Mon, 27 May 2013 17:33:09 +0100 Subject: [PATCH 5/5] fetch: don't try to update unfetched tracking refs Since commit f269048 (fetch: opportunistically update tracking refs, 2013-05-11) we update tracking refs opportunistically when fetching remote branches. However, if there is a configured non-pattern refspec that does not match any of the refspecs given on the command line then a fatal error occurs. Fix this by setting the "missing_ok" flag when calling get_fetch_map. Test-added-by: Jeff King Signed-off-by: John Keeping Acked-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- t/t5510-fetch.sh | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index e41cc0d73..d15a7343d 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -183,7 +183,7 @@ static struct ref *get_ref_map(struct transport *transport, old_tail = tail; for (i = 0; i < transport->remote->fetch_refspec_nr; i++) get_fetch_map(ref_map, &transport->remote->fetch[i], - &tail, 0); + &tail, 1); for (rm = *old_tail; rm; rm = rm->next) rm->fetch_head_status = FETCH_HEAD_IGNORE; } else { diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ff43e0875..fde689166 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -422,6 +422,22 @@ test_expect_success 'configured fetch updates tracking' ' ) ' +test_expect_success 'non-matching refspecs do not confuse tracking update' ' + cd "$D" && + git update-ref refs/odd/location HEAD && + ( + cd three && + git update-ref refs/remotes/origin/master base-origin-master && + git config --add remote.origin.fetch \ + refs/odd/location:refs/remotes/origin/odd && + o=$(git rev-parse --verify refs/remotes/origin/master) && + git fetch origin master && + n=$(git rev-parse --verify refs/remotes/origin/master) && + test "$o" != "$n" && + test_must_fail git rev-parse --verify refs/remotes/origin/odd + ) +' + test_expect_success 'pushing nonexistent branch by mistake should not segv' ' cd "$D" &&