{"thread":{"id":"34268","subject":"[PATCH] push: give early feedback","startedAt":"2013-06-24T17:41:02Z","lastAt":"2013-06-24T19:39:24Z","messageCount":8,"participants":["Ramkumar Ramachandra","Fredrik Gustafsson","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221864","messageId":"1372095662-24527-1-git-send-email-artagnon@gmail.com","threadId":"34268","inReplyTo":null,"subject":"[PATCH] push: give early feedback","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T17:41:02Z","receivedAt":"2013-06-24T17:41:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"There are many configuration variables that determine exactly what a\npush does.  Give the user early feedback so that she has a chance to\nabort if she doesn't mean to push those refspecs to that destination\nlike:\n\n  $ git push\n  # pushing refspecs 'master next' to ram (^C to abort)\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n Goes without saying: this is an early preview.\n\n builtin/push.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2d84d10..085d5ab 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)\n {\n \tint i, errs;\n \tstruct remote *remote = pushremote_get(repo);\n+\tstruct strbuf sb = STRBUF_INIT;\n \tconst char **url;\n \tint url_nr;\n \n@@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)\n \t}\n \terrs = 0;\n \turl_nr = push_url_of_remote(remote, &url);\n+\n+\tfor (i = 0; i < refspec_nr; i++)\n+\t\tstrbuf_addf(&sb, \"%s%s\", refspec[i], i == refspec_nr - 1 ? \"\": \" \");\n+\tprintf(\"# pushing refspecs '%s' to %s (^C to abort)\\n\", sb.buf, remote->name);\n+\n \tif (url_nr) {\n \t\tfor (i = 0; i < url_nr; i++) {\n \t\t\tstruct transport *transport =\n-- \n1.8.3.1.549.g1f3a412.dirty\n"},{"id":"221865","messageId":"20130624180419.GC26287@paksenarrion.iveqy.com","threadId":"34268","inReplyTo":"1372095662-24527-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] push: give early feedback","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-06-24T18:04:19Z","receivedAt":"2013-06-24T18:04:19Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:\n> There are many configuration variables that determine exactly what a\n> push does.  Give the user early feedback so that she has a chance to\n> abort if she doesn't mean to push those refspecs to that destination\n> like:\n> \n>   $ git push\n>   # pushing refspecs 'master next' to ram (^C to abort)\n> \n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  Goes without saying: this is an early preview.\n> \n>  builtin/push.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/builtin/push.c b/builtin/push.c\n> index 2d84d10..085d5ab 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)\n>  {\n>  \tint i, errs;\n>  \tstruct remote *remote = pushremote_get(repo);\n> +\tstruct strbuf sb = STRBUF_INIT;\n>  \tconst char **url;\n>  \tint url_nr;\n>  \n> @@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)\n>  \t}\n>  \terrs = 0;\n>  \turl_nr = push_url_of_remote(remote, &url);\n> +\n> +\tfor (i = 0; i < refspec_nr; i++)\n> +\t\tstrbuf_addf(&sb, \"%s%s\", refspec[i], i == refspec_nr - 1 ? \"\": \" \");\n> +\tprintf(\"# pushing refspecs '%s' to %s (^C to abort)\\n\", sb.buf, remote->name);\n> +\n\nHow about a strbuf_release here?\n\nCan you really be sure that refspec_nr is set here?\n\n>  \tif (url_nr) {\n>  \t\tfor (i = 0; i < url_nr; i++) {\n>  \t\t\tstruct transport *transport =\n> -- \n> 1.8.3.1.549.g1f3a412.dirty\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"221871","messageId":"7vzjuf8iz5.fsf@alter.siamese.dyndns.org","threadId":"34268","inReplyTo":"20130624180419.GC26287@paksenarrion.iveqy.com","subject":"Re: [PATCH] push: give early feedback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T18:24:46Z","receivedAt":"2013-06-24T18:24:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:\n>> There are many configuration variables that determine exactly what a\n>> push does.  Give the user early feedback so that she has a chance to\n>> abort if she doesn't mean to push those refspecs to that destination\n>> like:\n>> \n>>   $ git push\n>>   # pushing refspecs 'master next' to ram (^C to abort)\n>> \n>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n>> ---\n>>  Goes without saying: this is an early preview.\n>> \n>>  builtin/push.c | 6 ++++++\n>>  1 file changed, 6 insertions(+)\n>> \n>> diff --git a/builtin/push.c b/builtin/push.c\n>> index 2d84d10..085d5ab 100644\n>> --- a/builtin/push.c\n>> +++ b/builtin/push.c\n>> @@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)\n>>  {\n>>  \tint i, errs;\n>>  \tstruct remote *remote = pushremote_get(repo);\n>> +\tstruct strbuf sb = STRBUF_INIT;\n>>  \tconst char **url;\n>>  \tint url_nr;\n>>  \n>> @@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)\n>>  \t}\n>>  \terrs = 0;\n>>  \turl_nr = push_url_of_remote(remote, &url);\n>> +\n>> +\tfor (i = 0; i < refspec_nr; i++)\n>> +\t\tstrbuf_addf(&sb, \"%s%s\", refspec[i], i == refspec_nr - 1 ? \"\": \" \");\n>> +\tprintf(\"# pushing refspecs '%s' to %s (^C to abort)\\n\", sb.buf, remote->name);\n>> +\n>\n> How about a strbuf_release here?\n>\n> Can you really be sure that refspec_nr is set here?\n\nDoing this unconditionally when the user says \"git push there this\"\nwould be mildly annoying.\n\nI think this belongs to either --verbose or even --debug, for people\nwho are trying to make sure an underspecified \"git push\" (or \"git\npush there\") does what they want, but for those use cases, it\nprobably is better to tie this to --dry-run.\n"},{"id":"221874","messageId":"20130624182809.GA15296@sigill.intra.peff.net","threadId":"34268","inReplyTo":"1372095662-24527-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] push: give early feedback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-24T18:28:09Z","receivedAt":"2013-06-24T18:28:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:\n\n> There are many configuration variables that determine exactly what a\n> push does.  Give the user early feedback so that she has a chance to\n> abort if she doesn't mean to push those refspecs to that destination\n> like:\n> \n>   $ git push\n>   # pushing refspecs 'master next' to ram (^C to abort)\n\nIf your intent is to let people stop disastrous pushes before they\ncomplete, I think there are two failings:\n\n  1. It does not tell very much about how the refspecs are expanded or\n     what is going to happen. \"git push --dry-run\" gives a much more\n     complete view of what will be pushed.\n\n  2. You are creating a race with the user to hit ^C. It will probably\n     work if there are a lot of objects to be pushed. But not if the\n     push is small, or even has no pack at all (e.g., only deletions, or\n     moving refs to existing history). As an added bonus, since push\n     prints its output after receiving commitment from the server, it is\n     possible for the server to commit a change, have the user hit ^C,\n     thinking they beat the race, only to find out that the server did\n     indeed accept the change.\n\nI think you could deal with both of them by doing something like:\n\n  git push --dry-run \"$@\" || exit 1\n  echo >&2 \"OK to push?\"\n  read response\n  case \"$response\" in\n  [Yy]) ;;\n  *) exit 1\n  esac\n  exec git push \"$@\"\n\nThat is subject to a race condition in a busy repo (you do not know that\nthe refs are in the same state on either end between the two pushes).\nYou could solve that by having a \"pre-push\" hook on the local side that\ngets the list of proposed updates.\n\nIn fact, I think this came up before but no hook code ever materialized:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/128273\n\n  http://thread.gmane.org/gmane.comp.version-control.git/128426\n\nAs discussed in the top thread, this could also be used for \"interactive\npush\" where you could examine and confirm the changes for each proposed\nref change.\n\n-Peff\n"},{"id":"221875","messageId":"CALkWK0=d1wkKWngH+6gBd-2svj7r_tgC5=+zUbgJRDfUCzupSw@mail.gmail.com","threadId":"34268","inReplyTo":"20130624182809.GA15296@sigill.intra.peff.net","subject":"Re: [PATCH] push: give early feedback","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T18:42:16Z","receivedAt":"2013-06-24T18:42:16Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> If your intent is to let people stop disastrous pushes before they\n> complete, I think there are two failings:\n>\n>   1. It does not tell very much about how the refspecs are expanded or\n>      what is going to happen. \"git push --dry-run\" gives a much more\n>      complete view of what will be pushed.\n\nYes.\n\n  $ git push\n  # pushing refspecs ':' to ram\n\nCompletely useless.\n\nOn the other hand, if I implement it at the transport layer like\nTRANSPORT_PUSH_DRY_RUN, it takes *way* too long to say anything\nuseful; the whole \"early\" part has been thrown out the window.  The\nissue is again related to the same nightmare I'm having: not being\nable to implement @{push} refspec because the transport API is so\ntangled up; I can't call into the refspec-pattern-expander from\nanywhere else.\n\n>   2. You are creating a race with the user to hit ^C. It will probably\n>      work if there are a lot of objects to be pushed. But not if the\n>      push is small, or even has no pack at all (e.g., only deletions, or\n>      moving refs to existing history). As an added bonus, since push\n>      prints its output after receiving commitment from the server, it is\n>      possible for the server to commit a change, have the user hit ^C,\n>      thinking they beat the race, only to find out that the server did\n>      indeed accept the change.\n\nYes, ^C is a hack, but it's useful in practice (there is ofcourse no\nguarantee): I've been saved many times by it.  The only way to prevent\nthe race is to wait (either indefinitely for some user-input or for N\nseconds), but I don't want to trade of speed.\n\n>   http://thread.gmane.org/gmane.comp.version-control.git/128426\n\nA TRANSPORT_PUSH_CONFIM.  Cute.\n\n> As discussed in the top thread, this could also be used for \"interactive\n> push\" where you could examine and confirm the changes for each proposed\n> ref change.\n\nOverkill, I think.  I don't want to bolt on very heavy safety\nfeatures: just help the user help themselves with feedback.\n"},{"id":"221877","messageId":"20130624185546.GA25306@sigill.intra.peff.net","threadId":"34268","inReplyTo":"CALkWK0=d1wkKWngH+6gBd-2svj7r_tgC5=+zUbgJRDfUCzupSw@mail.gmail.com","subject":"Re: [PATCH] push: give early feedback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-24T18:55:46Z","receivedAt":"2013-06-24T18:55:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 25, 2013 at 12:12:16AM +0530, Ramkumar Ramachandra wrote:\n\n> >   1. It does not tell very much about how the refspecs are expanded or\n> >      what is going to happen. \"git push --dry-run\" gives a much more\n> >      complete view of what will be pushed.\n> \n> Yes.\n> \n>   $ git push\n>   # pushing refspecs ':' to ram\n> \n> Completely useless.\n> \n> On the other hand, if I implement it at the transport layer like\n> TRANSPORT_PUSH_DRY_RUN, it takes *way* too long to say anything\n> useful; the whole \"early\" part has been thrown out the window.  The\n> issue is again related to the same nightmare I'm having: not being\n> able to implement @{push} refspec because the transport API is so\n> tangled up; I can't call into the refspec-pattern-expander from\n> anywhere else.\n\nLeaving aside the transport API for a minute, you are always going to\nhave this lack-of-information versus time problem. A refspec like \":\"\nsays nothing particularly useful, but it can only be expanded once\ncontact is made with the other side (which is what takes time).\n\nI do not personally think the \"early\" information is particularly\nuseful. I don't have a problem with it as part of \"-v\" output (or\nenabled by config), but it seems useless for enough cases (e.g., user\ngave explicit refspecs, or refspecs are not useful without being\nexpanded) that showing it by default is going to be considered noisy\ncruft by many users.\n\nWas the unconditional nature of your earlier patch meant to be part of\nthe final version, or was it just illustrative?\n\n> Yes, ^C is a hack, but it's useful in practice (there is ofcourse no\n> guarantee): I've been saved many times by it.  The only way to prevent\n> the race is to wait (either indefinitely for some user-input or for N\n> seconds), but I don't want to trade of speed.\n\nI have had the opposite experience. Many times I tried \"rm -v\" to keep\nan eye on what was being removed, but I do not recall once where I\nfrantically reached for the keyboard in time to make a difference. But\nof course that is anecdotal, and push can be somewhat slower.\n\n> > As discussed in the top thread, this could also be used for \"interactive\n> > push\" where you could examine and confirm the changes for each proposed\n> > ref change.\n> \n> Overkill, I think.  I don't want to bolt on very heavy safety\n> features: just help the user help themselves with feedback.\n\nYes. I do not have any interest in such an interactive push, but the\npoint is that a potential first step to any confirmation scheme, no\nmatter what you want it to look like, is a hook. You don't seem to want\na confirmation scheme, though, due to the wait (and I cannot blame you,\nas I would not want it either; but then I would not want the extra\nrefspec message you propose, either).\n\n-Peff\n"},{"id":"221884","messageId":"CALkWK0mEWkXpsaUtMLc4r6vufDdSSdsX_PARwmObCSPW1mgAmQ@mail.gmail.com","threadId":"34268","inReplyTo":"20130624185546.GA25306@sigill.intra.peff.net","subject":"Re: [PATCH] push: give early feedback","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T19:24:17Z","receivedAt":"2013-06-24T19:24:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> Leaving aside the transport API for a minute, you are always going to\n> have this lack-of-information versus time problem. A refspec like \":\"\n> says nothing particularly useful, but it can only be expanded once\n> contact is made with the other side (which is what takes time).\n\nRight, and ':' is special in that aspect; it does not warrant slowing\ndown the expansion of refs/heads/*, for instance.  Besides, I suspect\n':' can be resolved much faster than using push --dry-run.\n\n> I do not personally think the \"early\" information is particularly\n> useful. I don't have a problem with it as part of \"-v\" output (or\n> enabled by config), but it seems useless for enough cases (e.g., user\n> gave explicit refspecs, or refspecs are not useful without being\n> expanded) that showing it by default is going to be considered noisy\n> cruft by many users.\n>\n> Was the unconditional nature of your earlier patch meant to be part of\n> the final version, or was it just illustrative?\n\nVery much illustrative.  The finer details of when exactly we should\nshow it can be discussed later.\n\n>> Yes, ^C is a hack, but it's useful in practice (there is ofcourse no\n>> guarantee): I've been saved many times by it.  The only way to prevent\n>> the race is to wait (either indefinitely for some user-input or for N\n>> seconds), but I don't want to trade of speed.\n>\n> I have had the opposite experience. Many times I tried \"rm -v\" to keep\n> an eye on what was being removed, but I do not recall once where I\n> frantically reached for the keyboard in time to make a difference. But\n> of course that is anecdotal, and push can be somewhat slower.\n\npush is an all-or-nothing network operation that has significant\nstartup time (name resolution etc.), very much unlike \"rm -v\".  Again,\nI'm talking about \"in practice\" *in the context of push*; not making\nany statements about the general usefulness or correctness of ^C.\n\n> Yes. I do not have any interest in such an interactive push, but the\n> point is that a potential first step to any confirmation scheme, no\n> matter what you want it to look like, is a hook. You don't seem to want\n> a confirmation scheme, though, due to the wait (and I cannot blame you,\n> as I would not want it either; but then I would not want the extra\n> refspec message you propose, either).\n\nI'm trying to figure out how to determine what a push will do without\nactually pushing (or --dry-run, which is almost as expensive).  You\nmight like to put that information in your prompt instead of stdout,\nbut do you agree that the information is worth getting?\n"},{"id":"221887","messageId":"20130624193924.GA27344@sigill.intra.peff.net","threadId":"34268","inReplyTo":"CALkWK0mEWkXpsaUtMLc4r6vufDdSSdsX_PARwmObCSPW1mgAmQ@mail.gmail.com","subject":"Re: [PATCH] push: give early feedback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-24T19:39:24Z","receivedAt":"2013-06-24T19:39:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 25, 2013 at 12:54:17AM +0530, Ramkumar Ramachandra wrote:\n\n> Jeff King wrote:\n> > Leaving aside the transport API for a minute, you are always going to\n> > have this lack-of-information versus time problem. A refspec like \":\"\n> > says nothing particularly useful, but it can only be expanded once\n> > contact is made with the other side (which is what takes time).\n> \n> Right, and ':' is special in that aspect; it does not warrant slowing\n> down the expansion of refs/heads/*, for instance.  Besides, I suspect\n> ':' can be resolved much faster than using push --dry-run.\n\nI think \":\" (or \"push.default=matching\") is the only thing that needs to\nactually contact the other side to decide which refs _might_ be pushed.\nBut to know which refs would _actually_ be pushed (and which ones would\nbe fast-forwards), you need to know where the remote refs are, which\ninvolves contacting the other side.\n\nIn both cases, you could potentially fake it with remote-tracking refs,\nif they are available (after all, that is what \"git status\" is reporting\nwith its ahead/behind counts, so there is certainly precedent). But it\nis important to note that what you see in the preview is not necessarily\nwhat is about to happen in the push, as we get new information on the\nremote tips during the push.\n\n> > Yes. I do not have any interest in such an interactive push, but the\n> > point is that a potential first step to any confirmation scheme, no\n> > matter what you want it to look like, is a hook. You don't seem to want\n> > a confirmation scheme, though, due to the wait (and I cannot blame you,\n> > as I would not want it either; but then I would not want the extra\n> > refspec message you propose, either).\n> \n> I'm trying to figure out how to determine what a push will do without\n> actually pushing (or --dry-run, which is almost as expensive).  You\n> might like to put that information in your prompt instead of stdout,\n> but do you agree that the information is worth getting?\n\nTo me personally, no, it is not interesting. But that does not mean it\nis not interesting to others. I didn't mean to dissuade you from\npursuing the topic, but rather only to qualify the my statements with \"I\nam probably not the user you are targeting with this feature\".\n\n-Peff\n"}]}