Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Hi Junio,

On 2015-10-12 22:28, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
>>> I think the most sensible regression fix as the first step at this
>>> point is to call it as a separate process, just like the code calls
>>> "apply" as a separate process for each patch.  Optimization can come
>>> later when it is shown that it matters---we need to regain
>>> correctness first.
>>
>> I fear that you might underestimate the finality of this "first
>> step". If you reintroduce that separate process, not only is it a
>> performance regression, but could we really realistically expect any
>> further steps to happen after that? I do not think so.
>> ...
>> For the above reasons, I respectfully remain convinced that
>> reintroducing the separate process would be a mistake.
> 
> I am not saying we should forever do run_command() going forward.

Fine, I will stop arguing about this and go back grumble in my corner.

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Hi Junio,

On 2015-10-09 20:40, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>>> Instead, stepping back a bit, I wonder if we can extend coverage of
>>> the helpful message to all die() calls when running git-am. We could
>>> just install a die routine with set_die_routine() in builtin/am.c.
>>> Then, should die() be called anywhere, the helpful error message will
>>> be printed as well.
>>
>> That could certainly be a valid approach and may give us a better
>> end result.  If it works, it could be a change that is localized
>> with a lot less impact.
> 
> I looked at the codepath involved, and I do not think that is a
> feasible way forward in this case.  It is not about a "helpful
> message" at all.  You would have to do everything that is done in
> the error codepath in your custom die routine, which does not make
> much sense.
> 
> I think the most sensible regression fix as the first step at this
> point is to call it as a separate process, just like the code calls
> "apply" as a separate process for each patch.  Optimization can come
> later when it is shown that it matters---we need to regain
> correctness first.

I fear that you might underestimate the finality of this "first step". If you 
reintroduce that separate process, not only is it a performance regression, but 
could we really realistically expect any further steps to happen after that? I 
do not think so.

Also, please let me clarify why I called reintroducing the separate process 
"heavy-handed" in an earlier message. As pointed out by Paul, the dying code 
paths indicate non-recoverable problems, i.e. serious problems that not even a 
rerere could fix. Modulo bugs, of course, but those bugs need to be fixed and 
not papered over. The real bug, after all, is that a non-recoverable code path 
is taken when it should just return with an error code.

Reintroducing the separate process would not help the endeavor to fix those 
code paths. Indeed, if we still had the separate process, I would never have 
discovered that bug!

And we should also keep in mind that this whole story demonstrates the rather 
serious shortcomings of the mindset we display throughout libgit.a where it 
does not behave like a library at all. Of course, hindsight is 20/20, so it is 
all too easy, and not exactly fair, to criticise the short-sightedness of 
writing code that does not clean up after itself "because it is a short-running 
process anyway". I certainly have contributed to these problems myself! All the 
more eager am I to help *increase* the number of functions in libgit.a that are 
reentrant, eventually making libgit.a behave like a true library. And in that 
light, what you called "the first step" appears like it would be a huge step 
backwards.

In contrast, introducing the "gentle" flag would be a step in the right 
direction. It is a much lighter stop-gap solution, too.

For the above reasons, I respectfully remain convinced that reintroducing the 
separate process would be a mistake.

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Hi Junio,

On 2015-10-09 20:55, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> I would prefer the endgame to be an efficient implementation of
> merge_recursive_generic(), a function that you can call without you
> having to worry about it dying.
> 
> But the patch in this thread is not that, if I am reading Johannes's
> description correctly.  

As pointed out by Paul, the recursive merge should only die() in case of a 
non-recoverable error so serious that not even rerere can do anything (such as 
a read error).

The bug is that a code path for a non-recoverable error is taken.

Spawning the recursive merge would be a work-around making the need to fix that 
bug less urgent, nothing more. (So would introducing the 'gentle' flag, of 
course, albeit without the performance regression of spawning a new process.)

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Hi Torsten,

On 2015-10-10 18:05, Torsten Bögershausen wrote:
> On 09.10.15 12:11, Johannes Schindelin wrote:
>> Me again,
>>
>> On 2015-10-09 11:50, Johannes Schindelin wrote:
>>>
>>> On 2015-10-09 03:40, Paul Tan wrote:
 On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>> Brendan Forster noticed that we no longer see the helpful message after
>> a failed `git pull --rebase`. It turns out that the builtin `am` calls
>> the recursive merge function directly, not via a separate process.
>>
>> [... cut lots of unnecessary text ...]
>>
>> +test_expect_success 'failed --rebase shows advice' '
>> +git checkout -b diverging &&
>> +test_commit attributes .gitattributes "* text=auto" attrs &&
>> +sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
>> +git update-index --cacheinfo 0644 $sha1 file &&
>> +git commit -m v1-with-cr &&
>> +git checkout -f -b fails-to-rebase HEAD^ &&
>> +test_commit v2-without-cr file "2" file2-lf &&
>> +test_must_fail git pull --rebase . diverging 2>err >out &&
>> +grep "When you have resolved this problem" out
>> +'
>> +
> 
> One other question:
> Is it good to mix 2 different things in one test case ?
> "shows the helpful advice when failing" is one thing,
> and the problematic CRLF handling another.

I do not necessarily test things that work, and have been working for a long 
time, so no, I do not want to split that into two.

I could trigger the regression only via CR/LF, that is the only reason why 
CR/LF are used here. I do *not* want to test for anything CR/LF specific.

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Johannes Schindelin  writes:

>> I think the most sensible regression fix as the first step at this
>> point is to call it as a separate process, just like the code calls
>> "apply" as a separate process for each patch.  Optimization can come
>> later when it is shown that it matters---we need to regain
>> correctness first.
>
> I fear that you might underestimate the finality of this "first
> step". If you reintroduce that separate process, not only is it a
> performance regression, but could we really realistically expect any
> further steps to happen after that? I do not think so.
> ...
> For the above reasons, I respectfully remain convinced that
> reintroducing the separate process would be a mistake.

I am not saying we should forever do run_command() going forward.

But I do not want to keep the direct call to merge_recursive() in
'maint'.  The topic was supposed to be "rewrite in C".  I do not
recall (and do not feel the need to read "git log" output to find
out) exactly how the series progressed, but a logical progression
would have been to run merge-recursive via run_command() like I
showed in the quick-fix in an early part of the series, followed by
a patch to turn it into a direct call to merge_recursive() as an
optimization change.

And the latter step turned out to be a regression caused by a
premature optimization.  If something introduces a regression, it
gets reverted.  As the scripted version certainly did not make an
internal call, we should just run_command().  And that is what we
want to have in the stable version people use every day.

The only thing I am saying is that the change to make a direct call
should come on top of the run_command() version with its own
justification as an optimization patch.

Going that route may require you to redo your patch, measure
performance improvements, ensure there is no unintended fallout in
other callers and longer term maintainability of the codebaths
involved, write a good log message, etc.  And such a fix to
merge_recursive() needs to be cooked sufficiently in 'pu' and
'next'.  And I view all that as a good thing.  I really hate to see
that this premature optimization to come back and bite us again---we
didn't see it while reviewing because "builtin-am: implement --3way"
was done in a single step with premature optimization from the
beginning.

Now, there are many reasons why the "first step" might turn out to
be the permanent optimal solution.

I did an unscientific experiment to rebase each of the 25 topics
that are cooking in 'next' on top of 'master'.  Only 3 of them will
fall back to the three-way merge machinery.  One possible reason why
the "first step" could stay a good-enough solution is that people
would not care and/or notice, because it is not like you are paying
unnecessary cost to spawn merge-recursive for each and every change.
It only kicks in when the patch does not apply.

Then I randomly picked one (jc/merge-drop-old-syntax) of the three
topics that does fall back, made it into a patch and ran "am -3" on
top of 'master' with and without the "first step".  The numbers from
5 runs of each look like this:

real0m0.109s  real0m0.109s
user0m0.080s  user0m0.079s
sys 0m0.034s  sys 0m0.035s

