Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 16, 2016, 16:45 UTC
Message-ID: <alpine.DEB.2.20.1605161830200.3303@virtualbox>
In-Reply-To: <CAPig+cQ=Gche_uO840wF6qE4WD-ZNZj1HFzNNgaBi_UjG1sjEg@mail.gmail.com>

Hi Eric,

On Mon, 16 May 2016, Eric Sunshine wrote:

Show 18 quoted lines

> On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 16 May 2016, Eric Sunshine wrote:
> >> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> >> > +       fp = fopen(".git/BISECT_TERMS", "w");
> >>
> >> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
> >> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
> >> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
> >> etc. functions in cache.h instead.
> >
> > Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> > better: follow the example of bisect.c and use
> > `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.
> 
> Thanks for pointing this out. My review time is severely limited these
> days so I didn't go the distance of re-studying and re-digesting which
> function was the correct one to use.

I am constantly amazed how much you manage to review, and how helpful your comments are. I am glad if I could contribute a little.

Show 19 quoted lines

> >> > +               strbuf_release(&content);
> >> > +               die_errno(_("could not open the file to read terms"));
> >>
> >> Is dying here correct? I thought we established previously that you
> >> should be reporting failure via normal -1 return value rather than
> >> dying. Indeed, you're doing so below when strbuf_write() fails.
> >
> > The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> > in *.c. So technically, it would be okay here, too. However, I think that
> > this code should be written with libification in mind, so I would also
> > prefer it to error() and return, to give the caller a chance to do other
> > things after an error occurred.
> 
> Agreed. Specific to the "established previously" I wrote above, I was
> referring to [1] from just a few days ago which explained why 'return
> -1' was preferable to die() in a similar case that had an odd mix of
> 'return -1' and die() in the same function.
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556

Yes, this is an excellent link. Maybe we would want something like this, too?

-- snipsnap --
(From: Eric Sunshine <sunshine@sunshineco.com>)
CodingGuidelines: mention that die() is for fatal errors only

Let's formalize the rule when to use die() and when to use error().

diff --git a/Documentation/CodingGuidelines
b/Documentation/CodingGuidelines
index 0ddd368..5d2d65f 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -333,6 +333,10 @@ For C programs:
 
  - When you come up with an API, document it.
 
+ - Use die() only to signal an exceptional conditions which should
+   abort the program. All other error conditions should instead
+   return e.g. using "return error(...)".
+
  - The first #include in C files, except in platform specific compat/
    implementations, must be either "git-compat-util.h", "cache.h" or
    "builtin.h".  You do not have to include more than one of these.