Hi Brian and Ævar
Firstly I think this is a useful feature to add to git stash, thanks for
working on it Brian
On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
On Thu, Mar 10 2022, brian m. carlson wrote:
+ size_t author_len, committer_len;
+ struct commit *this = NULL;
+ const char *orig_author = NULL, *orig_committer = NULL;
+ char *author = NULL, *committer = NULL;
+ const char *buffer = NULL;
+ unsigned long bufsize;
+ const char *p;
+ char *msg = NULL;
These shouldn't be initialized unless they really need to..
+ this = lookup_commit_reference(the_repository, &info->w_commit);
..and some are clobbered right away here, so all of these should not be initializzed.
+ 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);
You could start searching from orig_committer rather than buffer but I'm
sure it doesn't make any real difference. The sequencer does something
similar to this to replay commits when rebasing - is there any scope for
sharing code between the two?
...since by doing so we hide genuine "uninitialized"
warnings. E.g. "author_len" here isn't initialized, but is set by
find_commit_header(), but if that line was removed we'd warn below, but
not if it's initialized when the variables are declared..
+ for (size_t i = 0;; i++, nitems++) {
Do we need i and nitems?
+ char buf[32];
+ int ret;
+
+ if (nalloc <= i) {
+ size_t new = nalloc * 3 / 2 + 5;
+ items = xrealloc(items, new * sizeof(*items));
+ nalloc = new;
Can't we just use the usual ALLOC_GROW() pattern here?
ALLOC_GROW_BY() zeros out the memory which would mean we could remove
the memset() calls in the loops. I noticed in some other loops we know
the size in advance and could use CALLOC_ARRAY().
+ }
+ snprintf(buf, sizeof(buf), "%zu", i);
Aren't the %z formats unportable (even with our newly found reliance on
more C99)? I vaguely recall trying them recently and the windows CI jobs
erroring...
According to [1] it has been available since at least 2015. It is
certainly much nicer than casting every size_t to uintmax_t and having
to use PRIuMAX.
+ for (ssize_t i = nitems - 1; i >= 0; i--) {
The ssize_t type can be really small (it's not a signed size_t), so this
is unportable, but in practice maybe it's OK...
I'm not really convinced by this ssize_t can be small argument[2], do
you know of any platforms where it is true?
Best Wishes
Phillip
[1]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-140#size
[2]
https://lore.kernel.org/git/e881a455-88b5-9c87-03a8-caaee68bb344@xxxxxxxxx/