real0m0.109s  real0m0.105s
user0m0.095s  user0m0.087s
sys 0m0.018s  sys 0m0.022s

real0m0.109s  real0m0.110s
user0m0.075s  user0m0.086s
sys 0m0.038s  sys 0m0.028s

real0m0.107s  real0m0.108s
user0m0.083s  user0m0.075s
sys 0m0.029s  sys 0m0.038s

real0m0.106s  real0m0.108s
user0m0.086s  user0m0.090s
sys 0m0.025s  sys 0m0.023s

I am curious to see a similar number on platforms with slower
run_command().  From the above numbers alone, I cannot even see
which ones are with run_command(), even though I know the numbers on
the right hand side column were taken with run_command() and the
numbers on the left hand side column were taken with internal call.

Another possible reason why the "first step" could stay a
good-enough solution is that merge_recursive() in itself is a
heavy-weight operation that the cost of spawning a process is not
even felt [*1*].  After all, it's not like we are talking about
spawning the cost of "update-ref HEAD" dominating the cost of the
actual operation.

By the way, in order to make sure that I am running the correct
binary, I did "strace -f -e execve" on "am -3".  "mailsplit" and
"mailinfo", both of which are a lot more likely to be affected by
the cost of spawning because they are mostly dumb and straight
text-to-text 

Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Torsten Bögershausen]

On 09.10.15 12:11, Johannes Schindelin wrote:
> Me again,
> 
> On 2015-10-09 11:50, Johannes Schindelin wrote:
>>
>> On 2015-10-09 03:40, Paul Tan wrote:
>>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
 Johannes Schindelin  writes:

> Brendan Forster noticed that we no longer see the helpful message after
> a failed `git pull --rebase`. It turns out that the builtin `am` calls
> the recursive merge function directly, not via a separate process.
>
> But that function was not really safe to be called that way, as it
> die()s pretty liberally.
>>>
>>> I'm not too familiar with the merge-recursive.c code, but I was under
>>> the impression that it only called die() under fatal conditions. In
>>> common use cases, such as merge conflicts, it just errors out and the
>>> helpful error message does get printed. Is there a reproduction recipe
>>> for this?
>>
>> Yes. Sorry, I should have added that as part of the patch series.
>> Actually, I should have written it *before* making those patches.
>> Because it revealed that the underlying problem is completely
>> different: *Normally* you are correct, if `pull --rebase` fails with a
>> merge conflict, the advice is shown.
>>
>> The problem occurs with CR/LF.
> 
> I finally have that test case working, took way longer than I wanted to:
> 
> -- snip --
> Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice 
> when failing
> Author: Johannes Schindelin 
> Date:   Fri Oct 9 11:15:30 2015 +0200
> 
> Signed-off-by: Johannes Schindelin 
> 
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index a0013ee..bce332f 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -237,6 +237,18 @@ test_expect_success '--rebase' '
>   test new = "$(git show HEAD:file2)"
>  '
>  
> +test_expect_success 'failed --rebase shows advice' '
> + git checkout -b diverging &&
> + test_commit attributes .gitattributes "* text=auto" attrs &&
> + sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
> + git update-index --cacheinfo 0644 $sha1 file &&
> + git commit -m v1-with-cr &&
> + git checkout -f -b fails-to-rebase HEAD^ &&
> + test_commit v2-without-cr file "2" file2-lf &&
> + test_must_fail git pull --rebase . diverging 2>err >out &&
> + grep "When you have resolved this problem" out
> +'
> +

One other question:
Is it good to mix 2 different things in one test case ?
"shows the helpful advice when failing" is one thing,
and the problematic CRLF handling another.

Does it make sense to simply create "really-modified" file to test the helpful 
advice ?

And may be another one witch test the CRLF handling, (may be)








--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Hi Junio & Paul,

On 2015-10-09 03:40, Paul Tan wrote:
> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>>
>>> Brendan Forster noticed that we no longer see the helpful message after
>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls
>>> the recursive merge function directly, not via a separate process.
>>>
>>> But that function was not really safe to be called that way, as it
>>> die()s pretty liberally.
> 
> I'm not too familiar with the merge-recursive.c code, but I was under
> the impression that it only called die() under fatal conditions. In
> common use cases, such as merge conflicts, it just errors out and the
> helpful error message does get printed. Is there a reproduction recipe
> for this?

