[v3,1/3] factor out refresh_and_write_cache function
diff mbox series

Message ID 20190903191041.10470-2-t.gummerer@gmail.com
State New
Headers show
Series
  • make sure stash refreshes the index properly
Related show

Commit Message

Thomas Gummerer Sept. 3, 2019, 7:10 p.m. UTC
Getting the lock for the index, refreshing it and then writing it is a
pattern that happens more than once throughout the codebase, and isn't
trivial to get right.  Factor out the refresh_and_write_cache function
from builtin/am.c to read-cache.c, so it can be re-used in other
places in a subsequent commit.

Note that we return different error codes for failing to refresh the
cache, and failing to write the index.  The current caller only cares
about failing to write the index.  However for other callers we're
going to convert in subsequent patches we will need this distinction.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/am.c | 16 ++--------------
 cache.h      | 16 ++++++++++++++++
 read-cache.c | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 14 deletions(-)

Comments

Junio C Hamano Sept. 5, 2019, 10 p.m. UTC | #1
Thomas Gummerer <t.gummerer@gmail.com> writes:

> Getting the lock for the index, refreshing it and then writing it is a
> pattern that happens more than once throughout the codebase, and isn't
> trivial to get right.  Factor out the refresh_and_write_cache function
> from builtin/am.c to read-cache.c, so it can be re-used in other
> places in a subsequent commit.
>
> Note that we return different error codes for failing to refresh the
> cache, and failing to write the index.  The current caller only cares
> about failing to write the index.  However for other callers we're
> going to convert in subsequent patches we will need this distinction.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/am.c | 16 ++--------------
>  cache.h      | 16 ++++++++++++++++
>  read-cache.c | 19 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 14 deletions(-)

I think this goes in the right direction, but obviously conflicts
with what Dscho wants to do in the builtin-add-i series, and needs
to be reconciled by working better together.

For now, I'll eject builtin-add-i and queue this for a few days to
give it a bit more exposure, but after that requeue builtin-add-i
and discard these three patches.  By that time, hopefully you two
would have a rerolled version of this one and builtin-add-i that
agree what kind of refresh-and-write-index behaviour they both want.

The differences I see that need reconciling are:

 - builtin-add-i seems to allow 'gentle' and allow returning an
   error when we cannot open the index for writing by passing false
   to 'gentle'; this feature is not used yet, though.

 - This version allows to pass pathspec, seen and header_msg, while
   the one in builtin-add-i cannot limit the part of the index
   getting refreshed with pathspec.  It wouldn't be a brain surgery
   to use this version and adjust the caller (there only is one) in
   the builtin-add-i topic.

 - This version does not write the index back when refresh_index()
   returns non-zero, but the one in builtin-add-i ignores the
   returned value.  I think, as a performance measure, it probably
   is a better idea to write it back, even when the function returns
   non-zero (the local variable's name is has_errors, but having an
   entry in the index that does not get refreshed is *not* an error;
   e.g. an unmerged entry is a normal thing in the index, and as
   long as we refreshed other entries while having an unmerged and
   unrefreshable entry, we are making progress that is worth writing
   out).

Thanks.

> +int repo_refresh_and_write_index(struct  repository *repo,
> +				 unsigned int refresh_flags,
> +				 unsigned int write_flags,
> +				 const struct pathspec *pathspec,
> +				 char *seen, const char *header_msg)
> +{
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> +		rollback_lock_file(&lock_file);
> +		return 1;
> +	}
> +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> +		return -1;
> +	return 0;
> +}
> +
> +
>  int refresh_index(struct index_state *istate, unsigned int flags,
>  		  const struct pathspec *pathspec,
>  		  char *seen, const char *header_msg)
Thomas Gummerer Sept. 6, 2019, 2:18 p.m. UTC | #2
On 09/05, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Getting the lock for the index, refreshing it and then writing it is a
> > pattern that happens more than once throughout the codebase, and isn't
> > trivial to get right.  Factor out the refresh_and_write_cache function
> > from builtin/am.c to read-cache.c, so it can be re-used in other
> > places in a subsequent commit.
> >
> > Note that we return different error codes for failing to refresh the
> > cache, and failing to write the index.  The current caller only cares
> > about failing to write the index.  However for other callers we're
> > going to convert in subsequent patches we will need this distinction.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  builtin/am.c | 16 ++--------------
> >  cache.h      | 16 ++++++++++++++++
> >  read-cache.c | 19 +++++++++++++++++++
> >  3 files changed, 37 insertions(+), 14 deletions(-)
> 
> I think this goes in the right direction, but obviously conflicts
> with what Dscho wants to do in the builtin-add-i series, and needs
> to be reconciled by working better together.

