threads / patch / 47408

patch, 2 partsformat: create docs for pretty.h

Subject: [PATCH Outreachy 2/2] format: create docs for pretty.h

## tl;dr

14 messages between Dec 8, 2017 and Dec 12, 2017. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Olga Telezhnaya· Dec 8, 2017, 13:21 UTC · lore

[PATCH Outreachy 1/2] format: create pretty.h file

Create header for pretty.c to make formatting interface more structured. This is a middle point, this file would be merged futher with other files which contain formatting stuff.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 archive.c             |  1 +
 builtin/notes.c       |  2 +-
 builtin/reset.c       |  2 +-
 builtin/show-branch.c |  2 +-
 combine-diff.c        |  1 +
 commit.c              |  1 +
 commit.h              | 80 ----------------------------------------------
 diffcore-pickaxe.c    |  1 +
 grep.c                |  1 +
 log-tree.c            |  1 +
 notes-cache.c         |  1 +
 pretty.h              | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
 revision.h            |  2 +-
 sequencer.c           |  1 +
 sha1_name.c           |  1 +
 submodule.c           |  1 +
 16 files changed, 101 insertions(+), 84 deletions(-)
 create mode 100644 pretty.h
Show changes to 16 files +101 −85

archive.c, builtin/notes.c, builtin/reset.c, builtin/show-branch.c, combine-diff.c, commit.c, commit.h, diffcore-pickaxe.c, grep.c, log-tree.c, notes-cache.c, pretty.h, revision.h, sequencer.c, sha1_name.c, submodule.c

diff --git a/archive.c b/archive.c
index 0b7b62af0c3ec..60607e8c00857 100644
--- a/archive.c
+++ b/archive.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "refs.h"
 #include "commit.h"
+#include "pretty.h"
 #include "tree-walk.h"
 #include "attr.h"
 #include "archive.h"
diff --git a/builtin/notes.c b/builtin/notes.c
index 1a2c7d92ad7e7..7c8176164561b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -12,7 +12,7 @@
 #include "builtin.h"
 #include "notes.h"
 #include "blob.h"
-#include "commit.h"
+#include "pretty.h"
 #include "refs.h"
 #include "exec_cmd.h"
 #include "run-command.h"
diff --git a/builtin/reset.c b/builtin/reset.c
index 906e541658230..e15f595799c40 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -12,7 +12,7 @@
 #include "lockfile.h"
 #include "tag.h"
 #include "object.h"
-#include "commit.h"
+#include "pretty.h"
 #include "run-command.h"
 #include "refs.h"
 #include "diff.h"
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 2e24b5c330e8e..e8a4aa40cb4b6 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 #include "config.h"
-#include "commit.h"
+#include "pretty.h"
 #include "refs.h"
 #include "builtin.h"
 #include "color.h"
diff --git a/combine-diff.c b/combine-diff.c
index 2505de119a2be..01ba1b03a06d2 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "commit.h"
+#include "pretty.h"
 #include "blob.h"
 #include "diff.h"
 #include "diffcore.h"
diff --git a/commit.c b/commit.c
index cab8d4455bdbd..ac17a27a4ab0a 100644
--- a/commit.c
+++ b/commit.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "tag.h"
 #include "commit.h"
+#include "pretty.h"
 #include "pkt-line.h"
 #include "utf8.h"
 #include "diff.h"
diff --git a/commit.h b/commit.h
index 99a3fea68d3f6..41a2067809444 100644
--- a/commit.h
+++ b/commit.h
@@ -121,93 +121,13 @@ struct commit_list *copy_commit_list(struct commit_list *list);
 
 void free_commit_list(struct commit_list *list);
 
-/* Commit formats */
-enum cmit_fmt {
-	CMIT_FMT_RAW,
-	CMIT_FMT_MEDIUM,
-	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
-	CMIT_FMT_SHORT,
-	CMIT_FMT_FULL,
-	CMIT_FMT_FULLER,
-	CMIT_FMT_ONELINE,
-	CMIT_FMT_EMAIL,
-	CMIT_FMT_MBOXRD,
-	CMIT_FMT_USERFORMAT,
-
-	CMIT_FMT_UNSPECIFIED
-};
-
-static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
-{
-	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
-}
-
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 
-struct pretty_print_context {
-	/*
-	 * Callers should tweak these to change the behavior of pp_* functions.
-	 */
-	enum cmit_fmt fmt;
-	int abbrev;
-	const char *after_subject;
-	int preserve_subject;
-	struct date_mode date_mode;
-	unsigned date_mode_explicit:1;
-	int print_email_subject;
-	int expand_tabs_in_log;
-	int need_8bit_cte;
-	char *notes_message;
-	struct reflog_walk_info *reflog_info;
-	struct rev_info *rev;
-	const char *output_encoding;
-	struct string_list *mailmap;
-	int color;
-	struct ident_split *from_ident;
-
-	/*
-	 * Fields below here are manipulated internally by pp_* functions and
-	 * should not be counted on by callers.
-	 */
-	struct string_list in_body_headers;
-	int graph_width;
-};
-
-struct userformat_want {
-	unsigned notes:1;
-};
-
 extern int has_non_ascii(const char *text);
 extern const char *logmsg_reencode(const struct commit *commit,
 				   char **commit_encoding,
 				   const char *output_encoding);