Yes. Sorry, I should have added that as part of the patch series. Actually, I 
should have written it *before* making those patches. Because it revealed that 
the underlying problem is completely different: *Normally* you are correct, if 
`pull --rebase` fails with a merge conflict, the advice is shown.

The problem occurs with CR/LF.

I have a reproducer and will wiggle it down to a proper test case.

>> If that is the case, I'd thinkg that we'd prefer, as a regression
>> fix to correct "that", i.e., let recursive-merge die and let the
>> caller catch its exit status.
> 
> We could do that, but I don't think it would be worth the overhead to
> spawn an additional process for every patch just to print an
> additional message should merge_recursive() call die().

I would hope that we do not go that direction! The benefit of making `am` a 
builtin was to *avoid* spawning, after all. Let's not make the experience for 
Windows users *worse* again.

> Instead, stepping back a bit, I wonder if we can extend coverage of
> the helpful message to all die() calls when running git-am. We could
> just install a die routine with set_die_routine() in builtin/am.c.
> Then, should die() be called anywhere, the helpful error message will
> be printed as well. fast-import.c and http-backend.c seem to do this.

This looks more like a work-around to me. In general, I think it is not really 
a good idea to treat each and every code path as if it is safe to die(). That 
would just preclude the code from being used as a library function.

But it looks more and more as if the problem lies with the CR/LF handling of 
Git. Will keep you posted.

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Me again,

On 2015-10-09 11:50, Johannes Schindelin wrote:
> 
> On 2015-10-09 03:40, Paul Tan wrote:
>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
>>> Johannes Schindelin  writes:
>>>
 Brendan Forster noticed that we no longer see the helpful message after
 a failed `git pull --rebase`. It turns out that the builtin `am` calls
 the recursive merge function directly, not via a separate process.

 But that function was not really safe to be called that way, as it
 die()s pretty liberally.
>>
>> I'm not too familiar with the merge-recursive.c code, but I was under
>> the impression that it only called die() under fatal conditions. In
>> common use cases, such as merge conflicts, it just errors out and the
>> helpful error message does get printed. Is there a reproduction recipe
>> for this?
> 
> Yes. Sorry, I should have added that as part of the patch series.
> Actually, I should have written it *before* making those patches.
> Because it revealed that the underlying problem is completely
> different: *Normally* you are correct, if `pull --rebase` fails with a
> merge conflict, the advice is shown.
> 
> The problem occurs with CR/LF.

I finally have that test case working, took way longer than I wanted to:

-- snip --
Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice 
when failing
Author: Johannes Schindelin 
Date:   Fri Oct 9 11:15:30 2015 +0200

Signed-off-by: Johannes Schindelin 

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a0013ee..bce332f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -237,6 +237,18 @@ test_expect_success '--rebase' '
test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success 'failed --rebase shows advice' '
+   git checkout -b diverging &&
+   test_commit attributes .gitattributes "* text=auto" attrs &&
+   sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
+   git update-index --cacheinfo 0644 $sha1 file &&
+   git commit -m v1-with-cr &&
+   git checkout -f -b fails-to-rebase HEAD^ &&
+   test_commit v2-without-cr file "2" file2-lf &&
+   test_must_fail git pull --rebase . diverging 2>err >out &&
+   grep "When you have resolved this problem" out
+'
+
 test_expect_success '--rebase fails with multiple branches' '
git reset --hard before-rebase &&
test_must_fail git pull --rebase . copy master 2>err &&
--

So the reason is that `unpack_trees()` fails with

error: Your local changes to the following files would be overwritten by 
merge:
file
Please, commit your changes or stash them before you can merge.

then returns -1 to its caller, `git_merge_trees()`, which still returns -1 in 
turn to *its* caller, `merge_trees()`, which then gives up by die()ing:

Aborting

I think there is more than one fix necessary to truly address the issue: the 
underlying problem is that Git handles *committed* CR/LF really badly when the 
corresponding `.gitattributes` label the file as `text=auto`. In fact, those 
files are labeled as modified in `git status`. If you change the line endings 
of them, they are labeled as modified in `git status`. And after a `git reset 
--hard`, they are *still* labeled as modified in `git status`.

I will try to make some time to continue to work on this later today, but in 
the meantime I would be relatively happy if we could introduce that gentle 
flag. It is really a very gentle patch, after all, much gentler than reverting 
to the heavy-handed spawning of `merge-recursive`.

Ciao,
Dscho
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Junio C Hamano  writes:

> I think the most sensible regression fix as the first step at this
> point is to call it as a separate process, just like the code calls
> "apply" as a separate process for each patch.  Optimization can come
> later when it is shown that it matters---we need to regain
> correctness first.

And the fix would look like this, I think.

It passes the test Dscho's 3/2 adds to t5520 ;-) but that is not
surprising.

---
Subject: [PATCH] am -3: do not let failed merge abort the error codepath

When "am" was rewritten in C, the codepath for falling back to
three-way merge was mistakenly made to make an internal call to
merge-recursive, disabling the error reporting code for certain
types of errors merge-recursive detects and reports by calling
die().

This is a quick-fix for correctness.  The ideal endgame would be to
replace run_command() in run_fallback_merge_recursive() with a
direct call after making sure that internal call to merge-recursive
does not die().

Signed-off-by: Junio C Hamano 
---
 builtin/am.c | 49 +
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..c869796 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1590,16 +1590,44 @@ static int build_fake_ancestor(const struct am_state 
*state, const char *index_f
 }
 
 /**
+ * Do the three-way merge using fake ancestor, his tree constructed
+ * from the fake ancestor and the postimage of the patch, and our
+ * state.
+ */
+static int run_fallback_merge_recursive(const struct am_state *state,
+   unsigned char *orig_tree,
+   unsigned char *our_tree,
+   unsigned char *his_tree)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   int status;
+
+   cp.git_cmd = 1;
+
+   argv_array_pushf(_array, "GITHEAD_%s=%.*s",
+sha1_to_hex(his_tree), linelen(state->msg), 
state->msg);
+   if (state->quiet)
+   argv_array_push(_array, "GIT_MERGE_VERBOSITY=0");
+
+   argv_array_push(, "merge-recursive");
+   argv_array_push(, sha1_to_hex(orig_tree));
+   argv_array_push(, "--");
+   argv_array_push(, sha1_to_hex(our_tree));
+   argv_array_push(, sha1_to_hex(his_tree));
+
+   status = run_command() ? (-1) : 0;
+   discard_cache();
+   read_cache();
+   return status;
+}
+
+/**
  * Attempt a threeway merge, using index_path as the temporary index.
  */
 static int fall_back_threeway(const struct am_state *state, const char 
*index_path)
 {
unsigned char orig_tree[GIT_SHA1_RAWSZ], his_tree[GIT_SHA1_RAWSZ],
  our_tree[GIT_SHA1_RAWSZ];
-   const unsigned char *bases[1] = {orig_tree};
-   struct merge_options o;
-   struct commit *result;
-   char *his_tree_name;
 
if (get_sha1("HEAD", our_tree) < 0)
hashcpy(our_tree, EMPTY_TREE_SHA1_BIN);
@@ -1651,22 +1679,11 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
 * changes.
 */
 
-   init_merge_options();
-
-   o.branch1 = "HEAD";
-   his_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);
-   o.branch2 = his_tree_name;
-
-   if (state->quiet)
-   o.verbosity = 0;
-
-   if (merge_recursive_generic(, our_tree, his_tree, 1, bases, )) 
{
+   if (run_fallback_merge_recursive(state, orig_tree, our_tree, his_tree)) 
{
rerere(state->allow_rerere_autoupdate);
-   free(his_tree_name);
return error(_("Failed to merge in the changes."));
}
 
-   free(his_tree_name);
return 0;
 }
 
