Re: [PATCH] fetch: print an error when declining to request an unadvertised object

3 messages, 2 authors, 2017-02-19 · open the first message on its own page

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

From: Junio C Hamano <hidden>
Date: 2017-02-12 23:49:24

Matt McCutchen [off-list ref] writes:

What do you think?  Do you not care about having a more specific error,
in which case I can copy the code from builtin/fetch-pack.c to
fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
the flag?  Or what?

The fact that we have the above two choices tells me that a two-step
approach may be an appropriate approach.

The first step is to teach fetch_refs_via_pack() that it should not
ignore the information returned in sought[].  It would add new code
similar to what cmd_fetch_pack() uses to notice and report errors
[*1*] to the function.  It would be a sensible first step, but would
not let the user know which of multiple causes of "not matched" we
noticed.

By "a more specific error", I think you are envisioning that the
current boolean "matched" is made into an enum that allows the
caller to tell how each request did not match [*2*].  That can be
the topic of the second patch and would have to touch filter_refs()
[*3*], cmd_fetch_pack() and fetch_refs_via_pack().

I do not have strong preference myself offhand between stopping at
the first step or completing both.

Even if you did only the first step, as long as the second step can
be done without reverting what the first step did [*4*] by somebody
who cares the "specific error" deeply enough, I am OK with that.  Of
course if you did both steps, that is fine by me as well ;-)


[Footnote]

*1* While I know that it is not right to die() in filter_refs(), and
    fetch_refs_via_pack() is a better place to notice errors, I do
    not offhand know if it is the right place to report errors, or a
    caller higher in the callchain may want the callee to be silent
    and wants to show its own error message (in which case the error
    may have to percolate upwards in the callchain).

*2* e.g. "was it a ref but they did not advertise?  Did it request
    an explicit object name and they did not allow it?"  We may want
    to support other "more specific" errors that can be detected in
    the future.

*3* The current code flips the sought[i]->matched bit on for matched
    ones (relying on the initial state of the bit being false), but
    it now needs to stuff different kind of "not matched" to the
    field to allow the caller to act on it.

*4* IOW, I am OK with an initial "small" improvement, but I'd want
    to make sure that such an initial step does not make future
    enhancements by others harder.

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

From: Matt McCutchen <hidden>
Date: 2017-02-19 02:08:40

On Sun, 2017-02-12 at 15:49 -0800, Junio C Hamano wrote:

The fact that we have the above two choices tells me that a two-step
approach may be an appropriate approach. [...]

Even if you did only the first step, as long as the second step can
be done without reverting what the first step did [*4*] by somebody
who cares the "specific error" deeply enough, I am OK with that.  Of
course if you did both steps, that is fine by me as well ;-)

I appreciate the flexibility, but now that I've spent the time to
understand all the code involved, it would be a pity not to go for the
complete solution.  New patch coming.

Matt

[PATCH] fetch: print an error when declining to request an unadvertised object

From: Matt McCutchen <hidden>
Date: 2017-02-19 02:08:42

Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1.  The more common case
of requesting a nonexistent ref normally triggers a die() in
get_fetch_map, so "git fetch" wasn't bothering to check after the fetch
that it got all the refs it sought, like "git fetch-pack" does near the
end of cmd_fetch_pack.

Move the code from cmd_fetch_pack to a new function,
report_unmatched_refs, that is called by fetch_refs_via_pack as part of
"git fetch".  Also, change filter_refs (which checks whether a request
for an unadvertised object should be sent to the server) to set a new
match status on the "struct ref" when the request is not allowed, and
have report_unmatched_refs check for this status and print a special
error message, "Server does not allow request for unadvertised object".

Finally, add a simple test case for "git fetch REMOTE SHA1".

Signed-off-by: Matt McCutchen <redacted>
---
 builtin/fetch-pack.c  |  7 +------
 fetch-pack.c          | 51 ++++++++++++++++++++++++++++++++++++++-------------
 fetch-pack.h          |  9 +++++++++
 remote.h              |  9 +++++++--
 t/t5516-fetch-push.sh |  3 ++-
 transport.c           | 14 +++++++++-----
 6 files changed, 66 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e44..2a1c1c2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c

@@ -219,12 +219,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	 * remote no-such-ref' would silently succeed without issuing
 	 * an error.
 	 */
