Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
On Thu, Sep 6, 2012 at 9:47 PM, Junio C Hamano wrote: > "W. Trevor King" writes: >> On Wed, Sep 05, 2012 at 10:23:54PM -0700, Junio C Hamano wrote: >>> Really? Would "git log --expand master" be useful? >> >> I'm clearly not an expert on this, but isn't that what >> >> git show-ref master >> >> is for? > > But then can't you say the same against "git notes get-ref --expand" > with "git show-ref refs/remotes/origin/notes/foobla"? > > My primary point is that I think it is a UI mistake if the DWIM > ignores the user input that directly names that can be interpreted > without DWIMming. When the user wants to avoid ambiguity, it should > always be possible to spell it out without having to worry about the > DWIM code to get in the way. > > The problem with the current notes.c:expand_notes_ref() is that it > does not allow you to do that, and if you fixed that problem, the > user never has to ask "what does this expand to", no? > > Perhaps something like this. Note that this only deals with an > existing ref, and that is semi-deliberate (because I am not yet > convinced that it is a good idea to let any ref outside refs/notes/ > to be created to hold a notes tree). (sorry for the late reply; I was away this weekend) This patch looks like an acceptable solution to me. Especially since it prevents the notes code from "accidentally" creating new notes trees outside refs/notes/*. That said, we should check for more cases of hardcoded "refs/notes/" that potentially needs changing as well. ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
"W. Trevor King" writes:
> On Wed, Sep 05, 2012 at 10:23:54PM -0700, Junio C Hamano wrote:
>> Really? Would "git log --expand master" be useful?
>
> I'm clearly not an expert on this, but isn't that what
>
> git show-ref master
>
> is for?
But then can't you say the same against "git notes get-ref --expand"
with "git show-ref refs/remotes/origin/notes/foobla"?
My primary point is that I think it is a UI mistake if the DWIM
ignores the user input that directly names that can be interpreted
without DWIMming. When the user wants to avoid ambiguity, it should
always be possible to spell it out without having to worry about the
DWIM code to get in the way.
The problem with the current notes.c:expand_notes_ref() is that it
does not allow you to do that, and if you fixed that problem, the
user never has to ask "what does this expand to", no?
Perhaps something like this. Note that this only deals with an
existing ref, and that is semi-deliberate (because I am not yet
convinced that it is a good idea to let any ref outside refs/notes/
to be created to hold a notes tree).
notes.c | 2 ++
1 file changed, 2 insertions(+)
diff --git i/notes.c w/notes.c
index 93e9868..126839e 100644
--- i/notes.c
+++ w/notes.c
@@ -1289,6 +1289,8 @@ int copy_note(struct notes_tree *t,
void expand_notes_ref(struct strbuf *sb)
{
+ if (!prefixcmp(sb->buf, "refs/") && ref_exists(sb->buf))
+ return;
if (!prefixcmp(sb->buf, "refs/notes/"))
return; /* we're happy */
else if (!prefixcmp(sb->buf, "notes/"))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
On Wed, Sep 05, 2012 at 10:23:54PM -0700, Junio C Hamano wrote: > Really? Would "git log --expand master" be useful? I'm clearly not an expert on this, but isn't that what git show-ref master is for? Or is the fact that show-ref returns hashes the more important feature? There was a lot of "ambiguous refs" discussion in the follow-up for http://thread.gmane.org/gmane.comp.version-control.git/165885 Subject: [1.8.0] Provide proper remote ref namespaces For example: On Mon, Feb 07, 2011 at 03:25:51PM -0500, Jeff King wrote: > Will the same apply to refs/heads/foo versus refs/remotes/*/foo? Will it > also apply to refs/heads/foo versus refs/remotes/*/tags/foo? In the > final case, that does matter to "git push" (should the destination be in > the head or tag namespace?). So the actual names of the ref can matter, > and should probably be taken into account when deciding what is > ambiguous. So it seems like having a way to figure out how Git is interpreting a shorthand ref would be useful, especially while those of us with less experience are learning to think like Git. The expansion command doesn't have to be notes-specific, but since the current ref expansion is role specific (e.g. branch/tag/note/…), it seemed like the best place to put it. With more consistent ref interpretation, I'd be less interested in this expansion command ;). Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
Johan Herland writes: > On Wed, Sep 5, 2012 at 2:52 PM, W. Trevor King wrote: >> From: "W. Trevor King" >> >> Useful for debugging refs that don't seem to be expanding correctly. >> >> Signed-off-by: W. Trevor King > > Acked-by: Johan Herland Really? Would "git log --expand master" be useful? I'd have to say that a DWIM that the user needs to diagnose with such a "feature" is a total failure. Can't we fix that root cause instead? That is, for a name, e.g. "refs/remotes/origin/notes/commit" (the original example in [PATCH 0/2]) or "commit" (the usual): - If it is already a ref, just use that (e.g. use that stuff in the refs/remotes/ hierarchy); or - If it is not, prefix refs/notes/ to it. or something? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
On Wed, Sep 5, 2012 at 2:52 PM, W. Trevor King wrote: > From: "W. Trevor King" > > Useful for debugging refs that don't seem to be expanding correctly. > > Signed-off-by: W. Trevor King Acked-by: Johan Herland -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] notes get-ref: --expand expands the output notes ref.
From: "W. Trevor King"
Useful for debugging refs that don't seem to be expanding correctly.
Signed-off-by: W. Trevor King
---
Documentation/git-notes.txt | 6 +-
builtin/notes.c | 26 +-
t/t3301-notes.sh| 8
3 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index b95aafa..a93d211 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -19,7 +19,7 @@ SYNOPSIS
'git notes' merge --abort [-v | -q]
'git notes' remove [--ignore-missing] [--stdin] [...]
'git notes' prune [-n | -v]
-'git notes' get-ref
+'git notes' get-ref [-e]
DESCRIPTION
@@ -165,6 +165,10 @@ OPTIONS
input (there is no reason you cannot combine this with object
names from the command line).
+-e::
+--expand::
+ Expand the notes ref before printing it.
+
-n::
--dry-run::
Do not remove anything; just report the object names whose notes
diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..17c6136 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -31,7 +31,7 @@ static const char * const git_notes_usage[] = {
"git notes merge --abort [-v | -q]",
"git notes [--ref ] remove [...]",
"git notes [--ref ] prune [-n | -v]",
- "git notes [--ref ] get-ref",
+ "git notes [--ref ] get-ref [-e]",
NULL
};
@@ -84,7 +84,7 @@ static const char * const git_notes_prune_usage[] = {
};
static const char * const git_notes_get_ref_usage[] = {
- "git notes get-ref",
+ "git notes get-ref []",
NULL
};
@@ -1046,16 +1046,32 @@ static int prune(int argc, const char **argv, const
char *prefix)
static int get_ref(int argc, const char **argv, const char *prefix)
{
- struct option options[] = { OPT_END() };
+ const char *ref;
+ struct strbuf buf = STRBUF_INIT;
+ int expand = 0;
+ struct option options[] = {
+ OPT_BOOL('e', "expand", &expand,
+"expand the notes ref before printing it"),
+ OPT_END()
+ };
argc = parse_options(argc, argv, prefix, options,
git_notes_get_ref_usage, 0);
- if (argc) {
+ if (argc > 1) {
error("too many parameters");
usage_with_options(git_notes_get_ref_usage, options);
}
- puts(default_notes_ref());
+ ref = default_notes_ref();
+ if (expand) {
+ strbuf_insert(&buf, 0, ref, strlen(ref));
+ expand_notes_ref(&buf);
+ puts(buf.buf);
+ strbuf_release(&buf);
+ } else {
+ puts(ref);
+ }
+
return 0;
}
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 16de05a..c0486a0 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,4 +1222,12 @@ test_expect_success 'git notes get-ref (--ref)' '
test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" =
"refs/notes/baz"
'
+test_expect_success 'git notes get-ref (no expand)' '
+ test "$(GIT_NOTES_REF=commits git notes get-ref)" = "commits"
+'
+
+test_expect_success 'git notes get-ref (--expand)' '
+ test "$(GIT_NOTES_REF=commits git notes get-ref --expand)" =
"refs/notes/commits"
+'
+
test_done
--
1.7.12.176.g3fc0e4c.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
