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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

On 2025-05-22 at 21:09:23, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > +		const char *author, *committer;
> > +		size_t author_len, committer_len;
> > +		const char *p;
> > +		const char *expected = "git stash <git@stash> 1000684800 +0000";
> 
> Makes me wonder if we can somehow share this with [4/5] where we
> establish the author and committer ident (timestamp spelled in a
> different form).  Three strings that has to stay in sync is not a
> huge deal, though.

I'll take a look and see if I can come up with a nice way to solve this.

> > +		p = strstr(buffer, "\n\n");
> > +		if (!p) {
> > +			res = error(_("cannot parse commit %s"), oid_to_hex(&this->object.oid));
> > +			goto out;
> > +		}
> > +
> > +		p += 2;
> > +		if (((size_t)(bufsize - (p - buffer)) < strlen(prefix)) ||
> > +		    memcmp(prefix, p, strlen(prefix))) {
> > +			res = error(_("found stash commit %s with unexpected prefix"), oid_to_hex(&this->object.oid));
> > +			goto out;
> > +		}
> 
> I'd call that "without expected prefix" (the difference being "we
> expected 'git stash:' prefix but you have 'git stosh:' prefix" vs
> "your commit title is 'hello world'"), but OK.  The important thing
> is that we are being careful not to get confused.

Will fix.

> It is unfortunate historical practice to measure almost everything
> in "unsigned long" and bufsize here is one of them, but do we need
> that cast?  We know strlen(prefix) is a small known constant
> integer, we know p is not smaller than buffer, we know (p-buffer) is
> not larger than bufsize.

Yes, we do, because Windows will complain about a comparison between
signed and unsigned integer.  `p - buffer` is `ptrdiff_t`, which is 64
bits, but `unsigned long` is only 32 bite, so they both get promoted to
long long, which is 64 bits and signed, and then the comparison is to
`size_t`, which is 64 bits and unsigned.  The CI jobs fail without this.

I'll make a note in the commit message to that effect.

> OK.  Again, I suspect that commit_list may be a more natural thing
> to use to accumulate the stash entries, but that is not a huge deal.

I'll try to fix that in v7.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]