Re: [RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
Jeff King p...@peff.net writes: I found this hard to parse, I think because of the keeping (why would I not keep it?), and because you are talking about lines above and below. It is not as accurate to say: # -- 8 # Everything below this line will be removed. because it is technically the line above that is the cutoff. But I think the meaning is clear, and it is simpler to parse. I do think it would be simpler with a single line. I know handling the i18n was a question there, but I think we should be fine as long as we check for the exact bytes we wrote. Surely gettext can do something like: magic = _(# Everything below this line will be removed); fprintf(fh, %s, magic); ... p = strstr(magic); I don't know what guarantees on string lifetime gettext gives us, but the worst case is that we simply strdup the result. I suppose it's possible that the translated string could have utf8 with multiple representations, and the user's editor normalizes the text in a different way than we wrote it when it saves the result. I don't know if that is worth caring about or not; it seems kind of insane. I agree with your rewording suggestion. It might make it even more robust to do something like const char cut_here[] = # --- cut here --- 8 --- cut here ---; fprintf(fh, %s\n, cut_here); fputs(_(# Everything below this line will be removed\n), fh); ... p = strstr(cut_here); i.e. a real marker line that will never be translated, with an explanation immediately below that can be translated. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
On Sat, Nov 16, 2013 at 07:22:29PM -0500, Eric Sunshine wrote:
/* Truncate the message just before the diff, if any. */
if (verbose) {
- p = strstr(sb.buf, \ndiff --git );
- if (p != NULL)
- strbuf_setlen(sb, p - sb.buf + 1);
+ p = strstr(sb.buf, wt_status_diff_divider);
Would it make sense to use the more flexible is_scissors_line() from
builtin/mailinfo.c here?
I don't think so. We are not trying to be friendly to a remote source
which has given us an arbitrarily-written scissor line. Rather the
opposite: we are trying to be very strict only to break on the line we
have included ourselves.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
Am 17.11.2013 10:09, schrieb Jeff King:
diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..091a6e7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1602,9 +1602,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
/* Truncate the message just before the diff, if any. */
if (verbose) {
-p = strstr(sb.buf, \ndiff --git );
-if (p != NULL)
-strbuf_setlen(sb, p - sb.buf + 1);
+p = strstr(sb.buf, wt_status_diff_divider);
+if ((p != NULL) (p sb.buf) (p[-1] == '\n'))
+strbuf_setlen(sb, p - sb.buf);
I think your check for a preceding newline is too strict. If I delete
everything before the scissor line (e.g., because I am trying to abort
the commit), we should still remove the diff. With your patch, we do
not, and a commit message consisting solely of the diff.
So I think you want:
if (p (p == sb.buf || p[-1] == '\n'))
Thanks for catching this, will do so in v2.
+fprintf(s-fp, _(# The diff below will be removed when keeping the
previous line.\n));
I found this hard to parse, I think because of the keeping (why would
I not keep it?), and because you are talking about lines above and
below. It is not as accurate to say:
# -- 8
# Everything below this line will be removed.
because it is technically the line above that is the cutoff. But I think
the meaning is clear, and it is simpler to parse.
Ok.
I do think it would be simpler with a single line. I know handling the
i18n was a question there, but I think we should be fine as long as we
check for the exact bytes we wrote. Surely gettext can do something
like:
magic = _(# Everything below this line will be removed);
fprintf(fh, %s, magic);
...
p = strstr(magic);
I don't know what guarantees on string lifetime gettext gives us, but
the worst case is that we simply strdup the result.
I suppose it's possible that the translated string could have utf8 with
multiple representations, and the user's editor normalizes the text in a
different way than we wrote it when it saves the result. I don't know if
that is worth caring about or not; it seems kind of insane.
I don't have any strong feelings about this one. I'd be fine with
dropping the scissor line and taking the translated string as divider
line. What do others think?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
When using the '-v' option of git commit the diff added to the commit
message temporarily for editing is stripped off after the user exited the
editor by searching for \ndiff --git and truncating the commmit message
there if it is found. But this approach has two problems: when the commit
message itself contains a line starting with diff --git it will be
truncated there prematurely. And when the diff.submodule setting is set
to log, the diff may start with Submodule hash1..hash2, which will
be left in the commit message while it shouldn't.
Fix that by introducing a special scissor separator line starting with the
comment character '#' followed by a line describing what it is for. The
scissor line is used to reliably detect the start of the diff so it can be
chopped off from the commit message, no matter what the user enters there.
Turn a known test failure fixed by this change into a successful test and
add another one for a diff starting with a submodule log.
Reported-by: Ari Pollak a...@debian.org
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Am 13.11.2013 21:04, schrieb Junio C Hamano:
Jens Lehmann jens.lehm...@web.de writes:
If we were introducing a divider line for machine consumption, I do
not think it is wise to let that line even translated...
Ok, but then it won't mean much to readers who don't understand
English. I assume prefixing all diff lines with # is out of
the question because of backwards compatibility, so what about
using a descriptive text together with a scissor line? The former
can be be translated (and won't make it into the commit message
because it starts with a #) while the latter serves as a robust
divider line:
# Everything below the following line is a diff that will be removed.
# 8
Yeah, or swap them around if you are trying to protect the part
above the divider from getting contaminated by the noise.
Ok, did that. I couldn't find another user of the verbose setting,
so emitting the scissor line together with the description inside
wt_status_print_verbose() should be ok (or did I miss something
here?). But currently these lines use the hardcoded '#' instead of
the status.displaycommentprefix configuration and ignores coloring,
looks like I need to fix this in the next iteration.
builtin/commit.c | 6 +++---
t/t7507-commit-verbose.sh | 15 ++-
wt-status.c | 4
wt-status.h | 2 ++
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..091a6e7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1602,9 +1602,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
/* Truncate the message just before the diff, if any. */
if (verbose) {
- p = strstr(sb.buf, \ndiff --git );
- if (p != NULL)
- strbuf_setlen(sb, p - sb.buf + 1);
+ p = strstr(sb.buf, wt_status_diff_divider);
+ if ((p != NULL) (p sb.buf) (p[-1] == '\n'))
+ strbuf_setlen(sb, p - sb.buf);
}
if (cleanup_mode != CLEANUP_NONE)
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..09c1150 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -65,9 +65,22 @@ test_expect_success 'diff in message is retained without -v'
'
check_message diff
'
-test_expect_failure 'diff in message is retained with -v' '
+test_expect_success 'diff in message is retained with -v' '
git commit --amend -F diff -v
check_message diff
'
+test_expect_success 'submodule log is stripped out too with -v' '
+ git config diff.submodule log
+ git submodule add ./. sub
+ git commit -m sub added
+ (
+ cd sub
+ echo more file
+ git commit -a -m submodule commit
+ )
+ GIT_EDITOR=cat test_must_fail git commit -a -v 2err
+ test_i18ngrep Aborting commit due to empty commit message. err
+'
+
test_done
diff --git a/wt-status.c b/wt-status.c
index b4e44ba..a3f7115 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,8 @@
#include column.h
#include strbuf.h
+const char wt_status_diff_divider[] = #
---8---\n;
+
static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
GIT_COLOR_GREEN, /* WT_STATUS_UPDATED */
@@ -791,6 +793,8 @@ static void wt_status_print_verbose(struct wt_status *s)
*/
if (s-fp != stdout)
rev.diffopt.use_color = 0;
+ fprintf(s-fp, wt_status_diff_divider);
+ fprintf(s-fp, _(# The diff below will be removed when keeping the
previous line.\n));
run_diff_index(rev, 1);
}
diff --git a/wt-status.h b/wt-status.h
index 6c29e6f..cd2709f 100644
---
Re: [RFC PATCH] commit -v: strip diffs and submodule shortlogs from the commit message
On Sat, Nov 16, 2013 at 5:52 PM, Jens Lehmann jens.lehm...@web.de wrote:
When using the '-v' option of git commit the diff added to the commit
message temporarily for editing is stripped off after the user exited the
editor by searching for \ndiff --git and truncating the commmit message
there if it is found. But this approach has two problems: when the commit
message itself contains a line starting with diff --git it will be
truncated there prematurely. And when the diff.submodule setting is set
to log, the diff may start with Submodule hash1..hash2, which will
be left in the commit message while it shouldn't.
Fix that by introducing a special scissor separator line starting with the
comment character '#' followed by a line describing what it is for. The
scissor line is used to reliably detect the start of the diff so it can be
chopped off from the commit message, no matter what the user enters there.
Turn a known test failure fixed by this change into a successful test and
add another one for a diff starting with a submodule log.
Reported-by: Ari Pollak a...@debian.org
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
builtin/commit.c | 6 +++---
t/t7507-commit-verbose.sh | 15 ++-
wt-status.c | 4
wt-status.h | 2 ++
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..091a6e7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1602,9 +1602,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
/* Truncate the message just before the diff, if any. */
if (verbose) {
- p = strstr(sb.buf, \ndiff --git );
- if (p != NULL)
- strbuf_setlen(sb, p - sb.buf + 1);
+ p = strstr(sb.buf, wt_status_diff_divider);
Would it make sense to use the more flexible is_scissors_line() from
builtin/mailinfo.c here?
+ if ((p != NULL) (p sb.buf) (p[-1] == '\n'))
+ strbuf_setlen(sb, p - sb.buf);
}
if (cleanup_mode != CLEANUP_NONE)
diff --git a/wt-status.c b/wt-status.c
index b4e44ba..a3f7115 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,8 @@
#include column.h
#include strbuf.h
+const char wt_status_diff_divider[] = #
---8---\n;
+
static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
GIT_COLOR_GREEN, /* WT_STATUS_UPDATED */
@@ -791,6 +793,8 @@ static void wt_status_print_verbose(struct wt_status *s)
*/
if (s-fp != stdout)
rev.diffopt.use_color = 0;
+ fprintf(s-fp, wt_status_diff_divider);
+ fprintf(s-fp, _(# The diff below will be removed when keeping the
previous line.\n));
run_diff_index(rev, 1);
}
diff --git a/wt-status.h b/wt-status.h
index 6c29e6f..cd2709f 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -91,6 +91,8 @@ struct wt_status_state {
unsigned char cherry_pick_head_sha1[20];
};
+const char wt_status_diff_divider[];
+
void wt_status_prepare(struct wt_status *s);
void wt_status_print(struct wt_status *s);
void wt_status_collect(struct wt_status *s);
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
