From: Pieter de Bie <pdebie@ai.rug.nl>
Date: Thu, 05 Jun 2008 10:46:44 GMT
Subject: Re: [PATCH] builtin-fast-export: Add importing and exporting of revision marks
Message-ID: <BEF1F17D-6F0F-4F09-9CC4-B193B8907901@ai.rug.nl>
In-Reply-To: <alpine.DEB.1.00.0806050052390.21190@racer>


On 5 jun 2008, at 02:00, Johannes Schindelin wrote:

> Hi,
>
> On Wed, 4 Jun 2008, Pieter de Bie wrote:
>
>> +{
>> +	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?

I changed the uintmax_t to to a uint32_t. If I remove the "- (uint32_t  
*)NULL",
it won't return the same marks. The same is done in get_object_mark().

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

Right. I copied this code from fast-export.c. I changed it to two  
variables now.

>> +		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);
>
> Better write (void *)mark.

That won't return the same result, as pointer addition goes with 4  
bytes. The
same thing is done in mark_object().

I will send an updated patch.

- Pieter