Re: [PATCH 01/11] Resumable clone: create service git-prime-clone

2016-09-27 Thread Kevin Wern
On Fri, Sep 16, 2016 at 01:53:15PM -0700, Junio C Hamano wrote:
> Kevin Wern  writes:
> 
> > Create git-prime-clone, a program to be executed on the server that
> > returns the location and type of static resource to download before
> > performing the rest of a clone.
> >
> > Additionally, as this executable's location will be configurable (see:
> > upload-pack and receive-pack), add the program to
> > BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
> > git-prime-clone executable to gitignore, as well
> >
> > Signed-off-by: Kevin Wern 
> > ---
> 
> I wonder if we even need a separate service like this.
> 
> Wouldn't a new protocol capability that is advertised from
> upload-pack sufficient to tell the "git clone" that it can
> and should consider priming from this static resource?

The short answer is yes, it could be done that way. Both methods--extending
upload-pack and creating a new service--were suggested in different
discussions.

However, my thought was to implement the separate service because:
- It is much easier for an admin trying the feature to sanity check the
  output of an executable, compared to passing messages to upload-pack.
- In the other scenario, upload-pack might get too expansive in size
  and scope--not only codewise, but in terms of config namespace if
  "uploadpack" concerns too many things that are only tangentially
  related (the properties of the primer resource).
- The transport_prime_clone API can be called independent of other
  transport API functions, which might prove useful when revisiting or
  refactoring code.
- You favored the creation of a service in our original discussion [1].
  I'm not sure if your reasoning was similar to mine.

It definitely was a tight decision--for me, ultimately weighing the value added
in usability (point 1) against the need for a "failsafe" implementation. All
the other points are more speculative, IMO, but the first was strong enough for
me.

What do you think?

[1] http://www.spinics.net/lists/git/msg269992.html

> Two minor comments:
> 
>  - For whom are you going to localize these strings?  This program
>is running on the server side and we do not know the locale
>preferred by the end-user who is sitting on the other end of the
>connection, no?
> 
>  - Turn "}\n\s+else " into "} else ", please.

These are fair points. Changing for the revised version.

- Kevin


Re: [PATCH 01/11] Resumable clone: create service git-prime-clone

2016-09-16 Thread Junio C Hamano
Kevin Wern  writes:

> Create git-prime-clone, a program to be executed on the server that
> returns the location and type of static resource to download before
> performing the rest of a clone.
>
> Additionally, as this executable's location will be configurable (see:
> upload-pack and receive-pack), add the program to
> BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
> git-prime-clone executable to gitignore, as well
>
> Signed-off-by: Kevin Wern 
> ---

I wonder if we even need a separate service like this.

Wouldn't a new protocol capability that is advertised from
upload-pack sufficient to tell the "git clone" that it can
and should consider priming from this static resource?

> +static void prime_clone(void)
> +{
> + if (!enabled) {
> + fprintf(stderr, _("prime-clone not enabled\n"));
> + }
> + else if (url && filetype){
> + packet_write(1, "%s %s\n", filetype, url);
> + }
> + else if (url || filetype) {
> + if (filetype)
> + fprintf(stderr, _("prime-clone not properly "
> +   "configured: missing url\n"));
> + else if (url)
> + fprintf(stderr, _("prime-clone not properly "
> +   "configured: missing filetype\n"));
> + }
> + packet_flush(1);
> +}

Two minor comments:

 - For whom are you going to localize these strings?  This program
   is running on the server side and we do not know the locale
   preferred by the end-user who is sitting on the other end of the
   connection, no?

 - Turn "}\n\s+else " into "} else ", please.



[PATCH 01/11] Resumable clone: create service git-prime-clone

2016-09-15 Thread Kevin Wern
Create git-prime-clone, a program to be executed on the server that
returns the location and type of static resource to download before
performing the rest of a clone.

Additionally, as this executable's location will be configurable (see:
upload-pack and receive-pack), add the program to
BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
git-prime-clone executable to gitignore, as well

