| Message ID | 85b81f2f06bd1b40ee2de220cc84dd74b425daf3.1613459475.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | stash show: learn --include-untracked and --only-untracked | expand |
Denton Liu <liu.denton@gmail.com> writes: > +static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt) > +{ > + const struct object_id *oid[] = { &info->w_commit, &info->u_tree }; > + struct tree *tree[ARRAY_SIZE(oid)]; > + struct tree_desc tree_desc[ARRAY_SIZE(oid)]; > + struct unpack_trees_options unpack_tree_opt = { 0 }; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(oid); i++) { > + tree[i] = parse_tree_indirect(oid[i]); > + if (parse_tree(tree[i]) < 0) > + die(_("failed to parse tree")); > + init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size); > + } > + > + unpack_tree_opt.head_idx = -1; > + unpack_tree_opt.src_index = &the_index; > + unpack_tree_opt.dst_index = &the_index; > + unpack_tree_opt.fn = twoway_merge; OK, it looks like this was borrowed from read_tree implementation for reading two trees into the index, sort-of, but is a bit funny. The setting of .fn to twoway_merge is misleading. The .fn callback is to be used when .merge is set (otherwise nothing should call it inside unpack-trees.c), but nobody seems to set opt.merge to true. > + if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt)) > + die(_("failed to unpack trees")); > + > + do_diff_cache(&info->b_commit, diff_opt); > +} Nice to see that it was just a simple matter of programming ;-) builtin/stash.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git c/builtin/stash.c w/builtin/stash.c index c788a3e236..7e0204bd8a 100644 --- c/builtin/stash.c +++ w/builtin/stash.c @@ -807,10 +807,11 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size); } + /* mimic "git read-tree W U" without "-m" */ unpack_tree_opt.head_idx = -1; unpack_tree_opt.src_index = &the_index; unpack_tree_opt.dst_index = &the_index; - unpack_tree_opt.fn = twoway_merge; + unpack_tree_opt.fn = NULL; if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt)) die(_("failed to unpack trees"));
Denton Liu <liu.denton@gmail.com> writes: > diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh > index b26a97aef4..978bc97baf 100755 > --- a/t/t3905-stash-include-untracked.sh > +++ b/t/t3905-stash-include-untracked.sh > @@ -297,4 +297,88 @@ test_expect_success 'stash -u with globs' ' > test_path_is_missing untracked.txt > ' > > +test_expect_success 'stash show --include-untracked shows untracked files' ' > + ... > + cat >expect <<-EOF && > + diff --git a/tracked b/tracked > + new file mode 100644 > + index 0000000..e69de29 > + diff --git a/untracked b/untracked > + new file mode 100644 > + index 0000000..e69de29 > + EOF We'd need this on top. Thanks. ----- >8 ---------- >8 ---------- >8 ---------- >8 ----- Subject: [PATCH] SQUASH??? t/t3905-stash-include-untracked.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 8bcd4c5ca8..a706ab80a5 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -303,6 +303,7 @@ test_expect_success 'stash show --include-untracked shows untracked files' ' >untracked && >tracked && git add tracked && + empty_blob_oid=$(git rev-parse --short :tracked) && git stash -u && cat >expect <<-EOF && @@ -324,10 +325,10 @@ test_expect_success 'stash show --include-untracked shows untracked files' ' cat >expect <<-EOF && diff --git a/tracked b/tracked new file mode 100644 - index 0000000..e69de29 + index 0000000..$empty_blob_oid diff --git a/untracked b/untracked new file mode 100644 - index 0000000..e69de29 + index 0000000..$empty_blob_oid EOF git stash show -p --include-untracked >actual && test_cmp expect actual && @@ -341,6 +342,7 @@ test_expect_success 'stash show --only-untracked only shows untracked files' ' >untracked && >tracked && git add tracked && + empty_blob_oid=$(git rev-parse --short :tracked) && git stash -u && cat >expect <<-EOF && @@ -357,7 +359,7 @@ test_expect_success 'stash show --only-untracked only shows untracked files' ' cat >expect <<-EOF && diff --git a/untracked b/untracked new file mode 100644 - index 0000000..e69de29 + index 0000000..$empty_blob_oid EOF git stash show -p --only-untracked >actual && test_cmp expect actual &&
Hi Junio, On Tue, Feb 16, 2021 at 12:22:52PM -0800, Junio C Hamano wrote: > builtin/stash.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git c/builtin/stash.c w/builtin/stash.c > index c788a3e236..7e0204bd8a 100644 > --- c/builtin/stash.c > +++ w/builtin/stash.c > @@ -807,10 +807,11 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op > init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size); > } > > + /* mimic "git read-tree W U" without "-m" */ > unpack_tree_opt.head_idx = -1; > unpack_tree_opt.src_index = &the_index; > unpack_tree_opt.dst_index = &the_index; > - unpack_tree_opt.fn = twoway_merge; > + unpack_tree_opt.fn = NULL; Perhaps it would be even more clear if we just removed this line entirely, otherwise it may give future readers a false impression that .fn is significant in any way. Aside from that, both of your SQUASH??? commits look good to me. Thanks for tying up the loose ends. -Denton > if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt)) > die(_("failed to unpack trees"));
Denton Liu <liu.denton@gmail.com> writes: > Hi Junio, > > On Tue, Feb 16, 2021 at 12:22:52PM -0800, Junio C Hamano wrote: >> builtin/stash.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git c/builtin/stash.c w/builtin/stash.c >> index c788a3e236..7e0204bd8a 100644 >> --- c/builtin/stash.c >> +++ w/builtin/stash.c >> @@ -807,10 +807,11 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op >> init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size); >> } >> >> + /* mimic "git read-tree W U" without "-m" */ >> unpack_tree_opt.head_idx = -1; >> unpack_tree_opt.src_index = &the_index; >> unpack_tree_opt.dst_index = &the_index; >> - unpack_tree_opt.fn = twoway_merge; >> + unpack_tree_opt.fn = NULL; > > Perhaps it would be even more clear if we just removed this line > entirely, otherwise it may give future readers a false impression that > .fn is significant in any way. Assignment of something concrete like "twoway_merge" to .fn when it is a no-op was misleading, but between NULL or uninitialized, both state clear that it is not used, so I am OK with either. > Aside from that, both of your SQUASH??? commits look good to me. Thanks > for tying up the loose ends. We may want to write a custom unpack_trees() callback for this, and use it here to catch "this third tree claims to be untracked, but why does it have an entry that overlaps and/or D/F-conflicts with the entry from the working tree side?" that you talked about in the log message, but I think it is better to leave it for future [*], as the feature should already be usable in its current shape. Thanks. [Footnote] * Yes, I didn't say "we can leave it", but said "it is better to leave it"; as long as we do not declare victory too early and end up shipping a half-baked unusable mess, I think it is better to wrap a topic early and plan to make it nicer in a future follow-up. To encourage such future refinements, however, you may add a NEEDSWORK comment in the code as a reminder for our future selves.
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index f1197d641b..8eeb60feb1 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git stash' list [<log-options>] -'git stash' show [<diff-options>] [<stash>] +'git stash' show [-u|--include-untracked|--only-untracked] [<diff-options>] [<stash>] 'git stash' drop [-q|--quiet] [<stash>] 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>] 'git stash' branch <branchname> [<stash>] @@ -83,7 +83,7 @@ stash@{1}: On master: 9cc0589... Add git-stash The command takes options applicable to the 'git log' command to control what is shown and how. See linkgit:git-log[1]. -show [<diff-options>] [<stash>]:: +show [-u|--include-untracked|--only-untracked] [<diff-options>] [<stash>]:: Show the changes recorded in the stash entry as a diff between the stashed contents and the commit back when the stash entry was first @@ -160,10 +160,18 @@ up with `git clean`. -u:: --include-untracked:: - This option is only valid for `push` and `save` commands. +--no-include-untracked:: + When used with the `push` and `save` commands, + all untracked files are also stashed and then cleaned up with + `git clean`. + -All untracked files are also stashed and then cleaned up with -`git clean`. +When used with the `show` command, show the untracked files in the stash +entry as part of the diff. + +--only-untracked:: + This option is only valid for the `show` command. ++ +Show only the untracked files in the stash entry as part of the diff. --index:: This option is only valid for `pop` and `apply` commands. diff --git a/builtin/stash.c b/builtin/stash.c index 6f2b58f6ab..417ed2b4a1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -787,6 +787,32 @@ static int git_stash_config(const char *var, const char *value, void *cb) return git_diff_basic_config(var, value, cb); } +static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt) +{ + const struct object_id *oid[] = { &info->w_commit, &info->u_tree }; + struct tree *tree[ARRAY_SIZE(oid)]; + struct tree_desc tree_desc[ARRAY_SIZE(oid)]; + struct unpack_trees_options unpack_tree_opt = { 0 }; + int i; + + for (i = 0; i < ARRAY_SIZE(oid); i++) { + tree[i] = parse_tree_indirect(oid[i]); + if (parse_tree(tree[i]) < 0) + die(_("failed to parse tree")); + init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size); + } + + unpack_tree_opt.head_idx = -1; + unpack_tree_opt.src_index = &the_index; + unpack_tree_opt.dst_index = &the_index; + unpack_tree_opt.fn = twoway_merge; + + if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt)) + die(_("failed to unpack trees")); + + do_diff_cache(&info->b_commit, diff_opt); +} + static int show_stash(int argc, const char **argv, const char *prefix) { int i; @@ -795,7 +821,18 @@ static int show_stash(int argc, const char **argv, const char *prefix) struct rev_info rev; struct strvec stash_args = STRVEC_INIT; struct strvec revision_args = STRVEC_INIT; + enum { + UNTRACKED_NONE, + UNTRACKED_INCLUDE, + UNTRACKED_ONLY + } show_untracked = UNTRACKED_NONE; struct option options[] = { + OPT_SET_INT('u', "include-untracked", &show_untracked, + N_("include untracked files in the stash"), + UNTRACKED_INCLUDE), + OPT_SET_INT_F(0, "only-untracked", &show_untracked, + N_("only show untracked files in the stash"), + UNTRACKED_ONLY, PARSE_OPT_NONEG), OPT_END() }; @@ -803,6 +840,10 @@ static int show_stash(int argc, const char **argv, const char *prefix) git_config(git_diff_ui_config, NULL); init_revisions(&rev, prefix); + argc = parse_options(argc, argv, prefix, options, git_stash_show_usage, + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN | + PARSE_OPT_KEEP_DASHDASH); + strvec_push(&revision_args, argv[0]); for (i = 1; i < argc; i++) { if (argv[i][0] != '-') @@ -845,7 +886,17 @@ static int show_stash(int argc, const char **argv, const char *prefix) rev.diffopt.flags.recursive = 1; setup_diff_pager(&rev.diffopt); - diff_tree_oid(&info.b_commit, &info.w_commit, "", &rev.diffopt); + switch (show_untracked) { + case UNTRACKED_NONE: + diff_tree_oid(&info.b_commit, &info.w_commit, "", &rev.diffopt); + break; + case UNTRACKED_ONLY: + diff_root_tree_oid(&info.u_tree, "", &rev.diffopt); + break; + case UNTRACKED_INCLUDE: + diff_include_untracked(&info, &rev.diffopt); + break; + } log_tree_diff_flush(&rev); free_stash_info(&info); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 4b1f4264a6..64ef6ffa21 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3051,7 +3051,7 @@ _git_stash () __gitcomp "--name-status --oneline --patch-with-stat" ;; show,--*) - __gitcomp "$__git_diff_common_options" + __gitcomp "--include-untracked --only-untracked $__git_diff_common_options" ;; branch,--*) ;; diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index b26a97aef4..978bc97baf 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -297,4 +297,88 @@ test_expect_success 'stash -u with globs' ' test_path_is_missing untracked.txt ' +test_expect_success 'stash show --include-untracked shows untracked files' ' + git reset --hard && + git clean -xf && + >untracked && + >tracked && + git add tracked && + git stash -u && + + cat >expect <<-EOF && + tracked | 0 + untracked | 0 + 2 files changed, 0 insertions(+), 0 deletions(-) + EOF + git stash show --include-untracked >actual && + test_cmp expect actual && + git stash show -u >actual && + test_cmp expect actual && + git stash show --no-include-untracked --include-untracked >actual && + test_cmp expect actual && + git stash show --only-untracked --include-untracked >actual && + test_cmp expect actual && + + cat >expect <<-EOF && + diff --git a/tracked b/tracked + new file mode 100644 + index 0000000..e69de29 + diff --git a/untracked b/untracked + new file mode 100644 + index 0000000..e69de29 + EOF + git stash show -p --include-untracked >actual && + test_cmp expect actual && + git stash show --include-untracked -p >actual && + test_cmp expect actual +' + +test_expect_success 'stash show --only-untracked only shows untracked files' ' + git reset --hard && + git clean -xf && + >untracked && + >tracked && + git add tracked && + git stash -u && + + cat >expect <<-EOF && + untracked | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + EOF + git stash show --only-untracked >actual && + test_cmp expect actual && + git stash show --no-include-untracked --only-untracked >actual && + test_cmp expect actual && + git stash show --include-untracked --only-untracked >actual && + test_cmp expect actual && + + cat >expect <<-EOF && + diff --git a/untracked b/untracked + new file mode 100644 + index 0000000..e69de29 + EOF + git stash show -p --only-untracked >actual && + test_cmp expect actual && + git stash show --only-untracked -p >actual && + test_cmp expect actual +' + +test_expect_success 'stash show --no-include-untracked cancels --{include,show}-untracked' ' + git reset --hard && + git clean -xf && + >untracked && + >tracked && + git add tracked && + git stash -u && + + cat >expect <<-EOF && + tracked | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + EOF + git stash show --only-untracked --no-include-untracked >actual && + test_cmp expect actual && + git stash show --include-untracked --no-include-untracked >actual && + test_cmp expect actual +' + test_done
Stash entries can be made with untracked files via `git stash push --include-untracked`. However, because the untracked files are stored in the third parent of the stash entry and not the stash entry itself, running `git stash show` does not include the untracked files as part of the diff. With --include-untracked, untracked paths, which are recorded in the third-parent if it exists, are shown in addition to the paths that have modifications between the stash base and the working tree in the stash. One limitation of this is that it would be possible to manually craft a stash entry where duplicate untracked files in the stash entry will mask tracked files. This seems like an instance of "Doctor, it hurts when I do this! So don't do that!" so this can be written off. Also, teach stash the --only-untracked option which only shows the untracked files of a stash entry. This is similar to `git show stash^3` but it is nice to provide a convenient abstraction for it so that users do not have to think about the underlying implementation. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Notes: I am not familiar with the read-tree code so this attempt at replicating the read-tree code may in diff_include_untracked() may be incorrect (particularly the use of the_index?). Also, I could not figure out how to make unpack_trees() error out in the case where untracked tree entry contains duplicate entries with the worktree entry. Documentation/git-stash.txt | 18 ++++-- builtin/stash.c | 53 +++++++++++++++- contrib/completion/git-completion.bash | 2 +- t/t3905-stash-include-untracked.sh | 84 ++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 7 deletions(-)