MantisBT - ATutor
View Issue Details
0003541ATutorPatcherpublic2008-08-21 23:192008-11-14 08:23
IndieRect 
cindy 
normalminoralways
closedfixed 
1.6.1 
1.6.2 
1.6.1
0003541: Chunk failures not reported
If individual operations of a patch (those within < file >< /file > tags in the xml) fail, no error is reported and the feedback is that the patch has been applied successfully.

For patch developers it's a problem that complicates debugging and might cause flawed patches slip through. Also it will cause errors with correct patches working on files where replace-from chunks have been modified.
Not to mention that ignoring return values is generally a bad practice.
One scenario in which it may be particularly annoying for a programmer is when a patch is created from a diff (I believe it's convenient for scattered small changes, see 0003528 == http://atutor.no/contribs/group-deletion-test-submissions-plus-bugfixes [^] for an example of such a patch).
To that end, one copies fragments and then deletes leading '+', '-' and ' ' chars. When the latter is overlooked (likely to happen if tabs follow that space), a flawed and quite hard-to-debug patch will result.
No tags attached.
Issue History
2008-08-21 23:19IndieRectNew Issue
2008-08-21 23:19IndieRectAffects version => 1.6.1
2008-08-26 09:39IndieRectNote Added: 0003150
2008-08-27 05:07cindyNote Added: 0003152
2008-08-27 10:18IndieRectNote Added: 0003153
2008-08-28 05:22cindyNote Added: 0003154
2008-08-28 12:02IndieRectNote Added: 0003155
2008-08-28 12:06IndieRectNote Edited: 0003155
2008-09-02 04:39cindyNote Added: 0003158
2008-10-27 07:31cindyStatusnew => resolved
2008-10-27 07:31cindyFixed in Version => 1.6.2
2008-10-27 07:31cindyResolutionopen => fixed
2008-10-27 07:31cindyAssigned To => cindy
2008-10-27 07:31cindyNote Added: 0003245
2008-11-14 08:23gregStatusresolved => closed

Notes
(0003150)
IndieRect   
2008-08-26 09:39   
An example "in the wild": http://www.atutor.ca/view/7/14892/1.html [^] .
(0003152)
cindy   
2008-08-27 05:07   
Hi Indie, do you mean if the <replace_from> code doesn't match the part in user's copy because of whitespaces or tabs or any others, the patch installation should be reported fail with the errors? Thanks.
(0003153)
IndieRect   
2008-08-27 10:18   
Yes, if comparing in any chunk fails (whitespaces, edited code etc.), then all such chunks should be flagged. No changes at all should be made then, as they might be interdependent (again, as in patch 0007).
Consequently, patching could be a two-pass process first checking for possible errors and then applying if everything's OK.

How exactly Patcher should react to failed chunks is quite a problem.
I guess we may assume that if users have customized their code then they ARE programmers and can manage differences in failed <replace_from> themselves. But present implementation won't give them those difference between what is expected and what is found.

At least, they could be provided with as much info as currently possible on those failures; even that info (file + dir + replace_from + replace_to) may be sufficient for a programmer (I judge from myself).
(0003154)
cindy   
2008-08-28 05:22   
Good idea. There's no harm to provide more details.

I'm considering, in the last successful message, adding a notes of the non-found <replace-from> codes besides (or following) the script that the code is supposed to be found, but still reporting the installation as successful.
(0003155)
IndieRect   
2008-08-28 12:02   
(edited on: 2008-08-28 12:06)
In fact, you don't solve the problem but just suggest putting the responsibility onto the unprepared user.

I'm afraid that it won't work if only due to basic psychology. If you were presented with contradicting messages, of success and failure, which one would you accept?
Probably the one you're more comfortable with -- that everything's OK, not that you must dig into the code yourself (which might be something a user didn't do in his/her entire life).
Everyone thinks that the program understands better what it's doing and what the effects are. If it says it's OK with some (minor, as they appear) "side notes", then it must really be OK and require no intervention on the user's part.
Thus, we force users into ignoring error messages; so what's the reason of having them in the first place?

From another point of view, do you have redundant or optional chunks in your patches?
You don't. So unless each one has been put in its place the patch would probably miss the point. Would it be an empty column in a table or inability to log in, is a matter of luck.
So if one fails, everything has to be assumed failed. No reason to declare such a patch successful.


That's why I believe that a patch has to be reported failed if it cannot be applied in its entirety, and no actual changes should take place (or they should be rolled back automatically).

One possible way to assist users in resolving failed chunks is to present them not only with the <replace_from> fragment, but also a text field into which they have to paste the corresponding fragment of their customized code.
The script then could use that user-specified fragment as if it was the <replace_from>, and check for ability to proceed again, and so on in a loop. When all conflicts have been resolved in this way, the patch is applied.
It's something like having '--verbose' and '--dry-run' switches on in the Linux patch utility before really applying a diff.

(0003158)
cindy   
2008-09-02 04:39   
The only purpose of the message is to make it easier for patch creators to debug at creating patches.

One of the very first confirmations at beginning of patch installation is, users have to give go-ahead signal on the customized scripts. The confirmation message also tells users the customized code may not work the way they want once being modified by patch. Once users agree and choose to proceed, they have to take the responsibility on customized code. Nobody could guarantee the code still work the same way other than customizers themselves.

The way you suggested can be achieved by creating patch to hold the customized parts. This patch can be applied by customizers themselves each time when upgrading to a newer version.
(0003245)
cindy   
2008-10-27 07:31   
SVN Revision: 8091

Affected script: mods/_standard/patcher/classes/Patch.class.php

Add warning message at the end of patch installation, besides the according backup file that one or more code chunks are not found. Still report successful installation.