-extern void get_commit_format(const char *arg, struct rev_info *);
-extern const char *format_subject(struct strbuf *sb, const char *msg,
-				  const char *line_separator);
-extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
-extern int commit_format_is_empty(enum cmit_fmt);
 extern const char *skip_blank_lines(const char *msg);
-extern void format_commit_message(const struct commit *commit,
-				  const char *format, struct strbuf *sb,
-				  const struct pretty_print_context *context);
-extern void pretty_print_commit(struct pretty_print_context *pp,
-				const struct commit *commit,
-				struct strbuf *sb);
-extern void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
-			   struct strbuf *sb);
-void pp_user_info(struct pretty_print_context *pp,
-		  const char *what, struct strbuf *sb,
-		  const char *line, const char *encoding);
-void pp_title_line(struct pretty_print_context *pp,
-		   const char **msg_p,
-		   struct strbuf *sb,
-		   const char *encoding,
-		   int need_8bit_cte);
-void pp_remainder(struct pretty_print_context *pp,
-		  const char **msg_p,
-		  struct strbuf *sb,
-		  int indent);
-
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9476bd21081f4..c5e12fc096058 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,6 +8,7 @@
 #include "xdiff-interface.h"
 #include "kwset.h"
 #include "commit.h"
+#include "pretty.h"
 #include "quote.h"
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
diff --git a/grep.c b/grep.c
index a69c05edc2e4f..1b99ed01898b0 100644
--- a/grep.c
+++ b/grep.c
@@ -6,6 +6,7 @@
 #include "diff.h"
 #include "diffcore.h"
 #include "commit.h"
+#include "pretty.h"
 #include "quote.h"
 
 static int grep_source_load(struct grep_source *gs);
diff --git a/log-tree.c b/log-tree.c
index 3b904f0375e23..c305eeff80aa0 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "diff.h"
 #include "commit.h"
+#include "pretty.h"
 #include "tag.h"
 #include "graph.h"
 #include "log-tree.h"
diff --git a/notes-cache.c b/notes-cache.c
index 17ee8602b3d2f..d6346a095c18b 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "notes-cache.h"
 #include "commit.h"
+#include "pretty.h"
 #include "refs.h"
 
 static int notes_cache_match_validity(const char *ref, const char *validity)
diff --git a/pretty.h b/pretty.h
new file mode 100644
index 0000000000000..ef5167484fb64
--- /dev/null
+++ b/pretty.h
@@ -0,0 +1,87 @@
+#ifndef PRETTY_H
+#define PRETTY_H
+
+struct commit;
+
+/* Commit formats */
+enum cmit_fmt {
+	CMIT_FMT_RAW,
+	CMIT_FMT_MEDIUM,
+	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
+	CMIT_FMT_SHORT,
+	CMIT_FMT_FULL,
+	CMIT_FMT_FULLER,
+	CMIT_FMT_ONELINE,
+	CMIT_FMT_EMAIL,
+	CMIT_FMT_MBOXRD,
+	CMIT_FMT_USERFORMAT,
+
+	CMIT_FMT_UNSPECIFIED
+};
+
+struct pretty_print_context {
+	/*
+	 * Callers should tweak these to change the behavior of pp_* functions.
+	 */
+	enum cmit_fmt fmt;
+	int abbrev;
+	const char *after_subject;
+	int preserve_subject;
+	struct date_mode date_mode;
+	unsigned date_mode_explicit:1;
+	int print_email_subject;
+	int expand_tabs_in_log;
+	int need_8bit_cte;
+	char *notes_message;
+	struct reflog_walk_info *reflog_info;
+	struct rev_info *rev;
+	const char *output_encoding;
+	struct string_list *mailmap;
+	int color;
+	struct ident_split *from_ident;
+
+	/*
+	 * Fields below here are manipulated internally by pp_* functions and
+	 * should not be counted on by callers.
+	 */
+	struct string_list in_body_headers;
+	int graph_width;
+};
+
+static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
+{
+	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
+}
+
+struct userformat_want {
+	unsigned notes:1;
+};
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
+			struct strbuf *sb);
+void pp_user_info(struct pretty_print_context *pp, const char *what,
+			struct strbuf *sb, const char *line,
+			const char *encoding);
+void pp_title_line(struct pretty_print_context *pp, const char **msg_p,
+			struct strbuf *sb, const char *encoding,
+			int need_8bit_cte);
+void pp_remainder(struct pretty_print_context *pp, const char **msg_p,
+			struct strbuf *sb, int indent);
+
+void format_commit_message(const struct commit *commit,
+			const char *format, struct strbuf *sb,
+			const struct pretty_print_context *context);
+
+void get_commit_format(const char *arg, struct rev_info *);
+
+void pretty_print_commit(struct pretty_print_context *pp,
+			const struct commit *commit,
+			struct strbuf *sb);
+
+const char *format_subject(struct strbuf *sb, const char *msg,
+			const char *line_separator);
+
+int commit_format_is_empty(enum cmit_fmt);
+
+#endif /* PRETTY_H */
diff --git a/revision.h b/revision.h
index 54761200adf2d..747bce8d8a184 100644
--- a/revision.h
+++ b/revision.h
@@ -4,7 +4,7 @@
 #include "parse-options.h"
 #include "grep.h"
 #include "notes.h"
