Re: [PATCH v2] fetch: remove fetch_if_missing=0

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com, sunshine@sunshineco.com
Subject: Re: [PATCH v2] fetch: remove fetch_if_missing=0
Date: Fri, 08 Nov 2019 15:33:40 +0900	[thread overview]
Message-ID: <xmqqr22inagr.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20191105185619.207173-1-jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 5 Nov 2019 10:56:19 -0800")

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 0c345b5dfe..5ff7367dd7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)
>  	 * we need all direct targets to exist.
>  	 */
>  	for (r = rm; r; r = r->next) {
> -		if (!has_object_file(&r->old_oid))
> +		if (!has_object_file_with_flags(&r->old_oid,
> +						OBJECT_INFO_SKIP_FETCH_OBJECT))
>  			return -1;
>  	}
>  
> @@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  
>  	packet_trace_identity("fetch");
>  
> -	fetch_if_missing = 0;
> -
>  	/* Record the command line for the reflog */
>  	strbuf_addstr(&default_rla, "fetch");
>  	for (i = 1; i < argc; i++)

As this is on a stale codebase, let's base the fix on top of
c32ca691c2 or later.  The part of the patch for builtin/fetch.c
adjusted for that decision would look like attached.

An interesting thing is that c32ca691c2^2 that moved the assignment
to this big red switch variable around causes 3-way merge to fail in
a miserable way.  The "moving around" would involve removing from
the same location as the rebased patch below removes, plus adding
the assignment elsewhere, so "both sides removed the assignment from
this hunk, so take that" would correctly leave the original assignment
we see in the second hunk above removed, but fails to notice that
the assignment made elsewhere (the result of "moving around" patch)
is no longer needed, because "c32ca691c2^2 added one and this change
does not do anything there, so take the addition" cleanly resolves
to an incorrect merge result.

 builtin/fetch.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 863c858fde..5ff7367dd7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)
 	 * we need all direct targets to exist.
 	 */
 	for (r = rm; r; r = r->next) {
-		if (!has_object_file(&r->old_oid))
+		if (!has_object_file_with_flags(&r->old_oid,
+						OBJECT_INFO_SKIP_FETCH_OBJECT))
 			return -1;
 	}
 
@@ -1822,8 +1823,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	fetch_if_missing = 0;
-
 	if (remote) {
 		if (filter_options.choice || has_promisor_remote())
 			fetch_one_setup_partial(remote);
