From: ZheNing Hu <adlternative@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
Git List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Christian Couder <christian.couder@gmail.com>,
Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v5] ls-files: introduce "--format" option
Date: Mon, 11 Jul 2022 23:14:49 +0800 [thread overview]
Message-ID: <CAOLTT8Q+-Tr9-X=DBmf7wExT3L3DtAXzpmoV+dqfrY-nouP5pg@mail.gmail.com> (raw)
In-Reply-To: <220705.86sfng9c5a.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年7月5日周二 16:50写道:
>
>
> On Tue, Jul 05 2022, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Add a new option --format that output index enties
> > informations with custom format, taking inspiration
> > from the option with the same name in the `git ls-tree`
> > command.
> >
> > --format cannot used with -s, -o, -k, -t, --resolve-undo,
> > --deduplicate and --eol.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> > ls-files: introduce "--format" options
> >
> > v4->v5:
> >
> > 1. Let --format incompatible with -t.
> > 2. Fix %(eolinfo) and %(eolattr) docs suggested by Junio.
> >
> > Looking forward to Ævar's reviewing.
>
> Thanks again, I took a look at this and it looks good to me as-is.
>
> If you do want to further twiddle with it at this point I applied these
> changes to it locally while poking around, changes:
>
> * Some trivial whitespace between variable decl.
>
> * Removed a "return;" at the end of a function
>
> * I found the new write_*() helpers to be uneccesary, what I did spot
> after seeing if they could be factored out is the existing
> write_eolinfo() function.
>
> I see you just copied some of the code from there, but
> e.g. initializing to "" and doing an unconditional strbuf_addstr()
> looks odd IMO compared to just doing it inline as below.
>
Indeed, it may be a little inelegant...
> I think if helpers are to be introduced here I'd think it would make
> more sense to split out the small bits of behavior from
> write_eolinfo() so you can call it picemeal and share the code, but
> since it's calling trivial external functions I think just calling
> those directly probably makes more sense...
>
> * Likewise for the test I wondered if you were adding a bug by not
> reporting when lstat() failed, but then found that this is the same
> thing we do on --eol.
>
Yes, write_eolinfo() ignore lstat() error too, so this would not be a problem.
> So for the tests I think it's better just to demonstrate that we can
> emit the exact same thing that --eol does with --format.
>
> * We've gone back & forth a bit on whether this would combine with
> --debug, while it's an internal-only feature it would be nice to have a
> test for it combined with --format, noting that the behavior might
> change...
>
Oh, if we really want --format, --debug used with --eol, -t, some user
may curious about why --format can not used with --eol, -t (without --debug),
and I think this will make it interface more complicated. So now I pefer to keep
origin design.
> * There is one subtle behavior change here in that I deleted the "ce
> &&" part from write_index_eolinfo_to_buf() when moving the code
> over. I'm 99% sure this is the right thing to do, as other code in
> expand_show_index() unconditionally dereferences it.
>
> So perhaps we don't need that guard in write_eolinfo() either? In any
> case copy/pasting it over when we're already assuming a non-NULL "ce"
> in the same "if/elseif/else" chain looks a bit odd.
>
> Ah, I see it's because in show_dir_entry() we explicitly pass it as
> NULL, but that doesn't apply to our new codepath, so as long as we're
> not sharing that helper with write_eolinfo() it makes sense to not do
> that check.
>
Agree.
> Even then the helper should probably assume "ce", and write_eolinfo()
> itself should do the "is ce NULL?" check which is specific to its
> use-case.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 79ecdce2c9c..cc3cece3830 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -77,30 +77,6 @@ static void write_eolinfo(struct index_state *istate,
> }
> }
>
> -static void write_index_eolinfo_to_buf(struct strbuf *sb, struct index_state *istate,
> - const struct cache_entry *ce)
> -{
> - const char *i_txt = "";
> - if (ce && S_ISREG(ce->ce_mode))
> - i_txt = get_cached_convert_stats_ascii(istate, ce->name);
> - strbuf_addstr(sb, i_txt);
> -}
> -
> -static void write_worktree_eolinfo_to_buf(struct strbuf *sb, const char *path)
> -{
> - struct stat st;
> - const char *w_txt = "";
> - if (!lstat(path, &st) && S_ISREG(st.st_mode))
> - w_txt = get_wt_convert_stats_ascii(path);
> - strbuf_addstr(sb, w_txt);
> -}
> -
> -static void write_eolattr_to_buf(struct strbuf *sb, struct index_state *istate,
> - const char *path)
> -{
> - strbuf_addstr(sb, get_convert_attr_ascii(istate, path));
> -}
> -
> static void write_name(const char *name)
> {
> /*
> @@ -114,6 +90,7 @@ static void write_name(const char *name)
> static void write_name_to_buf(struct strbuf *sb, const char *name)
> {
> const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
> +
> if (line_terminator)
> quote_c_style(rel, sb, NULL, 0);
> else
> @@ -270,6 +247,8 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
> const char *end;
> const char *p;
> size_t len = strbuf_expand_literal_cb(sb, start, NULL);
> + struct stat st;
> +
> if (len)
> return len;
> if (*start != '(')
> @@ -288,12 +267,16 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
> strbuf_add_unique_abbrev(sb, &data->ce->oid, abbrev);
> else if (skip_prefix(start, "(stage)", &p))
> strbuf_addf(sb, "%d", ce_stage(data->ce));
> - else if (skip_prefix(start, "(eolinfo:index)", &p))
> - write_index_eolinfo_to_buf(sb, data->istate, data->ce);
> - else if (skip_prefix(start, "(eolinfo:worktree)", &p))
> - write_worktree_eolinfo_to_buf(sb, data->pathname);
> + else if (skip_prefix(start, "(eolinfo:index)", &p) &&
> + S_ISREG(data->ce->ce_mode))
> + strbuf_addstr(sb, get_cached_convert_stats_ascii(data->istate,
> + data->ce->name));
> + else if (skip_prefix(start, "(eolinfo:worktree)", &p) &&
> + !lstat(data->pathname, &st) && S_ISREG(st.st_mode))
> + strbuf_addstr(sb, get_wt_convert_stats_ascii(data->pathname));
> else if (skip_prefix(start, "(eolattr)", &p))
> - write_eolattr_to_buf(sb, data->istate, data->pathname);
> + strbuf_addstr(sb, get_convert_attr_ascii(data->istate,
> + data->pathname));
> else if (skip_prefix(start, "(path)", &p))
> write_name_to_buf(sb, data->pathname);
> else
> @@ -310,13 +293,12 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
> .istate = repo->index,
> .ce = ce,
> };
> -
> struct strbuf sb = STRBUF_INIT;
> +
> strbuf_expand(&sb, format, expand_show_index, &data);
> strbuf_addch(&sb, line_terminator);
> fwrite(sb.buf, sb.len, 1, stdout);
> strbuf_release(&sb);
> - return;
> }
>
> static void show_ce(struct repository *repo, struct dir_struct *dir,
> diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
> index 60c415aafd6..baf03f9096e 100755
> --- a/t/t3013-ls-files-format.sh
> +++ b/t/t3013-ls-files-format.sh
> @@ -40,27 +40,13 @@ test_expect_success 'git ls-files --format objectname' '
> test_cmp expect actual
> '
>
> -test_expect_success 'git ls-files --format eolinfo:index' '
> - cat >expect <<-\EOF &&
> - lf
> - lf
> - EOF
> - git ls-files --format="%(eolinfo:index)" >actual &&
> - test_cmp expect actual
> -'
> -
> -test_expect_success 'git ls-files --format eolinfo:worktree' '
> - cat >expect <<-\EOF &&
> - lf
> - lf
> - EOF
> - git ls-files --format="%(eolinfo:worktree)" >actual &&
> - test_cmp expect actual
> -'
> -
> -test_expect_success 'git ls-files --format eolattr' '
> - printf "\n\n" >expect &&
> - git ls-files --format="%(eolattr)" >actual &&
> +HT=' '
> +WS=' '
> +test_expect_success 'git ls-files --format v.s. --eol' '
> + git ls-files --eol >expect 2>err &&
> + test_must_be_empty err &&
> + git ls-files --format="i/%(eolinfo:index)${WS}w/%(eolinfo:worktree)${WS}attr/${WS}${WS}${WS}${WS} ${HT}%(path)" >actual 2>err &&
> + test_must_be_empty err &&
> test_cmp expect actual
> '
>
Thanks for review and help :-)
ZheNing Hu
next prev parent reply other threads:[~2022-07-11 15:15 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 13:45 [PATCH 0/2] ls-files: introduce "--format" and "--object-only" options ZheNing Hu via GitGitGadget
2022-06-15 13:45 ` [PATCH 1/2] ls-files: introduce "--format" option ZheNing Hu via GitGitGadget
2022-06-15 20:07 ` Ævar Arnfjörð Bjarmason
2022-06-18 10:50 ` ZheNing Hu
2022-06-15 13:45 ` [PATCH 2/2] ls-files: introduce "--object-only" option ZheNing Hu via GitGitGadget
2022-06-15 20:15 ` Ævar Arnfjörð Bjarmason
2022-06-18 10:59 ` ZheNing Hu
2022-06-19 9:13 ` [PATCH v2] ls-files: introduce "--format" option ZheNing Hu via GitGitGadget
2022-06-19 13:50 ` Phillip Wood
2022-06-20 13:32 ` ZheNing Hu
2022-06-21 2:05 ` [PATCH v3] " ZheNing Hu via GitGitGadget
2022-06-23 14:06 ` Phillip Wood
2022-06-23 15:57 ` Junio C Hamano
2022-06-24 10:16 ` Phillip Wood
2022-06-26 13:05 ` ZheNing Hu
2022-06-24 13:20 ` Ævar Arnfjörð Bjarmason
2022-06-24 15:28 ` Junio C Hamano
2022-06-26 13:01 ` ZheNing Hu
2022-06-24 13:25 ` Ævar Arnfjörð Bjarmason
2022-06-24 15:33 ` Junio C Hamano
2022-06-26 13:35 ` ZheNing Hu
2022-06-27 8:22 ` Junio C Hamano
2022-06-27 11:06 ` ZheNing Hu
2022-06-27 15:41 ` Junio C Hamano
2022-07-01 13:30 ` ZheNing Hu
2022-06-26 13:34 ` ZheNing Hu
2022-06-26 15:29 ` [PATCH v4] " ZheNing Hu via GitGitGadget
2022-06-27 8:32 ` Junio C Hamano
2022-06-27 11:18 ` ZheNing Hu
2022-06-27 18:34 ` Ævar Arnfjörð Bjarmason
2022-07-01 12:42 ` ZheNing Hu
2022-06-28 15:19 ` Phillip Wood
2022-07-01 12:47 ` ZheNing Hu
2022-07-05 6:32 ` [PATCH v5] " ZheNing Hu via GitGitGadget
2022-07-05 8:39 ` Ævar Arnfjörð Bjarmason
2022-07-11 15:14 ` ZheNing Hu [this message]
2022-07-05 19:28 ` Torsten Bögershausen
2022-07-11 15:27 ` ZheNing Hu
2022-07-11 16:53 ` [PATCH v6] " ZheNing Hu via GitGitGadget
2022-07-11 22:11 ` Junio C Hamano
2022-07-12 13:53 ` ZheNing Hu
2022-07-12 14:34 ` Junio C Hamano
2022-07-13 6:07 ` [PATCH v7] " ZheNing Hu via GitGitGadget
2022-07-18 8:09 ` Ævar Arnfjörð Bjarmason
2022-07-19 16:19 ` ZheNing Hu
2022-07-19 16:47 ` Junio C Hamano
2022-07-19 17:21 ` ZheNing Hu
2022-07-20 16:36 ` [PATCH v8] " ZheNing Hu via GitGitGadget
2022-07-20 17:37 ` Junio C Hamano
2022-07-21 15:54 ` Ævar Arnfjörð Bjarmason
2022-07-21 17:22 ` Eric Sunshine
2022-07-21 17:23 ` Junio C Hamano
2022-07-22 6:44 ` ZheNing Hu
2022-07-23 18:40 ` Junio C Hamano
2022-07-23 18:46 ` Junio C Hamano
2022-07-24 11:08 ` ZheNing Hu
2022-07-25 1:03 ` Junio C Hamano
2022-07-25 11:00 ` ZheNing Hu
2022-07-23 6:44 ` [PATCH v9] " ZheNing Hu via GitGitGadget
2022-09-08 2:01 ` Jiang Xin
2022-09-11 11:01 ` ZheNing Hu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOLTT8Q+-Tr9-X=DBmf7wExT3L3DtAXzpmoV+dqfrY-nouP5pg@mail.gmail.com' \
--to=adlternative@gmail.com \
--cc=avarab@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).