-	for (i = 0; i < nr_sought; i++) {
-		if (!sought[i] || sought[i]->matched)
-			continue;
-		error("no such remote ref %s", sought[i]->name);
-		ret = 1;
-	}
+	ret |= report_unmatched_refs(sought, nr_sought);
 
 	while (ref) {
 		printf("%s %s\n",

diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..f12bfcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -578,7 +578,7 @@ static void filter_refs(struct fetch_pack_args *args,
 					break; /* definitely do not have it */
 				else if (cmp == 0) {
 					keep = 1; /* definitely have it */
-					sought[i]->matched = 1;
+					sought[i]->match_status = REF_MATCHED;
 				}
 				i++;
 			}

@@ -598,22 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if ((allow_unadvertised_object_request &
-	    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-		for (i = 0; i < nr_sought; i++) {
-			unsigned char sha1[20];
+	for (i = 0; i < nr_sought; i++) {
+		unsigned char sha1[20];
 
-			ref = sought[i];
-			if (ref->matched)
-				continue;
-			if (get_sha1_hex(ref->name, sha1) ||
-			    ref->name[40] != '\0' ||
-			    hashcmp(sha1, ref->old_oid.hash))
-				continue;
+		ref = sought[i];
+		if (ref->match_status != REF_NOT_MATCHED)
+			continue;
+		if (get_sha1_hex(ref->name, sha1) ||
+		    ref->name[40] != '\0' ||
+		    hashcmp(sha1, ref->old_oid.hash))
+			continue;
 
-			ref->matched = 1;
+		if ((allow_unadvertised_object_request &
+		    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+			ref->match_status = REF_MATCHED;
 			*newtail = copy_ref(ref);
 			newtail = &(*newtail)->next;
+		} else {
+			ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
 		}
 	}
 	*refs = newlist;

@@ -1094,3 +1096,26 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 	clear_shallow_info(&si);
 	return ref_cpy;
 }
+
+int report_unmatched_refs(struct ref **sought, int nr_sought)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < nr_sought; i++) {
+		if (!sought[i])
+			continue;
+		switch (sought[i]->match_status) {
+		case REF_MATCHED:
+			continue;
+		case REF_NOT_MATCHED:
+			error(_("no such remote ref %s"), sought[i]->name);
+			break;
+		case REF_UNADVERTISED_NOT_ALLOWED:
+			error(_("Server does not allow request for unadvertised object %s"),
+			      sought[i]->name);
+			break;
+		}
+		ret = 1;
+	}
+	return ret;
+}

diff --git a/fetch-pack.h b/fetch-pack.h
index c912e3d..fd4d80e 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h

@@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 		       struct sha1_array *shallow,
 		       char **pack_lockfile);
 
+/*
+ * Print an appropriate error message for each sought ref that wasn't
+ * matched.  Return 0 if all sought refs were matched, otherwise 1.
+ *
+ * The type of "sought" should be "const struct ref *const *" but for
+ * http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c .
+ */
+int report_unmatched_refs(struct ref **sought, int nr_sought);
+
 #endif

diff --git a/remote.h b/remote.h
index 9248811..0b9d8c4 100644
--- a/remote.h
+++ b/remote.h

@@ -89,8 +89,13 @@ struct ref {
 		force:1,
 		forced_update:1,
 		expect_old_sha1:1,
-		deletion:1,
-		matched:1;
+		deletion:1;
+
+	enum {
+		REF_NOT_MATCHED = 0, /* initial value */
+		REF_MATCHED,
+		REF_UNADVERTISED_NOT_ALLOWED
+	} match_status;
 
 	/*
 	 * Order is important here, as we write to FETCH_HEAD

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2caf..78f3b8e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh

@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
 		test_must_fail git cat-file -t $the_commit &&
 
 		# fetching the hidden object should fail by default
-		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy &&
+		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+		test_i18ngrep "Server does not allow request for unadvertised object" err &&
 		test_must_fail git rev-parse --verify refs/heads/copy &&
 
 		# the server side can allow it to succeed

diff --git a/transport.c b/transport.c
index 04e5d66..c377907 100644
--- a/transport.c
+++ b/transport.c

@@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 static int fetch_refs_via_pack(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
+	int ret = 0;
 	struct git_transport_data *data = transport->data;
 	struct ref *refs;
 	char *dest = xstrdup(transport->url);

@@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport *transport,
 			  &transport->pack_lockfile);
 	close(data->fd[0]);
 	close(data->fd[1]);
-	if (finish_connect(data->conn)) {
-		free_refs(refs);
-		refs = NULL;
-	}
+	if (finish_connect(data->conn))
+		ret = -1;
 	data->conn = NULL;
 	data->got_remote_heads = 0;
 	data->options.self_contained_and_connected =
 		args.self_contained_and_connected;
 
+	if (refs == NULL)
+		ret = -1;
+	if (report_unmatched_refs(to_fetch, nr_heads))
+		ret = -1;
+
 	free_refs(refs_tmp);
 	free_refs(refs);
 	free(dest);
-	return (refs ? 0 : -1);
+	return ret;
 }
 
 static int push_had_errors(struct ref *ref)

-- 
2.9.3

`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