[PATCHv2 0/5] refactor unpack-trees

[PATCHv2 0/5] refactor unpack-trees

[Stefan Beller]

v2:

* Fixed the return type to be unsigned in patch 4.

This replaces origin/rs/unpack-trees-reduce-file-scope-global,
the first patch has the following diff to that branch
diff --git a/unpack-trees.c b/unpack-trees.c
index edf9fa2f6c..8e6768f283 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -221,10 +221,11 @@ static void unlink_entry(const struct cache_entry *ce)
 static int check_updates(struct unpack_trees_options *o)
 {
        unsigned cnt = 0, total = 0;
+       int i, errs = 0;
+
        struct progress *progress = NULL;
        struct index_state *index = &o->result;
        struct checkout state = CHECKOUT_INIT;
-       int i, errs = 0;
 
        state.force = 1;
        state.quiet = 1;
        
Thanks,
Stefan

v1:
unpack-trees is a central file needed for the understanding
of working tree manipulation. To help with the understanding
refactor the code to be more readable.

The first patch was a standalone patch 8 days ago;
now incorporated into this series as a v3,
reducing the scope of the checkout state.

The second patch removes a single continue statement;
it needed some digging to explain, but looks trivial.

The last 3 patches shorten the check_updates function by adding more
functions. If we ever want to parallelize file IO then these smaller
functions would be the scope to do it, keeping the check_updates as
a high level function guiding through the steps what is happening during
a working tree update.

Thanks,
Stefan

Stefan Beller (5):
  unpack-trees: move checkout state into check_updates
  unpack-trees: remove unneeded continue
  unpack-trees: factor progress setup out of check_updates
  unpack-trees: factor file removal out of check_updates
  unpack-trees: factor working tree update out of check_updates

 unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 32 deletions(-)

-- 
2.11.0.31.g919a8d0.dirty

[PATCHv2 1/5] unpack-trees: move checkout state into check_updates

[Stefan Beller]

The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.

The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 unpack-trees.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index ea6bdd20e0..8e6768f283 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,19 @@ static void unlink_entry(const struct cache_entry *ce)
 	schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
-static int check_updates(struct unpack_trees_options *o,
-			 const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
 {
 	unsigned cnt = 0, total = 0;
+	int i, errs = 0;
+
 	struct progress *progress = NULL;
 	struct index_state *index = &o->result;
-	int i;
-	int errs = 0;
+	struct checkout state = CHECKOUT_INIT;
+
+	state.force = 1;
+	state.quiet = 1;
+	state.refresh_cache = 1;
+	state.istate = index;
 
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +245,7 @@ static int check_updates(struct unpack_trees_options *o,
 	}
 
 	if (o->update)
-		git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
 	for (i = 0; i < index->cache_nr; i++) {
 		const struct cache_entry *ce = index->cache[i];
 
@@ -251,7 +256,7 @@ static int check_updates(struct unpack_trees_options *o,
 			continue;
 		}
 	}
-	remove_marked_cache_entries(&o->result);
+	remove_marked_cache_entries(index);
 	remove_scheduled_dirs();
 
 	for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +269,7 @@ static int check_updates(struct unpack_trees_options *o,
 			display_progress(progress, ++cnt);
 			ce->ce_flags &= ~CE_UPDATE;
 			if (o->update && !o->dry_run) {
-				errs |= checkout_entry(ce, state, NULL);
+				errs |= checkout_entry(ce, &state, NULL);
 			}
 		}
 	}
@@ -1094,14 +1099,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	int i, ret;
 	static struct cache_entry *dfc;
 	struct exclude_list el;
-	struct checkout state = CHECKOUT_INIT;
 
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
-	state.force = 1;
-	state.quiet = 1;
-	state.refresh_cache = 1;
-	state.istate = &o->result;
 
 	memset(&el, 0, sizeof(el));
 	if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1238,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	}
 
 	o->src_index = NULL;
-	ret = check_updates(o, &state) ? (-2) : 0;
+	ret = check_updates(o) ? (-2) : 0;
 	if (o->dst_index) {
 		if (!ret) {
 			if (!o->result.cache_tree)
-- 
2.11.0.rc2.30.g7c4be45.dirty

[PATCHv2 2/5] unpack-trees: remove unneeded continue

[Stefan Beller]

The continue is the last statement in the loop, so not needed.
This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
read-tree -u remove index entries outside sparse area) when statements
after the continue were removed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 8e6768f283..d443decb23 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
 			display_progress(progress, ++cnt);
 			if (o->update && !o->dry_run)
 				unlink_entry(ce);
-			continue;
 		}
 	}
 	remove_marked_cache_entries(index);
-- 
2.11.0.rc2.30.g7c4be45.dirty

[PATCHv2 3/5] unpack-trees: factor progress setup out of check_updates

[Stefan Beller]

