On Tuesday 23 February 2010 00:20:06 Junio C Hamano wrote:
> Thomas Rast <trast@xxxxxxxxxxxxxxx> writes:
> > diff --git a/notes.c b/notes.c
[...]
> > +struct string_list display_notes_refs;
> > +struct notes_tree **display_notes_trees;
>
> Do these need to be extern?
Don't think so? After all it's in the .c and just another static
variable.
> Nice use of strbuf_split(). I wonder if we should automatically add
> "refs/" and/or "refs/notes/" when the input is missing the prefix. I
> don't have strong preference myself but the users might make noises.
>
> Either way it needs to be documented in the final version before the
> series goes to 'master'.
I decided to do such dwimmery only on the command line, but not for
the config. This is consistent with what's currently happening for
core.notesRef. I also documented this.
> > +static int notes_display_config(const char *k, const char *v, void *cb)
> > +{
> > + /* Warning! This is currently not executed if
> > + * GIT_NOTES_DISPLAY_REF is set. Move the git_config() call
> > + * outside the test if you add more options. */
>
> Yuck. If you know what needs to be done, do that before other poeple add
> more options, please.
*shrug*
I was trying to be smart and save a call to git_config() when we know
we don't care about the config anyway. After all it does read a bunch
of files.
> >> > + if (flags & NOTES_SHOW_HEADER_WITH_REF && t->ref) {
[...]
> >> > + } else if (flags & (NOTES_SHOW_HEADER|NOTES_SHOW_HEADER_WITH_REF))
> >> > strbuf_addstr(sb, "\nNotes:\n");
>
> Meaning nobody will go through the latter "Notes:\n" codepath? Then what
> is that else clause for?
>
> Perhaps I am not reading your code right in which case this part needs a
> bit more commenting?
The catch is in the '&& t->ref'. Again I'm just trying to be a bit
defensive. Anyway, I ripped out the old formatting and put the NULL
test first this time.
> >> I expected to see "Notes:\n" regardless of the mode if the notes is coming
> >> from the default refs/notes/commits tree, but it probably is better to say
> >> "Notes (commits):\n" like your patch does.
> >
> > I special-cased GIT_NOTES_DEFAULT_REF (which is "refs/notes/commits")
> > above *at your request* to not change the output in the default case.
>
> > So which way do you want it?
>
> I don't have strong preference anymore with the above code.
[...]
> But if everybody calls with HEADER_WITH_REF, no matter what the end user
> preference is, then it becomes unclear what the right answer would be.
Well, I think we're into bikeshedding territory now, so I just left it
as it was.
The rest of the changes are listed in each email.
Thomas Rast (11):
test-lib: unset GIT_NOTES_REF to stop it from influencing tests
Support showing notes from more than one notes tree
Documentation: document post-rewrite hook
commit --amend: invoke post-rewrite hook
rebase: invoke post-rewrite hook
rebase -i: invoke post-rewrite hook
notes: implement 'git notes copy --stdin'
notes: implement helpers needed for note copying during rewrite
rebase: support automatic notes copying
commit --amend: copy notes to the new commit
notes: add shorthand --ref to override GIT_NOTES_REF
Documentation/config.txt | 53 +++++-
Documentation/git-notes.txt | 21 ++-
Documentation/githooks.txt | 38 ++++
Documentation/pretty-options.txt | 11 +-
builtin-commit.c | 45 +++++
builtin-log.c | 2 +
builtin-notes.c | 195 +++++++++++++++++++-
builtin.h | 18 ++
cache.h | 3 +
git-am.sh | 13 ++
git-rebase--interactive.sh | 52 +++++-
git-rebase.sh | 6 +
notes.c | 191 +++++++++++++++++++-
notes.h | 27 +++
pretty.c | 6 +-
refs.c | 4 +-
refs.h | 5 +
revision.c | 21 ++
revision.h | 5 +
t/t3301-notes.sh | 377 +++++++++++++++++++++++++++++++++++++-
t/t3400-rebase.sh | 17 ++
t/t3404-rebase-interactive.sh | 24 +++
t/t5407-post-rewrite-hook.sh | 172 +++++++++++++++++
t/t7501-commit.sh | 12 ++
t/test-lib.sh | 4 +
25 files changed, 1294 insertions(+), 28 deletions(-)
create mode 100755 t/t5407-post-rewrite-hook.sh
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html