Re: [PATCH v4] add --summary option to git-push and git-fetch

Larry D'Anna [off-list ref] writes:

--summary will cause git-push to output a one-line of each commit pushed.
--summary=n will display at most n commits for each ref pushed.

$ git push --dry-run --summary origin :
To /home/larry/gitsandbox/a
   80f0e50..5593a38  master -> master
    > 5593a38 foo
    > 81c03f8 bar
Fetch works the same way.

Signed-off-by: Larry D'Anna <redacted>
---
 
 Changes sicne last version of this patch: 

    * added some tests

Thanks.

Makes me wonder why update_local_ref() does not put that NUL upon entry.

+void print_summary_for_push_or_fetch(const char *quickref, int limit)
+{
+	struct rev_info rev;
+	int i, max;
+	struct object *obj;
+	struct commit *commit;
+
+	max = get_max_object_index();
+	for (i = 0; i < max; i++)  {
+		obj = get_indexed_object(i);
+		if (obj)
+			obj->flags &= ~ALL_REV_FLAGS;
+	}

Yuck; this is a horribly heavy sledgehammer.  Couldn't you at least do
clear_commit_marks() to limit the extent of the damage?

+	init_revisions(&rev, NULL);
+	rev.prune = 0;
+	assert(!handle_revision_arg(quickref, &rev, 0, 1));
+	assert(!prepare_revision_walk(&rev));
+
+	while ((commit = get_revision(&rev)) != NULL) {
+		struct strbuf buf = STRBUF_INIT;
+		if (limit == 0) {
+			fprintf(stderr, "    ...\n");

How would you know, when you asked 20 and you showed 20 here, that there
is no more to come?

+			break;
+		}

+		if (!commit->buffer) {
+			enum object_type type;
+			unsigned long size;
+			commit->buffer =
+				read_sha1_file(commit->object.sha1, &type, &size);
+			if (!commit->buffer)
+				die("Cannot read commit %s", sha1_to_hex(commit->object.sha1));
+		}
+		format_commit_message(commit, "    %m %h %s\n", &buf, 0);

Hmm, why so many spaces before %m and after %m?

-static int do_push(const char *repo, int flags)
+static int do_push(const char *repo, int flags, int summary)

Couldn't this be just another bit in the flag?  I didn't check but I
suspect you wouldn't have to touch the intermediate functions in the call
chain that way.

Please; not at the end, but at the front where all the other command
helpers live.  I actually suspect that it might be better to migrate some
part of builtin-log.c, together with your new helper function, to a new
file log.c with accompanying header file log.h, but that could be a
separate patch.

+test_expect_success 'fetch --summary' '
+	mk_empty &&
+	(
+		cd testrepo &&
+		git fetch --summary .. refs/heads/master:refs/remotes/origin/master 2>stderr &&
+
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l) &&
+
+		grep -E  "^    > [a-fA-F0-9]+ second$" stderr &&
+		grep -E  "^    > [a-fA-F0-9]+ repo$" stderr

Look at the output from

    $ git grep -n -e 'grep .*-E' -e 'egrep '

before applying your patch.  I think we support people with grep that does
not know about -E option.

+test_expect_success 'fetch --summary forced update' '
+	mk_empty &&
+	(
...
+	)
+
+'

There are at least two missing combinations. (1) "fetch --summary" to
fetch a new branch, and (2) "fetch --summary" does not try segfaulting by
accessing unavailable information after a failed fetch.

The same comment applies to the push side of the tests.

-static void print_ok_ref_status(struct ref *ref, int porcelain)
+static void print_ok_ref_status(struct ref *ref, int porcelain, int summary)

The same comment on "flags" applies here and the all the functions in the
call chain that adds this extra parameter.  porcelain/summary should be a
single int with two bits used.  It may be cleaner to change "porcelain" to
"a bit inside flag" in a separate patch _before_ this one, as a cleanup of
the previous "add --porcelain option to git-push" patch.