Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 6, 2022, 09:05 UTC
Message-ID: <220406.86fsmqefal.gmgdl@evledraar.gmail.com>
In-Reply-To: <YkwgGjj0JIcOhlMH@camp.crustytoothpaste.net>

On Tue, Apr 05 2022, brian m. carlson wrote:

Show 29 quoted lines

> [[PGP Signed Part:Undecided]]
> On 2022-03-31 at 01:56:01, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Mar 29 2022, brian m. carlson wrote:
>> 
>> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> > index 6e15f47525..162110314e 100644
>> > --- a/Documentation/git-stash.txt
>> > +++ b/Documentation/git-stash.txt
>> > @@ -20,6 +20,7 @@ SYNOPSIS
>> >  'git stash' clear
>> >  'git stash' create [<message>]
>> >  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
>> > +'git stash' export ( --print | --to-ref <ref> ) [<stash>...]
>> >  
>> >  DESCRIPTION
>> >  -----------
>> > @@ -151,6 +152,12 @@ store::
>> >  	reflog.  This is intended to be useful for scripts.  It is
>> >  	probably not the command you want to use; see "push" above.
>> >  
>> > +export ( --print | --to-ref <ref> ) [<stash>...]::
>> > +
>> 
>> I think this extra \n here isn't needed.
>
> All the rest of the entries have it that way.  Junio likes it, you
> don't, but it's consistent with the rest of the file and I'm just
> following along.

FWIW I really don't mind. I vaguely thought it might be an ASCIIDOC syntax error as I'm used to seeing the other form, as e.g. adding an extra \n before the following "+" is.

But it's not, and it's indeed consistent with the rest. looks good.

>> Can nalloc be moved into the if=else scopes?
>
> It _can_, but it's used in both, so I don't see a particular reason to
> do so.

I don't mind, FWIW the reason is just to save the reader skimming to track down the various bits of the manual memory allocation.

But as you noted in the v3 reply this can also just use oidset, so ...

Show 15 quoted lines

>> > +				goto out;
>> > +			}
>> > +		}
>> > +	} else {
>> > +		/*
>> > +		 * Walk the reflog, finding each stash entry, and load data into the
>> > +		 * array.
>> > +		 */
>> > +		for (int i = 0;; i++) {
>> 
>> Aside from the C99 dependency Junio mentioned, this should also be size_t.
>
> Nope.  I specifically decided not to do that and mentioned it in the
> cover letter.  Since Windows doesn't let us have nice things like %zu,
> I'm going to switch to int here and be consistent.

You mean to avoid PRIuMAX instead of %d with:

    snprintf(buf, sizeof(buf), "%d", i);

Show 8 quoted lines

> [...(moved around)...]
>> Did you spot my "count down" suggestion in
>> https://lore.kernel.org/git/220311.86bkydi65v.gmgdl@evledraar.gmail.com/
>> on the v1?
>
> I did, and I prefer this approach.  That would be necessary if we were
> using size_t here, but we're not.
> [...]

> I'm not coding for 16-bit systems here and I feel 2 billion stashes is
> sufficient for the needs of the project for the indefinite future
> based on the capabilities of current human beings.

I just thought you might not have seen the v1 feedback, fair enough. And I'm not going to argue that point here.

Just as an explanation: The reason to use these sorts of patterns isn't because we might need to support 16 bit in the future, which we won't, or that I think it's plausible that someone might have >2^31-1 stashes.

It's that we've been changing to larger types across the codebase, and e.g. in 99d60545f87 (string-list API: change "nr" and "alloc" to "size_t", 2022-03-07) changing to an unsigned type required changing such an iterator.

So if we really don't need negative numbers, but are just using -1 as a value to "stop" in a for-loop it's IMO worth it in general to just stick with unsigned, it makes things more future-proof.

Also, not gcc or clang, but e.g. HP/UX's aCC compiler screams bloddy murder about various "why are you assuming unsigned here?" in comparisons across the codebase, which gcc/clang stay quiet about, to the point where it downs out other useful warnings it gives about actual potential issues.

That's not *directly* related to this, which it wouldn't warn about as it's "int". But generally it's a result of us being loose in mixing unsigned and signed types when it comes to a count of a number of items that can't be negative, and won't be exact for "guard clause" code like this.

And finally, even if we might say that more than 2^31-1 stashes is insane it's just easier to reason about code where multiple "counts" are in play if you don't need to carefully think about that aspect.

Anyway, </by-way-of-explanation over>. But just to be clear I think it's fine that you want to keep this as-is.

Show 11 quoted lines

>> > +	ret = do_export_stash(ref, argc, argv);
>> > +	return ret;
>> 
>> Aside from the "ret" case above, maybe this would be better if the
>> "action" check became a swith, then the compiler would help check it
>> against the enum, and this wouldn't implicitly be both ACTION_PRINT and
>> ACTION_TO_REF, but could be done via a fall-through.
>
> Normally I'm a big fan of switch statements, but I don't feel in this
> case that it logically represents the current code.  I think, given the
> context, that an if-else is easier to read.

Makes sense. Thanks.