[v2,14/14] update-index: remove static globals from callbacks

Commit Message

Derrick Stolee Jan. 5, 2021, 4:43 a.m. UTC

From: Derrick Stolee <[e-mail]>

In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We remove these static globals inside the
option parsing callbacks, which are the final uses in update-index.

The callbacks cannot change their method signature, so we must use the
value member of 'struct option', assigned in the array of option macros.
There are several callback methods that require at least one of 'repo'
and 'istate', but they use a variety of different data types for the
callback value.

Unify these callback methods to use a consistent 'struct callback_data'
that contains 'repo' and 'istate', ready to use. This takes the place of
the previous 'struct refresh_params' which served only to group the
'flags' and 'has_errors' ints. We also collect other one-off settings,
but only those that require access to the index or repository in their
operation.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/update-index.c | 110 ++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 50 deletions(-)

Comments

Eric Sunshine Jan. 7, 2021, 5:09 a.m. UTC | #1

On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> In order to remove index compatibility macros cleanly, we relied upon
> static globals 'repo' and 'istate' to be pointers to the_repository and
> the_index, respectively. We remove these static globals inside the
> option parsing callbacks, which are the final uses in update-index.
>
> The callbacks cannot change their method signature, so we must use the
> value member of 'struct option', assigned in the array of option macros.
> There are several callback methods that require at least one of 'repo'
> and 'istate', but they use a variety of different data types for the
> callback value.
>
> Unify these callback methods to use a consistent 'struct callback_data'
> that contains 'repo' and 'istate', ready to use. This takes the place of
> the previous 'struct refresh_params' which served only to group the
> 'flags' and 'has_errors' ints. We also collect other one-off settings,
> but only those that require access to the index or repository in their
> operation.

