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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2020, 18:15 UTC
Message-ID: <xmqqy2qnidyy.fsf@gitster.c.googlers.com>
In-Reply-To: <20200422180245.196132-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 13 quoted lines

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