Bug 632594 created; review, approval, commit requested

[Geert Janssens]

On Wednesday 20 October 2010, Thomas Bullock wrote:
> Yawar,
> 
> I reviewed your links and have these questions.
> 
> 1.  Are you saying do not modify gnucash-guide.xml at this time?  If so, is
>  that because Cristian is working on this module?
You are free to modify gnucash-guide.xml. Especially if your changes are 
small. When I told you about using 'svn update' regularly to pull in changes 
made by other contributors, that is equally valid for Cristian. Before he 
commits his changes, he'll probably first run an svn update as well to be sure 
all changes made by others are in his working copy. If necessary he may have 
to tweak some of those external changes to align with his local changes. But 
that is normal in a collaborative environment. It would be more complicated if 
two people were making big structural changes at the same time in the same 
file. In that case you better coordinate more closely.

On the other hand, your patch as seen in bugzilla doesn't contain 
modifications to gnucash-guide.xml, so I wonder how you interpreted Yarar's 
reply as if you are not allowed to change that file.

>  If I don't modify it,
>  then I just drop it from what my 'svn diff' found?
If your diff contains changes you don't want to propagate (yet), you can do 
this indeed, but this particular method is error prone. A better way is to 
revert the changes from the original file (possibly saving a copy temporarily 
if you want to keep the changes for a later time) and recreate the patch with 
svn diff.

>  I thought I had to
>  correct all of 'xmllint' and 'svn diff''s findings?
xmllint reports on xml errors. svn diff simply shows you exactly what you have 
changed. In that context, "correcting svn diff's findings" doesn't seem to 
make much sense to me. svn diff will not judge if the changes are right or 
wrong, so it will not give you anything to correct per se. As just mentioned, 
if svn diff reports changes you don't like, the preferred method is to revert 
those changes in your original file and run svn diff again.

>  Cristian Marchi told
>  me to fix them when I found them.  See his email dated 10/19/2010, listed
>  as the #3 in the digest vol 91, Issue 26.  I don't know what you mean by
>  mean fixing line breaks.
> 
xmllint errors should be fixed indeed.

Just to understand what is happening here, I have checked out r19679, which is 
the last revision just before your patch is applied by Yawar. This revision 
doesn't hold any xmllint errors anymore as far as I can see.

At the same time, I don't see anything in your patch that would fix an xmllint 
error.

I can only assume this is what happened:
1. you had fixed a number of xmllint errors in your local working copy.
2. at about the same time, Cristian committed his work in r19679, which among 
others also fixes the same xmllint errors.
3. you dutifully ran svn update before creating your final patch. This svn 
update pulled in the final xmllint fixes that Cristian had just committed. 
Since your local fixes were the same as in the online svn repo, svn update 
marked these local changes as in sync with the online repo. In other words, 
your local fixes wouldn't appear in the subsequent svn diff, because they were 
already in the online repository.
4. You ran svn diff to create the patch, still assuming there would be xmllint 
fixes in it.

But because these fixes are already in the online repo, what is basically left 
in your patch is the tense correction, the addition of the new chapters (more 
on that in a minute) and some wrapped lines.

It's this wrapping of lines that Yawar asked to postpone to a later date, when 
the documentation updates have stabilized a little more.


> 2.  Regarding removing payroll chapter 14 by accident, please explain why
>  that is?  Before committing the patch I transformed the xml files to html
>  and examined chapter 1 for the overview. It showed me chapters 11 thru 16
>  without any chapters missing.  I looked at your changesets 19684 and 19686
>  and both seem to me to be saying that I am not deleting chapter 14,  but
>  am inserting 15 and 16 after it.  In what way am I reading that
>  incorrectly?
> 
The overview will always show continuous numbering even if you delete a 
chapter. The chapter numbering is dynamic. At best you could see the last 
number has gone missing.

But if you would look for a chapter on Payroll in your generated html, it 
would have gone missing, chapter 14 would now be the budgets chapter.

Yawar is correct, that is the change your patch is making even though you may 
not have intended this:
-          <para><xref linkend="chapter_bus_pay"></xref></para>
+          <para><xref linkend="chapter_budgets"></xref></para>

The link to chapter_bus_pay in the original file is being replaced (-) with a 
link to chapter_budgets (+).

You could check in your original xml file if you have still linkends for both 
chapter_bus_pay and chapter_budgets. The patch suggests you don't.

Of course, if you ran an svn update in the meantime, things may have changed 
once more because you then have pulled in what Yawar committed, which is a 
corrected version of your patch.

By the way, if you run svn status, do any of your files have a "C" state ? 
Those should always be fixed before making a patch. Read up on conflict 
resolution in the subversion book if you need help with that [1].

Geert

[1] http://svnbook.red-
bean.com/en/1.5/svn.tour.cycle.html#svn.tour.cycle.resolve