[PATCH 0/6] Importing and exporting stashes to refs

[PATCH 0/6] Importing and exporting stashes to refs

[brian m. carlson]

Stashes are currently stored using the reflog in a given repository.
This is an interesting and novel way to handle them, but there is no way
to easily move a stash across machines.  For example, stashes cannot be
bundled, pushed, or fetched.

This is suboptimal for a lot of reasons.  First, there is a recent push
towards ephemeral development environments, but many users make heavy
use of their stashes and wish to persist them long term[0].  Additionally,
it would be convenient to share a snapshot of in-progress work with a
colleague or a collaborator on a project.  And finally, many users wish
to sync their in-progress state across machines, and we currently have
no good way to do so, so they often do dangerous things like using cloud
syncing services for their repositories.

Let's solve this problem by allowing users to import and export stashes
to a chain of commits.  The commits used in a stash export are nearly
identical to those used in the stashes, with one notable change: the
first parent of a stash is a pointer to the previous stash, or an empty
commit if there is no previous stash.  All of the other parents used in
the stash commit are present following it in their normal order.

This allows users to write their exported stashes to a single ref and
then push that ref to a remote or to bundle it for easy transport, and
then fetch it on the receiving side.  It also permits saving the index
and even untracked files and syncing them across machines, unlike
temporary commits.

We intentionally attempt to exactly round-trip commits between stashes,
although we don't do so for the exported data due to the base commit not
having identical timestamps.  Preserving the commits exactly lets us
more efficiently test our code and it also permits users to more easily
determine if they have the same data.

The tooling here is intentionally plumbing.  It's designed to be simple
and functional and get the basic job done.  If we want additional
features, we can add them in the future, but this should be a simple,
basic feature set that can support additional uses.

[0] For example, the present author has 124 stash entries in his
repository for this project.

brian m. carlson (6):
  builtin/stash: factor out generic function to look up stash info
  builtin/stash: fill in all commit data
  object-name: make get_oid quietly return an error
  builtin/stash: provide a way to export stashes to a ref
  builtin/stash: provide a way to import stashes from a ref
  doc: add stash export and import to docs

 Documentation/git-stash.txt |  27 +++
 builtin/stash.c             | 359 +++++++++++++++++++++++++++++++++---
 cache.h                     |  21 ++-
 object-name.c               |   6 +-
 t/t3903-stash.sh            |  52 ++++++
 5 files changed, 431 insertions(+), 34 deletions(-)

[PATCH 1/6] builtin/stash: factor out generic function to look up stash info

[brian m. carlson]

We have a function called get_stash_info that looks up this data based
on a set of command-line arguments and produces diagnostics to the user
on failure.  While this is helpful in the existing use cases, we'd like
to make use of this logic in a more programmatic way in the future.

Split out much of the function into a function which can be used
internally and which knows how to suppress these error messages with a
quiet parameter.  Wire up the rest of the function to call this internal
function to preserve the existing behavior.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c | 50 ++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 5897febfbe..2aa06cc91d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -130,38 +130,21 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
 		die(_("'%s' is not a stash-like commit"), revision);
 }
 
-static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+static int get_stash_info_1(struct stash_info *info, const char *commit, int quiet)
 {
 	int ret;
 	char *end_of_rev;
 	char *expanded_ref;
 	const char *revision;
-	const char *commit = NULL;
 	struct object_id dummy;
 	struct strbuf symbolic = STRBUF_INIT;
 
-	if (argc > 1) {
-		int i;
-		struct strbuf refs_msg = STRBUF_INIT;
-
-		for (i = 0; i < argc; i++)
-			strbuf_addf(&refs_msg, " '%s'", argv[i]);
-
-		fprintf_ln(stderr, _("Too many revisions specified:%s"),
-			   refs_msg.buf);
-		strbuf_release(&refs_msg);
-
-		return -1;
-	}
-
-	if (argc == 1)
-		commit = argv[0];
-
 	strbuf_init(&info->revision, 0);
 	if (!commit) {
 		if (!ref_exists(ref_stash)) {
 			free_stash_info(info);
-			fprintf_ln(stderr, _("No stash entries found."));
+			if (!quiet)
+				fprintf_ln(stderr, _("No stash entries found."));
 			return -1;
 		}
 
@@ -175,7 +158,8 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	revision = info->revision.buf;
 
 	if (get_oid(revision, &info->w_commit)) {
-		error(_("%s is not a valid reference"), revision);
+		if (!quiet)
+			error(_("%s is not a valid reference"), revision);
 		free_stash_info(info);
 		return -1;
 	}
@@ -204,6 +188,30 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	return !(ret == 0 || ret == 1);
 }
 
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+	const char *commit = NULL;
+
+	if (argc > 1) {
+		int i;
+		struct strbuf refs_msg = STRBUF_INIT;
+
+		for (i = 0; i < argc; i++)
+			strbuf_addf(&refs_msg, " '%s'", argv[i]);
+
+		fprintf_ln(stderr, _("Too many revisions specified:%s"),
+			   refs_msg.buf);
+		strbuf_release(&refs_msg);
+
+		return -1;
+	}
+
+	if (argc == 1)
+		commit = argv[0];
+
+	return get_stash_info_1(info, commit, 0);
+}
+
 static int do_clear_stash(void)
 {
 	struct object_id obj;

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

[brian m. carlson]

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

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

[Junio C Hamano]

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

[PATCH 3/6] object-name: make get_oid quietly return an error

[brian m. carlson]

A reasonable person looking at the signature and usage of get_oid and
friends might conclude that in the event of an error, it always returns
-1.  However, this is not the case.  Instead, get_oid_basic dies if we
go too far back into the history of a reflog (or, when quiet, simply
exits).

This is not especially useful, since in many cases, we might want to
handle this error differently.  Let's add a flag here to make it just
return -1 like elsewhere in these code paths.

Note that we cannot make this behavior the default, since we have many
other codepaths that rely on the existing behavior, including in tests.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h       | 21 +++++++++++----------
 object-name.c |  6 +++++-
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 825ec17198..416a9d9983 100644
--- a/cache.h
+++ b/cache.h
@@ -1366,16 +1366,17 @@ struct object_context {
 	char *path;
 };
 
-#define GET_OID_QUIETLY           01
-#define GET_OID_COMMIT            02
-#define GET_OID_COMMITTISH        04
-#define GET_OID_TREE             010
-#define GET_OID_TREEISH          020
-#define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
-#define GET_OID_ONLY_TO_DIE    04000
-#define GET_OID_REQUIRE_PATH  010000
+#define GET_OID_QUIETLY            01
+#define GET_OID_COMMIT             02
+#define GET_OID_COMMITTISH         04
+#define GET_OID_TREE              010
+#define GET_OID_TREEISH           020
+#define GET_OID_BLOB              040
+#define GET_OID_FOLLOW_SYMLINKS  0100
+#define GET_OID_RECORD_PATH      0200
+#define GET_OID_ONLY_TO_DIE     04000
+#define GET_OID_REQUIRE_PATH   010000
+#define GET_OID_RETURN_FAILURE 020000
 
 #define GET_OID_DISAMBIGUATORS \
 	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index 92862eeb1a..daa3ef77ef 100644
--- a/object-name.c
+++ b/object-name.c
@@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 						len, str,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
-			} else {
+			} else if (!(flags & GET_OID_RETURN_FAILURE)) {
 				if (flags & GET_OID_QUIETLY) {
 					exit(128);
 				}
 				die(_("log for '%.*s' only has %d entries"),
 				    len, str, co_cnt);
 			}
+			if (flags & GET_OID_RETURN_FAILURE) {
+				free(real_ref);
+				return -1;
+			}
 		}
 	}
 

Re: [PATCH 3/6] object-name: make get_oid quietly return an error

[Junio C Hamano]

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

> A reasonable person looking at the signature and usage of get_oid and
> friends might conclude that in the event of an error, it always returns
> -1.  However, this is not the case.  Instead, get_oid_basic dies if we
> go too far back into the history of a reflog (or, when quiet, simply
> exits).
>
> This is not especially useful, since in many cases, we might want to
> handle this error differently.  Let's add a flag here to make it just
> return -1 like elsewhere in these code paths.
>
> Note that we cannot make this behavior the default, since we have many
> other codepaths that rely on the existing behavior, including in tests.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  cache.h       | 21 +++++++++++----------
>  object-name.c |  6 +++++-
>  2 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 825ec17198..416a9d9983 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1366,16 +1366,17 @@ struct object_context {
>  	char *path;
>  };
>  
> -#define GET_OID_QUIETLY           01
> -#define GET_OID_COMMIT            02
> -#define GET_OID_COMMITTISH        04
> -#define GET_OID_TREE             010
> -#define GET_OID_TREEISH          020
> -#define GET_OID_BLOB             040
> -#define GET_OID_FOLLOW_SYMLINKS 0100
> -#define GET_OID_RECORD_PATH     0200
> -#define GET_OID_ONLY_TO_DIE    04000
> -#define GET_OID_REQUIRE_PATH  010000
> +#define GET_OID_QUIETLY            01
> +#define GET_OID_COMMIT             02
> +#define GET_OID_COMMITTISH         04
> +#define GET_OID_TREE              010
> +#define GET_OID_TREEISH           020
> +#define GET_OID_BLOB              040
> +#define GET_OID_FOLLOW_SYMLINKS  0100
> +#define GET_OID_RECORD_PATH      0200
> +#define GET_OID_ONLY_TO_DIE     04000
> +#define GET_OID_REQUIRE_PATH   010000
> +#define GET_OID_RETURN_FAILURE 020000

I do not think we want this change.  The next time somebody adds an
overly long symbol, we reformat all the lines, making it hard to
spot that the change is only adding a single new symbol?

I think we'd rather go the other way not to tempt people into
right-aligning these constants, either by rewriting them into

	#define GET_OID_QUIETLY<TAB>(1U << 1)
	#define GET_OID_COMMIT<TAB>(1U << 2)
	#define GET_OID_COMMITTISH<TAB>(1U << 3)
	...
	
in a separate preliminary patch without adding a new symbol, or
adding the new symbol unaligned and without touching existing lines.

> diff --git a/object-name.c b/object-name.c
> index 92862eeb1a..daa3ef77ef 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
>  						len, str,
>  						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
>  				}
> -			} else {
> +			} else if (!(flags & GET_OID_RETURN_FAILURE)) {
>  				if (flags & GET_OID_QUIETLY) {
>  					exit(128);
>  				}
>  				die(_("log for '%.*s' only has %d entries"),
>  				    len, str, co_cnt);
>  			}
> +			if (flags & GET_OID_RETURN_FAILURE) {
> +				free(real_ref);
> +				return -1;
> +			}
>  		}

So, without the new bit, we used to die loudly or quietly.  The new
bit allows us to return an error to the caller without dying
ourselves.

You can call the bit _RETURN_ERROR and not to worry about the
right-alignment above ;-), but better yet, how about calling it
_GENTLY, which is how we call such a variant of behaviour?

Re: [PATCH 3/6] object-name: make get_oid quietly return an error

[Drew Stolee]

Hey guys - I’d been ignoring this thread as spam but just opened it and see you meant to include Derrick Stolee. I’m Drew Stolee, likely related but who knows how. dstolee@gmail.com is not the developer you’re looking for. 

Best,
Drew

Sent from my iPhone

> On Mar 16, 2022, at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
>> A reasonable person looking at the signature and usage of get_oid and
>> friends might conclude that in the event of an error, it always returns
>> -1.  However, this is not the case.  Instead, get_oid_basic dies if we
>> go too far back into the history of a reflog (or, when quiet, simply
>> exits).
>> 
>> This is not especially useful, since in many cases, we might want to
>> handle this error differently.  Let's add a flag here to make it just
>> return -1 like elsewhere in these code paths.
>> 
>> Note that we cannot make this behavior the default, since we have many
>> other codepaths that rely on the existing behavior, including in tests.
>> 
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>> cache.h       | 21 +++++++++++----------
>> object-name.c |  6 +++++-
>> 2 files changed, 16 insertions(+), 11 deletions(-)
>> 
>> diff --git a/cache.h b/cache.h
>> index 825ec17198..416a9d9983 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1366,16 +1366,17 @@ struct object_context {
>>    char *path;
>> };
>> 
>> -#define GET_OID_QUIETLY           01
>> -#define GET_OID_COMMIT            02
>> -#define GET_OID_COMMITTISH        04
>> -#define GET_OID_TREE             010
>> -#define GET_OID_TREEISH          020
>> -#define GET_OID_BLOB             040
>> -#define GET_OID_FOLLOW_SYMLINKS 0100
>> -#define GET_OID_RECORD_PATH     0200
>> -#define GET_OID_ONLY_TO_DIE    04000
>> -#define GET_OID_REQUIRE_PATH  010000
>> +#define GET_OID_QUIETLY            01
>> +#define GET_OID_COMMIT             02
>> +#define GET_OID_COMMITTISH         04
>> +#define GET_OID_TREE              010
>> +#define GET_OID_TREEISH           020
>> +#define GET_OID_BLOB              040
>> +#define GET_OID_FOLLOW_SYMLINKS  0100
>> +#define GET_OID_RECORD_PATH      0200
>> +#define GET_OID_ONLY_TO_DIE     04000
>> +#define GET_OID_REQUIRE_PATH   010000
>> +#define GET_OID_RETURN_FAILURE 020000
> 
> I do not think we want this change.  The next time somebody adds an
> overly long symbol, we reformat all the lines, making it hard to
> spot that the change is only adding a single new symbol?
> 
> I think we'd rather go the other way not to tempt people into
> right-aligning these constants, either by rewriting them into
> 
>    #define GET_OID_QUIETLY<TAB>(1U << 1)
>    #define GET_OID_COMMIT<TAB>(1U << 2)
>    #define GET_OID_COMMITTISH<TAB>(1U << 3)
>    ...
>    
> in a separate preliminary patch without adding a new symbol, or
> adding the new symbol unaligned and without touching existing lines.
> 
>> diff --git a/object-name.c b/object-name.c
>> index 92862eeb1a..daa3ef77ef 100644
>> --- a/object-name.c
>> +++ b/object-name.c
>> @@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
>>                        len, str,
>>                        show_date(co_time, co_tz, DATE_MODE(RFC2822)));
>>                }
>> -            } else {
>> +            } else if (!(flags & GET_OID_RETURN_FAILURE)) {
>>                if (flags & GET_OID_QUIETLY) {
>>                    exit(128);
>>                }
>>                die(_("log for '%.*s' only has %d entries"),
>>                    len, str, co_cnt);
>>            }
>> +            if (flags & GET_OID_RETURN_FAILURE) {
>> +                free(real_ref);
>> +                return -1;
>> +            }
>>        }
> 
> So, without the new bit, we used to die loudly or quietly.  The new
> bit allows us to return an error to the caller without dying
> ourselves.
> 
> You can call the bit _RETURN_ERROR and not to worry about the
> right-alignment above ;-), but better yet, how about calling it
> _GENTLY, which is how we call such a variant of behaviour?
> 

Re: [PATCH 3/6] object-name: make get_oid quietly return an error

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 2022-03-16 at 16:56:22, Junio C Hamano wrote:
> > diff --git a/object-name.c b/object-name.c
> > index 92862eeb1a..daa3ef77ef 100644
> > --- a/object-name.c
> > +++ b/object-name.c
> > @@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
> >  						len, str,
> >  						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
> >  				}
> > -			} else {
> > +			} else if (!(flags & GET_OID_RETURN_FAILURE)) {
> >  				if (flags & GET_OID_QUIETLY) {
> >  					exit(128);
> >  				}
> >  				die(_("log for '%.*s' only has %d entries"),
> >  				    len, str, co_cnt);
> >  			}
> > +			if (flags & GET_OID_RETURN_FAILURE) {
> > +				free(real_ref);
> > +				return -1;
> > +			}
> >  		}
> 
> So, without the new bit, we used to die loudly or quietly.  The new
> bit allows us to return an error to the caller without dying
> ourselves.
> 
> You can call the bit _RETURN_ERROR and not to worry about the
> right-alignment above ;-), but better yet, how about calling it
> _GENTLY, which is how we call such a variant of behaviour?

Sure, that works.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

A common user problem is how to sync in-progress work to another
machine.  Users currently must use some sort of transfer of the working
tree, which poses security risks and also necessarily causes the index
to become dirty.  The experience is suboptimal and frustrating for
users.

A reasonable idea is to use the stash for this purpose, but the stash is
stored in the reflog, not in a ref, and as such it cannot be pushed or
pulled.  This also means that it cannot be saved into a bundle or
preserved elsewhere, which is a problem when using throwaway development
environments.

Let's solve this problem by allowing the user to export the stash to a
ref (or, to just write it into the repository and print the hash, à la
git commit-tree).  Introduce git stash export, which writes a chain of
commits identical to the ones used for reflog-based stashes, except that
the first parent is always a chain to the previous stash, or to a
single, empty commit (for the final item).

Iterate over each stash from topmost to bottomost, looking up the data
for each one, and then create the chain from the single empty commit
back up in reverse order.  Preserve the author and committer
information, as well as the commit message, so we produce an identical
stash when importing later.

If the user has specified specific stashes they'd like to export
instead, use those instead of iterating over all of the stashes.

As part of this, specifically request quiet behavior when looking up the
OID for a revision because we will eventually hit a revision that
doesn't exist and we don't want to die when that occurs.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 179 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 128f0a01ef..582a04dbab 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -33,6 +33,7 @@ static const char * const git_stash_usage[] = {
 	   "          [--] [<pathspec>...]]"),
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	N_("git stash export (--print | --to-ref <ref>) [<stashes>]"),
 	NULL
 };
 
@@ -89,6 +90,12 @@ static const char * const git_stash_save_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_export_usage[] = {
+	N_("git stash export (--print | --to-ref <ref>) [<stashes>]"),
+	NULL
+};
+
+
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -139,6 +146,7 @@ static int get_stash_info_1(struct stash_info *info, const char *commit, int qui
 	const char *revision;
 	struct object_id dummy;
 	struct strbuf symbolic = STRBUF_INIT;
+	struct object_context unused;
 
 	strbuf_init(&info->revision, 0);
 	if (!commit) {
@@ -158,7 +166,9 @@ static int get_stash_info_1(struct stash_info *info, const char *commit, int qui
 
 	revision = info->revision.buf;
 
-	if (get_oid(revision, &info->w_commit)) {
+	if (get_oid_with_context(the_repository, revision,
+				 GET_OID_QUIETLY | GET_OID_RETURN_FAILURE,
+				 &info->w_commit, &unused)) {
 		if (!quiet)
 			error(_("%s is not a valid reference"), revision);
 		free_stash_info(info);
@@ -1775,6 +1785,172 @@ static int save_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static char *write_commit_with_parents(struct object_id *out, const struct stash_info *info, struct commit_list *parents)
+{
+	size_t author_len, committer_len;
+	struct commit *this = NULL;
+	const char *orig_author = NULL, *orig_committer = NULL;
+	char *author = NULL, *committer = NULL;
+	const char *buffer = NULL;
+	unsigned long bufsize;
+	const char *p;
+	char *msg = NULL;
+
+	this = lookup_commit_reference(the_repository, &info->w_commit);
+	buffer = get_commit_buffer(this, &bufsize);
+	orig_author = find_commit_header(buffer, "author", &author_len);
+	orig_committer = find_commit_header(buffer, "committer", &committer_len);
+	p = memmem(buffer, bufsize, "\n\n", 2);
+
+	if (!orig_author || !orig_committer || !p) {
+		error(_("cannot parse commit %s"), oid_to_hex(&info->w_commit));
+		goto out;
+	}
+	/* Jump to message. */
+	p += 2;
+
+	author = xmemdupz(orig_author, author_len);
+	committer = xmemdupz(orig_committer, committer_len);
+
+	if (commit_tree_extended(p, bufsize - (p - buffer),
+				 &info->w_tree, parents,
+				 out, author, committer,
+				 NULL, NULL)) {
+		error(_("could not write commit"));
+		goto out;
+	}
+	msg = xmemdupz(p, bufsize - (p - buffer));
+out:
+	unuse_commit_buffer(this, buffer);
+	free(author);
+	free(committer);
+	return msg;
+}
+
+static int do_export_stash(const char *ref, size_t argc, const char **argv)
+{
+	struct object_id base;
+	struct commit *prev;
+	struct stash_info *items = NULL;
+	size_t nitems = 0, nalloc = 0;
+	int res = 0;
+
+	prepare_fallback_ident("git stash", "git@stash");
+
+	/* First, we create a single empty commit. */
+	if (commit_tree(NULL, 0, the_hash_algo->empty_tree, NULL, &base, NULL, NULL))
+		return error(_("unable to write base commit"));
+
+	prev = lookup_commit_reference(the_repository, &base);
+
+
+	if (argc) {
+		/*
+		 * Find each specified stash, and load data into the array.
+		 */
+		for (size_t i = 0; i < argc; i++, nitems++) {
+			int ret;
+
+			if (nalloc <= i) {
+				size_t new = nalloc * 3 / 2 + 5;
+				items = xrealloc(items, new * sizeof(*items));
+				nalloc = new;
+			}
+			memset(&items[i], 0, sizeof(*items));
+			/* We want this to be quiet because it might not exist. */
+			ret = get_stash_info_1(&items[i], argv[i], 1);
+			if (ret)
+				return error(_("unable to find stash entry %s"), argv[i]);
+		}
+	} else {
+		/*
+		 * Walk the reflog, finding each stash entry, and load data into the
+		 * array.
+		 */
+		for (size_t i = 0;; i++, nitems++) {
+			char buf[32];
+			int ret;
+
+			if (nalloc <= i) {
+				size_t new = nalloc * 3 / 2 + 5;
+				items = xrealloc(items, new * sizeof(*items));
+				nalloc = new;
+			}
+			snprintf(buf, sizeof(buf), "%zu", i);
+			memset(&items[i], 0, sizeof(*items));
+			/* We want this to be quiet because it might not exist. */
+			ret = get_stash_info_1(&items[i], buf, 1);
+			if (ret)
+				break;
+		}
+	}
+
+	/*
+	 * Now, create a set of commits identical to the regular stash commits,
+	 * but where their first parents form a chain to our original empty
+	 * base commit.
+	 */
+	for (ssize_t i = nitems - 1; i >= 0; i--) {
+		struct commit_list *parents = NULL;
+		struct commit_list **next = &parents;
+		struct object_id out;
+		char *msg;
+
+		next = commit_list_append(prev, next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].b_commit), next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].i_commit), next);
+		if (items[i].has_u)
+			next = commit_list_append(lookup_commit_reference(the_repository,
+									  &items[i].u_commit),
+						  next);
+
+		msg = write_commit_with_parents(&out, &items[i], parents);
+		if (!msg) {
+			res = -1;
+			goto out;
+		}
+		free(msg);
+		prev = lookup_commit_reference(the_repository, &out);
+	}
+	if (ref) {
+		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+	} else {
+		puts(oid_to_hex(&prev->object.oid));
+	}
+out:
+	for (size_t i = 0; i < nitems; i++) {
+		free_stash_info(&items[i]);
+	}
+	free(items);
+
+	return res;
+}
+
+static int export_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	int print = 0;
+	const char *ref = NULL;
+	struct option options[] = {
+		OPT_BOOL(0, "print", &print,
+			 N_("print the object ID instead of writing it to a ref")),
+		OPT_STRING(0, "to-ref", &ref, "refname",
+			   N_("save the data to the given ref")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_export_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (!(!!ref ^ print))
+		return error("exactly one of --print or --to-ref is required");
+
+
+	ret = do_export_stash(ref, argc, argv);
+	return ret;
+}
+
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
 	pid_t pid = getpid();
@@ -1818,6 +1994,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!push_stash(argc, argv, prefix, 0);
 	else if (!strcmp(argv[0], "save"))
 		return !!save_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "export"))
+		return !!export_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 10 2022, brian m. carlson wrote:

> +	size_t author_len, committer_len;
> +	struct commit *this = NULL;
> +	const char *orig_author = NULL, *orig_committer = NULL;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer = NULL;
> +	unsigned long bufsize;
> +	const char *p;
> +	char *msg = NULL;

These shouldn't be initialized unless they really need to..

> +	this = lookup_commit_reference(the_repository, &info->w_commit);

..and some are clobbered right away here, so all of these should not be initializzed.

> +	buffer = get_commit_buffer(this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> +	p = memmem(buffer, bufsize, "\n\n", 2);

...since by doing so we hide genuine "uninitialized"
warnings. E.g. "author_len" here isn't initialized, but is set by
find_commit_header(), but if that line was removed we'd warn below, but
not if it's initialized when the variables are declared..

> +		for (size_t i = 0;; i++, nitems++) {
> +			char buf[32];
> +			int ret;
> +
> +			if (nalloc <= i) {
> +				size_t new = nalloc * 3 / 2 + 5;
> +				items = xrealloc(items, new * sizeof(*items));
> +				nalloc = new;

Can't we just use the usual ALLOC_GROW() pattern here?

> +			}
> +			snprintf(buf, sizeof(buf), "%zu", i);

Aren't the %z formats unportable (even with our newly found reliance on
more C99)? I vaguely recall trying them recently and the windows CI jobs
erroring...

> +	for (ssize_t i = nitems - 1; i >= 0; i--) {

The ssize_t type can be really small (it's not a signed size_t), so this
is unportable, but in practice maybe it's OK...

In this case if you're just wanting to count down in a list maybe you
can use the pattern I used in 99d60545f87 (string-list API: change "nr"
and "alloc" to "size_t", 2022-03-07)?

> +	if (ref) {
> +		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
> +	} else {
> +		puts(oid_to_hex(&prev->object.oid));
> +	}

Nit: braces can be gone.

> +out:
> +	for (size_t i = 0; i < nitems; i++) {
> +		free_stash_info(&items[i]);
> +	}

Ditto.

> +static int export_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int ret = 0;
> +	int print = 0;
> +	const char *ref = NULL;
> +	struct option options[] = {
> +		OPT_BOOL(0, "print", &print,
> +			 N_("print the object ID instead of writing it to a ref")),
> +		OPT_STRING(0, "to-ref", &ref, "refname",

Needs _("refname")

> +			   N_("save the data to the given ref")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_export_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (!(!!ref ^ print))
> +		return error("exactly one of --print or --to-ref is required");

Cute, but maybe we can just use OPT_CMDMODE(), and if it's "to-ref"
shift one off argv afterwards (which'll be your &ref).

I.e. it'll give you the option incompatibility check, and without a new
translaed string.

Which, if we're keeping this version should use _().

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Phillip Wood]

Hi Brian and Ævar

Firstly I think this is a useful feature to add to git stash, thanks for 
working on it Brian

On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Mar 10 2022, brian m. carlson wrote:
> 
>> +	size_t author_len, committer_len;
>> +	struct commit *this = NULL;
>> +	const char *orig_author = NULL, *orig_committer = NULL;
>> +	char *author = NULL, *committer = NULL;
>> +	const char *buffer = NULL;
>> +	unsigned long bufsize;
>> +	const char *p;
>> +	char *msg = NULL;
> 
> These shouldn't be initialized unless they really need to..
> 
>> +	this = lookup_commit_reference(the_repository, &info->w_commit);
> 
> ..and some are clobbered right away here, so all of these should not be initializzed.
> 
>> +	buffer = get_commit_buffer(this, &bufsize);
>> +	orig_author = find_commit_header(buffer, "author", &author_len);
>> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
>> +	p = memmem(buffer, bufsize, "\n\n", 2);

You could start searching from orig_committer rather than buffer but I'm 
sure it doesn't make any real difference. The sequencer does something 
similar to this to replay commits when rebasing - is there any scope for 
sharing code between the two?

> ...since by doing so we hide genuine "uninitialized"
> warnings. E.g. "author_len" here isn't initialized, but is set by
> find_commit_header(), but if that line was removed we'd warn below, but
> not if it's initialized when the variables are declared..
> 
>> +		for (size_t i = 0;; i++, nitems++) {

Do we need i and nitems?

>> +			char buf[32];
>> +			int ret;
>> +
>> +			if (nalloc <= i) {
>> +				size_t new = nalloc * 3 / 2 + 5;
>> +				items = xrealloc(items, new * sizeof(*items));
>> +				nalloc = new;
> 
> Can't we just use the usual ALLOC_GROW() pattern here?
ALLOC_GROW_BY() zeros out the memory which would mean we could remove 
the memset() calls in the loops. I noticed in some other loops we know 
the size in advance and could use CALLOC_ARRAY().

>> +			}
>> +			snprintf(buf, sizeof(buf), "%zu", i);
> 
> Aren't the %z formats unportable (even with our newly found reliance on
> more C99)? I vaguely recall trying them recently and the windows CI jobs
> erroring...

According to [1] it has been available since at least 2015. It is 
certainly much nicer than casting every size_t to uintmax_t and having 
to use PRIuMAX.

>> +	for (ssize_t i = nitems - 1; i >= 0; i--) {
> 
> The ssize_t type can be really small (it's not a signed size_t), so this
> is unportable, but in practice maybe it's OK...

I'm not really convinced by this ssize_t can be small argument[2], do 
you know of any platforms where it is true?

Best Wishes

Phillip

[1] 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-140#size

[2] 
https://lore.kernel.org/git/e881a455-88b5-9c87-03a8-caaee68bb344@gmail.com/

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Phillip Wood]

On 14/03/2022 21:19, Phillip Wood wrote:
> Hi Brian and Ævar

Sorry brian, I forgot you prefer a lowercase "b"

> Firstly I think this is a useful feature to add to git stash, thanks for 
> working on it Brian
> 
> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Mar 10 2022, brian m. carlson wrote:
>>
>>> +    size_t author_len, committer_len;
>>> +    struct commit *this = NULL;
>>> +    const char *orig_author = NULL, *orig_committer = NULL;
>>> +    char *author = NULL, *committer = NULL;
>>> +    const char *buffer = NULL;
>>> +    unsigned long bufsize;
>>> +    const char *p;
>>> +    char *msg = NULL;
>>
>> These shouldn't be initialized unless they really need to..
>>
>>> +    this = lookup_commit_reference(the_repository, &info->w_commit);
>>
>> ..and some are clobbered right away here, so all of these should not 
>> be initializzed.
>>
>>> +    buffer = get_commit_buffer(this, &bufsize);
>>> +    orig_author = find_commit_header(buffer, "author", &author_len);
>>> +    orig_committer = find_commit_header(buffer, "committer", 
>>> &committer_len);
>>> +    p = memmem(buffer, bufsize, "\n\n", 2);
> 
> You could start searching from orig_committer rather than buffer but I'm 
> sure it doesn't make any real difference. The sequencer does something 
> similar to this to replay commits when rebasing - is there any scope for 
> sharing code between the two?

I had a look at sequencer.c this morning and I think the answer is "not 
easily". In principle it would be nice to have a split_commit_header() 
function that filled a struct of pointer, length pairs for the author, 
committer, message and optionally the subject that could be used here, 
in the sequencer and in fast-export but I think that is more work than 
is reasonable for this series.

Best Wishes

Phillip

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]

On 2022-03-14 at 21:19:10, Phillip Wood wrote:
> Hi Brian and Ævar
> 
> Firstly I think this is a useful feature to add to git stash, thanks for
> working on it Brian

Thanks.  I'm glad folks other than me will find it useful.

> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Thu, Mar 10 2022, brian m. carlson wrote:
> > 
> > > +	size_t author_len, committer_len;
> > > +	struct commit *this = NULL;
> > > +	const char *orig_author = NULL, *orig_committer = NULL;
> > > +	char *author = NULL, *committer = NULL;
> > > +	const char *buffer = NULL;
> > > +	unsigned long bufsize;
> > > +	const char *p;
> > > +	char *msg = NULL;
> > 
> > These shouldn't be initialized unless they really need to..
> > 
> > > +	this = lookup_commit_reference(the_repository, &info->w_commit);
> > 
> > ..and some are clobbered right away here, so all of these should not be initializzed.

This function got hoisted out of what would otherwise be duplicated
code, and that's why they're all initialized (because we would otherwise
have called free on an uninitialized value).  I can remove the ones that
aren't strictly needed.

> > > +	buffer = get_commit_buffer(this, &bufsize);
> > > +	orig_author = find_commit_header(buffer, "author", &author_len);
> > > +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> > > +	p = memmem(buffer, bufsize, "\n\n", 2);
> 
> You could start searching from orig_committer rather than buffer but I'm
> sure it doesn't make any real difference. The sequencer does something
> similar to this to replay commits when rebasing - is there any scope for
> sharing code between the two?

I can look into it.  The amount of code that would be duplicated here is
very minimal, so I'm okay with just adding a few lines here.

> > ...since by doing so we hide genuine "uninitialized"
> > warnings. E.g. "author_len" here isn't initialized, but is set by
> > find_commit_header(), but if that line was removed we'd warn below, but
> > not if it's initialized when the variables are declared..
> > 
> > > +		for (size_t i = 0;; i++, nitems++) {
> 
> Do we need i and nitems?

I can look into removing them.

> > > +			char buf[32];
> > > +			int ret;
> > > +
> > > +			if (nalloc <= i) {
> > > +				size_t new = nalloc * 3 / 2 + 5;
> > > +				items = xrealloc(items, new * sizeof(*items));
> > > +				nalloc = new;
> > 
> > Can't we just use the usual ALLOC_GROW() pattern here?
> ALLOC_GROW_BY() zeros out the memory which would mean we could remove the
> memset() calls in the loops. I noticed in some other loops we know the size
> in advance and could use CALLOC_ARRAY().

Yeah, I can switch to that.  I was looking for that, but I was thinking
of a function and not a macro, so I missed it.

> > > +			}
> > > +			snprintf(buf, sizeof(buf), "%zu", i);
> > 
> > Aren't the %z formats unportable (even with our newly found reliance on
> > more C99)? I vaguely recall trying them recently and the windows CI jobs
> > erroring...
> 
> According to [1] it has been available since at least 2015. It is certainly
> much nicer than casting every size_t to uintmax_t and having to use PRIuMAX.

If we're relying on a new enough MSVC for C11, then it's much newer than
2015, so we should be fine.  It's mandatory on POSIX systems.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

C99 %zu support (on MSVC) (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref)

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 16 2022, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> On 2022-03-14 at 21:19:10, Phillip Wood wrote:
>> Hi Brian and Ævar
>> 
>> Firstly I think this is a useful feature to add to git stash, thanks for
>> working on it Brian
>
> Thanks.  I'm glad folks other than me will find it useful.
>
>> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
>> > 
>> > On Thu, Mar 10 2022, brian m. carlson wrote:
>> > 
>> > > +	size_t author_len, committer_len;
>> > > +	struct commit *this = NULL;
>> > > +	const char *orig_author = NULL, *orig_committer = NULL;
>> > > +	char *author = NULL, *committer = NULL;
>> > > +	const char *buffer = NULL;
>> > > +	unsigned long bufsize;
>> > > +	const char *p;
>> > > +	char *msg = NULL;
>> > 
>> > These shouldn't be initialized unless they really need to..
>> > 
>> > > +	this = lookup_commit_reference(the_repository, &info->w_commit);
>> > 
>> > ..and some are clobbered right away here, so all of these should not be initializzed.
>
> This function got hoisted out of what would otherwise be duplicated
> code, and that's why they're all initialized (because we would otherwise
> have called free on an uninitialized value).  I can remove the ones that
> aren't strictly needed.
>
>> > > +	buffer = get_commit_buffer(this, &bufsize);
>> > > +	orig_author = find_commit_header(buffer, "author", &author_len);
>> > > +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
>> > > +	p = memmem(buffer, bufsize, "\n\n", 2);
>> 
>> You could start searching from orig_committer rather than buffer but I'm
>> sure it doesn't make any real difference. The sequencer does something
>> similar to this to replay commits when rebasing - is there any scope for
>> sharing code between the two?
>
> I can look into it.  The amount of code that would be duplicated here is
> very minimal, so I'm okay with just adding a few lines here.
>
>> > ...since by doing so we hide genuine "uninitialized"
>> > warnings. E.g. "author_len" here isn't initialized, but is set by
>> > find_commit_header(), but if that line was removed we'd warn below, but
>> > not if it's initialized when the variables are declared..
>> > 
>> > > +		for (size_t i = 0;; i++, nitems++) {
>> 
>> Do we need i and nitems?
>
> I can look into removing them.
>
>> > > +			char buf[32];
>> > > +			int ret;
>> > > +
>> > > +			if (nalloc <= i) {
>> > > +				size_t new = nalloc * 3 / 2 + 5;
>> > > +				items = xrealloc(items, new * sizeof(*items));
>> > > +				nalloc = new;
>> > 
>> > Can't we just use the usual ALLOC_GROW() pattern here?
>> ALLOC_GROW_BY() zeros out the memory which would mean we could remove the
>> memset() calls in the loops. I noticed in some other loops we know the size
>> in advance and could use CALLOC_ARRAY().
>
> Yeah, I can switch to that.  I was looking for that, but I was thinking
> of a function and not a macro, so I missed it.
>
>> > > +			}
>> > > +			snprintf(buf, sizeof(buf), "%zu", i);
>> > 
>> > Aren't the %z formats unportable (even with our newly found reliance on
>> > more C99)? I vaguely recall trying them recently and the windows CI jobs
>> > erroring...
>> 
>> According to [1] it has been available since at least 2015. It is certainly
>> much nicer than casting every size_t to uintmax_t and having to use PRIuMAX.
>
> If we're relying on a new enough MSVC for C11, then it's much newer than
> 2015, so we should be fine.  It's mandatory on POSIX systems.

FWIW I dug into my logs and I ran into it with %zu (not %z), but that's
what you're using.

Sorry about being inaccurate, it seems %z's portability isn't the same
as %z.

I ran into it in mid-2021 in the GitHub CI, but those logs are deleted
now (and I didn't re-push that specific OID):

    https://github.com/avar/git/runs/2298653913

Where it would emit output like:

    builtin/log.c: In function 'gen_message_id':
    311
    builtin/log.c:1047:29: error: unknown conversion type character 'z' in format [-Werror=format=]
    312
     1047 |  strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);

This SO post, whose accuracy I can't verify, claims it is supported in
VS 2013 or later, and that the way to check for it is with "_MSC_VER >=
1800":

    https://stackoverflow.com/questions/44382862/how-to-printf-a-size-t-without-warning-in-mingw-w64-gcc-7-1

So if we are going to use it and that's true (which would be great!) it
would IMO make sense to introduce it in some prep commit where we delete
e.g. "_MSC_VER>=1310" and other things you'll see in-tree if you look
through:

    git grep _MSC_VER

I.e. to push out some canary commit for using that specific feature, and
along with it delete the old MSVC compatibility shims (which presumably
we can't use if we're going to hard depend on %zu).

Re: C99 %zu support (on MSVC) (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref)

[Phillip Wood]

On 18/03/2022 13:34, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Mar 16 2022, brian m. carlson wrote:
> 
>> [[PGP Signed Part:Undecided]]
>> On 2022-03-14 at 21:19:10, Phillip Wood wrote:
>>> Hi Brian and Ævar
>>>
>>> Firstly I think this is a useful feature to add to git stash, thanks for
>>> working on it Brian
>>
>> Thanks.  I'm glad folks other than me will find it useful.
>>
>>> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> On Thu, Mar 10 2022, brian m. carlson wrote:
>>>>
>>>>> +	size_t author_len, committer_len;
>>>>> +	struct commit *this = NULL;
>>>>> +	const char *orig_author = NULL, *orig_committer = NULL;
>>>>> +	char *author = NULL, *committer = NULL;
>>>>> +	const char *buffer = NULL;
>>>>> +	unsigned long bufsize;
>>>>> +	const char *p;
>>>>> +	char *msg = NULL;
>>>>
>>>> These shouldn't be initialized unless they really need to..
>>>>
>>>>> +	this = lookup_commit_reference(the_repository, &info->w_commit);
>>>>
>>>> ..and some are clobbered right away here, so all of these should not be initializzed.
>>
>> This function got hoisted out of what would otherwise be duplicated
>> code, and that's why they're all initialized (because we would otherwise
>> have called free on an uninitialized value).  I can remove the ones that
>> aren't strictly needed.
>>
>>>>> +	buffer = get_commit_buffer(this, &bufsize);
>>>>> +	orig_author = find_commit_header(buffer, "author", &author_len);
>>>>> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
>>>>> +	p = memmem(buffer, bufsize, "\n\n", 2);
>>>
>>> You could start searching from orig_committer rather than buffer but I'm
>>> sure it doesn't make any real difference. The sequencer does something
>>> similar to this to replay commits when rebasing - is there any scope for
>>> sharing code between the two?
>>
>> I can look into it.  The amount of code that would be duplicated here is
>> very minimal, so I'm okay with just adding a few lines here.
>>
>>>> ...since by doing so we hide genuine "uninitialized"
>>>> warnings. E.g. "author_len" here isn't initialized, but is set by
>>>> find_commit_header(), but if that line was removed we'd warn below, but
>>>> not if it's initialized when the variables are declared..
>>>>
>>>>> +		for (size_t i = 0;; i++, nitems++) {
>>>
>>> Do we need i and nitems?
>>
>> I can look into removing them.
>>
>>>>> +			char buf[32];
>>>>> +			int ret;
>>>>> +
>>>>> +			if (nalloc <= i) {
>>>>> +				size_t new = nalloc * 3 / 2 + 5;
>>>>> +				items = xrealloc(items, new * sizeof(*items));
>>>>> +				nalloc = new;
>>>>
>>>> Can't we just use the usual ALLOC_GROW() pattern here?
>>> ALLOC_GROW_BY() zeros out the memory which would mean we could remove the
>>> memset() calls in the loops. I noticed in some other loops we know the size
>>> in advance and could use CALLOC_ARRAY().
>>
>> Yeah, I can switch to that.  I was looking for that, but I was thinking
>> of a function and not a macro, so I missed it.
>>
>>>>> +			}
>>>>> +			snprintf(buf, sizeof(buf), "%zu", i);
>>>>
>>>> Aren't the %z formats unportable (even with our newly found reliance on
>>>> more C99)? I vaguely recall trying them recently and the windows CI jobs
>>>> erroring...
>>>
>>> According to [1] it has been available since at least 2015. It is certainly
>>> much nicer than casting every size_t to uintmax_t and having to use PRIuMAX.
>>
>> If we're relying on a new enough MSVC for C11, then it's much newer than
>> 2015, so we should be fine.  It's mandatory on POSIX systems.
> 
> FWIW I dug into my logs and I ran into it with %zu (not %z), but that's
> what you're using.
> 
> Sorry about being inaccurate, it seems %z's portability isn't the same
> as %z.
> 
> I ran into it in mid-2021 in the GitHub CI, but those logs are deleted
> now (and I didn't re-push that specific OID):
> 
>      https://github.com/avar/git/runs/2298653913
> 
> Where it would emit output like:
> 
>      builtin/log.c: In function 'gen_message_id':
>      311
>      builtin/log.c:1047:29: error: unknown conversion type character 'z' in format [-Werror=format=]
>      312
>       1047 |  strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);
> 
> This SO post, whose accuracy I can't verify, claims it is supported in
> VS 2013 or later, and that the way to check for it is with "_MSC_VER >=
> 1800":
> 
>      https://stackoverflow.com/questions/44382862/how-to-printf-a-size-t-without-warning-in-mingw-w64-gcc-7-1

Oh, so it is mingw that is the problem, not MSVC. Indeed using "%zu" 
(see the diff below) fails for the "win build" job[1] but the "win+VS 
build" succeeds[2]. mingw config.mak.uname to uses 
-D__MINGW_USE_ANSI_STDIO=0. Even if we could change that to =1 it would 
be insufficient as it does not affect the format specifiers allowed by 
__attribute__((format (printf, 3, 4))). I think to do that we would have 
to change "printf" to "__MINGW_PRINTF_FORMAT" for each attribute 
declaration[3].

In short unfortunately I don't think we can easily use "%zu"

Best wishes

Phillip

diff --git a/add-interactive.c b/add-interactive.c
index e1ab39cce3..1790ad6359 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -193,9 +193,8 @@ static ssize_t find_unique(const char *string, 
struct prefix_item_list *list)
         struct string_list_item *item;

         if (list->items.nr != list->sorted.nr)
-               BUG("prefix_item_list in inconsistent state (%"PRIuMAX
-                   " vs %"PRIuMAX")",
-                   (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr);
+               BUG("prefix_item_list in inconsistent state (%zu vs %zu)",
+                   list->items.nr, list->sorted.nr);

         if (index < 0)
                 item = list->sorted.items[-1 - index].util;

[1] 
https://github.com/phillipwood/git/runs/5601840748?check_suite_focus=true
[2] 
https://github.com/phillipwood/git/runs/5601840528?check_suite_focus=true
[3] https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf

> So if we are going to use it and that's true (which would be great!) it
> would IMO make sense to introduce it in some prep commit where we delete
> e.g. "_MSC_VER>=1310" and other things you'll see in-tree if you look
> through:
> 
>      git grep _MSC_VER
> 
> I.e. to push out some canary commit for using that specific feature, and
> along with it delete the old MSVC compatibility shims (which presumably
> we can't use if we're going to hard depend on %zu).

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

Hi brian,

On Wed, 16 Mar 2022, brian m. carlson wrote:

> On 2022-03-14 at 21:19:10, Phillip Wood wrote:
>
> > On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
> > >
> > > On Thu, Mar 10 2022, brian m. carlson wrote:
> > >
> > > > +			}
> > > > +			snprintf(buf, sizeof(buf), "%zu", i);
> > >
> > > Aren't the %z formats unportable (even with our newly found reliance on
> > > more C99)? I vaguely recall trying them recently and the windows CI jobs
> > > erroring...
> >
> > According to [1] it has been available since at least 2015. It is certainly
> > much nicer than casting every size_t to uintmax_t and having to use PRIuMAX.
>
> If we're relying on a new enough MSVC for C11, then it's much newer than
> 2015, so we should be fine.  It's mandatory on POSIX systems.

The MSVCRT we're using in GCC is much, much older, and that won't change
anytime soon, I don't think.

You _might_ get the code to compile a `%zu` format by playing some macro
tricks, but executing the result would still not work.

It sure would be nice if we could use all that POSIX promises... but we
can't. Sorry...

Ciao,
Dscho

ssize_t portability (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref)

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 14 2022, Phillip Wood wrote:

> Hi Brian and Ævar
> [...]

Hi, sorry about the late reply, just on this point (which I see is stil
unaddressed), and changing $subject for the generic question:

>>> +	for (ssize_t i = nitems - 1; i >= 0; i--) {
>> The ssize_t type can be really small (it's not a signed size_t), so
>> this
>> is unportable, but in practice maybe it's OK...
>
> I'm not really convinced by this ssize_t can be small argument[2], do
> you know of any platforms where it is true?

Where we'd overflow in this code as written? Yes, every platform we
build on, since e.g. on Linux it's got half the unsigned size of
SIZE_MAX, on my 64 bit box:

    SIZE_MAX  = 18446744073709551615
    SSIZE_MAX = 9223372036854775807

Of course exceeding 2^63-1 or even 2^31-1 number of stashes seems
unlikely in practice.

If you meant are there platforms where ssize_t is as small as you can
pedintically make it, i.e. not just half the signed range of size_t, but
something much smaller?

Possibly, but I think that's unlikely in practice given the
homogenization in computing. Even C just mandated 2's compliment!

Although I think it's still imporant to understand that in the specific
case of ssize_t, unlike other "signed version of type X" the *intent* of
the standard is clearly to *not* mandate that it's a signed version of
size_t. I.e. it's intended for functions like:

     ssize_t read(int fildes, void *buf, size_t nbyte);

Which both per the standard and in practice might have limits that are
much smaller than their respective types. E.g. on my Linux box read(2)
says:

    On Linux, read() (and similar system calls) will transfer at most
    0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
    actually transferred.  (This is true on both 32-bit and 64-bit
    systems.)

I can see how *some* platform might take those liberties in
practice. I.e. 1/2 of your addressable memory/offset != number of bytes
you'd want to return or consider at once for I/O purposes.

But in any case, we do have existing unportable code in-tree, but
generally with C it's good practice to avoid unportable code if it's
easy, you never know what platform will be released tomorrow that you
might have to scramble to support.

As I noted in this and other similar cases it's trivial to just use
size_t here. It's just a matter of changing (leaving the C99-specifics
here aside):

	for (i = nitems - 1; i >= 0; i--) {
		item = ary[i];
                [...];

To, as I did in 99d60545f87 (string-list API: change "nr" and "alloc" to
"size_t", 2022-03-07):

	for (i = nitems; i >= 1; i--) {
		item = ary[i - 1];
                [...];

Or even, if repeating "i - 1" is tedious:

	for (cnt = nitems; cnt >= 1; cnt--) {
		size_t i = cnt - 1;

		item = ary[i];
                [...];

Portability concerns aside I think such code is much clearer and easier
to reason about, since you no longer have to carefully squint to see if
we're doing the right thing with the two types in play.

Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Junio C Hamano]

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

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 128f0a01ef..582a04dbab 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -33,6 +33,7 @@ static const char * const git_stash_usage[] = {
>  	   "          [--] [<pathspec>...]]"),
>  	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
>  	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
> +	N_("git stash export (--print | --to-ref <ref>) [<stashes>]"),

If you mean we can take zero or more stash entries, the way to write
it I think is 

	[<stash>...]

cf. a few lines above about <pathspec>.

> +static const char * const git_stash_export_usage[] = {
> +	N_("git stash export (--print | --to-ref <ref>) [<stashes>]"),
> +	NULL
> +};

Likewise.

I think we agreed that it is a better design to reuse the actual
stash entries as one of the parents of the exported chain element
(while using another parent to string the entries together), so I
won't comment on the actual implementation that recreates multi-way
merge commits with no relation to the stash entries, which we won't
see in the next iteration.

Thanks.

[PATCH 5/6] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

Now that we have a way to export stashes to a ref, let's provide a way
to import them from such a ref back to the stash.  This works much the
way the export code does, except that we strip off the first parent
chain commit and then store each resulting commit back to the stash.

We don't clear the stash first and instead add the specified stashes to
the top of the stash.  This is because users may want to export just a
few stashes, such as to share a small amount of work in progress with a
colleague, and it would be undesirable for the receiving user to lose
all of their data.  For users who do want to replace the stash, it's
easy to do to: simply run "git stash clear" first.

We specifically rely on the fact that we'll produce identical stash
commits on both sides in our tests.  This provides a cheap,
straightforward check for our tests and also makes it easy for users to
see if they already have the same data in both repositories.  Note that
the exported commits don't because we don't write a predictable base
commit, however.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c  | 125 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t3903-stash.sh |  52 ++++++++++++++++++++
 2 files changed, 177 insertions(+)

diff --git a/builtin/stash.c b/builtin/stash.c
index 582a04dbab..626e7b8531 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -34,6 +34,7 @@ static const char * const git_stash_usage[] = {
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
 	N_("git stash export (--print | --to-ref <ref>) [<stashes>]"),
+	N_("git stash import <commit>"),
 	NULL
 };
 
@@ -95,6 +96,10 @@ static const char * const git_stash_export_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_import_usage[] = {
+	N_("git stash import <commit>"),
+	NULL
+};
 
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
  * 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
+ * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
  * w_tree is set to the working tree
  * b_tree is set to the base tree
  * i_tree is set to the index tree
@@ -114,6 +120,7 @@ struct stash_info {
 	struct object_id b_commit;
 	struct object_id i_commit;
 	struct object_id u_commit;
+	struct object_id c_commit;
 	struct object_id w_tree;
 	struct object_id b_tree;
 	struct object_id i_tree;
@@ -138,6 +145,32 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
 		die(_("'%s' is not a stash-like commit"), revision);
 }
 
+static int get_stash_info_for_import(struct stash_info *info, const struct object_id *oid)
+{
+	int has_parents;
+	char hexoid[GIT_MAX_HEXSZ + 1];
+	oid_to_hex_r(hexoid, oid);
+
+	oidcpy(&info->w_commit, oid);
+	if (get_oidf(&info->w_tree, "%s:", hexoid))
+		return -1;
+
+	has_parents = !get_oidf(&info->c_commit, "%s^1", hexoid);
+
+	/* If this tree is the empty one and we have no parents, we've reached the end. */
+	if (oideq(&info->w_tree, the_hash_algo->empty_tree) && !has_parents)
+		return 1;
+	if (get_oidf(&info->b_commit, "%s^2", hexoid) ||
+	    get_oidf(&info->i_commit, "%s^3", hexoid) ||
+	    get_oidf(&info->b_tree, "%s^2:", hexoid) ||
+	    get_oidf(&info->i_tree, "%s^3:", hexoid))
+		return -1;
+
+	info->has_u = !get_oidf(&info->u_commit, "%s^4", hexoid) &&
+		      !get_oidf(&info->u_tree, "%s^4:", hexoid);
+	return 0;
+}
+
 static int get_stash_info_1(struct stash_info *info, const char *commit, int quiet)
 {
 	int ret;
@@ -1827,6 +1860,96 @@ static char *write_commit_with_parents(struct object_id *out, const struct stash
 	return msg;
 }
 
+static int do_import_stash(const char *rev)
+{
+	struct object_id oid;
+	size_t nitems = 0, nalloc = 0;
+	struct stash_info *items = NULL;
+	int res = 0;
+
+	if (get_oid(rev, &oid))
+		return error(_("not a valid revision: %s"), rev);
+
+	/*
+	 * Walk the commit history, finding each stash entry, and load data into
+	 * the array.
+	 */
+	for (size_t i = 0;; i++, nitems++) {
+		int ret;
+
+		if (nalloc <= i) {
+			size_t new = nalloc * 3 / 2 + 5;
+			items = xrealloc(items, new * sizeof(*items));
+			nalloc = new;
+		}
+		memset(&items[i], 0, sizeof(*items));
+		/* We want this to be quiet because it might not exist. */
+		ret = get_stash_info_for_import(&items[i], &oid);
+		if (ret < 0)
+			return error(_("%s is not a valid exported stash commit"), oid_to_hex(&oid));
+		if (ret)
+			break;
+		oidcpy(&oid, &items[i].c_commit);
+	}
+
+	/*
+	 * Now, walk each entry, adding it to the stash as a normal stash
+	 * commit.
+	 */
+	for (ssize_t i = nitems - 1; i >= 0; i--) {
+		struct commit_list *parents = NULL;
+		struct commit_list **next = &parents;
+		struct object_id out;
+		char *msg;
+
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].b_commit), next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].i_commit), next);
+		if (items[i].has_u)
+			next = commit_list_append(lookup_commit_reference(the_repository,
+								   &items[i].u_commit),
+						  next);
+
+		msg = write_commit_with_parents(&out, &items[i], parents);
+		if (!msg) {
+			res = -1;
+			goto out;
+		}
+		if (do_store_stash(&out, msg, 1)) {
+			free(msg);
+			res = -1;
+			error(_("Cannot save the current status"));
+			goto out;
+		}
+		free(msg);
+	}
+out:
+	for (size_t i = 0; i < nitems; i++) {
+		free_stash_info(&items[i]);
+	}
+	free(items);
+
+	return res;
+}
+
+static int import_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_import_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (argc != 1)
+		return error(_("a revision to import from is required"));
+
+
+	ret = do_import_stash(argv[0]);
+	return ret;
+}
+
 static int do_export_stash(const char *ref, size_t argc, const char **argv)
 {
 	struct object_id base;
@@ -1996,6 +2119,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!save_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "export"))
 		return !!export_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "import"))