-- 
2.6.1-296-ge15092e

--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Johannes Schindelin  writes:

> I finally have that test case working, took way longer than I wanted to:

This certainly fails without any fix and passes either with your
two-patch or a more conservative run_command() fix that I sent
separately.

However, this new test (becomes 5520.20) seems to break the
precondition of some later tests---I didn't dig but 5520.22 (which
used to be .21) fails after letting this new test run and succeed.

> I think there is more than one fix necessary to truly address the
> issue: the underlying problem is that Git handles *committed*
> CR/LF really badly when the corresponding `.gitattributes` label
> the file as `text=auto`.

Yeah, that certainly is the right thing to tackle.
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Torsten Bögershausen]

On 2015-10-09 12.11, Johannes Schindelin wrote:
> Me again,
> 
> On 2015-10-09 11:50, Johannes Schindelin wrote:
>>
>> On 2015-10-09 03:40, Paul Tan wrote:
>>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
 Johannes Schindelin  writes:

> Brendan Forster noticed that we no longer see the helpful message after
> a failed `git pull --rebase`. It turns out that the builtin `am` calls
> the recursive merge function directly, not via a separate process.
>
> But that function was not really safe to be called that way, as it
> die()s pretty liberally.
>>>
>>> I'm not too familiar with the merge-recursive.c code, but I was under
>>> the impression that it only called die() under fatal conditions. In
>>> common use cases, such as merge conflicts, it just errors out and the
>>> helpful error message does get printed. Is there a reproduction recipe
>>> for this?
>>
>> Yes. Sorry, I should have added that as part of the patch series.
>> Actually, I should have written it *before* making those patches.
>> Because it revealed that the underlying problem is completely
>> different: *Normally* you are correct, if `pull --rebase` fails with a
>> merge conflict, the advice is shown.
>>
>> The problem occurs with CR/LF.
> 
> I finally have that test case working, took way longer than I wanted to:
> 
> -- snip --
> Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice 
> when failing
> Author: Johannes Schindelin 
> Date:   Fri Oct 9 11:15:30 2015 +0200
> 
> Signed-off-by: Johannes Schindelin 
> 
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index a0013ee..bce332f 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -237,6 +237,18 @@ test_expect_success '--rebase' '
>   test new = "$(git show HEAD:file2)"
>  '
>  
> +test_expect_success 'failed --rebase shows advice' '
> + git checkout -b diverging &&
> + test_commit attributes .gitattributes "* text=auto" attrs &&
> + sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
> + git update-index --cacheinfo 0644 $sha1 file &&
> + git commit -m v1-with-cr &&
> + git checkout -f -b fails-to-rebase HEAD^ &&
> + test_commit v2-without-cr file "2" file2-lf &&
> + test_must_fail git pull --rebase . diverging 2>err >out &&
> + grep "When you have resolved this problem" out
> +'
> +
>  test_expect_success '--rebase fails with multiple branches' '
>   git reset --hard before-rebase &&
>   test_must_fail git pull --rebase . copy master 2>err &&
> --
> 
> So the reason is that `unpack_trees()` fails with
> 
> error: Your local changes to the following files would be overwritten by 
> merge:
>   file
> Please, commit your changes or stash them before you can merge.
> 
> then returns -1 to its caller, `git_merge_trees()`, which still returns -1 in 
> turn to *its* caller, `merge_trees()`, which then gives up by die()ing:
> 
> Aborting
> 
> I think there is more than one fix necessary to truly address the issue: the 
> underlying problem is that Git handles *committed* CR/LF really badly when 
> the corresponding `.gitattributes` label the file as `text=auto`. In fact, 
> those files are labeled as modified in `git status`. If you change the line 
> endings of them, they are labeled as modified in `git status`. And after a 
> `git reset --hard`, they are *still* labeled as modified in `git status`.
This is related to the normalization feature of Git:
https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html
  *   text=auto
This ensures that all files that Git considers to be text will have normalized
(LF) line endings in the repository.

