From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v3 4/4] builtin/stash: provide a way to import stashes from a ref
Date: Mon, 04 Apr 2022 12:38:53 +0200 [thread overview]
Message-ID: <220404.86czhxjewx.gmgdl@evledraar.gmail.com> (raw)
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...
> [...]
> + 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?
next prev parent reply other threads:[~2022-04-04 10:56 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-10 17:32 [PATCH 0/6] Importing and exporting stashes to refs brian m. carlson
2022-03-10 17:32 ` [PATCH 1/6] builtin/stash: factor out generic function to look up stash info brian m. carlson
2022-03-10 17:32 ` [PATCH 2/6] builtin/stash: fill in all commit data brian m. carlson
2022-03-16 16:50 ` Junio C Hamano
2022-03-10 17:32 ` [PATCH 3/6] object-name: make get_oid quietly return an error brian m. carlson
2022-03-16 16:56 ` Junio C Hamano
2022-03-16 17:01 ` Drew Stolee
2022-03-16 21:40 ` brian m. carlson
2022-03-10 17:32 ` [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref brian m. carlson
2022-03-11 2:08 ` Ævar Arnfjörð Bjarmason
2022-03-14 21:19 ` Phillip Wood
2022-03-15 10:50 ` Phillip Wood
2022-03-16 21:48 ` brian m. carlson
2022-03-18 13:34 ` C99 %zu support (on MSVC) (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref) Ævar Arnfjörð Bjarmason
2022-03-18 16:26 ` Phillip Wood
2022-03-24 14:02 ` [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref Johannes Schindelin
2022-03-18 13:41 ` ssize_t portability (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref) Ævar Arnfjörð Bjarmason
2022-03-16 17:05 ` [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref Junio C Hamano
2022-03-10 17:32 ` [PATCH 5/6] builtin/stash: provide a way to import stashes from " brian m. carlson
2022-03-16 17:26 ` Junio C Hamano
2022-03-16 21:50 ` brian m. carlson
2022-03-10 17:32 ` [PATCH 6/6] doc: add stash export and import to docs brian m. carlson
2022-03-16 17:34 ` Junio C Hamano
2022-03-16 21:44 ` Junio C Hamano
2022-03-10 19:14 ` [PATCH 0/6] Importing and exporting stashes to refs Junio C Hamano
2022-03-10 21:04 ` brian m. carlson
2022-03-10 21:38 ` Junio C Hamano
2022-03-10 22:42 ` brian m. carlson
2022-03-29 21:49 ` [PATCH v2 0/4] " brian m. carlson
2022-03-29 21:49 ` [PATCH v2 1/4] object-name: make get_oid quietly return an error brian m. carlson
2022-03-29 21:49 ` [PATCH v2 2/4] builtin/stash: factor out revision parsing into a function brian m. carlson
2022-03-29 21:49 ` [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref brian m. carlson
2022-03-30 23:05 ` Junio C Hamano
2022-03-30 23:44 ` brian m. carlson
2022-03-31 1:56 ` Ævar Arnfjörð Bjarmason
2022-03-31 17:43 ` Junio C Hamano
2022-04-05 10:55 ` brian m. carlson
2022-04-06 9:05 ` Ævar Arnfjörð Bjarmason
2022-04-06 16:38 ` Junio C Hamano
2022-03-31 2:09 ` Ævar Arnfjörð Bjarmason
2022-04-05 10:22 ` brian m. carlson
2022-03-29 21:49 ` [PATCH v2 4/4] builtin/stash: provide a way to import stashes from " brian m. carlson
2022-03-31 1:48 ` [PATCH v2 0/4] Importing and exporting stashes to refs Junio C Hamano
2022-03-31 2:18 ` Ævar Arnfjörð Bjarmason
2022-04-03 18:22 ` [PATCH v3 " brian m. carlson
2022-04-03 18:22 ` [PATCH v3 1/4] object-name: make get_oid quietly return an error brian m. carlson
2022-04-03 18:22 ` [PATCH v3 2/4] builtin/stash: factor out revision parsing into a function brian m. carlson
2022-04-04 15:44 ` Phillip Wood
2022-04-03 18:22 ` [PATCH v3 3/4] builtin/stash: provide a way to export stashes to a ref brian m. carlson
2022-04-04 6:46 ` Ævar Arnfjörð Bjarmason
2022-04-03 18:22 ` [PATCH v3 4/4] builtin/stash: provide a way to import stashes from " brian m. carlson
2022-04-04 10:38 ` Ævar Arnfjörð Bjarmason [this message]
2022-04-05 10:03 ` brian m. carlson
2022-04-06 9:00 ` Ævar Arnfjörð Bjarmason
2022-04-04 0:05 ` [PATCH v3 0/4] Importing and exporting stashes to refs Junio C Hamano
2022-04-04 0:29 ` Junio C Hamano
2022-04-04 6:20 ` Ævar Arnfjörð Bjarmason
2022-04-05 9:15 ` brian m. carlson
2022-04-07 21:53 ` [PATCH v4 " brian m. carlson
2022-04-07 21:53 ` [PATCH v4 1/4] object-name: make get_oid quietly return an error brian m. carlson
2022-04-07 21:53 ` [PATCH v4 2/4] builtin/stash: factor out revision parsing into a function brian m. carlson
2022-04-07 21:53 ` [PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref brian m. carlson
2022-04-13 15:29 ` Ævar Arnfjörð Bjarmason
2022-04-13 15:36 ` Ævar Arnfjörð Bjarmason
2022-04-13 15:55 ` Ævar Arnfjörð Bjarmason
2022-04-07 21:53 ` [PATCH v4 4/4] builtin/stash: provide a way to import stashes from " brian m. carlson
2022-04-12 20:14 ` Jonathan Tan
2022-04-13 1:12 ` brian m. carlson
2022-04-13 17:34 ` Jonathan Tan
2022-04-13 18:25 ` Ævar Arnfjörð Bjarmason
2022-04-13 19:14 ` Jonathan Tan
2022-04-13 20:10 ` Junio C Hamano
2022-04-13 21:33 ` brian m. carlson
2022-04-13 21:43 ` Junio C Hamano
2022-04-13 18:33 ` Ævar Arnfjörð Bjarmason
2022-04-13 15:25 ` [PATCH v4 0/4] Importing and exporting stashes to refs Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220404.86czhxjewx.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood123@gmail.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).