Re: [RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
- From
Jens Lehmann <jens.lehmann@web.de>
- Date
- Nov 17, 2013, 12:20 UTC
- Message-ID
- <5288B49D.8080401@web.de>
- In-Reply-To
- <20131117090935.GC17016@sigill.intra.peff.net>
Am 17.11.2013 10:09, schrieb Jeff King:
Show 24 quoted lines
>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 6ab4605..091a6e7 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1602,9 +1602,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>
>> /* Truncate the message just before the diff, if any. */
>> if (verbose) {
>> - p = strstr(sb.buf, "\ndiff --git ");
>> - if (p != NULL)
>> - strbuf_setlen(&sb, p - sb.buf + 1);
>> + p = strstr(sb.buf, wt_status_diff_divider);
>> + if ((p != NULL) && (p > sb.buf) && (p[-1] == '\n'))
>> + strbuf_setlen(&sb, p - sb.buf);
>
> I think your check for a preceding newline is too strict. If I delete
> everything before the scissor line (e.g., because I am trying to abort
> the commit), we should still remove the diff. With your patch, we do
> not, and a commit message consisting solely of the diff.
>
> So I think you want:
>
> if (p && (p == sb.buf || p[-1] == '\n'))Thanks for catching this, will do so in v2.
Show 11 quoted lines
>> + fprintf(s->fp, _("# The diff below will be removed when keeping the previous line.\n"));
>
> I found this hard to parse, I think because of the "keeping" (why would
> I not keep it?), and because you are talking about lines above and
> below. It is not as accurate to say:
>
> # ------------------ >8 --------------------
> # Everything below this line will be removed.
>
> because it is technically the line above that is the cutoff. But I think
> the meaning is clear, and it is simpler to parse.Ok.
Show 17 quoted lines
> I do think it would be simpler with a single line. I know handling the
> i18n was a question there, but I think we should be fine as long as we
> check for the exact bytes we wrote. Surely gettext can do something
> like:
>
> magic = _("# Everything below this line will be removed");
> fprintf(fh, "%s", magic);
> ...
> p = strstr(magic);
>
> I don't know what guarantees on string lifetime gettext gives us, but
> the worst case is that we simply strdup the result.
>
> I suppose it's possible that the translated string could have utf8 with
> multiple representations, and the user's editor normalizes the text in a
> different way than we wrote it when it saves the result. I don't know if
> that is worth caring about or not; it seems kind of insane.I don't have any strong feelings about this one. I'd be fine with dropping the scissor line and taking the translated string as divider line. What do others think?