Skip to contentSkip to conversation

fix working directory file issues while using sparse-checkout

PATCH In review Kevin Willford 4 participants 16 messages Apr 7, 2017 – Apr 17, 2017

lore
Share & tools
lore

Kevin Willford

author
#0 lore raw

cover fix working directory file issues while using sparse-checkout

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
Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
author
#1 lore raw

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 <[email protected]>
---
 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
merge-recursive.c +8 -0
index b7ff1ada3c..54fce93dae 100644
@@ -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."),
t/t7614-merge-sparse-checkout.sh new file +27 -0
new file mode 100755
index 0000000000..6922f0244f
@@ -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
Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
#2 lore raw

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

Quoted 4 lines · 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.

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.

Quoted 63 lines
Signed-off-by Kevin Willford <[email protected]>
---
 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
merge-recursive.c +8 -0
index b7ff1ada3c..54fce93dae 100644
@@ -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."),
t/t7614-merge-sparse-checkout.sh new file +27 -0
new file mode 100755
index 0000000000..6922f0244f
@@ -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
Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 apply.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
apply.c +18 -0
index 0e2caeab9c..0da5d0b7c9 100644
@@ -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))
Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
#4 lore raw

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

Quoted 38 lines · 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 <[email protected]>
---
 apply.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
apply.c +5 -0
index 0e2caeab9c..0da5d0b7c9 100644
@@ -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

apply.c static int checkout_target(struct index_state *istate, · line 3344

Quoted 10 lines
apply.c +10 -0
+        * 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

Quoted 9 lines
apply.c +3 -1
+       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

#5 lore raw

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

In reply to Kevin Willford
Quoted 14 lines · 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.

Quoted 5 lines

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.

Quoted 38 lines
Signed-off-by Kevin Willford <[email protected]>
---
 apply.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
apply.c +18 -0
index 0e2caeab9c..0da5d0b7c9 100644
@@ -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))
Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 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
builtin/reset.c +34 -0
index 8ab915bfcb..8ba97999f4 100644
@@ -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);
t/t7114-reset-sparse-checkout.sh new file +58 -0

58 lines not on this page · raw message

Signature · 1 line
2.12.2.windows.2.1.g7df5db8d31
#7 lore raw

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

Quoted 35 lines · 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 <[email protected]>
---
 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
builtin/reset.c +1 -0
index 8ab915bfcb..8ba97999f4 100644
@@ -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>]"),

builtin/reset.c static void update_index_from_diff(struct diff_queue_struct *q, · line 121

Quoted 4 lines
builtin/reset.c +1 -0
@@ -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.

Quoted 11 lines
builtin/reset.c +5 -0
        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;
+

builtin/reset.c static void update_index_from_diff(struct diff_queue_struct *q, · line 142

Quoted 10 lines
builtin/reset.c +10 -0
+               /*
+                * 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. ;-)

builtin/reset.c static void update_index_from_diff(struct diff_queue_struct *q, · line 144

Quoted 2 lines
builtin/reset.c +2 -0
+               if (core_apply_sparse_checkout && !file_exists(two->path))
+               {

Coding style: Git prefers { at the end of the line:

  if (..) {
    ..

builtin/reset.c static void update_index_from_diff(struct diff_queue_struct *q, · line 149

Quoted 5 lines
builtin/reset.c +5 -0
+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

#8 lore raw

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

In reply to Kevin Willford
Quoted 1 line · Kevin Willford

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.

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

#9 lore raw

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

Quoted 16 lines

-----Original Message-----
From: Duy Nguyen [mailto:[email protected]]
Sent: Monday, April 10, 2017 4:24 AM
To: Kevin Willford <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Kevin Willford <[email protected]>
Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

Quoted 1 line · Kevin Willford

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?

Quoted 6 lines

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

Quoted 14 lines
Quoted 6 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.

Quoted 2 lines

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.

Quoted 7 lines

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

#10 lore raw

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

Quoted 8 lines · Kevin Willford

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?

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

Signature · 1 line
Duy
#11 lore raw

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

Quoted 21 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Duy Nguyen
Sent: Wednesday, April 12, 2017 7:21 AM
To: Kevin Willford <[email protected]>
Cc: Kevin Willford <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

Quoted 8 lines · Kevin Willford

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

Quoted 6 lines

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.

Quoted 7 lines
Quoted 6 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.

Quoted 10 lines

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

#12 lore raw

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

Quoted 23 lines · Kevin Willford
Quoted 23 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Duy Nguyen
Sent: Wednesday, April 12, 2017 7:21 AM
To: Kevin Willford <[email protected]>
Cc: Kevin Willford <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

Quoted 8 lines · Kevin Willford

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.

Quoted 7 lines
Quoted 4 lines

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

Quoted 17 lines
Quoted 6 lines
Quoted 5 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.

Quoted 10 lines

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

Quoted 7 lines

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.

Signature · 1 line
Duy
#13 lore raw

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

Quoted 5 lines · Duy Nguyen
Quoted 2 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.

Signature · 1 line
Duy
#14 lore raw
In reply to Kevin Willford

+ Duy, for sparse checkout

Quoted 21 lines · 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

#15 lore raw

This time for real.

Quoted 24 lines · Stefan Beller

+ Duy, for sparse checkout

Quoted 21 lines · 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