Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Duy Nguyen]

Sorry for this late reply. I've been quite busy lately..

On Tue, Apr 2, 2013 at 4:53 AM, Junio C Hamano gits...@pobox.com wrote:
 -void show_decorations(struct rev_info *opt, struct commit *commit)
 +void format_decoration(struct strbuf *sb,
 +const struct commit *commit,
 +int use_color)
  {
 - const char *prefix;
 - struct name_decoration *decoration;
 + const char *prefix =  (;
 + struct name_decoration *d;

 This renaming of variable from decoration to d seems to make the
 patched result unnecessarily different from the original in
 show_decorations, making it harder to compare.  Intentional?

I think I just happened to reuse the style of the old
format_decoration(). Will reuse the name decoration instead.

   const char *color_commit =
 - diff_get_color_opt(opt-diffopt, DIFF_COMMIT);
 + diff_get_color(use_color, DIFF_COMMIT);
   const char *color_reset =
 - decorate_get_color_opt(opt-diffopt, DECORATION_NONE);
 + decorate_get_color(use_color, DECORATION_NONE);
 +
 + load_ref_decorations(DECORATE_SHORT_REFS);

 In cmd_log_init_finish(), we have loaded decorations with specified
 decoration_style already.  Why is this needed (and with a hardcoded
 style that may be different from what the user specified)?

legacy from pretty.c:format_decoration(). Will move it to the caller
format_commit_one.


 + d = lookup_decoration(name_decoration, commit-object);
 + if (!d)
 + return;
 + while (d) {
 + strbuf_addstr(sb, color_commit);
 + strbuf_addstr(sb, prefix);
 + strbuf_addstr(sb, decorate_get_color(use_color, d-type));
 + if (d-type == DECORATION_REF_TAG)
 + strbuf_addstr(sb, tag: );
 + strbuf_addstr(sb, d-name);
 + strbuf_addstr(sb, color_reset);
 + prefix = , ;
 + d = d-next;
 + }
 + if (prefix[0] == ',') {
 + strbuf_addstr(sb, color_commit);
 + strbuf_addch(sb, ')');
 + strbuf_addstr(sb, color_reset);
 + }

 Was this change to conditionally close ' (' mentioned in the log
 message?  It is in line with the version taken from pretty.c, and I
 think it may be an improvement, but I do not think the check is
 necessary in the context of this function.  You will never see
 prefix[0] != ',' after the loop, because while (d) above runs at
 least once; otherwise the if (!d) return would have returned from
 the function early, no?

Yes, your eyeballs have really good quality ;)

 @@ -625,8 +639,8 @@ void show_log(struct rev_info *opt)
   printf( (from %s),
  find_unique_abbrev(parent-object.sha1,
 abbrev_commit));
 + fputs(diff_get_color_opt(opt-diffopt, DIFF_RESET), stdout);
   show_decorations(opt, commit);
 - printf(%s, diff_get_color_opt(opt-diffopt, DIFF_RESET));

 We used to show and then reset.  I can see the updated
 show_decorations() to format_decoration() callchain always reset at
 the end, so the loss of the final reset here is very sane, but is
 there a need to reset beforehand?  What is the calling convention
 for the updated show_decorations()?  The caller should make sure
 there is no funny colors in effect before calling, and the caller
 can rest assured that there is no funny colors when the function
 returns?

I think it's a sane convention, unless we want a some background color
going through show_decorations.

 +void format_decoration(struct strbuf *sb,
 +const struct commit *commit,
 +int use_color);

 I think you can fit these on a single line, especially if you drop
 the unused variable names (they help when there are more than one
 parameter of the same type to document the order of the arguments,
 but that does not apply here).  That would help people who run
 grep on the header files without using CTAGS/ETAGS.

No problem.

 Wouldn't it be format_decorations(), or does it handle only one?

All in one, apparently. Will rename.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Duy Nguyen]

On Fri, Apr 5, 2013 at 6:57 PM, Jakub Narębski jna...@gmail.com wrote:
 +void format_decoration(struct strbuf *sb,
 +   const struct commit *commit,
 +   int use_color);

 I think you can fit these on a single line, especially if you drop
 the unused variable names (they help when there are more than one
 parameter of the same type to document the order of the arguments,
 but that does not apply here).  That would help people who run
 grep on the header files without using CTAGS/ETAGS.

 Well, I think int use_color should be left with variable name,
 don't you think?

I don't care too much about this. If Junio does not respond, I'll
leave the names in place (in one long line).
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Jakub Narębski]

