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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 10, 2017 at 12:05 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> This makes check_updates shorter and easier to understand.
>>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>
> 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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]