Re: [PATCH 3/3] Tie it all together: "git log"

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 3/3] Tie it all together: "git log"

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:20

Linus Torvalds [off-list ref] writes:
Anyway, apart from that issue (which I think should be trivial to sort out 
if we accept breaking --merge-order), the rest looks like it should just 
get more testing and handling of the few missing flags from rev-parse in 
revision.c, and it should be good.
I would say we should just rip merge-order out.  Who uses it,
and why does it not work with topo-order, again?

Re: [PATCH 3/3] Tie it all together: "git log"

From: Linus Torvalds <torvalds@osdl.org>
Date: 2016-06-15 22:42:20


On Tue, 28 Feb 2006, Junio C Hamano wrote:
I would say we should just rip merge-order out.  Who uses it,
and why does it not work with topo-order, again?
Well, assuming breaking --merge-order is fine, here's a patch (on top of 
the other ones) that makes

	git log <filename>

actually work, as far as I can tell. 

I didn't add the logic for --before/--after flags, but that should be 
pretty trivial, and is independent of this anyway.

Perhaps more importantly, I didn't remove the tests that now start 
failing, nor did I remove the actual code to do --merge-order ;/

		Linus

----
diff --git a/rev-list.c b/rev-list.c
index 94f22dd..6993b1a 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -8,10 +8,9 @@
 #include "diff.h"
 #include "revision.h"
 
-/* bits #0-2 in revision.h */
+/* bits #0-3 in revision.h */
 
-#define COUNTED		(1u << 3)
-#define SHOWN		(1u << 4)
+#define COUNTED		(1u << 4)
 #define TMP_MARK	(1u << 5) /* for isolated cases; clean after use */
 
 static const char rev_list_usage[] =
@@ -25,7 +24,6 @@ static const char rev_list_usage[] =
 "    --remove-empty\n"
 "    --all\n"
 "  ordering output:\n"
-"    --merge-order [ --show-breaks ]\n"
 "    --topo-order\n"
 "    --date-order\n"
 "  formatting output:\n"
@@ -47,22 +45,9 @@ static int show_parents = 0;
 static int hdr_termination = 0;
 static const char *commit_prefix = "";
 static enum cmit_fmt commit_format = CMIT_FMT_RAW;