-#include "commit.h"
+#include "pretty.h"
 #include "diff.h"
 
 /* Remember to update object flag allocation in object.h */
diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2ce..654d577ca3cc7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5,6 +5,7 @@
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "pretty.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24ddee6..d35917612c8e7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "tag.h"
 #include "commit.h"
+#include "pretty.h"
 #include "tree.h"
 #include "blob.h"
 #include "tree-walk.h"
diff --git a/submodule.c b/submodule.c
index 95e6aff2bb74e..c05db7a45d9fe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -6,6 +6,7 @@
 #include "dir.h"
 #include "diff.h"
 #include "commit.h"
+#include "pretty.h"
 #include "revision.h"
 #include "run-command.h"
 #include "diffcore.h"

--
https://github.com/git/git/pull/439
Olga Telezhnaya· Dec 8, 2017, 13:21 UTC · re: Olga Telezhnaya · lore

Write some docs for functions in pretty.h. Take it as a first draft, they would be changed later.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 pretty.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
Show changes to pretty.h +44 −1
diff --git a/pretty.h b/pretty.h
index ef5167484fb64..5c85d94e332d7 100644
--- a/pretty.h
+++ b/pretty.h
@@ -48,6 +48,7 @@ struct pretty_print_context {
 	int graph_width;
 };
 
+/* Check whether commit format is mail. */
 static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
 {
 	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
@@ -57,31 +58,74 @@ struct userformat_want {
 	unsigned notes:1;
 };
 
+/* Set the flag "w->notes" if there is placeholder %N in "fmt". */
 void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+
+/*
+ * Shortcut for invoking pretty_print_commit if we do not have any context.
+ * Context would be set empty except "fmt".
+ */
 void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
 			struct strbuf *sb);
+
+/*
+ * Get information about user and date from "line", format it and
+ * put it into "sb".
+ * Format of "line" must be readable for split_ident_line function.
+ * The resulting format is "what: name <email> date".
+ */
 void pp_user_info(struct pretty_print_context *pp, const char *what,
 			struct strbuf *sb, const char *line,
 			const char *encoding);
+
+/*
+ * Format title line of commit message taken from "msg_p" and
+ * put it into "sb".
+ * First line of "msg_p" is also affected.
+ */
 void pp_title_line(struct pretty_print_context *pp, const char **msg_p,
 			struct strbuf *sb, const char *encoding,
 			int need_8bit_cte);
+
+/*
+ * Get current state of commit message from "msg_p" and continue formatting
+ * by adding indentation and '>' signs. Put result into "sb".
+ */
 void pp_remainder(struct pretty_print_context *pp, const char **msg_p,
 			struct strbuf *sb, int indent);
 
+/*
+ * Create a text message about commit using given "format" and "context".
+ * Put the result to "sb".
+ * Please use this function for custom formats.
+ */
 void format_commit_message(const struct commit *commit,
 			const char *format, struct strbuf *sb,
 			const struct pretty_print_context *context);
 
+/*
+ * Parse given arguments from "arg", check it for correctness and
+ * fill struct rev_info.
+ */
 void get_commit_format(const char *arg, struct rev_info *);
 
