threads / patch / 45632

patch, 3 partsfix working directory file issues while using sparse-checkout

Subject: [PATCH 0/3] fix working directory file issues while using sparse-checkout

## tl;dr

16 messages between Apr 7, 2017 and Apr 17, 2017. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Kevin Willford· Apr 7, 2017, 19:23 UTC · lore

While using the sparse-checkout feature there are scenarios where the working directory should and should not be updated. This patch series addresses issues found in reset, apply, and merge conflicts.

Kevin Willford (3):
  merge-recursive.c: conflict using sparse should update file
  apply.c: do not checkout file when skip-worktree bit set
  reset.c: update files when using sparse to avoid data loss.
 apply.c                          | 18 +++++++++++++
 builtin/reset.c                  | 34 +++++++++++++++++++++++
 merge-recursive.c                |  8 ++++++
 t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
 t/t7614-merge-sparse-checkout.sh | 27 +++++++++++++++++++
 5 files changed, 145 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh
 create mode 100755 t/t7614-merge-sparse-checkout.sh
-- 
2.12.2.windows.2.1.g7df5db8d31
Kevin Willford· Apr 7, 2017, 19:23 UTC · re: Kevin Willford · lore

[PATCH 1/3] merge-recursive.c: conflict using sparse should update file

Update the file when there is a conflict with a modify/delete scenario when using the sparse-checkout feature since the file might not be on disk because the skip-worktree bit is on and the user will need the file and content to determine how to resolve the conflict.

Signed-off-by: Kevin Willford <kewillf@microsoft.com>
---
 merge-recursive.c                |  8 ++++++++
 t/t7614-merge-sparse-checkout.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100755 t/t7614-merge-sparse-checkout.sh
Show changes to 2 files +35 −0

merge-recursive.c, t/t7614-merge-sparse-checkout.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index b7ff1ada3c..54fce93dae 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1103,6 +1103,14 @@ static int handle_change_delete(struct merge_options *o,
 			       "and %s in %s. Version %s of %s left in tree."),
 			       change, path, o->branch2, change_past,
 			       o->branch1, o->branch1, path);
+			/*
+			 * In a sparse checkout the file may not exist. Sadly,
+			 * the CE_SKIP_WORKTREE flag is not preserved in the
+			 * case of conflicts, therefore we do the next best
+			 * thing: verify that the file exists.
+			 */
+			if (!file_exists(path))
+				ret = update_file(o, 0, a_oid, a_mode, path);
 		} else {
 			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
 			       "and %s in %s. Version %s of %s left in tree at %s."),
diff --git a/t/t7614-merge-sparse-checkout.sh b/t/t7614-merge-sparse-checkout.sh
new file mode 100755
index 0000000000..6922f0244f
--- /dev/null
+++ b/t/t7614-merge-sparse-checkout.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='merge can handle sparse-checkout'
+
+. ./test-lib.sh
+
+# merges with conflicts
+
+test_expect_success 'setup' '
+	test_commit a &&
+	test_commit file &&
+	git checkout -b delete-file &&
+	git rm file.t &&
+	test_tick &&
+	git commit -m "remove file" &&
+	git checkout master &&
+	test_commit modify file.t changed
+'
+
+test_expect_success 'merge conflict deleted file and modified' '
+	echo "/a" >.git/info/sparse-checkout &&
+	test_config core.sparsecheckout true &&
+	test_must_fail git merge delete-file &&
+	test_path_is_file file.t
+'
+
+test_done

-- 
2.12.2.windows.2.1.g7df5db8d31
Duy Nguyen· Apr 10, 2017, 10:36 UTC · re: Kevin Willford · lore

Re: [PATCH 1/3] merge-recursive.c: conflict using sparse should update file

On Fri, Apr 07, 2017 at 12:23:55PM -0700, Kevin Willford wrote:
> Update the file when there is a conflict with a modify/delete scenario
> when using the sparse-checkout feature since the file might not be on disk
> because the skip-worktree bit is on and the user will need the file and
> content to determine how to resolve the conflict.

I'm a bit uncertain about this, but it's mostly because I'm not familiar with merge-recursive.c. I think the general principle is, if a file is conflicted it must NOT have skip-worktree bit on and the worktree version must be restored at the same time the bit is removed.

I think we should do that (restore the worktree version) when the skip-worktree bit is removed. What I'm not sure is when the bit is removed in this code.

I'm guessing that the bit is removed by unpack_trees() with the threeway merge. Maybe we should restore the worktree version there at that time. It does not matter what conflict type it is, just restore the file (which should be what merge-recursive.c expects in normal case) then merge-recursive.c can proceed to update, delete or whatever to present the conflict to the user.

