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

[Junio C Hamano]

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

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

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]

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

[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

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

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

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

[Junio C Hamano]

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

[Eric Sunshine]

On Fri, Dec 8, 2017 at 8:21 AM, Olga Telezhnaya
 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 
> Mentored-by: Christian Couder 
> Mentored by: Jeff King 

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

[Junio C Hamano]

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

[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 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 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