+/*
+ * Make a commit message with all rules from given "pp"
+ * and put it into "sb".
+ * Please use this function if you have a context (candidate for "pp").
+ */
 void pretty_print_commit(struct pretty_print_context *pp,
 			const struct commit *commit,
 			struct strbuf *sb);
 
+/*
+ * Change line breaks in "msg" to "line_separator" and put it into "sb".
+ * Return "msg" itself.
+ */
 const char *format_subject(struct strbuf *sb, const char *msg,
 			const char *line_separator);
 
+/* Check if "cmit_fmt" will produce an empty output. */
 int commit_format_is_empty(enum cmit_fmt);
 
 #endif /* PRETTY_H */

--
https://github.com/git/git/pull/439
Eric Sunshine· Dec 8, 2017, 19:26 UTC · re: Olga Telezhnaya · lore

Re: [PATCH Outreachy 2/2] format: create docs for pretty.h

On Fri, Dec 8, 2017 at 8:21 AM, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:

Show 21 quoted lines
> Write some docs for functions in pretty.h.
> Take it as a first draft, they would be changed later.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
> ---
> diff --git a/pretty.h b/pretty.h
> @@ -57,31 +58,74 @@ struct userformat_want {
> +/*
> + * Create a text message about commit using given "format" and "context".
> + * Put the result to "sb".
> + * Please use this function for custom formats.
> + */
>  void format_commit_message(const struct commit *commit,
>                         const char *format, struct strbuf *sb,
>                         const struct pretty_print_context *context);
>
> +/*
> + * Parse given arguments from "arg", check it for correctness and
> + * fill struct rev_info.

To be consistent with the way you formatted the other comments, I think you'd want quotes around rev_info.

> + */
>  void get_commit_format(const char *arg, struct rev_info *);
Junio C Hamano· Dec 8, 2017, 17:40 UTC · re: Olga Telezhnaya · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
Show 53 quoted lines
> -extern void get_commit_format(const char *arg, struct rev_info *);
> -extern const char *format_subject(struct strbuf *sb, const char *msg,
> -				  const char *line_separator);
> -extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
> -extern int commit_format_is_empty(enum cmit_fmt);
>  extern const char *skip_blank_lines(const char *msg);
> -extern void format_commit_message(const struct commit *commit,
> -				  const char *format, struct strbuf *sb,
> -				  const struct pretty_print_context *context);
> -extern void pretty_print_commit(struct pretty_print_context *pp,
> -				const struct commit *commit,
> -				struct strbuf *sb);
> -extern void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
> -			   struct strbuf *sb);
> -void pp_user_info(struct pretty_print_context *pp,
> -		  const char *what, struct strbuf *sb,
> -		  const char *line, const char *encoding);
> -void pp_title_line(struct pretty_print_context *pp,
> -		   const char **msg_p,
> -		   struct strbuf *sb,
> -		   const char *encoding,
> -		   int need_8bit_cte);
> -void pp_remainder(struct pretty_print_context *pp,
> -		  const char **msg_p,
> -		  struct strbuf *sb,
> -		  int indent);
> ...
> +void userformat_find_requirements(const char *fmt, struct userformat_want *w);
> +void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
> +			struct strbuf *sb);
> +void pp_user_info(struct pretty_print_context *pp, const char *what,
> +			struct strbuf *sb, const char *line,
> +			const char *encoding);
> +void pp_title_line(struct pretty_print_context *pp, const char **msg_p,
> +			struct strbuf *sb, const char *encoding,
> +			int need_8bit_cte);
> +void pp_remainder(struct pretty_print_context *pp, const char **msg_p,
> +			struct strbuf *sb, int indent);
> +
> +void format_commit_message(const struct commit *commit,
> +			const char *format, struct strbuf *sb,
> +			const struct pretty_print_context *context);
> +
> +void get_commit_format(const char *arg, struct rev_info *);
> +
> +void pretty_print_commit(struct pretty_print_context *pp,
> +			const struct commit *commit,
> +			struct strbuf *sb);
> +
> +const char *format_subject(struct strbuf *sb, const char *msg,
> +			const char *line_separator);
> +
> +int commit_format_is_empty(enum cmit_fmt);

I see you've "standardized" to drop "extern" from the declarations in the header; I have an impression that our preference however is to go in the other direction.

The choice of bits that are moved to the new header looks quite sensible to me.

Thanks.
Jeff King· Dec 10, 2017, 14:53 UTC · re: Junio C Hamano · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

On Fri, Dec 08, 2017 at 09:40:09AM -0800, Junio C Hamano wrote:
> I see you've "standardized" to drop "extern" from the declarations
> in the header; I have an impression that our preference however is
> to go in the other direction.
Can we revisit that?

