| Message ID | 20220108085419.79682-4-chiyutianyi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack large blobs in stream | expand |
Am 08.01.22 um 09:54 schrieb Han Xin: > From: Han Xin <hanxin.hx@alibaba-inc.com> > > 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 <avarab@gmail.com> > Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com> > --- > 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. > } > > /* > @@ -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? > break; > if (adjust_shared_perm(tmp->buf))
On Sun, Jan 9, 2022 at 1:24 AM René Scharfe <l.s.r@web.de> wrote: > > Am 08.01.22 um 09:54 schrieb Han Xin: > > From: Han Xin <hanxin.hx@alibaba-inc.com> > > > > 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 <avarab@gmail.com> > > Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com> > > --- > > 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. > > > } > > > > /* > > @@ -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 > > break; > > if (adjust_shared_perm(tmp->buf))
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; } /* @@ -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) break; if (adjust_shared_perm(tmp->buf))