git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/4] Some more on top of nd/switch-and-restore
@ 2019-06-20  9:55 Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 1/4] t2027: use test_must_be_empty Nguyễn Thái Ngọc Duy
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-20  9:55 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

This is small refinements (except 4/4).

2/4 relaxes the 'in-progress' check for bisect because switching while
bisecting is normal _and_ safe. 3/4 makes 'switch -d' completion much
more useful. 4/4 adds the last missing piece in 'git restore', records
new files in worktree as i-t-a.

Still on the agenda (but may take some or much more time to do):

- submodule support in 'git restore'
- handling "git restore *.c" where *.c is expanded by shell

One item I have a patch for but decided not to send, is to imply
--detach in 'git switch' if you are already in detached HEAD mode and
want to switch to a non-branch. In other words, it behaves just like
git-checkout.

No more protection is needed in that case because you're in trouble
already if you don't know about detached HEAD. And if you do know,
then adding '-d' is just annoyance.

But I don't find myself using it and I'm a pretty heavy detached user.
So while it kinda makes sense to do, I don't think it's worth the
complication.

Nguyễn Thái Ngọc Duy (4):
  t2027: use test_must_be_empty
  switch: allow to switch in the middle of bisect
  completion: disable dwim on "git switch -d"
  restore: add --intent-to-add (restoring worktree only)

 Documentation/git-restore.txt          |  7 +++
 builtin/checkout.c                     | 82 +++++++++++++++++++++++++-
 contrib/completion/git-completion.bash |  4 ++
 t/t2070-restore.sh                     | 22 ++++++-
 4 files changed, 109 insertions(+), 6 deletions(-)

-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] t2027: use test_must_be_empty
  2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
@ 2019-06-20  9:55 ` Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 2/4] switch: allow to switch in the middle of bisect Nguyễn Thái Ngọc Duy
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-20  9:55 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2070-restore.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 73ea13ede9..2650df1966 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -90,9 +90,8 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
 
 		git restore --ignore-unmerged --quiet . >output 2>&1 &&
 		git diff common >diff-output &&
-		: >empty &&
-		test_cmp empty output &&
-		test_cmp empty diff-output
+		test_must_be_empty output &&
+		test_must_be_empty diff-output
 	)
 '
 
-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] switch: allow to switch in the middle of bisect
  2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 1/4] t2027: use test_must_be_empty Nguyễn Thái Ngọc Duy
@ 2019-06-20  9:55 ` Nguyễn Thái Ngọc Duy
  2019-06-20 14:02   ` Derrick Stolee
  2019-06-20  9:55 ` [PATCH 3/4] completion: disable dwim on "git switch -d" Nguyễn Thái Ngọc Duy
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-20  9:55 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

In c45f0f525d (switch: reject if some operation is in progress,
2019-03-29), a check is added to prevent switching when some operation
is in progress. The reason is it's often not safe to do so.

This is true for merge, am, rebase, cherry-pick and revert, but not so
much for bisect because bisecting is basically jumping/switching between
a bunch of commits to pin point the first bad one. git-bisect suggests
the next commit to test, but it's not wrong for the user to test a
different commit because git-bisect cannot have the knowledge to know
better.

For this reason, allow to switch when bisecting (*). I considered if we
should still prevent switching by default and allow it with
--ignore-in-progress. But I don't think the prevention really adds
anything much.

If the user switches away by mistake, since we print the previous HEAD
value, even if they don't know about the "-" shortcut, switching back is
still possible.

The warning will be printed on every switch while bisect is still
ongoing, not the first time you switch away from bisect's suggested
commit, so it could become a bit annoying.

(*) of course when it's safe to do so, i.e. no loss of local changes and
stuff.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bed79ae595..f884d27f1f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1326,9 +1326,7 @@ static void die_if_some_operation_in_progress(void)
 		      "Consider \"git revert --quit\" "
 		      "or \"git worktree add\"."));
 	if (state.bisect_in_progress)
-		die(_("cannot switch branch while bisecting\n"
-		      "Consider \"git bisect reset HEAD\" "
-		      "or \"git worktree add\"."));
+		warning(_("you are switching branch while bisecting"));
 }
 
 static int checkout_branch(struct checkout_opts *opts,
-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] completion: disable dwim on "git switch -d"
  2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 1/4] t2027: use test_must_be_empty Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 2/4] switch: allow to switch in the middle of bisect Nguyễn Thái Ngọc Duy
