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

[Junio C Hamano <gitster@pobox.com>]

Joel Teichroeb <joel@teichroeb.net> writes:

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