[PATCH v2] Make fetch-pack a builtin with an internal API

[PATCH v2] Make fetch-pack a builtin with an internal API

[Daniel Barkalow]

In addition to making fetch-pack a builtin, this allows it to be called 
directly from other built-in code without generating and parsing argument 
lists, which will be useful for builtin-fetch.

Incidently, it makes git-fetch-pack not output lists of what it fetched 
when it fails.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Makefile                             |    1 +
 fetch-pack.c => builtin-fetch-pack.c |   92 ++++++++++++++++++++++++++--------
 builtin.h                            |    1 +
 fetch-pack.h                         |   16 ++++++
 git.c                                |    1 +
 5 files changed, 89 insertions(+), 22 deletions(-)
 rename fetch-pack.c => builtin-fetch-pack.c (92%)
 create mode 100644 fetch-pack.h

diff --git a/Makefile b/Makefile
index 4ea5e45..da750f8 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,7 @@ BUILTIN_OBJS = \
 	builtin-diff-files.o \
 	builtin-diff-index.o \
 	builtin-diff-tree.o \
+	builtin-fetch-pack.o \
 	builtin-fetch--tool.o \
 	builtin-fmt-merge-msg.o \
 	builtin-for-each-ref.o \
diff --git a/fetch-pack.c b/builtin-fetch-pack.c
similarity index 92%
rename from fetch-pack.c
rename to builtin-fetch-pack.c
index 9c81305..27daa33 100644
--- a/fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -6,6 +6,7 @@
 #include "exec_cmd.h"
 #include "pack.h"
 #include "sideband.h"
+#include "fetch-pack.h"
 
 static int keep_pack;
 static int transfer_unpack_limit = -1;
@@ -573,7 +574,7 @@ static int get_pack(int xd[2])
 	die("%s died of unnatural causes %d", argv[0], status);
 }
 
-static int fetch_pack(int fd[2], int nr_match, char **match)
+static struct ref *do_fetch_pack(int fd[2], int nr_match, char **match)
 {
 	struct ref *ref;
 	unsigned char sha1[20];
@@ -615,12 +616,7 @@ static int fetch_pack(int fd[2], int nr_match, char **match)
 		die("git-fetch-pack: fetch failed.");
 
  all_done:
-	while (ref) {
-		printf("%s %s\n",
-		       sha1_to_hex(ref->old_sha1), ref->name);
-		ref = ref->next;
-	}
-	return 0;
+	return ref;
 }
 
 static int remove_duplicates(int nr_heads, char **heads)
@@ -663,15 +659,36 @@ static int fetch_pack_config(const char *var, const char *value)
 
 static struct lock_file lock;
 
-int main(int argc, char **argv)
+void setup_fetch_pack(const char *_uploadpack,
+		      int _quiet,
+		      int _keep_pack,
+		      int _unpacklimit,
+		      int _use_thin_pack,
+		      int _fetch_all,
+		      int _verbose,
+		      int _depth,
+		      int _no_progress)
+{
+	uploadpack = _uploadpack;
+	quiet = _quiet;
+	keep_pack = _keep_pack;
+	if (_unpacklimit >= 0)
+		unpack_limit = _unpacklimit;
+	if (keep_pack)
+		unpack_limit = 0;
+	use_thin_pack = _use_thin_pack;
+	fetch_all = _fetch_all;
+	verbose = _verbose;
+	depth = _depth;
+	no_progress = _no_progress;
+}
+
+int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, ret, nr_heads;
+	struct ref *ref;
 	char *dest = NULL, **heads;
-	int fd[2];
-	pid_t pid;
-	struct stat st;
 
-	setup_git_directory();
 	git_config(fetch_pack_config);
 
 	if (0 <= transfer_unpack_limit)
