Re: git rebase -i failing due to phantom index.lock

[Giuseppe Bilotta]

Hello,

On Thu, Sep 17, 2015 at 2:57 PM, Duy Nguyen  wrote:
> On Thu, Sep 17, 2015 at 3:08 AM, Giuseppe Bilotta
>  wrote:
>
>> A somewhat problematic git bisect has allowed me to identify commit
>> 03b86647722f11ccc321cd7279aa49b811d17cc2 as the first bad commit.
>
> That commit is mostly refactoring, but there's one extra lock added in
> these hunks. Maybe you can revert it and see if it improves anything.
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 87439fa..5e13444 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3644,7 +3644,7 @@ static void build_fake_ancestor(struct patch
> *list, const char *filename)
>  {
> struct patch *patch;
> struct index_state result = { NULL };
> -   int fd;
> +   static struct lock_file lock;
>
> /* Once we start supporting the reverse patch, it may be
>  * worth showing the new sha1 prefix, but until then...
> @@ -3682,8 +3682,8 @@ static void build_fake_ancestor(struct patch
> *list, const char *filename)
> die ("Could not add %s to temporary index", name);
> }
>
> -   fd = open(filename, O_WRONLY | O_CREAT, 0666);
> -   if (fd < 0 || write_index(, fd) || close(fd))
> +   hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
> +   if (write_locked_index(, , COMMIT_LOCK))
> die ("Could not write temporary index to %s", filename);
>
> discard_index();

This is not trivial to revert because write_index isn't available
anymore. I'm not sure what I should replace it with.

>> The problem has all signs of a timing issue, which I'm having problems
>> isolating, although simply providing a timeout on the index lock calls
>> seems to fix it.
>
> lockfile has received some updates lately. This commit caught my eyes,
> 044b6a9 (lockfile: allow file locking to be retried with a timeout -
> 2015-05-11), but this is just a shot in the dark..

The point is that, if I add a timeout to the checkout lock, it works.
It works as if something locks it earlier, and the unlock doesn't
register quickly enough, so waiting a few tens of milliseconds makes
it work.

> So it fails at "git checkout". That'll give me something to look in
> git-rebase*.sh. Thanks.


For what it's worth, the problem seems easier to replicate with a very
long list of commits than with a short one.

Best regards,


-- 
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git rebase -i failing due to phantom index.lock

[Duy Nguyen]

On Thu, Sep 17, 2015 at 3:08 AM, Giuseppe Bilotta
 wrote:
> Hello all,
>
> I've recently started to note an issue with git rebase -i failing with
> alarming frequency, especially on one of my repositories, claiming
> that index.lock could not be created because it exists, even though it
> doesn't and nothing else seems to be locking the index. The rebase
> bombs more usually during the initial checkout than during any other
> of the steps.
>
> The problem is somewhat randomly reproducible, as it doesn't happen
> 100% of the time. It also seems to happen more consistently with more
> recent git versions, or at least with my custom built git than with
> the distro-shipped one.
>
> A somewhat problematic git bisect has allowed me to identify commit
> 03b86647722f11ccc321cd7279aa49b811d17cc2 as the first bad commit.

That commit is mostly refactoring, but there's one extra lock added in
these hunks. Maybe you can revert it and see if it improves anything.

diff --git a/builtin/apply.c b/builtin/apply.c
index 87439fa..5e13444 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3644,7 +3644,7 @@ static void build_fake_ancestor(struct patch
*list, const char *filename)
 {
struct patch *patch;
struct index_state result = { NULL };
-   int fd;
+   static struct lock_file lock;

/* Once we start supporting the reverse patch, it may be
 * worth showing the new sha1 prefix, but until then...
@@ -3682,8 +3682,8 @@ static void build_fake_ancestor(struct patch
*list, const char *filename)
die ("Could not add %s to temporary index", name);
}

-   fd = open(filename, O_WRONLY | O_CREAT, 0666);
-   if (fd < 0 || write_index(, fd) || close(fd))
+   hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
+   if (write_locked_index(, , COMMIT_LOCK))
die ("Could not write temporary index to %s", filename);

discard_index();

> The problem has all signs of a timing issue, which I'm having problems
> isolating, although simply providing a timeout on the index lock calls
> seems to fix it.

lockfile has received some updates lately. This commit caught my eyes,
044b6a9 (lockfile: allow file locking to be retried with a timeout -
2015-05-11), but this is just a shot in the dark..

> Making git stall instead of die on error allowed me
> to obtain this backtrace which I suspect will not be particularly
> useful:
>
> #0 0x004c4666 in unable_to_lock_message
> (path=path@entry=0x1bad940 ".git/index", err=,
> buf=buf@entry=0x7ffd3b990900) at lockfile.c:158
> #1 0x004c46c6 in unable_to_lock_die (path=path@entry=0x1bad940
> ".git/index", err=) at lockfile.c:167
> #2 0x004c480b in hold_lock_file_for_update_timeout
> (lk=lk@entry=0x1bd7be0, path=0x1bad940 ".git/index", flags= out>, timeout_ms=timeout_ms@entry=0) at lockfile.c:177
> #3 0x004e6e2a in hold_lock_file_for_update (flags=1,
> path=, lk=0x1bd7be0) at lockfile.h:165
> #4 hold_locked_index (lk=lk@entry=0x1bd7be0,
> die_on_error=die_on_error@entry=1) at read-cache.c:1411
> #5 0x00420cb0 in merge_working_tree (old=0x7ffd3b990a20,
> old=0x7ffd3b990a20, new=0x7ffd3b990a00, new=0x7ffd3b990a00,
> writeout_error=0x7ffd3b9909c0, opts=0x7ffd3b992a31)
> at builtin/checkout.c:471

So it fails at "git checkout". That'll give me something to look in
git-rebase*.sh. Thanks.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html