Ramkumar Ramachandra wrote:
> [Subject: revert: Avoid calling die; return error instead]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
Presumably this is because the sequencer is going to pick up after the
error and clean up a little (why doesn't the change description say
so?). Will it be resuming after that or just performing a little
cleanup before the exit?
Might make sense, but:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)
> return tree;
> }
>
> -static NORETURN void die_dirty_index(const char *me)
> +static int error_dirty_index(const char *me)
> {
> if (read_cache_unmerged()) {
> die_resolve_conflict(me);
Won't that exit?
> } else {
This "else" could be removed (decreasing the indent of the rest by
one tab stop) since the "if" case has already returned or exited.
Not the subject of this patch, just an idea for earlier or later. ;-)
[...]
> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> (write_cache(index_fd, active_cache, active_nr) ||
> commit_locked_index(&index_lock)))
> /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> - die(_("%s: Unable to write new index file"), me);
> + return error(_("%s: Unable to write new index file"), me);
> rollback_lock_file(&index_lock);
What happens to index_lock in the error case?
[...]
> @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
> revs->reverse = 1;
>
> argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> - if (argc > 1)
> - usage(*revert_or_cherry_pick_usage());
> + if (argc > 1) {
> + fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> + return 129;
> + }
Yuck. Maybe the error can be returned to the caller somehow, but
that seems somehow ambitious given that setup_revisions has all sorts
of ways to die anyway.
So you are bending the assumptions of many existing git functions (in
a good way).
I can think of at least three ways to go:
1) Come up with a convention to give more information about the nature
of returned errors in the functions you are touching. For
example, make sure errno is valid after the relevant functions, or
use multiple negative values to express the nature of the error.
So a caller could do:
if (prepare_revs(...)) {
if (errno == EINVAL)
usage(*revert_or_cherry_pick_usage());
die("BUG: unexpected error from prepare_revs");
}
Or:
2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
has to happen before exiting. Keep using die().
3) Provide a new facility to register cleanup handlers that will free
resources and otherwise return to a consistent state before
unwinding the stack. This way, you'd still have to audit die()
calls to look for missing cleanup handlers, but they could stay as
die() rather than changing to "return error" and the worried
caller could use
set_die_routine(longjmp_to_here);
to keep git alive. I don't suggest doing this. It is a pain to
get right and not obviously cleaner than "return error", and some
errors really are unrecoverable (rather than just being a symptom
of programmers to lazy to write error recovery code :)).
Okay, I'm stopping here. Hope that helps.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html