@ 2019-06-20  9:55 ` Nguyễn Thái Ngọc Duy
  2019-06-20  9:55 ` [PATCH 4/4] restore: add --intent-to-add (restoring worktree only) Nguyễn Thái Ngọc Duy
  2019-06-26 19:58 ` [PATCH 0/4] Some more on top of nd/switch-and-restore Junio C Hamano
  4 siblings, 0 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-20  9:55 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

Even though dwim is enabled by default, it will never be done when
--detached is specified. If you force "-d --guess" you will get an error
because --guess then implies -c which cannot be used with -d. So we can
disable dwim in "switch -d". It makes the completion list in this case a
bit shorter.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 contrib/completion/git-completion.bash | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 58d18d41a2..656e49710e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2183,6 +2183,10 @@ _git_switch ()
 		fi
 		if [ -z "$(__git_find_on_cmdline "-d --detach")" ]; then
 			only_local_ref=y
+		else
+			# --guess --detach is invalid combination, no
+			# dwim will be done when --detach is specified
+			track_opt=
 		fi
 		if [ $only_local_ref = y -a -z "$track_opt" ]; then
 			__gitcomp_direct "$(__git_heads "" "$cur" " ")"
-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)
  2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
                   ` (2 preceding siblings ...)
  2019-06-20  9:55 ` [PATCH 3/4] completion: disable dwim on "git switch -d" Nguyễn Thái Ngọc Duy
@ 2019-06-20  9:55 ` Nguyễn Thái Ngọc Duy
  2019-06-20 14:34   ` Derrick Stolee
  2019-06-26 19:58 ` [PATCH 0/4] Some more on top of nd/switch-and-restore Junio C Hamano
  4 siblings, 1 reply; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-20  9:55 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

"git restore --source" (without --staged) could create new files
(i.e. not present in index) on worktree to match the given source. But
the new files are not tracked, so both "git diff" and "git diff
<source>" ignore new files. "git commit -a" will not recreate a commit
exactly as the given source either.

Add --intent-to-add to help track new files in this case, which is the
default on the least surprise principle.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-restore.txt |  7 ++++
 builtin/checkout.c            | 78 +++++++++++++++++++++++++++++++++++
 t/t2070-restore.sh            | 17 ++++++++
 3 files changed, 102 insertions(+)

diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index d90093f195..43a7f43b2b 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -93,6 +93,13 @@ in linkgit:git-checkout[1] for details.
 	are "merge" (default) and "diff3" (in addition to what is
 	shown by "merge" style, shows the original contents).
 
+--intent-to-add::
+--no-intent-to-add::
+	When restoring files only on working tree with `--source`,
+	new files are marked as "intent to add" (see
+	linkgit:git-add[1]). This is the default behavior. Use
+	`--no-intent-to-add` to disable it.
+
 --ignore-unmerged::
 	When restoring files on the working tree from the index, do
 	not abort the operation if there are unmerged entries and
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f884d27f1f..c519067d3d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -70,6 +70,7 @@ struct checkout_opts {
 	int checkout_worktree;
 	const char *ignore_unmerged_opt;
 	int ignore_unmerged;
+	int intent_to_add;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)
 	return errs;
 }
 
+/*
+ * Input condition: r->index contains the file list matching worktree.
+ *
+ * r->index is reloaded with $GIT_DIR/index. Files that exist in the
+ * current worktree but not in $GIT_DIR/index are added back as
+ * intent-to-add.
+ */
+static int add_intent_to_add_files(struct repository *r)
+{
+	char **file_list;
+	int pos, worktree_nr, ita_nr = 0;
+	int ret = 0;
+
+	worktree_nr = r->index->cache_nr;
+	ALLOC_ARRAY(file_list, worktree_nr);
+	for (pos = 0; pos < worktree_nr; pos++)
+		file_list[pos] = xstrdup(r->index->cache[pos]->name);
+
+	discard_index(r->index);
+	if (repo_read_index(r) < 0) {
+		ret = error(_("index file corrupt"));
+		goto done;
+	}
+
+	for (pos = 0; pos < worktree_nr; ) {
+		const char *worktree = file_list[pos];
+		int index_pos = index_name_pos(r->index, worktree, strlen(worktree));
+
+		if (index_pos < 0) {
+			if (add_file_to_index(r->index, worktree, ADD_CACHE_INTENT))
+				ret = error(_("failed to add %s"), worktree);
+			else
+				ita_nr++;
+			pos++;
+			continue;
+		}
+
+		/*
+		 * Try to speed up the scanning process a bit.
+		 *
+		 * The assumption here is file_list[] and r->index->cache[]
+		 * are 90% the same. We can just skip a big chunk of the same
+		 * entries and reduce the number of binary lookups.
+		 */
+		pos++;
+		index_pos++;
+		while (pos < worktree_nr && index_pos < r->index->cache_nr &&
+		       !fspathcmp(file_list[pos], r->index->cache[index_pos]->name)) {
+			pos++;
+			index_pos++;
+		}
+	}
+
+	if (!ret)
+		ret = ita_nr;
+
+done:
+	for (pos = 0; pos < worktree_nr; pos++)
+		free(file_list[pos]);
+	free(file_list);
+	return ret;
+}
+
 static int checkout_paths(const struct checkout_opts *opts,
 			  const char *revision)
 {
@@ -531,6 +595,16 @@ static int checkout_paths(const struct checkout_opts *opts,
 	else
 		checkout_index = opts->checkout_index;
 
+	if (opts->intent_to_add && opts->from_treeish &&
+	    !opts->checkout_index && opts->checkout_worktree) {
+		int ita_nr = add_intent_to_add_files(the_repository);
+
+		if (ita_nr > 0)
+			checkout_index = 1;
+		if (ita_nr < 0)
+			errs = -1;
+	}
+
 	if (checkout_index) {
 		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die(_("unable to write new index file"));
@@ -1697,6 +1771,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.overlay_mode = -1;
 	opts.checkout_index = -2;    /* default on */
 	opts.checkout_worktree = -2; /* default on */
+	opts.intent_to_add = 0;
 
 	options = parse_options_dup(checkout_options);
 	options = add_common_options(&opts, options);
@@ -1758,6 +1833,8 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
 			   N_("restore the index")),
 		OPT_BOOL('W', "worktree", &opts.checkout_worktree,
 			   N_("restore the working tree (default)")),
+		OPT_BOOL(0, "intent-to-add", &opts.intent_to_add,
+			 N_("mark new files on working tree as intent-to-add (default)")),
 		OPT_BOOL(0, "ignore-unmerged", &opts.ignore_unmerged,
 			 N_("ignore unmerged entries")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
@@ -1773,6 +1850,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
 	opts.checkout_index = -1;    /* default off */
 	opts.checkout_worktree = -2; /* default on */
 	opts.ignore_unmerged_opt = "--ignore-unmerged";
+	opts.intent_to_add = 1;
 
 	options = parse_options_dup(restore_options);
 	options = add_common_options(&opts, options);
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 2650df1966..acbd80c1cd 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -95,4 +95,21 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
 	)
 '
 
+test_expect_success 'restore worktree only adds new files back as intent-to-add' '
+	git init ita &&
+	(
+		cd ita &&
+		test_commit one &&
+		test_commit two &&
+		git rm one.t &&
+		git commit -m one-is-gone &&
+		git restore --source one one.t &&
+		git diff --summary >actual &&
+		echo " create mode 100644 one.t" >expected &&
+		test_cmp expected actual &&
+		git diff --cached >empty &&
+		test_must_be_empty empty
+	)
+'
+
 test_done
-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] switch: allow to switch in the middle of bisect
  2019-06-20  9:55 ` [PATCH 2/4] switch: allow to switch in the middle of bisect Nguyễn Thái Ngọc Duy
@ 2019-06-20 14:02   ` Derrick Stolee
  2019-06-20 15:06     ` Duy Nguyen
  0 siblings, 1 reply; 13+ messages in thread
From: Derrick Stolee @ 2019-06-20 14:02 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy, git

On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> In c45f0f525d (switch: reject if some operation is in progress,
> 2019-03-29), a check is added to prevent switching when some operation
> is in progress. The reason is it's often not safe to do so.
> 
> This is true for merge, am, rebase, cherry-pick and revert, but not so
> much for bisect because bisecting is basically jumping/switching between
> a bunch of commits to pin point the first bad one. git-bisect suggests
> the next commit to test, but it's not wrong for the user to test a
> different commit because git-bisect cannot have the knowledge to know
> better.

When a user switches commits during a bisect, it can just create new
known-good or known-bad commits, right? It won't mess up the next
selection of a test commit? I'm imagining someone jumping to a commit
between two known-bad commits or something, when it is more likely that
they are jumping to a parallel history.

> For this reason, allow to switch when bisecting (*). I considered if we
> should still prevent switching by default and allow it with
> --ignore-in-progress. But I don't think the prevention really adds
> anything much.
>
> If the user switches away by mistake, since we print the previous HEAD
> value, even if they don't know about the "-" shortcut, switching back is
> still possible.

I tell everyone I know about the "-" shortcut, and I'm always surprised
they didn't already know about "cd -".

> The warning will be printed on every switch while bisect is still
> ongoing, not the first time you switch away from bisect's suggested
> commit, so it could become a bit annoying.

I think a one-line warning is fine, as a power user doing this a lot
will develop blinders that ignore the message as they switch during
a bisect.

-Stolee

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)
  2019-06-20  9:55 ` [PATCH 4/4] restore: add --intent-to-add (restoring worktree only) Nguyễn Thái Ngọc Duy
@ 2019-06-20 14:34   ` Derrick Stolee
  2019-06-20 14:58     ` Duy Nguyen
  0 siblings, 1 reply; 13+ messages in thread
From: Derrick Stolee @ 2019-06-20 14:34 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy, git

On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> "git restore --source" (without --staged) could create new files
> (i.e. not present in index) on worktree to match the given source. But
> the new files are not tracked, so both "git diff" and "git diff
> <source>" ignore new files. "git commit -a" will not recreate a commit
> exactly as the given source either.
> 
> Add --intent-to-add to help track new files in this case, which is the
> default on the least surprise principle.

I was unfamiliar with this behavior, but did check the 'restore' command
myself and saw that it would register the file as untracked. I agree that
could be confusing for a user, so adding it to the staging environment
makes this more in-line with `git checkout <rev> -- <path>`.

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-restore.txt |  7 ++++
>  builtin/checkout.c            | 78 +++++++++++++++++++++++++++++++++++
>  t/t2070-restore.sh            | 17 ++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
> index d90093f195..43a7f43b2b 100644
> --- a/Documentation/git-restore.txt
> +++ b/Documentation/git-restore.txt
> @@ -93,6 +93,13 @@ in linkgit:git-checkout[1] for details.
>  	are "merge" (default) and "diff3" (in addition to what is
>  	shown by "merge" style, shows the original contents).
>  
> +--intent-to-add::
> +--no-intent-to-add::
> +	When restoring files only on working tree with `--source`,
> +	new files are marked as "intent to add" (see
> +	linkgit:git-add[1]). This is the default behavior. Use
> +	`--no-intent-to-add` to disable it.
> +
>  --ignore-unmerged::
>  	When restoring files on the working tree from the index, do
>  	not abort the operation if there are unmerged entries and
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f884d27f1f..c519067d3d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -70,6 +70,7 @@ struct checkout_opts {
>  	int checkout_worktree;
>  	const char *ignore_unmerged_opt;
>  	int ignore_unmerged;
> +	int intent_to_add;
>  
>  	const char *new_branch;
>  	const char *new_branch_force;
> @@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)
>  	return errs;
>  }
>  
> +/*
> + * Input condition: r->index contains the file list matching worktree.
> + *
> + * r->index is reloaded with $GIT_DIR/index. Files that exist in the
> + * current worktree but not in $GIT_DIR/index are added back as
> + * intent-to-add.
> + */

