patch, 3 partsinterpret-trailers + commit -v bugfix

Hi all,

This patch series addresses a bug in interpret-trailers. If the commit that is being editted is "verbose", it will contain a scissors string ("-- >8 --") and a diff. interpret-trailers doesn't interpret the scissors and therefore places trailer information after the diff. A simple reproduction is:

	git config commit.verbose true
	GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
		git commit --amend

This patch series fixes the issue by obeying scissors when computing trailers location.

P.S. This is my first patch series to the git mailing list, to feel free to point out any mistakes I made when submitting.

 commit.c                      | 65 ++++++++++++++++++++++++++++++++++++++++---
 commit.h                      |  1 +
 mailinfo.c                    | 53 +----------------------------------
 t/t7513-interpret-trailers.sh | 17 +++++++++++
 4 files changed, 80 insertions(+), 56 deletions(-)

Move is_scissors_line to commit.c and expose it through commit.h.
This is needed in commit.c, and mailinfo.c shouldn't really own it.
---
 commit.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 commit.h   |  1 +
 mailinfo.c | 53 +----------------------------------------------------
 3 files changed, 54 insertions(+), 52 deletions(-)

On Thu, May 11, 2017 at 10:03:46PM -0700, Brian Malehorn wrote:

It was fine for mailinfo to own it until now, since it was the only user. :) I think there are some unsaid bits in your rationale. Perhaps something like:

 Subject: mailinfo: make is_scissors_line() public

 We parse scissors lines only when looking at emails, and thus the
 parsing function has always lived in mailinfo.c. However, we also
 generate scissors lines as part of "git commit -v". We should be able
 to reuse the same function to parse them.

 Once public, it doesn't really belong in mailinfo.c anymore. A better
 place is commit.c because...[I had trouble filling this in; it's really
 _not_ about commits in particular, but rather about the in-editor
 representation of the commit message used by git-commit].

Thinking on it more, though...are the scissors lines generated by "-v" really the same as the ones parsed by mailinfo? In the case of mailinfo, we are parsing an externally generated string according to a heuristic microformat. But with "commit -v", we are the ones who wrote the cut line in the first place. Shouldn't we be a bit more picky and make sure we parse the exact same cut line?

And indeed, we _do_ actually have to parse these cut lines already, when we read the commit message back in. The code is in wt_status_truncate_message_at_cut_line(), and it does do an exact match. We should probably be using that rather than making the mailinfo is_scissors_line() public.

-Peff

On Thu, May 11, 2017 at 10:03:44PM -0700, Brian Malehorn wrote:

Ah, I should have read your cover letter more carefully before responding to the third patch (or alternatively, you should have put your motivating example into the third patch's commit message :) ).

Your example definitely makes sense. But it does make me wonder if this should be an option to interpret-trailers, so that it doesn't accidentally trigger. Unfortunately you'd have to manually specify it (even though you'd presumably have commit.verbose in your config and have long forgotten about it). And at that point, you might as well just say "--no-verbose" on the commit command-line.

My responses so far have all been critical, so let me be positive for a moment. :)

Welcome to the community. Everything in your patches generally looks well-formatted, etc. And I do think you're on the right track with your solution.

As I said, I'm a little iffy on doing this unconditionally, but it may be the least-bad solution. I'd just worry about collateral damage to somebody who doesn't use commit.verbose, but has something scissors-like in their commit message.

If you were to switch out is_scissors_line() for checking the exact cut_line[] from wt-status.c, I think that would be a big improvement. We'd still have the possibility of a false positive, but it would be much less likely in practice.

-Peff

Yes, you're probably right. Using is_scissors_line() was the path of least resistance to fix my bug, but wasn't really the Right Thing.

I've made a new patch that uses the wt-status.c code to determine scissors lines. "git interpret-trailers" now uses the same logic as "git commit". This patch replaces the original 3.

And yeah, this is yet another heuristic in interpret-trailers aimed at git commit messages. But it's hardly the first heuristic we've added, and I'd say it makes more sense for interpret-trailers and commit to parse the same format.

If a commit message is being editted as "verbose", it will contain a scissors string ("-- >8 --") and a diff:

    my subject

    # ------------------------ >8 ------------------------
    # Do not touch the line above.
    # Everything below will be removed.
    diff --git a/foo.txt b/foo.txt
    index 5716ca5..7601807 100644
    --- a/foo.txt
    +++ b/foo.txt
    @@ -1 +1 @@
    -bar
    +baz

interpret-trailers doesn't interpret the scissors and therefore places trailer information after the diff. A simple reproduction is:

    git config commit.verbose true
    GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
        git commit --amend

This commit resolves the issue by teaching "git interpret-trailers" to
obey scissors the same way "git commit" does.
---
 builtin/commit.c              |  3 ++-
 commit.c                      |  6 ++++--
 t/t7513-interpret-trailers.sh | 17 +++++++++++++++++
 wt-status.c                   | 11 ++++++-----
 wt-status.h                   |  2 +-
 5 files changed, 30 insertions(+), 9 deletions(-)

On Sat, May 13, 2017 at 08:39:23PM -0700, Brian Malehorn wrote:

Overall, this patch looks good to me. A few comments below.

The commit message explains the situation much better than the original.

This hunk surprised me at first (that we would need to touch commit.c at all), but the refactoring makes sense.

The change to interpret-trailers here ended up delightfully simple (and looks right to me).

Two minor style nits. One, we'd usually use "\EOF" here unless you really do want to interpolate inside the here document. And two, we usually indent the contents to the same level as the outer cat/EOF pair (I actually don't mind at all how yours looks, but I just happened to notice that it is slightly unlike our usual style).

