{"thread":{"id":"20937","subject":"Patches for git-push --confirm and --show-subjects","startedAt":"2009-09-13T23:31:21Z","lastAt":"2009-09-15T11:50:32Z","messageCount":15,"participants":["Owen Taylor","Junio C Hamano","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"123121","messageId":"1252884685-9169-1-git-send-email-otaylor@redhat.com","threadId":"20937","inReplyTo":null,"subject":"Patches for git-push --confirm and --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-13T23:31:21Z","receivedAt":"2009-09-13T23:31:21Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"Here's a first try at something like what was discussed. Various notes:\n\n * I didn't try to implement --confirm for rsync and http pushes; it would\n   require completely different code and it sounds like they will eventually\n   be switched to the \"push_refs\" code path as well.\n\n * I picked the name --show-subjects for the that option because\n   --show-commits/--log-commits implied a closer connection to 'git show'\n   or 'git log'. --show-subjects implies (to me) something more free-form.\n\n * --show-subjects might actually benefit from having a short option\n   but I omitted that for now.\n\n * I ripped off a big hunk of code from builtin-fmt-merge-msg.c to do the\n   commit synopsis without completely understanding it. There are quite\n   a few differences from the original and it was beyond my knowledge of\n   the git code base to figure out whether some shared utility could be\n   added.\n\n   Along with differences in the input parameters and the output, there's\n   one \"bug fix\" I made to the code - in the orginal, if you have exactly\n   21 commits it will show:\n\n     (21 commits):\n      <commit 1>\n      <commit 2>\n      [...]\n      <commit 20>\n     ...\n\n   So the last commit is pointlessly substituted with '...'; that's more\n   annoying if you are showing just a few commits, so I fixed it in the\n   adapted code.\n\n * Passing three booleans 'int verbose, int show_subjects, int porcelain'\n   between functions in transport.c is somewhat error-prone, but I didn't\n   want to switch to flags, since it would have made the patches here\n   less incremental.\n\n * The interaction between --confirm and --show-subjects and --porcelain\n   is a bit tricky, but I think what I ended up with right - the basic\n   idea is that that '--confirm --porcelain' should let the user confirm\n   then output what actually got done to the wrapper script on stdout in\n   porcelain format. Didn't try to describe the details in the docs.\n\n * My first attempt at changing the git code, so probably some stupidity\n   in there somewhere :-)\n"},{"id":"123124","messageId":"1252884685-9169-2-git-send-email-otaylor@redhat.com","threadId":"20937","inReplyTo":"1252884685-9169-1-git-send-email-otaylor@redhat.com","subject":"[PATCH 1/4] push: add --confirm option to ask before sending updates","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-13T23:31:22Z","receivedAt":"2009-09-13T23:31:22Z","isPatch":true,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"From: Owen W. Taylor <otaylor@fishsoup.net>\n\nWhen --confirm is specified, the refs being updated are displayed\nto the user first, and the user is prompted whether to proceed\nor not.\n\nSigned-off-by: Owen W. Taylor <otaylor@fishsoup.net>\n---\n Documentation/git-push.txt |    9 ++++-\n builtin-push.c             |    8 +++--\n builtin-send-pack.c        |    4 ++-\n send-pack.h                |    3 +-\n transport.c                |   87 ++++++++++++++++++++++++++++++++++++++++----\n transport.h                |    3 +-\n 6 files changed, 98 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 58d2bd5..c0bbf16 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -9,8 +9,9 @@ git-push - Update remote refs along with associated objects\n SYNOPSIS\n --------\n [verse]\n-'git push' [--all | --mirror | --tags] [--dry-run] [--receive-pack=<git-receive-pack>]\n-\t   [--repo=<repository>] [-f | --force] [-v | --verbose]\n+'git push' [--all | --mirror | --tags] [--dry-run] [--confirm]\n+\t   [--receive-pack=<git-receive-pack>]  [--repo=<repository>]\n+\t   [-f | --force] [-v | --verbose]\n \t   [<repository> <refspec>...]\n \n DESCRIPTION\n@@ -85,6 +86,10 @@ nor in any Push line of the corresponding remotes file---see below).\n --dry-run::\n \tDo everything except actually send the updates.\n \n+--confirm::\n+\tPrint a summary of what will be done, and then ask the user\n+\tinteractively before actually sending the updates.\n+\n --porcelain::\n \tProduce machine-readable output.  The output status line for each ref\n \twill be tab-separated and sent to stdout instead of stderr.  The full\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 6eda372..231be5d 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -10,7 +10,7 @@\n #include \"parse-options.h\"\n \n static const char * const push_usage[] = {\n-\t\"git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]\",\n+\t\"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]\",\n \tNULL,\n };\n \n@@ -141,6 +141,7 @@ static int do_push(const char *repo, int flags)\n \t\t\ttransport_get(remote, url[i]);\n \t\tint err;\n \t\tint nonfastforward;\n+\t\tint disconfirmed;\n \t\tif (receivepack)\n \t\t\ttransport_set_option(transport,\n \t\t\t\t\t     TRANS_OPT_RECEIVEPACK, receivepack);\n@@ -150,10 +151,10 @@ static int do_push(const char *repo, int flags)\n \t\tif (flags & TRANSPORT_PUSH_VERBOSE)\n \t\t\tfprintf(stderr, \"Pushing to %s\\n\", url[i]);\n \t\terr = transport_push(transport, refspec_nr, refspec, flags,\n-\t\t\t\t     &nonfastforward);\n+\t\t\t\t     &nonfastforward, &disconfirmed);\n \t\terr |= transport_disconnect(transport);\n \n-\t\tif (!err)\n+\t\tif (!err || disconfirmed)\n \t\t\tcontinue;\n \n \t\terror(\"failed to push some refs to '%s'\", url[i]);\n@@ -182,6 +183,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT( 0 , \"mirror\", &flags, \"mirror all refs\",\n \t\t\t    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),\n \t\tOPT_BOOLEAN( 0 , \"tags\", &tags, \"push tags\"),\n+\t\tOPT_BIT( 0 , \"confirm\", &flags, \"ask before pushing\", TRANSPORT_PUSH_CONFIRM),\n \t\tOPT_BIT( 0 , \"dry-run\", &flags, \"dry run\", TRANSPORT_PUSH_DRY_RUN),\n \t\tOPT_BIT( 0,  \"porcelain\", &flags, \"machine-readable output\", TRANSPORT_PUSH_PORCELAIN),\n \t\tOPT_BIT('f', \"force\", &flags, \"force updates\", TRANSPORT_PUSH_FORCE),\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 37e528e..0264180 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -406,7 +406,9 @@ int send_pack(struct send_pack_args *args,\n \t\t\tREF_STATUS_OK;\n \t}\n \n-\tpacket_flush(out);\n+\t/* Don't flush until the second pass of 'git push --confirm' */\n+\tif (!(args->dry_run && args->confirm))\n+\t\tpacket_flush(out);\n \tif (new_refs && !args->dry_run) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\ndiff --git a/send-pack.h b/send-pack.h\nindex 8b3cf02..f2b9292 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -8,7 +8,8 @@ struct send_pack_args {\n \t\tforce_update:1,\n \t\tuse_thin_pack:1,\n \t\tuse_ofs_delta:1,\n-\t\tdry_run:1;\n+\t\tdry_run:1,\n+\t\tconfirm:1;\n };\n \n int send_pack(struct send_pack_args *args,\ndiff --git a/transport.c b/transport.c\nindex 4cb8077..aa1852d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -766,14 +766,18 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);\n \targs.quiet = !!(flags & TRANSPORT_PUSH_QUIET);\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n+\targs.confirm = !!(flags & TRANSPORT_PUSH_CONFIRM);\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t&data->extra_have);\n \n-\tclose(data->fd[1]);\n-\tclose(data->fd[0]);\n-\tret |= finish_connect(data->conn);\n-\tdata->conn = NULL;\n+\t/* On the first dry-run pass of --confirm, we need to leave the connection open */\n+\tif (!((flags & TRANSPORT_PUSH_CONFIRM) && (flags & TRANSPORT_PUSH_DRY_RUN))) {\n+\t\tclose(data->fd[1]);\n+\t\tclose(data->fd[0]);\n+\t\tret |= finish_connect(data->conn);\n+\t\tdata->conn = NULL;\n+\t}\n \n \treturn ret;\n }\n@@ -867,14 +871,37 @@ int transport_set_option(struct transport *transport,\n \treturn 1;\n }\n \n+static int prompt_yesno(const char *prompt)\n+{\n+\twhile (1) {\n+\t\tchar buf[128];\n+\n+\t\tfprintf(stderr, prompt);\n+\t\tif (!fgets(buf, sizeof(buf), stdin))\n+\t\t\treturn 0;\n+\t\tif (buf[0] == 'y' || buf[0] == 'Y')\n+\t\t\treturn 1;\n+\t\telse if (buf[0] == 'n' || buf[0] == 'N')\n+\t\t\treturn 0;\n+\t}\n+}\n+\n int transport_push(struct transport *transport,\n \t\t   int refspec_nr, const char **refspec, int flags,\n-\t\t   int * nonfastforward)\n+\t\t   int * nonfastforward, int * disconfirmed)\n {\n+\t*disconfirmed = 0;\n+\n \tverify_remote_names(refspec_nr, refspec);\n \n-\tif (transport->push)\n+\tif (transport->push) {\n+\t\tif (flags & TRANSPORT_PUSH_CONFIRM) {\n+\t\t\tfprintf(stderr, \"--confirm cannot be used with remote URL %s\\n\", transport->url);\n+\t\t\treturn -1;\n+\t\t}\n+\n \t\treturn transport->push(transport, refspec_nr, refspec, flags);\n+\t}\n \tif (transport->push_refs) {\n \t\tstruct ref *remote_refs =\n \t\t\ttransport->get_refs_list(transport, 1);\n@@ -883,6 +910,8 @@ int transport_push(struct transport *transport,\n \t\tint verbose = flags & TRANSPORT_PUSH_VERBOSE;\n \t\tint quiet = flags & TRANSPORT_PUSH_QUIET;\n \t\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n+\t\tint dry_run = flags & TRANSPORT_PUSH_DRY_RUN;\n+\t\tint confirm = flags & TRANSPORT_PUSH_CONFIRM;\n \t\tint ret;\n \n \t\tif (flags & TRANSPORT_PUSH_ALL)\n@@ -895,14 +924,56 @@ int transport_push(struct transport *transport,\n \t\t\treturn -1;\n \t\t}\n \n+\t\t/* --confirm is a no-op when --dry-run is also specified */\n+\t\tif (confirm && (dry_run || !isatty(0))) {\n+\t\t\tconfirm = 0;\n+\t\t\tflags &= ~TRANSPORT_PUSH_CONFIRM;\n+\t\t}\n+\n+\t\tif (confirm) {\n+\t\t\tstruct ref *ref;\n+\t\t\tint proceed = 0;\n+\n+\t\t\tret = transport->push_refs(transport, remote_refs,\n+\t\t\t\t\t\t   flags | TRANSPORT_PUSH_DRY_RUN);\n+\n+\t\t\t/* Interaction with --porcelain: we do the first pass interactively\n+\t\t\t * to stderr with normal formatting, and then once the user has\n+\t\t\t * confirmed, send the porcelain-formatted output to stdout.\n+\t\t\t */\n+\t\t\tprint_push_status(transport->url, remote_refs,\n+\t\t\t\t\t  verbose, 0,\n+\t\t\t\t\t  nonfastforward);\n+\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\n+\t\t\tif (!refs_pushed(remote_refs)) {\n+\t\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\n+\t\t\tproceed = prompt_yesno(\"Proceed [y/n]? \");\n+\t\t\tif (!proceed) {\n+\t\t\t\t*disconfirmed = 1;\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\n+\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\t\t\tref->status = REF_STATUS_NONE;\n+\t\t}\n+\n \t\tret = transport->push_refs(transport, remote_refs, flags);\n \n-\t\tif (!quiet || push_had_errors(remote_refs))\n+\t\t/* For --confirm, we don't need to print the details again unless\n+\t\t * something unexpected happened (denyNonFastforwards=true perhaps)\n+\t\t */\n+\t\tif (!(quiet || (confirm && !porcelain)) || push_had_errors(remote_refs))\n \t\t\tprint_push_status(transport->url, remote_refs,\n \t\t\t\t\tverbose | porcelain, porcelain,\n \t\t\t\t\tnonfastforward);\n \n-\t\tif (!(flags & TRANSPORT_PUSH_DRY_RUN)) {\n+\t\tif (!dry_run) {\n \t\t\tstruct ref *ref;\n \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n \t\t\t\tupdate_tracking_ref(transport->remote, ref, verbose);\ndiff --git a/transport.h b/transport.h\nindex c14da6f..1d691d7 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -37,6 +37,7 @@ struct transport {\n #define TRANSPORT_PUSH_VERBOSE 16\n #define TRANSPORT_PUSH_PORCELAIN 32\n #define TRANSPORT_PUSH_QUIET 64\n+#define TRANSPORT_PUSH_CONFIRM 128\n \n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n@@ -70,7 +71,7 @@ int transport_set_option(struct transport *transport, const char *name,\n \n int transport_push(struct transport *connection,\n \t\t   int refspec_nr, const char **refspec, int flags,\n-\t\t   int * nonfastforward);\n+\t\t   int * nonfastforward, int * disconfirmed);\n \n const struct ref *transport_get_remote_refs(struct transport *transport);\n \n-- \n1.6.2.5\n"},{"id":"123122","messageId":"1252884685-9169-3-git-send-email-otaylor@redhat.com","threadId":"20937","inReplyTo":"1252884685-9169-2-git-send-email-otaylor@redhat.com","subject":"[PATCH 2/4] push: allow configuring default for --confirm","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-13T23:31:23Z","receivedAt":"2009-09-13T23:31:23Z","isPatch":true,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"From: Owen W. Taylor <otaylor@fishsoup.net>\n\nA new configuration variable push.confirm sets the default\nbehavior for whether 'git push' should show prompt the user\ninteractively before proceeding to update refs.\n\nSigned-off-by: Owen W. Taylor <otaylor@fishsoup.net>\n---\n Documentation/config.txt |    4 ++++\n builtin-push.c           |    6 +++++-\n cache.h                  |    1 +\n config.c                 |    4 ++++\n environment.c            |    1 +\n 5 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex be0b8ca..3bb632f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1303,6 +1303,10 @@ pull.octopus::\n pull.twohead::\n \tThe default merge strategy to use when pulling a single branch.\n \n+push.confirm::\n+\tIf set to true, linkgit:git-push[1] will act as if the --confirm\n+\toption was passed, unless overriden with --no-confirm.\n+\n push.default::\n \tDefines the action git push should take if no refspec is given\n \ton the command line, no refspec is configured in the remote, and\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 231be5d..63a0bb0 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -66,7 +66,6 @@ static void setup_push_tracking(void)\n \n static void setup_default_push_refspecs(void)\n {\n-\tgit_config(git_default_config, NULL);\n \tswitch (push_default) {\n \tdefault:\n \tcase PUSH_DEFAULT_MATCHING:\n@@ -193,6 +192,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tgit_config(git_default_config, NULL);\n+\n+\tif (push_confirm)\n+\t\tflags |= TRANSPORT_PUSH_CONFIRM;\n+\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \n \tif (tags)\ndiff --git a/cache.h b/cache.h\nindex 1a6412d..ef8606d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -558,6 +558,7 @@ enum push_default_type {\n extern enum branch_track git_branch_track;\n extern enum rebase_setup_type autorebase;\n extern enum push_default_type push_default;\n+extern int push_confirm;\n \n enum object_creation_mode {\n \tOBJECT_CREATION_USES_HARDLINKS = 0,\ndiff --git a/config.c b/config.c\nindex c644061..1bc8e6f 100644\n--- a/config.c\n+++ b/config.c\n@@ -575,6 +575,10 @@ static int git_default_branch_config(const char *var, const char *value)\n \n static int git_default_push_config(const char *var, const char *value)\n {\n+\tif (!strcmp(var, \"push.confirm\")) {\n+\t\tpush_confirm = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"push.default\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\ndiff --git a/environment.c b/environment.c\nindex 5de6837..e1c82b9 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -44,6 +44,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n+int push_confirm;\n enum push_default_type push_default = PUSH_DEFAULT_MATCHING;\n #ifndef OBJECT_CREATION_MODE\n #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS\n-- \n1.6.2.5\n"},{"id":"123123","messageId":"1252884685-9169-4-git-send-email-otaylor@redhat.com","threadId":"20937","inReplyTo":"1252884685-9169-3-git-send-email-otaylor@redhat.com","subject":"[PATCH 3/4] push: add --show-subjects option to show commit synopsis","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-13T23:31:24Z","receivedAt":"2009-09-13T23:31:24Z","isPatch":true,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"From: Owen W. Taylor <otaylor@fishsoup.net>\n\nWhen --show-subjects is specified, include a synopsis of added\nand removed with each OK or REJECT_NONFASTFORWARD reference update.\n\n(The code for printing the synposis is borrowed and adapted from\nbuiltin-fmt-merge-msg.c)\n\nSigned-off-by: Owen W. Taylor <otaylor@fishsoup.net>\n---\n Documentation/git-push.txt |    4 +\n builtin-push.c             |    3 +-\n transport.c                |  174 +++++++++++++++++++++++++++++++++++++++++---\n transport.h                |    1 +\n 4 files changed, 171 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex c0bbf16..c9fd033 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -142,6 +142,10 @@ useful if you write an alias or script around 'git-push'.\n --verbose::\n \tRun verbosely.\n \n+--show-subjects::\n+\tWhen displaying ref updates, include a synopsis of what\n+\tcommits are being added and removed.\n+\n include::urls-remotes.txt[]\n \n OUTPUT\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 63a0bb0..7c9e394 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -10,7 +10,7 @@\n #include \"parse-options.h\"\n \n static const char * const push_usage[] = {\n-\t\"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]\",\n+\t\"git push [--all | --mirror] [--dry-run] [--confirm] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [--show-subjects] [<repository> <refspec>...]\",\n \tNULL,\n };\n \n@@ -177,6 +177,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BIT('q', \"quiet\", &flags, \"be quiet\", TRANSPORT_PUSH_QUIET),\n \t\tOPT_BIT('v', \"verbose\", &flags, \"be verbose\", TRANSPORT_PUSH_VERBOSE),\n+\t\tOPT_BIT(0, \"show-subjects\", &flags, \"show commit subjects\", TRANSPORT_PUSH_SHOW_SUBJECTS),\n \t\tOPT_STRING( 0 , \"repo\", &repo, \"repository\", \"repository\"),\n \t\tOPT_BIT( 0 , \"all\", &flags, \"push all refs\", TRANSPORT_PUSH_ALL),\n \t\tOPT_BIT( 0 , \"mirror\", &flags, \"mirror all refs\",\ndiff --git a/transport.c b/transport.c\nindex aa1852d..c07291e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1,4 +1,6 @@\n #include \"cache.h\"\n+#include \"commit.h\"\n+#include \"diff.h\"\n #include \"transport.h\"\n #include \"run-command.h\"\n #include \"pkt-line.h\"\n@@ -8,6 +10,7 @@\n #include \"bundle.h\"\n #include \"dir.h\"\n #include \"refs.h\"\n+#include \"revision.h\"\n \n /* rsync support */\n \n@@ -619,7 +622,151 @@ static const char *status_abbrev(unsigned char sha1[20])\n \treturn find_unique_abbrev(sha1, DEFAULT_ABBREV);\n }\n \n-static void print_ok_ref_status(struct ref *ref, int porcelain)\n+struct list {\n+\tchar **list;\n+\tunsigned nr, alloc;\n+};\n+\n+static void append_to_list(struct list *list, char *value)\n+{\n+\tif (list->nr == list->alloc) {\n+\t\tlist->alloc += 32;\n+\t\tlist->list = xrealloc(list->list, sizeof(char *) * list->alloc);\n+\t}\n+\tlist->list[list->nr++] = value;\n+}\n+\n+static void free_list(struct list *list)\n+{\n+\tint i;\n+\n+\tif (list->alloc == 0)\n+\t\treturn;\n+\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tfree(list->list[i]);\n+\t}\n+\tfree(list->list);\n+\tlist->nr = list->alloc = 0;\n+}\n+\n+static void shortlog(struct commit *from, struct commit *to,\n+\t\t     const char *heading, int limit)\n+{\n+\tstruct rev_info rev;\n+\tint i, count = 0;\n+\tstruct commit *commit;\n+\tstruct list subjects = { NULL, 0, 0 };\n+\tint flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;\n+\tconst char *prefix;\n+\n+\tinit_revisions(&rev, NULL);\n+\trev.commit_format = CMIT_FMT_ONELINE;\n+\trev.limited = 1;\n+\trev.ignore_merges = 0;\n+\n+\tsetup_revisions(0, NULL, &rev, NULL);\n+\n+\tadd_pending_object(&rev, &from->object, \"\");\n+\tadd_pending_object(&rev, &to->object, \"\");\n+\tfrom->object.flags |= UNINTERESTING;\n+\tif (prepare_revision_walk(&rev))\n+\t\tdie(\"revision walk setup failed\");\n+\twhile ((commit = get_revision(&rev)) != NULL) {\n+\t\tchar *oneline, *bol, *eol;\n+\n+\t\tcount++;\n+\t\tif (subjects.nr > limit)\n+\t\t\tcontinue;\n+\n+\t\tbol = strstr(commit->buffer, \"\\n\\n\");\n+\t\tif (bol) {\n+\t\t\tunsigned char c;\n+\t\t\tdo {\n+\t\t\t\tc = *++bol;\n+\t\t\t} while (isspace(c));\n+\t\t\tif (!c)\n+\t\t\t\tbol = NULL;\n+\t\t}\n+\n+\t\tif (!bol) {\n+\t\t\tappend_to_list(&subjects, xstrdup(sha1_to_hex(commit->object.sha1)));\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\teol = strchr(bol, '\\n');\n+\t\tif (eol) {\n+\t\t\toneline = xmemdupz(bol, eol - bol);\n+\t\t} else {\n+\t\t\toneline = xstrdup(bol);\n+\t\t}\n+\t\tappend_to_list(&subjects, oneline);\n+\t}\n+\n+\tif (heading || count > limit) {\n+\t\tfprintf(stderr, \"      \");\n+\t\tif (heading)\n+\t\t\tfprintf(stderr, \"%s\", heading);\n+\t\tif (heading && count > limit)\n+\t\t\tfprintf(stderr, \" (%d)\", count);\n+\t\telse if (count > limit)\n+\t\t\tfprintf(stderr, \"%d commits\", count);\n+\t\tfprintf(stderr, \":\\n\");\n+\t\tprefix = \"         \";\n+\t} else {\n+\t\tprefix = \"      \";\n+\t}\n+\n+\tfor (i = 0; i < count && i < limit; i++)\n+\t\tif (i == limit - 1 && count > limit)\n+\t\t\tfprintf(stderr, \"%s...\\n\", prefix);\n+\t\telse\n+\t\t\tfprintf(stderr, \"%s%s\\n\", prefix, subjects.list[i]);\n+\n+\tclear_commit_marks(from, flags);\n+\tclear_commit_marks(to, flags);\n+\tfree_commit_list(rev.commits);\n+\trev.commits = NULL;\n+\trev.pending.nr = 0;\n+\n+\tfree_list(&subjects);\n+}\n+\n+/* Maximum lines number of subjects to show (including ...) */\n+#define SUBJECTS_LIMIT 8\n+\n+static void print_subjects(struct ref *ref)\n+{\n+\tstruct commit *old;\n+\tstruct commit *new;\n+\tstruct commit_list *merge_bases;\n+\tint added = 1;\n+\tint removed = 1;\n+\n+\told = lookup_commit_reference_gently(ref->old_sha1, 1);\n+\tif (!old) {\n+\t\tfprintf(stderr, \"      Unknown changes (please run 'git fetch')\\n\");\n+\t\treturn;\n+\t}\n+\tnew = lookup_commit_reference(ref->new_sha1);\n+\n+\tmerge_bases = get_merge_bases(old, new, 1);\n+\tif (merge_bases && !merge_bases->next && merge_bases->item == old)\n+\t\tremoved = 0;\n+\tif (merge_bases && !merge_bases->next && merge_bases->item == new)\n+\t\tadded = 0;\n+\n+\tif (added && !removed) {\n+\t\tshortlog(old, new, NULL, SUBJECTS_LIMIT);\n+\t} else {\n+\t\tif (added)\n+\t\t\tshortlog(old, new, \"Added commits\", SUBJECTS_LIMIT);\n+\t\tif (removed)\n+\t\t\tshortlog(new, old, \"Removed commits\", SUBJECTS_LIMIT);\n+\t}\n+}\n+\n+static void print_ok_ref_status(struct ref *ref, int porcelain, int show_subjects)\n {\n \tif (ref->deletion)\n \t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL, porcelain);\n@@ -646,10 +793,13 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n \t\tstrcat(quickref, status_abbrev(ref->new_sha1));\n \n \t\tprint_ref_status(type, quickref, ref, ref->peer_ref, msg, porcelain);\n+\n+\t\tif (show_subjects)\n+\t\t\tprint_subjects(ref);\n \t}\n }\n \n-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)\n+static int print_one_push_status(struct ref *ref, const char *dest, int count, int show_subjects, int porcelain)\n {\n \tif (!count)\n \t\tfprintf(stderr, \"To %s\\n\", dest);\n@@ -669,6 +819,8 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n \t\t\t\t\t\t \"non-fast forward\", porcelain);\n+\t\tif (show_subjects)\n+\t\t\tprint_subjects(ref);\n \t\tbreak;\n \tcase REF_STATUS_REMOTE_REJECT:\n \t\tprint_ref_status('!', \"[remote rejected]\", ref,\n@@ -681,7 +833,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\t\t\t\t\t \"remote failed to report status\", porcelain);\n \t\tbreak;\n \tcase REF_STATUS_OK:\n-\t\tprint_ok_ref_status(ref, porcelain);\n+\t\tprint_ok_ref_status(ref, porcelain, show_subjects);\n \t\tbreak;\n \t}\n \n@@ -689,7 +841,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n }\n \n static void print_push_status(const char *dest, struct ref *refs,\n-\t\t\t      int verbose, int porcelain, int * nonfastforward)\n+\t\t\t      int verbose, int show_subjects, int porcelain, int * nonfastforward)\n {\n \tstruct ref *ref;\n \tint n = 0;\n@@ -697,19 +849,19 @@ static void print_push_status(const char *dest, struct ref *refs,\n \tif (verbose) {\n \t\tfor (ref = refs; ref; ref = ref->next)\n \t\t\tif (ref->status == REF_STATUS_UPTODATE)\n-\t\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\t\tn += print_one_push_status(ref, dest, n, show_subjects, porcelain);\n \t}\n \n \tfor (ref = refs; ref; ref = ref->next)\n \t\tif (ref->status == REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\tn += print_one_push_status(ref, dest, n, show_subjects, porcelain);\n \n \t*nonfastforward = 0;\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (ref->status != REF_STATUS_NONE &&\n \t\t    ref->status != REF_STATUS_UPTODATE &&\n \t\t    ref->status != REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\tn += print_one_push_status(ref, dest, n, show_subjects, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)\n \t\t\t*nonfastforward = 1;\n \t}\n@@ -912,6 +1064,7 @@ int transport_push(struct transport *transport,\n \t\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n \t\tint dry_run = flags & TRANSPORT_PUSH_DRY_RUN;\n \t\tint confirm = flags & TRANSPORT_PUSH_CONFIRM;\n+\t\tint show_subjects = flags & TRANSPORT_PUSH_SHOW_SUBJECTS;\n \t\tint ret;\n \n \t\tif (flags & TRANSPORT_PUSH_ALL)\n@@ -942,7 +1095,7 @@ int transport_push(struct transport *transport,\n \t\t\t * confirmed, send the porcelain-formatted output to stdout.\n \t\t\t */\n \t\t\tprint_push_status(transport->url, remote_refs,\n-\t\t\t\t\t  verbose, 0,\n+\t\t\t\t\t  verbose, show_subjects, 0,\n \t\t\t\t\t  nonfastforward);\n \n \t\t\tif (ret)\n@@ -970,8 +1123,9 @@ int transport_push(struct transport *transport,\n \t\t */\n \t\tif (!(quiet || (confirm && !porcelain)) || push_had_errors(remote_refs))\n \t\t\tprint_push_status(transport->url, remote_refs,\n-\t\t\t\t\tverbose | porcelain, porcelain,\n-\t\t\t\t\tnonfastforward);\n+\t\t\t\t\tverbose | porcelain,\n+\t\t\t\t\tshow_subjects && !confirm && !porcelain,\n+\t\t\t\t\tporcelain, nonfastforward);\n \n \t\tif (!dry_run) {\n \t\t\tstruct ref *ref;\ndiff --git a/transport.h b/transport.h\nindex 1d691d7..6a002a3 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -38,6 +38,7 @@ struct transport {\n #define TRANSPORT_PUSH_PORCELAIN 32\n #define TRANSPORT_PUSH_QUIET 64\n #define TRANSPORT_PUSH_CONFIRM 128\n+#define TRANSPORT_PUSH_SHOW_SUBJECTS 256\n \n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n-- \n1.6.2.5\n"},{"id":"123125","messageId":"1252884685-9169-5-git-send-email-otaylor@redhat.com","threadId":"20937","inReplyTo":"1252884685-9169-4-git-send-email-otaylor@redhat.com","subject":"[PATCH 4/4] push: allow configuring default for --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-13T23:31:25Z","receivedAt":"2009-09-13T23:31:25Z","isPatch":true,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"From: Owen W. Taylor <otaylor@fishsoup.net>\n\nA new configuration variable push.show-subjects sets the default\nbehavior for whether 'git push' should show a commit synopsis with\neach updated ref.\n\nSigned-off-by: Owen W. Taylor <otaylor@fishsoup.net>\n---\n Documentation/config.txt |    4 ++++\n builtin-push.c           |    2 ++\n cache.h                  |    1 +\n config.c                 |    4 ++++\n environment.c            |    1 +\n 5 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3bb632f..83bc5a5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1320,6 +1320,10 @@ push.default::\n * `tracking` push the current branch to its upstream branch.\n * `current` push the current branch to a branch of the same name.\n \n+push.show-subjects::\n+\tIf set to true, linkgit:git-push[1] will act as if the --show-subjects\n+\toption was passed, unless overriden with --no-show-subjects.\n+\n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\n \trebase. False by default.\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 7c9e394..e3dc579 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -197,6 +197,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \n \tif (push_confirm)\n \t\tflags |= TRANSPORT_PUSH_CONFIRM;\n+\tif (push_show_subjects)\n+\t\tflags |= TRANSPORT_PUSH_SHOW_SUBJECTS;\n \n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \ndiff --git a/cache.h b/cache.h\nindex ef8606d..9e6a1e6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -559,6 +559,7 @@ extern enum branch_track git_branch_track;\n extern enum rebase_setup_type autorebase;\n extern enum push_default_type push_default;\n extern int push_confirm;\n+extern int push_show_subjects;\n \n enum object_creation_mode {\n \tOBJECT_CREATION_USES_HARDLINKS = 0,\ndiff --git a/config.c b/config.c\nindex 1bc8e6f..bc78876 100644\n--- a/config.c\n+++ b/config.c\n@@ -597,6 +597,10 @@ static int git_default_push_config(const char *var, const char *value)\n \t\t}\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"push.show-subjects\")) {\n+\t\tpush_show_subjects = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\ndiff --git a/environment.c b/environment.c\nindex e1c82b9..303c54f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -46,6 +46,7 @@ enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n int push_confirm;\n enum push_default_type push_default = PUSH_DEFAULT_MATCHING;\n+int push_show_subjects;\n #ifndef OBJECT_CREATION_MODE\n #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS\n #endif\n-- \n1.6.2.5\n"},{"id":"123128","messageId":"7vpr9ugxn5.fsf@alter.siamese.dyndns.org","threadId":"20937","inReplyTo":"1252884685-9169-1-git-send-email-otaylor@redhat.com","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-14T00:47:42Z","receivedAt":"2009-09-14T00:47:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Without reading much of the code, my knee jerk reactions are:\n\n * This probably can (and from the longer term perspective, should) be\n   done inside a pre-push hook that can decline pushing;\n\n * I do not think it should use two separate push_refs call into transport\n   (first with dry-run and second with real).\n\n   Immediately after match_refs() call in transport_push(), you know if\n   the push is a non-fast-forward (in which case you do not know what you\n   will be losing anyway because you haven't seen what you are missing\n   from the other end) or exactly what your fast-forward push will be\n   sending, so between that call and the actual transport->push_refs()\n   would be the ideal place to call the hook, with a list of \"ref old\n   new\", without running a dry-run.\n\nfor a few reasons.\n\n (1) When push.confirm is set, you do not want to interact with the user\n     when the standard input is not a terminal.  But an automated script\n     that runs git-push can still use an appropriate pre-push hook to make\n     the decision to intervene without human presense.\n\n (2) As your --show-subjects patch shows, the likes and dislikes of the\n     output format for confirmation would be highly personal.  A separate\n     hook that is fed list of <ref, old, new> would make it easier to\n     customize this to suite people's tastes.\n\n (3) I do not trust the use of the fmt_merge_message() code in this\n     codepath.  That code, like all the major parts of git, relies on\n     being able to use the object flag bits for its own purpose, and there\n     is a chance that the way transports (present and future) optimizes\n     (or may want to optimize in the future) the object transfer by\n     implementing clever common ancestry discovery, similar to what is\n     done for the fetch-pack side.\n\n     If we force the actual confirmation process out to a separate process\n     that runs a hook, I do not have to worry about that, which is a huge\n     relief for maintainability of the system.\n\n (4) The same objects flag bits contamination issue makes me worried about\n     your approach of running one transport_push() with dry-run and then\n     another without.\n"},{"id":"123129","messageId":"7vd45ugxe9.fsf@alter.siamese.dyndns.org","threadId":"20937","inReplyTo":"7vpr9ugxn5.fsf@alter.siamese.dyndns.org","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-14T00:53:02Z","receivedAt":"2009-09-14T00:53:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Without reading much of the code, my knee jerk reactions are:\n>\n>  * This probably can (and from the longer term perspective, should) be\n>    done inside a pre-push hook that can decline pushing;\n>\n>  * I do not think it should use two separate push_refs call into transport\n>    (first with dry-run and second with real).\n>\n>    Immediately after match_refs() call in transport_push(), you know if\n>    the push is a non-fast-forward (in which case you do not know what you\n>    will be losing anyway because you haven't seen what you are missing\n>    from the other end) or exactly what your fast-forward push will be\n>    sending, so between that call and the actual transport->push_refs()\n>    would be the ideal place to call the hook, with a list of \"ref old\n>    new\", without running a dry-run.\n>\n> for a few reasons.\n>\n>  (1) When push.confirm is set, you do not want to interact with the user\n>      when the standard input is not a terminal.  But an automated script\n>      that runs git-push can still use an appropriate pre-push hook to make\n>      the decision to intervene without human presense.\n>\n>  (2) As your --show-subjects patch shows, the likes and dislikes of the\n>      output format for confirmation would be highly personal.  A separate\n>      hook that is fed list of <ref, old, new> would make it easier to\n>      customize this to suite people's tastes.\n>\n>  (3) I do not trust the use of the fmt_merge_message() code in this\n>      codepath.  That code, like all the major parts of git, relies on\n>      being able to use the object flag bits for its own purpose, and there\n>      is a chance that the way transports (present and future) optimizes\n>      (or may want to optimize in the future) the object transfer by\n>      implementing clever common ancestry discovery, similar to what is\n>      done for the fetch-pack side.\n\nPlease add \", would be interfered with fmt_merge_message() code\ncontaminating the object flag bits.\" at the end of this sentence.\n\n>\n>      If we force the actual confirmation process out to a separate process\n>      that runs a hook, I do not have to worry about that, which is a huge\n>      relief for maintainability of the system.\n>\n>  (4) The same objects flag bits contamination issue makes me worried about\n>      your approach of running one transport_push() with dry-run and then\n>      another without.\n"},{"id":"123130","messageId":"1252895719.11581.53.camel@localhost.localdomain","threadId":"20937","inReplyTo":"7vpr9ugxn5.fsf@alter.siamese.dyndns.org","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-14T02:35:19Z","receivedAt":"2009-09-14T02:35:19Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"On Sun, 2009-09-13 at 17:47 -0700, Junio C Hamano wrote:\n> Without reading much of the code, my knee jerk reactions are:\n> \n>  * This probably can (and from the longer term perspective, should) be\n>    done inside a pre-push hook that can decline pushing;\n\nCertainly, within the logic of the operation, there's a potential hook.\n(Much like pre-receive but on the client side.)\n\nBut even if this behavior was in pre-push.sample, it would not meet what\nI'm looking for. Which is an easy-to-use behavior you can turn on, no\nmatter how many different repositories you work in.\n\nWould it work to do it as a helper program? - if \"advanced\" options are \nfound it feeds the list of candidate ref updates through\ngit-push--helper or something?\n\n>  * I do not think it should use two separate push_refs call into transport\n>    (first with dry-run and second with real).\n> \n>    Immediately after match_refs() call in transport_push(), you know if\n>    the push is a non-fast-forward (in which case you do not know what you\n>    will be losing anyway because you haven't seen what you are missing\n>    from the other end) or exactly what your fast-forward push will be\n>    sending, so between that call and the actual transport->push_refs()\n>    would be the ideal place to call the hook, with a list of \"ref old\n>    new\", without running a dry-run.\n\nThe reason I had to do two calls to transport->push_refs is not because\nit actually pushes the refs twice. It's because the logic for\nclassifying the refs is in builtin-send-pack.c. When you pass in\nargs.dry_run=1 you get the classification logic without the network\ntraffic.\n\n(There's a little messiness about whether it sends the \"flush\" 0000 or\nnot that I had to work around, but that's peripheral.)\n\nThe way to clean it up is pretty obvious:\n\n - You add another vfunc to the transport - '->get_capabilities' or\n   something - that encapsulates server_supports(\"delete-refs\").\n\n - You split the classification logic out into a helper function\n   (maybe still in builtin-send-pack.c, maybe moved into some other\n   file... don't know what's appropriate.)\n\n   After all, if there was another push_refs backend, it shouldn't be\n   duplicating the classification logic...\n\n - You pass pre-classified ref updates to ->push_refs\n\nI don't know how that interacts with other planned changes to this code.\n\n> for a few reasons.\n> \n>  (1) When push.confirm is set, you do not want to interact with the user\n>      when the standard input is not a terminal.  But an automated script\n>      that runs git-push can still use an appropriate pre-push hook to make\n>      the decision to intervene without human presense.\n> \n>  (2) As your --show-subjects patch shows, the likes and dislikes of the\n>      output format for confirmation would be highly personal.  A separate\n>      hook that is fed list of <ref, old, new> would make it easier to\n>      customize this to suite people's tastes.\n\nThe --show-subjects idea is equally useful for --dry-run. And even when\nfor successful/failed pushes when neither --confirm not --dry-run is\npassed.\n\nI'm not that convinced that there's that much scope for configurability\nin this area. Clearly there's some arbitrary decisions I made - that\nabbreviated hashes wouldn't be useful. That up to 8 commit subjects\nshould be shown. Etc.\n\nBut as yet, there's no data as to whether people would actually want to\nmake *different* arbitrary decisions.\n\nAdding more configurability (formats, etc.) doesn't really bother me,\nthough it does seem like coding in advance of need. But what would\nbother me is if the feature isn't useful without complex configuration\nor installing custom scripts.\n\n>  (3) I do not trust the use of the fmt_merge_message() code in this\n>      codepath.  That code, like all the major parts of git, relies on\n>      being able to use the object flag bits for its own purpose, and there\n>      is a chance that the way transports (present and future) optimizes\n>      (or may want to optimize in the future) the object transfer by\n>      implementing clever common ancestry discovery, similar to what is\n>      done for the fetch-pack side.\n> \n>      If we force the actual confirmation process out to a separate process\n>      that runs a hook, I do not have to worry about that, which is a huge\n>      relief for maintainability of the system.\n\nWell, I guess that's one way to look at the maintainability issues\ninvolved...\n\nI have to take your word as gospel on this ... I don't have a\ncomprehensive or even a non-comprehensive view of the use of flags.\nCertainly almost same code could be put into a git-push--helper binary.\n\n>  (4) The same objects flag bits contamination issue makes me worried about\n>      your approach of running one transport_push() with dry-run and then\n>      another without.\n\nWith only one example of a transport that implements 'push_refs', it's a\nlittle hard to say what a transport *might* do. But as described above,\nthis is just a code-structure issue.\n\n- Owen\n"},{"id":"123194","messageId":"alpine.LNX.2.00.0909141745410.14907@iabervon.org","threadId":"20937","inReplyTo":"1252895719.11581.53.camel@localhost.localdomain","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-09-14T22:21:16Z","receivedAt":"2009-09-14T22:21:16Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 13 Sep 2009, Owen Taylor wrote:\n\n> On Sun, 2009-09-13 at 17:47 -0700, Junio C Hamano wrote:\n> >  * I do not think it should use two separate push_refs call into transport\n> >    (first with dry-run and second with real).\n> > \n> >    Immediately after match_refs() call in transport_push(), you know if\n> >    the push is a non-fast-forward (in which case you do not know what you\n> >    will be losing anyway because you haven't seen what you are missing\n> >    from the other end) or exactly what your fast-forward push will be\n> >    sending, so between that call and the actual transport->push_refs()\n> >    would be the ideal place to call the hook, with a list of \"ref old\n> >    new\", without running a dry-run.\n> \n> The reason I had to do two calls to transport->push_refs is not because\n> it actually pushes the refs twice. It's because the logic for\n> classifying the refs is in builtin-send-pack.c. When you pass in\n> args.dry_run=1 you get the classification logic without the network\n> traffic.\n\nI think the classification logic should move to match_refs(), assuming you \nmean the ref->nonfastforward and ref->deletion stuff. It would probably \nalso be worth having a bit for \"already up to date\". (Note that \ncmd_send_pack() calls match_refs(), so there wouldn't have to be \nduplication between the legacy cmd_send_pack() code path and the \ntransport_push() codepath if the code moved into match_refs()).\n\n> (There's a little messiness about whether it sends the \"flush\" 0000 or\n> not that I had to work around, but that's peripheral.)\n> \n> The way to clean it up is pretty obvious:\n> \n>  - You add another vfunc to the transport - '->get_capabilities' or\n>    something - that encapsulates server_supports(\"delete-refs\").\n\nI think it would be better to have a vfunc that takes refs with the \nclassification bits set and sets the statuses based on the idea that we're \nnot going to lose any races and the remote won't reject our change for \nsome reason we don't know about. There's a potentially large and varied \nset of restrictions on what the other side is willing to accept, and I \nthink it would be better to put that on the other side of the vfunc, \nrather than having the main transport code know that \"delete-refs\" means \nthat you can delete refs, \"nonfastforward\" means you can force a \nnon-fast-forward, something means you can create files named \"CVS\", etc.\n\nThis is pretty similar to having a \"dry run\" call first, except that it \nwouldn't end up rechecking the same things on the real run immediately \nfollowing the dry run, because the checking code has moved to a separate \nmethod.\n\nOf course, this step isn't entirely needed; without it, you just get asked \n\"Are you sure?\" for changes the local side can tell won't be permitted, in \naddition to for changes the local side can't tell won't be permitted. \n(Like, you're allowed to delete refs in general, but not the one you're \ntrying to delete.)\n\n>  - You split the classification logic out into a helper function\n>    (maybe still in builtin-send-pack.c, maybe moved into some other\n>    file... don't know what's appropriate.)\n> \n>    After all, if there was another push_refs backend, it shouldn't be\n>    duplicating the classification logic...\n\nI think match_refs should do it.\n\n>  - You pass pre-classified ref updates to ->push_refs\n> \n> I don't know how that interacts with other planned changes to this code.\n\nI think this is a good thing to clean up before further changes; any other \nimprovements would either duplicate the logic that's in builtin-send-pack \nor be missing it.\n\n> > for a few reasons.\n> > \n> >  (1) When push.confirm is set, you do not want to interact with the user\n> >      when the standard input is not a terminal.  But an automated script\n> >      that runs git-push can still use an appropriate pre-push hook to make\n> >      the decision to intervene without human presense.\n> > \n> >  (2) As your --show-subjects patch shows, the likes and dislikes of the\n> >      output format for confirmation would be highly personal.  A separate\n> >      hook that is fed list of <ref, old, new> would make it easier to\n> >      customize this to suite people's tastes.\n> \n> The --show-subjects idea is equally useful for --dry-run. And even when\n> for successful/failed pushes when neither --confirm not --dry-run is\n> passed.\n> \n> I'm not that convinced that there's that much scope for configurability\n> in this area. Clearly there's some arbitrary decisions I made - that\n> abbreviated hashes wouldn't be useful. That up to 8 commit subjects\n> should be shown. Etc.\n> \n> But as yet, there's no data as to whether people would actually want to\n> make *different* arbitrary decisions.\n> \n> Adding more configurability (formats, etc.) doesn't really bother me,\n> though it does seem like coding in advance of need. But what would\n> bother me is if the feature isn't useful without complex configuration\n> or installing custom scripts.\n\nI think a pre-push hook would be popular; I know I'd like to have a hook \nthat makes sure that I signed off anything I'm pushing (when the server \nmight check that *somebody* did, but wouldn't know that this push is \nsupposed to be me), and I'd like a hook that checks that I've referenced \nan issue in an issue tracker for each commit that I'm pushing (but only \nwhen I go to push it).\n\nBut I think a simple \"ask an interactive user\" check makes sense to have \nin the same part of the code.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"123196","messageId":"1252970294.11581.71.camel@localhost.localdomain","threadId":"20937","inReplyTo":"alpine.LNX.2.00.0909141745410.14907@iabervon.org","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-14T23:18:14Z","receivedAt":"2009-09-14T23:18:14Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:\n> On Sun, 13 Sep 2009, Owen Taylor wrote:\n\n[...]\n> I think the classification logic should move to match_refs(), assuming you \n> mean the ref->nonfastforward and ref->deletion stuff. It would probably \n> also be worth having a bit for \"already up to date\". (Note that \n> cmd_send_pack() calls match_refs(), so there wouldn't have to be \n> duplication between the legacy cmd_send_pack() code path and the \n> transport_push() codepath if the code moved into match_refs()).\n[...]\n> >  - You add another vfunc to the transport - '->get_capabilities' or\n> >    something - that encapsulates server_supports(\"delete-refs\").\n> \n> I think it would be better to have a vfunc that takes refs with the \n> classification bits set and sets the statuses based on the idea that we're \n> not going to lose any races and the remote won't reject our change for \n> some reason we don't know about. There's a potentially large and varied \n> set of restrictions on what the other side is willing to accept, and I \n> think it would be better to put that on the other side of the vfunc, \n> rather than having the main transport code know that \"delete-refs\" means \n> that you can delete refs, \"nonfastforward\" means you can force a \n> non-fast-forward, something means you can create files named \"CVS\", etc.\n\nmatch_refs seems like a reasonable place to put this logic, but I'm not\nsure I completely follow what you are proposing in terms of\ntransport-specific customization.\n\nmatch_refs() is called from a couple of places where there is no\n'transport' (cmd_send_pack() and http-push.c) so it can't itself call\ninto the transport code.\n\nAre you thinking of a virtual function that would be a \"second pass\"\nafter the main logic done is done by match_refs; so ->check_refs()\nvirtual function?\n\nHow would the 'bit for \"already up to date\"' differ from\nREF_STATUS_UPTODATE. ?\n\n> I think a pre-push hook would be popular; I know I'd like to have a hook \n> that makes sure that I signed off anything I'm pushing (when the server \n> might check that *somebody* did, but wouldn't know that this push is \n> supposed to be me), and I'd like a hook that checks that I've referenced \n> an issue in an issue tracker for each commit that I'm pushing (but only \n> when I go to push it).\n\nIf I can figure out the rest of it, I'll look at adding a hook on top as\na sweetener :-)\n\n- Owen\n"},{"id":"123198","messageId":"7v7hw19gr5.fsf@alter.siamese.dyndns.org","threadId":"20937","inReplyTo":"1252970294.11581.71.camel@localhost.localdomain","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-15T00:46:38Z","receivedAt":"2009-09-15T00:46:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Owen Taylor <otaylor@redhat.com> writes:\n\n> If I can figure out the rest of it, I'll look at adding a hook on top as\n> a sweetener :-)\n\nPlease don't.\n\nI seriously suggest you start from, and stick to, nothing but a hook.\n\nThe pre-push codepath is conceptually very simple --- something needs to\ninspect a list of <ref, old, new> and say yes or no.  But what the users\nwant needs great customizability (e.g. Daniel's sign-off validation\nexample).  It's the prime example of codepath that should have a hook and\nno built-in policy logic.\n\nYou have to enable the necessary hook in all your repositories, and if\nthat bothers you, then *that* can (and should) be solved as a separate\nissue by devising a mechanism that can be extended to the other hooks to\nsolve the same issue once and for all.\n\nE.g. perhaps in $HOME/.gitconfig, you may want to allow\n\n\t[hook]\n        \tprePush = $HOME/.githooks/my-pre-push-hook\n                preCommit = $HOME/.githooks/my-pre-commit-hook\n\nLack of a general mechanism to allow users to say \"I want this hook to\napply to all of my repositories\" is not an excuse to add tons of complex\ncode in the codepath.  Just give users the mechanism and leave the policy\nlogic to them.\n"},{"id":"123199","messageId":"alpine.LNX.2.00.0909142042560.14907@iabervon.org","threadId":"20937","inReplyTo":"1252970294.11581.71.camel@localhost.localdomain","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-09-15T00:55:18Z","receivedAt":"2009-09-15T00:55:18Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 14 Sep 2009, Owen Taylor wrote:\n\n> On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:\n> > On Sun, 13 Sep 2009, Owen Taylor wrote:\n> \n> [...]\n> > I think the classification logic should move to match_refs(), assuming you \n> > mean the ref->nonfastforward and ref->deletion stuff. It would probably \n> > also be worth having a bit for \"already up to date\". (Note that \n> > cmd_send_pack() calls match_refs(), so there wouldn't have to be \n> > duplication between the legacy cmd_send_pack() code path and the \n> > transport_push() codepath if the code moved into match_refs()).\n> [...]\n> > >  - You add another vfunc to the transport - '->get_capabilities' or\n> > >    something - that encapsulates server_supports(\"delete-refs\").\n> > \n> > I think it would be better to have a vfunc that takes refs with the \n> > classification bits set and sets the statuses based on the idea that we're \n> > not going to lose any races and the remote won't reject our change for \n> > some reason we don't know about. There's a potentially large and varied \n> > set of restrictions on what the other side is willing to accept, and I \n> > think it would be better to put that on the other side of the vfunc, \n> > rather than having the main transport code know that \"delete-refs\" means \n> > that you can delete refs, \"nonfastforward\" means you can force a \n> > non-fast-forward, something means you can create files named \"CVS\", etc.\n> \n> match_refs seems like a reasonable place to put this logic, but I'm not\n> sure I completely follow what you are proposing in terms of\n> transport-specific customization.\n> \n> match_refs() is called from a couple of places where there is no\n> 'transport' (cmd_send_pack() and http-push.c) so it can't itself call\n> into the transport code.\n> \n> Are you thinking of a virtual function that would be a \"second pass\"\n> after the main logic done is done by match_refs; so ->check_refs()\n> virtual function?\n\nYes. match_refs() would answer the question of what the change to the ref \nis, while ->check_refs() would determine whether, for this transport, that \nchange is possible. transport_push() would call match_refs(), then \n->check_refs() for transport-specific limitations, then local \"are you \nsure\" checks, then push_refs(). Other places that call match_refs() would \neither call the appropriate implementation of check_refs() \n(e.g., from cmd_send_pack) or just let the change get rejected when it is \nactually attempted.\n\n> How would the 'bit for \"already up to date\"' differ from\n> REF_STATUS_UPTODATE. ?\n\nIt would put all of the things that match_refs() generated in the \ncollection of 1-bit flags, and leave ->status entirely for the \ntransport-specific code to set. Possibly transport_push() should set \nstatus to REF_STATUS_UPTODATE if the bit is set, and similarly set status \nto REF_STATUS_REJECT_NONFASTFORWARD if nonfastforward and not force.\n\n> > I think a pre-push hook would be popular; I know I'd like to have a hook \n> > that makes sure that I signed off anything I'm pushing (when the server \n> > might check that *somebody* did, but wouldn't know that this push is \n> > supposed to be me), and I'd like a hook that checks that I've referenced \n> > an issue in an issue tracker for each commit that I'm pushing (but only \n> > when I go to push it).\n> \n> If I can figure out the rest of it, I'll look at adding a hook on top as\n> a sweetener :-)\n\nSounds like a good plan.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"123200","messageId":"1252982329.11581.111.camel@localhost.localdomain","threadId":"20937","inReplyTo":"7v7hw19gr5.fsf@alter.siamese.dyndns.org","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-15T02:38:49Z","receivedAt":"2009-09-15T02:38:49Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:\n> Owen Taylor <otaylor@redhat.com> writes:\n> \n> > If I can figure out the rest of it, I'll look at adding a hook on top as\n> > a sweetener :-)\n> \n> Please don't.\n> \n> I seriously suggest you start from, and stick to, nothing but a hook.\n> \n> The pre-push codepath is conceptually very simple --- something needs to\n> inspect a list of <ref, old, new> and say yes or no.  But what the users\n> want needs great customizability (e.g. Daniel's sign-off validation\n> example).  It's the prime example of codepath that should have a hook and\n> no built-in policy logic.\n\nLet me back up on this a little bit.\n\nIs confirmation a general need?\n\nIn the context of the kernel or git personal repository workflows,\nprobably not. If you push something wrong, and discover it quickly, you\ncan just push over it and nobody is wiser. But a large fraction of the\nprojects listed on the front page of git-scm.com are using shared\nrepositories. And with a shared repository, a messed up push is more of\nan issue: there may be notifications sent out over email or IRC, the\nrepository may be configured with denyFastForward true, people may\nquickly pull your accidental push, etc.\n\nIt's also a sticky point for first using git. The push syntax and\nbehavior is a bit cryptic until you are used to it. Is it going to push\nall branches or just the one I'm on? Is 'git push --tags' a superset of\n'git push'? etc. If the first repository you are pushing to is public\nand shared, heavy use of --dry-run at first is certainly advisable. But\nrepeating with --dry-run and without is pretty awkward.\n\nHow would the quality of use be as a hook?\n\nProbably good enough. The broad outlines are achievable anyways. There\nare some aspects of my patches that wouldn't be there. A few that come\nto mind:\n\n - The --show-subjects option applied to all displays of push\n   references, not just for --confirm.\n\n - In the case of a successful push when the updates are exactly what\n   was confirmed, outputting them again after the push is suppressed.\n\nHow would ease of configuration be for a hook?\n\n> E.g. perhaps in $HOME/.gitconfig, you may want to allow\n> \n> \t[hook]\n>         \tprePush = $HOME/.githooks/my-pre-push-hook\n>                 preCommit = $HOME/.githooks/my-pre-commit-hook\n\nThis is certainly better than having to set it up per-repo, but if I\nwanted to tell GNOME contributors how to turn it on, I'd have to provide\na gnome-contributor-git-setup.sh. Even if the hooks were shipped with\ngit, there's not going to be a cross-distro path to the where they are\ninstalled.\n\nMaybe if a there was a \"hook path\" that included ~/.githooks and a\nsystem directory? Though:\n\n git-config --global hook.prePush git-pre-push-confirm\n\ncould still overwrite something that they already have configured; it\nwouldn't be an \"orthogonal tip\" that you could find on a web page and\napply blindly.\n\nProviding a gnome-contributor-git-setup.sh is generally an approach of\nlast resort. I don't think there is anything unique or special about how\nwe do we do git on gnome.org that makes it different from other\nshared-repository workflows. I'd like the knowledge that people get\nusing Git with GNOME to carry over to other work they do with Git and\nvice-versa.\n\n- Owen\n"},{"id":"123206","messageId":"7v1vm892ow.fsf@alter.siamese.dyndns.org","threadId":"20937","inReplyTo":"1252982329.11581.111.camel@localhost.localdomain","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-15T05:50:23Z","receivedAt":"2009-09-15T05:50:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Owen Taylor <otaylor@redhat.com> writes:\n\n> On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:\n>> Owen Taylor <otaylor@redhat.com> writes:\n>> \n>> > If I can figure out the rest of it, I'll look at adding a hook on top as\n>> > a sweetener :-)\n>> \n>> Please don't.\n>> \n>> I seriously suggest you start from, and stick to, nothing but a hook.\n>> \n>> The pre-push codepath is conceptually very simple --- something needs to\n>> inspect a list of <ref, old, new> and say yes or no.  But what the users\n>> want needs great customizability (e.g. Daniel's sign-off validation\n>> example).  It's the prime example of codepath that should have a hook and\n>> no built-in policy logic.\n>\n> Let me back up on this a little bit.\n>\n> Is confirmation a general need?\n\nIf you limit it to the confirmation alone, the answer is probably \"not\nnecessarily\".  But a mechanism to allow validation logic to be plugged in\nprobably is.\n\nYou might not see a \"policy\" in your approach, but it makes some troubling\nhardcoded policy decisions.  Here are a few examples of what your patch\ndecides, and makes it harder for other people to build on (rather, \"around):\n\n - We support only interactive validation (confirmation).  If you want to\n   have an unattended validation scheme, there is no way to enhance the\n   mechanism this patch adds to do so.  You instead need to add yet\n   another command line option and hook into the same place as this patch\n   touches.\n\n - We assume \"git push\" is run from terminal, and the only kind of\n   interactive validation we support is via typed confirmation from a line\n   terminal \"[Y/n]?\"  If you want to run \"git push\" from a GUI frontend\n   and have the user interact with a dialog window popped up separately,\n   you are also out of luck.\n\n - We assume it is good enough to have various built-in presentations of\n   supporting information while asking for confirmations; there is no way\n   for casual end users to customize and enhance it.\n\nI honestly do not want to be a part of \"We\" in the above bullet points.\n\nI do not object to having a good default presentation and default\ninteraction (assuming for a while that we limit ourselves only to\n\"interactive confirmation\").  But that is a very different matter from\nclosing the door for other possibilities, which is essentially what the\napproach to use built-in policy logic that is configurable with unbounded\nnumber of future command line options to \"git push\" is.\n\n> Providing a gnome-contributor-git-setup.sh is generally an approach of\n> last resort.\n\nNo question about that.  We do not have any complex built-in policy code\nthat is triggered at post-receive time at all, but many people use the\nsample post-receive-email hook we ship unmodified in their repositories,\nbecause the script is written in a highly configurable way.  I do not see\nwhy pre-push has to be any different.\n\nIn any case, this topic won't be part of 1.6.5, and we have plenty of time\nto prototype and polish it before it goes to the end user.\n"},{"id":"123226","messageId":"1253015432.11581.135.camel@localhost.localdomain","threadId":"20937","inReplyTo":"7v1vm892ow.fsf@alter.siamese.dyndns.org","subject":"Re: Patches for git-push --confirm and --show-subjects","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-15T11:50:32Z","receivedAt":"2009-09-15T11:50:32Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"On Mon, 2009-09-14 at 22:50 -0700, Junio C Hamano wrote:\n\n> You might not see a \"policy\" in your approach, but it makes some troubling\n> hardcoded policy decisions.  Here are a few examples of what your patch\n> decides, and makes it harder for other people to build on (rather, \"around):\n> \n>  - We support only interactive validation (confirmation).  If you want to\n>    have an unattended validation scheme, there is no way to enhance the\n>    mechanism this patch adds to do so.  You instead need to add yet\n>    another command line option and hook into the same place as this patch\n>    touches.\n\nIt seems like the bulk of any patch is going to be creating a clean\nposition in the code to do confirmation.\n\n>  - We assume \"git push\" is run from terminal, and the only kind of\n>    interactive validation we support is via typed confirmation from a line\n>    terminal \"[Y/n]?\"  If you want to run \"git push\" from a GUI frontend\n>    and have the user interact with a dialog window popped up separately,\n>    you are also out of luck.\n\nThat's an interesting situation to consider. How do you see a pre-push\nhook being used for that?\n\n>  - We assume it is good enough to have various built-in presentations of\n>    supporting information while asking for confirmations; there is no way\n>    for casual end users to customize and enhance it.\n\nA shell script that duplicates the display logic from transport.c while\ninterleaving nicely abbreviated bits of log will be on the complex side.\nIs forking and modifying such a script going to be approachable for\ncasual end users?\n\n- Owen\n"}]}