Re: [PATCH] Fix #define logic regarding info format handling

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi C. Michael Pilato,

I have committed your suggested patch to the CVS sources.

        Thank you,
        -- Mark

C. Michael Pilato <cmpilato@collab.net> writes:

> This is totally a drive-by patch.  I happened to be reading this section
> of code, and something didn't look quite right.  I admit a general
> ignorance of the CVS codebase, but I hope that doesn't get in the way of
> someone knowledgeable evaluating this patch.
> 
> The code segment in question is this from commit.c:
> 
>     /* On success, retrieve the new version number of the file and
>        copy it into the log information (see logmsg.c
>        (logfile_write) for more details).  We should only update
>        the version number for files that have been added or
>        modified but not removed since classify_file_internal
>        will return the version number of a file even after it has
>        been removed from the archive, which is not the behavior we
>        want for our commitlog messages; we want the old version
>        number and then "NONE, unless UseNewInfoFmtStrings has
>        been specified in the config.  Expose the real version in that
>        case and allow the trigger scripts to decide how to use it.  */
> 
>     if (ci->status != T_REMOVED
> #ifdef SUPPORT_OLD_INFO_FMT_STRINGS
>         || config->UseNewInfoFmtStrings
> #endif
>         )
>     {
>     ...
> 
> My understanding of that somewhat confusing comment is that the
> new-style info handling doesn't care if ci->status is T_REMOVED or not
> -- it will always populate the new revision slot.  Old-style info
> handling (which requires both support for such things, and configuration
> that dictates it), doesn't want the new revision for removed things --
> it just wants the old revision and then "NONE".  This all seems like a
> reasonable thing to do, too.  The comment is a bit confusing because it
> seems to run like "behavior unless condition, unless condition".
> (Comparing with the 1.11.21 sources, I can see that that final ", unless
> condition" segment was just tacked onto a version of this comment that
> predates new info format styles altogether.)
> 
> The code, however, doesn't seem to reflect that understanding, hence the
> patch.  Of course, if my understanding of the comment is wrong, then my
> patch is probably likewise wrong.
> 
> I'm not subscribed to this list, but would love to be Cc:ed on
> follow-ups to this thread.  Also, I'm happy to submit a follow-up patch
> that fixes the comment a little more comprehensible.  Thanks.
> 
> -- 
> C. Michael Pilato <cmpilato@collab.net>
> CollabNet   <>   www.collab.net   <>   Distributed Development On Demand
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFEy+TCg7APGsDnFERAhlJAKDLmOKQoJM3meKDd087N4SNtawqMwCgrXda
5B8Y2gzkmADlkOZ9+bY65ic=
=ADct
-----END PGP SIGNATURE-----