I can see how the refactor changes this function (and it makes sense), but the "last nonscissors index" seems like a funny term. It is really the length, isn't, and therefore one past the last nonscissors index (or another way of putting it: it's the first index of the scissors).

I wonder if it makes sense to call it "length".

Another way to think of it is still as a truncation. Our strip_suffix() helper behaves quite similarly to this (not actually writing into the buffer, but returning the new length). Perhaps something like "wt_status_strip_scissors" would work.

-Peff

Fixed, and I learned something new.

Fixed.

I was indenting the same as the other tests in that file. But if the way you described is the preferred way, then sure.

I agree the name was pretty awkward. I was trying to avoid using the word "truncate" or "strip", since it doesn't make any change to the buffer. But if strip_suffix() is already around it shouldn't be to surprising.

I've now renamed it to "wt_status_strip_scissors".

If a commit message is being edited as "verbose", it will contain a scissors string ("-- >8 --") and a diff:

    my subject

    # ------------------------ >8 ------------------------
    # Do not touch the line above.
    # Everything below will be removed.
    diff --git a/foo.txt b/foo.txt
    index 5716ca5..7601807 100644
    --- a/foo.txt
    +++ b/foo.txt
    @@ -1 +1 @@
    -bar
    +baz

interpret-trailers doesn't interpret the scissors and therefore places trailer information after the diff. A simple reproduction is:

    git config commit.verbose true
    GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
        git commit --amend

This commit resolves the issue by teaching "git interpret-trailers" to
obey scissors the same way "git commit" does.
---
 builtin/commit.c              |  3 ++-
 commit.c                      | 13 +++++++------
 t/t7513-interpret-trailers.sh | 17 +++++++++++++++++
 wt-status.c                   | 11 ++++++-----
 wt-status.h                   |  2 +-
 5 files changed, 33 insertions(+), 13 deletions(-)

Brian Malehorn <bmalehorn@gmail.com> writes:

I'd prefer to see s/obey scissors lines/honor the cut line/, and the last paragraph rephrased somewhat, perhaps like:

        Subject: interpret-trailers: honor the cut line

        If a commit message is edited with the "verbose" option, the
        buffer will have a cut line and diff after the log message,
        like so:

             my subject

             # ------------------------ >8 ------------------------
             # Do not touch the line above.
             # Everything below will be removed.
             diff --git a/foo.txt b/foo.txt
             index 5716ca5..7601807 100644
             --- a/foo.txt
             +++ b/foo.txt
             @@ -1 +1 @@
             -bar
             +baz

        "git interpret-trailers" is unaware of the cut line, and
        assumes the trailer block would be at the end of the whole
        thing.  This can easily be seen with:

             $ GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
               git commit --amend -v

        Teach "git interpret-trailers" to notice the cut-line and
        ignore the remainder of the input when looking for a place
        to add new trailer block.  This makes it consistent with how
        "git commit -v -s" inserts a new Signed-off-by: line.

        This can be done by the same logic as the existing helper
        function, wt_status_truncate_message_at_cut_line(), uses,
        but it wants the caller to pass a strbuf to it.  Because the
        helper function ignore_non_trailer() used by the command
        takes a <pointer, length> pair, not a strbuf, steal the
        logic from wt_status_truncate_message_at_cut_line() to
        create a new wt_status_strip_scissors() helper function that
        takes <poiter, length> pair, and make ignore_non_trailer()
        call it to help "interpret-trailers".  Since there is only
        one caller of wt_status_truncate_message_at_cut_line() in
        cmd_commit(), rewrite it to call wt_status_strip_scissors()
        helper instead and remove the old helper that no longer has
        any caller.

