| Message ID | 8dafb2b3779715aa277eb825a94af87b72f3e093.1641440700.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ls-tree.c: introduce "--format" option | expand |
Hi Teng, On Thu, 6 Jan 2022, Teng Long wrote: > A convenient way to pad strings is to use something like > `strbuf_addf(&buf, "%20s", "Hello, world!")`. > > However, the Coccinelle rule that forbids a format `"%s"` with a > constant string argument cast too wide a net, and also forbade such > padding. > > The original rule was introduced by commit: > > https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac Doing this in 9/9 is too late, by this time you already introduced the code site that requires this workaround. At the same time, I wonder why you want to defend spinning up the full-blown `printf()` machinery just to pad text that you can easily pad yourself. It sounds like a lot of trouble to me to introduce this patch and then use an uncommon method to pad a fixed string at runtime. Too much trouble for my liking. Ciao, Dscho > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > contrib/coccinelle/strbuf.cocci | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci > index d9ada69b43..2d6e0f58fc 100644 > --- a/contrib/coccinelle/strbuf.cocci > +++ b/contrib/coccinelle/strbuf.cocci > @@ -44,7 +44,7 @@ struct strbuf *SBP; > > @@ > expression E1, E2; > -format F =~ "s"; > +format F =~ "^s$"; > @@ > - strbuf_addf(E1, "%@F@", E2); > + strbuf_addstr(E1, E2); > -- > 2.33.0.rc1.1794.g2ae0a9cb82 > >
I am not sure whether I have sent the email repeatedly, because it is not shown on mailist. If so, sorry to bother you. Johannes Schindelin writes: > Doing this in 9/9 is too late, by this time you already introduced the > code site that requires this workaround. Yes, you are correct. Will fixed if the patch is still remained to next one. > At the same time, I wonder why you want to defend spinning up the > full-blown `printf()` machinery just to pad text that you can easily pad > yourself. It sounds like a lot of trouble to me to introduce this patch > and then use an uncommon method to pad a fixed string at runtime. Too much > trouble for my liking. I may not have explained it clearly in the cover. Sorry for that, I'm going to explain some more here, please correct me if there is something wrong or the method is not recommended or is not best practice in community. Firstly, the patch needs to be introduced I think and it has nothing to do with using " -" or "%7s" here, because the fix recommandation is not accurate in terms of the "static-analysis" report if someone just uses the "addf" api: - strbuf_addf(line, "%7s", "-"); + strbuf_addstr(line, "-"); They have different execution results and bring confusion to people. Then secondly, about the using "strbuf_addf(line, "%7s" , "-");" or "strbuf_addstr(line, " -");". I think you prefer the later and I prefer the former, right? (I'm not a native English speaker, so I just want to make sure I understand whole your meannings). If I understand everything correctly so far, it's good :) As I metioned in a previous reply [1], I think there is no performance issue here.. Why I prefer more of the former that is because, for the single line, it's more readable I think. Maybe it's not going to modify very often, but If someone want to know what this is, might have to do a count. So I don't think this is any more readable than "%7s". Here's what I think and looking forward to your reply. Thanks. [1] https://public-inbox.org/git/CADMgQSRxko6nC0zfDiVVfL2ZkdQVbBq0s59Er+6Nmg9vz4uJKQ@mail.gmail.com/
Hi Teng, On Mon, 10 Jan 2022, Teng Long wrote: > [...] about the using "strbuf_addf(line, "%7s" , "-");" or > "strbuf_addstr(line, " -");". [...] > > Why I prefer more of the former that is because, for the single line, > it's more readable I think. I strongly disagree. Using a format requires the reader to interpret a `printf()` format, to remember (if they ever knew) the rules about padding with `%<number>s` formats, and then to satisfy themselves that the result is correct. That's quite the cognitive load you put on the reader for something as trivial as " -". Not a fan, Johannes
> I strongly disagree. Using a format requires the reader to interpret a > `printf()` format, to remember (if they ever knew) the rules about padding > with `%<number>s` formats, and then to satisfy themselves that the result > is correct. > > That's quite the cognitive load you put on the reader for something as > trivial as " -". > Not a fan, > Johannes Ok. I will modify the next patch according to your opinion, I just hope to understand the problems and make better contributions in the future. Thanks. On Mon, Jan 10, 2022 at 8:49 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Teng, > > On Mon, 10 Jan 2022, Teng Long wrote: > > > [...] about the using "strbuf_addf(line, "%7s" , "-");" or > > "strbuf_addstr(line, " -");". [...] > > > > Why I prefer more of the former that is because, for the single line, > > it's more readable I think. > > I strongly disagree. Using a format requires the reader to interpret a > `printf()` format, to remember (if they ever knew) the rules about padding > with `%<number>s` formats, and then to satisfy themselves that the result > is correct. > > That's quite the cognitive load you put on the reader for something as > trivial as " -". > > Not a fan, > Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> [...] about the using "strbuf_addf(line, "%7s" , "-");" or >> "strbuf_addstr(line, " -");". [...] >> >> Why I prefer more of the former that is because, for the single line, >> it's more readable I think. > > I strongly disagree. Using a format requires the reader to interpret a > `printf()` format, to remember (if they ever knew) the rules about padding > with `%<number>s` formats, and then to satisfy themselves that the result > is correct. Both "more readable" and "cognitive load" are quite subjective. FWIW, I have a slight preference to the former because I do not have to count the whitespaces to figure out at which column the construct is trying to align to. Most of the time, however, I may not deeply care if the thing is aligned exactly, and " -" might be easier to scan and getting alarmed by seeing "%7s" to wonder if there is something unusual going on. When I am reading not for finding out the precise output format but for general correctness, bunch of unknown number of spaces followed by a dash might be easier to see. But once you know the language, "%7s" is not so alarming, and it does make it easier to see both for casual scanning and for counting columns. It also is likely that those who know the language would make more efficient developers to fix and/or enhance the code, so I prefer to optimize the code for them.
On Thu, Jan 06 2022, Teng Long wrote: > A convenient way to pad strings is to use something like > `strbuf_addf(&buf, "%20s", "Hello, world!")`. > > However, the Coccinelle rule that forbids a format `"%s"` with a > constant string argument cast too wide a net, and also forbade such > padding. > > The original rule was introduced by commit: > > https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac Let's refer to commits like this: 28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25) > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > contrib/coccinelle/strbuf.cocci | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci > index d9ada69b43..2d6e0f58fc 100644 > --- a/contrib/coccinelle/strbuf.cocci > +++ b/contrib/coccinelle/strbuf.cocci > @@ -44,7 +44,7 @@ struct strbuf *SBP; > > @@ > expression E1, E2; > -format F =~ "s"; > +format F =~ "^s$"; > @@ > - strbuf_addf(E1, "%@F@", E2); > + strbuf_addstr(E1, E2); That file currently has: 18:format F =~ "s"; 26:format F =~ "s"; 47:format F =~ "s"; If we're fixing these let's fix the other logic errors as well.
On Mon, Jan 10 2022, Johannes Schindelin wrote: > Hi Teng, > > On Mon, 10 Jan 2022, Teng Long wrote: > >> [...] about the using "strbuf_addf(line, "%7s" , "-");" or >> "strbuf_addstr(line, " -");". [...] >> >> Why I prefer more of the former that is because, for the single line, >> it's more readable I think. > > I strongly disagree. Using a format requires the reader to interpret a > `printf()` format, to remember (if they ever knew) the rules about padding > with `%<number>s` formats, and then to satisfy themselves that the result > is correct. > > That's quite the cognitive load you put on the reader for something as > trivial as " -". > > Not a fan, > Johannes I think you can argue that, but saying that this series must change that existing "%7s" format just because it happened to trip over an existin coccinelle rule as code was changed from printf() to strbuf_addf() is going overboard. Also, the ls-tree output has existing alignment issues, and the documentation says: "right-justified with minimum width of 7 characters" So I'd think we'd want to keep the %7s, and in some future change change that format to be dynamic so we'd align things properly if some fields were longer than 7 characters.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> The original rule was introduced by commit: >> >> https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac > > Doing this in 9/9 is too late, by this time you already introduced the > code site that requires this workaround. Good to point this out. Thanks.
On Tue, Jan 11, 2022 at 2:02 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > The original rule was introduced by commit: > > > > https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac > > Let's refer to commits like this: > > 28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25) OK, I will. > That file currently has: > > 18:format F =~ "s"; > 26:format F =~ "s"; > 47:format F =~ "s"; > > If we're fixing these let's fix the other logic errors as well. Thanks for the reminding， they'll be applied in the next patch.
On Mon, Jan 10, 2022 at 07:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote: > On Thu, Jan 06 2022, Teng Long wrote: > > The original rule was introduced by commit: > > > > https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac > > Let's refer to commits like this: > > 28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25) I find it helpful to have an alias like: $ git config alias.ll !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short in my $HOME/.gitconfig so that I can easily format commits in the standard way. I think that this alias came from Peff, but I can't remember. Thanks, Taylor
Am 11.01.22 um 17:42 schrieb Taylor Blau: > On Mon, Jan 10, 2022 at 07:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote: >> On Thu, Jan 06 2022, Teng Long wrote: >>> The original rule was introduced by commit: >>> >>> https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac >> >> Let's refer to commits like this: >> >> 28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25) > > I find it helpful to have an alias like: > > $ git config alias.ll > !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short > > in my $HOME/.gitconfig so that I can easily format commits in the > standard way. You can shorten "--pretty='tformat:%h (%s, %ad)' --date=short" to "--pretty=reference" or "--format=reference". For me that's easy enough to remember that I don't need an alias. Silly question, going further off-topic: What's "git always" doing? René
On Tue, Jan 11, 2022 at 08:06:00PM +0100, René Scharfe wrote: > Am 11.01.22 um 17:42 schrieb Taylor Blau: > > I find it helpful to have an alias like: > > > > $ git config alias.ll > > !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short > > > > in my $HOME/.gitconfig so that I can easily format commits in the > > standard way. > > You can shorten "--pretty='tformat:%h (%s, %ad)' --date=short" to > "--pretty=reference" or "--format=reference". For me that's easy enough > to remember that I don't need an alias. Ah, of course. Peff's copy likely predates `--pretty=reference`, and I inherited the cruft from him. Your suggestion has the nice benefit of colorizing the output when going to the terminal. > Silly question, going further off-topic: What's "git always" doing? Oops, I should have mentioned. It's another alias to ensure that the following command is always run in a Git repository (either the current one or a hand-picked default): $ git config alias.always !git rev-parse 2>/dev/null || cd ~/src/git; git I often read mail out of my home directory, and the above works with my `:Git` command in Vim (which passes its arguments to `git always` and inserts the result back into my buffer). That way I don't have to first `:cd ~/src/git` and then `:Git ll xyz`, I can just `:Git ll xyz` and it does what I meant most of the time. Thanks, Taylor
On Tue, Jan 11 2022, Taylor Blau wrote: > On Mon, Jan 10, 2022 at 07:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote: >> On Thu, Jan 06 2022, Teng Long wrote: >> > The original rule was introduced by commit: >> > >> > https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac >> >> Let's refer to commits like this: >> >> 28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25) > > I find it helpful to have an alias like: > > $ git config alias.ll > !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short > > in my $HOME/.gitconfig so that I can easily format commits in the > standard way. > > I think that this alias came from Peff, but I can't remember. Nowadays you can do this as: git show -s --pretty=reference See Documentation/SubmittingPatches I use: $ git help reference 'reference' is aliased to '!git --no-pager log --pretty=reference -1'
On Wed, Jan 12, 2022 at 12:42 AM Taylor Blau <me@ttaylorr.com> wrote: > > I find it helpful to have an alias like: > > $ git config alias.ll > !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short > > in my $HOME/.gitconfig so that I can easily format commits in the > standard way. > > I think that this alias came from Peff, but I can't remember. Wow. That's cool and efficient. Thanks.
On Wed, Jan 12, 2022 at 4:11 AM Taylor Blau <me@ttaylorr.com> wrote: > > Silly question, going further off-topic: What's "git always" doing? > > Oops, I should have mentioned. It's another alias to ensure that the > following command is always run in a Git repository (either the current > one or a hand-picked default): > > $ git config alias.always > !git rev-parse 2>/dev/null || cd ~/src/git; git > > I often read mail out of my home directory, and the above works with my > `:Git` command in Vim (which passes its arguments to `git always` and > inserts the result back into my buffer). That way I don't have to first > `:cd ~/src/git` and then `:Git ll xyz`, I can just `:Git ll xyz` and it > does what I meant most of the time. The same question is clear now。 Thanks for the explanations from Taylor Blau and René Scharfe.
On Wed, Jan 12, 2022 at 4:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Nowadays you can do this as: > > git show -s --pretty=reference > > See Documentation/SubmittingPatches > > I use: > > $ git help reference > 'reference' is aliased to '!git --no-pager log --pretty=reference -1' Make sense. Thanks.
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci index d9ada69b43..2d6e0f58fc 100644 --- a/contrib/coccinelle/strbuf.cocci +++ b/contrib/coccinelle/strbuf.cocci @@ -44,7 +44,7 @@ struct strbuf *SBP; @@ expression E1, E2; -format F =~ "s"; +format F =~ "^s$"; @@ - strbuf_addf(E1, "%@F@", E2); + strbuf_addstr(E1, E2);
A convenient way to pad strings is to use something like `strbuf_addf(&buf, "%20s", "Hello, world!")`. However, the Coccinelle rule that forbids a format `"%s"` with a constant string argument cast too wide a net, and also forbade such padding. The original rule was introduced by commit: https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Teng Long <dyroneteng@gmail.com> --- contrib/coccinelle/strbuf.cocci | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)