Re: [RFC PATCH v3 10/17] Move WebDAV HTTP push under remote-curl

[Tay Ray Chuan <rctay89@gmail.com>]

Hi,

On Thu, Oct 15, 2009 at 11:36 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> @@ -34,6 +34,10 @@ Commands are given by the caller on the helper's standard input, one per line.
>        value of the ref. A space-separated list of attributes follows
>        the name; unrecognized attributes are ignored. After the
>        complete list, outputs a blank line.
> ++
> +If 'push' is supported this may be called as 'list for-push'
> +to obtain the current refs prior to sending one or more 'push'
> +commands to the helper.

The new paragraph should have the same indentation as 'list'.

It would have been great if you implemented this as a filter, such that
'list <attr>' lists the refs with the specified attribute <attr>,
rather than hardcoding it.

> @@ -59,6 +63,22 @@ suitably updated.
> +When the push is complete, outputs one or more 'ok <dst>' or
> +'error <dst> <why>?' lines to indicate success or failure of
> +each pushed ref.  The status report output is terminated by
> +a blank line.  The option field <why> may be quoted in a C
> +style string if it contains an LF.

You should mention that this behaviour only occurs when the
--helper-status option is used.

> @@ -106,6 +132,11 @@ OPTIONS
> +'option dry-run' \{'true'|'false'\}:
> +       If true, pretend like the operation completed successfully,
> +       but don't actually change any repository data.  For most
> +       helpers this only applies to the 'push', if supported.
> +

The 'like' after 'pretend' can be, like, removed. :)

> diff --git a/http-push.c b/http-push.c
> @@ -1941,9 +1946,14 @@ int main(int argc, char **argv)
>
>                if (is_null_sha1(ref->peer_ref->new_sha1)) {
>                        if (delete_remote_branch(ref->name, 1) == -1) {
> -                               error("Could not remove %s", ref->name);
> +                               if (helper_status)
> +                                       printf("error %s cannot remove\n", ref->name);
> +                               else
> +                                       error("Could not remove %s", ref->name);
>                                rc = -4;
>                        }

I think error() calls should be left intact (as indicators to the
user), even when --helper-status is specified. It wouldn't affect
transport-helper.c's parsing of '(ok|error) <ref>' lines, since it
reads stdout only.

In other words, the above would read:

>                                error("Could not remove %s", ref->name);
> +                               if (helper_status)
> +                                       printf("error %s cannot remove\n", ref->name);
>                                rc = -4;
>                        }

> @@ -1968,12 +1980,15 @@ int main(int argc, char **argv)
>                                 * commits at the remote end and likely
>                                 * we were not up to date to begin with.
>                                 */
> -                               error("remote '%s' is not an ancestor of\n"
> -                                     "local '%s'.\n"
> -                                     "Maybe you are not up-to-date and "
> -                                     "need to pull first?",
> -                                     ref->name,
> -                                     ref->peer_ref->name);
> +                               if (helper_status)
> +                                       printf("error %s non-fast forward\n", ref->name);
> +                               else
> +                                       error("remote '%s' is not an ancestor of\n"
> +                                                 "local '%s'.\n"
> +                                                 "Maybe you are not up-to-date and "
> +                                                 "need to pull first?",
> +                                                 ref->name,
> +                                                 ref->peer_ref->name);
>                                rc = -2;
>                                continue;
>                        }

Same here.

> @@ -1987,14 +2002,20 @@ int main(int argc, char **argv)
>                /* Lock remote branch ref */
>                ref_lock = lock_remote(ref->name, LOCK_TIME);
>                if (ref_lock == NULL) {
> -                       fprintf(stderr, "Unable to lock remote branch %s\n",
> -                               ref->name);
> +                       if (helper_status)
> +                               printf("error %s lock error\n", ref->name);
> +                       else
> +                               fprintf(stderr, "Unable to lock remote branch %s\n",
> +                                       ref->name);
>                        rc = 1;
>                        continue;
>                }

Same here.

Two more areas in http-push.c that should have status messages
(generated on top of pu):

(I'm not sure what ref should read when there's no match, but
transport-helper.c should have no problems parsing it with 'null'.)

-->8--

diff --git a/http-push.c b/http-push.c
index 9010ccc..e979feb 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1916,9 +1916,12 @@ int main(int argc, char **argv)

 	/* Remove a remote branch if -d or -D was specified */
 	if (delete_branch) {
-		if (delete_remote_branch(refspec[0], force_delete) == -1)
+		if (delete_remote_branch(refspec[0], force_delete) == -1) {
 			fprintf(stderr, "Unable to delete remote branch %s\n",
 				refspec[0]);
+			if (helper_status)
+				printf("error %s cannot remove\n", refspec[0]);
+		}
 		goto cleanup;
 	}

@@ -1930,6 +1933,8 @@ int main(int argc, char **argv)
 	}
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n");
+		if (helper_status)
+			printf("error null no match\n");
 		rc = 0;
 		goto cleanup;
 	}

--
Cheers,
Ray Chuan