Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates

From: Stefan Beller <sbeller@google.com>
Date: Jan 10, 2017, 20:51 UTC
Message-ID: <CAGZ79kbXVCvZuj6rTckGwOfFtRSFpx9p611oKfFLAayTgB242Q@mail.gmail.com>
In-Reply-To: <xmqqd1fusmsu.fsf@gitster.mtv.corp.google.com>

On Tue, Jan 10, 2017 at 12:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> This makes check_updates shorter and easier to understand.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I agree that 3/5 made it easier to understand by ejecting a block
> that is not essential to the functionality of the function out of
> it, making the remainder of the fuction about "removing gone files
> and then write out the modified files".
>
> The ejecting of the first half of these two operations, both are
> what this function is about, done by this step feels backwards.  If
> anything, the "only do the actual working tree manipulation when not
> doing a dry-run and told to update" logic that must be in both are
> spread in two helper functions after step 5/5, and with the added
> boilerplate for these two helpers, the end result becomes _longer_
> to understand what is really going on when check_updates() is
> called.
>
> Is the original after step 3/5 too long and hard to understand?

It is ok to understand for the current state of the world, so please drop 4 and 5 for now.

In a future, when
* working tree operations would learn about threading or
* working tree operations were aware of submodules
then steps of 4 and 5 would be longer, such that the check_updates
function may require further splitting up.

As far as I understand the operations now, a working tree operation is done like this:

* First compute the new index, how the world would look like
  (don't touch a thing in the working tree, it is like a complete
  dry run, that just checks for potential loss of data, e.g.
  untracked files, merge conflicts etc)
* remove all files to be marked for deletion
* add and update all files that are new or change content.

check_updates does the last two things listed here, which in the grand scheme of things looked odd to me.

When introducing e.g. threaded checkout, then each of the functions introduced in patch 4&5 would be one compartment, i.e. the function remove_workingtree_files would need to have all its tasks finished before we even queue up tasks for updating files, such that we do not run into D/F conflicts racily.

So yeah, maybe this is too much future-visionary thinking, so please drop 4/5; I can revive them if needed. I think I mainly did them for my own clarity and understanding.

Thanks, Stefan