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

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 21, 2020, 22:21 UTC
Message-ID: <20200421222128.GA74697@syl.local>
In-Reply-To: <xmqq5zdsmuht.fsf@gitster.c.googlers.com>

On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

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

Show 5 quoted lines

> 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