Reading this code (and being unfamiliar with the cache array) I thought
it might accidentally add untracked files from the working directory into
the index. A local test verified that was not the case. Is that worth
adding to your test below?
  
> +test_expect_success 'restore worktree only adds new files back as intent-to-add' '
> +	git init ita &&
> +	(
> +		cd ita &&
> +		test_commit one &&
> +		test_commit two &&
> +		git rm one.t &&
> +		git commit -m one-is-gone &&
+		touch garbage &&
> +		git restore --source one one.t &&
> +		git diff --summary >actual &&
> +		echo " create mode 100644 one.t" >expected &&
> +		test_cmp expected actual &&
> +		git diff --cached >empty &&
> +		test_must_be_empty empty
> +	)
> +'
> +
>  test_done

Perhaps the line I inserted above would suffice to add this extra check?

Outside of that extra test (which may not be necessary), this series makes
sense to me.

Thanks,
-Stolee



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)
  2019-06-20 14:34   ` Derrick Stolee
@ 2019-06-20 14:58     ` Duy Nguyen
  0 siblings, 0 replies; 13+ messages in thread
From: Duy Nguyen @ 2019-06-20 14:58 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Git Mailing List

On Thu, Jun 20, 2019 at 9:34 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> > "git restore --source" (without --staged) could create new files
> > (i.e. not present in index) on worktree to match the given source. But
> > the new files are not tracked, so both "git diff" and "git diff
> > <source>" ignore new files. "git commit -a" will not recreate a commit
> > exactly as the given source either.
> >
> > Add --intent-to-add to help track new files in this case, which is the
> > default on the least surprise principle.
>
> I was unfamiliar with this behavior, but did check the 'restore' command
> myself and saw that it would register the file as untracked. I agree that
> could be confusing for a user, so adding it to the staging environment
> makes this more in-line with `git checkout <rev> -- <path>`.

