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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote:
> 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.

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