Signed-off-by: Kevin Wern 
---
 .gitignore|  1 +
 Documentation/git-prime-clone.txt | 39 
 Makefile  |  2 +
 builtin.h |  1 +
 builtin/prime-clone.c | 77 +++
 git.c |  1 +
 6 files changed, 121 insertions(+)
 create mode 100644 Documentation/git-prime-clone.txt
 create mode 100644 builtin/prime-clone.c

diff --git a/.gitignore b/.gitignore
index 5087ce1..bfea25c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,6 +106,7 @@
 /git-pack-refs
 /git-parse-remote
 /git-patch-id
+/git-prime-clone
 /git-prune
 /git-prune-packed
 /git-pull
diff --git a/Documentation/git-prime-clone.txt 
b/Documentation/git-prime-clone.txt
new file mode 100644
index 000..fc5917d
--- /dev/null
+++ b/Documentation/git-prime-clone.txt
@@ -0,0 +1,39 @@
+git-prime-clone(1)
+
+
+NAME
+
+git-prime-clone - Get the location of an alternate resource
+to fetch before clone
+
+
+SYNOPSIS
+
+[verse]
+'git prime-clone' [--strict] 
+
+DESCRIPTION
+---
+
+Outputs the resource, if configured to do so. Otherwise, returns
+nothing (packet flush ).
+
+CONFIGURE
+-
+
+primeclone.url::
+   The full url of the resource (e.g.
+   http://examplehost/pack-$NAME.pack).
+
+primeclone.filetype::
+   The type of the resource (e.g. pack).
+
+primeclone.enabled::
+   When 'false', git-prime-clone will return an empty response,
+   regardless of what the rest of the configuration specifies;
+   otherwise, it will return the configured response. Is 'true'
+   by default.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 24bef8d..f2564ec 100644
--- a/Makefile
+++ b/Makefile
@@ -648,6 +648,7 @@ OTHER_PROGRAMS = git$X
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
 BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-prime-clone
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
 BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-shell
@@ -904,6 +905,7 @@ BUILTIN_OBJS += builtin/pack-objects.o
 BUILTIN_OBJS += builtin/pack-redundant.o
 BUILTIN_OBJS += builtin/pack-refs.o
 BUILTIN_OBJS += builtin/patch-id.o
+BUILTIN_OBJS += builtin/prime-clone.o
 BUILTIN_OBJS += builtin/prune-packed.o
 BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
diff --git a/builtin.h b/builtin.h
index 6b95006..c9e2254 100644
--- a/builtin.h
+++ b/builtin.h
@@ -97,6 +97,7 @@ extern int cmd_notes(int argc, const char **argv, const char 
*prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);
 extern int cmd_patch_id(int argc, const char **argv, const char *prefix);
+extern int cmd_prime_clone(int argc, const char **argv, const char *prefix);
 extern int cmd_prune(int argc, const char **argv, const char *prefix);
 extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
diff --git a/builtin/prime-clone.c b/builtin/prime-clone.c
new file mode 100644
index 000..ce914d3
--- /dev/null
+++ b/builtin/prime-clone.c
@@ -0,0 +1,77 @@
+#include "cache.h"
+#include "parse-options.h"
+#include "pkt-line.h"
+
+static char const * const prime_clone_usage[] = {
+   N_("git prime-clone [--strict] "),
+   NULL
+};
+
+static unsigned int enabled = 1;
+static const char *url = NULL, *filetype = NULL;
+static int strict;
+
+static struct option prime_clone_options[] = {
+   OPT_BOOL(0, "strict", &strict, N_("Do not attempt /.git if  "
+ "is not a git directory")),
+   OPT_END(),
+};
+
+static void prime_clone(void)
+{
+   if (!enabled) {
+   fprintf(stderr, _("prime-clone not enabled\n"));
+   }
+   else if (url && filetype){
+   packet_write(1, "%s %s\n", filetype, url);
+   }
+   else if (url || filetype) {
+   if (filetype)
+   fprintf(stderr, _("prime-clone not properly "
+ "configured: missing url\n"));
+   else if (url)
+   fprintf(stderr, _("prime-clone not properly "
+ "configured: missing filetype\n"));
+   }
+   packet_flush(1);
+}
+
+static int prime_clone_config(const char *v