login
Search
This small patchset introduces notes support in gitweb. The feature is designed to be powerful and flexible: any amount of notes ref spaces can be used, and per-namespace styling is possible. The implementation is not exactly high-performance, needing no less than one extra git call per commit per notes ref space (two if a note exist), and is thus disabled by default. It's quite likely that appropriate C plumbing in git could speed this considerably; the current implementation, OTOH, has the advantage of working even when the git core does not support notes itself. Giuseppe Bilotta (4): gitweb: notes feature gitweb: show notes in shortlog view gitweb: show notes in log gitweb: show notes in commit(diff) view gitweb/gitweb.css | 51 +++++++++++++++++++++++ gitweb/gitweb.perl | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 166 insertions(+), 1 deletions(-) --
Introduce support for notes by collecting them when creating commit
lists. The list of noterefs to look into is configurable, and can be a(n
array of) refspec(s), which will be looked for in the refs/notes
namespace.
The feature is disabled by default because it's presently not very
efficient (one extra git call per configured refspec, plus two extra git
calls per commit per noteref).
---
gitweb/gitweb.perl | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d0c3ff2..9ba5815 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -411,6 +411,22 @@ our %feature = (
'override' => 0,
'default' => [16]},
+ # Notes support. When this feature is enabled, the presence of notes
+ # for any commit is signaled, and the note content is made available
+ # in a way appropriate for the current view.
+ # Set this to '*' to enable all notes namespace, or to a shell-glob
+ # specification to enable specific namespaces only.
+
+ # To enable system wide have in $GITWEB_CONFIG
+ # $feature{'notes'}{'default'} = ['*'];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'notes'}{'override'} = 1;
+ # and in project config gitweb.notes = namespace;
+ 'notes' => {
+ 'sub' => \&feature_notes,
+ 'override' => 0,
+ 'default' => []},
+
# Avatar support. When this feature is enabled, views such as
# shortlog or commit will display an avatar associated with
# the email of the committer(s) and/or author(s).
@@ -513,6 +529,16 @@ sub feature_patches {
return ($_[0]);
}
+sub feature_notes {
+ my @val = (git_get_project_config('notes'));
+
+ if (@val) {
+ return @val;
+ }
+
+ return @_;
+}
+
sub feature_avatar {
my @val = (git_get_project_config('avatar'));
@@ -2786,10 +2812,30 @@ sub parse_commit {
return %co;
}
+# return all refs matching refs/notes/<globspecs> where the globspecs
+# are taken ...I think this look-up is wrong (meaning: will stop working anytime in the future, and needs to be rewritten). Other parts of this patch looked Ok from a cursory reading, though. --
IOW, the code should be reading output from:
GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
as the notes tree may not be storing notes in a flat one-level namespace
like you are assuming.
--
First, for some mechanism of deployment (IIRC Apache's mod_perl) changes
to environment variables from CGI script are not passed to invoked
commands (I guess for security reasons). That is why gitweb uses --git-dir
parameter to git wrapper, and not GIT_DIR environment variable since
25691fb (gitweb: Use --git-dir parameter instead of setting $ENV{'GIT_DIR'},
2006-08-28). So for proper support we would need --notes-ref (or similar)
option to git wrapper
git --notes-ref=$note_ref show -s --format=%N $co{'id'}
Second, parse_commit / parse_commits use
git rev-list -z --parents --header --max-count-X
If this command automatically shows notes (or it can be modified to
automatically show notes) after unindented "Notes:" line (as per
git-notes documentation), then the only thing that needs to be
changed to fill %commit{'notes'} is parse_commit_text subroutine.
There would be no need for extra subroutine (and extra command, or
even up to two extra commands per note) to get notes data.
--
Jakub Narebski
Poland
--
As I mentioned on the cover letter, I was hoping to be able to make something that could be deployable without requiring core changes and thus a specific minimum git version. I do realize however that this is inherently not robust (unless the code is updated if and when the notes storage mechanism changes). The wrapper is probably needed anyway, I would say, so it makes sense to implement it. However, see below for a caveat about its But unless the --notes-ref is made to accept multiple refspecs, this will only access _one_ note namespace. It is not hard to envision situations where multiple namespaces are used for the same repo (e.g. one to store git-svn metadata, one for bugzilla-related entries), in which case you'd want _all_ of them to be accessible in gitweb. And if we do support multiple refspecs for --notes-ref, we also need some way to identify which note belongs to which namespace, of course, in any of the output formats that include notes. An alternative approach would be some git notes-list command that accepts both multiple namespaces and multiple commits, and is specifically dedicated to display notes-commits relationships (together with notes content, obviously) -- Giuseppe "Oblomov" Bilotta --
AFAIU, the note code on the core side already creates a fan-out structure when notes tree gets large (see recent "What's cooking"; the series is parked in 'pu' but that is primarily because we are in feature freeze); it is not just "inherently not robust" but is much closer to "broken from day one" ;-). Otherwise I wouldn't have wasted time to point it out. Your code is a very good proof-of-concept, though. Regarding support of multiple notes hierarchies, listing, etc. See for example: http://thread.gmane.org/gmane.comp.version-control.git/138079/focus=138128 I expect more ideas from needs by end-user would come, as we gain experience with using notes in real projects. You will certainly find some other needs of your own, like the "not an environment but a command line option" which Jakub mentioned, and "multiple hierarchies" like both you and I found need for. Share them and let us together make the notes mechanism nicer to use. Thanks. --
Thanks. I guess at this point a proper implementation can wait for the Collecting those ideas together would also help define some sort of roadmap, or at least have a clear idea of what's needed, to help drive the design of the features themselves. Maybe we could start a TODO page on the wiki collecting these ideas? -- Giuseppe "Oblomov" Bilotta --
I already maintain a TODO list at the end of the cover letter to the notes
series. Here is a preview of it (I plan to send the next iteration of
jh/notes as soon as v1.7.0 is released):
- Suggestion by Matthieu Moy and Sverre Rabbelier:
Add notes support to git-format-patch, where note contents in
refs/notes/format-patch are added to the "comments section"
(i.e. following the '---' separator) of generated patches.
- Better integration with rebase/amend/cherry-pick. Optionally bring
notes across a commit rewrite. Controlled by command-line options
and/or config variables. Add "git notes move" and "git notes copy"
to suit. Junio says:
I used to fix minor issues (styles, decl-after-stmt, etc.) using
rebase-i long after running "am" in bulk, but these days I find
myself going back to my "inbox" and fix them in MUA; this is
only because I know these notes do not propagate across rebases
and amends -- adjusting the workflow to the tool's limitation is
not very good.
- Junio says:
The interface to tell tools to use which notes ref to use should be
able to say "these refs", not just "this ref" i.e. GIT_NOTES_REF=a:b
just like PATH=a:b:c...); I am fairly certain that we would want to
store different kind of information in separate notes trees and
aggregate them, as we gain experience with notes.
- Junio says:
There should be an interface to tell tools to use which notes refs via
command line options; "!alias" does not TAB-complete, and "git lgm"
above doesn't, either. "git log --notes=notes/amlog --notes=notes/other"
would probably be the way to go.
- Add a "git notes grep" subcommand: Junio says:
While reviewing the "inbox", I sometimes wonder if I applied a message
to somewhere already, but there is no obvious way to grep in the notes
tree and get the object name that a note is attached to. Of course I
know I can "git grep -c johan@herland.net notes/amlog" and it will give
me something like:
...Random additional thoughts.
* Futureproofing
We have to admit that our notes support, especially on the output side, is
still in its infancy. We may want to advertise it as such -- highly
experimental and subject to change.
* format-patch
To add notes to format-patch output, we might want to do something like:
$ git format-patch --notes-ref=commits --notes-ref=amlog -1
and produce:
From 8bff7c5383ed833bd1df9c8d85c00a27af3e5b02 Mon Sep 17 00:00:00 2001
From: Andrew Myrick <amyrick@apple.com>
Date: Sat, 30 Jan 2010 03:14:22 +0000
Subject: [PATCH] git-svn: persistent memoization
X-Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
from git://git.bogomips.org/git-svn.git/
X-Notes-amlog: <1264821262-28322-1-git-send-email-amyrick@apple.com>
Make memoization of the svn:mergeinfo processing functions persistent with
...
Points to notice:
- There is no point forcing users to spell "--notes-ref" parameter
starting from refs/notes/; we should DWIM if they are missing;
- We would want to allow more than one notes hierarchy specified. This
would affect format_note() function---take list of struct notes_tree,
perhaps;
- Allow callers of tell format_note() to add the name of the notes
hierarchy the note came from (or just always add it if it is not the
default "refs/notes/commits").
- For format-patch that produces a mbox output, the email header part may
be a better place to put notes (obeying the usual "indent by one space
to continue the line" convention).
* "log --format=%N" and "log --show-notes"
Currently %N expands to the hardcoded "log --show-notes" default format.
We can probably keep it that way. When the user asked for a non default
notes hierarchy (i.e. other than refs/notes/commits), we may want to
adjust "Notes:" string to use "Notes-%s:" to show which hierarchy it came
from, and concatenate them together.
For "log --show-notes" output, we also might want to move the notes to ...This is probably the most bothering issue: find, for each output format that involves notes, the smart way of also outputting the We might want to do without the dash in standard log output: Notes: The footer approach has the benefit of allowing multi-line notes to just be printed the same way as multi-line commit messages, whereas the header output would require one header line per commit line. -- Giuseppe "Oblomov" Bilotta --
Giuseppe was wondering about multi-line thing, so the illustration should
be adjusted to match the "format-patch" example to show a multi-line note,
I think. Here is what I meant.
Instead of showing:
$ git log --notes-ref=amlog -1 4d0cc22
commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
Author: Junio C Hamano <gitster@pobox.com>
Date: Thu Feb 4 11:10:44 2010 -0800
fast-import: count --max-pack-size in bytes
Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
...
Notes:
pulled on Fri Feb 5 07:36:12 2010 -0800
from git://git.bogomips.org/git-svn.git/
Notes-amlog:
<7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
which is what 1.6.6 added, showing it like this:
$ git log --notes-ref=amlog -1 4d0cc22
commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
Author: Junio C Hamano <gitster@pobox.com>
Date: Thu Feb 4 11:10:44 2010 -0800
Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
from git://git.bogomips.org/git-svn.git/
Notes-amlog: <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
fast-import: count --max-pack-size in bytes
Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
...
might be easier to see. After all, notes are metainformation on commits,
and people who are interested will look at the header, and those who are
not will skim over the block of text at the beginning, knowing that is
where all the metainformation is.
But this is just "might", not "should---I strongly believe".
--
I'm not convinced. I believe format-patch output should be as close as possible to RFC2822 compliance, given how it can be used as basis for actual email. According to the RFC, lines should be no more than 78 characters and must be less than 998. The 'one space indent on newline', if I'm not mistaken, is a way to indicate a _wrapped_ single line header, rather than a multi-line one. A scenario such as the following is thus possible: format-patch creates a single-line header which is longer than 78 characters (X-Git-Notes-somerefname: 70ish characters line). The patch gets sent by email, and a properly configured send-email sends the X-Git-Notes* headers through (BTW, this should probably be added to the notes/send-email TODO). Some intermediate forwarder or user agent decides to rewrap the longish line so that it isn't longer than 78 characters. The user saves the raw email and uses git am to apply it -> oops, the single line note has become a two-liner. If we use the convention (at least in format-patch, not necessarily in log) of one header line per notes line we should be more on the safe side. It would also allow us to keep the headers wrapped at < 78 characters which is nice for legibility on terminals. -- Giuseppe "Oblomov" Bilotta --
I do not believe you are unable to spawn open $fd, '-|' 'sh', '-c', "GIT_NOTES_REF=$note_ref git show ..." and read from it ;-). For possible enhancement to make notes easier to use, see the other response. --
You meant here my $git_command = quote_command(git_cmd(), 'show', ...); open my $fd, '-|', 'sh', '-c', "GIT_NOTES_REF=$note_ref $git_command" So you need to take care to quote parameters ($note_ref fortunately doesn't need to be quoted)... and you have one more process (shell) spawned. Therefore I think it would be nice to have --notes-ref option to git wrapper,... especially that it should be easy to set it up in such way that it would be possible to pass --notes-ref multiple times, e.g.: git --notes-ref=commits --notes-ref=git-svn show ... -- Jakub Narebski Poland --
This command automatically shows notes, even in absence of GIT_NOTES_REF environment variable or core.notesRef (if core.notesRef is not unset), so unless we want gitweb to display notes flattened into commit message (which I think was intended behavior - design decision for notes display), we would need to modify parse_commit_text to gather notes You are right that we would need it if we want to display notes from non-default namespace. Still, 1 or 2 git commands per commit is a bit too much (with shortlog displaying 100 commits per page): that is what "git cat-file --batch" was invented ;-) -- Jakub Narebski Poland --
But who said you need to display notes for all commits by default? It could be a thing that you would get _on demand_, just like patch text or diffstat is given only on demand by going to the commitdiff page. --
But while diffstat or patch text always make sense for _any_ commit, since you always have them; notes don't, in the sense that you might not have notes in most of the commits, and you might actually be looking for commits with notes. At the very least, a view like shortlog should give an indicator that a commit has notes, and possible the namespaces where its notes are (e.g., you're only browsing for bugzilla notes). Anyway, since git cat-file --batch, or --batch-check if we only want the indicator, (didn't know about these options, btw, thanks a bunch) means that we can get all the notes with a single extra call, what's the problem with displaying them? ;-) -- Giuseppe "Oblomov" Bilotta --
On Thu, 4 Feb 2010, Giuseppe Bilotta wrote: It is not obvious from this description that you can provide _list_ of How you can provide list of notes here? Is overriding limited to single name or shell-glob? First, this I think limits overriding in repository config to single value. Second, perhaps it is time to refactor all those similar feature_xxx open my $fd, '-|', git_cmd(), 'for-each-ref', '--format=%(refname)', "refs/notes/$glob" or return; Why not simply First, there are '--batch' and '--batch-check' options to git-cat-file. With these I think you can get all notes with just single git command, although using it is a bit complicated (requires open2 from IPC::Open2 for bidi communication). Second, if not using 'git cat-file --batch', perhaps it would be easier to read each $note_ref tree using 'git ls-tree'/'git ls-tree -r', and parse its output to check for which commits/objects there are notes available, and only then call 'git show' (or reuse connection to 'git cat-file --batch'). The second solution, with a bit more work, could work even in presence -- Jakub Narebski Poland --
Y'know, one would figure that, this not being my first contribution
I'm starting to think it might make sense to not have a list here, but
rather a single value only. First of all, multiple refs can be
indicated à la shell with {ref1,ref2,ref3}. Or, we can also use the
intended command-line syntax ref1:ref2:ref3, which would help
consistency. It also makes things easier for project overrides, as per
As mentioned above, I'd rather use the same syntax deployed on the
An interesting approach. Without fan-out, git ls-tree -r
refs/notes/whatever [hash ...] gives us the blobs we're interested in.
In case of fan-out schemes, the efficiency of this approach probably
depends on the kind of fan-out we have, and would require some
heavy-duty grepping. A git ls-notes plumbing with a similar syntax and
output would be a nice thing to have.
--
Giuseppe "Oblomov" Bilotta
--
So it is to be single shell-glob / fnmatch (I think) compatible pattern,
isn't it?
It would look something like the following (fragment of my WIP code):
use IPC::Open2 qw(open2);
use IO::Handle;
# ...
unless ($object_stdout) {
# Open bidi pipe the first time get_object is called.
# open2 raises an exception on error, no need to 'or die'.
$object_pid =
open2($object_stdout, $object_stdin,
git_cmd(), 'cat-file', '--batch');
}
$object_stdin->printflush("$object_id\n") # NB: \n required to avoid deadlocking
or die "get_object: cannot write to pipe: $!";
my ($sha1, $type, $size) =
split ' ', $object_stdout->getline()
or die "get_object: cannot read from pipe: $!";
die "'$object_id' not found in repository"
if $type eq 'missing';
$object_stdout->read(my $content, $size);
$object_stdout->getline(); # eat trailing newline
The above fragment of code is tested that it works. You would probably
No grepping, just pass '-r' option to 'git-ls-tree', and use
parse_ls_tree_line() to parse lines. Then if we have fanout scheme
we would get, I guess, something like the following:
100644 blob 23da787d... de/adbeef...
100644 blob bc10f25f... c5/31d986...
100644 blob c9656ece... 24/d93129...
Now you only need to s!/!!g on filename to get SHA1 of annotated object
(for which is the note).
The identifier of note itself would be either id from tree (e.g. 23da787d...
for note from first line), or note namespace composed with note "pathname",
(e.g. refs/notes/commits:de/adbeef... for note from first line).
Even if you use one git-show per note, it would need only git commands
to discover object to note mapping.
P.S. We still would want parse_commit_text to parse notes from default
namespace. parse_commit / parse_commits output contains notes from
default namespace, e.g.:
d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3 94ac0c7b30a7dc43d926b0ffbe90892c1e19e5f6
tree b9ee8876df81b80b13c6b017be993fff8427cfaf
parent ...Sort of. fnmatch doesn't do brace expansion, which is a pity IMO, but that's just my personal preference. Colon-separated, fnmatched components is probably the easiest thing to implement to have multiple On the other hand, as mentioned by Junio, this approach is not What worries me is that you're going to get fan-outs when there are LOTS of notes, and that's precisely the kind of situation where you _don't_ want to go through all the notes to pick the ones you're only interested in. If we have a guarantee that the fan-outs follow a 2/[2/...] scheme, the open2 approach might still be the best way to go, by just trying not only namespace:xxxxx...xxx but also namespace:xx/xxxxx etc. Horrible, but could still be coalesced in a single call. It mgiht also I'm not getting these in the repo I'm testing this. And I think this is indeed the behavior of current git next -- Giuseppe "Oblomov" Bilotta --
Well, fnmatch is what I think git uses for <pattern> e.g. for
I think that having actual list of patterns in $feature{'notes'}{'default'}
might be more clear; you would still need colon separated (or space
separated) list of patterns in per-repo override in gitweb.notes config
variable.
So it would be
$feature{'notes'}{'default'} = ['commits', '*svn*'];
$feature{'notes'}{'override'} = 1;
but
[gitweb]
notes = commits:*svn*
Note that refs names cannot contain either colon ':' or space ' '
On the third hand ;-P you propose below a trick to deal with fan-out
schemes, assuming that they use 2-character component breaking.
Also, perhaps "git notes show" should acquire --batch / --batch-check
Right. This method would be contrary to the goals of fan-out schemes...
well, we could use 'git ls-tree' without '-r' option, or simply
'git cat-file --batch' to read trees (note that we would get raw,
unformatted tree, which is parseable with Perl, but it is not that easy),
Nice trick! It seems like quite a good idea... but it would absolutely
Errr, I not made myself clear.
I have added a note to a commit, using "git notes edit d6bbe7f". Now if
you take a look at gitweb output for this commit (e.g. 'commit' view for
this commit) using gitweb without your changes, you would see that it
flattened notes at the bottom of the commit message (which I think is
intended result by notes implementation).
If you run the command that parse_commit runs, namely
$ git rev-list --parents --header -z --max-count=1 \
d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3
you would get (up to invisible NUL characters) the output shown above.
From this output I would like to separate commit message from notes
in parse_commit_text subroutine.
I have set neither GIT_NOTES_REF nor core.notesRef.
--
Jakub Narebski
Poland
--
FYI, here is how you can parse raw tree output from 'git cat-file --batch',
assuming that you have plain-ASCII filenames ('use bytes;' would probably
be needed):
-- 8< --
sub decode_tree {
my $contents = shift;
# ...
while (my @entry = decode_tree_entry($contents)) {
# ...
my $len = tree_entry_len(@entry);
contents = substr($contents, $len);
last unless $contents;
}
# ...
}
sub tree_entry_len {
my ($mode_str, $filename) = @_;
# length of mode string + separator + 20 bytes of SHA-1
# + length of filename (in bytes) + terminating NUL ('\0')
length($mode_str)+1 + length($filename)+1 + 20;
}
sub decode_tree_entry {
my $buf = shift;
$buf =~ s/^([0-7]+) //;
my ($mode_str) = $1;
my ($filename, $sha1_str) = unpack('Z*H[40]', $buf);
return ($mode_str, $filename, $sha1_str);
}
-- >8 --
--
Jakub Narebski
Poland
--
The current notes code (as it stands in 'pu') use only 2-character component
breaking, and I don't see any other fanout mechanism being added anytime
I'd much rather have support for ^{notes} (or similar) in the rev-parse
machinery, so that you could look up deadbeef's notes by passing
IMHO, it's much better/nicer to re-use the notes code for parsing note
Still, I'd still much rather use the notes.c code itself for doing this
since it should always be the fastest (not to mention future-proof) way of
making lookups in the notes tree.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
--
On one hand checking <notes-ref>:403f3b3e..., then <notes-ref>:40/3f3b3e...,
then <notes-ref>:40/3f/3b3e... etc. feels a bit kludgy...
On the other hand it would allow to support notes in gitweb even if git
binary does not have notes support (and yet notes did get somehow into
+1. Good idea!
The only caveat is that if/when we support either of:
* allowing to specify multiple notes namespaces (e.g. multiple --notes-ref
or --notes option, or PATH-like GIT_NOTES_REF with colon-separated list
of multiple notes namespaces)
* allowing to have multiple notes per object ('tree' notes)
then <commit-ish>^{notes} would mean multiple objects. But it is not
much different from supported <commit-ish>^! and <commit-ish>^@ syntax.
One of Giuseppe goals seems to be to support notes in gitweb even if used
git binary doesn't have support for notes (git-notes command, ^{notes}
extended SHA1 syntax).
--
Jakub Narebski
Poland
--
Maybe something like deadbeef@{notes[:namespace]}? The ability to
embed the notes namespace to use in the call is very useful to be able
I agree with you on this, btw. As I mentioned in the other message,
these would just be workarounds for the current lack of support of
these features in core. I'd probably try and have a go at the thing
myself, too (i.e. first implement the core functionality, and then use
it in gitweb), but I honestly don't feel confident enough to hack at
git core.
--
Giuseppe "Oblomov" Bilotta
--
That is just bikeshedding, but I'd rather not use '@', which currently
is used only for _reflog_ based revision specifiers: [<ref>]@{<date>},
[<ref>]@{<n>}, @{-<n>}, for notes which are not reflog based.
We can use
echo <commit>^{notes} | git --notes-ref=<namespace> cat-file --batch
or perhaps
echo <commit>^{notes:<namespace>} | git cat-file --batch
--
Jakub Narebski
Poland
--
Probably a nicer way to say the same thing is to avoid "reflog based"
which sounds like you are talking about an implementation detail.
A fundamental reason to favor your "bikeshedding" (I don't think it is a
bikeshedding---it is a sound argument against using "@{...}") is that the
at-brace notation applies to a ref, not to an arbitrary commit. Applying
@{yesterday} to an arbitrary commit does not make any sense.
Notes are fundamenally metainformation about an _object_ [*1*] and are not
metainformation about refs. Since whatever magic notation to denote notes
we choose wants to be applied to an arbitrary commit, it shouldn't be the
at-brace syntax.
[Footnote]
*1* Yes, I am aware of movements to misuse notes to annotate anything
after mapping it to a random SHA-1 value, but I think that is outside the
scope of notes. Our design decision should be based on supporting the
primary use of annotating an object, and that might still keep such a use
working, in which case that would be an added bonus. But our design
shouldn't be constrained by such a secondary use.
--
Makes sense. ^{note[:namespace]} is ok for me too btw, although maybe
it looks a little off-base when compared with the tag indicator ^{}
BTW, I still think that notes should be attachable to named refs (not
SHA-1, thus) too.
--
Giuseppe "Oblomov" Bilotta
--
Well, notes refer to objects (commits), but the whole idea of notes
was to have easy mapping in the reverse direction, from object to
its annotations.
We could invent yet another syntax, e.g. ^@{} or ^@{<namespace>}
I have just realized that it is totally no-go. Why? Because names of
refs are local to repository: what is one 'master' might be other 'origin';
what is one 'for-linus' might be other 'from-alan', what's one
'refs/heads/next' might be other 'refs/remotes/origin/next'.
Also I think that there would be problem with renaming and deleting refs.
--
Jakub Narebski
Poland
--
fnmatch is the function to use to match a single component. The
question is how to group components together in a single spec (aside
from shell globs and []); my personal choice would be brace expansion,
colon-separated was suggested by (IIRC) Junio. Of course, for gitweb
That makes sense. Colon-separated values will probably be allowed in
$feature{'notes'} too, which will keep the code more consistent.
There is little doubt that batch note processing should be available
in core, be it with git notes show --batch(-check), git ls-notes, or
both. I also agree with Johan that gitweb should use these mechanisms
as soon as they are available. The approaches we're discussing here
are basically a temporary workaround for the missing core support.
Oh, that's a given. git-show was just the only approach I could think
And that's what I don't get. git version 1.7.0.rc1.193.ge8618. If I
remember correctly, the behaviour of automatically displaying notes in
git log & friends was changed recently. So git log -1
e8618c52b5f815624251f048609744c9558d92a1 gives me the notes I put
there for testing, git rev-list --parents --header -z --max-count=1
does not.
--
Giuseppe "Oblomov" Bilotta
--
Ah, sorry. Git version 1.6.6.1 here. -- Jakub Narebski Poland --
The notes are shown in full to the left of the log message.
---
gitweb/gitweb.css | 11 +++++++++++
gitweb/gitweb.perl | 11 +++++++----
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 7d1836b..81d66d3 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -601,3 +601,14 @@ span.notes span.note-container:hover span.note {
z-index:10;
overflow:visible;
}
+
+div.notes {
+ max-width:150px;
+ float:left;
+}
+
+div.notes div.note {
+ background-color:#ffffad;
+ border:1px solid #c9bb83;
+ padding:4px;margin:0;
+}
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1701ed1..0d0877e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1631,6 +1631,7 @@ sub format_subject_html {
# display notes next to a commit
sub format_notes_html {
my %notes = %{$_[0]};
+ my $tag = $_[1] || 'span' ;
my $ret = "";
while (my ($ref, $text) = each %notes) {
# remove 'refs/notes/' and an optional final s
@@ -1639,15 +1640,15 @@ sub format_notes_html {
# double markup is needed to allow pure CSS cross-browser 'popup'
# of the note
- $ret .= "<span title='$ref' class='note-container $ref'>";
- $ret .= "<span title='$ref' class='note $ref'>";
+ $ret .= "<$tag title='$ref' class='note-container $ref'>";
+ $ret .= "<$tag title='$ref' class='note $ref'>";
foreach my $line (split /\n/, $text) {
$ret .= esc_html($line) . "<br/>";
}
- $ret .= "</span></span>";
+ $ret .= "</$tag></$tag>";
}
if ($ret) {
- return "<span class='notes'>$ret</span>";
+ return "<$tag class='notes'>$ret</$tag>";
} else {
return $ret;
}
@@ -4581,6 +4582,7 @@ sub git_log_body {
next if !%co;
my $commit = $co{'id'};
my $ref = format_ref_marker($refs, $commit);
+ my $notes = format_notes_html($co{'notes'}, 'div');
my %ad = parse_date($co{'author_epoch'});
git_print_header_div('commit',
"<span class=\"age\">$co{'age_string'}</span>" .
@@ ...Thats all good if you have wide (high resolution) screen, and your project follows common commit message conventions of keeping lines in commit message no longer than at most 80 characters, and you don't need to use large size fonts. What happens if screen size is too small to contain both commit message and notes? Does it do the sensible thing of putting notes _below_ commit message in such situation? I do not know CSS+HTML enogh to answer this question myself. BTW. signoff? P.S. We would probably want some support for notes also in feeds (Atom This could be my $notes = shift; my $tag = shift || 'span' ; With respect to the question about what happens if the screen is not wide enough, shouldn't notes be put in HTML source below body (commit message)? -- Jakub Narebski Poland --
The CSS forces the width of the notes div at 150px, which is the amount left to the left of the commit message. This means that notes I think so. The distinction is useful both from the structural point of view (block elements with block elements, inline elements with inline elements) and for CSS selection (the block case has totally As I mentioned, notes width is fixed at the amount of the whitespace to the left of the log, so this should not be an issue. Additionally, putting notes below makes it much harder to let them float to the left of the log body. -- Giuseppe "Oblomov" Bilotta --
Perhaps the log body should be floated to the right, instead of notes being floated to the left, so that when screen width is to narrow for both commit message and notes, notes would be put below commit message. A question how to create styles using HTML elements and CSS styling to get side-by-side with one below other as fallback can be asked on http://stackoverflow.com, or perhaps even better on http://doctype.com/ -- Jakub Narebski Poland --
The presence of the note is shown by a small icon, hovering on which
reveals the actual note content.
---
gitweb/gitweb.css | 29 +++++++++++++++++++++++++++++
gitweb/gitweb.perl | 30 +++++++++++++++++++++++++++++-
2 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 50067f2..7d1836b 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -572,3 +572,32 @@ span.match {
div.binary {
font-style: italic;
}
+
+span.notes {
+ float:right;
+ position:relative;
+}
+
+span.notes span.note-container:before {
+ content: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAgAAAAIAgMAAAC5YVYYAAAAAXNSR0IArs4c6QAAAAlQTFRFAABnybuD//+t5rXizQAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAAGElEQVQI12MIDWXIWglDQHYIQ1YAQ6gDAFWPBrAKFe0fAAAAAElFTkSuQmCC');
+}
+
+span.notes span.note {
+ display:none;
+}
+
+span.notes span.note-container img {
+ content: normal;
+}
+
+span.notes span.note-container:hover span.note {
+ display:block;
+ content:normal;
+ background-color:#ffffad;
+ border:1px solid #c9bb83;
+ padding:4px;margin:0;
+ position:absolute;
+ top:0;right:0;
+ z-index:10;
+ overflow:visible;
+}
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9ba5815..1701ed1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1628,6 +1628,33 @@ sub format_subject_html {
}
}
+# display notes next to a commit
+sub format_notes_html {
+ my %notes = %{$_[0]};
+ my $ret = "";
+ while (my ($ref, $text) = each %notes) {
+ # remove 'refs/notes/' and an optional final s
+ $ref =~ s/^refs\/notes\///;
+ $ref =~ s/s$//;
+
+ # double markup is needed to allow pure CSS cross-browser 'popup'
+ # of the note
+ $ret .= "<span title='$ref' class='note-container $ref'>";
+ $ret .= "<span title='$ref' class='note $ref'>";
+ foreach my $line (split /\n/, $text) {
+ $ret .= esc_html($line) . "<br/>";
+ }
+ $ret .= "</span></span>";
+ }
+ if ($ret) {
+ return "<span class='notes'>$ret</span>";
+ } ...Is it RFC?
Why it is only for 'shortlog' view, and not also for 'history' which is
also shortlog-like view? Or is there reason why it is not present for
Not all web browsers support ':before' pseudo-element, and 'content'
(pseudo-)property.
Not all web browsers support 'data:' URI schema in CSS; also such image
cannot be cached (on the other hand it doesn't require extra TCP
connection on first access, and CSS file is cached anyway).
On the other hand adding extra images to gitweb would probably require
additional (yet another) build time parameter to tell where static
images are (besides logo and favicon).
So perhaps it is good solution, at least for a first attempt.
Perhaps $return or $result would be a better name, to better distinguish
You can use different delimiter than / to avoid 'leaning toothpick'
Probably would want
$ret .= esc_html($line) . "<br/>\n";
here. Or do we want single string here?
Also, do you want/need final <br>? If not, perhaps
join("<br/>", map { esc_html($_) } split(/\n/, $text);
Nice.
--
Jakub Narebski
Poland
--
I know it's neither good form nor good webdesigner attitude, but I stopped caring about IE a long time ago. I understand however that some ancient versions of Mozilla browsers might have the same issue A possible alternative could maybe do without images and just use borders and backgrounds of an 8x8 fixed-size element. Wouldn't look as nice, probably, but should render decently in everything that supports No particular reason. I didn't check for syntax preferences regarding this in gitweb, or I would have noticed there was a preference for the It's within a span element so I was trying to stick to single line in I did notice that the final br didn't seem to affect the box height, so I didn't bother looking at ways to do without it, but it's probably nicer to not have it. -- Giuseppe "Oblomov" Bilotta --
The notes are shown side-by-side along the bottom of the commit message.
---
gitweb/gitweb.css | 11 +++++++++++
gitweb/gitweb.perl | 21 +++++++++++++++++++++
2 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 81d66d3..10acab4 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -145,6 +145,7 @@ div.list_head {
border: solid #d9d8d1;
border-width: 1px 0px 0px;
font-style: italic;
+ clear: both;
}
.author_date, .author {
@@ -612,3 +613,13 @@ div.notes div.note {
border:1px solid #c9bb83;
padding:4px;margin:0;
}
+
+
+div.page_body div.notes {
+ max-width:100%;
+ float:none;
+}
+
+div.page_body div.notes div.note {
+ float:left;
+}
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0d0877e..0d03026 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2837,12 +2837,31 @@ sub parse_commit {
%co = parse_commit_text(<$fd>, 1);
close $fd;
+ my %notes = ();
+ foreach my $note_ref (get_note_refs()) {
+ my $obj = "$note_ref:$co{'id'}";
+ if (open my $fd, '-|', git_cmd(), 'rev-parse',
+ '--verify', '-q', $obj) {
+ my $exists = <$fd>;
+ close $fd;
+ if (defined $exists) {
+ if (open $fd, '-|', git_cmd(), 'show', $obj) {
+ $notes{$note_ref} = scalar <$fd>;
+ close $fd;
+ }
+ }
+ }
+ }
+ $co{'notes'} = \%notes;
+
return %co;
}
# return all refs matching refs/notes/<globspecs> where the globspecs
# are taken from the notes feature content.
sub get_note_refs {
+ local $/ = "";
+
my @globs = gitweb_get_feature('notes');
my @note_refs = ();
foreach my $glob (@globs) {
@@ -5875,6 +5894,7 @@ sub git_commit {
print "<div class=\"page_body\">\n";
git_print_log($co{'comment'});
+ print format_notes_html($co{'notes'}, 'div');
print "</div>\n";
git_difftree_body(\@difftree, $hash, @$parents);
@@ -6230,6 +6250,7 @@ sub git_commitdiff {
git_print_log($co{'comment'}, -final_empty_line=> 1, ...The same question apply as for previous commit. What happens if screen size is too small to contain both commit message and notes? Does it do the sensible thing of putting notes _below_ commit message in such situation? I do not know CSS+HTML enogh to Duplicated code. Please put this code in a separate subroutine, to be Why it is needed here? Why you want to use empty lines as terminator (which means reading whole paragraphs), while treating two or more consecutive empty lines as a single empty line (according to perlvar(1))? If you want to slurp whole file, this should be local $/; or more explicit This of course assumes that we want notes treated exactly (or almost exactly) the same way for 'log', 'commit' and 'commitdiff' views. Perhaps it is a good assumption (at least for first step)... -- Jakub Narebski Poland --
In this view the notes are printed side-by-side to each other, but at Ah, sorry, for some reason I thought "" was the default. -- Giuseppe "Oblomov" Bilotta --
If you wanted to use default value, why set it at all? -- Jakub Narebski Poland --
Ach, sorry, forgot to reply to the first part of the question. It's used in a context where $/ is locally set to \0, so it needs to be reset. -- Giuseppe "Oblomov" Bilotta --
Oh, so it should be something like the following, then?
sub get_note_refs {
+ # reset to default value (can be called with $/ set to "\0")
--
Jakub Narebski
Poland
--
Yes, it's probably worth to mention it in a comment. -- Giuseppe "Oblomov" Bilotta --