Makes sense. The patch itself is necessarily a bit noisy, but there's
nothing particularly complicated in that noise.

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> @@ -784,19 +784,21 @@ static int do_reupdate(struct repository *repo,
> -struct refresh_params {
> +struct callback_data {
> +       struct repository *repo;
> +       struct index_state *istate;
> +
>         unsigned int flags;
> -       int *has_errors;
> +       unsigned int has_errors;
> +       unsigned nul_term_line;
> +       unsigned read_from_stdin;
>  };

The only mildly unexpected thing here is that `has_errors` is now a
simple value rather than a pointer to a value, but you handle that
easily enough by always accessing `has_error` directly from the
structure, even within the function in which `has_error` used to be a
local variable. Fine.

> @@ -818,7 +820,7 @@ static int really_refresh_callback(const struct option *opt,
>  static int chmod_callback(const struct option *opt,
> -                               const char *arg, int unset)
> +                         const char *arg, int unset)
> @@ -829,11 +831,12 @@ static int chmod_callback(const struct option *opt,
>  static int resolve_undo_clear_callback(const struct option *opt,
> -                               const char *arg, int unset)
> +                                      const char *arg, int unset)

A couple drive-by indentation fixes. Okay.

> @@ -1098,8 +1103,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> -       istate = repo->index;
> +       cd.repo = repo;
> +       cd.istate = istate = repo->index;

Will there ever be a case in which `cd.istate` will be different from
`cd.repo->index`? If not, then we could get by with having only
`cd.repo`; callers requiring access to `istate` can fetch it from
`cd.repo`. If, on the other hand, `cd.istate` can be different from
`cd.repo->istate` -- or if that might become a possibility in the
future -- then having `cd.istate` makes sense. Not a big deal, though.
Just generally curious about it.

Derrick Stolee Jan. 7, 2021, 11:19 a.m. UTC | #2

On 1/7/2021 12:09 AM, Eric Sunshine wrote:
> On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>> @@ -1098,8 +1103,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> -       istate = repo->index;
>> +       cd.repo = repo;
>> +       cd.istate = istate = repo->index;
> 
> Will there ever be a case in which `cd.istate` will be different from
> `cd.repo->index`? If not, then we could get by with having only
> `cd.repo`; callers requiring access to `istate` can fetch it from
> `cd.repo`. If, on the other hand, `cd.istate` can be different from
> `cd.repo->istate` -- or if that might become a possibility in the
> future -- then having `cd.istate` makes sense. Not a big deal, though.
> Just generally curious about it.
 
I don't believe that 'istate' and 'repo->index' will ever be
different in this file. This includes the members of the
callback_data struct, but also the method parameters throughout.

Mostly, this could be seen as an artifact of how we got here:

1. References to the_index or other compatibility macros were
   converted to use the static global 'istate'.

2. References to the static global 'istate' were replaced with
   method parameters for everything except these callbacks.

3. These callbacks were updated to use 'cd.istate' instead of
   the (now defunct) static global 'istate'.

It could be possible to replace all references to 'istate' with
'repo->index' but the patches get slightly more messy. I also
think the code looks messier, but you do make a good point that
there is no concrete reason to separate the two.

Thanks,
-Stolee

Eric Sunshine Jan. 7, 2021, 6:53 p.m. UTC | #3

On Thu, Jan 7, 2021 at 6:19 AM Derrick Stolee <[e-mail]> wrote:
> On 1/7/2021 12:09 AM, Eric Sunshine wrote:
> > Will there ever be a case in which `cd.istate` will be different from
> > `cd.repo->index`? If not, then we could get by with having only
> > `cd.repo`; callers requiring access to `istate` can fetch it from
> > `cd.repo`. If, on the other hand, `cd.istate` can be different from
> > `cd.repo->istate` -- or if that might become a possibility in the
> > future -- then having `cd.istate` makes sense. Not a big deal, though.
> > Just generally curious about it.
>
> I don't believe that 'istate' and 'repo->index' will ever be
> different in this file. This includes the members of the
> callback_data struct, but also the method parameters throughout.
>
> It could be possible to replace all references to 'istate' with
> 'repo->index' but the patches get slightly more messy. I also
> think the code looks messier, but you do make a good point that
> there is no concrete reason to separate the two.

I agree that it would make the code a bit noisier (to read) if
`istate` is eliminated from the callback structure, however, even
though I didn't originally feel strongly one way or the other about
having both `repo` and `istate` in the structure, I'm now leaning more
toward seeing `istate` eliminated. My one (big) concern with `istate`
is that it confuses readers into wondering whether `istate` and
`repo->istate` will ever be different. One way to avoid such confusion
would be to leave a comment in the code stating that the two values
will always be the same. The other way, of course, is to eliminate
`istate` from the structure altogether. I don't want to make more work
for you, but the more I think about it, the more I feel that removing
`istate` is the sensible thing to do. (And it doesn't require an extra
patch -- it can just be how this patch is crafted -- without ever
introducing `istate` to the structure in the first place.)

Junio C Hamano Jan. 7, 2021, 7:57 p.m. UTC | #4

Eric Sunshine <[e-mail]> writes:

>> It could be possible to replace all references to 'istate' with
>> 'repo->index' but the patches get slightly more messy. I also
>> think the code looks messier, but you do make a good point that
>> there is no concrete reason to separate the two.
>
> I agree that it would make the code a bit noisier (to read) if
> `istate` is eliminated from the callback structure, however, even
> though I didn't originally feel strongly one way or the other about
> having both `repo` and `istate` in the structure, I'm now leaning more
> toward seeing `istate` eliminated. My one (big) concern with `istate`
> is that it confuses readers into wondering whether `istate` and
> `repo->istate` will ever be different.

Some applications may want to work on more than one in-core index at
the same time (perhaps a merge strategy may want to keep a copy of
the original index and update a second copy with the result of the
merge), and it may be useful for such applications if 'repo->istate'
does not have to be the in-core index instance to be worked on.  So
things that go in libgit.a may want to allow such distinction.

But what goes in builtin/ is a different story.  As long as this
application has no need for such a feature and will always work on
the primary in-core index, prepared for the in-core repository
structure for convenience, it may not worth it to support such a
feature that no callers benefit from.

Thanks.

Derrick Stolee Jan. 8, 2021, 1:52 a.m. UTC | #5

On 1/7/2021 2:57 PM, Junio C Hamano wrote:
> Eric Sunshine <[e-mail]> writes:
> 
>>> It could be possible to replace all references to 'istate' with
>>> 'repo->index' but the patches get slightly more messy. I also
>>> think the code looks messier, but you do make a good point that
>>> there is no concrete reason to separate the two.
>>
>> I agree that it would make the code a bit noisier (to read) if
>> `istate` is eliminated from the callback structure, however, even
>> though I didn't originally feel strongly one way or the other about
>> having both `repo` and `istate` in the structure, I'm now leaning more
>> toward seeing `istate` eliminated. My one (big) concern with `istate`
>> is that it confuses readers into wondering whether `istate` and
>> `repo->istate` will ever be different.
> 
> Some applications may want to work on more than one in-core index at
> the same time (perhaps a merge strategy may want to keep a copy of
> the original index and update a second copy with the result of the
> merge), and it may be useful for such applications if 'repo->istate'
> does not have to be the in-core index instance to be worked on.  So
> things that go in libgit.a may want to allow such distinction.
> 
> But what goes in builtin/ is a different story.  As long as this
> application has no need for such a feature and will always work on
> the primary in-core index, prepared for the in-core repository
> structure for convenience, it may not worth it to support such a
> feature that no callers benefit from.

I'll try to restructure my patches to do the following order:

1. replace compatibility macros with static global references, except
   i. use 'istate' in the methods that don't need a repository.
  ii. use 'repo->index' in the methods that need a repository.

2. replace static globals with method parameters.
   i. drop 'istate' static global with method parameters. Methods that
      have a repo will pass 'repo->index' to these methods.
  ii. drop 'repo' static global with method parameters.

3. replace static globals in callback methods using 'repo->index',
   where 'repo' is a member of the callback_data struct.

That should keep the structure as presented in v2 while also avoiding
this question of "can istate differ from repo->index?"

Thanks,
-Stolee

Patch

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fb8d7879783..258db619a40 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -784,19 +784,21 @@  static int do_reupdate(struct repository *repo,
 	return 0;
 }
 
-struct refresh_params {
+struct callback_data {
+	struct repository *repo;
+	struct index_state *istate;
+
 	unsigned int flags;
-	int *has_errors;
+	unsigned int has_errors;
+	unsigned nul_term_line;
+	unsigned read_from_stdin;
 };
 
-static struct repository *repo;
-static struct index_state *istate;
-
-static int refresh(struct refresh_params *o, unsigned int flag)
+static int refresh(struct callback_data *cd, unsigned int flag)
 {
 	setup_work_tree();
-	repo_read_index(repo);
-	*o->has_errors |= refresh_index(istate, o->flags | flag,
+	repo_read_index(cd->repo);
+	cd->has_errors |= refresh_index(cd->istate, cd->flags | flag,
 					NULL, NULL, NULL);
 	return 0;
 }
@@ -818,7 +820,7 @@  static int really_refresh_callback(const struct option *opt,
 }
 
 static int chmod_callback(const struct option *opt,
-				const char *arg, int unset)
+			  const char *arg, int unset)
 {
 	char *flip = opt->value;
 	BUG_ON_OPT_NEG(unset);
@@ -829,11 +831,12 @@  static int chmod_callback(const struct option *opt,
 }
 
 static int resolve_undo_clear_callback(const struct option *opt,
-				const char *arg, int unset)
+				       const char *arg, int unset)
 {
+	struct callback_data *cd = opt->value;
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
-	resolve_undo_clear_index(istate);
+	resolve_undo_clear_index(cd->istate);
 	return 0;
 }
 
@@ -868,12 +871,13 @@  static enum parse_opt_result cacheinfo_callback(
 	struct object_id oid;
 	unsigned int mode;
 	const char *path;
+	struct callback_data *cd = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
 	if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, &oid, &path)) {
-		if (add_cacheinfo(istate, mode, &oid, path, 0))
+		if (add_cacheinfo(cd->istate, mode, &oid, path, 0))
 			die("git update-index: --cacheinfo cannot add %s", path);
 		ctx->argv++;
 		ctx->argc--;
@@ -883,7 +887,7 @@  static enum parse_opt_result cacheinfo_callback(
 		return error("option 'cacheinfo' expects <mode>,<sha1>,<path>");
 	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
 	    get_oid_hex(*++ctx->argv, &oid) ||
-	    add_cacheinfo(istate, mode, &oid, *++ctx->argv, 0))
+	    add_cacheinfo(cd->istate, mode, &oid, *++ctx->argv, 0))
 		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);
 	ctx->argc -= 3;
 	return 0;
@@ -893,7 +897,7 @@  static enum parse_opt_result stdin_cacheinfo_callback(
 	struct parse_opt_ctx_t *ctx, const struct option *opt,
 	const char *arg, int unset)
 {
-	int *nul_term_line = opt->value;
+	struct callback_data *cd = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
@@ -901,7 +905,7 @@  static enum parse_opt_result stdin_cacheinfo_callback(
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);
 	allow_add = allow_replace = allow_remove = 1;
-	read_index_info(istate, *nul_term_line);
+	read_index_info(cd->istate, cd->nul_term_line);
 	return 0;
 }
 
@@ -909,14 +913,14 @@  static enum parse_opt_result stdin_callback(
 	struct parse_opt_ctx_t *ctx, const struct option *opt,
 	const char *arg, int unset)
 {
-	int *read_from_stdin = opt->value;
+	struct callback_data *cd = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);
-	*read_from_stdin = 1;
+	cd->read_from_stdin = 1;
 	return 0;
 }
 
