* git-svn rebase screwing up commit messages @ 2007-07-28 10:07 Benoit SIGOURE 2007-07-28 12:14 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Benoit SIGOURE @ 2007-07-28 10:07 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 418 bytes --] Hello, git version 1.5.2.3 over here, I've googled and searched in the ML archives but did not find this: when I git-svn rebase, my commits in Git (that are temporarily removed and then re-applied) get their commit message flattened on a single line (needless to say this is utterly annoying :D). Is this a bug or something? Thanks. -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 186 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 10:07 git-svn rebase screwing up commit messages Benoit SIGOURE @ 2007-07-28 12:14 ` Junio C Hamano 2007-07-28 12:35 ` Sean 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2007-07-28 12:14 UTC (permalink / raw) To: Benoit SIGOURE; +Cc: git Benoit SIGOURE <tsuna@lrde.epita.fr> writes: > git version 1.5.2.3 over here, I've googled and searched in the ML > archives but did not find this: when I git-svn rebase, my commits in > Git (that are temporarily removed and then re-applied) get their > commit message flattened on a single line... Do you mean by "my commits in Git" a commit you created with git in your git repository? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 12:14 ` Junio C Hamano @ 2007-07-28 12:35 ` Sean 2007-07-28 13:10 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Sean @ 2007-07-28 12:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Benoit SIGOURE, git On Sat, 28 Jul 2007 05:14:26 -0700 Junio C Hamano <gitster@pobox.com> wrote: Hi, > Benoit SIGOURE <tsuna@lrde.epita.fr> writes: > > > git version 1.5.2.3 over here, I've googled and searched in the ML > > archives but did not find this: when I git-svn rebase, my commits in > > Git (that are temporarily removed and then re-applied) get their > > commit message flattened on a single line... > > Do you mean by "my commits in Git" a commit you created with git > in your git repository? > Tested this here (rc3.24.g83b3d) and can confirm the reported problem. After making a commit in git and then running "git svn rebase" to receive updates from the svn repo, the rebased commit has a borked description (multi-lined commit message appears all on one line). Sean ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 12:35 ` Sean @ 2007-07-28 13:10 ` Junio C Hamano 2007-07-28 13:29 ` Sean 2007-07-28 17:33 ` Benoit SIGOURE 0 siblings, 2 replies; 18+ messages in thread From: Junio C Hamano @ 2007-07-28 13:10 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git Sean <seanlkml@sympatico.ca> writes: >> Do you mean by "my commits in Git" a commit you created with git >> in your git repository? > > Tested this here (rc3.24.g83b3d) and can confirm the reported problem. > After making a commit in git and then running "git svn rebase" to > receive updates from the svn repo, the rebased commit has a borked > description (multi-lined commit message appears all on one line). In short, your original commit log message is broken. The recommended convention for commit messages is to start it with a single line that describes what it does, followed by a blank line (i.e. the first paragraph consists of a single line), followed by a longer explanation of why the change brought by the commit is a good thing. Following this convention is recommended to make other peoples' lives more pleasant, and git assumes you follow this convention at several places. For example, "git log --pretty=oneline", "git reflog", and "git show-branch" are ways to get concise listing of commits; git-shortlog gives the list of such commit titles in its output, omitting the longer explanation. Patches prepared for e-mail exchange ("git format-patch", and --pretty=email) use the title on the Subject: line of the message. The behaviour of these tools are _not_ the primarly reasons why you are better off following the commit message convention. The reason behind the behaviour of these tools is to help readers of your commit messages. The punch line comes at the beginning, single line short-and-sweet, and when readers want to get the birds-eye view, that is the only thing they see. Try them on a commit that has several lines in its first paragraph and you will see why this is a good convention. Until recently, we chomped a multi-line first paragraph at the first line, and used the resulting partial sentence as the title. This had an interesting effect on badly formatted commit like this: tree 2a003266ee6fda9305833a4f6e6dc7194018805a parent fef221460121c8a1b9e242422d8b521fbd0f6dc0 author Junio C Hamano <gitster@pobox.com> 1185621830 -0700 committer Junio C Hamano <gitster@pobox.com> 1185621830 -0700 A long and unsightly commit log message that has more than one lines in the first paragraph Such a first paragraph is flattened by --pretty=email When this was formatted for e-mail, it resulted in something like this: From: Junio C Hamano <gitster@pobox.com> Date: Sat, 28 Jul 2007 04:23:50 -0700 Subject: [PATCH] A long and unsightly commit log message that has more than one lines in the first paragraph Such a first paragraph is flattened by --pretty=email As rebase/am uses the same machinery as e-mailed patch acceptance, the paragraph was chomped at the first line, while normalizing it for other git commands' use, like this: tree 842c159f584fd4f970b6aceed21f479c1b62e333 parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44 author Junio C Hamano <gitster@pobox.com> 1185621830 -0700 committer Junio C Hamano <gitster@pobox.com> 1185626445 -0700 A long and unsightly commit log message that has more than one lines in the first paragraph Such a first paragraph is flattened by --pretty=email This would mean that "oneline" format will see only the initial part of the sentence. If your message is properly formatted, it is not a problem. Recent enough git instead uses RFC2822 line-folding like this, to help noncomforming messages somewhat: From 4c04a94... From: Junio C Hamano <gitster@pobox.com> Date: Sat, 28 Jul 2007 04:23:50 -0700 Subject: [PATCH] A long and unsightly commit log message that has more than one lines in the first paragraph Such a first paragraph is flattened by --pretty=email But this has an interesting side effect itself. Such a folded line is logically treated as a single line, and rebase/am makes a commit like this out of such a message: tree 842c159f584fd4f970b6aceed21f479c1b62e333 parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44 author Junio C Hamano <gitster@pobox.com> 1185621830 -0700 committer Junio C Hamano <gitster@pobox.com> 1185626469 -0700 A long and unsightly commit log message that has more than one lines in the first paragraph Such a first paragraph is flattened by --pretty=email which is still unsightly (but the original message is unfriendly to oneline summary format to begin with). At least, this is an improvement in that the new formatting does not lose information when viewed in "oneline" format. Having said all that, so that the readers understand the background, here is a not-so-heavily-tested patch, which might help. It passes all the test suite as before, but that tells how existing git-svn tests do not test many things. I am not considering this for inclusion right now, by the way. -- >8 -- Rebase/am: preserve multi-line first paragraph better. This is actually three patches folded into one; as such it should not be applied as-is. It needs to be split into: * Changes to refs.c::log_ref_write() to sanitize embedded newlines from the log message, instead of chomping it at the first newline; * Changes to symbolic-ref and update-ref, so that they do not refuse a reflog message that has embedded newlines. They have no business in dictating what the reflog message should look like. * Changes to builtin-mailinfo.c to preserve LF in Subject: header, when it is folded using RFC2822 header folding. With this, the first paragraph of a commit message that has multiple lines is reproduced by "am" without being squashed into a single line. --- builtin-mailinfo.c | 29 +++++++++++++++++++++++------ builtin-symbolic-ref.c | 2 -- builtin-update-ref.c | 2 -- refs.c | 39 +++++++++++++++++++++++---------------- 4 files changed, 46 insertions(+), 26 deletions(-) diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c index b4f6e91..9d2064a 100644 --- a/builtin-mailinfo.c +++ b/builtin-mailinfo.c @@ -272,15 +272,15 @@ static char *cleanup_subject(char *subject) } } -static void cleanup_space(char *buf) +static void cleanup_space(char *buf, int keep_LF) { unsigned char c; while ((c = *buf) != 0) { buf++; - if (isspace(c)) { + if (isspace(c) && (!keep_LF || c != '\n')) { buf[-1] = ' '; c = *buf; - while (isspace(c)) { + while (isspace(c) && (!keep_LF || c != '\n')) { int len = strlen(buf); memmove(buf, buf+1, len); c = *buf; @@ -425,6 +425,7 @@ static int read_one_header_line(char *line, int sz, FILE *in) if (addlen >= sz - len) addlen = sz - len - 1; memcpy(line + len, continuation, addlen); + line[len] = '\n'; len += addlen; } } @@ -846,6 +847,22 @@ static void handle_body(void) return; } +static void output_header_lines(FILE *fout, const char *hdr, char *data) +{ + while (1) { + char *ep = strchr(data, '\n'); + int len; + if (!ep) + len = strlen(data); + else + len = ep - data; + fprintf(fout, "%s: %.*s\n", hdr, len, data); + if (!ep) + break; + data = ep + 1; + } +} + static void handle_info(void) { char *sub; @@ -864,14 +881,14 @@ static void handle_info(void) if (!memcmp(header[i], "Subject", 7)) { sub = cleanup_subject(hdr); - cleanup_space(sub); - fprintf(fout, "Subject: %s\n", sub); + cleanup_space(sub, 1); + output_header_lines(fout, "Subject", sub); } else if (!memcmp(header[i], "From", 4)) { handle_from(hdr); fprintf(fout, "Author: %s\n", name); fprintf(fout, "Email: %s\n", email); } else { - cleanup_space(hdr); + cleanup_space(hdr, 0); fprintf(fout, "%s: %s\n", header[i], hdr); } } diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c index d41b406..9eb95e5 100644 --- a/builtin-symbolic-ref.c +++ b/builtin-symbolic-ref.c @@ -43,8 +43,6 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) msg = argv[1]; if (!*msg) die("Refusing to perform update with empty message"); - if (strchr(msg, '\n')) - die("Refusing to perform update with \\n in message"); } else if (!strcmp("--", arg)) { argc--; diff --git a/builtin-update-ref.c b/builtin-update-ref.c index feac2ed..8339cf1 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -23,8 +23,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) msg = argv[++i]; if (!*msg) die("Refusing to perform update with empty message."); - if (strchr(msg, '\n')) - die("Refusing to perform update with \\n in message."); continue; } if (!strcmp("-d", argv[i])) { diff --git a/refs.c b/refs.c index 2694e70..98e3202 100644 --- a/refs.c +++ b/refs.c @@ -1036,6 +1036,27 @@ void unlock_ref(struct ref_lock *lock) free(lock); } +static int copy_msg(char *buf, const char *msg) +{ + char *cp = buf; + char c; + int wasspace = 1; + + *cp++ = '\t'; + while ((c = *msg++)) { + if (wasspace && isspace(c)) + continue; + wasspace = isspace(c); + if (wasspace) + c = ' '; + *cp++ = c; + } + while (buf < cp && isspace(cp[-1])) + cp--; + *cp++ = '\n'; + return cp - buf; +} + static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg) { @@ -1080,21 +1101,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, adjust_shared_perm(log_file); - msglen = 0; - if (msg) { - /* clean up the message and make sure it is a single line */ - for ( ; *msg; msg++) - if (!isspace(*msg)) - break; - if (*msg) { - const char *ep = strchr(msg, '\n'); - if (ep) - msglen = ep - msg; - else - msglen = strlen(msg); - } - } - + msglen = msg ? strlen(msg) : 0; committer = git_committer_info(-1); maxlen = strlen(committer) + msglen + 100; logrec = xmalloc(maxlen); @@ -1103,7 +1110,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, sha1_to_hex(new_sha1), committer); if (msglen) - len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1; + len += copy_msg(logrec + len - 1, msg) - 1; written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); if (close(logfd) != 0 || written != len) ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:10 ` Junio C Hamano @ 2007-07-28 13:29 ` Sean 2007-07-28 13:38 ` Junio C Hamano 2007-07-28 17:33 ` Benoit SIGOURE 1 sibling, 1 reply; 18+ messages in thread From: Sean @ 2007-07-28 13:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Benoit SIGOURE, git On Sat, 28 Jul 2007 06:10:49 -0700 Junio C Hamano <gitster@pobox.com> wrote: > In short, your original commit log message is broken. > > The recommended convention for commit messages is to start it > with a single line that describes what it does, followed by a > blank line (i.e. the first paragraph consists of a single line), > followed by a longer explanation of why the change brought by > the commit is a good thing. That explains why I hadn't seen the problem before, since I usually follow the commit message convention. For testing purposes I had simply mashed the home row to add five or six lines without thinking... > Following this convention is recommended to make other peoples' > lives more pleasant, and git assumes you follow this convention > at several places. For example, "git log --pretty=oneline", > "git reflog", and "git show-branch" are ways to get concise > listing of commits; git-shortlog gives the list of such commit > titles in its output, omitting the longer explanation. Patches > prepared for e-mail exchange ("git format-patch", and > --pretty=email) use the title on the Subject: line of the > message. Yes, Bisecting shows this "problem" was introduced in your commit 4234a76167 which mentions that commit messages following the normal convention would be unaffected. ...[SNIPPED]... > Having said all that, so that the readers understand the > background, here is a not-so-heavily-tested patch, which might > help. It passes all the test suite as before, but that tells > how existing git-svn tests do not test many things. > > I am not considering this for inclusion right now, by the way. FWIW your patch fixed my test case here. Cheers, Sean ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:29 ` Sean @ 2007-07-28 13:38 ` Junio C Hamano 2007-07-28 14:11 ` Sean ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Junio C Hamano @ 2007-07-28 13:38 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git Sean <seanlkml@sympatico.ca> writes: >> Having said all that, so that the readers understand the >> background, here is a not-so-heavily-tested patch, which might >> help. It passes all the test suite as before, but that tells >> how existing git-svn tests do not test many things. >> >> I am not considering this for inclusion right now, by the way. > > FWIW your patch fixed my test case here. Actually the patched behaviour actively encourages a bad (not in the sense that those oneline tools will not work well, but in the sense that these messages are reader unfriendly) practice; I do not think what the patch did deserves to be called "fixed". And that is one of the reasons, other than that we are in -rc freeze that we do not add anything but unarguable fixes, that I am not considering the patch for inclusion right now. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:38 ` Junio C Hamano @ 2007-07-28 14:11 ` Sean 2007-07-28 20:41 ` Junio C Hamano 2007-07-28 14:32 ` git-svn rebase screwing up commit messages Jakub Narebski 2007-07-28 19:48 ` Robin Rosenberg 2 siblings, 1 reply; 18+ messages in thread From: Sean @ 2007-07-28 14:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Benoit SIGOURE, git On Sat, 28 Jul 2007 06:38:19 -0700 Junio C Hamano <gitster@pobox.com> wrote: > Actually the patched behaviour actively encourages a bad (not in > the sense that those oneline tools will not work well, but in > the sense that these messages are reader unfriendly) practice; I > do not think what the patch did deserves to be called "fixed". > And that is one of the reasons, other than that we are in -rc > freeze that we do not add anything but unarguable fixes, that I > am not considering the patch for inclusion right now. > First, i didn't read the patch and i have no stake at all in non-conforming commit messages; i always follow the convention. Having said that, the current behavior of Git crosses the line from advocating the common commit message format into the realm of not-working properly with non-conforming commit messages. I would argue that you shouldn't try to have it both ways. Either Git supports non-conforming message formats, in which case the current behavior seems buggy. Or Git does not support commit messages that deviate from the standard, in which case the documentation should be updated to state so bluntly. If handling email-mangled commit messages means that non-conforming formats can no longer be fully supported (while discouraged) then perhaps the time has come to explicitly state that people should not expect Git to handle anything but the 1+empty+description commit message format. Sean ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 14:11 ` Sean @ 2007-07-28 20:41 ` Junio C Hamano 2007-07-29 1:08 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2007-07-28 20:41 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git Sean <seanlkml@sympatico.ca> writes: > I would argue that you shouldn't try to have it both ways. You cannot have it both ways as-is, but this is solvable. The invocation of am from rebase needs an extra (internal to implementation) option to use the code it patches, and the regular am can fold what are found on Subject: lines it used to. The patch as-is breaks the more important case of e-mail acceptance codepath, because MUAs are free to fold the Subject: line when the original line is long, and what the user (the original patch submitter) expects to happen is that a single-line-ness of the original Subject: of the message to be kept. The patch breaks such a line at the place MUA happens to fold such a long, single line, for comforming messages. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 20:41 ` Junio C Hamano @ 2007-07-29 1:08 ` Junio C Hamano 2007-07-29 1:10 ` [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Junio C Hamano @ 2007-07-29 1:08 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git Junio C Hamano <gitster@pobox.com> writes: > Sean <seanlkml@sympatico.ca> writes: > >> I would argue that you shouldn't try to have it both ways. > > You cannot have it both ways as-is, but this is solvable. The > invocation of am from rebase needs an extra (internal to > implementation) option to use the code it patches, and the > regular am can fold what are found on Subject: lines it used > to. > > The patch as-is breaks the more important case of e-mail > acceptance codepath, because MUAs are free to fold the Subject: > line when the original line is long, and what the user (the > original patch submitter) expects to happen is that a > single-line-ness of the original Subject: of the message to be > kept. The patch breaks such a line at the place MUA happens to > fold such a long, single line, for comforming messages. So here comes an updated series, properly split along the logical lines. 0001-log_ref_write-do-not-chomp-reflog-message-at-the.txt 0002-symbolic-ref-update-ref-do-not-refuse-reflog-message.txt 0003-rebase-try-not-to-munge-commit-log-message.txt I am not absolutely sure about the implications of mailinfo change. I tried to be careful by making sure that: - the updated code kicks in when "format-patch piped to am" toolchain uses the -k option on both ends, as rebase/am does. This is the case where we would want to keep the way original commit log message was formatted. - otherwise the original "line folding" clean-up is used, so that usual e-mailed patch acceptance codepath cleanses the oneline summary obtained from the "Subject:" header. Because it would be a serious regression to break the latter, while the former is just "an improvement" [*1], somebody really should double check the change for me. [Footnote] *1* It does not mean that improved support for foreign SCM interoperation does not matter. It is "merely an improvement" in the sense that people do not like what the code currently does anyway, and if the updated code is still not liked, then not having such "an improvement" does not matter. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF 2007-07-29 1:08 ` Junio C Hamano @ 2007-07-29 1:10 ` Junio C Hamano 2007-07-29 11:57 ` Johannes Schindelin 2007-07-29 1:10 ` [PATCH 2/3] symbolic-ref, update-ref: do not refuse reflog message with LF Junio C Hamano 2007-07-29 1:11 ` [PATCH 3/3] rebase: try not to munge commit log message Junio C Hamano 2 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2007-07-29 1:10 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git A reflog file is organized as one-line-per-entry records, and we enforced the file format integrity by chomping the given message at the first LF. This changes it to convert them to SP, which is more in line with the --pretty=oneline format. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- refs.c | 44 ++++++++++++++++++++++++++++---------------- 1 files changed, 28 insertions(+), 16 deletions(-) diff --git a/refs.c b/refs.c index 2694e70..fac6548 100644 --- a/refs.c +++ b/refs.c @@ -1036,6 +1036,32 @@ void unlock_ref(struct ref_lock *lock) free(lock); } +/* + * copy the reflog message msg to buf, which has been allocated sufficiently + * large, while cleaning up the whitespaces. Especially, convert LF to space, + * because reflog file is one line per entry. + */ +static int copy_msg(char *buf, const char *msg) +{ + char *cp = buf; + char c; + int wasspace = 1; + + *cp++ = '\t'; + while ((c = *msg++)) { + if (wasspace && isspace(c)) + continue; + wasspace = isspace(c); + if (wasspace) + c = ' '; + *cp++ = c; + } + while (buf < cp && isspace(cp[-1])) + cp--; + *cp++ = '\n'; + return cp - buf; +} + static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg) { @@ -1080,21 +1106,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, adjust_shared_perm(log_file); - msglen = 0; - if (msg) { - /* clean up the message and make sure it is a single line */ - for ( ; *msg; msg++) - if (!isspace(*msg)) - break; - if (*msg) { - const char *ep = strchr(msg, '\n'); - if (ep) - msglen = ep - msg; - else - msglen = strlen(msg); - } - } - + msglen = msg ? strlen(msg) : 0; committer = git_committer_info(-1); maxlen = strlen(committer) + msglen + 100; logrec = xmalloc(maxlen); @@ -1103,7 +1115,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, sha1_to_hex(new_sha1), committer); if (msglen) - len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1; + len += copy_msg(logrec + len - 1, msg) - 1; written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); if (close(logfd) != 0 || written != len) -- 1.5.3.rc3.24.g83b3d ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF 2007-07-29 1:10 ` [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF Junio C Hamano @ 2007-07-29 11:57 ` Johannes Schindelin 2007-07-29 18:47 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Johannes Schindelin @ 2007-07-29 11:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sean, Benoit SIGOURE, git Hi, On Sat, 28 Jul 2007, Junio C Hamano wrote: > A reflog file is organized as one-line-per-entry records, and we > enforced the file format integrity by chomping the given message > at the first LF. This changes it to convert them to SP, which > is more in line with the --pretty=oneline format. Would it not be better to chop off before the first "\n", and just append "..."? IOW something like -- snip -- refs.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 2694e70..554436b 100644 --- a/refs.c +++ b/refs.c @@ -1043,7 +1043,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, unsigned maxlen, len; int msglen; char *log_file, *logrec; - const char *committer; + const char *committer, *postmsg; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); @@ -1088,15 +1088,16 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, break; if (*msg) { const char *ep = strchr(msg, '\n'); - if (ep) + if (ep) { msglen = ep - msg; - else + postmsg = (ep[1] && !isspace(ep[1])) ? "..." : NULL; + } else msglen = strlen(msg); } } committer = git_committer_info(-1); - maxlen = strlen(committer) + msglen + 100; + maxlen = strlen(committer) + msglen + 100 + 3; logrec = xmalloc(maxlen); len = sprintf(logrec, "%s %s %s\n", sha1_to_hex(old_sha1), @@ -1104,6 +1105,10 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, committer); if (msglen) len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1; + if (postmsg) { + len += strlen(postmsg); + strcat(logrec + len - 1, postmsg); + } written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); if (close(logfd) != 0 || written != len) -- snap -- It is not like the reflog messages have to be very verbose; they only have to give a hint what the commit was about, and the commit name is the important information. What do you think? Ciao, Dscho ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF 2007-07-29 11:57 ` Johannes Schindelin @ 2007-07-29 18:47 ` Junio C Hamano 2007-07-29 19:02 ` Johannes Schindelin 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2007-07-29 18:47 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Sean, Benoit SIGOURE, git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > It is not like the reflog messages have to be very verbose; they only have > to give a hint what the commit was about, and the commit name is the > important information. I've considered it, but decided against it because: (1) I did not like the information lossage; (2) this is solely to help log messages coming from outside the recommended convention, and having excess will not hurt the normal usage; (3) it is not known if messages from outside the recommended convention have enough information on its first line of the first paragraph; the result of s/\n.*/.../ may not be sufficient "hint"; (4) log output are by default piped to "less -S" so extra output gives the user chance to inspect more if he chosses to, without hurting the end user display; and (5) the code was simpler and less error prone to go wrong. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF 2007-07-29 18:47 ` Junio C Hamano @ 2007-07-29 19:02 ` Johannes Schindelin 0 siblings, 0 replies; 18+ messages in thread From: Johannes Schindelin @ 2007-07-29 19:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sean, Benoit SIGOURE, git Hi, On Sun, 29 Jul 2007, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > It is not like the reflog messages have to be very verbose; they only have > > to give a hint what the commit was about, and the commit name is the > > important information. > > I've considered it, but decided against it because: > > (1) I did not like the information lossage; > > (2) this is solely to help log messages coming from outside the > recommended convention, and having excess will not hurt the > normal usage; > > (3) it is not known if messages from outside the recommended > convention have enough information on its first line of the > first paragraph; the result of s/\n.*/.../ may not be > sufficient "hint"; Good catch. Didn't think of that. Ciao, Dscho ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] symbolic-ref, update-ref: do not refuse reflog message with LF 2007-07-29 1:08 ` Junio C Hamano 2007-07-29 1:10 ` [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF Junio C Hamano @ 2007-07-29 1:10 ` Junio C Hamano 2007-07-29 1:11 ` [PATCH 3/3] rebase: try not to munge commit log message Junio C Hamano 2 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2007-07-29 1:10 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git Earlier these tools refused to create a reflog entry when the message given by the calling Porcelain had a LF in it, partially to keep the file format integrity of reflog file, which is one-entry-per-line. These tools should not be dictating such a policy. Instead, let the codepath to write out the reflog entry worry about the format integrity and allow messages with LF in them. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin-symbolic-ref.c | 2 -- builtin-update-ref.c | 2 -- 2 files changed, 0 insertions(+), 4 deletions(-) diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c index d41b406..9eb95e5 100644 --- a/builtin-symbolic-ref.c +++ b/builtin-symbolic-ref.c @@ -43,8 +43,6 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) msg = argv[1]; if (!*msg) die("Refusing to perform update with empty message"); - if (strchr(msg, '\n')) - die("Refusing to perform update with \\n in message"); } else if (!strcmp("--", arg)) { argc--; diff --git a/builtin-update-ref.c b/builtin-update-ref.c index feac2ed..8339cf1 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -23,8 +23,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) msg = argv[++i]; if (!*msg) die("Refusing to perform update with empty message."); - if (strchr(msg, '\n')) - die("Refusing to perform update with \\n in message."); continue; } if (!strcmp("-d", argv[i])) { -- 1.5.3.rc3.24.g83b3d ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] rebase: try not to munge commit log message 2007-07-29 1:08 ` Junio C Hamano 2007-07-29 1:10 ` [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF Junio C Hamano 2007-07-29 1:10 ` [PATCH 2/3] symbolic-ref, update-ref: do not refuse reflog message with LF Junio C Hamano @ 2007-07-29 1:11 ` Junio C Hamano 2 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2007-07-29 1:11 UTC (permalink / raw) To: Sean; +Cc: Benoit SIGOURE, git This makes rebase/am keep the original commit log message better, even when it does not conform to "single line paragraph to say what it does, then explain and defend why it is a good change in later paragraphs" convention. This change is a two-edged sword. While the earlier behaviour would make such commit log messages more friendly to readers who expect to get the birds-eye view with oneline summary formats, users who primarily use git as a way to interact with foreign SCM systems would not care much about the convenience of oneline git log tools, but care more about preserving their own convention. This changes their commits less useful to readers who read them with git tools while keeping them more consistent with the foreign SCM systems they interact with. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin-mailinfo.c | 29 +++++++++++++++++++++---- t/t3405-rebase-malformed.sh | 48 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) create mode 100755 t/t3405-rebase-malformed.sh diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c index b4f6e91..b558754 100644 --- a/builtin-mailinfo.c +++ b/builtin-mailinfo.c @@ -237,8 +237,6 @@ static int eatspace(char *line) static char *cleanup_subject(char *subject) { - if (keep_subject) - return subject; for (;;) { char *p; int len, remove; @@ -425,6 +423,7 @@ static int read_one_header_line(char *line, int sz, FILE *in) if (addlen >= sz - len) addlen = sz - len - 1; memcpy(line + len, continuation, addlen); + line[len] = '\n'; len += addlen; } } @@ -846,6 +845,22 @@ static void handle_body(void) return; } +static void output_header_lines(FILE *fout, const char *hdr, char *data) +{ + while (1) { + char *ep = strchr(data, '\n'); + int len; + if (!ep) + len = strlen(data); + else + len = ep - data; + fprintf(fout, "%s: %.*s\n", hdr, len, data); + if (!ep) + break; + data = ep + 1; + } +} + static void handle_info(void) { char *sub; @@ -863,9 +878,13 @@ static void handle_info(void) continue; if (!memcmp(header[i], "Subject", 7)) { - sub = cleanup_subject(hdr); - cleanup_space(sub); - fprintf(fout, "Subject: %s\n", sub); + if (keep_subject) + sub = hdr; + else { + sub = cleanup_subject(hdr); + cleanup_space(sub); + } + output_header_lines(fout, "Subject", sub); } else if (!memcmp(header[i], "From", 4)) { handle_from(hdr); fprintf(fout, "Author: %s\n", name); diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh new file mode 100755 index 0000000..e4e2e64 --- /dev/null +++ b/t/t3405-rebase-malformed.sh @@ -0,0 +1,48 @@ +#!/bin/sh + +test_description='rebase should not insist on git message convention' + +. ./test-lib.sh + +cat >F <<\EOF +This is an example of a commit log message +that does not conform to git commit convention. + +It has two paragraphs, but its first paragraph is not friendly +to oneline summary format. +EOF + +test_expect_success setup ' + + >file1 && + >file2 && + git add file1 file2 && + test_tick && + git commit -m "Initial commit" && + + git checkout -b side && + cat F >file2 && + git add file2 && + test_tick && + git commit -F F && + + git cat-file commit HEAD | sed -e "1,/^\$/d" >F0 && + + git checkout master && + + echo One >file1 && + test_tick && + git add file1 && + git commit -m "Second commit" +' + +test_expect_success rebase ' + + git rebase master side && + git cat-file commit HEAD | sed -e "1,/^\$/d" >F1 && + + diff -u F0 F1 && + diff -u F F0 +' + +test_done -- 1.5.3.rc3.24.g83b3d ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:38 ` Junio C Hamano 2007-07-28 14:11 ` Sean @ 2007-07-28 14:32 ` Jakub Narebski 2007-07-28 19:48 ` Robin Rosenberg 2 siblings, 0 replies; 18+ messages in thread From: Jakub Narebski @ 2007-07-28 14:32 UTC (permalink / raw) To: git Junio C Hamano wrote: > Sean <seanlkml@sympatico.ca> writes: > >>> Having said all that, so that the readers understand the >>> background, here is a not-so-heavily-tested patch, which might >>> help. It passes all the test suite as before, but that tells >>> how existing git-svn tests do not test many things. >>> >>> I am not considering this for inclusion right now, by the way. >> >> FWIW your patch fixed my test case here. > > Actually the patched behaviour actively encourages a bad (not in > the sense that those oneline tools will not work well, but in > the sense that these messages are reader unfriendly) practice; I > do not think what the patch did deserves to be called "fixed". > > And that is one of the reasons, other than that we are in -rc > freeze that we do not add anything but unarguable fixes, that I > am not considering the patch for inclusion right now. I think that git should not enforce this policy. Think import and exchange with foreign SCMs which do not follow this, argueably very reasonable, one-line summary policy. -- Jakub Narebski Warsaw, Poland ShadeHawk on #git ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:38 ` Junio C Hamano 2007-07-28 14:11 ` Sean 2007-07-28 14:32 ` git-svn rebase screwing up commit messages Jakub Narebski @ 2007-07-28 19:48 ` Robin Rosenberg 2 siblings, 0 replies; 18+ messages in thread From: Robin Rosenberg @ 2007-07-28 19:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sean, Benoit SIGOURE, git lördag 28 juli 2007 skrev Junio C Hamano: > Sean <seanlkml@sympatico.ca> writes: > > >> Having said all that, so that the readers understand the > >> background, here is a not-so-heavily-tested patch, which might > >> help. It passes all the test suite as before, but that tells > >> how existing git-svn tests do not test many things. > >> > >> I am not considering this for inclusion right now, by the way. > > > > FWIW your patch fixed my test case here. > > Actually the patched behaviour actively encourages a bad (not in > the sense that those oneline tools will not work well, but in > the sense that these messages are reader unfriendly) practice; I > do not think what the patch did deserves to be called "fixed". git-svn should not enforce git conventions when managing commits intended for svn. Those commits should obviously follow the conventions for the target (svn) repo. -- robin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: git-svn rebase screwing up commit messages 2007-07-28 13:10 ` Junio C Hamano 2007-07-28 13:29 ` Sean @ 2007-07-28 17:33 ` Benoit SIGOURE 1 sibling, 0 replies; 18+ messages in thread From: Benoit SIGOURE @ 2007-07-28 17:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 1655 bytes --] On Jul 28, 2007, at 3:10 PM, Junio C Hamano wrote: > Sean <seanlkml@sympatico.ca> writes: > >>> Do you mean by "my commits in Git" a commit you created with git >>> in your git repository? >> >> Tested this here (rc3.24.g83b3d) and can confirm the reported >> problem. >> After making a commit in git and then running "git svn rebase" to >> receive updates from the svn repo, the rebased commit has a borked >> description (multi-lined commit message appears all on one line). > > In short, your original commit log message is broken. > > The recommended convention for commit messages is to start it > with a single line that describes what it does, followed by a > blank line (i.e. the first paragraph consists of a single line), > followed by a longer explanation of why the change brought by > the commit is a good thing. > Hi people, thanks for the quick and complete replies. As a user, I do not expect git-rebase to change my commit message. I was aware of this convention in Git, but it looks like it's more than just a convention. This trap should either be fixed (your patch works for me) or it should be clearly stated in the documentation that commit messages must really be formatted according to the convention. Personally I expected Git to keep the 1st line of my commit message as the "short version" (which is what git log and the like do ATM) and although I don't leave a blank line between the 1st line and the rest of the message, the 1st line is always the sentence that can be used as a short version of the commit message. Cheers, -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 186 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2007-07-29 19:02 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-07-28 10:07 git-svn rebase screwing up commit messages Benoit SIGOURE 2007-07-28 12:14 ` Junio C Hamano 2007-07-28 12:35 ` Sean 2007-07-28 13:10 ` Junio C Hamano 2007-07-28 13:29 ` Sean 2007-07-28 13:38 ` Junio C Hamano 2007-07-28 14:11 ` Sean 2007-07-28 20:41 ` Junio C Hamano 2007-07-29 1:08 ` Junio C Hamano 2007-07-29 1:10 ` [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF Junio C Hamano 2007-07-29 11:57 ` Johannes Schindelin 2007-07-29 18:47 ` Junio C Hamano 2007-07-29 19:02 ` Johannes Schindelin 2007-07-29 1:10 ` [PATCH 2/3] symbolic-ref, update-ref: do not refuse reflog message with LF Junio C Hamano 2007-07-29 1:11 ` [PATCH 3/3] rebase: try not to munge commit log message Junio C Hamano 2007-07-28 14:32 ` git-svn rebase screwing up commit messages Jakub Narebski 2007-07-28 19:48 ` Robin Rosenberg 2007-07-28 17:33 ` Benoit SIGOURE
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).