| Message ID | 20170411163340.GJ2455@edanaher.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Evan Danaher posted on Tue, 11 Apr 2017 12:33:40 -0400 as excerpted: > 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> > --- I'm not a dev so won't attempt to comment on the patch itself, but it's worth noting that according to kernel patch submission guidelines (which btrfs-progs use as well) on V2+ patch postings, there should be a short, often one-line per version, summary of what changed between versions. This helps both reviewers and would-be patch-using admins such as myself understand how a patch is evolving, as well as for reviewers preventing unnecessary work when re-reviewing a new version of a patch previously reviewed in an earlier version. On patch series this summary is generally found in the 0/N post, while on individual patches without a 0/N, it's normally found below the first --- delimiter, so as to avoid including the patch history in the final merged version comment. See pretty much any other multi-version posted patch for examples.
On Wed, Apr 12, 2017 at 7:05 PM, Duncan <1i5t5.duncan@cox.net> wrote: > Evan Danaher posted on Tue, 11 Apr 2017 12:33:40 -0400 as excerpted: > >> 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> >> --- > > I'm not a dev so won't attempt to comment on the patch itself, but it's > worth noting that according to kernel patch submission guidelines (which > btrfs-progs use as well) on V2+ patch postings, there should be a short, > often one-line per version, summary of what changed between versions. > This helps both reviewers and would-be patch-using admins such as myself > understand how a patch is evolving, as well as for reviewers preventing > unnecessary work when re-reviewing a new version of a patch previously > reviewed in an earlier version. > > On patch series this summary is generally found in the 0/N post, while on > individual patches without a 0/N, it's normally found below the first --- > delimiter, so as to avoid including the patch history in the final merged > version comment. To be specific, something like > --- > >v2: fixed an off-by-one error which caused padding to be 33 characters for short paths > --- instead of the email tail you currently appended (for future reference). FWIW, I'm fine with you adding my 'Reviewed-by', but I don't think it carries much weight yet. :-) -- Noah -- 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 Thu, Apr 13, 2017 at 10:38:02AM -0400, Noah Massey wrote: > On Wed, Apr 12, 2017 at 7:05 PM, Duncan <1i5t5.duncan@cox.net> wrote: > > Evan Danaher posted on Tue, 11 Apr 2017 12:33:40 -0400 as excerpted: > > > >> 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> > >> --- > > > > I'm not a dev so won't attempt to comment on the patch itself, but it's > > worth noting that according to kernel patch submission guidelines (which > > btrfs-progs use as well) on V2+ patch postings, there should be a short, > > often one-line per version, summary of what changed between versions. > > This helps both reviewers and would-be patch-using admins such as myself > > understand how a patch is evolving, as well as for reviewers preventing > > unnecessary work when re-reviewing a new version of a patch previously > > reviewed in an earlier version. > > > > On patch series this summary is generally found in the 0/N post, while on > > individual patches without a 0/N, it's normally found below the first --- > > delimiter, so as to avoid including the patch history in the final merged > > version comment. > > To be specific, something like > > --- > > > >v2: fixed an off-by-one error which caused padding to be 33 characters for short paths > > --- > > instead of the email tail you currently appended (for future reference). > FWIW, I'm fine with you adding my 'Reviewed-by', but I don't think it > carries much weight yet. :-) It does, I'll add the tag, thanks. -- 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 12:33:40PM -0400, Evan Danaher 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> Applied, thanks. -- 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..1591e0c 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> --- --- Thanks to Noah Massey for catching an off-by-one; I had kept the condition as (ret++ < 32), but that would pad to 33 spaces, not 32. Changing the loop condition to preincrement keeps the old behavior for short names. -- 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