threads / rfc / 48896

RFC patch, 6 partssha1-name: Teach `get_oid_with_context[_1]()` to be gentle

Subject: [RFC PATCH 5/6] sha1-name: Teach `get_oid_with_context[_1]()` to be gentle

## tl;dr

13 messages between Jul 17, 2018 and Jul 18, 2018. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · lore

[RFC PATCH 0/6] Add gentle alternative for `get_oid()`

At the moment, `get_oid()` might call `die()` in some cases. To prevent that from happening, this patches introduces a new flag called `GET_OID_GENTLY` and a new function `get_oid_gently()`, which passes the mentioned flag further to `get_oid_with_context()`.

The call graph of `get_oid()` is pretty complex and I hope I covered all the cases where `exit()` might be called. Although I believe this series of patches will not introduce any regression and work as intended, I think that is better to mark it with [RFC].

This patch would be useful for converting `git stash` to C. At the moment, `git stash` spawns a child process to avoid `get_oid()` to die. If this series turns out to be good enough to be accepted, do I need to wait until it gets merged in `master` to use it in the other project mentioned before?

Thanks, Paul

Paul-Sebastian Ungureanu (6):
  sha1-name: Add `GET_OID_GENTLY` flag
  tree-walk: Add three new gentle helpers
  refs.c: Teach `read_ref_at()` to accept `GET_OID_GENTLY` flag
  sha1-name: Teach `get_oid_basic()` to be gentle
  sha1-name: Teach `get_oid_with_context[_1]()` to be gentle
  sha1-name: Add gentle alternative for `get_oid()`
 cache.h     |   2 +
 refs.c      |   2 +
 sha1-name.c | 127 +++++++++++++++++++++++++++++++++++++++++-----------
 tree-walk.c | 108 +++++++++++++++++++++++++++++++++++++-------
 tree-walk.h |   3 +-
 5 files changed, 199 insertions(+), 43 deletions(-)
-- 
2.18.0.rc2.184.ga79db55c2.dirty
Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

This commit makes `get_oid_with_context()` and `get_oid_with_context_1()` to recognize the `GET_OID_GENTLY` flag.

The `gentle` flag does not imply `quiet` and we might need to reconsider whether we should display any message if `GET_OID_GENTLY` is given.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 sha1-name.c | 103 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 20 deletions(-)
Show changes to sha1-name.c +83 −20
diff --git a/sha1-name.c b/sha1-name.c
index 74ecbd550..a5d4e0dc7 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1521,11 +1521,12 @@ int get_oid_blob(const char *name, struct object_id *oid)
 }
 
 /* Must be called only when object_name:filename doesn't exist. */
