[PATCH 2/2] bundle: keep a copy of bundle file...

[PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:08:35

This will be necessary when we start reading from a split bundle
where the header and the thin-pack data live in different files.

The in-core bundle header will read from a file that has the header,
and will record the path to that file.  We would find the name of
the file that hosts the thin-pack data from the header, and we would
take that name as relative to the file we read the header from.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/bundle.c |  5 +++--
 bundle.c         | 21 +++++++++++----------
 bundle.h         |  3 ++-
 transport.c      |  3 ++-
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 4883a43..e63388d 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -36,8 +36,9 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	}
 
 	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
+	header.bundle_file = bundle_file;
+	if (strcmp(cmd, "create") &&
+	    (bundle_fd = read_bundle_header(&header)) < 0)
 		return 1;
 
 	if (!strcmp(cmd, "verify")) {

diff --git a/bundle.c b/bundle.c
index 9c5a6f0..efe19e0 100644
--- a/bundle.c
+++ b/bundle.c

@@ -21,8 +21,7 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
 	list->nr++;
 }
 
-static int parse_bundle_header(int fd, struct bundle_header *header,
-			       const char *report_path)
+static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int status = 0;

@@ -30,9 +29,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	/* The bundle header begins with the signature */
 	if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
 	    strcmp(buf.buf, bundle_signature)) {
-		if (report_path)
+		if (!quiet)
 			error(_("'%s' does not look like a v2 bundle file"),
-			      report_path);
+			      header->bundle_file);
 		status = -1;
 		goto abort;
 	}

@@ -57,7 +56,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 		if (get_sha1_hex(buf.buf, sha1) ||
 		    (buf.len > 40 && !isspace(buf.buf[40])) ||
 		    (!is_prereq && buf.len <= 40)) {
-			if (report_path)
+			if (!quiet)
 				error(_("unrecognized header: %s%s (%d)"),
 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
 			status = -1;

@@ -79,13 +78,13 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	return fd;
 }
 
-int read_bundle_header(const char *path, struct bundle_header *header)
+int read_bundle_header(struct bundle_header *header)
 {
-	int fd = open(path, O_RDONLY);
+	int fd = open(header->bundle_file, O_RDONLY);
 
 	if (fd < 0)
-		return error(_("could not open '%s'"), path);
-	return parse_bundle_header(fd, header, path);
+		return error(_("could not open '%s'"), header->bundle_file);
+	return parse_bundle_header(fd, header, 0);
 }
 
 int is_bundle(const char *path, int quiet)

@@ -96,7 +95,7 @@ int is_bundle(const char *path, int quiet)
 	if (fd < 0)
 		return 0;
 	memset(&header, 0, sizeof(header));
-	fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
+	fd = parse_bundle_header(fd, &header, quiet);
 	if (fd >= 0)
 		close(fd);
 	return (fd >= 0);

@@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)
 	for (i = 0; i < header->references.nr; i++)
 		free(header->references.list[i].name);
 	free(header->references.list);
+
+	free((void *)header->bundle_file);
 }
 
 static int list_refs(struct ref_list *r, int argc, const char **argv)

diff --git a/bundle.h b/bundle.h
index f7ce23b..cf771df 100644
--- a/bundle.h
+++ b/bundle.h

@@ -10,12 +10,13 @@ struct ref_list {
 };
 
 struct bundle_header {
+	const char *bundle_file;
 	struct ref_list prerequisites;
 	struct ref_list references;
 };
 
 int is_bundle(const char *path, int quiet);
-int read_bundle_header(const char *path, struct bundle_header *header);
+int read_bundle_header(struct bundle_header *header);
 int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv);
 int verify_bundle(struct bundle_header *header, int verbose);

diff --git a/transport.c b/transport.c
index 08e15c5..03ce149 100644
--- a/transport.c
+++ b/transport.c

@@ -81,7 +81,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus
 
 	if (data->fd > 0)
 		close(data->fd);
-	data->fd = read_bundle_header(transport->url, &data->header);
+	data->header.bundle_file = xstrdup(transport->url);
+	data->fd = read_bundle_header(&data->header);
 	if (data->fd < 0)
 		die ("Could not read bundle '%s'.", transport->url);
 	for (i = 0; i < data->header.references.nr; i++) {

-- 
2.8.0-rc0-114-g0b3e5e5

Re: [PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header

From: Jeff King <hidden>
Date: 2016-06-15 23:08:35

On Tue, Mar 01, 2016 at 03:36:26PM -0800, Junio C Hamano wrote:

This will be necessary when we start reading from a split bundle
where the header and the thin-pack data live in different files.

The in-core bundle header will read from a file that has the header,
and will record the path to that file.  We would find the name of
the file that hosts the thin-pack data from the header, and we would
take that name as relative to the file we read the header from.

Neat. I'm hoping this means you're working on split bundles. :)

quoted hunk

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 4883a43..e63388d 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -36,8 +36,9 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	}
 
 	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
+	header.bundle_file = bundle_file;

What are the memory ownership rules for header.bundle_file?

Here you assign from either an argv parameter or a stack buffer, and
here...

quoted hunk

@@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)
 	for (i = 0; i < header->references.nr; i++)
 		free(header->references.list[i].name);
 	free(header->references.list);
+
+	free((void *)header->bundle_file);
 }

You free it.

The call in get_refs_from_bundle does do an xstrdup().

Should we have:

  void init_bundle_header(struct bundle_header *header, const char *file)
  {
	memset(header, 0, sizeof(*header));
	header.bundle_file = xstrdup(file);
  }

to abstract the whole procedure?

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help