[PATCH 0/3] more terse push output

[PATCH 0/3] more terse push output

[Jeff King]

Here are some patches to match push output to the new fetch output. The
entire output of "git-push" for my test repo now looks like this:

To file:///tmp/parent
 + f3325dc...3b91d1c hasforce -> mirror/hasforce (forced update)
   f3325dc..bb022dc  master -> mirror/master
 ! [rejected]        needsforce -> mirror/needsforce (non-fast forward)
 * [new branch]      newbranch -> mirror/newbranch
 * [new tag]         v1.0 -> v1.0
Counting objects: 5, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.

(where I'm mirroring the heads in refs/heads/mirror/* to show off the name
mapping).

The interesting part is in the first patch; the other two are just
removing clutter on stderr. The patches are based on 'next' to follow
Daniel's send-pack work. I'm happy to hold on to them and rebase once
things settle down in this area.

A few caveats:

  - the output is generated by send-pack, so it's actually "here's what
    I'm about to do" rather than "here's what has happened or is
    happening." receive-pack could generate more accurate results
    (including marking rejections based on hooks), but it has no
    knowledge of the name mapping (so it can't say "foo -> other/foo").
    Right now receive-pack generates 'error: blah blah blah' as it
    always has.

  - the name mapping is perhaps superfluous here, since most of the time
    people are just pushing using the same branch name. But it matches
    the new fetch output, and if you do use a refspec, it's a lot more
    clear.

  - this only covers send-pack, since that is where we do the ref
    matching. Presumably it would be possible to cover pushing over http
    and rsync, but it would have to be totally separate code.

  - no code is shared with the git-fetch implementation (they just
    happen to use the same format). The code is short enough that I
    think it would just end up more confusing to try to factor out the
    commonality.

  - the 'ref is not up-to-date, maybe you need to push' message has gone
    away in favor of the terse '[rejected] ... (non-fast forward)'. I
    know there was some discussion recently of enhancing that message.
    Is this perhaps too terse?

-Peff

[PATCH 1/3] more terse push output

[Jeff King]

This changes the output of send-pack to match the new,
more terse fetch output. It looks like this:

To git://host.tld/path/to/repo
 + f3325dc...3b91d1c hasforce -> mirror/hasforce (forced update)
   f3325dc..bb022dc  master -> mirror/master
 ! [rejected]        needsforce -> mirror/needsforce (non-fast forward)
 * [new branch]      newbranch -> mirror/newbranch
 * [new tag]         v1.0 -> v1.0

instead of:

updating 'refs/heads/mirror/hasforce' using 'refs/heads/hasforce'
  from f3325dca9c4a34d74012c0e159254f454930cec7
  to   3b91d1c310ca9d7b547b85466dd876e143498304
updating 'refs/heads/mirror/master' using 'refs/heads/master'
  from f3325dca9c4a34d74012c0e159254f454930cec7
  to   bb022dc363d5c2aa9aa3026beb9706d44fbe1328
error: remote 'refs/heads/mirror/needsforce' is not an ancestor of
 local  'refs/heads/needsforce'.
 Maybe you are not up-to-date and need to pull first?
updating 'refs/heads/mirror/newbranch' using 'refs/heads/newbranch'
  from 0000000000000000000000000000000000000000
  to   3b91d1c310ca9d7b547b85466dd876e143498304
updating 'refs/tags/v1.0'
  from 0000000000000000000000000000000000000000
  to   bb022dc363d5c2aa9aa3026beb9706d44fbe1328

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |   81 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 947c42b..d74cc3c 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -206,7 +206,18 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	}
 }
 
-static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec, const char **refspec)
+static const char *prettify_ref(const char *name)
+{
+	return name + (
+		!prefixcmp(name, "refs/heads/") ? 11 :
+		!prefixcmp(name, "refs/tags/") ? 10 :
+		!prefixcmp(name, "refs/remotes/") ? 13 :
+		0);
+}
+
+#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+
+static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
 	struct ref *ref;
 	int new_refs;
@@ -214,6 +225,7 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
 	int expect_status_report = 0;
+	int shown_dest = 0;
 
 	/* No funny business with the matcher */
 	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
@@ -245,21 +257,33 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char old_hex[60], *new_hex;
 		int will_delete_ref;
+		const char *pretty_ref;
+		const char *pretty_peer;
 
 		if (!ref->peer_ref)
 			continue;
 
+		if (!shown_dest) {
+			fprintf(stderr, "To %s\n", dest);
+			shown_dest = 1;
+		}
+
+		pretty_ref = prettify_ref(ref->name);
+		pretty_peer = prettify_ref(ref->peer_ref->name);
 
 		will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
 		if (will_delete_ref && !allow_deleting_refs) {
-			error("remote does not support deleting refs");
+			fprintf(stderr, " ! %-*s %s (remote does not support deleting refs)\n",
+					SUMMARY_WIDTH, "[rejected]", pretty_ref);
 			ret = -2;
 			continue;
 		}
 		if (!will_delete_ref &&
 		    !hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (args.verbose)
-				fprintf(stderr, "'%s': up-to-date\n", ref->name);
+				fprintf(stderr, " = %-*s %s -> %s\n",
+					SUMMARY_WIDTH, "[up to date]",
+					pretty_peer, pretty_ref);
 			continue;
 		}
 
@@ -296,12 +320,9 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
 				 * commits at the remote end and likely
 				 * we were not up to date to begin with.
 				 */
