Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Fabian Ruch baf...@gmail.com writes: On 05/27/2014 08:42 PM, Junio C Hamano wrote: Fabian Ruch baf...@gmail.com writes: [..] In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Hmph... do we still pass negative to exit() anywhere in our codebase? No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the sequencer to the shell. I had another look and found that `cmd_cherry_pick` calls `die` instead. Now, the commit inserts 128 as exit status in `fast_forward_to`. Would it be appropriate to mention the choice of exit status in the coding guidelines? I didn't know that the int-argument to exit(3) gets truncated and that this is why it is a general rule to only use values in the range from 0 to 255 with exit(3). I personally do not think of a reason why it is necessary to mention how the argument to exit(3) is expected to be used by the system, but if it is common not to know it, I guess it would not hurt to have a single paragraph with at most two lines. In any case, I agree that exiting with 1 that signals failed with conflict can be confusing to the caller. Can we have a test to demonstrate when this fix matters? Thanks. -- 8 -- Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge `do_pick_commit` handles three situations if it is not fast-forwarding. In order for `do_pick_commit` to identify the situation, it examines the return value of the selected merge command. 1. return value 0 stands for a clean merge 2. 1 is passed in case of a failed merge due to conflict 3. any other return value means that the merge did not even start So far, the sequencer returns 1 in case of a failed fast-forward, which would mean failed merge due to conflict. However, a fast-forward either starts and succeeds or does not start at all. In particular, it cannot fail in between like a merge with a dirty index due to conflicts. In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was chosen in line with the other situations in which the sequencer encounters an error. In such situations, the sequencer returns a negative value and `cherry-pick` translates this into a call to `die`. `die` then terminates the process with exit status 128. Signed-off-by: Fabian Ruch baf...@gmail.com --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90cac7b..225afcb 100644 --- a/sequencer.c +++ b/sequencer.c @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(1); /* the callee should have complained already */ + exit(128); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0, NULL); strbuf_addf(sb, %s: fast-forward, action_name(opts)); -- 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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
On 06/10/2014 01:56 PM, Junio C Hamano wrote: Fabian Ruch baf...@gmail.com writes: On 05/27/2014 08:42 PM, Junio C Hamano wrote: Fabian Ruch baf...@gmail.com writes: [..] In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Hmph... do we still pass negative to exit() anywhere in our codebase? No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the sequencer to the shell. I had another look and found that `cmd_cherry_pick` calls `die` instead. Now, the commit inserts 128 as exit status in `fast_forward_to`. Would it be appropriate to mention the choice of exit status in the coding guidelines? I didn't know that the int-argument to exit(3) gets truncated and that this is why it is a general rule to only use values in the range from 0 to 255 with exit(3). I personally do not think of a reason why it is necessary to mention how the argument to exit(3) is expected to be used by the system, but if it is common not to know it, I guess it would not hurt to have a single paragraph with at most two lines. In any case, I agree that exiting with 1 that signals failed with conflict can be confusing to the caller. Can we have a test to demonstrate when this fix matters? I think you are asking for a test and not for clarification. But a test was provided in 3/3 in this series. Was it not related directly enough? For clarification, this tri-state return value matters when the caller is planning to do some cleanup and needs to handle the fallout correctly. Maybe changing this return value is not the correct way forward, though. It might be better if the caller could examine the result after-the-fact instead. This would require some reliable state functions which I recall were somewhat scattered last time I looked. Also I cannot think of a reliable test for the previous cherry-pick failed during pre-condition checks and I'm not sure anything should exist to track this in .git outside of the return value for this function. -- 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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Phil Hord ho...@cisco.com writes: In any case, I agree that exiting with 1 that signals failed with conflict can be confusing to the caller. Can we have a test to demonstrate when this fix matters? I think you are asking for a test and not for clarification. But a test was provided in 3/3 in this series. Was it not related directly enough? X- Somehow I missed the 3 in 1/3 above and did not look beyond this first patch. For clarification, this tri-state return value matters when the caller is planning to do some cleanup and needs to handle the fallout correctly. Maybe changing this return value is not the correct way forward, though. It might be better if the caller could examine the result after-the-fact instead. I am not sure about that. For merge strategies exit with 1 iff you left the conflict in the index is the contract between git merge frontend and the strategies backend; if a similar contract is needed between the sequencer and its users, it is good to follow the same pattern for consistency. The resulting index and/or the working tree may or may not match the contents recorded in the HEAD commit but without the backend telling the caller, the caller cannot tell if the difference was from before the operation or created by the operation. -- 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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Hi Junio, On 05/27/2014 08:42 PM, Junio C Hamano wrote: Fabian Ruch baf...@gmail.com writes: [..] In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Hmph... do we still pass negative to exit() anywhere in our codebase? No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the sequencer to the shell. I had another look and found that `cmd_cherry_pick` calls `die` instead. Now, the commit inserts 128 as exit status in `fast_forward_to`. Would it be appropriate to mention the choice of exit status in the coding guidelines? I didn't know that the int-argument to exit(3) gets truncated and that this is why it is a general rule to only use values in the range from 0 to 255 with exit(3). Kind regards, Fabian -- 8 -- Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge `do_pick_commit` handles three situations if it is not fast-forwarding. In order for `do_pick_commit` to identify the situation, it examines the return value of the selected merge command. 1. return value 0 stands for a clean merge 2. 1 is passed in case of a failed merge due to conflict 3. any other return value means that the merge did not even start So far, the sequencer returns 1 in case of a failed fast-forward, which would mean failed merge due to conflict. However, a fast-forward either starts and succeeds or does not start at all. In particular, it cannot fail in between like a merge with a dirty index due to conflicts. In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was chosen in line with the other situations in which the sequencer encounters an error. In such situations, the sequencer returns a negative value and `cherry-pick` translates this into a call to `die`. `die` then terminates the process with exit status 128. Signed-off-by: Fabian Ruch baf...@gmail.com --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90cac7b..225afcb 100644 --- a/sequencer.c +++ b/sequencer.c @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(1); /* the callee should have complained already */ + exit(128); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0, NULL); strbuf_addf(sb, %s: fast-forward, action_name(opts)); -- 2.0.0 -- 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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Fabian Ruch baf...@gmail.com writes: `do_pick_commit` handles three situations if it is not fast-forwarding. In order for `do_pick_commit` to identify the situation, it examines the return value of the selected merge command. 1. return value 0 stands for a clean merge 2. 1 is passed in case of a failed merge due to conflict 3. any other return value means that the merge did not even start So far, the sequencer returns 1 in case of a failed fast-forward, which would mean failed merge due to conflict. However, a fast-forward either starts and succeeds or does not start at all. In particular, it cannot fail in between like a merge with a dirty index due to conflicts. In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Hmph... do we still pass negative to exit() anywhere in our codebase? Signed-off-by: Fabian Ruch baf...@gmail.com --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90cac7b..97cecca 100644 --- a/sequencer.c +++ b/sequencer.c @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(1); /* the callee should have complained already */ + exit(-1); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0, NULL); strbuf_addf(sb, %s: fast-forward, action_name(opts)); -- 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
[RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
`do_pick_commit` handles three situations if it is not fast-forwarding. In order for `do_pick_commit` to identify the situation, it examines the return value of the selected merge command. 1. return value 0 stands for a clean merge 2. 1 is passed in case of a failed merge due to conflict 3. any other return value means that the merge did not even start So far, the sequencer returns 1 in case of a failed fast-forward, which would mean failed merge due to conflict. However, a fast-forward either starts and succeeds or does not start at all. In particular, it cannot fail in between like a merge with a dirty index due to conflicts. In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Signed-off-by: Fabian Ruch baf...@gmail.com --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90cac7b..97cecca 100644 --- a/sequencer.c +++ b/sequencer.c @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(1); /* the callee should have complained already */ + exit(-1); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0, NULL); strbuf_addf(sb, %s: fast-forward, action_name(opts)); -- 1.9.3 -- 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