-static void diagnose_invalid_oid_path(const char *prefix,
+static int diagnose_invalid_oid_path(const char *prefix,
 				      const char *filename,
 				      const struct object_id *tree_oid,
 				      const char *object_name,
-				      int object_name_len)
+				      int object_name_len,
+				      int gentle)
 {
 	struct object_id oid;
 	unsigned mode;
@@ -1533,12 +1534,19 @@ static void diagnose_invalid_oid_path(const char *prefix,
 	if (!prefix)
 		prefix = "";
 
-	if (file_exists(filename))
+	if (file_exists(filename)) {
+		if (gentle)
+			return -1;
 		die("Path '%s' exists on disk, but not in '%.*s'.",
 		    filename, object_name_len, object_name);
+	}
 	if (is_missing_file_error(errno)) {
 		char *fullname = xstrfmt("%s%s", prefix, filename);
-
+		if (gentle) {
+			warning(_("%s or %s does not exist."), fullname,
+				filename);
+			return -1;
+		}
 		if (!get_tree_entry(tree_oid, fullname, &oid, &mode)) {
 			die("Path '%s' exists, but not '%s'.\n"
 			    "Did you mean '%.*s:%s' aka '%.*s:./%s'?",
@@ -1552,12 +1560,14 @@ static void diagnose_invalid_oid_path(const char *prefix,
 		die("Path '%s' does not exist in '%.*s'",
 		    filename, object_name_len, object_name);
 	}
+	return 0;
 }
 
 /* Must be called only when :stage:filename doesn't exist. */
-static void diagnose_invalid_index_path(int stage,
+static int diagnose_invalid_index_path(int stage,
 					const char *prefix,
-					const char *filename)
+					const char *filename,
+					int gentle)
 {
 	const struct cache_entry *ce;
 	int pos;
@@ -1574,11 +1584,20 @@ static void diagnose_invalid_index_path(int stage,
 	if (pos < active_nr) {
 		ce = active_cache[pos];
 		if (ce_namelen(ce) == namelen &&
-		    !memcmp(ce->name, filename, namelen))
+		    !memcmp(ce->name, filename, namelen)) {
+			if (gentle) {
+				warning("Path '%s' is in the index "
+					"but not at stage %d.\n"
+					"Did you mean ':%d:%s'?",
+					filename, stage,
+					ce_stage(ce), filename);
+				return -1;
+			}
 			die("Path '%s' is in the index, but not at stage %d.\n"
 			    "Did you mean ':%d:%s'?",
 			    filename, stage,
 			    ce_stage(ce), filename);
+		}
 	}
 
 	/* Confusion between relative and absolute filenames? */
@@ -1590,31 +1609,58 @@ static void diagnose_invalid_index_path(int stage,
 	if (pos < active_nr) {
 		ce = active_cache[pos];
 		if (ce_namelen(ce) == fullname.len &&
-		    !memcmp(ce->name, fullname.buf, fullname.len))
+		    !memcmp(ce->name, fullname.buf, fullname.len)) {
+			if (gentle)
+				return -1;
 			die("Path '%s' is in the index, but not '%s'.\n"
 			    "Did you mean ':%d:%s' aka ':%d:./%s'?",
 			    fullname.buf, filename,
 			    ce_stage(ce), fullname.buf,
 			    ce_stage(ce), filename);
+		}
 	}
 
-	if (file_exists(filename))
+	if (file_exists(filename)) {
+		if (gentle)
+			return -1;
 		die("Path '%s' exists on disk, but not in the index.", filename);
-	if (is_missing_file_error(errno))
+	}
+	if (is_missing_file_error(errno)) {
+		if (gentle)
+			return -1;
 		die("Path '%s' does not exist (neither on disk nor in the index).",
 		    filename);
+	}
 
 	strbuf_release(&fullname);
+	return 0;
 }
 
+static const char *resolve_error = "dummy";
 
-static char *resolve_relative_path(const char *rel)
+static char *resolve_relative_path_gently(const char *rel, int gentle)
 {
 	if (!starts_with(rel, "./") && !starts_with(rel, "../"))
 		return NULL;
 
-	if (!is_inside_work_tree())
+	if (!is_inside_work_tree()) {
+		/*
+		 * `resolve_error` is a dummy variable and it is used to verify
+		 * if there was any problem inside this function. This is
+		 * returned only in the case we want to perform gently,
+		 * otherwise, `exit()` or `die()` can be called.
+		 */
+		if (gentle)
+			return (char*) resolve_error;
 		die("relative path syntax can't be used outside working tree.");
+	}
+
+	if (gentle) {
+		return prefix_path_gently(startup_info->prefix,
+					  startup_info->prefix ?
+					  strlen(startup_info->prefix) : 0,
+					  NULL, rel);
+	}
 
 	/* die() inside prefix_path() if resolved path is outside worktree */
 	return prefix_path(startup_info->prefix,
@@ -1669,7 +1715,16 @@ static int get_oid_with_context_1(const char *name,
 			stage = name[1] - '0';
 			cp = name + 3;
 		}
-		new_path = resolve_relative_path(cp);
+		/*
+		 * Note that `resolve_relative_path_gently()` may die if
+		 * the second parameter is zero. If it is a non-zero value,
+		 * the function will return `resolve_error` on failure. This
+		 * dummy variable is defined as a `static const char *`.
+		 */
+		new_path = resolve_relative_path_gently(cp, flags & GET_OID_GENTLY);
+		if (new_path == resolve_error)
+			return -1;
+
 		if (!new_path) {
 			namelen = namelen - (cp - name);
 		} else {
@@ -1698,8 +1753,11 @@ static int get_oid_with_context_1(const char *name,
 			}
 			pos++;
 		}
-		if (only_to_die && name[1] && name[1] != '/')
-			diagnose_invalid_index_path(stage, prefix, cp);
+		if (only_to_die && name[1] && name[1] != '/' &&
+			diagnose_invalid_index_path(stage, prefix, cp,
+						    flags & GET_OID_GENTLY))
+			return -1;
+
 		free(new_path);
 		return -1;
 	}
@@ -1723,7 +1781,10 @@ static int get_oid_with_context_1(const char *name,
 			const char *filename = cp+1;
 			char *new_filename = NULL;
 
-			new_filename = resolve_relative_path(filename);
+			new_filename = resolve_relative_path_gently(filename,
+								    flags & GET_OID_GENTLY);
+			if (new_filename == resolve_error)
+				return -1;
 			if (new_filename)
 				filename = new_filename;
 			if (flags & GET_OID_FOLLOW_SYMLINKS) {
@@ -1731,13 +1792,14 @@ static int get_oid_with_context_1(const char *name,
 					filename, oid, &oc->symlink_path,
 					&oc->mode, flags);
 			} else {
-				ret = get_tree_entry(&tree_oid, filename, oid,
-						     &oc->mode);
+				ret = get_tree_entry_gently(&tree_oid, filename,
+							    oid, &oc->mode,
+							    flags & GET_OID_GENTLY);
 				if (ret && only_to_die) {
 					diagnose_invalid_oid_path(prefix,
 								   filename,
 								   &tree_oid,
-								   name, len);
+								   name, len, 0);
 				}
 			}
 			if (flags & GET_OID_RECORD_PATH)
@@ -1769,7 +1831,8 @@ void maybe_die_on_misspelt_object_name(const char *name, const char *prefix)
 
 int get_oid_with_context(const char *str, unsigned flags, struct object_id *oid, struct object_context *oc)
 {
-	if (flags & GET_OID_FOLLOW_SYMLINKS && flags & GET_OID_ONLY_TO_DIE)
+	if (flags & (GET_OID_FOLLOW_SYMLINKS | GET_OID_GENTLY) &&
+	    flags & GET_OID_ONLY_TO_DIE)
 		BUG("incompatible flags for get_sha1_with_context");
 	return get_oid_with_context_1(str, flags, NULL, oid, oc);
 }

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Junio C Hamano· Jul 17, 2018, 19:13 UTC · re: Paul-Sebastian Ungureanu · lore

Re: [RFC PATCH 5/6] sha1-name: Teach `get_oid_with_context[_1]()` to be gentle

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
Show 10 quoted lines
> @@ -1769,7 +1831,8 @@ void maybe_die_on_misspelt_object_name(const char *name, const char *prefix)
>  
>  int get_oid_with_context(const char *str, unsigned flags, struct object_id *oid, struct object_context *oc)
>  {
> -	if (flags & GET_OID_FOLLOW_SYMLINKS && flags & GET_OID_ONLY_TO_DIE)
> +	if (flags & (GET_OID_FOLLOW_SYMLINKS | GET_OID_GENTLY) &&
> +	    flags & GET_OID_ONLY_TO_DIE)
>  		BUG("incompatible flags for get_sha1_with_context");
>  	return get_oid_with_context_1(str, flags, NULL, oid, oc);
>  }

This points us back to "only-to-die" which was "gently" before 2e83b66c ("fix overslow :/no-such-string-ever-existed diagnostics", 2011-05-10). I think we have to keep them both, as only-to-die means more than just being not gentle, and we cannot revert the renaming s/!gently/only-to-die/ done by 2e83b66c and teach GENTLY to more codepaths, I think. But I might be mistaken and we may be able to get rid of only-to-die at the end of this series. I dunno.

In any case, what's the reason why this new "gentle" option is incompatible with "only-to-die"?

Paul-Sebastian Ungureanu· Jul 18, 2018, 23:14 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 5/6] sha1-name: Teach `get_oid_with_context[_1]()` to be gentle

Hello,
Show 10 quoted lines
> This points us back to "only-to-die" which was "gently" before
> 2e83b66c ("fix overslow :/no-such-string-ever-existed diagnostics",
> 2011-05-10).  I think we have to keep them both, as only-to-die
> means more than just being not gentle, and we cannot revert the
> renaming s/!gently/only-to-die/ done by 2e83b66c and teach GENTLY to
> more codepaths, I think.  But I might be mistaken and we may be able
> to get rid of only-to-die at the end of this series.  I dunno.
> 
> In any case, what's the reason why this new "gentle" option is
> incompatible with "only-to-die"?

"GET_OID_GENTLY" would be used in the case we want to handle fatal errors, while "GET_OID_ONLY_TO_DIE" means that if there is any fatal error, we can just exit. They are not compatible because that would mean that if there is any fatal error, the program should die and not die at the same time.

Thank you for taking time to look over these patches!

Best, Paul

Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

[RFC PATCH 6/6] sha1-name: Add gentle alternative for `get_oid()`

Add `get_oid_gently()` to be a gentle alternative to `get_oid()`.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 cache.h     | 1 +
 sha1-name.c | 6 ++++++
 2 files changed, 7 insertions(+)
Show changes to 2 files +7 −0

cache.h, sha1-name.c

diff --git a/cache.h b/cache.h
index cb8803e2f..36e196202 100644
--- a/cache.h
+++ b/cache.h
@@ -1321,6 +1321,7 @@ struct object_context {
 	GET_OID_TREE | GET_OID_TREEISH | \
 	GET_OID_BLOB)
 
+extern int get_oid_gently(const char *str, struct object_id *oid);
 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);
diff --git a/sha1-name.c b/sha1-name.c
index a5d4e0dc7..6ee48fdf7 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1474,6 +1474,12 @@ int get_oid(const char *name, struct object_id *oid)
 	return get_oid_with_context(name, 0, oid, &unused);
 }
 
+int get_oid_gently(const char *name, struct object_id *oid)
+{
+	struct object_context unused;
+	return get_oid_with_context(name, GET_OID_GENTLY, oid, &unused);
+}
+
 
 /*
  * Many callers know that the user meant to name a commit-ish by

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

[RFC PATCH 4/6] sha1-name: Teach `get_oid_basic()` to be gentle

After teaching `read_ref_at()` we need to teach `get_oid_basic()` that `read_ref_at()` might not call `exit()`, but report an error through the return value.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 sha1-name.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
Show changes to sha1-name.c +12 −4
diff --git a/sha1-name.c b/sha1-name.c
index d741e1129..74ecbd550 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -778,6 +778,7 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 		timestamp_t at_time;
 		timestamp_t co_time;
 		int co_tz, co_cnt;
+		int ret;
 
 		/* Is it asking for N-th entry, or approxidate? */
 		for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
@@ -802,8 +803,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 				return -1;
 			}
 		}
