threads / discuss / 44841

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

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

## tl;dr

10 messages between Jan 9, 2017 and Jan 10, 2017.

replies: 9people: 2as markdown or json

Stefan Beller· Jan 9, 2017, 19:46 UTC · lore

[PATCHv2 0/5] refactor unpack-trees

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
Stefan Beller· Jan 9, 2017, 19:46 UTC · re: Stefan Beller · lore
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
Stefan Beller· Jan 9, 2017, 19:46 UTC · re: Stefan Beller · lore

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

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
Junio C Hamano· Jan 10, 2017, 20:05 UTC · re: Stefan Beller · lore

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

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();
Stefan Beller· Jan 10, 2017, 20:51 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Jan 10, 2017, 21:34 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 12 quoted lines
> 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.

Stefan Beller· Jan 9, 2017, 19:46 UTC · re: Stefan Beller · lore

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

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
Stefan Beller· Jan 9, 2017, 19:46 UTC · re: Stefan Beller · lore

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

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
Stefan Beller· Jan 9, 2017, 19:46 UTC · re: Stefan Beller · lore

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

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
Junio C Hamano· Jan 10, 2017, 19:55 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> 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.

Show 15 quoted 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);

← back to recent threads