send-pack question.

Junio C Hamano· Jul 31, 2005, 05:47 UTC · lore

I have been looking at send-pack because some people seem to want to push into a remote repository that names heads differently from local. I have some questions that do not have to do with anything about their request, but about what the current code intends to do.

 * Right now, "send-pack --all" into an empty repository does
   not do anything, but "send-pack --all master" into an empty
   repository pushes all local heads.  This is because we do not
   check "send_all" when deciding if we want to call try_match
   on local references.  I am assuming this is an oversight; am
   I correct?  If so, does the attached patch look OK?

 * It appears to me that you can say "send-pack net", and
   depending on how the remote lists its refs, you can end up
   updating their refs/heads/net or refs/tags/net.  More
   confusingly, you could say "send-pack net net" to update
   both.  More realistically, you could get confused with a
   remote that has refs/heads/jgarzik/net and
   refs/heads/dsmiller/net in this way.  I think it should
   detect, stop and warn about the ambiguity and require the
   user to be more explicit.  Am I reading the current code
   correctly?

   I've always _hated_ the interface to path_match() which
   pretends to be just a boolean function but actually has a
   grave side effect, by the way.

--- # - pu: git-fetch-script http fix. # + (working tree)

diff --git a/send-pack.c b/send-pack.c
--- a/send-pack.c
+++ b/send-pack.c
@@ -4,7 +4,8 @@
 #include "pkt-line.h"
 
 static const char send_pack_usage[] =
-"git-send-pack [--exec=git-receive-pack] [host:]directory [heads]*";
+"git-send-pack [--all] [--exec=git-receive-pack] <remote> [<head>...]\n"
+"  --all and explicit <head> specification are mutually exclusive.";
 static const char *exec = "git-receive-pack";
 static int send_all = 0;
 static int force_update = 0;
@@ -214,7 +215,7 @@ static int send_pack(int in, int out, in
 	/*
 	 * See if we have any refs that the other end didn't have
 	 */
-	if (nr_match) {
+	if (nr_match || send_all) {
 		local_ref_nr_match = nr_match;
 		local_ref_match = match;
 		local_ref_list = ref_list;
@@ -281,6 +282,8 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(send_pack_usage);
+	if (heads && send_all)
+		usage(send_pack_usage);
 	pid = git_connect(fd, dest, exec);
 	if (pid < 0)
 		return 1;

Linus Torvalds· Jul 31, 2005, 06:02 UTC · re: Junio C Hamano · lore

Re: send-pack question.

On Sat, 30 Jul 2005, Junio C Hamano wrote:

Show 7 quoted lines

> 
>  * Right now, "send-pack --all" into an empty repository does
>    not do anything, but "send-pack --all master" into an empty
>    repository pushes all local heads.  This is because we do not
>    check "send_all" when deciding if we want to call try_match
>    on local references.  I am assuming this is an oversight; am
>    I correct?  If so, does the attached patch look OK?

Yeah, that sounds like me just not having taken my 'meds. The patch looks fine.

>  * It appears to me that you can say "send-pack net", and
>    depending on how the remote lists its refs, you can end up
>    updating their refs/heads/net or refs/tags/net.

Yeh. I was wanting to sort the refs to make everything be totally repeatable, but that was more of an urge than a real plan.

Show 8 quoted lines

>						  More
>    confusingly, you could say "send-pack net net" to update
>    both.  More realistically, you could get confused with a
>    remote that has refs/heads/jgarzik/net and
>    refs/heads/dsmiller/net in this way.  I think it should
>    detect, stop and warn about the ambiguity and require the
>    user to be more explicit.  Am I reading the current code
>    correctly?

Yes, warning on ambiguity sounds like a sound plan, and then you don't need to sort.

You also probably to come up with a syntax for saying "xyz is the local name, abc is the remote name". That's needed for both the pulling and the pushing side, but I didn't ever do it.

>    I've always _hated_ the interface to path_match() which
>    pretends to be just a boolean function but actually has a
>    grave side effect, by the way.

It's "interesting" but useful. But I agree, we've had one bug already due to the interesting part.

If you do the ambiguity thing, you might mark them used some separate way, and maybe avoid that side effect (or rather - the side effect would still exist, but instead of removing the entry, it would just mark it as "seen", ie make it less drastic).

		Linus