threads / patch / 40406

patchnotes: document behavior of --ref and --notes DWIMery

Subject: [PATCH] notes: document behavior of --ref and --notes DWIMery

## tl;dr

5 messages between Sep 22, 2015 and Sep 22, 2015. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jacob Keller· Sep 22, 2015, 20:24 UTC · lore
From: Jacob Keller <jacob.keller@gmail.com>

The --notes and --ref parameter for selecting which notes ref to operate on are based off of expand_notes_ref functionality. The documentation mentioned that an unqualified ref argument would be taken as under `refs/notes/`. However, this does not clearly indicate that `refs/heads/master` will expand to `refs/notes/refs/heads/master`, so document this behavior.

Add a further test for the expected behavior of git notes --ref refs/heads/master get-ref as well, to ensure future patches do not break this assumption.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-notes.txt      | 4 +++-
 Documentation/pretty-options.txt | 5 +++--
 t/t3301-notes.sh                 | 6 ++++++
 3 files changed, 12 insertions(+), 3 deletions(-)
Show changes to 3 files +12 −3

Documentation/git-notes.txt, Documentation/pretty-options.txt, t/t3301-notes.sh

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index a9a916f360ec..2ed9c16a1aac 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -162,7 +162,9 @@ OPTIONS
 --ref <ref>::
 	Manipulate the notes tree in <ref>.  This overrides
 	'GIT_NOTES_REF' and the "core.notesRef" configuration.  The ref
-	is taken to be in `refs/notes/` if it is not qualified.
+	is taken to be in `refs/notes/` even if it is qualified under some
+	other location; in other words, `refs/heads/master` will be expanded
+	to `refs/notes/refs/heads/master`.
 
 --ignore-missing::
 	Do not consider it an error to request removing notes from an
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 8d6c5cec4c5e..abcb787e7149 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -55,8 +55,9 @@ By default, the notes shown are from the notes refs listed in the
 environment overrides). See linkgit:git-config[1] for more details.
 +
 With an optional '<ref>' argument, show this notes ref instead of the
-default notes ref(s). The ref is taken to be in `refs/notes/` if it
-is not qualified.
+default notes ref(s). The ref is taken to be in `refs/notes/` even if it is
+qualified under some other location; in other words, `refs/heads/master`
+will be expanded to `refs/notes/refs/heads/master`.
 +
 Multiple --notes options can be combined to control which notes are
 being displayed. Examples: "--notes=foo" will show only notes from
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 06b45847c147..2d200fdf36c6 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1132,6 +1132,12 @@ test_expect_success 'git notes copy diagnoses too many or too few parameters' '
 	test_must_fail git notes copy one two three
 '
 
+test_expect_success 'git notes get-ref expands refs/heads/master to refs/notes/refs/heads/master' '
+	test_unconfig core.notesRef &&
+	sane_unset GIT_NOTES_REF &&
+	test "$(git notes --ref=refs/heads/master get-ref)" = "refs/notes/refs/heads/master"
+'
+
 test_expect_success 'git notes get-ref (no overrides)' '
 	test_unconfig core.notesRef &&
 	sane_unset GIT_NOTES_REF &&
-- 
2.6.0.rc3.238.gc07a1e8
Junio C Hamano· Sep 22, 2015, 20:40 UTC · re: Jacob Keller · lore

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 15 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
>
> The --notes and --ref parameter for selecting which notes ref to operate
> on are based off of expand_notes_ref functionality. The documentation
> mentioned that an unqualified ref argument would be taken as under
> `refs/notes/`. However, this does not clearly indicate that
> `refs/heads/master` will expand to `refs/notes/refs/heads/master`, so
> document this behavior.
>
> Add a further test for the expected behavior of git notes --ref
> refs/heads/master get-ref as well, to ensure future patches do not break
> this assumption.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

Looks OK to a cursory read, but I find "even if it is qualified under some other location" a bit tiring to read without adding much value. To readers who consider "other" in that phrase to be clear enough (i.e. "location other than refs/notes"), it is totally redundant. To other readers who feel "other" in that phrase to be under qualified (i.e. "location other than what???"), it is not informative enough. Middle-ground readers who would not know if "refs/a" is inside or outside some "other" location, it is confusing.

