Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Karl Fogel]

Regarding this change:

Chong Yidong <cyd@stupidchicken.com> writes:
> [[[
> (bookmark-get-bookmark-record): Signal error for invalid bookmark.
> ]]]
>
> Index: bookmark.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
> retrieving revision 1.117
> retrieving revision 1.118
> diff -u -b -r1.117 -r1.118
> --- bookmark.el	29 Oct 2008 17:42:49 -0000	1.117
> +++ bookmark.el	29 Oct 2008 18:22:12 -0000	1.118
> @@ -330,7 +330,8 @@
>  (defun bookmark-get-bookmark-record (bookmark)
>    "Return the guts of the entry for BOOKMARK in `bookmark-alist'.
>  That is, all information but the name."
> -  (let ((alist (cdr (bookmark-get-bookmark bookmark))))
> +  (let ((alist (cdr (or (bookmark-get-bookmark bookmark)
> +			(error "Invalid bookmark %s" bookmark)))))
>      ;; The bookmark objects can either look like (NAME ALIST) or
>      ;; (NAME . ALIST), so we have to distinguish the two here.
>      (if (and (null (cdr alist)) (consp (caar alist)))

Seeing this change made me ask: "Why doesn't `bookmark-get-bookmark'
just return error itself, if no such bookmark?"

Answer: "Because some callers might expect it to return nil in that
case, rather than error."  (Even though it is not documented to do
either -- its doc string is silent on this question!)  I think there are
only two callers who would care, though.  In `bookmark-store', we have
this:

    (if (and (bookmark-get-bookmark stripped-name) (not no-overwrite))
        ;; already existing bookmark under that name and
        ;; no prefix arg means just overwrite old bookmark
        ;; Use the new (NAME . ALIST) format.
        (setcdr (bookmark-get-bookmark stripped-name) alist)
      ;; otherwise just cons it onto the front (either the bookmark
      ;; doesn't exist already, or there is no prefix arg.  In either
      ;; case, we want the new bookmark consed onto the alist...)
      (push (cons stripped-name alist) bookmark-alist))

And `bookmark-delete', this:

    (or (bookmark-get-bookmark bookmark-current-bookmark)
        (setq bookmark-current-bookmark nil)))

I'm tempted to add an optional NOERROR parameter to
`bookmark-get-bookmark'.  That way we'd many more error checks for free,
and could remove the special-case error check added in Chong Yidong's
change above.

Any objections to that?

As an aside: it's very difficult to review & respond to commits to
Emacs, because the ChangeLog entry arrives in a separate email from the
diff.  If the change package were kept intact (i.e., arrived as one
email, the way every other project in the universe does it), we'd
probably get more review here.

-Karl

Re: Reviewing changes (was: [Emacs-diffs] Changes to emacs/lisp/bookmark.el, v)

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Wed, 19 Nov 2008 02:50:44 -0500
> Cc: emacs-devel@gnu.org
> 
> As an aside: it's very difficult to review & respond to commits to
> Emacs, because the ChangeLog entry arrives in a separate email from the
> diff.  If the change package were kept intact (i.e., arrived as one
> email, the way every other project in the universe does it)

ChangeLog changes arriving separately is just a particular case of a
more general phenomenon: that each file's changes arrive separately,
even if they all belong to the same changeset.  CVS simply doesn't
support anything else.  But you already knew that, I'm sure.

> we'd probably get more review here.

A formal review system was suggested a couple of times to core
maintainers, but was rejected each time.  Until we have some
conventions on such reviews, there's no sense IMO to insist on VCS
changes to support it.

Re: Reviewing changes

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>> As an aside: it's very difficult to review & respond to commits to
>> Emacs, because the ChangeLog entry arrives in a separate email from the
>> diff.  If the change package were kept intact (i.e., arrived as one
>> email, the way every other project in the universe does it)
>
> ChangeLog changes arriving separately is just a particular case of a
> more general phenomenon: that each file's changes arrive separately,
> even if they all belong to the same changeset.  CVS simply doesn't
> support anything else.  But you already knew that, I'm sure.

I thought log-accum.pl was the answer to this, in CVS-land?  (It's been
a long time since I set up a CVS repository, so my memory might be
faulty...)  IIRC, log-accum at least sends one email for all the changes
in a given directory, which would cover the common case for us.  That
would be much better than the current situation, even if not perfect.

> A formal review system was suggested a couple of times to core
> maintainers, but was rejected each time.  Until we have some
> conventions on such reviews, there's no sense IMO to insist on VCS
> changes to support it.

Why let the perfect be the enemy of the good?

We don't need to have a fully-specified, formal review system to benefit
from more frequent informal reviews.  Many projects get by on just
having the diff+log appear in the same email -- then the review "system"
is simply people reading their email.  It works quite well.

I'm not against a more formal system; I just don't see how it's a
prerequisite for incrementally improving what we have.

-Karl

Re: Reviewing changes

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: cyd@stupidchicken.com,  emacs-devel@gnu.org
> Date: Wed, 19 Nov 2008 17:06:07 -0500
> 
> We don't need to have a fully-specified, formal review system to benefit
> from more frequent informal reviews.  Many projects get by on just
> having the diff+log appear in the same email -- then the review "system"
> is simply people reading their email.  It works quite well.

If there's no agreement to have a review process, I can simply ignore
your review.

Re: Reviewing changes

[Thomas Lord]

One advantage of a formal review process
is that (if designed well) it creates a 
record of the review -- there is accountability.

In Karl's system, if someone asks "were all
of the changes in this release reviewed?" karl
can say "Well, all the diffs were posted to a
mailing list."

In a formal system, one can say, "yes, here
is a list of who signed off on each change" and
perhaps even "here is the checklist they filled
in and the comments they gave in answer to more
general questions that are part of a review".

Would such accountability and such discipline of
practice improve quality?   It would seem to 
depend on the nature of the particular review 
process, and of the people in the project, 
wouldn't it?

-t



On Thu, 2008-11-20 at 00:12 +0200, Eli Zaretskii wrote:
> > From: Karl Fogel <kfogel@red-bean.com>
> > Cc: cyd@stupidchicken.com,  emacs-devel@gnu.org
> > Date: Wed, 19 Nov 2008 17:06:07 -0500
> > 
> > We don't need to have a fully-specified, formal review system to benefit
> > from more frequent informal reviews.  Many projects get by on just
> > having the diff+log appear in the same email -- then the review "system"
> > is simply people reading their email.  It works quite well.
> 
> If there's no agreement to have a review process, I can simply ignore
> your review.
> 
> 

Re: Reviewing changes

[Stephen J. Turnbull]

Karl Fogel writes:
 > Eli Zaretskii <eliz@gnu.org> writes:
 > >
 > > ChangeLog changes arriving separately is just a particular case of a
 > > more general phenomenon: that each file's changes arrive separately,
 > > even if they all belong to the same changeset.  CVS simply doesn't
 > > support anything else.

"CVS simply", no, but "CVS complicatedly", yes.

 > I thought log-accum.pl was the answer to this, in CVS-land?

I guess you would think that ;-), but apparently this is now a "lie":

    # (Ask Karl Fogel <kfogel@collab.net> if questions.)
                                        -- from CVSROOT/log_accum, l.29

Cracked me up.  Anyway, I don't know offhand what XEmacs uses, looks
like it is log_accum.  If Emacs decides to do something, ping me and
I'll investigate our configuration.

Re: Reviewing changes

[Stefan Monnier]

> I thought log-accum.pl was the answer to this, in CVS-land?  (It's been

By "thought" I guess you meant "hoped"? ;-)

> We don't need to have a fully-specified, formal review system to benefit
> from more frequent informal reviews.  Many projects get by on just
> having the diff+log appear in the same email -- then the review "system"
> is simply people reading their email.  It works quite well.

Agreed.  Improving the diff&commit messages is just a good thing.  If it
encourages reviews, even better.

> If there's no agreement to have a review process, I can simply ignore
> your review.

Of course.  Would that be a problem?


        Stefan

Re: Reviewing changes

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  cyd@stupidchicken.com,  emacs-devel@gnu.org
> Date: Wed, 19 Nov 2008 21:13:33 -0500
> 
> > If there's no agreement to have a review process, I can simply ignore
> > your review.
> 
> Of course.  Would that be a problem?

It makes the whole review process unreliable and inefficient.

Re: Reviewing changes

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: cyd@stupidchicken.com,  emacs-devel@gnu.org
>> Date: Wed, 19 Nov 2008 17:06:07 -0500
>> 
>> We don't need to have a fully-specified, formal review system to benefit
>> from more frequent informal reviews.  Many projects get by on just
>> having the diff+log appear in the same email -- then the review "system"
>> is simply people reading their email.  It works quite well.
>
> If there's no agreement to have a review process, I can simply ignore
> your review.

Sure.  You could ignore my comments on any change you make, right now.
I'm just talking about making it easier for people to review and comment
on others' changes, so that we catch more bugs.

We do have a review process currently: it consists of watching two
separate mailing lists and re-stitching together various messages in
order to comprehend (and maybe respond to) a logical change.  But every
part of that before the word "comprehend" is unnecessary work for our
developers.  The more of that stuff we can automate away, the more
inclined people will be to look at changes.

This is about making it easier to add information to the system.  What
people do with the information is up to them.  But generally, people
seem to not ignore such comments now, so we have no reason to worry that
they would suddenly start ignoring them.

[Maybe I should have read the rest of the thread before responding :-),
as it seems others agree that making review easier would be a good
thing.  I'll follow up to Stephen Turnbull's message now.]

-Karl

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Miles Bader]

Karl Fogel <kfogel@red-bean.com> writes:
> As an aside: it's very difficult to review & respond to commits to
> Emacs, because the ChangeLog entry arrives in a separate email from the
> diff.  If the change package were kept intact (i.e., arrived as one
> email, the way every other project in the universe does it), we'd
> probably get more review here.

I'd suggest tracking the git mirror instead; it does a pretty good job
of packaging up related file changes into single commits, as long as the
user committed them all at once with CVS.  Many people _don't_ do that,
of course, but that's largely for historical reasons, and hopefully they
can be convinced to start doing so.

-Miles

-- 
Advice, n. The smallest current coin.

Re: Reviewing changes

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>> > If there's no agreement to have a review process, I can simply ignore
>> > your review.
>> 
>> Of course.  Would that be a problem?
>
> It makes the whole review process unreliable and inefficient.

It is already unreliable and inefficient.  This is about making more
reliable and efficient, though not perfectly so.

Re: Reviewing changes

[Karl Fogel]

"Stephen J. Turnbull" <stephen@xemacs.org> writes:
> Karl Fogel writes:
>  > Eli Zaretskii <eliz@gnu.org> writes:
>  > >
>  > > ChangeLog changes arriving separately is just a particular case of a
>  > > more general phenomenon: that each file's changes arrive separately,
>  > > even if they all belong to the same changeset.  CVS simply doesn't
>  > > support anything else.
>
> "CVS simply", no, but "CVS complicatedly", yes.
>
>  > I thought log-accum.pl was the answer to this, in CVS-land?
>
> I guess you would think that ;-), but apparently this is now a "lie":
>
>     # (Ask Karl Fogel <kfogel@collab.net> if questions.)
>                                         -- from CVSROOT/log_accum, l.29

Er, heh.  Um.  Many facts have passed through my brain over the last
decade and a half; some of them left deeper traces than others :-).

> Cracked me up.  Anyway, I don't know offhand what XEmacs uses, looks
> like it is log_accum.  If Emacs decides to do something, ping me and
> I'll investigate our configuration.

I'm not sure how "Emacs" decides to do things, but I'm very much in
favor of having our commits produce emails that are more useful, and it
sounds like I'm not alone.  Please investigate -- let's see if we can
make it happen here!

-Karl

Re: Reviewing changes

[Stephen J. Turnbull]

Karl Fogel writes:

 > >     # (Ask Karl Fogel <kfogel@collab.net> if questions.)
 > >                                         -- from CVSROOT/log_accum, l.29
 > 
 > Er, heh.  Um.

Hey, you don't need any excuse.  Nobody has bothered you 'cause
log_accum is in "Zarro Boogs found" state!  But you might want to stop
forwarding on that email address just in case a boog should appear. ;-)

 > Please investigate -- let's see if we can make it happen here!

OK, they're not so long, so I'll just include the needed files here
(removing some historical XEmacs cruft, but leaving in any log_accum
cruft, eg, the unused read_line subroutine in commit_prep).  I don't
know if there are newer versions, but these work for us.  YMMV.

The first file is loginfo, which is a config file whose content is a
regexp -> action map.  The regexp should match the name of the file to
commit, relative to $CVSROOT.  It can be quite simple, as you see.
The second is commit_prep, which is a prehook that does some
preparatory work to allow log_accum to figure out when it's done.
AFAIK these two files must be given exactly those names, or CVS won't
find them.  The third is log_accum itself.  That file's name needs to
match the action in loginfo.  commit_prep and log_accum need to be
executable.  All of them live in $CVSROOT.  And that seems to be it.

If there are any questions I'll see what I can do, but I'm not the
person who wrangled all this stuff (I'm not entirely sure who did,
actually).

-------------------------------- loginfo --------------------------------

# The "loginfo" file controls where "cvs commit" log information
# is sent.  The first entry on a line is a regular expression which must match
# the directory that the change is being made to, relative to the
# $CVSROOT.  If a match is found, then the remainder of the line is a filter
# program that should expect log information on its standard input.
#
# If the repository name does not match any of the regular expressions in this
# file, the "DEFAULT" line is used, if it is specified.
#
# If the name ALL appears as a regular expression it is always used
# in addition to the first matching regex or DEFAULT.
#
# You may specify a format string as part of the
# filter.  The string is composed of a `%' followed
# by a single format character, or followed by a set of format
# characters surrounded by `{' and `}' as separators.  The format
# characters are:
#
#   s = file name
#   V = old version number (pre-checkin)
#   v = new version number (post-checkin)
#
# For example:
#DEFAULT (echo ""; id; echo %s; date; cat) >> $CVSROOT/CVSROOT/commitlog
# or
#DEFAULT (echo ""; id; echo %{sVv}; date; cat) >> $CVSROOT/CVSROOT/commitlog

# perhaps useful for testing
# create a random module named logtest and commit there; see if the
# mail comes!

^logtest	$CVSROOT/CVSROOT/log_accum %s

# handles commit messages for packages (core dev has moved to Mercurial)
# log_accum expects the log on stdin; the %s is the file name (see
# above), I guess you could do something like "%s (new version %v)",
# too, but I haven't tried that.

^XEmacs/packages\(/[^/]+\)*$	$CVSROOT/CVSROOT/log_accum %s

------------------------------ commit_prep ------------------------------

#! /coll/local/bin/perl
# -*-Perl-*-
#
# Perl filter to handle pre-commit checking of files.  This program
# records the last directory where commits will be taking place for
# use by the log_accum.pl script.
#
# IMPORTANT: this script interacts with log_accum, they have to agree
# on the tmpfile name to use.  See $LAST_FILE below.
#
# Contributed by David Hampton <hampton@cisco.com>
# Stripped to minimum by Roy Fielding
#
############################################################

$TMPDIR        = $ENV{'TMPDIR'} || '/tmp';
$FILE_PREFIX   = '#cvs.';

$CVS_USERNAME = $ENV{'USER'} || getlogin || (getpwuid($<))[0] || sprintf("uid#%d",$<);

# This needs to match the corresponding var in log_accum.pl, including
# the appending of the pgrp and username suffixes (see uses of this
# var farther down).
$LAST_FILE = "$TMPDIR/${FILE_PREFIX}lastdir";

sub write_line {
    my ($filename, $line) = @_;

# A check of some kind is needed here, but the rules aren't apparent
# at the moment:

#    foreach($filename, $line){	
#        $_ =~ m#^([-\@\w.\#]+)$#;
#        $_ = $1;
#    }

    open(FILE, ">$filename") || die("Cannot open $filename: $!\n");
    print(FILE $line, "\n");
    close(FILE);
}

#
# Record this directory as the last one checked.  This will be used
# by the log_accumulate script to determine when it is processing
# the final directory of a multi-directory commit.
#
$id = getpgrp();

&write_line("$LAST_FILE.$id.$CVS_USERNAME", $ARGV[0]);

sub read_line {
    local($filename) = @_;
    local($line);

    open(FILE, "<$filename") || die("Cannot open file $filename: $!\n");
    $line = <FILE>;
    close(FILE);
    chomp($line);
    $line;
}

exit(0);

------------------------------- log_accum -------------------------------

#! /coll/local/bin/perl
# -*-Perl-*-
#
# Perl filter to handle the log messages from the checkin of files in
# a directory.  This script will group the lists of files by log
# message, and mail a single consolidated log message at the end of
# the commit.
#
# This file assumes a pre-commit checking program that leaves the
# names of the first and last commit directories in a temporary file.
#
# IMPORTANT: what the above means is, this script interacts with
# commit_prep, in that they have to agree on the tmpfile name to use.
# See $LAST_FILE below. 
#
# How this works: CVS triggers this script once for each directory
# involved in the commit -- in other words, a single commit can invoke
# this script N times.  It knows when it's on the last invocation by
# examining the contents of $LAST_FILE.  Between invocations, it
# caches information for its future incarnations in various temporary
# files in /tmp, which are named according to the process group and
# the committer (by themselves, neither of these are unique, but
# together they almost always are, unless the same user is doing two
# commits simultaneously).  The final invocation is the one that
# actually sends the mail -- it gathers up the cached information,
# combines that with what it found out on this pass, and sends a
# commit message to the appropriate mailing list.
#
# (Ask Karl Fogel <kfogel@collab.net> if questions.)
#
# Contributed by David Hampton <hampton@cisco.com>
# Roy Fielding removed useless code and added log/mail of new files
# Ken Coar added special processing (i.e., no diffs) for binary files
#

############################################################
#
# Configurable options
#
############################################################
#
# Where do you want the RCS ID and delta info?
# 0 = none,
# 1 = in mail only,
# 2 = in both mail and logs.
#
$rcsidinfo = 2;

#if you are using CVS web then set this to some value... if not set it to ""
#
# When set properly, this will cause links to aspects of the project to
# print in the commit emails.
#$CVSWEB_SCHEME = "http";
#$CVSWEB_DOMAIN = "cvshome.org";
#$CVSWEB_PORT = "80";
#$CVSWEB_URI = "source/browse/";
#$SEND_URL = "true";
$SEND_DIFF = "true";


# Set this to a domain to have CVS pretend that all users who make
# commits have mail accounts within that domain.
#$EMULATE_LOCAL_MAIL_USER="cvshome.org"; 

# Set this to '-c' for context diffs; defaults to '-u' for unidiff format.
$difftype = '-uNp';

############################################################
#
# Constants
#
############################################################
$STATE_NONE    = 0;
$STATE_CHANGED = 1;
$STATE_ADDED   = 2;
$STATE_REMOVED = 3;
$STATE_LOG     = 4;

$TMPDIR        = $ENV{'TMPDIR'} || '/tmp';
$FILE_PREFIX   = '#cvs.';

$LAST_FILE     = "$TMPDIR/${FILE_PREFIX}lastdir";  # Created by commit_prep!
$ADDED_FILE    = "$TMPDIR/${FILE_PREFIX}files.added";
$REMOVED_FILE  = "$TMPDIR/${FILE_PREFIX}files.removed";
$LOG_FILE      = "$TMPDIR/${FILE_PREFIX}files.log";
$BRANCH_FILE   = "$TMPDIR/${FILE_PREFIX}files.branch";
$MLIST_FILE    = "$TMPDIR/${FILE_PREFIX}files.mlist";
$SUMMARY_FILE  = "$TMPDIR/${FILE_PREFIX}files.summary";

$CVSROOT       = $ENV{'CVSROOT'};

$MAIL_CMD      = "| /coll/local/bin/sendmail -i -t";
#$MAIL_CMD      = "| /var/qmail/bin/qmail-inject";
$MAIL_FROM     = 'xemacs-cvs@xemacs.org';  #not needed if EMULATE_LOCAL_MAIL_USER
$SUBJECT_PRE   = 'CVS update';


############################################################
#
# Subroutines
#
############################################################

sub format_names {
    local($dir, @files) = @_;
    local(@lines);

    $lines[0] = sprintf(" %-08s", $dir);
    foreach $file (@files) {
        if (length($lines[$#lines]) + length($file) > 60) {
            $lines[++$#lines] = sprintf(" %8s", " ");
        }
        $lines[$#lines] .= " ".$file;
    }
    @lines;
}

sub cleanup_tmpfiles {
    local(@files);

    opendir(DIR, $TMPDIR);
    push(@files, grep(/^${FILE_PREFIX}.*\.${id}\.${cvs_user}$/, readdir(DIR)));
    closedir(DIR);
    foreach (@files) {
        unlink "$TMPDIR/$_";
    }
}

sub write_logfile {
    local($filename, @lines) = @_;

    open(FILE, ">$filename") || die ("Cannot open log file $filename: $!\n");
    print(FILE join("\n", @lines), "\n");
    close(FILE);
}

sub append_to_file {
    local($filename, $dir, @files) = @_;

    if (@files) {
        local(@lines) = &format_names($dir, @files);
        open(FILE, ">>$filename") || die ("Cannot open file $filename: $!\n");
        print(FILE join("\n", @lines), "\n");
        close(FILE);
    }
}

sub write_line {
    local($filename, $line) = @_;

    open(FILE, ">$filename") || die("Cannot open file $filename: $!\n");
    print(FILE $line, "\n");
    close(FILE);
}

sub append_line {
    local($filename, $line) = @_;

    open(FILE, ">>$filename") || die("Cannot open file $filename: $!\n");
    print(FILE $line, "\n");
    close(FILE);
}

sub read_line {
    local($filename) = @_;
    local($line);

    open(FILE, "<$filename") || die("Cannot open file $filename: $!\n");
    $line = <FILE>;
    close(FILE);
    chomp($line);
    $line;
}

sub read_line_nodie {
    local($filename) = @_;
    local($line);
    open(FILE, "<$filename") || return ("");

    $line = <FILE>;
    close(FILE);
    chomp($line);
    $line;
}

sub read_file_lines {
    local($filename) = @_;
    local(@text) = ();

    open(FILE, "<$filename") || return ();
    while (<FILE>) {
        chomp;
        push(@text, $_);
    }
    close(FILE);
    @text;
}

sub read_file {
    local($filename, $leader) = @_;
    local(@text) = ();

    open(FILE, "<$filename") || return ();
    while (<FILE>) {
        chomp;
        push(@text, sprintf("  %-10s  %s", $leader, $_));
        $leader = "";
    }
    close(FILE);
    @text;
}

sub read_logfile {
    local($filename, $leader) = @_;
    local(@text) = ();

    open(FILE, "<$filename") || die ("Cannot open log file $filename: $!\n");
    while (<FILE>) {
        chomp;
        push(@text, $leader.$_);
    }
    close(FILE);
    @text;
}

#
# do an 'cvs -Qn status' on each file in the arguments, and extract info.
#
sub change_summary {
    local($out, @filenames) = @_;
    local(@revline);
    local($file, $rev, $rcsfile, $line, $vhost, $cvsweb_base);

    while (@filenames) {

        $file = shift @filenames;

        if ("$file" eq "") {
            next;
        }

        open(RCS, "-|") || exec "$cvsbin/cvs", '-Qn', 'status', '--', $file;

        $rev = "";
        $delta = "";
        $rcsfile = "";

        while (<RCS>) {
            if (/^[ \t]*Repository revision/) {
                chomp;
                @revline = split(' ', $_);
                $rev = $revline[2];
                $rcsfile = $revline[3];
                $rcsfile =~ s,^$CVSROOT/,,;
                $rcsfile =~ s/,v$//;
            }
        }
        close(RCS);

        if ($rev ne '' && $rcsfile ne '') {
            open(RCS, "-|") || exec "$cvsbin/cvs", '-Qn', 'log', "-r$rev",
				    '--', $file;
            while (<RCS>) {
                if (/^date:/) {
                    chomp;
                    $delta = $_;
                    $delta =~ s/^.*;//;
                    $delta =~ s/^[\s]+lines://;
                }
            }
            close(RCS);
        }

        $diff = "\n\n";
        $vhost = $path[0];
        if ($CVSWEB_PORT eq "80") {
          $cvsweb_base = "$CVSWEB_SCHEME://$vhost.$CVSWEB_DOMAIN/$CVSWEB_URI";
        }
        else {
          $cvsweb_base = "$CVSWEB_SCHEME://$vhost.$CVSWEB_DOMAIN:$CVSWEB_PORT/$CVSWEB_URI";
        }
        if ($SEND_URL eq "true") {
          $diff .= $cvsweb_base . join("/", @path) . "/$file";
        }

        #
        # If this is a binary file, don't try to report a diff; not only is
        # it meaningless, but it also screws up some mailers.  We rely on
        # Perl's 'is this binary' algorithm; it's pretty good.  But not
        # perfect.
        #
        if (($file =~ /\.(?:pdf|gif|jpg|mpg)$/i) || (-B $file)) {
          if ($SEND_URL eq "true") {
            $diff .= "?rev=$rev&content-type=text/x-cvsweb-markup\n\n";
          }
          if ($SEND_DIFF eq "true") {
            $diff .= "\t<<Binary file>>\n\n";
          }
        }
        else {
            #
            # Get the differences between this and the previous revision,
            # being aware that new files always have revision '1.1' and
            # new branches always end in '.n.1'.
            #
            if ($rev =~ /^(.*)\.([0-9]+)$/) {
                $prev = $2 - 1;
                $prev_rev = $1 . '.' .  $prev;

                $prev_rev =~ s/\.[0-9]+\.0$//;# Truncate if first rev on branch

                if ($rev eq '1.1') {
                  if ($SEND_URL eq "true") {
                    $diff .= "?rev=$rev&content-type=text/x-cvsweb-markup\n\n";
                  }
                  if ($SEND_DIFF eq "true") {
                    open(DIFF, "-|")
                      || exec "$cvsbin/cvs", '-Qn', 'update', '-p', '-r1.1',
			      '--', $file;
                    $diff .= "Index: $file\n=================================="
                      . "=================================\n";
                  }
                }
                else {
                  if ($SEND_URL eq "true") {
                    $diff .= ".diff?r1=$prev_rev&r2=$rev\n\n";
                  }
                  if ($SEND_DIFF eq "true") {
                    open(DIFF, "-|")
                      || exec "$cvsbin/cvs", '-Qn', 'diff', "$difftype",
                      "-r$prev_rev", "-r$rev", '--', $file;
                  }
                }

                if ($SEND_DIFF eq "true") {
                  while (<DIFF>) {
                    $diff .= $_;
                  }
                  close(DIFF);
                }
                $diff .= "\n\n";
            }
        }

        &append_line($out, sprintf("%-9s%-12s%s%s", $rev, $delta,
                                   $rcsfile, $diff));

    }
}


sub build_header {
    local($header);
    delete $ENV{'TZ'};
    local($sec,$min,$hour,$mday,$mon,$year) = localtime(time);

    $header = sprintf("  User: %-8s\n  Date: %02d/%02d/%02d %02d:%02d:%02d",
                       $cvs_user, $year%100, $mon+1, $mday,
                       $hour, $min, $sec);
#    $header = sprintf("%-8s    %02d/%02d/%02d %02d:%02d:%02d",
#                       $login, $year%100, $mon+1, $mday,
#                       $hour, $min, $sec);
}

# !!! Destination Mailing-list and history file mappings here !!!

sub mlist_map
{
    local($path) = @_;
    return "xemacs-cvs\@xemacs.org";
#    my $domain = "cvshome.org";
#    
#    if ($path =~ /^([^\/]+)/) {
#        return "cvs\@$1.$domain";
#    } else {
#        return "cvs\@$domain";
#    }
}

sub derive_subject_from_changes_file ()
{
  my $subj = "";

  for ($i = 0; ; $i++)
  {
    open (CH, "<$CHANGED_FILE.$i.$id.$cvs_user") or last;

    while (my $change = <CH>)
    {
      # A changes file looks like this:
      #
      #  src      foo.c newfile.html
      #  www      index.html project_nav.html
      #
      # Each line is " Dir File1 File2 ..."
      # We only care about Dir, since the subject line should
      # summarize. 
      
      $change =~ s/^[ \t]*//;
      $change =~ /^([^ \t]+)[ \t]*/;
      my $dir = $1;
      # Fold to rightmost directory component
      # $dir =~ /([^\/]+)$/;
      # $dir = $1;
      if ($subj eq "") {
        $subj = $dir;
      } else {
        $subj .= ", $dir"; 
      }
    }
    close (CH);
  }

  if ($subj ne "") {
      $subj = "$subj ..."; 
  }
  else {
      # NPM: See if there's any file-addition notifications.
      my $added = &read_line_nodie("$ADDED_FILE.$i.$id.$cvs_user");
      if ($added ne "") {
          $subj .= "$added "; 
      }
    
#    print "derive_subject_from_changes_file().. added== $added \n";
    
       ## NPM: See if there's any file-removal notications.
      my $removed = &read_line_nodie("$REMOVED_FILE.$i.$id.$cvs_user");
      if ($removed ne "") {
          $subj .= "$removed "; 
      }
    
#    print "derive_subject_from_changes_file().. removed== $removed \n";
    
      ## NPM: See if there's any branch notifications.
      my $branched = &read_line_nodie("$BRANCH_FILE.$i.$id.$cvs_user");
      if ($branched ne "") {
          $subj .= "$branched"; 
      }
    
#    print "derive_subject_from_changes_file().. branched== $branched \n";
    
      ## NPM: DEFAULT: DIRECTORY CREATION (c.f. "Check for a new directory first" in main mody)
      if ($subj eq "") {
          my $subject = join("/", @path);
          $subj = "$subject"; 
      }    
  }

  return $subj;
}

sub mail_notification
{
    local($addr_list, $log_subject, @text) = @_;
    local($mail_to);

    my $subj = &derive_subject_from_changes_file ();

    if ($EMULATE_LOCAL_MAIL_USER ne "") {
        $MAIL_FROM = "$cvs_user\@$EMULATE_LOCAL_MAIL_USER";
    }

    $mail_to = join(", ", @{$addr_list});

    if ($log_subject eq '') {
      print "Mailing the commit message to $mail_to (from $MAIL_FROM)\n";
    } else {
      print "Mailing the commit message ($log_subject) to $mail_to (from $MAIL_FROM)\n";
    }

    $ENV{'MAILUSER'} = $MAIL_FROM;
    # Commented out on hocus, so comment it out here.  -kff
    # $ENV{'QMAILINJECT'} = 'f';

    open(MAIL, "$MAIL_CMD -f$MAIL_FROM");
    print MAIL "From: $MAIL_FROM\n";
    print MAIL "To: $mail_to\n";
    if ($log_subject eq '') {
      print MAIL "Subject: $SUBJECT_PRE by $cvs_user $subj\n\n";
    } else {
      print MAIL "Subject: $SUBJECT_PRE by $cvs_user ($log_subject) $subj\n\n";
    }
    print(MAIL join("\n", @text));
    close(MAIL);
#    print "Mailing the commit message to $MAIL_TO...\n";
#
#    #added by jrobbins@collab.net 1999/12/15
#    # attempt to get rid of anonymous
#    $ENV{'MAILUSER'} = 'commitlogger';
#    $ENV{'QMAILINJECT'} = 'f';
#
#    open(MAIL, "| /var/qmail/bin/qmail-inject");
#    print(MAIL "To: $MAIL_TO\n"); 
#    print(MAIL "Subject: cvs commit: $ARGV[0]\n"); 
#    print(MAIL join("\n", @text));
#    close(MAIL);
}

## process the command line arguments sent to this script
## it returns an array of files, %s, sent from the loginfo
## command
sub process_argv
{
    local(@argv) = @_;
    local(@files);
    local($arg);
    print "Processing log script arguments...\n";

    while (@argv) {
        $arg = shift @argv;

        if ($arg eq '-u') {
                $cvs_user = shift @argv;
        } else {
                ($donefiles) && die "Too many arguments!\n";
                $donefiles = 1;
                $ARGV[0] = $arg;
                @files = split(' ', $arg);
        }
    }
    return @files;
}


#############################################################
#
# Main Body
#
############################################################
#
# Setup environment
#
umask (002);

# Connect to the database
$cvsbin = "/coll/local/bin";

#
# Initialize basic variables
#
$id = getpgrp();
$state = $STATE_NONE;
$cvs_user = $ENV{'USER'} || getlogin || (getpwuid($<))[0] || sprintf("uid#%d",$<);
@files = process_argv(@ARGV);
@path = split('/', $files[0]);
if ($#path == 0) {
    $dir = ".";
} else {
    $dir = join('/', @path[1..$#path]);
}
#print("ARGV  - ", join(":", @ARGV), "\n");
#print("files - ", join(":", @files), "\n");
#print("path  - ", join(":", @path), "\n");
#print("dir   - ", $dir, "\n");
#print("id    - ", $id, "\n");

#
# Map the repository directory to an email address for commitlogs to be sent
# to.
#
$mlist = &mlist_map($files[0]);

##########################
#
# Check for a new directory first.  This will always appear as a
# single item in the argument list, and an empty log message.
#
if ($ARGV[0] =~ /New directory/) {
    $header = &build_header;
    @text = ();
    push(@text, $header);
    push(@text, "");
    push(@text, "  ".$ARGV[0]);
    &mail_notification([ $mlist ], @text);
    exit 0;
}

#
# Iterate over the body of the message collecting information.
#
while (<STDIN>) {
    chomp;                      # Drop the newline
    if (/^Revision\/Branch:/) {
        s,^Revision/Branch:,,;
        push (@branch_lines, split);
        next;
    }
    if (/^[ \t]+Tag: /) {
        s,^[ \t]+Tag: ,,;
        push (@branch_lines, split);
        next;
    }
    if (/^[ \t]+No tag$/) {
        next;
    }

#    next if (/^[ \t]+Tag:/ && $state != $STATE_LOG);
    if (/^Modified Files/) { $state = $STATE_CHANGED; next; }
    if (/^Added Files/)    { $state = $STATE_ADDED;   next; }
    if (/^Removed Files/)  { $state = $STATE_REMOVED; next; }
    if (/^Log Message/)    { $state = $STATE_LOG;     next; }
    s/[ \t\n]+$//;              # delete trailing space
    
    push (@changed_files, split) if ($state == $STATE_CHANGED);
    push (@added_files,   split) if ($state == $STATE_ADDED);
    push (@removed_files, split) if ($state == $STATE_REMOVED);
    if ($state == $STATE_LOG) {
        if (/^PR:$/i ||
            /^Reviewed by:$/i ||
            /^Submitted by:$/i ||
            /^Obtained from:$/i) {
            next;
        }
        push (@log_lines,     $_);
    }
}

#
# Strip leading and trailing blank lines from the log message.  Also
# compress multiple blank lines in the body of the message down to a
# single blank line.
# (Note, this only does the mail and changes log, not the rcs log).
#
while ($#log_lines > -1) {
    last if ($log_lines[0] ne "");
    shift(@log_lines);
}
while ($#log_lines > -1) {
    last if ($log_lines[$#log_lines] ne "");
    pop(@log_lines);
}
for ($i = $#log_lines; $i > 0; $i--) {
    if (($log_lines[$i - 1] eq "") && ($log_lines[$i] eq "")) {
        splice(@log_lines, $i, 1);
    }
}

#
# Find the log file that matches this log message
#
for ($i = 0; ; $i++) {
    last if (! -e "$LOG_FILE.$i.$id.$cvs_user");
    @text = &read_logfile("$LOG_FILE.$i.$id.$cvs_user", "");
    last if ($#text == -1);
    last if (join(" ", @log_lines) eq join(" ", @text));
}

#
# Spit out the information gathered in this pass.
#
&write_logfile("$LOG_FILE.$i.$id.$cvs_user", @log_lines);
&append_to_file("$BRANCH_FILE.$i.$id.$cvs_user",  $dir, @branch_lines);
&append_to_file("$ADDED_FILE.$i.$id.$cvs_user",   $dir, @added_files);
&append_to_file("$CHANGED_FILE.$i.$id.$cvs_user", $dir, @changed_files);
&append_to_file("$REMOVED_FILE.$i.$id.$cvs_user", $dir, @removed_files);
&append_line("$MLIST_FILE.$i.$id.$cvs_user", $mlist);
if ($rcsidinfo) {
    &change_summary("$SUMMARY_FILE.$i.$id.$cvs_user", (@changed_files, @added_files));
}

#
# Check whether this is the last directory.  If not, quit.
#
if (-e "$LAST_FILE.$id.$cvs_user") {
   $_ = &read_line("$LAST_FILE.$id.$cvs_user");
   $tmpfiles = $files[0];
   $tmpfiles =~ s,([^a-zA-Z0-9_/]),\\$1,g;
   if (! grep(/$tmpfiles$/, $_)) {
        print "More commits to come...\n";
        exit 0
   }
}

#
# This is it.  The commits are all finished.  Lump everything together
# into a single message, fire a copy off to the mailing list, and drop
# it on the end of the Changes file.
#
$header = &build_header;

#
# Produce the final compilation of the log messages
#
@text = ();
@mlist_list = ();
push(@text, $header);
push(@text, "");
for ($i = 0; ; $i++) {
    last if (! -e "$LOG_FILE.$i.$id.$cvs_user");
    push(@text, &read_file("$BRANCH_FILE.$i.$id.$cvs_user", "Branch:"));
    push(@text, &read_file("$CHANGED_FILE.$i.$id.$cvs_user", "Modified:"));
    push(@text, &read_file("$ADDED_FILE.$i.$id.$cvs_user", "Added:"));
    push(@text, &read_file("$REMOVED_FILE.$i.$id.$cvs_user", "Removed:"));
    push(@text, "Log:");
    push(@text, &read_logfile("$LOG_FILE.$i.$id.$cvs_user", ""));
    push(@mlist_list, &read_file_lines("$MLIST_FILE.$i.$id.$cvs_user"));
    if ($rcsidinfo == 2) {
        if (-e "$SUMMARY_FILE.$i.$id.$cvs_user") {
            push(@text, "");
            push(@text, "Revision  Changes    Path");
            push(@text, &read_logfile("$SUMMARY_FILE.$i.$id.$cvs_user", ""));
        }
    }
    push(@text, "");
}

#
# Now generate the extra info for the mail message..
#
if ($rcsidinfo == 1) {
    $revhdr = 0;
    for ($i = 0; ; $i++) {
        last if (! -e "$LOG_FILE.$i.$id.$cvs_user");
        if (-e "$SUMMARY_FILE.$i.$id.$cvs_user") {
            if (!$revhdr++) {
                push(@text, "Revision  Changes    Path");
            }
            push(@text, &read_logfile("$SUMMARY_FILE.$i.$id.$cvs_user", ""));
        }
    }
    if ($revhdr) {
        push(@text, "");        # consistancy...
    }
}

#
# See if the log has a Subject: line
#
my $log_subject = "";
foreach (@log_lines) {
  if (index($_, "Subject: ") == 0) {
    $log_subject = substr($_, 9);
    last;
  }
}

%mlist_hash = ();

foreach (@mlist_list) { $mlist_hash{ $_ } = 1; }

#
# Mail out the notification.
#
&mail_notification([ keys(%mlist_hash) ], $log_subject, @text);
&cleanup_tmpfiles;
exit 0;

--------------------------- end of inclusions ---------------------------

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Andreas Schwab]

Miles Bader <miles@gnu.org> writes:

> I'd suggest tracking the git mirror instead; it does a pretty good job
> of packaging up related file changes into single commits, as long as the
> user committed them all at once with CVS.

Those are also exactly the commits that show up as a single mail on
emacs-commit.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Miles Bader]

Andreas Schwab <schwab@suse.de> writes:
>> I'd suggest tracking the git mirror instead; it does a pretty good job
>> of packaging up related file changes into single commits, as long as the
>> user committed them all at once with CVS.
>
> Those are also exactly the commits that show up as a single mail on
> emacs-commit.

Hmmm, so all the more reason for people to stop committing stuff
separately... :-/

-Miles

-- 
Christian, n. One who follows the teachings of Christ so long as they are not
inconsistent with a life of sin.

Re: Reviewing changes

[Yavor Doganov]

Karl Fogel wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > ChangeLog changes arriving separately is just a particular case of a
> > more general phenomenon: that each file's changes arrive separately,
> > even if they all belong to the same changeset.  CVS simply doesn't
> > support anything else.  But you already knew that, I'm sure.
> 
> I thought log-accum.pl was the answer to this, in CVS-land?  (It's been
> a long time since I set up a CVS repository, so my memory might be
> faulty...)  IIRC, log-accum at least sends one email for all the changes
> in a given directory, which would cover the common case for us.  That
> would be much better than the current situation, even if not perfect.

The log_accum.pl script used by Savannah by default sends all of the
changes (summary + commit message + diffs) in one email even if the
commit touches files in different directories (like [1]).  Note that
with the recent size limit that was set up for lists.gnu.org by the
GNU sysadmins, some messages are not delivered if the diffs are too
large.

The Emacs case is special (two separate lists and single-file chanages
even if the files were committed enbloc), and probably was requested
by the maintainers long time ago for consistency with the policy that
was in place until recently.

[1] http://lists.gnu.org/archive/html/trans-coord-devel/2008-11/msg00005.html

Re: Reviewing changes

[Stefan Monnier]

>> > If there's no agreement to have a review process, I can simply ignore
>> > your review.
>> Of course.  Would that be a problem?
> It makes the whole review process unreliable and inefficient.

I don't follow you.  We already have code reviews, just rarely so.
And we can already ignore them, they're already unreliable.


        Stefan


PS: In practice, we don't seem to ignore them, tho (maybe because
they're rare, but still).

Re: Reviewing changes

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: kfogel@red-bean.com,  cyd@stupidchicken.com,  emacs-devel@gnu.org
> Date: Thu, 20 Nov 2008 09:30:25 -0500
> 
> >> > If there's no agreement to have a review process, I can simply ignore
> >> > your review.
> >> Of course.  Would that be a problem?
> > It makes the whole review process unreliable and inefficient.
> 
> I don't follow you.

I really don't understand why.  It's not like we are inventing some
new practices here.  Code reviews are used in almost any organization
that develops software.  Books are written on how to do that, and none
of those I've read recommend what is being suggested here as "good,
though not perfect".

IMO, if we cannot do it well, it isn't worth doing.

> We already have code reviews, just rarely so.

No, we don't.  What we have is random (and very rare) comments, and no
mechanism to resolve disagreements when they happen.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Eli Zaretskii]

> From: Miles Bader <miles@gnu.org>
> Date: Thu, 20 Nov 2008 19:09:38 +0900
> Cc: Karl Fogel <kfogel@red-bean.com>, Chong Yidong <cyd@stupidchicken.com>,
> 	emacs-devel@gnu.org
> 
> Hmmm, so all the more reason for people to stop committing stuff
> separately... :-/

As long as we use CVS, which lacks the means to see the change history
of a single file, committing several files at once makes "cvs log"
unusable for the purposes of tracking the history of changes to some
specific code fragment in a single file, something I need to do a lot
in Emacs.

That is why I commit the files individually, and then commit all their
ChangeLog entries in one go.  Don't expect me to change that any time
soon.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Juanma Barranquero]

On Thu, Nov 20, 2008 at 21:22, Eli Zaretskii <eliz@gnu.org> wrote:

> As long as we use CVS, which lacks the means to see the change history
> of a single file, committing several files at once makes "cvs log"
> unusable for the purposes of tracking the history of changes to some
> specific code fragment in a single file

Why, if I may ask?

  Juanma

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Eli Zaretskii]

> Date: Thu, 20 Nov 2008 21:32:01 +0100
> From: "Juanma Barranquero" <lekktu@gmail.com>
> Cc: kfogel@red-bean.com, schwab@suse.de, cyd@stupidchicken.com,
> 	emacs-devel@gnu.org, Miles Bader <miles@gnu.org>
> 
> On Thu, Nov 20, 2008 at 21:22, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > As long as we use CVS, which lacks the means to see the change history
> > of a single file, committing several files at once makes "cvs log"
> > unusable for the purposes of tracking the history of changes to some
> > specific code fragment in a single file
> 
> Why, if I may ask?

Because the changes to the file I'm interested in are all but lost in
the heap of changes to other files.

Re: Reviewing changes

[Stefan Monnier]

> IMO, if we cannot do it well, it isn't worth doing.

Sounds surprisingly dogmatic.

>> We already have code reviews, just rarely so.
> No, we don't.  What we have is random (and very rare) comments, and no
> mechanism to resolve disagreements when they happen.

So you object to naming them "code reviews"?  That's OK, let's call them
"random comments", then.

I just can't see how encouraging such random comments can hurt.


        Stefan

Re: Reviewing changes

[Stefan Monnier]

> The log_accum.pl script used by Savannah by default sends all of the
> changes (summary + commit message + diffs) in one email even if the
> commit touches files in different directories (like [1]).  Note that
> with the recent size limit that was set up for lists.gnu.org by the
> GNU sysadmins, some messages are not delivered if the diffs are too
> large.

> The Emacs case is special (two separate lists and single-file chanages
> even if the files were committed enbloc), and probably was requested
> by the maintainers long time ago for consistency with the policy that
> was in place until recently.

It was a mistake.  Could you arrange to fix it: use for Emacs the
"single email per commit" script used by default (containing
summary+message+diff), and send it to the emacs-diffs mailing list.
We can keep the emacs-commits mailing list unchanged.


        Stefan

Re: Reviewing changes

[mail]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> No, we don't.  What we have is random (and very rare) comments, and no
>> mechanism to resolve disagreements when they happen.
>
> So you object to naming them "code reviews"?  That's OK, let's call them
> "random comments", then.
>
> I just can't see how encouraging such random comments can hurt.
>

Agreed, this isn't a proposal for "code reviews", but for more frequent
random comments.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
> As long as we use CVS, which lacks the means to see the change history
> of a single file, 

Hrm?  'cvs log FILENAME'

> committing several files at once makes "cvs log"
> unusable for the purposes of tracking the history of changes to some
> specific code fragment in a single file, something I need to do a lot
> in Emacs.

'cvs annotate'

...and, of course, http://www.red-bean.com/cvs2cl/, which re-unifies CVS
changesets quite reliably.  http://cvs2svn.tigris.org/ also does so
(note that it can be used for conversion to git as well as to svn,
despite its name -- really, it's mainly a CVS changeset re-unifier, and
only secondarily a converter).  These tools are fairly widely used.

> That is why I commit the files individually, and then commit all their
> ChangeLog entries in one go.  Don't expect me to change that any time
> soon.

By doing so, you are tossing away information that *can* be
reconstructed (as per above).

When we convert to a modern version control system, other people's
multi-file changes will be unified back into single changesets, but
yours will not be, if you commit in the way you describe above.

-Karl

Re: Reviewing changes

[Eli Zaretskii]

> So you object to naming them "code reviews"?  That's OK, let's call them
> "random comments", then.
> 
> I just can't see how encouraging such random comments can hurt.

I didn't say it will hurt.  I said it won't help us make the code
quality better, which I assumed what this was all about.  Sounds like
my assumption was wrong.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Miles Bader <miles@gnu.org>,  schwab@suse.de,  cyd@stupidchicken.com,  emacs-devel@gnu.org
> Date: Fri, 21 Nov 2008 00:38:45 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > As long as we use CVS, which lacks the means to see the change history
> > of a single file, 
> 
> Hrm?  'cvs log FILENAME'

That's the whole point: if several files are committed in one go, the
log message for each file will be usually taken from several ChangeLog
entries, sometimes from several different directories (e.g., if C
files and Lisp files are modified together, or if the documentation is
changed as well).  Then "cvs log FILENAME" will show a huge pile of
different entries for each FILENAME in the changeset.  What I want is
an option to "cvs log" that will show only the entries of the one file
I'm interested in.  CVS doesn't record that information, so I'm down
to grepping "cvs log" output, which is unreliable because I don't
always know what I'm looking for, and because of inconsistencies in
how the ChangeLog entries are formatted.

> > committing several files at once makes "cvs log"
> > unusable for the purposes of tracking the history of changes to some
> > specific code fragment in a single file, something I need to do a lot
> > in Emacs.
> 
> 'cvs annotate'

Not useful for tracking history, since it lacks the ability to show a
series of changes, or at least I'm not familiar with a way of having
it do that.

> > That is why I commit the files individually, and then commit all their
> > ChangeLog entries in one go.  Don't expect me to change that any time
> > soon.
> 
> By doing so, you are tossing away information that *can* be
> reconstructed (as per above).

That information is available in the ChangeLog files.

> When we convert to a modern version control system, other people's
> multi-file changes will be unified back into single changesets, but
> yours will not be, if you commit in the way you describe above.

Sorry, I hate wasting many minutes, sometimes hours, looking for a
history of a specific code fragment, so I will not make this problem
more grave by my own contributions.  When we switch to a more modern
VCS, I will review my practices, of course, but until then, we are
stuck with CVS and its limitations.

Re: Reviewing changes

[Stefan Monnier]

>> So you object to naming them "code reviews"?  That's OK, let's call them
>> "random comments", then.
>> I just can't see how encouraging such random comments can hurt.

> I didn't say it will hurt.  I said it won't help us make the code
> quality better, which I assumed what this was all about.  Sounds like
> my assumption was wrong.

No, this was all about asking to change the format of the email sent to
emacs-diffs and emacs-commit so that they'd contain both the changelog
and the diffs (hopefully of all the files modified by the given
commit) together.
Karl mentioned he'd prefer it, and also added that it would make
reviewing easier.

Your argumentation seemed to say that you disagreed with such a change
specifically because it would make people start to do such
pseudo-reviewing which is worse than not doing reviewing at all.


        Stefan

Re: Reviewing changes

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: kfogel@red-bean.com,  cyd@stupidchicken.com,  emacs-devel@gnu.org
> Date: Fri, 21 Nov 2008 09:32:28 -0500
> 
> >> So you object to naming them "code reviews"?  That's OK, let's call them
> >> "random comments", then.
> >> I just can't see how encouraging such random comments can hurt.
> 
> > I didn't say it will hurt.  I said it won't help us make the code
> > quality better, which I assumed what this was all about.  Sounds like
> > my assumption was wrong.
> 
> No, this was all about asking to change the format of the email sent to
> emacs-diffs and emacs-commit so that they'd contain both the changelog
> and the diffs (hopefully of all the files modified by the given
> commit) together.
> Karl mentioned he'd prefer it, and also added that it would make
> reviewing easier.

Karl _asked_ for it, and he said he was asking _because_ it will make
reviewing easier.

> Your argumentation seemed to say that you disagreed with such a change
> specifically because it would make people start to do such
> pseudo-reviewing which is worse than not doing reviewing at all.

I don't think I wrote anything that could be read like this.  In
particular, I don't think I ever said I disagreed with the change.

Re: Reviewing changes

[Stefan Monnier]

>> Your argumentation seemed to say that you disagreed with such a change
>> specifically because it would make people start to do such
>> pseudo-reviewing which is worse than not doing reviewing at all.

> I don't think I wrote anything that could be read like this.

Never underestimate my ability misunderstand.

> In particular, I don't think I ever said I disagreed with the change.

Great, thanks,


        Stefan

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Stefan Monnier]

> I'm tempted to add an optional NOERROR parameter to
> `bookmark-get-bookmark'.  That way we'd many more error checks for free,

Done,


        Stefan

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

 > That's the whole point: if several files are committed in one go, the
 > log message for each file will be usually taken from several ChangeLog
 > entries, sometimes from several different directories (e.g., if C
 > files and Lisp files are modified together, or if the documentation is
 > changed as well).  Then "cvs log FILENAME" will show a huge pile of
 > different entries for each FILENAME in the changeset.  What I want is
 > an option to "cvs log" that will show only the entries of the one file
 > I'm interested in.  CVS doesn't record that information, so I'm down
 > to grepping "cvs log" output, which is unreliable because I don't
 > always know what I'm looking for, and because of inconsistencies in
 > how the ChangeLog entries are formatted.

How about something in the direction of the attached patch?

martin

[-- Attachment #2: log-view.diff --]
[-- Type: text/plain, Size: 5866 bytes --]

*** log-view.el.~1.55.~	2008-06-16 11:28:44.000000000 +0200
--- log-view.el	2008-11-23 20:06:16.218750000 +0100
***************
*** 137,142 ****
--- 137,145 ----
      ([backtab] . log-view-msg-prev)
      ("N" . log-view-file-next)
      ("P" . log-view-file-prev)
+     ("o" . log-view-show-object-only)
+     ("c" . log-view-show-current-only)
+     ("y" . log-view-show-any)
      ("\M-n" . log-view-file-next)
      ("\M-p" . log-view-file-prev))
    "Log-View's keymap."
***************
*** 171,177 ****
      ["Next File"  log-view-file-next
       :help "Go to the next count'th file"]
      ["Previous File"  log-view-file-prev
!      :help "Go to the previous count'th file"]))
  
  (defvar log-view-mode-hook nil
    "Hook run at the end of `log-view-mode'.")
--- 174,187 ----
      ["Next File"  log-view-file-next
       :help "Go to the next count'th file"]
      ["Previous File"  log-view-file-prev
!      :help "Go to the previous count'th file"]
!     "-----"
!     ["Show Object Only"  log-view-show-object-only
!      :help "Show entries for object at point only"]
!     ["Show Current Only"  log-view-show-current-only
!      :help "Show entries for current file only"]
!     ["Show Any"  log-view-show-any
!      :help "Show any entry"]))
  
  (defvar log-view-mode-hook nil
    "Hook run at the end of `log-view-mode'.")
***************
*** 532,537 ****
--- 542,673 ----
       (list log-view-vc-backend nil)
       to fr)))
  
