[pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

newer
[pacman-dev] [RFC] pacman, package...

older
[pacman-dev] Make pacscripts faster

Patrick Steinhardt

4 Jun 2013 4 Jun '13

9:20 a.m.

Fixes FS#35515. --- lib/libalpm/add.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index c20e7c6..cab04a8 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -309,11 +309,13 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, _alpm_log(handle, ALPM_LOG_DEBUG, "original: %s\n", hash_orig); if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n", + entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + } } else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */ -- 1.8.3

Attachments:

attachment.sig (application/pgp-signature — 836 bytes)

Show replies by date

Ross Lagerwall

4 Jun 4 Jun

11:10 a.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:

...

if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",

Perhaps this should be changed to something like: "action: updating existing file's timestamps\n" to differentiate it from when the file is actually new.

...

+ entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + }

Can we not just ignore it if it fails since it is non-fatal?

...

} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */ -- 1.8.3

FWIW, I actually tried to send a similar patch through on Sunday (attached) but it clearly didn't make it to the list (I guess you need to be subscribed). -- Ross Lagerwall

...

From b8927439aba6f7408f75883dfed9616c70df989a Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Sun, 2 Jun 2013 13:31:54 +0100 Subject: [PATCH] add.c: update identical config files to preserve mtime

If two config files are identical, overwrite the existing one so that "pacman -Qkk" does not report "Modification time mismatch" after a package upgrade even though no files have been touched. --- lib/libalpm/add.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 9cbf767..d7d2382 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -331,11 +331,11 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, _alpm_log(handle, ALPM_LOG_DEBUG, "original: %s\n", hash_orig); if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ + /* local and new files are the same, overwrite so that timestamps + * are maintained correctly for pacman -Qkk */ _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + "action: updating existing file's timestamps\n"); + try_rename(handle, checkfile, filename); } else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */ -- 1.8.3

Patrick Steinhardt

12:17 p.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

Hi, On Tue, Jun 04, 2013 at 12:10:17PM +0100, Ross Lagerwall wrote:

...

On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:

...
if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",

Perhaps this should be changed to something like: "action: updating existing file's timestamps\n" to differentiate it from when the file is actually new.

Probably true, I'll change it later.

...

...
+ entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + }

Can we not just ignore it if it fails since it is non-fatal?

If we ignore it we should at least add a debug/warning message if it fails, I guess. [snip]

...

if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ + /* local and new files are the same, overwrite so that timestamps + * are maintained correctly for pacman -Qkk */ _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile);

I don't think the checkfile should be unlinked here, should it? It will certainly fail if you first unlink and afterwards try to rename it. Regards, Patrick

Ross Lagerwall

12:41 p.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On Tue, Jun 04, 2013 at 02:17:07PM +0200, Patrick Steinhardt wrote:

...

...
Can we not just ignore it if it fails since it is non-fatal?

If we ignore it we should at least add a debug/warning message if it fails, I guess.

Fair enough.

...

[snip]

...
if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ + /* local and new files are the same, overwrite so that timestamps + * are maintained correctly for pacman -Qkk */ _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile);

I don't think the checkfile should be unlinked here, should it? It will certainly fail if you first unlink and afterwards try to rename it.

Of course. I think you're reading the diff wrong. The unlink is *replaced* with a rename in the new code. Regards -- Ross Lagerwall

Patrick Steinhardt

1:05 p.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On Tue, Jun 04, 2013 at 01:41:07PM +0100, Ross Lagerwall wrote:

...

On Tue, Jun 04, 2013 at 02:17:07PM +0200, Patrick Steinhardt wrote:

...
...
Can we not just ignore it if it fails since it is non-fatal?

If we ignore it we should at least add a debug/warning message if it fails, I guess.

Fair enough.

I've attached an updated version of the patch which emits a warning when the timestamp could not be updated. The debug message has been updated to state that the extraction is just a timestamp update, as well.

...

...
I don't think the checkfile should be unlinked here, should it? It will certainly fail if you first unlink and afterwards try to rename it.

Of course. I think you're reading the diff wrong. The unlink is *replaced* with a rename in the new code.

