[PATCH] use child_process_init() to initialize struct child_process variables

[PATCH] use child_process_init() to initialize struct child_process variables

[René Scharfe]

Call child_process_init() instead of zeroing the memory of variables of
type struct child_process by hand before use because the former is both
clearer and shorter.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 bundle.c           | 2 +-
 column.c           | 2 +-
 trailer.c          | 2 +-
 transport-helper.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/bundle.c b/bundle.c
index fa67057..c846092 100644
--- a/bundle.c
+++ b/bundle.c
@@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	write_or_die(bundle_fd, "\n", 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",
diff --git a/column.c b/column.c
index 8082a94..786abe6 100644
--- a/column.c
+++ b/column.c
@@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
 	if (fd_out != -1)
 		return -1;
 
-	memset(&column_process, 0, sizeof(column_process));
+	child_process_init(&column_process);
 	argv = &column_process.args;
 
 	argv_array_push(argv, "column");
diff --git a/trailer.c b/trailer.c
index 8514566..7ff036c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
 		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
 
 	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;
diff --git a/transport-helper.c b/transport-helper.c
index 6cd9dd1..0224687 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport,
 	struct child_process *helper = get_helper(transport);
 	int i;
 
-	memset(fastexport, 0, sizeof(*fastexport));
+	child_process_init(fastexport);
 
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
-- 
2.1.2

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[mike.gorchak.qnx]

Sent from my BlackBerry 10 smartphone on the Rogers network.
  Original Message  
From: René Scharfe
Sent: Tuesday, October 28, 2014 16:59
To: Git Mailing List
Cc: Junio C Hamano
Subject: [PATCH] use child_process_init() to initialize struct child_process variables

Call child_process_init() instead of zeroing the memory of variables of
type struct child_process by hand before use because the former is both
clearer and shorter.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
bundle.c | 2 +-
column.c | 2 +-
trailer.c | 2 +-
transport-helper.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/bundle.c b/bundle.c
index fa67057..c846092 100644
--- a/bundle.c
+++ b/bundle.c
@@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
write_or_die(bundle_fd, "\n", 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",
diff --git a/column.c b/column.c
index 8082a94..786abe6 100644
--- a/column.c
+++ b/column.c
@@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
if (fd_out != -1)
return -1;

-	memset(&column_process, 0, sizeof(column_process));
+	child_process_init(&column_process);
argv = &column_process.args;

argv_array_push(argv, "column");
diff --git a/trailer.c b/trailer.c
index 8514566..7ff036c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);

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;
diff --git a/transport-helper.c b/transport-helper.c
index 6cd9dd1..0224687 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport,
struct child_process *helper = get_helper(transport);
int i;

-	memset(fastexport, 0, sizeof(*fastexport));
+	child_process_init(fastexport);

/* we need to duplicate helper->in because we want to use it after
* fastexport is done with it. */
-- 
2.1.2

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:

> --- a/bundle.c
> +++ b/bundle.c
> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	write_or_die(bundle_fd, "\n", 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.

> diff --git a/column.c b/column.c
> index 8082a94..786abe6 100644
> --- a/column.c
> +++ b/column.c
> @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
>  	if (fd_out != -1)
>  		return -1;
>  
> -	memset(&column_process, 0, sizeof(column_process));
> +	child_process_init(&column_process);
>  	argv = &column_process.args;
>  
>  	argv_array_push(argv, "column");

This one uses a static child_process which needs to be reinitialized on
each run of the function. So it definitely needs child_process_init.

> diff --git a/trailer.c b/trailer.c
> index 8514566..7ff036c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
>  		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
>  
>  	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.

-Peff

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:
>
>> --- a/bundle.c
>> +++ b/bundle.c
>> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
>>  	write_or_die(bundle_fd, "\n", 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?

It has been this way since day one at b1daf300 (Replace
fork_with_pipe in bundle with run_command, 2007-03-12); I agree that
two variables might make things less confusing.

> 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

Thanks.

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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

[PATCH] bundle: split out a helper function to compute and write prerequisites

[Junio C Hamano]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] bundle: split out a helper function to compute and write prerequisites

[Jeff King]

On Thu, Oct 30, 2014 at 11:08:17AM -0700, Junio C Hamano wrote:

> The new helper compute_and_write_prerequistes() is ugly, but it

s/quistes/quisites/

The same typo is in the function name in the code.

-Peff

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Philip Oakley]

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
> 

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Philip Oakley]

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 

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Junio C Hamano]

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.

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Philip Oakley]

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 

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Philip Oakley]

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 

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Philip Oakley]

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

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[René Scharfe]

Am 29.10.2014 um 18:21 schrieb Jeff King:
> On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:
>> diff --git a/trailer.c b/trailer.c
>> index 8514566..7ff036c 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
>>   		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
>>   
>>   	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.

Agreed, thanks.

-- >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 <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 trailer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/trailer.c b/trailer.c
index 7ff036c..6ae7865 100644
--- a/trailer.c
+++ b/trailer.c
@@ -228,7 +228,7 @@ static const char *apply_command(const char *command, const char *arg)
 {
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	struct child_process cp;
+	struct child_process cp = CHILD_PROCESS_INIT;
 	const char *argv[] = {NULL, NULL};
 	const char *result;
 
@@ -237,7 +237,6 @@ static const char *apply_command(const char *command, const char *arg)
 		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
 
 	argv[0] = cmd.buf;
-	child_process_init(&cp);
 	cp.argv = argv;
 	cp.env = local_repo_env;
 	cp.no_stdin = 1;
-- 
2.1.3

Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King]

On Sun, Nov 09, 2014 at 02:49:58PM +0100, René Scharfe wrote:

> -- >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 <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  trailer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Thanks, both this one and the other you just sent (to use
child_process.args in more places) look good to me.

-Peff