Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations
On Fri, Apr 5, 2013 at 6:57 PM, Jakub Narębski 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
Sorry for this late reply. I've been quite busy lately..
On Tue, Apr 2, 2013 at 4:53 AM, Junio C Hamano 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
Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy 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
Nguyễn Thái Ngọc Duy writes:
> This also adds color support to format_decoration()
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> 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..
[PATCH v2 02/12] pretty: share code between format_decoration and show_decorations
This also adds color support to format_decoration()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
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);
-