The normalization feature has 2 consequences:
a) - Files will get normalized at the next commit,
 Line endings of the changed lines are normalized
 Line endings of unchanged lines are normalized
b) - Not normalized files will get normalized (at the next commit),
even if they are unchanged otherwise.


As Git knows (* text=auto), that files are normalized at the next commit,
they will change in the repo, and they are marked as changed already now.
This is by design.

The normalization has been disabled for core.autocrlf = true in commit
fd6cce9e (Eyvind Bernhardsen   2010-05-19 22:43:10 +0200  207)  
 * This is the
new safer autocrlf handling.
(See convert.c)

I'm in the mood to propose a patch that disables/suppresses the normalization
even for "* text=auto", if a file has CRLF in the repo.
This would make core.autocrlf = true do the same as "* text=auto".

I'm nearly sure, that this change would break things for some users,
and improve for others.

Currently t0027 tests this behavior, and as soon as we have the new
NNO tests establish, I will propose some cleanups in convert.c
(without change of behavour), and later to make
core.autocrlf = true
to do the same as
* 

Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Paul Tan  writes:

> That said, I do agree that even if we die(), we could try to be more
> helpful by printing additional helpful instructions.
>
>> If that is the case, I'd thinkg that we'd prefer, as a regression
>> fix to correct "that", i.e., let recursive-merge die and let the
>> caller catch its exit status.
>
> We could do that, but I don't think it would be worth the overhead to
> spawn an additional process for every patch just to print an
> additional message should merge_recursive() call die().

For a thing that (1) has to be run every time in the whole operation
and (2) is a very small operation itself whose runtime cost can be
dwarfed by cost of spawning on some platforms, it is clearly better
to run it internally instead of running it via run_command()
interface.  This is especially so if it (3) wants to just kill the
whole operation when it finds a problem anyway.  For example, it
would be crazy to run "update-ref" via run_command() in the "am"
that is rewritten in C.

But does the same reasoning apply to the use of merge-recursive in
"am -3" codepath, where it (1) runs only as a fallback when straight
application of the patch fails, (2) runs a heavy-weight recursive
merge machinery, and (3) now a regression is pointed out that it
wants to do more than just calling die() when there is a problem?

You seem to be viewing the world in black-and-white and thinking
that run_command() is unconditionally bad.  You need to stop doing
that.  Instead, view it as another tool that gives a much better
isolation from the main flow of logic (hence flexiblity) that comes
with a bigger cost.  I am not convinced with your "I don't think it
would be worth".

> Instead, stepping back a bit, I wonder if we can extend coverage of
> the helpful message to all die() calls when running git-am. We could
> just install a die routine with set_die_routine() in builtin/am.c.
> Then, should die() be called anywhere, the helpful error message will
> be printed as well.

That could certainly be a valid approach and may give us a better
end result.  If it works, it could be a change that is localized
with a lot less impact.

Thanks.
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Junio C Hamano  writes:

>> Instead, stepping back a bit, I wonder if we can extend coverage of
>> the helpful message to all die() calls when running git-am. We could
>> just install a die routine with set_die_routine() in builtin/am.c.
>> Then, should die() be called anywhere, the helpful error message will
>> be printed as well.
>
> That could certainly be a valid approach and may give us a better
> end result.  If it works, it could be a change that is localized
> with a lot less impact.

I looked at the codepath involved, and I do not think that is a
feasible way forward in this case.  It is not about a "helpful
message" at all.  You would have to do everything that is done in
the error codepath in your custom die routine, which does not make
much sense.

I think the most sensible regression fix as the first step at this
point is to call it as a separate process, just like the code calls
"apply" as a separate process for each patch.  Optimization can come
later when it is shown that it matters---we need to regain
correctness first.
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Junio C Hamano  writes:

> I looked at the codepath involved, and I do not think that is a
> feasible way forward in this case.  It is not about a "helpful
> message" at all.  You would have to do everything that is done in
> the error codepath in your custom die routine, which does not make
> much sense.
>
> I think the most sensible regression fix as the first step at this
> point is to call it as a separate process, just like the code calls
> "apply" as a separate process for each patch.  Optimization can come
> later when it is shown that it matters---we need to regain
> correctness first.