It's actually not the same as "git checkout <rev>" which would restore
<path> in both index and worktree, while "git restore" (no --staged)
only touches worktree . Try "git diff --cached" and "git diff" in both
cases, you'll see the differences.

Or in other words, "git commit" (no -a) after "git checkout" records
the version of <path> from <rev>, while "git commit" after "git
restore" will commit whatever you have before git-restore. "git commit
-a" behaves the same way for both (though it drops <path> without this
patch).

> > @@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)
> >       return errs;
> >  }
> >
> > +/*
> > + * Input condition: r->index contains the file list matching worktree.
> > + *
> > + * r->index is reloaded with $GIT_DIR/index. Files that exist in the
> > + * current worktree but not in $GIT_DIR/index are added back as
> > + * intent-to-add.
> > + */
>
> Reading this code (and being unfamiliar with the cache array) I thought
> it might accidentally add untracked files from the working directory into
> the index. A local test verified that was not the case. Is that worth
> adding to your test below?

It never occured to me because r->index (before this function) should
be the same as <rev>, more or less. But yeah, adding a garbage file
and checking that it remains garbage is a good idea. I'll rename it
"untracked" though to be clear.
-- 
Duy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] switch: allow to switch in the middle of bisect
  2019-06-20 14:02   ` Derrick Stolee
@ 2019-06-20 15:06     ` Duy Nguyen
  0 siblings, 0 replies; 13+ messages in thread
From: Duy Nguyen @ 2019-06-20 15:06 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Git Mailing List

On Thu, Jun 20, 2019 at 9:02 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> > In c45f0f525d (switch: reject if some operation is in progress,
> > 2019-03-29), a check is added to prevent switching when some operation
> > is in progress. The reason is it's often not safe to do so.
> >
> > This is true for merge, am, rebase, cherry-pick and revert, but not so
> > much for bisect because bisecting is basically jumping/switching between
> > a bunch of commits to pin point the first bad one. git-bisect suggests
> > the next commit to test, but it's not wrong for the user to test a
> > different commit because git-bisect cannot have the knowledge to know
> > better.
>
> When a user switches commits during a bisect, it can just create new
> known-good or known-bad commits, right? It won't mess up the next
> selection of a test commit? I'm imagining someone jumping to a commit
> between two known-bad commits or something, when it is more likely that
> they are jumping to a parallel history.

Until you run "git bisect bad" or "git bisect good" I don't think
switching could mess up bisect. And I'm pretty sure I marked wrong
ones once or twice and git-bisect did complain.

It would be a good idea to warn about jumping in known-good-or-bad
commit ranges. I'll keep this as an improvement point (there's still
another one I haven't done, also about warning improvement, sigh).

> > For this reason, allow to switch when bisecting (*). I considered if we
> > should still prevent switching by default and allow it with
> > --ignore-in-progress. But I don't think the prevention really adds
> > anything much.
> >
> > If the user switches away by mistake, since we print the previous HEAD
> > value, even if they don't know about the "-" shortcut, switching back is
> > still possible.
>
> I tell everyone I know about the "-" shortcut, and I'm always surprised
> they didn't already know about "cd -".