I haven't see any compelling reason to include the "extern" in a declaration. And all things being equal, I'd prefer the thing that makes the source code shorter, and is one less thing for authors to remember to do.

-Peff
Junio C Hamano· Dec 10, 2017, 22:36 UTC · re: Jeff King · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> On Fri, Dec 08, 2017 at 09:40:09AM -0800, Junio C Hamano wrote:
>
>> I see you've "standardized" to drop "extern" from the declarations
>> in the header; I have an impression that our preference however is
>> to go in the other direction.
>
> Can we revisit that?
>
> I haven't see any compelling reason to include the "extern" in a
> declaration. And all things being equal, I'd prefer the thing that makes
> the source code shorter, and is one less thing for authors to remember
> to do.

Surely, but there is no point revisiting. I simply misremembered what we did at around 1354c9b2 ("refs: remove unnecessary "extern" keywords", 2016-03-31).

As long as we know which way we are standardizing, I personally do not have strong preference either way. I appreciate shorter-to-type (i.e. missing "extern") but I also appreciate the more familiar and logical declaration in a header file that indicates something exists somewhere (i.e. explicit "extern") ;-).

Thanks.
Junio C Hamano· Dec 11, 2017, 20:43 UTC · re: Оля Тележная · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

Оля Тележная  <olyatelezhnaya@gmail.com> writes:
> Is it true that I need to fix only one commit message? (a typo
> s/futher/further/)
>
> Do you have any other advises what do I need to change?

I thought I mentioned that adding #include to all the current users of "commit.h" is way too noisy. I may have pointed out other issues as well, but I offhand do not remember ;-)

Eric Sunshine· Dec 8, 2017, 18:11 UTC · re: Olga Telezhnaya · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

On Fri, Dec 8, 2017 at 8:21 AM, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:

> Create header for pretty.c to make formatting interface more structured.
> This is a middle point, this file would be merged futher with other
s/futher/further/
Show 5 quoted lines
> files which contain formatting stuff.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
Junio C Hamano· Dec 8, 2017, 20:37 UTC · re: Olga Telezhnaya · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
Show 31 quoted lines
>  archive.c             |  1 +
>  builtin/notes.c       |  2 +-
>  builtin/reset.c       |  2 +-
>  builtin/show-branch.c |  2 +-
>  combine-diff.c        |  1 +
>  commit.c              |  1 +
>  commit.h              | 80 ----------------------------------------------
>  diffcore-pickaxe.c    |  1 +
>  grep.c                |  1 +
>  log-tree.c            |  1 +
>  notes-cache.c         |  1 +
>  pretty.h              | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  revision.h            |  2 +-
>  sequencer.c           |  1 +
>  sha1_name.c           |  1 +
>  submodule.c           |  1 +
>  16 files changed, 101 insertions(+), 84 deletions(-)
>  create mode 100644 pretty.h
>
> diff --git a/archive.c b/archive.c
> index 0b7b62af0c3ec..60607e8c00857 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -2,6 +2,7 @@
>  #include "config.h"
>  #include "refs.h"
>  #include "commit.h"
> +#include "pretty.h"
>  #include "tree-walk.h"
>  #include "attr.h"
>  #include "archive.h"

This has a toll on topics in flight that expect the symbols for pretty are available in "commit.h"; they are forced to include this new file they did not even know about.

I notice that "commit.h" is included in "builtin.h"; perhaps adding a new include for "pretty.h" there would be of lessor impact? I dunno.

Оля Тележная· Dec 9, 2017, 07:48 UTC · re: Junio C Hamano · lore

Re: [PATCH Outreachy 1/2] format: create pretty.h file

> I see you've "standardized" to drop "extern" from the declarations
> in the header; I have an impression that our preference however is
> to go in the other direction.

OK, absolutely not a problem, I will return them. Do I need to write "extern" further in function declarations? And why did everyone choose writing "extern" every time? It looks obvious for me that declaration of function is extern, that's why I decided to throw them away.

> The choice of bits that are moved to the new header looks quite
> sensible to me.
I'm very happy and satisfied with it :-)
> s/futher/further/
It was a typo that I missed. Thank you! Will fix it also.
Show 8 quoted lines
> This has a toll on topics in flight that expect the symbols for
> pretty are available in "commit.h"; they are forced to include
> this new file they did not even know about.
>
> I notice that "commit.h" is included in "builtin.h"; perhaps adding
> a new include for "pretty.h" there would be of lessor impact?  I
> dunno.
>

