From: Johannes Schindelin Date: Mon, 16 May 2016 13:16:49 GMT Subject: Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C Message-ID: In-Reply-To: Hi, On Mon, 16 May 2016, Eric Sunshine wrote: > On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva 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")`. > > + strbuf_release(&content); > > + die_errno(_("could not open the file to read terms")); > > "read terms"? I thought this was writing. > > 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. Ciao, Dscho