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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi brian

On 09/05/2025 00:44, 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.

I think the commit message would be more convincing if it concentrated
on the need to import / export chains of stashes and the convenience of
having a dedicated command to import a stash as one can export a single
stash with

    git push <remote> refs/stash@{<n>}:refs/exported-stash

and then import it with

    git pull <remote> refs/exported-stash
    git update-ref refs/stash FETCH_HEAD

Having said that I do agree that adding these new commands is a good
idea.

@@ -154,6 +155,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 was surprised to see that --to-ref does not take an argument. I think
that is confusing and it would be better to use OPT_STRING() rather than
OPT_CMDMODE() and manually check that only one of "--print" or
"--to-ref" is given. In the end it is no more work because now you have
to manually check that we have a ref when "--to-ref" is given.

I was also surprised to find that "git stash export HEAD" did not error
out. I was looking for a function that checked the topology of a stash
commit and could not find one so I thought I see how difficult it was to
add that. The fixup commit below adds a new function to check that a
commit looks like a stash and refuses to export anything else. The patch
adds a new function rather than using assert_stash_like() because
despite its name that function is pretty lax (any merge commit will
satisfy it) and is more concerned about filling out a stash_info struct.

Best Wishes

Phillip

---- >8 ----
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
Subject: [PATCH] fixup! builtin/stash: provide a way to export stashes to a
 ref

Reject commits that do not look like stashes.

Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
---
 builtin/stash.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/builtin/stash.c b/builtin/stash.c
index d8332af4017..02cf344ed96 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -180,6 +180,33 @@ static void free_stash_info(struct stash_info *info)
 	strbuf_release(&info->revision);
 }

+static int check_stash_topology(struct repository *r, struct commit *stash)
+{
+	struct commit *p1, *p2, *p3 = NULL;
+
+	/* stash must have two or three parents */
+	if (!stash->parents || !stash->parents->next ||
+	    (stash->parents->next->next && stash->parents->next->next->next))
+		return -1;
+	p1 = stash->parents->item;
+	p2 = stash->parents->next->item;
+	if (stash->parents->next->next)
+		p3 = stash->parents->next->next->item;
+	if (repo_parse_commit(r, p1) || repo_parse_commit(r, p2) ||
+	    (p3 && repo_parse_commit(r, p3)))
+		return -1;
+	/* p2 must have a single parent, p3 must have no parents */
+	if (!p2->parents || p2->parents->next || (p3 && p3->parents))
+		return -1;
+	if (repo_parse_commit(r, p2->parents->item))
+		return -1;
+	/* p2^1 must equal p1 */
+	if (!oideq(&p1->object.oid, &p2->parents->item->object.oid))
+		return -1;
+
+	return 0;
+}
+
 static void assert_stash_like(struct stash_info *info, const char *revision)
 {
 	if (get_oidf(&info->b_commit, "%s^1", revision) ||
@@ -2101,13 +2128,21 @@ static int do_export_stash(struct repository *r,
 		 */
 		for (i = 0; i < argc; i++) {
 			struct object_id oid;
+			struct commit *stash;
+
 			if (parse_revision(&revision, argv[i], 1) ||
 			    get_oid_with_context(r, revision.buf,
 						 GET_OID_QUIETLY | GET_OID_GENTLY,
 						 &oid, &unused)) {
 				res = error(_("unable to find stash entry %s"), argv[i]);
 				goto out;
 			}
+			stash = lookup_commit_reference(r, &oid);
+			if (!stash || check_stash_topology(r, stash)) {
+				res = error(_("%s does not look like a stash commit"),
+					    argv[i]);
+				goto out;
+			}
 			oid_array_append(&items, &oid);
 		}
 	} else {
@@ -2118,13 +2153,20 @@ static int do_export_stash(struct repository *r,
 		for (i = 0;; i++) {
 			char buf[32];
 			struct object_id oid;
+			struct commit *stash;

 			snprintf(buf, sizeof(buf), "%d", i);
 			if (parse_revision(&revision, buf, 1) ||
 			    get_oid_with_context(r, revision.buf,
 						 GET_OID_QUIETLY | GET_OID_GENTLY,
 						 &oid, &unused))
 				break;
+			stash = lookup_commit_reference(r, &oid);
+			if (!stash || check_stash_topology(r, stash)) {
+				res = error(_("%s does not look like a stash commit"),
+					    revision.buf);
+				goto out;
+			}
 			oid_array_append(&items, &oid);
 		}
 	}
--
2.49.0.300.g813f75aecee




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux