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

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Johannes,

On Mon, May 16, 2016 at 6:46 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> 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. I will look into this.


>> > +               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.

Frankly, I have no idea what you mean by libification but I will use
error() since efforts for libification have already started.

Regards,
Pranit Bauva

> Ciao,
> Dscho