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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 4, 2022, 10:38 UTC
Message-ID: <220404.86czhxjewx.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220403182250.904933-5-sandals@crustytoothpaste.net>

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

> +	struct object_id *items = NULL;

Is there a reason for why this...

Show 8 quoted lines

> [...]
> +		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?

Show 14 quoted lines

> [...]
> +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:

Show 52 quoted lines

> +	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.

Show 8 quoted lines

> +	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?