-static int merge_order = 0;
-static int show_breaks = 0;
-static int stop_traversal = 0;
-static int no_merges = 0;
 
 static void show_commit(struct commit *commit)
 {
-	commit->object.flags |= SHOWN;
-	if (show_breaks) {
-		commit_prefix = "| ";
-		if (commit->object.flags & DISCONTINUITY) {
-			commit_prefix = "^ ";     
-		} else if (commit->object.flags & BOUNDARY) {
-			commit_prefix = "= ";
-		} 
-        }        		
 	printf("%s%s", commit_prefix, sha1_to_hex(commit->object.sha1));
 	if (show_parents) {
 		struct commit_list *parents = commit->parents;
@@ -96,73 +81,6 @@ static void show_commit(struct commit *c
 	fflush(stdout);
 }
 
-static int rewrite_one(struct commit **pp)
-{
-	for (;;) {
-		struct commit *p = *pp;
-		if (p->object.flags & (TREECHANGE | UNINTERESTING))
-			return 0;
-		if (!p->parents)
-			return -1;
-		*pp = p->parents->item;
-	}
-}
-
-static void rewrite_parents(struct commit *commit)
-{
-	struct commit_list **pp = &commit->parents;
-	while (*pp) {
-		struct commit_list *parent = *pp;
-		if (rewrite_one(&parent->item) < 0) {
-			*pp = parent->next;
-			continue;
-		}
-		pp = &parent->next;
-	}
-}
-
-static int filter_commit(struct commit * commit)
-{
-	if (stop_traversal && (commit->object.flags & BOUNDARY))
-		return STOP;
-	if (commit->object.flags & (UNINTERESTING|SHOWN))
-		return CONTINUE;
-	if (revs.min_age != -1 && (commit->date > revs.min_age))
-		return CONTINUE;
-	if (revs.max_age != -1 && (commit->date < revs.max_age)) {
-		stop_traversal=1;
-		return CONTINUE;
-	}
-	if (no_merges && (commit->parents && commit->parents->next))
-		return CONTINUE;
-	if (revs.paths && revs.dense) {
-		if (!(commit->object.flags & TREECHANGE))
-			return CONTINUE;
-		rewrite_parents(commit);
-	}
-	return DO;
-}
-
-static int process_commit(struct commit * commit)
-{
-	int action=filter_commit(commit);
-
-	if (action == STOP) {
-		return STOP;
-	}
-
-	if (action == CONTINUE) {
-		return CONTINUE;
-	}
-
-	if (revs.max_count != -1 && !revs.max_count--)
-		return STOP;
-
-	show_commit(commit);
-
-	return CONTINUE;
-}
-
 static struct object_list **process_blob(struct blob *blob,
 					 struct object_list **p,
 					 struct name_path *path,
@@ -219,8 +137,7 @@ static void show_commit_list(struct rev_
 
 	while ((commit = get_revision(revs)) != NULL) {
 		p = process_tree(commit->tree, p, NULL, "");
-		if (process_commit(commit) == STOP)
-			break;
+		show_commit(commit);
 	}
 	for (pending = revs->pending_objects; pending; pending = pending->next) {
 		struct object *obj = pending->item;
@@ -416,10 +333,6 @@ int main(int argc, const char **argv)
 				commit_prefix = "commit ";
 			continue;
 		}
-		if (!strncmp(arg, "--no-merges", 11)) {
-			no_merges = 1;
-			continue;
-		}
 		if (!strcmp(arg, "--parents")) {
 			show_parents = 1;
 			continue;
@@ -428,14 +341,6 @@ int main(int argc, const char **argv)
 			bisect_list = 1;
 			continue;
 		}
-		if (!strcmp(arg, "--merge-order")) {
-		        merge_order = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--show-breaks")) {
-			show_breaks = 1;
-			continue;
-		}
 		usage(rev_list_usage);
 
 	}
@@ -456,17 +361,7 @@ int main(int argc, const char **argv)
 	save_commit_buffer = verbose_header;
 	track_object_refs = 0;
 
-	if (!merge_order) {
-		show_commit_list(&revs);
-	} else {
-#ifndef NO_OPENSSL
-		if (sort_list_in_merge_order(list, &process_commit)) {
-			die("merge order sort failed\n");
-		}
-#else
-		die("merge order sort unsupported, OpenSSL not linked");
-#endif
-	}
+	show_commit_list(&revs);
 
 	return 0;
 }
diff --git a/revision.c b/revision.c
index fb728c1..f98fae9 100644
--- a/revision.c
+++ b/revision.c
@@ -381,6 +381,9 @@ static void limit_list(struct rev_info *
 	struct commit_list *newlist = NULL;
 	struct commit_list **p = &newlist;
 
+	if (revs->paths)
+		diff_tree_setup_paths(revs->paths);
+
 	while (list) {
 		struct commit_list *entry = list;
 		struct commit *commit = list->item;
@@ -436,12 +439,13 @@ static void handle_all(struct rev_info *
  * Parse revision information, filling in the "rev_info" structure,
  * and removing the used arguments from the argument list.
  *
- * Returns the number of arguments left ("new argc").
+ * Returns the number of arguments left that weren't recognized
+ * (which are also moved to the head of the argument list)
  */
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)
 {
 	int i, flags, seen_dashdash;
-	const char **unrecognized = argv+1;
+	const char **unrecognized = argv + 1;
 	int left = 1;
 
 	memset(revs, 0, sizeof(*revs));
@@ -525,6 +529,10 @@ int setup_revisions(int argc, const char
 				revs->remove_empty_trees = 1;
 				continue;
 			}
+			if (!strncmp(arg, "--no-merges", 11)) {
+				revs->no_merges = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--objects")) {
 				revs->tag_objects = 1;
 				revs->tree_objects = 1;
@@ -601,14 +609,11 @@ int setup_revisions(int argc, const char
 	}
 	if (revs->paths)
 		revs->limited = 1;
-	*unrecognized = NULL;
 	return left;
 }
 
 void prepare_revision_walk(struct rev_info *revs)
 {
-	if (revs->paths)
-		diff_tree_setup_paths(revs->paths);
 	sort_by_date(&revs->commits);
 	if (revs->limited)
 		limit_list(revs);
@@ -616,11 +621,67 @@ void prepare_revision_walk(struct rev_in
 		sort_in_topological_order(&revs->commits, revs->lifo);
 }
 
+static int rewrite_one(struct commit **pp)
+{
+	for (;;) {
+		struct commit *p = *pp;
+		if (p->object.flags & (TREECHANGE | UNINTERESTING))
+			return 0;
+		if (!p->parents)
+			return -1;
+		*pp = p->parents->item;
+	}
+}
+
+static void rewrite_parents(struct commit *commit)
+{
+	struct commit_list **pp = &commit->parents;
+	while (*pp) {
+		struct commit_list *parent = *pp;
+		if (rewrite_one(&parent->item) < 0) {
+			*pp = parent->next;
+			continue;
+		}
+		pp = &parent->next;
+	}
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
-	if (!revs->commits)
+	struct commit_list *list = revs->commits;
+	struct commit *commit;
+
+	if (!list)
 		return NULL;
-	return pop_most_recent_commit(&revs->commits, SEEN);
-}
 
+	/* Check the max_count ... */
+	commit = list->item;
+	switch (revs->max_count) {
+	case -1:
+		break;
+	case 0:
+		return NULL;
+	default:
+		revs->max_count--;
+	}
 
+	do {
+		commit = pop_most_recent_commit(&revs->commits, SEEN);
+		if (commit->object.flags & (UNINTERESTING|SHOWN))
+			continue;
+		if (revs->min_age != -1 && (commit->date > revs->min_age))
+			continue;
+		if (revs->max_age != -1 && (commit->date < revs->max_age))
+			return NULL;
+		if (revs->no_merges && commit->parents && commit->parents->next)
+			continue;
+		if (revs->paths && revs->dense) {
+			if (!(commit->object.flags & TREECHANGE))
+				continue;
+			rewrite_parents(commit);
+		}
+		commit->object.flags |= SHOWN;
+		return commit;
+	} while (revs->commits);
+	return NULL;
+}
diff --git a/revision.h b/revision.h
index 0bed3c0..0043c16 100644
--- a/revision.h
+++ b/revision.h
@@ -4,6 +4,7 @@
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)
 #define TREECHANGE	(1u<<2)
+#define SHOWN		(1u<<3)
 
 struct rev_info {
 	/* Starting list */
@@ -16,6 +17,7 @@ struct rev_info {
 
 	/* Traversal flags */
 	unsigned int	dense:1,
+			no_merges:1,
 			remove_empty_trees:1,
 			lifo:1,
 			topo_order:1,

Re: [PATCH 3/3] Tie it all together: "git log"

From: Martin Langhoff <hidden>
Date: 2016-06-15 22:42:20

On 3/1/06, Junio C Hamano [off-list ref] wrote:
I would say we should just rip merge-order out.  Who uses it,
and why does it not work with topo-order, again?
IIRC archimport uses it, but there's no reason why topo-order wouldn't work.

cheers,


martin

[PATCH 1/2] git-log (internal): add approxidate.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:20

Next will be the pretty-print format.

Signed-off-by: Junio C Hamano <redacted>

---
  Linus Torvalds [off-list ref] writes:

  > I didn't add the logic for --before/--after flags, but that should be 
  > pretty trivial, and is independent of this anyway.
  >
  > Perhaps more importantly, I didn't remove the tests that now start 
  > failing, nor did I remove the actual code to do --merge-order ;/

  I've done the janitorial, and have two more on top.  Here is the
  first one.  I'd appreciate comments on the second one.

 revision.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

2eba658eaffdf4c5c9d0767b49e4c27d7281cda6
diff --git a/revision.c b/revision.c
index c84f146..4885871 100644
--- a/revision.c
+++ b/revision.c
@@ -492,6 +492,26 @@ int setup_revisions(int argc, const char
 				revs->limited = 1;
 				continue;
 			}
+			if (!strncmp(arg, "--since=", 8)) {
+				revs->max_age = approxidate(arg + 8);
+				revs->limited = 1;
+				continue;
+			}
+			if (!strncmp(arg, "--after=", 8)) {
+				revs->max_age = approxidate(arg + 8);
+				revs->limited = 1;
+				continue;
+			}
+			if (!strncmp(arg, "--before=", 9)) {
+				revs->min_age = approxidate(arg + 9);
+				revs->limited = 1;
+				continue;
+			}
+			if (!strncmp(arg, "--until=", 8)) {
+				revs->min_age = approxidate(arg + 8);
+				revs->limited = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--all")) {
 				handle_all(revs, flags);
 				continue;
-- 
1.2.3.g9425

[PATCH 2/2] git-log (internal): more options.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:20

This ports the following options from rev-list based git-log
implementation:

 * -<n>, -n<n>, and -n <n>.  I am still wondering if we want
    this natively supported by setup_revisions(), which already
    takes --max-count.  We may want to move them in the next
    round.  Also I am not sure if we can get away with not
    setting revs->limited when we set max-count.  The latest
    rev-list.c and revision.c in this series do not, so I left
    them as they are.

 * --pretty and --pretty=<fmt>.

 * --abbrev=<n> and --no-abbrev.

The previous commit already handles time-based limiters
(--since, --until and friends).  The remaining things that
rev-list based git-log happens to do are not useful in a pure
log-viewing purposes, and not ported:

 * --bisect (obviously).

 * --header.  I am actually in favor of doing the NUL
   terminated record format, but rev-list based one always
   passed --pretty, which defeated this option.  Maybe next
   round.

 * --parents.  I do not think of a reason a log viewer wants
   this.  The flag is primarily for feeding squashed history
   via pipe to downstream tools.

Signed-off-by: Junio C Hamano <redacted>

---

 * comes on top of --since/--until patch which in turn comes on
   top of janitorial "remove merge-order" change.

 git.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 rev-list.c |    5 ++--
 revision.h |    1 +
 3 files changed, 72 insertions(+), 4 deletions(-)

4e365d2558356cd091ebf57f689c477a86822d53
diff --git a/git.c b/git.c
index b0da6b1..bf68dac 100644
--- a/git.c
+++ b/git.c
@@ -256,12 +256,80 @@ static int cmd_log(int argc, char **argv
 	struct rev_info rev;
 	struct commit *commit;
 	char *buf = xmalloc(LOGSIZE);
+	static enum cmit_fmt commit_format = CMIT_FMT_DEFAULT;
+	int abbrev = DEFAULT_ABBREV;
+	int show_parents = 0;
+	const char *commit_prefix = "commit ";
 
 	argc = setup_revisions(argc, argv, &rev, "HEAD");
+	while (1 < argc) {
+		char *arg = argv[1];
+		/* accept -<digit>, like traditilnal "head" */
+		if ((*arg == '-') && isdigit(arg[1])) {
+			rev.max_count = atoi(arg + 1);
+		}
+		else if (!strcmp(arg, "-n")) {
+			if (argc < 2)
+				die("-n requires an argument");
+			rev.max_count = atoi(argv[2]);
+			argc--; argv++;
+		}
+		else if (!strncmp(arg,"-n",2)) {
+			rev.max_count = atoi(arg + 2);
+		}
+		else if (!strncmp(arg, "--pretty", 8)) {
+			commit_format = get_commit_format(arg + 8);
+			if (commit_format == CMIT_FMT_ONELINE)
+				commit_prefix = "";
+		}
+		else if (!strcmp(arg, "--parents")) {
+			show_parents = 1;
+		}
+		else if (!strcmp(arg, "--no-abbrev")) {
+			abbrev = 0;
+		}
+		else if (!strncmp(arg, "--abbrev=", 9)) {
+			abbrev = strtoul(arg + 9, NULL, 10);
+			if (abbrev && abbrev < MINIMUM_ABBREV)
+				abbrev = MINIMUM_ABBREV;
+			else if (40 < abbrev)
+				abbrev = 40;
+		}
+		else
+			die("unrecognized argument: %s", arg);
+		argc--; argv++;
+	}
+
 	prepare_revision_walk(&rev);
 	setup_pager();
 	while ((commit = get_revision(&rev)) != NULL) {
-		pretty_print_commit(CMIT_FMT_DEFAULT, commit, ~0, buf, LOGSIZE, 18);
+		printf("%s%s", commit_prefix,
+		       sha1_to_hex(commit->object.sha1));
+		if (show_parents) {
+			struct commit_list *parents = commit->parents;
+			while (parents) {
+				struct object *o = &(parents->item->object);
+				parents = parents->next;
+				if (o->flags & TMP_MARK)
+					continue;
+				printf(" %s", sha1_to_hex(o->sha1));
+				o->flags |= TMP_MARK;
+			}
+			/* TMP_MARK is a general purpose flag that can
+			 * be used locally, but the user should clean
+			 * things up after it is done with them.
+			 */
+			for (parents = commit->parents;
+			     parents;
+			     parents = parents->next)
+				parents->item->object.flags &= ~TMP_MARK;
+		}
+		if (commit_format == CMIT_FMT_ONELINE)
+			putchar(' ');
+		else
+			putchar('\n');
+		pretty_print_commit(commit_format, commit, ~0, buf,
+				    LOGSIZE, abbrev);
 		printf("%s\n", buf);
 	}
 	free(buf);
diff --git a/rev-list.c b/rev-list.c
index 6af8d86..8e4d83e 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -7,10 +7,9 @@
 #include "diff.h"
 #include "revision.h"
 
-/* bits #0-3 in revision.h */
+/* bits #0-4 in revision.h */
 
-#define COUNTED		(1u << 4)
-#define TMP_MARK	(1u << 5) /* for isolated cases; clean after use */
+#define COUNTED		(1u<<5)
 
 static const char rev_list_usage[] =
 "git-rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
diff --git a/revision.h b/revision.h
index 0043c16..31e8f61 100644
--- a/revision.h
+++ b/revision.h
@@ -5,6 +5,7 @@
 #define UNINTERESTING   (1u<<1)
 #define TREECHANGE	(1u<<2)
 #define SHOWN		(1u<<3)
+#define TMP_MARK	(1u<<4) /* for isolated cases; clean after use */
 
 struct rev_info {
 	/* Starting list */
-- 
1.2.3.g9425

Re: [PATCH 2/2] git-log (internal): more options.

From: Linus Torvalds <torvalds@osdl.org>
Date: 2016-06-15 22:42:20


On Wed, 1 Mar 2006, Junio C Hamano wrote:
This ports the following options from rev-list based git-log
implementation:

 * -<n>, -n<n>, and -n <n>.  I am still wondering if we want
    this natively supported by setup_revisions(), which already
    takes --max-count.  We may want to move them in the next
    round.  Also I am not sure if we can get away with not
    setting revs->limited when we set max-count.  The latest
    rev-list.c and revision.c in this series do not, so I left
    them as they are.

 * --pretty and --pretty=<fmt>.

 * --abbrev=<n> and --no-abbrev.
Looks good.

I _suspect_ that we want to handle them all in setup_revision(), but I 
wasn't sure, so I left them in rev-list.c originally.

Most helpers that want a list of commits probably want the printing 
options too, and the ones that do not probably simply don't care (ie if 
they silently pass a "--pretty=raw" without it affecting anything, who 
really cares?)
The previous commit already handles time-based limiters
(--since, --until and friends).  The remaining things that
rev-list based git-log happens to do are not useful in a pure
log-viewing purposes, and not ported:

 * --bisect (obviously).

 * --header.  I am actually in favor of doing the NUL
   terminated record format, but rev-list based one always
   passed --pretty, which defeated this option.  Maybe next
   round.

 * --parents.  I do not think of a reason a log viewer wants
   this.  The flag is primarily for feeding squashed history
   via pipe to downstream tools.
I can actually imagine using "--parents" as a way of parsing both the 
commit log and the history. Of course, any such use is likely in a script, 
at which point the script probably doesn't actually want "git log", but 
just a raw "git-rev-list".

After all, the only _real_ difference between "git log" and "git-rev-list" 
is the purely syntactic one (things like defaulting to HEAD in "git log" 
and requiring revisions in git-rev-list), and the use of PAGER.

To me, the question whether a flag would be parsed in the "revision.c" 
library or in the "rev-list.c" binary was more a question of whether that 
flag makes sense for other things than just "git log". 

For example, "git whatchanged" and "git diff" could both use 
setup_revision(), although "git diff" wouldn't actually _walk_ the 
revisions (it would just look at the "revs->commits" list to see what was 
passed in).

"git whatchanged" would obviously take all the same flags "git log" does, 
and "git diff" could take them and just test the values for sanity (ie 
error out if min/max_date is not -1, for example).

"git show" is like a "git-whatchanged" except it wouldn't walk the diffs 
(I considered adding a "--nowalk" option to setup_revisions(), which would 
just suppress the "add_parents_to_list()" entirely)

			Linus

Re: [PATCH 2/2] git-log (internal): more options.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:20

Linus Torvalds [off-list ref] writes:
Most helpers that want a list of commits probably want the printing 
options too, and the ones that do not probably simply don't care (ie if 
they silently pass a "--pretty=raw" without it affecting anything, who 
really cares?)
Perhaps (meaning, agree in general but not 100% convinced and
haven't made up my mind yet).
I can actually imagine using "--parents" as a way of parsing both the 
commit log and the history. Of course, any such use is likely in a script, 
at which point the script probably doesn't actually want "git log", but 
just a raw "git-rev-list".
Yes, that is exactly why I did not see why "log viewer" wants --parents.
To me, the question whether a flag would be parsed in the "revision.c" 
library or in the "rev-list.c" binary was more a question of whether that 
flag makes sense for other things than just "git log". 
Good to know we are in agreement (iow, I wasn't totally off the
mark) that revision.c should handle things that are common.
That means:

 * --bisect and --parents are for scripted use only and do not
   concern log viewer, so we would leave them in rev-list.

 * --header is good for anything that shows more than one
   record, so it may be worthwhile to have it in generic.
For example, "git whatchanged" and "git diff" could both use 
setup_revision(), although "git diff" wouldn't actually _walk_ the 
revisions (it would just look at the "revs->commits" list to see what was 
passed in).

"git whatchanged" would obviously take all the same flags "git log" does, 
and "git diff" could take them and just test the values for sanity (ie 
error out if min/max_date is not -1, for example).
Perhaps.
"git show" is like a "git-whatchanged" except it wouldn't walk the diffs 
(I considered adding a "--nowalk" option to setup_revisions(), which would 
just suppress the "add_parents_to_list()" entirely)
Umm.  The current "git show -4" walks and I find the behaviour
useful.  They are the same program with different defaults.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help