Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 19, 2011, 10:39 UTC
Message-ID: <20110519103938.GA2949@domU-12-31-39-06-A8-0A.compute-1.internal>
In-Reply-To: <20110511095949.GA2676@elie>

Hi,

I'm preparing a large series dedicated to solving error-handling issues before getting to the sequencer series. I plan to post some quick-and-dirty diffs of various things and ask for feedback.

Jonathan Nieder writes:

Show 32 quoted lines

> Ramkumar Ramachandra wrote:
> > @@ -418,19 +434,19 @@ static int do_pick_commit(void)
> >             struct commit_list *p;
> >
> >             if (!mainline)
> > -                   die(_("Commit %s is a merge but no -m option was given."),
> > -                       sha1_to_hex(commit->object.sha1));
> > +                   return error(_("Commit %s is a merge but no -m option was given."),
> > +                           sha1_to_hex(commit->object.sha1));
>
> This is not a conflict but an error in usage.  Stepping back for a
> second, what is the best way to handle that?  The aforementioned
> hypothetical caller considering "try an alternate strategy" would make
> a wrong move, so we need to use a return value that would dissuade it.
>
> I’m not sure what the most appropriate thing to do is.  Maybe
>
>       /*
>        * Positive numbers are exit status from conflicts; negative
>        * numbers are other errors.
>        */
>       enum pick_commit_error {
>               PICK_COMMIT_USAGE_ERROR = -1
>       };
>
> but it’s hard to think clearly about it since it seems too
> hypothetical to me.  If all callers are going to exit, then
>
>       error(...);
>       return 129;
>
> will work, but in that case why not exit for them?

For this part, I think the correct way to handle the usage error is to print a message like this:

   usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.

And exit with status 129. Is this acceptable?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

@@ -418,20 +424,20 @@ static int do_pick_commit(void)
               struct commit_list *p;

               if (!mainline)
-                       die(_("Commit %s is a merge but no -m option was given."),
-                           sha1_to_hex(commit->object.sha1));
+                       usage(_("%s: Commit %s is a merge but no -m option was given."),
+                               me, sha1_to_hex(commit->object.sha1));

               for (cnt = 1, p = commit->parents;
                    cnt != mainline && p;
                    cnt++)
                       p = p->next;
               if (cnt != mainline || !p)
-                       die(_("Commit %s does not have parent %d"),
-                           sha1_to_hex(commit->object.sha1), mainline);
+                       usage(_("%s: Commit %s does not have parent %d"),
+                               me, sha1_to_hex(commit->object.sha1), mainline);
               parent = p->item;
-       } else if (0 < mainline)
-               die(_("Mainline was specified but commit %s is not a merge."),
-                   sha1_to_hex(commit->object.sha1));
+       } else if (mainline > 0)
+               usage(_("%s: Mainline was specified but commit %s is not a merge."),
+                       me, sha1_to_hex(commit->object.sha1));