It's a middle point, as I said. I have plans to create unifying format.h then (for all formatting issues). I guess that pretty.h and ref-filter.h will be deleted later. But, I really need to create now that pretty.h because it is much easier to work with existing interface. If you have another ideas how to achieve the main goal - please share them with me, I would appreciate that so much. I am not sure that my solution is the best, but I can't come up with something better for now.

Olga Telezhnaya· Dec 12, 2017, 08:55 UTC · re: Olga Telezhnaya · lore

[PATCH Outreachy v2 1/2] format: create pretty.h file

Create header for pretty.c to make formatting interface more structured. This is a middle point, this file would be merged further with other files which contain formatting stuff.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/notes.c       |  2 +-
 builtin/reset.c       |  2 +-
 builtin/show-branch.c |  2 +-
 commit.h              | 81 +----------------------------------------------
 pretty.h              | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
 revision.h            |  2 +-
 6 files changed, 92 insertions(+), 84 deletions(-)
 create mode 100644 pretty.h
Show changes to 6 files +92 −85

builtin/notes.c, builtin/reset.c, builtin/show-branch.c, commit.h, pretty.h, revision.h

diff --git a/builtin/notes.c b/builtin/notes.c
index 1a2c7d92ad7e7..7c8176164561b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -12,7 +12,7 @@
 #include "builtin.h"
 #include "notes.h"
 #include "blob.h"
-#include "commit.h"
+#include "pretty.h"
 #include "refs.h"
 #include "exec_cmd.h"
 #include "run-command.h"
diff --git a/builtin/reset.c b/builtin/reset.c
index 906e541658230..e15f595799c40 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -12,7 +12,7 @@
 #include "lockfile.h"
 #include "tag.h"
 #include "object.h"
-#include "commit.h"
+#include "pretty.h"
 #include "run-command.h"
 #include "refs.h"
 #include "diff.h"
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 2e24b5c330e8e..e8a4aa40cb4b6 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 #include "config.h"
-#include "commit.h"
+#include "pretty.h"
 #include "refs.h"
 #include "builtin.h"
 #include "color.h"
diff --git a/commit.h b/commit.h
index 99a3fea68d3f6..8c68ca1a5a187 100644
--- a/commit.h
+++ b/commit.h
@@ -7,6 +7,7 @@
 #include "decorate.h"
 #include "gpg-interface.h"
 #include "string-list.h"
