dir: point treat_leading_path() warning to the right place
diff mbox series

Message ID 20200115202146.GA4091171@coredump.intra.peff.net
State New
Headers show
Series
  • dir: point treat_leading_path() warning to the right place
Related show

Commit Message

Jeff King Jan. 15, 2020, 8:21 p.m. UTC
On Wed, Jan 15, 2020 at 02:21:18PM +0000, Elijah Newren via GitGitGadget wrote:

> From: Jeff King <peff@peff.net>
> 
> Restructure the code slightly to avoid passing around a struct dirent
> anywhere, which also enables us to avoid trying to manufacture one.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Elijah Newren <newren@gmail.com>

Thanks, this looks good.

I wondered briefly whether this:

> @@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
>  			break;
>  		strbuf_reset(&sb);
>  		strbuf_add(&sb, path, prevlen);
> -		memcpy(de->d_name, path+prevlen, baselen-prevlen);
> -		de->d_name[baselen-prevlen] = '\0';
> +		strbuf_reset(&subdir);
> +		strbuf_add(&subdir, path+prevlen, baselen-prevlen);
> +		cdir.d_name = subdir.buf;

...could avoid the extra strbuf by pointing into an existing string
(since d_name is now a pointer, and not part of a dirent). But I think
the answer is "no", because in a path like "a/b/c", the loop may see
just "b" (so offsetting into path isn't sufficient, because we also have
to cut off the trailing part).

I did notice one other small thing while looking at this code:

-- >8 --
Subject: [PATCH] dir: point treat_leading_path() warning to the right place

Commit 777b420347 (dir: synchronize treat_leading_path() and
read_directory_recursive(), 2019-12-19) tried to add two warning
comments in those functions, pointing at each other. But the one in
treat_leading_path() just points at itself.

Let's fix that. Since the comment also redirects the reader for more
details to "the commit that added this warning", and since we're now
modifying the warning (creating a new commit without those details),
let's mention the actual commit id.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Elijah Newren Jan. 15, 2020, 8:28 p.m. UTC | #1
On Wed, Jan 15, 2020 at 12:21 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jan 15, 2020 at 02:21:18PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > From: Jeff King <peff@peff.net>
> >
> > Restructure the code slightly to avoid passing around a struct dirent
> > anywhere, which also enables us to avoid trying to manufacture one.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Thanks, this looks good.
>
> I wondered briefly whether this:
>
> > @@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
> >                       break;
> >               strbuf_reset(&sb);
> >               strbuf_add(&sb, path, prevlen);
> > -             memcpy(de->d_name, path+prevlen, baselen-prevlen);
> > -             de->d_name[baselen-prevlen] = '\0';
> > +             strbuf_reset(&subdir);
> > +             strbuf_add(&subdir, path+prevlen, baselen-prevlen);
> > +             cdir.d_name = subdir.buf;
>
> ...could avoid the extra strbuf by pointing into an existing string
> (since d_name is now a pointer, and not part of a dirent). But I think
> the answer is "no", because in a path like "a/b/c", the loop may see
> just "b" (so offsetting into path isn't sufficient, because we also have
> to cut off the trailing part).
>
> I did notice one other small thing while looking at this code:
>
> -- >8 --
> Subject: [PATCH] dir: point treat_leading_path() warning to the right place
>
> Commit 777b420347 (dir: synchronize treat_leading_path() and
> read_directory_recursive(), 2019-12-19) tried to add two warning
> comments in those functions, pointing at each other. But the one in
> treat_leading_path() just points at itself.
>
> Let's fix that. Since the comment also redirects the reader for more
> details to "the commit that added this warning", and since we're now
> modifying the warning (creating a new commit without those details),
> let's mention the actual commit id.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  dir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 7d255227b1..31e83d982a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2308,9 +2308,9 @@ static int treat_leading_path(struct dir_struct *dir,
>          * WARNING WARNING WARNING:
>          *
>          * Any updates to the traversal logic here may need corresponding
> -        * updates in treat_leading_path().  See the commit message for the
> -        * commit adding this warning as well as the commit preceding it
> -        * for details.
> +        * updates in read_directory_recursive().  See 777b420347 (dir:
> +        * synchronize treat_leading_path() and read_directory_recursive(),
> +        * 2019-12-19) and its parent commit for details.
>          */
>
>         struct strbuf sb = STRBUF_INIT;
> --
> 2.25.0.639.gb9b1511416

Looks good to me; thanks.

Patch
diff mbox series

diff --git a/dir.c b/dir.c
index 7d255227b1..31e83d982a 100644
--- a/dir.c
+++ b/dir.c
@@ -2308,9 +2308,9 @@  static int treat_leading_path(struct dir_struct *dir,
 	 * WARNING WARNING WARNING:
 	 *
 	 * Any updates to the traversal logic here may need corresponding
-	 * updates in treat_leading_path().  See the commit message for the
-	 * commit adding this warning as well as the commit preceding it
-	 * for details.
+	 * updates in read_directory_recursive().  See 777b420347 (dir:
+	 * synchronize treat_leading_path() and read_directory_recursive(),
+	 * 2019-12-19) and its parent commit for details.
 	 */
 
 	struct strbuf sb = STRBUF_INIT;