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

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:

Ramkumar Ramachandra wrote:

quoted

@@ -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 <redacted>

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