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

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

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?

>  unpack-trees.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b564024472..ac59510251 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)
>  	schedule_dir_for_removal(ce->name, ce_namelen(ce));
>  }
>  
> +static unsigned remove_workingtree_files(struct unpack_trees_options *o,
> +					 struct progress *progress)
> +{
> +	int i;
> +	unsigned cnt = 0;
> +	struct index_state *index = &o->result;
> +
> +	for (i = 0; i < index->cache_nr; i++) {
> +		const struct cache_entry *ce = index->cache[i];
> +
> +		if (ce->ce_flags & CE_WT_REMOVE) {
> +			display_progress(progress, ++cnt);
> +			if (o->update && !o->dry_run)
> +				unlink_entry(ce);
> +		}
> +	}
> +
> +	return cnt;
> +}
> +
>  static struct progress *get_progress(struct unpack_trees_options *o)
>  {
>  	unsigned cnt = 0, total = 0;
> @@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)
>  
>  	if (o->update)
>  		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
> -	for (i = 0; i < index->cache_nr; i++) {
> -		const struct cache_entry *ce = index->cache[i];
>  
> -		if (ce->ce_flags & CE_WT_REMOVE) {
> -			display_progress(progress, ++cnt);
> -			if (o->update && !o->dry_run)
> -				unlink_entry(ce);
> -		}
> -	}
> +	cnt = remove_workingtree_files(o, progress);
>  	remove_marked_cache_entries(index);
>  	remove_scheduled_dirs();