On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxxx/.
>
> 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.
Sorry, I think that the linked message is confusing (at least, it
confused me the second time I read it because I wasn't sure which part
of the mail Jonathan was asking about: my patch, or his response to my
patch).
I think that he was referring to how I had it in the original patch I
sent in that thread, which was wrong. Based on my understanding of his
message, his recommendations match what I have in _this_ patch.
> 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.
Is the description that I attached in the earlier patch sufficient? I
could certainly add more detail if it's not.
In either case, I'm sitting on another patch locally to improve the
style of the surrounding tests, which is done as a preparatory step
before this patch. I'll re-send after hearing back from you.
> Thanks.
Thanks,
Taylor
