Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 10, 2017, 20:05 UTC
- Message-ID
- <xmqqd1fusmsu.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170109194621.17013-5-sbeller@google.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?
Show 50 quoted lines
> 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();