Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> The cleanup section is reached with `retcode` set in several scenarios:
>
>    - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set
>      `retcode` before the transaction is created, so no commit is
>      attempted.
>
>    - `fetch_and_consume_refs()` and `backfill_tags()` are the primary
>      cases this fix targets, both setting a positive `retcode` to
>      trigger the committing of the transaction.
>
> This simplifies error handling and ensures future modifications to
> `do_fetch()` don't need special handling for batched updates.

This may be sufficient to clean out the hanging transaction that the
original code forgot to commit, but in scenarios where this change
makes a difference, i.e., where the code does "goto cleanup" before
it calls commit_ref_transaction() in the main flow of the code,
there are things that are not performed that we may still want to
perform.  Namely, we do not

 - call commit_fetch_head()

 - run set_upstream processing

 - honor do_set_head flag that was left for remote that does not
   have followremotehead=never

but don't we want to do some of them at least?

If it turns out that we want to do all of them, I also wonder if the
resulting code would become easier to follow if we lose the call to
commit_ref_transaction() in the main code flow, do the above three
points before committing the ref transaction, and then after the
cleanup label, make a call to commit_ref_transaction() if we have an
open transaction and we are not atomic (regardless of the value of
retcode at that point).  That call may yield another retcode that
the existing error reporting at the end of this function may have to
react to.

>  cleanup:
> +	/*
> +	 * When using batched updates, we want to commit the non-rejected
> +	 * updates and also handle the rejections.
> +	 */
> +	if (retcode && !atomic_fetch && transaction)
> +		commit_ref_transaction(&transaction, false,
> +				       transport->remote->name, &err);
>
>  	if (retcode) {
>  		if (err.len) {
>  			error("%s", err.buf);

IOW, something like this on top of this patch (not even compile tested).

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

diff --git c/builtin/fetch.c w/builtin/fetch.c
index b19fa8e966..d3eb65dac6 100644
--- c/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -1888,11 +1888,6 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	retcode = commit_ref_transaction(&transaction, atomic_fetch,
-					 transport->remote->name, &err);
-	if (retcode)
-		goto cleanup;
-
 	commit_fetch_head(&fetch_head);
 
 	if (set_upstream) {
@@ -1957,14 +1952,9 @@ static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
-	/*
-	 * When using batched updates, we want to commit the non-rejected
-	 * updates and also handle the rejections.
-	 */
-	if (retcode && !atomic_fetch && transaction)
-		commit_ref_transaction(&transaction, false,
-				       transport->remote->name, &err);
-
+	if (!atomic_fetch && transaction)
+		retcode = commit_ref_transaction(&transaction, false,
+						 transport->remote->name, &err);
 	if (retcode) {
 		if (err.len) {
 			error("%s", err.buf);