Patches for git-push --confirm and --show-subjects

Patches for git-push --confirm and --show-subjects

[Owen Taylor]

Here's a first try at something like what was discussed. Various notes:

 * I didn't try to implement --confirm for rsync and http pushes; it would
   require completely different code and it sounds like they will eventually
   be switched to the "push_refs" code path as well.

 * I picked the name --show-subjects for the that option because
   --show-commits/--log-commits implied a closer connection to 'git show'
   or 'git log'. --show-subjects implies (to me) something more free-form.

 * --show-subjects might actually benefit from having a short option
   but I omitted that for now.

 * I ripped off a big hunk of code from builtin-fmt-merge-msg.c to do the
   commit synopsis without completely understanding it. There are quite
   a few differences from the original and it was beyond my knowledge of
   the git code base to figure out whether some shared utility could be
   added.

   Along with differences in the input parameters and the output, there's
   one "bug fix" I made to the code - in the orginal, if you have exactly
   21 commits it will show:

     (21 commits):
      <commit 1>
      <commit 2>
      [...]
      <commit 20>
     ...

   So the last commit is pointlessly substituted with '...'; that's more
   annoying if you are showing just a few commits, so I fixed it in the
   adapted code.

 * Passing three booleans 'int verbose, int show_subjects, int porcelain'
   between functions in transport.c is somewhat error-prone, but I didn't
   want to switch to flags, since it would have made the patches here
   less incremental.

 * The interaction between --confirm and --show-subjects and --porcelain
   is a bit tricky, but I think what I ended up with right - the basic
   idea is that that '--confirm --porcelain' should let the user confirm
   then output what actually got done to the wrapper script on stdout in
   porcelain format. Didn't try to describe the details in the docs.

 * My first attempt at changing the git code, so probably some stupidity
   in there somewhere :-)

[PATCH 1/4] push: add --confirm option to ask before sending updates

[Owen Taylor]

From: Owen W. Taylor <otaylor@fishsoup.net>

When --confirm is specified, the refs being updated are displayed
to the user first, and the user is prompted whether to proceed
or not.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
---
 Documentation/git-push.txt |    9 ++++-
 builtin-push.c             |    8 +++--
 builtin-send-pack.c        |    4 ++-
 send-pack.h                |    3 +-
 transport.c                |   87 ++++++++++++++++++++++++++++++++++++++++----
 transport.h                |    3 +-
 6 files changed, 98 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 58d2bd5..c0bbf16 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,8 +9,9 @@ git-push - Update remote refs along with associated objects
 SYNOPSIS
 --------
 [verse]
-'git push' [--all | --mirror | --tags] [--dry-run] [--receive-pack=<git-receive-pack>]
-	   [--repo=<repository>] [-f | --force] [-v | --verbose]
+'git push' [--all | --mirror | --tags] [--dry-run] [--confirm]
+	   [--receive-pack=<git-receive-pack>]  [--repo=<repository>]
+	   [-f | --force] [-v | --verbose]
 	   [<repository> <refspec>...]
 
 DESCRIPTION
@@ -85,6 +86,10 @@ nor in any Push line of the corresponding remotes file---see below).
 --dry-run::
 	Do everything except actually send the updates.
 
+--confirm::
+	Print a summary of what will be done, and then ask the user
+	interactively before actually sending the updates.
+
 --porcelain::
 	Produce machine-readable output.  The output status line for each ref
 	will be tab-separated and sent to stdout instead of stderr.  The full
diff --git a/builtin-push.c b/builtin-push.c
index 6eda372..231be5d 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,7 +10,7 @@
 #include "parse-options.h"
 
 static const char * const push_usage[] = {
-	"git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]",
+	"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]",
 	NULL,
 };
 
@@ -141,6 +141,7 @@ static int do_push(const char *repo, int flags)
 			transport_get(remote, url[i]);
 		int err;
 		int nonfastforward;
+		int disconfirmed;
 		if (receivepack)
 			transport_set_option(transport,
 					     TRANS_OPT_RECEIVEPACK, receivepack);
@@ -150,10 +151,10 @@ static int do_push(const char *repo, int flags)
 		if (flags & TRANSPORT_PUSH_VERBOSE)
 			fprintf(stderr, "Pushing to %s\n", url[i]);
 		err = transport_push(transport, refspec_nr, refspec, flags,
-				     &nonfastforward);
+				     &nonfastforward, &disconfirmed);
 		err |= transport_disconnect(transport);
 
-		if (!err)
+		if (!err || disconfirmed)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
@@ -182,6 +183,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
 		OPT_BOOLEAN( 0 , "tags", &tags, "push tags"),
+		OPT_BIT( 0 , "confirm", &flags, "ask before pushing", TRANSPORT_PUSH_CONFIRM),
 		OPT_BIT( 0 , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 37e528e..0264180 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -406,7 +406,9 @@ int send_pack(struct send_pack_args *args,
 			REF_STATUS_OK;
 	}
 
-	packet_flush(out);
+	/* Don't flush until the second pass of 'git push --confirm' */
+	if (!(args->dry_run && args->confirm))
+		packet_flush(out);
 	if (new_refs && !args->dry_run) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
diff --git a/send-pack.h b/send-pack.h
index 8b3cf02..f2b9292 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -8,7 +8,8 @@ struct send_pack_args {
 		force_update:1,
 		use_thin_pack:1,
 		use_ofs_delta:1,
-		dry_run:1;
+		dry_run:1,
+		confirm:1;
 };
 
 int send_pack(struct send_pack_args *args,
diff --git a/transport.c b/transport.c
index 4cb8077..aa1852d 100644
--- a/transport.c
+++ b/transport.c
@@ -766,14 +766,18 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);
 	args.quiet = !!(flags & TRANSPORT_PUSH_QUIET);
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
+	args.confirm = !!(flags & TRANSPORT_PUSH_CONFIRM);
 
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);
 
-	close(data->fd[1]);
-	close(data->fd[0]);
-	ret |= finish_connect(data->conn);
-	data->conn = NULL;
+	/* On the first dry-run pass of --confirm, we need to leave the connection open */
+	if (!((flags & TRANSPORT_PUSH_CONFIRM) && (flags & TRANSPORT_PUSH_DRY_RUN))) {
+		close(data->fd[1]);
+		close(data->fd[0]);
+		ret |= finish_connect(data->conn);
+		data->conn = NULL;
+	}
 
 	return ret;
 }