@@ -682,7 +699,7 @@ int main(int argc, char **argv)
 	nr_heads = 0;
 	heads = NULL;
 	for (i = 1; i < argc; i++) {
-		char *arg = argv[i];
+		const char *arg = argv[i];
 
 		if (*arg == '-') {
 			if (!prefixcmp(arg, "--upload-pack=")) {
@@ -716,8 +733,6 @@ int main(int argc, char **argv)
 			}
 			if (!prefixcmp(arg, "--depth=")) {
 				depth = strtol(arg + 8, NULL, 0);
-				if (stat(git_path("shallow"), &st))
-					st.st_mtime = 0;
 				continue;
 			}
 			if (!strcmp("--no-progress", arg)) {
@@ -726,22 +741,52 @@ int main(int argc, char **argv)
 			}
 			usage(fetch_pack_usage);
 		}
-		dest = arg;
-		heads = argv + i + 1;
+		dest = (char *)arg;
+		heads = (char **)(argv + i + 1);
 		nr_heads = argc - i - 1;
 		break;
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	pid = git_connect(fd, dest, uploadpack, verbose ? CONNECT_VERBOSE : 0);
+
+	ref = fetch_pack(dest, nr_heads, heads);
+
+	ret = !ref;
+
+	while (ref) {
+		printf("%s %s\n",
+		       sha1_to_hex(ref->old_sha1), ref->name);
+		ref = ref->next;
+	}
+
+	return ret;
+}
+
+struct ref *fetch_pack(const char *dest, int nr_heads, char **heads)
+{
+	int i, ret;
+	int fd[2];
+	pid_t pid;
+	struct ref *ref;
+	struct stat st;
+
+	if (depth > 0) {
+		if (stat(git_path("shallow"), &st))
+			st.st_mtime = 0;
+	}
+
+	printf("connect to %s\n", dest);
+
+	pid = git_connect(fd, (char *)dest, uploadpack,
+                          verbose ? CONNECT_VERBOSE : 0);
 	if (pid < 0)
-		return 1;
+		return NULL;
 	if (heads && nr_heads)
 		nr_heads = remove_duplicates(nr_heads, heads);
-	ret = fetch_pack(fd, nr_heads, heads);
+	ref = do_fetch_pack(fd, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret = finish_connect(pid);
 
 	if (!ret && nr_heads) {
 		/* If the heads to pull were given, we should have
@@ -785,5 +830,8 @@ int main(int argc, char **argv)
 		}
 	}
 
-	return !!ret;
+	if (ret)
+		ref = NULL;
+
+	return ref;
 }
diff --git a/builtin.h b/builtin.h
index 661a92f..8fa38d4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -31,6 +31,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch__tool(int argc, const char **argv, const char *prefix);
 extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix);
 extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);
diff --git a/fetch-pack.h b/fetch-pack.h
new file mode 100644
index 0000000..2bd05a8
--- /dev/null
+++ b/fetch-pack.h
@@ -0,0 +1,16 @@
+#ifndef FETCH_PACK_API
+#define FETCH_PACK_API
+
+void setup_fetch_pack(const char *_uploadpack,
+		      int _quiet,
+		      int _keep_pack,
+		      int _unpacklimit,
+		      int _use_thin_pack,
+		      int _fetch_all,
+		      int _verbose,
+		      int _depth,
+		      int _no_progress);
+
+struct ref *fetch_pack(const char *dest, int nr_heads, char **heads);
+
+#endif
diff --git a/git.c b/git.c
index b949cbb..df45161 100644
--- a/git.c
+++ b/git.c
@@ -307,6 +307,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "diff-files", cmd_diff_files },
 		{ "diff-index", cmd_diff_index, RUN_SETUP },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
 		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
 		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
 		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
-- 
1.5.2.2.1399.g097d5-dirty

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Junio C Hamano]

Are _identifiers with leading underscore Kosher thing to do, I
wonder...  We do have ones with trailing ones (mostly qsort
functions) and I think they are done that way for the sake of
standards conformance.

Gone is a "#if 0/#endif" which is good.

diff --git a/fetch-pack.h b/fetch-pack.h
new file mode 100644
index 0000000..2bd05a8
--- /dev/null
+++ b/fetch-pack.h
@@ -0,0 +1,16 @@
+#ifndef FETCH_PACK_API

We seem to say "#ifndef FETCH_PACK_H" in such a case, though.
Nobody seems to call setup_fetch_pack() yet.  How complete is
this patch meant to be?

The program is somehow much more pleasant to follow, even though
there is no fundamental change anywhere.

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Daniel Barkalow]

On Sun, 8 Jul 2007, Junio C Hamano wrote:

> Are _identifiers with leading underscore Kosher thing to do, I
> wonder...  We do have ones with trailing ones (mostly qsort
> functions) and I think they are done that way for the sake of
> standards conformance.

I'm not sure; I inherited that bit of code from Julian. Do we have a 
standard idiom for a function that sets a bunch of static variables?

> diff --git a/fetch-pack.h b/fetch-pack.h
> new file mode 100644
> index 0000000..2bd05a8
> --- /dev/null
> +++ b/fetch-pack.h
> @@ -0,0 +1,16 @@
> +#ifndef FETCH_PACK_API
> 
> We seem to say "#ifndef FETCH_PACK_H" in such a case, though.

I was trying to convey that this is the C API to call fetch-pack directly, 
rather than something used by the builtin, or by the wrapper for calling 
the builtin. But the inclusion guard is probably not going to be noticed 
anyway, and I don't think it's worth making the header filename verbose.

> Nobody seems to call setup_fetch_pack() yet.  How complete is
> this patch meant to be?

It's part of a series that leads up to making fetch a builtin. I'm trying 
to get in bits that are bounded by logical APIs. The roadmap here is that 
transport.{c,h} from one of my other patches will get a function to fetch 
a set of refs, and it will (for a suitable URL format) call 
setup_fetch_pack() with the appropriate options and then call 
fetch_pack(). builtin-fetch will use this function to actually get objects 
once it has determined which ones it should get.

I think I've now got the whole series to a point where everything's 
submittable, if you'd like to see the whole thing. It's actually composed 
of 6 initial independant sub-series (mostly single patches) of which I've 
submitted 4 (three today and the one that modularizes the commit-walker 
infrastructure and removes the obsolete ones), and a final series of 3 
that implements fetch on top of the rest. How should I number this?

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Theodore Tso]

On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> Are _identifiers with leading underscore Kosher thing to do, I
> wonder...  We do have ones with trailing ones (mostly qsort
> functions) and I think they are done that way for the sake of
> standards conformance.

_[a-z]* is kosher for file scopes or function scoping:

	http://c-faq.com/decl/namespace.html

Some Projects' coding styles prohibit use of _[a-z]* altogether, as
_[_A-Z]* is reserved for C libraries and system header files, and it's
simpler just to ban all identifiers begining with an underscore just
on general princinpals.  It's not strictly speaking necessary, however.

	       		     	    - Ted

P.S.  Note that the Linux kernel does not worry about such rules; in
particular, _[a-z]* is very commonly used at the global scope level,
without any problems.  It doesn't have to be as portable as git
probably needs to be, however.

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Andy Parkins]

On Monday 2007 July 09, Theodore Tso wrote:
> On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> > Are _identifiers with leading underscore Kosher thing to do, I
> > wonder...  We do have ones with trailing ones (mostly qsort
> > functions) and I think they are done that way for the sake of
> > standards conformance.
>
> _[a-z]* is kosher for file scopes or function scoping:

Perhaps I'm reading it wrong but:

"All identifiers beginning with an underscore are reserved for ordinary 
identifiers (functions, variables, typedefs, enumeration constants) with file 
scope."

Doesn't agree with what you've said.  I think that you _can_ use _[a-z]* for 
labels or structure members - however, not within file or function scope.

However, the rule of thumb I've always used is "don't start identifiers with 
underscore".  I can't think of a situation that would mean you have to use an 
underscore to start an identifier - so why get into detailed worries about 
where it's allowed and where it isn't.  Just don't use it.  The document you 
linked to gives exactly this advice:

"Don't give anything a name with a leading underscore."

> P.S.  Note that the Linux kernel does not worry about such rules; in
> particular, _[a-z]* is very commonly used at the global scope level,
> without any problems.  It doesn't have to be as portable as git
> probably needs to be, however.

The kernel doesn't use any standard library so there is nothing for it to 
conflict with.  I don't think it's that the kernel doesn't worry about those 
rules, it's that those rules don't apply (just as they don't for glibc).  The 
underscore rules aren't a limitation of the language, they are a limitation 
imposed by the standard library.  



Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Theodore Tso]

On Mon, Jul 09, 2007 at 02:16:35PM +0100, Andy Parkins wrote:
> On Monday 2007 July 09, Theodore Tso wrote:
> > On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> > > Are _identifiers with leading underscore Kosher thing to do, I
> > > wonder...  We do have ones with trailing ones (mostly qsort
> > > functions) and I think they are done that way for the sake of
> > > standards conformance.
> >
> > _[a-z]* is kosher for file scopes or function scoping:
> 
> Perhaps I'm reading it wrong but:
> 
> "All identifiers beginning with an underscore are reserved for ordinary 
> identifiers (functions, variables, typedefs, enumeration constants) with file 
                                                                      ^^^^^^^^^
> scope."
  ^^^^^^
> 
> Doesn't agree with what you've said.  I think that you _can_ use _[a-z]* for 
> labels or structure members - however, not within file or function scope.

I think the above does agree with what I said.  It says that you can
use functions, variables, typdefs, enumeration constants (not just
labels or structure members) WITH FILE SCOPE.  I.e., so long as it
doesn't leak across a .o linkage.  So one .o file can use a static
_my_strdup, and another .o file can use a static _my_strdup, and they
don't have to worry about multiply defined function conflicts, since
they are static functions with file or smaller scoping.

And if it's safe to use a file-level static scoping, then obviously it
would be safe to use a function-level static scoping.

> However, the rule of thumb I've always used is "don't start identifiers with 
> underscore".  I can't think of a situation that would mean you have to use an 
> underscore to start an identifier - so why get into detailed worries about 
> where it's allowed and where it isn't.  Just don't use it.  The document you 
> linked to gives exactly this advice:

Yep, this is the safer thing to do if you don't want to remember the
more complicated rule.  But it's not *necessary*; no system library
will use a single underscore followed by a lower-case letter, since
that's reserved for programs for local file-level scoping.  A system
library will use for its private function identifiers that begin
either a double underscore, or a underscore followed by an uppercase
latter.

Regards,

							- Ted

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Florian Weimer]

* Theodore Tso:

> On Mon, Jul 09, 2007 at 02:16:35PM +0100, Andy Parkins wrote:

>> Perhaps I'm reading it wrong but:
>> 
>> "All identifiers beginning with an underscore are reserved for ordinary 
>> identifiers (functions, variables, typedefs, enumeration constants) with file 
>                                                                       ^^^^^^^^^
>> scope."
>   ^^^^^^
>> 
>> Doesn't agree with what you've said.  I think that you _can_ use _[a-z]* for 
>> labels or structure members - however, not within file or function scope.
>
> I think the above does agree with what I said.  It says that you can
> use functions, variables, typdefs, enumeration constants (not just
> labels or structure members) WITH FILE SCOPE.

No, they are are reserved.

> I.e., so long as it doesn't leak across a .o linkage.  So one .o
> file can use a static _my_strdup, and another .o file can use a
> static _my_strdup, and they don't have to worry about multiply
> defined function conflicts, since they are static functions with
> file or smaller scoping.

This is not sufficient because the implementation might have a
*typedef* for _my_strdup in any header file, and neither translation
unit would compile.  Or they could have been declared with a different
prototype.

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Andy Parkins]

On Monday 2007 July 09, Theodore Tso wrote:
> > "All identifiers beginning with an underscore are reserved for ordinary
> > identifiers (functions, variables, typedefs, enumeration constants) with
> > file
> > scope."

> I think the above does agree with what I said.  It says that you can
> use functions, variables, typdefs, enumeration constants (not just
> labels or structure members) WITH FILE SCOPE.  I.e., so long as it
> doesn't leak across a .o linkage.  So one .o file can use a static

I'm reading it as meaning they are reserved at file scope; not that you can 
use them at file scope.

> _my_strdup, and another .o file can use a static _my_strdup, and they
> don't have to worry about multiply defined function conflicts, since
> they are static functions with file or smaller scoping.

Erm, but we're not talking about your own .o files we're talking about 
conflicting with the library; what you say would be true for any identifier.  
We have no way of guaranteeing that _my_strdup() isn't defined by one of the 
standard library headers that have been included.  The standard header is 
entitled to use underscore identifiers because they have been reserved at 
file scope.

Reading a little further into the FAQ you posted, I found the following in the 
list of exceptions:

"You may use identifiers consisting of an underscore followed by a digit or 
lower case letter for labels and structure/union members." 
and
"You may use identifiers consisting of an underscore followed by a digit or 
lower case letter at function, block, or prototype scope."

I'm more sure now - you can't use underscore identifiers at file scope.

Regardless, we're just splitting hairs now.  We seem to both agree that it's 
easiest just to outright not use underscore-prefixed identifiers; so I'm 
happy. :-)




Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[René Scharfe]

Daniel Barkalow schrieb:
> On Sun, 8 Jul 2007, Junio C Hamano wrote:
> 
>> Are _identifiers with leading underscore Kosher thing to do, I
>> wonder...  We do have ones with trailing ones (mostly qsort
>> functions) and I think they are done that way for the sake of
>> standards conformance.
> 
> I'm not sure; I inherited that bit of code from Julian. Do we have a 
> standard idiom for a function that sets a bunch of static variables?

You could put all flags and settings into a struct fetch_pack_args.

You could then go one step further by exporting the struct definition
and letting the internal interface take a pointer to such a struct,
thereby getting rid of static variables and their setup routine.

René

Re: [PATCH v2] Make fetch-pack a builtin with an internal API

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1394 bytes --]

On Mon, 9 Jul 2007, René Scharfe wrote:

> Daniel Barkalow schrieb:
> > On Sun, 8 Jul 2007, Junio C Hamano wrote:
> > 
> >> Are _identifiers with leading underscore Kosher thing to do, I
> >> wonder...  We do have ones with trailing ones (mostly qsort
> >> functions) and I think they are done that way for the sake of
> >> standards conformance.
> > 
> > I'm not sure; I inherited that bit of code from Julian. Do we have a 
> > standard idiom for a function that sets a bunch of static variables?
> 
> You could put all flags and settings into a struct fetch_pack_args.
> 
> You could then go one step further by exporting the struct definition
> and letting the internal interface take a pointer to such a struct,
> thereby getting rid of static variables and their setup routine.

This code is going to get an extensive refactor at some point (I'd like to 
get to the point where git-fetch uses only a single connection), but 
that's a much bigger patch, and I want to get builtin-fetch done with 
minimal changes to other code; passing configuration through to everywhere 
that uses it would add a lot of noise at this stage.

Passing a pointer to a struct instead of a pile of ints in hopefully the 
right order is probably a worthwhile improvement to reviewability, though, 
so I'll try that much this evening.

	-Daniel
*This .sig left intentionally blank*