+		return !!import_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..d2ddede9be 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1295,6 +1295,58 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash export and import round-trip stashes' '
+	git reset &&
+	>untracked &&
+	>tracked1 &&
+	>tracked2 &&
+	git add tracked* &&
+	git stash -- &&
+	>subdir/untracked &&
+	>subdir/tracked1 &&
+	>subdir/tracked2 &&
+	git add subdir/tracked* &&
+	git stash -- subdir/ &&
+	stash0=$(git rev-parse --verify stash@{0}) &&
+	stash1=$(git rev-parse --verify stash@{1}) &&
+	simple=$(git stash export --print) &&
+	git stash clear &&
+	git stash import "$simple" &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1" &&
+	git stash export --to-ref refs/heads/foo &&
+	git stash clear &&
+	git stash import foo &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1"
+'
+
+test_expect_success 'stash import appends commits' '
+	git log --format=oneline -g refs/stash >actual &&
+	echo $(cat actual | wc -l) >count &&
+	git stash import refs/heads/foo &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = $(($(cat count) * 2)) actual
+'
+
+test_expect_success 'stash export can accept specified stashes' '
+	git stash clear &&
+	git stash import foo &&
+	git stash export --to-ref bar stash@{1} stash@{0} &&
+	git stash clear &&
+	git stash import bar &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported1" = "$stash0" &&
+	test "$imported0" = "$stash1" &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = 2 actual
+'
+
 test_expect_success 'stash apply should succeed with unmodified file' '
 	echo base >file &&
 	git add file &&

Re: [PATCH 5/6] builtin/stash: provide a way to import stashes from a ref

[Junio C Hamano]

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

> @@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
>   * 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
> + * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
>   * w_tree is set to the working tree
>   * b_tree is set to the base tree
>   * i_tree is set to the index tree
> @@ -114,6 +120,7 @@ struct stash_info {
>  	struct object_id b_commit;
>  	struct object_id i_commit;
>  	struct object_id u_commit;
> +	struct object_id c_commit;
>  	struct object_id w_tree;
>  	struct object_id b_tree;
>  	struct object_id i_tree;

With the redesign that an exported chain is a series of two-parent
merges, where the first parent is used to string them together in a
single strand of pearls and the second parent is the stash entry,
the above change becomes totally unnecessary, right?  The import
side will be doing a first-parent walk of the export, pushing the
second parent into reflog of refs/stash---we may want sanity check
these second parents with assert_stash_like(), but there is no need
to re-synthesize the stash entries anymore, which would simplify the
implementation quite a bit, right?
		
Namely:

> +static int do_import_stash(const char *rev)
> +{
> +	struct object_id oid;
> +	size_t nitems = 0, nalloc = 0;
> +	struct stash_info *items = NULL;
> +	int res = 0;
> +
> +	if (get_oid(rev, &oid))
> +		return error(_("not a valid revision: %s"), rev);
> +
> +	/*
> +	 * Walk the commit history, finding each stash entry, and load data into
> +	 * the array.
> +	 */
> +	for (size_t i = 0;; i++, nitems++) {
> +		int ret;
> +
> +		if (nalloc <= i) {
> +			size_t new = nalloc * 3 / 2 + 5;
> +			items = xrealloc(items, new * sizeof(*items));
> +			nalloc = new;
> +		}
> +		memset(&items[i], 0, sizeof(*items));
> +		/* We want this to be quiet because it might not exist. */
> +		ret = get_stash_info_for_import(&items[i], &oid);

The new helper function is not necessary; we can use vanilla
get_stash_info() on the second parent to get the same information,
and we do not really need to keep it in core.  We can sanity check
the shape of the imported stash entry right away and discard
everything except for the commit object name.

> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (ssize_t i = nitems - 1; i >= 0; i--) {
> +		struct commit_list *parents = NULL;
> +		struct commit_list **next = &parents;
> +		struct object_id out;
> +		char *msg;
> +
> +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].b_commit), next);
> +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].i_commit), next);
> +		if (items[i].has_u)
> +			next = commit_list_append(lookup_commit_reference(the_repository,
> +								   &items[i].u_commit),
> +						  next);
> +
> +		msg = write_commit_with_parents(&out, &items[i], parents);

And this part becomes completely unnecessary if we reuse what the
origin repository already had, which directly can be ...

> +		if (!msg) {
> +			res = -1;
> +			goto out;
> +		}
> +		if (do_store_stash(&out, msg, 1)) {

... fed to this call.

Re: [PATCH 5/6] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

On 2022-03-16 at 17:26:28, Junio C Hamano wrote:
> > +static int do_import_stash(const char *rev)
> > +{
> > +	struct object_id oid;
> > +	size_t nitems = 0, nalloc = 0;
> > +	struct stash_info *items = NULL;
> > +	int res = 0;
> > +
> > +	if (get_oid(rev, &oid))
> > +		return error(_("not a valid revision: %s"), rev);
> > +
> > +	/*
> > +	 * Walk the commit history, finding each stash entry, and load data into
> > +	 * the array.
> > +	 */
> > +	for (size_t i = 0;; i++, nitems++) {
> > +		int ret;
> > +
> > +		if (nalloc <= i) {
> > +			size_t new = nalloc * 3 / 2 + 5;
> > +			items = xrealloc(items, new * sizeof(*items));
> > +			nalloc = new;
> > +		}
> > +		memset(&items[i], 0, sizeof(*items));
> > +		/* We want this to be quiet because it might not exist. */
> > +		ret = get_stash_info_for_import(&items[i], &oid);
> 
> The new helper function is not necessary; we can use vanilla
> get_stash_info() on the second parent to get the same information,
> and we do not really need to keep it in core.  We can sanity check
> the shape of the imported stash entry right away and discard
> everything except for the commit object name.

Yes, in the new approach, I think we can do that.  We may still need the
behavior which doesn't die on error, but I think we can centralize it.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH 6/6] doc: add stash export and import to docs

[brian m. carlson]

Now that we have an easy way for users to import and export their
stashes, let's document this in manual page so users will know how to
use it.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 6e15f47525..283677314a 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,6 +20,8 @@ SYNOPSIS
 'git stash' clear
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
+'git stash' export ( --print | --to-ref <ref> ) [<stashes>]
+'git stash' import <commit>
 
 DESCRIPTION
 -----------
@@ -151,6 +153,18 @@ store::
 	reflog.  This is intended to be useful for scripts.  It is
 	probably not the command you want to use; see "push" above.
 
+export ( --print | --to-ref <ref> ) [<stashes>]::
+
+	Export the specified stashes, or all of them if none are specified, to
+	a chain of commits which can be transferred using the normal fetch and
+	push mechanisms, then imported using the `import` subcommand.
+
+import <commit>::
+
+	Import the specified stashes from the specified commit, which must have been
+	created by `export`, and add them to the list of stashes.  To replace the
+	existing stashes, use `clear` first.
+
 OPTIONS
 -------
 -a::
@@ -239,6 +253,19 @@ literally (including newlines and quotes).
 +
 Quiet, suppress feedback messages.
 
+--print::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes without
+storing it anywhere in the ref namespace and print the object ID to
+standard output.  This is designed for scripts.
+
+--to-ref::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes and store
+it to the specified ref.
+
 \--::
 	This option is only valid for `push` command.
 +

Re: [PATCH 6/6] doc: add stash export and import to docs

[Junio C Hamano]

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

> Now that we have an easy way for users to import and export their
> stashes, let's document this in manual page so users will know how to
> use it.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  Documentation/git-stash.txt | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 6e15f47525..283677314a 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -20,6 +20,8 @@ SYNOPSIS
>  'git stash' clear
>  'git stash' create [<message>]
>  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
> +'git stash' export ( --print | --to-ref <ref> ) [<stashes>]

Again, "[<stash>...]", I think.  Do we want SP inside () but no SP
inside []?  That looks inconsistent.

I.e.

    'git stash' export (--print|--to-ref <ref>) [<stash>...]


> +export ( --print | --to-ref <ref> ) [<stashes>]::
> +
> +	Export the specified stashes, or all of them if none are specified, to
> +	a chain of commits which can be transferred using the normal fetch and
> +	push mechanisms, then imported using the `import` subcommand.

Ditto.

> +import <commit>::
> +
> +	Import the specified stashes from the specified commit, which must have been
> +	created by `export`, and add them to the list of stashes.  To replace the
> +	existing stashes, use `clear` first.

A tangent.

I personally prefer the style to have a blank line after the
"<dt>::" in asciidoc's way of doing description list used here, both
in these two new entries and many existing ones, by the way.

We probably should do that consistently, though.

>  OPTIONS
>  -------
>  -a::
> @@ -239,6 +253,19 @@ literally (including newlines and quotes).
>  +
>  Quiet, suppress feedback messages.
>  
> +--print::
> +	This option is only valid for `export`.
> ++
> +Create the chain of commits representing the exported stashes without
> +storing it anywhere in the ref namespace and print the object ID to
> +standard output.  This is designed for scripts.
> +
> +--to-ref::
> +	This option is only valid for `export`.
> ++
> +Create the chain of commits representing the exported stashes and store
> +it to the specified ref.
> +
>  \--::
>  	This option is only valid for `push` command.
>  +

There is one more necessary update after this, no?

diff --git i/Documentation/git-stash.txt w/Documentation/git-stash.txt
index 283677314a..acce92b5d9 100644
--- i/Documentation/git-stash.txt
+++ w/Documentation/git-stash.txt
@@ -283,7 +283,7 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 <stash>::
 	This option is only valid for `apply`, `branch`, `drop`, `pop`,
-	`show` commands.
+	`show`, and `export` commands.
 +
 A reference of the form `stash@{<revision>}`. When no `<stash>` is
 given, the latest stash is assumed (that is, `stash@{0}`).

Re: [PATCH 6/6] doc: add stash export and import to docs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> Now that we have an easy way for users to import and export their
>> stashes, let's document this in manual page so users will know how to
>> use it.

It was raised during the review club meeting that it would be easier
to read if these documentation updates are made in the same commit as
the ones that added the feature.

Re: [PATCH 0/6] Importing and exporting stashes to refs

[Junio C Hamano]

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

> ...  The commits used in a stash export are nearly
> identical to those used in the stashes, with one notable change: the
> first parent of a stash is a pointer to the previous stash, or an empty
> commit if there is no previous stash.  All of the other parents used in
> the stash commit are present following it in their normal order.
> ...
> We intentionally attempt to exactly round-trip commits between stashes,
> although we don't do so for the exported data due to the base commit not
> having identical timestamps.  Preserving the commits exactly lets us
> more efficiently test our code and it also permits users to more easily
> determine if they have the same data.

Hmph, out of reflog entries stash@{0}, stash@{1}, stash@{3}, if we
create a chain of commits A, B, C such that

	A^2 = B, A^1 = stash@{0}
	B^2 = C, B^1 = stash@{1}
	         C^1 = stash@{2}

then the original stash entry commits can be recreated identically,
and after you export the stash as "A", you can "import" from it
without creating any new commit to represent the stash entries, no?

When we create A, if we use a predictable commit log message and
the same author/committer ident as A^1 (i.e. stash@{0}), and do it
the same for B and C, then no matter who exports the stash and at
which time, we'd get an identical result, I would presume.

> The tooling here is intentionally plumbing.  It's designed to be simple
> and functional and get the basic job done.  If we want additional
> features, we can add them in the future, but this should be a simple,
> basic feature set that can support additional uses.

Sounds sensible.

Re: [PATCH 0/6] Importing and exporting stashes to refs

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

On 2022-03-10 at 19:14:59, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > ...  The commits used in a stash export are nearly
> > identical to those used in the stashes, with one notable change: the
> > first parent of a stash is a pointer to the previous stash, or an empty
> > commit if there is no previous stash.  All of the other parents used in
> > the stash commit are present following it in their normal order.
> > ...
> > We intentionally attempt to exactly round-trip commits between stashes,
> > although we don't do so for the exported data due to the base commit not
> > having identical timestamps.  Preserving the commits exactly lets us
> > more efficiently test our code and it also permits users to more easily
> > determine if they have the same data.
> 
> Hmph, out of reflog entries stash@{0}, stash@{1}, stash@{3}, if we
> create a chain of commits A, B, C such that
> 
> 	A^2 = B, A^1 = stash@{0}
> 	B^2 = C, B^1 = stash@{1}
> 	         C^1 = stash@{2}
> 
> then the original stash entry commits can be recreated identically,
> and after you export the stash as "A", you can "import" from it
> without creating any new commit to represent the stash entries, no?

True, that's an alternative approach.  Mine has the nice ability that
you can see the items in the stash with log --first-parent, which I
found to be useful in my testing.  We could of course change yours
to have that property as well by reversing the order, but then the last
item in the chain would have a base commit or a different pattern.

Yours does have the nice ability that we can see the actual original
stash commits as well.

> When we create A, if we use a predictable commit log message and
> the same author/committer ident as A^1 (i.e. stash@{0}), and do it
> the same for B and C, then no matter who exports the stash and at
> which time, we'd get an identical result, I would presume.

True.

I do want to preserve my nice --first-parent property.  What I propose
to do is this: I'll take your approach and reverse the parents to
preserve the --first-parent chain and synthesize a predictable root
commit based on the fake ID information we use for stashes when nobody's
provided any.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/6] Importing and exporting stashes to refs

[Junio C Hamano]

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

> I do want to preserve my nice --first-parent property.  What I propose
> to do is this: I'll take your approach and reverse the parents to
> preserve the --first-parent chain and synthesize a predictable root
> commit based on the fake ID information we use for stashes when nobody's
> provided any.

I am wondering if this can be made not an export format but a new
mechanism to store stashes that we use without having to export and
import.  Capping the series of "stash entry" commits with an extra
commit that is continuously amended, and recording which stash entry
has already been used (and not to be shown) etc., in the log message
part of that commit, would give us "stash drop" without rewriting
all the history and would easily bring us to feature parity with the
reflog based implementation, I would hope?

Re: [PATCH 0/6] Importing and exporting stashes to refs

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On 2022-03-10 at 21:38:42, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > I do want to preserve my nice --first-parent property.  What I propose
> > to do is this: I'll take your approach and reverse the parents to
> > preserve the --first-parent chain and synthesize a predictable root
> > commit based on the fake ID information we use for stashes when nobody's
> > provided any.
> 
> I am wondering if this can be made not an export format but a new
> mechanism to store stashes that we use without having to export and
> import.  Capping the series of "stash entry" commits with an extra
> commit that is continuously amended, and recording which stash entry
> has already been used (and not to be shown) etc., in the log message
> part of that commit, would give us "stash drop" without rewriting
> all the history and would easily bring us to feature parity with the
> reflog based implementation, I would hope?

I had thought of providing a different stash format via
extensions.stashFormat or something instead of this.  The problem becomes
backward compatibility: we essentially can't use stashes with older
versions, and a very common use case in development is to work inside a
Docker container or such where the version of Git is whatever the OS
happened to ship, so such a change can be painful.  It also impedes
working with the repository using libgit2, which is heavily used in some
environments (e.g., the Rust toolchain), until someone ports that change
there.

I'm not opposed to seeing such a format change where refs/stash itself
becomes pushable via a format change, but for my present use case, I
don't want to do that right now.  It's possible that I might write such
a series in the future, or someone else could write it, but I won't be
writing it at the present moment.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 0/4] Importing and exporting stashes to refs

[brian m. carlson]

Stashes are currently stored using the reflog in a given repository.
This is an interesting and novel way to handle them, but there is no way
to easily move a stash across machines.  For example, stashes cannot be
bundled, pushed, or fetched.

Let's solve this problem by allowing users to import and export stashes
to a chain of commits.  The commits used in a stash export contain two
parents: one which is the pointer to the next exported stash (or to an
empty commit with no parents if there are no more) and the second is the
stash commit that would normally be stored in the reflog.

We now produce completely reproducible exports by creating the base
commit with predictable author and committer names and timestamps.
The timestamp is arbitrary but picked to be the same as the one used by
format-patch.

We also now avoid size_t.  Because of the constraints of portability, we
can't have nice things like %zu, so now we use int.  Parties wanting to
import or export more than 2 billion stashes are welcome to send a patch
improving the situation.  While I have many stashes, I'm at least a
billion or so away from that point, so I'm going to punt that to a
future series.

Changes from v1:
* Change storage format as suggested by Junio.
* Rename to GIT_OID_GENTLY.
* Remove unnecessary initializations.
* Use ALLOC_GROW_BY.
* Ensure completely reproducible exports.
* Avoid size_t.
* Various other code cleanups.

brian m. carlson (4):
  object-name: make get_oid quietly return an error
  builtin/stash: factor out revision parsing into a function
  builtin/stash: provide a way to export stashes to a ref
  builtin/stash: provide a way to import stashes from a ref

 Documentation/git-stash.txt |  29 +++-
 builtin/stash.c             | 324 ++++++++++++++++++++++++++++++++++--
 cache.h                     |   1 +
 object-name.c               |   6 +-
 t/t3903-stash.sh            |  52 ++++++
 5 files changed, 397 insertions(+), 15 deletions(-)

[PATCH v2 1/4] object-name: make get_oid quietly return an error

[brian m. carlson]

A reasonable person looking at the signature and usage of get_oid and
friends might conclude that in the event of an error, it always returns
-1.  However, this is not the case.  Instead, get_oid_basic dies if we
go too far back into the history of a reflog (or, when quiet, simply
exits).

This is not especially useful, since in many cases, we might want to
handle this error differently.  Let's add a flag here to make it just
return -1 like elsewhere in these code paths.

Note that we cannot make this behavior the default, since we have many
other codepaths that rely on the existing behavior, including in tests.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h       | 1 +
 object-name.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 825ec17198..657e3ff17f 100644
--- a/cache.h
+++ b/cache.h
@@ -1376,6 +1376,7 @@ struct object_context {
 #define GET_OID_RECORD_PATH     0200
 #define GET_OID_ONLY_TO_DIE    04000
 #define GET_OID_REQUIRE_PATH  010000
+#define GET_OID_GENTLY        020000
 
 #define GET_OID_DISAMBIGUATORS \
 	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index 92862eeb1a..46dbfe36a6 100644
--- a/object-name.c
+++ b/object-name.c
@@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 						len, str,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
-			} else {
+			} else if (!(flags & GET_OID_GENTLY)) {
 				if (flags & GET_OID_QUIETLY) {
 					exit(128);
 				}
 				die(_("log for '%.*s' only has %d entries"),
 				    len, str, co_cnt);
 			}
+			if (flags & GET_OID_GENTLY) {
+				free(real_ref);
+				return -1;
+			}
 		}
 	}
 