Junio C Hamano wrote:
 Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 diff --git a/log-tree.h b/log-tree.h
 index 9140f48..e6a2da5 100644
 --- a/log-tree.h
 +++ b/log-tree.h
 @@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
  int log_tree_commit(struct rev_info *, struct commit *);
  int log_tree_opt_parse(struct rev_info *, const char **, int);
  void show_log(struct rev_info *opt);
 +void format_decoration(struct strbuf *sb,
 +   const struct commit *commit,
 +   int use_color);
 
 I think you can fit these on a single line, especially if you drop
 the unused variable names (they help when there are more than one
 parameter of the same type to document the order of the arguments,
 but that does not apply here).  That would help people who run
 grep on the header files without using CTAGS/ETAGS.

Well, I think int use_color should be left with variable name,
don't you think?

-- 
Jakub Narębski

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 This also adds color support to format_decoration()

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  log-tree.c   | 60 
 +---
  log-tree.h   |  3 ++
  pretty.c | 19 +
  t/t4207-log-decoration-colors.sh |  8 +++---
  4 files changed, 45 insertions(+), 45 deletions(-)

 diff --git a/log-tree.c b/log-tree.c
 index 5dc45c4..7467a1d 100644
 --- a/log-tree.c
 +++ b/log-tree.c
 @@ -174,36 +174,50 @@ static void show_children(struct rev_info *opt, struct 
 commit *commit, int abbre
   }
  }
  
 -void show_decorations(struct rev_info *opt, struct commit *commit)
 +void format_decoration(struct strbuf *sb,
 +const struct commit *commit,
 +int use_color)
  {
 - const char *prefix;
 - struct name_decoration *decoration;
 + const char *prefix =  (;
 + struct name_decoration *d;

This renaming of variable from decoration to d seems to make the
patched result unnecessarily different from the original in
show_decorations, making it harder to compare.  Intentional?

   const char *color_commit =
 - diff_get_color_opt(opt-diffopt, DIFF_COMMIT);
 + diff_get_color(use_color, DIFF_COMMIT);
   const char *color_reset =
 - decorate_get_color_opt(opt-diffopt, DECORATION_NONE);
 + decorate_get_color(use_color, DECORATION_NONE);
 +
 + load_ref_decorations(DECORATE_SHORT_REFS);

In cmd_log_init_finish(), we have loaded decorations with specified
decoration_style already.  Why is this needed (and with a hardcoded
style that may be different from what the user specified)?

 + d = lookup_decoration(name_decoration, commit-object);
 + if (!d)
 + return;
 + while (d) {
 + strbuf_addstr(sb, color_commit);
 + strbuf_addstr(sb, prefix);
 + strbuf_addstr(sb, decorate_get_color(use_color, d-type));
 + if (d-type == DECORATION_REF_TAG)
 + strbuf_addstr(sb, tag: );
 + strbuf_addstr(sb, d-name);
 + strbuf_addstr(sb, color_reset);
 + prefix = , ;
 + d = d-next;
 + }
 + if (prefix[0] == ',') {
 + strbuf_addstr(sb, color_commit);
 + strbuf_addch(sb, ')');
 + strbuf_addstr(sb, color_reset);
 + }

Was this change to conditionally close ' (' mentioned in the log
message?  It is in line with the version taken from pretty.c, and I
think it may be an improvement, but I do not think the check is
necessary in the context of this function.  You will never see
prefix[0] != ',' after the loop, because while (d) above runs at
least once; otherwise the if (!d) return would have returned from
the function early, no?

 +}
 +
 +void show_decorations(struct rev_info *opt, struct commit *commit)
 +{
 + struct strbuf sb = STRBUF_INIT;
  
   if (opt-show_source  commit-util)
   printf(\t%s, (char *) commit-util);
   if (!opt-show_decorations)
   return;
 - decoration = lookup_decoration(name_decoration, commit-object);
 - if (!decoration)
 - return;
 - prefix =  (;
 - while (decoration) {
 - printf(%s, prefix);
 - fputs(decorate_get_color_opt(opt-diffopt, decoration-type),
 -   stdout);
 - if (decoration-type == DECORATION_REF_TAG)
 - fputs(tag: , stdout);
 - printf(%s, decoration-name);
 - fputs(color_reset, stdout);
 - fputs(color_commit, stdout);
 - prefix = , ;
 - decoration = decoration-next;
 - }
 - putchar(')');
 + format_decoration(sb, commit, opt-diffopt.use_color);
 + fputs(sb.buf, stdout);
 + strbuf_release(sb);
  }
  
  /*
 @@ -625,8 +639,8 @@ void show_log(struct rev_info *opt)
   printf( (from %s),
  find_unique_abbrev(parent-object.sha1,
 abbrev_commit));
 + fputs(diff_get_color_opt(opt-diffopt, DIFF_RESET), stdout);
   show_decorations(opt, commit);
 - printf(%s, diff_get_color_opt(opt-diffopt, DIFF_RESET));

We used to show and then reset.  I can see the updated
show_decorations() to format_decoration() callchain always reset at
the end, so the loss of the final reset here is very sane, but is
there a need to reset beforehand?  What is the calling convention
for the updated show_decorations()?  The caller should make sure
there is no funny colors in effect before calling, and the caller
can rest assured that there is no funny colors when the function
returns?

 diff --git a/log-tree.h b/log-tree.h
 index 9140f48..e6a2da5 100644
 --- a/log-tree.h
 +++ b/log-tree.h
 @@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
  int 

[PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Nguyễn Thái Ngọc Duy]

This also adds color support to format_decoration()

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 log-tree.c   | 60 +---
 log-tree.h   |  3 ++
 pretty.c | 19 +
 t/t4207-log-decoration-colors.sh |  8 +++---
 4 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 5dc45c4..7467a1d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -174,36 +174,50 @@ static void show_children(struct rev_info *opt, struct 
commit *commit, int abbre
}
 }
 
-void show_decorations(struct rev_info *opt, struct commit *commit)
+void format_decoration(struct strbuf *sb,
+  const struct commit *commit,
+  int use_color)
 {
-   const char *prefix;
-   struct name_decoration *decoration;
+   const char *prefix =  (;
+   struct name_decoration *d;
const char *color_commit =
-   diff_get_color_opt(opt-diffopt, DIFF_COMMIT);
+   diff_get_color(use_color, DIFF_COMMIT);
const char *color_reset =
-   decorate_get_color_opt(opt-diffopt, DECORATION_NONE);
+   decorate_get_color(use_color, DECORATION_NONE);
+
+   load_ref_decorations(DECORATE_SHORT_REFS);
+   d = lookup_decoration(name_decoration, commit-object);
+   if (!d)
+   return;
+   while (d) {
+   strbuf_addstr(sb, color_commit);
+   strbuf_addstr(sb, prefix);
+   strbuf_addstr(sb, decorate_get_color(use_color, d-type));
+   if (d-type == DECORATION_REF_TAG)
+   strbuf_addstr(sb, tag: );
+   strbuf_addstr(sb, d-name);
+   strbuf_addstr(sb, color_reset);
+   prefix = , ;
+   d = d-next;
+   }
+   if (prefix[0] == ',') {
+   strbuf_addstr(sb, color_commit);
+   strbuf_addch(sb, ')');
+   strbuf_addstr(sb, color_reset);
+   }
+}
+
+void show_decorations(struct rev_info *opt, struct commit *commit)
+{
+   struct strbuf sb = STRBUF_INIT;
 
if (opt-show_source  commit-util)
printf(\t%s, (char *) commit-util);
if (!opt-show_decorations)
return;
-   decoration = lookup_decoration(name_decoration, commit-object);
-   if (!decoration)
-   return;
-   prefix =  (;
-   while (decoration) {
-   printf(%s, prefix);
-   fputs(decorate_get_color_opt(opt-diffopt, decoration-type),
- stdout);
-   if (decoration-type == DECORATION_REF_TAG)
-   fputs(tag: , stdout);
-   printf(%s, decoration-name);
-   fputs(color_reset, stdout);
-   fputs(color_commit, stdout);
-   prefix = , ;
-   decoration = decoration-next;
-   }
-   putchar(')');
+   format_decoration(sb, commit, opt-diffopt.use_color);
+   fputs(sb.buf, stdout);
+   strbuf_release(sb);
 }
 
 /*
@@ -625,8 +639,8 @@ void show_log(struct rev_info *opt)
printf( (from %s),
   find_unique_abbrev(parent-object.sha1,
  abbrev_commit));
+   fputs(diff_get_color_opt(opt-diffopt, DIFF_RESET), stdout);
show_decorations(opt, commit);
-   printf(%s, diff_get_color_opt(opt-diffopt, DIFF_RESET));
if (opt-commit_format == CMIT_FMT_ONELINE) {
putchar(' ');
} else {
diff --git a/log-tree.h b/log-tree.h
index 9140f48..e6a2da5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
 void show_log(struct rev_info *opt);
+void format_decoration(struct strbuf *sb,
+  const struct commit *commit,
+  int use_color);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 const char **subject_p,
diff --git a/pretty.c b/pretty.c
index eae57ad..79784be 100644
--- a/pretty.c
+++ b/pretty.c
@@ -898,23 +898,6 @@ static void parse_commit_message(struct 
format_commit_context *c)
c-commit_message_parsed = 1;
 }
 
-static void format_decoration(struct strbuf *sb, const struct commit *commit)
-{
-   struct name_decoration *d;
-   const char *prefix =  (;
-
-   load_ref_decorations(DECORATE_SHORT_REFS);
-   d = lookup_decoration(name_decoration, commit-object);
-   while (d) {
-   strbuf_addstr(sb, prefix);
-   prefix = , ;
-   strbuf_addstr(sb, d-name);
-   d = d-next;
-   }
-   if