Hi Jonathan,
Jonathan Nieder writes:
> 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?
I didn't write commit messages for any of the patches in the previous
round -- I just wanted to show the idea quickly. Anyway, it's fixed
in the next round (coming soon).
> > --- 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?
Fixed.
> > } 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. ;-)
Fixed.
> [...]
> > @@ -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?
Fixed.
> [...]
> > @@ -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 :)).
Hm, I'm a little confused about error handling now. I'll defer this
part until my patches naturally establish some convention for error
handling -- designing one in advance isn't as easy as I thought.
Finally, thanks for the review! I'll look forward to more reviews as
I post more iterations of the series.
-- Ram
--
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