| Message ID | 20220403182250.904933-5-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Importing and exporting stashes to refs | expand |
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?
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.
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.
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 &&
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(+)