[PATCH v2 2/4] builtin/stash: factor out revision parsing into a function

[brian m. carlson]

We allow several special forms of stash names in this code.  In the
future, we'll want to allow these same forms without parsing a stash
commit, so let's refactor this code out into a function for reuse.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 5897febfbe..4c281a5781 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -130,6 +130,24 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
 		die(_("'%s' is not a stash-like commit"), revision);
 }
 
+static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
+{
+	strbuf_init(revision, 0);
+	if (!commit) {
+		if (!ref_exists(ref_stash)) {
+			fprintf_ln(stderr, _("No stash entries found."));
+			return -1;
+		}
+
+		strbuf_addf(revision, "%s@{0}", ref_stash);
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(revision, "%s@{%s}", ref_stash, commit);
+	} else {
+		strbuf_addstr(revision, commit);
+	}
+	return 0;
+}
+
 static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 {
 	int ret;
@@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	if (argc == 1)
 		commit = argv[0];
 
-	strbuf_init(&info->revision, 0);
-	if (!commit) {
-		if (!ref_exists(ref_stash)) {
-			free_stash_info(info);
-			fprintf_ln(stderr, _("No stash entries found."));
-			return -1;
-		}
-
-		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
-	} else if (strspn(commit, "0123456789") == strlen(commit)) {
-		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
-	} else {
-		strbuf_addstr(&info->revision, commit);
+	if (parse_revision(&info->revision, commit, 0)) {
+		free_stash_info(info);
+		return -1;
 	}
 
 	revision = info->revision.buf;

[PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

A common user problem is how to sync in-progress work to another
machine.  Users currently must use some sort of transfer of the working
tree, which poses security risks and also necessarily causes the index
to become dirty.  The experience is suboptimal and frustrating for
users.

A reasonable idea is to use the stash for this purpose, but the stash is
stored in the reflog, not in a ref, and as such it cannot be pushed or
pulled.  This also means that it cannot be saved into a bundle or
preserved elsewhere, which is a problem when using throwaway development
environments.

Let's solve this problem by allowing the user to export the stash to a
ref (or, to just write it into the repository and print the hash, à la
git commit-tree).  Introduce git stash export, which writes a chain of
commits where the first parent is always a chain to the previous stash,
or to a single, empty commit (for the final item) and the second is the
stash commit normally written to the reflog.

Iterate over each stash from topmost to bottomost, looking up the data
for each one, and then create the chain from the single empty commit
back up in reverse order.  Generate a predictable empty commit so our
behavior is reproducible.  Create a useful commit message, preserving
the author and committer information, to help users identify stash
commits when viewing them as normal commits.

If the user has specified specific stashes they'd like to export
instead, use those instead of iterating over all of the stashes.

As part of this, specifically request quiet behavior when looking up the
OID for a revision because we will eventually hit a revision that
doesn't exist and we don't want to die when that occurs.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |  22 ++++-
 builtin/stash.c             | 183 ++++++++++++++++++++++++++++++++++++
 2 files changed, 204 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 6e15f47525..162110314e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 'git stash' clear
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
+'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
 
 DESCRIPTION
 -----------
@@ -151,6 +152,12 @@ store::
 	reflog.  This is intended to be useful for scripts.  It is
 	probably not the command you want to use; see "push" above.
 
+export ( --print | --to-ref <ref> ) [<stash>...]::
+
+	Export the specified stashes, or all of them if none are specified, to
+	a chain of commits which can be transferred using the normal fetch and
+	push mechanisms, then imported using the `import` subcommand.
+
 OPTIONS
 -------
 -a::
@@ -239,6 +246,19 @@ literally (including newlines and quotes).
 +
 Quiet, suppress feedback messages.
 
+--print::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes without
+storing it anywhere in the ref namespace and print the object ID to
+standard output.  This is designed for scripts.
+
+--to-ref::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes and store
+it to the specified ref.
+
 \--::
 	This option is only valid for `push` command.
 +
@@ -256,7 +276,7 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 <stash>::
 	This option is only valid for `apply`, `branch`, `drop`, `pop`,
-	`show` commands.
+	`show`, and `export` commands.
 +
 A reference of the form `stash@{<revision>}`. When no `<stash>` is
 given, the latest stash is assumed (that is, `stash@{0}`).
diff --git a/builtin/stash.c b/builtin/stash.c
index 4c281a5781..6f1fa19172 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -33,6 +33,7 @@ static const char * const git_stash_usage[] = {
 	   "          [--] [<pathspec>...]]"),
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
 	NULL
 };
 
@@ -89,6 +90,12 @@ static const char * const git_stash_save_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_export_usage[] = {
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	NULL
+};
+
+
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -1773,6 +1780,180 @@ static int save_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
+{
+	size_t author_len, committer_len;
+	struct commit *this;
+	const char *orig_author, *orig_committer;
+	char *author = NULL, *committer = NULL;
+	const char *buffer;
+	unsigned long bufsize;
+	const char *p;
+	struct strbuf msg = STRBUF_INIT;
+	int ret = 0;
+
+	this = lookup_commit_reference(the_repository, oid);
+	buffer = get_commit_buffer(this, &bufsize);
+	orig_author = find_commit_header(buffer, "author", &author_len);
+	orig_committer = find_commit_header(buffer, "committer", &committer_len);
+	p = memmem(buffer, bufsize, "\n\n", 2);
+
+	if (!orig_author || !orig_committer || !p) {
+		error(_("cannot parse commit %s"), oid_to_hex(oid));
+		goto out;
+	}
+	/* Jump to message. */
+	p += 2;
+	strbuf_addstr(&msg, "git stash: ");
+	strbuf_add(&msg, p, bufsize - (p - buffer));
+
+	author = xmemdupz(orig_author, author_len);
+	committer = xmemdupz(orig_committer, committer_len);
+
+	if (commit_tree_extended(msg.buf, msg.len,
+				 the_hash_algo->empty_tree, parents,
+				 out, author, committer,
+				 NULL, NULL)) {
+		ret = -1;
+		error(_("could not write commit"));
+		goto out;
+	}
+out:
+	strbuf_reset(&msg);
+	unuse_commit_buffer(this, buffer);
+	free(author);
+	free(committer);
+	return ret;
+}
+
+static int do_export_stash(const char *ref, size_t argc, const char **argv)
+{
+	struct object_id base;
+	struct object_context unused;
+	struct commit *prev;
+	struct object_id *items = NULL;
+	int nitems = 0, nalloc = 0;
+	int res = 0;
+	struct strbuf revision;
+	const char *author, *committer;
+
+	/*
+	 * This is an arbitrary, fixed date, specifically the one used by git
+	 * format-patch.  The goal is merely to produce reproducible output.
+	 */
+	prepare_fallback_ident("git stash", "git@stash");
+	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
+	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
+
+	/* First, we create a single empty commit. */
+	if (commit_tree_extended(NULL, 0, the_hash_algo->empty_tree, NULL, &base, author, committer, NULL, NULL))
+		return error(_("unable to write base commit"));
+
+	prev = lookup_commit_reference(the_repository, &base);
+
+	if (argc) {
+		/*
+		 * Find each specified stash, and load data into the array.
+		 */
+		for (int i = 0; i < argc; i++) {
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			if (parse_revision(&revision, argv[i], 0) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &items[i], &unused)) {
+				error(_("unable to find stash entry %s"), argv[i]);
+				res = -1;
+				goto out;
+			}
+		}
+	} else {
+		/*
+		 * Walk the reflog, finding each stash entry, and load data into the
+		 * array.
+		 */
+		for (int i = 0;; i++) {
+			char buf[32];
+			struct object_id oid;
+
+			snprintf(buf, sizeof(buf), "%d", i);
+			if (parse_revision(&revision, buf, 1) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &oid, &unused))
+				break;
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			oidcpy(&items[i], &oid);
+		}
+	}
+
+	/*
+	 * Now, create a set of commits identical to the regular stash commits,
+	 * but where their first parents form a chain to our original empty
+	 * base commit.
+	 */
+	for (int i = nitems - 1; i >= 0; i--) {
+		struct commit_list *parents = NULL;
+		struct commit_list **next = &parents;
+		struct object_id out;
+
+		next = commit_list_append(prev, next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
+		if (write_commit_with_parents(&out, &items[i], parents)) {
+			res = -1;
+			goto out;
+		}
+		prev = lookup_commit_reference(the_repository, &out);
+	}
+	if (ref)
+		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+	else
+		puts(oid_to_hex(&prev->object.oid));
+out:
+	free(items);
+
+	return res;
+}
+
+enum export_action {
+	ACTION_NONE,
+	ACTION_PRINT,
+	ACTION_TO_REF,
+};
+
+static int export_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	const char *ref = NULL;
+	enum export_action action = ACTION_NONE;
+	struct option options[] = {
+		OPT_CMDMODE(0, "print", &action,
+			    N_("print the object ID instead of writing it to a ref"),
+			    ACTION_PRINT),
+		OPT_CMDMODE(0, "to-ref", &action,
+			    N_("save the data to the given ref"),
+			    ACTION_TO_REF),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_export_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (action == ACTION_NONE) {
+		return error(_("exactly one of --print and --to-ref is required"));
+	} else if (action == ACTION_TO_REF) {
+		if (!argc)
+			return error(_("--to-ref requires an argument"));
+		ref = argv[0];
+		argc--;
+		argv++;
+	}
+
+
+	ret = do_export_stash(ref, argc, argv);
+	return ret;
+}
+
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
 	pid_t pid = getpid();
@@ -1816,6 +1997,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!push_stash(argc, argv, prefix, 0);
 	else if (!strcmp(argv[0], "save"))
 		return !!save_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "export"))
+		return !!export_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Junio C Hamano]

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