+ 
+ (defun log-view-hide (from to)
+   "Hide text within FROM and TO."
+   (when (< from to)
+     (put-text-property from to 'invisible t)))
+ 
+ (defun log-view-show-any ()
+   "Show any entry.
+ Show any entry hidden by a previous `log-view-show-object-only'
+ or `log-view-show-current-only' command."
+   (interactive)
+   (let ((inhibit-read-only t))
+     (remove-text-properties (point-min) (point-max) '(invisible nil))))
+ 
+ (defun log-view-object-at-point ()
+   "Return name of object at point.
+ The object at point must be a name preceded by a left paren or a
+ comma and followed by a right paren or a comma."
+   (save-excursion
+     (save-restriction
+       (narrow-to-region (line-beginning-position) (line-end-position))
+       (let ((from (save-excursion
+ 		    (when (re-search-backward ",[ ]+\\|(" nil t)
+ 		      (match-end 0))))
+ 	    (to (save-excursion
+ 		  (when (re-search-forward ",\\|)" nil t)
+ 		    (match-beginning 0)))))
+ 	 (when (and from to)
+ 	   (buffer-substring-no-properties from to))))))
+ 
+ (defun log-view-show-object-only ()
+   "Show log entries for object at point only."
+   (interactive)
+   (let* ((name (log-view-object-at-point))
+ 	 (name-rq (when name (regexp-quote name)))
+ 	 (inhibit-read-only t)
+ 	 from to)
+     (unless name
+       (error "No fun round point"))
+     ;; Avoid to get the same object for another file.
+     (log-view-show-current-only)
+     (save-excursion
+       (save-restriction
+ 	(widen)
+ 	(goto-char (point-min))
+ 	(setq from (when (re-search-forward log-view-message-re nil t)
+ 		     (line-beginning-position)))
+ 	(while (< from (point-max))
+ 	  (setq to (if (re-search-forward log-view-message-re nil t)
+ 		       (line-beginning-position)
+ 		     (point-max)))
+ 	  (save-excursion
+ 	    (goto-char from)
+ 	    (unless (and (re-search-forward name-rq to t)
+ 			 (string-equal name (log-view-object-at-point)))
+ 	      (log-view-hide from to)))
+ 	  (setq from to)
+ 	  (goto-char to)
+ 	  (forward-line))))))
+ 
+ (defconst log-view-show-entry-re "^[ \t\n]*\\*[ \t]+")
+ 
+ (defun log-view-show-current-only ()
+   "Show log entries for current file only."
+   (interactive)
+   (log-view-show-any)
+   (let* ((name (file-name-nondirectory (log-view-current-file)))
+ 	 (name-rq (when name (regexp-quote name)))
+ 	 (name-re
+ 	  (when name (concat ".*" name-rq)))
+ 	 (log-view-show-entry-and-name-re
+ 	  (when name
+ 	    (concat log-view-show-entry-re ".*" name-rq)))
+ 	 (inhibit-read-only t)
+ 	 from to)
+     (unless name
+       (error "Couldn't find file name"))
+     (save-excursion
+       (save-restriction
+ 	(widen)
+ 	(goto-char (point-min))
+ 	(while (< (setq from
+ 			(if (re-search-forward log-view-message-re nil 'move)
+ 			    (line-beginning-position 2)
+ 			  (point-max)))
+ 		  (point-max))
+ 	  (setq to (if (re-search-forward log-view-message-re nil t)
+ 		       (line-beginning-position 0)
+ 		     (point-max)))
+ 	  (goto-char from)
+ 	  (let ((before-from (line-beginning-position))
+ 		before-to after-from within-from within-to)
+ 	    (while (re-search-forward log-view-show-entry-re to t)
+ 	      (if (looking-at name-re)
+ 		  ;; Relevant entry.
+ 		  (setq before-to before-from)
+ 		;; Irrelevant entry.
+ 		(setq before-to
+ 		      (if (re-search-forward
+ 			   log-view-show-entry-and-name-re to 'move)
+ 			  (line-beginning-position)
+ 			to)))
+ 	      (log-view-hide before-from before-to)
+ 	      (setq after-from
+ 		    (if (re-search-forward log-view-show-entry-re to 'move)
+ 			(line-beginning-position)
+ 		      to))
+ 	      (when (< before-to after-from)
+ 		(goto-char before-to)
+ 		;; Handle the special case where a common ChangeLog text
+ 		;; is written for entries from different files.
+ 		(when (and (re-search-forward ":[ \t\n]+" after-from t)
+ 			   (setq within-from (match-beginning 0))
+ 			   (looking-at log-view-show-entry-re)
+ 			   (re-search-forward ":[ \t\n]+[^*]" to t)
+ 			   (setq within-to (match-beginning 0)))
+ 		  (log-view-hide within-from within-to)
+ 		  (setq after-from
+ 			(if (re-search-forward log-view-show-entry-re to 'move)
+ 			    (line-beginning-position)
+ 			  to))))
+ 	      (goto-char (setq before-from after-from)))
+ 	    (when after-from
+ 	      (log-view-hide after-from to)))
+ 	  (goto-char to))))))
+ 
  (provide 'log-view)
  
  ;; arch-tag: 0d64220b-ce7e-4f62-9c2a-6b04c2f81f4f

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Eli Zaretskii]

> Date: Sun, 23 Nov 2008 20:11:45 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: Karl Fogel <kfogel@red-bean.com>, schwab@suse.de, cyd@stupidchicken.com,
> 	miles@gnu.org, emacs-devel@gnu.org
> 
> How about something in the direction of the attached patch?

Thanks, but could you perhaps describe in a few words what is this
supposed to do?  I'm too busy these days to find time to play with
patches.  What use cases does this support?  What will it show for the
monster commits like the on in lisp/org a few days ago, where the same
function in the same file could be mentioned several times in the same
log message?  What will it show for change logs like the one below,
assuming I'm looking for the log entries of kmacro-edit-lossage?

    2008-10-11  Romain Francoise  <romain@orebokech.com>

	    * help.el (view-lossage): Fix docstring, lossage is now 300 keys.
	    * kmacro.el (kmacro-edit-lossage): Ditto.
	    * edmacro.el (edit-kbd-macro): Ditto.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Sun, 23 Nov 2008 20:11:45 +0100
>> From: martin rudalics <rudalics@gmx.at>
>> Cc: Karl Fogel <kfogel@red-bean.com>, schwab@suse.de, cyd@stupidchicken.com,
>> 	miles@gnu.org, emacs-devel@gnu.org
>> 
>> How about something in the direction of the attached patch?
>
> Thanks, but could you perhaps describe in a few words what is this
> supposed to do?  

What Eli said, yeah.

Ironic that we're (I think) talking about a patch to make log messages
more useable, but the patch arrived without a log message :-).  Having
one would make the change much more comprehensible to those who have to
decide whether to invest one minute to decide whether to invest the next
five minutes, and so on...

Thanks,
-Karl

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[martin rudalics]

 > Thanks, but could you perhaps describe in a few words what is this
 > supposed to do?  I'm too busy these days to find time to play with
 > patches.  What use cases does this support?  What will it show for the
 > monster commits like the on in lisp/org a few days ago, where the same
 > function in the same file could be mentioned several times in the same
 > log message?  What will it show for change logs like the one below,
 > assuming I'm looking for the log entries of kmacro-edit-lossage?
 >
 >     2008-10-11  Romain Francoise  <romain@orebokech.com>
 >
 > 	    * help.el (view-lossage): Fix docstring, lossage is now 300 keys.
 > 	    * kmacro.el (kmacro-edit-lossage): Ditto.
 > 	    * edmacro.el (edit-kbd-macro): Ditto.

I didn't know you cared about ChangeLog buffers (where such entries have
been around ever since).  My patch was intended for Log-View mode.
Anyway, the idea is to show only entries related to the current file,
that is in the above example, just show

2008-10-11  Romain Francoise  <romain@orebokech.com>

	* kmacro.el (kmacro-edit-lossage): Fix docstring, lossage is now 300 keys.

I'll send you a better patch (for ChangeLog buffers too) as soon as I
satisfactorily solved things like the "Ditto" (and "Likewise") issue.

martin

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[martin rudalics]

 > Ironic that we're (I think) talking about a patch to make log messages
 > more useable, but the patch arrived without a log message :-).  Having
 > one would make the change much more comprehensible to those who have to
 > decide whether to invest one minute to decide whether to invest the next
 > five minutes, and so on...

... sorry, my bad.

martin

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Eli Zaretskii]

> Date: Tue, 25 Nov 2008 17:25:32 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: emacs-devel@gnu.org
> 
>  >     2008-10-11  Romain Francoise  <romain@orebokech.com>
>  >
>  > 	    * help.el (view-lossage): Fix docstring, lossage is now 300 keys.
>  > 	    * kmacro.el (kmacro-edit-lossage): Ditto.
>  > 	    * edmacro.el (edit-kbd-macro): Ditto.
> 
> I didn't know you cared about ChangeLog buffers

I don't.  I care about VC logs (in the context of this discussion).
The problem is, people who commit several files at once tend to simply
copy the ChangeLog entries verbatim, complete with the TABs and date
lines, as CVS log entries for their commits.  Please try "cvs log
lisp/org/org.el", and you will see what I mean.

> My patch was intended for Log-View mode.

And that is the only thing I care about here.  We are on the same
page, I just have a bit more gray hair ;-)

> Anyway, the idea is to show only entries related to the current file,
> that is in the above example, just show
> 
> 2008-10-11  Romain Francoise  <romain@orebokech.com>
> 
> 	* kmacro.el (kmacro-edit-lossage): Fix docstring, lossage is now 300 keys.

Yes, that's what I want.  I hope you won't find this too hard to
implement, what with all the possible variations of this ``style'' of
log entries.

> I'll send you a better patch (for ChangeLog buffers too) as soon as I
> satisfactorily solved things like the "Ditto" (and "Likewise") issue.

Thanks.

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Stefan Monnier]

>> * help.el (view-lossage): Fix docstring, lossage is now 300 keys.
>> * kmacro.el (kmacro-edit-lossage): Ditto.
>> * edmacro.el (edit-kbd-macro): Ditto.

BTW, I'd rather not use "ditto" and use the following form instead:

   * help.el (view-lossage):
   * kmacro.el (kmacro-edit-lossage):
   * edmacro.el (edit-kbd-macro): Fix docstring, lossage is now 300 keys.


-- Stefan

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Stephen J. Turnbull]

Eli Zaretskii writes:

 > > Anyway, the idea is to show only entries related to the current file,
 > > that is in the above example, just show
 > > 
 > > 2008-10-11  Romain Francoise  <romain@orebokech.com>
 > > 
 > > 	* kmacro.el (kmacro-edit-lossage): Fix docstring, lossage is now 300 keys.
 > 
 > Yes, that's what I want.  I hope you won't find this too hard to
 > implement, what with all the possible variations of this ``style'' of
 > log entries.

If that's implementable, it shouldn't be too hard to refine to the
parenthesized objects.  Don't kill yourself, but XEmacs has a fair
number of 1000-line ChangeLogs from Ben Wing making that level of
refinement useful.

(For me it's not urgent since I use isearch in the ChangeLog at the
function level anyway, and I'm sure by the time I can change my habits
it will be implemented. ;-)

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

  >> 2008-10-11  Romain Francoise  <romain@orebokech.com>
  >>
  >> 	* kmacro.el (kmacro-edit-lossage): Fix docstring, lossage is now 300 keys.
  >
  > Yes, that's what I want.  I hope you won't find this too hard to
  > implement, what with all the possible variations of this ``style'' of
  > log entries.

I attached an improved version of this.  When in a Log-View mode buffer,
type `c' to shadow/hide all (well, most) entries not related to the
current file.  Type `o' when on an object in a parenthesized list, to
only show entries for that object.  Type `y' to remove any shadowing or
hiding.  Use `log-view-shadow' to toggle shadowing (it's currently by
default on for the `c' command to find errors sooner).

Suggestions welcome, martin.

[-- Attachment #2: log-view.el.diff --]
[-- Type: text/plain, Size: 11093 bytes --]

*** log-view.el.~1.56.~	2008-11-25 07:49:21.937500000 +0100
--- log-view.el	2008-11-27 10:50:46.250000000 +0100
***************
*** 137,142 ****
--- 137,145 ----
      ([backtab] . log-view-msg-prev)
      ("N" . log-view-file-next)
      ("P" . log-view-file-prev)
+     ("o" . log-view-show-object-only)
+     ("c" . log-view-show-current-only)
+     ("y" . log-view-show-any)
      ("\M-n" . log-view-file-next)
      ("\M-p" . log-view-file-prev))
    "Log-View's keymap."
***************
*** 171,177 ****
      ["Next File"  log-view-file-next
       :help "Go to the next count'th file"]
      ["Previous File"  log-view-file-prev
!      :help "Go to the previous count'th file"]))
  
  (defvar log-view-mode-hook nil
    "Hook run at the end of `log-view-mode'.")
--- 174,187 ----
      ["Next File"  log-view-file-next
       :help "Go to the next count'th file"]
      ["Previous File"  log-view-file-prev
!      :help "Go to the previous count'th file"]
!     "-----"
!     ["Show Object Only"  log-view-show-object-only
!      :help "Show entries for object at point only"]
!     ["Show Current Only"  log-view-show-current-only
!      :help "Show entries for current file only"]
!     ["Show Any"  log-view-show-any
!      :help "Show any entry"]))
  
  (defvar log-view-mode-hook nil
    "Hook run at the end of `log-view-mode'.")
***************
*** 196,201 ****
--- 206,221 ----
  (put 'log-view-message-face 'face-alias 'log-view-message)
  (defvar log-view-message-face 'log-view-message)
  
+ (defcustom log-view-shadow 'current-only
+   "When non-nil, shadow entries with selective viewing.
+ That means, text normally hidden by `log-view-show-current-only'
+ is shadowed instead.  When this is t, also shadow text normally
+ hidden by `log-view-show-object-only'."
+   :type '(choice (const :tag "Never" nil)
+ 		 (const :tag "For current file only" 'current-only)
+ 		 (const :tag "Always" t))
+   :group 'log-view)
+ 
  (defvar log-view-file-re
    (concat "^\\(?:Working file: \\(?1:.+\\)"                ;RCS and CVS.
            ;; Subversion has no such thing??
***************
*** 532,537 ****
--- 552,795 ----
       (list log-view-vc-backend nil)
       to fr)))
  
+ ;;; View entries selectively.
+ (defconst log-view-entry-re "^[ \t]*\\*[ \t]+")
+ 
+ (defvar log-view-entry-or-paren-re
+   (concat "\\(?:" log-view-entry-re "\\)\\|\\(?:^[ \t]*(\\)"))
+ 
+ (defvar log-view-hide-is-shadow nil)
+ 
+ (defun log-view--hide (from to &optional before)
+   "Hide text between FROM and TO.
+ Optional arg BEFORE non-nil means hide empty lines before FROM
+ and do not hide last line before TO.  If `log-view-shadow' is
+ non-nil, shadow text instead of hiding it."
+   (when before
+     (save-excursion
+       ;; Skip empty lines preceding FROM.
+       (if (and (> from (point-min))
+ 	       (get-text-property (1- from) 'invisible))
+ 	  ;; There is invisible text, look before it.
+ 	  (progn
+ 	    (goto-char
+ 	     (previous-single-property-change from 'invisible))
+ 	    (skip-chars-backward "[ \t\n]")
+ 	    (forward-line)
+ 	    (setq from (point)))
+ 	;; No invisible text, look right here.
+ 	(goto-char from)
+ 	(skip-chars-backward "[ \t\n]")
+ 	(forward-line)
+ 	(setq from (point))))
+     ;; Do not hide last line before TO.
+     (setq to (save-excursion
+ 	       (goto-char to)
+ 	       (forward-line -1)
+ 	       (point))))
+   ;; Hide or shadow text.
+   (when (< from to)
+     (if log-view-hide-is-shadow
+ 	(let ((overlay (make-overlay from to)))
+ 	  (overlay-put overlay 'face 'shadow))
+       (put-text-property from to 'invisible t))))
+ 
+ (defun log-view--object-at-point ()
+   "Return name of object at point.
+ The object at point is the string at point preceded by a left
+ paren or a comma and followed by a right paren or a comma.
+ Return nil if there's no such string."
+   (save-excursion
+     (save-restriction
+       (narrow-to-region (line-beginning-position) (line-end-position))
+       (let ((from (save-excursion
+ 		    (when (re-search-backward "(\\|,[ ]+" nil t)
+ 		      (match-end 0))))
+ 	    (to (save-excursion
+ 		  (when (re-search-forward ")\\|," nil t)
+ 		    (match-beginning 0)))))
+ 	 (when (and from to)
+ 	   (buffer-substring-no-properties from to))))))
+ 
+ (defun log-view--prev-match (regexp &optional bound move)
+   "Return position of previous match for REGEXP.
+ Optional argument BOUND non-nil means don't search before BOUND.
+ BOUND defaults to `point-min'.  Return BOUND when nothing is
+ found.  Optional argument MOVE non-nil means also move point
+ there.  Else keep point unchanged."
+   (setq bound (or bound (point-min)))
+   (if move
+       (goto-char (if (re-search-backward regexp bound t)
+ 		     (match-beginning 0)
+ 		   bound))
+     (save-excursion
+       (if (re-search-backward regexp bound t)
+ 	  (match-beginning 0)
+ 	bound))))
+ 
+ (defun log-view--next-match(regexp &optional bound move)
+   "Return position of next match for REGEXP.
+ Optional argument BOUND non-nil means don't search before BOUND.
+ BOUND defaults to `point-max'.  Return BOUND when nothing is
+ found.  Optional argument MOVE non-nil means also move point
+ there.  Else keep point unchanged."
+   (setq bound (or bound (point-max)))
+   (if move
+       (goto-char (if (re-search-forward regexp bound t)
+ 		     (match-beginning 0)
+ 		   bound))
+     (save-excursion
+       (if (re-search-forward regexp bound t)
+ 	  (match-beginning 0)
+ 	bound))))
+ 
+ (defun log-view--collapse-comments (from to)
+   "Try to collapse entry comments between FROM and TO."
+   ;; point must be where the entry is.
+   (let (at)
+     (cond
+      ;; Handle messages like
+      ;; foo.ext (foobar):
+      ;; bar.ext (foobar): Make foobar prominent.
+      ((save-excursion
+ 	(and (re-search-forward ":[ \t\n]+" to t)
+ 	     (setq at (match-beginning 0))
+ 	     (looking-at log-view-entry-re)
+ 	     (re-search-forward ":[ \t\n]+[^*]" to t)))
+       (log-view--hide at (match-beginning 0))
+       (goto-char (match-end 0))
+       (log-view--next-match log-view-entry-re to t))
+      ;; Handle messages like
+      ;; foo.ext (foobar): Make foobar prominent.
+      ;; bar.ext (foobar): Likewise.
+      ((and (re-search-forward ":[ \t]+" to t)
+ 	   (setq at (match-beginning 0))
+ 	   (looking-at "\\(?:[Ll]ikewise\\|[Dd]itto\\)[.]*")
+ 	   (let ((string (match-string-no-properties 0))
+ 		 (display-from (match-beginning 0))
+ 		 (display-to (match-end 0)))
+ 	     (goto-char at)
+ 	     (while (and (re-search-backward "):[ \t]+" from t)
+ 			 (save-excursion
+ 			   (goto-char (match-end 0))
+ 			   (looking-at string))))
+ 	     (goto-char (match-end 0))
+ 	     (put-text-property
+ 	      display-from display-to
+ 	      'display
+ 	      (buffer-substring-no-properties
+ 	       (point)
+ 	       (save-excursion
+ 		 (log-view--next-match log-view-entry-or-paren-re at t)
+ 		 ;; Skip last newline char.
+ 		 (skip-chars-backward "[ \t\n]")
+ 		 (point))))
+ 	     (goto-char at)
+ 	     (log-view--next-match log-view-entry-re to t)))))))
+ 
+ (defun log-view-show-any ()
+   "Show any entries obscured by selective viewing.
+ Show any entries hidden by a previous `log-view-show-object-only'
+ or `log-view-show-current-only' command."
+   (interactive)
+   (let ((inhibit-read-only t))
+     ;; This must be changed as soon as people want to install their own
+     ;; invisible or display stuff.
+     (remove-text-properties (point-min) (point-max) '(invisible nil display nil))
+     (remove-overlays nil nil 'face 'shadow)))
+ 
+ (defun log-view-show-current-only ()
+   "Selectively show log entries for current file only."
+   (interactive)
+   (log-view-show-any)
+   (setq log-view-hide-is-shadow log-view-shadow)
+   (let* ((name (or (file-name-nondirectory (log-view-current-file))
+ 		   (error "No current file")))
+ 	 (name-rq (regexp-quote name))
+ 	 (name-re (concat ".*" name-rq))
+ 	 (entry-and-name-re (concat log-view-entry-re ".*" name-rq))
+ 	 (inhibit-read-only t)
+ 	 (buffer-undo-list t)
+ 	 from to)
+     (save-excursion
+       (save-restriction
+ 	(widen)
+ 	(goto-char (point-min))
+ 	;; Search for next entry.
+ 	(while (re-search-forward log-view-entry-re nil t)
+ 	  (setq from (match-beginning 0))
+ 	  (setq to (log-view--next-match log-view-message-re))
+ 	  (if (looking-at name-re)
+ 	      ;; A relevant entry, make sure it has a comment.
+ 	      (log-view--collapse-comments
+ 	       (log-view--prev-match log-view-message-re) to)
+ 	      ;; An irrelevant entry, hide it.
+ 	    (if (< (log-view--next-match log-view-entry-re to t) to)
+ 		;; There's a following entry.
+ 		(log-view--hide from (point))
+ 	    ;; No following entry.
+ 	    (log-view--hide from to t))))))))
+ 
+ (defun log-view-show-object-only ()
+   "Selectively show log entries for object at point only."
+   (interactive)
+   (log-view-show-any)
+   (setq log-view-hide-is-shadow (eq log-view-shadow t))
+   (let* ((name (or (log-view--object-at-point)
+ 		   (error "No suitable object found")))
+ 	 (name-rq (regexp-quote name))
+ 	 (name-re (concat "\\(?:(\\|,\\)[ \t\n]*\\(" name-rq
+ 			  "\\)[ \t\n]*\\(?:)\\|,\\)"))
+ 	 (inhibit-read-only t)
+ 	 (buffer-undo-list t)
+ 	 (last-from (point-min))
+ 	 from to paren-from paren-to)
+     (save-excursion
+       (save-restriction
+ 	(widen)
+ 	(goto-char (point-min))
+ 	(setq to (log-view--next-match log-view-message-re nil t))
+ 	(setq paren-to to)
+ 	(while (re-search-forward name-re nil t)
+ 	  (goto-char (match-beginning 1))
+ 	  ;; We found a relevant entry, record its message.
+ 	  (setq from (log-view--prev-match log-view-message-re))
+ 	  ;; Record its open paren, it _should_ be on this line ...
+ 	  (setq paren-from (save-excursion
+ 			     (if (re-search-backward "(" from t)
+ 				 (match-beginning 0)
+ 			       from)))
+ 	  (if (= from last-from)
+ 	      ;; We're still in the previous message.  Hide any
+ 	      ;; preceding entries.
+ 	      (log-view--hide paren-to paren-from)
+ 	    ;; We're in a new message, record that.
+ 	    (setq last-from from)
+ 	    ;; Hide any entries in previous message.
+ 	    (log-view--hide paren-to to t)
+ 	    ;; Hide any messages after previous one.
+ 	    (log-view--hide to from)
+ 	    ;; Hide everything from first entry or paren in message to
+ 	    ;; our paren.
+ 	    (log-view--hide
+ 	     (save-excursion
+ 	       (goto-char from)
+ 	       (log-view--next-match log-view-entry-or-paren-re paren-from))
+ 	     paren-from)
+ 	    (setq to (log-view--next-match log-view-message-re)))
+ 	  (setq paren-to (progn
+ 			   (re-search-forward ")" to 'move)
+ 			   (log-view--next-match
+ 			    log-view-entry-or-paren-re to)))
+ 	  (log-view--collapse-comments from to)
+ 	  (when (> (point) paren-to)
+ 	    (setq paren-to (log-view--next-match
+ 			    log-view-entry-or-paren-re to 'move))))
+ 	;; Hide any entries in previous message.
+ 	(log-view--hide paren-to to t)
+ 	;; Hide any messages left.
+ 	(log-view--hide to (point-max))))))
+ 
  (provide 'log-view)
  
  ;; arch-tag: 0d64220b-ce7e-4f62-9c2a-6b04c2f81f4f

Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Stefan Monnier]

> I attached an improved version of this.  When in a Log-View mode buffer,
> type `c' to shadow/hide all (well, most) entries not related to the
> current file.  Type `o' when on an object in a parenthesized list, to
> only show entries for that object.  Type `y' to remove any shadowing or
> hiding.  Use `log-view-shadow' to toggle shadowing (it's currently by
> default on for the `c' command to find errors sooner).

Rather than "shadow" I'd prefer a name like "focus", or "limit" (limit
the view to a subset, that's the name used in Gnus's summary, it uses
the / key prefix).


        Stefan