[MERGE] add modification dates to diff output, get rid of /dev/null (bug #43033)

[James Henstridge]

On 23/05/06, John A Meinel <john at arbash-meinel.com> wrote:
> James Henstridge wrote:
> > On 17/05/06, James Henstridge <james.henstridge at gmail.com> wrote:
> >> I posted this patch a few days ago, but have updated it with a few
> >> more tests, and think it is ready to merge.  The branch is available
> >> here (currently at revision 1712):
> >>
> >>     http://people.ubuntu.com/~jamesh/bzr/bzr.diff-output
> >
> > Would it be possible to get this merged (possibly with a second +1)?
> >
> > The branch should merge cleanly against bzr.dev (I just did a merge
> > and ran the test suite again).
> >
> > James.
> >
> >
>
> I just wanted to discuss briefly if we really want the times to be
> shown. It seems to be adding a lot of clutter. And after what happened
> with -p1 diffs, I would like to see us be careful about what we add to
> the 'diff' output.

I hadn't really thought of it as a clutter issue, since the timestamps
are present in diffs produced by normal diff and by "cvs diff", among
others.  Also, the date is separated by a tab, so is usually fairly
distinct from the filename.

When reviewing patches, the dates can also be used to give some idea
of the age of the code they are modifying.  This can be quite useful
in larger source trees.

> Probably adding timestamps is less of a pain than adding prefixes
> (because it is easier to mentally throw away trailing space separated
> stuff, than stuff that is attached to the path you are looking at).
>
> I personally prefer not to have timestamps. But with them a tab
> character away, they don't seem terrible.
> I do find '/dev/null' to be more obvious than 1970 to indicate the file
> didn't exist. We still have the === removed/added line, but it is one
> less visual clue.

Well, there is also the visual clue that all the lines are removed in
the diff :)  I guess I am still used to the output of cvs and normal
diff so don't usually look for "/dev/null" when trying to identify
added/removed files.

> Should we try to have the old label be the old name in the case of
> renames? It would seem not. But why in the world does patch need 2 names
> then.

I preserved the existing behaviour here, so for a rename you get:
    --- $oldname $olddate
    +++ $newname $newdate

I don't think patch has ever used this sort of construct as a hint to
move the file though (it will usually just patch the old file and
leave it as is).

To produce a diff that would get patch to move the file, it would be
necessary to include a full-file add and full-file remove.  This is
less useful for reviewing diffs though, so I didn't include such a
change.

> The code changes themselves look clean, and the test cases look
> sufficient. So if we decide we really do want the timestamps, you have
> my +1.

Thanks.