[PATCH] push: give early feedback

2013-06-24 Thread Ramkumar Ramachandra 
There are many configuration variables that determine exactly what a
push does.  Give the user early feedback so that she has a chance to
abort if she doesn't mean to push those refspecs to that destination
like:

  $ git push
  # pushing refspecs 'master next' to ram (^C to abort)

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 Goes without saying: this is an early preview.

 builtin/push.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..085d5ab 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)
 {
int i, errs;
struct remote *remote = pushremote_get(repo);
+   struct strbuf sb = STRBUF_INIT;
const char **url;
int url_nr;
 
@@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)
}
errs = 0;
url_nr = push_url_of_remote(remote, url);
+
+   for (i = 0; i  refspec_nr; i++)
+   strbuf_addf(sb, %s%s, refspec[i], i == refspec_nr - 1 ? : 
 );
+   printf(# pushing refspecs '%s' to %s (^C to abort)\n, sb.buf, 
remote-name);
+
if (url_nr) {
for (i = 0; i  url_nr; i++) {
struct transport *transport =
-- 
1.8.3.1.549.g1f3a412.dirty

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Fredrik Gustafsson 
On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:
 There are many configuration variables that determine exactly what a
 push does.  Give the user early feedback so that she has a chance to
 abort if she doesn't mean to push those refspecs to that destination
 like:
 
   $ git push
   # pushing refspecs 'master next' to ram (^C to abort)
 
 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---
  Goes without saying: this is an early preview.
 
  builtin/push.c | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/builtin/push.c b/builtin/push.c
 index 2d84d10..085d5ab 100644
 --- a/builtin/push.c
 +++ b/builtin/push.c
 @@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)
  {
   int i, errs;
   struct remote *remote = pushremote_get(repo);
 + struct strbuf sb = STRBUF_INIT;
   const char **url;
   int url_nr;
  
 @@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)
   }
   errs = 0;
   url_nr = push_url_of_remote(remote, url);
 +
 + for (i = 0; i  refspec_nr; i++)
 + strbuf_addf(sb, %s%s, refspec[i], i == refspec_nr - 1 ? : 
  );
 + printf(# pushing refspecs '%s' to %s (^C to abort)\n, sb.buf, 
 remote-name);
 +

How about a strbuf_release here?

Can you really be sure that refspec_nr is set here?

   if (url_nr) {
   for (i = 0; i  url_nr; i++) {
   struct transport *transport =
 -- 
 1.8.3.1.549.g1f3a412.dirty

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Junio C Hamano 
Fredrik Gustafsson iv...@iveqy.com writes:

 On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:
 There are many configuration variables that determine exactly what a
 push does.  Give the user early feedback so that she has a chance to
 abort if she doesn't mean to push those refspecs to that destination
 like:
 
   $ git push
   # pushing refspecs 'master next' to ram (^C to abort)
 
 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---
  Goes without saying: this is an early preview.
 
  builtin/push.c | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/builtin/push.c b/builtin/push.c
 index 2d84d10..085d5ab 100644
 --- a/builtin/push.c
 +++ b/builtin/push.c
 @@ -330,6 +330,7 @@ static int do_push(const char *repo, int flags)
  {
  int i, errs;
  struct remote *remote = pushremote_get(repo);
 +struct strbuf sb = STRBUF_INIT;
  const char **url;
  int url_nr;
  
 @@ -375,6 +376,11 @@ static int do_push(const char *repo, int flags)
  }
  errs = 0;
  url_nr = push_url_of_remote(remote, url);
 +
 +for (i = 0; i  refspec_nr; i++)
 +strbuf_addf(sb, %s%s, refspec[i], i == refspec_nr - 1 ? : 
  );
 +printf(# pushing refspecs '%s' to %s (^C to abort)\n, sb.buf, 
 remote-name);
 +

 How about a strbuf_release here?

 Can you really be sure that refspec_nr is set here?

Doing this unconditionally when the user says git push there this
would be mildly annoying.

I think this belongs to either --verbose or even --debug, for people
who are trying to make sure an underspecified git push (or git
push there) does what they want, but for those use cases, it
probably is better to tie this to --dry-run.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Jeff King 
On Mon, Jun 24, 2013 at 11:11:02PM +0530, Ramkumar Ramachandra wrote:

 There are many configuration variables that determine exactly what a
 push does.  Give the user early feedback so that she has a chance to
 abort if she doesn't mean to push those refspecs to that destination
 like:
 
   $ git push
   # pushing refspecs 'master next' to ram (^C to abort)

If your intent is to let people stop disastrous pushes before they
complete, I think there are two failings:

  1. It does not tell very much about how the refspecs are expanded or
 what is going to happen. git push --dry-run gives a much more
 complete view of what will be pushed.

  2. You are creating a race with the user to hit ^C. It will probably
 work if there are a lot of objects to be pushed. But not if the
 push is small, or even has no pack at all (e.g., only deletions, or
 moving refs to existing history). As an added bonus, since push
 prints its output after receiving commitment from the server, it is
 possible for the server to commit a change, have the user hit ^C,
 thinking they beat the race, only to find out that the server did
 indeed accept the change.

I think you could deal with both of them by doing something like:

  git push --dry-run $@ || exit 1
  echo 2 OK to push?
  read response
  case $response in
  [Yy]) ;;
  *) exit 1
  esac
  exec git push $@

