Re: [PATCH 2/6] builtin/stash: fill in all commit data

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

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> get_stash_info doesn't ensure that all entries are filled in in all
> cases.  However, we'll want to use this information to write new
> commits, and when we do so, we'll need all the information to be
> present.  Fill in all the commit information whenever we call this
> function.
>
> Note that the behavior of info->has_u doesn't change here.  If we
> previously read a tree a refs/stash^3:, then refs/stash^3 must be a
> treeish.  We already here assume that the other parents specifically
> commits, so it should be safe to do so here as well.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/stash.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 2aa06cc91d..128f0a01ef 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -124,6 +124,7 @@ static void free_stash_info(struct stash_info *info)

Before this function ...

>  static void assert_stash_like(struct stash_info *info, const char *revision)
>  {
>  	if (get_oidf(&info->b_commit, "%s^1", revision) ||
> +	    get_oidf(&info->i_commit, "%s^2", revision) ||
>  	    get_oidf(&info->w_tree, "%s:", revision) ||
>  	    get_oidf(&info->b_tree, "%s^1:", revision) ||
>  	    get_oidf(&info->i_tree, "%s^2:", revision))

... there is this comment.

/*
 * w_commit is set to the commit containing the working tree
 * b_commit is set to the base commit
 * i_commit is set to the commit containing the index tree
 * u_commit is set to the commit containing the untracked files tree
 * w_tree is set to the working tree
 * b_tree is set to the base tree
 * i_tree is set to the index tree
 * u_tree is set to the untracked files tree
 */

We probably would want to comment that u_commit (hence u_tree) is
optional.  That is why assert_stash_like() does not say anything
about u_commit or u_tree, right?

It is curious why the function does not learn to check w_commit,
even though the proposed commit log message claims that this is
about filling all commit data.

> @@ -166,7 +167,8 @@ static int get_stash_info_1(struct stash_info *info, const char *commit, int qui
>  
>  	assert_stash_like(info, revision);
>  
> -	info->has_u = !get_oidf(&info->u_tree, "%s^3:", revision);
> +	info->has_u = !get_oidf(&info->u_commit, "%s^3", revision) &&
> +		      !get_oidf(&info->u_tree, "%s^3:", revision);
>  
>  	end_of_rev = strchrnul(revision, '@');
>  	strbuf_add(&symbolic, revision, end_of_rev - revision);