Subject: Re: [PATCH] repack: rewrite the shell script in C.

From: Johannes Sixt <[email-masked]>
Date: Thu, 22 Aug 2013 12:46:50 +0200

Am 21.08.2013 10:49, schrieb Matthieu Moy:
> Stefan Beller <[email-masked]> writes:
>> +	for_each_string_list_item(item, &names) {
>> +		for (ext = 0; ext < 2; ext++) {
>> +			char *fname, *fname_old;
>> +			fname = mkpathdup("%s/%s%s", packdir, item->string, exts[ext]);
>> +			if (!file_exists(fname)) {
>> +				free(fname);
>> +				continue;
>> +			}
>> +
>> +			fname_old = mkpath("%s/old-%s%s", packdir, item->string, exts[ext]);
>> +			if (file_exists(fname_old))
>> +				unlink(fname_old);
>
> Unchecked returned value.

Good catch! The original was 'rm -f ... && mv ... || failed=t'

>> +	/* Now the ones with the same name are out of the way... */
>> +	for_each_string_list_item(item, &names) {
>> +		for (ext = 0; ext < 2; ext++) {
>> +			char *fname, *fname_old;
>> +			struct stat statbuffer;
>> +			fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
>> +			fname_old = mkpath("%s-%s%s", packtmp, item->string, exts[ext]);
>> +			if (!stat(fname_old, &statbuffer)) {
>> +				statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
>> +				chmod(fname_old, statbuffer.st_mode);
>
> Unchecked return value.

The original was an unchecked 'chmod a-w', so we don't care.

Of course, we could mimic the original better by issuing warnings.

>
>> +	/* Remove the "old-" files */
>> +	for_each_string_list_item(item, &names) {
>> +		char *fname;
>> +		fname = mkpath("%s/old-pack-%s.idx", packdir, item->string);
>> +		if (remove_path(fname))
>> +			die_errno(_("removing '%s' failed"), fname);
>> +
>> +		fname = mkpath("%s/old-pack-%s.pack", packdir, item->string);
>> +		if (remove_path(fname))
>> +			die_errno(_("removing '%s' failed"), fname);
>
> Does this have to be a fatal error? If I read correctly, it wasn't fatal
> in the shell version.

Good catch.

-- Hannes