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

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
Date: Tue, 21 Apr 2020 13:52:46 -0700	[thread overview]
Message-ID: <xmqq5zdsmuht.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200421204516.GA69777@syl.local> (Taylor Blau's message of "Tue, 21 Apr 2020 14:45:16 -0600")

Taylor Blau <me@ttaylorr.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);
>> >  	}
>>
>> Here, we reset only after we realize we cannot write the updated
>> shallow file.  Intended?
>
> Yes, see this earlier discussion I had about it with Jonathan:
> https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.

I did, and then I asked the question, because I couldn't quite get
if JTan was asking a question similar to the one he asked earlier in
the message ("do you need a reset in the "else" branch as well?"),
or if he was saying what he sees there, "the opposite case", was
good.

Also, I was sort-of reacting to """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.""" in that
message.

Thanks.