[PATCH] revision.c: introduce --notes-ref= to use one notes ref only

As notes become increasingly popular, it's often interesting to show
notes from a particular notes ref only. Introduce '--notes-ref=<ref>'
as a convenience shortcut for '--no-standard-notes --show-notes=<ref>'.

Signed-off-by: Michael J Gruber <redacted>
---
The idea is to use the same name as in "git notes --ref=<ref>" but make
it clear for the rev-list option to be about notes, thus "--notes-ref=<ref>".

 Documentation/git-log.txt        |    3 ++-
 Documentation/pretty-options.txt |    4 ++++
 revision.c                       |   15 +++++++++++----
 t/t3301-notes.sh                 |    5 +++++
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2c84028..56ffccd 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt

@@ -179,7 +179,8 @@ multiple times.  A warning will be issued for refs that do not exist,
 but a glob that does not match any refs is silently ignored.
 +
 This setting can be disabled by the `--no-standard-notes` option,
-overridden by the 'GIT_NOTES_DISPLAY_REF' environment variable,
+overridden by the 'GIT_NOTES_DISPLAY_REF' environment variable
+or the `--notes-ref` option,
 and supplemented by the `--show-notes` option.
 
 GIT

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 50923e2..ef5eed4 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt

@@ -46,3 +46,7 @@ is taken to be in `refs/notes/` if it is not qualified.
 	'core.notesRef' and 'notes.displayRef' variables (or
 	corresponding environment overrides).  Enabled by default.
 	See linkgit:git-config[1].
+
+--notes-ref[=<ref>]::
+	This is the same as `--no-standard-notes --show-notes=<ref>`,
+	i.e. it shows only the notes from the notes tree at `<ref>`.

diff --git a/revision.c b/revision.c
index 0f38364..d620926 100644
--- a/revision.c
+++ b/revision.c