-		if (read_ref_at(real_ref, flags, at_time, nth, oid, NULL,
-				&co_time, &co_tz, &co_cnt)) {
+
+		ret = read_ref_at(real_ref, flags, at_time, nth, oid, NULL,
+				&co_time, &co_tz, &co_cnt);
+		if (ret == -1)
+			return -1;
+		if (ret) {
 			if (!len) {
 				if (starts_with(real_ref, "refs/heads/")) {
 					str = real_ref + 11;
@@ -821,9 +826,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
 			} else {
-				if (flags & GET_OID_QUIETLY) {
-					exit(128);
+				if (flags & GET_OID_GENTLY) {
+					free(real_ref);
+					return -1;
 				}
+				if (flags & GET_OID_QUIETLY)
+					exit(128);
 				die("Log for '%.*s' only has %d entries.",
 				    len, str, co_cnt);
 			}

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

[RFC PATCH 3/6] refs.c: Teach `read_ref_at()` to accept `GET_OID_GENTLY` flag

This commit introduces a way to call `read_ref_at()` without exiting on failure.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to refs.c +2 −0
diff --git a/refs.c b/refs.c
index 0eb379f93..4a470158e 100644
--- a/refs.c
+++ b/refs.c
@@ -932,6 +932,8 @@ int read_ref_at(const char *refname, unsigned int flags, timestamp_t at_time, in
 	for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
 
 	if (!cb.reccnt) {
+		if (flags & GET_OID_GENTLY)
+			return -1;
 		if (flags & GET_OID_QUIETLY)
 			exit(128);
 		else

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

[RFC PATCH 2/6] tree-walk: Add three new gentle helpers

Add `get_tree_entry_gently()`, `find_tree_entry_gently()` and `get_tree_entry_follow_symlinks_gently()`, which will make `get_oid()` to be more gently.

Since `get_tree_entry()` is used in more than 20 places, adding a new parameter will make this commit harder to read. In every place it is called there will need to be an additional 0 parameter at the end of the call. The solution to avoid this is to rename the function in `get_tree_entry_gently()` which gets an additional `flags` variable. A new `get_tree_entry()` will call `get_tree_entry_gently()` with `flags` being 0. This way, no additional changes will be needed.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 sha1-name.c |   2 +-
 tree-walk.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
 tree-walk.h |   3 +-
 3 files changed, 94 insertions(+), 19 deletions(-)
Show changes to 3 files +94 −19

sha1-name.c, tree-walk.c, tree-walk.h

diff --git a/sha1-name.c b/sha1-name.c
index 60d9ef3c7..d741e1129 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1721,7 +1721,7 @@ static int get_oid_with_context_1(const char *name,
 			if (flags & GET_OID_FOLLOW_SYMLINKS) {
 				ret = get_tree_entry_follow_symlinks(&tree_oid,
 					filename, oid, &oc->symlink_path,
-					&oc->mode);
+					&oc->mode, flags);
 			} else {
 				ret = get_tree_entry(&tree_oid, filename, oid,
 						     &oc->mode);
diff --git a/tree-walk.c b/tree-walk.c
index 8f5090862..2925eaec2 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -491,7 +491,9 @@ struct dir_state {
 	struct object_id oid;
 };
 
-static int find_tree_entry(struct tree_desc *t, const char *name, struct object_id *result, unsigned *mode)
+static int find_tree_entry(struct tree_desc *t, const char *name,
+				  struct object_id *result, unsigned *mode,
+				  int flags)
 {
 	int namelen = strlen(name);
 	while (t->size) {
@@ -501,7 +503,11 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_
 
 		oid = tree_entry_extract(t, &entry, mode);
 		entrylen = tree_entry_len(&t->entry);
-		update_tree_entry(t);
+
+		if (!(flags & GET_OID_GENTLY))
+			update_tree_entry(t);
+		else if (update_tree_entry_gently(t))
+			return -1;
 		if (entrylen > namelen)
 			continue;
 		cmp = memcmp(name, entry, entrylen);
@@ -521,19 +527,28 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_
 			oidcpy(result, oid);
 			return 0;
 		}
-		return get_tree_entry(oid, name + entrylen, result, mode);
+		return get_tree_entry_gently(oid, name + entrylen, result, mode, flags);
 	}
 	return -1;
 }
 
-int get_tree_entry(const struct object_id *tree_oid, const char *name, struct object_id *oid, unsigned *mode)
+int get_tree_entry_gently(const struct object_id *tree_oid, const char *name,
+			  struct object_id *oid, unsigned *mode, int flags)
 {
 	int retval;
 	void *tree;
 	unsigned long size;
 	struct object_id root;
 
-	tree = read_object_with_reference(tree_oid, tree_type, &size, &root);
+	if (!(flags & GET_OID_GENTLY)) {
+		tree = read_object_with_reference(tree_oid, tree_type, &size, &root);
+	} else {
+		struct object_info oi = OBJECT_INFO_INIT;
+
+		oi.contentp = tree;
+		if (oid_object_info_extended(the_repository, tree_oid, &oi, 0) < 0)
+			return -1;
+	}
 	if (!tree)
 		return -1;
 
@@ -547,13 +562,27 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob
 		retval = -1;
 	} else {
 		struct tree_desc t;
-		init_tree_desc(&t, tree, size);
-		retval = find_tree_entry(&t, name, oid, mode);
+		if (!(flags & GET_OID_GENTLY)) {
+			init_tree_desc(&t, tree, size);
+		} else {
+			if (init_tree_desc_gently(&t, tree, size)) {
+				retval = -1;
+				goto done;
+			}
+		}
+		retval = find_tree_entry(&t, name, oid, mode, flags);
 	}
+done:
 	free(tree);
 	return retval;
 }
 
+int get_tree_entry(const struct object_id *tree_oid, const char *name,
+		   struct object_id *oid, unsigned *mode)
+{
+	return get_tree_entry_gently(tree_oid, name, oid, mode, 0);
+}
+
 /*
  * This is Linux's built-in max for the number of symlinks to follow.
  * That limit, of course, does not affect git, but it's a reasonable
@@ -576,7 +605,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob
  * See the code for enum follow_symlink_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 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, int flags)
 {
 	int retval = MISSING_OBJECT;
 	struct dir_state *parents = NULL;
@@ -600,9 +629,21 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 			void *tree;
 			struct object_id root;
 			unsigned long size;
-			tree = read_object_with_reference(&current_tree_oid,
-							  tree_type, &size,
-							  &root);
+			if (!(flags & GET_OID_GENTLY)) {
+				tree = read_object_with_reference(&current_tree_oid,
+								  tree_type, &size,
+								  &root);
+			} else {
+				struct object_info oi = OBJECT_INFO_INIT;
+
+				oi.contentp = tree;
+				if (oid_object_info_extended(the_repository,
+				    &current_tree_oid, &oi, 0) < 0) {
+					retval = MISSING_OBJECT;
+					goto done;
+				}
+			}
+
 			if (!tree)
 				goto done;
 
@@ -622,7 +663,14 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 				goto done;
 
 			/* descend */
-			init_tree_desc(&t, tree, size);
+			if (!(flags & GET_OID_GENTLY)) {
+				init_tree_desc(&t, tree, size);
+			} else {
+				if (init_tree_desc_gently(&t, tree, size)) {
+					retval = MISSING_OBJECT;
+					goto done;
+				}
+			}
 		}
 
 		/* Handle symlinks to e.g. a//b by removing leading slashes */
@@ -656,7 +704,15 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 			free(parent->tree);
 			parents_nr--;
 			parent = &parents[parents_nr - 1];
-			init_tree_desc(&t, parent->tree, parent->size);
+			if (!(flags & GET_OID_GENTLY)) {
+				init_tree_desc(&t, parent->tree, parent->size);
+			} else {
+				if (init_tree_desc_gently(&t, parent->tree,
+				    parent->size)) {
+					retval = MISSING_OBJECT;
+					goto done;
+				}
+			}
 			strbuf_remove(&namebuf, 0, remainder ? 3 : 2);
 			continue;
 		}
@@ -670,7 +726,7 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 
 		/* Look up the first (or only) path component in the tree. */
 		find_result = find_tree_entry(&t, namebuf.buf,
-					      &current_tree_oid, mode);
+					      &current_tree_oid, mode, flags);
 		if (find_result) {
 			goto done;
 		}
@@ -713,8 +769,19 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 			 */
 			retval = DANGLING_SYMLINK;
 
-			contents = read_object_file(&current_tree_oid, &type,
-						    &link_len);
+			if (!(flags & GET_OID_GENTLY)) {
+				contents = read_object_file(&current_tree_oid,
+							    &type, &link_len);
+			} else {
+				struct object_info oi = OBJECT_INFO_INIT;
+				oi.contentp = (void*) contents;
+
+				if (oid_object_info_extended(the_repository,
+				    &current_tree_oid, &oi, 0) < 0) {
+					retval = MISSING_OBJECT;
+					goto done;
+				}
+			}
 
 			if (!contents)
 				goto done;
@@ -735,7 +802,14 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
 			contents_start = contents;
 
 			parent = &parents[parents_nr - 1];
-			init_tree_desc(&t, parent->tree, parent->size);
+			if (!(flags & GET_OID_GENTLY)) {
+				init_tree_desc(&t, parent->tree, parent->size);
+			} else {
+				if (init_tree_desc_gently(&t, parent->tree, parent->size)) {
+					retval = MISSING_OBJECT;
+					goto done;
+				}
+			}
 			strbuf_splice(&namebuf, 0, len,
 				      contents_start, link_len);
 			if (remainder)
diff --git a/tree-walk.h b/tree-walk.h
index 805f58f00..6f043af6e 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -64,7 +64,7 @@ enum follow_symlinks_result {
 		       */
 };
 
-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 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, int flags);
 
 struct traverse_info {
 	const char *traverse_path;
@@ -79,6 +79,7 @@ struct traverse_info {
 	int show_all_errors;
 };
 
+int get_tree_entry_gently(const struct object_id *, const char *, struct object_id *, unsigned *, int);
 int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned *);
 extern char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n);
 extern void setup_traverse_info(struct traverse_info *info, const char *base);

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Junio C Hamano· Jul 17, 2018, 18:55 UTC · re: Paul-Sebastian Ungureanu · lore

