* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-29 19:16 ` Junio C Hamano
@ 2014-10-30 18:07 ` Junio C Hamano
2014-10-30 21:25 ` Jeff King
2014-10-30 18:08 ` [PATCH] bundle: split out a helper function to compute and write prerequisites Junio C Hamano
2014-10-30 21:35 ` [PATCH] use child_process_init() to initialize struct child_process variables Jeff King
2 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2014-10-30 18:07 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git Mailing List
Junio C Hamano <gitster@pobox.com> writes:
> Probably three helper functions:
>
> - The first is to find tops and bottoms (this translates fuzzy
> specifications such as "--since 30.days" into a more concrete
> revision range "^A ^B ... Z" to establish bundle prerequisites),
> which is done by running a "rev-list --boundary".
>
> - The second is to show refs, while paying attention to things like
> "--10 maint master" which may result in the tip of 'maint' not
> being shown at all. I am not sure if this part can/should take
> advantage of revs.cmdline, though.
>
> - The last is to create the actual pack data.
>
> I agree with your analysis on the change in column.c and trailer.c
>
> Thanks.
So here are a few patches on top of René's change. This is the
third point in the above list.
-- >8 --
Subject: [PATCH] bundle: split out a helper function to create a pack data
The create_bundle() function, while it does one single logical thing
and tries to do it well, that single logical thing takes a rather
large implementation.
Let's start separating what it does into smaller steps to make it
easier what is going on. This is a first step to separate out the
actual pack-data generation, after the earlier part of the function
figures out which part of the history to place in the bundle.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
bundle.c | 64 +++++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/bundle.c b/bundle.c
index c846092..9c87532 100644
--- a/bundle.c
+++ b/bundle.c
@@ -235,6 +235,41 @@ out:
return result;
}
+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs)
+{
+ struct child_process pack_objects = CHILD_PROCESS_INIT;
+ int i;
+
+ argv_array_pushl(&pack_objects.args,
+ "pack-objects", "--all-progress-implied",
+ "--stdout", "--thin", "--delta-base-offset",
+ NULL);
+ pack_objects.in = -1;
+ pack_objects.out = bundle_fd;
+ pack_objects.git_cmd = 1;
+ if (start_command(&pack_objects))
+ return error(_("Could not spawn pack-objects"));
+
+ /*
+ * start_command closed bundle_fd if it was > 1
+ * so set the lock fd to -1 so commit_lock_file()
+ * won't fail trying to close it.
+ */
+ lock->fd = -1;
+
+ for (i = 0; i < revs->pending.nr; i++) {
+ struct object *object = revs->pending.objects[i].item;
+ if (object->flags & UNINTERESTING)
+ write_or_die(pack_objects.in, "^", 1);
+ write_or_die(pack_objects.in, sha1_to_hex(object->sha1), 40);
+ write_or_die(pack_objects.in, "\n", 1);
+ }
+ close(pack_objects.in);
+ if (finish_command(&pack_objects))
+ return error(_("pack-objects died"));
+ return 0;
+}
+
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
@@ -381,34 +416,9 @@ int create_bundle(struct bundle_header *header, const char *path,
write_or_die(bundle_fd, "\n", 1);
/* write pack */
- child_process_init(&rls);
- argv_array_pushl(&rls.args,
- "pack-objects", "--all-progress-implied",
- "--stdout", "--thin", "--delta-base-offset",
- NULL);
- rls.in = -1;
- rls.out = bundle_fd;
- rls.git_cmd = 1;
- if (start_command(&rls))
- return error(_("Could not spawn pack-objects"));
-
- /*
- * start_command closed bundle_fd if it was > 1
- * so set the lock fd to -1 so commit_lock_file()
- * won't fail trying to close it.
- */
- lock.fd = -1;
+ if (write_pack_data(bundle_fd, &lock, &revs))
+ return -1;
- for (i = 0; i < revs.pending.nr; i++) {
- struct object *object = revs.pending.objects[i].item;
- if (object->flags & UNINTERESTING)
- write_or_die(rls.in, "^", 1);
- write_or_die(rls.in, sha1_to_hex(object->sha1), 40);
- write_or_die(rls.in, "\n", 1);
- }
- close(rls.in);
- if (finish_command(&rls))
- return error(_("pack-objects died"));
if (!bundle_to_stdout) {
if (commit_lock_file(&lock))
die_errno(_("cannot create '%s'"), path);
--
2.1.3-612-g493e79e
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-30 18:07 ` Junio C Hamano
@ 2014-10-30 21:25 ` Jeff King
0 siblings, 0 replies; 25+ messages in thread
From: Jeff King @ 2014-10-30 21:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git Mailing List
On Thu, Oct 30, 2014 at 11:07:39AM -0700, Junio C Hamano wrote:
> -- >8 --
> Subject: [PATCH] bundle: split out a helper function to create a pack data
s/a pack data/pack data/
> The create_bundle() function, while it does one single logical thing
> and tries to do it well, that single logical thing takes a rather
> large implementation.
I had minor trouble parsing this. I think it might be more clearly said
as just:
The create_bundle() function, while it does one single logical thing,
takes a rather large implementation to do so.
> Let's start separating what it does into smaller steps to make it
> easier what is going on. This is a first step to separate out the
s/easier/& to see/
> bundle.c | 64 +++++++++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 37 insertions(+), 27 deletions(-)
The patch itself looked OK to me.
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] bundle: split out a helper function to compute and write prerequisites
2014-10-29 19:16 ` Junio C Hamano
2014-10-30 18:07 ` Junio C Hamano
@ 2014-10-30 18:08 ` Junio C Hamano
2014-10-30 21:26 ` Jeff King
2014-10-30 21:35 ` [PATCH] use child_process_init() to initialize struct child_process variables Jeff King
2 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2014-10-30 18:08 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git Mailing List
The new helper compute_and_write_prerequistes() is ugly, but it
cannot be avoided. Ideally we should avoid a function that computes
and does I/O at the same time, but the prerequisites lines in the
output needs the human readable title only to help the recipient of
the bundle. The code copies them straight from the rev-list output
and immediately discards as no other internal computation needs that
information.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* And this is to address the first one in the three-bullet list.
bundle.c | 59 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/bundle.c b/bundle.c
index 9c87532..0ca8737 100644
--- a/bundle.c
+++ b/bundle.c
@@ -270,33 +270,15 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf
return 0;
}
-int create_bundle(struct bundle_header *header, const char *path,
- int argc, const char **argv)
+static int compute_and_write_prerequistes(int bundle_fd,
+ struct rev_info *revs,
+ int argc, const char **argv)
{
- static struct lock_file lock;
- int bundle_fd = -1;
- int bundle_to_stdout;
- int i, ref_count = 0;
- struct strbuf buf = STRBUF_INIT;
- struct rev_info revs;
struct child_process rls = CHILD_PROCESS_INIT;
+ struct strbuf buf = STRBUF_INIT;
FILE *rls_fout;
+ int i;
- bundle_to_stdout = !strcmp(path, "-");
- if (bundle_to_stdout)
- bundle_fd = 1;
- else
- bundle_fd = hold_lock_file_for_update(&lock, path,
- LOCK_DIE_ON_ERROR);
-
- /* write signature */
- write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
-
- /* init revs to list objects for pack-objects later */
- save_commit_buffer = 0;
- init_revisions(&revs, NULL);
-
- /* write prerequisites */
argv_array_pushl(&rls.args,
"rev-list", "--boundary", "--pretty=oneline",
NULL);
@@ -314,7 +296,7 @@ int create_bundle(struct bundle_header *header, const char *path,
if (!get_sha1_hex(buf.buf + 1, sha1)) {
struct object *object = parse_object_or_die(sha1, buf.buf);
object->flags |= UNINTERESTING;
- add_pending_object(&revs, object, buf.buf);
+ add_pending_object(revs, object, buf.buf);
}
} else if (!get_sha1_hex(buf.buf, sha1)) {
struct object *object = parse_object_or_die(sha1, buf.buf);
@@ -325,6 +307,35 @@ int create_bundle(struct bundle_header *header, const char *path,
fclose(rls_fout);
if (finish_command(&rls))
return error(_("rev-list died"));
+ return 0;
+}
+
+int create_bundle(struct bundle_header *header, const char *path,
+ int argc, const char **argv)
+{
+ static struct lock_file lock;
+ int bundle_fd = -1;
+ int bundle_to_stdout;
+ int i, ref_count = 0;
+ struct rev_info revs;
+
+ bundle_to_stdout = !strcmp(path, "-");
+ if (bundle_to_stdout)
+ bundle_fd = 1;
+ else
+ bundle_fd = hold_lock_file_for_update(&lock, path,
+ LOCK_DIE_ON_ERROR);
+
+ /* write signature */
+ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
+
+ /* init revs to list objects for pack-objects later */
+ save_commit_buffer = 0;
+ init_revisions(&revs, NULL);
+
+ /* write prerequisites */
+ if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))
+ return -1;
/* write references */
argc = setup_revisions(argc, argv, &revs, NULL);
--
2.1.3-612-g493e79e
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-29 19:16 ` Junio C Hamano
2014-10-30 18:07 ` Junio C Hamano
2014-10-30 18:08 ` [PATCH] bundle: split out a helper function to compute and write prerequisites Junio C Hamano
@ 2014-10-30 21:35 ` Jeff King
2014-10-31 0:19 ` Philip Oakley
2 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2014-10-30 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git Mailing List
On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:
> Probably three helper functions:
>
> - The first is to find tops and bottoms (this translates fuzzy
> specifications such as "--since 30.days" into a more concrete
> revision range "^A ^B ... Z" to establish bundle prerequisites),
> which is done by running a "rev-list --boundary".
>
> - The second is to show refs, while paying attention to things like
> "--10 maint master" which may result in the tip of 'maint' not
> being shown at all. I am not sure if this part can/should take
> advantage of revs.cmdline, though.
>
> - The last is to create the actual pack data.
>
> I agree with your analysis on the change in column.c and trailer.c
I was not planning to work on this, but since you did the first and
third bullet points, I think it makes sense to start the second one with
this cleanup:
-- >8 --
Subject: bundle: split out ref writing from bundle_create
The bundle_create() function has a number of logical steps:
process the input, write the refs, and write the packfile.
Recent commits split the first and third into separate
sub-functions. It's worth splitting the middle step out,
too, if only because it makes the progression of the steps
more obvious.
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously this should be dropped if somebody is actively working on the
revs.cmdline thing you mentioned, as it would conflict horribly. But I
think it is a nice step for somebody working on it later, because the
revs.cmdline changes should be isolated to write_bundle_refs.
bundle.c | 97 ++++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 58 insertions(+), 39 deletions(-)
diff --git a/bundle.c b/bundle.c
index 0ca8737..ca4803b 100644
--- a/bundle.c
+++ b/bundle.c
@@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int bundle_fd,
return 0;
}
-int create_bundle(struct bundle_header *header, const char *path,
- int argc, const char **argv)
+/*
+ * Write out bundle refs based on the tips already
+ * parsed into revs.pending. As a side effect, may
+ * manipulate revs.pending to include additional
+ * necessary objects (like tags).
+ *
+ * Returns the number of refs written, or negative
+ * on error.
+ */
+static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
{
- static struct lock_file lock;
- int bundle_fd = -1;
- int bundle_to_stdout;
- int i, ref_count = 0;
- struct rev_info revs;
-
- bundle_to_stdout = !strcmp(path, "-");
- if (bundle_to_stdout)
- bundle_fd = 1;
- else
- bundle_fd = hold_lock_file_for_update(&lock, path,
- LOCK_DIE_ON_ERROR);
-
- /* write signature */
- write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
-
- /* init revs to list objects for pack-objects later */
- save_commit_buffer = 0;
- init_revisions(&revs, NULL);
-
- /* write prerequisites */
- if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))
- return -1;
-
- /* write references */
- argc = setup_revisions(argc, argv, &revs, NULL);
-
- if (argc > 1)
- return error(_("unrecognized argument: %s"), argv[1]);
-
- object_array_remove_duplicates(&revs.pending);
+ int i;
+ int ref_count = 0;
- for (i = 0; i < revs.pending.nr; i++) {
- struct object_array_entry *e = revs.pending.objects + i;
+ for (i = 0; i < revs->pending.nr; i++) {
+ struct object_array_entry *e = revs->pending.objects + i;
unsigned char sha1[20];
char *ref;
const char *display_ref;
@@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header, const char *path,
display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
if (e->item->type == OBJ_TAG &&
- !is_tag_in_date_range(e->item, &revs)) {
+ !is_tag_in_date_range(e->item, revs)) {
e->item->flags |= UNINTERESTING;
continue;
}
@@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header, const char *path,
*/
obj = parse_object_or_die(sha1, e->name);
obj->flags |= SHOWN;
- add_pending_object(&revs, obj, e->name);
+ add_pending_object(revs, obj, e->name);
}
free(ref);
continue;
@@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header, const char *path,
write_or_die(bundle_fd, "\n", 1);
free(ref);
}
- if (!ref_count)
- die(_("Refusing to create empty bundle."));
/* end header */
write_or_die(bundle_fd, "\n", 1);
+ return ref_count;
+}
+
+int create_bundle(struct bundle_header *header, const char *path,
+ int argc, const char **argv)
+{
+ static struct lock_file lock;
+ int bundle_fd = -1;
+ int bundle_to_stdout;
+ int ref_count = 0;
+ struct rev_info revs;
+
+ bundle_to_stdout = !strcmp(path, "-");
+ if (bundle_to_stdout)
+ bundle_fd = 1;
+ else
+ bundle_fd = hold_lock_file_for_update(&lock, path,
+ LOCK_DIE_ON_ERROR);
+
+ /* write signature */
+ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
+
+ /* init revs to list objects for pack-objects later */
+ save_commit_buffer = 0;
+ init_revisions(&revs, NULL);
+
+ /* write prerequisites */
+ if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))
+ return -1;
+
+ argc = setup_revisions(argc, argv, &revs, NULL);
+
+ if (argc > 1)
+ return error(_("unrecognized argument: %s"), argv[1]);
+
+ object_array_remove_duplicates(&revs.pending);
+
+ ref_count = write_bundle_refs(bundle_fd, &revs);
+ if (!ref_count)
+ die(_("Refusing to create empty bundle."));
+ else if (ref_count < 0)
+ return -1;
/* write pack */
if (write_pack_data(bundle_fd, &lock, &revs))
--
2.1.2.596.g7379948
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-30 21:35 ` [PATCH] use child_process_init() to initialize struct child_process variables Jeff King
@ 2014-10-31 0:19 ` Philip Oakley
2014-10-31 21:48 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Philip Oakley @ 2014-10-31 0:19 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: René Scharfe, Git Mailing List
From: "Jeff King" <peff@peff.net>
> On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:
>
>> Probably three helper functions:
>>
>> - The first is to find tops and bottoms (this translates fuzzy
>> specifications such as "--since 30.days" into a more concrete
>> revision range "^A ^B ... Z" to establish bundle prerequisites),
>> which is done by running a "rev-list --boundary".
>>
>> - The second is to show refs, while paying attention to things like
>> "--10 maint master" which may result in the tip of 'maint' not
>> being shown at all. I am not sure if this part can/should take
>> advantage of revs.cmdline, though.
>>
>> - The last is to create the actual pack data.
>>
>> I agree with your analysis on the change in column.c and trailer.c
>
> I was not planning to work on this, but since you did the first and
> third bullet points, I think it makes sense to start the second one
> with
> this cleanup:
>
> -- >8 --
> Subject: bundle: split out ref writing from bundle_create
>
> The bundle_create() function has a number of logical steps:
> process the input, write the refs, and write the packfile.
> Recent commits split the first and third into separate
> sub-functions. It's worth splitting the middle step out,
> too, if only because it makes the progression of the steps
> more obvious.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Obviously this should be dropped if somebody is actively working on
> the
> revs.cmdline thing you mentioned, as it would conflict horribly. But I
> think it is a nice step for somebody working on it later, because the
> revs.cmdline changes should be isolated to write_bundle_refs.
As a side project (slow time) I've been looking at the loss of the HEAD
symbolic ref when multiple heads are bundled that point at the same rev.
That is, when the HEAD detection heuristic fails.
What I've noticed so far is that a duplicated ref (added to the bundle
manually) simply creates a warning (currently, and the warning names
that ref) rather than failing when cloned (and by implication fetched).
Not only that the bundle verify command does not report any error for
such a bundle containing a duplicate ref.
Given that, if the refs were sorted, and HEAD was listed last (as is the
case with '--all'), then one could add the duplicate ref immediately
after HEAD, of it's symbolic ref. I.e if a duplicate ref is found after
HEAD then that ref is the true HEAD ref. This duplicate ref would only
need to be present if there are multiple (two or more) heads that point
to the same rev, and the HEAD isn't detatched. Sorting is necessary to
ensure that HEAD is last and its duplicate refs/head ref immediately
follows.
Thus the first step is to ensure that the positive refs list is sorted
such that HEAD (and it's ilk) is last.
I'd also planned an option '--HEAD' which would add such a duplicate ref
to a V2 bundle (resulting in a warning for older users which displays
the duplicate head ref!)
An option '--V3' would be the same as '--HEAD' but would also change the
bundle string version number (to V3), and so would not be acceptable to
older systems (for those that require such separation) but the
clone/fetch on a newer system would detect the V3 in the header string
and then use the extra ref rather than the heuristic to determine HEAD
(with appropriate checks).
I hadn't finished my studies of the refs.c code to fully understand what
I'd need to change, but hopefully the changes in this patch can be
aligned in the same direction (or the errors in my reasoning be pointed
out;-)
The need to sort the refs in this method would separate the
determination of the correct refs from the writing of the refs. All
assuming the idea has merit...
--
Philip
>
> bundle.c | 97
> ++++++++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 58 insertions(+), 39 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 0ca8737..ca4803b 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int
> bundle_fd,
> return 0;
> }
>
> -int create_bundle(struct bundle_header *header, const char *path,
> - int argc, const char **argv)
> +/*
> + * Write out bundle refs based on the tips already
> + * parsed into revs.pending. As a side effect, may
> + * manipulate revs.pending to include additional
> + * necessary objects (like tags).
> + *
> + * Returns the number of refs written, or negative
> + * on error.
> + */
> +static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
> {
> - static struct lock_file lock;
> - int bundle_fd = -1;
> - int bundle_to_stdout;
> - int i, ref_count = 0;
> - struct rev_info revs;
> -
> - bundle_to_stdout = !strcmp(path, "-");
> - if (bundle_to_stdout)
> - bundle_fd = 1;
> - else
> - bundle_fd = hold_lock_file_for_update(&lock, path,
> - LOCK_DIE_ON_ERROR);
> -
> - /* write signature */
> - write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
> -
> - /* init revs to list objects for pack-objects later */
> - save_commit_buffer = 0;
> - init_revisions(&revs, NULL);
> -
> - /* write prerequisites */
> - if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))
> - return -1;
> -
> - /* write references */
> - argc = setup_revisions(argc, argv, &revs, NULL);
> -
> - if (argc > 1)
> - return error(_("unrecognized argument: %s"), argv[1]);
> -
> - object_array_remove_duplicates(&revs.pending);
> + int i;
> + int ref_count = 0;
>
> - for (i = 0; i < revs.pending.nr; i++) {
> - struct object_array_entry *e = revs.pending.objects + i;
> + for (i = 0; i < revs->pending.nr; i++) {
> + struct object_array_entry *e = revs->pending.objects + i;
> unsigned char sha1[20];
> char *ref;
> const char *display_ref;
> @@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header,
> const char *path,
> display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
>
> if (e->item->type == OBJ_TAG &&
> - !is_tag_in_date_range(e->item, &revs)) {
> + !is_tag_in_date_range(e->item, revs)) {
> e->item->flags |= UNINTERESTING;
> continue;
> }
> @@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header,
> const char *path,
> */
> obj = parse_object_or_die(sha1, e->name);
> obj->flags |= SHOWN;
> - add_pending_object(&revs, obj, e->name);
> + add_pending_object(revs, obj, e->name);
> }
> free(ref);
> continue;
> @@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header,
> const char *path,
> write_or_die(bundle_fd, "\n", 1);
> free(ref);
> }
> - if (!ref_count)
> - die(_("Refusing to create empty bundle."));
>
> /* end header */
> write_or_die(bundle_fd, "\n", 1);
> + return ref_count;
> +}
> +
> +int create_bundle(struct bundle_header *header, const char *path,
> + int argc, const char **argv)
> +{
> + static struct lock_file lock;
> + int bundle_fd = -1;
> + int bundle_to_stdout;
> + int ref_count = 0;
> + struct rev_info revs;
> +
> + bundle_to_stdout = !strcmp(path, "-");
> + if (bundle_to_stdout)
> + bundle_fd = 1;
> + else
> + bundle_fd = hold_lock_file_for_update(&lock, path,
> + LOCK_DIE_ON_ERROR);
> +
> + /* write signature */
> + write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
> +
> + /* init revs to list objects for pack-objects later */
> + save_commit_buffer = 0;
> + init_revisions(&revs, NULL);
> +
> + /* write prerequisites */
> + if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))
> + return -1;
> +
> + argc = setup_revisions(argc, argv, &revs, NULL);
> +
> + if (argc > 1)
> + return error(_("unrecognized argument: %s"), argv[1]);
> +
> + object_array_remove_duplicates(&revs.pending);
> +
> + ref_count = write_bundle_refs(bundle_fd, &revs);
> + if (!ref_count)
> + die(_("Refusing to create empty bundle."));
> + else if (ref_count < 0)
> + return -1;
>
> /* write pack */
> if (write_pack_data(bundle_fd, &lock, &revs))
> --
> 2.1.2.596.g7379948
>
> --
> 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
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-31 0:19 ` Philip Oakley
@ 2014-10-31 21:48 ` Junio C Hamano
2014-11-01 3:33 ` Jeff King
0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2014-10-31 21:48 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeff King, René Scharfe, Git Mailing List
"Philip Oakley" <philipoakley@iee.org> writes:
> As a side project (slow time) I've been looking at the loss of the
> HEAD symbolic ref when multiple heads are bundled that point at the
> same rev. That is, when the HEAD detection heuristic fails.
It think you are talking about the logic used by the "clone", where
- if there is only one branch ref that matches the value of HEAD,
that is the branch;
- if there are more than one refs that match the value of HEAD,
and if one of them is 'master', then that is the branch;
- if there are more than one refs that match the value of HEAD,
and if none of them is 'master', then pick the earliest one.
So you would be in trouble _if_ you have two refs pointing at the
same commit, one of them being 'master', and the current branch is
the other ref. All other cases you shouldn't have to change the
file format and have older client understand which branch is the
current one.
Programs that read a pack data stream unpack-objects were originally
designed to ignore cruft after the pack data stream ends, and
because the bundle file format ends with pack data stream, you
should have been able to append extra information at the end without
breaking older clients. Alas, this principle is still true for
unpack-objects, but index-pack broke it fairly early on, and we use
the latter to deal with bundles, so we cannot just tuck extra info
at the end of an existing bundle. You'd instead need a new option
to create a bundle that cannot be read by existing clients X-<.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-10-31 21:48 ` Junio C Hamano
@ 2014-11-01 3:33 ` Jeff King
2014-11-02 22:54 ` Philip Oakley
0 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2014-11-01 3:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Philip Oakley, René Scharfe, Git Mailing List
On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote:
> Programs that read a pack data stream unpack-objects were originally
> designed to ignore cruft after the pack data stream ends, and
> because the bundle file format ends with pack data stream, you
> should have been able to append extra information at the end without
> breaking older clients. Alas, this principle is still true for
> unpack-objects, but index-pack broke it fairly early on, and we use
> the latter to deal with bundles, so we cannot just tuck extra info
> at the end of an existing bundle. You'd instead need a new option
> to create a bundle that cannot be read by existing clients X-<.
I think you could use a similar NUL-trick to what we do in the online
protocol, and have a ref section like:
...sha1... refs/heads/master
...sha1... refs/heads/confused-with-master
...sha1... HEAD\0symref=refs/heads/master
The current parser reads into a strbuf up to the newline, but we ignore
everything after the NUL, treating it like a C string. Prior to using
strbufs, we used fgets, which behaves similarly (you could not know from
fgets that there is extra data after the NUL, but that is OK; we only
want older versions to ignore the data, not do anything useful with it).
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-01 3:33 ` Jeff King
@ 2014-11-02 22:54 ` Philip Oakley
2014-11-03 18:26 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Philip Oakley @ 2014-11-02 22:54 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: René Scharfe, Git Mailing List
From: "Jeff King" <peff@peff.net>
> On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote:
>
>> Programs that read a pack data stream unpack-objects were originally
>> designed to ignore cruft after the pack data stream ends, and
>> because the bundle file format ends with pack data stream, you
>> should have been able to append extra information at the end without
>> breaking older clients.
It's an option, I'd been looking at sneaking the information into the
refs header section.
>> Alas, this principle is still true for
>> unpack-objects, but index-pack broke it fairly early on, and we use
>> the latter to deal with bundles, so we cannot just tuck extra info
>> at the end of an existing bundle. You'd instead need a new option
>> to create a bundle that cannot be read by existing clients X-<.
>
> I think you could use a similar NUL-trick to what we do in the online
I like this 'trick'. I'd not appreciated the use of the null separator
for breaking a line into separate strings that way before (I'd
understood it, just never appreciated it!).
> protocol, and have a ref section like:
>
> ...sha1... refs/heads/master
> ...sha1... refs/heads/confused-with-master
> ...sha1... HEAD\0symref=refs/heads/master
>
> The current parser reads into a strbuf up to the newline, but we
> ignore
> everything after the NUL, treating it like a C string. Prior to using
> strbufs, we used fgets, which behaves similarly (you could not know
> from
> fgets that there is extra data after the NUL, but that is OK; we only
> want older versions to ignore the data, not do anything useful with
> it).
>
This certainly looks the way to go. The one extra question would be
whether the symref should be included by default when HEAD is present,
or only if there was possible ambiguity between the other listed refs.
Previously I'd assumed the latter. The former would appear stronger, as
long as the symref was within the listed refs, and excluded otherwise.
Philip
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-02 22:54 ` Philip Oakley
@ 2014-11-03 18:26 ` Junio C Hamano
2014-11-03 22:04 ` Jeff King
0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2014-11-03 18:26 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeff King, René Scharfe, Git Mailing List
"Philip Oakley" <philipoakley@iee.org> writes:
> This certainly looks the way to go. The one extra question would be
> whether the symref should be included by default when HEAD is present,
> or only if there was possible ambiguity between the other listed
> refs.
Just include the "\0symref=..." for any symbolic ref you mention,
and the ref in question does not even have to be "HEAD", I would
say.
The mechanism chosen should be something that will be transparently
ignored by existing implementations, there is no need to make the
data format conditional. If the new implementations of the reading
side want to make a choice between following the new "\0symref=..."
and ignoring it and use the traditional heuristics for some
unknown/unanticipated reason, that should be the choice for the
readers, not for the writers.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-03 18:26 ` Junio C Hamano
@ 2014-11-03 22:04 ` Jeff King
2014-11-03 23:42 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2014-11-03 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Philip Oakley, René Scharfe, Git Mailing List
On Mon, Nov 03, 2014 at 10:26:48AM -0800, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
> > This certainly looks the way to go. The one extra question would be
> > whether the symref should be included by default when HEAD is present,
> > or only if there was possible ambiguity between the other listed
> > refs.
>
> Just include the "\0symref=..." for any symbolic ref you mention,
> and the ref in question does not even have to be "HEAD", I would
> say.
>
> The mechanism chosen should be something that will be transparently
> ignored by existing implementations, there is no need to make the
> data format conditional.
One thing I glossed over in my suggestion of the NUL trick: it works on
git.git, but no clue about elsewhere. I can imagine that other non-C
implementations might treat the whole thing (NUL and extra data
included) as the refname. Back when we did the NUL trick to the online
protocol, git.git was the only serious implementation. But nowadays we
should at least consider the impact on JGit, libgit2, and/or dulwich
(breaking them is not necessarily a showstopper IMHO, but we should at
least know what we are breaking).
I peeked at libgit2 and I think it does not support bundles at all yet,
so that is safe. Grepping for "bundle" in dulwich turns up no hits,
either.
Looks like JGit does support them. I did a very brief test, and it seems
to silently ignore a HEAD ref that has the NUL (I guess maybe it just
rejects it as a malformed refname).
We could make JGit happier either by:
1. Only including the symref magic in ambiguous cases, so that regular
ones Just Work as usual.
2. Including two lines, like:
$sha1 HEAD\0symref=refs/heads/master
$sha1 HEAD
which JGit does the right thing with (and git.git seems to, as
well).
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-03 22:04 ` Jeff King
@ 2014-11-03 23:42 ` Junio C Hamano
2014-11-04 21:56 ` Junio C Hamano
2014-11-05 13:35 ` Philip Oakley
0 siblings, 2 replies; 25+ messages in thread
From: Junio C Hamano @ 2014-11-03 23:42 UTC (permalink / raw)
To: Jeff King; +Cc: Philip Oakley, René Scharfe, Git Mailing List
Jeff King <peff@peff.net> writes:
> I peeked at libgit2 and I think it does not support bundles at all yet,
> so that is safe. Grepping for "bundle" in dulwich turns up no hits,
> either.
>
> Looks like JGit does support them. I did a very brief test, and it seems
> to silently ignore a HEAD ref that has the NUL (I guess maybe it just
> rejects it as a malformed refname).
>
> We could make JGit happier either by:
>
> 1. Only including the symref magic in ambiguous cases, so that regular
> ones Just Work as usual.
>
> 2. Including two lines, like:
>
> $sha1 HEAD\0symref=refs/heads/master
> $sha1 HEAD
>
> which JGit does the right thing with (and git.git seems to, as
> well).
Sounds sensible, even though it looks ugly X-<.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-03 23:42 ` Junio C Hamano
@ 2014-11-04 21:56 ` Junio C Hamano
2014-11-04 23:32 ` Jeff King
2014-11-05 13:35 ` Philip Oakley
1 sibling, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2014-11-04 21:56 UTC (permalink / raw)
To: Jeff King; +Cc: Philip Oakley, René Scharfe, Git Mailing List
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> I peeked at libgit2 and I think it does not support bundles at all yet,
>> so that is safe. Grepping for "bundle" in dulwich turns up no hits,
>> either.
>>
>> Looks like JGit does support them. I did a very brief test, and it seems
>> to silently ignore a HEAD ref that has the NUL (I guess maybe it just
>> rejects it as a malformed refname).
>>
>> We could make JGit happier either by:
>>
>> 1. Only including the symref magic in ambiguous cases, so that regular
>> ones Just Work as usual.
>>
>> 2. Including two lines, like:
>>
>> $sha1 HEAD\0symref=refs/heads/master
>> $sha1 HEAD
>>
>> which JGit does the right thing with (and git.git seems to, as
>> well).
>
> Sounds sensible, even though it looks ugly X-<.
I have a mild preference for a syntax that is more similar to the
on-wire protocol, so that connect.c::parse_feature_value() can be
reused to parse it and possibly annotate_refs_with_symref_info() can
also be reused by calling it from transport.c::get_refs_from_bundle().
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-04 21:56 ` Junio C Hamano
@ 2014-11-04 23:32 ` Jeff King
2014-11-05 0:32 ` Junio C Hamano
2014-11-05 13:41 ` Philip Oakley
0 siblings, 2 replies; 25+ messages in thread
From: Jeff King @ 2014-11-04 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Philip Oakley, René Scharfe, Git Mailing List
On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:
> >> 2. Including two lines, like:
> >>
> >> $sha1 HEAD\0symref=refs/heads/master
> >> $sha1 HEAD
> >>
> >> which JGit does the right thing with (and git.git seems to, as
> >> well).
> >
> > Sounds sensible, even though it looks ugly X-<.
>
> I have a mild preference for a syntax that is more similar to the
> on-wire protocol, so that connect.c::parse_feature_value() can be
> reused to parse it and possibly annotate_refs_with_symref_info() can
> also be reused by calling it from transport.c::get_refs_from_bundle().
Yeah, what I wrote above was the simplest thing that could work, and
does not need to be the final form. I know that you already know what
I'm about to describe below, Junio, but I want to expand on the
situation for the benefit of onlookers (and potential implementers like
Philip).
The online protocol is hampered by the "if you see something after a
NUL, it is a capabilities string, and you must throw out the previous
capabilities string and replace it with this one" historical rule. And
that's why we cannot do:
$sha1 refs/heads/master\0thin-pack side-band etc
$sha1 HEAD\0symref=refs/heads/master
as it would throw out "thin-pack", "side-band", etc. Instead we do it
more like:
$sha1 refs/heads/master\0thin-pack side-band etc symref=HEAD:refs/heads/master
$sha1 HEAD
to shove _all_ of the symref mappings into the capability string, rather
than letting them ride along with their respective refs. The downside is
that we are bounded in the number of symref mappings we can send (by the
maximum length for a single pkt-line), and therefore send only the value
of HEAD.
The bundle code is not bound by this historical legacy, and could do it
in a different (and more efficient and flexible) way. But it is probably
saner to just keep them identical. It makes the code simpler, and having
bundle as the only transport which has the extra flexibility does not
really buy us much (and probably just invites confusion).
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-04 23:32 ` Jeff King
@ 2014-11-05 0:32 ` Junio C Hamano
2014-11-05 13:41 ` Philip Oakley
1 sibling, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2014-11-05 0:32 UTC (permalink / raw)
To: Jeff King; +Cc: Philip Oakley, René Scharfe, Git Mailing List
Jeff King <peff@peff.net> writes:
> The bundle code is not bound by this historical legacy, and could do it
> in a different (and more efficient and flexible) way. But it is probably
> saner to just keep them identical. It makes the code simpler, and having
> bundle as the only transport which has the extra flexibility does not
> really buy us much (and probably just invites confusion).
Yeah, so let's have only symref=HEAD:refs/heads/master for now.
I would like to have the protocol update on the on-wire side during
2015 to lift various limits and correct inefficiencies (the largest
of which is the "who speaks first" issue). We should make sure that
the bundle format can be enhanced to match when it happens.
Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-04 23:32 ` Jeff King
2014-11-05 0:32 ` Junio C Hamano
@ 2014-11-05 13:41 ` Philip Oakley
1 sibling, 0 replies; 25+ messages in thread
From: Philip Oakley @ 2014-11-05 13:41 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: René Scharfe, Git Mailing List
From: "Jeff King" <peff@peff.net>
Subject: Re: [PATCH] use child_process_init() to initialize struct
child_process variables
> On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:
>
>> >> 2. Including two lines, like:
>> >>
>> >> $sha1 HEAD\0symref=refs/heads/master
>> >> $sha1 HEAD
>> >>
>> >> which JGit does the right thing with (and git.git seems to,
>> >> as
>> >> well).
>> >
>> > Sounds sensible, even though it looks ugly X-<.
>>
>> I have a mild preference for a syntax that is more similar to the
>> on-wire protocol, so that connect.c::parse_feature_value() can be
>> reused to parse it and possibly annotate_refs_with_symref_info() can
>> also be reused by calling it from
>> transport.c::get_refs_from_bundle().
>
> Yeah, what I wrote above was the simplest thing that could work, and
> does not need to be the final form. I know that you already know what
> I'm about to describe below, Junio, but I want to expand on the
> situation for the benefit of onlookers (and potential implementers
> like
> Philip).
I think I'm keeping up ;-)
>
> The online protocol is hampered by the "if you see something after a
> NUL, it is a capabilities string, and you must throw out the previous
> capabilities string and replace it with this one" historical rule. And
> that's why we cannot do:
>
> $sha1 refs/heads/master\0thin-pack side-band etc
> $sha1 HEAD\0symref=refs/heads/master
>
> as it would throw out "thin-pack", "side-band", etc. Instead we do it
> more like:
>
> $sha1 refs/heads/master\0thin-pack side-band etc
> symref=HEAD:refs/heads/master
> $sha1 HEAD
>
> to shove _all_ of the symref mappings into the capability string,
> rather
> than letting them ride along with their respective refs. The downside
> is
> that we are bounded in the number of symref mappings we can send (by
> the
> maximum length for a single pkt-line), and therefore send only the
> value
> of HEAD.
>
> The bundle code is not bound by this historical legacy, and could do
> it
> in a different (and more efficient and flexible) way. But it is
> probably
> saner to just keep them identical. It makes the code simpler, and
> having
> bundle as the only transport which has the extra flexibility does not
> really buy us much (and probably just invites confusion).
>
> -Peff
>
Obviously bundles are always off-line, so it's reasonable to be cautious
about using an on-line sideband method, though the re-use of a standard
format is good.
Finding the right parsing method will be important, as well as ensuring
there are no races from the update of unsorted refs.
Philip
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-03 23:42 ` Junio C Hamano
2014-11-04 21:56 ` Junio C Hamano
@ 2014-11-05 13:35 ` Philip Oakley
2014-11-05 19:35 ` Jeff King
1 sibling, 1 reply; 25+ messages in thread
From: Philip Oakley @ 2014-11-05 13:35 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: René Scharfe, Git Mailing List
From: "Junio C Hamano" <gitster@pobox.com>
Sent: Monday, November 03, 2014 11:42 PM
> Jeff King <peff@peff.net> writes:
>
>> I peeked at libgit2 and I think it does not support bundles at all
>> yet,
>> so that is safe. Grepping for "bundle" in dulwich turns up no hits,
>> either.
>>
>> Looks like JGit does support them. I did a very brief test, and it
>> seems
>> to silently ignore a HEAD ref that has the NUL (I guess maybe it just
>> rejects it as a malformed refname).
>>
>> We could make JGit happier either by:
>>
>> 1. Only including the symref magic in ambiguous cases, so that
>> regular
>> ones Just Work as usual.
>>
>> 2. Including two lines, like:
>>
>> $sha1 HEAD\0symref=refs/heads/master
>> $sha1 HEAD
>>
>> which JGit does the right thing with (and git.git seems to, as
>> well).
>
> Sounds sensible, even though it looks ugly X-<.
>
I believe that the 'two HEADs' mechanism would also fall foul of the
'duplicate refs' warning (untested).
Philip
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-05 13:35 ` Philip Oakley
@ 2014-11-05 19:35 ` Jeff King
2014-11-05 23:50 ` Philip Oakley
0 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2014-11-05 19:35 UTC (permalink / raw)
To: Philip Oakley; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
On Wed, Nov 05, 2014 at 01:35:21PM -0000, Philip Oakley wrote:
> >> 2. Including two lines, like:
> [...]
> I believe that the 'two HEADs' mechanism would also fall foul of the
> 'duplicate refs' warning (untested).
It didn't in my very brief testing of what I posted above, but maybe
there is some other case that triggers it that I didn't exercise.
I grepped through the code and the only "duplicate ref" warning I see
comes from the refs.c code, which comes from commit_packed_refs(). If
the duplicate line is HEAD, I think it shouldn't trigger that, as it is
not a regular ref. That would explain why I didn't see it in my testing.
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] use child_process_init() to initialize struct child_process variables
2014-11-05 19:35 ` Jeff King
@ 2014-11-05 23:50 ` Philip Oakley
0 siblings, 0 replies; 25+ messages in thread
From: Philip Oakley @ 2014-11-05 23:50 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
From: "Jeff King" <peff@peff.net>
> On Wed, Nov 05, 2014 at 01:35:21PM -0000, Philip Oakley wrote:
>
>> >> 2. Including two lines, like:
>> [...]
>> I believe that the 'two HEADs' mechanism would also fall foul of the
>> 'duplicate refs' warning (untested).
>
> It didn't in my very brief testing of what I posted above, but maybe
> there is some other case that triggers it that I didn't exercise.
I'd been testing the inclusion of a duplicate of the ref that matched
the HEAD symref (rather than HEAD itself), and had hit that message a
few times, hence my concern.
>
> I grepped through the code and the only "duplicate ref" warning I see
> comes from the refs.c code, which comes from commit_packed_refs().
I had it from is_dup_ref(), also in refs.c, though I may have followed
the call stack incorrectly back to the bundle effects.
> If
> the duplicate line is HEAD, I think it shouldn't trigger that, as it
> is
> not a regular ref. That would explain why I didn't see it in my
> testing.
I've now also done a test and found the same (no warning/error) when
there are two HEADs listed in the bundle preamble. Sorry for the
confusion.
--
Philip
^ permalink raw reply [flat|nested] 25+ messages in thread