Re: [PATCH v3] Add by-hash support
Julien Cristau <jcristau@debian.org> writes:
>> In case of `No-Action`, the transaction should be rolled back.
>>
> I was following what the other clean* functions in that file seem to be
> doing, AFAICT they rely on the final session.rollback() in main(). I
> can add it here, though.
Hmm, I think an explicit rollback might be safer. Then it is clear that
nothing happens locally, no matter what others might do with the
session.
>> > + strong_hash = hashes[-1]
>>
>> Hmm, I don't like this assumption, but then I have nothing better for
>> now.
>>
> You mean the assumption that the suite has at least one of md5, sha1,
> sha256?
The assumption that the strongest hash is the last one ;)
>> > + if h == strong_hash:
>> > + with open(filename, 'rb') as src:
>> > + contents = src.read()
>> > + with open(hashfile + '.new', 'wb') as dst:
>> > + dst.write(contents)
>> > + os.rename(hashfile + '.new', hashfile)
>> > + os.unlink(filename)
>>
>> This is just `os.rename(filename, hashfile)`? Or `shutil.move` for
>> the theoretic case of `filename` and `hashfile` being on different
>> filesystems.
>>
> Not quite. If you're calling generate-releases twice in a row, e.g.
> after an error, filename is already a symlink, so moving would break
> things. I could check for that explicitly, if you prefer?
Hmm, then `shutil.copy2()` + `os.unlink()`? Note that `copy2` will not
copy symlinks, but the actual file (plus metadata).
>> > + os.symlink(os.path.join('by-hash', h, fileinfo[filename][h]),
>> > + filename)
>> > + elif not os.path.exists(hashfile):
>> > + os.symlink(os.path.join('..', strong_hash,
>> > fileinfo[filename][strong_hash]),
>> > + hashfile)
>>
>> I wonder if hardlinks are the better choice (no risk of dangling
>> symlinks), but symlinks are probably better for mirrors that don't
>> mirror hardlinks as such, but duplicate the data?
>>
> No preference here.
Then let's keep the current implementation for now. We can always change
it later.
Ansgar
Reply to: