git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* specifying revision - how to enforce matching a tag/branch-name or revision only
@ 2019-06-20 14:32 Boettger, Heiko
  2019-06-20 22:54 ` Kyle Meyer
  2019-06-26  8:41 ` [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process Nguyễn Thái Ngọc Duy
  0 siblings, 2 replies; 11+ messages in thread
From: Boettger, Heiko @ 2019-06-20 14:32 UTC (permalink / raw)
  To: git@vger.kernel.org

Hi,

I discovered an interesting problem when using `git checkout` to which I couldn't find a good solution. We have an automatic system trying to checkout a branch only when it exists. To do so we check whether `git rev-parse` finds a commit for given branch name:

git rev-parse "${BRANCH_NAME}"  || git rev-parse "refs/remotes/${UPSTREAM}/${BRANCH_NAME}"

Unfortunately somebody used the branch name "add-gcc10" and `git rev-parse` which didn't exist on one repository. However `git rev-parse`
also supports to parse the `git-describe` format which resulted in checkout a commit starting with "cc10".

We saw a similar problem with disambiguation of tag and branch-name. If there any possibility to enforce that `git checkout` and other commands such `git rev-parse` handle a revision as a specific type (branch, tag, commit-id). Is there something like
`git checkout name@{branch}`  or `git checkout name@{tag}` similar to other rev-parse format such as  `branch@{upstream}`.

Best Regards

Heiko Böttger


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: specifying revision - how to enforce matching a tag/branch-name or revision only
  2019-06-20 14:32 specifying revision - how to enforce matching a tag/branch-name or revision only Boettger, Heiko
@ 2019-06-20 22:54 ` Kyle Meyer
  2019-06-21 15:16   ` Junio C Hamano
  2019-06-26  8:41 ` [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process Nguyễn Thái Ngọc Duy
  1 sibling, 1 reply; 11+ messages in thread
From: Kyle Meyer @ 2019-06-20 22:54 UTC (permalink / raw)
  To: Boettger, Heiko; +Cc: git@vger.kernel.org

Hi,

"Boettger, Heiko" <Heiko.Boettger@karlstorz.com> writes:

> Hi,
>
> I discovered an interesting problem when using `git checkout` to which
> I couldn't find a good solution. We have an automatic system trying to
> checkout a branch only when it exists. To do so we check whether `git
> rev-parse` finds a commit for given branch name:
>
> git rev-parse "${BRANCH_NAME}"  || git rev-parse "refs/remotes/${UPSTREAM}/${BRANCH_NAME}"
>
> Unfortunately somebody used the branch name "add-gcc10" and `git rev-parse` which didn't exist on one repository. However `git rev-parse`
> also supports to parse the `git-describe` format which resulted in checkout a commit starting with "cc10".

Can't you prepend "refs/heads/" to BRANCH_NAME to disambiguate?

-- 
Kyle

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: specifying revision - how to enforce matching a tag/branch-name or revision only
  2019-06-20 22:54 ` Kyle Meyer
@ 2019-06-21 15:16   ` Junio C Hamano
  2019-06-25 11:43     ` Duy Nguyen
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2019-06-21 15:16 UTC (permalink / raw)
  To: Kyle Meyer, Nguyễn Thái Ngọc Duy
  Cc: Boettger, Heiko, git@vger.kernel.org

Kyle Meyer <kyle@kyleam.com> writes:

>> git rev-parse "${BRANCH_NAME}"  || git rev-parse "refs/remotes/${UPSTREAM}/${BRANCH_NAME}"
>>
>> Unfortunately somebody used the branch name "add-gcc10" and `git rev-parse` which didn't exist on one repository. However `git rev-parse`
>> also supports to parse the `git-describe` format which resulted in checkout a commit starting with "cc10".
>
> Can't you prepend "refs/heads/" to BRANCH_NAME to disambiguate?

Yes, that is the kosher way for most commands.  

It gets a bit tricky for "checkout <branch-or-committish>" that
changes its behaviour (a local branch is checked out and the next
commit extends it, other committishes like tags and remote-tracking
branch tips are checked out on a detached HEAD), and has special
rules for a "${BRANCH_NAME}" that is both the name of a local branch
and something else.

Hopefully (Duy Cc'ed) "git switch" would improve the situation.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: specifying revision - how to enforce matching a tag/branch-name or revision only
  2019-06-21 15:16   ` Junio C Hamano
@ 2019-06-25 11:43     ` Duy Nguyen
  2019-06-25 14:35       ` AW: " Boettger, Heiko
  0 siblings, 1 reply; 11+ messages in thread
From: Duy Nguyen @ 2019-06-25 11:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Kyle Meyer, Boettger, Heiko, git@vger.kernel.org

On Fri, Jun 21, 2019 at 10:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Meyer <kyle@kyleam.com> writes:
>
> >> git rev-parse "${BRANCH_NAME}"  || git rev-parse "refs/remotes/${UPSTREAM}/${BRANCH_NAME}"
> >>
> >> Unfortunately somebody used the branch name "add-gcc10" and `git rev-parse` which didn't exist on one repository. However `git rev-parse`
> >> also supports to parse the `git-describe` format which resulted in checkout a commit starting with "cc10".

I wonder if something like refs/heads/foo-g<hash> could trip the
parser and mistake it as a `git-describe` output. Staring at
get_describe_name() alone the answer might be an unfortunate "yes".
But maybe something will kick in earlier and reject it.

> >
> > Can't you prepend "refs/heads/" to BRANCH_NAME to disambiguate?
>
> Yes, that is the kosher way for most commands.

Some commands always prepend refs/heads/ to the <branch> argument you
give it if I remember correctly. Or I think I accidentally made
refs/heads/refs/heads/something once with some command (then hell
ensued). If true, prepending refs/heads/ is not really foolproof.

> It gets a bit tricky for "checkout <branch-or-committish>" that
> changes its behaviour (a local branch is checked out and the next
> commit extends it, other committishes like tags and remote-tracking
> branch tips are checked out on a detached HEAD), and has special
> rules for a "${BRANCH_NAME}" that is both the name of a local branch
> and something else.

I think "git checkout --no-guess --no-detach <branch>" should only
accept a branch (i.e. ref: refs/heads/<branch>). Dropping --no-detach
should allow any ref, which interprets refs/heads/foo as an absolute
ref, not as refs/heads/refs/heads/foo.

> Hopefully (Duy Cc'ed) "git switch" would improve the situation.

"git switch" has slightly saner defaults, but with --guess being
default to be friendlier to interactive usage, it's still not that
much friendlier to scripts, unfortunately.
-- 
Duy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* AW: specifying revision - how to enforce matching a tag/branch-name or revision only
  2019-06-25 11:43     ` Duy Nguyen
@ 2019-06-25 14:35       ` Boettger, Heiko
  0 siblings, 0 replies; 11+ messages in thread
From: Boettger, Heiko @ 2019-06-25 14:35 UTC (permalink / raw)
  To: Duy Nguyen, Junio C Hamano; +Cc: Kyle Meyer, git@vger.kernel.org

First of all many thanks for the responses.

And yes refs/heads/foo-g<hash> will be interpreted by "git checkout" as "git-describe" output, if there is no such branch. My current solution for the moment is to extract the commit-sha1 from the output of "git for-each-ref" and fail if there is no exact match otherwise git checkout will anyway do what I want, because it finds the branch.

Regards
Heiko

________________________________________
Von: Duy Nguyen [pclouds@gmail.com]
Gesendet: Dienstag, 25. Juni 2019 13:43
An: Junio C Hamano
Cc: Kyle Meyer; Boettger, Heiko; git@vger.kernel.org
Betreff: Re: specifying revision - how to enforce matching a tag/branch-name or revision only

On Fri, Jun 21, 2019 at 10:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Meyer <kyle@kyleam.com> writes:
>
> >> git rev-parse "${BRANCH_NAME}"  || git rev-parse "refs/remotes/${UPSTREAM}/${BRANCH_NAME}"
> >>
> >> Unfortunately somebody used the branch name "add-gcc10" and `git rev-parse` which didn't exist on one repository. However `git rev-parse`
> >> also supports to parse the `git-describe` format which resulted in checkout a commit starting with "cc10".

I wonder if something like refs/heads/foo-g<hash> could trip the
parser and mistake it as a `git-describe` output. Staring at
get_describe_name() alone the answer might be an unfortunate "yes".
But maybe something will kick in earlier and reject it.

> >
> > Can't you prepend "refs/heads/" to BRANCH_NAME to disambiguate?
>
> Yes, that is the kosher way for most commands.

Some commands always prepend refs/heads/ to the <branch> argument you
give it if I remember correctly. Or I think I accidentally made
refs/heads/refs/heads/something once with some command (then hell
ensued). If true, prepending refs/heads/ is not really foolproof.

> It gets a bit tricky for "checkout <branch-or-committish>" that
> changes its behaviour (a local branch is checked out and the next
> commit extends it, other committishes like tags and remote-tracking
> branch tips are checked out on a detached HEAD), and has special
> rules for a "${BRANCH_NAME}" that is both the name of a local branch
> and something else.

I think "git checkout --no-guess --no-detach <branch>" should only
accept a branch (i.e. ref: refs/heads/<branch>). Dropping --no-detach
should allow any ref, which interprets refs/heads/foo as an absolute
ref, not as refs/heads/refs/heads/foo.

> Hopefully (Duy Cc'ed) "git switch" would improve the situation.

"git switch" has slightly saner defaults, but with --guess being
default to be friendlier to interactive usage, it's still not that
much friendlier to scripts, unfortunately.
--
Duy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-20 14:32 specifying revision - how to enforce matching a tag/branch-name or revision only Boettger, Heiko
  2019-06-20 22:54 ` Kyle Meyer
@ 2019-06-26  8:41 ` Nguyễn Thái Ngọc Duy
  2019-06-26 18:51   ` Junio C Hamano
  2019-06-30  9:30   ` Jakub Narebski
  1 sibling, 2 replies; 11+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-26  8:41 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, kyle, Heiko.Boettger,
	Nguyễn Thái Ngọc Duy

[I feel like this is something we should do, but I'm not sure. Hence
RFC. The patch is mostly to play with if you're curious. The die() thing
there is definitely wrong. And yeah all the syntax names are
placeholders.]

get_oid() is flexible and accepts multiple SHA-1 sources. Sometimes this
flexibility is actually unwanted, especially for scripts. Let's talk
problems:

- Ambiguity aside, a script may want to check if branch A exists. Of
  course "git rev-parse A" won't cut it. But even "git rev-parse
  refs/heads/A" may fail: if you have refs/heads/refs/heads/A for
  whatever reason and the real branch "A" does not exist, the rev-parse
  rules allow to expand "refs/heads/A" to the long ref.

- And then there's problem with using the wrong rule. 9309ba may look
  like a short hash. But if such short hash does not match any object,
  and there is refs/heads/9309ba, you'll have a little surprise.

- The same for blahblah-g9309ba which could either be expanded to
  refs/heads/blahblah-g9309ba, or interpreted as git-describe output.

- Ambiguation will also cause problems, but I don't think we need to
  get into that. Ambiguation may be addressed separately actually.

There could be existing mitigation (e.g. maybe you can resolve
blahblah-g9309ba and see if it's a ref or not). But it feels like we
work around the problem than addressing it.

The problem is we try every possible way to resolve a rev. Let's have
some annotation to express that we only want to resolve a rev in a
certain way:

- <hash>@{hash} only accepts a full hash or a short hash. If it's a
  short hash, it cannot be ambiguous.

- <ref>@{literal} only accepts full ref. No turning "master" into
  "refs/heads/master".

- <output>@{describe} interprets <output> as git-describe output
  only, not an object name or a reference.

This gives scripts much better control over get_oid(), which
translates to rev-parse and a bunch other commands.

PS. The new syntax can stack with existing ones. E.g. you could write
refs/heads/master@{literal}@{yesterday} or <hash>@{hash}^{tree}.
Perhaps I should allow these tags at the end too, so you can enforce a
variable like "$REV"@{literal} where $REV could be even HEAD~123

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/revisions.txt | 10 ++++++++
 refs.c                      | 41 +++++++++++++++++++++++++++++--
 refs.h                      |  2 ++
 sha1-name.c                 | 48 +++++++++++++++++++++++++++++++++++--
 4 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 82c1e5754e..93eb278743 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -86,6 +86,16 @@ some output processing may assume ref names in UTF-8.
   immediately following a ref name and the ref must have an existing
   log ('$GIT_DIR/logs/<refname>').
 
+'<refname>@{literal}'::
+  The ref is not expanded by Git. In other words, if '$GIT_DIR/<refname>'
+  does not exist, the ref is not valid.
+
+'<sha1>@{hash}'::
+  '<sha1>' must be an unambiguous (short or full) object name.
+
+'<describeOutput>@{describe}'::
+  ...
+
 '@{<n>}', e.g. '@\{1\}'::
   You can use the '@' construct with an empty ref part to get at a
   reflog entry of the current branch. For example, if you are on
diff --git a/refs.c b/refs.c
index b8a8430c96..2ee33257fd 100644
--- a/refs.c
+++ b/refs.c
@@ -634,6 +634,27 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,
 	return refs_found;
 }
 
+int repo_dwim_ref_strict(struct repository *r,
+			 const char *str, int len,
+			 struct object_id *oid,
+			 char **ref)
+{
+	char *last_branch = substitute_branch_name(r, &str, &len);
+	struct strbuf sb = STRBUF_INIT;
+	int flag;
+
+	FREE_AND_NULL(last_branch);
+	strbuf_add(&sb, str, len);
+	*ref = xstrdup_or_null(
+		refs_resolve_ref_unsafe(get_main_ref_store(r),
+					sb.buf,
+					RESOLVE_REF_READING,
+					oid,
+					&flag));
+	strbuf_release(&sb);
+	return *ref != NULL;
+}
+
 int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)
 {
 	return repo_dwim_ref(the_repository, str, len, oid, ref);
@@ -673,8 +694,9 @@ int expand_ref(struct repository *repo, const char *str, int len,
 	return refs_found;
 }
 
-int repo_dwim_log(struct repository *r, const char *str, int len,
-		  struct object_id *oid, char **log)
+static int do_dwim_log(struct repository *r, const char *str, int len,
+		       struct object_id *oid, char **log,
+		       int ignore_rev_parse_rules)
 {
 	struct ref_store *refs = get_main_ref_store(r);
 	char *last_branch = substitute_branch_name(r, &str, &len);
@@ -687,6 +709,9 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 		struct object_id hash;
 		const char *ref, *it;
 
+		if (ignore_rev_parse_rules && p != ref_rev_parse_rules)
+			break;
+
 		strbuf_reset(&path);
 		strbuf_addf(&path, *p, len, str);
 		ref = refs_resolve_ref_unsafe(refs, path.buf,
@@ -713,6 +738,18 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 	return logs_found;
 }
 
+int repo_dwim_log(struct repository *r, const char *str, int len,
+		  struct object_id *oid, char **log)
+{
+	return do_dwim_log(r, str, len, oid, log, 0);
+}
+
+int repo_dwim_log_strict(struct repository *r, const char *str, int len,
+			 struct object_id *oid, char **log)
+{
+	return do_dwim_log(r, str, len, oid, log, 1);
+}
+
 int dwim_log(const char *str, int len, struct object_id *oid, char **log)
 {
 	return repo_dwim_log(the_repository, str, len, oid, log);
diff --git a/refs.h b/refs.h
index 730d05ad91..9395a2e708 100644
--- a/refs.h
+++ b/refs.h
@@ -151,6 +151,8 @@ void expand_ref_prefix(struct argv_array *prefixes, const char *prefix);
 int expand_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
 int repo_dwim_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
 int repo_dwim_log(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
+int repo_dwim_ref_strict(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
+int repo_dwim_log_strict(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
 int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
 
diff --git a/sha1-name.c b/sha1-name.c
index 728e6f1f61..2d05414140 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -789,6 +789,25 @@ static inline int push_mark(const char *string, int len)
 	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
+static inline int literal_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{literal}" };
+	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
+}
+
+static inline int find_mark(const char *mark, const char *string, int len, int *len_p)
+{
+	int mark_len = strlen(mark);
+
+	if (mark_len <= len &&
+	    !strncasecmp(string + len - mark_len, mark, mark_len)) {
+		if (len_p)
+			*len_p -= mark_len;
+		return 1;
+	}
+	return 0;
+}
+
 static enum get_oid_result get_oid_1(struct repository *r, const char *name, int len, struct object_id *oid, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(struct repository *r, const char *name, int namelen, struct strbuf *buf);
 
@@ -811,6 +830,18 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 	int refs_found = 0;
 	int at, reflog_len, nth_prior = 0;
 
+	if (find_mark("@{hash}", str, len, &len)) {
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_add(&sb, str, len);
+		if (!get_oid_hex(sb.buf, oid) ||
+		    !get_short_oid(r, sb.buf, sb.len, oid, flags)) {
+			strbuf_release(&sb);
+			return 0;
+		}
+		die("Not a hash. Yes I know this should be fatal");
+	}
+
 	if (len == r->hash_algo->hexsz && !get_oid_hex(str, oid)) {
 		if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
 			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref);
@@ -837,7 +868,8 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 					continue;
 				}
 				if (!upstream_mark(str + at, len - at) &&
-				    !push_mark(str + at, len - at)) {
+				    !push_mark(str + at, len - at) &&
+				    !literal_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -862,7 +894,14 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 		}
 	}
 
-	if (!len && reflog_len)
+	if (find_mark("@{literal}", str, len, &len)) {
+		if (reflog_len)
+			refs_found = repo_dwim_log_strict(r, str, len, oid, &real_ref);
+		else
+			refs_found = repo_dwim_ref_strict(r, str, len, oid, &real_ref);
+		if (!refs_found)
+			die("bad ref");
+	} else if (!len && reflog_len)
 		/* allow "@{...}" to mean the current branch reflog */
 		refs_found = repo_dwim_ref(r, "HEAD", 4, oid, &real_ref);
 	else if (reflog_len)
@@ -1180,6 +1219,11 @@ static enum get_oid_result get_oid_1(struct repository *r,
 	if (!ret)
 		return FOUND;
 
+	if (find_mark("@{describe}", name, len, &len)) {
+		ret = get_describe_name(r, name, len, oid);
+		return !ret ? FOUND : MISSING_OBJECT;
+	}
+
 	ret = get_oid_basic(r, name, len, oid, lookup_flags);
 	if (!ret)
 		return FOUND;
-- 
2.22.0.rc0.322.g2b0371e29a


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-26  8:41 ` [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process Nguyễn Thái Ngọc Duy
@ 2019-06-26 18:51   ` Junio C Hamano
  2019-06-27  2:52     ` Duy Nguyen
  2019-06-30  9:30   ` Jakub Narebski
  1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2019-06-26 18:51 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, kyle, Heiko.Boettger

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> The problem is we try every possible way to resolve a rev. Let's have
> some annotation to express that we only want to resolve a rev in a
> certain way:
>
> - <hash>@{hash} only accepts a full hash or a short hash. If it's a
>   short hash, it cannot be ambiguous.

This is to defeat something like refs/heads/deadbeef (the branch
whose name is "deadbeef") interfering an object whose name begins
with that hexadecimal substring.  Makes sense.

> - <ref>@{literal} only accepts full ref. No turning "master" into
>   "refs/heads/master".

This is already doable by saying "refs/heads/master", isn't it?  Any
ambiguous ones like a branch or a tag whose name is 'refs/heads/master'
(i.e. refs/heads/refs/heads/master and refs/tags/refs/heads/master
respectively) won't interfere once you use the explicit prefix refs/
like so.  I'd rather not to see this one added.

> - <output>@{describe} interprets <output> as git-describe output
>   only, not an object name or a reference.

Again this is to defeat refs/heads/v2.22.0-221-gdeadbeef branch
interferring with the object "deadbeef" thusly described.  Makes
sense.

In the above, my two "Makes sense" only refer to the capability the
syntax allows us to have, not the suggested syntax itself (which I
am not convinced is a good use of @{<token>} yet).

Thanks.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-26 18:51   ` Junio C Hamano
@ 2019-06-27  2:52     ` Duy Nguyen
  2019-06-27 16:47       ` Junio C Hamano
  0 siblings, 1 reply; 11+ messages in thread
From: Duy Nguyen @ 2019-06-27  2:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Kyle Meyer, Boettger, Heiko

On Thu, Jun 27, 2019 at 1:51 AM Junio C Hamano <gitster@pobox.com> wrote:
> > - <ref>@{literal} only accepts full ref. No turning "master" into
> >   "refs/heads/master".
>
> This is already doable by saying "refs/heads/master", isn't it?  Any
> ambiguous ones like a branch or a tag whose name is 'refs/heads/master'
> (i.e. refs/heads/refs/heads/master and refs/tags/refs/heads/master
> respectively) won't interfere once you use the explicit prefix refs/
> like so.  I'd rather not to see this one added.

If refs/heads/master does _not_ exist, then ref/heads/master can
resolve to refs/heads/refs/heads/master, which I think is unexpected.
"ref not found" error or something should be returned instead.
-- 
Duy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-27  2:52     ` Duy Nguyen
@ 2019-06-27 16:47       ` Junio C Hamano
  0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-06-27 16:47 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Kyle Meyer, Boettger, Heiko

Duy Nguyen <pclouds@gmail.com> writes:

> If refs/heads/master does _not_ exist, then ref/heads/master can
> resolve to refs/heads/refs/heads/master, which I think is unexpected.
> "ref not found" error or something should be returned instead.

Ah, that was what I missed.  Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-26  8:41 ` [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process Nguyễn Thái Ngọc Duy
  2019-06-26 18:51   ` Junio C Hamano
@ 2019-06-30  9:30   ` Jakub Narebski
  2019-07-01  0:30     ` Duy Nguyen
  1 sibling, 1 reply; 11+ messages in thread
From: Jakub Narebski @ 2019-06-30  9:30 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, kyle, Heiko.Boettger

Bikeshed painting ahead.

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
[...]
> The problem is we try every possible way to resolve a rev. Let's have
> some annotation to express that we only want to resolve a rev in a
> certain way:
>
> - <hash>@{hash} only accepts a full hash or a short hash. If it's a
>   short hash, it cannot be ambiguous.
>
> - <ref>@{literal} only accepts full ref. No turning "master" into
>   "refs/heads/master".
>
> - <output>@{describe} interprets <output> as git-describe output
>   only, not an object name or a reference.
>
> This gives scripts much better control over get_oid(), which
> translates to rev-parse and a bunch other commands.
>
> PS. The new syntax can stack with existing ones. E.g. you could write
> refs/heads/master@{literal}@{yesterday} or <hash>@{hash}^{tree}.
> Perhaps I should allow these tags at the end too, so you can enforce a
> variable like "$REV"@{literal} where $REV could be even HEAD~123

I think it would be better to use <hash>^{hash} instead of
<hash>@{hash}.

The <ref-ish>@{<something>} is used currently for information that is
outside the DAG, like @{<date>}, @{<n>}, @{-<n>} uses information from
reflog, and @{upstream} and @{push} uses information from the config.

On the other hand ^{<type>}, ^{/<search text>}, and the future
^{<hashalgo>} all use DAG-only information.

Though one could argue that refs information _is_ outside the DAG...


P.S. We have 'git show-ref --verify' that requires exact match (no
DWIM-mery), which can be used together with '--quiet' in a script.
But this doesn't allow for checking if an exact match, or a describe
output exists in repository.


Thanks for taking it up,
--
Jakub Narębski

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process
  2019-06-30  9:30   ` Jakub Narebski
@ 2019-07-01  0:30     ` Duy Nguyen
  0 siblings, 0 replies; 11+ messages in thread
From: Duy Nguyen @ 2019-07-01  0:30 UTC (permalink / raw)
  To: Jakub Narebski
  Cc: Git Mailing List, Junio C Hamano, Kyle Meyer, Boettger, Heiko,
	brian m. carlson

On Sun, Jun 30, 2019 at 4:30 PM Jakub Narebski <jnareb@gmail.com> wrote:
>
> Bikeshed painting ahead.
>
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> [...]
> > The problem is we try every possible way to resolve a rev. Let's have
> > some annotation to express that we only want to resolve a rev in a
> > certain way:
> >
> > - <hash>@{hash} only accepts a full hash or a short hash. If it's a
> >   short hash, it cannot be ambiguous.
> >
> > - <ref>@{literal} only accepts full ref. No turning "master" into
> >   "refs/heads/master".
> >
> > - <output>@{describe} interprets <output> as git-describe output
> >   only, not an object name or a reference.
> >
> > This gives scripts much better control over get_oid(), which
> > translates to rev-parse and a bunch other commands.
> >
> > PS. The new syntax can stack with existing ones. E.g. you could write
> > refs/heads/master@{literal}@{yesterday} or <hash>@{hash}^{tree}.
> > Perhaps I should allow these tags at the end too, so you can enforce a
> > variable like "$REV"@{literal} where $REV could be even HEAD~123
>
> I think it would be better to use <hash>^{hash} instead of
> <hash>@{hash}.
>
> The <ref-ish>@{<something>} is used currently for information that is
> outside the DAG, like @{<date>}, @{<n>}, @{-<n>} uses information from
> reflog, and @{upstream} and @{push} uses information from the config.
>
> On the other hand ^{<type>}, ^{/<search text>}, and the future
> ^{<hashalgo>} all use DAG-only information.

I wasn't aware of ^{<hashalgo>}. hash-function-transition.txt is a bit
light on the exact behavior, but maybe we can just define it like
<hash>@{hash} in this thread (then we don't even need to introduce
@{hash})?

That is, if <hash>^{<hashalgo>} fails to find a matching object, the
get_oid() result is "not found", there will be no further attempt to
interpret the given revision in a different way (e.g. try to see if
the same ref exists...). It also means <hash>^{<hashalgo>} will remain
supported even after the sha-512 transition period.

@{describe} might go  the same way as @{hash} (that is, becoming
^{describe}). I think it's just a slightly different way to write
hashes.

> Though one could argue that refs information _is_ outside the DAG...

Refs info is outside DAG. But that's mostly for <ref>@{literal} and it
allows something like <ref>@{literal}@{upstream}. But hash (or object
name) is already used inside DAG (to link between commits, trees and
blobs). Though one might still argue that's only true for full hash,
not short ones.
-- 
Duy

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-07-01  0:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-20 14:32 specifying revision - how to enforce matching a tag/branch-name or revision only Boettger, Heiko
2019-06-20 22:54 ` Kyle Meyer
2019-06-21 15:16   ` Junio C Hamano
2019-06-25 11:43     ` Duy Nguyen
2019-06-25 14:35       ` AW: " Boettger, Heiko
2019-06-26  8:41 ` [PATCH/RFC] get_oid: new extended SHA-1 syntax to control resolution process Nguyễn Thái Ngọc Duy
2019-06-26 18:51   ` Junio C Hamano
2019-06-27  2:52     ` Duy Nguyen
2019-06-27 16:47       ` Junio C Hamano
2019-06-30  9:30   ` Jakub Narebski
2019-07-01  0:30     ` Duy Nguyen

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).