diff mbox series

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

Message ID 20220310173236.4165310-3-sandals@crustytoothpaste.net (mailing list archive)
State Superseded
Headers show
Series Importing and exporting stashes to refs | expand

Commit Message

brian m. carlson March 10, 2022, 5:32 p.m. UTC
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(-)

Comments

Junio C Hamano March 16, 2022, 4:50 p.m. UTC | #1
"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);
diff mbox series

Patch

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)
 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))
@@ -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);