-				error("remote '%s' is not an ancestor of\n"
-				      " local  '%s'.\n"
-				      " Maybe you are not up-to-date and "
-				      "need to pull first?",
-				      ref->name,
-				      ref->peer_ref->name);
+				fprintf(stderr, " ! %-*s %s -> %s (non-fast forward)\n",
+						SUMMARY_WIDTH, "[rejected]",
+						pretty_peer, pretty_ref);
 				ret = -2;
 				continue;
 			}
@@ -325,14 +346,40 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
 					old_hex, new_hex, ref->name);
 		}
 		if (will_delete_ref)
-			fprintf(stderr, "deleting '%s'\n", ref->name);
+			fprintf(stderr, " - %-*s %s\n",
+				SUMMARY_WIDTH, "[deleting]",
+				pretty_ref);
+		else if (is_null_sha1(ref->old_sha1)) {
+			const char *msg;
+
+			if (!prefixcmp(ref->name, "refs/tags/"))
+				msg = "[new tag]";
+			else
+				msg = "[new branch]";
+			fprintf(stderr, " * %-*s %s -> %s\n",
+				SUMMARY_WIDTH, msg,
+				pretty_peer, pretty_ref);
+		}
 		else {
-			fprintf(stderr, "updating '%s'", ref->name);
-			if (strcmp(ref->name, ref->peer_ref->name))
-				fprintf(stderr, " using '%s'",
-					ref->peer_ref->name);
-			fprintf(stderr, "\n  from %s\n  to   %s\n",
-				old_hex, new_hex);
+			char quickref[83];
+			char type = ' ';
+			const char *msg = "";
+
+			strcpy(quickref, find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV));
+			if (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))
+				strcat(quickref, "..");
+			else {
+				strcat(quickref, "...");
+				type = '+';
+				msg = " (forced update)";
+			}
+			strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+
+			fprintf(stderr, " %c %-*s %s -> %s%s\n",
+				type,
+				SUMMARY_WIDTH, quickref,
+				pretty_peer, pretty_ref,
+				msg);
 		}
 	}
 
@@ -460,7 +507,7 @@ int send_pack(struct send_pack_args *my_args,
 	verify_remote_names(nr_heads, heads);
 
 	conn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);
-	ret = do_send_pack(fd[0], fd[1], remote, nr_heads, heads);
+	ret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
 	ret |= finish_connect(conn);
-- 
1.5.3.5.1530.g7353

[PATCH 2/3] receive-pack: don't mention successful updates

[Jeff King]

The proposed updates are already shown to the user by
send-pack, so there's no point. We continue to show errors,
since they are unexpected.

Signed-off-by: Jeff King <peff@peff.net>
---
 receive-pack.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/receive-pack.c b/receive-pack.c
index 38e35c0..ed44b89 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -204,8 +204,6 @@ static const char *update(struct command *cmd)
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
-		fprintf(stderr, "%s: %s -> deleted\n", name,
-			sha1_to_hex(old_sha1));
 		return NULL; /* good */
 	}
 	else {
@@ -217,8 +215,6 @@ static const char *update(struct command *cmd)
 		if (write_ref_sha1(lock, new_sha1, "push")) {
 			return "failed to write"; /* error() already called */
 		}
-		fprintf(stderr, "%s: %s -> %s\n", name,
-			sha1_to_hex(old_sha1), sha1_to_hex(new_sha1));
 		return NULL; /* good */
 	}
 }
-- 
1.5.3.5.1530.g7353

[PATCH 3/3] send-pack: require --verbose to show update of tracking refs

[Jeff King]

This is really an uninteresting detail, and it just takes
attention away from the actual push updates and posssible
errors.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d74cc3c..c1fd3f5 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -195,7 +195,8 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		return;
 
 	if (!remote_find_tracking(remote, &rs)) {
-		fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
+		if (args.verbose)
+			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (is_null_sha1(ref->peer_ref->new_sha1)) {
 			if (delete_ref(rs.dst, NULL))
 				error("Failed to delete");
-- 
1.5.3.5.1530.g7353

Re: [PATCH 0/3] more terse push output

[Steffen Prohaska]

On Nov 5, 2007, at 6:05 AM, Jeff King wrote:

>   - the 'ref is not up-to-date, maybe you need to push' message has  
> gone
>     away in favor of the terse '[rejected] ... (non-fast forward)'. I
>     know there was some discussion recently of enhancing that message.
>     Is this perhaps too terse?

I like the general idea of the terse output.

If we want a suggestion to the user, we could put it as
a summary. If a ref was rejected send-pack could print a
concluding message:

---
To file:///tmp/parent
  + f3325dc...3b91d1c hasforce -> mirror/hasforce (forced update)
    f3325dc..bb022dc  master -> mirror/master
  ! [rejected]        needsforce -> mirror/needsforce (non-fast forward)
  * [new branch]      newbranch -> mirror/newbranch
  * [new tag]         v1.0 -> v1.0
Counting objects: 5, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.

warning: some refs were rejected.
  Maybe they are not up-to-date and you want to pull or rebase first?
  Or you may need to force an update?
---

In this way the terse output would not be disrupted and the
suggestion would be given to the user nonetheless.

I propose to use "warning" because it is not a real error. push
updates all other refs as expected. It only rejects some
refs. An error would be appropriate only after push learnt
transactions, that is either completely succeeds or doesn't
update any ref at all.

	Steffen

Re: [PATCH 0/3] more terse push output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   - the 'ref is not up-to-date, maybe you need to push' message has gone
>     away in favor of the terse '[rejected] ... (non-fast forward)'. I
>     know there was some discussion recently of enhancing that message.
>     Is this perhaps too terse?

I kind of like this part.

I also like the part that stops mentioning the "pretend fetching
back" action.  This would mostly parrot the same set of refs for
people who do use the tracking branches anyway.