git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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-name

Look 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
Previous: Dmitry PotapovNext: Dmitry Potapov
Message 6 of 7 in “make prefix_path() never return NULL”
  1. make prefix_path() never return NULLDmitry Potapov, Oct 5, 2008
  2. do not segfault if make_cache_entry failedDmitry Potapov, Oct 5, 2008
  3. error out if path is invalidDmitry Potapov, Oct 5, 2008
  4. Johannes SixtOct 6, 2008
  5. Dmitry PotapovOct 7, 2008
  6. Johannes SixtOct 7, 2008
  7. print an error message for invalid pathDmitry Potapov, Oct 11, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.