Re: [PATCH v4 5/5] stash: implement builtin stash

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 22, 2017, 17:23 UTC
Message-ID: <xmqqefuckkcj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+CzEk8+B71RoMeiZukfST-e6Ry+BijkNzHBusHycq2nhh2sPw@mail.gmail.com>

Joel Teichroeb <joel@teichroeb.net> writes:

Show 9 quoted lines

> On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Then you write exactly the same index contents again, this time to
>> info->u_tree here.  I am not sure why you need to do this twice, and
>> I do not see how orig_tree.hash you wrote earlier is used?
>
> I'm not sure I understand what's happening here either. When I was
> writing this, it was essentially a lot of trial and error in order to
> get the index handling correct....

Thanks for being honest. I agree that we do not want to say "we do not yet know the exact mechanism how X happens, but X does happen" for any value of X (in this case "the code happens to do the same thing as the original"). In biology or physics experiments, that may be how science advances, but it is different when it comes for us to explain our own code ;-). After all, its our creation.

I haven't followed the big picture in your codepath, but if you had something like this, I can see how you need a seemingly unneeded reading of the index:

    function A
	discard and read index
	do A's thing

    function B
	discard and read index
	do B's thing

    function C
	discard and read index
	if (some condition)
		do things that involves smudging the index
		call A
	else
		call B

    function D
	read index
	if (some other condition)
		call A
	else
		do things that involves smudging the index
		call B

That is, when the division of labor for preparing the in-core index is not very well defined between the caller and the callee. When function C calls function B, the index is unnecessarily discarded and read at the beginning of function B, but if you remove it without changing anything else, the call to it from function D would break. One way to fix it would be to make the two helpers work from the given in-core index, iow, make their callers responsible for preparing the in-core index to desired state, i.e.

    function A
	do A's thing

    function B
	do B's thing

    function C
	discard and read index
	if (some condition)
		do things that involves smudging the index
		discard and read index
		call A
	else
		call B

    function D
	read index
	if (some other condition)
		call A
	else
		do things that involves smudging the index
                discard and read index
		call B

Again, I didn't follow the big picture callpath in your patch, so the above may not be why your extra read-index calls are needed, and I do not know which of your functions correspond to A, B, C and D in the above illustration. But I think you get the idea.