Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Thu, Mar 01, 2018 at 09:40:20PM +0100, Martin Ågren wrote:

> After thinking about it, I tend to agree. That rewrite loses an
> indentation level and makes it a bit clearer that we have two steps,
> "maybe bail" and "write". But at the cost of duplicating logic -- after
> all, those two steps are very closely related, so there's no need to
> artificially separate them.
> 
> Here it is again, without that hunk, and without the commit message
> claim that it'd be a good thing to have just a few uses of
> "active_cache_changed" remaining.

Thanks, this version looks good to me. The name SKIP_IF_UNCHANGED is
generic and may result in clashes down the road. But then so is the name
COMMIT_LOCK. I'm OK to punt on that until we do see such a collision, at
which point we may want to provide a consistent namespace for these
flags.

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

On 1 March 2018 at 20:24, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> IMHO the result is easier to follow. Except for one case:
>>
>>> [...]
>>
>> where I think the logic just ends up repeating itself.
>
> Yup, this one I also had a bit of trouble with.

Thanks for your feedback, both of you. It's much appreciated.

After thinking about it, I tend to agree. That rewrite loses an
indentation level and makes it a bit clearer that we have two steps,
"maybe bail" and "write". But at the cost of duplicating logic -- after
all, those two steps are very closely related, so there's no need to
artificially separate them.

Here it is again, without that hunk, and without the commit message
claim that it'd be a good thing to have just a few uses of
"active_cache_changed" remaining.

This could go as a patch 6/5 onto ma/roll-back-lockfiles. I keep
debating myself whether this would do better earlier in such a six-pack,
where it would be "also, this releases a lock where we used to forget
to". Right now, there is overlap between patch 3/5 (which adds a line)
and this patch (which removes it). This patch doesn't entirely negate
the need for patch 3/5 though..

Martin

-- >8 --
Subject: write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

if (active_cache_changed && write_locked_index(...))
handle_error();
rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. Leave the most complicated of the callers (in
builtin/update-index.c) unchanged. Rewriting it to use this new flag
would end up duplicating logic.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 cache.h   |  4 
 builtin/add.c |  7 +++
 builtin/commit.c  | 10 +++---
 builtin/merge.c   | 15 ++-
 builtin/mv.c  |  4 ++--
 builtin/rm.c  |  7 +++
 merge-recursive.c |  5 ++---
 read-cache.c  |  6 ++
 rerere.c  |  8 +++-
 sequencer.c   | 11 +--
 10 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..905d8eb6cc 100644
