* [PATCH] sequencer: reschedule pick if index can't be locked
@ 2017-11-15 10:41 Phillip Wood
2017-11-15 18:44 ` Martin Ågren
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Phillip Wood @ 2017-11-15 10:41 UTC (permalink / raw)
To: Git Mailing List; +Cc: Johannes Schindelin, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Return an error instead of dying if the index cannot be locked in
do_recursive_merge() as if the commit cannot be picked it needs to be
rescheduled when performing an interactive rebase. If the pick is not
rescheduled and the user runs 'git rebase --continue' rather than 'git
rebase --abort' then the commit gets silently dropped.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
char **xopt;
static struct lock_file index_lock;
- hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
+ if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR))
+ return -1;
read_cache();
--
2.15.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] sequencer: reschedule pick if index can't be locked
2017-11-15 10:41 [PATCH] sequencer: reschedule pick if index can't be locked Phillip Wood
@ 2017-11-15 18:44 ` Martin Ågren
2017-11-16 10:43 ` Phillip Wood
2017-11-15 22:03 ` Johannes Schindelin
2017-11-16 5:22 ` [PATCH v2] " Junio C Hamano
2 siblings, 1 reply; 7+ messages in thread
From: Martin Ågren @ 2017-11-15 18:44 UTC (permalink / raw)
To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin
On 15 November 2017 at 11:41, Phillip Wood <phillip.wood@talktalk.net> wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Return an error instead of dying if the index cannot be locked in
> do_recursive_merge() as if the commit cannot be picked it needs to be
> rescheduled when performing an interactive rebase. If the pick is not
> rescheduled and the user runs 'git rebase --continue' rather than 'git
> rebase --abort' then the commit gets silently dropped.
Makes sense. (Your analysis, not the current behavior. ;-) )
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> sequencer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> char **xopt;
> static struct lock_file index_lock;
>
> - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
> + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR))
> + return -1;
>
> read_cache();
From the commit message, I would have expected the flags to be zero. This patch
does not only turn off the die-ing, it also tells the lockfile-API to print an
error message before returning. I don't have an opinion on whether that extra
verboseness is good or bad, but if it's wanted, I think the commit message
should mention this change.
Also, don't you want to check "< 0" rather than "!= 0"? If all goes
well, the return value will be a file descriptor. I think that it will
always be non-zero, so I think you'll always return -1 here.
Martin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sequencer: reschedule pick if index can't be locked
2017-11-15 10:41 [PATCH] sequencer: reschedule pick if index can't be locked Phillip Wood
2017-11-15 18:44 ` Martin Ågren
@ 2017-11-15 22:03 ` Johannes Schindelin
2017-11-16 5:22 ` [PATCH v2] " Junio C Hamano
2 siblings, 0 replies; 7+ messages in thread
From: Johannes Schindelin @ 2017-11-15 22:03 UTC (permalink / raw)
To: Phillip Wood; +Cc: Git Mailing List
Hi Phillip,
On Wed, 15 Nov 2017, Phillip Wood wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> char **xopt;
> static struct lock_file index_lock;
>
> - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
> + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR))
If you test the return value for *negative* values, I am fully on board
with the change.
As far as I understand the code, hold_locked_index() returns -1 on error,
but *a file descriptor* (which is usually not 0) upon success...
Ciao,
Dscho
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] sequencer: reschedule pick if index can't be locked
2017-11-15 10:41 [PATCH] sequencer: reschedule pick if index can't be locked Phillip Wood
2017-11-15 18:44 ` Martin Ågren
2017-11-15 22:03 ` Johannes Schindelin
@ 2017-11-16 5:22 ` Junio C Hamano
2017-11-16 14:19 ` Phillip Wood
2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2017-11-16 5:22 UTC (permalink / raw)
To: Phillip Wood
Cc: Git Mailing List, Johannes Schindelin, Phillip Wood,
Martin Ågren
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Date: Wed, 15 Nov 2017 10:41:25 +0000
If the index cannot be locked in do_recursive_merge(), issue an
error message and go on to the error recovery codepath, instead of
dying. When the commit cannot be picked, it needs to be rescheduled
when performing an interactive rebase, but just dying there won't
allow that to happen, and when the user runs 'git rebase --continue'
rather than 'git rebase --abort', the commit gets silently dropped.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
* I've queue this, taking input from responses by Dscho and Martin.
sequencer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 332a383b03..10924ffd49 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
char **xopt;
static struct lock_file index_lock;
- hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
+ if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
+ return -1;
read_cache();
--
2.15.0-360-gf2497ca0e5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] sequencer: reschedule pick if index can't be locked
2017-11-15 18:44 ` Martin Ågren
@ 2017-11-16 10:43 ` Phillip Wood
2017-11-16 10:55 ` Martin Ågren
0 siblings, 1 reply; 7+ messages in thread
From: Phillip Wood @ 2017-11-16 10:43 UTC (permalink / raw)
To: Martin Ågren, Phillip Wood
Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano
On 15/11/17 18:44, Martin Ågren wrote:
> On 15 November 2017 at 11:41, Phillip Wood <phillip.wood@talktalk.net> wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Return an error instead of dying if the index cannot be locked in
>> do_recursive_merge() as if the commit cannot be picked it needs to be
>> rescheduled when performing an interactive rebase. If the pick is not
>> rescheduled and the user runs 'git rebase --continue' rather than 'git
>> rebase --abort' then the commit gets silently dropped.
>
> Makes sense. (Your analysis, not the current behavior. ;-) )
>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> sequencer.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>> char **xopt;
>> static struct lock_file index_lock;
>>
>> - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
>> + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR))
>> + return -1;
>>
>> read_cache();
>
> From the commit message, I would have expected the flags to be zero. This patch
> does not only turn off the die-ing, it also tells the lockfile-API to print an
> error message before returning. I don't have an opinion on whether that extra
> verboseness is good or bad, but if it's wanted, I think the commit message
> should mention this change.
Hi Martin, thanks for your comments. LOCK_DIE_ON_ERROR also prints the
same warning so that behavior is unchanged by this patch, though
mentioning it in the commit message would be no bad thing.
>
> Also, don't you want to check "< 0" rather than "!= 0"? If all goes
> well, the return value will be a file descriptor. I think that it will
> always be non-zero, so I think you'll always return -1 here.
Yes you're right, thanks. I thought I had tested this but I now realise
my so called test just fast-forwarded so didn't touch this code path
Best Wishes
Phillip
> Martin
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sequencer: reschedule pick if index can't be locked
2017-11-16 10:43 ` Phillip Wood
@ 2017-11-16 10:55 ` Martin Ågren
0 siblings, 0 replies; 7+ messages in thread
From: Martin Ågren @ 2017-11-16 10:55 UTC (permalink / raw)
To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano
On 16 November 2017 at 11:43, Phillip Wood <phillip.wood@talktalk.net> wrote:
> On 15/11/17 18:44, Martin Ågren wrote:
>>
>> On 15 November 2017 at 11:41, Phillip Wood <phillip.wood@talktalk.net>
>> wrote:
>>
>> From the commit message, I would have expected the flags to be zero. This
>> patch
>> does not only turn off the die-ing, it also tells the lockfile-API to
>> print an
>> error message before returning. I don't have an opinion on whether that
>> extra
>> verboseness is good or bad, but if it's wanted, I think the commit message
>> should mention this change.
>
>
> Hi Martin, thanks for your comments. LOCK_DIE_ON_ERROR also prints the same
> warning so that behavior is unchanged by this patch, though mentioning it in
> the commit message would be no bad thing.
Argh, you're right of course. Sorry for this.
Martin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] sequencer: reschedule pick if index can't be locked
2017-11-16 5:22 ` [PATCH v2] " Junio C Hamano
@ 2017-11-16 14:19 ` Phillip Wood
0 siblings, 0 replies; 7+ messages in thread
From: Phillip Wood @ 2017-11-16 14:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Johannes Schindelin, Phillip Wood,
Martin Ågren
On 16/11/17 05:22, Junio C Hamano wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> Date: Wed, 15 Nov 2017 10:41:25 +0000
>
> If the index cannot be locked in do_recursive_merge(), issue an
> error message and go on to the error recovery codepath, instead of
> dying. When the commit cannot be picked, it needs to be rescheduled
> when performing an interactive rebase, but just dying there won't
> allow that to happen, and when the user runs 'git rebase --continue'
> rather than 'git rebase --abort', the commit gets silently dropped.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> * I've queue this, taking input from responses by Dscho and Martin.
That's great thanks, sorry for the bug in the original
Best Wishes
Phillip
> sequencer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 332a383b03..10924ffd49 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> char **xopt;
> static struct lock_file index_lock;
>
> - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
> + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
>
> read_cache();
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-16 14:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-15 10:41 [PATCH] sequencer: reschedule pick if index can't be locked Phillip Wood
2017-11-15 18:44 ` Martin Ågren
2017-11-16 10:43 ` Phillip Wood
2017-11-16 10:55 ` Martin Ågren
2017-11-15 22:03 ` Johannes Schindelin
2017-11-16 5:22 ` [PATCH v2] " Junio C Hamano
2017-11-16 14:19 ` Phillip Wood
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).