@@ -867,14 +871,37 @@ int transport_set_option(struct transport *transport,
 	return 1;
 }
 
+static int prompt_yesno(const char *prompt)
+{
+	while (1) {
+		char buf[128];
+
+		fprintf(stderr, prompt);
+		if (!fgets(buf, sizeof(buf), stdin))
+			return 0;
+		if (buf[0] == 'y' || buf[0] == 'Y')
+			return 1;
+		else if (buf[0] == 'n' || buf[0] == 'N')
+			return 0;
+	}
+}
+
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward)
+		   int * nonfastforward, int * disconfirmed)
 {
+	*disconfirmed = 0;
+
 	verify_remote_names(refspec_nr, refspec);
 
-	if (transport->push)
+	if (transport->push) {
+		if (flags & TRANSPORT_PUSH_CONFIRM) {
+			fprintf(stderr, "--confirm cannot be used with remote URL %s\n", transport->url);
+			return -1;
+		}
+
 		return transport->push(transport, refspec_nr, refspec, flags);
+	}
 	if (transport->push_refs) {
 		struct ref *remote_refs =
 			transport->get_refs_list(transport, 1);
@@ -883,6 +910,8 @@ int transport_push(struct transport *transport,
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
 		int quiet = flags & TRANSPORT_PUSH_QUIET;
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
+		int dry_run = flags & TRANSPORT_PUSH_DRY_RUN;
+		int confirm = flags & TRANSPORT_PUSH_CONFIRM;
 		int ret;
 
 		if (flags & TRANSPORT_PUSH_ALL)
@@ -895,14 +924,56 @@ int transport_push(struct transport *transport,
 			return -1;
 		}
 
+		/* --confirm is a no-op when --dry-run is also specified */
+		if (confirm && (dry_run || !isatty(0))) {
+			confirm = 0;
+			flags &= ~TRANSPORT_PUSH_CONFIRM;
+		}
+
+		if (confirm) {
+			struct ref *ref;
+			int proceed = 0;
+
+			ret = transport->push_refs(transport, remote_refs,
+						   flags | TRANSPORT_PUSH_DRY_RUN);
+
+			/* Interaction with --porcelain: we do the first pass interactively
+			 * to stderr with normal formatting, and then once the user has
+			 * confirmed, send the porcelain-formatted output to stdout.
+			 */
+			print_push_status(transport->url, remote_refs,
+					  verbose, 0,
+					  nonfastforward);
+
+			if (ret)
+				return ret;
+
+			if (!refs_pushed(remote_refs)) {
+				fprintf(stderr, "Everything up-to-date\n");
+				return 0;
+			}
+
+			proceed = prompt_yesno("Proceed [y/n]? ");
+			if (!proceed) {
+				*disconfirmed = 1;
+				return -1;
+			}
+
+			for (ref = remote_refs; ref; ref = ref->next)
+				ref->status = REF_STATUS_NONE;
+		}
+
 		ret = transport->push_refs(transport, remote_refs, flags);
 
-		if (!quiet || push_had_errors(remote_refs))
+		/* For --confirm, we don't need to print the details again unless
+		 * something unexpected happened (denyNonFastforwards=true perhaps)
+		 */
+		if (!(quiet || (confirm && !porcelain)) || push_had_errors(remote_refs))
 			print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
 					nonfastforward);
 
-		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
+		if (!dry_run) {
 			struct ref *ref;
 			for (ref = remote_refs; ref; ref = ref->next)
 				update_tracking_ref(transport->remote, ref, verbose);
diff --git a/transport.h b/transport.h
index c14da6f..1d691d7 100644
--- a/transport.h
+++ b/transport.h
@@ -37,6 +37,7 @@ struct transport {
 #define TRANSPORT_PUSH_VERBOSE 16
 #define TRANSPORT_PUSH_PORCELAIN 32
 #define TRANSPORT_PUSH_QUIET 64
+#define TRANSPORT_PUSH_CONFIRM 128
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
@@ -70,7 +71,7 @@ int transport_set_option(struct transport *transport, const char *name,
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward);
+		   int * nonfastforward, int * disconfirmed);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
-- 
1.6.2.5

[PATCH 2/4] push: allow configuring default for --confirm

[Owen Taylor]

From: Owen W. Taylor <otaylor@fishsoup.net>

A new configuration variable push.confirm sets the default
behavior for whether 'git push' should show prompt the user
interactively before proceeding to update refs.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
---
 Documentation/config.txt |    4 ++++
 builtin-push.c           |    6 +++++-
 cache.h                  |    1 +
 config.c                 |    4 ++++
 environment.c            |    1 +
 5 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index be0b8ca..3bb632f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1303,6 +1303,10 @@ pull.octopus::
 pull.twohead::
 	The default merge strategy to use when pulling a single branch.
 
+push.confirm::
+	If set to true, linkgit:git-push[1] will act as if the --confirm
+	option was passed, unless overriden with --no-confirm.
+
 push.default::
 	Defines the action git push should take if no refspec is given
 	on the command line, no refspec is configured in the remote, and
diff --git a/builtin-push.c b/builtin-push.c
index 231be5d..63a0bb0 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -66,7 +66,6 @@ static void setup_push_tracking(void)
 
 static void setup_default_push_refspecs(void)
 {
-	git_config(git_default_config, NULL);
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_MATCHING:
@@ -193,6 +192,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	git_config(git_default_config, NULL);
+
+	if (push_confirm)
+		flags |= TRANSPORT_PUSH_CONFIRM;
+
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
 	if (tags)
diff --git a/cache.h b/cache.h
index 1a6412d..ef8606d 100644
--- a/cache.h
+++ b/cache.h
@@ -558,6 +558,7 @@ enum push_default_type {
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
+extern int push_confirm;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index c644061..1bc8e6f 100644
--- a/config.c
+++ b/config.c
@@ -575,6 +575,10 @@ static int git_default_branch_config(const char *var, const char *value)
 
 static int git_default_push_config(const char *var, const char *value)
 {
+	if (!strcmp(var, "push.confirm")) {
+		push_confirm = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "push.default")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/environment.c b/environment.c
index 5de6837..e1c82b9 100644
--- a/environment.c
+++ b/environment.c
@@ -44,6 +44,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
+int push_confirm;
 enum push_default_type push_default = PUSH_DEFAULT_MATCHING;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
-- 
1.6.2.5

[PATCH 3/4] push: add --show-subjects option to show commit synopsis

[Owen Taylor]

From: Owen W. Taylor <otaylor@fishsoup.net>

When --show-subjects is specified, include a synopsis of added
and removed with each OK or REJECT_NONFASTFORWARD reference update.

(The code for printing the synposis is borrowed and adapted from
builtin-fmt-merge-msg.c)

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
---
 Documentation/git-push.txt |    4 +
 builtin-push.c             |    3 +-
 transport.c                |  174 +++++++++++++++++++++++++++++++++++++++++---
 transport.h                |    1 +
 4 files changed, 171 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index c0bbf16..c9fd033 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -142,6 +142,10 @@ useful if you write an alias or script around 'git-push'.
 --verbose::
 	Run verbosely.
 
+--show-subjects::
+	When displaying ref updates, include a synopsis of what
+	commits are being added and removed.
+
 include::urls-remotes.txt[]
 
 OUTPUT
diff --git a/builtin-push.c b/builtin-push.c
index 63a0bb0..7c9e394 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,7 +10,7 @@
 #include "parse-options.h"
 
 static const char * const push_usage[] = {
-	"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]",
+	"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [--show-subjects] [<repository> <refspec>...]",
 	NULL,
 };
 
@@ -177,6 +177,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BIT('q', "quiet", &flags, "be quiet", TRANSPORT_PUSH_QUIET),
 		OPT_BIT('v', "verbose", &flags, "be verbose", TRANSPORT_PUSH_VERBOSE),
+		OPT_BIT(0, "show-subjects", &flags, "show commit subjects", TRANSPORT_PUSH_SHOW_SUBJECTS),
 		OPT_STRING( 0 , "repo", &repo, "repository", "repository"),
 		OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
diff --git a/transport.c b/transport.c
index aa1852d..c07291e 100644
--- a/transport.c
+++ b/transport.c
@@ -1,4 +1,6 @@
 #include "cache.h"
+#include "commit.h"
+#include "diff.h"
 #include "transport.h"
 #include "run-command.h"
 #include "pkt-line.h"
@@ -8,6 +10,7 @@
 #include "bundle.h"
 #include "dir.h"
 #include "refs.h"
+#include "revision.h"
 
 /* rsync support */
 
@@ -619,7 +622,151 @@ static const char *status_abbrev(unsigned char sha1[20])
 	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
 }
 
-static void print_ok_ref_status(struct ref *ref, int porcelain)
+struct list {
+	char **list;
+	unsigned nr, alloc;
+};
+
+static void append_to_list(struct list *list, char *value)
+{
+	if (list->nr == list->alloc) {
+		list->alloc += 32;
+		list->list = xrealloc(list->list, sizeof(char *) * list->alloc);
+	}
+	list->list[list->nr++] = value;
+}
+
+static void free_list(struct list *list)
+{
+	int i;
+
+	if (list->alloc == 0)
+		return;
+
+	for (i = 0; i < list->nr; i++) {
+		free(list->list[i]);
+	}
+	free(list->list);
+	list->nr = list->alloc = 0;
+}
+
+static void shortlog(struct commit *from, struct commit *to,
+		     const char *heading, int limit)
+{
+	struct rev_info rev;
+	int i, count = 0;
+	struct commit *commit;
+	struct list subjects = { NULL, 0, 0 };
+	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
+	const char *prefix;
+
+	init_revisions(&rev, NULL);
+	rev.commit_format = CMIT_FMT_ONELINE;
+	rev.limited = 1;
+	rev.ignore_merges = 0;
+
+	setup_revisions(0, NULL, &rev, NULL);
+
+	add_pending_object(&rev, &from->object, "");
+	add_pending_object(&rev, &to->object, "");
+	from->object.flags |= UNINTERESTING;
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+	while ((commit = get_revision(&rev)) != NULL) {
+		char *oneline, *bol, *eol;
+
+		count++;
+		if (subjects.nr > limit)
+			continue;
+
+		bol = strstr(commit->buffer, "\n\n");
+		if (bol) {
+			unsigned char c;
+			do {
+				c = *++bol;
+			} while (isspace(c));
+			if (!c)
+				bol = NULL;
+		}
+
+		if (!bol) {
+			append_to_list(&subjects, xstrdup(sha1_to_hex(commit->object.sha1)));
+			continue;
+		}
+
+		eol = strchr(bol, '\n');
+		if (eol) {
+			oneline = xmemdupz(bol, eol - bol);
+		} else {
+			oneline = xstrdup(bol);
+		}
+		append_to_list(&subjects, oneline);
+	}
+
+	if (heading || count > limit) {
+		fprintf(stderr, "      ");
+		if (heading)
+			fprintf(stderr, "%s", heading);
+		if (heading && count > limit)
+			fprintf(stderr, " (%d)", count);
+		else if (count > limit)
+			fprintf(stderr, "%d commits", count);
+		fprintf(stderr, ":\n");
+		prefix = "         ";
+	} else {
+		prefix = "      ";
+	}
+
+	for (i = 0; i < count && i < limit; i++)
+		if (i == limit - 1 && count > limit)
+			fprintf(stderr, "%s...\n", prefix);
+		else
+			fprintf(stderr, "%s%s\n", prefix, subjects.list[i]);
+
+	clear_commit_marks(from, flags);
+	clear_commit_marks(to, flags);
+	free_commit_list(rev.commits);
+	rev.commits = NULL;
+	rev.pending.nr = 0;
+
+	free_list(&subjects);
+}
+
+/* Maximum lines number of subjects to show (including ...) */
+#define SUBJECTS_LIMIT 8
+
+static void print_subjects(struct ref *ref)
+{
+	struct commit *old;
+	struct commit *new;
+	struct commit_list *merge_bases;
+	int added = 1;
+	int removed = 1;
+
+	old = lookup_commit_reference_gently(ref->old_sha1, 1);
+	if (!old) {
+		fprintf(stderr, "      Unknown changes (please run 'git fetch')\n");
+		return;
+	}
+	new = lookup_commit_reference(ref->new_sha1);
+
+	merge_bases = get_merge_bases(old, new, 1);
+	if (merge_bases && !merge_bases->next && merge_bases->item == old)
+		removed = 0;
+	if (merge_bases && !merge_bases->next && merge_bases->item == new)
+		added = 0;
+
+	if (added && !removed) {
+		shortlog(old, new, NULL, SUBJECTS_LIMIT);
+	} else {
+		if (added)
+			shortlog(old, new, "Added commits", SUBJECTS_LIMIT);
+		if (removed)
+			shortlog(new, old, "Removed commits", SUBJECTS_LIMIT);
+	}
+}
+
+static void print_ok_ref_status(struct ref *ref, int porcelain, int show_subjects)
 {
 	if (ref->deletion)
 		print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
@@ -646,10 +793,13 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
 		strcat(quickref, status_abbrev(ref->new_sha1));
 
 		print_ref_status(type, quickref, ref, ref->peer_ref, msg, porcelain);
+
+		if (show_subjects)
+			print_subjects(ref);
 	}
 }
 
-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
+static int print_one_push_status(struct ref *ref, const char *dest, int count, int show_subjects, int porcelain)
 {
 	if (!count)
 		fprintf(stderr, "To %s\n", dest);
@@ -669,6 +819,8 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast forward", porcelain);
+		if (show_subjects)
+			print_subjects(ref);
 		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
@@ -681,7 +833,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 						 "remote failed to report status", porcelain);
 		break;
 	case REF_STATUS_OK:
-		print_ok_ref_status(ref, porcelain);
+		print_ok_ref_status(ref, porcelain, show_subjects);
 		break;
 	}
 
@@ -689,7 +841,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 static void print_push_status(const char *dest, struct ref *refs,
-			      int verbose, int porcelain, int * nonfastforward)
+			      int verbose, int show_subjects, int porcelain, int * nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
@@ -697,19 +849,19 @@ static void print_push_status(const char *dest, struct ref *refs,
 	if (verbose) {
 		for (ref = refs; ref; ref = ref->next)
 			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n, porcelain);
+				n += print_one_push_status(ref, dest, n, show_subjects, porcelain);
 	}
 
 	for (ref = refs; ref; ref = ref->next)
 		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
+			n += print_one_push_status(ref, dest, n, show_subjects, porcelain);
 
 	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
+			n += print_one_push_status(ref, dest, n, show_subjects, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
 			*nonfastforward = 1;
 	}
@@ -912,6 +1064,7 @@ int transport_push(struct transport *transport,
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
 		int dry_run = flags & TRANSPORT_PUSH_DRY_RUN;
 		int confirm = flags & TRANSPORT_PUSH_CONFIRM;
+		int show_subjects = flags & TRANSPORT_PUSH_SHOW_SUBJECTS;
 		int ret;
 
 		if (flags & TRANSPORT_PUSH_ALL)
@@ -942,7 +1095,7 @@ int transport_push(struct transport *transport,
 			 * confirmed, send the porcelain-formatted output to stdout.
 			 */
 			print_push_status(transport->url, remote_refs,
-					  verbose, 0,
+					  verbose, show_subjects, 0,
 					  nonfastforward);
 
 			if (ret)
@@ -970,8 +1123,9 @@ int transport_push(struct transport *transport,
 		 */
 		if (!(quiet || (confirm && !porcelain)) || push_had_errors(remote_refs))
 			print_push_status(transport->url, remote_refs,
-					verbose | porcelain, porcelain,
-					nonfastforward);
+					verbose | porcelain,
+					show_subjects && !confirm && !porcelain,
+					porcelain, nonfastforward);
 
 		if (!dry_run) {
 			struct ref *ref;
diff --git a/transport.h b/transport.h
index 1d691d7..6a002a3 100644
--- a/transport.h
+++ b/transport.h
@@ -38,6 +38,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 32
 #define TRANSPORT_PUSH_QUIET 64
 #define TRANSPORT_PUSH_CONFIRM 128
+#define TRANSPORT_PUSH_SHOW_SUBJECTS 256
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
1.6.2.5

[PATCH 4/4] push: allow configuring default for --show-subjects

[Owen Taylor]

From: Owen W. Taylor <otaylor@fishsoup.net>

A new configuration variable push.show-subjects sets the default
behavior for whether 'git push' should show a commit synopsis with
each updated ref.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
---
 Documentation/config.txt |    4 ++++
 builtin-push.c           |    2 ++
 cache.h                  |    1 +
 config.c                 |    4 ++++
 environment.c            |    1 +
 5 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3bb632f..83bc5a5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1320,6 +1320,10 @@ push.default::
 * `tracking` push the current branch to its upstream branch.
 * `current` push the current branch to a branch of the same name.
 
+push.show-subjects::
+	If set to true, linkgit:git-push[1] will act as if the --show-subjects
+	option was passed, unless overriden with --no-show-subjects.
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/builtin-push.c b/builtin-push.c
index 7c9e394..e3dc579 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -197,6 +197,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 
 	if (push_confirm)
 		flags |= TRANSPORT_PUSH_CONFIRM;
+	if (push_show_subjects)
+		flags |= TRANSPORT_PUSH_SHOW_SUBJECTS;
 
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
diff --git a/cache.h b/cache.h
index ef8606d..9e6a1e6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,7 @@ extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 extern int push_confirm;
+extern int push_show_subjects;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index 1bc8e6f..bc78876 100644
--- a/config.c
+++ b/config.c
@@ -597,6 +597,10 @@ static int git_default_push_config(const char *var, const char *value)
 		}
 		return 0;
 	}
+	if (!strcmp(var, "push.show-subjects")) {
+		push_show_subjects = git_config_bool(var, value);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
diff --git a/environment.c b/environment.c
index e1c82b9..303c54f 100644
--- a/environment.c
+++ b/environment.c
@@ -46,6 +46,7 @@ enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 int push_confirm;
 enum push_default_type push_default = PUSH_DEFAULT_MATCHING;
+int push_show_subjects;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-- 
1.6.2.5

Re: Patches for git-push --confirm and --show-subjects

[Junio C Hamano]

Without reading much of the code, my knee jerk reactions are:

 * This probably can (and from the longer term perspective, should) be
   done inside a pre-push hook that can decline pushing;

 * I do not think it should use two separate push_refs call into transport
   (first with dry-run and second with real).

   Immediately after match_refs() call in transport_push(), you know if
   the push is a non-fast-forward (in which case you do not know what you
   will be losing anyway because you haven't seen what you are missing
   from the other end) or exactly what your fast-forward push will be
   sending, so between that call and the actual transport->push_refs()
   would be the ideal place to call the hook, with a list of "ref old
   new", without running a dry-run.

for a few reasons.

 (1) When push.confirm is set, you do not want to interact with the user
     when the standard input is not a terminal.  But an automated script
     that runs git-push can still use an appropriate pre-push hook to make
     the decision to intervene without human presense.

 (2) As your --show-subjects patch shows, the likes and dislikes of the
     output format for confirmation would be highly personal.  A separate
     hook that is fed list of <ref, old, new> would make it easier to
     customize this to suite people's tastes.

 (3) I do not trust the use of the fmt_merge_message() code in this
     codepath.  That code, like all the major parts of git, relies on
     being able to use the object flag bits for its own purpose, and there
     is a chance that the way transports (present and future) optimizes
     (or may want to optimize in the future) the object transfer by
     implementing clever common ancestry discovery, similar to what is
     done for the fetch-pack side.

     If we force the actual confirmation process out to a separate process
     that runs a hook, I do not have to worry about that, which is a huge
     relief for maintainability of the system.

 (4) The same objects flag bits contamination issue makes me worried about
     your approach of running one transport_push() with dry-run and then
     another without.

Re: Patches for git-push --confirm and --show-subjects

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Without reading much of the code, my knee jerk reactions are:
>
>  * This probably can (and from the longer term perspective, should) be
>    done inside a pre-push hook that can decline pushing;
>
>  * I do not think it should use two separate push_refs call into transport
>    (first with dry-run and second with real).
>
>    Immediately after match_refs() call in transport_push(), you know if
>    the push is a non-fast-forward (in which case you do not know what you
>    will be losing anyway because you haven't seen what you are missing
>    from the other end) or exactly what your fast-forward push will be
>    sending, so between that call and the actual transport->push_refs()
>    would be the ideal place to call the hook, with a list of "ref old
>    new", without running a dry-run.
>
> for a few reasons.
>
>  (1) When push.confirm is set, you do not want to interact with the user
>      when the standard input is not a terminal.  But an automated script
>      that runs git-push can still use an appropriate pre-push hook to make
>      the decision to intervene without human presense.
>
>  (2) As your --show-subjects patch shows, the likes and dislikes of the
>      output format for confirmation would be highly personal.  A separate
>      hook that is fed list of <ref, old, new> would make it easier to
>      customize this to suite people's tastes.
>
>  (3) I do not trust the use of the fmt_merge_message() code in this
>      codepath.  That code, like all the major parts of git, relies on
>      being able to use the object flag bits for its own purpose, and there
>      is a chance that the way transports (present and future) optimizes
>      (or may want to optimize in the future) the object transfer by
>      implementing clever common ancestry discovery, similar to what is
>      done for the fetch-pack side.

Please add ", would be interfered with fmt_merge_message() code
contaminating the object flag bits." at the end of this sentence.

>
>      If we force the actual confirmation process out to a separate process
>      that runs a hook, I do not have to worry about that, which is a huge
>      relief for maintainability of the system.
>
>  (4) The same objects flag bits contamination issue makes me worried about
>      your approach of running one transport_push() with dry-run and then
>      another without.

Re: Patches for git-push --confirm and --show-subjects

[Owen Taylor]

On Sun, 2009-09-13 at 17:47 -0700, Junio C Hamano wrote:
> Without reading much of the code, my knee jerk reactions are:
> 
>  * This probably can (and from the longer term perspective, should) be
>    done inside a pre-push hook that can decline pushing;

Certainly, within the logic of the operation, there's a potential hook.
(Much like pre-receive but on the client side.)

But even if this behavior was in pre-push.sample, it would not meet what
I'm looking for. Which is an easy-to-use behavior you can turn on, no
matter how many different repositories you work in.

Would it work to do it as a helper program? - if "advanced" options are 
found it feeds the list of candidate ref updates through
git-push--helper or something?

>  * I do not think it should use two separate push_refs call into transport
>    (first with dry-run and second with real).
> 
>    Immediately after match_refs() call in transport_push(), you know if
>    the push is a non-fast-forward (in which case you do not know what you
>    will be losing anyway because you haven't seen what you are missing
>    from the other end) or exactly what your fast-forward push will be
>    sending, so between that call and the actual transport->push_refs()
>    would be the ideal place to call the hook, with a list of "ref old
>    new", without running a dry-run.

The reason I had to do two calls to transport->push_refs is not because
it actually pushes the refs twice. It's because the logic for
classifying the refs is in builtin-send-pack.c. When you pass in
args.dry_run=1 you get the classification logic without the network
traffic.

(There's a little messiness about whether it sends the "flush" 0000 or
not that I had to work around, but that's peripheral.)

The way to clean it up is pretty obvious:

 - You add another vfunc to the transport - '->get_capabilities' or
   something - that encapsulates server_supports("delete-refs").

 - You split the classification logic out into a helper function
   (maybe still in builtin-send-pack.c, maybe moved into some other
   file... don't know what's appropriate.)

   After all, if there was another push_refs backend, it shouldn't be
   duplicating the classification logic...

 - You pass pre-classified ref updates to ->push_refs

I don't know how that interacts with other planned changes to this code.

> for a few reasons.
> 
>  (1) When push.confirm is set, you do not want to interact with the user
>      when the standard input is not a terminal.  But an automated script
>      that runs git-push can still use an appropriate pre-push hook to make
>      the decision to intervene without human presense.
> 
>  (2) As your --show-subjects patch shows, the likes and dislikes of the
>      output format for confirmation would be highly personal.  A separate
>      hook that is fed list of <ref, old, new> would make it easier to
>      customize this to suite people's tastes.

The --show-subjects idea is equally useful for --dry-run. And even when
for successful/failed pushes when neither --confirm not --dry-run is
passed.

I'm not that convinced that there's that much scope for configurability
in this area. Clearly there's some arbitrary decisions I made - that
abbreviated hashes wouldn't be useful. That up to 8 commit subjects
should be shown. Etc.

But as yet, there's no data as to whether people would actually want to
make *different* arbitrary decisions.

Adding more configurability (formats, etc.) doesn't really bother me,
though it does seem like coding in advance of need. But what would
bother me is if the feature isn't useful without complex configuration
or installing custom scripts.

>  (3) I do not trust the use of the fmt_merge_message() code in this
>      codepath.  That code, like all the major parts of git, relies on
>      being able to use the object flag bits for its own purpose, and there
>      is a chance that the way transports (present and future) optimizes
>      (or may want to optimize in the future) the object transfer by
>      implementing clever common ancestry discovery, similar to what is
>      done for the fetch-pack side.
> 
>      If we force the actual confirmation process out to a separate process
>      that runs a hook, I do not have to worry about that, which is a huge
>      relief for maintainability of the system.

Well, I guess that's one way to look at the maintainability issues
involved...

I have to take your word as gospel on this ... I don't have a
comprehensive or even a non-comprehensive view of the use of flags.
Certainly almost same code could be put into a git-push--helper binary.

>  (4) The same objects flag bits contamination issue makes me worried about
>      your approach of running one transport_push() with dry-run and then
>      another without.

With only one example of a transport that implements 'push_refs', it's a
little hard to say what a transport *might* do. But as described above,
this is just a code-structure issue.

- Owen

Re: Patches for git-push --confirm and --show-subjects

[Daniel Barkalow]

On Sun, 13 Sep 2009, Owen Taylor wrote:

> On Sun, 2009-09-13 at 17:47 -0700, Junio C Hamano wrote:
> >  * I do not think it should use two separate push_refs call into transport
> >    (first with dry-run and second with real).
> > 
> >    Immediately after match_refs() call in transport_push(), you know if
> >    the push is a non-fast-forward (in which case you do not know what you
> >    will be losing anyway because you haven't seen what you are missing
> >    from the other end) or exactly what your fast-forward push will be
> >    sending, so between that call and the actual transport->push_refs()
> >    would be the ideal place to call the hook, with a list of "ref old
> >    new", without running a dry-run.
> 
> The reason I had to do two calls to transport->push_refs is not because
> it actually pushes the refs twice. It's because the logic for
> classifying the refs is in builtin-send-pack.c. When you pass in
> args.dry_run=1 you get the classification logic without the network
> traffic.

I think the classification logic should move to match_refs(), assuming you 
mean the ref->nonfastforward and ref->deletion stuff. It would probably 
also be worth having a bit for "already up to date". (Note that 
cmd_send_pack() calls match_refs(), so there wouldn't have to be 
duplication between the legacy cmd_send_pack() code path and the 
transport_push() codepath if the code moved into match_refs()).

> (There's a little messiness about whether it sends the "flush" 0000 or
> not that I had to work around, but that's peripheral.)
> 
> The way to clean it up is pretty obvious:
> 
>  - You add another vfunc to the transport - '->get_capabilities' or
>    something - that encapsulates server_supports("delete-refs").

I think it would be better to have a vfunc that takes refs with the 
classification bits set and sets the statuses based on the idea that we're 
not going to lose any races and the remote won't reject our change for 
some reason we don't know about. There's a potentially large and varied 
set of restrictions on what the other side is willing to accept, and I 
think it would be better to put that on the other side of the vfunc, 
rather than having the main transport code know that "delete-refs" means 
that you can delete refs, "nonfastforward" means you can force a 
non-fast-forward, something means you can create files named "CVS", etc.

This is pretty similar to having a "dry run" call first, except that it 
wouldn't end up rechecking the same things on the real run immediately 
following the dry run, because the checking code has moved to a separate 
method.

Of course, this step isn't entirely needed; without it, you just get asked 
"Are you sure?" for changes the local side can tell won't be permitted, in 
addition to for changes the local side can't tell won't be permitted. 
(Like, you're allowed to delete refs in general, but not the one you're 
trying to delete.)

>  - You split the classification logic out into a helper function
>    (maybe still in builtin-send-pack.c, maybe moved into some other
>    file... don't know what's appropriate.)
> 
>    After all, if there was another push_refs backend, it shouldn't be
>    duplicating the classification logic...

I think match_refs should do it.

>  - You pass pre-classified ref updates to ->push_refs
> 
> I don't know how that interacts with other planned changes to this code.

I think this is a good thing to clean up before further changes; any other 
improvements would either duplicate the logic that's in builtin-send-pack 
or be missing it.

> > for a few reasons.
> > 
> >  (1) When push.confirm is set, you do not want to interact with the user
> >      when the standard input is not a terminal.  But an automated script
> >      that runs git-push can still use an appropriate pre-push hook to make
> >      the decision to intervene without human presense.
> > 
> >  (2) As your --show-subjects patch shows, the likes and dislikes of the
> >      output format for confirmation would be highly personal.  A separate
> >      hook that is fed list of <ref, old, new> would make it easier to
> >      customize this to suite people's tastes.
> 
> The --show-subjects idea is equally useful for --dry-run. And even when
> for successful/failed pushes when neither --confirm not --dry-run is
> passed.
> 
> I'm not that convinced that there's that much scope for configurability
> in this area. Clearly there's some arbitrary decisions I made - that
> abbreviated hashes wouldn't be useful. That up to 8 commit subjects
> should be shown. Etc.
> 
> But as yet, there's no data as to whether people would actually want to
> make *different* arbitrary decisions.
> 
> Adding more configurability (formats, etc.) doesn't really bother me,
> though it does seem like coding in advance of need. But what would
> bother me is if the feature isn't useful without complex configuration
> or installing custom scripts.

I think a pre-push hook would be popular; I know I'd like to have a hook 
that makes sure that I signed off anything I'm pushing (when the server 
might check that *somebody* did, but wouldn't know that this push is 
supposed to be me), and I'd like a hook that checks that I've referenced 
an issue in an issue tracker for each commit that I'm pushing (but only 
when I go to push it).

But I think a simple "ask an interactive user" check makes sense to have 
in the same part of the code.

	-Daniel
*This .sig left intentionally blank*

Re: Patches for git-push --confirm and --show-subjects

[Owen Taylor]

On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:
> On Sun, 13 Sep 2009, Owen Taylor wrote:

[...]
> I think the classification logic should move to match_refs(), assuming you 
> mean the ref->nonfastforward and ref->deletion stuff. It would probably 
> also be worth having a bit for "already up to date". (Note that 
> cmd_send_pack() calls match_refs(), so there wouldn't have to be 
> duplication between the legacy cmd_send_pack() code path and the 
> transport_push() codepath if the code moved into match_refs()).
[...]
> >  - You add another vfunc to the transport - '->get_capabilities' or
> >    something - that encapsulates server_supports("delete-refs").
> 
> I think it would be better to have a vfunc that takes refs with the 
> classification bits set and sets the statuses based on the idea that we're 
> not going to lose any races and the remote won't reject our change for 
> some reason we don't know about. There's a potentially large and varied 
> set of restrictions on what the other side is willing to accept, and I 
> think it would be better to put that on the other side of the vfunc, 
> rather than having the main transport code know that "delete-refs" means 
> that you can delete refs, "nonfastforward" means you can force a 
> non-fast-forward, something means you can create files named "CVS", etc.

match_refs seems like a reasonable place to put this logic, but I'm not
sure I completely follow what you are proposing in terms of
transport-specific customization.

match_refs() is called from a couple of places where there is no
'transport' (cmd_send_pack() and http-push.c) so it can't itself call
into the transport code.

Are you thinking of a virtual function that would be a "second pass"
after the main logic done is done by match_refs; so ->check_refs()
virtual function?

How would the 'bit for "already up to date"' differ from
REF_STATUS_UPTODATE. ?

> I think a pre-push hook would be popular; I know I'd like to have a hook 
> that makes sure that I signed off anything I'm pushing (when the server 
> might check that *somebody* did, but wouldn't know that this push is 
> supposed to be me), and I'd like a hook that checks that I've referenced 
> an issue in an issue tracker for each commit that I'm pushing (but only 
> when I go to push it).

If I can figure out the rest of it, I'll look at adding a hook on top as
a sweetener :-)

- Owen

Re: Patches for git-push --confirm and --show-subjects

[Junio C Hamano]

Owen Taylor <otaylor@redhat.com> writes:

> If I can figure out the rest of it, I'll look at adding a hook on top as
> a sweetener :-)

Please don't.

I seriously suggest you start from, and stick to, nothing but a hook.

The pre-push codepath is conceptually very simple --- something needs to
inspect a list of <ref, old, new> and say yes or no.  But what the users
want needs great customizability (e.g. Daniel's sign-off validation
example).  It's the prime example of codepath that should have a hook and
no built-in policy logic.

You have to enable the necessary hook in all your repositories, and if
that bothers you, then *that* can (and should) be solved as a separate
issue by devising a mechanism that can be extended to the other hooks to
solve the same issue once and for all.

E.g. perhaps in $HOME/.gitconfig, you may want to allow

	[hook]
        	prePush = $HOME/.githooks/my-pre-push-hook
                preCommit = $HOME/.githooks/my-pre-commit-hook

Lack of a general mechanism to allow users to say "I want this hook to
apply to all of my repositories" is not an excuse to add tons of complex
code in the codepath.  Just give users the mechanism and leave the policy
logic to them.

Re: Patches for git-push --confirm and --show-subjects

[Owen Taylor]

On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:
> Owen Taylor <otaylor@redhat.com> writes:
> 
> > If I can figure out the rest of it, I'll look at adding a hook on top as
> > a sweetener :-)
> 
> Please don't.
> 
> I seriously suggest you start from, and stick to, nothing but a hook.
> 
> The pre-push codepath is conceptually very simple --- something needs to
> inspect a list of <ref, old, new> and say yes or no.  But what the users
> want needs great customizability (e.g. Daniel's sign-off validation
> example).  It's the prime example of codepath that should have a hook and
> no built-in policy logic.

Let me back up on this a little bit.

Is confirmation a general need?

In the context of the kernel or git personal repository workflows,
probably not. If you push something wrong, and discover it quickly, you
can just push over it and nobody is wiser. But a large fraction of the
projects listed on the front page of git-scm.com are using shared
repositories. And with a shared repository, a messed up push is more of
an issue: there may be notifications sent out over email or IRC, the
repository may be configured with denyFastForward true, people may
quickly pull your accidental push, etc.

It's also a sticky point for first using git. The push syntax and
behavior is a bit cryptic until you are used to it. Is it going to push
all branches or just the one I'm on? Is 'git push --tags' a superset of
'git push'? etc. If the first repository you are pushing to is public
and shared, heavy use of --dry-run at first is certainly advisable. But
repeating with --dry-run and without is pretty awkward.

How would the quality of use be as a hook?

Probably good enough. The broad outlines are achievable anyways. There
are some aspects of my patches that wouldn't be there. A few that come
to mind:

 - The --show-subjects option applied to all displays of push
   references, not just for --confirm.

 - In the case of a successful push when the updates are exactly what
   was confirmed, outputting them again after the push is suppressed.

How would ease of configuration be for a hook?

> E.g. perhaps in $HOME/.gitconfig, you may want to allow
> 
> 	[hook]
>         	prePush = $HOME/.githooks/my-pre-push-hook
>                 preCommit = $HOME/.githooks/my-pre-commit-hook

This is certainly better than having to set it up per-repo, but if I
wanted to tell GNOME contributors how to turn it on, I'd have to provide
a gnome-contributor-git-setup.sh. Even if the hooks were shipped with
git, there's not going to be a cross-distro path to the where they are
installed.

Maybe if a there was a "hook path" that included ~/.githooks and a
system directory? Though:

 git-config --global hook.prePush git-pre-push-confirm

could still overwrite something that they already have configured; it
wouldn't be an "orthogonal tip" that you could find on a web page and
apply blindly.

Providing a gnome-contributor-git-setup.sh is generally an approach of
last resort. I don't think there is anything unique or special about how
we do we do git on gnome.org that makes it different from other
shared-repository workflows. I'd like the knowledge that people get
using Git with GNOME to carry over to other work they do with Git and
vice-versa.

- Owen

Re: Patches for git-push --confirm and --show-subjects

[Junio C Hamano]

Owen Taylor <otaylor@redhat.com> writes:

> On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:
>> Owen Taylor <otaylor@redhat.com> writes:
>> 
>> > If I can figure out the rest of it, I'll look at adding a hook on top as
>> > a sweetener :-)
>> 
>> Please don't.
>> 
>> I seriously suggest you start from, and stick to, nothing but a hook.
>> 
>> The pre-push codepath is conceptually very simple --- something needs to
>> inspect a list of <ref, old, new> and say yes or no.  But what the users
>> want needs great customizability (e.g. Daniel's sign-off validation
>> example).  It's the prime example of codepath that should have a hook and
>> no built-in policy logic.
>
> Let me back up on this a little bit.
>
> Is confirmation a general need?