Show 63 quoted lines
>
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> ---
>  merge-recursive.c                |  8 ++++++++
>  t/t7614-merge-sparse-checkout.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100755 t/t7614-merge-sparse-checkout.sh
> 
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b7ff1ada3c..54fce93dae 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1103,6 +1103,14 @@ static int handle_change_delete(struct merge_options *o,
>  			       "and %s in %s. Version %s of %s left in tree."),
>  			       change, path, o->branch2, change_past,
>  			       o->branch1, o->branch1, path);
> +			/*
> +			 * In a sparse checkout the file may not exist. Sadly,
> +			 * the CE_SKIP_WORKTREE flag is not preserved in the
> +			 * case of conflicts, therefore we do the next best
> +			 * thing: verify that the file exists.
> +			 */
> +			if (!file_exists(path))
> +				ret = update_file(o, 0, a_oid, a_mode, path);
>  		} else {
>  			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>  			       "and %s in %s. Version %s of %s left in tree at %s."),
> diff --git a/t/t7614-merge-sparse-checkout.sh b/t/t7614-merge-sparse-checkout.sh
> new file mode 100755
> index 0000000000..6922f0244f
> --- /dev/null
> +++ b/t/t7614-merge-sparse-checkout.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh
> +
> +test_description='merge can handle sparse-checkout'
> +
> +. ./test-lib.sh
> +
> +# merges with conflicts
> +
> +test_expect_success 'setup' '
> +	test_commit a &&
> +	test_commit file &&
> +	git checkout -b delete-file &&
> +	git rm file.t &&
> +	test_tick &&
> +	git commit -m "remove file" &&
> +	git checkout master &&
> +	test_commit modify file.t changed
> +'
> +
> +test_expect_success 'merge conflict deleted file and modified' '
> +	echo "/a" >.git/info/sparse-checkout &&
> +	test_config core.sparsecheckout true &&
> +	test_must_fail git merge delete-file &&
> +	test_path_is_file file.t
> +'
> +
> +test_done
> -- 
> 2.12.2.windows.2.1.g7df5db8d31
>
Kevin Willford· Apr 7, 2017, 19:23 UTC · re: Kevin Willford · lore

[PATCH 2/3] apply.c: do not checkout file when skip-worktree bit set

When using the sparse-checkout feature git should not write to the working directory for files with the skip-worktree bit on. With the skip-worktree bit on the file may or may not be in the working directory and if it is not we don't want or need to create it by calling checkout_entry.

There are two callers of checkout_target. Both of which check that the file does not exist before calling checkout_target. load_current which make a call to lstat right before calling checkout_target and check_preimage which will only run checkout_taret it stat_ret is less than zero. It sets stat_ret to zero and only if !stat->cached will it lstat the file and set stat_ret to something other than zero.

This patch checks if skip-worktree bit is on in checkout_target and just returns so that the entry doesn't not end up in the working directory. This is so that apply will not create a file in the working directory, then update the index but not keep the working directory up to date with the changes that happened in the index.

Signed-off-by: Kevin Willford <kewillf@microsoft.com>
---
 apply.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
