Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate

[Taylor Blau <me@ttaylorr.com>]

On Wed, Apr 22, 2020 at 11:15:33AM -0700, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> >> @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)
> >>  	} else {
> >>  		unlink(git_path_shallow(the_repository));
> >>  		rollback_lock_file(&shallow_lock);
> >> +		reset_repository_shallow(the_repository);
> >>  	}
> >>  	strbuf_release(&sb);
> >>  }
> >
> > The "if" part (not quoted here) commits the shallow lock file, and thus
> > possibly modifies (or creates) the shallow file, so I think we need to
> > put reset_repository_shallow() outside the whole "if" block. I have done
> > that in the patch after the scissors.
>
> Is there any rollback_lock_file() or commit_lock_file() call on the
> shallow lock file in the files involved in this patch that does not
> need a call to reset_repository_shallow() left after your work?
>
> What I am trying to get at is if it would be safer to have a pair of
> thin wrapper for rolling back or committing a new version of new
> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),
> and replace calls to {rollback,commit}_lock_file() with calls to
> them.

Very elegant. Thanks for an excellent suggestion. v2 incoming just as
soon as 'make test' finishes...

Thanks,
Taylor