On Sun, Jan 9, 2022 at 1:24 AM René Scharfe [off-list ref] wrote:

Am 08.01.22 um 09:54 schrieb Han Xin:

quoted

From: Han Xin <redacted>

Since "mkdir foo/" works as well as "mkdir foo", let's remove the end
slash as many users of it want.

Suggested-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Han Xin <redacted>
---
 object-file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/object-file.c b/object-file.c
index 5d163081b1..4f0127e823 100644
--- a/object-file.c
+++ b/object-file.c

@@ -1831,13 +1831,13 @@ static void close_loose_object(int fd)
              die_errno(_("error when closing loose object file"));
 }

-/* Size of directory component, including the ending '/' */
+/* Size of directory component, excluding the ending '/' */
 static inline int directory_size(const char *filename)
 {
      const char *s = strrchr(filename, '/');
      if (!s)
              return 0;
-     return s - filename + 1;
+     return s - filename;

This will return zero both for "filename" and "/filename".  Hmm.  Since
it's only used for loose object files we can assume that at least one
slash is present, so this removal of functionality is not actually a
problem.  But I don't understand its benefit.

quoted

 }

 /*

@@ -1854,7 +1854,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename,

      strbuf_reset(tmp);
      strbuf_add(tmp, filename, dirlen);
-     strbuf_addstr(tmp, "tmp_obj_XXXXXX");
+     strbuf_addstr(tmp, "/tmp_obj_XXXXXX");
      fd = git_mkstemp_mode(tmp->buf, 0444);
      do {
              if (fd >= 0 || !dirlen || errno != ENOENT)

@@ -1866,7 +1866,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename,
               * scratch.
               */
              strbuf_reset(tmp);
-             strbuf_add(tmp, filename, dirlen - 1);
+             strbuf_add(tmp, filename, dirlen);
              if (mkdir(tmp->buf, 0777) && errno != EEXIST)

This code makes sure that mkdir(2) is called without the trailing slash,
both with or without this patch.  From the commit message above I
somehow expected a change in this regard -- but again I wouldn't
understand its benefit.

Is this change really needed?  Is streaming unpack not possible with the
original directory_size() function?

*nod*
Streaming unpacking still works with the original directory_size().

This patch is more of a code cleanup that reduces the extra handling of
directory size first increasing and then decreasing. I'll seriously consider
if I should remove this patch, or move it to the tail.

Thanks
-Han Xin

quoted

                      break;
              if (adjust_shared_perm(tmp->buf))