Re: [PATCH v2 08/14] revision.c: use commit-slab for show_source

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> -refname = commit->util;
>> +refname = *revision_sources_peek(_sources, commit);
>
> At first glance, I thought that the reason why this uses _peek() is
> because the "sources" is expected to only sparsely be populated,
> perhaps because get_tags_and_duplicates() annotates only the tips
> mentioned on the command line via rev_cmdline mechanism and this
> code does not want to auto-vivify the slot, only to read NULL from
> it.
>
> But the code that follows this point liberally uses refname without
> checking if it is NULL, so I am not quite sure what is going on.

Ah, of course, this is about the code that propagates the "source"
(i.e. from which tip given on the command line did we start the
traversal to reach this commit?), so that is what ensures there is
something in commit->util and revision_sources not just has an entry
for the commit but the entry should have a non-NULL string.

So shouldn't *slab_peek() here be *slab_at() instead?

> In
> any case, wouldn't *slab_peek() an anti-pattern?  You use _peek()
> because you expect that a slot is not yet allocated for a commit,
> you desire not to vivify all the slots for all the commits, and
> instead you are prepared to see a NULL returned from the call.  But
> I do not think that is what is happening here, so shouldn't it be
> using _at() instead of _peek()?
>
>>  if (anonymize) {
>>  refname = anonymize_refname(refname);
>
>>  anonymize_ident_line(, _end);
>> @@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct 
>> rev_cmdline_info *info)
>>   * This ref will not be updated through a commit, lets make
>>   * sure it gets properly updated eventually.
>>   */
>> -if (commit->util || commit->object.flags & SHOWN)
>> +if (*revision_sources_at(_sources, commit) ||
>> +commit->object.flags & SHOWN)
>
> Here it uses *slab_at() which makes more sense.

Re: [PATCH v2 08/14] revision.c: use commit-slab for show_source

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> - refname = commit->util;
> + refname = *revision_sources_peek(_sources, commit);

At first glance, I thought that the reason why this uses _peek() is
because the "sources" is expected to only sparsely be populated,
perhaps because get_tags_and_duplicates() annotates only the tips
mentioned on the command line via rev_cmdline mechanism and this
code does not want to auto-vivify the slot, only to read NULL from
it.

But the code that follows this point liberally uses refname without
checking if it is NULL, so I am not quite sure what is going on. In
any case, wouldn't *slab_peek() an anti-pattern?  You use _peek()
because you expect that a slot is not yet allocated for a commit,
you desire not to vivify all the slots for all the commits, and
instead you are prepared to see a NULL returned from the call.  But
I do not think that is what is happening here, so shouldn't it be
using _at() instead of _peek()?

>   if (anonymize) {
>   refname = anonymize_refname(refname);

>   anonymize_ident_line(, _end);
> @@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct 
> rev_cmdline_info *info)
>* This ref will not be updated through a commit, lets make
>* sure it gets properly updated eventually.
>*/
> - if (commit->util || commit->object.flags & SHOWN)
> + if (*revision_sources_at(_sources, commit) ||
> + commit->object.flags & SHOWN)

Here it uses *slab_at() which makes more sense.

[PATCH v2 08/14] revision.c: use commit-slab for show_source

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

Instead of relying on commit->util to store the source string, let the
user provide a commit-slab to store the source strings in.

It's done so that commit->util can be removed. See more explanation in
the commit that removes commit->util.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/fast-export.c | 14 +-
 builtin/log.c |  7 +--
 log-tree.c|  8 ++--
 revision.c| 19 +++
 revision.h|  5 -
 5 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 530df12f05..b08e5ea0e3 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -21,6 +21,7 @@
 #include "quote.h"
 #include "remote.h"
 #include "blob.h"
+#include "commit-slab.h"
 
 static const char *fast_export_usage[] = {
N_("git fast-export [rev-list-opts]"),
@@ -38,6 +39,7 @@ static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
 static struct refspec *refspecs;
 static int refspecs_nr;
 static int anonymize;
+static struct revision_sources revision_sources;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
 const char *arg, int unset)
@@ -590,7 +592,7 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev,
if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
export_blob(_queued_diff.queue[i]->two->oid);
 
-   refname = commit->util;
+   refname = *revision_sources_peek(_sources, commit);
if (anonymize) {
refname = anonymize_refname(refname);
anonymize_ident_line(, _end);
@@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct 
rev_cmdline_info *info)
 * This ref will not be updated through a commit, lets make
 * sure it gets properly updated eventually.
 */
-   if (commit->util || commit->object.flags & SHOWN)
+   if (*revision_sources_at(_sources, commit) ||
+   commit->object.flags & SHOWN)
string_list_append(_refs, full_name)->util = 
commit;
-   if (!commit->util)
-   commit->util = full_name;
+   if (!*revision_sources_at(_sources, commit))
+   *revision_sources_at(_sources, commit) = 
full_name;
}
 }
 
@@ -1029,8 +1032,9 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
git_config(git_default_config, NULL);
 
init_revisions(, prefix);
+   init_revision_sources(_sources);
revs.topo_order = 1;
-   revs.show_source = 1;
+   revs.sources = _sources;
revs.rewrite_parents = 1;
argc = parse_options(argc, argv, prefix, options, fast_export_usage,
PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
diff --git a/builtin/log.c b/builtin/log.c
index 71f68a3e4f..d017e57475 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -148,6 +148,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
static struct string_list decorate_refs_include = 
STRING_LIST_INIT_NODUP;
struct decoration_filter decoration_filter = {_refs_include,
  _refs_exclude};
+   static struct revision_sources revision_sources;
 
const struct option builtin_log_options[] = {
OPT__QUIET(, N_("suppress diff output")),
@@ -194,8 +195,10 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
rev->diffopt.filter || rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
 
-   if (source)
-   rev->show_source = 1;
+   if (source) {
+   init_revision_sources(_sources);
+   rev->sources = _sources;
+   }
 
if (mailmap) {
rev->mailmap = xcalloc(1, sizeof(struct string_list));
diff --git a/log-tree.c b/log-tree.c
index d1c0bedf24..0b41ee3235 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -295,8 +295,12 @@ void show_decorations(struct rev_info *opt, struct commit 
*commit)
 {
struct strbuf sb = STRBUF_INIT;
 
-   if (opt->show_source && commit->util)
-   fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
+   if (opt->sources) {
+   char **slot = revision_sources_peek(opt->sources, commit);
+
+   if (slot && *slot)
+   fprintf(opt->diffopt.file, "\t%s", *slot);
+   }
if (!opt->show_decorations)
return;
format_decorations(, commit, opt->diffopt.use_color);
diff --git a/revision.c b/revision.c
index 1cff11833e..be8fe7d67b 100644
--- a/revision.c
+++ b/revision.c
@@ -29,6 +29,8 @@ volatile show_early_output_fn_t show_early_output;
 static const char *term_bad;
 static const char *term_good;