On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > [...] > Two style nits: > > * opening curly brace of function goes on its own line > > * we don't normally have a blank line at the top of the function body > preceding the declarations > > So: > > static int prepare_amend_commit(...) > { > const char *buffer, *subject, *fmt; > Okay, I will fix it. > > + buffer = get_commit_buffer(commit, NULL); > > + find_commit_subject(buffer, &subject); > > + /* > > + * If we amend the 'amend!' commit then we don't want to > > + * duplicate the subject line. > > + */ > > + fmt = starts_with(subject, "amend!") ? "%b" : "%B"; > > + format_commit_message(commit, fmt, sb, ctx); > > + unuse_commit_buffer(commit, buffer); > > + return 0; > > +} > > What is the significance of this function's return value? At least in > this patch, the single caller of this function ignores the return > value, which suggests that the function need not return any value. > Will a later patch add other possible return values to indicate an > error or something? > No, it will not return another value later. I will remove it from here. > > @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > > + char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix); > > + commit = lookup_commit_reference_by_name(fixup_commit); > > if (!commit) > > + die(_("could not lookup commit %s"), fixup_commit); > > ctx.output_encoding = get_commit_output_encoding(); > > + format_commit_message(commit, fmt, &sb, &ctx); > > + free(fmt); > > Nit: it would reduce the cognitive load slightly if `fmt` is prepared > just before it is used rather than being prepared at the top of the > block: > > fmt = xstrfmt("%s! %%s\n\n", fixup_prefix); > format_commit_message(commit, fmt, &sb, &ctx); > free(fmt); > > Subjective and not at all worth a re-roll. > Agree, will fix it. > > @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[], > > + if (fixup_message) { > > + /* > > + * To check if fixup_message that contains ':' is a commit > > + * reference for example: --fixup="HEAD^{/^area: string}" or > > + * a suboption of `--fixup`. > > + * > > + * As `amend` suboption contains only alpha character. > > + * So check if first non alpha character in fixup_message > > + * is ':'. > > + */ > > I have a tough time figuring out what this comment is trying to say, > and I don't think I would have understood it if Junio had not already > explained earlier in this thread why this code is as complex as it is > (rather than using, say, skip_prefix()). Perhaps the entire comment > can be replaced with this: > I admit, this comment seems confusing... > Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`, > if present. To avoid being fooled by a legitimate ":" in <commit> > (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be > composed of only alphabetic characters. > > Not necessarily worth a re-roll. > .. and I think we can reword it as suggested by Junio in patch[v4 3/6], as it seems more clear. > > + size_t len = get_alpha_len(fixup_message); > > + if (len && fixup_message[len] == ':') { > > + fixup_message[len++] = '\0'; > > + fixup_commit = fixup_message + len; > > An alternate -- just about as compact and perhaps more idiomatic -- > way to write all this without introducing the new get_alpha_len() > function: > > char *p = fixup_mesage; > while (isalpha(*p)) > p++; > if (p > fixup_message && *p == ':') { > *p = '\0'; > fixup_commit = p + 1; > > Subjective and not at all worth a re-roll. > Earlier we had discussed[1] keeping a separate helper function, so that it may re-use it later. But I agree above is easier to get and compact so I think maybe it will be ok, for this patch series to replace it with the above and remove the function. [1] https://lore.kernel.org/git/xmqqpn0xdse8.fsf@gitster.g/