Re: [PATCH] sequencer: reschedule pick if index can't be locked

[Martin Ågren]

On 16 November 2017 at 11:43, Phillip Wood  wrote:
> On 15/11/17 18:44, Martin Ågren wrote:
>>
>> On 15 November 2017 at 11:41, Phillip Wood 
>> 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

Re: [PATCH] sequencer: reschedule pick if index can't be locked

[Phillip Wood]

On 15/11/17 18:44, Martin Ågren wrote:

On 15 November 2017 at 11:41, Phillip Wood  wrote:

From: Phillip Wood 

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 
---
  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(_lock, LOCK_DIE_ON_ERROR);
+   if (hold_locked_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

Re: [PATCH] sequencer: reschedule pick if index can't be locked

[Johannes Schindelin]

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(_lock, LOCK_DIE_ON_ERROR);
> + if (hold_locked_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

Re: [PATCH] sequencer: reschedule pick if index can't be locked

[Martin Ågren]

On 15 November 2017 at 11:41, Phillip Wood  wrote:
> From: Phillip Wood 
>
> 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 
> ---
>  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(_lock, LOCK_DIE_ON_ERROR);
> +   if (hold_locked_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