--- a/cache.h
+++ b/cache.h
@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
+#define SKIP_IF_UNCHANGED  (1 << 1)
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..9e5a80cc6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char 
*prefix)
unplug_bulk_checkin();
 
 finish:
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
-   die(_("Unable to write new index file"));
-   }
+   if (write_locked_index(_index, _file,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("Unable to write new index file"));
 
UNLEAK(pathspec);
UNLEAK(dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..8d9b4081c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (active_cache_changed
|| !cache_tree_fully_valid(active_cache_tree))
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _lock,
-  COMMIT_LOCK))
-   die(_("unable to write new_index file"));
-   } else {
-   rollback_lock_file(_lock);
- 

Re: [PATCH v2 0/5] roll back locks in various code paths

[Junio C Hamano]

Jeff King  writes:

> IMHO the result is easier to follow. Except for one case:
>
>> -if (active_cache_changed || force_write) {
>> -if (newfd < 0) {
>> -if (refresh_args.flags & REFRESH_QUIET)
>> -exit(128);
>> -unable_to_lock_die(get_index_file(), lock_error);
>> -}
>> -if (write_locked_index(_index, _file, COMMIT_LOCK))
>> -die("Unable to write new index file");
>> +if (newfd < 0 && (active_cache_changed || force_write)) {
>> +if (refresh_args.flags & REFRESH_QUIET)
>> +exit(128);
>> +unable_to_lock_die(get_index_file(), lock_error);
>>  }
>>  
>> -rollback_lock_file(_file);
>> +if (write_locked_index(_index, _file,
>> +   COMMIT_LOCK | (force_write ? 0 : 
>> SKIP_IF_UNCHANGED)))
>> +die("Unable to write new index file");
>
> where I think the logic just ends up repeating itself.

Yup, this one I also had a bit of trouble with.

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:

> > I'll follow up with a patch to
> > address the confusing pattern which Peff mentioned and which fooled me
> > when I prepared v1.
> 
> Here is such a patch on top of the others. I'm not particularly proud of the
> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.
> 
> I think this makes the current users a bit more obvious, and should help 
> future
> users get this optimization right.

IMHO the result is easier to follow. Except for one case:

> - if (active_cache_changed || force_write) {
> - if (newfd < 0) {
> - if (refresh_args.flags & REFRESH_QUIET)
> - exit(128);
> - unable_to_lock_die(get_index_file(), lock_error);
> - }
> - if (write_locked_index(_index, _file, COMMIT_LOCK))
> - die("Unable to write new index file");
> + if (newfd < 0 && (active_cache_changed || force_write)) {
> + if (refresh_args.flags & REFRESH_QUIET)
> + exit(128);
> + unable_to_lock_die(get_index_file(), lock_error);
>   }
>  
> - rollback_lock_file(_file);
> + if (write_locked_index(_index, _file,
> +COMMIT_LOCK | (force_write ? 0 : 
> SKIP_IF_UNCHANGED)))
> + die("Unable to write new index file");

where I think the logic just ends up repeating itself. I guess you were
anxious to try to get rid of active_cached_changed, but I don't think
keeping it around is really that big a deal (and certainly another trick
is to just say "the_index.cache_changed").

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:07:53PM +0100, Martin Ågren wrote:

> This is v2 of my series to always release locks. As before, there's a
> conflict with pu, where the correct resolution is to take my version of
> the conflicting hunk.
> 
> The only difference to v1 is in patch 3. I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

This looks good to me. And I'm glad my rambling helped find something
useful. ;)

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

On 1 March 2018 at 00:20, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> A further upshot of this patch is that `active_cache_changed`, which is
>> defined as `the_index.cache_changed`, now only has a few users left.
>
> I am undecided if this is a *good* thing.  In a few codepaths where
> we make a speculative update to the on-disk index file, I find that
> the explicit mention of active_cache_changed clarifies what is going
> on.  Perhaps once my (and other old timers') eyes get used to this
> new SKIP_IF_UNCHANGED symbol, it would start serving the same
> purpose ;-)

Right, you might say that this trades one symbol for another. What I
meant was, we only have a few "active_cache_changed" and soon (TM) they
might all be "the_index.cache_changed" or "index->cache_changed" and the
macro could be retired. My understanding of the history is limited, but
I was under the impression that this was like a transition macro (albeit
a very old one!).

Martin

Re: [PATCH v2 0/5] roll back locks in various code paths

[Junio C Hamano]

Martin Ågren  writes:

> A further upshot of this patch is that `active_cache_changed`, which is
> defined as `the_index.cache_changed`, now only has a few users left.

I am undecided if this is a *good* thing.  In a few codepaths where
we make a speculative update to the on-disk index file, I find that
the explicit mention of active_cache_changed clarifies what is going
on.  Perhaps once my (and other old timers') eyes get used to this
new SKIP_IF_UNCHANGED symbol, it would start serving the same
purpose ;-)

> We could have made the new flag behave the other way round
> ("FORCE_WRITE"), but that could break existing users behind their backs.

Yes, that would break topics in flight that add new calls to
write_locked_index(); they want to write out the index even when
active_cache_changed says there is no update.

Looking at a typical pre/post image of this change like this place:

>   hold_locked_index(, LOCK_DIE_ON_ERROR);
>   refresh_cache(REFRESH_QUIET);
> - if (active_cache_changed &&
> - write_locked_index(_index, , COMMIT_LOCK))
> + if (write_locked_index(_index, ,
> +COMMIT_LOCK | SKIP_IF_UNCHANGED))
>   return error(_("Unable to write index."));
> - rollback_lock_file();

this certainly simplifies what the caller needs to do.

> @@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
>   * With `COMMIT_LOCK`, the lock is always committed or rolled back.
>   * Without it, the lock is closed, but neither committed nor rolled
>   * back.
> + *
> + * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
> + * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
>   */
>  extern int write_locked_index(struct index_state *, struct lock_file *lock, 
> unsigned flags);

Good.

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

> I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

Here is such a patch on top of the others. I'm not particularly proud of the
name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.

I think this makes the current users a bit more obvious, and should help future
users get this optimization right.

Martin

-- >8 --
Subject: write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

if (active_cache_changed && write_locked_index(...))
handle_error();
rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. The most complicated of the callers is in
builtin/update-index.c, where we need to check with `force_write` to
decide whether we should pass the new flag or not.

A further upshot of this patch is that `active_cache_changed`, which is
defined as `the_index.cache_changed`, now only has a few users left.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 cache.h|  4 
 builtin/add.c  |  7 +++
 builtin/commit.c   | 10 +++---
 builtin/merge.c| 15 ++-
 builtin/mv.c   |  4 ++--
 builtin/rm.c   |  7 +++
 builtin/update-index.c | 16 +++-
 merge-recursive.c  |  5 ++---
 read-cache.c   |  6 ++
 rerere.c   |  8 +++-
 sequencer.c| 11 +--
 11 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..905d8eb6cc 100644
--- a/cache.h
+++ b/cache.h
@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
+#define SKIP_IF_UNCHANGED  (1 << 1)
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..9e5a80cc6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char 
*prefix)
unplug_bulk_checkin();
 
 finish:
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
-   die(_("Unable to write new index file"));
-   }
+   if (write_locked_index(_index, _file,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("Unable to write new index file"));
 
UNLEAK(pathspec);
UNLEAK(dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..8d9b4081c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (active_cache_changed
|| !cache_tree_fully_valid(active_cache_tree))
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _lock,
-  COMMIT_LOCK))
-   die(_("unable to write new_index file"));
-   } else {
-   rollback_lock_file(_lock);
-   }
+   if (write_locked_index(_index, _lock,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("unable to write new_index file"));
commit_style = COMMIT_AS_IS;
ret = get_index_file();
goto out;
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..30c65dfeff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -651,10 +651,9 @@ static int try_merge_strategy(const