Re: [RFC PATCH 2/6] tree-walk: Add three new gentle helpers

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
Show 12 quoted lines
> Add `get_tree_entry_gently()`, `find_tree_entry_gently()`
> and `get_tree_entry_follow_symlinks_gently()`, which will
> make `get_oid()` to be more gently.
>
> Since `get_tree_entry()` is used in more than 20 places,
> adding a new parameter will make this commit harder to read.
> In every place it is called there will need to be an additional
> 0 parameter at the end of the call. The solution to avoid this is
> to rename the function in `get_tree_entry_gently()` which gets
> an additional `flags` variable. A new `get_tree_entry()`
> will call `get_tree_entry_gently()` with `flags` being 0.
> This way, no additional changes will be needed.

And that is the right way to introduce a new feature to existing API with many callers in general.

I wonder if the GENTLY option should apply to update_tree_entry() the same way as it would to the other codepaths that currently die to express "we were handed this string by the caller and told to give back object ID the string represents, and we found no good answer". In this one (and the "bad ref" one), the existing failures in these two codepaths are not "we got a string and that does not resolve to an object name", but "we didn't have the data to work on to begin with (either a corrupt tree object or a corrupt ref").

In other words, it's not like "We were given HEAD:no-such-path and there is no such path in that tree"; it is "We tried to read HEAD: tree for no-such-path in it, but the tree was corrupt and we couldn't even tell if such a path is or is not in it", no?