@@ -1368,19 +1368,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--show-notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-	} else if (!prefixcmp(arg, "--show-notes=")) {
+	} else if (!prefixcmp(arg, "--show-notes=") || !prefixcmp(arg, "--notes-ref=")) {
 		struct strbuf buf = STRBUF_INIT;
+		int offset = strlen("--show-notes=");
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
+		if (!prefixcmp(arg, "--notes-ref=")) {
+			offset = strlen("--notes-ref=");
+			if (revs->notes_opt.extra_notes_refs)
+				string_list_clear(revs->notes_opt.extra_notes_refs, 0);
+			revs->notes_opt.suppress_default_notes = 1;
+		}
 		if (!revs->notes_opt.extra_notes_refs)
 			revs->notes_opt.extra_notes_refs = xcalloc(1, sizeof(struct string_list));
-		if (!prefixcmp(arg+13, "refs/"))
+		if (!prefixcmp(arg+offset, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(arg+13, "notes/"))
+		else if (!prefixcmp(arg+offset, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, arg+13);
+		strbuf_addstr(&buf, arg+offset);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
 	} else if (!strcmp(arg, "--no-notes")) {

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1921ca3..3fcfdc7 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh

@@ -625,6 +625,11 @@ test_expect_success '--show-notes=ref accumulates' '
 	test_cmp expect-both-reversed output
 '
 
+test_expect_success '--notes-ref=' '
+	git log --notes-ref=other -1 > output &&
+	test_cmp expect-other output
+'
+
 test_expect_success 'Allow notes on non-commits (trees, blobs, tags)' '
 	git config core.notesRef refs/notes/other &&
 	echo "Note on a tree" > expect &&

-- 
1.7.4.1.607.g888da

On Tuesday 29 March 2011, Michael J Gruber wrote:

As notes become increasingly popular, it's often interesting to show
notes from a particular notes ref only. Introduce '--notes-ref=<ref>'
as a convenience shortcut for '--no-standard-notes
--show-notes=<ref>'.

Signed-off-by: Michael J Gruber <redacted>
---
The idea is to use the same name as in "git notes --ref=<ref>" but
make it clear for the rev-list option to be about notes, thus
"--notes-ref=<ref>".

The idea and implementation look good to me. Not sure I like the 
option "bloat" (somehow feels it should be possible to express the same 
behavior using fewer options), but if there's not a better way to 
reorganize the options, then you can consider it Acked-by me.


Thanks! :)

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Tue, Mar 29, 2011 at 02:39:17PM +0200, Johan Herland wrote:

On Tuesday 29 March 2011, Michael J Gruber wrote:

quoted

As notes become increasingly popular, it's often interesting to show
notes from a particular notes ref only. Introduce '--notes-ref=<ref>'
as a convenience shortcut for '--no-standard-notes
--show-notes=<ref>'.

Signed-off-by: Michael J Gruber <redacted>
---
The idea is to use the same name as in "git notes --ref=<ref>" but
make it clear for the rev-list option to be about notes, thus
"--notes-ref=<ref>".

The idea and implementation look good to me. Not sure I like the 
option "bloat" (somehow feels it should be possible to express the same 
behavior using fewer options), but if there's not a better way to 
reorganize the options, then you can consider it Acked-by me.

I feel this would be more consistent with most other options that take
an optional argument:

  1. "--show-notes" uses default refs

  2. "--show-notes=<ref>" shows _just_ <ref>, no defaults

  3. "--show-notes=<ref1> --show-notes=<ref2>" shows <ref1> and <ref2>

  4. (Probably) "--show-notes --show-notes=<ref>" should show default
     refs and <ref>. This is the one I'm least sure of, as it leaves no
     way to override what came earlier on the command line (which is
     useful if, for example, we end up with Michael's proposed ui.log).
     Perhaps "--no-notes" would reset, so:

       --show-notes --no-notes --show-notes=<ref>

     would be equivalent to:

       --show-notes=<ref>

Of course a total behavior change of what --show-notes currently does.

Speaking of which, it is kind of weird that --show-notes is negated by
--no-notes. So maybe it makes sense to introduce "--notes[=<ref>]" to do
what I wrote above, and deprecate --show-notes.

-Peff

On Tue, Mar 29, 2011 at 12:05:09PM +0200, Michael J Gruber wrote:

This issue is not introduced by your patch, but maybe it is a good
opportunity to refactor this to use expand_notes_ref from notes.c?

-Peff

Jeff King venit, vidit, dixit 29.03.2011 16:33:

On Tue, Mar 29, 2011 at 02:39:17PM +0200, Johan Herland wrote:

quoted

On Tuesday 29 March 2011, Michael J Gruber wrote:

quoted

As notes become increasingly popular, it's often interesting to show
notes from a particular notes ref only. Introduce '--notes-ref=<ref>'
as a convenience shortcut for '--no-standard-notes
--show-notes=<ref>'.

Signed-off-by: Michael J Gruber <redacted>
---
The idea is to use the same name as in "git notes --ref=<ref>" but
make it clear for the rev-list option to be about notes, thus
"--notes-ref=<ref>".

The idea and implementation look good to me. Not sure I like the 
option "bloat" (somehow feels it should be possible to express the same 
behavior using fewer options), but if there's not a better way to 
reorganize the options, then you can consider it Acked-by me.

I feel this would be more consistent with most other options that take
an optional argument:

  1. "--show-notes" uses default refs

  2. "--show-notes=<ref>" shows _just_ <ref>, no defaults

  3. "--show-notes=<ref1> --show-notes=<ref2>" shows <ref1> and <ref2>

  4. (Probably) "--show-notes --show-notes=<ref>" should show default
     refs and <ref>. This is the one I'm least sure of, as it leaves no
     way to override what came earlier on the command line (which is
     useful if, for example, we end up with Michael's proposed ui.log).

My "git log" shows notes from ref/notes/commits by default without alias
or config, and that is what I want to override per command (to show
Thomas' notes, e.g.).

     Perhaps "--no-notes" would reset, so:

       --show-notes --no-notes --show-notes=<ref>

     would be equivalent to:

       --show-notes=<ref>

Of course a total behavior change of what --show-notes currently does.

I somehow stopped proposing behavior changes. Guess why? (I know I have
my occasional relapse, but still...)

Speaking of which, it is kind of weird that --show-notes is negated by
--no-notes. So maybe it makes sense to introduce "--notes[=<ref>]" to do
what I wrote above, and deprecate --show-notes.

Also, "git notes" has "--ref". Maybe this (which may be what you
proposed above):

--notes: show standard notes
--notes=<ref>: show notes from <ref> only
--notes --notes=<ref>: show standard notes + those from <ref>
(i.e., if any notes argument was given they accumulate; a single
argument does not add to, but replaces the default)
--no-notes: you guess it

One could deprecate --[no-]stand-notes as well, then.

Changing status "PATCH" back to "PATCH/RFC"...

Michael

On Tue, Mar 29, 2011 at 10:35:47AM -0400, Jeff King wrote:

On Tue, Mar 29, 2011 at 12:05:09PM +0200, Michael J Gruber wrote:

quoted

-		if (!prefixcmp(arg+13, "refs/"))
+		if (!prefixcmp(arg+offset, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(arg+13, "notes/"))
+		else if (!prefixcmp(arg+offset, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, arg+13);
+		strbuf_addstr(&buf, arg+offset);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));

This issue is not introduced by your patch, but maybe it is a good
opportunity to refactor this to use expand_notes_ref from notes.c?

Oops, I just realized this is in builtin/notes.c in master. I had
already written a patch for another topic that made it globally
accessible. :)

-Peff

Jeff King venit, vidit, dixit 29.03.2011 21:01:

On Tue, Mar 29, 2011 at 10:35:47AM -0400, Jeff King wrote:

quoted

On Tue, Mar 29, 2011 at 12:05:09PM +0200, Michael J Gruber wrote:

quoted

-		if (!prefixcmp(arg+13, "refs/"))
+		if (!prefixcmp(arg+offset, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(arg+13, "notes/"))
+		else if (!prefixcmp(arg+offset, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, arg+13);
+		strbuf_addstr(&buf, arg+offset);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));

This issue is not introduced by your patch, but maybe it is a good
opportunity to refactor this to use expand_notes_ref from notes.c?

Oops, I just realized this is in builtin/notes.c in master. I had
already written a patch for another topic that made it globally
accessible. :)

Yeah, I (figured and) factored it out myself meanwhile, and rebased. I'm
wondering though where we are going. Junio seems to be in a mood for
major changes to the notes ui, so maybe I should hold on until we
decided about a ui restructuring.

I think, though, that any notes ui revamp is correlated with our
(stalled?) discussions about the layout of refs/. It affects not only
the default notes ref ("commits" for all notes?) but also the question
what a standard notes ref is, and where to store (and how to specify)
upstream notes refs.

Michael

On Tue, Mar 29, 2011 at 09:48:34PM +0200, Michael J Gruber wrote:

quoted

quoted

This issue is not introduced by your patch, but maybe it is a good
opportunity to refactor this to use expand_notes_ref from notes.c?

Oops, I just realized this is in builtin/notes.c in master. I had
already written a patch for another topic that made it globally
accessible. :)

Yeah, I (figured and) factored it out myself meanwhile, and rebased. I'm
wondering though where we are going. Junio seems to be in a mood for
major changes to the notes ui, so maybe I should hold on until we
decided about a ui restructuring.

I have a series I'll send in a few minutes. It _would_ be a lot cleaner
if we just dropped --show-notes and company entirely, but I think that
is perhaps too aggressive, even for such a young feature.

I think, though, that any notes ui revamp is correlated with our
(stalled?) discussions about the layout of refs/. It affects not only
the default notes ref ("commits" for all notes?) but also the question
what a standard notes ref is, and where to store (and how to specify)
upstream notes refs.

Yeah, I think those are open questions. But we can probably get away
with at least this option refactoring without having to answer them.

-Peff