Re: [PATCH] interpret-trailers: obey scissors lines

From: Jeff King <peff@peff.net>
Date: May 15, 2017, 03:07 UTC
Message-ID: <20170515030726.mmbb43zxqgtbrbuy@sigill.intra.peff.net>
In-Reply-To: <xmqqfug6yiks.fsf@gitster.mtv.corp.google.com>

On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:

Show 17 quoted lines

> >> 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).

Show 5 quoted lines

> 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