267158 - Spurious conflicts on update because of RCS tags

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. cvs co mozilla/client.mk; cd mozilla/; make -f client.mk checkout
2. wait 3 months
3. make -f client.mk checkout (to update)

Actual result:
- CVs reports conflicts. Source files have conflicts because of CVS/RCS tags, e.g.:
<<<<<<< certdata.c
static const char CVS_ID[] = "@(#) $RCSfile: certdata.c,v $ $Revision: 1.27 $ $Dat
e: 2003/06/05 00:53:26 $ $Name: AVIARY_1_0_20040515_BRANCH $""; @(#) $RCSfile: cer
tdata.c,v $ $Revision: 1.27 $ $Date: 2003/06/05 00:53:26 $ $Name: AVIARY_1_0_20040
515_BRANCH $";
=======
static const char CVS_ID[] = "@(#) $RCSfile: certdata.c,v $ $Revision: 1.27.22.1 $
 $Date: 2004/10/15 21:13:52 $ $Name:  $""; @(#) $RCSfile: certdata.c,v $ $Revision
: 1.27.22.1 $ $Date: 2004/10/15 21:13:52 $ $Name:  $";
>>>>>>> 1.27.22.1

Expected result:
- No conflicts, because no loca changes.

Additional comments:
I don't know why this happens, if it's a CVS bug or my fault or whatever, but
it's annoying, because I manually have to deal with it every time NSS changes.
If there's no better and practical solution, please remove the RCS/CVS tags from
the source files.

Comment 1

[Wan-Teh Chang]

Can you come up with another procedure to reproduce
this CVS conflict?

Your current procedure requires waiting three months
between the two checkouts.

Comment 2

[Ben Bucksch (:BenB)]

I guess you could make some fake checkins, so that the RCS flags would change in
the Mozilla tree in the same way as noted. Probably the freaky mozilla.org CVS
setup is involved, so you may not see this in a pure NSS module.

Note that "AVIARY_1_0_20040515_BRANCH" in the local version.

Comment 3

[Ben Bucksch (:BenB)]

> Reproduction:
> 1. cvs co mozilla/client.mk

To be precise, I checked out a branch, i.e.
cvs co -r AVIARY_1_0_20040515_BRANCH mozilla/client.mk
or similar.

Of course I didn't expect you to wait 3 months during reproduction ;-), just
wanted to show what I did to run into the bug, and I left it to you to guess
what happened, as I don't know CVS or RCS tags enough.

My only guess is that CVS changes the tags during checkout, has some dirty hacks
in its code to prevent these conflicts, and the hacks don't work together with
mozilla.org's CVS setup or when
1. somebody checks out a version from one CVS module
2. checks it into a second
3. you check out of the second
4. somebody else updates the second with the first
5. you update your copy of the second
You may run into similar problems, if you try to check in your own, modified
copy of Mozilla into your CVS repository.

Comment 4

[Ben Bucksch (:BenB)]

I checked out a trunk tree a while ago - it seems the first checkout was
2004-06-05 and updated (using make -f client.mk checkout) on 2004-10-13. When I
now do a cvs diff on the whole tree, I get pretty much no changes apart from
*lots* of crap like this from NSS:

Index: security/nss/lib/util/utf8.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/util/utf8.c,v
retrieving revision 1.7
diff -u -r1.7 utf8.c
--- security/nss/lib/util/utf8.c        20 Jan 2004 19:57:17 -0000      1.7
+++ security/nss/lib/util/utf8.c        17 Jan 2005 02:16:57 -0000
@@ -33,7 +33,7 @@
  */

 #ifdef DEBUG
-static const char CVS_ID[] = "@(#) $RCSfile: utf8.c,v $ $Revision: 1.7 $ $Date:
2004/01/20 19:57:17 $ $Name:  $";
+static const char CVS_ID[] = "@(#) $RCSfile: utf8.c,v $ $Revision: 1.7 $ $Date:
2004/01/20 19:57:17 $ $Name: NSS_CLIENT_TAG $";
 #endif /* DEBUG */

 #include "seccomon.h"



Please remove these RCS tags (unless you have a better idea).

IIRC, they were also one reason why it was practically impossible to keep a
Mozilla derivate in a private CVS tree, but I am not sure about that, and I
don't want to try.

Comment 5

[Wan-Teh Chang]

Will do.

Comment 6

[Wan-Teh Chang]

Attachment: Remove the $Name$ RCS keyword

I believe $Name$ is the only RCS keyword that's
causing the spurious CVS conflicts, so this patch
only removes that keyword and leaves the other
keywords alone.

The $Name$ keyword also adds a lot of noise to
our debug builds when we run the ident command:

    ident libnss3.so | grep NSS

to extract the NSS version string.  So that's
another reason I want to remove it.

This patch is generated using the following
commands:

cvs co -kk mozilla/security/coreconf mozilla/security/nss
find . -type f	-exec perl -pi -e 's/ \$Name\$//g' {} \;
cvs -q diff mozilla > patch.txt

Comment 7

[Ben Bucksch (:BenB)]

> Will do.

Thanks a lot! :-)

> I believe $Name$ is the only RCS keyword that's
> causing the spurious CVS conflicts

I would think that the versions (and any other replacements) are problematic, too,
as they differed, too, see comment 0. But you probably know CVS better than me,
maybe the versions do work automagically.

Comment 8

[Wan-Teh Chang]

I don't know CVS that well.  The reason I think only
the $Name$ keyword is problematic is the cvs diff output
you showed in comment 4.  You can see that only the values
for the $Name$ keyword are different.

Let's see if removing $Name$ fixes the problem.  Some NSS
developers think those CVS_ID strings are useful, which
is why I want to remove only the keyword that's causing
problem.

Comment 9

[Ben Bucksch (:BenB)]

In the conflict cited in the initial description, the version was problematic, too.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 171559 [details] [diff] [review]
Remove the $Name$ RCS keyword

r=nelson

While reviewing this patch I found 5 places in 4 source files, shown below, 
where the RCS info was shown twice on the same line.  IINM, all 4 of these
source files are machine generated from other files.   It would be nice if
we could eliminate the duplication. Perhaps the code that generates them 
causes the RCS info to be repeated, and so if we were to try to eliminate 
the duplicate copies, they would return the next time the program is run 
that generates them.  

But this patch is not the cause of these duplications, and needn't be 
delayed until those duplicates are removed.


>Index: mozilla/security/nss/lib/ckfw/nssck.api
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/ckfw/nssck.api,v
>retrieving revision 1.5
>diff -r1.5 nssck.api
>36c36
>< static const char NSSCKAPI_CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$ ; @(#) $RCSfile$ $Revision$ $Date$ $Name$";
>---
>> static const char NSSCKAPI_CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ ; @(#) $RCSfile$ $Revision$ $Date$";


>Index: mozilla/security/nss/lib/ckfw/builtins/certdata.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v
>retrieving revision 1.30
>diff -r1.30 certdata.c
>35c35
>< static const char CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$""; @(#) $RCSfile$ $Revision$ $Date$ $Name$";
>---
>> static const char CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$""; @(#) $RCSfile$ $Revision$ $Date$";
>583c583
><   { (void *)"@(#) $RCSfile$ $Revision$ $Date$ $Name$""; @(#) $RCSfile$ $Revision$ $Date$ $Name$", (PRUint32)179 }
>---
>>   { (void *)"@(#) $RCSfile$ $Revision$ $Date$""; @(#) $RCSfile$ $Revision$ $Date$", (PRUint32)179 }


>Index: mozilla/security/nss/lib/pki1/oiddata.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pki1/oiddata.c,v
>retrieving revision 1.1
>diff -r1.1 oiddata.c
>35c35
>< static const char CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$ ; @(#) $RCSfile$ $Revision$ $Date$ $Name$";
>---
>> static const char CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ ; @(#) $RCSfile$ $Revision$ $Date$";
>Index: mozilla/security/nss/lib/pki1/oiddata.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pki1/oiddata.h,v
>retrieving revision 1.1
>diff -r1.1 oiddata.h
>39c39
>< static const char OIDDATA_CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$ ; @(#) $RCSfile$ $Revision$ $Date$ $Name$";
>---
>> static const char OIDDATA_CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ ; @(#) $RCSfile$ $Revision$ $Date$";

Comment 11

[Wan-Teh Chang]

Comment on attachment 171559 [details] [diff] [review]
Remove the $Name$ RCS keyword

Thanks for the code review, Nelson.  I've
checked in this patch on the NSS trunk (3.10).

I noticed the duplicated RCS keywords in
those four or five generated files, too.
It seems that the duplications are intentional
and mean the following:
   <generator version>; <generated file version>

where "generator" is the (Perl) script used
to generate the file.

Comment 12

[Wan-Teh Chang]

Ben is right, in the initial bug description,
the $Revision$ and $Date$ keywords are also
different.  So I don't know if removing the
$Name$ keyword alone will fix this bug.

Perhaps this has to do with the version of
the cvs client?  Just a wild guess.  Ben,
what's the output of your "cvs -v"?

I did some search and found:
http://www.network-theory.co.uk/docs/cvsmanual/cvs_64.html

But we are not doing merging (cvs update -j).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

It is desirable for the ident command to work on NSS binaries. 
The chief value of ident's output (IMO) is the revision numbers of the 
sources from which the binaries were built.  I would object to removing 
RCS version numbers from all CVS_ID lines, because it would completely 
disable ident, and render it useless.

If a diff of two trees shows that the only differences are in RCS version
numbers, then the obvious implication is that the two trees are on different
branches.  There is a known technique (demonstrated in comment 6) for 
checking out trees from different branches in a way where version number
differences will be suppressed.  If the purpose is to ask what differences
(other than version numbers) exist between the checked-in sources on two
branches, then that technique should be used in preparing the checked-out
sources for making the diffs.  That technique requires no changes to NSS
sources, not even the ones Wan-Teh just checked in.

Comment 14

[Ben Bucksch (:BenB)]

> Ben, what's the output of your "cvs -v"?

Concurrent Versions System (CVS) 1.11.14 (client/server)

As shipped with SuSE 9.1


Nelson, please consider that seamonkey is not checked out via cvs co commands,
but via make -f client.mk checkout, partly because of NSS and NSPR. So, I
wouldn't even know how to apply the -kk switch, now that I know it. But I think
that such advanced CVS knowledge shouldn't be required to do somethin as simple
as comparing 2 normally checked out Mozilla trees or doing cvs diff. An
alternate fix might be to add the -kk switch to the Mozilla checkout scripts, so
that everybody checkin out Mozilla gets no keyword expansion.

Comment 15

[Wan-Teh Chang]

I did some search for this problem last night
and have some preliminary findings.

The most likely reason for the spurious conflicts
(in the initial description of the bug) or diffs
(in comment 4) is that CVS thinks those files are
modified.  CVS uses the timestamp on a file
to determine if a file is modified.  So we just
need to find out why the timestamps on the files
in the source tree are wrong.

Some of my speculations are change of the system's
time zone setting, and change of the interpretion
of a timestamp as local time or UTC/GMT.  But I
haven't been able to conclusively prove these
changes are problematic to CVS.

Unfortunately I've spent enough time looking into
this and need to get back to more important work.

Comment 16

[Ben Bucksch (:BenB)]

> CVS uses the timestamp on a file

That may well fail. I am often copying trees around, tarring them up,
extracting, rsyncing to other machines and back (including machines with other
timezones) etc.. I typically use flags to preserve last mod times, so I think
they should be OK, but I can't garantee it.

And I think they *will* differ, if I check the tree into a version control
system (other CVS server or even other system like GNU arch), check it out again
and try to update the checked out tree. That's a typical scenario, if you work
on Mozilla apart from mozilla.org.

> Unfortunately I've spent enough time looking into
> this and need to get back to more important work.

Right, totally understandable. I didn't expect you to investigate the roots of
this problem.

If you don't want to remove the versions, I'll try to modify my Mozilla checkout
script to use -kk, and if that works, submit it to cls for checkin to the
Mozilla tree.

Thanks for your support!

Comment 17

[Wan-Teh Chang]

Ben, if you want to use -kk, please note the warning
about -kk in http://www.network-theory.co.uk/docs/cvsmanual/cvs_64.html.
I don't quite understand what that warning says,
but it may imply that CVS 1.12.2 or later is required
if cls checks in the -kk option into the Mozilla tree.

Comment 18

[Ben Bucksch (:BenB)]

If I understand that correctly, that applies only for CVS repositories which
have -kb set as default (for all files) and then don't use -kb for individual
binary files, which I don't think is the case for cvs.mozilla.org.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #11)

> It seems that the duplications are intentional
> and mean the following:
>    <generator version>; <generated file version>
> 
> where "generator" is the (Perl) script used
> to generate the file.

That may be the intent, but that is not what is actually accomplished 
in the 5 places in the sources where these duplications are found.  
In a given source file, the string $RCSFILE$ has only one value.  
So a line on which that string appears twice simply repeats that value twice.

If the intent of a string such as "@(#) $RCSfile$ $Revision$ $Date$ $Name$ ;
@(#) $RCSfile$ $Revision$ $Date$ $Name$" is to show the name, revision and 
date of the generator separately from the name, revision and date of the
generated file, then that line fails to accomplish its intended purpose.  
In the generated file, that line ends up showing the information about the
generated file twice, and not showing any info about the generator.

To fix that, I think the generator should remove the "$" symbols from its
own information in the generated source, so that RCS will not replace the
generator's own info in the generated sources when they are checked in.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #14 and comment #16)

> Nelson, please consider that seamonkey is not checked out via cvs co commands,

Of course it is.  Those commands may be in a script, but those are the only
commands that can check out the sources.

> but via make -f client.mk checkout, partly because of NSS and NSPR. So, I 
> wouldn't even know how to apply the -kk switch, now that I know it. But I 
> think that such advanced CVS knowledge shouldn't be required to do somethin 
> as simple as comparing 2 normally checked out Mozilla trees or doing cvs 
> diff. An alternate fix might be to add the -kk switch to the Mozilla 
> checkout scripts, so that everybody checkin out Mozilla gets no keyword 
> expansion.

I would consider that any mozilla developer/contributor with checkin 
privileges should not consider the cvs -kk option to be too "advanced".

> If you don't want to remove the versions, I'll try to modify my Mozilla 
> checkout script to use -kk, and if that works, submit it to cls for 
> checkin to the Mozilla tree.

I'd expect Chris to reject such a suggestion.  I would not expect him to 
make a change that puts the majority of mozilla developers at a disadvantage 
so that one (or a very few) mozilla users with special needs don't need to 
learn to use cvs checkout commands for themselves.  

I think your personal needs (sources without version numbers, for diffing whole
source trees, presumably to identify code to be ported to some other branch
and/or private source tree) are rather specialized, and not common to most
mozilla developers.  I don't think it makes sense to ask the majority of 
mozilla builders to give up version ident info so that you can avoid using
the cvs command with the -kk option for your special checkout needs.  

There are a few others who spend much of their time porting changes from
one branch to another.  I think knowledge of how to use cvs -kk is requisite
for such a job, and I expect most of them know how to do it.  I've not heard
any complaints from any of them about the $Revision$ symbols.  

Perhaps Chris might consider adding a special makefile target to client.mk for
you, one that uses the -kk option.  

Comment 21

[Wan-Teh Chang]

I think we can mark this bug fixed in NSS 3.10 because
I've checked in the patch to remove the $Name$ RCS keyword.