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?
