[PATCH 0/2] Small log simplifications

These are just some things I noticed while looking at the revision
walking code. I'm not very familiar with the code at all, so the
patches may very well be completely wrong; view them as newbie
questions in patch-form if you like.


Martin von Zweigbergk (2):
  remove unnecessary parameter from get_patch_ids()
  log: remove redundant check for merge commit

 builtin/log.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

-- 
1.7.11.1.104.ge7b44f1

While walking the revision list in get_patch_ids and cmd_cherry, we
ignore merges by checking if there is more than one parent. However,
since the revision walk was initialized with revs.ignore_merges = 1,
this would never happen. Remove the unnecessary checks.

Also re-initializing rev_info fields to the same values already set in
init_revisions().

Signed-off-by: Martin von Zweigbergk <redacted>
---
 builtin/log.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 7a92e3f..8182a18 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -726,10 +726,6 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 		die(_("revision walk setup failed"));
 
 	while ((commit = get_revision(&check_rev)) != NULL) {
-		/* ignore merges */
-		if (commit->parents && commit->parents->next)
-			continue;
-
 		add_commit_patch_id(commit, ids);
 	}
 

@@ -1509,8 +1505,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&revs, prefix);
 	revs.diff = 1;
-	revs.combine_merges = 0;
-	revs.ignore_merges = 1;
 	DIFF_OPT_SET(&revs.diffopt, RECURSIVE);
 
 	if (add_pending_commit(head, &revs, 0))

@@ -1534,10 +1528,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 	while ((commit = get_revision(&revs)) != NULL) {
-		/* ignore merges */
-		if (commit->parents && commit->parents->next)
-			continue;
-
 		commit_list_insert(commit, &list);
 	}
 

-- 
1.7.11.1.104.ge7b44f1

get_patch_ids() takes an already initialized rev_info and a
prefix. The prefix is used when initalizing a second rev_info. Since
the initialized rev_info already has a prefix and it seems the prefix
doesn't change, we can used the prefix from the initialized rev_info
to initialize the second rev_info.

Signed-off-by: Martin von Zweigbergk <redacted>
---
 builtin/log.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index ecc2793..7a92e3f 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -696,7 +696,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 	return 0;
 }
 
-static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const char *prefix)
+static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 {
 	struct rev_info check_rev;
 	struct commit *commit;

@@ -717,7 +717,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 	init_patch_ids(ids);
 
 	/* given a range a..b get all patch ids for b..a */
-	init_revisions(&check_rev, prefix);
+	init_revisions(&check_rev, rev->prefix);
 	o1->flags ^= UNINTERESTING;
 	o2->flags ^= UNINTERESTING;
 	add_pending_object(&check_rev, o1, "o1");

@@ -1306,7 +1306,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			if (hashcmp(o[0].item->sha1, o[1].item->sha1) == 0)
 				return 0;
 		}
-		get_patch_ids(&rev, &ids, prefix);
+		get_patch_ids(&rev, &ids);
 	}
 
 	if (!use_stdout)

@@ -1525,7 +1525,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			return 0;
 	}
 
-	get_patch_ids(&revs, &ids, prefix);
+	get_patch_ids(&revs, &ids);
 
 	if (limit && add_pending_commit(limit, &revs, UNINTERESTING))
 		die(_("Unknown commit %s"), limit);

-- 
1.7.11.1.104.ge7b44f1

Martin von Zweigbergk [off-list ref] writes:

get_patch_ids() takes an already initialized rev_info and a
prefix. The prefix is used when initalizing a second rev_info. Since
the initialized rev_info already has a prefix and it seems the prefix
doesn't change, we can used the prefix from the initialized rev_info
to initialize the second rev_info.

s/it seems the prefix doesn't change/the prefix never changes/;

Signed-off-by: Martin von Zweigbergk <redacted>

I think this change shouldn't change the behaviour and is good.

Just FYI, during the revision traversal, prefix affects two things
in the current code:

 (1) it is used as the basename for prune data that determine which
     parts of the tree structure to pay attention to (e.g. "cd t &&
     git log .").

 (2) it is used as the basename for diff output when revs->diffopt
     is used (e.g. "cd t && git log -p").

And check_rev is used with neither.  It does not want to prune any
part of the tree nor it wants to filter with fancier options like
grep/pickaxe (i.e. it does not use setup_revisions()).

In that sense it probably does not even matter if you did not pass
rev->prefix down to check_rev, and instead gave NULL to it, but that
only holds true in the current codebase, so I think what your patch
does is the right thing to do.

Thanks.

On Fri, Jul 27, 2012 at 3:44 PM, Junio C Hamano [off-list ref] wrote:

Martin von Zweigbergk [off-list ref] writes:

s/it seems the prefix doesn't change/the prefix never changes/;

:-) Thanks for confirming.

In that sense it probably does not even matter if you did not pass
rev->prefix down to check_rev, and instead gave NULL to it, but that
only holds true in the current codebase, so I think what your patch
does is the right thing to do.

Makes sense. Thanks for explaining!