Don't get me wrong by the above, though.

I would prefer the endgame to be an efficient implementation of
merge_recursive_generic(), a function that you can call without you
having to worry about it dying.

But the patch in this thread is not that, if I am reading Johannes's
description correctly.  

And by calling merge_recursive_generic() instead of spawning
merge-recursive via run_command(), your GSoC series introduced a
regression to "am -3".  I'd like to see the regression fixed, and
spawning merge-recursive is an obviously correct way to do so.

That is how "am -3" did it before builtin/am.c after all.

Once that is done, the users will not have to worry about this
regression, and merge_recursive_generic() implementation can be
improved separately.  The patch in this thread may serve as a good
starting point for that.

--
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

[PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin]

Brendan Forster noticed that we no longer see the helpful message after
a failed `git pull --rebase`. It turns out that the builtin `am` calls
the recursive merge function directly, not via a separate process. But
that function was not really safe to be called that way, as it die()s
pretty liberally.

As a consequence, the code that wanted to see whether the merge failed
is not even executed, and the helpful message advising how to fix the
mess is not displayed.

This topic branch fixes this.

Please note that there are a couple of unhandled die() calls in
merge-recursive.c, most of which indicate code paths that should
never be reached (except in the case of a bug).

But there are two other functions that can die(): `update_file_flags()`
(which returns void) and `merge_file_1()`.

The latter function is already nested quite deeply so that the code
would have to be made much uglier to handle the `gentle` flag. It is
also not quite clear to me whether those error cases can be hit in a
regular `git pull --rebase` (which is what I really care about most).

As `update_file_flags()` is called by functions returning void and
that are again called in turn by other functions that also return void,
fixing this part is more involved, so I would like to avoid it, unless
it is deemed absolutely necessary to address in this patch series.


Johannes Schindelin (2):
  merge_recursive_options: introduce the "gentle" flag
  pull --rebase: reinstate helpful message on abort

 builtin/am.c  |  1 +
 merge-recursive.c | 44 +++-
 merge-recursive.h |  1 +
 3 files changed, 37 insertions(+), 9 deletions(-)

-- 
2.6.1.windows.1


--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Paul Tan]

On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>> Brendan Forster noticed that we no longer see the helpful message after
>> a failed `git pull --rebase`. It turns out that the builtin `am` calls
>> the recursive merge function directly, not via a separate process.
>>
>> But that function was not really safe to be called that way, as it
>> die()s pretty liberally.

I'm not too familiar with the merge-recursive.c code, but I was under
the impression that it only called die() under fatal conditions. In
common use cases, such as merge conflicts, it just errors out and the
helpful error message does get printed. Is there a reproduction recipe
for this?

That said, I do agree that even if we die(), we could try to be more
helpful by printing additional helpful instructions.

> If that is the case, I'd thinkg that we'd prefer, as a regression
> fix to correct "that", i.e., let recursive-merge die and let the
> caller catch its exit status.

We could do that, but I don't think it would be worth the overhead to
spawn an additional process for every patch just to print an
additional message should merge_recursive() call die().

Instead, stepping back a bit, I wonder if we can extend coverage of
the helpful message to all die() calls when running git-am. We could
just install a die routine with set_die_routine() in builtin/am.c.
Then, should die() be called anywhere, the helpful error message will
be printed as well. fast-import.c and http-backend.c seem to do this.

Regards,
Paul
--
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: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Junio C Hamano]

Johannes Schindelin  writes:

> Brendan Forster noticed that we no longer see the helpful message after
> a failed `git pull --rebase`. It turns out that the builtin `am` calls
> the recursive merge function directly, not via a separate process.
>
> But that function was not really safe to be called that way, as it
> die()s pretty liberally.

If that is the case, I'd thinkg that we'd prefer, as a regression
fix to correct "that", i.e., let recursive-merge die and let the
caller catch its exit status.

Paul, what do you think?
--
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