| Message ID | 20170411000904.GF2455@edanaher.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 10, 2017 at 8:09 PM, Evan Danaher <github@edanaher.net> wrote: > I was shocked to discover that 'btrfs receive --dump' doesn't print a > space after long filenames, so it runs together into the metadata; for > example: > > truncate ./20-00-03/this-name-is-32-characters-longsize=0 > > This is a trivial patch to add a single space unconditionally, so the > result is the following: > > truncate ./20-00-03/this-name-is-32-characters-long size=0 > > I suppose this is technically a breaking change, but it seems unlikely > to me that anyone would depend on the existing behavior given how > unfriendly it is. > > > Signed-off-by: Evan Danaher <github@edanaher.net> > --- > diff --git a/send-dump.c b/send-dump.c > index 67f7977..493389f 100644 > --- a/send-dump.c > +++ b/send-dump.c > @@ -116,9 +116,10 @@ static int __print_dump(int subvol, void *user, const char *path, > putchar('\n'); > return 0; > } > - /* Short paths ale aligned to 32 chars */ > - while (ret++ < 32) > + /* Short paths are aligned to 32 chars; longer paths get a single space */ > + do { > putchar(' '); > + } while (ret++ < 32); while (++ret < 32); Since we're performing the check after the put, we need to pre-increment to count the space already added. > va_start(args, fmt); > /* Operation specified ones */ > vprintf(fmt, args); > --- > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for catching that! I overthought this and managed to convince myself that it was correct as it stood. Should I re-send the whole patch with that change? This is my first attempt at contributing to a project managed in the Linux kernel style, so I'm not sure about the process; documentation that I've found around responding to feedback seems to be more about social issues than the mechanics of what to actually do... On Tue, Apr 11, 2017 at 09:24:56AM -0400, Noah Massey wrote: > On Mon, Apr 10, 2017 at 8:09 PM, Evan Danaher <github@edanaher.net> wrote: > > I was shocked to discover that 'btrfs receive --dump' doesn't print a > > space after long filenames, so it runs together into the metadata; for > > example: > > > > truncate ./20-00-03/this-name-is-32-characters-longsize=0 > > > > This is a trivial patch to add a single space unconditionally, so the > > result is the following: > > > > truncate ./20-00-03/this-name-is-32-characters-long size=0 > > > > I suppose this is technically a breaking change, but it seems unlikely > > to me that anyone would depend on the existing behavior given how > > unfriendly it is. > > > > > > Signed-off-by: Evan Danaher <github@edanaher.net> > > --- > > diff --git a/send-dump.c b/send-dump.c > > index 67f7977..493389f 100644 > > --- a/send-dump.c > > +++ b/send-dump.c > > @@ -116,9 +116,10 @@ static int __print_dump(int subvol, void *user, const char *path, > > putchar('\n'); > > return 0; > > } > > - /* Short paths ale aligned to 32 chars */ > > - while (ret++ < 32) > > + /* Short paths are aligned to 32 chars; longer paths get a single space */ > > + do { > > putchar(' '); > > + } while (ret++ < 32); > > while (++ret < 32); > > Since we're performing the check after the put, we need to > pre-increment to count the space already added. > > > va_start(args, fmt); > > /* Operation specified ones */ > > vprintf(fmt, args); > > --- > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 11, 2017 at 09:49:02AM -0400, Evan Danaher wrote: > Thanks for catching that! I overthought this and managed to convince > myself that it was correct as it stood. > > Should I re-send the whole patch with that change? This is my first > attempt at contributing to a project managed in the Linux kernel style, > so I'm not sure about the process; documentation that I've found around > responding to feedback seems to be more about social issues than the > mechanics of what to actually do... Here's some howto https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ Depends on what you take from the 'linux kernel style'. We stick to good changelogs, signed-off-by lines and patches that do one thing at a time. The interactions experience differs across the subsystems, the btrfs community is friendly. Patches from strangers are apprecated as they usually do the extra step and also try to fix the problem they encounter. The patch does not need to be perfect, small things I fix myself. If it's not obvious it's better to do another iteration, eg. when the change needs to be retested. > On Tue, Apr 11, 2017 at 09:24:56AM -0400, Noah Massey wrote: > > On Mon, Apr 10, 2017 at 8:09 PM, Evan Danaher <github@edanaher.net> wrote: > > > I was shocked to discover that 'btrfs receive --dump' doesn't print a > > > space after long filenames, so it runs together into the metadata; for > > > example: > > > > > > truncate ./20-00-03/this-name-is-32-characters-longsize=0 > > > > > > This is a trivial patch to add a single space unconditionally, so the > > > result is the following: > > > > > > truncate ./20-00-03/this-name-is-32-characters-long size=0 > > > > > > I suppose this is technically a breaking change, but it seems unlikely > > > to me that anyone would depend on the existing behavior given how > > > unfriendly it is. Agreed, the output needs to be fixed, backward compatibility is not an issue here. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/send-dump.c b/send-dump.c index 67f7977..493389f 100644 --- a/send-dump.c +++ b/send-dump.c @@ -116,9 +116,10 @@ static int __print_dump(int subvol, void *user, const char *path, putchar('\n'); return 0; } - /* Short paths ale aligned to 32 chars */ - while (ret++ < 32) + /* Short paths are aligned to 32 chars; longer paths get a single space */ + do { putchar(' '); + } while (ret++ < 32); va_start(args, fmt); /* Operation specified ones */ vprintf(fmt, args);
I was shocked to discover that 'btrfs receive --dump' doesn't print a space after long filenames, so it runs together into the metadata; for example: truncate ./20-00-03/this-name-is-32-characters-longsize=0 This is a trivial patch to add a single space unconditionally, so the result is the following: truncate ./20-00-03/this-name-is-32-characters-long size=0 I suppose this is technically a breaking change, but it seems unlikely to me that anyone would depend on the existing behavior given how unfriendly it is. Signed-off-by: Evan Danaher <github@edanaher.net> --- --- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html