Re: [PATCH v2 3/4] builtin/stash: provide a way to export stashes to a ref
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Apr 5, 2022, 10:55 UTC
- Message-ID
- <YkwgGjj0JIcOhlMH@camp.crustytoothpaste.net>
- In-Reply-To
- <220331.86pmm2swm9.gmgdl@evledraar.gmail.com>
On 2022-03-31 at 01:56:01, Ævar Arnfjörð Bjarmason wrote:
Show 23 quoted lines
> > 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.
Show 8 quoted lines
> > +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.
Show 11 quoted lines
> > + 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.
Show 11 quoted lines
> > + 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.
Show 11 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. 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.
Show 10 quoted lines
> > + /*
> > + * 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.
Show 21 quoted lines
> > + 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.
Show 7 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.
-- brian m. carlson (he/him or they/them) Toronto, Ontario, CA