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.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
