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

[Jonathan Tan <jonathantanmy@google.com>]

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

> @@ -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.

> @@ -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.