* [PATCH] Make pull.c match the structural conventions
@ 2025-12-12 2:09 K Jayatheerth
2025-12-12 4:50 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: K Jayatheerth @ 2025-12-12 2:09 UTC (permalink / raw)
To: git; +Cc: jayatheerthkulkarni2005
The builtin sources follow a predictable structure, and pull.c departs
from that pattern by arranging its option table in a way that disrupts
the expected flow of the file. The irregular placement makes the file
harder to read, breaks the visual rhythm shared by other builtins, and
forces readers to jump around to understand how options are handled.
The lack of consistency makes pull.c feel like an outlier rather than
a peer alongside the other commands.
A consistent layout helps readers rely on established mental models,
so bringing pull.c into alignment improves clarity and makes the file
easier to navigate and maintain.
Pull.c, become structured like the other builtin/*.c files, keeping the
option definitions where the reader naturally expects them and restoring
the uniformity of the builtin command layout.
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail•com>
---
I was going through pull.c for a different bug hunt and found this out.
I thought there must be some issue pulling the options table locally to main but
git compiles and also passes the tests and also the CI/CD,
so I can't say if this part was intentional or not.
builtin/pull.c | 285 ++++++++++++++++++++++++-------------------------
1 file changed, 142 insertions(+), 143 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 5ebd529620..97814edbc5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -119,148 +119,6 @@ static int opt_show_forced_updates = -1;
static const char *set_upstream;
static struct strvec opt_fetch = STRVEC_INIT;
-static struct option pull_options[] = {
- /* Shared options */
- OPT__VERBOSITY(&opt_verbosity),
- OPT_PASSTHRU(0, "progress", &opt_progress, NULL,
- N_("force progress reporting"),
- PARSE_OPT_NOARG),
- OPT_CALLBACK_F(0, "recurse-submodules",
- &recurse_submodules_cli, N_("on-demand"),
- N_("control for recursive fetching of submodules"),
- PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
-
- /* Options passed to git-merge or git-rebase */
- OPT_GROUP(N_("Options related to merging")),
- OPT_CALLBACK_F('r', "rebase", &opt_rebase,
- "(false|true|merges|interactive)",
- N_("incorporate changes by rebasing rather than merging"),
- PARSE_OPT_OPTARG, parse_opt_rebase),
- OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
- N_("do not show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG),
- OPT_PASSTHRU(0, "stat", &opt_diffstat, NULL,
- N_("show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "summary", &opt_diffstat, NULL,
- N_("(synonym to --stat)"),
- PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
- OPT_PASSTHRU(0, "compact-summary", &opt_diffstat, NULL,
- N_("show a compact-summary at the end of the merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "log", &opt_log, N_("n"),
- N_("add (at most <n>) entries from shortlog to merge commit message"),
- PARSE_OPT_OPTARG),
- OPT_PASSTHRU(0, "signoff", &opt_signoff, NULL,
- N_("add a Signed-off-by trailer"),
- PARSE_OPT_OPTARG),
- OPT_PASSTHRU(0, "squash", &opt_squash, NULL,
- N_("create a single commit instead of doing a merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "commit", &opt_commit, NULL,
- N_("perform a commit if the merge succeeds (default)"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
- N_("edit message before committing"),
- PARSE_OPT_NOARG),
- OPT_CLEANUP(&cleanup_arg),
- OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
- N_("allow fast-forward"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
- N_("abort if fast-forward is not possible"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG),
- OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
- N_("control use of pre-merge-commit and commit-msg hooks"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
- N_("verify that the named commit has a valid GPG signature"),
- PARSE_OPT_NOARG),
- OPT_BOOL(0, "autostash", &opt_autostash,
- N_("automatically stash/stash pop before and after")),
- OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
- N_("merge strategy to use"),
- 0),
- OPT_PASSTHRU_ARGV('X', "strategy-option", &opt_strategy_opts,
- N_("option=value"),
- N_("option for selected merge strategy"),
- 0),
- OPT_PASSTHRU('S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
- N_("GPG sign commit"),
- PARSE_OPT_OPTARG),
- OPT_SET_INT(0, "allow-unrelated-histories",
- &opt_allow_unrelated_histories,
- N_("allow merging unrelated histories"), 1),
-
- /* Options passed to git-fetch */
- OPT_GROUP(N_("Options related to fetching")),
- OPT_PASSTHRU(0, "all", &opt_all, NULL,
- N_("fetch from all remotes"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('a', "append", &opt_append, NULL,
- N_("append to .git/FETCH_HEAD instead of overwriting"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "upload-pack", &opt_upload_pack, N_("path"),
- N_("path to upload pack on remote end"),
- 0),
- OPT__FORCE(&opt_force, N_("force overwrite of local branch"), 0),
- OPT_PASSTHRU('t', "tags", &opt_tags, NULL,
- N_("fetch all tags and associated objects"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('p', "prune", &opt_prune, NULL,
- N_("prune remote-tracking branches no longer on remote"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
- N_("number of submodules pulled in parallel"),
- PARSE_OPT_OPTARG),
- OPT_BOOL(0, "dry-run", &opt_dry_run,
- N_("dry run")),
- OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
- N_("keep downloaded pack"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "depth", &opt_depth, N_("depth"),
- N_("deepen history of shallow clone"),
- 0),
- OPT_PASSTHRU_ARGV(0, "shallow-since", &opt_fetch, N_("time"),
- N_("deepen history of shallow repository based on time"),
- 0),
- OPT_PASSTHRU_ARGV(0, "shallow-exclude", &opt_fetch, N_("ref"),
- N_("deepen history of shallow clone, excluding ref"),
- 0),
- OPT_PASSTHRU_ARGV(0, "deepen", &opt_fetch, N_("n"),
- N_("deepen history of shallow clone"),
- 0),
- OPT_PASSTHRU(0, "unshallow", &opt_unshallow, NULL,
- N_("convert to a complete repository"),
- PARSE_OPT_NONEG | PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "update-shallow", &opt_update_shallow, NULL,
- N_("accept refs that update .git/shallow"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "refmap", &opt_refmap, N_("refmap"),
- N_("specify fetch refmap"),
- PARSE_OPT_NONEG),
- OPT_PASSTHRU_ARGV('o', "server-option", &opt_fetch,
- N_("server-specific"),
- N_("option to transmit"),
- 0),
- OPT_PASSTHRU('4', "ipv4", &opt_ipv4, NULL,
- N_("use IPv4 addresses only"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
- N_("use IPv6 addresses only"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
- N_("report that we have only objects reachable from this object"),
- 0),
- OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
- N_("check for forced-updates on all updated branches")),
- OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
- N_("set upstream for git pull/fetch"),
- PARSE_OPT_NOARG),
-
- OPT_END()
-};
-
/**
* Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
*/
@@ -1008,7 +866,148 @@ int cmd_pull(int argc,
int can_ff;
int divergent;
int ret;
-
+ static struct option pull_options[] = {
+ /* Shared options */
+ OPT__VERBOSITY(&opt_verbosity),
+ OPT_PASSTHRU(0, "progress", &opt_progress, NULL,
+ N_("force progress reporting"),
+ PARSE_OPT_NOARG),
+ OPT_CALLBACK_F(0, "recurse-submodules",
+ &recurse_submodules_cli, N_("on-demand"),
+ N_("control for recursive fetching of submodules"),
+ PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
+
+ /* Options passed to git-merge or git-rebase */
+ OPT_GROUP(N_("Options related to merging")),
+ OPT_CALLBACK_F('r', "rebase", &opt_rebase,
+ "(false|true|merges|interactive)",
+ N_("incorporate changes by rebasing rather than merging"),
+ PARSE_OPT_OPTARG, parse_opt_rebase),
+ OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
+ N_("do not show a diffstat at the end of the merge"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+ OPT_PASSTHRU(0, "stat", &opt_diffstat, NULL,
+ N_("show a diffstat at the end of the merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "summary", &opt_diffstat, NULL,
+ N_("(synonym to --stat)"),
+ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
+ OPT_PASSTHRU(0, "compact-summary", &opt_diffstat, NULL,
+ N_("show a compact-summary at the end of the merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "log", &opt_log, N_("n"),
+ N_("add (at most <n>) entries from shortlog to merge commit message"),
+ PARSE_OPT_OPTARG),
+ OPT_PASSTHRU(0, "signoff", &opt_signoff, NULL,
+ N_("add a Signed-off-by trailer"),
+ PARSE_OPT_OPTARG),
+ OPT_PASSTHRU(0, "squash", &opt_squash, NULL,
+ N_("create a single commit instead of doing a merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "commit", &opt_commit, NULL,
+ N_("perform a commit if the merge succeeds (default)"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
+ N_("edit message before committing"),
+ PARSE_OPT_NOARG),
+ OPT_CLEANUP(&cleanup_arg),
+ OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
+ N_("allow fast-forward"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
+ N_("abort if fast-forward is not possible"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+ OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
+ N_("control use of pre-merge-commit and commit-msg hooks"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
+ N_("verify that the named commit has a valid GPG signature"),
+ PARSE_OPT_NOARG),
+ OPT_BOOL(0, "autostash", &opt_autostash,
+ N_("automatically stash/stash pop before and after")),
+ OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
+ N_("merge strategy to use"),
+ 0),
+ OPT_PASSTHRU_ARGV('X', "strategy-option", &opt_strategy_opts,
+ N_("option=value"),
+ N_("option for selected merge strategy"),
+ 0),
+ OPT_PASSTHRU('S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
+ N_("GPG sign commit"),
+ PARSE_OPT_OPTARG),
+ OPT_SET_INT(0, "allow-unrelated-histories",
+ &opt_allow_unrelated_histories,
+ N_("allow merging unrelated histories"), 1),
+
+ /* Options passed to git-fetch */
+ OPT_GROUP(N_("Options related to fetching")),
+ OPT_PASSTHRU(0, "all", &opt_all, NULL,
+ N_("fetch from all remotes"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('a', "append", &opt_append, NULL,
+ N_("append to .git/FETCH_HEAD instead of overwriting"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "upload-pack", &opt_upload_pack, N_("path"),
+ N_("path to upload pack on remote end"),
+ 0),
+ OPT__FORCE(&opt_force, N_("force overwrite of local branch"), 0),
+ OPT_PASSTHRU('t', "tags", &opt_tags, NULL,
+ N_("fetch all tags and associated objects"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('p', "prune", &opt_prune, NULL,
+ N_("prune remote-tracking branches no longer on remote"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
+ N_("number of submodules pulled in parallel"),
+ PARSE_OPT_OPTARG),
+ OPT_BOOL(0, "dry-run", &opt_dry_run,
+ N_("dry run")),
+ OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
+ N_("keep downloaded pack"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "depth", &opt_depth, N_("depth"),
+ N_("deepen history of shallow clone"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "shallow-since", &opt_fetch, N_("time"),
+ N_("deepen history of shallow repository based on time"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "shallow-exclude", &opt_fetch, N_("ref"),
+ N_("deepen history of shallow clone, excluding ref"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "deepen", &opt_fetch, N_("n"),
+ N_("deepen history of shallow clone"),
+ 0),
+ OPT_PASSTHRU(0, "unshallow", &opt_unshallow, NULL,
+ N_("convert to a complete repository"),
+ PARSE_OPT_NONEG | PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "update-shallow", &opt_update_shallow, NULL,
+ N_("accept refs that update .git/shallow"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "refmap", &opt_refmap, N_("refmap"),
+ N_("specify fetch refmap"),
+ PARSE_OPT_NONEG),
+ OPT_PASSTHRU_ARGV('o', "server-option", &opt_fetch,
+ N_("server-specific"),
+ N_("option to transmit"),
+ 0),
+ OPT_PASSTHRU('4', "ipv4", &opt_ipv4, NULL,
+ N_("use IPv4 addresses only"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
+ N_("use IPv6 addresses only"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
+ N_("report that we have only objects reachable from this object"),
+ 0),
+ OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
+ N_("check for forced-updates on all updated branches")),
+ OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+ N_("set upstream for git pull/fetch"),
+ PARSE_OPT_NOARG),
+
+ OPT_END()
+ };
+
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
--
2.52.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Make pull.c match the structural conventions
2025-12-12 2:09 [PATCH] Make pull.c match the structural conventions K Jayatheerth
@ 2025-12-12 4:50 ` Junio C Hamano
2025-12-12 7:44 ` [PATCH v2] pull: move options[] array into function scope K Jayatheerth
2025-12-12 14:08 ` [PATCH] Make pull.c match the structural conventions Kristoffer Haugsbakk
0 siblings, 2 replies; 5+ messages in thread
From: Junio C Hamano @ 2025-12-12 4:50 UTC (permalink / raw)
To: K Jayatheerth; +Cc: git
K Jayatheerth <jayatheerthkulkarni2005@gmail•com> writes:
> The builtin sources follow a predictable structure, and pull.c departs
> from that pattern by arranging its option table in a way that disrupts
> the expected flow of the file. The irregular placement makes the file
> harder to read, breaks the visual rhythm shared by other builtins, and
> forces readers to jump around to understand how options are handled.
> The lack of consistency makes pull.c feel like an outlier rather than
> a peer alongside the other commands.
>
> A consistent layout helps readers rely on established mental models,
> so bringing pull.c into alignment improves clarity and makes the file
> easier to navigate and maintain.
>
> Pull.c, become structured like the other builtin/*.c files, keeping the
> option definitions where the reader naturally expects them and restoring
> the uniformity of the builtin command layout.
The above is, what should we say, overhyped? I do not know an
appropriate phrase, but there are subjective judgements without
backing it up with exactly which pattern the code "departs from".
In other words, too many adjectives, so little substance.
I expected something a lot more than a simple change that can be
summarized a lot more concisely, like
Unless there are good reasons, it is customary to have the
options[] array given to parseopt API in the function scope,
not in the file scope.
Make builtin/pull.c:cmd_pull() to follow that convention.
or something.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] pull: move options[] array into function scope
2025-12-12 4:50 ` Junio C Hamano
@ 2025-12-12 7:44 ` K Jayatheerth
2025-12-12 14:08 ` [PATCH] Make pull.c match the structural conventions Kristoffer Haugsbakk
1 sibling, 0 replies; 5+ messages in thread
From: K Jayatheerth @ 2025-12-12 7:44 UTC (permalink / raw)
To: gitster; +Cc: git, jayatheerthkulkarni2005
Unless there are good reasons, it is customary to have the options[]
array used with the parse-options API declared in function scope rather
than at file scope.
Move builtin/pull.c:cmd_pull()’s options[] array into the function to
match that convention.
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail•com>
---
builtin/pull.c | 285 ++++++++++++++++++++++++-------------------------
1 file changed, 142 insertions(+), 143 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 5ebd529620..97814edbc5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -119,148 +119,6 @@ static int opt_show_forced_updates = -1;
static const char *set_upstream;
static struct strvec opt_fetch = STRVEC_INIT;
-static struct option pull_options[] = {
- /* Shared options */
- OPT__VERBOSITY(&opt_verbosity),
- OPT_PASSTHRU(0, "progress", &opt_progress, NULL,
- N_("force progress reporting"),
- PARSE_OPT_NOARG),
- OPT_CALLBACK_F(0, "recurse-submodules",
- &recurse_submodules_cli, N_("on-demand"),
- N_("control for recursive fetching of submodules"),
- PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
-
- /* Options passed to git-merge or git-rebase */
- OPT_GROUP(N_("Options related to merging")),
- OPT_CALLBACK_F('r', "rebase", &opt_rebase,
- "(false|true|merges|interactive)",
- N_("incorporate changes by rebasing rather than merging"),
- PARSE_OPT_OPTARG, parse_opt_rebase),
- OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
- N_("do not show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG),
- OPT_PASSTHRU(0, "stat", &opt_diffstat, NULL,
- N_("show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "summary", &opt_diffstat, NULL,
- N_("(synonym to --stat)"),
- PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
- OPT_PASSTHRU(0, "compact-summary", &opt_diffstat, NULL,
- N_("show a compact-summary at the end of the merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "log", &opt_log, N_("n"),
- N_("add (at most <n>) entries from shortlog to merge commit message"),
- PARSE_OPT_OPTARG),
- OPT_PASSTHRU(0, "signoff", &opt_signoff, NULL,
- N_("add a Signed-off-by trailer"),
- PARSE_OPT_OPTARG),
- OPT_PASSTHRU(0, "squash", &opt_squash, NULL,
- N_("create a single commit instead of doing a merge"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "commit", &opt_commit, NULL,
- N_("perform a commit if the merge succeeds (default)"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
- N_("edit message before committing"),
- PARSE_OPT_NOARG),
- OPT_CLEANUP(&cleanup_arg),
- OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
- N_("allow fast-forward"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
- N_("abort if fast-forward is not possible"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG),
- OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
- N_("control use of pre-merge-commit and commit-msg hooks"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
- N_("verify that the named commit has a valid GPG signature"),
- PARSE_OPT_NOARG),
- OPT_BOOL(0, "autostash", &opt_autostash,
- N_("automatically stash/stash pop before and after")),
- OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
- N_("merge strategy to use"),
- 0),
- OPT_PASSTHRU_ARGV('X', "strategy-option", &opt_strategy_opts,
- N_("option=value"),
- N_("option for selected merge strategy"),
- 0),
- OPT_PASSTHRU('S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
- N_("GPG sign commit"),
- PARSE_OPT_OPTARG),
- OPT_SET_INT(0, "allow-unrelated-histories",
- &opt_allow_unrelated_histories,
- N_("allow merging unrelated histories"), 1),
-
- /* Options passed to git-fetch */
- OPT_GROUP(N_("Options related to fetching")),
- OPT_PASSTHRU(0, "all", &opt_all, NULL,
- N_("fetch from all remotes"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('a', "append", &opt_append, NULL,
- N_("append to .git/FETCH_HEAD instead of overwriting"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "upload-pack", &opt_upload_pack, N_("path"),
- N_("path to upload pack on remote end"),
- 0),
- OPT__FORCE(&opt_force, N_("force overwrite of local branch"), 0),
- OPT_PASSTHRU('t', "tags", &opt_tags, NULL,
- N_("fetch all tags and associated objects"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('p', "prune", &opt_prune, NULL,
- N_("prune remote-tracking branches no longer on remote"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
- N_("number of submodules pulled in parallel"),
- PARSE_OPT_OPTARG),
- OPT_BOOL(0, "dry-run", &opt_dry_run,
- N_("dry run")),
- OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
- N_("keep downloaded pack"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "depth", &opt_depth, N_("depth"),
- N_("deepen history of shallow clone"),
- 0),
- OPT_PASSTHRU_ARGV(0, "shallow-since", &opt_fetch, N_("time"),
- N_("deepen history of shallow repository based on time"),
- 0),
- OPT_PASSTHRU_ARGV(0, "shallow-exclude", &opt_fetch, N_("ref"),
- N_("deepen history of shallow clone, excluding ref"),
- 0),
- OPT_PASSTHRU_ARGV(0, "deepen", &opt_fetch, N_("n"),
- N_("deepen history of shallow clone"),
- 0),
- OPT_PASSTHRU(0, "unshallow", &opt_unshallow, NULL,
- N_("convert to a complete repository"),
- PARSE_OPT_NONEG | PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "update-shallow", &opt_update_shallow, NULL,
- N_("accept refs that update .git/shallow"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "refmap", &opt_refmap, N_("refmap"),
- N_("specify fetch refmap"),
- PARSE_OPT_NONEG),
- OPT_PASSTHRU_ARGV('o', "server-option", &opt_fetch,
- N_("server-specific"),
- N_("option to transmit"),
- 0),
- OPT_PASSTHRU('4', "ipv4", &opt_ipv4, NULL,
- N_("use IPv4 addresses only"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
- N_("use IPv6 addresses only"),
- PARSE_OPT_NOARG),
- OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
- N_("report that we have only objects reachable from this object"),
- 0),
- OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
- N_("check for forced-updates on all updated branches")),
- OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
- N_("set upstream for git pull/fetch"),
- PARSE_OPT_NOARG),
-
- OPT_END()
-};
-
/**
* Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
*/
@@ -1008,7 +866,148 @@ int cmd_pull(int argc,
int can_ff;
int divergent;
int ret;
-
+ static struct option pull_options[] = {
+ /* Shared options */
+ OPT__VERBOSITY(&opt_verbosity),
+ OPT_PASSTHRU(0, "progress", &opt_progress, NULL,
+ N_("force progress reporting"),
+ PARSE_OPT_NOARG),
+ OPT_CALLBACK_F(0, "recurse-submodules",
+ &recurse_submodules_cli, N_("on-demand"),
+ N_("control for recursive fetching of submodules"),
+ PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
+
+ /* Options passed to git-merge or git-rebase */
+ OPT_GROUP(N_("Options related to merging")),
+ OPT_CALLBACK_F('r', "rebase", &opt_rebase,
+ "(false|true|merges|interactive)",
+ N_("incorporate changes by rebasing rather than merging"),
+ PARSE_OPT_OPTARG, parse_opt_rebase),
+ OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
+ N_("do not show a diffstat at the end of the merge"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+ OPT_PASSTHRU(0, "stat", &opt_diffstat, NULL,
+ N_("show a diffstat at the end of the merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "summary", &opt_diffstat, NULL,
+ N_("(synonym to --stat)"),
+ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
+ OPT_PASSTHRU(0, "compact-summary", &opt_diffstat, NULL,
+ N_("show a compact-summary at the end of the merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "log", &opt_log, N_("n"),
+ N_("add (at most <n>) entries from shortlog to merge commit message"),
+ PARSE_OPT_OPTARG),
+ OPT_PASSTHRU(0, "signoff", &opt_signoff, NULL,
+ N_("add a Signed-off-by trailer"),
+ PARSE_OPT_OPTARG),
+ OPT_PASSTHRU(0, "squash", &opt_squash, NULL,
+ N_("create a single commit instead of doing a merge"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "commit", &opt_commit, NULL,
+ N_("perform a commit if the merge succeeds (default)"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
+ N_("edit message before committing"),
+ PARSE_OPT_NOARG),
+ OPT_CLEANUP(&cleanup_arg),
+ OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
+ N_("allow fast-forward"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
+ N_("abort if fast-forward is not possible"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+ OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
+ N_("control use of pre-merge-commit and commit-msg hooks"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
+ N_("verify that the named commit has a valid GPG signature"),
+ PARSE_OPT_NOARG),
+ OPT_BOOL(0, "autostash", &opt_autostash,
+ N_("automatically stash/stash pop before and after")),
+ OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
+ N_("merge strategy to use"),
+ 0),
+ OPT_PASSTHRU_ARGV('X', "strategy-option", &opt_strategy_opts,
+ N_("option=value"),
+ N_("option for selected merge strategy"),
+ 0),
+ OPT_PASSTHRU('S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
+ N_("GPG sign commit"),
+ PARSE_OPT_OPTARG),
+ OPT_SET_INT(0, "allow-unrelated-histories",
+ &opt_allow_unrelated_histories,
+ N_("allow merging unrelated histories"), 1),
+
+ /* Options passed to git-fetch */
+ OPT_GROUP(N_("Options related to fetching")),
+ OPT_PASSTHRU(0, "all", &opt_all, NULL,
+ N_("fetch from all remotes"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('a', "append", &opt_append, NULL,
+ N_("append to .git/FETCH_HEAD instead of overwriting"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "upload-pack", &opt_upload_pack, N_("path"),
+ N_("path to upload pack on remote end"),
+ 0),
+ OPT__FORCE(&opt_force, N_("force overwrite of local branch"), 0),
+ OPT_PASSTHRU('t', "tags", &opt_tags, NULL,
+ N_("fetch all tags and associated objects"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('p', "prune", &opt_prune, NULL,
+ N_("prune remote-tracking branches no longer on remote"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
+ N_("number of submodules pulled in parallel"),
+ PARSE_OPT_OPTARG),
+ OPT_BOOL(0, "dry-run", &opt_dry_run,
+ N_("dry run")),
+ OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
+ N_("keep downloaded pack"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "depth", &opt_depth, N_("depth"),
+ N_("deepen history of shallow clone"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "shallow-since", &opt_fetch, N_("time"),
+ N_("deepen history of shallow repository based on time"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "shallow-exclude", &opt_fetch, N_("ref"),
+ N_("deepen history of shallow clone, excluding ref"),
+ 0),
+ OPT_PASSTHRU_ARGV(0, "deepen", &opt_fetch, N_("n"),
+ N_("deepen history of shallow clone"),
+ 0),
+ OPT_PASSTHRU(0, "unshallow", &opt_unshallow, NULL,
+ N_("convert to a complete repository"),
+ PARSE_OPT_NONEG | PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "update-shallow", &opt_update_shallow, NULL,
+ N_("accept refs that update .git/shallow"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU(0, "refmap", &opt_refmap, N_("refmap"),
+ N_("specify fetch refmap"),
+ PARSE_OPT_NONEG),
+ OPT_PASSTHRU_ARGV('o', "server-option", &opt_fetch,
+ N_("server-specific"),
+ N_("option to transmit"),
+ 0),
+ OPT_PASSTHRU('4', "ipv4", &opt_ipv4, NULL,
+ N_("use IPv4 addresses only"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
+ N_("use IPv6 addresses only"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
+ N_("report that we have only objects reachable from this object"),
+ 0),
+ OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
+ N_("check for forced-updates on all updated branches")),
+ OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+ N_("set upstream for git pull/fetch"),
+ PARSE_OPT_NOARG),
+
+ OPT_END()
+ };
+
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
--
2.52.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Make pull.c match the structural conventions
2025-12-12 4:50 ` Junio C Hamano
2025-12-12 7:44 ` [PATCH v2] pull: move options[] array into function scope K Jayatheerth
@ 2025-12-12 14:08 ` Kristoffer Haugsbakk
2025-12-12 14:48 ` JAYATHEERTH K
1 sibling, 1 reply; 5+ messages in thread
From: Kristoffer Haugsbakk @ 2025-12-12 14:08 UTC (permalink / raw)
To: Junio C Hamano, JAYATHEERTH K; +Cc: git
On Fri, Dec 12, 2025, at 05:50, Junio C Hamano wrote:
> K Jayatheerth <jayatheerthkulkarni2005@gmail•com> writes:
>
>> The builtin sources follow a predictable structure, and pull.c departs
>> from that pattern by arranging its option table in a way that disrupts
>> the expected flow of the file. The irregular placement makes the file
>> harder to read, breaks the visual rhythm shared by other builtins, and
>> forces readers to jump around to understand how options are handled.
>> The lack of consistency makes pull.c feel like an outlier rather than
>> a peer alongside the other commands.
>>
>> A consistent layout helps readers rely on established mental models,
>> so bringing pull.c into alignment improves clarity and makes the file
>> easier to navigate and maintain.
>>
>> Pull.c, become structured like the other builtin/*.c files, keeping the
>> option definitions where the reader naturally expects them and restoring
>> the uniformity of the builtin command layout.
>
>
> The above is, what should we say, overhyped? I do not know an
> appropriate phrase, but there are subjective judgements without
> backing it up with exactly which pattern the code "departs from".
>
> In other words, too many adjectives, so little substance.
I’ve seen some commit messages in the last few months that have too many
adjectives. I’ve never seen that style before.
>
> I expected something a lot more than a simple change that can be
> summarized a lot more concisely, like
>
> Unless there are good reasons, it is customary to have the
> options[] array given to parseopt API in the function scope,
> not in the file scope.
>
> Make builtin/pull.c:cmd_pull() to follow that convention.
>
> or something.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make pull.c match the structural conventions
2025-12-12 14:08 ` [PATCH] Make pull.c match the structural conventions Kristoffer Haugsbakk
@ 2025-12-12 14:48 ` JAYATHEERTH K
0 siblings, 0 replies; 5+ messages in thread
From: JAYATHEERTH K @ 2025-12-12 14:48 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Junio C Hamano, git
>
> I’ve seen some commit messages in the last few months that have too many
> adjectives. I’ve never seen that style before.
>
Ahh, well
I had this format saved from Junio
---
First line should be an order (Not added but add) and then give a line
empty space.
Then
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
---
I tried to force this format here too
That caused a lot of adjective issues.
The patch above is just a copy and a paste
and I get why the commit message felt the way it did.
Thank you
- Jayatheerth
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-12-12 14:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-12 2:09 [PATCH] Make pull.c match the structural conventions K Jayatheerth
2025-12-12 4:50 ` Junio C Hamano
2025-12-12 7:44 ` [PATCH v2] pull: move options[] array into function scope K Jayatheerth
2025-12-12 14:08 ` [PATCH] Make pull.c match the structural conventions Kristoffer Haugsbakk
2025-12-12 14:48 ` JAYATHEERTH K
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox