| Message ID | 20220407215352.3491567-5-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Importing and exporting stashes to refs | expand |
"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.
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" <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.
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?
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.
Æ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/
"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.
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" <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.
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 &&
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(+)