From: Eric Sunshine Date: Mon, 16 May 2016 15:42:01 GMT Subject: Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C Message-ID: In-Reply-To: On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin wrote: > 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")`. 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. >> > + 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