Show changes to apply.c +18 −0
diff --git a/apply.c b/apply.c
index 0e2caeab9c..0da5d0b7c9 100644
--- a/apply.c
+++ b/apply.c
@@ -3327,6 +3327,24 @@ static int checkout_target(struct index_state *istate,
 {
 	struct checkout costate = CHECKOUT_INIT;
 
+	/*
+	 * Do not checkout the entry if the skipworktree bit is set
+	 *
+	 * Both callers of this method (check_preimage and load_current)
+	 * check for the existance of the file before calling this
+	 * method so we know that the file doesn't exist at this point
+	 * and we don't need to perform that check again here.
+	 * We just need to check the skip-worktree and return.
+	 *
+	 * This is to prevent git from creating a file in the
+	 * working directory that has the skip-worktree bit on,
+	 * then updating the index from the patch and not keeping
+	 * the working directory version up to date with what it
+	 * changed the index version to be.
+	 */
+	if (ce_skip_worktree(ce))
+		return 0;
+
 	costate.refresh_cache = 1;
 	costate.istate = istate;
 	if (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st))

-- 
2.12.2.windows.2.1.g7df5db8d31
Stefan Beller· Apr 7, 2017, 22:28 UTC · re: Kevin Willford · lore

Re: [PATCH 2/3] apply.c: do not checkout file when skip-worktree bit set

On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
Show 48 quoted lines
> When using the sparse-checkout feature git should not write to
> the working directory for files with the skip-worktree bit on.
> With the skip-worktree bit on the file may or may not be in
> the working directory and if it is not we don't want or need to
> create it by calling checkout_entry.
>
> There are two callers of checkout_target.  Both of which check
> that the file does not exist before calling checkout_target.
> load_current which make a call to lstat right before calling checkout_target
> and check_preimage which will only run checkout_taret it stat_ret
> is less than zero.  It sets stat_ret to zero and only if
> !stat->cached will it lstat the file and set stat_ret to
> something other than zero.
>
> This patch checks if skip-worktree bit is on in checkout_target
> and just returns so that the entry doesn't not end up in the
> working directory.  This is so that apply will not create a file
> in the working directory, then update the index but not keep the
> working directory up to date with the changes that happened in the index.
>
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> ---
>  apply.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index 0e2caeab9c..0da5d0b7c9 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3327,6 +3327,24 @@ static int checkout_target(struct index_state *istate,
>  {
>         struct checkout costate = CHECKOUT_INIT;
>
> +       /*
> +        * Do not checkout the entry if the skipworktree bit is set
> +        *
> +        * Both callers of this method (check_preimage and load_current)
> +        * check for the existance of the file before calling this
> +        * method so we know that the file doesn't exist at this point
> +        * and we don't need to perform that check again here.
> +        * We just need to check the skip-worktree and return.
> +        *
> +        * This is to prevent git from creating a file in the
> +        * working directory that has the skip-worktree bit on,
> +        * then updating the index from the patch and not keeping
> +        * the working directory version up to date with what it
> +        * changed the index version to be.
> +        */

I have the impression that this information is better in the commit message rather than in the code itself, i.e. we can drop this comment here?

Thanks, Stefan

Show 9 quoted lines
> +       if (ce_skip_worktree(ce))
> +               return 0;
> +
>         costate.refresh_cache = 1;
>         costate.istate = istate;
>         if (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st))
> --
> 2.12.2.windows.2.1.g7df5db8d31
>
Duy Nguyen· Apr 10, 2017, 10:11 UTC · re: Kevin Willford · lore

Re: [PATCH 2/3] apply.c: do not checkout file when skip-worktree bit set

On Fri, Apr 07, 2017 at 12:23:56PM -0700, Kevin Willford wrote:
Show 19 quoted lines
> When using the sparse-checkout feature git should not write to
> the working directory for files with the skip-worktree bit on.
> With the skip-worktree bit on the file may or may not be in
> the working directory and if it is not we don't want or need to
> create it by calling checkout_entry.
> 
> There are two callers of checkout_target.  Both of which check
> that the file does not exist before calling checkout_target.
> load_current which make a call to lstat right before calling checkout_target
> and check_preimage which will only run checkout_taret it stat_ret
> is less than zero.  It sets stat_ret to zero and only if
> !stat->cached will it lstat the file and set stat_ret to
> something other than zero.
> 
> This patch checks if skip-worktree bit is on in checkout_target
> and just returns so that the entry doesn't not end up in the
> working directory.  This is so that apply will not create a file
> in the working directory, then update the index but not keep the
> working directory up to date with the changes that happened in the index.

No. I think we should check the skip-worktree at the call sites instead of just exit early here. What if the caller expects the file to exist if checkout_target returns zero? We just don't have enough information to decide what is the right thing to do in this function.

There are two call sites.

In load_current() we could skip the lstat() call before checkout_target() too if skip-worktree is set. We also need to skip the verify_index_match(). I'm not exactly sure if that code expects the target file on disk afterwarsd though (apply.c is not really my thing).

In check_preimage() it looks easier because we could follow the state->cached code for skip-worktree, I think.

Show 38 quoted lines
> 
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> ---
>  apply.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/apply.c b/apply.c
> index 0e2caeab9c..0da5d0b7c9 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3327,6 +3327,24 @@ static int checkout_target(struct index_state *istate,
>  {
>  	struct checkout costate = CHECKOUT_INIT;
>  
> +	/*
> +	 * Do not checkout the entry if the skipworktree bit is set
> +	 *
> +	 * Both callers of this method (check_preimage and load_current)
> +	 * check for the existance of the file before calling this
> +	 * method so we know that the file doesn't exist at this point
> +	 * and we don't need to perform that check again here.
> +	 * We just need to check the skip-worktree and return.
> +	 *
> +	 * This is to prevent git from creating a file in the
> +	 * working directory that has the skip-worktree bit on,
> +	 * then updating the index from the patch and not keeping
> +	 * the working directory version up to date with what it
> +	 * changed the index version to be.
> +	 */
> +	if (ce_skip_worktree(ce))
> +		return 0;
> +
>  	costate.refresh_cache = 1;
>  	costate.istate = istate;
>  	if (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st))
> -- 
> 2.12.2.windows.2.1.g7df5db8d31
>
Kevin Willford· Apr 7, 2017, 19:23 UTC · re: Kevin Willford · lore

[PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

When using the sparse checkout feature the git reset command will add entries to the index that will have the skip-worktree bit off but will leave the working directory empty. File data is lost because the index version of the files has been changed but there is nothing that is in the working directory. This will cause the next status call to show either deleted for files modified or deleting or nothing for files added. The added files should be shown as untracked and modified files should be shown as modified.

To fix this when the reset is running if there is not a file in the working directory and if it will be missing with the new index entry or was not missing in the previous version, we create the previous index version of the file in the working directory so that status will report correctly and the files will be availble for the user to deal with.

Signed-off-by: Kevin Willford <kewillf@microsoft.com>
---
 builtin/reset.c                  | 34 +++++++++++++++++++++++
 t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh
Show changes to 2 files +92 −0

builtin/reset.c, t/t7114-reset-sparse-checkout.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfcb..8ba97999f4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,6 +21,7 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "dir.h"
 
 static const char * const git_reset_usage[] = {
 	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
@@ -117,12 +118,45 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		struct diff_options *opt, void *data)
 {
 	int i;
+	int pos;
 	int intent_to_add = *(int *)data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filespec *one = q->queue[i]->one;
+		struct diff_filespec *two = q->queue[i]->two;
 		int is_missing = !(one->mode && !is_null_oid(&one->oid));
+		int was_missing = !two->mode && is_null_oid(&two->oid);
 		struct cache_entry *ce;
+		struct cache_entry *ceBefore;
+		struct checkout state = CHECKOUT_INIT;
+
+		/*
+		 * When using the sparse-checkout feature the cache entries that are
+		 * added here will not have the skip-worktree bit set.
+		 * Without this code there is data that is lost because the files that
+		 * would normally be in the working directory are not there and show as
+		 * deleted for the next status or in the case of added files just disappear.
+		 * We need to create the previous version of the files in the working
+		 * directory so that they will have the right content and the next
+		 * status call will show modified or untracked files correctly.
+		 */
+		if (core_apply_sparse_checkout && !file_exists(two->path))
+		{
+			pos = cache_name_pos(two->path, strlen(two->path));
+			if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) && (is_missing || !was_missing))
+			{
+				state.force = 1;
+				state.refresh_cache = 1;
+				state.istate = &the_index;
+				ceBefore = make_cache_entry(two->mode, two->oid.hash, two->path,
+					0, 0);
+				if (!ceBefore)
+					die(_("make_cache_entry failed for path '%s'"),
+						two->path);
+
+				checkout_entry(ceBefore, &state, NULL);
+			}
+		}
 
 		if (is_missing && !intent_to_add) {
 			remove_file_from_cache(one->path);
diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
new file mode 100755
index 0000000000..8dd88fd46d
--- /dev/null
+++ b/t/t7114-reset-sparse-checkout.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='reset when using a sparse-checkout'
+
+. ./test-lib.sh
+
+# reset using a sparse-checkout file
+
+test_expect_success 'setup' '
+	test_tick &&
+	echo "checkout file" >c &&
+	echo "modify file" >m &&
+	echo "delete file" >d &&
+	git add . &&
+	git commit -m "initial commit" &&
+	echo "added file" >a &&
+	echo "modification of a file" >m &&
+	git rm d &&
+	git add . &&
+	git commit -m "second commit" &&
+	git checkout -b endCommit
+'
+
+test_expect_success 'reset when there is a sparse-checkout' '
+	echo "/c" >.git/info/sparse-checkout &&
+	test_config core.sparsecheckout true &&
+	git checkout -b resetBranch &&
+	test_path_is_missing m &&
+	test_path_is_missing a &&
+	test_path_is_missing d &&
+	git reset HEAD~1 &&
+	test "checkout file" = "$(cat c)" &&
+	test "modification of a file" = "$(cat m)" &&
+	test "added file" = "$(cat a)" &&
+	test_path_is_missing d
+'
+
+test_expect_success 'reset after deleting file without skip-worktree bit' '
+	git checkout -f endCommit &&
+	git clean -xdf &&
+	echo "/c
+/m" >.git/info/sparse-checkout &&
+	test_config core.sparsecheckout true &&
+	git checkout -b resetAfterDelete &&
+	test_path_is_file m &&
+	test_path_is_missing a &&
+	test_path_is_missing d &&
+	rm -f m &&
+	git reset HEAD~1 &&
+	test "checkout file" = "$(cat c)" &&
+	test "added file" = "$(cat a)" &&
+	test_path_is_missing m &&
+	test_path_is_missing d
+'
+
+
+
+test_done

-- 
2.12.2.windows.2.1.g7df5db8d31
Stefan Beller· Apr 7, 2017, 22:41 UTC · re: Kevin Willford · lore

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
Show 39 quoted lines
> When using the sparse checkout feature the git reset command will add
> entries to the index that will have the skip-worktree bit off but will
> leave the working directory empty.  File data is lost because the index
> version of the files has been changed but there is nothing that is in the
> working directory.  This will cause the next status call to show either
> deleted for files modified or deleting or nothing for files added.
> The added files should be shown as untracked and modified files should
> be shown as modified.
>
> To fix this when the reset is running if there is not a file in the
> working directory and if it will be missing with the new index entry
> or was not missing in the previous version, we create the previous index
> version of the file in the working directory so that status will report
> correctly and the files will be availble for the user to deal with.
>
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> ---
>  builtin/reset.c                  | 34 +++++++++++++++++++++++
>  t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>  create mode 100755 t/t7114-reset-sparse-checkout.sh
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 8ab915bfcb..8ba97999f4 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -21,6 +21,7 @@
>  #include "parse-options.h"
>  #include "unpack-trees.h"
>  #include "cache-tree.h"
> +#include "dir.h"
>
>  static const char * const git_reset_usage[] = {
>         N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
> @@ -117,12 +118,45 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>                 struct diff_options *opt, void *data)
>  {
>         int i;
> +       int pos;
You could declare this inside the for loop.
Show 21 quoted lines
>         int intent_to_add = *(int *)data;
>
>         for (i = 0; i < q->nr; i++) {
>                 struct diff_filespec *one = q->queue[i]->one;
> +               struct diff_filespec *two = q->queue[i]->two;
>                 int is_missing = !(one->mode && !is_null_oid(&one->oid));
> +               int was_missing = !two->mode && is_null_oid(&two->oid);
>                 struct cache_entry *ce;
> +               struct cache_entry *ceBefore;
> +               struct checkout state = CHECKOUT_INIT;
> +
> +               /*
> +                * When using the sparse-checkout feature the cache entries that are
> +                * added here will not have the skip-worktree bit set.
> +                * Without this code there is data that is lost because the files that
> +                * would normally be in the working directory are not there and show as
> +                * deleted for the next status or in the case of added files just disappear.
> +                * We need to create the previous version of the files in the working
> +                * directory so that they will have the right content and the next
> +                * status call will show modified or untracked files correctly.
> +                */

This comment also belongs to the commit message IMHO, as it describes the bug on a high level, and when it is in the code it wastes screen real estate; commit messages however have low prices for screen real estate. ;-)

> +               if (core_apply_sparse_checkout && !file_exists(two->path))
> +               {
Coding style: Git prefers { at the end of the line:
  if (..) {
    ..
Show 5 quoted lines
> +test_expect_success 'reset after deleting file without skip-worktree bit' '
> +       git checkout -f endCommit &&
> +       git clean -xdf &&
> +       echo "/c
> +/m" >.git/info/sparse-checkout &&

huh? Did your mailer wrap lines here or do you intend to have a LF in the output?

Assuming the latter, I think we prefer cat with here-doc for multi line content, i.e.

    cat >.git/info/sparse-checkout <<-\EOF
    /c
    /m
    EOF
    test_config ...
    ...

Thanks, Stefan

Duy Nguyen· Apr 10, 2017, 10:24 UTC · re: Kevin Willford · lore

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

On Fri, Apr 07, 2017 at 12:23:57PM -0700, Kevin Willford wrote:
> When using the sparse checkout feature the git reset command will add

"git reset" has three different modes. It would be good if you mention what mode is affected here. The tests are for --mixed only. I wonder if we need to do anything for --hard and --soft?

--soft touches branch SHA-1 index only, not worktree, so probably not.
--hard should be handled by unpack_trees(), I think.

But it would be good to cover these in the commit message as well to stop readers from wondering.

Show 7 quoted lines
> entries to the index that will have the skip-worktree bit off but will
> leave the working directory empty.  File data is lost because the index
> version of the files has been changed but there is nothing that is in the
> working directory.  This will cause the next status call to show either
> deleted for files modified or deleting or nothing for files added.
> The added files should be shown as untracked and modified files should
> be shown as modified.

Hmm.. reading --mixed documentation again ("Resets the index but not working tree"), I think the current behavior is expected regardless of skip-worktree bits.

Perhaps the problem is the loss of skip-worktree bits on entries added by update_index_from_diff()? If the bits are at the right place, then it should not matter if the same version exists on worktree or not and "status" or "commit" should work as expected, I think.

-- Duy

Kevin Willford· Apr 11, 2017, 22:30 UTC · re: Duy Nguyen · lore

RE: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

Show 22 quoted lines
> -----Original Message-----
> From: Duy Nguyen [mailto:pclouds@gmail.com]
> Sent: Monday, April 10, 2017 4:24 AM
> To: Kevin Willford <kcwillford@gmail.com>
> Cc: git@vger.kernel.org; gitster@pobox.com; peff@peff.net; Kevin Willford
> <kewillf@microsoft.com>
> Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid
> data loss.
> 
> On Fri, Apr 07, 2017 at 12:23:57PM -0700, Kevin Willford wrote:
> > When using the sparse checkout feature the git reset command will add
> 
> "git reset" has three different modes. It would be good if you mention what
> mode is affected here. The tests are for --mixed only. I wonder if we need to
> do anything for --hard and --soft?
> 
> --soft touches branch SHA-1 index only, not worktree, so probably not.
> 
> --hard should be handled by unpack_trees(), I think.
> 
> But it would be good to cover these in the commit message as well to stop
> readers from wondering.
Sounds good.
Show 21 quoted lines
> 
> > entries to the index that will have the skip-worktree bit off but will
> > leave the working directory empty.  File data is lost because the
> > index version of the files has been changed but there is nothing that
> > is in the working directory.  This will cause the next status call to
> > show either deleted for files modified or deleting or nothing for files
> added.
> > The added files should be shown as untracked and modified files should
> > be shown as modified.
> 
> Hmm.. reading --mixed documentation again ("Resets the index but not
> working tree"), I think the current behavior is expected regardless of skip-
> worktree bits.
> 
> Perhaps the problem is the loss of skip-worktree bits on entries added by
> update_index_from_diff()? If the bits are at the right place, then it should
> not matter if the same version exists on worktree or not and "status" or
> "commit" should work as expected, I think.
> 
> --
> Duy

The loss of the skip-worktree bits is part of the problem if you are talking about modified files. The other issue that I was having is when running a reset and there were files added in the commit that is being reset, there will not be an entry in the index and not a file on disk so the data for that file is completely lost at that point. "status" also doesn't include anything about this loss of data. On modified files status will at least have the file as deleted since there is still an index entry but again the previous version of the file and it's data is lost.

To me this is totally unexpected behavior, for example if I am on a commit where there are only files that where added and run a reset HEAD~1 and then a status, it will show a clean working directory. Regardless of skip-worktree bits the user needs to have the data in the working directory after the reset or data is lost which is always bad.

Kevin
Duy Nguyen· Apr 12, 2017, 13:21 UTC · re: Kevin Willford · lore

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

On Wed, Apr 12, 2017 at 5:30 AM, Kevin Willford <kewillf@microsoft.com> wrote:
Show 8 quoted lines
> The loss of the skip-worktree bits is part of the problem if you are talking
> about modified files.  The other issue that I was having is when running a reset
> and there were files added in the commit that is being reset, there will not
> be an entry in the index and not a file on disk so the data for that file is
> completely lost at that point.  "status" also doesn't include anything about
> this loss of data.  On modified files status will at least have the file as deleted
> since there is still an index entry but again the previous version of the file
> and it's data is lost.

Well, we could have "deleted" index entries, those marked with CE_REMOVE. They are never written down to file though, so 'status' won't benefit from that. Hopefully we can restore the file before the index file is written down and we really lose skip-worktree bits?

Show 5 quoted lines
> To me this is totally unexpected behavior, for example if I am on a commit
> where there are only files that where added and run a reset HEAD~1 and
> then a status, it will show a clean working directory. Regardless of
> skip-worktree bits the user needs to have the data in the working directory
> after the reset or data is lost which is always bad.

I agree we no longer have a place to save the skip-worktree bit, we have to restore the state back as if skip-worktree bit does not exist. It would be good if we could keep the logic in unpack_trees() though. For example, if the file is present on disk even if skip-worktree bit is on, unpack_trees() would abort instead of silently overwriting it. This is a difference between skip-worktree and assume-unchanged bits. If you do explicit checkout_entry() you might have to add more checks to keep behavior consistent.

-- 
Duy
Kevin Willford· Apr 12, 2017, 15:37 UTC · re: Duy Nguyen · lore

RE: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

Show 26 quoted lines
> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Duy Nguyen
> Sent: Wednesday, April 12, 2017 7:21 AM
> To: Kevin Willford <kewillf@microsoft.com>
> Cc: Kevin Willford <kcwillford@gmail.com>; git@vger.kernel.org;
> gitster@pobox.com; peff@peff.net
> Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid
> data loss.
> 
> On Wed, Apr 12, 2017 at 5:30 AM, Kevin Willford <kewillf@microsoft.com>
> wrote:
> > The loss of the skip-worktree bits is part of the problem if you are
> > talking about modified files.  The other issue that I was having is
> > when running a reset and there were files added in the commit that is
> > being reset, there will not be an entry in the index and not a file on
> > disk so the data for that file is completely lost at that point.
> > "status" also doesn't include anything about this loss of data.  On
> > modified files status will at least have the file as deleted since
> > there is still an index entry but again the previous version of the file and it's
> data is lost.
> 
> Well, we could have "deleted" index entries, those marked with
> CE_REMOVE. They are never written down to file though, so 'status'
> won't benefit from that. Hopefully we can restore the file before the index
> file is written down and we really lose skip-worktree bits?

Because this is a reset --mixed it will never run through unpack_trees and The entries are never marked with CE_REMOVE.

Show 17 quoted lines
> 
> > To me this is totally unexpected behavior, for example if I am on a
> > commit where there are only files that where added and run a reset
> > HEAD~1 and then a status, it will show a clean working directory.
> > Regardless of skip-worktree bits the user needs to have the data in
> > the working directory after the reset or data is lost which is always bad.
> 
> I agree we no longer have a place to save the skip-worktree bit, we have to
> restore the state back as if skip-worktree bit does not exist.
> It would be good if we could keep the logic in unpack_trees() though.
> For example, if the file is present on disk even if skip-worktree bit is on,
> unpack_trees() would abort instead of silently overwriting it.
> This is a difference between skip-worktree and assume-unchanged bits.
> If you do explicit checkout_entry() you might have to add more checks to
> keep behavior consistent.
> --
> Duy

Because this is a reset --mixed it will follow the code path calling read_from_tree and ends up calling update_index_from_diff in the format_callback of the diff, so unpack_trees() is never called in the --mixed case. This code change also only applies when the file does not exist and the skip-worktree bit is on and the updated index entry either will be missing (covers the added scenario) or was not missing before (covers the modified scenario). If there is a better way to get the previous index entry to disk than what I am doing, I am happy to implement it correctly.

Thanks, Kevin

Duy Nguyen· Apr 16, 2017, 04:25 UTC · re: Kevin Willford · lore

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

On Wed, Apr 12, 2017 at 10:37 PM, Kevin Willford <kewillf@microsoft.com> wrote:
Show 30 quoted lines
>
>> -----Original Message-----
>> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
>> Behalf Of Duy Nguyen
>> Sent: Wednesday, April 12, 2017 7:21 AM
>> To: Kevin Willford <kewillf@microsoft.com>
>> Cc: Kevin Willford <kcwillford@gmail.com>; git@vger.kernel.org;
>> gitster@pobox.com; peff@peff.net
>> Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid
>> data loss.
>>
>> On Wed, Apr 12, 2017 at 5:30 AM, Kevin Willford <kewillf@microsoft.com>
>> wrote:
>> > The loss of the skip-worktree bits is part of the problem if you are
>> > talking about modified files.  The other issue that I was having is
>> > when running a reset and there were files added in the commit that is
>> > being reset, there will not be an entry in the index and not a file on
>> > disk so the data for that file is completely lost at that point.
>> > "status" also doesn't include anything about this loss of data.  On
>> > modified files status will at least have the file as deleted since
>> > there is still an index entry but again the previous version of the file and it's
>> data is lost.
>>
>> Well, we could have "deleted" index entries, those marked with
>> CE_REMOVE. They are never written down to file though, so 'status'
>> won't benefit from that. Hopefully we can restore the file before the index
>> file is written down and we really lose skip-worktree bits?
>
> Because this is a reset --mixed it will never run through unpack_trees and
> The entries are never marked with CE_REMOVE.

I know. But in my view, it should. All updates from a tree object to the index should happen through unpack_trees().

Show 24 quoted lines
>> > To me this is totally unexpected behavior, for example if I am on a
>> > commit where there are only files that where added and run a reset
>> > HEAD~1 and then a status, it will show a clean working directory.
>> > Regardless of skip-worktree bits the user needs to have the data in
>> > the working directory after the reset or data is lost which is always bad.
>>
>> I agree we no longer have a place to save the skip-worktree bit, we have to
>> restore the state back as if skip-worktree bit does not exist.
>> It would be good if we could keep the logic in unpack_trees() though.
>> For example, if the file is present on disk even if skip-worktree bit is on,
>> unpack_trees() would abort instead of silently overwriting it.
>> This is a difference between skip-worktree and assume-unchanged bits.
>> If you do explicit checkout_entry() you might have to add more checks to
>> keep behavior consistent.
>> --
>> Duy
>
> Because this is a reset --mixed it will follow the code path calling read_from_tree
> and ends up calling update_index_from_diff in the format_callback of the diff,
> so unpack_trees() is never called in the --mixed case.  This code change also only applies
> when the file does not exist and the skip-worktree bit is on and the updated
> index entry either will be missing (covers the added scenario) or was not missing
> before (covers the modified scenario).  If there is a better way to get the previous
> index entry to disk than what I am doing, I am happy to implement it correctly.

I think it's ok to just look at the diff (from update_index_from_diff) and restore the on-disk version for now. I'd like to make --mixed use unpack_trees() too but I haven't studied this code long enough to see why it went with "diff" instead of "read-tree" (which translates directly to unpack_trees). Maybe there is some subtle reason for that. Though it looks like it was more convenient to do "diff" in the git-reset.sh version, and that got translated literally to C when the command was rewritten.

-- 
Duy
Duy Nguyen· Apr 17, 2017, 12:09 UTC · re: Duy Nguyen · lore

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

On Sun, Apr 16, 2017 at 11:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 5 quoted lines
>> Because this is a reset --mixed it will never run through unpack_trees and
>> The entries are never marked with CE_REMOVE.
>
> I know. But in my view, it should. All updates from a tree object to
> the index should happen through unpack_trees().

Just fyi. My view is wrong. We need to handle a diff here, not through unpack_trees() because "git reset --mixed" support partial reset, see 2ce633b928 (git-reset [--mixed] <tree> [--] <paths>... - 2006-12-14). We might be able to make unpack_trees() leave certain paths(pec) untouched, but I don't think it is worth it. In other words, your original patch is the way to go.

PS. I briefly wondered if "git checkout <tree> -- <pathspec>" had the same problem. I think not, because while --mixed does not touch worktree, checkout does, so it should restore on-disk versions if needed. The read_tree_some() call in checkout_paths() should respect sparse patterns and add skip-worktree bits back if needed though, but I don't think it does that.

-- 
Duy
Stefan Beller· Apr 7, 2017, 19:27 UTC · re: Kevin Willford · lore

Re: [PATCH 0/3] fix working directory file issues while using sparse-checkout

+ Duy, for sparse checkout
On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
Show 21 quoted lines
> While using the sparse-checkout feature there are scenarios where
> the working directory should and should not be updated.  This patch
> series addresses issues found in reset, apply, and merge conflicts.
>
> Kevin Willford (3):
>   merge-recursive.c: conflict using sparse should update file
>   apply.c: do not checkout file when skip-worktree bit set
>   reset.c: update files when using sparse to avoid data loss.
>
>  apply.c                          | 18 +++++++++++++
>  builtin/reset.c                  | 34 +++++++++++++++++++++++
>  merge-recursive.c                |  8 ++++++
>  t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
>  t/t7614-merge-sparse-checkout.sh | 27 +++++++++++++++++++
>  5 files changed, 145 insertions(+)
>  create mode 100755 t/t7114-reset-sparse-checkout.sh
>  create mode 100755 t/t7614-merge-sparse-checkout.sh
>
> --
> 2.12.2.windows.2.1.g7df5db8d31
>
Stefan Beller· Apr 7, 2017, 19:27 UTC · re: Stefan Beller · lore

Re: [PATCH 0/3] fix working directory file issues while using sparse-checkout

This time for real.
On Fri, Apr 7, 2017 at 12:27 PM, Stefan Beller <sbeller@google.com> wrote:
Show 24 quoted lines
> + Duy, for sparse checkout
>
> On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
>> While using the sparse-checkout feature there are scenarios where
>> the working directory should and should not be updated.  This patch
>> series addresses issues found in reset, apply, and merge conflicts.
>>
>> Kevin Willford (3):
>>   merge-recursive.c: conflict using sparse should update file
>>   apply.c: do not checkout file when skip-worktree bit set
>>   reset.c: update files when using sparse to avoid data loss.
>>
>>  apply.c                          | 18 +++++++++++++
>>  builtin/reset.c                  | 34 +++++++++++++++++++++++
>>  merge-recursive.c                |  8 ++++++
>>  t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
>>  t/t7614-merge-sparse-checkout.sh | 27 +++++++++++++++++++
>>  5 files changed, 145 insertions(+)
>>  create mode 100755 t/t7114-reset-sparse-checkout.sh
>>  create mode 100755 t/t7614-merge-sparse-checkout.sh
>>
>> --
>> 2.12.2.windows.2.1.g7df5db8d31
>>

← back to recent threads