Re: [PATCH] error out if path is invalid
- From
Johannes Sixt <j.sixt@viscovery.net>
- Date
- Oct 7, 2008, 06:03 UTC
- Message-ID
- <48EAFBC2.7020305@viscovery.net>
- In-Reply-To
- <20081007002221.GS21650@dpotapov.dyndns.org>
Dmitry Potapov schrieb:
Show 15 quoted lines
> On Mon, Oct 06, 2008 at 09:02:22AM +0200, Johannes Sixt wrote:
>> Dmitry Potapov schrieb:
>>> if (!verify_path(path))
>>> - return -1;
>>> + return error("Invalid path '%s'", path);
>> Look at this change. Didn't the code error out before, too?
>
> It is certainly did not here. As to its caller, it depends. In fact,
> there are two chunks like that in my patch, so I am not sure to which
> one you refer here. If we speak about add_cacheinfo() then though the
> function did not error out, its caller died with one of the following
> messages:
> git update-index: unable to update some-file-name
> or
> git update-index: --cacheinfo cannot add some-file-nameLook at the original patch. You did not change the behavior except to write more error messages. Maybe I misunderstand the words "to error out". I understand them as "to detect an error and return early", but not "write an error message".
Show 22 quoted lines
> However, if we speak about add_index_entry_with_check then the caller > will not produce any error. The git would exit successfully (it still > does) and there was no error message as if everything was fine. > > Perhaps, the exit code should be corrected too, but if the git just dies > when add_index_entry() fails it may cause that having one invalid path > will prevent to check out other files, which does not seem to be the > right thing to do. > > As to correction to correction to make_cache_entry then after my > previous patch, it started to error out: > > make_cache_entry failed for path 'some-file-name' > > before that it silently segfaulted. > >> Same in the >> other cases. Hence, your patch subject does not describe the patch. > > Should I include the above explanation in the commit message or do you > have any objection to having the above error message in cases where the > caller already produce some message when it dies?
I don't object the change, only its (missing or IMHO incorrect) justification. I don't think that the above text would be the correct description because as far as I can see the only change you made was to add error messages.
-- Hannes