Oops, I didn't realize there was another series in flight that also
introduces 'repo_refresh_and_write_index'.  Probably should have done
a test merge of this with pu.

> For now, I'll eject builtin-add-i and queue this for a few days to
> give it a bit more exposure, but after that requeue builtin-add-i
> and discard these three patches.  By that time, hopefully you two
> would have a rerolled version of this one and builtin-add-i that
> agree what kind of refresh-and-write-index behaviour they both want.
>
> The differences I see that need reconciling are:

Thanks for writing these down.

>  - builtin-add-i seems to allow 'gentle' and allow returning an
>    error when we cannot open the index for writing by passing false
>    to 'gentle'; this feature is not used yet, though.

Right, and if gentle is set to false, it avoids writing the index,
which seems fine from my perspective.

>  - This version allows to pass pathspec, seen and header_msg, while
>    the one in builtin-add-i cannot limit the part of the index
>    getting refreshed with pathspec.  It wouldn't be a brain surgery
>    to use this version and adjust the caller (there only is one) in
>    the builtin-add-i topic.

'pathspec', 'seen' and 'header_msg' are not used in my version either,
I just implemented it for completeness and compatibility.  So I'd be
fine to do without them.

>  - This version does not write the index back when refresh_index()
>    returns non-zero, but the one in builtin-add-i ignores the
>    returned value.  I think, as a performance measure, it probably
>    is a better idea to write it back, even when the function returns
>    non-zero (the local variable's name is has_errors, but having an
>    entry in the index that does not get refreshed is *not* an error;
>    e.g. an unmerged entry is a normal thing in the index, and as
>    long as we refreshed other entries while having an unmerged and
>    unrefreshable entry, we are making progress that is worth writing
>    out).

I'm happy with writing the index back even if there are errors.
However I think we still need the option to get the return code from
'refresh_index()', as some callers where I'm using
'refresh_and_write_index()' in this series behave differently
depending on its return code.

There's two more differences between the versions:

 - The version in my series allows passing in write_flags to be passed
   to write_locked_index, which is required to convert the callers in
   builtin/merge.c.

 - Dscho's version also calls 'repo_read_index_preload()', which I
   don't do in mine.  Some callers don't need to do that, so I think it
   would be nice to keep that outside of the
   'repo_refresh_and_write_index()' function.

I can think of a few ways forward here:

 - I incorporate features that are needed for the builtin-add-i series
   here, and that is rebased on top of this series.

 - We drop the first two patches of this series, so we only fix the
   problems in 'git stash' for now.  Later we can have a refactoring
   series that uses repo_refresh_and_write_index in the places we
   converted here, once the dust of the builtin-add-i series settled.

 - I rebase this on top of builtin-add-i.

I'm happy with either of the first two, but less so with the last
option.  I was hoping this series could potentially go to maint as it
was a bugfix, which we obviously can't do with that option.

Dscho, what do you think? :)

> Thanks.
> 
> > +int repo_refresh_and_write_index(struct  repository *repo,
> > +				 unsigned int refresh_flags,
> > +				 unsigned int write_flags,
> > +				 const struct pathspec *pathspec,
> > +				 char *seen, const char *header_msg)
> > +{
> > +	struct lock_file lock_file = LOCK_INIT;
> > +
> > +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> > +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> > +		rollback_lock_file(&lock_file);
> > +		return 1;
> > +	}
> > +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> > +		return -1;
> > +	return 0;
> > +}
> > +
> > +
> >  int refresh_index(struct index_state *istate, unsigned int flags,
> >  		  const struct pathspec *pathspec,
> >  		  char *seen, const char *header_msg)
Johannes Schindelin Sept. 11, 2019, 10:57 a.m. UTC | #3
Hi Thomas,

On Fri, 6 Sep 2019, Thomas Gummerer wrote:

