Re: [PATCH v2 8/9] stash show: teach --include-untracked and --only-untracked

[Junio C Hamano <gitster@pobox.com>]

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.