* [PATCH] Display change history as a diff between two dirs
From: Roland Kaufmann @ 2011-10-29 20:51 UTC (permalink / raw)
To: gitster; +Cc: git
Watching patches serially it can be difficult to get an overview of how
a pervasive change is distributed through-out different modules. Thus:
Extract snapshots of the files that have changed between two revisions
into temporary directories and launch a graphical tool to show the diff
between them.
Use existing functionality in git-diff to figure out which files have
changed, and to get the files themselves.
Based on a script called 'git-diffc' by Nitin Gupta.
Signed-off-by: Roland Kaufmann <rlndkfmn+git@gmail.com>
---
Requests for such a scripts surface occationally, so I believe it could
be useful to have in the distribution itself.
Documentation/git-dirdiff.txt | 55 +++++++++++++++++++++++++++++++++++++++++
Makefile | 2 +
git-dirdiff--helper.sh | 28 +++++++++++++++++++++
git-dirdiff.sh | 34 +++++++++++++++++++++++++
4 files changed, 119 insertions(+), 0 deletions(-)
create mode 100644 Documentation/git-dirdiff.txt
create mode 100755 git-dirdiff--helper.sh
create mode 100755 git-dirdiff.sh
diff --git a/Documentation/git-dirdiff.txt b/Documentation/git-dirdiff.txt
new file mode 100644
index 0000000..bdd2581
--- /dev/null
+++ b/Documentation/git-dirdiff.txt
@@ -0,0 +1,55 @@
+git-dirdiff(1)
+==============
+
+NAME
+----
+git-dirdiff - Show changes using directory compare
+
+SYNOPSIS
+--------
+[verse]
+'git dirdiff' [<options>] [<commit> [<commit>]] [--] [<path>...]
+
+DESCRIPTION
+-----------
+'git dirdiff' is a git command that allows you to compare revisions
+as a difference between two directories. 'git dirdiff' is a frontend
+to linkgit:git-diff[1].
+
+OPTIONS
+-------
+See linkgit:git-diff[1] for the list of supported options.
+
+CONFIG VARIABLES
+----------------
+'git dirdiff' uses the same config variables as linkgit:git-difftool[1]
+to determine which difftool should be used.
+
+TEMPORARY FILES
+---------------
+'git dirdiff' creates a directory with 'mktemp' to hold snapshots of the
+files which are different in the two revisions. This directory is removed
+when the diff viewer terminates.
+
+NOTES
+-----
+The diff viewer must support being passed directories instead of files
+as its arguments.
++
+Files that are not put under version control are not included when
+viewing the difference between a revision and the working directory.
+
+SEE ALSO
+--------
+linkgit:git-diff[1]::
+ Show changes between commits, commit and working tree, etc
+
+linkgit:git-difftool[1]::
+ Show changes using common diff tools
+
+linkgit:git-config[1]::
+ Get and set repository or global options
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 3139c19..03771cf 100644
--- a/Makefile
+++ b/Makefile
@@ -365,6 +365,8 @@ unexport CDPATH
SCRIPT_SH += git-am.sh
SCRIPT_SH += git-bisect.sh
SCRIPT_SH += git-difftool--helper.sh
+SCRIPT_SH += git-dirdiff.sh
+SCRIPT_SH += git-dirdiff--helper.sh
SCRIPT_SH += git-filter-branch.sh
SCRIPT_SH += git-lost-found.sh
SCRIPT_SH += git-merge-octopus.sh
diff --git a/git-dirdiff--helper.sh b/git-dirdiff--helper.sh
new file mode 100755
index 0000000..bc0b49d
--- /dev/null
+++ b/git-dirdiff--helper.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+#
+# Accumulate files in a changeset into a pre-defined directory.
+#
+# Copyright (C) 2011 Roland Kaufmann
+# Based on a script called git-diffc by Nitin Gupta
+#
+# This file is licensed under the GPL v2, or a later version
+# at the discretion of the official Git maintainer.
+
+# bail out if there is any problems copying
+set -e
+
+# check that we are called by git-dirdiff
+if [ -z $__GIT_DIFF_DIR ]; then
+ echo Error: Do not call $(basename $0) directly 1>&2
+ exit 1
+fi
+
+# don't attempt to copy new or removed files
+if [ "$2" != "/dev/null" ]; then
+ mkdir -p $__GIT_DIFF_DIR/old/$(dirname $1)
+ cp $2 $__GIT_DIFF_DIR/old/$1
+fi
+if [ "$5" != "/dev/null" ]; then
+ mkdir -p $__GIT_DIFF_DIR/new/$(dirname $1)
+ cp $5 $__GIT_DIFF_DIR/new/$1
+fi
diff --git a/git-dirdiff.sh b/git-dirdiff.sh
new file mode 100755
index 0000000..4e75eda
--- /dev/null
+++ b/git-dirdiff.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+#
+# Display differences between two commits with a directory comparison.
+#
+# Copyright (C) 2011 Roland Kaufmann
+# Based on a script called git-diffc by Nitin Gupta
+#
+# This file is licensed under the GPL v2, or a later version
+# at the discretion of the official Git maintainer.
+
+# bail out if there is any problems in getting a diff
+set -e
+
+# create a temporary directory to hold snapshots of changed files
+__GIT_DIFF_DIR=$(mktemp --tmpdir -d git-dirdiff.XXXXXX)
+export __GIT_DIFF_DIR
+
+# cleanup after we're done
+trap 'rm -rf $__GIT_DIFF_DIR' 0
+
+# list all files that have changed. store this list in a separate
+# file so that we can test the exit status of this command. (if we had
+# bash we could use pipefail, or if we had Posix we could use mkfifo)
+git diff --raw "$@" > $__GIT_DIFF_DIR/toc
+
+# let the helper script accumulate them into the temporary directory
+cut -f 2- -s $__GIT_DIFF_DIR/toc | while read f; do
+ GIT_EXTERNAL_DIFF=git-dirdiff--helper git --no-pager diff "$@" $f
+done
+
+# run original diff program, reckoning it will understand directories
+# modes and shas does not apply to the root directories so submit dummy
+# values for those, hoping that the diff tool does not use them.
+git-difftool--helper - $__GIT_DIFF_DIR/old deadbeef 0755 $__GIT_DIFF_DIR/new babeface 0755
--
1.7.1
^ permalink raw reply related
* [PATCH] document 'T' status from git-status
From: Mark Dominus @ 2011-10-30 0:06 UTC (permalink / raw)
To: git, gitster; +Cc: Mark Dominus, Mark Dominus
From: Mark Dominus <mjd@icgroup.com>
Signed-off-by: Mark Dominus <mjd@plover.com>
---
Documentation/git-status.txt | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 3d51717..e7fc5c3 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -122,6 +122,8 @@ codes can be interpreted as follows:
* 'R' = renamed
* 'C' = copied
* 'U' = updated but unmerged
+* 'T' = file type changed
+ (typically from plain file to symlink, or vice versa)
Ignored files are not listed, unless `--ignored` option is in effect,
in which case `XY` are `!!`.
@@ -134,9 +136,11 @@ in which case `XY` are `!!`.
D [ M] deleted from index
R [ MD] renamed in index
C [ MD] copied in index
+ T [ MD] file type changed in index
[MARC] index and work tree matches
[ MARC] M work tree changed since index
[ MARC] D deleted in work tree
+ [ MARC] T file type changed in work tree
-------------------------------------------------
D D unmerged, both deleted
A U unmerged, added by us
--
1.7.7.dirty
^ permalink raw reply related
* Re: [RFC/PATCH] define the way new representation types are encoded in the pack
From: Nguyen Thai Ngoc Duy @ 2011-10-30 5:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nicolas Pitre, Shawn O. Pearce, Jeff King
In-Reply-To: <7vwrbptzjm.fsf@alter.siamese.dyndns.org>
On Fri, Oct 28, 2011 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> As people may be able to guess from the name, CAT_TREE is envisioned to
> encode a large data (primarily of type "blob") by recording the object
> name of a tree object and probably the total length, and would represent
> the concatenation of all blobs contained in the tree object when the tree
> is traversed in some fixed order (e.g. Avery's "bup split"). I am guessing
> that the payload for CAT_TREE representation type will be:
>
> - 20-byte object name for the top-level tree object;
Because all blobs in this tree object must be in a fixed order, and
they won't likely have meaningful names nor permission, should
CAT_TREE payload is a SHA-1 sequence of all blobs (or cat-trees if we
want nested trees) instead? IOW the tree is integrated into cat-tree
object, not as a separate tree object.
> - type of the basic object (commit, tree, blob, or tag) it represents,
> even though it is unlikely that we would want to record such a large
> commit or tag that needs CAT_TREE representation;
>
> - the total length of the basic object it represents, even though it is
> redundant (you could traverse and sum the sizes of blobs contained in
> the tree object), it would help sha1_object_info() and friends. This
> will be the "some size" I mentioned in the previous message for this
> representation type.
Not sure if it's related to representation types, but is there any way
(perhaps FLAT_BLOB type?) we can mark an object uncompressed, so we
can mmap() and access it directly?
--
Duy
^ permalink raw reply
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Nguyen Thai Ngoc Duy @ 2011-10-30 5:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vobx2z60w.fsf@alter.siamese.dyndns.org>
On Fri, Oct 28, 2011 at 12:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ... Should we stick with one way only?
>
> Whatever we have been doing should not change, especially in corner cases.
I disagree. If it's not right, then we should change it even though it
may face unpleasant consequences from misusing it. And I don't think
it's sane to behave like what we're doing now:
$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 1
$ ls -l clone2/2 3
-rw-r--r-- 1 pclouds users 0 Th10 30 12:40 3
-rw-r--r-- 1 pclouds users 0 Th10 30 12:40 clone2/2
$ GIT_DIR=clone2/.git git add clone2/2 3
$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 1
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 3
$ GIT_DIR=clone2/.git git add clone2/2
$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 1
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 3
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 clone2/2
"git add" behaves inconsistently when "clone2/2" and "3" are given and
when clone2/2 is given alone. This is just bad to me.
Note that this has nothing to do with read_directory() discussion we
had in the notes-merge patch, I agree we should keep the prefix. This
is about the calculating common prefix automatically from pathspec.
But prefix and pathspec are treated differently by read_directory().
In "git add clone2/2 3", common prefix is "" while in "git add
clone2/2" common prefix is "clone2".
--
Duy
^ permalink raw reply
* Re: [PATCH] Display change history as a diff between two dirs
From: Junio C Hamano @ 2011-10-30 6:09 UTC (permalink / raw)
To: Roland Kaufmann; +Cc: git
In-Reply-To: <4EAC6765.4030003@gmail.com>
Roland Kaufmann <rlndkfmn+git@gmail.com> writes:
> diff --git a/git-dirdiff--helper.sh b/git-dirdiff--helper.sh
> new file mode 100755
> index 0000000..bc0b49d
> --- /dev/null
> +++ b/git-dirdiff--helper.sh
> @@ -0,0 +1,28 @@
> ...
> +# bail out if there is any problems copying
> +set -e
I do not think any of our scripted Porcelains use "set -e"; rather they
try to catch errors and produce messages that are more appropriate for
specific error sites.
> +# check that we are called by git-dirdiff
> +if [ -z $__GIT_DIFF_DIR ]; then
> + echo Error: Do not call $(basename $0) directly 1>&2
> + exit 1
> +fi
(style)
if test -z "$__GIT_DIFF_DIR"
then
echo >&2 "..."
exit 1
fi
If this were to become part of the main Porcelain set, you would probably
source in the git-sh-setup helper near the beginning with
. git-sh-setup
and use "die" instead of "echo && exit 1".
> diff --git a/git-dirdiff.sh b/git-dirdiff.sh
> new file mode 100755
> index 0000000..4e75eda
> --- /dev/null
> +++ b/git-dirdiff.sh
> @@ -0,0 +1,34 @@
> ...
> +# create a temporary directory to hold snapshots of changed files
> +__GIT_DIFF_DIR=$(mktemp --tmpdir -d git-dirdiff.XXXXXX)
> +export __GIT_DIFF_DIR
I do not think any of our scripted Porcelains use "mktemp" especially
"mktemp -d". How portable is it?
> +git diff --raw "$@" > $__GIT_DIFF_DIR/toc
> +
> +# let the helper script accumulate them into the temporary directory
> +cut -f 2- -s $__GIT_DIFF_DIR/toc | while read f; do
> + GIT_EXTERNAL_DIFF=git-dirdiff--helper git --no-pager diff "$@" $f
> +done
(style)
... upstream command ... |
while read f
do
...
done
It also is not clear what could be used in "$@". Obviously you would not
want things like "-U20" and "--stat" fed to the first "list of paths"
phase, but there may be some options you may want to give to the inner
"external diff" thing.
It is not clear to me why you need to grab the list of paths in toc and
iterate over them one by one. IOW, why isn't this sufficient?
# dirdiff--helper will copy the files in the temporary directory
GIT_EXTERNAL_DIFF=git-dirdiff--helper git --no-pager diff "$@"
# Now the temporary old/ and new/ are populated, compare them
git-difftool--helper - .....
Overall, I found it interesting, but I am not convinced yet that this
should be in the set of main Porcelains. It seems to be a hack to work
around the design of the current external diff interface that specifically
targets tools that are about comparing single pair of paths at a time.
If "compare two sets of files, each extracted in its own temporary
directory" turns out to be sufficiently useful thing to do (which I
suspect is true), we would probably want to make it an option to "git
diff" itself, and not a separate git subcommand like "dirdiff". I can see
"git diff" (and possibly even things like "git log -p") populating two
temporary directories (your old/ and new/) and running a custom program
specified by GIT_EXTERNAL_TREEDIFF environment variable, instead of doing
any textual diff generation internally.
I wouldn't mind carrying a polished version of this in contrib/ for a
cycle or two in order to let people try it out and get kinks out of its
design.
For example, how well does this approach work with -M/-C (I do not think
it would do anything useful, but I haven't thought things through). It
would be nice if we gave the hint to the external program that compares
the populated temporary directories how paths in the preimage tree and
postimage tree correspond to each other.
^ permalink raw reply
* Re: [PATCH] document 'T' status from git-status
From: Junio C Hamano @ 2011-10-30 6:25 UTC (permalink / raw)
To: Mark Dominus; +Cc: git, Mark Dominus
In-Reply-To: <1319933204-21587-1-git-send-email-mjd@plover.com>
Mark Dominus <mjd@plover.com> writes:
> From: Mark Dominus <mjd@icgroup.com>
>
> Signed-off-by: Mark Dominus <mjd@plover.com>
> ---
> Documentation/git-status.txt | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 3d51717..e7fc5c3 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -122,6 +122,8 @@ codes can be interpreted as follows:
> * 'R' = renamed
> * 'C' = copied
> * 'U' = updated but unmerged
> +* 'T' = file type changed
> + (typically from plain file to symlink, or vice versa)
>
> Ignored files are not listed, unless `--ignored` option is in effect,
> in which case `XY` are `!!`.
> @@ -134,9 +136,11 @@ in which case `XY` are `!!`.
> D [ M] deleted from index
> R [ MD] renamed in index
> C [ MD] copied in index
> + T [ MD] file type changed in index
> [MARC] index and work tree matches
> [ MARC] M work tree changed since index
> [ MARC] D deleted in work tree
> + [ MARC] T file type changed in work tree
The current organization of this table may need to be rethought, but if we
were to keep it, then this change is far from sufficient. For example, you
do not explain what XY = TT means.
Side note. The reason why we do not have two independent tables,
one of which shows "if X is M then it means updated in index, if X
is A then..." and the other shows "if Y is ' ' then index and
working tree matches, if Y is 'M' then ...", is because this table
is meant to show _possible_ combinations. For example, the row
with X = D shows that only two possible value for Y are ' ' and M
and A or D are not possible values for Y in that case.
It may be easier to explain if you treated that 'T' is merely a variant of
'M' (this comment applies to the first hunk of your patch that starts at
line 122), i.e.
Documentation/git-status.txt | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 3d51717..bc2552e 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -126,6 +126,10 @@ codes can be interpreted as follows:
Ignored files are not listed, unless `--ignored` option is in effect,
in which case `XY` are `!!`.
+Note that a change in filetype (i.e. a regular file to symbolic link or
+vice versa) is a special case of "modified" and shown as 'T' instead of
+'M'; in the table below an 'M' could be a 'T'.
+
X Y Meaning
-------------------------------------------------
[MD] not updated
^ permalink raw reply related
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Junio C Hamano @ 2011-10-30 7:08 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8DdQXXoYT2gB2L5z6pdCNU_vL2w7c8eJvKRGX2T9iAC3Q@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> Note that this has nothing to do with read_directory() discussion we
> had in the notes-merge patch...
I think we are in agreement on that point.
Going back to your example...
> $ GIT_DIR=clone2/.git git add clone2/2 3
>
> $ GIT_DIR=clone2/.git git ls-files --stage
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 1
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 3
You probably found a bug here. It is simply wrong to choose not to add
clone2/2, especially without telling the caller anything.
Side note. I just did this and I am not getting what you saw above.
$ mkdir -p /var/tmp/j/y && cd /var/tmp/j/y
$ git init; git init clone2
$ : >3; : >clone2/2
$ GIT_DIR=clone2/.git git add clone2/2 3
$ GIT_DIR=clone2/.git git ls-files
3
clone2/2
The behavour is different when clone2/.git already has commit, and
whatever codepath that gives these two different behaviour needs
to be fixed.
By the way, I think I know where you are coming from.
If we think clone2/ and everything underneath belongs to a repository that
is _not_ governed by our GIT_DIR (which usually is .git), it may be nicer
when the user attempts to add clone2/2 (which would normally belong to
clone2/.git) to at least warn about it, or even error out. I would not be
entirely opposed to a change in the behaviour if the above example were
done without GIT_DIR and produced an error, like this:
$ git add clone2/2 3; echo $?
error: clone2/2 is outside our repository, possibly governed by clone2/.git
1
$ git ls-files
1
After all, if clone2 were a submodule of our repository, we do notice and
error out an attempt to add clone2/2 to our repository, so if we changed
the way how "git add" behaves to do the above, I can buy an argument that
calls it a bugfix.
When GIT_DIR=clone2/.git is given, however, the caller explicitly declines
the repository discovery. We do not know how the repository we are dealing
with (which we were explicitly told with $GIT_DIR) and a directory whose
name is ".git" under "clone2" we happened to find in read_directory()
relates to each other, especially when our index does not have clone2 as
our submodule.
We however *do* know that our working tree is our current directory, so
it would be wrong to do this:
$ GIT_DIR=clone2/.git git add clone2/2 3; echo $?
error: 3 is outside our repository, possibly goverened by .git
1
The command should just add clone2/2 and 3 as it was told to.
^ permalink raw reply
* Re: [RFC/PATCH] define the way new representation types are encoded in the pack
From: Junio C Hamano @ 2011-10-30 7:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Nicolas Pitre, Shawn O. Pearce, Jeff King
In-Reply-To: <CACsJy8Cz0R_s+VYRd+1wTTfbt_vH5dd3ALgZip0xn7rfYf6gpw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> Because all blobs in this tree object must be in a fixed order, and
> they won't likely have meaningful names nor permission, should
> CAT_TREE payload is a SHA-1 sequence of all blobs (or cat-trees if we
> want nested trees) instead? IOW the tree is integrated into cat-tree
> object, not as a separate tree object.
I have no problem with that (I am not worried about minor details of the
actual implementation of cat-tree yet).
> Not sure if it's related to representation types, but is there any way
> (perhaps FLAT_BLOB type?) we can mark an object uncompressed, so we
> can mmap() and access it directly?
In pack? Loose? Both?
What kind of payload and use case do you have in mind?
^ permalink raw reply
* Re: [RFC/PATCH] define the way new representation types are encoded in the pack
From: Junio C Hamano @ 2011-10-30 7:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Nicolas Pitre, Shawn O. Pearce, Jeff King
In-Reply-To: <7v4nyrrm1w.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> Because all blobs in this tree object must be in a fixed order, and
>> they won't likely have meaningful names nor permission, should
>> CAT_TREE payload is a SHA-1 sequence of all blobs (or cat-trees if we
>> want nested trees) instead? IOW the tree is integrated into cat-tree
>> object, not as a separate tree object.
>
> I have no problem with that (I am not worried about minor details of the
> actual implementation of cat-tree yet).
Side note. It should be renamed "split-object" or something if we go the
route you suggest, as "tree"-ness of the actual representation is not
essential.
^ permalink raw reply
* Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
From: Mika Fischer @ 2011-10-30 7:19 UTC (permalink / raw)
To: Daniel Stenberg; +Cc: git
In-Reply-To: <alpine.DEB.2.00.1110292230500.28196@tvnag.unkk.fr>
On Sat, Oct 29, 2011 at 22:33, Daniel Stenberg <daniel@haxx.se> wrote:
>> + curl_multi_fdset(curlm, &readfds, &writefds,
>> &excfds, &max_fd);
>> +
>> + select(max_fd+1, &readfds, &writefds, &excfds,
>> &select_timeout);
>
> At times, curl_multi_fdset() might return -1 in max_fd, as when there's no
> internal socket around to provide to the application to wait for.
>
> Calling select() with max_fd+1 (== 0) will then not be appreciated by all
> implementations of select() so that case should probably also be covered by
> the 50ms sleep approach...
Actually, the 50ms sleep was also implemented using select(0, ...)
before the patch. I tried to keep the previous behavior when curl does
not give us any information.
I assumed that the select(0, ...) was some portable way to sleep with
microsecond granularity.
Is there some other way to tell select not to check any fds, or should
I just call select(1, ...)?
Best,
Mika
^ permalink raw reply
* Re: imap-send badly handles commit bodies beginning with "From <"
From: Magnus Bäck @ 2011-10-30 9:01 UTC (permalink / raw)
To: git; +Cc: Andrew Eikum, Jeff King
In-Reply-To: <20111028203256.GA15082@sigill.intra.peff.net>
On Friday, October 28, 2011 at 22:32 CEST,
Jeff King <peff@peff.net> wrote:
> On Fri, Oct 28, 2011 at 01:00:44PM -0500, Andrew Eikum wrote:
>
> > On the server side, it was split into two mails on either side
> > of that commit message's From line with neither mail actually
> > containing the From line. To fix it, I just changed it to "Copied
> > from <url>:" :-P
> >
> > Ain't mbox grand?
>
> Mbox does have this problem, but I think in this case it is a
> particularly crappy implementation of mbox in imap-send. Look at
> imap-send.c:split_msg; it just looks for "From ".
While there seems to be about a million different implementations of
mbox creation and parsing, the relevant RFC[0] points to [1] as an
authoritative source. The latter claims that lines matching "^From "
denote a message boundary and that lines within a message that match
the same pattern should be quoted with ">". That would suggest that
the problem isn't imap-send.c but whatever code produces the mbox
file in the first place. Of course, if that software isn't part of
Git I guess we'll have to deal with the situation anyway. And whatever
the RFCs say, we still need to be as compatible is possible with
whatever software is out there.
> It should at least check for something that looks like a timestamp,
> like git-mailsplit does. Maybe mailsplit's is_from_line should be
> factored out so that it can be reused in imap-send.
I guess that's a reasonable "liberal in what you accept" mitigation.
(As a sidenote, I'm getting the ">From" quoting in my maildir message
files where no such quoting is expected, so "From" lines are shown as
">From" in my MUA. I don't know if it's Procmail screwing things up or
what's going on.)
[0] http://tools.ietf.org/html/rfc4155
[1] http://qmail.org./man/man5/mbox.html
--
Magnus Bäck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson
^ permalink raw reply
* Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()
From: Nguyen Thai Ngoc Duy @ 2011-10-30 9:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbot2z3gf.fsf@alter.siamese.dyndns.org>
2011/10/28 Junio C Hamano <gitster@pobox.com>:
>> static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
>> - struct diff_options *opt, int *match)
>> + struct diff_options *opt,
>> + enum interesting *match)
>> {
>> while (t->size) {
>> *match = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
>> if (*match) {
>> - if (*match < 0)
>> + if (*match == all_entries_not_interesting)
>> t->size = 0;
>> break;
>> }
>
> The caller of this function needs to be updated as well.
Yeah, thanks.
> But I have to wonder why this skip_uninteresting() does not peek the
> original value of *match and skip, which is the loop structure the other
> caller of tree_entry_interesting() in this file has.
Probably because no one asked that question before. I think it makes
sense for skip_uninteresting() to skip t_e_i() when *match == -1 or 2.
Thanks.
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] define the way new representation types are encoded in the pack
From: Nguyen Thai Ngoc Duy @ 2011-10-30 9:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nicolas Pitre, Shawn O. Pearce, Jeff King
In-Reply-To: <7v4nyrrm1w.fsf@alter.siamese.dyndns.org>
On Sun, Oct 30, 2011 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Not sure if it's related to representation types, but is there any way
>> (perhaps FLAT_BLOB type?) we can mark an object uncompressed, so we
>> can mmap() and access it directly?
>
> In pack? Loose? Both?
>
> What kind of payload and use case do you have in mind?
Hmm.. big files in general with partial file transfer (e.g. git
torrent). But if CAT_TREE is properly used, then all blob pieces
should be reasonably small and decompress time should not be a
problem. I think we won't need this. Sorry for the noise.
--
Duy
^ permalink raw reply
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Nguyen Thai Ngoc Duy @ 2011-10-30 9:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vaa8jrm6a.fsf@alter.siamese.dyndns.org>
On Sun, Oct 30, 2011 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Side note. I just did this and I am not getting what you saw above.
>
> $ mkdir -p /var/tmp/j/y && cd /var/tmp/j/y
> $ git init; git init clone2
> $ : >3; : >clone2/2
> $ GIT_DIR=clone2/.git git add clone2/2 3
> $ GIT_DIR=clone2/.git git ls-files
> 3
> clone2/2
>
> The behavour is different when clone2/.git already has commit, and
> whatever codepath that gives these two different behaviour needs
> to be fixed.
It's resolve_gitlink_ref() in treat_directory(). I think replacing
that call() with is_git_directory() would fix this problem. We may
want to do the same with remove_dir_recursively().
> When GIT_DIR=clone2/.git is given, however, the caller explicitly declines
> the repository discovery. We do not know how the repository we are dealing
> with (which we were explicitly told with $GIT_DIR) and a directory whose
> name is ".git" under "clone2" we happened to find in read_directory()
> relates to each other, especially when our index does not have clone2 as
> our submodule.
>
> We however *do* know that our working tree is our current directory, so
> it would be wrong to do this:
>
> $ GIT_DIR=clone2/.git git add clone2/2 3; echo $?
> error: 3 is outside our repository, possibly goverened by .git
> 1
>
> The command should just add clone2/2 and 3 as it was told to.
I am concerned about clone2/2 in this case, not 3. I guess we can
check if clone2/.git is the repo we are using. If it is, skip it.
--
Duy
^ permalink raw reply
* [PATCH] typo in git-gui/lib/sshkeys.tcl
From: Dejan Ribič @ 2011-10-30 10:15 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 258 bytes --]
Hi,
I am new at this, but I am trying to fix a few "bitesize" bugs in
Ubuntu for start and somebody suggested, that I post this patch to
Upstream, so I'm doing this.
Cheers,
Dejan
P.S.: I am not subscribed to this list, so please CC to me.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: sshkeys.tcl.diff --]
[-- Type: text/x-patch; name="sshkeys.tcl.diff", Size: 501 bytes --]
=== modified file 'git-gui/lib/sshkey.tcl'
--- git-gui/lib/sshkey.tcl 2010-04-03 15:07:19 +0000
+++ git-gui/lib/sshkey.tcl 2011-10-29 23:08:15 +0000
@@ -117,7 +117,7 @@
} else {
set finfo [find_ssh_key]
if {$finfo eq {}} {
- set sshkey_title [mc "Generation succeded, but no keys found."]
+ set sshkey_title [mc "Generation succeeded, but no keys found."]
$w.contents insert end $sshkey_output
} else {
set sshkey_title [mc "Your key is in: %s" [lindex $finfo 0]]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 554 bytes --]
^ permalink raw reply
* Re: [PATCH] typo in git-gui/lib/sshkeys.tcl
From: Ramkumar Ramachandra @ 2011-10-30 14:47 UTC (permalink / raw)
To: Dejan Ribič; +Cc: git
In-Reply-To: <4EAD23AA.8000400@gmail.com>
Hi Dejan,
Dejan Ribič writes:
> I am new at this, but I am trying to fix a few "bitesize" bugs in
> Ubuntu for start and somebody suggested, that I post this patch to
> Upstream, so I'm doing this.
Welcome to the Git community. Please read
Documentation/SubmittingPatches so that your patch can be considered
for inclusion: in short, prepare and send the patch using 'git
format-patch'/ 'git send-email' along with a nice commit message and
signoff.
> P.S.: I am not subscribed to this list, so please CC to me.
That's the default convention on this list- no need to specify explicitly.
Cheers.
-- Ram
^ permalink raw reply
* [PATCH] Fix a typo in line 117 of git-gui/lib/sshkeys.tcl.
From: Dejan Ribič @ 2011-10-30 17:18 UTC (permalink / raw)
To: Git ML; +Cc: Dejan Ribič
"succeded" changed to "succeeded".
Reference Launchpad bug #879427.
modified: git-gui/lib/sshkey.tcl
Signed-off-by: Dejan Ribič <dejan.ribic@gmail.com>
---
git-gui/lib/sshkey.tcl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-gui/lib/sshkey.tcl b/git-gui/lib/sshkey.tcl
index 5f75bc9..aa6457b 100644
--- a/git-gui/lib/sshkey.tcl
+++ b/git-gui/lib/sshkey.tcl
@@ -117,7 +117,7 @@ proc read_sshkey_output {fd w} {
} else {
set finfo [find_ssh_key]
if {$finfo eq {}} {
- set sshkey_title [mc "Generation succeded, but no keys found."]
+ set sshkey_title [mc "Generation succeeded, but no keys found."]
$w.contents insert end $sshkey_output
} else {
set sshkey_title [mc "Your key is in: %s" [lindex $finfo 0]]
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] gitweb/Makefile: Remove static/gitweb.js in the clean target
From: Ramsay Jones @ 2011-10-29 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, Drew Northup, GIT Mailing-list
In-Reply-To: <7vk47qz5na.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>
>>> gitweb.js is nowadays a generated file. Though that bit should be
>>> in commit message...
>> Yep, will do ...
>
> Thanks; here is what I already queued.
Yeah, I saw this about two hours after I sent those emails ...
Also, I prefer your commit message! ;-)
Thanks.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Jakub Narebski @ 2011-10-30 18:56 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git, Jakub Narebski
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Those are additional comments, most of which I have come about when
rewriting this series and testing it.
There are to complement existing comments in other post(s).
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> ---
> gitweb/gitweb.perl | 81 +++++++++++++++++++++++++++++++++++++++++----
> gitweb/static/gitweb.css | 15 ++++++++
> 2 files changed, 88 insertions(+), 8 deletions(-)
No tests.
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (
> extra_options => "opt",
> search_use_regexp => "sr",
> ctag => "by_tag",
> + diff_style => "ds",
> # this must be last entry (for manipulation from JavaScript)
> javascript => "js"
> );
Alternate solution would be to use 'extra_options' ("opt") for
that... though current use of it in gitweb seems to suggest that it is
more about passing extra options to underlying git commands; and "git
diff" doesn't support '--side-by-side' like GNU diff does, (yet?).
So currently I favor neither.
> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
> }
> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
> }
> +
> + $input_params{diff_style} ||= 'inline';
> }
Hmmm... similar option 'order' ("o") had default value set in action
subroutine. I wonder if it wouldn't be better to do the same also in
this situation.
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
> - return "$div_open$line</div>\n";
> + return $diff_class, "$div_open$line</div>\n";
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
This subroutine no longer *format* line to be printed, isn't it?
> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
> print "</table>\n";
> }
>
> +sub format_diff_chunk {
Name: it is not about diff chunk (or hunk), but about a block of lines
in a chunk; in this case block of change lines (rem / add). Also, it
is not about generic diff, only about sidebyside one.
BTW. I think it would be better if this subroutine also managed
context lines.
> + my @chunk = @_;
> +
> + my $first_class = $chunk[0]->[0];
Style: You can use simply $chunk[0][0] here. perlref(1) says:
"The arrow is optional between brackets subscripts, [...]"
> + my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> + if (scalar @partial < scalar @chunk) {
Style: you can write simply
+ if (@partial < @chunk) {
> + return join '', ("<div class='chunk'><div class='old'>",
> + @partial,
> + "</div>",
> + "<div class='new'>",
> + (map {
> + $_->[1];
> + } @chunk[scalar @partial..scalar @chunk-1]),
> + "</div></div>");
> + } else {
> + return join '', ("<div class='chunk'><div class='",
> + ($first_class eq 'add' ? 'new' : 'old'),
> + "'>",
> + @partial,
> + "</div></div>");
> + }
> +}
Anyway this code is not very clear. You rely on the fact that if
there are two classes, then they are "rem" first, and "add" second.
Also, it is I think overly complicated.
> +
> sub git_patchset_body {
> - my ($fd, $difftree, $hash, @hash_parents) = @_;
> + my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;
Rather than passing $is_inline, I think it would be better to pass
$diff_style (with default value filled in)
> my ($hash_parent) = $hash_parents[0];
>
> my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
>
> # the patch itself
> LINE:
> + my @chunk;
> while ($patch_line = <$fd>) {
> chomp $patch_line;
>
> next PATCH if ($patch_line =~ m/^diff /);
Here is a bug. If patchset consists of more than one patch, if
not-last patches have change that does not have trailing context lines
(changed, added or removed lines at the end of file), then the last
block will be lost (@chunk can be non-empty here).
>
> - print format_diff_line($patch_line, \%from, \%to);
> + my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> + if ($is_inline) {
That is wrong to test for. You should test if you can use
side-by-side diff, not if you use default output.
Especially that diff can be combined diff of a merge commit, which
cannot be represented as 2-sided side-by-side diff; for such diff
gitweb needs to use inline diff.
> + print $line;
> + } elsif ($class eq 'add' || $class eq 'rem') {
> + push @chunk, [ $class, $line ];
> + } else {
> + if (@chunk) {
> + print format_diff_chunk(@chunk);
> + @chunk = ();
> + } elsif ($class eq 'chunk_header') {
> + print $line;
> + } else {
> + print '<div class="chunk"><div class="old">',
> + $line,
> + '</div><div class="new">',
> + $line,
> + '</div></div>';
All of this should in my opinion be done in format_diff_chunk(), not
in caller. This also introduces a bit of inconsistency in that
added/removed lines are in single block and context lines are each in
its own block.
Additionally you forgot about incomplete lines here, which can apply
either to added lines, removed lines, both of added and removed lines,
and to context lines. Your code generates incorrect info in the case
if incomplete line is either removed line only, or added line only.
[Nb. I have to check my code yet again.]
> +sub diff_nav {
> + my ($style) = @_;
> +
> + my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> + join '', ($cgi->start_form({ method => 'get' }),
> + $cgi->hidden('p'),
> + $cgi->hidden('a'),
> + $cgi->hidden('h'),
> + $cgi->hidden('hp'),
> + $cgi->hidden('hb'),
> + $cgi->hidden('hpb'),
> + $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> + $cgi->submit('change'),
> + $cgi->end_form);
> +}
What about 'f' and 'fp' for "blobdiff" view?
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
> cursor: pointer;
> }
>
> +/* side-by-side diff */
> +div.chunk {
> + overflow: hidden;
> +}
> +
> +div.chunk div.old {
> + float: left;
> + width: 50%;
> + overflow: hidden;
> +}
> +
> +div.chunk div.new {
> + margin-left: 50%;
> + width: 50%;
> +}
Nice trick of composing CSS layout... though I wonder if there is
perhaps a better solution.
Anyway, I think this addition should be put near style for div.diff
etc.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] document 'T' status from git-status
From: Mark Dominus @ 2011-10-30 20:34 UTC (permalink / raw)
To: git; +Cc: Mark Dominus
In-Reply-To: <7vmxcjro5t.fsf@alter.siamese.dyndns.org>
On 10/30/2011 02:25 AM, Junio C Hamano wrote:
> Mark Dominus<mjd@plover.com> writes:
>
>
>> +* 'T' = file type changed
>> + (typically from plain file to symlink, or vice versa)
>>
>> Ignored files are not listed, unless `--ignored` option is in effect,
>> in which case `XY` are `!!`.
>> @@ -134,9 +136,11 @@ in which case `XY` are `!!`.
>> D [ M] deleted from index
>> R [ MD] renamed in index
>> C [ MD] copied in index
>> + T [ MD] file type changed in index
>> [MARC] index and work tree matches
>> [ MARC] M work tree changed since index
>> [ MARC] D deleted in work tree
>> + [ MARC] T file type changed in work tree
> The current organization of this table may need to be rethought, but if we
> were to keep it, then this change is far from sufficient. For example, you
> do not explain what XY = TT means.
Thanks for your response.
I did not try to document that because in my experimenting I was not
able to produce that situation. T occurs when the filetype (as reported
by the IS_FMT macro) is different between the two files. On systems I
have available, there are essentially five filetypes: plain file,
symlink, directory, block and character devices. Directories are
handled separately and are not reported by git-status. Device files
cannot be added to the index at all. That leaves only two possible
filetypes, so of the three files (the committed version, the cached
version, and the working tree version) two must be the same.
I am aware that on some systems other filetypes may exist. For example,
HPUX has an 'H' filetype that is a variant of a directory. But git
would treat this as a directory. Since I was not aware of any situation
in which TT could arise, I did not try to document it.
Will you be applying the alternative patch you suggested, or would you
prefer that I try to produce one along those lines?
^ permalink raw reply
* Re: [msysGit] Re: What's cooking in git.git (Oct 2011, #11; Fri, 28)
From: Johannes Sixt @ 2011-10-30 21:43 UTC (permalink / raw)
To: kusmabite; +Cc: Junio C Hamano, git, msysGit
In-Reply-To: <CABPQNSYi7gJKbUb7y2hNvF9KXXyt8ShgJD8AoBhryGwAxp6ejw@mail.gmail.com>
Am 29.10.2011 17:42, schrieb Erik Faye-Lund:
> On Fri, Oct 28, 2011 at 8:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> * ef/mingw-upload-archive (2011-10-26) 3 commits
>> - upload-archive: use start_command instead of fork
>> - compat/win32/poll.c: upgrade from upstream
>> - mingw: move poll out of sys-folder
>>
>> Are msysgit folks OK with this series (I didn't see msysgit list Cc'ed on
>> these patches)? If so let's move this forward, as the changes to the core
>> part seem solid.
>
> The msysgit list not being Cc'ed on the patches was a slip-up on my
> behalf. I believe the changes are relatively uncontroversial from an
> msysgit point of view, though. However, an ack/nack would be
> appreciated ;)
The patch series looks good and passes my tests. Therefore:
Acked-by: Johannes Sixt <j6t@kdbg.org>
-- Hannes
^ permalink raw reply
* [PATCHv3 0/8] gitweb: side-by-side diff
From: Jakub Narebski @ 2011-10-30 23:36 UTC (permalink / raw)
To: git; +Cc: Kato Kazuyoshi, Jakub Narebski
NOTE: As it is feature-freeze period, this patch series is for review.
This is refinement and extension of Kato Kazuyoshi patch series, sent
originally as
[PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
http://thread.gmane.org/gmane.comp.version-control.git/183744
and then refined and split into two-patch series
[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
http://thread.gmane.org/gmane.comp.version-control.git/183770
[PATCH 2/2] gitweb: add a feature to show side-by-side diff
http://thread.gmane.org/gmane.comp.version-control.git/183769
This patch series originally started as rebasing second patch in above
two part series on top of diff line classification refactoring
suggested by me and proposed by Junio. Then I thought about putting
all code printing side-by-side diff in print_sidebyside_diff_chunk()
subroutine, then...
Main changes from v2 version from Kato Kazuyoshi:
* Built on top of refactoring of code related to diff
output formatting (patches 1 and 2)
* Code reworked so that is in my opinion easier to follow; gitweb now
handles merges and diffs with incomplete lines correctly (patch 3)
- well, it handles merges by turning off side-by-side diff for them.
* Adding background color to distinguish empty context lines from
vertical align, similarly to e.g.
http://community.activestate.com/files/images/sbsdiffs.png
but without refinement (word diff of changes).
* Adds some very basic test for side-by-side diff (patch 6 (and 5))
* Split adding navigation into a separate commit, and uses [nav] links
rather than HTML form for selecting between inline and side-by-side
diff style (diff 8). Thanks to more thorough use of href(-replay=>1,..)
(patch 7) style of diff should be preserved with this series.
Please excuse me for essentially hijacking this patch series.
P.S. I really, really need to finish work on splitting gitweb into
smaller pieces. With around 8,000 lines it becomes quite unwieldy.
But this would probably need total rework of error handling (the
die_error subroutine), and that would need another changes, etc....
Pull request:
~~~~~~~~~~~~~
These changes are available in the git repository(-y/+ies) at:
git://repo.or.cz/git/jnareb-git.git gitweb/side-by-side-diff-v4
git://github.com/jnareb/git gitweb/side-by-side-diff-v4
Table of contents:
~~~~~~~~~~~~~~~~~~
[PATCHv3 1/8] gitweb: Refactor diff body line classification
[PATCHv3 2/8] gitweb: Extract formatting of diff chunk header
[PATCHv3 3/8] gitweb: Add a feature to show side-by-side diff
[PATCHv3 4/8] gitweb: Give side-by-side diff extra CSS styling
[PATCHv3 5/8] t9500: Add test for handling incomplete lines in diff
by gitweb
[PATCHv3 6/8] t9500: Add basic sanity tests for side-by-side diff in
gitweb
[PATCHv3 7/8] gitweb: Use href(-replay=>1,...) for formats links in
"commitdiff"
[PATCHv3 8/8] gitweb: Add navigation to select side-by-side diff
Shortlog:
~~~~~~~~~
Jakub Narebski (6):
gitweb: Refactor diff body line classification
gitweb: Extract formatting of diff chunk header
gitweb: Give side-by-side diff extra CSS styling
t9500: Add test for handling incomplete lines in diff by gitweb
t9500: Add basic sanity tests for side-by-side diff in gitweb
gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"
Kato Kazuyoshi (2):
gitweb: Add a feature to show side-by-side diff
gitweb: Add navigation to select side-by-side diff
Diffstat:
~~~~~~~~~
gitweb/gitweb.perl | 339 +++++++++++++++++++++++--------
gitweb/static/gitweb.css | 30 +++
t/t9500-gitweb-standalone-no-errors.sh | 73 +++++++-
3 files changed, 353 insertions(+), 89 deletions(-)
--
1.7.6
^ permalink raw reply
* [PATCHv3 1/8] gitweb: Refactor diff body line classification
From: Jakub Narebski @ 2011-10-30 23:36 UTC (permalink / raw)
To: git; +Cc: Kato Kazuyoshi, Jakub Narebski, Junio C Hamano
In-Reply-To: <1320017787-18048-1-git-send-email-jnareb@gmail.com>
Simplify classification of diff line body in format_diff_line(),
replacing two long if-elsif chains (one for ordinary diff and one for
combined diff of a merge commit) with a single regexp match. Refactor
this code into diff_line_class() function.
While at it:
* Fix an artifact in that $diff_class included leading space to be
able to compose classes like this "class=\"diff$diff_class\"', even
when $diff_class was an empty string. This made code unnecessary
ugly: $diff_class is now just class name or an empty string.
* Introduce "ctx" class for context lines ($diff_class was set to ""
in this case before this commit).
Idea and initial code by Junio C Hamano, polish and testing by Jakub
Narebski. Inspired by patch adding side-by-side diff by Kato Kazuyoshi,
which required $diff_class to be name of class without extra space.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this version of side-by-side diff series.
Junio, as your proposal was of "what if" variety, and not as a proper
patch, I have taken authorship (after reqorking and testing it).
Should I revert authorship to you?
gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f0c3bd..914fd4c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2225,40 +2225,47 @@ sub format_diff_cc_simplified {
return $result;
}
+sub diff_line_class {
+ my ($line, $from, $to) = @_;
+
+ # ordinary diff
+ my $num_sign = 1;
+ # combined diff
+ if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
+ $num_sign = scalar @{$from->{'href'}};
+ }
+
+ my @diff_line_classifier = (
+ { regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
+ { regexp => qr/^\\/, class => "incomplete" },
+ { regexp => qr/^ {$num_sign}/, class => "ctx" },
+ # classifier for context must come before classifier add/rem,
+ # or we would have to use more complicated regexp, for example
+ # qr/(?= {0,$m}\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;
+ { regexp => qr/^[+ ]{$num_sign}/, class => "add" },
+ { regexp => qr/^[- ]{$num_sign}/, class => "rem" },
+ );
+ for my $clsfy (@diff_line_classifier) {
+ return $clsfy->{'class'}
+ if ($line =~ $clsfy->{'regexp'});
+ }
+
+ # fallback
+ return "";
+}
+
# format patch (diff) line (not to be used for diff headers)
sub format_diff_line {
my $line = shift;
my ($from, $to) = @_;
- my $diff_class = "";
+
+ my $diff_class = diff_line_class($line, $from, $to);
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
chomp $line;
-
- if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
- # combined diff
- my $prefix = substr($line, 0, scalar @{$from->{'href'}});
- if ($line =~ m/^\@{3}/) {
- $diff_class = " chunk_header";
- } elsif ($line =~ m/^\\/) {
- $diff_class = " incomplete";
- } elsif ($prefix =~ tr/+/+/) {
- $diff_class = " add";
- } elsif ($prefix =~ tr/-/-/) {
- $diff_class = " rem";
- }
- } else {
- # assume ordinary diff
- my $char = substr($line, 0, 1);
- if ($char eq '+') {
- $diff_class = " add";
- } elsif ($char eq '-') {
- $diff_class = " rem";
- } elsif ($char eq '@') {
- $diff_class = " chunk_header";
- } elsif ($char eq "\\") {
- $diff_class = " incomplete";
- }
- }
$line = untabify($line);
+
if ($from && $to && $line =~ m/^\@{2} /) {
my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
@@ -2276,7 +2283,7 @@ sub format_diff_line {
}
$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
} elsif ($from && $to && $line =~ m/^\@{3}/) {
my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
@@ -2309,9 +2316,9 @@ sub format_diff_line {
}
$line .= " $prefix</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
}
- return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+ return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
--
1.7.6
^ permalink raw reply related
* [PATCHv3 2/8] gitweb: Extract formatting of diff chunk header
From: Jakub Narebski @ 2011-10-30 23:36 UTC (permalink / raw)
To: git; +Cc: Kato Kazuyoshi, Jakub Narebski
In-Reply-To: <1320017787-18048-1-git-send-email-jnareb@gmail.com>
Refactor main parts of HTML-formatting for diff chunk headers
(formatting means here adding links and syntax hightlighting) into
separate subroutines:
* format_unidiff_chunk_header for ordinary diff,
* format_cc_diff_chunk_header for combined diff
(more than one parent)
This makes format_diff_line() subroutine easier to follow.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this version of side-by-side diff series.
After those changes format_diff_line() fits in one page (has less than
25 lines). Nice, isn't it?
gitweb/gitweb.perl | 114 ++++++++++++++++++++++++++++++---------------------
1 files changed, 67 insertions(+), 47 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 914fd4c..95d278a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2254,6 +2254,69 @@ sub diff_line_class {
return "";
}
+# assumes that $from and $to are defined and correctly filled,
+# and that $line holds a line of chunk header for unified diff
+sub format_unidiff_chunk_header {
+ my ($line, $from, $to) = @_;
+
+ my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
+ $line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
+
+ $from_lines = 0 unless defined $from_lines;
+ $to_lines = 0 unless defined $to_lines;
+
+ if ($from->{'href'}) {
+ $from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start",
+ -class=>"list"}, $from_text);
+ }
+ if ($to->{'href'}) {
+ $to_text = $cgi->a({-href=>"$to->{'href'}#l$to_start",
+ -class=>"list"}, $to_text);
+ }
+ $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
+ "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+ return $line;
+}
+
+# assumes that $from and $to are defined and correctly filled,
+# and that $line holds a line of chunk header for combined diff
+sub format_cc_diff_chunk_header {
+ my ($line, $from, $to) = @_;
+
+ my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
+ my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
+
+ @from_text = split(' ', $ranges);
+ for (my $i = 0; $i < @from_text; ++$i) {
+ ($from_start[$i], $from_nlines[$i]) =
+ (split(',', substr($from_text[$i], 1)), 0);
+ }
+
+ $to_text = pop @from_text;
+ $to_start = pop @from_start;
+ $to_nlines = pop @from_nlines;
+
+ $line = "<span class=\"chunk_info\">$prefix ";
+ for (my $i = 0; $i < @from_text; ++$i) {
+ if ($from->{'href'}[$i]) {
+ $line .= $cgi->a({-href=>"$from->{'href'}[$i]#l$from_start[$i]",
+ -class=>"list"}, $from_text[$i]);
+ } else {
+ $line .= $from_text[$i];
+ }
+ $line .= " ";
+ }
+ if ($to->{'href'}) {
+ $line .= $cgi->a({-href=>"$to->{'href'}#l$to_start",
+ -class=>"list"}, $to_text);
+ } else {
+ $line .= $to_text;
+ }
+ $line .= " $prefix</span>" .
+ "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+ return $line;
+}
+
# format patch (diff) line (not to be used for diff headers)
sub format_diff_line {
my $line = shift;
@@ -2267,56 +2330,13 @@ sub format_diff_line {
$line = untabify($line);
if ($from && $to && $line =~ m/^\@{2} /) {
- my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
- $line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
-
- $from_lines = 0 unless defined $from_lines;
- $to_lines = 0 unless defined $to_lines;
-
- if ($from->{'href'}) {
- $from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start",
- -class=>"list"}, $from_text);
- }
- if ($to->{'href'}) {
- $to_text = $cgi->a({-href=>"$to->{'href'}#l$to_start",
- -class=>"list"}, $to_text);
- }
- $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
- "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+ $line = format_unidiff_chunk_header($line, $from, $to);
return "<div class=\"$diff_classes\">$line</div>\n";
+
} elsif ($from && $to && $line =~ m/^\@{3}/) {
- my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
- my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
-
- @from_text = split(' ', $ranges);
- for (my $i = 0; $i < @from_text; ++$i) {
- ($from_start[$i], $from_nlines[$i]) =
- (split(',', substr($from_text[$i], 1)), 0);
- }
-
- $to_text = pop @from_text;
- $to_start = pop @from_start;
- $to_nlines = pop @from_nlines;
-
- $line = "<span class=\"chunk_info\">$prefix ";
- for (my $i = 0; $i < @from_text; ++$i) {
- if ($from->{'href'}[$i]) {
- $line .= $cgi->a({-href=>"$from->{'href'}[$i]#l$from_start[$i]",
- -class=>"list"}, $from_text[$i]);
- } else {
- $line .= $from_text[$i];
- }
- $line .= " ";
- }
- if ($to->{'href'}) {
- $line .= $cgi->a({-href=>"$to->{'href'}#l$to_start",
- -class=>"list"}, $to_text);
- } else {
- $line .= $to_text;
- }
- $line .= " $prefix</span>" .
- "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+ $line = format_cc_diff_chunk_header($line, $from, $to);
return "<div class=\"$diff_classes\">$line</div>\n";
+
}
return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
}
--
1.7.6
^ permalink raw reply related
* [PATCHv3 3/8] gitweb: Add a feature to show side-by-side diff
From: Jakub Narebski @ 2011-10-30 23:36 UTC (permalink / raw)
To: git; +Cc: Kato Kazuyoshi, Jakub Narebski
In-Reply-To: <1320017787-18048-1-git-send-email-jnareb@gmail.com>
From: Kato Kazuyoshi <kato.kazuyoshi@gmail.com>
This commits adds to support for showing "side-by-side" style diff.
Currently you have to hand-craft the URL; navigation for selecting
diff style is to be added in the next commit.
The diff output in unified format from "git diff-tree" is reorganized to
side-by-side style chunk by chunk with format_sidebyside_diff_chunk().
This reorganization requires knowledge about diff line classification,
so format_diff_line() was renamed to process_diff_line(), and changed to
return tuple (list) consisting of class of diff line and of
HTML-formatted (but not wrapped in <div class="diff ...">...</div>) diff
line. Wrapping is now done by caller, i.e. git_patchset_body().
Gitweb uses float+margin CSS-based layout for "side by side" diff.
You can specify style of diff with "ds" ('diff_style') query
parameter. Currently supported values are 'inline' and 'sidebyside';
the default is 'inline'.
Another solution would be to use "opt" ('extra_options') for that...
though current use of it in gitweb seems to suggest that "opt" is more
about passing extra options to underlying git commands, and "git diff"
doesn't support '--side-by-side' like GNU diff does, (yet?).
Signed-off-by: Kato Kazuyoshi <kato.kazuyoshi@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Heavily changed from original submission by Kato Kazuyoshi, but the
main idea, code structure (somewhat) and CSS-base layout remains.
That's why authorship remains with him; Kato, please speak up if you
would like to change this.
The decision to move some of formatting outside process_diff_line()
(formerly format_diff_line()) was not really necessary, in hindsight...
gitweb/gitweb.perl | 116 +++++++++++++++++++++++++++++++++++++++++----
gitweb/static/gitweb.css | 17 +++++++
2 files changed, 122 insertions(+), 11 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95d278a..68629f6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -759,6 +759,7 @@ our @cgi_param_mapping = (
extra_options => "opt",
search_use_regexp => "sr",
ctag => "by_tag",
+ diff_style => "ds",
# this must be last entry (for manipulation from JavaScript)
javascript => "js"
);
@@ -2317,28 +2318,27 @@ sub format_cc_diff_chunk_header {
return $line;
}
-# format patch (diff) line (not to be used for diff headers)
-sub format_diff_line {
+# process patch (diff) line (not to be used for diff headers),
+# returning class and HTML-formatted (but not wrapped) line
+sub process_diff_line {
my $line = shift;
my ($from, $to) = @_;
my $diff_class = diff_line_class($line, $from, $to);
- my $diff_classes = "diff";
- $diff_classes .= " $diff_class" if ($diff_class);
chomp $line;
$line = untabify($line);
if ($from && $to && $line =~ m/^\@{2} /) {
$line = format_unidiff_chunk_header($line, $from, $to);
- return "<div class=\"$diff_classes\">$line</div>\n";
+ return $diff_class, $line;
} elsif ($from && $to && $line =~ m/^\@{3}/) {
$line = format_cc_diff_chunk_header($line, $from, $to);
- return "<div class=\"$diff_classes\">$line</div>\n";
+ return $diff_class, $line;
}
- return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+ return $diff_class, esc_html($line, -nbsp=>1);
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
@@ -4860,8 +4860,78 @@ sub git_difftree_body {
print "</table>\n";
}
+sub print_sidebyside_diff_chunk {
+ my @chunk = @_;
+ my (@ctx, @rem, @add);
+
+ return unless @chunk;
+
+ # incomplete last line might be among removed or added lines,
+ # or both, or among context lines: find which
+ for (my $i = 1; $i < @chunk; $i++) {
+ if ($chunk[$i][0] eq 'incomplete') {
+ $chunk[$i][0] = $chunk[$i-1][0];
+ }
+ }
+
+ # guardian
+ push @chunk, ["", ""];
+
+ foreach my $line_info (@chunk) {
+ my ($class, $line) = @$line_info;
+
+ # print chunk headers
+ if ($class && $class eq 'chunk_header') {
+ print $line;
+ next;
+ }
+
+ ## print from accumulator when type of class of lines change
+ # empty contents block on start rem/add block, or end of chunk
+ if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
+ print join '',
+ '<div class="chunk_block">',
+ '<div class="old">',
+ @ctx,
+ '</div>',
+ '<div class="new">',
+ @ctx,
+ '</div>',
+ '</div>';
+ @ctx = ();
+ }
+ # empty add/rem block on start context block, or end of chunk
+ if ((@rem || @add) && (!$class || $class eq 'ctx')) {
+ print join '',
+ '<div class="chunk_block">',
+ '<div class="old">',
+ @rem,
+ '</div>',
+ '<div class="new">',
+ @add,
+ '</div>',
+ '</div>';
+ @rem = @add = ();
+ }
+
+ ## adding lines to accumulator
+ # guardian value
+ last unless $line;
+ # rem, add or change
+ if ($class eq 'rem') {
+ push @rem, $line;
+ } elsif ($class eq 'add') {
+ push @add, $line;
+ }
+ # context line
+ if ($class eq 'ctx') {
+ push @ctx, $line;
+ }
+ }
+}
+
sub git_patchset_body {
- my ($fd, $difftree, $hash, @hash_parents) = @_;
+ my ($fd, $diff_style, $difftree, $hash, @hash_parents) = @_;
my ($hash_parent) = $hash_parents[0];
my $is_combined = (@hash_parents > 1);
@@ -4871,6 +4941,7 @@ sub git_patchset_body {
my $diffinfo;
my $to_name;
my (%from, %to);
+ my @chunk; # for side-by-side diff
print "<div class=\"patchset\">\n";
@@ -4977,10 +5048,29 @@ sub git_patchset_body {
next PATCH if ($patch_line =~ m/^diff /);
- print format_diff_line($patch_line, \%from, \%to);
+ my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
+ my $diff_classes = "diff";
+ $diff_classes .= " $class" if ($class);
+ $line = "<div class=\"$diff_classes\">$line</div>\n";
+
+ if ($diff_style eq 'sidebyside' && !$is_combined) {
+ if ($class eq 'chunk_header') {
+ print_sidebyside_diff_chunk(@chunk);
+ @chunk = ( [ $class, $line ] );
+ } else {
+ push @chunk, [ $class, $line ];
+ }
+ } else {
+ # default 'inline' style and unknown styles
+ print $line;
+ }
}
} continue {
+ if (@chunk) {
+ print_sidebyside_diff_chunk(@chunk);
+ @chunk = ();
+ }
print "</div>\n"; # class="patch"
}
@@ -6976,6 +7066,7 @@ sub git_object {
sub git_blobdiff {
my $format = shift || 'html';
+ my $diff_style = $input_params{'diff_style'} || 'inline';
my $fd;
my @difftree;
@@ -7085,7 +7176,8 @@ sub git_blobdiff {
if ($format eq 'html') {
print "<div class=\"page_body\">\n";
- git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base);
+ git_patchset_body($fd, $diff_style,
+ [ \%diffinfo ], $hash_base, $hash_parent_base);
close $fd;
print "</div>\n"; # class="page_body"
@@ -7113,6 +7205,7 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my %params = @_;
my $format = $params{-format} || 'html';
+ my $diff_style = $input_params{'diff_style'} || 'inline';
my ($patch_max) = gitweb_get_feature('patches');
if ($format eq 'patch') {
@@ -7316,7 +7409,8 @@ sub git_commitdiff {
$use_parents ? @{$co{'parents'}} : $hash_parent);
print "<br/>\n";
- git_patchset_body($fd, \@difftree, $hash,
+ git_patchset_body($fd, $diff_style,
+ \@difftree, $hash,
$use_parents ? @{$co{'parents'}} : $hash_parent);
close $fd;
print "</div>\n"; # class="page_body"
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 7d88509..21842a6 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -475,6 +475,23 @@ div.diff.nodifferences {
color: #600000;
}
+/* side-by-side diff */
+div.chunk_block {
+ overflow: hidden;
+}
+
+div.chunk_block div.old {
+ float: left;
+ width: 50%;
+ overflow: hidden;
+}
+
+div.chunk_block div.new {
+ margin-left: 50%;
+ width: 50%;
+}
+
+
div.index_include {
border: solid #d9d8d1;
border-width: 0px 0px 1px;
--
1.7.6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox