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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 5, 2008, 00:00 UTC
Message-ID: <alpine.DEB.1.00.0806050052390.21190@racer>
In-Reply-To: <1212612947-34720-1-git-send-email-pdebie@ai.rug.nl>

Hi,

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

> +static void export_marks(char * file)

Extra space after star.

Show 14 quoted lines

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

Show 21 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 8 quoted lines

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

Unnecessary change.

Other than that: ACK.

Thanks, Dscho