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

RE: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

From
KWKevin Willford <kewillf@microsoft.com>
Date
Aug 29, 2017, 15:58 UTC
Message-ID
<DM2PR21MB0041575B6D9EE53A07C7D3EDB79F0@DM2PR21MB0041.namprd21.prod.outlook.com>
In-Reply-To
<20170829081752.nq5r776rjyf2amzh@sigill.intra.peff.net>
Show 11 quoted lines
> 
> On Mon, Aug 28, 2017 at 02:28:28PM -0600, Kevin Willford wrote:
> 
> > The return value of the get_files_dirs call is never being used.
> > Looking at the history of the file and it was originally only
> > being used for debug output statements.  Also when
> > read_tree_recursive return value is non zero it is changed to
> > zero.  This leads me to believe that it doesn't matter if
> > read_tree_recursive gets an error.
> 
> Or that the function is buggy. :)

That was one of my questions as well. Should read_tree_recursive be propagating a -1 and merge_trees be checking for that and bail when the call to get_files_dirs return is < 0? I made a commit with this change and ran the tests and they all still passed so either this return really doesn't matter or there are not sufficient tests covering it.

I went with this change because it was not changing any of the current functionality and if we find a case where it matters that read_tree_recursive fails due to bad tree or something else we can address it then.

Show 13 quoted lines
> 
> I'm tempted to say that we should probably die() when
> read_tree_recursive fails. This should only happen if we fail to parse
> the tree, or if our callback (save_files_dirs here) returns failure, and
> the latter looks like it never happens.
> 
> > Since the debug output has been removed and the caller isn't
> > checking the return value there is no reason to keep calulating
> > and returning a value.
> 
> Agreed, and I'm happy to see dead code go.
> 
> Minor nit: s/calulating/calculating/ in your commit message.
When will that spell checker for git messages be ready? ;)
> 
> -Peff
Previous: Jeff KingNext: Jeff King
Message 9 of 15 in “merge-recursive: replace string_list with hashmap”
  1. 0/3 merge-recursive: replace string_list with hashmapKevin Willford, Aug 28, 2017
  2. 1/3 merge-recursive: fix memory leakKevin Willford, Aug 28, 2017
  3. Ben PeartAug 28, 2017
  4. Jeff KingAug 29, 2017
  5. 2/3 merge-recursive: remove return value from get_files_dirsKevin Willford, Aug 28, 2017
  6. Ben PeartAug 28, 2017
  7. Jeff KingAug 29, 2017
  8. Jeff KingAug 29, 2017
  9. Kevin WillfordAug 29, 2017
  10. Jeff KingAug 29, 2017
  11. Stefan BellerAug 31, 2017
  12. 3/3 merge-recursive: change current file dir string_lists to hashmapKevin Willford, Aug 28, 2017
  13. Ben PeartAug 28, 2017
  14. Jeff KingAug 29, 2017
  15. Junio C HamanoSep 6, 2017

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.