On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:
> >> diff --git a/builtin/commit.c b/builtin/commit.c
> >> index 2de5f6cc6..2ce9c339d 100644
> >> --- a/builtin/commit.c
> >> +++ b/builtin/commit.c
> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const
> >> char *prefix)
> >>
> >> if (verbose || /* Truncate the message just before the diff, if any. */
> >> cleanup_mode == CLEANUP_SCISSORS)
> >> - wt_status_truncate_message_at_cut_line(&sb);
> >> + strbuf_setlen(&sb,
> >> + wt_status_last_nonscissors_index(sb.buf, sb.len));
> >
> > This hunk surprised me at first (that we would need to touch commit.c at
> > all), but the refactoring makes sense.
>
> This still surprises me. If the problem is in interpret-trailers,
> why do we even need to touch cmd_commit()? If GIT_EDITOR returns us
The behavior of cmd_commit() shouldn't be changed by the patch. But to
make the interface suitable for the new callsite (which doesn't have a
strbuf, but a ptr/len buffer), it needs to return the length rather than
shortening the strbuf. We could leave in place:
void wt_status_truncate_message_at_cut_line(struct strbuf *sb)
{
strbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));
}
but it would only have this one caller.
If I were doing the patch series, I'd probably have split that
refactoring into its own patch and discussed the reason separately. I
waffled on whether or not to ask Brian to do so (and obviously didn't in
the end).
> The proposed log message calls the cut-line "scissors", but that is
> probably a source of this confusion. The cut-line and scissors do
> not have much in commmon. For one thing, scissors is a mechanism to
> discard everything _ABOVE_ it. The cut-line we see in this example,
> on the other hand, is about discarding everything _BELOW_ it.
I suspect the confusion comes from calling it CLEANUP_SCISSORS in the
quoted context above, then. Certainly we use scissors for "snip
everything above this line" in mailinfo, but here it is "snip everything
after this line". I don't think it's wrong to refer to it as a scissors
line, even though the operation is different.
-Peff
