Cleanup: Remove hash field from picture-structure

The hash field in the picture-structure was in principle non-operational.
It was set on loading, but never actually changed. The authoritative
hash comes from the filename->hash map.

Therefore, make this explicit by removing the hash field from the
picture structure.

Instead of filling the picture structure on loading, add the
hash directly to the filename->hash map. This is done in the
register_hash() function, which does not overwrite old entries.
I.e. the local hash has priority over the save-file. This
policy might be refined in the future.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

core/dive.c

@@ -455,7 +455,6 @@ static void copy_pl(struct picture *sp, struct picture *dp)
 {
 	*dp = *sp;
 	dp->filename = copy_string(sp->filename);
-	dp->hash = copy_string(sp->hash);
 }
 
 /* copy an element in a list of tags */
@@ -3804,7 +3803,6 @@ void picture_free(struct picture *picture)
 	if (!picture)
 		return;
 	free(picture->filename);
-	free(picture->hash);
 	free(picture);
 }
 

core/dive.h

@@ -412,7 +412,6 @@ struct dive_components {
 /* picture list and methods related to dive picture handling */
 struct picture {
 	char *filename;
-	char *hash;
 	offset_t offset;
 	degrees_t latitude;
 	degrees_t longitude;

core/imagedownloader.cpp

@@ -9,8 +9,9 @@
 
 #include <QtConcurrent>
 
-static QUrl cloudImageURL(const char *hash)
+static QUrl cloudImageURL(const char *filename)
 {
+	QString hash = hashString(filename);
 	return QUrl::fromUserInput(QString("https://cloud.subsurface-divelog.org/images/").append(hash));
 }
 
@@ -26,7 +27,7 @@ ImageDownloader::~ImageDownloader()
 
 void ImageDownloader::load(bool fromHash)
 {
-	if (fromHash && loadFromUrl(cloudImageURL(picture->hash)))
+	if (fromHash && loadFromUrl(cloudImageURL(picture->filename)))
 		return;
 
 	// If loading from hash failed, try to load from filename
@@ -107,12 +108,11 @@ SHashedImage::SHashedImage(struct picture *picture) : QImage()
 	if (isNull()) {
 		// This did not load anything. Let's try to get the image from other sources
 		// Let's try to load it locally via its hash
-		QString filename = fileFromHash(picture->hash);
-		if (filename.isNull())
-			filename = QString(picture->filename);
+		QString filename = localFilePath(picture->filename);
 		if (filename.isNull()) {
 			// That didn't produce a local filename.
 			// Try the cloud server
+			// TODO: This is dead code at the moment.
 			QtConcurrent::run(loadPicture, clone_picture(picture), true);
 		} else {
 			// Load locally from translated file name

core/load-git.c

@@ -50,10 +50,13 @@ static void save_picture_from_git(struct picture *picture)
 	struct picture_entry_list *pic_entry = pel;
 
 	while (pic_entry) {
-		if (same_string(pic_entry->hash, picture->hash)) {
-			savePictureLocal(picture, pic_entry->data, pic_entry->len);
+		char *hash = hashstring(picture->filename);
+		if (same_string(pic_entry->hash, hash)) {
+			savePictureLocal(picture, hash, pic_entry->data, pic_entry->len);
+			free(hash);
 			return;
 		}
+		free(hash);
 		pic_entry = pic_entry->next;
 	}
 	fprintf(stderr, "didn't find picture entry for %s\n", picture->filename);
@@ -971,7 +974,9 @@ static void parse_picture_hash(char *line, struct membuffer *str, void *_pic)
 {
 	(void) line;
 	struct picture *pic = _pic;
-	pic->hash = get_utf8(str);
+	char *hash = get_utf8(str);
+	register_hash(pic->filename, get_utf8(str));
+	free(hash);
 }
 
 /* These need to be sorted! */

core/parse-xml.c

@@ -1154,6 +1154,7 @@ static void gps_picture_location(char *buffer, struct picture *pic)
 /* We're in the top-level dive xml. Try to convert whatever value to a dive value */
 static void try_to_fill_dive(struct dive *dive, const char *name, char *buf)
 {
+	char *hash;
 	start_match("dive", name, buf);
 
 	switch (import_source) {
@@ -1197,8 +1198,11 @@ static void try_to_fill_dive(struct dive *dive, const char *name, char *buf)
 		return;
 	if (MATCH("gps.picture", gps_picture_location, cur_picture))
 		return;
-	if (MATCH("hash.picture", utf8_string, &cur_picture->hash))
+	if (MATCH("hash.picture", utf8_string, &hash)) {
+		register_hash(cur_picture->filename, hash);
+		free(hash);
 		return;
+	}
 	if (MATCH("cylinderstartpressure", pressure, &dive->cylinder[0].start))
 		return;
 	if (MATCH("cylinderendpressure", pressure, &dive->cylinder[0].end))

core/qthelper.cpp

@@ -1075,10 +1075,20 @@ QMutex hashOfMutex;
 QHash<QByteArray, QString> localFilenameOf;
 QHash <QString, QImage > thumbnailCache;
 
-extern "C" char * hashstring(const char *filename)
+static QByteArray getHash(const QString &filename)
 {
 	QMutexLocker locker(&hashOfMutex);
-	return strdup(hashOf[QString(filename)].toHex().data());
+	return hashOf[filename];
+}
+
+QString hashString(const char *filename)
+{
+	return getHash(QString(filename)).toHex();
+}
+
+extern "C" char * hashstring(const char *filename)
+{
+	return strdup(qPrintable(hashString(filename)));
 }
 
 const QString hashfile_name()
@@ -1136,6 +1146,21 @@ void add_hash(const QString &filename, const QByteArray &hash)
 	localFilenameOf[hash] = filename;
 }
 
+// Add hash if not already known
+extern "C" void register_hash(const char *filename, const char *hash)
+{
+	if (empty_string(filename) || empty_string(hash))
+		return;
+	QString filenameString(filename);
+
+	QMutexLocker locker(&hashOfMutex);
+	if (!hashOf.contains(filenameString)) {
+		QByteArray hashBuf = QByteArray::fromHex(hash);
+		hashOf[filename] =  hashBuf;
+		localFilenameOf[hashBuf] = filenameString;
+	}
+}
+
 QByteArray hashFile(const QString &filename)
 {
 	QCryptographicHash hash(QCryptographicHash::Sha1);
@@ -1174,22 +1199,14 @@ QString localFilePath(const QString &originalFilename)
 		return originalFilename;
 }
 
-QString fileFromHash(const char *hash)
-{
-	if (empty_string(hash))
-		return "";
-	QMutexLocker locker(&hashOfMutex);
-
-	return localFilenameOf[QByteArray::fromHex(hash)];
-}
-
 // This needs to operate on a copy of picture as it frees it after finishing!
 void hashPicture(struct picture *picture)
 {
 	if (!picture)
 		return;
+	QByteArray oldHash = getHash(QString(picture->filename));
 	QByteArray hash = hashFile(localFilePath(picture->filename));
-	if (!hash.isNull() && !same_string(hash.toHex().data(), picture->hash))
+	if (!hash.isNull() && hash != oldHash)
 		mark_divelist_changed(true);
 	picture_free(picture);
 }
@@ -1230,23 +1247,16 @@ void learnImages(const QDir dir, int max_recursions)
 
 extern "C" const char *local_file_path(struct picture *picture)
 {
-	QString hashString = picture->hash;
-	if (hashString.isEmpty()) {
-		QByteArray hash = hashFile(picture->filename);
-		free(picture->hash);
-		picture->hash = strdup(hash.toHex().data());
-	}
-	QString localFileName = fileFromHash(picture->hash);
-	if (localFileName.isEmpty())
-		localFileName = picture->filename;
-	return strdup(qPrintable(localFileName));
+	return strdup(qPrintable(localFilePath(picture->filename)));
 }
 
 extern "C" bool picture_exists(struct picture *picture)
 {
-	QString localFilename = fileFromHash(picture->hash);
-	QByteArray hash = hashFile(localFilename);
-	return same_string(hash.toHex().data(), picture->hash);
+	QString localPath = localFilePath(picture->filename);
+	if (localPath.isEmpty())
+		return false;
+	QByteArray hash = hashFile(localPath);
+	return !hash.isEmpty() && getHash(QString(picture->filename)) == hash;
 }
 
 const QString picturedir()
@@ -1262,19 +1272,19 @@ extern "C" char *picturedir_string()
 /* when we get a picture from git storage (local or remote) and can't find the picture
  * based on its hash, we create a local copy with the hash as filename and the appropriate
  * suffix */
-extern "C" void savePictureLocal(struct picture *picture, const char *data, int len)
+extern "C" void savePictureLocal(struct picture *picture, const char *hash, const char *data, int len)
 {
 	QString dirname = picturedir();
 	QDir localPictureDir(dirname);
 	localPictureDir.mkpath(dirname);
 	QString suffix(picture->filename);
 	suffix.replace(QRegularExpression(".*\\."), "");
-	QString filename(dirname + picture->hash + "." + suffix);
+	QString filename(dirname + hash + "." + suffix);
 	QSaveFile out(filename);
 	if (out.open(QIODevice::WriteOnly)) {
 		out.write(data, len);
 		out.commit();
-		add_hash(filename, QByteArray::fromHex(picture->hash));
+		add_hash(filename, QByteArray::fromHex(hash));
 	}
 }
 

core/qthelper.h

@@ -34,12 +34,12 @@ void read_hashes();
 void write_hashes();
 void updateHash(struct picture *picture);
 QByteArray hashFile(const QString &filename);
+QString hashString(const char *filename);
 void learnImages(const QDir dir, int max_recursions);
 void add_hash(const QString &filename, const QByteArray &hash);
 void hashPicture(struct picture *picture);
 extern "C" char *hashstring(const char *filename);
 QString localFilePath(const QString &originalFilename);
-QString fileFromHash(const char *hash);
 void learnHash(const QString &originalName, const QString &localName, const QByteArray &hash);
 weight_t string_to_weight(const char *str);
 depth_t string_to_depth(const char *str);
@@ -74,10 +74,11 @@ void subsurface_mkdir(const char *dir);
 char *get_file_name(const char *fileName);
 void copy_image_and_overwrite(const char *cfileName, const char *path, const char *cnewName);
 char *hashstring(const char *filename);
+void register_hash(const char *filename, const char *hash);
 bool picture_exists(struct picture *picture);
 char *move_away(const char *path);
 const char *local_file_path(struct picture *picture);
-void savePictureLocal(struct picture *picture, const char *data, int len);
+void savePictureLocal(struct picture *picture, const char *hash, const char *data, int len);
 void cache_picture(struct picture *picture);
 char *cloud_url();
 char *hashfile_name_string();

tests/testpicture.cpp

@@ -44,8 +44,6 @@ void TestPicture::addPicture()
 	QVERIFY(pic1->longitude.udeg == 11334500);
 	QVERIFY(pic2->offset.seconds == 1321);
 
-	QVERIFY(pic1->hash == NULL);
-	QVERIFY(pic2->hash == NULL);
 	QByteArray hash1 = hashFile(localFilePath(pic1->filename));
 	QByteArray hash2 = hashFile(localFilePath(pic2->filename));
 	learnHash(pic1->filename, PIC1_NAME, hash1);
@@ -54,8 +52,8 @@ void TestPicture::addPicture()
 	QCOMPARE(hashstring(pic2->filename), PIC2_HASH);
 	QCOMPARE(hashstring(PIC1_NAME), PIC1_HASH);
 	QCOMPARE(hashstring(PIC2_NAME), PIC2_HASH);
-	QCOMPARE(fileFromHash(PIC1_HASH), QString(PIC1_NAME));
-	QCOMPARE(fileFromHash(PIC2_HASH), QString(PIC2_NAME));
+	QCOMPARE(localFilePath(pic1->filename), QString(PIC1_NAME));
+	QCOMPARE(localFilePath(pic2->filename), QString(PIC2_NAME));
 }