> On 09/05, Junio C Hamano wrote:
> > Thomas Gummerer <t.gummerer@gmail.com> writes:
> >
> > > Getting the lock for the index, refreshing it and then writing it is a
> > > pattern that happens more than once throughout the codebase, and isn't
> > > trivial to get right.  Factor out the refresh_and_write_cache function
> > > from builtin/am.c to read-cache.c, so it can be re-used in other
> > > places in a subsequent commit.
> > >
> > > Note that we return different error codes for failing to refresh the
> > > cache, and failing to write the index.  The current caller only cares
> > > about failing to write the index.  However for other callers we're
> > > going to convert in subsequent patches we will need this distinction.
> > >
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > >  builtin/am.c | 16 ++--------------
> > >  cache.h      | 16 ++++++++++++++++
> > >  read-cache.c | 19 +++++++++++++++++++
> > >  3 files changed, 37 insertions(+), 14 deletions(-)
> >
> > I think this goes in the right direction, but obviously conflicts
> > with what Dscho wants to do in the builtin-add-i series, and needs
> > to be reconciled by working better together.
>
> Oops, I didn't realize there was another series in flight that also
> introduces 'repo_refresh_and_write_index'.  Probably should have done
> a test merge of this with pu.

Yep, our patches clash. I would not mind placing my patch series on top
of yours, provided that you can make a few changes that I need ;-)

> > For now, I'll eject builtin-add-i and queue this for a few days to
> > give it a bit more exposure, but after that requeue builtin-add-i
> > and discard these three patches.  By that time, hopefully you two
> > would have a rerolled version of this one and builtin-add-i that
> > agree what kind of refresh-and-write-index behaviour they both want.
> >
> > The differences I see that need reconciling are:
>
> Thanks for writing these down.
>
> >  - builtin-add-i seems to allow 'gentle' and allow returning an
> >    error when we cannot open the index for writing by passing false
> >    to 'gentle'; this feature is not used yet, though.
>
> Right, and if gentle is set to false, it avoids writing the index,
> which seems fine from my perspective.

This also suggests that it would make sense to avoid
`LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
library function, not just a helper function for a one-shot built-in
(don't you like how this idea "it is okay to use exit() to clean up
after us, we don't care" comes back to bite us?).

> >  - This version allows to pass pathspec, seen and header_msg, while
> >    the one in builtin-add-i cannot limit the part of the index
> >    getting refreshed with pathspec.  It wouldn't be a brain surgery
> >    to use this version and adjust the caller (there only is one) in
> >    the builtin-add-i topic.
>
> 'pathspec', 'seen' and 'header_msg' are not used in my version either,
> I just implemented it for completeness and compatibility.  So I'd be
> fine to do without them.

Oh, why not keep them? I'd rather keep them and adjust the caller in
`builtin-add-i`.

> >  - This version does not write the index back when refresh_index()
> >    returns non-zero, but the one in builtin-add-i ignores the
> >    returned value.  I think, as a performance measure, it probably
> >    is a better idea to write it back, even when the function returns
> >    non-zero (the local variable's name is has_errors, but having an
> >    entry in the index that does not get refreshed is *not* an error;
> >    e.g. an unmerged entry is a normal thing in the index, and as
> >    long as we refreshed other entries while having an unmerged and
> >    unrefreshable entry, we are making progress that is worth writing
> >    out).
>
> I'm happy with writing the index back even if there are errors.
> However I think we still need the option to get the return code from
> 'refresh_index()', as some callers where I'm using
> 'refresh_and_write_index()' in this series behave differently
> depending on its return code.
>
> There's two more differences between the versions:
>
>  - The version in my series allows passing in write_flags to be passed
>    to write_locked_index, which is required to convert the callers in
>    builtin/merge.c.

I can always pass in 0 as `write_flags`.

>  - Dscho's version also calls 'repo_read_index_preload()', which I
>    don't do in mine.  Some callers don't need to do that, so I think it
>    would be nice to keep that outside of the
>    'repo_refresh_and_write_index()' function.

Agreed.

> I can think of a few ways forward here:
>
>  - I incorporate features that are needed for the builtin-add-i series
>    here, and that is rebased on top of this series.

I'd prefer this way forward. The `builtin-add-i` patch series is
evolving more slowly than yours.

>  - We drop the first two patches of this series, so we only fix the
>    problems in 'git stash' for now.  Later we can have a refactoring
>    series that uses repo_refresh_and_write_index in the places we
>    converted here, once the dust of the builtin-add-i series settled.
>
>  - I rebase this on top of builtin-add-i.
>
> I'm happy with either of the first two, but less so with the last
> option.  I was hoping this series could potentially go to maint as it
> was a bugfix, which we obviously can't do with that option.
>
> Dscho, what do you think? :)

See above ;-)

Thank you!
Dscho

>
> > Thanks.
> >
> > > +int repo_refresh_and_write_index(struct  repository *repo,
> > > +				 unsigned int refresh_flags,
> > > +				 unsigned int write_flags,
> > > +				 const struct pathspec *pathspec,
> > > +				 char *seen, const char *header_msg)
> > > +{
> > > +	struct lock_file lock_file = LOCK_INIT;
> > > +
> > > +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> > > +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> > > +		rollback_lock_file(&lock_file);
> > > +		return 1;
> > > +	}
> > > +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> > > +		return -1;
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  int refresh_index(struct index_state *istate, unsigned int flags,
> > >  		  const struct pathspec *pathspec,
> > >  		  char *seen, const char *header_msg)
>
Thomas Gummerer Sept. 11, 2019, 5:52 p.m. UTC | #4
On 09/11, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Fri, 6 Sep 2019, Thomas Gummerer wrote:
> > Oops, I didn't realize there was another series in flight that also
> > introduces 'repo_refresh_and_write_index'.  Probably should have done
> > a test merge of this with pu.
> 
> Yep, our patches clash. I would not mind placing my patch series on top
> of yours, provided that you can make a few changes that I need ;-)

Sounds good.  Looking ahead further I don't mind these changes at all!

> > Right, and if gentle is set to false, it avoids writing the index,
> > which seems fine from my perspective.
> 
> This also suggests that it would make sense to avoid
> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
> library function, not just a helper function for a one-shot built-in
> (don't you like how this idea "it is okay to use exit() to clean up
> after us, we don't care" comes back to bite us?).

Yup, returning an error for this definitely makes sense, especially
for future proofing.

> > >  - This version allows to pass pathspec, seen and header_msg, while
> > >    the one in builtin-add-i cannot limit the part of the index
> > >    getting refreshed with pathspec.  It wouldn't be a brain surgery
> > >    to use this version and adjust the caller (there only is one) in
> > >    the builtin-add-i topic.
> >
> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,
> > I just implemented it for completeness and compatibility.  So I'd be
> > fine to do without them.
> 
> Oh, why not keep them? I'd rather keep them and adjust the caller in
> `builtin-add-i`.

Great, I'm happy to keep them.

> > There's two more differences between the versions:
> >
> >  - The version in my series allows passing in write_flags to be passed
> >    to write_locked_index, which is required to convert the callers in
> >    builtin/merge.c.
> 
> I can always pass in 0 as `write_flags`.
> 
> >  - Dscho's version also calls 'repo_read_index_preload()', which I
> >    don't do in mine.  Some callers don't need to do that, so I think it
> >    would be nice to keep that outside of the
> >    'repo_refresh_and_write_index()' function.
> 
> Agreed.
> 
> > I can think of a few ways forward here:
> >
> >  - I incorporate features that are needed for the builtin-add-i series
> >    here, and that is rebased on top of this series.
> 
> I'd prefer this way forward. The `builtin-add-i` patch series is
> evolving more slowly than yours.

Great!  I'll send an updated version of my series soon.  Thanks!
Junio C Hamano Sept. 12, 2019, 4:46 p.m. UTC | #5
Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 09/11, Johannes Schindelin wrote:
>> Hi Thomas,
>> 
>> On Fri, 6 Sep 2019, Thomas Gummerer wrote:
>> > Oops, I didn't realize there was another series in flight that also
>> > introduces 'repo_refresh_and_write_index'.  Probably should have done
>> > a test merge of this with pu.
>> 
>> Yep, our patches clash. I would not mind placing my patch series on top
>> of yours, provided that you can make a few changes that I need ;-)
>
> Sounds good.  Looking ahead further I don't mind these changes at all!
>
>> > Right, and if gentle is set to false, it avoids writing the index,
>> > which seems fine from my perspective.
>> 
>> This also suggests that it would make sense to avoid
>> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
>> library function, not just a helper function for a one-shot built-in
>> (don't you like how this idea "it is okay to use exit() to clean up
>> after us, we don't care" comes back to bite us?).
>
> Yup, returning an error for this definitely makes sense, especially
> for future proofing.
>
>> > >  - This version allows to pass pathspec, seen and header_msg, while
>> > >    the one in builtin-add-i cannot limit the part of the index
>> > >    getting refreshed with pathspec.  It wouldn't be a brain surgery
>> > >    to use this version and adjust the caller (there only is one) in
>> > >    the builtin-add-i topic.
>> >
>> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,
>> > I just implemented it for completeness and compatibility.  So I'd be
>> > fine to do without them.
>> 
>> Oh, why not keep them? I'd rather keep them and adjust the caller in
>> `builtin-add-i`.
>
> Great, I'm happy to keep them.
>
>> > There's two more differences between the versions:
>> >
>> >  - The version in my series allows passing in write_flags to be passed
>> >    to write_locked_index, which is required to convert the callers in
>> >    builtin/merge.c.
>> 
>> I can always pass in 0 as `write_flags`.
>> 
>> >  - Dscho's version also calls 'repo_read_index_preload()', which I
>> >    don't do in mine.  Some callers don't need to do that, so I think it
>> >    would be nice to keep that outside of the
>> >    'repo_refresh_and_write_index()' function.
>> 
>> Agreed.
>> 
>> > I can think of a few ways forward here:
>> >
>> >  - I incorporate features that are needed for the builtin-add-i series
>> >    here, and that is rebased on top of this series.
>> 
>> I'd prefer this way forward. The `builtin-add-i` patch series is
>> evolving more slowly than yours.
>
> Great!  I'll send an updated version of my series soon.  Thanks!

I just read the conclusion you two reached (after being down and
offline for two days) and found the reasoning totally sensible.

Thanks, both of you, for working well together.

Patch
diff mbox series

diff --git a/builtin/am.c b/builtin/am.c
index 1aea657a7f..ddedd2b9d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1071,19 +1071,6 @@  static const char *msgnum(const struct am_state *state)
 	return sb.buf;
 }
 
-/**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file lock_file = LOCK_INIT;
-
-	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
@@ -1703,7 +1690,8 @@  static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)
+		die(_("unable to write index file"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/cache.h b/cache.h
index b1da1ab08f..2b14768bea 100644
--- a/cache.h
+++ b/cache.h
@@ -414,6 +414,7 @@  extern struct index_state the_index;
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
 #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))
 #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
+#define refresh_and_write_cache(refresh_flags, write_flags) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), NULL, NULL, NULL)
 #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
 #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
 #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))
@@ -812,6 +813,21 @@  void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 #define REFRESH_PROGRESS	0x0040  /* show progress bar if stderr is tty */
 int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
+/*
+ * Refresh the index and write it to disk.
+ *
+ * 'refresh_flags' is passed directly to 'refresh_index()', while
+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
+ * the lockfile is always either committed or rolled back.
+ *
+ * Return 1 if refreshing the index returns an error, -1 if writing
+ * the index to disk fails, 0 on success.
+ *
+ * Note that if refreshing the index returns an error, we don't write
+ * the result to disk.
+ */
+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
+
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
 void set_alternate_index_output(const char *);
diff --git a/read-cache.c b/read-cache.c
index 52ffa8a313..2ad96677ae 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1472,6 +1472,25 @@  static void show_file(const char * fmt, const char * name, int in_porcelain,
 	printf(fmt, name);
 }
 
+int repo_refresh_and_write_index(struct  repository *repo,
+				 unsigned int refresh_flags,
+				 unsigned int write_flags,
+				 const struct pathspec *pathspec,
+				 char *seen, const char *header_msg)
+{
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
+		rollback_lock_file(&lock_file);
+		return 1;
+	}
+	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
+		return -1;
+	return 0;
+}
+
+
 int refresh_index(struct index_state *istate, unsigned int flags,
 		  const struct pathspec *pathspec,
 		  char *seen, const char *header_msg)