I actually wanted to go a bit more aggressive/verbose about "teaching"
users via the advice framework, but allows the user to turn off the
"lessons" they already learned. Something like gcc adding
[-Wno-foobar] in a warning to hint how to turn it off. I'll come back
to this at some point, hopefully.
-- 
Duy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] Some more on top of nd/switch-and-restore
  2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
                   ` (3 preceding siblings ...)
  2019-06-20  9:55 ` [PATCH 4/4] restore: add --intent-to-add (restoring worktree only) Nguyễn Thái Ngọc Duy
@ 2019-06-26 19:58 ` Junio C Hamano
  2019-06-27  2:53   ` Duy Nguyen
  4 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2019-06-26 19:58 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is small refinements (except 4/4).

What's the status of these?  As another low-prio topic interferes
with the code touched by nd/switch-and-restore and hence needs to
wait for these to stabilize, I'd rather see us focus on finishing
these before switching our attention to other things.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] Some more on top of nd/switch-and-restore
  2019-06-26 19:58 ` [PATCH 0/4] Some more on top of nd/switch-and-restore Junio C Hamano
@ 2019-06-27  2:53   ` Duy Nguyen
  2019-06-27  8:53     ` Duy Nguyen
  0 siblings, 1 reply; 13+ messages in thread
From: Duy Nguyen @ 2019-06-27  2:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > This is small refinements (except 4/4).
>
> What's the status of these?

Small test fixup needed. I should be able to do it later today.

> As another low-prio topic interferes
> with the code touched by nd/switch-and-restore and hence needs to
> wait for these to stabilize, I'd rather see us focus on finishing
> these before switching our attention to other things.
>
> Thanks.
-- 
Duy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] Some more on top of nd/switch-and-restore
  2019-06-27  2:53   ` Duy Nguyen
@ 2019-06-27  8:53     ` Duy Nguyen
  2019-06-27 17:53       ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Duy Nguyen @ 2019-06-27  8:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Thu, Jun 27, 2019 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> >
> > > This is small refinements (except 4/4).
> >
> > What's the status of these?
>
> Small test fixup needed. I should be able to do it later today.

Actually since the patch that needs updates is 4/4, which is not part
of nd/switch-and-store-more, I think the status is "ready". You
probably could safely make them part nd/switch-and-restore.
-- 
Duy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] Some more on top of nd/switch-and-restore
  2019-06-27  8:53     ` Duy Nguyen
@ 2019-06-27 17:53       ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2019-06-27 17:53 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jun 27, 2019 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:
>>
>> On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>> >
>> > > This is small refinements (except 4/4).
>> >
>> > What's the status of these?
>>
>> Small test fixup needed. I should be able to do it later today.
>
> Actually since the patch that needs updates is 4/4, which is not part
> of nd/switch-and-store-more, I think the status is "ready". You
> probably could safely make them part nd/switch-and-restore.

Yup, that matches my understanding after re-reading them.

Thanks.\

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-06-27 17:53 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-20  9:55 [PATCH 0/4] Some more on top of nd/switch-and-restore Nguyễn Thái Ngọc Duy
2019-06-20  9:55 ` [PATCH 1/4] t2027: use test_must_be_empty Nguyễn Thái Ngọc Duy
2019-06-20  9:55 ` [PATCH 2/4] switch: allow to switch in the middle of bisect Nguyễn Thái Ngọc Duy
2019-06-20 14:02   ` Derrick Stolee
2019-06-20 15:06     ` Duy Nguyen
2019-06-20  9:55 ` [PATCH 3/4] completion: disable dwim on "git switch -d" Nguyễn Thái Ngọc Duy
2019-06-20  9:55 ` [PATCH 4/4] restore: add --intent-to-add (restoring worktree only) Nguyễn Thái Ngọc Duy
2019-06-20 14:34   ` Derrick Stolee
2019-06-20 14:58     ` Duy Nguyen
2019-06-26 19:58 ` [PATCH 0/4] Some more on top of nd/switch-and-restore Junio C Hamano
2019-06-27  2:53   ` Duy Nguyen
2019-06-27  8:53     ` Duy Nguyen
2019-06-27 17:53       ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).