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

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

[Paul-Sebastian Ungureanu]

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

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

[Paul-Sebastian Ungureanu]

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(+)

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

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

[Paul-Sebastian Ungureanu]

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(-)

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

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

[Junio C Hamano]

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

> 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?

> 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);

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

[Paul-Sebastian Ungureanu]

Hello,

On 17.07.2018 21:55, Junio C Hamano wrote:
> 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

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

[Paul-Sebastian Ungureanu]

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(+)

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

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

[Paul-Sebastian Ungureanu]

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(-)

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

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

[Paul-Sebastian Ungureanu]

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(-)

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

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

[Junio C Hamano]

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

> @@ -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"?

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

[Paul-Sebastian Ungureanu]

Hello,

> 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

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

[Paul-Sebastian Ungureanu]

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(+)

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

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

[Duy Nguyen]

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

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

[Paul-Sebastian Ungureanu]

Hello,

On 17.07.2018 20:45, Duy Nguyen wrote:
> 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