The last paragraph would have saved me from getting puzzled.

The mention of "'commit -v -s' works that way, too" was my attempt to justify why it is OK to make this change unconditionally to intepret-trailers, but I am still not 100% convinced with that reasoning (or your original log message) that it is a safe thing to do. It is not like its sole purpose is to serve as GIT_EDITOR for the "git commit" command.

The patch text itself looks sensible.  

We still need to see your patch with your sign-off, though.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

And re-reading the above, wt_status_strip_scissors() does not sound right to me.

Yes, I am bikeshedding at this point, but names matter. I prefer to keep the distinction between the two clear by not calling the cut_line[] array "scissors", but in any case, the new function is not about "finding" the cut-line or scissors, in other words, wt_status_locate_scissors() is not a good name either (we do not say "not found" when there is none).

We are locating the logical end of the commit log message. It ends at "---\n", if exists, in the output of "git format-patch", and it ends at the cut-line, if exists, in "commit -v" editor buffer, and if there aren't these funny End-Of-Message marks, then we return the location of the last byte. IOW, the helper function you added will be the place to add more logic in the future if we ever found the need to notice other kinds of "logical end" markers (which may be ones we will invent in the future) while accepting a proposed commit log message.

So how about calling it wt_status_locate_end() or something like that, without limiting ourselves only to the "scissors"?

Alright, I've made a new patch based on this feedback:

* make indentation consistent with rest of test file again
* rename function to wt_status_locate_end()
* rephrase commit message; add paragraph about wt_status_locate_end()
* s/scissors/cut line/
* Signed-off-by: me

Yup, this is a heuristic and it will sometimes hit false. But a far more grievous heuristic is ignoring all lines that begin with '#', yet we do that anyway for its convenience in editing commit messages. Since we already *partially* honor commit message syntax in interpret-trailers, we might as well *completely* honor it by parsing cut lines as well. At least, that's my reasoning.

If a commit message is edited with the "verbose" option, the buffer will have a cut line and diff after the log message, like so:

    my subject

    # ------------------------ >8 ------------------------
    # Do not touch the line above.
    # Everything below will be removed.
    diff --git a/foo.txt b/foo.txt
    index 5716ca5..7601807 100644
    --- a/foo.txt
    +++ b/foo.txt
    @@ -1 +1 @@
    -bar
    +baz

"git interpret-trailers" is unaware of the cut line, and assumes the trailer block would be at the end of the whole thing. This can easily be seen with:

     $ GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
       git commit --amend -v

Teach "git interpret-trailers" to notice the cut-line and ignore the remainder of the input when looking for a place to add new trailer block. This makes it consistent with how "git commit -v -s" inserts a new Signed-off-by: line.

This can be done by the same logic as the existing helper function, wt_status_truncate_message_at_cut_line(), uses, but it wants the caller to pass a strbuf to it. Because the helper function ignore_non_trailer() used by the command takes a <pointer, length> pair, not a strbuf, steal the logic from wt_status_truncate_message_at_cut_line() to create a new wt_status_locate_end() helper function that takes <pointer, length> pair, and make ignore_non_trailer() call it to help "interpret-trailers". Since there is only one caller of wt_status_truncate_message_at_cut_line() in cmd_commit(), rewrite it to call wt_status_locate_end() helper instead and remove the old helper that no longer has any caller.

Signed-off-by: Brian Malehorn <bmalehorn@gmail.com>
---
 builtin/commit.c              |  2 +-
 commit.c                      | 13 +++++++------
 t/t7513-interpret-trailers.sh | 17 +++++++++++++++++
 wt-status.c                   | 11 ++++++-----
 wt-status.h                   |  2 +-
 5 files changed, 32 insertions(+), 13 deletions(-)

