[RFC/PATCH 0/4] update tracking refs on explicit fetch

STALE3738d

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

[RFC/PATCH 0/4] update tracking refs on explicit fetch

From: Jeff King <hidden>
Date: 2016-06-15 22:57:13

This is a cleaned-up version of the oft-discussed[1] concept that "git
pull origin master" should update "refs/remotes/origin/master".

It is a little bit of a risky change, in that anybody who deeply cares
about when their tracking ref branches are updated would be impacted.
But I think the general consensus is that while you can come up with a
hypothetical use case, the logic is quite tortured, and you could
accomplish the same thing by keeping a local branch. Most people seem to
have the mental model that remote tracking branches are basically a
cache for what's on the remote, updated opportunistically as
appropriate.

  [1/4]: t5510: start tracking-ref tests from a known state
  [2/4]: fetch/pull doc: untangle meaning of bare <ref>
  [3/4]: refactor "ref->merge" flag
  [4/4]: fetch: opportunistically update tracking refs

The final patch is the moral equivalent of the patch I've posted before
when this topic comes up (and referenced in the threads below). But that
patch did not take care not to write duplicate (mergable!) entries into
FETCH_HEAD, leading to great confusion from git-pull.  This series deals
with that, and takes care to write the exact same FETCH_HEAD we would
have written before the series.

However, Jonathan raised an interesting question: what is the point of
not-for-merge entries at all? We are not going to merge them, so none of
the pull/merge machinery cares about them. We do not consider extra
entries in FETCH_HEAD for reachability or anything like that. Is there
anybody who reads them?

If we do not care about them, we can simplify this much further, and
either write the duplicates as not-for-merge, or even omit not-for-merge
entries entirely. However, it is possible that some third-party scripts
process FETCH_HEAD, so I took the conservative route.

Note that this patch is as greedy as possible[2]; whenever we see the
LHS is fetched, we update the RHS side, even if the user did:

  git fetch origin master:foobar

Some of the discussions below argue that the behavior should not kick in
for such a case. I am not sure I agree (and argue against it in those
discussions). I think at this point that is the only potentially
contentious item.

-Peff

[1] Past discussions:

    http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215

    http://thread.gmane.org/gmane.comp.version-control.git/192252

    http://thread.gmane.org/gmane.comp.version-control.git/203357/focus=203442

    http://article.gmane.org/gmane.comp.version-control.git/221234

[PATCH 1/4] t5510: start tracking-ref tests from a known state

From: Jeff King <hidden>
Date: 2016-06-15 22:57:13

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 <redacted>
---
 t/t5510-fetch.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d7a19a1..789c228 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh

@@ -370,12 +370,20 @@ test_expect_success 'explicit fetch should not update tracking' '
 
 '
 
+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) &&

-- 
1.8.3.rc1.2.g12db477

[PATCH 2/4] fetch/pull doc: untangle meaning of bare <ref>

From: Jeff King <hidden>
Date: 2016-06-15 22:57:13

From: Thomas Rast <redacted>

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 "<ref> is equivalent to <ref>:" doesn't
really help anyone who still needs to read manpages.  Clarify what is
actually going on.

Signed-off-by: Thomas Rast <redacted>
Signed-off-by: Jeff King <redacted>
---
 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 94a9d32..6f5ca21 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 <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`;
   it requests fetching everything up to the given tag.
-* A parameter <ref> without a colon is equivalent to
-  <ref>: when pulling/fetching, so it merges <ref> into the current
-  branch without storing the remote branch anywhere locally
+ifndef::git-pull[]
+* A parameter <ref> without a colon fetches that ref into FETCH_HEAD,
+endif::git-pull[]
+ifdef::git-pull[]
+* A parameter <ref> without a colon merges <ref> into the current
+  branch,
+endif::git-pull[]
+  while not storing the branch anywhere locally.

-- 
1.8.3.rc1.2.g12db477

[PATCH 3/4] refactor "ref->merge" flag

From: Jeff King <hidden>
Date: 2016-06-15 22:57:13

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 <redacted>
---
 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 4b6b1df..287cf4c 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(&note, "%s ", kind);
 				strbuf_addf(&note, "'%s' of ", what);
 			}
-			fprintf(fp, "%s\t%s\t%s",
-				sha1_to_hex(rm->old_sha1),
-				rm->merge ? "" : "not-for-merge",
-				note.buf);
-			for (i = 0; i < url_len; ++i)
-				if ('\n' == url[i])
-					fputs("\\n", fp);
-				else
-					fputc(url[i], fp);
-			fputc('\n', fp);
+			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(&note);
 			if (ref) {

diff --git a/cache.h b/cache.h
index 94ca1ac..9670d99 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,

-- 
1.8.3.rc1.2.g12db477

[PATCH 4/4] fetch: opportunistically update tracking refs

From: Jeff King <hidden>
Date: 2016-06-15 22:57:13

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 <redacted>
---
 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 6f5ca21..18cffc2 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt

@@ -75,4 +75,4 @@ endif::git-pull[]
 * A parameter <ref> without a colon merges <ref> 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 287cf4c..e41cc0d 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 789c228..ff43e08 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
 	)
 '

-- 
1.8.3.rc1.2.g12db477

Re: [PATCH 1/4] t5510: start tracking-ref tests from a known state

From: Eric Sunshine <hidden>
Date: 2016-06-15 22:57:15

On Sat, May 11, 2013 at 12:14 PM, Jeff King [off-list ref] wrote:

We have three sequential tests for for whether tracking refs

s/for for/for/
[or]
s/for for/for checking/

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 <redacted>

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help