From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Jiang Xin" <zhiyou.jx@alibaba-inc.com>,
"Emily Shaffer" <emilyshaffer@google.com>,
"Jonathan Tan" <jonathantanmy@google.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing
Date: Mon, 21 Jun 2021 17:10:12 +0200 [thread overview]
Message-ID: <cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com>
This v3 fixes issues noted by Emily Shaffer & Jonathan Tan on v2,
thanks both!
The REV_INFO_STDIN_LINE_BREAK is gone, it was just a result of some
WIP hacking, but I was too close to the code to notice. The "!len"
check in pack-objects.c was also redundant by moving to the revision.c
API. I have a parallel just-submitted series[1] that tests for that,
this version passes tests when combined with those more exhaustive
tests.
There were various other nits & suggestions that I either addressed by
changing the relevant code / documentation as suggested, or (e.g. in
the case of the "--not" parsing) explained in commit message(s) why it
made sense to leave those special-cases in place.
1. https://lore.kernel.org/git/cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (4):
upload-pack: run is_repository_shallow() before setup_revisions()
revision.h: refactor "disable_stdin" and "read_from_stdin"
pack-objects.c: do stdin parsing via revision.c's API
pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
builtin/am.c | 4 +--
builtin/blame.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------
builtin/rev-list.c | 2 +-
revision.c | 38 +++++++++++++++++++++-----
revision.h | 59 ++++++++++++++++++++++++++++++++++++----
sequencer.c | 4 +--
8 files changed, 119 insertions(+), 53 deletions(-)
Range-diff against v2:
1: 6a8b20a7cf3 = 1: 3840ac28e8d upload-pack: run is_repository_shallow() before setup_revisions()
2: d88b2c04102 ! 2: 6f69644b808 revision.h: unify "disable_stdin" and "read_from_stdin"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- revision.h: unify "disable_stdin" and "read_from_stdin"
+ revision.h: refactor "disable_stdin" and "read_from_stdin"
- In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
- added the "disable_stdin" flag, and then much later in
- a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
- gained a "read_from_stdin" flag.
+ Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
+ in preparation for a subsequent commit adding more flags.
The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
@@ Commit message
read it". Let's avoid this confusion by using "consume" and "consumed"
instead, i.e. a word whose present and past tense isn't the same.
+ See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
+ where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
+ make empty --stdin not an error, 2018-08-22) for the addition of the
+ "read_from_stdin" flag.
+
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/am.c ##
@@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
(!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
!revs.pending.nr) &&
- !revs.rev_input_given && !revs.read_from_stdin) ||
-+ !revs.rev_input_given && !revs.consumed_stdin_per_option) ||
++ !revs.rev_input_given && !revs.consumed_stdin) ||
revs.diff)
usage(rev_list_usage);
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
if (!strcmp(arg, "--stdin")) {
- if (revs->disable_stdin) {
-+ if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
++ switch (revs->stdin_handling) {
++ case REV_INFO_STDIN_IGNORE:
argv[left++] = arg;
continue;
++ case REV_INFO_STDIN_CONSUME_ON_OPTION:
++ if (revs->consumed_stdin++)
++ die("--stdin given twice?");
++ read_revisions_from_stdin(revs, &prune_data);
++ continue;
}
- if (revs->read_from_stdin++)
-+ if (revs->consumed_stdin_per_option++)
- die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data);
- continue;
+- die("--stdin given twice?");
+- read_revisions_from_stdin(revs, &prune_data);
+- continue;
+ }
+
+ if (!strcmp(arg, "--end-of-options")) {
## revision.h ##
@@ revision.h: struct rev_cmdline_info {
@@ revision.h: struct rev_info {
- * Whether we read from stdin due to the --stdin option.
+ * How should we handle seeing --stdin?
+ *
-+ * Defaults to reading if we see it with
-+ * REV_INFO_STDIN_CONSUME_ON_OPTION.
++ * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll
++ * attempt to read it if we see the --stdin option.
+ *
-+ * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
-+ * --stdin option.
++ * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
++ * option.
+ */
+ enum rev_info_stdin stdin_handling;
+
@@ revision.h: struct rev_info {
+ * option?
*/
- int read_from_stdin;
-+ int consumed_stdin_per_option;
++ int consumed_stdin;
/* topo-sort */
enum rev_sort_order sort_order;
3: d433d7b24a3 ! 3: 943b1b4c12a pack-objects.c: do stdin parsing via revision.c's API
@@ Metadata
## Commit message ##
pack-objects.c: do stdin parsing via revision.c's API
- Change the fgets(..., stdin) parsing in pack-objects.c to use a
- now-extended version of the rev_info stdin parsing API.
+ Extend the rev_info stdin parsing API to support hooking into its
+ read_revisions_from_stdin() function, and change the custom stdin
+ parsing in pack-objects.c to use it.
- The fgets() loop being refactored away here was first added in Linus's
- c323ac7d9c5 (git-pack-objects: create a packed object representation.,
- 2005-06-25).
+ The pack-objects.c code being refactored away here was first added in
+ Linus's c323ac7d9c5 (git-pack-objects: create a packed object
+ representation., 2005-06-25).
Later on rev-list started doing similar parsing in 42cabc341c4 (Teach
- rev-list an option to read revs from the standard input., 2006-09-05),
- and that code was promoted to a more general API in 1fc561d169a (Move
+ rev-list an option to read revs from the standard input., 2006-09-05).
+ That code was promoted to a more general API in 1fc561d169a (Move
read_revisions_from_stdin from builtin-rev-list.c to revision.c,
2008-07-05).
@@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)
+{
+ int *flags = stdin_line_priv;
+ char *line = line_sb->buf;
-+ size_t len = line_sb->len;
+
-+ if (!len)
-+ return REV_INFO_STDIN_LINE_BREAK;
+ if (*line == '-') {
+ if (!strcmp(line, "--not")) {
+ *flags ^= UNINTERESTING;
@@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
break;
+
+ if (revs->handle_stdin_line) {
-+ int do_break = 0;
+ enum rev_info_stdin_line ret = revs->handle_stdin_line(
+ revs, &sb, revs->stdin_line_priv);
+
+ switch (ret) {
+ case REV_INFO_STDIN_LINE_PROCESS:
+ break;
-+ case REV_INFO_STDIN_LINE_BREAK:
-+ do_break = 1;
-+ break;
+ case REV_INFO_STDIN_LINE_CONTINUE:
+ continue;
+ }
-+ if (do_break)
-+ break;
+ }
+
if (sb.buf[0] == '-') {
if (len == 2 && sb.buf[1] == '-') {
seen_dashdash = 1;
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
+
+ if (!strcmp(arg, "--stdin")) {
+ switch (revs->stdin_handling) {
++ case REV_INFO_STDIN_ALWAYS_READ:
+ case REV_INFO_STDIN_IGNORE:
+ argv[left++] = arg;
+ continue;
+@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
}
+ /*
-+ * We've got always_read_from_stdin but no --stdin (or
-+ * "consumed_stdin_per_option" would be set).
++ * We're asked to ALWAYS_READ from stdin, but no --stdin
++ * option (or "consumed_stdin" would be set).
+ */
-+ if (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&
-+ !revs->consumed_stdin_per_option)
++ if (!revs->consumed_stdin &&
++ revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+ read_revisions_from_stdin(revs, &prune_data);
+
if (prune_data.nr) {
@@ revision.h: struct topo_walk_info;
+enum rev_info_stdin_line {
+ REV_INFO_STDIN_LINE_PROCESS,
-+ REV_INFO_STDIN_LINE_BREAK,
+ REV_INFO_STDIN_LINE_CONTINUE,
+};
+
@@ revision.h: struct topo_walk_info;
struct commit_list *commits;
@@ revision.h: struct rev_info {
*
- * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
- * --stdin option.
+ * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
+ * option.
+ *
+ * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data
+ * on stdin to be read, even if no --stdin option is provided.
@@ revision.h: struct rev_info {
@@ revision.h: struct rev_info {
*/
- int consumed_stdin_per_option;
+ int consumed_stdin;
+ /*
+ * When reading from stdin (see "stdin_handling" above) define
@@ revision.h: struct rev_info {
+ * revision.c's normal processing of the line (after
+ * possibly munging the provided strbuf).
+ *
-+ * - Return REV_INFO_STDIN_LINE_BREAK to process no further
-+ * lines, or anything further from the current line (just
-+ * like REV_INFO_STDIN_LINE_CONTINUE).
-+ *
+ * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
+ * line is fully processed, moving onto the next line (if
+ * any)
4: e59a06c3148 ! 4: 34750ab81cf pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
@@ Commit message
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.
- This also means that we can make the handle_revision_arg() function
- static. Now that the only external user of the API has been migrated
- over to the callback mechanism nothing external to revision.c needs to
- call handle_revision_arg() anymore.
+ I considered making the "--not" parsing be another flag handled by the
+ revision.c API itself, but since there's only one caller who wants
+ this, and the "write_bitmap_index = 0" case is more specific than
+ marking things UNINTERESTING I think it's better to handle it with a
+ more general mechanism.
+
+ These changes means that we can make the handle_revision_arg()
+ function static. Now that the only external user of the API has been
+ migrated over to the callback mechanism nothing external to revision.c
+ needs to call handle_revision_arg() anymore.
That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
@@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)
{
- int *flags = stdin_line_priv;
char *line = line_sb->buf;
- size_t len = line_sb->len;
-@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(
- return REV_INFO_STDIN_LINE_BREAK;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
+- if (*line == '-') {
+- if (!strcmp(line, "--not")) {
- *flags ^= UNINTERESTING;
-+ revs->revarg_flags ^= UNINTERESTING;
- write_bitmap_index = 0;
- return REV_INFO_STDIN_LINE_CONTINUE;
- }
-@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(
- }
+- write_bitmap_index = 0;
+- return REV_INFO_STDIN_LINE_CONTINUE;
+- }
+- if (starts_with(line, "--shallow ")) {
+- struct object_id oid;
+- if (get_oid_hex(line + 10, &oid))
+- die("not an object name '%s'", line + 10);
+- register_shallow(the_repository, &oid);
+- use_bitmap_index = 0;
+- return REV_INFO_STDIN_LINE_CONTINUE;
+- }
++ if (*line != '-')
++ return REV_INFO_STDIN_LINE_PROCESS;
++
++ if (!strcmp(line, "--not")) {
++ revs->revarg_flags ^= UNINTERESTING;
++ write_bitmap_index = 0;
++ return REV_INFO_STDIN_LINE_CONTINUE;
++ } else if (starts_with(line, "--shallow ")) {
++ struct object_id oid;
++ if (get_oid_hex(line + 10, &oid))
++ die("not an object name '%s'", line + 10);
++ register_shallow(the_repository, &oid);
++ use_bitmap_index = 0;
++ return REV_INFO_STDIN_LINE_CONTINUE;
++ } else {
die(_("not a rev '%s'"), line);
}
- if (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))
- die(_("bad revision '%s'"), line);
- return REV_INFO_STDIN_LINE_CONTINUE;
-+ return REV_INFO_STDIN_LINE_PROCESS;
}
static void get_object_list(int ac, const char **av)
@@ revision.h: struct rev_info {
+ * Change "revarg_flags" to affect the subsequent handling
+ * in handle_revision_arg()
+ *
- * - Return REV_INFO_STDIN_LINE_BREAK to process no further
- * lines, or anything further from the current line (just
- * like REV_INFO_STDIN_LINE_CONTINUE).
- *
* - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
-+ *
-+ * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the
* line is fully processed, moving onto the next line (if
* any)
- *
@@ revision.h: struct rev_info {
* around.
*/
--
2.32.0.599.g3967b4fa4ac
next prev parent reply other threads:[~2021-06-21 15:10 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-08 12:16 [PATCH 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
2021-06-08 12:16 ` [PATCH 1/4] upload-pack: run is_repository_shallow() before setup_revisions() Ævar Arnfjörð Bjarmason
2021-06-08 12:16 ` [PATCH 2/4] revision.h: unify "disable_stdin" and "read_from_stdin" Ævar Arnfjörð Bjarmason
2021-06-08 12:16 ` [PATCH 3/4] pack-objects.c: do stdin parsing via revision.c's API Ævar Arnfjörð Bjarmason
2021-06-09 8:10 ` Junio C Hamano
2021-06-08 12:16 ` [PATCH 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS Ævar Arnfjörð Bjarmason
2021-06-17 10:57 ` [PATCH v2 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
2021-06-17 10:57 ` [PATCH v2 1/4] upload-pack: run is_repository_shallow() before setup_revisions() Ævar Arnfjörð Bjarmason
2021-06-17 10:57 ` [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin" Ævar Arnfjörð Bjarmason
2021-06-17 23:44 ` Emily Shaffer
2021-06-18 17:54 ` Jonathan Tan
2021-06-17 10:57 ` [PATCH v2 3/4] pack-objects.c: do stdin parsing via revision.c's API Ævar Arnfjörð Bjarmason
2021-06-18 18:57 ` Jonathan Tan
2021-06-17 10:57 ` [PATCH v2 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS Ævar Arnfjörð Bjarmason
2021-06-21 15:10 ` Ævar Arnfjörð Bjarmason [this message]
2021-06-21 15:10 ` [PATCH v3 1/4] upload-pack: run is_repository_shallow() before setup_revisions() Ævar Arnfjörð Bjarmason
2021-06-21 15:10 ` [PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin" Ævar Arnfjörð Bjarmason
2021-07-08 22:10 ` Junio C Hamano
2021-06-21 15:10 ` [PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API Ævar Arnfjörð Bjarmason
2021-07-08 22:21 ` Junio C Hamano
2021-06-21 15:10 ` [PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS Ævar Arnfjörð Bjarmason
2021-07-08 22:21 ` Junio C Hamano
2021-07-09 11:06 ` [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
2021-07-09 11:06 ` [PATCH v4 1/5] upload-pack: run is_repository_shallow() before setup_revisions() Ævar Arnfjörð Bjarmason
2021-07-09 11:06 ` [PATCH v4 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin" Ævar Arnfjörð Bjarmason
2021-07-09 20:17 ` Junio C Hamano
2021-07-09 11:06 ` [PATCH v4 3/5] revision.[ch]: add a "handle_stdin_line" API Ævar Arnfjörð Bjarmason
2021-07-09 11:06 ` [PATCH v4 4/5] pack-objects.c: do stdin parsing via revision.c's API Ævar Arnfjörð Bjarmason
2021-07-09 11:06 ` [PATCH v4 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 0/5] add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 1/5] upload-pack: run is_repository_shallow() before setup_revisions() Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin" Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 3/5] revision.[ch]: add a "handle_stdin_line" API Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 4/5] pack-objects.c: do stdin parsing via revision.c's API Ævar Arnfjörð Bjarmason
2021-07-26 12:46 ` [PATCH v5 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS Ævar Arnfjörð Bjarmason
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
--cc=zhiyou.jx@alibaba-inc.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.
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 NNTP newsgroup(s).