Brian Malehorn <bmalehorn@gmail.com> writes:

I was primarily wondering if we want to protect this behind an option to "interpret-trailers", instead of making this change unconditionally. But your above reasoning makes sense to me.

Thanks.

On Sun, May 14, 2017 at 01:33:48AM -0700, Brian Malehorn wrote:

Doh. I didn't notice that. That file _is_ unlike the rest of the code base, but it's generally OK to match the style of surrounding code.

-Peff

Jeff King <peff@peff.net> writes:

So, "git commit --amend --verbose" prepares something like the above example and passes it to the GIT_EDITOR. The problem description indicates that because interpret-trailers (which happens to be used as the GIT_EDITOR) treats the cut-line used by "git commit" just like any other random line, it tries to find the trailer block way below, in the patch text. I agree that it is a problem. But...

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

     my subject
 
     Acked-by: me
     # ------------------------ >8 ------------------------
     # Do not touch the line above.
     # Everything below will be removed.
     diff --git a/foo.txt b/foo.txt
     index 5716ca5..7601807 100644
     --- a/foo.txt
     +++ b/foo.txt
     @@ -1 +1 @@
     -bar
     +baz

after this patch fixes interpret-trailers, wouldn't the cmd_commit() function work as expected without any change?

Puzzled.

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.

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

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

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

Jeff King <peff@peff.net> writes:

I suspect that you would have just explained "since there is only one caller, let's open-code it" in the log message, without making this a two-patch series, and that would also have been perfectly understandable (and in this case probably preferrable).

So the patch text would be OK; it was surprising to have the change without being explained, that's all.

Thanks.

On Mon, May 15, 2017 at 12:32:24PM +0900, Junio C Hamano wrote:

Yeah, I'd agree it could use a better explanation in the commit message. I was surprised to see it, too. Since both of us were surprised, that's probably a good indicator. :)

-Peff

On Sun, May 14, 2017 at 5:39 AM, Brian Malehorn <bmalehorn@gmail.com> wrote:

Typo, should be "edited": https://en.wiktionary.org/wiki/editted

Needed to work with git interpret-trailers. Since "line" is, of course,
a line, it will always end with "\n\0" and therefore we can safely end
on "\n".
---
 mailinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, May 11, 2017 at 10:03:45PM -0700, Brian Malehorn wrote:

Hrm. So I don't think this is _wrong_ per se, but the first thing we do in the loop is check isspace(), which a newline should match. And you can't see it from the diff context, but it always hits a continue, which would then advance to the NUL and exit the loop.

So I'm puzzled why this change is necessary. Am I missing something? We would increase the gap and perforation if we're in them, so I guess it could have an impact on the line-length heuristics that come at the end of the function. Is that it?

-Peff

"scissors" ("----- >8 -----") can be automatically added to commit
messages by setting commit.verbose = true. Prevent this from interfering
with trailer calculations by automatically skipping over scissors,
instead of (usually) treating them as a comment.
---
 commit.c                      | 13 +++++++++----
 t/t7513-interpret-trailers.sh | 17 +++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

On Thu, May 11, 2017 at 10:03:47PM -0700, Brian Malehorn wrote:

What's the sequence of commands where you end up with a scissors line in your "commit -v" output and it gets fed to interpret-trailers? Is it when you run interpret-trailers from a commit-msg hook? Or do we invoke it as part of "commit -v" itself?

I ask because I think we can probably come up with a more realistic test, which may impact what the solution looks like (see below). Grepping for ignore_non_trailer(), it looks like the issue may be the append_signoff() call in builtin/commit.c? I couldn't get "git commit -s -v" to fail, though (it handles the signoff with the verbose bits removed).

This unconditionally ignores scissors lines. But that means if you have any inside your commit message, we may quietly corrupt the commit message. It would be better to remove the scissors lines only when we know that we've added them.

And that's part of my question above. If this is a call happening inside builtin/commit.c, then we definitely know when we've added scissors. But if it's interpret-trailers being run by a hook, passing the information down to this function is a little tricky.

Maybe I'm being overly picky. We already do a lot of gross heuristic stuff here like skipping comments and old-style Conflicts blocks (that we don't even generate anymore!).

-Peff