| Message ID | 20190620095523.10003-5-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some more on top of nd/switch-and-restore | expand |
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
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.
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
"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(+)