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

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

[Kevin Willford]

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

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

[Kevin Willford]

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

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

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

[Duy Nguyen]

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.

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

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

[Kevin Willford]

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.
+	 */
+	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

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

[Stefan Beller]

On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
> 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

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

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

[Duy Nguyen]

On Fri, Apr 07, 2017 at 12:23:56PM -0700, Kevin Willford wrote:
> 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.

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

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

[Kevin Willford]

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

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

[Stefan Beller]

On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:
> 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.

>         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 (..) {
    ..

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

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

[Duy Nguyen]

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.

> 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

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

[Kevin Willford]

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

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

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

[Duy Nguyen]

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?

> 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

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

[Kevin Willford]

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

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

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

[Duy Nguyen]

On Wed, Apr 12, 2017 at 10:37 PM, Kevin Willford <kewillf@microsoft.com> wrote:
>
>> -----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().

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

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

[Duy Nguyen]

On Sun, Apr 16, 2017 at 11:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> 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

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

[Stefan Beller]

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

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

[Stefan Beller]

This time for real.

On Fri, Apr 7, 2017 at 12:27 PM, Stefan Beller <sbeller@google.com> wrote:
> + 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
>>