@@ -924,17 +928,17 @@  static enum parse_opt_result unresolve_callback(
 	struct parse_opt_ctx_t *ctx, const struct option *opt,
 	const char *arg, int unset)
 {
-	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
+	struct callback_data *cd = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
-	*has_errors = do_unresolve(repo, istate, ctx->argc, ctx->argv,
-				   prefix, prefix ? strlen(prefix) : 0);
-	if (*has_errors)
-		istate->cache_changed = 0;
+	cd->has_errors = do_unresolve(cd->repo, cd->istate, ctx->argc, ctx->argv,
+				      prefix, prefix ? strlen(prefix) : 0);
+	if (cd->has_errors)
+		cd->istate->cache_changed = 0;
 
 	ctx->argv += ctx->argc - 1;
 	ctx->argc = 1;
@@ -945,17 +949,18 @@  static enum parse_opt_result reupdate_callback(
 	struct parse_opt_ctx_t *ctx, const struct option *opt,
 	const char *arg, int unset)
 {
-	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
+	struct callback_data *cd = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
 	setup_work_tree();
-	*has_errors = do_reupdate(repo, istate, ctx->argc, ctx->argv, prefix);
-	if (*has_errors)
-		istate->cache_changed = 0;
+	cd->has_errors = do_reupdate(cd->repo, cd->istate,
+				     ctx->argc, ctx->argv, prefix);
+	if (cd->has_errors)
+		cd->istate->cache_changed = 0;
 
 	ctx->argv += ctx->argc - 1;
 	ctx->argc = 1;
@@ -964,13 +969,13 @@  static enum parse_opt_result reupdate_callback(
 
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
-	int newfd, entries, has_errors = 0, nul_term_line = 0;
+	struct repository *repo = the_repository;
+	struct callback_data cd;
+	int newfd, entries;
 	enum uc_mode untracked_cache = UC_UNSPECIFIED;
-	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	int preferred_index_format = 0;
 	char set_executable_bit = 0;
-	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
 	int force_write = 0;
@@ -979,11 +984,13 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	struct parse_opt_ctx_t ctx;
 	strbuf_getline_fn getline_fn;
 	int parseopt_state = PARSE_OPT_UNKNOWN;
+	struct index_state *istate;
+
 	struct option options[] = {
-		OPT_BIT('q', NULL, &refresh_args.flags,
+		OPT_BIT('q', NULL, &cd.flags,
 			N_("continue refresh even when index needs update"),
 			REFRESH_QUIET),
-		OPT_BIT(0, "ignore-submodules", &refresh_args.flags,
+		OPT_BIT(0, "ignore-submodules", &cd.flags,
 			N_("refresh: ignore submodules"),
 			REFRESH_IGNORE_SUBMODULES),
 		OPT_SET_INT(0, "add", &allow_add,
@@ -992,18 +999,18 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("let files replace directories and vice-versa"), 1),
 		OPT_SET_INT(0, "remove", &allow_remove,
 			N_("notice files missing from worktree"), 1),
-		OPT_BIT(0, "unmerged", &refresh_args.flags,
+		OPT_BIT(0, "unmerged", &cd.flags,
 			N_("refresh even if index contains unmerged entries"),
 			REFRESH_UNMERGED),
-		OPT_CALLBACK_F(0, "refresh", &refresh_args, NULL,
+		OPT_CALLBACK_F(0, "refresh", &cd, NULL,
 			N_("refresh stat information"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			refresh_callback),
-		OPT_CALLBACK_F(0, "really-refresh", &refresh_args, NULL,
+		OPT_CALLBACK_F(0, "really-refresh", &cd, NULL,
 			N_("like --refresh, but ignore assume-unchanged setting"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			really_refresh_callback),
-		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
+		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", &cd,
 			N_("<mode>,<object>,<path>"),
 			N_("add the specified entry to the index"),
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
@@ -1032,30 +1039,30 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
 			N_("remove named paths even if present in worktree"), 1),
-		OPT_BOOL('z', NULL, &nul_term_line,
+		OPT_BOOL('z', NULL, &cd.nul_term_line,
 			 N_("with --stdin: input lines are terminated by null bytes")),
-		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,
+		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &cd, NULL,
 			N_("read list of paths to be updated from standard input"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
 			NULL, 0, stdin_callback},
-		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &nul_term_line, NULL,
+		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &cd, NULL,
 			N_("add entries from standard input to the index"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
 			NULL, 0, stdin_cacheinfo_callback},
-		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &has_errors, NULL,
+		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &cd, NULL,
 			N_("repopulate stages #2 and #3 for the listed paths"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
 			NULL, 0, unresolve_callback},
-		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &has_errors, NULL,
+		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &cd, NULL,
 			N_("only update entries that differ from HEAD"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
 			NULL, 0, reupdate_callback},
-		OPT_BIT(0, "ignore-missing", &refresh_args.flags,
+		OPT_BIT(0, "ignore-missing", &cd.flags,
 			N_("ignore files missing from worktree"),
 			REFRESH_IGNORE_MISSING),
 		OPT_SET_INT(0, "verbose", &verbose,
 			N_("report actions to standard output"), 1),
-		OPT_CALLBACK_F(0, "clear-resolve-undo", NULL, NULL,
+		OPT_CALLBACK_F(0, "clear-resolve-undo", &cd, NULL,
 			N_("(for porcelains) forget saved unresolved conflicts"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			resolve_undo_clear_callback),
@@ -1087,8 +1094,6 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	repo = the_repository;
-
 	/* we will diagnose later if it turns out that we need to update it */
 	newfd = repo_hold_locked_index(repo, &lock_file, 0);
 	if (newfd < 0)
@@ -1098,8 +1103,13 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (entries < 0)
 		die("cache corrupted");
 
-	istate = repo->index;
+	cd.repo = repo;
+	cd.istate = istate = repo->index;
 	istate->updated_skipworktree = 1;
+	cd.flags = 0;
+	cd.has_errors = 0;
+	cd.nul_term_line = 0;
+	cd.read_from_stdin = 0;
 
 	/*
 	 * Custom copy of parse_options() because we want to handle
@@ -1145,7 +1155,7 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 	argc = parse_options_end(&ctx);
 
-	getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
+	getline_fn = cd.nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
 	if (preferred_index_format) {
 		if (preferred_index_format < INDEX_FORMAT_LB ||
 		    INDEX_FORMAT_UB < preferred_index_format)
@@ -1158,14 +1168,14 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 		istate->version = preferred_index_format;
 	}
 
-	if (read_from_stdin) {
+	if (cd.read_from_stdin) {
 		struct strbuf buf = STRBUF_INIT;
 		struct strbuf unquoted = STRBUF_INIT;
 
 		setup_work_tree();
 		while (getline_fn(&buf, stdin) != EOF) {
 			char *p;
-			if (!nul_term_line && buf.buf[0] == '"') {
+			if (!cd.nul_term_line && buf.buf[0] == '"') {
 				strbuf_reset(&unquoted);
 				if (unquote_c_style(&unquoted, buf.buf, NULL))
 					die("line is badly quoted");
@@ -1244,7 +1254,7 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	if (istate->cache_changed || force_write) {
 		if (newfd < 0) {
-			if (refresh_args.flags & REFRESH_QUIET)
+			if (cd.flags & REFRESH_QUIET)
 				exit(128);
 			unable_to_lock_die(get_index_file(), lock_error);
 		}
@@ -1254,5 +1264,5 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	rollback_lock_file(&lock_file);
 
-	return has_errors ? 1 : 0;
+	return cd.has_errors ? 1 : 0;
 }