| Message ID | 88d47912595b5650fbca595a6dd5b7b943a93301.1612855690.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: > 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. > Teach stash the --include-untracked option, which also displays the > untracked files in a stash entry from the third parent (if it exists). A few points: - "Teach stash the --include-untracked option"? (some part of) "stash" knows --include-untracked already. Perhaps "Teach 'stash show' the '--include-untracked' option"? - "which also displays"? Let's spell it out that untracked paths are shown in addition to what. "With this option, untracked paths recorded in the third-parent (if exists) are shown, in addition to the paths whose modifications between the stash base and the working tree are stashed", or something like that, perhaps? > Do this via something like > > GIT_INDEX_FILE=... git read-tree stash stash^3 > > and diffing the resulting tree object against the stash base. That explains the implementation, but does not make it clear what the implementation wants to achieve. So we read the tree from stash (i.e. working tree) into a temporary index, and then overlay the tree of stash^3 (i.e. untracked) on top---which means the resulting "index" has the state of the working tree plus the untracked cruft in it. And comparing that with "stash base" (by the way is that a term well understood? I borrowed it for the above review comment, which shows that there certainly is need for such a term) would show the diff between the "HEAD" and the state that would have result if you were to do an "git add ." in the working tree. OK. > One improvement that this could use for the future is performing the > action without writing anything to disk as one would expect this to be a > read-only operation. This can be fixed in the future, however. Is it so difficult that we have to delay the fix for "the future"? After reading two trees into an in-core index, without writing it out to any file, all that remains to be done is just a matter of running diff-lib.c::do_diff_cache(), no? I must be missing something.q > Another 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. Well, when you read the second tree into the in-core index to overlay what you read from the working tree state, you can certainly report the collision and error it out. > 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. OK. > -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`. Back when "--include-untracked" was there and no "--only-untracked" existed, it made sense for the former to squat on a short-and-sweet "-u". Now it comes back to bite us ;-) > + > -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. OK. > +--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. OK. > diff --git a/builtin/stash.c b/builtin/stash.c > index 6f2b58f6ab..f7220fad56 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -787,6 +787,47 @@ static int git_stash_config(const char *var, const char *value, void *cb) > return git_diff_basic_config(var, value, cb); > } > > +static int merge_track_untracked(struct object_id *result, const struct stash_info *info) > +{ > + int ret = 0; > + struct index_state istate = { NULL }; > + struct child_process cp_read_tree = CHILD_PROCESS_INIT; > + > + if (!info->has_u) { > + oidcpy(result, &info->w_commit); > + return 0; > + } > + > + /* > + * TODO: is there a way of doing this all in-memory without writing > + * anything to disk? > + */ Of course. Read and study what read-tree does, which boils down to a call to unpack_trees() without .merge option.
Hi Junio, On Tue, Feb 09, 2021 at 11:53:06PM -0800, Junio C Hamano wrote: > > Do this via something like > > > > GIT_INDEX_FILE=... git read-tree stash stash^3 > > > > and diffing the resulting tree object against the stash base. > > That explains the implementation, but does not make it clear what > the implementation wants to achieve. So we read the tree from stash > (i.e. working tree) into a temporary index, and then overlay the > tree of stash^3 (i.e. untracked) on top---which means the resulting > "index" has the state of the working tree plus the untracked cruft > in it. And comparing that with "stash base" (by the way is that a > term well understood? I borrowed it for the above review comment, > which shows that there certainly is need for such a term) would show I'm not sure if it's a well-understood term but I can't think of any other meanings for the term so it doesn't seem very ambiguous. > the diff between the "HEAD" and the state that would have result if > you were to do an "git add ." in the working tree. OK. > > > One improvement that this could use for the future is performing the > > action without writing anything to disk as one would expect this to be a > > read-only operation. This can be fixed in the future, however. > > Is it so difficult that we have to delay the fix for "the future"? > After reading two trees into an in-core index, without writing it > out to any file, all that remains to be done is just a matter of > running diff-lib.c::do_diff_cache(), no? I must be missing something.q No, I don't think it's difficult. It's just my inexperience with this area of the code. > > Another 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. > > Well, when you read the second tree into the in-core index to > overlay what you read from the working tree state, you can certainly > report the collision and error it out. I'll send out my revised patch later today and I was unable to figure out an easy way of doing this. Thanks, Denton
Denton Liu <liu.denton@gmail.com> writes: > Hi Junio, > > On Tue, Feb 09, 2021 at 11:53:06PM -0800, Junio C Hamano wrote: >> > Do this via something like >> > >> > GIT_INDEX_FILE=... git read-tree stash stash^3 >> > >> > and diffing the resulting tree object against the stash base. >> >> That explains the implementation, but does not make it clear what >> the implementation wants to achieve. So we read the tree from stash >> (i.e. working tree) into a temporary index, and then overlay the >> tree of stash^3 (i.e. untracked) on top---which means the resulting >> "index" has the state of the working tree plus the untracked cruft >> in it. And comparing that with "stash base" (by the way is that a >> term well understood? I borrowed it for the above review comment, >> which shows that there certainly is need for such a term) would show > > I'm not sure if it's a well-understood term but I can't think of any > other meanings for the term so it doesn't seem very ambiguous. Thanks. I was hoping to hear either "Yes, glossary defines it like this" or "I believe it is an unambiguous good term; let's add it to the glossary". > I'll send out my revised patch later today and I was unable to figure > out an easy way of doing this. OK.
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 04e55eb826..9d4b9f0b5c 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -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..f7220fad56 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -787,6 +787,47 @@ static int git_stash_config(const char *var, const char *value, void *cb) return git_diff_basic_config(var, value, cb); } +static int merge_track_untracked(struct object_id *result, const struct stash_info *info) +{ + int ret = 0; + struct index_state istate = { NULL }; + struct child_process cp_read_tree = CHILD_PROCESS_INIT; + + if (!info->has_u) { + oidcpy(result, &info->w_commit); + return 0; + } + + /* + * TODO: is there a way of doing this all in-memory without writing + * anything to disk? + */ + remove_path(stash_index_path.buf); + + cp_read_tree.git_cmd = 1; + strvec_push(&cp_read_tree.args, "read-tree"); + strvec_push(&cp_read_tree.args, oid_to_hex(&info->w_commit)); + strvec_push(&cp_read_tree.args, oid_to_hex(&info->u_tree)); + strvec_pushf(&cp_read_tree.env_array, "GIT_INDEX_FILE=%s", + stash_index_path.buf); + + if (run_command(&cp_read_tree)) { + ret = -1; + goto done; + } + + if (write_index_as_tree(result, &istate, stash_index_path.buf, 0, + NULL)) { + ret = -1; + goto done; + } + +done: + discard_index(&istate); + remove_path(stash_index_path.buf); + return ret; +} + static int show_stash(int argc, const char **argv, const char *prefix) { int i; @@ -795,7 +836,21 @@ 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; + struct object_id *before = NULL; + struct object_id *after = NULL; + struct object_id untracked_merged_tree; + 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 +858,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 +904,23 @@ 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: + before = &info.b_commit; + after = &info.w_commit; + break; + case UNTRACKED_ONLY: + before = NULL; + after = &info.u_tree; + break; + case UNTRACKED_INCLUDE: + if (merge_track_untracked(&untracked_merged_tree, &info) < 0) + die(_("unable merge stash index with untracked files index")); + before = &info.b_commit; + after = &untracked_merged_tree; + break; + } + diff_tree_oid(before, after, "", &rev.diffopt); 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. Teach stash the --include-untracked option, which also displays the untracked files in a stash entry from the third parent (if it exists). Do this via something like GIT_INDEX_FILE=... git read-tree stash stash^3 and diffing the resulting tree object against the stash base. One improvement that this could use for the future is performing the action without writing anything to disk as one would expect this to be a read-only operation. This can be fixed in the future, however. Another 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> --- Documentation/git-stash.txt | 16 +++-- builtin/stash.c | 77 ++++++++++++++++++++++- contrib/completion/git-completion.bash | 2 +- t/t3905-stash-include-untracked.sh | 84 ++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 6 deletions(-)