That is subject to a race condition in a busy repo (you do not know that
the refs are in the same state on either end between the two pushes).
You could solve that by having a pre-push hook on the local side that
gets the list of proposed updates.

In fact, I think this came up before but no hook code ever materialized:

  http://thread.gmane.org/gmane.comp.version-control.git/128273

  http://thread.gmane.org/gmane.comp.version-control.git/128426

As discussed in the top thread, this could also be used for interactive
push where you could examine and confirm the changes for each proposed
ref change.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Ramkumar Ramachandra 
Jeff King wrote:
 If your intent is to let people stop disastrous pushes before they
 complete, I think there are two failings:

   1. It does not tell very much about how the refspecs are expanded or
  what is going to happen. git push --dry-run gives a much more
  complete view of what will be pushed.

Yes.

  $ git push
  # pushing refspecs ':' to ram

Completely useless.

On the other hand, if I implement it at the transport layer like
TRANSPORT_PUSH_DRY_RUN, it takes *way* too long to say anything
useful; the whole early part has been thrown out the window.  The
issue is again related to the same nightmare I'm having: not being
able to implement @{push} refspec because the transport API is so
tangled up; I can't call into the refspec-pattern-expander from
anywhere else.

   2. You are creating a race with the user to hit ^C. It will probably
  work if there are a lot of objects to be pushed. But not if the
  push is small, or even has no pack at all (e.g., only deletions, or
  moving refs to existing history). As an added bonus, since push
  prints its output after receiving commitment from the server, it is
  possible for the server to commit a change, have the user hit ^C,
  thinking they beat the race, only to find out that the server did
  indeed accept the change.

Yes, ^C is a hack, but it's useful in practice (there is ofcourse no
guarantee): I've been saved many times by it.  The only way to prevent
the race is to wait (either indefinitely for some user-input or for N
seconds), but I don't want to trade of speed.

   http://thread.gmane.org/gmane.comp.version-control.git/128426

A TRANSPORT_PUSH_CONFIM.  Cute.

 As discussed in the top thread, this could also be used for interactive
 push where you could examine and confirm the changes for each proposed
 ref change.

Overkill, I think.  I don't want to bolt on very heavy safety
features: just help the user help themselves with feedback.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Jeff King 
On Tue, Jun 25, 2013 at 12:12:16AM +0530, Ramkumar Ramachandra wrote:

1. It does not tell very much about how the refspecs are expanded or
   what is going to happen. git push --dry-run gives a much more
   complete view of what will be pushed.
 
 Yes.
 
   $ git push
   # pushing refspecs ':' to ram
 
 Completely useless.
 
 On the other hand, if I implement it at the transport layer like
 TRANSPORT_PUSH_DRY_RUN, it takes *way* too long to say anything
 useful; the whole early part has been thrown out the window.  The
 issue is again related to the same nightmare I'm having: not being
 able to implement @{push} refspec because the transport API is so
 tangled up; I can't call into the refspec-pattern-expander from
 anywhere else.

Leaving aside the transport API for a minute, you are always going to
have this lack-of-information versus time problem. A refspec like :
says nothing particularly useful, but it can only be expanded once
contact is made with the other side (which is what takes time).

I do not personally think the early information is particularly
useful. I don't have a problem with it as part of -v output (or
enabled by config), but it seems useless for enough cases (e.g., user
gave explicit refspecs, or refspecs are not useful without being
expanded) that showing it by default is going to be considered noisy
cruft by many users.

Was the unconditional nature of your earlier patch meant to be part of
the final version, or was it just illustrative?

 Yes, ^C is a hack, but it's useful in practice (there is ofcourse no
 guarantee): I've been saved many times by it.  The only way to prevent
 the race is to wait (either indefinitely for some user-input or for N
 seconds), but I don't want to trade of speed.

I have had the opposite experience. Many times I tried rm -v to keep
an eye on what was being removed, but I do not recall once where I
frantically reached for the keyboard in time to make a difference. But
of course that is anecdotal, and push can be somewhat slower.

  As discussed in the top thread, this could also be used for interactive
  push where you could examine and confirm the changes for each proposed
  ref change.
 
 Overkill, I think.  I don't want to bolt on very heavy safety
 features: just help the user help themselves with feedback.

Yes. I do not have any interest in such an interactive push, but the
point is that a potential first step to any confirmation scheme, no
matter what you want it to look like, is a hook. You don't seem to want
a confirmation scheme, though, due to the wait (and I cannot blame you,
as I would not want it either; but then I would not want the extra
refspec message you propose, either).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Ramkumar Ramachandra 
Jeff King wrote:
 Leaving aside the transport API for a minute, you are always going to
 have this lack-of-information versus time problem. A refspec like :
 says nothing particularly useful, but it can only be expanded once
 contact is made with the other side (which is what takes time).

Right, and ':' is special in that aspect; it does not warrant slowing
down the expansion of refs/heads/*, for instance.  Besides, I suspect
':' can be resolved much faster than using push --dry-run.

 I do not personally think the early information is particularly
 useful. I don't have a problem with it as part of -v output (or
 enabled by config), but it seems useless for enough cases (e.g., user
 gave explicit refspecs, or refspecs are not useful without being
 expanded) that showing it by default is going to be considered noisy
 cruft by many users.

 Was the unconditional nature of your earlier patch meant to be part of
 the final version, or was it just illustrative?

Very much illustrative.  The finer details of when exactly we should
show it can be discussed later.

 Yes, ^C is a hack, but it's useful in practice (there is ofcourse no
 guarantee): I've been saved many times by it.  The only way to prevent
 the race is to wait (either indefinitely for some user-input or for N
 seconds), but I don't want to trade of speed.

 I have had the opposite experience. Many times I tried rm -v to keep
 an eye on what was being removed, but I do not recall once where I
 frantically reached for the keyboard in time to make a difference. But
 of course that is anecdotal, and push can be somewhat slower.

push is an all-or-nothing network operation that has significant
startup time (name resolution etc.), very much unlike rm -v.  Again,
I'm talking about in practice *in the context of push*; not making
any statements about the general usefulness or correctness of ^C.

 Yes. I do not have any interest in such an interactive push, but the
 point is that a potential first step to any confirmation scheme, no
 matter what you want it to look like, is a hook. You don't seem to want
 a confirmation scheme, though, due to the wait (and I cannot blame you,
 as I would not want it either; but then I would not want the extra
 refspec message you propose, either).

I'm trying to figure out how to determine what a push will do without
actually pushing (or --dry-run, which is almost as expensive).  You
might like to put that information in your prompt instead of stdout,
but do you agree that the information is worth getting?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: give early feedback

2013-06-24 Thread Jeff King 
On Tue, Jun 25, 2013 at 12:54:17AM +0530, Ramkumar Ramachandra wrote:

 Jeff King wrote:
  Leaving aside the transport API for a minute, you are always going to
  have this lack-of-information versus time problem. A refspec like :
  says nothing particularly useful, but it can only be expanded once
  contact is made with the other side (which is what takes time).
 
 Right, and ':' is special in that aspect; it does not warrant slowing
 down the expansion of refs/heads/*, for instance.  Besides, I suspect
 ':' can be resolved much faster than using push --dry-run.

I think : (or push.default=matching) is the only thing that needs to
actually contact the other side to decide which refs _might_ be pushed.
But to know which refs would _actually_ be pushed (and which ones would
be fast-forwards), you need to know where the remote refs are, which
involves contacting the other side.

In both cases, you could potentially fake it with remote-tracking refs,
if they are available (after all, that is what git status is reporting
with its ahead/behind counts, so there is certainly precedent). But it
is important to note that what you see in the preview is not necessarily
what is about to happen in the push, as we get new information on the
remote tips during the push.

  Yes. I do not have any interest in such an interactive push, but the
  point is that a potential first step to any confirmation scheme, no
  matter what you want it to look like, is a hook. You don't seem to want
  a confirmation scheme, though, due to the wait (and I cannot blame you,
  as I would not want it either; but then I would not want the extra
  refspec message you propose, either).
 
 I'm trying to figure out how to determine what a push will do without
 actually pushing (or --dry-run, which is almost as expensive).  You
 might like to put that information in your prompt instead of stdout,
 but do you agree that the information is worth getting?

To me personally, no, it is not interesting. But that does not mean it
is not interesting to others. I didn't mean to dissuade you from
pursuing the topic, but rather only to qualify the my statements with I
am probably not the user you are targeting with this feature.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html