+#include "pretty.h"
 
 struct commit_list {
 	struct commit *item;
@@ -121,93 +122,13 @@ struct commit_list *copy_commit_list(struct commit_list *list);
 
 void free_commit_list(struct commit_list *list);
 
-/* Commit formats */
-enum cmit_fmt {
-	CMIT_FMT_RAW,
-	CMIT_FMT_MEDIUM,
-	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
-	CMIT_FMT_SHORT,
-	CMIT_FMT_FULL,
-	CMIT_FMT_FULLER,
-	CMIT_FMT_ONELINE,
-	CMIT_FMT_EMAIL,
-	CMIT_FMT_MBOXRD,
-	CMIT_FMT_USERFORMAT,
-
-	CMIT_FMT_UNSPECIFIED
-};
-
-static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
-{
-	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
-}
-
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 
-struct pretty_print_context {
-	/*
-	 * Callers should tweak these to change the behavior of pp_* functions.
-	 */
-	enum cmit_fmt fmt;
-	int abbrev;
-	const char *after_subject;
-	int preserve_subject;
-	struct date_mode date_mode;
-	unsigned date_mode_explicit:1;
-	int print_email_subject;
-	int expand_tabs_in_log;
-	int need_8bit_cte;
-	char *notes_message;
-	struct reflog_walk_info *reflog_info;
-	struct rev_info *rev;
-	const char *output_encoding;
-	struct string_list *mailmap;
-	int color;
-	struct ident_split *from_ident;
-
-	/*
-	 * Fields below here are manipulated internally by pp_* functions and
-	 * should not be counted on by callers.
-	 */
-	struct string_list in_body_headers;
-	int graph_width;
-};
-
-struct userformat_want {
-	unsigned notes:1;
-};
-
 extern int has_non_ascii(const char *text);
 extern const char *logmsg_reencode(const struct commit *commit,
 				   char **commit_encoding,
 				   const char *output_encoding);
-extern void get_commit_format(const char *arg, struct rev_info *);
-extern const char *format_subject(struct strbuf *sb, const char *msg,
-				  const char *line_separator);
-extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
-extern int commit_format_is_empty(enum cmit_fmt);
 extern const char *skip_blank_lines(const char *msg);
-extern void format_commit_message(const struct commit *commit,
-				  const char *format, struct strbuf *sb,
-				  const struct pretty_print_context *context);
-extern void pretty_print_commit(struct pretty_print_context *pp,
-				const struct commit *commit,
-				struct strbuf *sb);
-extern void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
-			   struct strbuf *sb);
-void pp_user_info(struct pretty_print_context *pp,
-		  const char *what, struct strbuf *sb,
-		  const char *line, const char *encoding);
-void pp_title_line(struct pretty_print_context *pp,
-		   const char **msg_p,
-		   struct strbuf *sb,
-		   const char *encoding,
-		   int need_8bit_cte);
-void pp_remainder(struct pretty_print_context *pp,
-		  const char **msg_p,
-		  struct strbuf *sb,
-		  int indent);
-
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.
diff --git a/pretty.h b/pretty.h
new file mode 100644
index 0000000000000..ef5167484fb64
--- /dev/null
+++ b/pretty.h
@@ -0,0 +1,87 @@
+#ifndef PRETTY_H
+#define PRETTY_H
+
+struct commit;
+
+/* Commit formats */
+enum cmit_fmt {
+	CMIT_FMT_RAW,
+	CMIT_FMT_MEDIUM,
+	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
+	CMIT_FMT_SHORT,
+	CMIT_FMT_FULL,
+	CMIT_FMT_FULLER,
+	CMIT_FMT_ONELINE,
+	CMIT_FMT_EMAIL,
+	CMIT_FMT_MBOXRD,
+	CMIT_FMT_USERFORMAT,
+
+	CMIT_FMT_UNSPECIFIED
+};
+
+struct pretty_print_context {
+	/*
+	 * Callers should tweak these to change the behavior of pp_* functions.
+	 */
+	enum cmit_fmt fmt;
+	int abbrev;
+	const char *after_subject;
+	int preserve_subject;
+	struct date_mode date_mode;
+	unsigned date_mode_explicit:1;
+	int print_email_subject;
+	int expand_tabs_in_log;
+	int need_8bit_cte;
+	char *notes_message;
+	struct reflog_walk_info *reflog_info;
+	struct rev_info *rev;
+	const char *output_encoding;
+	struct string_list *mailmap;
+	int color;
+	struct ident_split *from_ident;
+
+	/*
+	 * Fields below here are manipulated internally by pp_* functions and
+	 * should not be counted on by callers.
+	 */
+	struct string_list in_body_headers;
+	int graph_width;
+};
+
+static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
+{
+	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
+}
+
+struct userformat_want {
+	unsigned notes:1;
+};
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
+			struct strbuf *sb);
+void pp_user_info(struct pretty_print_context *pp, const char *what,
+			struct strbuf *sb, const char *line,
+			const char *encoding);
+void pp_title_line(struct pretty_print_context *pp, const char **msg_p,
+			struct strbuf *sb, const char *encoding,
+			int need_8bit_cte);
+void pp_remainder(struct pretty_print_context *pp, const char **msg_p,
+			struct strbuf *sb, int indent);
+
+void format_commit_message(const struct commit *commit,
+			const char *format, struct strbuf *sb,
+			const struct pretty_print_context *context);
+
+void get_commit_format(const char *arg, struct rev_info *);
+
+void pretty_print_commit(struct pretty_print_context *pp,
+			const struct commit *commit,
+			struct strbuf *sb);
+
+const char *format_subject(struct strbuf *sb, const char *msg,
+			const char *line_separator);
+
+int commit_format_is_empty(enum cmit_fmt);
+
+#endif /* PRETTY_H */
diff --git a/revision.h b/revision.h
index 54761200adf2d..747bce8d8a184 100644
--- a/revision.h
+++ b/revision.h
@@ -4,7 +4,7 @@
 #include "parse-options.h"
 #include "grep.h"
 #include "notes.h"
-#include "commit.h"
+#include "pretty.h"
 #include "diff.h"
 
 /* Remember to update object flag allocation in object.h */

--
https://github.com/git/git/pull/439
Olga Telezhnaya· Dec 12, 2017, 08:55 UTC · re: Olga Telezhnaya · lore

[PATCH Outreachy v2 2/2] format: create docs for pretty.h

Write some docs for functions in pretty.h. Take it as a first draft, they would be changed later.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 pretty.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
Show changes to pretty.h +44 −1
diff --git a/pretty.h b/pretty.h
index ef5167484fb64..5c85d94e332d7 100644
--- a/pretty.h
+++ b/pretty.h
@@ -48,6 +48,7 @@ struct pretty_print_context {
 	int graph_width;
 };
 
