sequencer: reschedule pick if index can't be locked
PATCH 1/1 sequencer: reschedule pick if index can't be locked
From: Phillip Wood <[email protected]>
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.
--- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
sequencer.c +2 -1
@@ -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 ();
Signature · 1 line
2.15.0
Quoted 7 lines · Phillip Wood
From: Phillip Wood <[email protected]>
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. ;-) )
Quoted 10 lines
Signed-off-by Phillip Wood <[email protected]>--- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)sequencer.c +0
-0
sequencer.c static int do_recursive_merge(struct commit *base, struct commit *next, · line 444
Quoted 8 lines
sequencer.c +2
-1@@ -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
Quoted 28 lines · Martin Ågren
Quoted 7 lines · Phillip Wood
From: Phillip Wood <[email protected]>
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. ;-) )
Quoted 17 lines
Signed-off-by Phillip Wood <[email protected]>--- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)sequencer.c +2
-1@@ -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 ;
Quoted 7 lines
sequencer.c static int do_recursive_merge(struct commit *base, struct commit *next, · line 444
Quoted 1 line
sequencer.c +0
-0read_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.
Quoted 4 lines
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
Quoted 2 lines
Martin
Quoted 15 lines · Phillip Wood
Quoted 4 lines · Martin Ågren
On 15 November 2017 at 11:41, Phillip Wood <[email protected]> wrote:
Quoted 8 lines
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.
Quoted 3 lines
Hi Martin, thanks for your comments.
LOCK_DIE_ON_ERRORalso 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
Hi Phillip,
sequencer.c static int do_recursive_merge(struct commit *base, struct commit *next, · line 441
Quoted 10 lines · Phillip Wood
sequencer.c +1
-1@@ -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
PATCH v2 1/1 sequencer: reschedule pick if index can't be locked
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
---
* I've queue this, taking input from responses by Dscho and Martin.
sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
sequencer.c +2 -1
@@ -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 ();
Signature · 1 line
2.15.0-360-gf2497ca0e5
Quoted 10 lines · Junio C Hamano
From: Phillip Wood <[email protected]>
Date: Wed, 15 Nov 2017 10:41:25 +0000If 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.
Quoted 4 lines
Signed-off-by Phillip Wood <[email protected]>---
* 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
Quoted 18 lines
sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)sequencer.c +2
-1@@ -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 ();