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

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

[Olga Telezhnaya]

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

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

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

[Olga Telezhnaya]

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(+)

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

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

[Eric Sunshine]

On Fri, Dec 8, 2017 at 8:21 AM, Olga Telezhnaya
<olyatelezhnaya@gmail.com> wrote:
> 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 *);

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

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> -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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Оля Тележная]

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?

Thanks!

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

[Junio C Hamano]

Оля Тележная  <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 ;-)

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

[Eric Sunshine]

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/

> 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>

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

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

>  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.

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.


> 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.

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

[Olga Telezhnaya]

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

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

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

[Olga Telezhnaya]

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(+)

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

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

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

>  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.