+/* Check whether commit format is mail. */
 static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
 {
 	return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
@@ -57,31 +58,74 @@ struct userformat_want {
 	unsigned notes:1;
 };
 
+/* Set the flag "w->notes" if there is placeholder %N in "fmt". */
 void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+
+/*
+ * Shortcut for invoking pretty_print_commit if we do not have any context.
+ * Context would be set empty except "fmt".
+ */
 void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
 			struct strbuf *sb);
+
+/*
+ * Get information about user and date from "line", format it and
+ * put it into "sb".
+ * Format of "line" must be readable for split_ident_line function.
+ * The resulting format is "what: name <email> date".
+ */
 void pp_user_info(struct pretty_print_context *pp, const char *what,
 			struct strbuf *sb, const char *line,
 			const char *encoding);
+
+/*
+ * Format title line of commit message taken from "msg_p" and
+ * put it into "sb".
+ * First line of "msg_p" is also affected.
+ */
 void pp_title_line(struct pretty_print_context *pp, const char **msg_p,
 			struct strbuf *sb, const char *encoding,
 			int need_8bit_cte);
+
+/*
+ * Get current state of commit message from "msg_p" and continue formatting
+ * by adding indentation and '>' signs. Put result into "sb".
+ */
 void pp_remainder(struct pretty_print_context *pp, const char **msg_p,
 			struct strbuf *sb, int indent);
 
+/*
+ * Create a text message about commit using given "format" and "context".
+ * Put the result to "sb".
+ * Please use this function for custom formats.
+ */
 void format_commit_message(const struct commit *commit,
 			const char *format, struct strbuf *sb,
 			const struct pretty_print_context *context);
 
+/*
+ * Parse given arguments from "arg", check it for correctness and
+ * fill struct rev_info.
+ */
 void get_commit_format(const char *arg, struct rev_info *);
 
+/*
+ * Make a commit message with all rules from given "pp"
+ * and put it into "sb".
+ * Please use this function if you have a context (candidate for "pp").
+ */
 void pretty_print_commit(struct pretty_print_context *pp,
 			const struct commit *commit,
 			struct strbuf *sb);
 
+/*
+ * Change line breaks in "msg" to "line_separator" and put it into "sb".
+ * Return "msg" itself.
+ */
 const char *format_subject(struct strbuf *sb, const char *msg,
 			const char *line_separator);
 
+/* Check if "cmit_fmt" will produce an empty output. */
 int commit_format_is_empty(enum cmit_fmt);
 
 #endif /* PRETTY_H */

--
https://github.com/git/git/pull/439
Junio C Hamano· Dec 12, 2017, 22:12 UTC · re: Olga Telezhnaya · lore

Re: [PATCH Outreachy v2 1/2] format: create pretty.h file

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
Show 32 quoted lines
>  builtin/notes.c       |  2 +-
>  builtin/reset.c       |  2 +-
>  builtin/show-branch.c |  2 +-
>  commit.h              | 81 +----------------------------------------------
>  pretty.h              | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  revision.h            |  2 +-
>  6 files changed, 92 insertions(+), 84 deletions(-)
>  create mode 100644 pretty.h
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 1a2c7d92ad7e7..7c8176164561b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -12,7 +12,7 @@
>  #include "builtin.h"
>  #include "notes.h"
>  #include "blob.h"
> -#include "commit.h"
> +#include "pretty.h"
>  #include "refs.h"
>  #include "exec_cmd.h"
>  #include "run-command.h"
> ...
> diff --git a/commit.h b/commit.h
> index 99a3fea68d3f6..8c68ca1a5a187 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -7,6 +7,7 @@
>  #include "decorate.h"
>  #include "gpg-interface.h"
>  #include "string-list.h"
> +#include "pretty.h"

This is much nicer than what I imagined, which was to just add this line here, move decls from commit.h to pretty.h, and do nothing else, which would be the absolute safest thing from the point of view of other topics in flight. Separation of "pretty.h" would stay to be an implementation detail of the "commit.h" file, where everybody expects to find these decls.

Instead, this patch inspects each and every .c user of "commit.h" and replaces its '#include' with the new one if it only uses things declared in "pretty.h", which makes it very clear who have been depending on what in the patch. Those that include "commit.h" because they need both the "what is a commit object" aspect and "how to pretty print" aspect can keep using their original '#include' to ease the transition.

Let's see how well this plays with other topics in flight---I had to apply an evil merge to queue the previous one, if I recall right, as a user of "commit.h" that did not use pretty-print (hence did not get "pretty.h" with the previous round of this patch) gained use of pretty-print function, or something like that.

Will queue.
Thanks.

← back to recent threads