Never mind, I was screwed because no syntax highlighting existed. Regards, Patrick

Allan McRae

5 Jun 5 Jun

4:52 a.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On 04/06/13 21:10, Ross Lagerwall wrote:

...

On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:

...
if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",

Perhaps this should be changed to something like: "action: updating existing file's timestamps\n" to differentiate it from when the file is actually new.

I like "installing new file" because it is exactly what we say we will do in the pacman(8) man page. Also, the situations are easily distinguished by the previous debug output.

...

...
+ entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + }

Can we not just ignore it if it fails since it is non-fatal?

No. If something fails here, we are in a very bad situation for the rest of the transaction. I want that error to propagate so that we abort the transaction after this package is finished. (Aside) What I have just noticed, is we do not print any actual errors during this whole section. So the transaction will stop with errors occurred during extraction but give no indication what actually happened. Another patch....

...

...
} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */

So... with my two comments above, I think the original patch is fine. I will pull that in the next few days unless there is objections to my above reasoning. Allan

Patrick Steinhardt

10 Jun 10 Jun

6:33 a.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On Wed, Jun 05, 2013 at 02:52:53PM +1000, Allan McRae wrote:

...

On 04/06/13 21:10, Ross Lagerwall wrote:

...
On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:

...
if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",

Perhaps this should be changed to something like: "action: updating existing file's timestamps\n" to differentiate it from when the file is actually new.

I like "installing new file" because it is exactly what we say we will do in the pacman(8) man page. Also, the situations are easily distinguished by the previous debug output.

...
...
+ entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + }

Can we not just ignore it if it fails since it is non-fatal?

No. If something fails here, we are in a very bad situation for the rest of the transaction. I want that error to propagate so that we abort the transaction after this package is finished.

(Aside) What I have just noticed, is we do not print any actual errors during this whole section. So the transaction will stop with errors occurred during extraction but give no indication what actually happened. Another patch....

...
...
} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */

So... with my two comments above, I think the original patch is fine. I will pull that in the next few days unless there is objections to my above reasoning.

Are you going to pull this patch for pacman 4.1.2? Just curious as it is not included in the master branch yet. Patrick

Allan McRae

15 Jun 15 Jun

1:59 p.m.

New subject: [pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

On 10/06/13 16:33, Patrick Steinhardt wrote:

...

On Wed, Jun 05, 2013 at 02:52:53PM +1000, Allan McRae wrote:

...
On 04/06/13 21:10, Ross Lagerwall wrote:

...
On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:

...
if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) { - /* local and new files are the same, no sense in installing the file - * over itself, regardless of what the original file was */ - _alpm_log(handle, ALPM_LOG_DEBUG, - "action: leaving existing file in place\n"); - unlink(checkfile); + /* local and new files are the same, updating anyway to get + * correct timestamps */ + _alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",

Perhaps this should be changed to something like: "action: updating existing file's timestamps\n" to differentiate it from when the file is actually new.

I like "installing new file" because it is exactly what we say we will do in the pacman(8) man page. Also, the situations are easily distinguished by the previous debug output.

...
...
+ entryname_orig); + if(try_rename(handle, checkfile, filename)) { + errors++; + }

Can we not just ignore it if it fails since it is non-fatal?

No. If something fails here, we are in a very bad situation for the rest of the transaction. I want that error to propagate so that we abort the transaction after this package is finished.

(Aside) What I have just noticed, is we do not print any actual errors during this whole section. So the transaction will stop with errors occurred during extraction but give no indication what actually happened. Another patch....

...
...
} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) { /* original and new files are the same, leave the local version alone, * including any user changes */

So... with my two comments above, I think the original patch is fine. I will pull that in the next few days unless there is objections to my above reasoning.

Are you going to pull this patch for pacman 4.1.2? Just curious as it is not included in the master branch yet.

I'll pull into master sometime after the 4.1.2 release. It will not be part of a 4.1.x release. Allan

3974

Age (days ago)

3985

Last active (days ago)

List overview

Download

7 comments

3 participants

participants (3)

Allan McRae
Patrick Steinhardt
Ross Lagerwall