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

Re: [PATCH] sha1_file: use access(), not lstat(), if possible

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jul 25, 2017, 10:19 UTC
Message-ID
<alpine.DEB.2.21.1.1707251217250.4271@virtualbox>
In-Reply-To
<xmqq1sp8fnxv.fsf@gitster.mtv.corp.google.com>
Hi,
On Sat, 22 Jul 2017, Junio C Hamano wrote:
Show 39 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But this whole thread taps into a gripe I have with parts of Git's code
> > base: part of the code is not clear at all in its intent by virtue of
> > calling whatever POSIX function may seem to give the answer for the
> > intended question, instead of implementing a function whose name says
> > precisely what question is asked.
> >
> > In this instance, we do not call a helper get_file_size(). Oh no. That
> > would make it too obvious. We call lstat() instead.
> 
> I agree with you for this case and a case like this in general.  
> 
> In codepaths at a lot lower level (they tend to be the ancient and
> quite fundamental ones) in our codebase, lstat() is often directly
> used by the caller because they are interested not only in a single
> aspect of a path but many fields in struct stat are of interest.
> 
> When the code is interested in existence or size or whatever single
> aspect of a path and nothing else, however, the code would become
> easier to read if a helper function with a more specific name is
> used.  And it may even help individual platforms that do not want to
> use the full lstat() emulation, by telling them that other fields in
> struct stat are not needed.
> 
> Of course, then the issue becomes what to do when we are interested
> in not just one but a selected few attributes.  Perhaps we create a
> helper "get_A_B_and_C_attributes_for_path()", which may use lstat()
> on POSIX and the most efficient way to get only A, B and C attributes
> on non-POSIX platforms.  The implementation would be OK, but the naming
> becomes a bit hard; we need to give it a good name.
> 
> Things gets even more interesting when the set of attributes we are
> interested in grows by one and we need to rename the function to
> "get_A_B_C_and_D_attributes_for_path()".  When it is a lot easier to
> fall back to the full lstat() emulation on non-POSIX platforms, the
> temptation to just use it even though it would grab attributes that
> are not needed in that function grows, which needs to be resisted by
> those who are doing the actual implementation for a particular platform.

It becomes a lot easier to fall back to lstat(), if a lot less readable, yes.

Until, that is, one realises that the function name does not have to encode what information is sought. It can be a bit field in a parameter instead. There are even precendents in Git's own source code for that rather smart paradigm.

Ciao, Dscho

Previous: Junio C HamanoNext: Jonathan Tan
Message 60 of 65 in “Improvements to sha1_file”
  1. 0/4 Improvements to sha1_fileJonathan Tan, Jun 9, 2017
  2. 1/4 sha1_file: teach packed_object_info about typenameJonathan Tan, Jun 9, 2017
  3. Junio C HamanoJun 12, 2017
  4. 2/4 sha1_file: extract type and size from object_infoJonathan Tan, Jun 9, 2017
  5. Jeff KingJun 10, 2017
  6. Jonathan TanJun 12, 2017
  7. Jeff KingJun 12, 2017
  8. 4/4 sha1_file, fsck: add missing blob supportJonathan Tan, Jun 9, 2017
  9. 3/4 sha1_file: consolidate storage-agnostic object fnsJonathan Tan, Jun 9, 2017
  10. 0/4 Improvements to sha1_fileJonathan Tan, Jun 13, 2017
  11. 1/4 sha1_file: teach packed_object_info about typenameJonathan Tan, Jun 13, 2017
  12. 2/4 sha1_file: move delta base cache code upJonathan Tan, Jun 13, 2017
  13. Junio C HamanoJun 15, 2017
  14. 3/4 sha1_file: consolidate storage-agnostic object fnsJonathan Tan, Jun 13, 2017
  15. Junio C HamanoJun 15, 2017
  16. Jonathan TanJun 15, 2017
  17. Jeff KingJun 17, 2017
  18. Junio C HamanoJun 19, 2017
  19. 4/4 sha1_file, fsck: add missing blob supportJonathan Tan, Jun 13, 2017
  20. Junio C HamanoJun 15, 2017
  21. Jonathan TanJun 15, 2017
  22. Junio C HamanoJun 15, 2017
  23. 0/8 Improvements to sha1_fileJonathan Tan, Jun 20, 2017
  24. 1/8 sha1_file: teach packed_object_info about typenameJonathan Tan, Jun 20, 2017
  25. 2/8 sha1_file: rename LOOKUP_UNKNOWN_OBJECTJonathan Tan, Jun 20, 2017
  26. Junio C HamanoJun 21, 2017
  27. Jonathan TanJun 21, 2017
  28. 3/8 sha1_file: rename LOOKUP_REPLACE_OBJECTJonathan Tan, Jun 20, 2017
  29. Junio C HamanoJun 21, 2017
  30. 4/8 sha1_file: move delta base cache code upJonathan Tan, Jun 20, 2017
  31. 5/8 sha1_file: refactor read_objectJonathan Tan, Jun 20, 2017
  32. Junio C HamanoJun 21, 2017
  33. 6/8 sha1_file: improve sha1_object_info_extendedJonathan Tan, Jun 20, 2017
  34. Jeff KingJun 24, 2017
  35. Jonathan TanJun 26, 2017
  36. Junio C HamanoJun 26, 2017
  37. Jonathan TanJun 26, 2017
  38. Junio C HamanoJun 26, 2017
  39. 8/8 sha1_file: refactor has_sha1_file_with_flagsJonathan Tan, Jun 20, 2017
  40. 7/8 sha1_file: do not access pack if unneededJonathan Tan, Jun 20, 2017
  41. Junio C HamanoJun 21, 2017
  42. Jeff KingJun 24, 2017
  43. Junio C HamanoJun 24, 2017
  44. Jeff KingJun 24, 2017
  45. Jonathan TanJun 26, 2017
  46. Junio C HamanoJun 21, 2017
  47. Jeff KingJun 24, 2017
  48. 0/8 Improvements to sha1_fileJonathan Tan, Jun 22, 2017
  49. 1/8 sha1_file: teach packed_object_info about typenameJonathan Tan, Jun 22, 2017
  50. 2/8 sha1_file: rename LOOKUP_UNKNOWN_OBJECTJonathan Tan, Jun 22, 2017
  51. 3/8 sha1_file: rename LOOKUP_REPLACE_OBJECTJonathan Tan, Jun 22, 2017
  52. 8/8 sha1_file: refactor has_sha1_file_with_flagsJonathan Tan, Jun 22, 2017
  53. Christian CouderJul 18, 2017
  54. Jonathan TanJul 18, 2017
  55. Johannes SchindelinJul 19, 2017
  56. sha1_file: use access(), not lstat(), if possibleJonathan Tan, Jul 19, 2017
  57. Junio C HamanoJul 20, 2017
  58. Johannes SchindelinJul 22, 2017
  59. Junio C HamanoJul 22, 2017
  60. Johannes SchindelinJul 25, 2017
  61. 6/8 sha1_file: improve sha1_object_info_extendedJonathan Tan, Jun 22, 2017
  62. 5/8 sha1_file: refactor read_objectJonathan Tan, Jun 22, 2017
  63. 4/8 sha1_file: move delta base cache code upJonathan Tan, Jun 22, 2017
  64. 7/8 sha1_file: do not access pack if unneededJonathan Tan, Jun 22, 2017
  65. Junio C HamanoJun 22, 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.