Re: [PATCH] builtin-fast-export: Add importing and exporting of revision marks

Hi,

On Wed, 4 Jun 2008, Pieter de Bie wrote:

+static void export_marks(char * file)

Extra space after star.

+{
+	unsigned int i;
+	uintmax_t mark;
+	struct object_decoration *deco = idnums.hash;
+	FILE *f;
+
+	f = fopen(file, "w");
+	if (!f)
+		error("Unable to open marks file %s for writing", file);
+
+	for (i = 0; i < idnums.size; ++i) {
+		deco++;
+		if (deco && deco->base && deco->base->type == 1) {
+			mark = (uint32_t *) deco-> decoration - (uint32_t *)NULL;

Why do you use uint32_t here, when you use uintmax_t to declare "mark"?

Also, there is an extra space after the closing paren.

Is "- (uint32_t *)NULL" needed?

+			fprintf(f, ":%" PRIuMAX " %s\n", mark, sha1_to_hex(deco->base->sha1));

Too long line.

If you already only use uint32_t, I think you do not need the (ugly) 
PRIuMAX.

+static void import_marks(char * input_file)
+{
+	char line[512];
+	FILE *f = fopen(input_file, "r");
+	if (!f)
+		die("cannot read %s: %s", input_file, strerror(errno));
+
+	while (fgets(line, sizeof(line), f)) {
+		uintmax_t mark;
+		char *end;
+		unsigned char sha1[20];
+		struct object *object;
+
+		end = strchr(line, '\n');
+		if (line[0] != ':' || !end)
+			die("corrupt mark line: %s", line);
+		*end = 0;
+		mark = strtoumax(line + 1, &end, 10);
+		if (!mark || end == line + 1
+			|| *end != ' ' || get_sha1(end + 1, sha1))
+			die("corrupt mark line: %s", line);

You do a bit too much with "end" for my liking.  Better use two variables, 
and spare the reader a (brief) "Huh?" moment.

+		object = parse_object(sha1);
+		if (!object)
+			die ("Could not read blob %s", sha1_to_hex(sha1));
+
+		if (object->flags & SHOWN)
+			error("Object %s was already has a mark", sha1);

s/was //

+		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);

Better write (void *)mark.

+	char *export_filename, *import_filename;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    "show progress after <n> objects"),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
 			     "select handling of signed tags",
 			     parse_opt_signed_tag_mode),
+		OPT_STRING(0, "export-marks", &export_filename, "FILE", "Dump marks to this file"),
+		OPT_STRING(0, "import-marks", &import_filename, "FILE", "Import marks from this file"),

Two long lines.

 		OPT_END()
 	};
 
 	/* we handle encodings */
 	git_config(git_default_config, NULL);
 
+
 	init_revisions(&revs, prefix);

Unnecessary change.

Other than that: ACK.

Thanks,
Dscho