Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 16, 2020, 02:05 UTC
Message-ID: <20200416020509.225014-1-jonathantanmy@google.com>
In-Reply-To: <20200414235057.GA6863@syl.local>

Show 9 quoted lines

> Here's a patch that I didn't sign-off on that fixes the problem for me.
> 
> --- >8 ---
> 
> Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
> 
> In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,
> 2019-01-10), the author noted that 'is_repository_shallow' produces
> visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.

Thanks for the reference to my commit. When I wrote that, if I remember correctly, I found it difficult to be able to guarantee that the clearing of is_shallow and shallow_stat was performed upon every commit-lock (which creates a new shallow file) and <unlink+rollback-lock> (which removes the shallow file, potentially changing the repo state from shallow to none), so I added the NEEDSWORK instead. I see that this patch makes some progress in solving that.

Show 8 quoted lines

> @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,
>  		if (*alternate_shallow_file == '\0') { /* --unshallow */
>  			unlink_or_warn(git_path_shallow(the_repository));
>  			rollback_lock_file(&shallow_lock);
> +			reset_repository_shallow(the_repository);
>  		} else
>  			commit_lock_file(&shallow_lock);
>  		alternate_shallow_file = NULL;

Here, do you need a reset in the "else" branch as well? (I.e. in both branches, so you can put it outside the "if" block.) I didn't look at it too closely, but I would think that when the shallow_lock file is committed, there might have been no shallow beforehand, changing the state from no-shallow to shallow.

Show 8 quoted lines

> @@ -411,6 +411,7 @@ void prune_shallow(unsigned options)
>  			die_errno("failed to write to %s",
>  				  get_lock_file_path(&shallow_lock));
>  		commit_lock_file(&shallow_lock);
> +		reset_repository_shallow(the_repository);
>  	} else {
>  		unlink(git_path_shallow(the_repository));
>  		rollback_lock_file(&shallow_lock);

And the opposite case here - reset in the "else" branch because the state could have changed from shallow to no-shallow.

In any case, I think the commit message should discuss why reset_repository_shallow() is added only on the unlink+rollback side in one "if" statement, but only on the opposite "commit" side in another "if" statement.