Hi,
2nd part of my review:
On Fri, 8 Feb 2008, Jan Holesovsky wrote:
I thought about this function again. It seems we have something similar
in builtin-pack-objects.c, which is easier to read. The equivalent would
be:
static void read_from_stdin(int *num, char ***records)
{
char line[4096];
int alloc = 0;
*num = 0;
*records = NULL;
for (;;) {
if (!fgets(line, sizeof(line), stdin)) {
if (feof(stdin))
break;
if (!ferror(stdin))
die("fgets returned NULL, not EOF, nor error!");
if (errno != EINTR)
die("fgets: %s", strerror(errno));
clearerr(stdin);
continue;
}
if (!line[0])
continue;
ALLOC_GROW(*records, *num + 1, alloc);
(*records)[(*num)++] = xstrdup(line);
}
}
Please have a different option than --shared for lazy clones. Maybe
--lazy? ;-)
I can see why you reused --shared, though. But let's make this more
fool-proof: a user should explicitely ask for a lazy clone.
I might be missing something, but I do not believe this is necessary.
index-pack only works on packs anyway. Am I wrong?
I think it would make sense. For example if you have a local machine
which has most, but maybe not all, of the remote objects.
<bikeshedding>maybe remote-alternates (note the dash instead
of the underscore)</bikeshedding>
Seems that you do something like the read_from_stdin() here, only from a
file. It appears to me as if the function wants to be a library function
(taking a FILE * parameter, and maybe closing it after use, or even
taking a filename parameter, which signifies stdin when NULL).
It'd be probably better to make this an array which uses ALLOC_GROW() in
order to avoid memory fragmentation/allocation overhead.
That is a
return error("Error %d while calling fetch-pack", err);
And it does not really matter what type of error it is: you must report
the error and continue without this object.
The curly brackets are not necessary. Plus, with fill_remote_list() as
you defined it, it will break down with submodules (see 481f0ee6(Fix
rev-list when showing objects involving submodules) for inspiration).
This just cries out loud for a non-recursive approach: have two arrays,
clear the second, fetch the objects in the first array, then fill the
second with the objects referred to by the first array's objects. Then
swap the arrays. Loop.
Maybe it would be nicer to have the has_remote_alternates() check only in
download_remote_sha1()? Same applies to read_sha1_file().
Or
revs.tag_objects = revs.tree_objects = revs.blob_objects
= !commits_only;
Hmm... AFAICT lookup_unknown_object() does not return NULL. It creates a
"none" object if it did not find anything under that sha1.
I think you'd rather want
o = lookup_object(sha1_buf);
- if (!o || !(o->flags & OUR_REF))
+ if (!o || (!exact_objects && !(o->flags & OUR_REF)))
die("git-upload-pack: not our ref %s", line+5);
Puh. What a big patch! But as I said, it is nice to know somebody is
working on this. (I do not necessarily see possibilities to break it
down into smaller chunks, though.)
But I think that your needs can be satisfied with partial shallow clones,
too: e.g.
$ mkdir my-new-workdir
$ cd my-new-workdir
$ git init
$ git remote add -t master origin <url>
$ git fetch --depth 1 origin
$ git checkout -b master origin/master
I cannot think of a proper place to make this a one-shot command.
As you probably know, I am a strong believer in semantics, so I would hate
"git clone" being taught to not clone the whole repository, but only a
single branch.
But hey, I have been wrong before.
Ciao,
Dscho
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
