Hi,
On Mon, 16 May 2016, Eric Sunshine wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html