If you limit it to the confirmation alone, the answer is probably "not
necessarily".  But a mechanism to allow validation logic to be plugged in
probably is.

You might not see a "policy" in your approach, but it makes some troubling
hardcoded policy decisions.  Here are a few examples of what your patch
decides, and makes it harder for other people to build on (rather, "around):

 - We support only interactive validation (confirmation).  If you want to
   have an unattended validation scheme, there is no way to enhance the
   mechanism this patch adds to do so.  You instead need to add yet
   another command line option and hook into the same place as this patch
   touches.

 - We assume "git push" is run from terminal, and the only kind of
   interactive validation we support is via typed confirmation from a line
   terminal "[Y/n]?"  If you want to run "git push" from a GUI frontend
   and have the user interact with a dialog window popped up separately,
   you are also out of luck.

 - We assume it is good enough to have various built-in presentations of
   supporting information while asking for confirmations; there is no way
   for casual end users to customize and enhance it.

I honestly do not want to be a part of "We" in the above bullet points.

I do not object to having a good default presentation and default
interaction (assuming for a while that we limit ourselves only to
"interactive confirmation").  But that is a very different matter from
closing the door for other possibilities, which is essentially what the
approach to use built-in policy logic that is configurable with unbounded
number of future command line options to "git push" is.

> Providing a gnome-contributor-git-setup.sh is generally an approach of
> last resort.

No question about that.  We do not have any complex built-in policy code
that is triggered at post-receive time at all, but many people use the
sample post-receive-email hook we ship unmodified in their repositories,
because the script is written in a highly configurable way.  I do not see
why pre-push has to be any different.

In any case, this topic won't be part of 1.6.5, and we have plenty of time
to prototype and polish it before it goes to the end user.

Re: Patches for git-push --confirm and --show-subjects

[Owen Taylor]

On Mon, 2009-09-14 at 22:50 -0700, Junio C Hamano wrote:

> You might not see a "policy" in your approach, but it makes some troubling
> hardcoded policy decisions.  Here are a few examples of what your patch
> decides, and makes it harder for other people to build on (rather, "around):
> 
>  - We support only interactive validation (confirmation).  If you want to
>    have an unattended validation scheme, there is no way to enhance the
>    mechanism this patch adds to do so.  You instead need to add yet
>    another command line option and hook into the same place as this patch
>    touches.

It seems like the bulk of any patch is going to be creating a clean
position in the code to do confirmation.

>  - We assume "git push" is run from terminal, and the only kind of
>    interactive validation we support is via typed confirmation from a line
>    terminal "[Y/n]?"  If you want to run "git push" from a GUI frontend
>    and have the user interact with a dialog window popped up separately,
>    you are also out of luck.

That's an interesting situation to consider. How do you see a pre-push
hook being used for that?

>  - We assume it is good enough to have various built-in presentations of
>    supporting information while asking for confirmations; there is no way
>    for casual end users to customize and enhance it.

A shell script that duplicates the display logic from transport.c while
interleaving nicely abbreviated bits of log will be on the complex side.
Is forking and modifying such a script going to be approachable for
casual end users?

- Owen

Re: Patches for git-push --confirm and --show-subjects

[Daniel Barkalow]

On Mon, 14 Sep 2009, Owen Taylor wrote:

> On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:
> > On Sun, 13 Sep 2009, Owen Taylor wrote:
> 
> [...]
> > I think the classification logic should move to match_refs(), assuming you 
> > mean the ref->nonfastforward and ref->deletion stuff. It would probably 
> > also be worth having a bit for "already up to date". (Note that 
> > cmd_send_pack() calls match_refs(), so there wouldn't have to be 
> > duplication between the legacy cmd_send_pack() code path and the 
> > transport_push() codepath if the code moved into match_refs()).
> [...]
> > >  - You add another vfunc to the transport - '->get_capabilities' or
> > >    something - that encapsulates server_supports("delete-refs").
> > 
> > I think it would be better to have a vfunc that takes refs with the 
> > classification bits set and sets the statuses based on the idea that we're 
> > not going to lose any races and the remote won't reject our change for 
> > some reason we don't know about. There's a potentially large and varied 
> > set of restrictions on what the other side is willing to accept, and I 
> > think it would be better to put that on the other side of the vfunc, 
> > rather than having the main transport code know that "delete-refs" means 
> > that you can delete refs, "nonfastforward" means you can force a 
> > non-fast-forward, something means you can create files named "CVS", etc.
> 
> match_refs seems like a reasonable place to put this logic, but I'm not
> sure I completely follow what you are proposing in terms of
> transport-specific customization.
> 
> match_refs() is called from a couple of places where there is no
> 'transport' (cmd_send_pack() and http-push.c) so it can't itself call
> into the transport code.
> 
> Are you thinking of a virtual function that would be a "second pass"
> after the main logic done is done by match_refs; so ->check_refs()
> virtual function?

Yes. match_refs() would answer the question of what the change to the ref 
is, while ->check_refs() would determine whether, for this transport, that 
change is possible. transport_push() would call match_refs(), then 
->check_refs() for transport-specific limitations, then local "are you 
sure" checks, then push_refs(). Other places that call match_refs() would 
either call the appropriate implementation of check_refs() 
(e.g., from cmd_send_pack) or just let the change get rejected when it is 
actually attempted.

> How would the 'bit for "already up to date"' differ from
> REF_STATUS_UPTODATE. ?

It would put all of the things that match_refs() generated in the 
collection of 1-bit flags, and leave ->status entirely for the 
transport-specific code to set. Possibly transport_push() should set 
status to REF_STATUS_UPTODATE if the bit is set, and similarly set status 
to REF_STATUS_REJECT_NONFASTFORWARD if nonfastforward and not force.

> > I think a pre-push hook would be popular; I know I'd like to have a hook 
> > that makes sure that I signed off anything I'm pushing (when the server 
> > might check that *somebody* did, but wouldn't know that this push is 
> > supposed to be me), and I'd like a hook that checks that I've referenced 
> > an issue in an issue tracker for each commit that I'm pushing (but only 
> > when I go to push it).
> 
> If I can figure out the rest of it, I'll look at adding a hook on top as
> a sweetener :-)

Sounds like a good plan.

	-Daniel
*This .sig left intentionally blank*