Re: More builtin git-am issues..

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> [1] I think part of the reason people are interested in "fancy" here is
>     that this topic extends beyond "git am". There's "commit -s", of
>     course, but there's also the generic "interpret-trailers" command,
>     which is supposed to be a generalization of the "--signoff" option.
>     It would be nice if the rules remained consistent for when we add a
>     trailer to an existing block, rather than special-casing signoffs.
>
>     But again, I think that's something to shoot for in the long run.
>     It's more important in the short term not to regress "am".

Yes.  I personally think the global append_signoff() everybody else
uses can and should implement the same logic (whichever the exact
logic is) as whatever is used by "am" because the caller that makes
a call to that function knows it is adding a "Signed-off-by:" line,
so existing "Signed-off-by" lines are already special in that context.

But for the purpose of 2.6-rc period, I think we should start from
doing a separate implementation inside builtin/am.c without touching
append_signoff().

We can do a more thoughtful refactoring for append_signoff() in the
next cycle.  Another thing that needs consideration is the other
user of the has_conforming_footer() helper append_signoff() uses,
which is the codepath to add "cherry-picked-from"; it is less
obvious that "find Signed-off-by: anywhere in the text to decide if
the new footer needs its own paragraph" is a good logic in that
context.

To salvage "interpret-trailers" needs a lot more, as we are
realizing that the definition that led to its external design does
not match the way users use footers in the real world.  This affects
the internal data representation and the whole thing needs to be
rethought.

Here is a quick attempt to do the "just fix am regression without
changing anything else".


 builtin/am.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 83b3d86..c63d238 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1213,9 +1213,24 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 static void am_append_signoff(struct am_state *state)
 {
 	struct strbuf sb = STRBUF_INIT;
+	char *cp;
 
 	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
-	append_signoff(&sb, 0, 0);
+	strbuf_complete_line(&sb);
+
+	for (cp = sb.buf;
+	     cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
+	     cp = strchr(cp, '\n')) {
+		if (sb.buf < cp && cp[-1] == '\n')
+			break;
+	}
+	if (!cp)
+		strbuf_addch(&sb, '\n');
+	strbuf_addf(&sb, "%s%s\n",
+		    sign_off_header,
+		    fmt_name(getenv("GIT_COMMITTER_NAME"),
+			     getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sb, '\n');
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }