git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: David Turner <novalis@novalis.org>
To: Eric Wong <e@80x24.org>
Cc: git@vger.kernel.org
Subject: Re: cat-file ambiguity prints "dangling"
Date: Fri, 18 Jan 2019 11:20:48 -0500
Message-ID: <672a6fb9e480becbfcb5df23ae37193784811b6b.camel@novalis.org> (raw)
In-Reply-To: <20190118104519.fqpzirux5qulkcwa@dcvr>

[-- Attachment #1: Type: text/plain, Size: 1839 bytes --]

Let's try this version.

On Fri, 2019-01-18 at 10:45 +0000, Eric Wong wrote:
> David Turner <novalis@novalis.org> wrote:
> > It appears that get_oid_with_context calls into get_short_oid for
> > that
> > case, and get_short_oid returns SHORT_NAME_AMBIGUOUS, which is
> > -2.  We
> > treat that as DANGLING_SYMLINK, which also seems to have the value
> > -2. 
> > So, it's an ambiguity in ambiguity resolution.
> > 
> > Fix attached.  
> 
> Thanks for the quick response.  Unfortunately, the patch makes
> the situation worse for me...
> 
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 2ca56fd086..f2344b199f 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -380,7 +380,7 @@ static void batch_one_object(const char
> > *obj_name,
> >  {
> >  	struct object_context ctx;
> >  	int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;
> > -	enum follow_symlinks_result result;
> > +	enum get_oid_result result;
> >  
> >  	result = get_oid_with_context(obj_name, flags, &data->oid,
> > &ctx);
> >  	if (result != FOUND) {
> > @@ -388,6 +388,9 @@ static void batch_one_object(const char
> > *obj_name,
> >  		case MISSING_OBJECT:
> >  			printf("%s missing\n", obj_name);
> >  			break;
> > +		case SHORT_NAME_AMBIGUOUS:
> > +			/* We've already printed the ambiguous message
> > */
> > +			break;
> 
> Because the message about ambiguity only goes to stderr.
> Something needs to go to stdout, or else the reader process
> would hang forever.
> 
> I've already updated my code[1] to work around "dangling",
> but maybe printing "ambiguous" similar to the "missing" message
> would be appropriate?  Something like:
> 
> 		case SHORT_NAME_AMBIGUOUS:
> 			printf("%s ambiguous\n", obj_name);
> 			break;
> 
> (Along with a manpage update)
> 
> 
> [1] new stuff for public-inbox on the way :>
> 

[-- Attachment #2: v2-0001-Do-not-print-dangling-for-cat-file-in-case-of-amb.patch --]
[-- Type: text/x-patch, Size: 11078 bytes --]

From b152b3463477a8260d2b8c0ab46ad76562feed37 Mon Sep 17 00:00:00 2001
From: David Turner <novalis@novalis.org>
Date: Thu, 17 Jan 2019 23:19:43 -0500
Subject: [PATCH v2] Do not print 'dangling' for cat-file in case of ambiguity

The return values -1 and -2 from get_oid could mean two different
things, depending on whether they were from an enum returned by
get_tree_entry_follow_symlinks, or from a different code path.  This
caused 'dangling' to be printed from a git cat-file in the case of an
ambiguous (-2) result.

Unify the results of get_oid* and get_tree_entry_follow_symlinks to be
one common type, with unambiguous values.

Signed-off-by: David Turner <novalis@novalis.org>
Reported-by: Eric Wong <e@80x24.org>
---
 Documentation/git-cat-file.txt |  6 ++++
 builtin/cat-file.c             |  5 ++-
 cache.h                        | 20 +++++++++++-
 sha1-name.c                    | 58 ++++++++++++++++++----------------
 tree-walk.c                    |  4 +--
 tree-walk.h                    | 18 +----------
 6 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 74013335a1..e27a940985 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -252,6 +252,12 @@ the repository, then `cat-file` will ignore any custom format and print:
 <object> SP missing LF
 ------------
 
+If a name is specified that might refer to more than one object (an ambiguous short sha), then `cat-file` will ignore any custom format and print:
+
+------------
+<object> SP ambiguous LF
+------------
+
 If --follow-symlinks is used, and a symlink in the repository points
 outside the repository, then `cat-file` will ignore any custom format
 and print:
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2ca56fd086..cebc6d7f8a 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -380,7 +380,7 @@ static void batch_one_object(const char *obj_name,
 {
 	struct object_context ctx;
 	int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;
-	enum follow_symlinks_result result;
+	enum get_oid_result result;
 
 	result = get_oid_with_context(obj_name, flags, &data->oid, &ctx);
 	if (result != FOUND) {
@@ -388,6 +388,9 @@ static void batch_one_object(const char *obj_name,
 		case MISSING_OBJECT:
 			printf("%s missing\n", obj_name);
 			break;
+		case SHORT_NAME_AMBIGUOUS:
+			printf("%s ambiguous\n", obj_name);
+			break;
 		case DANGLING_SYMLINK:
 			printf("dangling %"PRIuMAX"\n%s\n",
 			       (uintmax_t)strlen(obj_name), obj_name);
diff --git a/cache.h b/cache.h
index 49713cc5a5..70652e99dc 100644
--- a/cache.h
+++ b/cache.h
@@ -1332,6 +1332,24 @@ struct object_context {
 	GET_OID_TREE | GET_OID_TREEISH | \
 	GET_OID_BLOB)
 
+enum get_oid_result {
+	FOUND = 0,
+	MISSING_OBJECT = -1, /* The requested object is missing */
+	SHORT_NAME_AMBIGUOUS = -2,
+	/* The following only apply when symlinks are followed */
+	DANGLING_SYMLINK = -4, /*
+				* The initial symlink is there, but
+				* (transitively) points to a missing
+				* in-tree file
+				*/
+	SYMLINK_LOOP = -5,
+	NOT_DIR = -6, /*
+		       * Somewhere along the symlink chain, a path is
+		       * requested which contains a file as a
+		       * non-final element.
+		       */
+};
+
 extern int get_oid(const char *str, struct object_id *oid);
 extern int get_oid_commit(const char *str, struct object_id *oid);
 extern int get_oid_committish(const char *str, struct object_id *oid);
@@ -1339,7 +1357,7 @@ extern int get_oid_tree(const char *str, struct object_id *oid);
 extern int get_oid_treeish(const char *str, struct object_id *oid);
 extern int get_oid_blob(const char *str, struct object_id *oid);
 extern void maybe_die_on_misspelt_object_name(const char *name, const char *prefix);
-extern int get_oid_with_context(const char *str, unsigned flags, struct object_id *oid, struct object_context *oc);
+extern enum get_oid_result get_oid_with_context(const char *str, unsigned flags, struct object_id *oid, struct object_context *oc);
 
 
 typedef int each_abbrev_fn(const struct object_id *oid, void *);
diff --git a/sha1-name.c b/sha1-name.c
index b24502811b..12313165d0 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -190,9 +190,6 @@ static void find_short_packed_object(struct disambiguate_state *ds)
 		unique_in_pack(p, ds);
 }
 
-#define SHORT_NAME_NOT_FOUND (-1)
-#define SHORT_NAME_AMBIGUOUS (-2)
-
 static int finish_object_disambiguation(struct disambiguate_state *ds,
 					struct object_id *oid)
 {
@@ -200,7 +197,7 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 		return SHORT_NAME_AMBIGUOUS;
 
 	if (!ds->candidate_exists)
-		return SHORT_NAME_NOT_FOUND;
+		return MISSING_OBJECT;
 
 	if (!ds->candidate_checked)
 		/*
@@ -414,8 +411,9 @@ static int sort_ambiguous(const void *a, const void *b)
 	return a_type_sort > b_type_sort ? 1 : -1;
 }
 
-static int get_short_oid(const char *name, int len, struct object_id *oid,
-			  unsigned flags)
+static enum get_oid_result get_short_oid(const char *name, int len,
+					 struct object_id *oid,
+					 unsigned flags)
 {
 	int status;
 	struct disambiguate_state ds;
@@ -733,7 +731,7 @@ static inline int push_mark(const char *string, int len)
 	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
-static int get_oid_1(const char *name, int len, struct object_id *oid, unsigned lookup_flags);
+static enum get_oid_result get_oid_1(const char *name, int len, struct object_id *oid, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
 
 static int get_oid_basic(const char *str, int len, struct object_id *oid,
@@ -883,11 +881,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 	return 0;
 }
 
-static int get_parent(const char *name, int len,
-		      struct object_id *result, int idx)
+static enum get_oid_result get_parent(const char *name, int len,
+				      struct object_id *result, int idx)
 {
 	struct object_id oid;
-	int ret = get_oid_1(name, len, &oid, GET_OID_COMMITTISH);
+	enum get_oid_result ret = get_oid_1(name, len, &oid,
+					    GET_OID_COMMITTISH);
 	struct commit *commit;
 	struct commit_list *p;
 
@@ -895,24 +894,25 @@ static int get_parent(const char *name, int len,
 		return ret;
 	commit = lookup_commit_reference(the_repository, &oid);
 	if (parse_commit(commit))
-		return -1;
+		return MISSING_OBJECT;
 	if (!idx) {
 		oidcpy(result, &commit->object.oid);
-		return 0;
+		return FOUND;
 	}
 	p = commit->parents;
 	while (p) {
 		if (!--idx) {
 			oidcpy(result, &p->item->object.oid);
-			return 0;
+			return FOUND;
 		}
 		p = p->next;
 	}
-	return -1;
+	return MISSING_OBJECT;
 }
 
-static int get_nth_ancestor(const char *name, int len,
-			    struct object_id *result, int generation)
+static enum get_oid_result get_nth_ancestor(const char *name, int len,
+					    struct object_id *result,
+					    int generation)
 {
 	struct object_id oid;
 	struct commit *commit;
@@ -923,15 +923,15 @@ static int get_nth_ancestor(const char *name, int len,
 		return ret;
 	commit = lookup_commit_reference(the_repository, &oid);
 	if (!commit)
-		return -1;
+		return MISSING_OBJECT;
 
 	while (generation--) {
 		if (parse_commit(commit) || !commit->parents)
-			return -1;
+			return MISSING_OBJECT;
 		commit = commit->parents->item;
 	}
 	oidcpy(result, &commit->object.oid);
-	return 0;
+	return FOUND;
 }
 
 struct object *peel_to_type(const char *name, int namelen,
@@ -1077,7 +1077,9 @@ static int get_describe_name(const char *name, int len, struct object_id *oid)
 	return -1;
 }
 
-static int get_oid_1(const char *name, int len, struct object_id *oid, unsigned lookup_flags)
+static enum get_oid_result get_oid_1(const char *name, int len,
+				     struct object_id *oid,
+				     unsigned lookup_flags)
 {
 	int ret, has_suffix;
 	const char *cp;
@@ -1111,16 +1113,16 @@ static int get_oid_1(const char *name, int len, struct object_id *oid, unsigned
 
 	ret = peel_onion(name, len, oid, lookup_flags);
 	if (!ret)
-		return 0;
+		return FOUND;
 
 	ret = get_oid_basic(name, len, oid, lookup_flags);
 	if (!ret)
-		return 0;
+		return FOUND;
 
 	/* It could be describe output that is "SOMETHING-gXXXX" */
 	ret = get_describe_name(name, len, oid);
 	if (!ret)
-		return 0;
+		return FOUND;
 
 	return get_short_oid(name, len, oid, lookup_flags);
 }
@@ -1664,11 +1666,11 @@ static char *resolve_relative_path(const char *rel)
 			   rel);
 }
 
-static int get_oid_with_context_1(const char *name,
-				  unsigned flags,
-				  const char *prefix,
-				  struct object_id *oid,
-				  struct object_context *oc)
+static enum get_oid_result get_oid_with_context_1(const char *name,
+						  unsigned flags,
+						  const char *prefix,
+						  struct object_id *oid,
+						  struct object_context *oc)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
diff --git a/tree-walk.c b/tree-walk.c
index 08210a4109..fb7959b21d 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -579,10 +579,10 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob
  * with the sha1 of the found object, and *mode will hold the mode of
  * the object.
  *
- * See the code for enum follow_symlink_result for a description of
+ * See the code for enum get_oid_result for a description of
  * the return values.
  */
-enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned *mode)
+enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned *mode)
 {
 	int retval = MISSING_OBJECT;
 	struct dir_state *parents = NULL;
diff --git a/tree-walk.h b/tree-walk.h
index eefd26bb62..de6b95179d 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -51,23 +51,7 @@ struct traverse_info;
 typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *);
 int traverse_trees(struct index_state *istate, int n, struct tree_desc *t, struct traverse_info *info);
 
-enum follow_symlinks_result {
-	FOUND = 0, /* This includes out-of-tree links */
-	MISSING_OBJECT = -1, /* The initial symlink is missing */
-	DANGLING_SYMLINK = -2, /*
-				* The initial symlink is there, but
-				* (transitively) points to a missing
-				* in-tree file
-				*/
-	SYMLINK_LOOP = -3,
-	NOT_DIR = -4, /*
-		       * Somewhere along the symlink chain, a path is
-		       * requested which contains a file as a
-		       * non-final element.
-		       */
-};
-
-enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned *mode);
+enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned *mode);
 
 struct traverse_info {
 	const char *traverse_path;
-- 
2.19.1


  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-18  3:38 Eric Wong
2019-01-18  4:46 ` David Turner
2019-01-18 10:45   ` Eric Wong
2019-01-18 16:20     ` David Turner [this message]
2019-01-18 22:26       ` [PATCH] t1512: test ambiguous cat-file --batch and --batch-output Eric Wong
2019-01-18 22:29         ` David Turner
2019-01-23 11:11         ` SZEDER Gábor
2019-01-30 21:33           ` [PATCH v2] " Eric Wong

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=672a6fb9e480becbfcb5df23ae37193784811b6b.camel@novalis.org \
    --to=novalis@novalis.org \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox