[PATCHv5 0/8] Add commit message options for rebase --autosquash

[PATCHv5 0/8] Add commit message options for rebase --autosquash

[Pat Notz]

This patch series adds new command line options to git-commit to make
it easy to specify messages for commits correctly formatted for use
wit 'rebase -i --autosquash'.

The first three patches introduce minor refactorings that set the stage
for subsequent patches.

The fourth patch teaches format_commit_message to reencode the content
if the caller's commit object uses an encoding different from the
commit encoding.

The remaining patches add the --fixup and --squash commands to
git-commit including tests of commit, interactions with rebase and
i18n encodings.  

One issue which limits the testing (but not the implementation, I
think) is that when 'rebase --autosquash' is comparing commit subject
lines it does not first make sure that the commits use a common
encoding.  That's follow-on work.

Pat Notz (8):
  commit.c: prefer get_header() to manual searching
  commit.c: new function for looking up a comit by name
  pretty.c: helper methods for getting output encodings
  pretty.c: teach format_commit_message() to reencode the output
  commit: --fixup option for use with rebase --autosquash
  add tests of commit --fixup
  commit: --squash option for use with rebase --autosquash
  add tests of commit --squash

 Documentation/git-commit.txt |   21 +++++++++--
 archive.c                    |    2 +-
 builtin/commit.c             |   76 +++++++++++++++++++++++++++------------
 builtin/fmt-merge-msg.c      |    2 +-
 cache.h                      |    3 ++
 commit.c                     |   13 +++++++
 commit.h                     |    5 ++-
 environment.c                |   11 ++++++
 log-tree.c                   |    2 +-
 notes-cache.c                |    2 +-
 pretty.c                     |   47 +++++++++++++++++++-----
 submodule.c                  |    4 +-
 t/t3415-rebase-autosquash.sh |   29 ++++++++++++++-
 t/t3900-i18n-commit.sh       |   28 +++++++++++++++
 t/t7500-commit.sh            |   80 ++++++++++++++++++++++++++++++++++++++++++
 t/t7500/edit-content         |    4 ++
 16 files changed, 283 insertions(+), 46 deletions(-)
 create mode 100755 t/t7500/edit-content

-- 
1.7.3.1

[PATCHv5 1/8] commit.c: prefer get_header() to manual searching

[Pat Notz]

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 builtin/commit.c |   11 +++--------
 commit.h         |    1 +
 pretty.c         |    2 +-
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..bf9fcc1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -899,7 +899,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		unsigned char sha1[20];
 		static char utf8[] = "UTF-8";
 		const char *out_enc;
-		char *enc, *end;
+		char *enc;
 		struct commit *commit;
 
 		if (get_sha1(use_message, sha1))
@@ -908,13 +908,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		if (!commit || parse_commit(commit))
 			die("could not parse commit %s", use_message);
 
-		enc = strstr(commit->buffer, "\nencoding");
-		if (enc) {
-			end = strchr(enc + 10, '\n');
-			enc = xstrndup(enc + 10, end - (enc + 10));
-		} else {
-			enc = utf8;
-		}
+		enc = get_header(commit, "encoding");
+		enc = enc ? enc : utf8;
 		out_enc = git_commit_encoding ? git_commit_encoding : utf8;
 
 		if (strcmp(out_enc, enc))
diff --git a/commit.h b/commit.h
index 9113bbe..c246c94 100644
--- a/commit.h
+++ b/commit.h
@@ -87,6 +87,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern char *reencode_commit_message(const struct commit *commit,
 				     const char **encoding_p);
 extern void get_commit_format(const char *arg, struct rev_info *);
+extern char *get_header(const struct commit *commit, const char *key);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
diff --git a/pretty.c b/pretty.c
index f85444b..839944c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -348,7 +348,7 @@ static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
 	strbuf_addch(sb, '\n');
 }
 
-static char *get_header(const struct commit *commit, const char *key)
+char *get_header(const struct commit *commit, const char *key)
 {
 	int key_len = strlen(key);
 	const char *line = commit->buffer;
-- 
1.7.3.1

[PATCHv5 2/8] commit.c: new function for looking up a comit by name

[Pat Notz]

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 builtin/commit.c |    7 +------
 commit.c         |   13 +++++++++++++
 commit.h         |    1 +
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index bf9fcc1..9fe4bdc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -896,17 +896,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (!use_message && renew_authorship)
 		die("--reset-author can be used only with -C, -c or --amend.");
 	if (use_message) {
-		unsigned char sha1[20];
 		static char utf8[] = "UTF-8";
 		const char *out_enc;
 		char *enc;
 		struct commit *commit;
 
-		if (get_sha1(use_message, sha1))
-			die("could not lookup commit %s", use_message);
-		commit = lookup_commit_reference(sha1);
-		if (!commit || parse_commit(commit))
-			die("could not parse commit %s", use_message);
+		commit = lookup_commit_reference_by_name(use_message);
 
 		enc = get_header(commit, "encoding");
 		enc = enc ? enc : utf8;
diff --git a/commit.c b/commit.c
index 0094ec1..f51098a 100644
--- a/commit.c
+++ b/commit.c
@@ -49,6 +49,19 @@ struct commit *lookup_commit(const unsigned char *sha1)
 	return check_commit(obj, sha1, 0);
 }
 
+struct commit *lookup_commit_reference_by_name(const char *name)
+{
+	unsigned char sha1[20];
+	struct commit *commit;
+
+	if (get_sha1(name, sha1))
+		die("could not lookup commit %s", name);
+	commit = lookup_commit_reference(sha1);
+	if (!commit || parse_commit(commit))
+		die("could not parse commit %s", name);
+	return commit;
+}
+
 static unsigned long parse_commit_date(const char *buf, const char *tail)
 {
 	const char *dateptr;
diff --git a/commit.h b/commit.h
index c246c94..6f4b586 100644
--- a/commit.h
+++ b/commit.h
@@ -36,6 +36,7 @@ struct commit *lookup_commit(const unsigned char *sha1);
 struct commit *lookup_commit_reference(const unsigned char *sha1);
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet);
+struct commit *lookup_commit_reference_by_name(const char *name);
 
 int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);
 
-- 
1.7.3.1

[PATCHv5 3/8] pretty.c: helper methods for getting output encodings

[Pat Notz]

Add helpers get_log_output_encoding() and get_commit_output_encoding()
that eliminate some messy and duplicate if-blocks.

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 builtin/commit.c |    2 +-
 cache.h          |    3 +++
 environment.c    |   11 +++++++++++
 pretty.c         |    6 +-----
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9fe4bdc..ea3801d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -905,7 +905,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 		enc = get_header(commit, "encoding");
 		enc = enc ? enc : utf8;
-		out_enc = git_commit_encoding ? git_commit_encoding : utf8;
+		out_enc = get_commit_output_encoding();
 
 		if (strcmp(out_enc, enc))
 			use_message_buffer =
diff --git a/cache.h b/cache.h
index 3d5ed51..7d49805 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,6 +1003,9 @@ extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
+extern const char *get_log_output_encoding(void);
+extern const char *get_commit_output_encoding(void);
+
 extern const char *config_exclusive_filename;
 
 #define MAX_GITNAME (1000)
diff --git a/environment.c b/environment.c
index de5581f..a9d44a2 100644
--- a/environment.c
+++ b/environment.c
@@ -192,3 +192,14 @@ int set_git_dir(const char *path)
 	setup_git_env();
 	return 0;
 }
+
+const char *get_log_output_encoding(void)
+{
+	return git_log_output_encoding ? git_log_output_encoding
+		: get_commit_output_encoding();
+}
+
+const char *get_commit_output_encoding(void)
+{
+	return git_commit_encoding ? git_commit_encoding : "UTF-8";
+}
diff --git a/pretty.c b/pretty.c
index 839944c..a607fd6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1159,11 +1159,7 @@ char *reencode_commit_message(const struct commit *commit, const char **encoding
 {
 	const char *encoding;
 
-	encoding = (git_log_output_encoding
-		    ? git_log_output_encoding
-		    : git_commit_encoding);
-	if (!encoding)
-		encoding = "UTF-8";
+	encoding = get_log_output_encoding();
 	if (encoding_p)
 		*encoding_p = encoding;
 	return logmsg_reencode(commit, encoding);
-- 
1.7.3.1

[PATCHv5 4/8] pretty.c: teach format_commit_message() to reencode the output

[Pat Notz]

format_commit_message() will now reencode the content if the desired
output encoding is different from the encoding in the passed in
commit.

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 archive.c               |    2 +-
 builtin/commit.c        |    6 +++---
 builtin/fmt-merge-msg.c |    2 +-
 commit.h                |    3 ++-
 log-tree.c              |    2 +-
 notes-cache.c           |    2 +-
 pretty.c                |   39 +++++++++++++++++++++++++++++++++++----
 submodule.c             |    4 ++--
 8 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/archive.c b/archive.c
index edd6853..42b7ab1 100644
--- a/archive.c
+++ b/archive.c
@@ -51,7 +51,7 @@ static void format_subst(const struct commit *commit,
 		strbuf_add(&fmt, b + 8, c - b - 8);
 
 		strbuf_add(buf, src, b - src);
-		format_commit_message(commit, fmt.buf, buf, &ctx);
+		format_commit_message(commit, fmt.buf, buf, &ctx, NULL);
 		len -= c + 1 - src;
 		src  = c + 1;
 	}
diff --git a/builtin/commit.c b/builtin/commit.c
index ea3801d..e66f10c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -826,7 +826,7 @@ static const char *find_author_by_nickname(const char *name)
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = DATE_NORMAL;
 		strbuf_release(&buf);
-		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
+		format_commit_message(commit, "%an <%ae>", &buf, &ctx, NULL);
 		return strbuf_detach(&buf, NULL);
 	}
 	die("No existing author found with '%s'", name);
@@ -1135,8 +1135,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	strbuf_addstr(&format, "format:%h] %s");
 
-	format_commit_message(commit, "%an <%ae>", &author_ident, &pctx);
-	format_commit_message(commit, "%cn <%ce>", &committer_ident, &pctx);
+	format_commit_message(commit, "%an <%ae>", &author_ident, &pctx, NULL);
+	format_commit_message(commit, "%cn <%ce>", &committer_ident, &pctx, NULL);
 	if (strbuf_cmp(&author_ident, &committer_ident)) {
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 78c7774..7619f4f 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -173,7 +173,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 		if (subjects.nr > limit)
 			continue;
 
-		format_commit_message(commit, "%s", &sb, &ctx);
+		format_commit_message(commit, "%s", &sb, &ctx, NULL);
 		strbuf_ltrim(&sb);
 
 		if (!sb.len)
diff --git a/commit.h b/commit.h
index 6f4b586..51c7110 100644
--- a/commit.h
+++ b/commit.h
@@ -92,7 +92,8 @@ extern char *get_header(const struct commit *commit, const char *key);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
-				  const struct pretty_print_context *context);
+				  const struct pretty_print_context *context,
+				  const char *output_encoding);
 extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 				struct strbuf *sb,
 				const struct pretty_print_context *context);
diff --git a/log-tree.c b/log-tree.c
index b46ed3b..af3200d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -274,7 +274,7 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix,
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = DATE_NORMAL;
 
-		format_commit_message(commit, "%f", buf, &ctx);
+		format_commit_message(commit, "%f", buf, &ctx, NULL);
 		if (max_len < buf->len)
 			strbuf_setlen(buf, max_len);
 		strbuf_addstr(buf, suffix);
diff --git a/notes-cache.c b/notes-cache.c
index dee6d62..461c474 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -19,7 +19,7 @@ static int notes_cache_match_validity(const char *ref, const char *validity)
 		return 0;
 
 	memset(&pretty_ctx, 0, sizeof(pretty_ctx));
-	format_commit_message(commit, "%s", &msg, &pretty_ctx);
+	format_commit_message(commit, "%s", &msg, &pretty_ctx, NULL);
 	strbuf_trim(&msg);
 
 	ret = !strcmp(msg.buf, validity);
diff --git a/pretty.c b/pretty.c
index a607fd6..e5ce7fb 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1009,16 +1009,47 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
 
 void format_commit_message(const struct commit *commit,
 			   const char *format, struct strbuf *sb,
-			   const struct pretty_print_context *pretty_ctx)
+			   const struct pretty_print_context *pretty_ctx,
+			   const char *output_encoding)
 {
 	struct format_commit_context context;
+	static char utf8[] = "UTF-8";
+	char *enc;
+	char *buffer;
+	char *enc_buffer;
+	struct strbuf scratch_sb = STRBUF_INIT;
+	struct strbuf *sb_ptr;
+
+	enc = get_header(commit, "encoding");
+	enc = enc ? enc : utf8;
+	if(output_encoding && strcmp(enc,output_encoding)) {
+		sb_ptr = &scratch_sb;
+	} else {
+		sb_ptr = sb;
+	}
 
 	memset(&context, 0, sizeof(context));
 	context.commit = commit;
 	context.pretty_ctx = pretty_ctx;
 	context.wrap_start = sb->len;
-	strbuf_expand(sb, format, format_commit_item, &context);
-	rewrap_message_tail(sb, &context, 0, 0, 0);
+	strbuf_expand(sb_ptr, format, format_commit_item, &context);
+	rewrap_message_tail(sb_ptr, &context, 0, 0, 0);
+
+	if(sb_ptr != sb) {
+		/* if re-encoding fails, take the content byte-for-byte */
+		buffer = strbuf_detach(sb_ptr, 0);
+		enc_buffer = reencode_string(buffer, output_encoding, enc);
+		enc_buffer = enc_buffer ? enc_buffer : buffer;
+
+		strbuf_addstr(sb,enc_buffer);
+
+		if(enc_buffer != buffer)
+			free(enc_buffer);
+		free(buffer);
+	}
+
+	if(enc != utf8)
+		free(enc);
 }
 
 static void pp_header(enum cmit_fmt fmt,
@@ -1177,7 +1208,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	int need_8bit_cte = context->need_8bit_cte;
 
 	if (fmt == CMIT_FMT_USERFORMAT) {
-		format_commit_message(commit, user_format, sb, context);
+		format_commit_message(commit, user_format, sb, context, NULL);
 		return;
 	}
 
diff --git a/submodule.c b/submodule.c
index 91a4758..c108ff6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -217,7 +217,7 @@ void show_submodule_summary(FILE *f, const char *path,
 			}
 			else if (add)
 				strbuf_addstr(&sb, add);
-			format_commit_message(commit, format, &sb, &ctx);
+			format_commit_message(commit, format, &sb, &ctx, NULL);
 			if (reset)
 				strbuf_addstr(&sb, reset);
 			strbuf_addch(&sb, '\n');
@@ -362,7 +362,7 @@ static void print_commit(struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 	ctx.date_mode = DATE_NORMAL;
-	format_commit_message(commit, " %h: %m %s", &sb, &ctx);
+	format_commit_message(commit, " %h: %m %s", &sb, &ctx, NULL);
 	fprintf(stderr, "%s\n", sb.buf);
 	strbuf_release(&sb);
 }
-- 
1.7.3.1

[PATCHv5 5/8] commit: --fixup option for use with rebase --autosquash

[Pat Notz]

This option makes it convenient to construct commit messages for use
with 'rebase --autosquash'.  The resulting commit message will be
"fixup! ..." where "..." is the subject line of the specified commit
message.

Example usage:
  $ git commit --fixup HEAD~2

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 Documentation/git-commit.txt |   14 ++++++++++----
 builtin/commit.c             |   20 ++++++++++++++++----
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 42fb1f5..f4a2b8c 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -9,10 +9,10 @@ SYNOPSIS
 --------
 [verse]
 'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
-	   [(-c | -C) <commit>] [-F <file> | -m <msg>] [--reset-author]
-	   [--allow-empty] [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
-	   [--date=<date>] [--cleanup=<mode>] [--status | --no-status] [--]
-	   [[-i | -o ]<file>...]
+	   [(-c | -C | --fixup) <commit>] [-F <file> | -m <msg>]
+	   [--reset-author] [--allow-empty] [--allow-empty-message] [--no-verify]
+	   [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>]
+	   [--status | --no-status] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -70,6 +70,12 @@ OPTIONS
 	Like '-C', but with '-c' the editor is invoked, so that
 	the user can further edit the commit message.
 
+--fixup=<commit>::
+	Construct a commit message for use with `rebase --autosquash`.
+	The commit message will be the subject line from the specified
+	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
+	for details.
+
 --reset-author::
 	When used with -C/-c/--amend options, declare that the
 	authorship of the resulting commit now belongs of the committer.
diff --git a/builtin/commit.c b/builtin/commit.c
index e66f10c..851bb59 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -69,6 +69,7 @@ static enum {
 static const char *logfile, *force_author;
 static const char *template_file;
 static char *edit_message, *use_message;
+static char *fixup_message;
 static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -124,6 +125,7 @@ static struct option builtin_commit_options[] = {
 	OPT_CALLBACK('m', "message", &message, "MESSAGE", "specify commit message", opt_parse_m),
 	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit"),
 	OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
+	OPT_STRING(0, "fixup", &fixup_message, "COMMIT", "use autosquash formatted message to fixup specified commit"),
 	OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
@@ -586,6 +588,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
 		hook_arg1 = "commit";
 		hook_arg2 = use_message;
+	} else if (fixup_message) {
+		struct pretty_print_context ctx = {0};
+		struct commit *commit;
+		const char *out_enc;
+		commit = lookup_commit_reference_by_name(fixup_message);
+		out_enc = get_commit_output_encoding();
+		format_commit_message(commit, "fixup! %s\n\n", &sb, &ctx, out_enc);
+		hook_arg1 = "message";
 	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
 			die_errno("could not read MERGE_MSG");
@@ -863,7 +873,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die("Using both --reset-author and --author does not make sense");
 
-	if (logfile || message.len || use_message)
+	if (logfile || message.len || use_message || fixup_message)
 		use_editor = 0;
 	if (edit_flag)
 		use_editor = 1;
@@ -883,15 +893,17 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		f++;
 	if (edit_message)
 		f++;
+	if (fixup_message)
+		f++;
 	if (logfile)
 		f++;
 	if (f > 1)
-		die("Only one of -c/-C/-F can be used.");
+		die("Only one of -c/-C/-F/--fixup can be used.");
 	if (message.len && f > 0)
-		die("Option -m cannot be combined with -c/-C/-F.");
+		die("Option -m cannot be combined with -c/-C/-F/--fixup.");
 	if (edit_message)
 		use_message = edit_message;
-	if (amend && !use_message)
+	if (amend && !use_message && !fixup_message)
 		use_message = "HEAD";
 	if (!use_message && renew_authorship)
 		die("--reset-author can be used only with -C, -c or --amend.");
-- 
1.7.3.1

[PATCHv5 6/8] add tests of commit --fixup

[Pat Notz]

t7500: test expected behavior of commit --fixup
t3415: test interaction of commit --fixup with rebase --autosquash
t3900: test commit --fixup with i18n encodings

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 t/t3415-rebase-autosquash.sh |   25 +++++++++++++++++++++++--
 t/t3900-i18n-commit.sh       |   26 ++++++++++++++++++++++++++
 t/t7500-commit.sh            |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index fd2184c..b77a413 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -14,6 +14,7 @@ test_expect_success setup '
 	git add . &&
 	test_tick &&
 	git commit -m "first commit" &&
+	git tag first-commit &&
 	echo 3 >file3 &&
 	git add . &&
 	test_tick &&
@@ -21,7 +22,7 @@ test_expect_success setup '
 	git tag base
 '
 
-test_auto_fixup() {
+test_auto_fixup () {
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -50,7 +51,7 @@ test_expect_success 'auto fixup (config)' '
 	test_must_fail test_auto_fixup final-fixup-config-false
 '
 
-test_auto_squash() {
+test_auto_squash () {
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -94,4 +95,24 @@ test_expect_success 'misspelled auto squash' '
 	test 0 = $(git rev-list final-missquash...HEAD | wc -l)
 '
 
+test_auto_commit_flags () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit --$1 first-commit &&
+	git tag final-commit-$1 &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	test 3 = $(wc -l <actual) &&
+	git diff --exit-code final-commit-$1 &&
+	test 1 = "$(git cat-file blob HEAD^:file1)" &&
+	test $2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+}
+
+test_expect_success 'use commit --fixup' '
+	test_auto_commit_flags fixup 1
+'
+
 test_done
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 256c4c9..ff6d8dd 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -133,4 +133,30 @@ do
 	'
 done
 
+test_commit_autosquash_flags () {
+	H=$1
+	flag=$2
+	test_expect_success "commit --$flag with $H encoding" '
+		git config i18n.commitencoding $H &&
+		git checkout -b $H-$flag C0 &&
+		echo $H >>F &&
+		git commit -a -F "$TEST_DIRECTORY"/t3900/$H.txt &&
+		test_tick &&
+		echo intermediate stuff >>G &&
+		git add G &&
+		git commit -a -m "intermediate commit" &&
+		test_tick &&
+		echo $H $flag >>F &&
+		git commit -a --$flag HEAD~1 $3 &&
+		E=$(git cat-file commit '$H-$flag' | sed -ne "s/^encoding //p") &&
+		test "z$E" = "z'$H'" &&
+		git config --unset-all i18n.commitencoding &&
+		git rebase --autosquash -i HEAD^^^ &&
+		git log --oneline >actual &&
+		test 3 = $(wc -l <actual)
+	'
+}
+
+test_commit_autosquash_flags eucJP fixup
+
 test_done
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index aa9c577..a41b819 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -215,4 +215,37 @@ test_expect_success 'Commit a message with --allow-empty-message' '
 	commit_msg_is "hello there"
 '
 
+commit_for_rebase_autosquash_setup () {
+	echo "first content line" >>foo &&
+	git add foo &&
+	cat >log <<EOF &&
+target message subject line
+
+target message body line 1
+target message body line 2
+EOF
+	git commit -F log &&
+	echo "second content line" >>foo &&
+	git add foo &&
+	git commit -m "intermediate commit" &&
+	echo "third content line" >>foo &&
+	git add foo
+}
+
+test_expect_success 'commit --fixup provides correct one-line commit message' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --fixup HEAD~1 &&
+	commit_msg_is "fixup! target message subject line"
+'
+
+test_expect_success 'invalid message options when using --fixup' '
+	echo changes >>foo &&
+	echo "message" >log &&
+	git add foo &&
+	test_must_fail git commit --fixup HEAD~1 -C HEAD~2 &&
+	test_must_fail git commit --fixup HEAD~1 -c HEAD~2 &&
+	test_must_fail git commit --fixup HEAD~1 -m "cmdline message" &&
+	test_must_fail git commit --fixup HEAD~1 -F log
+'
+
 test_done
-- 
1.7.3.1

[PATCHv5 7/8] commit: --squash option for use with rebase --autosquash

[Pat Notz]

This option makes it convenient to construct commit messages for use
with 'rebase --autosquash'.  The resulting commit message will be
"squash! ..." where "..." is the subject line of the specified commit
message.  This option can be used with other commit message options
such as -m, -c, -C and -F.

If an editor is invoked (as with -c or -eF or no message options) the
commit message is seeded with the correctly formatted subject line.

Example usage:
  $ git commit --squash HEAD~2
  $ git commit --squash HEAD~2 -m "clever comment"
  $ git commit --squash HEAD~2 -F msgfile
  $ git commit --squash HEAD~2 -C deadbeef

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 Documentation/git-commit.txt |    9 ++++++++-
 builtin/commit.c             |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f4a2b8c..6e4c220 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
-	   [(-c | -C | --fixup) <commit>] [-F <file> | -m <msg>]
+	   [(-c | -C | --fixup | --squash) <commit>] [-F <file> | -m <msg>]
 	   [--reset-author] [--allow-empty] [--allow-empty-message] [--no-verify]
 	   [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>]
 	   [--status | --no-status] [--] [[-i | -o ]<file>...]
@@ -76,6 +76,13 @@ OPTIONS
 	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
 	for details.
 
+--squash=<commit>::
+	Construct a commit message for use with `rebase --autosquash`.
+	The commit message subject line is taken from the specified
+	commit with a prefix of "squash! ".  Can be used with additional
+	commit message options (`-m`/`-c`/`-C`/`-F`). See
+	linkgit:git-rebase[1] for details.
+
 --reset-author::
 	When used with -C/-c/--amend options, declare that the
 	authorship of the resulting commit now belongs of the committer.
diff --git a/builtin/commit.c b/builtin/commit.c
index 851bb59..6dfad73 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -69,7 +69,7 @@ static enum {
 static const char *logfile, *force_author;
 static const char *template_file;
 static char *edit_message, *use_message;
-static char *fixup_message;
+static char *fixup_message, *squash_message;
 static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -126,6 +126,7 @@ static struct option builtin_commit_options[] = {
 	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit"),
 	OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
 	OPT_STRING(0, "fixup", &fixup_message, "COMMIT", "use autosquash formatted message to fixup specified commit"),
+	OPT_STRING(0, "squash", &squash_message, "COMMIT", "use autosquash formatted message to squash specified commit"),
 	OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
@@ -567,6 +568,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
 
+	if (squash_message) {
+		/*
+		 * Insert the proper subject line before other commit
+		 * message options add their content.
+		 */
+		struct pretty_print_context ctx = {0};
+		struct commit *commit;
+		const char *out_enc;
+		commit = lookup_commit_reference_by_name(squash_message);
+		out_enc = get_commit_output_encoding();
+		if(use_message && !strcmp(use_message, squash_message))
+			strbuf_addstr(&sb,"squash! ");
+		else
+			format_commit_message(commit, "squash! %s\n\n", &sb, &ctx, out_enc);
+	}
+
 	if (message.len) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
@@ -617,6 +634,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	else if (in_merge)
 		hook_arg1 = "merge";
 
+	if (squash_message) {
+		/*
+		 * If squash_commit was used for the commit subject,
+		 * then we're possibly hijacking other commit log options.
+		 * Reset the hook args to tell the real story.
+		 */
+		hook_arg1 = "message";
+		hook_arg2 = "";
+	}
+
 	fp = fopen(git_path(commit_editmsg), "w");
 	if (fp == NULL)
 		die_errno("could not open '%s'", git_path(commit_editmsg));
@@ -888,7 +915,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die("You have nothing to amend.");
 	if (amend && in_merge)
 		die("You are in the middle of a merge -- cannot amend.");
-
+	if (fixup_message && squash_message)
+		die("Options --squash and --fixup cannot be used together");
 	if (use_message)
 		f++;
 	if (edit_message)
-- 
1.7.3.1

[PATCHv5 8/8] add tests of commit --squash

[Pat Notz]

t7500: test expected behavior of commit --squash
t3415: test interaction of commit --squash with rebase --autosquash
t3900: test commit --squash with i18n encodings

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
As noted in the cover letter, one issue which limits the testing is
that rebase --autosquash does not (yet) consider encodings when
comparing commit subjects.  So, we can't test the case of a fixup! or
squash! commit having a different encoding than the commit with which
it will be combined.

 t/t3415-rebase-autosquash.sh |    4 +++
 t/t3900-i18n-commit.sh       |    2 +
 t/t7500-commit.sh            |   47 ++++++++++++++++++++++++++++++++++++++++++
 t/t7500/edit-content         |    4 +++
 4 files changed, 57 insertions(+), 0 deletions(-)
 create mode 100755 t/t7500/edit-content

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index b77a413..0028533 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -115,4 +115,8 @@ test_expect_success 'use commit --fixup' '
 	test_auto_commit_flags fixup 1
 '
 
+test_expect_success 'use commit --squash' '
+	test_auto_commit_flags squash 2
+'
+
 test_done
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index ff6d8dd..dfabb3a 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -159,4 +159,6 @@ test_commit_autosquash_flags () {
 
 test_commit_autosquash_flags eucJP fixup
 
+test_commit_autosquash_flags ISO-2022-JP squash '-m "squash message"'
+
 test_done
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index a41b819..162527c 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -238,10 +238,57 @@ test_expect_success 'commit --fixup provides correct one-line commit message' '
 	commit_msg_is "fixup! target message subject line"
 '
 
+test_expect_success 'commit --squash works with -F' '
+	commit_for_rebase_autosquash_setup &&
+	echo "log message from file" >msgfile &&
+	git commit --squash HEAD~1 -F msgfile  &&
+	commit_msg_is "squash! target message subject linelog message from file"
+'
+
+test_expect_success 'commit --squash works with -m' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 -m "foo bar\nbaz" &&
+	commit_msg_is "squash! target message subject linefoo bar\nbaz"
+'
+
+test_expect_success 'commit --squash works with -C' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 -C HEAD &&
+	commit_msg_is "squash! target message subject lineintermediate commit"
+'
+
+test_expect_success 'commit --squash works with -c' '
+	commit_for_rebase_autosquash_setup &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/edit-content &&
+	git commit --squash HEAD~1 -c HEAD &&
+	commit_msg_is "squash! target message subject lineedited commit"
+'
+
+test_expect_success 'commit --squash works with -C for same commit' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD -C HEAD &&
+	commit_msg_is "squash! intermediate commit"
+'
+
+test_expect_success 'commit --squash works with -c for same commit' '
+	commit_for_rebase_autosquash_setup &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/edit-content &&
+	git commit --squash HEAD -c HEAD &&
+	commit_msg_is "squash! edited commit"
+'
+
+test_expect_success 'commit --squash works with editor' '
+	commit_for_rebase_autosquash_setup &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+	git commit --squash HEAD~1 &&
+	commit_msg_is "squash! target message subject linecommit message"
+'
+
 test_expect_success 'invalid message options when using --fixup' '
 	echo changes >>foo &&
 	echo "message" >log &&
 	git add foo &&
+	test_must_fail git commit --fixup HEAD~1 --squash HEAD~2 &&
 	test_must_fail git commit --fixup HEAD~1 -C HEAD~2 &&
 	test_must_fail git commit --fixup HEAD~1 -c HEAD~2 &&
 	test_must_fail git commit --fixup HEAD~1 -m "cmdline message" &&
diff --git a/t/t7500/edit-content b/t/t7500/edit-content
new file mode 100755
index 0000000..08db9fd
--- /dev/null
+++ b/t/t7500/edit-content
@@ -0,0 +1,4 @@
+#!/bin/sh
+sed -e "s/intermediate/edited/g" <"$1" >"$1-"
+mv "$1-" "$1"
+exit 0
-- 
1.7.3.1

Re: [PATCHv5 1/8] commit.c: prefer get_header() to manual searching

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 21:10, Pat Notz <patnotz@gmail.com> wrote:
> Signed-off-by: Pat Notz <patnotz@gmail.com>

Cover letter?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCHv5 1/8] commit.c: prefer get_header() to manual searching

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 23:11, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Cover letter?

Ah, there it is, sorry for the noise.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCHv5 0/8] Add commit message options for rebase --autosquash

[Pat Notz]

Hiya -

I didn't receive any feedback from this patch series so I thought I'd
just ping the list to see if this is of interest at all (I fully
accept that it might not be). Or perhaps no news is good news. Anyway,
sorry for the spam.

~ pat

On Thu, Oct 7, 2010 at 1:10 PM, Pat Notz <patnotz@gmail.com> wrote:
> This patch series adds new command line options to git-commit to make
> it easy to specify messages for commits correctly formatted for use
> wit 'rebase -i --autosquash'.
>
> The first three patches introduce minor refactorings that set the stage
> for subsequent patches.
>
> The fourth patch teaches format_commit_message to reencode the content
> if the caller's commit object uses an encoding different from the
> commit encoding.
>
> The remaining patches add the --fixup and --squash commands to
> git-commit including tests of commit, interactions with rebase and
> i18n encodings.
>
> One issue which limits the testing (but not the implementation, I
> think) is that when 'rebase --autosquash' is comparing commit subject
> lines it does not first make sure that the commits use a common
> encoding.  That's follow-on work.
>
> Pat Notz (8):
>  commit.c: prefer get_header() to manual searching
>  commit.c: new function for looking up a comit by name
>  pretty.c: helper methods for getting output encodings
>  pretty.c: teach format_commit_message() to reencode the output
>  commit: --fixup option for use with rebase --autosquash
>  add tests of commit --fixup
>  commit: --squash option for use with rebase --autosquash
>  add tests of commit --squash
>
>  Documentation/git-commit.txt |   21 +++++++++--
>  archive.c                    |    2 +-
>  builtin/commit.c             |   76 +++++++++++++++++++++++++++------------
>  builtin/fmt-merge-msg.c      |    2 +-
>  cache.h                      |    3 ++
>  commit.c                     |   13 +++++++
>  commit.h                     |    5 ++-
>  environment.c                |   11 ++++++
>  log-tree.c                   |    2 +-
>  notes-cache.c                |    2 +-
>  pretty.c                     |   47 +++++++++++++++++++-----
>  submodule.c                  |    4 +-
>  t/t3415-rebase-autosquash.sh |   29 ++++++++++++++-
>  t/t3900-i18n-commit.sh       |   28 +++++++++++++++
>  t/t7500-commit.sh            |   80 ++++++++++++++++++++++++++++++++++++++++++
>  t/t7500/edit-content         |    4 ++
>  16 files changed, 283 insertions(+), 46 deletions(-)
>  create mode 100755 t/t7500/edit-content
>
> --
> 1.7.3.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv5 0/8] Add commit message options for rebase --autosquash

[Sverre Rabbelier]

Heya,

On Mon, Oct 11, 2010 at 23:06, Pat Notz <patnotz@gmail.com> wrote:
> I didn't receive any feedback from this patch series so I thought I'd
> just ping the list to see if this is of interest at all (I fully
> accept that it might not be). Or perhaps no news is good news. Anyway,
> sorry for the spam.

I'm definitely interested as a user, but I don't think I'm qualified
to review the actual code.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCHv5 1/8] commit.c: prefer get_header() to manual searching

[Junio C Hamano]

"Pat Notz" <patnotz@gmail.com> writes:

> diff --git a/commit.h b/commit.h
> index 9113bbe..c246c94 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -87,6 +87,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
>  extern char *reencode_commit_message(const struct commit *commit,
>  				     const char **encoding_p);
>  extern void get_commit_format(const char *arg, struct rev_info *);
> +extern char *get_header(const struct commit *commit, const char *key);

The name of the function was perfectly Ok within the context of pretty.c,
as the file was about commit log message and nothing else, but it is a bit
mindless to expose it to outside world still with such a generic name.

But the bigger question is why the patch doesn't get rid of the bulk of
the body of the block and replace it with a call to logmsg_reencode().


diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..021fb1c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -899,7 +899,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		unsigned char sha1[20];
 		static char utf8[] = "UTF-8";
 		const char *out_enc;
-		char *enc, *end;
 		struct commit *commit;
 
 		if (get_sha1(use_message, sha1))
@@ -908,18 +907,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		if (!commit || parse_commit(commit))
 			die("could not parse commit %s", use_message);
 
-		enc = strstr(commit->buffer, "\nencoding");
-		if (enc) {
-			end = strchr(enc + 10, '\n');
-			enc = xstrndup(enc + 10, end - (enc + 10));
-		} else {
-			enc = utf8;
-		}
 		out_enc = git_commit_encoding ? git_commit_encoding : utf8;
-
-		if (strcmp(out_enc, enc))
-			use_message_buffer =
-				reencode_string(commit->buffer, out_enc, enc);
+		use_message_buffer = logmsg_reencode(commit, out_enc);
 
 		/*
 		 * If we failed to reencode the buffer, just copy it
@@ -929,8 +918,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		 */
 		if (use_message_buffer == NULL)
 			use_message_buffer = xstrdup(commit->buffer);
-		if (enc != utf8)
-			free(enc);
 	}
 
 	if (!!also + !!only + !!all + !!interactive > 1)

Re: [PATCHv5 2/8] commit.c: new function for looking up a comit by name

[Junio C Hamano]

s/comit/commit/;

"Pat Notz" <patnotz@gmail.com> writes:

> diff --git a/commit.c b/commit.c
> index 0094ec1..f51098a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -49,6 +49,19 @@ struct commit *lookup_commit(const unsigned char *sha1)
>  	return check_commit(obj, sha1, 0);
>  }
>  
> +struct commit *lookup_commit_reference_by_name(const char *name)
> +{
> +	unsigned char sha1[20];
> +	struct commit *commit;
> +
> +	if (get_sha1(name, sha1))
> +		die("could not lookup commit %s", name);
> +	commit = lookup_commit_reference(sha1);
> +	if (!commit || parse_commit(commit))
> +		die("could not parse commit %s", name);
> +	return commit;
> +}

Although this wouldn't _hurt_, not very excited without seeing codepaths
that can use this helper to reduce lines (there should be a lot of places,
I would think).  And when that happens, the helper that dies would not be
very useful, as some potential callers that can benefit may want to decide
what messages to issue themselves.

Re: [PATCHv5 4/8] pretty.c: teach format_commit_message() to reencode the output

[Junio C Hamano]

"Pat Notz" <patnotz@gmail.com> writes:

> diff --git a/pretty.c b/pretty.c
> index a607fd6..e5ce7fb 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1009,16 +1009,47 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
>  
>  void format_commit_message(const struct commit *commit,
>  			   const char *format, struct strbuf *sb,
> -			   const struct pretty_print_context *pretty_ctx)
> +			   const struct pretty_print_context *pretty_ctx,
> +			   const char *output_encoding)
>  {
>  	struct format_commit_context context;
> +	static char utf8[] = "UTF-8";
> +	char *enc;
> +	char *buffer;
> +	char *enc_buffer;
> +	struct strbuf scratch_sb = STRBUF_INIT;
> +	struct strbuf *sb_ptr;
> +
> +	enc = get_header(commit, "encoding");
> +	enc = enc ? enc : utf8;
> +	if(output_encoding && strcmp(enc,output_encoding)) {
> +		sb_ptr = &scratch_sb;
> +	} else {
> +		sb_ptr = sb;
> +	}

Style...

>  	memset(&context, 0, sizeof(context));
>  	context.commit = commit;
>  	context.pretty_ctx = pretty_ctx;
>  	context.wrap_start = sb->len;
> -	strbuf_expand(sb, format, format_commit_item, &context);
> -	rewrap_message_tail(sb, &context, 0, 0, 0);
> +	strbuf_expand(sb_ptr, format, format_commit_item, &context);
> +	rewrap_message_tail(sb_ptr, &context, 0, 0, 0);
> +
> +	if(sb_ptr != sb) {
> +		/* if re-encoding fails, take the content byte-for-byte */
> +		buffer = strbuf_detach(sb_ptr, 0);
> +		enc_buffer = reencode_string(buffer, output_encoding, enc);
> +		enc_buffer = enc_buffer ? enc_buffer : buffer;
> +
> +		strbuf_addstr(sb,enc_buffer);
> +
> +		if(enc_buffer != buffer)
> +			free(enc_buffer);
> +		free(buffer);
> +	}
> +
> +	if(enc != utf8)
> +		free(enc);
>  }

You are expanding and wrapping commit->buf before re-encoding, but I am
not sure it is the right thing to do.  Wouldn't it become much simpler and
more consistent if you re-encode first and then give the result to later
expansion and wrapping process?  format_commit_one() would need to take
"msg" not from c->commit->buffer but from a new field to hold reencoded
result you will add in your patch to the structure, if you do so, of
course.

Besides, I am a bit lost as to what this patch has to do with the stated
goal of the series, "Add commit message options for rebase --autosquash".

Re: [PATCHv5 4/8] pretty.c: teach format_commit_message() to reencode the output

[Pat Notz]

On Wed, Oct 13, 2010 at 3:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Pat Notz" <patnotz@gmail.com> writes:
>
>> diff --git a/pretty.c b/pretty.c
>> index a607fd6..e5ce7fb 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1009,16 +1009,47 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
>>
>>  void format_commit_message(const struct commit *commit,
>>                          const char *format, struct strbuf *sb,
>> -                        const struct pretty_print_context *pretty_ctx)
>> +                        const struct pretty_print_context *pretty_ctx,
>> +                        const char *output_encoding)
>>  {
>>       struct format_commit_context context;
>> +     static char utf8[] = "UTF-8";
>> +     char *enc;
>> +     char *buffer;
>> +     char *enc_buffer;
>> +     struct strbuf scratch_sb = STRBUF_INIT;
>> +     struct strbuf *sb_ptr;
>> +
>> +     enc = get_header(commit, "encoding");
>> +     enc = enc ? enc : utf8;
>> +     if(output_encoding && strcmp(enc,output_encoding)) {
>> +             sb_ptr = &scratch_sb;
>> +     } else {
>> +             sb_ptr = sb;
>> +     }
>
> Style...
>
>>       memset(&context, 0, sizeof(context));
>>       context.commit = commit;
>>       context.pretty_ctx = pretty_ctx;
>>       context.wrap_start = sb->len;
>> -     strbuf_expand(sb, format, format_commit_item, &context);
>> -     rewrap_message_tail(sb, &context, 0, 0, 0);
>> +     strbuf_expand(sb_ptr, format, format_commit_item, &context);
>> +     rewrap_message_tail(sb_ptr, &context, 0, 0, 0);
>> +
>> +     if(sb_ptr != sb) {
>> +             /* if re-encoding fails, take the content byte-for-byte */
>> +             buffer = strbuf_detach(sb_ptr, 0);
>> +             enc_buffer = reencode_string(buffer, output_encoding, enc);
>> +             enc_buffer = enc_buffer ? enc_buffer : buffer;
>> +
>> +             strbuf_addstr(sb,enc_buffer);
>> +
>> +             if(enc_buffer != buffer)
>> +                     free(enc_buffer);
>> +             free(buffer);
>> +     }
>> +
>> +     if(enc != utf8)
>> +             free(enc);
>>  }
>
> You are expanding and wrapping commit->buf before re-encoding, but I am
> not sure it is the right thing to do.  Wouldn't it become much simpler and
> more consistent if you re-encode first and then give the result to later
> expansion and wrapping process?  format_commit_one() would need to take
> "msg" not from c->commit->buffer but from a new field to hold reencoded
> result you will add in your patch to the structure, if you do so, of
> course.
>
> Besides, I am a bit lost as to what this patch has to do with the stated
> goal of the series, "Add commit message options for rebase --autosquash".

Thank you for the reviews; I'll digest your comments shortly. In
response to your final comment, this patch was in response to your
earlier suggestion[1] to teach format_commit_message() to deal with
potentially different encodings.  I went for your option #3 there but
perhaps it's overkill.  I still think that's the right thing to do
despite my implementation.  Otherwise, this routine is just adding
bits to the buffer without regard for consistent encoding.  My goal
was to introduce this separately from any new features.

Again, thanks for all your comments -- I'll try to improve the series.

[1] http://thread.gmane.org/gmane.comp.version-control.git/156883/focus=156891