Show 245 quoted lines
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---
>  sha1-name.c |   2 +-
>  tree-walk.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
>  tree-walk.h |   3 +-
>  3 files changed, 94 insertions(+), 19 deletions(-)
>
> diff --git a/sha1-name.c b/sha1-name.c
> index 60d9ef3c7..d741e1129 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1721,7 +1721,7 @@ static int get_oid_with_context_1(const char *name,
>  			if (flags & GET_OID_FOLLOW_SYMLINKS) {
>  				ret = get_tree_entry_follow_symlinks(&tree_oid,
>  					filename, oid, &oc->symlink_path,
> -					&oc->mode);
> +					&oc->mode, flags);
>  			} else {
>  				ret = get_tree_entry(&tree_oid, filename, oid,
>  						     &oc->mode);
> diff --git a/tree-walk.c b/tree-walk.c
> index 8f5090862..2925eaec2 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -491,7 +491,9 @@ struct dir_state {
>  	struct object_id oid;
>  };
>  
> -static int find_tree_entry(struct tree_desc *t, const char *name, struct object_id *result, unsigned *mode)
> +static int find_tree_entry(struct tree_desc *t, const char *name,
> +				  struct object_id *result, unsigned *mode,
> +				  int flags)
>  {
>  	int namelen = strlen(name);
>  	while (t->size) {
> @@ -501,7 +503,11 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_
>  
>  		oid = tree_entry_extract(t, &entry, mode);
>  		entrylen = tree_entry_len(&t->entry);
> -		update_tree_entry(t);
> +
> +		if (!(flags & GET_OID_GENTLY))
> +			update_tree_entry(t);
> +		else if (update_tree_entry_gently(t))
> +			return -1;
>  		if (entrylen > namelen)
>  			continue;
>  		cmp = memcmp(name, entry, entrylen);
> @@ -521,19 +527,28 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_
>  			oidcpy(result, oid);
>  			return 0;
>  		}
> -		return get_tree_entry(oid, name + entrylen, result, mode);
> +		return get_tree_entry_gently(oid, name + entrylen, result, mode, flags);
>  	}
>  	return -1;
>  }
>  
> -int get_tree_entry(const struct object_id *tree_oid, const char *name, struct object_id *oid, unsigned *mode)
> +int get_tree_entry_gently(const struct object_id *tree_oid, const char *name,
> +			  struct object_id *oid, unsigned *mode, int flags)
>  {
>  	int retval;
>  	void *tree;
>  	unsigned long size;
>  	struct object_id root;
>  
> -	tree = read_object_with_reference(tree_oid, tree_type, &size, &root);
> +	if (!(flags & GET_OID_GENTLY)) {
> +		tree = read_object_with_reference(tree_oid, tree_type, &size, &root);
> +	} else {
> +		struct object_info oi = OBJECT_INFO_INIT;
> +
> +		oi.contentp = tree;
> +		if (oid_object_info_extended(the_repository, tree_oid, &oi, 0) < 0)
> +			return -1;
> +	}
>  	if (!tree)
>  		return -1;
>  
> @@ -547,13 +562,27 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob
>  		retval = -1;
>  	} else {
>  		struct tree_desc t;
> -		init_tree_desc(&t, tree, size);
> -		retval = find_tree_entry(&t, name, oid, mode);
> +		if (!(flags & GET_OID_GENTLY)) {
> +			init_tree_desc(&t, tree, size);
> +		} else {
> +			if (init_tree_desc_gently(&t, tree, size)) {
> +				retval = -1;
> +				goto done;
> +			}
> +		}
> +		retval = find_tree_entry(&t, name, oid, mode, flags);
>  	}
> +done:
>  	free(tree);
>  	return retval;
>  }
>  
> +int get_tree_entry(const struct object_id *tree_oid, const char *name,
> +		   struct object_id *oid, unsigned *mode)
> +{
> +	return get_tree_entry_gently(tree_oid, name, oid, mode, 0);
> +}
> +
>  /*
>   * This is Linux's built-in max for the number of symlinks to follow.
>   * That limit, of course, does not affect git, but it's a reasonable
> @@ -576,7 +605,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob
>   * See the code for enum follow_symlink_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 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, int flags)
>  {
>  	int retval = MISSING_OBJECT;
>  	struct dir_state *parents = NULL;
> @@ -600,9 +629,21 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  			void *tree;
>  			struct object_id root;
>  			unsigned long size;
> -			tree = read_object_with_reference(&current_tree_oid,
> -							  tree_type, &size,
> -							  &root);
> +			if (!(flags & GET_OID_GENTLY)) {
> +				tree = read_object_with_reference(&current_tree_oid,
> +								  tree_type, &size,
> +								  &root);
> +			} else {
> +				struct object_info oi = OBJECT_INFO_INIT;
> +
> +				oi.contentp = tree;
> +				if (oid_object_info_extended(the_repository,
> +				    &current_tree_oid, &oi, 0) < 0) {
> +					retval = MISSING_OBJECT;
> +					goto done;
> +				}
> +			}
> +
>  			if (!tree)
>  				goto done;
>  
> @@ -622,7 +663,14 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  				goto done;
>  
>  			/* descend */
> -			init_tree_desc(&t, tree, size);
> +			if (!(flags & GET_OID_GENTLY)) {
> +				init_tree_desc(&t, tree, size);
> +			} else {
> +				if (init_tree_desc_gently(&t, tree, size)) {
> +					retval = MISSING_OBJECT;
> +					goto done;
> +				}
> +			}
>  		}
>  
>  		/* Handle symlinks to e.g. a//b by removing leading slashes */
> @@ -656,7 +704,15 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  			free(parent->tree);
>  			parents_nr--;
>  			parent = &parents[parents_nr - 1];
> -			init_tree_desc(&t, parent->tree, parent->size);
> +			if (!(flags & GET_OID_GENTLY)) {
> +				init_tree_desc(&t, parent->tree, parent->size);
> +			} else {
> +				if (init_tree_desc_gently(&t, parent->tree,
> +				    parent->size)) {
> +					retval = MISSING_OBJECT;
> +					goto done;
> +				}
> +			}
>  			strbuf_remove(&namebuf, 0, remainder ? 3 : 2);
>  			continue;
>  		}
> @@ -670,7 +726,7 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  
>  		/* Look up the first (or only) path component in the tree. */
>  		find_result = find_tree_entry(&t, namebuf.buf,
> -					      &current_tree_oid, mode);
> +					      &current_tree_oid, mode, flags);
>  		if (find_result) {
>  			goto done;
>  		}
> @@ -713,8 +769,19 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  			 */
>  			retval = DANGLING_SYMLINK;
>  
> -			contents = read_object_file(&current_tree_oid, &type,
> -						    &link_len);
> +			if (!(flags & GET_OID_GENTLY)) {
> +				contents = read_object_file(&current_tree_oid,
> +							    &type, &link_len);
> +			} else {
> +				struct object_info oi = OBJECT_INFO_INIT;
> +				oi.contentp = (void*) contents;
> +
> +				if (oid_object_info_extended(the_repository,
> +				    &current_tree_oid, &oi, 0) < 0) {
> +					retval = MISSING_OBJECT;
> +					goto done;
> +				}
> +			}
>  
>  			if (!contents)
>  				goto done;
> @@ -735,7 +802,14 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id *tre
>  			contents_start = contents;
>  
>  			parent = &parents[parents_nr - 1];
> -			init_tree_desc(&t, parent->tree, parent->size);
> +			if (!(flags & GET_OID_GENTLY)) {
> +				init_tree_desc(&t, parent->tree, parent->size);
> +			} else {
> +				if (init_tree_desc_gently(&t, parent->tree, parent->size)) {
> +					retval = MISSING_OBJECT;
> +					goto done;
> +				}
> +			}
>  			strbuf_splice(&namebuf, 0, len,
>  				      contents_start, link_len);
>  			if (remainder)
> diff --git a/tree-walk.h b/tree-walk.h
> index 805f58f00..6f043af6e 100644
> --- a/tree-walk.h
> +++ b/tree-walk.h
> @@ -64,7 +64,7 @@ enum follow_symlinks_result {
>  		       */
>  };
>  
> -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 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, int flags);
>  
>  struct traverse_info {
>  	const char *traverse_path;
> @@ -79,6 +79,7 @@ struct traverse_info {
>  	int show_all_errors;
>  };
>  
> +int get_tree_entry_gently(const struct object_id *, const char *, struct object_id *, unsigned *, int);
>  int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned *);
>  extern char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n);
>  extern void setup_traverse_info(struct traverse_info *info, const char *base);
Paul-Sebastian Ungureanu· Jul 18, 2018, 23:11 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 2/6] tree-walk: Add three new gentle helpers

