Re: User-relative paths (was: Server side programs)

[Petr Baudis <pasky@suse.cz>]

Dear diary, on Sun, Oct 23, 2005 at 11:41:52AM CEST, I got a letter
where Andreas Ericsson <ae@op5.se> told me that...
> Anyways, the attached patch does this. I've tested all the various 
> syntaxes and they work as expected. rsync, http and local files take the 
> same syntax as before. I haven't added support for user-relative paths 
> to the git-daemon (can't see the point, really) although that can be 
> done easily enough.

It would be useful to add a [PATCH] tag to subject when you submit a
patch, so that we notice it better. ;-)

You don't update the documentation even though there seem to be some
syntactic changes. You should at least update

	Documentation/pull-fetch-param.txt

Also before Junio asks you, in the followup patches, you might want to
sign off the patch if you want it integrated.

> diff --git a/Makefile b/Makefile
> index 903c57c..87188ea 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -359,6 +362,9 @@ git-cherry-pick: git-revert
>  %.o: %.S
>  	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
>  
> +$(SERVERSIDE_PROGRAMS) : git-%$X : %.o srvside-ssh.o $(LIB_FILE)
> +	$(CC) $(ALL_CFLAGS) -o $@ $(filter %o,$^) $(LIBS)
> +
>  git-%$X: %.o $(LIB_FILE)
>  	$(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)
>  

Why are you adding own compilation command, and why is it inconsistent
with the git-%$X's one?

> diff --git a/connect.c b/connect.c
> index b171c5d..0d78b3e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -436,33 +436,44 @@ static int git_tcp_connect(int fd[2], co
> +	/* leading colon marks relative path for ssh.
> +	 * Check for host == url and default to PROTO_SSH to allow
> +	 *   $ git fetch kernel.org:git
> +	 */
> +	if(ptr && (!path || ptr < path)) {
> +		if(host == url)
> +			protocol = PROTO_SSH;
> +
> +		if(protocol == PROTO_SSH) {
> +			*ptr = '\0';
> +			path = ptr + 1;
>  		}
>  	}

If I understand this right,

	ssh://foo.bar:baz/quux

will make foo.bar the host and baz/quux the path. Please, do NOT do
this! It is supposed to be a URL, dammit! And you know, URLs have
defined _syntax_, and that's important at least every time the URL gets
out of GIT's context. Or stop it calling URL altogether, to prevent any
confusion. But in URLs, the space between : and / is a port definition.
See also RFC3986 (aka STD066) and RFC2718.

Thanks.

> diff --git a/receive-pack.c b/receive-pack.c
> index 8f157bc..9a040ff 100644
> --- a/receive-pack.c
> +++ b/receive-pack.c
> @@ -265,18 +267,9 @@ int main(int argc, char **argv)
>  	if (!dir)
>  		usage(receive_pack_usage);
>  
> -	/* chdir to the directory. If that fails, try appending ".git" */
> -	if (chdir(dir) < 0) {
> -		if (chdir(mkpath("%s.git", dir)) < 0)
> -			die("unable to cd to %s", dir);
> -	}
> -
> -	/* If we have a ".git" directory, chdir to it */
> -	chdir(".git");
> -	putenv("GIT_DIR=.");
> +	/* Find the right directory */
> +	srvside_chdir(dir, 0);
>  
> -	if (access("objects", X_OK) < 0 || access("refs/heads", X_OK) < 0)
> -		die("%s doesn't appear to be a git directory", dir);
>  	write_head_info();
>  
>  	/* EOF */

No srvside_chdir() declaration?

> diff --git a/srvside-ssh.c b/srvside-ssh.c
> new file mode 100644
> index 0000000..0ed5d30
> --- /dev/null
> +++ b/srvside-ssh.c
> @@ -0,0 +1,63 @@
> +#include "cache.h"
> +#include <unistd.h>
> +#include <pwd.h>
> +
> +extern const char *__progname;

How portable is this? It appears that no standard really defines this,
and Google faintly hints at least some Cygwin-related problems...

> diff --git a/upload-pack.c b/upload-pack.c
> index accdba6..356c9b1 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -5,6 +5,7 @@
>  #include "object.h"
>  
>  static const char upload_pack_usage[] = "git-upload-pack [--strict] [--timeout=nn] <dir>";
> +extern void srvside_chdir(const char *path, int strict);
>  
>  #define MAX_HAS 256
>  #define MAX_NEEDS 256

What about a .h file?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.