Re: [PATCH] Make the exit code of add_file_to_index actually useful

From: Alex Riesen <raa.lkml@gmail.com>
Date: May 12, 2008, 20:54 UTC
Message-ID: <20080512205414.GH3128@steel.home>
In-Reply-To: <7vzlqvxt1p.fsf@gitster.siamese.dyndns.org>

Junio C Hamano, Mon, May 12, 2008 20:42:58 +0200:

Show 6 quoted lines

> >
> > I corrected the series to use die() again and rebased it off current
> > master (65ea3b8c). So it is more compatible with libification (does
> > not hinder it more than previos code) and keep the exit code.
> 
> But you did not answer my question in the part you did not quote, did you?

I believe I did:

Date:	Sun, 2 Mar 2008 22:42:41 +0100
Subject: Re: [PATCH] Make the exit code of add_file_to_index actually useful
Message-ID: <20080302214241.GB13954@steel.home>

Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:

Show 5 quoted lines

> Why is this even needed to begin with?  I am aware of Dirk's original
> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
> afford to, and this option deliberately breaks it.  What is the real
> reason why such an unreadable (either for privilege or for I/O error)
> file should not live in .gitignore?

Another program keeps the file open. There is an exclusive mode for opening files, which locks the files for everyone. I believe it is even default mode, unless selected otherwise.

> Now when somebody either forgets to check the return value from this
> function, or deliberately ignores it, the resulting index will not match
> what the code is told to update it with.

I think I got them all in the current code:

    $git grep -E 'add_(file_)?to_(index|cache)'
    Documentation/technical/api-in-core-index.txt:* add_file_to_index()
    builtin-add.c:                  if (add_file_to_cache(path, data->flags
    builtin-add.c:          if (add_file_to_cache(dir.entries[i]->name, ver
    builtin-commit.c:                       if (add_to_cache(p->path, &st,
    builtin-mv.c:                   if (add_file_to_cache(path, verbose))
    cache.h:#define add_to_cache(path, st, verbose) add_to_index(&the_index
    cache.h:#define add_file_to_cache(path, verbose) add_file_to_index(&the
    cache.h:extern int add_to_index(struct index_state *, const char *path,
    cache.h:extern int add_file_to_index(struct index_state *, const char *
    read-cache.c:int add_to_index(struct index_state *istate, const char *p
    read-cache.c:int add_file_to_index(struct index_state *istate, const ch
    read-cache.c:   return add_to_index(istate, path, &st, verbose);

Regarding the return value: isn't it very often a bug to ignore them? Or do you mean to say I should have renamed the function so that old interface cannot be used accidentally by someone how just knows it never returns in case of an error?