{"thread":{"id":"51350","subject":"[PATCH 0/4] Some more on top of nd/switch-and-restore","startedAt":"2019-06-20T09:55:30Z","lastAt":"2019-06-27T17:53:20Z","messageCount":13,"participants":["Nguyễn Thái Ngọc Duy","Derrick Stolee","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"377637","messageId":"20190620095523.10003-1-pclouds@gmail.com","threadId":"51350","inReplyTo":null,"subject":"[PATCH 0/4] Some more on top of nd/switch-and-restore","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T09:55:19Z","receivedAt":"2019-06-20T09:55:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is small refinements (except 4/4).\n\n2/4 relaxes the 'in-progress' check for bisect because switching while\nbisecting is normal _and_ safe. 3/4 makes 'switch -d' completion much\nmore useful. 4/4 adds the last missing piece in 'git restore', records\nnew files in worktree as i-t-a.\n\nStill on the agenda (but may take some or much more time to do):\n\n- submodule support in 'git restore'\n- handling \"git restore *.c\" where *.c is expanded by shell\n\nOne item I have a patch for but decided not to send, is to imply\n--detach in 'git switch' if you are already in detached HEAD mode and\nwant to switch to a non-branch. In other words, it behaves just like\ngit-checkout.\n\nNo more protection is needed in that case because you're in trouble\nalready if you don't know about detached HEAD. And if you do know,\nthen adding '-d' is just annoyance.\n\nBut I don't find myself using it and I'm a pretty heavy detached user.\nSo while it kinda makes sense to do, I don't think it's worth the\ncomplication.\n\nNguyễn Thái Ngọc Duy (4):\n  t2027: use test_must_be_empty\n  switch: allow to switch in the middle of bisect\n  completion: disable dwim on \"git switch -d\"\n  restore: add --intent-to-add (restoring worktree only)\n\n Documentation/git-restore.txt          |  7 +++\n builtin/checkout.c                     | 82 +++++++++++++++++++++++++-\n contrib/completion/git-completion.bash |  4 ++\n t/t2070-restore.sh                     | 22 ++++++-\n 4 files changed, 109 insertions(+), 6 deletions(-)\n\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377638","messageId":"20190620095523.10003-2-pclouds@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-1-pclouds@gmail.com","subject":"[PATCH 1/4] t2027: use test_must_be_empty","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T09:55:20Z","receivedAt":"2019-06-20T09:55:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n t/t2070-restore.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 73ea13ede9..2650df1966 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -90,9 +90,8 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n \n \t\tgit restore --ignore-unmerged --quiet . >output 2>&1 &&\n \t\tgit diff common >diff-output &&\n-\t\t: >empty &&\n-\t\ttest_cmp empty output &&\n-\t\ttest_cmp empty diff-output\n+\t\ttest_must_be_empty output &&\n+\t\ttest_must_be_empty diff-output\n \t)\n '\n \n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377639","messageId":"20190620095523.10003-3-pclouds@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-1-pclouds@gmail.com","subject":"[PATCH 2/4] switch: allow to switch in the middle of bisect","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T09:55:21Z","receivedAt":"2019-06-20T09:55:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In c45f0f525d (switch: reject if some operation is in progress,\n2019-03-29), a check is added to prevent switching when some operation\nis in progress. The reason is it's often not safe to do so.\n\nThis is true for merge, am, rebase, cherry-pick and revert, but not so\nmuch for bisect because bisecting is basically jumping/switching between\na bunch of commits to pin point the first bad one. git-bisect suggests\nthe next commit to test, but it's not wrong for the user to test a\ndifferent commit because git-bisect cannot have the knowledge to know\nbetter.\n\nFor this reason, allow to switch when bisecting (*). I considered if we\nshould still prevent switching by default and allow it with\n--ignore-in-progress. But I don't think the prevention really adds\nanything much.\n\nIf the user switches away by mistake, since we print the previous HEAD\nvalue, even if they don't know about the \"-\" shortcut, switching back is\nstill possible.\n\nThe warning will be printed on every switch while bisect is still\nongoing, not the first time you switch away from bisect's suggested\ncommit, so it could become a bit annoying.\n\n(*) of course when it's safe to do so, i.e. no loss of local changes and\nstuff.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex bed79ae595..f884d27f1f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1326,9 +1326,7 @@ static void die_if_some_operation_in_progress(void)\n \t\t      \"Consider \\\"git revert --quit\\\" \"\n \t\t      \"or \\\"git worktree add\\\".\"));\n \tif (state.bisect_in_progress)\n-\t\tdie(_(\"cannot switch branch while bisecting\\n\"\n-\t\t      \"Consider \\\"git bisect reset HEAD\\\" \"\n-\t\t      \"or \\\"git worktree add\\\".\"));\n+\t\twarning(_(\"you are switching branch while bisecting\"));\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377640","messageId":"20190620095523.10003-4-pclouds@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-1-pclouds@gmail.com","subject":"[PATCH 3/4] completion: disable dwim on \"git switch -d\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T09:55:22Z","receivedAt":"2019-06-20T09:55:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Even though dwim is enabled by default, it will never be done when\n--detached is specified. If you force \"-d --guess\" you will get an error\nbecause --guess then implies -c which cannot be used with -d. So we can\ndisable dwim in \"switch -d\". It makes the completion list in this case a\nbit shorter.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n contrib/completion/git-completion.bash | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 58d18d41a2..656e49710e 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2183,6 +2183,10 @@ _git_switch ()\n \t\tfi\n \t\tif [ -z \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n \t\t\tonly_local_ref=y\n+\t\telse\n+\t\t\t# --guess --detach is invalid combination, no\n+\t\t\t# dwim will be done when --detach is specified\n+\t\t\ttrack_opt=\n \t\tfi\n \t\tif [ $only_local_ref = y -a -z \"$track_opt\" ]; then\n \t\t\t__gitcomp_direct \"$(__git_heads \"\" \"$cur\" \" \")\"\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377641","messageId":"20190620095523.10003-5-pclouds@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-1-pclouds@gmail.com","subject":"[PATCH 4/4] restore: add --intent-to-add (restoring worktree only)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T09:55:23Z","receivedAt":"2019-06-20T09:55:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git restore --source\" (without --staged) could create new files\n(i.e. not present in index) on worktree to match the given source. But\nthe new files are not tracked, so both \"git diff\" and \"git diff\n<source>\" ignore new files. \"git commit -a\" will not recreate a commit\nexactly as the given source either.\n\nAdd --intent-to-add to help track new files in this case, which is the\ndefault on the least surprise principle.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-restore.txt |  7 ++++\n builtin/checkout.c            | 78 +++++++++++++++++++++++++++++++++++\n t/t2070-restore.sh            | 17 ++++++++\n 3 files changed, 102 insertions(+)\n\ndiff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\nindex d90093f195..43a7f43b2b 100644\n--- a/Documentation/git-restore.txt\n+++ b/Documentation/git-restore.txt\n@@ -93,6 +93,13 @@ in linkgit:git-checkout[1] for details.\n \tare \"merge\" (default) and \"diff3\" (in addition to what is\n \tshown by \"merge\" style, shows the original contents).\n \n+--intent-to-add::\n+--no-intent-to-add::\n+\tWhen restoring files only on working tree with `--source`,\n+\tnew files are marked as \"intent to add\" (see\n+\tlinkgit:git-add[1]). This is the default behavior. Use\n+\t`--no-intent-to-add` to disable it.\n+\n --ignore-unmerged::\n \tWhen restoring files on the working tree from the index, do\n \tnot abort the operation if there are unmerged entries and\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f884d27f1f..c519067d3d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -70,6 +70,7 @@ struct checkout_opts {\n \tint checkout_worktree;\n \tconst char *ignore_unmerged_opt;\n \tint ignore_unmerged;\n+\tint intent_to_add;\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)\n \treturn errs;\n }\n \n+/*\n+ * Input condition: r->index contains the file list matching worktree.\n+ *\n+ * r->index is reloaded with $GIT_DIR/index. Files that exist in the\n+ * current worktree but not in $GIT_DIR/index are added back as\n+ * intent-to-add.\n+ */\n+static int add_intent_to_add_files(struct repository *r)\n+{\n+\tchar **file_list;\n+\tint pos, worktree_nr, ita_nr = 0;\n+\tint ret = 0;\n+\n+\tworktree_nr = r->index->cache_nr;\n+\tALLOC_ARRAY(file_list, worktree_nr);\n+\tfor (pos = 0; pos < worktree_nr; pos++)\n+\t\tfile_list[pos] = xstrdup(r->index->cache[pos]->name);\n+\n+\tdiscard_index(r->index);\n+\tif (repo_read_index(r) < 0) {\n+\t\tret = error(_(\"index file corrupt\"));\n+\t\tgoto done;\n+\t}\n+\n+\tfor (pos = 0; pos < worktree_nr; ) {\n+\t\tconst char *worktree = file_list[pos];\n+\t\tint index_pos = index_name_pos(r->index, worktree, strlen(worktree));\n+\n+\t\tif (index_pos < 0) {\n+\t\t\tif (add_file_to_index(r->index, worktree, ADD_CACHE_INTENT))\n+\t\t\t\tret = error(_(\"failed to add %s\"), worktree);\n+\t\t\telse\n+\t\t\t\tita_nr++;\n+\t\t\tpos++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Try to speed up the scanning process a bit.\n+\t\t *\n+\t\t * The assumption here is file_list[] and r->index->cache[]\n+\t\t * are 90% the same. We can just skip a big chunk of the same\n+\t\t * entries and reduce the number of binary lookups.\n+\t\t */\n+\t\tpos++;\n+\t\tindex_pos++;\n+\t\twhile (pos < worktree_nr && index_pos < r->index->cache_nr &&\n+\t\t       !fspathcmp(file_list[pos], r->index->cache[index_pos]->name)) {\n+\t\t\tpos++;\n+\t\t\tindex_pos++;\n+\t\t}\n+\t}\n+\n+\tif (!ret)\n+\t\tret = ita_nr;\n+\n+done:\n+\tfor (pos = 0; pos < worktree_nr; pos++)\n+\t\tfree(file_list[pos]);\n+\tfree(file_list);\n+\treturn ret;\n+}\n+\n static int checkout_paths(const struct checkout_opts *opts,\n \t\t\t  const char *revision)\n {\n@@ -531,6 +595,16 @@ static int checkout_paths(const struct checkout_opts *opts,\n \telse\n \t\tcheckout_index = opts->checkout_index;\n \n+\tif (opts->intent_to_add && opts->from_treeish &&\n+\t    !opts->checkout_index && opts->checkout_worktree) {\n+\t\tint ita_nr = add_intent_to_add_files(the_repository);\n+\n+\t\tif (ita_nr > 0)\n+\t\t\tcheckout_index = 1;\n+\t\tif (ita_nr < 0)\n+\t\t\terrs = -1;\n+\t}\n+\n \tif (checkout_index) {\n \t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(_(\"unable to write new index file\"));\n@@ -1697,6 +1771,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \topts.overlay_mode = -1;\n \topts.checkout_index = -2;    /* default on */\n \topts.checkout_worktree = -2; /* default on */\n+\topts.intent_to_add = 0;\n \n \toptions = parse_options_dup(checkout_options);\n \toptions = add_common_options(&opts, options);\n@@ -1758,6 +1833,8 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \t\t\t   N_(\"restore the index\")),\n \t\tOPT_BOOL('W', \"worktree\", &opts.checkout_worktree,\n \t\t\t   N_(\"restore the working tree (default)\")),\n+\t\tOPT_BOOL(0, \"intent-to-add\", &opts.intent_to_add,\n+\t\t\t N_(\"mark new files on working tree as intent-to-add (default)\")),\n \t\tOPT_BOOL(0, \"ignore-unmerged\", &opts.ignore_unmerged,\n \t\t\t N_(\"ignore unmerged entries\")),\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode\")),\n@@ -1773,6 +1850,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \topts.checkout_index = -1;    /* default off */\n \topts.checkout_worktree = -2; /* default on */\n \topts.ignore_unmerged_opt = \"--ignore-unmerged\";\n+\topts.intent_to_add = 1;\n \n \toptions = parse_options_dup(restore_options);\n \toptions = add_common_options(&opts, options);\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 2650df1966..acbd80c1cd 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -95,4 +95,21 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n \t)\n '\n \n+test_expect_success 'restore worktree only adds new files back as intent-to-add' '\n+\tgit init ita &&\n+\t(\n+\t\tcd ita &&\n+\t\ttest_commit one &&\n+\t\ttest_commit two &&\n+\t\tgit rm one.t &&\n+\t\tgit commit -m one-is-gone &&\n+\t\tgit restore --source one one.t &&\n+\t\tgit diff --summary >actual &&\n+\t\techo \" create mode 100644 one.t\" >expected &&\n+\t\ttest_cmp expected actual &&\n+\t\tgit diff --cached >empty &&\n+\t\ttest_must_be_empty empty\n+\t)\n+'\n+\n test_done\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377657","messageId":"446ace86-714f-9109-99d8-95554ceaf26b@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-3-pclouds@gmail.com","subject":"Re: [PATCH 2/4] switch: allow to switch in the middle of bisect","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-20T14:02:26Z","receivedAt":"2019-06-20T14:02:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:\n> In c45f0f525d (switch: reject if some operation is in progress,\n> 2019-03-29), a check is added to prevent switching when some operation\n> is in progress. The reason is it's often not safe to do so.\n> \n> This is true for merge, am, rebase, cherry-pick and revert, but not so\n> much for bisect because bisecting is basically jumping/switching between\n> a bunch of commits to pin point the first bad one. git-bisect suggests\n> the next commit to test, but it's not wrong for the user to test a\n> different commit because git-bisect cannot have the knowledge to know\n> better.\n\nWhen a user switches commits during a bisect, it can just create new\nknown-good or known-bad commits, right? It won't mess up the next\nselection of a test commit? I'm imagining someone jumping to a commit\nbetween two known-bad commits or something, when it is more likely that\nthey are jumping to a parallel history.\n\n> For this reason, allow to switch when bisecting (*). I considered if we\n> should still prevent switching by default and allow it with\n> --ignore-in-progress. But I don't think the prevention really adds\n> anything much.\n>\n> If the user switches away by mistake, since we print the previous HEAD\n> value, even if they don't know about the \"-\" shortcut, switching back is\n> still possible.\n\nI tell everyone I know about the \"-\" shortcut, and I'm always surprised\nthey didn't already know about \"cd -\".\n\n> The warning will be printed on every switch while bisect is still\n> ongoing, not the first time you switch away from bisect's suggested\n> commit, so it could become a bit annoying.\n\nI think a one-line warning is fine, as a power user doing this a lot\nwill develop blinders that ignore the message as they switch during\na bisect.\n\n-Stolee\n"},{"id":"377661","messageId":"e232fbc4-06ec-d4ed-826a-3bcbc923cafe@gmail.com","threadId":"51350","inReplyTo":"20190620095523.10003-5-pclouds@gmail.com","subject":"Re: [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-20T14:34:39Z","receivedAt":"2019-06-20T14:34:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:\n> \"git restore --source\" (without --staged) could create new files\n> (i.e. not present in index) on worktree to match the given source. But\n> the new files are not tracked, so both \"git diff\" and \"git diff\n> <source>\" ignore new files. \"git commit -a\" will not recreate a commit\n> exactly as the given source either.\n> \n> Add --intent-to-add to help track new files in this case, which is the\n> default on the least surprise principle.\n\nI was unfamiliar with this behavior, but did check the 'restore' command\nmyself and saw that it would register the file as untracked. I agree that\ncould be confusing for a user, so adding it to the staging environment\nmakes this more in-line with `git checkout <rev> -- <path>`.\n\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Documentation/git-restore.txt |  7 ++++\n>  builtin/checkout.c            | 78 +++++++++++++++++++++++++++++++++++\n>  t/t2070-restore.sh            | 17 ++++++++\n>  3 files changed, 102 insertions(+)\n> \n> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\n> index d90093f195..43a7f43b2b 100644\n> --- a/Documentation/git-restore.txt\n> +++ b/Documentation/git-restore.txt\n> @@ -93,6 +93,13 @@ in linkgit:git-checkout[1] for details.\n>  \tare \"merge\" (default) and \"diff3\" (in addition to what is\n>  \tshown by \"merge\" style, shows the original contents).\n>  \n> +--intent-to-add::\n> +--no-intent-to-add::\n> +\tWhen restoring files only on working tree with `--source`,\n> +\tnew files are marked as \"intent to add\" (see\n> +\tlinkgit:git-add[1]). This is the default behavior. Use\n> +\t`--no-intent-to-add` to disable it.\n> +\n>  --ignore-unmerged::\n>  \tWhen restoring files on the working tree from the index, do\n>  \tnot abort the operation if there are unmerged entries and\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index f884d27f1f..c519067d3d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -70,6 +70,7 @@ struct checkout_opts {\n>  \tint checkout_worktree;\n>  \tconst char *ignore_unmerged_opt;\n>  \tint ignore_unmerged;\n> +\tint intent_to_add;\n>  \n>  \tconst char *new_branch;\n>  \tconst char *new_branch_force;\n> @@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)\n>  \treturn errs;\n>  }\n>  \n> +/*\n> + * Input condition: r->index contains the file list matching worktree.\n> + *\n> + * r->index is reloaded with $GIT_DIR/index. Files that exist in the\n> + * current worktree but not in $GIT_DIR/index are added back as\n> + * intent-to-add.\n> + */\n\nReading this code (and being unfamiliar with the cache array) I thought\nit might accidentally add untracked files from the working directory into\nthe index. A local test verified that was not the case. Is that worth\nadding to your test below?\n  \n> +test_expect_success 'restore worktree only adds new files back as intent-to-add' '\n> +\tgit init ita &&\n> +\t(\n> +\t\tcd ita &&\n> +\t\ttest_commit one &&\n> +\t\ttest_commit two &&\n> +\t\tgit rm one.t &&\n> +\t\tgit commit -m one-is-gone &&\n+\t\ttouch garbage &&\n> +\t\tgit restore --source one one.t &&\n> +\t\tgit diff --summary >actual &&\n> +\t\techo \" create mode 100644 one.t\" >expected &&\n> +\t\ttest_cmp expected actual &&\n> +\t\tgit diff --cached >empty &&\n> +\t\ttest_must_be_empty empty\n> +\t)\n> +'\n> +\n>  test_done\n\nPerhaps the line I inserted above would suffice to add this extra check?\n\nOutside of that extra test (which may not be necessary), this series makes\nsense to me.\n\nThanks,\n-Stolee\n\n\n"},{"id":"377663","messageId":"CACsJy8BcXU_BfHhkwnK4MA2nQ0ZyBnHN8zMoV+5aK5G+w+uOPg@mail.gmail.com","threadId":"51350","inReplyTo":"e232fbc4-06ec-d4ed-826a-3bcbc923cafe@gmail.com","subject":"Re: [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T14:58:01Z","receivedAt":"2019-06-20T14:58:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 20, 2019 at 9:34 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:\n> > \"git restore --source\" (without --staged) could create new files\n> > (i.e. not present in index) on worktree to match the given source. But\n> > the new files are not tracked, so both \"git diff\" and \"git diff\n> > <source>\" ignore new files. \"git commit -a\" will not recreate a commit\n> > exactly as the given source either.\n> >\n> > Add --intent-to-add to help track new files in this case, which is the\n> > default on the least surprise principle.\n>\n> I was unfamiliar with this behavior, but did check the 'restore' command\n> myself and saw that it would register the file as untracked. I agree that\n> could be confusing for a user, so adding it to the staging environment\n> makes this more in-line with `git checkout <rev> -- <path>`.\n\nIt's actually not the same as \"git checkout <rev>\" which would restore\n<path> in both index and worktree, while \"git restore\" (no --staged)\nonly touches worktree . Try \"git diff --cached\" and \"git diff\" in both\ncases, you'll see the differences.\n\nOr in other words, \"git commit\" (no -a) after \"git checkout\" records\nthe version of <path> from <rev>, while \"git commit\" after \"git\nrestore\" will commit whatever you have before git-restore. \"git commit\n-a\" behaves the same way for both (though it drops <path> without this\npatch).\n\n> > @@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)\n> >       return errs;\n> >  }\n> >\n> > +/*\n> > + * Input condition: r->index contains the file list matching worktree.\n> > + *\n> > + * r->index is reloaded with $GIT_DIR/index. Files that exist in the\n> > + * current worktree but not in $GIT_DIR/index are added back as\n> > + * intent-to-add.\n> > + */\n>\n> Reading this code (and being unfamiliar with the cache array) I thought\n> it might accidentally add untracked files from the working directory into\n> the index. A local test verified that was not the case. Is that worth\n> adding to your test below?\n\nIt never occured to me because r->index (before this function) should\nbe the same as <rev>, more or less. But yeah, adding a garbage file\nand checking that it remains garbage is a good idea. I'll rename it\n\"untracked\" though to be clear.\n-- \nDuy\n"},{"id":"377664","messageId":"CACsJy8DoEwoR67BopXLPiBmuidixREn-Ac16V5di6sFZx_Gbng@mail.gmail.com","threadId":"51350","inReplyTo":"446ace86-714f-9109-99d8-95554ceaf26b@gmail.com","subject":"Re: [PATCH 2/4] switch: allow to switch in the middle of bisect","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-20T15:06:51Z","receivedAt":"2019-06-20T15:07:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 20, 2019 at 9:02 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:\n> > In c45f0f525d (switch: reject if some operation is in progress,\n> > 2019-03-29), a check is added to prevent switching when some operation\n> > is in progress. The reason is it's often not safe to do so.\n> >\n> > This is true for merge, am, rebase, cherry-pick and revert, but not so\n> > much for bisect because bisecting is basically jumping/switching between\n> > a bunch of commits to pin point the first bad one. git-bisect suggests\n> > the next commit to test, but it's not wrong for the user to test a\n> > different commit because git-bisect cannot have the knowledge to know\n> > better.\n>\n> When a user switches commits during a bisect, it can just create new\n> known-good or known-bad commits, right? It won't mess up the next\n> selection of a test commit? I'm imagining someone jumping to a commit\n> between two known-bad commits or something, when it is more likely that\n> they are jumping to a parallel history.\n\nUntil you run \"git bisect bad\" or \"git bisect good\" I don't think\nswitching could mess up bisect. And I'm pretty sure I marked wrong\nones once or twice and git-bisect did complain.\n\nIt would be a good idea to warn about jumping in known-good-or-bad\ncommit ranges. I'll keep this as an improvement point (there's still\nanother one I haven't done, also about warning improvement, sigh).\n\n> > For this reason, allow to switch when bisecting (*). I considered if we\n> > should still prevent switching by default and allow it with\n> > --ignore-in-progress. But I don't think the prevention really adds\n> > anything much.\n> >\n> > If the user switches away by mistake, since we print the previous HEAD\n> > value, even if they don't know about the \"-\" shortcut, switching back is\n> > still possible.\n>\n> I tell everyone I know about the \"-\" shortcut, and I'm always surprised\n> they didn't already know about \"cd -\".\n\nI actually wanted to go a bit more aggressive/verbose about \"teaching\"\nusers via the advice framework, but allows the user to turn off the\n\"lessons\" they already learned. Something like gcc adding\n[-Wno-foobar] in a warning to hint how to turn it off. I'll come back\nto this at some point, hopefully.\n-- \nDuy\n"},{"id":"378078","messageId":"xmqq8stoce5w.fsf@gitster-ct.c.googlers.com","threadId":"51350","inReplyTo":"20190620095523.10003-1-pclouds@gmail.com","subject":"Re: [PATCH 0/4] Some more on top of nd/switch-and-restore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-26T19:58:03Z","receivedAt":"2019-06-26T19:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This is small refinements (except 4/4).\n\nWhat's the status of these?  As another low-prio topic interferes\nwith the code touched by nd/switch-and-restore and hence needs to\nwait for these to stabilize, I'd rather see us focus on finishing\nthese before switching our attention to other things.\n\nThanks.\n"},{"id":"378111","messageId":"CACsJy8Di1720RhDxgieVNTfpNONJhi5ZniKje=wj4pZDy-0EwQ@mail.gmail.com","threadId":"51350","inReplyTo":"xmqq8stoce5w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] Some more on top of nd/switch-and-restore","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-27T02:53:33Z","receivedAt":"2019-06-27T02:54:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n> > This is small refinements (except 4/4).\n>\n> What's the status of these?\n\nSmall test fixup needed. I should be able to do it later today.\n\n> As another low-prio topic interferes\n> with the code touched by nd/switch-and-restore and hence needs to\n> wait for these to stabilize, I'd rather see us focus on finishing\n> these before switching our attention to other things.\n>\n> Thanks.\n-- \nDuy\n"},{"id":"378131","messageId":"CACsJy8CeChO6wJkr_Pp8aH1a6rJwUCDYtK89SLwLw_9KOgQHeA@mail.gmail.com","threadId":"51350","inReplyTo":"CACsJy8Di1720RhDxgieVNTfpNONJhi5ZniKje=wj4pZDy-0EwQ@mail.gmail.com","subject":"Re: [PATCH 0/4] Some more on top of nd/switch-and-restore","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-27T08:53:43Z","receivedAt":"2019-06-27T08:54:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 27, 2019 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n> >\n> > > This is small refinements (except 4/4).\n> >\n> > What's the status of these?\n>\n> Small test fixup needed. I should be able to do it later today.\n\nActually since the patch that needs updates is 4/4, which is not part\nof nd/switch-and-store-more, I think the status is \"ready\". You\nprobably could safely make them part nd/switch-and-restore.\n-- \nDuy\n"},{"id":"378180","messageId":"xmqq8stmc3uf.fsf@gitster-ct.c.googlers.com","threadId":"51350","inReplyTo":"CACsJy8CeChO6wJkr_Pp8aH1a6rJwUCDYtK89SLwLw_9KOgQHeA@mail.gmail.com","subject":"Re: [PATCH 0/4] Some more on top of nd/switch-and-restore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-27T17:53:12Z","receivedAt":"2019-06-27T17:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Thu, Jun 27, 2019 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:\n>>\n>> On Thu, Jun 27, 2019 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> >\n>> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>> >\n>> > > This is small refinements (except 4/4).\n>> >\n>> > What's the status of these?\n>>\n>> Small test fixup needed. I should be able to do it later today.\n>\n> Actually since the patch that needs updates is 4/4, which is not part\n> of nd/switch-and-store-more, I think the status is \"ready\". You\n> probably could safely make them part nd/switch-and-restore.\n\nYup, that matches my understanding after re-reading them.\n\nThanks.\\\n"}]}