* [PATCH v3 1/4] upload-pack: run is_repository_shallow() before setup_revisions()
2021-06-21 15:10 ` [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
@ 2021-06-21 15:10 ` Æ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
` (3 subsequent siblings)
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 15:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().
Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 65579e09fe0..a4e2ae059c0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3767,11 +3767,12 @@ static void get_object_list(int ac, const char **av)
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
- setup_revisions(ac, av, &revs, &s_r_opt);
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ setup_revisions(ac, av, &revs, &s_r_opt);
+
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
--
2.32.0.599.g3967b4fa4ac
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin"
2021-06-21 15:10 ` [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
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 ` Æ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
` (2 subsequent siblings)
4 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 15:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
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
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.
The "read" is intended to understood as "I read it", not "you should
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 | 4 ++--
builtin/blame.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/rev-list.c | 2 +-
revision.c | 12 +++++++-----
revision.h | 23 ++++++++++++++++++++---
sequencer.c | 4 ++--
7 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.show_root_diff = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
fp = xfopen(am_path(state, "patch"), "w");
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.no_commit_id = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
argv[argc - 1] = "--";
}
- revs.disable_stdin = 1;
+ revs.stdin_handling = REV_INFO_STDIN_IGNORE;
setup_revisions(argc, argv, &revs, NULL);
if (!revs.pending.nr && is_bare_repository()) {
struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
die(_("index file corrupt"));
opt->abbrev = 0;
opt->diff = 1;
- opt->disable_stdin = 1;
+ opt->stdin_handling = REV_INFO_STDIN_IGNORE;
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.tweak = diff_tree_tweak_rev;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a4..524632ba328 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
(!(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) ||
revs.diff)
usage(rev_list_usage);
diff --git a/revision.c b/revision.c
index 8140561b6c7..a65f9b89e99 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,14 +2741,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (!strcmp(arg, "--stdin")) {
- if (revs->disable_stdin) {
+ 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++)
- die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data);
- continue;
}
if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 17698cb51ac..79a3421cd1f 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
struct oidset;
struct topo_walk_info;
+enum rev_info_stdin {
+ REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
+ REV_INFO_STDIN_IGNORE,
+};
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
int rev_input_given;
/*
- * Whether we read from stdin due to the --stdin option.
+ * How should we handle seeing --stdin?
+ *
+ * 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 the --stdin
+ * option.
+ */
+ enum rev_info_stdin stdin_handling;
+
+ /*
+ * Did we read from stdin due to stdin_handling ==
+ * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+ * option?
*/
- int read_from_stdin;
+ int consumed_stdin;
/* topo-sort */
enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
date_mode_explicit:1,
preserve_subject:1,
encode_email_headers:1;
- unsigned int disable_stdin:1;
/* --show-linear-break */
unsigned int track_linear:1,
track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
log_tree_opt.abbrev = 0;
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
log_tree_opt.no_commit_id = 1;
log_tree_opt.diffopt.file = fopen(buf.buf, "w");
log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format =
DIFF_FORMAT_DIFFSTAT;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
!get_oid(buf.buf, &orig) &&
--
2.32.0.599.g3967b4fa4ac
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin"
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
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-07-08 22:10 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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
> of "disable_stdin", rather we read stdin if we see the "--stdin"
> option.
>
> The "read" is intended to understood as "I read it", not "you should
"I've read it already" would have been easier to understand.
> if (!strcmp(arg, "--stdin")) {
> - if (revs->disable_stdin) {
> + 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;
> }
More importantly, consumed_stdin will start at zero and if it is
found to be non-zero here, it is an immediate error, hence ...
> + /*
> + * Did we read from stdin due to stdin_handling ==
> + * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> + * option?
> */
> - int read_from_stdin;
> + int consumed_stdin;
... it does not have to be a full integer.
Just a single-bit would do.
But none of the above is grave enough to require an update.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API
2021-06-21 15:10 ` [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
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-06-21 15:10 ` Æ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-09 11:06 ` [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
4 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 15:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
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 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).
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).
Since then the API in revision.c has received improvements that have
been missed here. E.g. the arbitrary limit of 1000 bytes was removed
in 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),
and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:
read --stdin with strbuf_getline(), 2015-10-28).
Let's do the same here, as in 6e8d46f9d4b we can remove the "if (len
&& line[len - 1] == '\n')" check, it's now redundant to using
strbuf_getline(), and we get to skip the whole
"warn_on_object_refname_ambiguity" dance. The
read_revisions_from_stdin() function in revision.c we're now using
does it for us.
For now there isn't all that much point in this whole exercises. We
just end up calling setup_revisions() to loop over stdin for us, but
the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we
still need to call handle_revision_arg() ourselves because we'd like
to call it with different flags.
This very light use of the API will be further refined in a subsequent
commit, for now we're just doing the bare minimum to move this
existing code over to the new callback pattern without any functional
changes, and making it as friendly to "git show -w" and "the
--color-moved-ws=allow-indentation-change" mode as possible.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------
revision.c | 22 +++++++++++++++
revision.h | 30 ++++++++++++++++++++
3 files changed, 82 insertions(+), 33 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a4e2ae059c0..f8cae6c305a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3755,15 +3755,40 @@ static void mark_bitmap_preferred_tips(void)
}
}
+static enum rev_info_stdin_line get_object_list_handle_stdin_line(
+ struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
+{
+ int *flags = stdin_line_priv;
+ char *line = line_sb->buf;
+
+ if (*line == '-') {
+ if (!strcmp(line, "--not")) {
+ *flags ^= UNINTERESTING;
+ 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;
+ }
+ 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;
+}
+
static void get_object_list(int ac, const char **av)
{
struct rev_info revs;
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- char line[1000];
int flags = 0;
- int save_warning;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3771,39 +3796,11 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
+ revs.handle_stdin_line = get_object_list_handle_stdin_line;
+ revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
- save_warning = warn_on_object_refname_ambiguity;
- warn_on_object_refname_ambiguity = 0;
-
- while (fgets(line, sizeof(line), stdin) != NULL) {
- int len = strlen(line);
- if (len && line[len - 1] == '\n')
- line[--len] = 0;
- if (!len)
- break;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- flags ^= UNINTERESTING;
- write_bitmap_index = 0;
- 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;
- continue;
- }
- die(_("not a rev '%s'"), line);
- }
- if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
- die(_("bad revision '%s'"), line);
- }
-
- warn_on_object_refname_ambiguity = save_warning;
-
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
return;
diff --git a/revision.c b/revision.c
index a65f9b89e99..f55b3a1caf4 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
int len = sb.len;
if (!len)
break;
+
+ if (revs->handle_stdin_line) {
+ 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_CONTINUE:
+ continue;
+ }
+ }
+
if (sb.buf[0] == '-') {
if (len == 2 && sb.buf[1] == '-') {
seen_dashdash = 1;
@@ -2742,6 +2755,7 @@ 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;
@@ -2789,6 +2803,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
}
+ /*
+ * We're asked to ALWAYS_READ from stdin, but no --stdin
+ * option (or "consumed_stdin" would be set).
+ */
+ if (!revs->consumed_stdin &&
+ revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+ read_revisions_from_stdin(revs, &prune_data);
+
if (prune_data.nr) {
/*
* If we need to introduce the magic "a lone ':' means no
diff --git a/revision.h b/revision.h
index 79a3421cd1f..ff7994bb13d 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ struct topo_walk_info;
enum rev_info_stdin {
REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
REV_INFO_STDIN_IGNORE,
+ REV_INFO_STDIN_ALWAYS_READ,
};
+enum rev_info_stdin_line {
+ REV_INFO_STDIN_LINE_PROCESS,
+ REV_INFO_STDIN_LINE_CONTINUE,
+};
+
+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(
+ struct rev_info *revs, struct strbuf *line, void *stdin_line_priv);
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -126,6 +135,9 @@ struct rev_info {
*
* 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.
*/
enum rev_info_stdin stdin_handling;
@@ -136,6 +148,24 @@ struct rev_info {
*/
int consumed_stdin;
+ /*
+ * When reading from stdin (see "stdin_handling" above) define
+ * a handle_stdin_line function to consume the lines.
+ *
+ * - Return REV_INFO_STDIN_LINE_PROCESS to continue
+ * revision.c's normal processing of the line (after
+ * possibly munging the provided strbuf).
+ *
+ * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
+ * line is fully processed, moving onto the next line (if
+ * any)
+ *
+ * Use the "stdin_line_priv" to optionally pass your own data
+ * around.
+ */
+ rev_info_stdin_line_func handle_stdin_line;
+ void *stdin_line_priv;
+
/* topo-sort */
enum rev_sort_order sort_order;
--
2.32.0.599.g3967b4fa4ac
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API
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
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-07-08 22:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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.
This step, while it might not be introducing new bugs, does not look
anything more than just adding yet another new API for the sake of
adding one (especially, the number of lines added vs removed does
not look so impressive).
Perhaps we'll see why it makes sense in the next step?
> builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------
> revision.c | 22 +++++++++++++++
> revision.h | 30 ++++++++++++++++++++
> 3 files changed, 82 insertions(+), 33 deletions(-)
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
2021-06-21 15:10 ` [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
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-06-21 15:10 ` Æ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
4 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 15:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.
The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.
This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.
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
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).
This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.
I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:
1. It's a common pattern to allow such a "void *" to be used for
callback data, so we might as well follow that pattern here in
anticipation of a future in-tree user.
2. I have patches for such an in-tree user already in a series
that'll be submitted after this one.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 35 +++++++++++++++--------------------
revision.c | 4 ++--
revision.h | 6 ++++--
3 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f8cae6c305a..50f703e1795 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3758,28 +3758,25 @@ static void mark_bitmap_preferred_tips(void)
static enum rev_info_stdin_line get_object_list_handle_stdin_line(
struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
{
- int *flags = stdin_line_priv;
char *line = line_sb->buf;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- *flags ^= UNINTERESTING;
- 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;
}
static void get_object_list(int ac, const char **av)
@@ -3788,7 +3785,6 @@ static void get_object_list(int ac, const char **av)
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- int flags = 0;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3798,7 +3794,6 @@ static void get_object_list(int ac, const char **av)
revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
revs.handle_stdin_line = get_object_list_handle_stdin_line;
- revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index f55b3a1caf4..17121f40c0b 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
return 0;
}
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
{
int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
if (!ret)
@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
}
die("options not supported in --stdin mode");
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
diff --git a/revision.h b/revision.h
index ff7994bb13d..2b96c05ced8 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
* revision.c's normal processing of the line (after
* possibly munging the provided strbuf).
*
+ * Change "revarg_flags" to affect the subsequent handling
+ * in handle_revision_arg()
+ *
* - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
* line is fully processed, moving onto the next line (if
* any)
@@ -164,6 +167,7 @@ struct rev_info {
* around.
*/
rev_info_stdin_line_func handle_stdin_line;
+ int revarg_flags;
void *stdin_line_priv;
/* topo-sort */
@@ -421,8 +425,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const char * const usagestr[]);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
- int flags, unsigned revarg_opt);
/**
* Reset the flags used by the revision walking api. You can use this to do
--
2.32.0.599.g3967b4fa4ac
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
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
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-07-08 22:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> @@ -3758,28 +3758,25 @@ static void mark_bitmap_preferred_tips(void)
> static enum rev_info_stdin_line get_object_list_handle_stdin_line(
> struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
> {
> - int *flags = stdin_line_priv;
> char *line = line_sb->buf;
>
> - if (*line == '-') {
> - if (!strcmp(line, "--not")) {
> - *flags ^= UNINTERESTING;
> - 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;
> }
Now, this does make things slightly more modular (specifically, it
no longer is necessary for handle_revision_arg() to be visible
outside revision.c).
Having said that, it is not immediately obvious if the amount of
code churn by 3/4 and 4/4 is justified. This still looks more like
"we can add a new API, so we added it" than "we needed a new API for
such and such reasons---now we can do this more easily then before".
It's not wrong per-se, but I am not all that impressed. Sorry.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects
2021-06-21 15:10 ` [PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
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-09 11:06 ` Æ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
` (5 more replies)
4 siblings, 6 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
A re-roll of v3[1], this v4 fixes specific issues noted there, but
more importantly I've tried to address Junio's comments to the effect
of "what's the point of this churny thing?"" in [2].
As the new 3/5 explains what I really wanted this for is a new feature
for "git bundle create" that I have working locally, which allows it
to do:
$ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
git bundle create initial.bundle --stdin
As well as things like:
$ git for-each-ref 'refs/remotes/origin' |
sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
git bundle create origin.bundle --stdin
As elaborated on in 3/5 for that it's much better to have an API like
this, but since those changes also needed what I've got in
ab/bundle-updates and ab/bundle-doc (well, not that one really, but
the merge conflict would be painful otherwise) I split this series off
so I wouldn't end up with another 10-20 patch monster.
Junio: I hope that'll convence you to accept this, I can also submit
that "monster", but it'll be harder to read & review sinc so much of
it (e.g. this series, and doc refactoring) will be needed just to get
to the point of implementing that feature.
1. https://lore.kernel.org/git/cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqfswoa1ku.fsf@gitster.g/
Ævar Arnfjörð Bjarmason (5):
upload-pack: run is_repository_shallow() before setup_revisions()
revision.h: refactor "disable_stdin" and "read_from_stdin"
revision.[ch]: add a "handle_stdin_line" API
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 | 39 ++++++++++++++++++++++-----
revision.h | 59 ++++++++++++++++++++++++++++++++++++----
sequencer.c | 4 +--
8 files changed, 120 insertions(+), 53 deletions(-)
Range-diff against v3:
1: dd2efd1cfb9 < -: ----------- pack-objects tests: cover blindspots in stdin handling
2: a9702132385 < -: ----------- pack-objects: fix segfault in --stdin-packs option
3: 3840ac28e8d = 1: 8b2872227c5 upload-pack: run is_repository_shallow() before setup_revisions()
4: 6f69644b808 ! 2: 630feb06650 revision.h: refactor "disable_stdin" and "read_from_stdin"
@@ Commit message
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.
- The "read" is intended to understood as "I read it", not "you should
- 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.
+ The "read" is intended to understood as "I've read it already", not
+ "you should 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:
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
argv[left++] = arg;
continue;
+ case REV_INFO_STDIN_CONSUME_ON_OPTION:
-+ if (revs->consumed_stdin++)
++ if (revs->consumed_stdin)
+ die("--stdin given twice?");
+ read_revisions_from_stdin(revs, &prune_data);
++ revs->consumed_stdin = 1;
+ continue;
}
- if (revs->read_from_stdin++)
@@ revision.h: struct rev_cmdline_info {
struct topo_walk_info;
+enum rev_info_stdin {
-+ REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
++ REV_INFO_STDIN_CONSUME_ON_OPTION,
+ REV_INFO_STDIN_IGNORE,
+};
+
@@ revision.h: struct rev_info {
+ * option?
*/
- int read_from_stdin;
-+ int consumed_stdin;
++ int consumed_stdin:1;
/* topo-sort */
enum rev_sort_order sort_order;
-: ----------- > 3: 60ef49df3b8 revision.[ch]: add a "handle_stdin_line" API
5: 943b1b4c12a ! 4: ebffeb5891f 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
- 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.
+ Use the new "handle_stdin_line" API in revision.c to parse stdin in
+ pack-objects.c, instead of using custom pack-objects.c-specific code
+ to do so.
+
+ This means that we can remove the "if (len && line[len - 1] == '\n')"
+ check, it's now redundant to using strbuf_getline(), and we get to
+ skip the whole "warn_on_object_refname_ambiguity" dance. The
+ read_revisions_from_stdin() function in revision.c we're now using
+ does it for us.
The pack-objects.c code being refactored away here was first added in
Linus's c323ac7d9c5 (git-pack-objects: create a packed object
@@ Commit message
and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:
read --stdin with strbuf_getline(), 2015-10-28).
- Let's do the same here, as in 6e8d46f9d4b we can remove the "if (len
- && line[len - 1] == '\n')" check, it's now redundant to using
- strbuf_getline(), and we get to skip the whole
- "warn_on_object_refname_ambiguity" dance. The
- read_revisions_from_stdin() function in revision.c we're now using
- does it for us.
-
- For now there isn't all that much point in this whole exercises. We
- just end up calling setup_revisions() to loop over stdin for us, but
- the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we
+ For now we've just made setup_revisions() loop over stdin for us, but
+ the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We
still need to call handle_revision_arg() ourselves because we'd like
to call it with different flags.
@@ builtin/pack-objects.c: static void get_object_list(int ac, const char **av)
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
return;
-
- ## revision.c ##
-@@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
- int len = sb.len;
- if (!len)
- break;
-+
-+ if (revs->handle_stdin_line) {
-+ 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_CONTINUE:
-+ continue;
-+ }
-+ }
-+
- 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're asked to ALWAYS_READ from stdin, but no --stdin
-+ * option (or "consumed_stdin" would be set).
-+ */
-+ if (!revs->consumed_stdin &&
-+ revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
-+ read_revisions_from_stdin(revs, &prune_data);
-+
- if (prune_data.nr) {
- /*
- * If we need to introduce the magic "a lone ':' means no
-
- ## revision.h ##
-@@ revision.h: struct topo_walk_info;
- enum rev_info_stdin {
- REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
- REV_INFO_STDIN_IGNORE,
-+ REV_INFO_STDIN_ALWAYS_READ,
- };
-
-+enum rev_info_stdin_line {
-+ REV_INFO_STDIN_LINE_PROCESS,
-+ REV_INFO_STDIN_LINE_CONTINUE,
-+};
-+
-+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(
-+ struct rev_info *revs, struct strbuf *line, void *stdin_line_priv);
-+
- struct rev_info {
- /* Starting list */
- struct commit_list *commits;
-@@ revision.h: struct rev_info {
- *
- * 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.
- */
- enum rev_info_stdin stdin_handling;
-
-@@ revision.h: struct rev_info {
- */
- int consumed_stdin;
-
-+ /*
-+ * When reading from stdin (see "stdin_handling" above) define
-+ * a handle_stdin_line function to consume the lines.
-+ *
-+ * - Return REV_INFO_STDIN_LINE_PROCESS to continue
-+ * revision.c's normal processing of the line (after
-+ * possibly munging the provided strbuf).
-+ *
-+ * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
-+ * line is fully processed, moving onto the next line (if
-+ * any)
-+ *
-+ * Use the "stdin_line_priv" to optionally pass your own data
-+ * around.
-+ */
-+ rev_info_stdin_line_func handle_stdin_line;
-+ void *stdin_line_priv;
-+
- /* topo-sort */
- enum rev_sort_order sort_order;
-
6: 34750ab81cf ! 5: 0c049aee67c pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
@@ Commit message
anticipation of a future in-tree user.
2. I have patches for such an in-tree user already in a series
- that'll be submitted after this one.
+ that'll be submitted after this one. See the reference to "git
+ bundle create --stdin" in the commit that added the "handle_stdin_line"
+ API.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
--
2.32.0.636.g43e71d69cff
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v4 1/5] upload-pack: run is_repository_shallow() before setup_revisions()
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 ` Æ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
` (4 subsequent siblings)
5 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().
Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index df49f656b96..3639d0379ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3774,11 +3774,12 @@ static void get_object_list(int ac, const char **av)
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
- setup_revisions(ac, av, &revs, &s_r_opt);
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ setup_revisions(ac, av, &revs, &s_r_opt);
+
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
--
2.32.0.636.g43e71d69cff
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v4 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin"
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 ` Æ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
` (3 subsequent siblings)
5 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
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
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.
The "read" is intended to understood as "I've read it already", not
"you should 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 | 4 ++--
builtin/blame.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/rev-list.c | 2 +-
revision.c | 13 ++++++++-----
revision.h | 23 ++++++++++++++++++++---
sequencer.c | 4 ++--
7 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.show_root_diff = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
fp = xfopen(am_path(state, "patch"), "w");
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.no_commit_id = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
argv[argc - 1] = "--";
}
- revs.disable_stdin = 1;
+ revs.stdin_handling = REV_INFO_STDIN_IGNORE;
setup_revisions(argc, argv, &revs, NULL);
if (!revs.pending.nr && is_bare_repository()) {
struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
die(_("index file corrupt"));
opt->abbrev = 0;
opt->diff = 1;
- opt->disable_stdin = 1;
+ opt->stdin_handling = REV_INFO_STDIN_IGNORE;
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.tweak = diff_tree_tweak_rev;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a4..524632ba328 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
(!(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) ||
revs.diff)
usage(rev_list_usage);
diff --git a/revision.c b/revision.c
index cddd0542a65..f30b9d40af4 100644
--- a/revision.c
+++ b/revision.c
@@ -2742,14 +2742,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (!strcmp(arg, "--stdin")) {
- if (revs->disable_stdin) {
+ 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);
+ revs->consumed_stdin = 1;
+ continue;
}
- if (revs->read_from_stdin++)
- die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data);
- continue;
}
if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 5c5510d4220..a942b1250ad 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
struct oidset;
struct topo_walk_info;
+enum rev_info_stdin {
+ REV_INFO_STDIN_CONSUME_ON_OPTION,
+ REV_INFO_STDIN_IGNORE,
+};
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
int rev_input_given;
/*
- * Whether we read from stdin due to the --stdin option.
+ * How should we handle seeing --stdin?
+ *
+ * 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 the --stdin
+ * option.
+ */
+ enum rev_info_stdin stdin_handling;
+
+ /*
+ * Did we read from stdin due to stdin_handling ==
+ * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+ * option?
*/
- int read_from_stdin;
+ int consumed_stdin:1;
/* topo-sort */
enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
date_mode_explicit:1,
preserve_subject:1,
encode_email_headers:1;
- unsigned int disable_stdin:1;
/* --show-linear-break */
unsigned int track_linear:1,
track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
log_tree_opt.abbrev = 0;
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
log_tree_opt.no_commit_id = 1;
log_tree_opt.diffopt.file = fopen(buf.buf, "w");
log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format =
DIFF_FORMAT_DIFFSTAT;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
!get_oid(buf.buf, &orig) &&
--
2.32.0.636.g43e71d69cff
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v4 3/5] revision.[ch]: add a "handle_stdin_line" API
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 11:06 ` Æ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
` (2 subsequent siblings)
5 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Extend the rev_info stdin parsing API to support hooking into its
read_revisions_from_stdin() function, in the next commit we'll change
the the custom stdin parsing in pack-objects.c to use it..
For that use-case adding API is barely justified. We'll be able to
make the handle_revision_arg() static in exchange for it, and we'll
avoid the duplicate dance around setting "save_warning" and
"warn_on_object_refname_ambiguity", but we could just continue to do
that ourselves in builtin/pack-objects.c
The real reason to add this is for a change not part of this
series. We'll soon teach "git bundle create" to accept
revision/refname pairs on stdin, and thus do away with the limitation
of it being impossible to create bundles with ref tips that don't
correspond to the state of the current repository. I.e. this will
work:
$ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
git bundle create initial.bundle --stdin
As well as things like:
$ git for-each-ref 'refs/remotes/origin' |
sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
git bundle create origin.bundle --stdin
In order to do that we'll need to modify the lines we consume on stdin
revision.c (which bundle.c uses already), and be assured that that
stripping extra bundle-specific data from them is the only change in
behavior.
That change will be much more complex if bundle.c needs to start doing
its own stdin parsing again outside of revision.c, it was recently
converted to use revision.c's parsing in 5bb0fd2cab5 (bundle:
arguments can be read from stdin, 2021-01-11)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
revision.c | 22 ++++++++++++++++++++++
revision.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/revision.c b/revision.c
index f30b9d40af4..557b7f113a9 100644
--- a/revision.c
+++ b/revision.c
@@ -2120,6 +2120,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
int len = sb.len;
if (!len)
break;
+
+ if (revs->handle_stdin_line) {
+ 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_CONTINUE:
+ continue;
+ }
+ }
+
if (sb.buf[0] == '-') {
if (len == 2 && sb.buf[1] == '-') {
seen_dashdash = 1;
@@ -2743,6 +2756,7 @@ 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;
@@ -2791,6 +2805,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
}
+ /*
+ * We're asked to ALWAYS_READ from stdin, but no --stdin
+ * option (or "consumed_stdin" would be set).
+ */
+ if (!revs->consumed_stdin &&
+ revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+ read_revisions_from_stdin(revs, &prune_data);
+
if (prune_data.nr) {
/*
* If we need to introduce the magic "a lone ':' means no
diff --git a/revision.h b/revision.h
index a942b1250ad..52241c84c5b 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ struct topo_walk_info;
enum rev_info_stdin {
REV_INFO_STDIN_CONSUME_ON_OPTION,
REV_INFO_STDIN_IGNORE,
+ REV_INFO_STDIN_ALWAYS_READ,
};
+enum rev_info_stdin_line {
+ REV_INFO_STDIN_LINE_PROCESS,
+ REV_INFO_STDIN_LINE_CONTINUE,
+};
+
+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(
+ struct rev_info *revs, struct strbuf *line, void *stdin_line_priv);
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -126,6 +135,9 @@ struct rev_info {
*
* 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.
*/
enum rev_info_stdin stdin_handling;
@@ -136,6 +148,24 @@ struct rev_info {
*/
int consumed_stdin:1;
+ /*
+ * When reading from stdin (see "stdin_handling" above) define
+ * a handle_stdin_line function to consume the lines.
+ *
+ * - Return REV_INFO_STDIN_LINE_PROCESS to continue
+ * revision.c's normal processing of the line (after
+ * possibly munging the provided strbuf).
+ *
+ * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
+ * line is fully processed, moving onto the next line (if
+ * any)
+ *
+ * Use the "stdin_line_priv" to optionally pass your own data
+ * around.
+ */
+ rev_info_stdin_line_func handle_stdin_line;
+ void *stdin_line_priv;
+
/* topo-sort */
enum rev_sort_order sort_order;
--
2.32.0.636.g43e71d69cff
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v4 4/5] pack-objects.c: do stdin parsing via revision.c's API
2021-07-09 11:06 ` [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
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 ` Æ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
5 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Use the new "handle_stdin_line" API in revision.c to parse stdin in
pack-objects.c, instead of using custom pack-objects.c-specific code
to do so.
This means that we can remove the "if (len && line[len - 1] == '\n')"
check, it's now redundant to using strbuf_getline(), and we get to
skip the whole "warn_on_object_refname_ambiguity" dance. The
read_revisions_from_stdin() function in revision.c we're now using
does it for us.
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).
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).
Since then the API in revision.c has received improvements that have
been missed here. E.g. the arbitrary limit of 1000 bytes was removed
in 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),
and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:
read --stdin with strbuf_getline(), 2015-10-28).
For now we've just made setup_revisions() loop over stdin for us, but
the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We
still need to call handle_revision_arg() ourselves because we'd like
to call it with different flags.
This very light use of the API will be further refined in a subsequent
commit, for now we're just doing the bare minimum to move this
existing code over to the new callback pattern without any functional
changes, and making it as friendly to "git show -w" and "the
--color-moved-ws=allow-indentation-change" mode as possible.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 33 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3639d0379ed..595adc89c12 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3762,15 +3762,40 @@ static void mark_bitmap_preferred_tips(void)
}
}
+static enum rev_info_stdin_line get_object_list_handle_stdin_line(
+ struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
+{
+ int *flags = stdin_line_priv;
+ char *line = line_sb->buf;
+
+ if (*line == '-') {
+ if (!strcmp(line, "--not")) {
+ *flags ^= UNINTERESTING;
+ 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;
+ }
+ 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;
+}
+
static void get_object_list(int ac, const char **av)
{
struct rev_info revs;
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- char line[1000];
int flags = 0;
- int save_warning;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3778,39 +3803,11 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
+ revs.handle_stdin_line = get_object_list_handle_stdin_line;
+ revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
- save_warning = warn_on_object_refname_ambiguity;
- warn_on_object_refname_ambiguity = 0;
-
- while (fgets(line, sizeof(line), stdin) != NULL) {
- int len = strlen(line);
- if (len && line[len - 1] == '\n')
- line[--len] = 0;
- if (!len)
- break;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- flags ^= UNINTERESTING;
- write_bitmap_index = 0;
- 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;
- continue;
- }
- die(_("not a rev '%s'"), line);
- }
- if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
- die(_("bad revision '%s'"), line);
- }
-
- warn_on_object_refname_ambiguity = save_warning;
-
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
return;
--
2.32.0.636.g43e71d69cff
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v4 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
2021-07-09 11:06 ` [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
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 ` Æ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
5 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-09 11:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.
The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.
This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.
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
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).
This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.
I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:
1. It's a common pattern to allow such a "void *" to be used for
callback data, so we might as well follow that pattern here in
anticipation of a future in-tree user.
2. I have patches for such an in-tree user already in a series
that'll be submitted after this one. See the reference to "git
bundle create --stdin" in the commit that added the "handle_stdin_line"
API.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 35 +++++++++++++++--------------------
revision.c | 4 ++--
revision.h | 6 ++++--
3 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 595adc89c12..9f1d7efeb61 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3765,28 +3765,25 @@ static void mark_bitmap_preferred_tips(void)
static enum rev_info_stdin_line get_object_list_handle_stdin_line(
struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
{
- int *flags = stdin_line_priv;
char *line = line_sb->buf;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- *flags ^= UNINTERESTING;
- 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;
}
static void get_object_list(int ac, const char **av)
@@ -3795,7 +3792,6 @@ static void get_object_list(int ac, const char **av)
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- int flags = 0;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3805,7 +3801,6 @@ static void get_object_list(int ac, const char **av)
revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
revs.handle_stdin_line = get_object_list_handle_stdin_line;
- revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 557b7f113a9..f910b7882ae 100644
--- a/revision.c
+++ b/revision.c
@@ -2090,7 +2090,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
return 0;
}
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
{
int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
if (!ret)
@@ -2140,7 +2140,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
}
die("options not supported in --stdin mode");
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
diff --git a/revision.h b/revision.h
index 52241c84c5b..3da56333960 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
* revision.c's normal processing of the line (after
* possibly munging the provided strbuf).
*
+ * Change "revarg_flags" to affect the subsequent handling
+ * in handle_revision_arg()
+ *
* - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
* line is fully processed, moving onto the next line (if
* any)
@@ -164,6 +167,7 @@ struct rev_info {
* around.
*/
rev_info_stdin_line_func handle_stdin_line;
+ int revarg_flags;
void *stdin_line_priv;
/* topo-sort */
@@ -422,8 +426,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const char * const usagestr[]);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
- int flags, unsigned revarg_opt);
/**
* Reset the flags used by the revision walking api. You can use this to do
--
2.32.0.636.g43e71d69cff
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 0/5] add --stdin parsing API, use in pack-objects
2021-07-09 11:06 ` [PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
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 ` Æ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
` (4 more replies)
5 siblings, 5 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
A trivial re-roll of v4[1] on top of
gitster/bc/rev-list-without-commit-line, which it had a conflict
with. Also includes the s/int/unsigned int/ fixup that Junio applied
on the v4 he picked up.
https://lore.kernel.org/git/cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (5):
upload-pack: run is_repository_shallow() before setup_revisions()
revision.h: refactor "disable_stdin" and "read_from_stdin"
revision.[ch]: add a "handle_stdin_line" API
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 | 39 ++++++++++++++++++++++-----
revision.h | 60 +++++++++++++++++++++++++++++++++++++----
sequencer.c | 4 +--
8 files changed, 121 insertions(+), 53 deletions(-)
Range-diff against v4:
1: f2cc9695306 = 1: b0c7ec31ca9 upload-pack: run is_repository_shallow() before setup_revisions()
2: 50c3b81820b ! 2: 4e5e6620245 revision.h: refactor "disable_stdin" and "read_from_stdin"
@@ revision.h: struct rev_info {
+ * option?
*/
- int read_from_stdin;
-+ int consumed_stdin:1;
++ unsigned int consumed_stdin:1;
/* topo-sort */
enum rev_sort_order sort_order;
@@ revision.h: struct rev_info {
- date_mode_explicit:1,
preserve_subject:1,
- encode_email_headers:1;
+ encode_email_headers:1,
+ include_header:1;
- unsigned int disable_stdin:1;
++
/* --show-linear-break */
unsigned int track_linear:1,
track_first_time:1,
3: 0339af8c39a ! 3: e3d24bd6e8a revision.[ch]: add a "handle_stdin_line" API
@@ revision.h: struct rev_info {
@@ revision.h: struct rev_info {
*/
- int consumed_stdin:1;
+ unsigned int consumed_stdin:1;
+ /*
+ * When reading from stdin (see "stdin_handling" above) define
4: d8080b7fd9c = 4: 4787490a90f pack-objects.c: do stdin parsing via revision.c's API
5: 32a172aa80a = 5: a8b0976649a pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v5 1/5] upload-pack: run is_repository_shallow() before setup_revisions()
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 ` Æ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
` (3 subsequent siblings)
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().
Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de00adbb9e0..1fbaa34f91b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
- setup_revisions(ac, av, &revs, &s_r_opt);
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ setup_revisions(ac, av, &revs, &s_r_opt);
+
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin"
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 ` Æ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
` (2 subsequent siblings)
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
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
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.
The "read" is intended to understood as "I've read it already", not
"you should 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 | 4 ++--
builtin/blame.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/rev-list.c | 2 +-
revision.c | 13 ++++++++-----
revision.h | 24 +++++++++++++++++++++---
sequencer.c | 4 ++--
7 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.show_root_diff = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
fp = xfopen(am_path(state, "patch"), "w");
repo_init_revisions(the_repository, &rev_info, NULL);
rev_info.diff = 1;
- rev_info.disable_stdin = 1;
+ rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
rev_info.no_commit_id = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
argv[argc - 1] = "--";
}
- revs.disable_stdin = 1;
+ revs.stdin_handling = REV_INFO_STDIN_IGNORE;
setup_revisions(argc, argv, &revs, NULL);
if (!revs.pending.nr && is_bare_repository()) {
struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
die(_("index file corrupt"));
opt->abbrev = 0;
opt->diff = 1;
- opt->disable_stdin = 1;
+ opt->stdin_handling = REV_INFO_STDIN_IGNORE;
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.tweak = diff_tree_tweak_rev;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 36cb909ebaa..f1778833401 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -666,7 +666,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
(!(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) ||
revs.diff)
usage(rev_list_usage);
diff --git a/revision.c b/revision.c
index 8140561b6c7..50909339a59 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,14 +2741,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (!strcmp(arg, "--stdin")) {
- if (revs->disable_stdin) {
+ 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);
+ revs->consumed_stdin = 1;
+ continue;
}
- if (revs->read_from_stdin++)
- die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data);
- continue;
}
if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 7464434f600..99458cc0647 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
struct oidset;
struct topo_walk_info;
+enum rev_info_stdin {
+ REV_INFO_STDIN_CONSUME_ON_OPTION,
+ REV_INFO_STDIN_IGNORE,
+};
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
int rev_input_given;
/*
- * Whether we read from stdin due to the --stdin option.
+ * How should we handle seeing --stdin?
+ *
+ * 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 the --stdin
+ * option.
+ */
+ enum rev_info_stdin stdin_handling;
+
+ /*
+ * Did we read from stdin due to stdin_handling ==
+ * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+ * option?
*/
- int read_from_stdin;
+ unsigned int consumed_stdin:1;
/* topo-sort */
enum rev_sort_order sort_order;
@@ -217,7 +235,7 @@ struct rev_info {
preserve_subject:1,
encode_email_headers:1,
include_header:1;
- unsigned int disable_stdin:1;
+
/* --show-linear-break */
unsigned int track_linear:1,
track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
log_tree_opt.abbrev = 0;
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
log_tree_opt.no_commit_id = 1;
log_tree_opt.diffopt.file = fopen(buf.buf, "w");
log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
log_tree_opt.diff = 1;
log_tree_opt.diffopt.output_format =
DIFF_FORMAT_DIFFSTAT;
- log_tree_opt.disable_stdin = 1;
+ log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
!get_oid(buf.buf, &orig) &&
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 3/5] revision.[ch]: add a "handle_stdin_line" API
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 ` Æ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
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Extend the rev_info stdin parsing API to support hooking into its
read_revisions_from_stdin() function, in the next commit we'll change
the the custom stdin parsing in pack-objects.c to use it..
For that use-case adding API is barely justified. We'll be able to
make the handle_revision_arg() static in exchange for it, and we'll
avoid the duplicate dance around setting "save_warning" and
"warn_on_object_refname_ambiguity", but we could just continue to do
that ourselves in builtin/pack-objects.c
The real reason to add this is for a change not part of this
series. We'll soon teach "git bundle create" to accept
revision/refname pairs on stdin, and thus do away with the limitation
of it being impossible to create bundles with ref tips that don't
correspond to the state of the current repository. I.e. this will
work:
$ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
git bundle create initial.bundle --stdin
As well as things like:
$ git for-each-ref 'refs/remotes/origin' |
sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
git bundle create origin.bundle --stdin
In order to do that we'll need to modify the lines we consume on stdin
revision.c (which bundle.c uses already), and be assured that that
stripping extra bundle-specific data from them is the only change in
behavior.
That change will be much more complex if bundle.c needs to start doing
its own stdin parsing again outside of revision.c, it was recently
converted to use revision.c's parsing in 5bb0fd2cab5 (bundle:
arguments can be read from stdin, 2021-01-11)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
revision.c | 22 ++++++++++++++++++++++
revision.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/revision.c b/revision.c
index 50909339a59..3f6ab834aff 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
int len = sb.len;
if (!len)
break;
+
+ if (revs->handle_stdin_line) {
+ 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_CONTINUE:
+ continue;
+ }
+ }
+
if (sb.buf[0] == '-') {
if (len == 2 && sb.buf[1] == '-') {
seen_dashdash = 1;
@@ -2742,6 +2755,7 @@ 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;
@@ -2790,6 +2804,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
}
+ /*
+ * We're asked to ALWAYS_READ from stdin, but no --stdin
+ * option (or "consumed_stdin" would be set).
+ */
+ if (!revs->consumed_stdin &&
+ revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+ read_revisions_from_stdin(revs, &prune_data);
+
if (prune_data.nr) {
/*
* If we need to introduce the magic "a lone ':' means no
diff --git a/revision.h b/revision.h
index 99458cc0647..644b7c8a217 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ struct topo_walk_info;
enum rev_info_stdin {
REV_INFO_STDIN_CONSUME_ON_OPTION,
REV_INFO_STDIN_IGNORE,
+ REV_INFO_STDIN_ALWAYS_READ,
};
+enum rev_info_stdin_line {
+ REV_INFO_STDIN_LINE_PROCESS,
+ REV_INFO_STDIN_LINE_CONTINUE,
+};
+
+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(
+ struct rev_info *revs, struct strbuf *line, void *stdin_line_priv);
+
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -126,6 +135,9 @@ struct rev_info {
*
* 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.
*/
enum rev_info_stdin stdin_handling;
@@ -136,6 +148,24 @@ struct rev_info {
*/
unsigned int consumed_stdin:1;
+ /*
+ * When reading from stdin (see "stdin_handling" above) define
+ * a handle_stdin_line function to consume the lines.
+ *
+ * - Return REV_INFO_STDIN_LINE_PROCESS to continue
+ * revision.c's normal processing of the line (after
+ * possibly munging the provided strbuf).
+ *
+ * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
+ * line is fully processed, moving onto the next line (if
+ * any)
+ *
+ * Use the "stdin_line_priv" to optionally pass your own data
+ * around.
+ */
+ rev_info_stdin_line_func handle_stdin_line;
+ void *stdin_line_priv;
+
/* topo-sort */
enum rev_sort_order sort_order;
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 4/5] pack-objects.c: do stdin parsing via revision.c's API
2021-07-26 12:46 ` [PATCH v5 0/5] add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
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 ` Æ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
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Use the new "handle_stdin_line" API in revision.c to parse stdin in
pack-objects.c, instead of using custom pack-objects.c-specific code
to do so.
This means that we can remove the "if (len && line[len - 1] == '\n')"
check, it's now redundant to using strbuf_getline(), and we get to
skip the whole "warn_on_object_refname_ambiguity" dance. The
read_revisions_from_stdin() function in revision.c we're now using
does it for us.
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).
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).
Since then the API in revision.c has received improvements that have
been missed here. E.g. the arbitrary limit of 1000 bytes was removed
in 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),
and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:
read --stdin with strbuf_getline(), 2015-10-28).
For now we've just made setup_revisions() loop over stdin for us, but
the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We
still need to call handle_revision_arg() ourselves because we'd like
to call it with different flags.
This very light use of the API will be further refined in a subsequent
commit, for now we're just doing the bare minimum to move this
existing code over to the new callback pattern without any functional
changes, and making it as friendly to "git show -w" and "the
--color-moved-ws=allow-indentation-change" mode as possible.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 33 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1fbaa34f91b..35d5247f85a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3745,15 +3745,40 @@ static void mark_bitmap_preferred_tips(void)
}
}
+static enum rev_info_stdin_line get_object_list_handle_stdin_line(
+ struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
+{
+ int *flags = stdin_line_priv;
+ char *line = line_sb->buf;
+
+ if (*line == '-') {
+ if (!strcmp(line, "--not")) {
+ *flags ^= UNINTERESTING;
+ 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;
+ }
+ 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;
+}
+
static void get_object_list(int ac, const char **av)
{
struct rev_info revs;
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- char line[1000];
int flags = 0;
- int save_warning;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3761,39 +3786,11 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
+ revs.handle_stdin_line = get_object_list_handle_stdin_line;
+ revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
- save_warning = warn_on_object_refname_ambiguity;
- warn_on_object_refname_ambiguity = 0;
-
- while (fgets(line, sizeof(line), stdin) != NULL) {
- int len = strlen(line);
- if (len && line[len - 1] == '\n')
- line[--len] = 0;
- if (!len)
- break;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- flags ^= UNINTERESTING;
- write_bitmap_index = 0;
- 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;
- continue;
- }
- die(_("not a rev '%s'"), line);
- }
- if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
- die(_("bad revision '%s'"), line);
- }
-
- warn_on_object_refname_ambiguity = save_warning;
-
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
return;
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
2021-07-26 12:46 ` [PATCH v5 0/5] add --stdin parsing API, use in pack-objects Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
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 ` Ævar Arnfjörð Bjarmason
4 siblings, 0 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Jiang Xin, Emily Shaffer, Jonathan Tan,
Taylor Blau, Ævar Arnfjörð Bjarmason
Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.
The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.
This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.
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
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).
This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.
I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:
1. It's a common pattern to allow such a "void *" to be used for
callback data, so we might as well follow that pattern here in
anticipation of a future in-tree user.
2. I have patches for such an in-tree user already in a series
that'll be submitted after this one. See the reference to "git
bundle create --stdin" in the commit that added the "handle_stdin_line"
API.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/pack-objects.c | 35 +++++++++++++++--------------------
revision.c | 4 ++--
revision.h | 6 ++++--
3 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 35d5247f85a..06a085a9a2a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3748,28 +3748,25 @@ static void mark_bitmap_preferred_tips(void)
static enum rev_info_stdin_line get_object_list_handle_stdin_line(
struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
{
- int *flags = stdin_line_priv;
char *line = line_sb->buf;
- if (*line == '-') {
- if (!strcmp(line, "--not")) {
- *flags ^= UNINTERESTING;
- 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;
}
static void get_object_list(int ac, const char **av)
@@ -3778,7 +3775,6 @@ static void get_object_list(int ac, const char **av)
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
- int flags = 0;
repo_init_revisions(the_repository, &revs, NULL);
save_commit_buffer = 0;
@@ -3788,7 +3784,6 @@ static void get_object_list(int ac, const char **av)
revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
revs.handle_stdin_line = get_object_list_handle_stdin_line;
- revs.stdin_line_priv = &flags;
setup_revisions(ac, av, &revs, &s_r_opt);
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 3f6ab834aff..4164a4fcd11 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
return 0;
}
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
{
int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
if (!ret)
@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
}
die("options not supported in --stdin mode");
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
diff --git a/revision.h b/revision.h
index 644b7c8a217..fd7fdbe42e8 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
* revision.c's normal processing of the line (after
* possibly munging the provided strbuf).
*
+ * Change "revarg_flags" to affect the subsequent handling
+ * in handle_revision_arg()
+ *
* - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
* line is fully processed, moving onto the next line (if
* any)
@@ -164,6 +167,7 @@ struct rev_info {
* around.
*/
rev_info_stdin_line_func handle_stdin_line;
+ int revarg_flags;
void *stdin_line_priv;
/* topo-sort */
@@ -423,8 +427,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const char * const usagestr[]);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
- int flags, unsigned revarg_opt);
/**
* Reset the flags used by the revision walking api. You can use this to do
--
2.32.0.956.g6b0c84ceda8
^ permalink raw reply related [flat|nested] 35+ messages in thread