Hello,
On 17.07.2018 21:55, Junio C Hamano wrote:
Show 13 quoted lines
> I wonder if the GENTLY option should apply to update_tree_entry()
> the same way as it would to the other codepaths that currently die
> to express "we were handed this string by the caller and told to
> give back object ID the string represents, and we found no good
> answer".  In this one (and the "bad ref" one), the existing failures
> in these two codepaths are not "we got a string and that does not
> resolve to an object name", but "we didn't have the data to work on
> to begin with (either a corrupt tree object or a corrupt ref").
> 
> In other words, it's not like "We were given HEAD:no-such-path and
> there is no such path in that tree"; it is "We tried to read HEAD:
> tree for no-such-path in it, but the tree was corrupt and we couldn't
> even tell if such a path is or is not in it", no?

I can definitely say there is a clear difference between these two cases, but I am not entirely sure how `GENTLY` should apply to `update_tree_entry()`.

On one side, even before this patch, there was the gentle version of this function, `update_tree_entry_gently()`, which could still die. And it makes sense. It should be ok to call `die()` when there was detected a "bigger" issue.

On the other side, in some cases like `read_ref_at()` I think that it could be useful if the caller could handle any error (which is what patches 3/6 and 4/6 try to accomplish).

I really do not know which way would be the best in this particular case.

Best, Paul

Paul-Sebastian Ungureanu· Jul 17, 2018, 12:06 UTC · re: Paul-Sebastian Ungureanu · lore

[RFC PATCH 1/6] sha1-name: Add `GET_OID_GENTLY` flag

The current API does not provide a method to call `get_oid()` and avoid `exit()` to be called. This commit intention is to introduce a flag in order to make `get_oid()` able to get the sha1 safely, without exiting the program.

Since `get_oid()` calls a lot of functions, which call other functions as well (and so on), there are a lot of cases in which `exit()` could be called. To make this idea more clear, here is one example, which could cause `get_oid()` to die.

  get_oid() -> get_oid_with_context() -> get_oid_with_context_1()
  -> get_oid_1() -> read_ref_at() -> exit()

Where `function1() -> function2()` means that `function1()` might call `function2()` at some point.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 cache.h | 1 +
 1 file changed, 1 insertion(+)
Show changes to cache.h +1 −0
diff --git a/cache.h b/cache.h
index d49092d94..cb8803e2f 100644
--- a/cache.h
+++ b/cache.h
@@ -1314,6 +1314,7 @@ struct object_context {
 #define GET_OID_FOLLOW_SYMLINKS 0100
 #define GET_OID_RECORD_PATH     0200
 #define GET_OID_ONLY_TO_DIE    04000
+#define GET_OID_GENTLY	      010000
 
 #define GET_OID_DISAMBIGUATORS \
 	(GET_OID_COMMIT | GET_OID_COMMITTISH | \

-- 
2.18.0.rc2.184.ga79db55c2.dirty
Duy Nguyen· Jul 17, 2018, 17:45 UTC · re: Paul-Sebastian Ungureanu · lore

Re: [RFC PATCH 0/6] Add gentle alternative for `get_oid()`

On Tue, Jul 17, 2018 at 2:10 PM Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> wrote:

Show 5 quoted lines
>
> At the moment, `get_oid()` might call `die()` in some cases. To
> prevent that from happening, this patches introduces a new flag
> called `GET_OID_GENTLY` and a new function `get_oid_gently()`,
> which passes the mentioned flag further to `get_oid_with_context()`.

Since get_oid() callers must handle failure when it returns non-zero, I would say "gently" is already implied by get_oid() and we could just convert those die() to error() or warning(). Unless some of those die() are very special that we need to choose which call sites should go "even gentler" where some sites should still die()?

-- 
Duy
Paul-Sebastian Ungureanu· Jul 18, 2018, 23:03 UTC · re: Duy Nguyen · lore

Re: [RFC PATCH 0/6] Add gentle alternative for `get_oid()`

Hello,
On 17.07.2018 20:45, Duy Nguyen wrote:
Show 5 quoted lines
> Since get_oid() callers must handle failure when it returns non-zero,
> I would say "gently" is already implied by get_oid() and we could just
> convert those die() to error() or warning(). Unless some of those
> die() are very special that we need to choose which call sites should
> go "even gentler" where some sites should still die()?
Of course, "gently" is already implied by `get_oid()` to some extent.
 From the beginning I tried to follow the safer method to do that.
Changing `die()` into `error()` or `warning()` and handling the
error in the caller function not only would mean a harder patch
to read, but could also introduce some regressions since some of
the functions in the call graph of `get_oid()` are used by other
functions as well.

I think that it might be a good idea, but I am not entirely sure. The codebase is pretty complex and this might make it harder to follow. I am not able to give a clear answer, but thank you for taking time to look over these patches!

Best, Paul

← back to recent threads