After all, "a/b" is qualified under some location (i.e. a/) other than "refs/notes/", and it does mean "refs/notes/a/b".

How about phrasing it totally differently?
	The ref specifies the full refname when it begins with
	`refs/notes/`; otherwise `ref/notes/` is prefixed to form a
	full name of the ref.

I think that would remove the need to illustrate with concrete examples like refs/heads/blah.

Keller, Jacob E· Sep 22, 2015, 21:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

On Tue, 2015-09-22 at 13:40 -0700, Junio C Hamano wrote:
Show 43 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
> > From: Jacob Keller <jacob.keller@gmail.com>
> > 
> > The --notes and --ref parameter for selecting which notes ref to
> > operate
> > on are based off of expand_notes_ref functionality. The
> > documentation
> > mentioned that an unqualified ref argument would be taken as under
> > `refs/notes/`. However, this does not clearly indicate that
> > `refs/heads/master` will expand to `refs/notes/refs/heads/master`,
> > so
> > document this behavior.
> > 
> > Add a further test for the expected behavior of git notes --ref
> > refs/heads/master get-ref as well, to ensure future patches do not
> > break
> > this assumption.
> > 
> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> > ---
> 
> Looks OK to a cursory read, but I find "even if it is qualified
> under some other location" a bit tiring to read without adding much
> value.  To readers who consider "other" in that phrase to be clear
> enough (i.e. "location other than refs/notes"), it is totally
> redundant.  To other readers who feel "other" in that phrase to be
> under qualified (i.e. "location other than what???"), it is not
> informative enough.  Middle-ground readers who would not know if
> "refs/a" is inside or outside some "other" location, it is confusing.
> 
> After all, "a/b" is qualified under some location (i.e. a/) other
> than "refs/notes/", and it does mean "refs/notes/a/b".
> 
> How about phrasing it totally differently?
> 
> 	The ref specifies the full refname when it begins with
> 	`refs/notes/`; otherwise `ref/notes/` is prefixed to form a
> 	full name of the ref.
> 
> I think that would remove the need to illustrate with concrete
> examples like refs/heads/blah.
> 
Yes, let's go with that.

Regards, Jake

Keller, Jacob E· Sep 22, 2015, 21:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

On Tue, 2015-09-22 at 13:40 -0700, Junio C Hamano wrote:
Show 9 quoted lines
> How about phrasing it totally differently?
> 
> 	The ref specifies the full refname when it begins with
> 	`refs/notes/`; otherwise `ref/notes/` is prefixed to form a
> 	full name of the ref.
> 
> I think that would remove the need to illustrate with concrete
> examples like refs/heads/blah.
> 

Wait, what about the DWIM of notes/<name> goes to refs/notes/<name>.. do we need to explain that here? it might seem that "notes/foo" ends up as "refs/notes/notes/foo" which is not really what we mean.

Regards, Jake

Junio C Hamano· Sep 22, 2015, 21:23 UTC · re: Keller, Jacob E · lore

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

"Keller, Jacob E" <jacob.e.keller@intel.com> writes:
Show 13 quoted lines
> On Tue, 2015-09-22 at 13:40 -0700, Junio C Hamano wrote:
>> How about phrasing it totally differently?
>> 
>> 	The ref specifies the full refname when it begins with
>> 	`refs/notes/`; otherwise `ref/notes/` is prefixed to form a
>> 	full name of the ref.
>> 
>> I think that would remove the need to illustrate with concrete
>> examples like refs/heads/blah.
>> 
>
> Wait, what about the DWIM of notes/<name> goes to refs/notes/<name>..
> do we need to explain that here?
Yeah, throw that in, too.
	... when it begins with `refs/notes/`; when it begins with
	`notes/`, `refs/` and otherwise `refs/notes/` is prefixed to
	form a full name of the ref.
We could emphasize the end result by ending the sentence like so:
	... form a full name of the ref that is under `refs/notes/`.

if you wanted to. I am neutral (I do not think it would hurt, but I do not think it adds much clarity).

Thanks.

← back to recent threads