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

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

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

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.

> > +static const char * const git_stash_export_usage[] = {
> > +	N_("git stash export (--print | --to-ref <ref>) [<stash>...]"),
> > +	NULL
> > +};
> > +
> > +
> 
> Stray too-much-whitespace.

Fixed in v3 already.

> > +	this = lookup_commit_reference(the_repository, oid);
> > +	buffer = get_commit_buffer(this, &bufsize);
> > +	orig_author = find_commit_header(buffer, "author", &author_len);
> > +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> > +	p = memmem(buffer, bufsize, "\n\n", 2);
> > +
> > +	if (!orig_author || !orig_committer || !p) {
> > +		error(_("cannot parse commit %s"), oid_to_hex(oid));
> > +		goto out;
> 
> ..isn't this a logic error, shouldn't we return -1 here?

Yes, it is.

> better as "ret = error(..."?

Yup.  v4 will have it fixed in both places.

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

> Some very long lines here.

Will wrap in v4.

> > +		return error(_("unable to write base commit"));
> > +
> > +	prev = lookup_commit_reference(the_repository, &base);
> > +
> > +	if (argc) {
> > +		/*
> > +		 * Find each specified stash, and load data into the array.
> > +		 */
> > +		for (int i = 0; i < argc; i++) {
> 
> ...as this is size_t, not int.

I'll make argc int.

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

The C99 dependency has been removed in v3.

> > +	/*
> > +	 * Now, create a set of commits identical to the regular stash commits,
> > +	 * but where their first parents form a chain to our original empty
> > +	 * base commit.
> > +	 */
> > +	for (int i = nitems - 1; i >= 0; i--) {
> 
> 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.

> > +		struct commit_list *parents = NULL;
> > +		struct commit_list **next = &parents;
> > +		struct object_id out;
> > +
> > +		next = commit_list_append(prev, next);
> > +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i]), next);
> > +		if (write_commit_with_parents(&out, &items[i], parents)) {
> 
> Here we returned -1 from this earlier, I think this would be more
> straightforward as:
> 	
> 	res = write_commit_with_parents(...)
> 	if (res < 0)
> 		goto out;
> 	
> 
> > +			res = -1;
> > +			goto out;
> 
> So one doesn't have to wonder why we're ignoring the error value, and
> using -1, but then treating all non-zero as errors.

That will be fixed in v4.

> It looks like we don't need to initialize this.

It'll be removed in v4.

> > +	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.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

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