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

I know it's been a while but.....

quoted

@@ -373,12 +379,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				fputc(url[i], fp);
 		fputc('\n', fp);
 
-		if (ref)
-			rc |= update_local_ref(ref, what, note);
-		else
+		if (ref) {
+			*quickref = 0;
+			rc |= update_local_ref(ref, what, note, quickref);

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

I'm not sure what you mean.  Could you elaborate?

quoted

+	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?

If there's more it will print the "...", if there isn't then it won't.

quoted

+			break;
+		}

quoted

+		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?

So the summary lines are nicely indented with respect to the other output.

quoted

-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.

It can't just be a bit because the "summary" parameter contains number of
summary lines to print.

quoted

+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.

What would be a good way to induce a failed fetch for this test?


     --larry