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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Apr 5, 2022, 10:03 UTC
Message-ID: <YkwT6pwfC1d/PG4Q@camp.crustytoothpaste.net>
In-Reply-To: <220404.86czhxjewx.gmgdl@evledraar.gmail.com>

On 2022-04-04 at 10:38:53, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 20 quoted lines

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

Show 5 quoted lines

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

Show 14 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?

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