use child_process_init() to initialize struct child_process variables

Quoted 5 lines · René Scharfe

bundle.c +0 -0

@@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	write_or_die(bundle_fd, "\n", 1);
 

bundle.c int create_bundle(struct bundle_header *header, const char *path, · line 387

Quoted 6 lines

bundle.c +1 -1

 	/* write pack */
-	memset(&rls, 0, sizeof(rls));
+	child_process_init(&rls);
 	argv_array_pushl(&rls.args,
 			 "pack-objects", "--all-progress-implied",
 			 "--stdout", "--thin", "--delta-base-offset",

I wondered if this one could use CHILD_PROCESS_INIT in the declaration instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use the same variable twice for two different child processes in the same function. Besides variable reuse being slightly confusing, the name "rls" (which presumably stands for "rev-list" for the first child) means nothing here, where we are calling "pack-objects". Maybe it would be cleaner to introduce a second variable?

I also suspect the function would be a lot more readable broken into two sub-functions (reading from rev-list and writing to pack-objects), but I did not look closely enough to see whether there were any complicating factors.

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

Quoted 8 lines

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".

Quoted 8 lines

- 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 <[email protected]>

---
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(-)

bundle.c +14 -35

index 0ca8737..ca4803b 100644
@@ -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;

bundle.c +1 -1

@@ -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;

}

bundle.c +1 -1

@@ -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;

bundle.c +42 -2

@@ -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))

Signature · 6 lines

2.1.2.596.g7379948

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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-<.

Quoted 6 lines

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.

Quoted 5 lines

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).

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.

Quoted 4 lines · Philip Oakley

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.

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).

Quoted 16 lines · Jeff King

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:

Quoted 5 lines

$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-<.

Quoted 1 line

Quoted 1 line

2. Including two lines, like:

[...]
I believe that the 'two HEADs' mechanism would also fall foul of the 'duplicate refs' warning (untested).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 15 lines · Jeff King

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:

Quoted 5 lines

$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-<.

Quoted 8 lines

Quoted 7 lines

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).

Quoted 1 line

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().

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

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).

Quoted 6 lines

Quoted 1 line

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

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).

The new helper compute_and_write_prerequistes() is ugly, but it

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.

-- >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

 bundle.c | 64 +++++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 27 deletions(-)

Quoted 7 lines

trailer.c +0 -0

index 8514566..7ff036c 100644
@@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
  		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
 

trailer.c static const char *apply_command(const char *command, const char *arg) · line 243

Quoted 6 lines

trailer.c +1 -1

  	argv[0] = cmd.buf;
-	memset(&cp, 0, sizeof(cp));
+	child_process_init(&cp);
  	cp.argv = argv;
  	cp.env = local_repo_env;
  	cp.no_stdin = 1;

I think this one can use CHILD_PROCESS_INIT in the declaration. I guess it is debatable whether that is actually preferable, but I tend to think it is cleaner and less error-prone.

-- >8 --
Subject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command()

Initialize the struct child_process variable cp at declaration time. This is shorter, saves a function call and prevents using the variable before initialization by mistake.

Suggested-by Jeff King <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 trailer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)