From: Johannes Sixt <j6t@kdbg.org>
Date: Thu, 22 Aug 2013 10:46:50 GMT
Subject: Re: [PATCH] repack: rewrite the shell script in C.
Message-ID: <5215EC1A.7060004@kdbg.org>
In-Reply-To: <vpqhaeje8e0.fsf@anie.imag.fr>

Am 21.08.2013 10:49, schrieb Matthieu Moy:
> Stefan Beller <stefanbeller@googlemail.com> 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