> +static int do_export_stash(const char *ref, size_t argc, const char **argv)
> +{
> +	struct object_id base;
> +	struct object_context unused;
> +	struct commit *prev;
> +	struct object_id *items = NULL;
> +	int nitems = 0, nalloc = 0;
> +	int res = 0;
> +	struct strbuf revision;
> +	const char *author, *committer;
> +
> +	/*
> +	 * This is an arbitrary, fixed date, specifically the one used by git
> +	 * format-patch.  The goal is merely to produce reproducible output.
> +	 */
> +	prepare_fallback_ident("git stash", "git@stash");
> +	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
> +	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
> +
> +	/* First, we create a single empty commit. */
> +	if (commit_tree_extended(NULL, 0, the_hash_algo->empty_tree, NULL, &base, author, committer, NULL, NULL))
> +		return error(_("unable to write base commit"));
> +
> +	prev = lookup_commit_reference(the_repository, &base);
> +
> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (int i = 0; i < argc; i++) {
> +			ALLOC_GROW_BY(items, nitems, 1, nalloc);

Documentation/CodingGuidelines still says this

 - Declaring a variable in the for loop "for (int i = 0; i < 10; i++)"
   is still not allowed in this codebase.

We have been experimenting since we merged 44ba10d6 (revision: use
C99 declaration of variable in for() loop, 2021-11-14) at 5a4069a1
(Merge branch 'jc/c99-var-decl-in-for-loop', 2021-12-21), which is
shipped as a part of v2.35.0 just a few months ago.

Let's not add more until we are sure that we do not have to revert
44ba10d6 (revision: use C99 declaration of variable in for() loop,
2021-11-14) to limit the potential damage, especially when it is so
easy to do so.

Just declare "int i" at the beginning of the funcion and keep
reusing it would be fine here.

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

On 2022-03-30 at 23:05:28, Junio C Hamano wrote:
> Documentation/CodingGuidelines still says this
> 
>  - Declaring a variable in the for loop "for (int i = 0; i < 10; i++)"
>    is still not allowed in this codebase.
> 
> We have been experimenting since we merged 44ba10d6 (revision: use
> C99 declaration of variable in for() loop, 2021-11-14) at 5a4069a1
> (Merge branch 'jc/c99-var-decl-in-for-loop', 2021-12-21), which is
> shipped as a part of v2.35.0 just a few months ago.
> 
> Let's not add more until we are sure that we do not have to revert
> 44ba10d6 (revision: use C99 declaration of variable in for() loop,
> 2021-11-14) to limit the potential damage, especially when it is so
> easy to do so.
> 
> Just declare "int i" at the beginning of the funcion and keep
> reusing it would be fine here.

Okay, I can reroll with that.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 29 2022, brian m. carlson wrote:

> A common user problem is how to sync in-progress work to another
> machine.  Users currently must use some sort of transfer of the working
> tree, which poses security risks and also necessarily causes the index
> to become dirty.  The experience is suboptimal and frustrating for
> users.
>
> A reasonable idea is to use the stash for this purpose, but the stash is
> stored in the reflog, not in a ref, and as such it cannot be pushed or
> pulled.  This also means that it cannot be saved into a bundle or
> preserved elsewhere, which is a problem when using throwaway development
> environments.
>
> Let's solve this problem by allowing the user to export the stash to a
> ref (or, to just write it into the repository and print the hash, à la
> git commit-tree).  Introduce git stash export, which writes a chain of
> commits where the first parent is always a chain to the previous stash,
> or to a single, empty commit (for the final item) and the second is the
> stash commit normally written to the reflog.
>
> Iterate over each stash from topmost to bottomost, looking up the data
> for each one, and then create the chain from the single empty commit
> back up in reverse order.  Generate a predictable empty commit so our
> behavior is reproducible.  Create a useful commit message, preserving
> the author and committer information, to help users identify stash
> commits when viewing them as normal commits.
>
> If the user has specified specific stashes they'd like to export
> instead, use those instead of iterating over all of the stashes.
>
> As part of this, specifically request quiet behavior when looking up the
> OID for a revision because we will eventually hit a revision that
> doesn't exist and we don't want to die when that occurs.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  Documentation/git-stash.txt |  22 ++++-
>  builtin/stash.c             | 183 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 204 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 6e15f47525..162110314e 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -20,6 +20,7 @@ SYNOPSIS
>  'git stash' clear
>  'git stash' create [<message>]
>  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
> +'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
>  
>  DESCRIPTION
>  -----------
> @@ -151,6 +152,12 @@ store::
>  	reflog.  This is intended to be useful for scripts.  It is
>  	probably not the command you want to use; see "push" above.
>  
> +export ( --print | --to-ref <ref> ) [<stash>...]::
> +

I think this extra \n here isn't needed.

> +static const char * const git_stash_export_usage[] = {
> +	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
> +	NULL
> +};
> +
> +

Stray too-much-whitespace.

>  static const char ref_stash[] = "refs/stash";
>  static struct strbuf stash_index_path = STRBUF_INIT;
>  
> @@ -1773,6 +1780,180 @@ static int save_stash(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> +static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;
> +	int ret = 0;

With this...

> +	this = lookup_commit_reference(the_repository, oid);
> +	buffer = get_commit_buffer(this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> +	p = memmem(buffer, bufsize, "\n\n", 2);
> +
> +	if (!orig_author || !orig_committer || !p) {
> +		error(_("cannot parse commit %s"), oid_to_hex(oid));
> +		goto out;

..isn't this a logic error, shouldn't we return -1 here?

> +	}
> +	/* Jump to message. */
> +	p += 2;
> +	strbuf_addstr(&msg, "git stash: ");
> +	strbuf_add(&msg, p, bufsize - (p - buffer));
> +
> +	author = xmemdupz(orig_author, author_len);
> +	committer = xmemdupz(orig_committer, committer_len);
> +
> +	if (commit_tree_extended(msg.buf, msg.len,
> +				 the_hash_algo->empty_tree, parents,
> +				 out, author, committer,
> +				 NULL, NULL)) {
> +		ret = -1;
> +		error(_("could not write commit"));

better as "ret = error(..."?

> +		goto out;
> +	}
> +out:
> +	strbuf_reset(&msg);
> +	unuse_commit_buffer(this, buffer);
> +	free(author);
> +	free(committer);
> +	return ret;
> +}
> +
> +static int do_export_stash(const char *ref, size_t argc, const char **argv)
> +{
> +	struct object_id base;
> +	struct object_context unused;
> +	struct commit *prev;
> +	struct object_id *items = NULL;
> +	int nitems = 0, nalloc = 0;

Can nalloc be moved into the if=else scopes?

Also shouldn't these be size_t...?

> +	int res = 0;
> +	struct strbuf revision;
> +	const char *author, *committer;
> +
> +	/*
> +	 * This is an arbitrary, fixed date, specifically the one used by git
> +	 * format-patch.  The goal is merely to produce reproducible output.
> +	 */
> +	prepare_fallback_ident("git stash", "git@stash");
> +	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
> +	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
> +
> +	/* First, we create a single empty commit. */
> +	if (commit_tree_extended(NULL, 0, the_hash_algo->empty_tree, NULL, &base, author, committer, NULL, NULL))

Some very long lines here.

> +		return error(_("unable to write base commit"));
> +
> +	prev = lookup_commit_reference(the_repository, &base);
> +
> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (int i = 0; i < argc; i++) {

...as this is size_t, not int.

> +			ALLOC_GROW_BY(items, nitems, 1, nalloc);
> +			if (parse_revision(&revision, argv[i], 0) ||
> +			    get_oid_with_context(the_repository, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &items[i], &unused)) {
> +				error(_("unable to find stash entry %s"), argv[i]);
> +				res = -1;

ditto "ret = error(..."
> +				goto out;
> +			}
> +		}
> +	} else {
> +		/*
> +		 * Walk the reflog, finding each stash entry, and load data into the
> +		 * array.
> +		 */
> +		for (int i = 0;; i++) {

Aside from the C99 dependency Junio mentioned, this should also be size_t.

> +	/*
> +	 * Now, create a set of commits identical to the regular stash commits,
> +	 * but where their first parents form a chain to our original empty
> +	 * base commit.
> +	 */
> +	for (int i = nitems - 1; i >= 0; i--) {

Did you spot my "count down" suggestion in
https://lore.kernel.org/git/220311.86bkydi65v.gmgdl@evledraar.gmail.com/
on the v1?

> +		struct commit_list *parents = NULL;
> +		struct commit_list **next = &parents;
> +		struct object_id out;
> +
> +		next = commit_list_append(prev, next);
> +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
> +		if (write_commit_with_parents(&out, &items[i], parents)) {

Here we returned -1 from this earlier, I think this would be more
straightforward as:
	
	res = write_commit_with_parents(...)
	if (res < 0)
		goto out;
	

> +			res = -1;
> +			goto out;

So one doesn't have to wonder why we're ignoring the error value, and
using -1, but then treating all non-zero as errors.

> +		}
> +		prev = lookup_commit_reference(the_repository, &out);
> +	}
> +	if (ref)
> +		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
> +	else
> +		puts(oid_to_hex(&prev->object.oid));
> +out:
> +	free(items);
> +
> +	return res;
> +}
> +
> +enum export_action {
> +	ACTION_NONE,
> +	ACTION_PRINT,
> +	ACTION_TO_REF,
> +};
> +
> +static int export_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int ret = 0;

It looks like we don't need to initialize this.

> +	const char *ref = NULL;
> +	enum export_action action = ACTION_NONE;
> +	struct option options[] = {
> +		OPT_CMDMODE(0, "print", &action,
> +			    N_("print the object ID instead of writing it to a ref"),
> +			    ACTION_PRINT),
> +		OPT_CMDMODE(0, "to-ref", &action,
> +			    N_("save the data to the given ref"),
> +			    ACTION_TO_REF),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_export_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (action == ACTION_NONE) {
> +		return error(_("exactly one of --print and --to-ref is required"));
> +	} else if (action == ACTION_TO_REF) {
> +		if (!argc)
> +			return error(_("--to-ref requires an argument"));
> +		ref = argv[0];
> +		argc--;
> +		argv++;
> +	}
> +
> +

nit: Too much whitespace

> +	ret = do_export_stash(ref, argc, argv);
> +	return ret;

Aside from the "ret" case above, maybe this would be better if the
"action" check became a swith, then the compiler would help check it
against the enum, and this wouldn't implicitly be both ACTION_PRINT and
ACTION_TO_REF, but could be done via a fall-through.

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> @@ -151,6 +152,12 @@ store::
>>  	reflog.  This is intended to be useful for scripts.  It is
>>  	probably not the command you want to use; see "push" above.
>>  
>> +export ( --print | --to-ref <ref> ) [<stash>...]::
>> +
>
> I think this extra \n here isn't needed.

I wrote the same in my review on one of the previous rounds, but
removed it before sending it out, because this is better with than
without.  The blank line isn't necessary but does not hurt.  And
other existing entries in this file seem to have them consistently.

Thanks for carefully reading the posted patch.

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5018 bytes --]

On 2022-03-31 at 01:56:01, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 29 2022, brian m. carlson wrote:
> 
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index 6e15f47525..162110314e 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -20,6 +20,7 @@ SYNOPSIS
> >  'git stash' clear
> >  'git stash' create [<message>]
> >  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
> > +'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
> >  
> >  DESCRIPTION
> >  -----------
> > @@ -151,6 +152,12 @@ store::
> >  	reflog.  This is intended to be useful for scripts.  It is
> >  	probably not the command you want to use; see "push" above.
> >  
> > +export ( --print | --to-ref <ref> ) [<stash>...]::
> > +
> 
> I think this extra \n here isn't needed.

All the rest of the entries have it that way.  Junio likes it, you
don't, but it's consistent with the rest of the file and I'm just
following along.

> > +static const char * const git_stash_export_usage[] = {
> > +	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
> > +	NULL
> > +};
> > +
> > +
> 
> Stray too-much-whitespace.

Fixed in v3 already.

> > +	this = lookup_commit_reference(the_repository, oid);
> > +	buffer = get_commit_buffer(this, &bufsize);
> > +	orig_author = find_commit_header(buffer, "author", &author_len);
> > +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> > +	p = memmem(buffer, bufsize, "\n\n", 2);
> > +
> > +	if (!orig_author || !orig_committer || !p) {
> > +		error(_("cannot parse commit %s"), oid_to_hex(oid));
> > +		goto out;
> 
> ..isn't this a logic error, shouldn't we return -1 here?

Yes, it is.

> better as "ret = error(..."?

Yup.  v4 will have it fixed in both places.

> Can nalloc be moved into the if=else scopes?

It _can_, but it's used in both, so I don't see a particular reason to
do so.

> Some very long lines here.

Will wrap in v4.

> > +		return error(_("unable to write base commit"));
> > +
> > +	prev = lookup_commit_reference(the_repository, &base);
> > +
> > +	if (argc) {
> > +		/*
> > +		 * Find each specified stash, and load data into the array.
> > +		 */
> > +		for (int i = 0; i < argc; i++) {
> 
> ...as this is size_t, not int.

I'll make argc int.

> > +				goto out;
> > +			}
> > +		}
> > +	} else {
> > +		/*
> > +		 * Walk the reflog, finding each stash entry, and load data into the
> > +		 * array.
> > +		 */
> > +		for (int i = 0;; i++) {
> 
> Aside from the C99 dependency Junio mentioned, this should also be size_t.

Nope.  I specifically decided not to do that and mentioned it in the
cover letter.  Since Windows doesn't let us have nice things like %zu,
I'm going to switch to int here and be consistent.  I'm not coding for
16-bit systems here and I feel 2 billion stashes is sufficient for the
needs of the project for the indefinite future based on the capabilities
of current human beings.

The C99 dependency has been removed in v3.

> > +	/*
> > +	 * Now, create a set of commits identical to the regular stash commits,
> > +	 * but where their first parents form a chain to our original empty
> > +	 * base commit.
> > +	 */
> > +	for (int i = nitems - 1; i >= 0; i--) {
> 
> Did you spot my "count down" suggestion in
> https://lore.kernel.org/git/220311.86bkydi65v.gmgdl@evledraar.gmail.com/
> on the v1?

I did, and I prefer this approach.  That would be necessary if we were
using size_t here, but we're not.

> > +		struct commit_list *parents = NULL;
> > +		struct commit_list **next = &parents;
> > +		struct object_id out;
> > +
> > +		next = commit_list_append(prev, next);
> > +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
> > +		if (write_commit_with_parents(&out, &items[i], parents)) {
> 
> Here we returned -1 from this earlier, I think this would be more
> straightforward as:
> 	
> 	res = write_commit_with_parents(...)
> 	if (res < 0)
> 		goto out;
> 	
> 
> > +			res = -1;
> > +			goto out;
> 
> So one doesn't have to wonder why we're ignoring the error value, and
> using -1, but then treating all non-zero as errors.

That will be fixed in v4.

> It looks like we don't need to initialize this.

It'll be removed in v4.

> > +	ret = do_export_stash(ref, argc, argv);
> > +	return ret;
> 
> Aside from the "ret" case above, maybe this would be better if the
> "action" check became a swith, then the compiler would help check it
> against the enum, and this wouldn't implicitly be both ACTION_PRINT and
> ACTION_TO_REF, but could be done via a fall-through.

Normally I'm a big fan of switch statements, but I don't feel in this
case that it logically represents the current code.  I think, given the
context, that an if-else is easier to read.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> On 2022-03-31 at 01:56:01, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Mar 29 2022, brian m. carlson wrote:
>> 
>> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> > index 6e15f47525..162110314e 100644
>> > --- a/Documentation/git-stash.txt
>> > +++ b/Documentation/git-stash.txt
>> > @@ -20,6 +20,7 @@ SYNOPSIS
>> >  'git stash' clear
>> >  'git stash' create [<message>]
>> >  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
>> > +'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
>> >  
>> >  DESCRIPTION
>> >  -----------
>> > @@ -151,6 +152,12 @@ store::
>> >  	reflog.  This is intended to be useful for scripts.  It is
>> >  	probably not the command you want to use; see "push" above.
>> >  
>> > +export ( --print | --to-ref <ref> ) [<stash>...]::
>> > +
>> 
>> I think this extra \n here isn't needed.
>
> All the rest of the entries have it that way.  Junio likes it, you
> don't, but it's consistent with the rest of the file and I'm just
> following along.

FWIW I really don't mind. I vaguely thought it might be an ASCIIDOC
syntax error as I'm used to seeing the other form, as e.g. adding an
extra \n before the following "+" is.

But it's not, and it's indeed consistent with the rest. looks good.

>> Can nalloc be moved into the if=else scopes?
>
> It _can_, but it's used in both, so I don't see a particular reason to
> do so.

I don't mind, FWIW the reason is just to save the reader skimming to
track down the various bits of the manual memory allocation.

But as you noted in the v3 reply this can also just use oidset, so ...

>> > +				goto out;
>> > +			}
>> > +		}
>> > +	} else {
>> > +		/*
>> > +		 * Walk the reflog, finding each stash entry, and load data into the
>> > +		 * array.
>> > +		 */
>> > +		for (int i = 0;; i++) {
>> 
>> Aside from the C99 dependency Junio mentioned, this should also be size_t.
>
> Nope.  I specifically decided not to do that and mentioned it in the
> cover letter.  Since Windows doesn't let us have nice things like %zu,
> I'm going to switch to int here and be consistent.

You mean to avoid PRIuMAX instead of %d with:

    snprintf(buf, sizeof(buf), "%d", i);

?

> [...(moved around)...]
>> Did you spot my "count down" suggestion in
>> https://lore.kernel.org/git/220311.86bkydi65v.gmgdl@evledraar.gmail.com/
>> on the v1?
>
> I did, and I prefer this approach.  That would be necessary if we were
> using size_t here, but we're not.
> [...]

> I'm not coding for 16-bit systems here and I feel 2 billion stashes is
> sufficient for the needs of the project for the indefinite future
> based on the capabilities of current human beings.

I just thought you might not have seen the v1 feedback, fair enough. And
I'm not going to argue that point here.

Just as an explanation: The reason to use these sorts of patterns isn't
because we might need to support 16 bit in the future, which we won't,
or that I think it's plausible that someone might have >2^31-1 stashes.

It's that we've been changing to larger types across the codebase, and
e.g. in 99d60545f87 (string-list API: change "nr" and "alloc" to
"size_t", 2022-03-07) changing to an unsigned type required changing
such an iterator.

So if we really don't need negative numbers, but are just using -1 as a
value to "stop" in a for-loop it's IMO worth it in general to just stick
with unsigned, it makes things more future-proof.

Also, not gcc or clang, but e.g. HP/UX's aCC compiler screams bloddy
murder about various "why are you assuming unsigned here?" in
comparisons across the codebase, which gcc/clang stay quiet about, to
the point where it downs out other useful warnings it gives about actual
potential issues.

That's not *directly* related to this, which it wouldn't warn about as
it's "int". But generally it's a result of us being loose in mixing
unsigned and signed types when it comes to a count of a number of items
that can't be negative, and won't be exact for "guard clause" code like
this.

And finally, even if we might say that more than 2^31-1 stashes is
insane it's just easier to reason about code where multiple "counts" are
in play if you don't need to carefully think about that aspect.

Anyway, </by-way-of-explanation over>. But just to be clear I think it's
fine that you want to keep this as-is.

>> > +	ret = do_export_stash(ref, argc, argv);
>> > +	return ret;
>> 
>> Aside from the "ret" case above, maybe this would be better if the
>> "action" check became a swith, then the compiler would help check it
>> against the enum, and this wouldn't implicitly be both ACTION_PRINT and
>> ACTION_TO_REF, but could be done via a fall-through.
>
> Normally I'm a big fan of switch statements, but I don't feel in this
> case that it logically represents the current code.  I think, given the
> context, that an if-else is easier to read.

Makes sense. Thanks.

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Junio C Hamano]

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

>> >  	probably not the command you want to use; see "push" above.
>> >  
>> > +export ( --print | --to-ref <ref> ) [<stash>...]::
>> > +
>> 
>> I think this extra \n here isn't needed.
>
> All the rest of the entries have it that way.  Junio likes it, you
> don't, but it's consistent with the rest of the file and I'm just
> following along.

I do not have preference either way.  The only reason why I think
this patch is better with the blank line is because other existing
entries are with a blank line between the header and the body.

Thanks.

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 29 2022, brian m. carlson wrote:

> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (int i = 0; i < argc; i++) {
> +			ALLOC_GROW_BY(items, nitems, 1, nalloc);
> +			if (parse_revision(&revision, argv[i], 0) ||
> +			    get_oid_with_context(the_repository, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &items[i], &unused)) {
> +				error(_("unable to find stash entry %s"), argv[i]);
> +				res = -1;
> +				goto out;
> +			}
> +		}

One thing I missed on the first read-through, in the earlier commit you
factored out parse_revision(), which now contains this code (which was
here before this series):
	
	+	if (!commit) {
	+		if (!ref_exists(ref_stash)) {
	+			fprintf_ln(stderr, _("No stash entries found."));
	+			return -1;
	+		}

Aren't we going to print both "No stash entries" and "unable to find
stash entry %s" in those cases?

Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On 2022-03-31 at 02:09:21, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 29 2022, brian m. carlson wrote:
> 
> > +	if (argc) {
> > +		/*
> > +		 * Find each specified stash, and load data into the array.
> > +		 */
> > +		for (int i = 0; i < argc; i++) {
> > +			ALLOC_GROW_BY(items, nitems, 1, nalloc);
> > +			if (parse_revision(&revision, argv[i], 0) ||
> > +			    get_oid_with_context(the_repository, revision.buf,
> > +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> > +						 &items[i], &unused)) {
> > +				error(_("unable to find stash entry %s"), argv[i]);
> > +				res = -1;
> > +				goto out;
> > +			}
> > +		}
> 
> One thing I missed on the first read-through, in the earlier commit you
> factored out parse_revision(), which now contains this code (which was
> here before this series):
> 	
> 	+	if (!commit) {
> 	+		if (!ref_exists(ref_stash)) {
> 	+			fprintf_ln(stderr, _("No stash entries found."));
> 	+			return -1;
> 	+		}
> 
> Aren't we going to print both "No stash entries" and "unable to find
> stash entry %s" in those cases?

No, we're not, because commit isn't NULL.  However, I'll put this under
the quiet flag nevertheless for the situation in the next chunk down
(where argc is 0) where this is a problem.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

Now that we have a way to export stashes to a ref, let's provide a way
to import them from such a ref back to the stash.  This works much the
way the export code does, except that we strip off the first parent
chain commit and then store each resulting commit back to the stash.

We don't clear the stash first and instead add the specified stashes to
the top of the stash.  This is because users may want to export just a
few stashes, such as to share a small amount of work in progress with a
colleague, and it would be undesirable for the receiving user to lose
all of their data.  For users who do want to replace the stash, it's
easy to do to: simply run "git stash clear" first.

We specifically rely on the fact that we'll produce identical stash
commits on both sides in our tests.  This provides a cheap,
straightforward check for our tests and also makes it easy for users to
see if they already have the same data in both repositories.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |   7 +++
 builtin/stash.c             | 107 ++++++++++++++++++++++++++++++++++++
 t/t3903-stash.sh            |  52 ++++++++++++++++++
 3 files changed, 166 insertions(+)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 162110314e..28eb9cab0c 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -21,6 +21,7 @@ SYNOPSIS
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
+'git stash' import <commit>
 
 DESCRIPTION
 -----------
@@ -158,6 +159,12 @@ export ( --print | --to-ref <ref> ) [<stash>...]::
 	a chain of commits which can be transferred using the normal fetch and
 	push mechanisms, then imported using the `import` subcommand.
 
+import <commit>::
+
+	Import the specified stashes from the specified commit, which must have been
+	created by `export`, and add them to the list of stashes.  To replace the
+	existing stashes, use `clear` first.
+
 OPTIONS
 -------
 -a::
diff --git a/builtin/stash.c b/builtin/stash.c
index 6f1fa19172..4b198f1ab7 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -34,6 +34,7 @@ static const char * const git_stash_usage[] = {
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
 	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	N_("git stash import <commit>"),
 	NULL
 };
 
@@ -95,6 +96,10 @@ static const char * const git_stash_export_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_import_usage[] = {
+	N_("git stash import <commit>"),
+	NULL
+};
 
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
  * 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
+ * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
  * w_tree is set to the working tree
  * b_tree is set to the base tree
  * i_tree is set to the index tree
@@ -114,6 +120,7 @@ struct stash_info {
 	struct object_id b_commit;
 	struct object_id i_commit;
 	struct object_id u_commit;
+	struct object_id c_commit;
 	struct object_id w_tree;
 	struct object_id b_tree;
 	struct object_id i_tree;
@@ -1826,6 +1833,104 @@ static int write_commit_with_parents(struct object_id *out, const struct object_
 	return ret;
 }
 
+static int do_import_stash(const char *rev)
+{
+	struct object_id chain;
+	size_t nalloc = 0;
+	struct object_id *items = NULL;
+	int nitems = 0;
+	int res = 0;
+	const char *buffer = NULL;
+	struct commit *this = NULL;
+	char *msg = NULL;
+
+	if (get_oid(rev, &chain))
+		return error(_("not a valid revision: %s"), rev);
+
+	/*
+	 * Walk the commit history, finding each stash entry, and load data into
+	 * the array.
+	 */
+	for (int i = 0;; i++) {
+		struct object_id tree, oid;
+		char revision[GIT_MAX_HEXSZ + 1];
+
+		oid_to_hex_r(revision, &chain);
+
+		if (get_oidf(&tree, "%s:", revision) ||
+		    !oideq(&tree, the_hash_algo->empty_tree)) {
+			return error(_("%s is not a valid exported stash commit"), revision);
+		}
+		if (get_oidf(&chain, "%s^1", revision) ||
+		    get_oidf(&oid, "%s^2", revision))
+			break;
+		ALLOC_GROW_BY(items, nitems, 1, nalloc);
+		oidcpy(&items[i], &oid);
+	}
+
+	/*
+	 * Now, walk each entry, adding it to the stash as a normal stash
+	 * commit.
+	 */
+	for (int i = nitems - 1; i >= 0; i--) {
+		unsigned long bufsize;
+		const char *p;
+
+		this = lookup_commit_reference(the_repository, &items[i]);
+		buffer = get_commit_buffer(this, &bufsize);
+		if (!buffer) {
+			res = -1;
+			error(_("cannot read commit buffer for %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+
+		p = memmem(buffer, bufsize, "\n\n", 2);
+		if (!p) {
+			res = -1;
+			error(_("cannot parse commit %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+
+		p += 2;
+		msg = xmemdupz(p, bufsize - (p - buffer));
+		unuse_commit_buffer(this, buffer);
+		buffer = NULL;
+
+		if (do_store_stash(&items[i], msg, 1)) {
+			res = -1;
+			error(_("cannot save the stash for %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+		FREE_AND_NULL(msg);
+	}
+out:
+	if (this && buffer)
+		unuse_commit_buffer(this, buffer);
+	free(items);
+	free(msg);
+
+	return res;
+}
+
+static int import_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_import_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (argc != 1)
+		return error(_("a revision to import from is required"));
+
+
+	ret = do_import_stash(argv[0]);
+	return ret;
+}
+
 static int do_export_stash(const char *ref, size_t argc, const char **argv)
 {
 	struct object_id base;
@@ -1999,6 +2104,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!save_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "export"))
 		return !!export_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "import"))
+		return !!import_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..d2ddede9be 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1295,6 +1295,58 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash export and import round-trip stashes' '
+	git reset &&
+	>untracked &&
+	>tracked1 &&
+	>tracked2 &&
+	git add tracked* &&
+	git stash -- &&
+	>subdir/untracked &&
+	>subdir/tracked1 &&
+	>subdir/tracked2 &&
+	git add subdir/tracked* &&
+	git stash -- subdir/ &&
+	stash0=$(git rev-parse --verify stash@{0}) &&
+	stash1=$(git rev-parse --verify stash@{1}) &&
+	simple=$(git stash export --print) &&
+	git stash clear &&
+	git stash import "$simple" &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1" &&
+	git stash export --to-ref refs/heads/foo &&
+	git stash clear &&
+	git stash import foo &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1"
+'
+
+test_expect_success 'stash import appends commits' '
+	git log --format=oneline -g refs/stash >actual &&
+	echo $(cat actual | wc -l) >count &&
+	git stash import refs/heads/foo &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = $(($(cat count) * 2)) actual
+'
+
+test_expect_success 'stash export can accept specified stashes' '
+	git stash clear &&
+	git stash import foo &&
+	git stash export --to-ref bar stash@{1} stash@{0} &&
+	git stash clear &&
+	git stash import bar &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported1" = "$stash0" &&
+	test "$imported0" = "$stash1" &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = 2 actual
+'
+
 test_expect_success 'stash apply should succeed with unmodified file' '
 	echo base >file &&
 	git add file &&

Re: [PATCH v2 0/4] Importing and exporting stashes to refs

[Junio C Hamano]

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

> brian m. carlson (4):
>   object-name: make get_oid quietly return an error
>   builtin/stash: factor out revision parsing into a function
>   builtin/stash: provide a way to export stashes to a ref
>   builtin/stash: provide a way to import stashes from a ref
>
>  Documentation/git-stash.txt |  29 +++-
>  builtin/stash.c             | 324 ++++++++++++++++++++++++++++++++++--
>  cache.h                     |   1 +
>  object-name.c               |   6 +-
>  t/t3903-stash.sh            |  52 ++++++
>  5 files changed, 397 insertions(+), 15 deletions(-)

Merged to 'seen', t3903 seems to have a handful of breakages.
e.g. https://github.com/git/git/runs/5763421702?check_suite_focus=true

Thanks.

Re: [PATCH v2 0/4] Importing and exporting stashes to refs

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> brian m. carlson (4):
>>   object-name: make get_oid quietly return an error
>>   builtin/stash: factor out revision parsing into a function
>>   builtin/stash: provide a way to export stashes to a ref
>>   builtin/stash: provide a way to import stashes from a ref
>>
>>  Documentation/git-stash.txt |  29 +++-
>>  builtin/stash.c             | 324 ++++++++++++++++++++++++++++++++++--
>>  cache.h                     |   1 +
>>  object-name.c               |   6 +-
>>  t/t3903-stash.sh            |  52 ++++++
>>  5 files changed, 397 insertions(+), 15 deletions(-)
>
> Merged to 'seen', t3903 seems to have a handful of breakages.
> e.g. https://github.com/git/git/runs/5763421702?check_suite_focus=true

One is a segfault because of a missing STRBUF_INIT:
	
	diff --git a/builtin/stash.c b/builtin/stash.c
	index 7cab57bbe4b..92a1682941c 100644
	--- a/builtin/stash.c
	+++ b/builtin/stash.c
	@@ -1931,7 +1931,7 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 	struct object_id *items = NULL;
	 	int nitems = 0, nalloc = 0;
	 	int res = 0;
	-	struct strbuf revision;
	+	struct strbuf revision = STRBUF_INIT;
	 	const char *author, *committer;
	 
	 	/*
	

[PATCH v3 0/4] Importing and exporting stashes to refs

[brian m. carlson]

Stashes are currently stored using the reflog in a given repository.
This is an interesting and novel way to handle them, but there is no way
to easily move a stash across machines.  For example, stashes cannot be
bundled, pushed, or fetched.

Let's solve this problem by allowing users to import and export stashes
to a chain of commits.  The commits used in a stash export contain two
parents: one which is the pointer to the next exported stash (or to an
empty commit with no parents if there are no more) and the second is the
stash commit that would normally be stored in the reflog.

Changes from v2:
* Fix uninitialized strbuf.
* Avoid C99-style initializations.

Changes from v1:
* Change storage format as suggested by Junio.
* Rename to GIT_OID_GENTLY.
* Remove unnecessary initializations.
* Use ALLOC_GROW_BY.
* Ensure completely reproducible exports.
* Avoid size_t.
* Various other code cleanups.

brian m. carlson (4):
  object-name: make get_oid quietly return an error
  builtin/stash: factor out revision parsing into a function
  builtin/stash: provide a way to export stashes to a ref
  builtin/stash: provide a way to import stashes from a ref

 Documentation/git-stash.txt |  29 +++-
 builtin/stash.c             | 326 ++++++++++++++++++++++++++++++++++--
 cache.h                     |   1 +
 object-name.c               |   6 +-
 t/t3903-stash.sh            |  52 ++++++
 5 files changed, 399 insertions(+), 15 deletions(-)

[PATCH v3 1/4] object-name: make get_oid quietly return an error

[brian m. carlson]

A reasonable person looking at the signature and usage of get_oid and
friends might conclude that in the event of an error, it always returns
-1.  However, this is not the case.  Instead, get_oid_basic dies if we
go too far back into the history of a reflog (or, when quiet, simply
exits).

This is not especially useful, since in many cases, we might want to
handle this error differently.  Let's add a flag here to make it just
return -1 like elsewhere in these code paths.

Note that we cannot make this behavior the default, since we have many
other codepaths that rely on the existing behavior, including in tests.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h       | 1 +
 object-name.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 825ec17198..657e3ff17f 100644
--- a/cache.h
+++ b/cache.h
@@ -1376,6 +1376,7 @@ struct object_context {
 #define GET_OID_RECORD_PATH     0200
 #define GET_OID_ONLY_TO_DIE    04000
 #define GET_OID_REQUIRE_PATH  010000
+#define GET_OID_GENTLY        020000
 
 #define GET_OID_DISAMBIGUATORS \
 	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index 92862eeb1a..46dbfe36a6 100644
--- a/object-name.c
+++ b/object-name.c
@@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 						len, str,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
-			} else {
+			} else if (!(flags & GET_OID_GENTLY)) {
 				if (flags & GET_OID_QUIETLY) {
 					exit(128);
 				}
 				die(_("log for '%.*s' only has %d entries"),
 				    len, str, co_cnt);
 			}
+			if (flags & GET_OID_GENTLY) {
+				free(real_ref);
+				return -1;
+			}
 		}
 	}
 

[PATCH v3 2/4] builtin/stash: factor out revision parsing into a function

[brian m. carlson]

We allow several special forms of stash names in this code.  In the
future, we'll want to allow these same forms without parsing a stash
commit, so let's refactor this code out into a function for reuse.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 5897febfbe..4c281a5781 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -130,6 +130,24 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
 		die(_("'%s' is not a stash-like commit"), revision);
 }
 
+static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
+{
+	strbuf_init(revision, 0);
+	if (!commit) {
+		if (!ref_exists(ref_stash)) {
+			fprintf_ln(stderr, _("No stash entries found."));
+			return -1;
+		}
+
+		strbuf_addf(revision, "%s@{0}", ref_stash);
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(revision, "%s@{%s}", ref_stash, commit);
+	} else {
+		strbuf_addstr(revision, commit);
+	}
+	return 0;
+}
+
 static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 {
 	int ret;
@@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	if (argc == 1)
 		commit = argv[0];
 
-	strbuf_init(&info->revision, 0);
-	if (!commit) {
-		if (!ref_exists(ref_stash)) {
-			free_stash_info(info);
-			fprintf_ln(stderr, _("No stash entries found."));
-			return -1;
-		}
-
-		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
-	} else if (strspn(commit, "0123456789") == strlen(commit)) {
-		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
-	} else {
-		strbuf_addstr(&info->revision, commit);
+	if (parse_revision(&info->revision, commit, 0)) {
+		free_stash_info(info);
+		return -1;
 	}
 
 	revision = info->revision.buf;

Re: [PATCH v3 2/4] builtin/stash: factor out revision parsing into a function

[Phillip Wood]

Hi brian

On 03/04/2022 19:22, brian m. carlson wrote:
> We allow several special forms of stash names in this code.  In the
> future, we'll want to allow these same forms without parsing a stash
> commit, so let's refactor this code out into a function for reuse.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>   builtin/stash.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 5897febfbe..4c281a5781 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -130,6 +130,24 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
>   		die(_("'%s' is not a stash-like commit"), revision);
>   }
>   
> +static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
> +{
> +	strbuf_init(revision, 0);

I think this should become strbuf_reset() and the caller should call 
strbuf_init() once before calling this function (or use STASH_INFO_INIT 
from ab/plug-leak-in-revisions). That should fix one of the leaks Ævar 
was talking about, otherwise we reallocate the strbuf each time this 
function is called and leak the previous allocation.

Best Wishes

Phillip

> +	if (!commit) {
> +		if (!ref_exists(ref_stash)) {
> +			fprintf_ln(stderr, _("No stash entries found."));
> +			return -1;
> +		}
> +
> +		strbuf_addf(revision, "%s@{0}", ref_stash);
> +	} else if (strspn(commit, "0123456789") == strlen(commit)) {
> +		strbuf_addf(revision, "%s@{%s}", ref_stash, commit);
> +	} else {
> +		strbuf_addstr(revision, commit);
> +	}
> +	return 0;
> +}
> +
>   static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>   {
>   	int ret;
> @@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>   	if (argc == 1)
>   		commit = argv[0];
>   
> -	strbuf_init(&info->revision, 0);
> -	if (!commit) {
> -		if (!ref_exists(ref_stash)) {
> -			free_stash_info(info);
> -			fprintf_ln(stderr, _("No stash entries found."));
> -			return -1;
> -		}
> -
> -		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
> -	} else if (strspn(commit, "0123456789") == strlen(commit)) {
> -		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
> -	} else {
> -		strbuf_addstr(&info->revision, commit);
> +	if (parse_revision(&info->revision, commit, 0)) {
> +		free_stash_info(info);
> +		return -1;
>   	}
>   
>   	revision = info->revision.buf;

[PATCH v3 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

A common user problem is how to sync in-progress work to another
machine.  Users currently must use some sort of transfer of the working
tree, which poses security risks and also necessarily causes the index
to become dirty.  The experience is suboptimal and frustrating for
users.

A reasonable idea is to use the stash for this purpose, but the stash is
stored in the reflog, not in a ref, and as such it cannot be pushed or
pulled.  This also means that it cannot be saved into a bundle or
preserved elsewhere, which is a problem when using throwaway development
environments.

Let's solve this problem by allowing the user to export the stash to a
ref (or, to just write it into the repository and print the hash, à la
git commit-tree).  Introduce git stash export, which writes a chain of
commits where the first parent is always a chain to the previous stash,
or to a single, empty commit (for the final item) and the second is the
stash commit normally written to the reflog.

Iterate over each stash from topmost to bottomost, looking up the data
for each one, and then create the chain from the single empty commit
back up in reverse order.  Generate a predictable empty commit so our
behavior is reproducible.  Create a useful commit message, preserving
the author and committer information, to help users identify stash
commits when viewing them as normal commits.

If the user has specified specific stashes they'd like to export
instead, use those instead of iterating over all of the stashes.

As part of this, specifically request quiet behavior when looking up the
OID for a revision because we will eventually hit a revision that
doesn't exist and we don't want to die when that occurs.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |  22 ++++-
 builtin/stash.c             | 184 ++++++++++++++++++++++++++++++++++++
 2 files changed, 205 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 6e15f47525..162110314e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 'git stash' clear
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
+'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
 
 DESCRIPTION
 -----------
@@ -151,6 +152,12 @@ store::
 	reflog.  This is intended to be useful for scripts.  It is
 	probably not the command you want to use; see "push" above.
 
+export ( --print | --to-ref <ref> ) [<stash>...]::
+
+	Export the specified stashes, or all of them if none are specified, to
+	a chain of commits which can be transferred using the normal fetch and
+	push mechanisms, then imported using the `import` subcommand.
+
 OPTIONS
 -------
 -a::
@@ -239,6 +246,19 @@ literally (including newlines and quotes).
 +
 Quiet, suppress feedback messages.
 
+--print::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes without
+storing it anywhere in the ref namespace and print the object ID to
+standard output.  This is designed for scripts.
+
+--to-ref::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes and store
+it to the specified ref.
+
 \--::
 	This option is only valid for `push` command.
 +
@@ -256,7 +276,7 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 <stash>::
 	This option is only valid for `apply`, `branch`, `drop`, `pop`,
-	`show` commands.
+	`show`, and `export` commands.
 +
 A reference of the form `stash@{<revision>}`. When no `<stash>` is
 given, the latest stash is assumed (that is, `stash@{0}`).
diff --git a/builtin/stash.c b/builtin/stash.c
index 4c281a5781..89e22d0cdd 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -33,6 +33,7 @@ static const char * const git_stash_usage[] = {
 	   "          [--] [<pathspec>...]]"),
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
 	NULL
 };
 
@@ -89,6 +90,12 @@ static const char * const git_stash_save_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_export_usage[] = {
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	NULL
+};
+
+
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -1773,6 +1780,181 @@ static int save_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
+{
+	size_t author_len, committer_len;
+	struct commit *this;
+	const char *orig_author, *orig_committer;
+	char *author = NULL, *committer = NULL;
+	const char *buffer;
+	unsigned long bufsize;
+	const char *p;
+	struct strbuf msg = STRBUF_INIT;
+	int ret = 0;
+
+	this = lookup_commit_reference(the_repository, oid);
+	buffer = get_commit_buffer(this, &bufsize);
+	orig_author = find_commit_header(buffer, "author", &author_len);
+	orig_committer = find_commit_header(buffer, "committer", &committer_len);
+	p = memmem(buffer, bufsize, "\n\n", 2);
+
+	if (!orig_author || !orig_committer || !p) {
+		error(_("cannot parse commit %s"), oid_to_hex(oid));
+		goto out;
+	}
+	/* Jump to message. */
+	p += 2;
+	strbuf_addstr(&msg, "git stash: ");
+	strbuf_add(&msg, p, bufsize - (p - buffer));
+
+	author = xmemdupz(orig_author, author_len);
+	committer = xmemdupz(orig_committer, committer_len);
+
+	if (commit_tree_extended(msg.buf, msg.len,
+				 the_hash_algo->empty_tree, parents,
+				 out, author, committer,
+				 NULL, NULL)) {
+		ret = -1;
+		error(_("could not write commit"));
+		goto out;
+	}
+out:
+	strbuf_reset(&msg);
+	unuse_commit_buffer(this, buffer);
+	free(author);
+	free(committer);
+	return ret;
+}
+
+static int do_export_stash(const char *ref, size_t argc, const char **argv)
+{
+	struct object_id base;
+	struct object_context unused;
+	struct commit *prev;
+	struct object_id *items = NULL;
+	int nitems = 0, nalloc = 0;
+	int res = 0;
+	int i;
+	struct strbuf revision = STRBUF_INIT;
+	const char *author, *committer;
+
+	/*
+	 * This is an arbitrary, fixed date, specifically the one used by git
+	 * format-patch.  The goal is merely to produce reproducible output.
+	 */
+	prepare_fallback_ident("git stash", "git@stash");
+	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
+	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT, "2001-09-17T00:00:00Z", 0);
+
+	/* First, we create a single empty commit. */
+	if (commit_tree_extended(NULL, 0, the_hash_algo->empty_tree, NULL, &base, author, committer, NULL, NULL))
+		return error(_("unable to write base commit"));
+
+	prev = lookup_commit_reference(the_repository, &base);
+
+	if (argc) {
+		/*
+		 * Find each specified stash, and load data into the array.
+		 */
+		for (i = 0; i < argc; i++) {
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			if (parse_revision(&revision, argv[i], 0) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &items[i], &unused)) {
+				error(_("unable to find stash entry %s"), argv[i]);
+				res = -1;
+				goto out;
+			}
+		}
+	} else {
+		/*
+		 * Walk the reflog, finding each stash entry, and load data into the
+		 * array.
+		 */
+		for (i = 0;; i++) {
+			char buf[32];
+			struct object_id oid;
+
+			snprintf(buf, sizeof(buf), "%d", i);
+			if (parse_revision(&revision, buf, 1) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &oid, &unused))
+				break;
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			oidcpy(&items[i], &oid);
+		}
+	}
+
+	/*
+	 * Now, create a set of commits identical to the regular stash commits,
+	 * but where their first parents form a chain to our original empty
+	 * base commit.
+	 */
+	for (i = nitems - 1; i >= 0; i--) {
+		struct commit_list *parents = NULL;
+		struct commit_list **next = &parents;
+		struct object_id out;
+
+		next = commit_list_append(prev, next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
+		if (write_commit_with_parents(&out, &items[i], parents)) {
+			res = -1;
+			goto out;
+		}
+		prev = lookup_commit_reference(the_repository, &out);
+	}
+	if (ref)
+		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+	else
+		puts(oid_to_hex(&prev->object.oid));
+out:
+	free(items);
+
+	return res;
+}
+
+enum export_action {
+	ACTION_NONE,
+	ACTION_PRINT,
+	ACTION_TO_REF,
+};
+
+static int export_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	const char *ref = NULL;
+	enum export_action action = ACTION_NONE;
+	struct option options[] = {
+		OPT_CMDMODE(0, "print", &action,
+			    N_("print the object ID instead of writing it to a ref"),
+			    ACTION_PRINT),
+		OPT_CMDMODE(0, "to-ref", &action,
+			    N_("save the data to the given ref"),
+			    ACTION_TO_REF),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_export_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (action == ACTION_NONE) {
+		return error(_("exactly one of --print and --to-ref is required"));
+	} else if (action == ACTION_TO_REF) {
+		if (!argc)
+			return error(_("--to-ref requires an argument"));
+		ref = argv[0];
+		argc--;
+		argv++;
+	}
+
+
+	ret = do_export_stash(ref, argc, argv);
+	return ret;
+}
+
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
 	pid_t pid = getpid();
@@ -1816,6 +1998,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!push_stash(argc, argv, prefix, 0);
 	else if (!strcmp(argv[0], "save"))
 		return !!save_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "export"))
+		return !!export_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);

Re: [PATCH v3 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 03 2022, brian m. carlson wrote:

> [...snip...]

Per
https://lore.kernel.org/git/220404.86tub9jql5.gmgdl@evledraar.gmail.com/
there's a bug here, in the preceding commit you:
	
	+static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	+{
	+	strbuf_init(revision, 0);
	[...]
	 static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 {
	 	int ret;
	@@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	strbuf_init(&info->revision, 0);
	-	if (!commit) {
	-		if (!ref_exists(ref_stash)) {
	[...]
	+	if (parse_revision(&info->revision, commit, 0)) {
	+		free_stash_info(info);
	+		return -1;
	 	}

That caller wasn't dealing with an init'd "info", so we init'd it late
and freed it (which my revisions_release() makes a lot less icky). But
unrelated to that:
	
> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (i = 0; i < argc; i++) {
> +			ALLOC_GROW_BY(items, nitems, 1, nalloc);
> +			if (parse_revision(&revision, argv[i], 0) ||

Here...

> +			    get_oid_with_context(the_repository, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &items[i], &unused)) {
> +				error(_("unable to find stash entry %s"), argv[i]);
> +				res = -1;
> +				goto out;
> +			}
> +		}
> +	} else {
> +		/*
> +		 * Walk the reflog, finding each stash entry, and load data into the
> +		 * array.
> +		 */
> +		for (i = 0;; i++) {
> +			char buf[32];
> +			struct object_id oid;
> +
> +			snprintf(buf, sizeof(buf), "%d", i);
> +			if (parse_revision(&revision, buf, 1) ||

...and here you are leaking memory in a loop. Per the above you need a
strbuf_reset() for both.

Then (moved from earlier):

> +static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;

This is a leak, because:

> +out:
> +	strbuf_reset(&msg);

This should be strbuf_release(), not strbuf_reset().

Anyway, even if builtin/stash.c is currently a big fail whale it's still
useful to test new code with SANITIZE=leak, e.g. (and most of this is a
lot less painful on top of my in-flight topic) on top of yours you can
try:

        printf "leak:%s\n" setup_revisions add_object_array_with_path \
        	add_rev_cmdline cmd_log_init_finish strvec_push \
                get_untracked_files pump_io_round setup_rename_conflict_info parse_pathspec \
                prep_parse_options init_reflog_walk >/tmp/supp.txt;
	LSAN_OPTIONS=print_suppressions=0:suppressions=/tmp/supp.txt ./t3903-stash.sh  -vixd

Where that printf is just something I came up with via trial & error by
re-running the test with -vixd and quieting existing leaks, until
getting to the first leak new in your topic.

[PATCH v3 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

Now that we have a way to export stashes to a ref, let's provide a way
to import them from such a ref back to the stash.  This works much the
way the export code does, except that we strip off the first parent
chain commit and then store each resulting commit back to the stash.

We don't clear the stash first and instead add the specified stashes to
the top of the stash.  This is because users may want to export just a
few stashes, such as to share a small amount of work in progress with a
colleague, and it would be undesirable for the receiving user to lose
all of their data.  For users who do want to replace the stash, it's
easy to do to: simply run "git stash clear" first.

We specifically rely on the fact that we'll produce identical stash
commits on both sides in our tests.  This provides a cheap,
straightforward check for our tests and also makes it easy for users to
see if they already have the same data in both repositories.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |   7 +++
 builtin/stash.c             | 108 ++++++++++++++++++++++++++++++++++++
 t/t3903-stash.sh            |  52 +++++++++++++++++
 3 files changed, 167 insertions(+)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 162110314e..28eb9cab0c 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -21,6 +21,7 @@ SYNOPSIS
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
+'git stash' import <commit>
 
 DESCRIPTION
 -----------
@@ -158,6 +159,12 @@ export ( --print | --to-ref <ref> ) [<stash>...]::
 	a chain of commits which can be transferred using the normal fetch and
 	push mechanisms, then imported using the `import` subcommand.
 
+import <commit>::
+
+	Import the specified stashes from the specified commit, which must have been
+	created by `export`, and add them to the list of stashes.  To replace the
+	existing stashes, use `clear` first.
+
 OPTIONS
 -------
 -a::
diff --git a/builtin/stash.c b/builtin/stash.c
index 89e22d0cdd..93b1a996c4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -34,6 +34,7 @@ static const char * const git_stash_usage[] = {
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
 	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	N_("git stash import <commit>"),
 	NULL
 };
 
@@ -95,6 +96,10 @@ static const char * const git_stash_export_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_import_usage[] = {
+	N_("git stash import <commit>"),
+	NULL
+};
 
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
  * 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
+ * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
  * w_tree is set to the working tree
  * b_tree is set to the base tree
  * i_tree is set to the index tree
@@ -114,6 +120,7 @@ struct stash_info {
 	struct object_id b_commit;
 	struct object_id i_commit;
 	struct object_id u_commit;
+	struct object_id c_commit;
 	struct object_id w_tree;
 	struct object_id b_tree;
 	struct object_id i_tree;
@@ -1826,6 +1833,105 @@ static int write_commit_with_parents(struct object_id *out, const struct object_
 	return ret;
 }
 
+static int do_import_stash(const char *rev)
+{
+	struct object_id chain;
+	size_t nalloc = 0;
+	struct object_id *items = NULL;
+	int nitems = 0;
+	int res = 0;
+	int i;
+	const char *buffer = NULL;
+	struct commit *this = NULL;
+	char *msg = NULL;
+
+	if (get_oid(rev, &chain))
+		return error(_("not a valid revision: %s"), rev);
+
+	/*
+	 * Walk the commit history, finding each stash entry, and load data into
+	 * the array.
+	 */
+	for (i = 0;; i++) {
+		struct object_id tree, oid;
+		char revision[GIT_MAX_HEXSZ + 1];
+
+		oid_to_hex_r(revision, &chain);
+
+		if (get_oidf(&tree, "%s:", revision) ||
+		    !oideq(&tree, the_hash_algo->empty_tree)) {
+			return error(_("%s is not a valid exported stash commit"), revision);
+		}
+		if (get_oidf(&chain, "%s^1", revision) ||
+		    get_oidf(&oid, "%s^2", revision))
+			break;
+		ALLOC_GROW_BY(items, nitems, 1, nalloc);
+		oidcpy(&items[i], &oid);
+	}
+
+	/*
+	 * Now, walk each entry, adding it to the stash as a normal stash
+	 * commit.
+	 */
+	for (i = nitems - 1; i >= 0; i--) {
+		unsigned long bufsize;
+		const char *p;
+
+		this = lookup_commit_reference(the_repository, &items[i]);
+		buffer = get_commit_buffer(this, &bufsize);
+		if (!buffer) {
+			res = -1;
+			error(_("cannot read commit buffer for %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+
+		p = memmem(buffer, bufsize, "\n\n", 2);
+		if (!p) {
+			res = -1;
+			error(_("cannot parse commit %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+
+		p += 2;
+		msg = xmemdupz(p, bufsize - (p - buffer));
+		unuse_commit_buffer(this, buffer);
+		buffer = NULL;
+
+		if (do_store_stash(&items[i], msg, 1)) {
+			res = -1;
+			error(_("cannot save the stash for %s"), oid_to_hex(&items[i]));
+			goto out;
+		}
+		FREE_AND_NULL(msg);
+	}
+out:
+	if (this && buffer)
+		unuse_commit_buffer(this, buffer);
+	free(items);
+	free(msg);
+
+	return res;
+}
+
+static int import_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret = 0;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_import_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (argc != 1)
+		return error(_("a revision to import from is required"));
+
+
+	ret = do_import_stash(argv[0]);
+	return ret;
+}
+
 static int do_export_stash(const char *ref, size_t argc, const char **argv)
 {
 	struct object_id base;
@@ -2000,6 +2106,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!save_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "export"))
 		return !!export_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "import"))
+		return !!import_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..d2ddede9be 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1295,6 +1295,58 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash export and import round-trip stashes' '
+	git reset &&
+	>untracked &&
+	>tracked1 &&
+	>tracked2 &&
+	git add tracked* &&
+	git stash -- &&
+	>subdir/untracked &&
+	>subdir/tracked1 &&
+	>subdir/tracked2 &&
+	git add subdir/tracked* &&
+	git stash -- subdir/ &&
+	stash0=$(git rev-parse --verify stash@{0}) &&
+	stash1=$(git rev-parse --verify stash@{1}) &&
+	simple=$(git stash export --print) &&
+	git stash clear &&
+	git stash import "$simple" &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1" &&
+	git stash export --to-ref refs/heads/foo &&
+	git stash clear &&
+	git stash import foo &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported0" = "$stash0" &&
+	test "$imported1" = "$stash1"
+'
+
+test_expect_success 'stash import appends commits' '
+	git log --format=oneline -g refs/stash >actual &&
+	echo $(cat actual | wc -l) >count &&
+	git stash import refs/heads/foo &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = $(($(cat count) * 2)) actual
+'
+
+test_expect_success 'stash export can accept specified stashes' '
+	git stash clear &&
+	git stash import foo &&
+	git stash export --to-ref bar stash@{1} stash@{0} &&
+	git stash clear &&
+	git stash import bar &&
+	imported0=$(git rev-parse --verify stash@{0}) &&
+	imported1=$(git rev-parse --verify stash@{1}) &&
+	test "$imported1" = "$stash0" &&
+	test "$imported0" = "$stash1" &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = 2 actual
+'
+
 test_expect_success 'stash apply should succeed with unmodified file' '
 	echo base >file &&
 	git add file &&

Re: [PATCH v3 4/4] builtin/stash: provide a way to import stashes from a ref

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 03 2022, brian m. carlson wrote:


> +	struct object_id *items = NULL;

Is there a reason for why this...

> [...]
> +		ALLOC_GROW_BY(items, nitems, 1, nalloc);
> +		oidcpy(&items[i], &oid);
> [...]
> +	for (i = nitems - 1; i >= 0; i--) {
> [...]
> +		this = lookup_commit_reference(the_repository, &items[i]);
> +	free(msg);

...can't just use the existing "oid_array" APi?

> [...]
> +static int import_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int ret = 0;
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_import_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (argc != 1)
> +		return error(_("a revision to import from is required"));

I think you want to use usage_with_options() here instead.

In any case, I think you can de-init "ret" above I think, as the
compiler will then spot a future logic error if we don't reach this:

> +	ret = do_import_stash(argv[0]);
> +	return ret;
> +}
> +
>  static int do_export_stash(const char *ref, size_t argc, const char **argv)
>  {
>  	struct object_id base;
> @@ -2000,6 +2106,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  		return !!save_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "export"))
>  		return !!export_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "import"))
> +		return !!import_stash(argc, argv, prefix);
>  	else if (*argv[0] != '-')
>  		usage_msg_optf(_("unknown subcommand: %s"),
>  			       git_stash_usage, options, argv[0]);
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index b149e2af44..d2ddede9be 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1295,6 +1295,58 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
>  	test_path_is_missing to-remove
>  '
>  
> +test_expect_success 'stash export and import round-trip stashes' '
> +	git reset &&
> +	>untracked &&
> +	>tracked1 &&
> +	>tracked2 &&
> +	git add tracked* &&
> +	git stash -- &&
> +	>subdir/untracked &&
> +	>subdir/tracked1 &&
> +	>subdir/tracked2 &&
> +	git add subdir/tracked* &&
> +	git stash -- subdir/ &&
> +	stash0=$(git rev-parse --verify stash@{0}) &&
> +	stash1=$(git rev-parse --verify stash@{1}) &&
> +	simple=$(git stash export --print) &&
> +	git stash clear &&
> +	git stash import "$simple" &&
> +	imported0=$(git rev-parse --verify stash@{0}) &&
> +	imported1=$(git rev-parse --verify stash@{1}) &&
> +	test "$imported0" = "$stash0" &&
> +	test "$imported1" = "$stash1" &&
> +	git stash export --to-ref refs/heads/foo &&
> +	git stash clear &&
> +	git stash import foo &&
> +	imported0=$(git rev-parse --verify stash@{0}) &&
> +	imported1=$(git rev-parse --verify stash@{1}) &&
> +	test "$imported0" = "$stash0" &&
> +	test "$imported1" = "$stash1"

This whole variable assignment/test/manual rev-parse would be better as
just feeding tags you created earlier to test_cmp_rev, should be a lot
fewer lines that way, I.e. these last 4 lines become:

	git tag t-stash0 # earlier
	test_cmp_rev t-stash0 stash@{0} &&
	test_cmp_rev t-stash stash@{1}

Perhaps adding N files in one commit isn't critical, then you could even
piggy-back on "test_commit"....

> +test_expect_success 'stash import appends commits' '
> +	git log --format=oneline -g refs/stash >actual &&
> +	echo $(cat actual | wc -l) >count &&

Hrm...

> +	git stash import refs/heads/foo &&
> +	git log --format=oneline -g refs/stash >actual &&
> +	test_line_count = $(($(cat count) * 2)) actual

FWIW I think I'd save myself the trivial disk space here and less shell
trickery with:

	git log >out &&
	cat out out >out2

Then:

	test_line_count = $(wc -l <out2) actual

Or just:

	test_line_count = $(cat log-out log-out | wc -l) actual

I.e. parts of this are presumably working around the $(()) operation not
jiving with a whitespace-padded $count, so you're doing the echo instead
of a more obvious redirection to avoid that.

Which, I'd think is made more obvious by just cat-ing the earlier output
twice. Just my 0.02...

> +test_expect_success 'stash export can accept specified stashes' '
> +	git stash clear &&

This looks like it belongs as a test_when_finished line of an earlier
test that should be cleaning up after itself.

> +	git stash import foo &&
> +	git stash export --to-ref bar stash@{1} stash@{0} &&
> +	git stash clear &&
> +	git stash import bar &&
> +	imported0=$(git rev-parse --verify stash@{0}) &&
> +	imported1=$(git rev-parse --verify stash@{1}) &&
> +	test "$imported1" = "$stash0" &&
> +	test "$imported0" = "$stash1" &&

This test has an implicit dependency on your earlier test and will break
if it's not defining stash0, e.g. if you use --run=N or use other skip
test features of test-lib.sh.

Just factor that into a setup function & have the rest call it?

Re: [PATCH v3 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]

On 2022-04-04 at 10:38:53, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Apr 03 2022, brian m. carlson wrote:
> 
> 
> > +	struct object_id *items = NULL;
> 
> Is there a reason for why this...
> 
> ...can't just use the existing "oid_array" APi?

None in particular off the top of my head, except that it didn't
originally.

> I think you want to use usage_with_options() here instead.
> 
> In any case, I think you can de-init "ret" above I think, as the
> compiler will then spot a future logic error if we don't reach this:

Sure, I can do that.

> This whole variable assignment/test/manual rev-parse would be better as
> just feeding tags you created earlier to test_cmp_rev, should be a lot
> fewer lines that way, I.e. these last 4 lines become:
> 
> 	git tag t-stash0 # earlier
> 	test_cmp_rev t-stash0 stash@{0} &&
> 	test_cmp_rev t-stash stash@{1}

Yeah, I think this would be a nice improvement.

> Perhaps adding N files in one commit isn't critical, then you could even
> piggy-back on "test_commit"....

I don't think test_commit works here because stash only operates on
things that are not committed.  The original goal here was to ensure
that we round-tripped the untracked files.  Since the design has
changed, that's not as critical, but I still think it's a useful thing
to check.

> FWIW I think I'd save myself the trivial disk space here and less shell
> trickery with:
> 
> 	git log >out &&
> 	cat out out >out2
> 
> Then:
> 
> 	test_line_count = $(wc -l <out2) actual
> 
> Or just:
> 
> 	test_line_count = $(cat log-out log-out | wc -l) actual
> 
> I.e. parts of this are presumably working around the $(()) operation not
> jiving with a whitespace-padded $count, so you're doing the echo instead
> of a more obvious redirection to avoid that.
> 
> Which, I'd think is made more obvious by just cat-ing the earlier output
> twice. Just my 0.02...

Your assumption is correct, and I can make that change.

> > +test_expect_success 'stash export can accept specified stashes' '
> > +	git stash clear &&
> 
> This looks like it belongs as a test_when_finished line of an earlier
> test that should be cleaning up after itself.

Not really.  We don't clear the stashes elsewhere in the test.  In fact,
last I looked, we had about 25 of them by this point in the test.

> > +	git stash import foo &&
> > +	git stash export --to-ref bar stash@{1} stash@{0} &&
> > +	git stash clear &&
> > +	git stash import bar &&
> > +	imported0=$(git rev-parse --verify stash@{0}) &&
> > +	imported1=$(git rev-parse --verify stash@{1}) &&
> > +	test "$imported1" = "$stash0" &&
> > +	test "$imported0" = "$stash1" &&
> 
> This test has an implicit dependency on your earlier test and will break
> if it's not defining stash0, e.g. if you use --run=N or use other skip
> test features of test-lib.sh.
> 
> Just factor that into a setup function & have the rest call it?

Yes, most of our tests have that problem.  I don't think it's worth
changing the way we do things unless we plan to make a concerted effort
to do that across the codebase and verify that in CI.

If we really want to make that change across the codebase for the
future, that's fine, but I haven't seen such a discussion on the list so
far.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v3 4/4] builtin/stash: provide a way to import stashes from a ref

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, brian m. carlson wrote:

>> This test has an implicit dependency on your earlier test and will break
>> if it's not defining stash0, e.g. if you use --run=N or use other skip
>> test features of test-lib.sh.
>> 
>> Just factor that into a setup function & have the rest call it?
>
> Yes, most of our tests have that problem.  I don't think it's worth
> changing the way we do things unless we plan to make a concerted effort
> to do that across the codebase and verify that in CI.
>
> If we really want to make that change across the codebase for the
> future, that's fine, but I haven't seen such a discussion on the list so
> far.

Fair enough.

I do think it's a good pattern when adding /new/ code to follow that
practice, even if it requires the first added test to have a "git reset
--hard" (in liue of fixing various tests above).

But not enough to quibbly about it here :) Thanks.

Re: [PATCH v3 0/4] Importing and exporting stashes to refs

[Junio C Hamano]

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

> Changes from v2:
> * Fix uninitialized strbuf.
> * Avoid C99-style initializations.

Thanks.

[1] is a CI run of 'seen' without this topic, while [2] is the same
but with this topic.

t3903.115-117 (stash export) are not very happy in the latter.

e.g. https://github.com/git/git/runs/5808828105?check_suite_focus=true#step:7:6623

[References]

*1* https://github.com/git/git/actions/runs/2086776970 
*2* https://github.com/git/git/actions/runs/2086887176 

Re: [PATCH v3 0/4] Importing and exporting stashes to refs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> Changes from v2:
>> * Fix uninitialized strbuf.
>> * Avoid C99-style initializations.
>
> Thanks.
>
> [1] is a CI run of 'seen' without this topic, while [2] is the same
> but with this topic.
>
> t3903.115-117 (stash export) are not very happy in the latter.
>
> e.g. https://github.com/git/git/runs/5808828105?check_suite_focus=true#step:7:6623
>
> [References]
>
> *1* https://github.com/git/git/actions/runs/2086776970 
> *2* https://github.com/git/git/actions/runs/2086887176 

I think this is coming from a mismerge, conflicting topic being
Ævar's revisions leakage series.  I'll push out an updated
resolution later today.

Thanks.

Re: [PATCH v3 0/4] Importing and exporting stashes to refs

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 03 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>>> Changes from v2:
>>> * Fix uninitialized strbuf.
>>> * Avoid C99-style initializations.
>>
>> Thanks.
>>
>> [1] is a CI run of 'seen' without this topic, while [2] is the same
>> but with this topic.
>>
>> t3903.115-117 (stash export) are not very happy in the latter.
>>
>> e.g. https://github.com/git/git/runs/5808828105?check_suite_focus=true#step:7:6623
>>
>> [References]
>>
>> *1* https://github.com/git/git/actions/runs/2086776970 
>> *2* https://github.com/git/git/actions/runs/2086887176 
>
> I think this is coming from a mismerge, conflicting topic being
> Ævar's revisions leakage series.  I'll push out an updated
> resolution later today.

It looks like it, I looked at your resolution. It seems that since the
failure you pushed out this on top:
	
	diff --git a/builtin/stash.c b/builtin/stash.c
	index 35d7c2e828b..b4da17265a1 100644
	--- a/builtin/stash.c
	+++ b/builtin/stash.c
	@@ -150,6 +150,7 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
	 
	 static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	 {
	+	strbuf_init(revision, 0);
	 	if (!commit) {
	 		if (!ref_exists(ref_stash)) {
	 			fprintf_ln(stderr, _("No stash entries found."));
	@@ -192,8 +193,10 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	if (parse_revision(&info->revision, commit, 0))
	+	if (parse_revision(&info->revision, commit, 0)) {
	+		free_stash_info(info);
	 		return -1;
	+	}
	 
	 	revision = info->revision.buf;

Which per my [1] is odd. I.e. in my topic I'd gotten rid of that
strbuf_init() entirely, see [2].

But looking again the odd interaction with brian's topic is that in his
[2] and [3] he refactored the parse_revision() into existance.

In the version he based things on we always init'd the strbuf there, but
between [2] and [3] that "strbuf_init()" was tasked with the work of a
strbuf_reset().

And on "seen" this (new code) still leaks:
	
	$ ~/g/git/git stash export --print
	6b0514ae1f670e55f450518c4b4421a997c8d082
	
	=================================================================
	==25882==ERROR: LeakSanitizer: detected memory leaks
	
	Direct leak of 910 byte(s) in 14 object(s) allocated from:
	    #0 0x45658d in __interceptor_realloc (/home/avar/g/git/git+0x45658d)
	    #1 0x763f6d in xrealloc /home/avar/g/git/wrapper.c:136:8
	    #2 0x71c031 in strbuf_grow /home/avar/g/git/strbuf.c:99:2
	    #3 0x71d224 in strbuf_vaddf /home/avar/g/git/strbuf.c:395:3
	    #4 0x71d1e1 in strbuf_addf /home/avar/g/git/strbuf.c:336:2
	    #5 0x546f3e in parse_revision /home/avar/g/git/builtin/stash.c:162:3
	    #6 0x547f99 in do_export_stash /home/avar/g/git/builtin/stash.c:1987:8
	    #7 0x543b2b in export_stash /home/avar/g/git/builtin/stash.c:2061:8
	    #8 0x541846 in cmd_stash /home/avar/g/git/builtin/stash.c:2109:12
	    #9 0x45a38a in run_builtin /home/avar/g/git/git.c:466:11
	    #10 0x458e21 in handle_builtin /home/avar/g/git/git.c:720:3
	    #11 0x459d65 in run_argv /home/avar/g/git/git.c:787:4
	    #12 0x458bda in cmd_main /home/avar/g/git/git.c:920:19
	    #13 0x567839 in main /home/avar/g/git/common-main.c:56:11
	    #14 0x7fb9a380f7ec in __libc_start_main csu/../csu/libc-start.c:332:16
	    #15 0x431119 in _start (/home/avar/g/git/git+0x431119)

We just don't test t3903 under linux-leaks yet.

Anyway. I belive that the correct resolution is (but see caveats below):
	
	diff --git a/builtin/stash.c b/builtin/stash.c
	index b4da17265a1..3b9a6c5b753 100644
	--- a/builtin/stash.c
	+++ b/builtin/stash.c
	@@ -150,7 +150,6 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
	 
	 static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	 {
	-	strbuf_init(revision, 0);
	 	if (!commit) {
	 		if (!ref_exists(ref_stash)) {
	 			fprintf_ln(stderr, _("No stash entries found."));
	@@ -193,10 +192,8 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	if (parse_revision(&info->revision, commit, 0)) {
	-		free_stash_info(info);
	+	if (parse_revision(&info->revision, commit, 0))
	 		return -1;
	-	}
	 
	 	revision = info->revision.buf;
	 
	@@ -1827,7 +1824,7 @@ static int write_commit_with_parents(struct object_id *out, const struct object_
	 		goto out;
	 	}
	 out:
	-	strbuf_reset(&msg);
	+	strbuf_release(&msg);
	 	unuse_commit_buffer(this, buffer);
	 	free(author);
	 	free(committer);
	@@ -1965,6 +1962,7 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 		 */
	 		for (i = 0; i < argc; i++) {
	 			ALLOC_GROW_BY(items, nitems, 1, nalloc);
	+			strbuf_reset(&revision);
	 			if (parse_revision(&revision, argv[i], 0) ||
	 			    get_oid_with_context(the_repository, revision.buf,
	 						 GET_OID_QUIETLY | GET_OID_GENTLY,
	@@ -1983,6 +1981,8 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 			char buf[32];
	 			struct object_id oid;
	 
	+			strbuf_reset(&revision);
	+
	 			snprintf(buf, sizeof(buf), "%d", i);
	 			if (parse_revision(&revision, buf, 1) ||
	 			    get_oid_with_context(the_repository, revision.buf,
	@@ -2018,6 +2018,7 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 		puts(oid_to_hex(&prev->object.oid));
	 out:
	 	free(items);
	+	strbuf_release(&revision);
	 
	 	return res;
	 }

I.e. most of that is really feedback/a bug report on [3] and [4],and the
third hunk here is really an unrelated fix where [4] conflated
strbuf_reset() with strbuf_release().

1. https://lore.kernel.org/git/cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v5-10.27-4c5404912e9-20220402T102002Z-avarab@gmail.com/
2. https://lore.kernel.org/git/20220403182250.904933-3-sandals@crustytoothpaste.net/
4. https://lore.kernel.org/git/20220403182250.904933-4-sandals@crustytoothpaste.net/

Re: [PATCH v3 0/4] Importing and exporting stashes to refs

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On 2022-04-04 at 06:20:29, Ævar Arnfjörð Bjarmason wrote:
> I.e. most of that is really feedback/a bug report on [3] and [4],and the
> third hunk here is really an unrelated fix where [4] conflated
> strbuf_reset() with strbuf_release().

Your resolutions seem reasonable.  And yes, I always conflate
strbuf_reset and strbuf_release.

I'll put out a v4 with changes to my series and responses to some other
feedback I've received.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v4 0/4] Importing and exporting stashes to refs

[brian m. carlson]

Stashes are currently stored using the reflog in a given repository.
This is an interesting and novel way to handle them, but there is no way
to easily move a stash across machines.  For example, stashes cannot be
bundled, pushed, or fetched.

Let's solve this problem by allowing users to import and export stashes
to a chain of commits.  The commits used in a stash export contain two
parents: one which is the pointer to the next exported stash (or to an
empty commit with no parents if there are no more) and the second is the
stash commit that would normally be stored in the reflog.

Changes from v3:
* Fix strbuf handling to avoid leaks and generally be more sensible.
* Make use of the error return code more often.
* Use oid_array.
* Tidy various parts of the code and fix long lines.
* Simplify tests using git tag.
* Shorten and tidy tests.
* Add an additional test covering the base commit OID and importing and
  exporting empty stashes.

Changes from v2:
* Fix uninitialized strbuf.
* Avoid C99-style initializations.

Changes from v1:
* Change storage format as suggested by Junio.
* Rename to GIT_OID_GENTLY.
* Remove unnecessary initializations.
* Use ALLOC_GROW_BY.
* Ensure completely reproducible exports.
* Avoid size_t.
* Various other code cleanups.

brian m. carlson (4):
  object-name: make get_oid quietly return an error
  builtin/stash: factor out revision parsing into a function
  builtin/stash: provide a way to export stashes to a ref
  builtin/stash: provide a way to import stashes from a ref

 Documentation/git-stash.txt |  29 +++-
 builtin/stash.c             | 321 ++++++++++++++++++++++++++++++++++--
 cache.h                     |   1 +
 object-name.c               |   6 +-
 t/t3903-stash.sh            |  63 +++++++
 5 files changed, 406 insertions(+), 14 deletions(-)

[PATCH v4 1/4] object-name: make get_oid quietly return an error

[brian m. carlson]

A reasonable person looking at the signature and usage of get_oid and
friends might conclude that in the event of an error, it always returns
-1.  However, this is not the case.  Instead, get_oid_basic dies if we
go too far back into the history of a reflog (or, when quiet, simply
exits).

This is not especially useful, since in many cases, we might want to
handle this error differently.  Let's add a flag here to make it just
return -1 like elsewhere in these code paths.

Note that we cannot make this behavior the default, since we have many
other codepaths that rely on the existing behavior, including in tests.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h       | 1 +
 object-name.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 825ec17198..657e3ff17f 100644
--- a/cache.h
+++ b/cache.h
@@ -1376,6 +1376,7 @@ struct object_context {
 #define GET_OID_RECORD_PATH     0200
 #define GET_OID_ONLY_TO_DIE    04000
 #define GET_OID_REQUIRE_PATH  010000
+#define GET_OID_GENTLY        020000
 
 #define GET_OID_DISAMBIGUATORS \
 	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index 92862eeb1a..46dbfe36a6 100644
--- a/object-name.c
+++ b/object-name.c
@@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 						len, str,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
-			} else {
+			} else if (!(flags & GET_OID_GENTLY)) {
 				if (flags & GET_OID_QUIETLY) {
 					exit(128);
 				}
 				die(_("log for '%.*s' only has %d entries"),
 				    len, str, co_cnt);
 			}
+			if (flags & GET_OID_GENTLY) {
+				free(real_ref);
+				return -1;
+			}
 		}
 	}
 

[PATCH v4 2/4] builtin/stash: factor out revision parsing into a function

[brian m. carlson]

We allow several special forms of stash names in this code.  In the
future, we'll want to allow these same forms without parsing a stash
commit, so let's refactor this code out into a function for reuse.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/stash.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 5897febfbe..b677739bcd 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -130,6 +130,25 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
 		die(_("'%s' is not a stash-like commit"), revision);
 }
 
+static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
+{
+	strbuf_reset(revision);
+	if (!commit) {
+		if (!ref_exists(ref_stash)) {
+			if (!quiet)
+				fprintf_ln(stderr, _("No stash entries found."));
+			return -1;
+		}
+
+		strbuf_addf(revision, "%s@{0}", ref_stash);
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(revision, "%s@{%s}", ref_stash, commit);
+	} else {
+		strbuf_addstr(revision, commit);
+	}
+	return 0;
+}
+
 static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 {
 	int ret;
@@ -158,18 +177,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 		commit = argv[0];
 
 	strbuf_init(&info->revision, 0);
-	if (!commit) {
-		if (!ref_exists(ref_stash)) {
-			free_stash_info(info);
-			fprintf_ln(stderr, _("No stash entries found."));
-			return -1;
-		}
-
-		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
-	} else if (strspn(commit, "0123456789") == strlen(commit)) {
-		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
-	} else {
-		strbuf_addstr(&info->revision, commit);
+	if (parse_revision(&info->revision, commit, 0)) {
+		free_stash_info(info);
+		return -1;
 	}
 
 	revision = info->revision.buf;

[PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref

[brian m. carlson]

A common user problem is how to sync in-progress work to another
machine.  Users currently must use some sort of transfer of the working
tree, which poses security risks and also necessarily causes the index
to become dirty.  The experience is suboptimal and frustrating for
users.

A reasonable idea is to use the stash for this purpose, but the stash is
stored in the reflog, not in a ref, and as such it cannot be pushed or
pulled.  This also means that it cannot be saved into a bundle or
preserved elsewhere, which is a problem when using throwaway development
environments.

Let's solve this problem by allowing the user to export the stash to a
ref (or, to just write it into the repository and print the hash, à la
git commit-tree).  Introduce git stash export, which writes a chain of
commits where the first parent is always a chain to the previous stash,
or to a single, empty commit (for the final item) and the second is the
stash commit normally written to the reflog.

Iterate over each stash from topmost to bottomost, looking up the data
for each one, and then create the chain from the single empty commit
back up in reverse order.  Generate a predictable empty commit so our
behavior is reproducible.  Create a useful commit message, preserving
the author and committer information, to help users identify stash
commits when viewing them as normal commits.

If the user has specified specific stashes they'd like to export
instead, use those instead of iterating over all of the stashes.

As part of this, specifically request quiet behavior when looking up the
OID for a revision because we will eventually hit a revision that
doesn't exist and we don't want to die when that occurs.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |  22 ++++-
 builtin/stash.c             | 182 ++++++++++++++++++++++++++++++++++++
 2 files changed, 203 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 6e15f47525..162110314e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 'git stash' clear
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
+'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
 
 DESCRIPTION
 -----------
@@ -151,6 +152,12 @@ store::
 	reflog.  This is intended to be useful for scripts.  It is
 	probably not the command you want to use; see "push" above.
 
+export ( --print | --to-ref <ref> ) [<stash>...]::
+
+	Export the specified stashes, or all of them if none are specified, to
+	a chain of commits which can be transferred using the normal fetch and
+	push mechanisms, then imported using the `import` subcommand.
+
 OPTIONS
 -------
 -a::
@@ -239,6 +246,19 @@ literally (including newlines and quotes).
 +
 Quiet, suppress feedback messages.
 
+--print::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes without
+storing it anywhere in the ref namespace and print the object ID to
+standard output.  This is designed for scripts.
+
+--to-ref::
+	This option is only valid for `export`.
++
+Create the chain of commits representing the exported stashes and store
+it to the specified ref.
+
 \--::
 	This option is only valid for `push` command.
 +
@@ -256,7 +276,7 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 <stash>::
 	This option is only valid for `apply`, `branch`, `drop`, `pop`,
-	`show` commands.
+	`show`, and `export` commands.
 +
 A reference of the form `stash@{<revision>}`. When no `<stash>` is
 given, the latest stash is assumed (that is, `stash@{0}`).
diff --git a/builtin/stash.c b/builtin/stash.c
index b677739bcd..07b0897eda 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -33,6 +33,7 @@ static const char * const git_stash_usage[] = {
 	   "          [--] [<pathspec>...]]"),
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
 	NULL
 };
 
@@ -89,6 +90,12 @@ static const char * const git_stash_save_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_export_usage[] = {
+	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	NULL
+};
+
+
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -1775,6 +1782,179 @@ static int save_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
+{
+	size_t author_len, committer_len;
+	struct commit *this;
+	const char *orig_author, *orig_committer;
+	char *author = NULL, *committer = NULL;
+	const char *buffer;
+	unsigned long bufsize;
+	const char *p;
+	struct strbuf msg = STRBUF_INIT;
+	int ret = 0;
+
+	this = lookup_commit_reference(the_repository, oid);
+	buffer = get_commit_buffer(this, &bufsize);
+	orig_author = find_commit_header(buffer, "author", &author_len);
+	orig_committer = find_commit_header(buffer, "committer", &committer_len);
+	p = memmem(buffer, bufsize, "\n\n", 2);
+
+	if (!orig_author || !orig_committer || !p) {
+		ret = error(_("cannot parse commit %s"), oid_to_hex(oid));
+		goto out;
+	}
+	/* Jump to message. */
+	p += 2;
+	strbuf_addstr(&msg, "git stash: ");
+	strbuf_add(&msg, p, bufsize - (p - buffer));
+
+	author = xmemdupz(orig_author, author_len);
+	committer = xmemdupz(orig_committer, committer_len);
+
+	if (commit_tree_extended(msg.buf, msg.len,
+				 the_hash_algo->empty_tree, parents,
+				 out, author, committer,
+				 NULL, NULL)) {
+		ret = error(_("could not write commit"));
+		goto out;
+	}
+out:
+	strbuf_release(&msg);
+	unuse_commit_buffer(this, buffer);
+	free(author);
+	free(committer);
+	return ret;
+}
+
+static int do_export_stash(const char *ref, int argc, const char **argv)
+{
+	struct object_id base;
+	struct object_context unused;
+	struct commit *prev;
+	struct object_id *items = NULL;
+	int nitems = 0, nalloc = 0;
+	int res = 0;
+	int i;
+	struct strbuf revision = STRBUF_INIT;
+	const char *author, *committer;
+
+	/*
+	 * This is an arbitrary, fixed date, specifically the one used by git
+	 * format-patch.  The goal is merely to produce reproducible output.
+	 */
+	prepare_fallback_ident("git stash", "git@stash");
+	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT,
+			   "2001-09-17T00:00:00Z", 0);
+	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT,
+			      "2001-09-17T00:00:00Z", 0);
+
+	/* First, we create a single empty commit. */
+	if (commit_tree_extended(NULL, 0, the_hash_algo->empty_tree, NULL,
+				 &base, author, committer, NULL, NULL))
+		return error(_("unable to write base commit"));
+
+	prev = lookup_commit_reference(the_repository, &base);
+
+	if (argc) {
+		/*
+		 * Find each specified stash, and load data into the array.
+		 */
+		for (i = 0; i < argc; i++) {
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			if (parse_revision(&revision, argv[i], 1) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &items[i], &unused)) {
+				res = error(_("unable to find stash entry %s"), argv[i]);
+				goto out;
+			}
+		}
+	} else {
+		/*
+		 * Walk the reflog, finding each stash entry, and load data into the
+		 * array.
+		 */
+		for (i = 0;; i++) {
+			char buf[32];
+			struct object_id oid;
+
+			snprintf(buf, sizeof(buf), "%d", i);
+			if (parse_revision(&revision, buf, 1) ||
+			    get_oid_with_context(the_repository, revision.buf,
+						 GET_OID_QUIETLY | GET_OID_GENTLY,
+						 &oid, &unused))
+				break;
+			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			oidcpy(&items[i], &oid);
+		}
+	}
+
+	/*
+	 * Now, create a set of commits identical to the regular stash commits,
+	 * but where their first parents form a chain to our original empty
+	 * base commit.
+	 */
+	for (i = nitems - 1; i >= 0; i--) {
+		struct commit_list *parents = NULL;
+		struct commit_list **next = &parents;
+		struct object_id out;
+
+		next = commit_list_append(prev, next);
+		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
+		res = write_commit_with_parents(&out, &items[i], parents);
+		if (res)
+			goto out;
+		prev = lookup_commit_reference(the_repository, &out);
+	}
+	if (ref)
+		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+	else
+		puts(oid_to_hex(&prev->object.oid));
+out:
+	strbuf_release(&revision);
+	free(items);
+
+	return res;
+}
+
+enum export_action {
+	ACTION_NONE,
+	ACTION_PRINT,
+	ACTION_TO_REF,
+};
+
+static int export_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *ref = NULL;
+	enum export_action action = ACTION_NONE;
+	struct option options[] = {
+		OPT_CMDMODE(0, "print", &action,
+			    N_("print the object ID instead of writing it to a ref"),
+			    ACTION_PRINT),
+		OPT_CMDMODE(0, "to-ref", &action,
+			    N_("save the data to the given ref"),
+			    ACTION_TO_REF),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_export_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (action == ACTION_NONE) {
+		return error(_("exactly one of --print and --to-ref is required"));
+	} else if (action == ACTION_TO_REF) {
+		if (!argc)
+			return error(_("--to-ref requires an argument"));
+		ref = argv[0];
+		argc--;
+		argv++;
+	}
+
+	return do_export_stash(ref, argc, argv);
+}
+
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
 	pid_t pid = getpid();
@@ -1818,6 +1998,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!push_stash(argc, argv, prefix, 0);
 	else if (!strcmp(argv[0], "save"))
 		return !!save_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "export"))
+		return !!export_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);

Re: [PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 07 2022, brian m. carlson wrote:

> +'git stash' export ( --print | --to-ref <ref> ) [<stash>...]

This has whitespace-padding around "(" and ")" but the C code verison
you added doesn't, i.e.:

> +static const char * const git_stash_export_usage[] = {
> +	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
> +	NULL
> +};

The *.txt version should be made the same as the C one.

(Spotted with an automated local check I have for finding these
differences in *.txt v.s. code).

Re: [PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 07 2022, brian m. carlson wrote:

> +static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;
> +	int ret = 0;
> +
> +	this = lookup_commit_reference(the_repository, oid);
> +	buffer = get_commit_buffer(this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);

In builtin/am.c we also start with this, but follow it with passing the
data through split_ident_line(), any reason we're not doing the same
thing here, and are there cases wehre that would result in propagating
not-sanity-checked-enough data?


> +	p = memmem(buffer, bufsize, "\n\n", 2);
> +
> +	if (!orig_author || !orig_committer || !p) {
> +		ret = error(_("cannot parse commit %s"), oid_to_hex(oid));
> +		goto out;
> +	}
> +	/* Jump to message. */
> +	p += 2;
> +	strbuf_addstr(&msg, "git stash: ");
> +	strbuf_add(&msg, p, bufsize - (p - buffer));

And more on those behavior differences, the am.c version seems to not
care that we *might* have a \0 in a commit buffer and just uses
xstrdup(msg + 2).

But this one looks like it will faithfully carry that \0-containing
message forward.

Probably OK, just checking...

Re: [PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 07 2022, brian m. carlson wrote:

> +	/*
> +	 * This is an arbitrary, fixed date, specifically the one used by git
> +	 * format-patch.  The goal is merely to produce reproducible output.
> +	 */
> +	prepare_fallback_ident("git stash", "git@stash");
> +	author = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT,
> +			   "2001-09-17T00:00:00Z", 0);
> +	committer = fmt_ident("git stash", "git@stash", WANT_BLANK_IDENT,
> +			      "2001-09-17T00:00:00Z", 0);

Hrm, reproducable with what? Because you're doing this we won't get a
different OID every time you do:

    git stash export --print

That part I'm completely on board with, but not pick the author/date
from the tip of the stash we're exporting instead?

AFAICT the only negative side-effect of that is that we'll create
another such "base" commit for every stash we export, which is a bit of
garbage churn.

But I think it's worth it to not create magical author/committer entries
with nonsensical dates.

[PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

Now that we have a way to export stashes to a ref, let's provide a way
to import them from such a ref back to the stash.  This works much the
way the export code does, except that we strip off the first parent
chain commit and then store each resulting commit back to the stash.

We don't clear the stash first and instead add the specified stashes to
the top of the stash.  This is because users may want to export just a
few stashes, such as to share a small amount of work in progress with a
colleague, and it would be undesirable for the receiving user to lose
all of their data.  For users who do want to replace the stash, it's
easy to do to: simply run "git stash clear" first.

We specifically rely on the fact that we'll produce identical stash
commits on both sides in our tests.  This provides a cheap,
straightforward check for our tests and also makes it easy for users to
see if they already have the same data in both repositories.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-stash.txt |   7 +++
 builtin/stash.c             | 105 ++++++++++++++++++++++++++++++++++++
 t/t3903-stash.sh            |  63 ++++++++++++++++++++++
 3 files changed, 175 insertions(+)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 162110314e..28eb9cab0c 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -21,6 +21,7 @@ SYNOPSIS
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
+'git stash' import <commit>
 
 DESCRIPTION
 -----------
@@ -158,6 +159,12 @@ export ( --print | --to-ref <ref> ) [<stash>...]::
 	a chain of commits which can be transferred using the normal fetch and
 	push mechanisms, then imported using the `import` subcommand.
 
+import <commit>::
+
+	Import the specified stashes from the specified commit, which must have been
+	created by `export`, and add them to the list of stashes.  To replace the
+	existing stashes, use `clear` first.
+
 OPTIONS
 -------
 -a::
diff --git a/builtin/stash.c b/builtin/stash.c
index 07b0897eda..06fa74c0c2 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -34,6 +34,7 @@ static const char * const git_stash_usage[] = {
 	N_("git stash save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
 	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
+	N_("git stash import <commit>"),
 	NULL
 };
 
@@ -95,6 +96,10 @@ static const char * const git_stash_export_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_import_usage[] = {
+	N_("git stash import <commit>"),
+	NULL
+};
 
 static const char ref_stash[] = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
  * 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
+ * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
  * w_tree is set to the working tree
  * b_tree is set to the base tree
  * i_tree is set to the index tree
@@ -114,6 +120,7 @@ struct stash_info {
 	struct object_id b_commit;
 	struct object_id i_commit;
 	struct object_id u_commit;
+	struct object_id c_commit;
 	struct object_id w_tree;
 	struct object_id b_tree;
 	struct object_id i_tree;
@@ -1827,6 +1834,102 @@ static int write_commit_with_parents(struct object_id *out, const struct object_
 	return ret;
 }
 
+static int do_import_stash(const char *rev)
+{
+	struct object_id chain;
+	struct oid_array items = OID_ARRAY_INIT;
+	int res = 0;
+	int i;
+	const char *buffer = NULL;
+	struct commit *this = NULL;
+	char *msg = NULL;
+
+	if (get_oid(rev, &chain))
+		return error(_("not a valid revision: %s"), rev);
+
+	/*
+	 * Walk the commit history, finding each stash entry, and load data into
+	 * the array.
+	 */
+	for (i = 0;; i++) {
+		struct object_id tree, oid;
+		char revision[GIT_MAX_HEXSZ + 1];
+
+		oid_to_hex_r(revision, &chain);
+
+		if (get_oidf(&tree, "%s:", revision) ||
+		    !oideq(&tree, the_hash_algo->empty_tree)) {
+			return error(_("%s is not a valid exported stash commit"), revision);
+		}
+		if (get_oidf(&chain, "%s^1", revision) ||
+		    get_oidf(&oid, "%s^2", revision))
+			break;
+		oid_array_append(&items, &oid);
+	}
+
+	/*
+	 * Now, walk each entry, adding it to the stash as a normal stash
+	 * commit.
+	 */
+	for (i = items.nr - 1; i >= 0; i--) {
+		unsigned long bufsize;
+		const char *p;
+		const struct object_id *oid = items.oid + i;
+
+		this = lookup_commit_reference(the_repository, oid);
+		buffer = get_commit_buffer(this, &bufsize);
+		if (!buffer) {
+			res = -1;
+			error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
+			goto out;
+		}
+
+		p = memmem(buffer, bufsize, "\n\n", 2);
+		if (!p) {
+			res = -1;
+			error(_("cannot parse commit %s"), oid_to_hex(oid));
+			goto out;
+		}
+
+		p += 2;
+		msg = xmemdupz(p, bufsize - (p - buffer));
+		unuse_commit_buffer(this, buffer);
+		buffer = NULL;
+
+		if (do_store_stash(oid, msg, 1)) {
+			res = -1;
+			error(_("cannot save the stash for %s"), oid_to_hex(oid));
+			goto out;
+		}
+		FREE_AND_NULL(msg);
+	}
+out:
+	if (this && buffer)
+		unuse_commit_buffer(this, buffer);
+	oid_array_clear(&items);
+	free(msg);
+
+	return res;
+}
+
+static int import_stash(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_import_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (argc != 1) {
+		usage_with_options(git_stash_import_usage, options);
+		return -1;
+	}
+
+	return do_import_stash(argv[0]);
+}
+
 static int do_export_stash(const char *ref, int argc, const char **argv)
 {
 	struct object_id base;
@@ -2000,6 +2103,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		return !!save_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "export"))
 		return !!export_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "import"))
+		return !!import_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_optf(_("unknown subcommand: %s"),
 			       git_stash_usage, options, argv[0]);
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..03607a5a38 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -10,6 +10,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOF
+	export_base sha1:73c9bab443d1f88ac61aa533d2eeaaa15451239c
+	export_base sha256:f210fa6346e3e2ce047bdb570426b17075980c1ac01fec8fc4b75bd3ab4bcfe4
+	EOF
+'
+
 test_expect_success 'usage on cmd and subcommand invalid option' '
 	test_expect_code 129 git stash --invalid-option 2>usage &&
 	grep "or: git stash" usage &&
@@ -1295,6 +1302,62 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash export and import round-trip stashes' '
+	git reset &&
+	>untracked &&
+	>tracked1 &&
+	>tracked2 &&
+	git add tracked* &&
+	git stash -- &&
+	>subdir/untracked &&
+	>subdir/tracked1 &&
+	>subdir/tracked2 &&
+	git add subdir/tracked* &&
+	git stash --include-untracked -- subdir/ &&
+	git tag t-stash0 stash@{0} &&
+	git tag t-stash1 stash@{1} &&
+	simple=$(git stash export --print) &&
+	git stash clear &&
+	git stash import "$simple" &&
+	test_cmp_rev stash@{0} t-stash0 &&
+	test_cmp_rev stash@{1} t-stash1 &&
+	git stash export --to-ref refs/heads/foo &&
+	git stash clear &&
+	git stash import foo &&
+	test_cmp_rev stash@{0} t-stash0 &&
+	test_cmp_rev stash@{1} t-stash1
+'
+
+test_expect_success 'stash import appends commits' '
+	git log --format=oneline -g refs/stash >out &&
+	cat out out >out2 &&
+	git stash import refs/heads/foo &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = $(wc -l <out2) actual
+'
+
+test_expect_success 'stash export can accept specified stashes' '
+	git stash clear &&
+	git stash import foo &&
+	git stash export --to-ref bar stash@{1} stash@{0} &&
+	git stash clear &&
+	git stash import bar &&
+	test_cmp_rev stash@{1} t-stash0 &&
+	test_cmp_rev stash@{0} t-stash1 &&
+	git log --format=oneline -g refs/stash >actual &&
+	test_line_count = 2 actual
+'
+
+test_expect_success 'stash can import and export zero stashes' '
+	git stash clear &&
+	git stash export --to-ref baz &&
+	test_cmp_rev "$(test_oid empty_tree)" baz: &&
+	test_cmp_rev "$(test_oid export_base)" baz &&
+	test_must_fail git rev-parse baz^1 &&
+	git stash import baz &&
+	test_must_fail git rev-parse refs/stash
+'
+
 test_expect_success 'stash apply should succeed with unmodified file' '
 	echo base >file &&
 	git add file &&

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Jonathan Tan]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Now that we have a way to export stashes to a ref, let's provide a way
> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {
> +		unsigned long bufsize;
> +		const char *p;
> +		const struct object_id *oid = items.oid + i;
> +
> +		this = lookup_commit_reference(the_repository, oid);
> +		buffer = get_commit_buffer(this, &bufsize);
> +		if (!buffer) {
> +			res = -1;
> +			error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p = memmem(buffer, bufsize, "\n\n", 2);
> +		if (!p) {
> +			res = -1;
> +			error(_("cannot parse commit %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p += 2;
> +		msg = xmemdupz(p, bufsize - (p - buffer));
> +		unuse_commit_buffer(this, buffer);
> +		buffer = NULL;
> +
> +		if (do_store_stash(oid, msg, 1)) {

This seems like you're using the commit message as the reflog message -
is this necessary? For what it's worth, all tests still pass if I
replace "msg" with "NULL".

Other than that, everything looks good to me.

It might be worth adding tests that check that the exported stashes are
in the expected format (to ensure that we can read stashes exported from
another Git version) but I don't think that has to block the submission
of this patch set.

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
> This seems like you're using the commit message as the reflog message -
> is this necessary? For what it's worth, all tests still pass if I
> replace "msg" with "NULL".

I think that's what the existing stash code does, and so I did the same
here.  It's not strictly necessary, but it's a nice to have.

I didn't think it worth testing, because I don't think we test it
elsewhere, either.

> It might be worth adding tests that check that the exported stashes are
> in the expected format (to ensure that we can read stashes exported from
> another Git version) but I don't think that has to block the submission
> of this patch set.

There's a tiny patch for that for the base commit, but you're right that
some more tests wouldn't hurt.  I can send a followup patch or two as
part of a new series.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Jonathan Tan]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
> > This seems like you're using the commit message as the reflog message -
> > is this necessary? For what it's worth, all tests still pass if I
> > replace "msg" with "NULL".
> 
> I think that's what the existing stash code does, and so I did the same
> here.  It's not strictly necessary, but it's a nice to have.
> 
> I didn't think it worth testing, because I don't think we test it
> elsewhere, either.
> 
> > It might be worth adding tests that check that the exported stashes are
> > in the expected format (to ensure that we can read stashes exported from
> > another Git version) but I don't think that has to block the submission
> > of this patch set.
> 
> There's a tiny patch for that for the base commit, but you're right that
> some more tests wouldn't hurt.  I can send a followup patch or two as
> part of a new series.

OK, these sound good to me.

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 13 2022, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
>> This seems like you're using the commit message as the reflog message -
>> is this necessary? For what it's worth, all tests still pass if I
>> replace "msg" with "NULL".
>
> I think that's what the existing stash code does, and so I did the same
> here.  It's not strictly necessary, but it's a nice to have.

In any case: Jonathan changed the code to omit the commit message & the
tests still passed? Is that to do with:

    We specifically rely on the fact that we'll produce identical stash
    commits on both sides in our tests.

I.e. that they're checking round-tripping, but not known correctness?

Also: Maybe I'm missing something but stashes start with "WIP" or "On",
but the export adds a prefix "git stash: ". That seems like an odd
inconsistency, why not keep the message as-is as the commit message?

In any case, the import then takes the message as-is without stripping
the ""git stash: " prefix?

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Jonathan Tan]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> On Wed, Apr 13 2022, brian m. carlson wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
> >> This seems like you're using the commit message as the reflog message -
> >> is this necessary? For what it's worth, all tests still pass if I
> >> replace "msg" with "NULL".
> >
> > I think that's what the existing stash code does, and so I did the same
> > here.  It's not strictly necessary, but it's a nice to have.
> 
> In any case: Jonathan changed the code to omit the commit message & the
> tests still passed? Is that to do with:
> 
>     We specifically rely on the fact that we'll produce identical stash
>     commits on both sides in our tests.
> 
> I.e. that they're checking round-tripping, but not known correctness?

To clarify: I omitted something in the reflog message, not in any of the
commit messages. The round-tripping only checks commit messages (which I
agree is the important thing). I still do think that the correctness of
what's being exported is important, but brian has already said that he
can send a followup as part of a new series [1].

[1] https://lore.kernel.org/git/YlYjgLcnNH8V1yj0@camp.crustytoothpaste.net/

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Junio C Hamano]

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

> On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
>> This seems like you're using the commit message as the reflog message -
>> is this necessary? For what it's worth, all tests still pass if I
>> replace "msg" with "NULL".
>
> I think that's what the existing stash code does, and so I did the same
> here.  It's not strictly necessary, but it's a nice to have.
>
> I didn't think it worth testing, because I don't think we test it
> elsewhere, either.
>
>> It might be worth adding tests that check that the exported stashes are
>> in the expected format (to ensure that we can read stashes exported from
>> another Git version) but I don't think that has to block the submission
>> of this patch set.
>
> There's a tiny patch for that for the base commit, but you're right that
> some more tests wouldn't hurt.  I can send a followup patch or two as
> part of a new series.

Is this about the log messages recorded in the throw-away commits
that are only used to form a single backbone chain, to which the
commits used to represent stash entries are linked to?

Are these messages meant to be used in any way?  I do not think
these messages contribute anything to the end result (they are just
discarded once they serve their purpose of transferring the
underlying stash entries, if I recall the design discussion
correctly), so I am not sure if we would even want to cast in stone
what they would say.

If on the other hand they are meant to be read by something (either
programs or end-user humans), it does make sense to ensure that we
are recording what we think we are recording.

Thanks.

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

On 2022-04-13 at 20:10:07, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
> >> This seems like you're using the commit message as the reflog message -
> >> is this necessary? For what it's worth, all tests still pass if I
> >> replace "msg" with "NULL".
> >
> > I think that's what the existing stash code does, and so I did the same
> > here.  It's not strictly necessary, but it's a nice to have.
> >
> > I didn't think it worth testing, because I don't think we test it
> > elsewhere, either.
> >
> >> It might be worth adding tests that check that the exported stashes are
> >> in the expected format (to ensure that we can read stashes exported from
> >> another Git version) but I don't think that has to block the submission
> >> of this patch set.
> >
> > There's a tiny patch for that for the base commit, but you're right that
> > some more tests wouldn't hurt.  I can send a followup patch or two as
> > part of a new series.
> 
> Is this about the log messages recorded in the throw-away commits
> that are only used to form a single backbone chain, to which the
> commits used to represent stash entries are linked to?

My response to the first paragraph was talking about the messages in the
reflog.  When we create a reflog entry, we add a message, which I've set
to the commit message of the stash entry, like the existing code does.
I don't think that's an important detail, but I did it to be consistent.
I think it's better to put something useful there, at least, rather than
not put any message at all.

The log messages recorded in the chain of throwaway commits are
identical to the corresponding stash commit's message with a prefix of
"git stash: ".  There's currently a test for the base commit having a
certain fixed value, but not the contents of the other commits.

> Are these messages meant to be used in any way?  I do not think
> these messages contribute anything to the end result (they are just
> discarded once they serve their purpose of transferring the
> underlying stash entries, if I recall the design discussion
> correctly), so I am not sure if we would even want to cast in stone
> what they would say.
> 
> If on the other hand they are meant to be read by something (either
> programs or end-user humans), it does make sense to ensure that we
> are recording what we think we are recording.

The log (not reflog) messages are valuable because since these message
are pushed as refs, someone may look at them (e.g., on GitHub or with
git log) and it is helpful to provide something that tells the user
what's going on.

For example, since our throwaway commits are empty, it would seem
bizarre to users that someone pushed a commit with an empty tree, but if
they see that the commits are stash commits, that may help them have
useful context.

I'm happy to add a few more tests for this in a followup series.  I'll
likely get to it this weekend and can include some checks for the commit
message and some improved verification of commits in the testsuite.  I
don't think this should hold up the rest of the series, however.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Junio C Hamano]

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

> For example, since our throwaway commits are empty, it would seem
> bizarre to users that someone pushed a commit with an empty tree, but if
> they see that the commits are stash commits, that may help them have
> useful context.

Understood.  Also recording the tree of the state the stash entry
represents the working tree state (instead of an empty tree) might
make it easier to see.  Then even in UI like GitHub web interface,
the users can inspect how various paths in the tree looks like.

> I'm happy to add a few more tests for this in a followup series.  I'll
> likely get to it this weekend and can include some checks for the commit
> message and some improved verification of commits in the testsuite.  I
> don't think this should hold up the rest of the series, however.

THanks.

Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 07 2022, brian m. carlson wrote:

[Finding time to go over this in a few passes, so some disjointed
replies, sorry]

> +	for (i = 0;; i++) {
> +		struct object_id tree, oid;
> +		char revision[GIT_MAX_HEXSZ + 1];
> +
> +		oid_to_hex_r(revision, &chain);
> +
> +		if (get_oidf(&tree, "%s:", revision) ||
> +		    !oideq(&tree, the_hash_algo->empty_tree)) {
> +			return error(_("%s is not a valid exported stash commit"), revision);

I think you're leaking memory here, i.e. you're in the for-loop and
doing oid_array_append()< but here you won't clear that or do other
"out" free-ing at the end.

But I also checked if your tests leaked with SANITIZE=leak, and (after
omitting the existing leaks) they didn't, so either I'm wrong or it's a
test blindspot.

Have you tried "make coverage-report" with this?

> +		}
> +		if (get_oidf(&chain, "%s^1", revision) ||
> +		    get_oidf(&oid, "%s^2", revision))
> +			break;
> +		oid_array_append(&items, &oid);
> +	}
> +
> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {
> +		unsigned long bufsize;
> +		const char *p;
> +		const struct object_id *oid = items.oid + i;
> +
> +		this = lookup_commit_reference(the_repository, oid);
> +		buffer = get_commit_buffer(this, &bufsize);
> +		if (!buffer) {
> +			res = -1;
> +			error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p = memmem(buffer, bufsize, "\n\n", 2);

Nit: Grepping in-tree all other API users of get_commit_buffer() just
use strstr(buffer, "\n\n"), if this one needs to handle \0 specially
(for reasons I'm missing) perhaps a comment here discussing why?

> +		if (!p) {
> +			res = -1;
> +			error(_("cannot parse commit %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p += 2;
> +		msg = xmemdupz(p, bufsize - (p - buffer));
> +		unuse_commit_buffer(this, buffer);
> +		buffer = NULL;
> +
> +		if (do_store_stash(oid, msg, 1)) {
> +			res = -1;
> +			error(_("cannot save the stash for %s"), oid_to_hex(oid));

Maybe just "res = error" for these? You use that in 3/4, would be good
to continue the same pattern consistently in 4/4.

> +			goto out;
> +		}
> +		FREE_AND_NULL(msg);
> +	}
> +out:
> +	if (this && buffer)
> +		unuse_commit_buffer(this, buffer);
> +	oid_array_clear(&items);
> +	free(msg);
> +
> +	return res;
> +}
> +
> +static int import_stash(int argc, const char **argv, const char *prefix)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_import_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (argc != 1) {
> +		usage_with_options(git_stash_import_usage, options);

This function is a NORETURN....

> +		return -1;

...so this code isn't reachable, and will warn on some compilers (suncc
at least).

But consider using usage_msg_opt() instead, i.e. tell the user what went
wrong.

Re: [PATCH v4 0/4] Importing and exporting stashes to refs

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 07 2022, brian m. carlson wrote:

> * Use oid_array.

It looks like only 1/2 of the functions using the manual
not-quite-an-oid_array were converted. Here's a diff to squash for the
other one:

diff --git a/builtin/stash.c b/builtin/stash.c
index e4b99188836..7d5493581e6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1925,8 +1925,7 @@ static int do_export_stash(const char *ref, int argc, const char **argv)
 	struct object_id base;
 	struct object_context unused;
 	struct commit *prev;
-	struct object_id *items = NULL;
-	int nitems = 0, nalloc = 0;
+	struct oid_array items = OID_ARRAY_INIT;
 	int res = 0;
 	int i;
 	struct strbuf revision = STRBUF_INIT;
@@ -1954,14 +1953,16 @@ static int do_export_stash(const char *ref, int argc, const char **argv)
 		 * Find each specified stash, and load data into the array.
 		 */
 		for (i = 0; i < argc; i++) {
-			ALLOC_GROW_BY(items, nitems, 1, nalloc);
+			struct object_id oid;
+
 			if (parse_revision(&revision, argv[i], 1) ||
 			    get_oid_with_context(the_repository, revision.buf,
 						 GET_OID_QUIETLY | GET_OID_GENTLY,
-						 &items[i], &unused)) {
+						 &oid, &unused)) {
 				res = error(_("unable to find stash entry %s"), argv[i]);
 				goto out;
 			}
+			oid_array_append(&items, &oid);
 		}
 	} else {
 		/*
@@ -1978,8 +1979,7 @@ static int do_export_stash(const char *ref, int argc, const char **argv)
 						 GET_OID_QUIETLY | GET_OID_GENTLY,
 						 &oid, &unused))
 				break;
-			ALLOC_GROW_BY(items, nitems, 1, nalloc);
-			oidcpy(&items[i], &oid);
+			oid_array_append(&items, &oid);
 		}
 	}
 
@@ -1988,14 +1988,15 @@ static int do_export_stash(const char *ref, int argc, const char **argv)
 	 * but where their first parents form a chain to our original empty
 	 * base commit.
 	 */
-	for (i = nitems - 1; i >= 0; i--) {
+	for (i = items.nr - 1; i >= 0; i--) {
 		struct commit_list *parents = NULL;
 		struct commit_list **next = &parents;
 		struct object_id out;
+		const struct object_id *oid = items.oid + i;
 
 		next = commit_list_append(prev, next);
-		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
-		res = write_commit_with_parents(&out, &items[i], parents);
+		next = commit_list_append(lookup_commit_reference(the_repository, oid), next);
+		res = write_commit_with_parents(&out, oid, parents);
 		if (res)
 			goto out;
 		prev = lookup_commit_reference(the_repository, &out);
@@ -2006,7 +2007,7 @@ static int do_export_stash(const char *ref, int argc, const char **argv)
 		puts(oid_to_hex(&prev->object.oid));
 out:
 	strbuf_release(&revision);
-	free(items);
+	oid_array_clear(&items);
 
 	return res;
 }