This makes check_updates shorter and easier to understand.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index d443decb23..b564024472 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,9 +218,27 @@ static void unlink_entry(const struct cache_entry *ce)
 	schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
-static int check_updates(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o)
 {
 	unsigned cnt = 0, total = 0;
+	struct index_state *index = &o->result;
+
+	if (!o->update || !o->verbose_update)
+		return NULL;
+
+	for (; cnt < index->cache_nr; cnt++) {
+		const struct cache_entry *ce = index->cache[cnt];
+		if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+			total++;
+	}
+
+	return start_progress_delay(_("Checking out files"),
+				    total, 50, 1);
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+	unsigned cnt = 0;
 	int i, errs = 0;
 
 	struct progress *progress = NULL;
@@ -232,17 +250,7 @@ static int check_updates(struct unpack_trees_options *o)
 	state.refresh_cache = 1;
 	state.istate = index;
 
-	if (o->update && o->verbose_update) {
-		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
-			const struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
-				total++;
-		}
-
-		progress = start_progress_delay(_("Checking out files"),
-						total, 50, 1);
-		cnt = 0;
-	}
+	progress = get_progress(o);
 
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
-- 
2.11.0.rc2.30.g7c4be45.dirty

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

[Stefan Beller]

This makes check_updates shorter and easier to understand.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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();
 
-- 
2.11.0.rc2.30.g7c4be45.dirty

[PATCHv2 5/5] unpack-trees: factor working tree update out of check_updates

[Stefan Beller]

This makes check_updates shorter and easier to understand.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index ac59510251..ddb3e3dcff 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -256,12 +256,13 @@ static struct progress *get_progress(struct unpack_trees_options *o)
 				    total, 50, 1);
 }
 
-static int check_updates(struct unpack_trees_options *o)
+static int update_working_tree_files(struct unpack_trees_options *o,
+				     struct progress *progress,
+				     unsigned start_cnt)
 {
-	unsigned cnt = 0;
+	unsigned cnt = start_cnt;
 	int i, errs = 0;
 
-	struct progress *progress = NULL;
 	struct index_state *index = &o->result;
 	struct checkout state = CHECKOUT_INIT;
 
@@ -270,15 +271,6 @@ static int check_updates(struct unpack_trees_options *o)
 	state.refresh_cache = 1;
 	state.istate = index;
 
-	progress = get_progress(o);
-
-	if (o->update)
-		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
-
-	cnt = remove_workingtree_files(o, progress);
-	remove_marked_cache_entries(index);
-	remove_scheduled_dirs();
-
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
@@ -288,11 +280,31 @@ static int check_updates(struct unpack_trees_options *o)
 				    ce->name);
 			display_progress(progress, ++cnt);
 			ce->ce_flags &= ~CE_UPDATE;
-			if (o->update && !o->dry_run) {
+			if (o->update && !o->dry_run)
 				errs |= checkout_entry(ce, &state, NULL);
-			}
 		}
 	}
+
+	return errs;
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+	struct progress *progress = NULL;
+	struct index_state *index = &o->result;
+	int errs;
+	unsigned total_removed;
+
+	progress = get_progress(o);
+
+	if (o->update)
+		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+	total_removed = remove_workingtree_files(o, progress);
+	remove_marked_cache_entries(index);
+	remove_scheduled_dirs();
+	errs = update_working_tree_files(o, progress, total_removed);
+
 	stop_progress(&progress);
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
-- 
2.11.0.rc2.30.g7c4be45.dirty

Re: [PATCHv2 2/5] unpack-trees: remove unneeded continue

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The continue is the last statement in the loop, so not needed.
> This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
> read-tree -u remove index entries outside sparse area) when statements
> after the continue were removed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Thanks for digging the history to find the culprit.  

This is an unrelated tangent, but I do not think of a way other than
"log -L" to find it; conceptually, "blame" ought to be usable for
it, but it is not useful for finding deleted lines.

>  unpack-trees.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8e6768f283..d443decb23 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
>  			display_progress(progress, ++cnt);
>  			if (o->update && !o->dry_run)
>  				unlink_entry(ce);
> -			continue;
>  		}
>  	}
>  	remove_marked_cache_entries(index);

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

[Junio C Hamano]

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();

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

[Stefan Beller]

On Tue, Jan 10, 2017 at 12:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

In the grand scheme of things, the flow has always been understood
more like this two-step process:

    - compute the result in core (rejecting an inconsistent result
      before touching the working tree)

    - reflect the result to the working tree

and the latter is done by check_updates().  

Removing gone entries before instanciating possibly new entries is
done in order to make room where we can create a new path D/F in the
result, after removing an old path D [*1*].  But it is merely an
implementation detail of the latter, i.e. "reflect the result to the
working tree.

It is arguably true that check_updates() is not about "checking" but
is about "doing", hence is misnamed.

[Footnote]

*1* If the result computed in-core wants to create D/F, it must have
removal of D---otherwise the result is inconsistent.