Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails
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 * text=auto > > I will try to make some time to continue to work on this later today, but in > t
[PATCH v2] t0027: Improve test for not-normalized files
When a text file with mixed line endings is commited into the repo,
it is called "not normalized" (or NNO) in t0027.
The existing test case using repoMIX did not fully test all combinations:
(Especially when core.autocrlf = true)
Files with NL are not converted at commit, but at checkout, so a warning
NL->CRLF is given.
Files with CRLF are not converted at all (so no warning will be given),
unless they are marked as "text" or "auto".
Remove repoMIX introduced in commit 8eeab92f02, and replace it with
a combination of NNO tests.
Signed-off-by: Torsten Bögershausen
---
This is marked as V2, but is the same as V1
Probably my mail program mangled the V1 patch,
I'll try a different one (mail instead of thunderbird)
t/t0027-auto-crlf.sh | 191 ++-
1 file changed, 157 insertions(+), 34 deletions(-)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 1a56e5e..b343651 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -55,6 +55,26 @@ create_gitattributes () {
esac
}
+create_NNO_files () {
+ lfname=$1
+ crlfname=$2
+ lfmixcrlf=$3
+ lfmixcr=$4
+ crlfnul=$5
+ for crlf in false true input
+ do
+ for attr in "" auto text -text lf crlf
+ do
+ pfx=NNO_${crlf}_attr_${attr} &&
+ cp $lfname${pfx}_LF.txt &&
+ cp $crlfname ${pfx}_CRLF.txt &&
+ cp $lfmixcrlf ${pfx}_CRLF_mix_LF.txt &&
+ cp $lfmixcr ${pfx}_LF_mix_CR.txt &&
+ cp $crlfnul ${pfx}_CRLF_nul.txt
+ done
+ done
+}
+
check_warning () {
case "$1" in
LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
@@ -62,7 +82,7 @@ check_warning () {
'') >"$2".expect ;;
*) echo >&2 "Illegal 1": "$1" ; return false ;;
esac
- grep "will be replaced by" "$2" | sed -e "s/\(.*\) in [^ ]*$/\1/"
>"$2".actual
+ grep "will be replaced by" "$2" | sed -e "s/\(.*\) in [^ ]*$/\1/" |
uniq >"$2".actual
test_cmp "$2".expect "$2".actual
}
@@ -71,19 +91,10 @@ commit_check_warn () {
attr=$2
lfname=$3
crlfname=$4
- repoMIX=$5
- lfmixcrlf=$6
- lfmixcr=$7
- crlfnul=$8
+ lfmixcrlf=$5
+ lfmixcr=$6
+ crlfnul=$7
pfx=crlf_${crlf}_attr_${attr}
- # Special handling for repoMIX: It should already be in the repo
- # with CRLF
- f=repoMIX
- fname=${pfx}_$f.txt
- echo >.gitattributes &&
- cp $f $fname &&
- git -c core.autocrlf=false add $fname 2>"${pfx}_$f.err" &&
- git commit -m "repoMIX" &&
create_gitattributes "$attr" &&
for f in LF CRLF repoMIX LF_mix_CR CRLF_mix_LF LF_nul CRLF_nul
do
@@ -99,6 +110,45 @@ commit_check_warn () {
check_warning "$crlfnul" ${pfx}_CRLF_nul.err
}
+commit_chk_wrnNNO () {
+ crlf=$1
+ attr=$2
+ lfwarn=$3
+ crlfwarn=$4
+ lfmixcrlf=$5
+ lfmixcr=$6
+ crlfnul=$7
+ pfx=NNO_${crlf}_attr_${attr}
+ #Commit files on top of existing file
+ create_gitattributes "$attr" &&
+ for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
+ do
+ fname=${pfx}_$f.txt &&
+ cp $f $fname &&
+ git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
+ git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname
>"${pfx}_$f.err" 2>&1
+ done
+
+ test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
+ check_warning "$lfwarn" ${pfx}_LF.err
+ '
+ test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF" '
+ check_warning "$crlfwarn" ${pfx}_CRLF.err
+ '
+
+ test_expect_success "commit NNO files crlf=$crlf attr=$attr
CRLF_mix_LF" '
+ check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
+ '
+
+ test_expect_success "commit NNO files crlf=$crlf attr=$attr LF_mix_cr" '
+ check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
+ '
+
+ test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF_nul" '
+ check_warning "$crlfnul" ${pfx}_CRLF_nul.err
+ '
+}
+
check_files_in_repo () {
crlf=$1
attr=$2
@@ -115,6 +165,31 @@ check_files_in_repo () {
compare_files $crlfnul ${pfx}CRLF_nul.txt
}
+check_in_repo_NNO () {
+ crlf=$1
+ attr=$2
+ lfname=$3
+ crlfname=$4
+ lfmixcrlf=$5
+ lfmixcr=$6
+ crlfnul=$7
+ pfx=NNO_${crlf}_attr_${attr}_
+ test_expect_success "compare_files $lfname ${pfx}LF.txt" '
+ compare_files $lfname ${pfx}LF.txt
+ '
+ test_expect_success "compare_files $crlfname ${pfx}CRLF.txt" '
+ compare_files $crlfname ${pfx}CRLF.txt
+ '
+ test_expect_s
Re: [PATCH 0/1] merge: fix cache_entry use-after-free
David Turner writes:
>> > + assert(removed == dir);
>> > + drop_ce_ref(dir->ce);
>>
>> This is curious. In remove_name_hash() you do not have the
>> corresponding assert. Why is it necessary here (or is it
>> unnecessary over there)?
>
> It is unnecessary in any case: it's an assert rather than a check for an
> expected (or even unexpected) case. That just happens to be where Keith
> first managed to track down the use-after free, so that's where he
> happened to put the assert while trying to debug it. I'm in general in
> favor of more asserts rather than fewer, so I would prefer to keep it
> in. But I can remove it if you like.
OK, it was just the inconsistency between the two made them look
curious, as if one of them is more likely to get broken, or the
patch author was not so sure about the assumption, or something.
>> > + add_ce_ref(ce);
>> >add_name_hash(istate, ce);
>> > }
>>
>> What happens to the existing entry that used to be istate->cache[nr],
>> which may or may not be 'ce' that is replacing it?
>>
>> It turns out that all three calling sites are safe, but it is not
>> immediately obvious why. Perhaps some comment in front of the
>> function is in order, to warn those who may have to add a new caller
>> or restructure the existing calling chain, that istate->cache[nr] is
>> expected not to hold a live reference when the function is called,
>> or something?
>
> Happy to add it if you want, but to me it was clear without because if
> there were a value in istate->cache[nr], that old value would be leaked.
OK, that's sort-of-cheating, but is a sound short-cut ;-).
>> > + if (old != istate->split_index->base->cache[new->index - 1]) {
>> > + struct cache_entry *ce =
>> > istate->split_index->base->cache[new->index - 1];
>> > + drop_ce_ref(ce);
>> > + }
>> >istate->split_index->base->cache[new->index - 1] = new;
>>
>> Does 'new' already have the right refcount at this point?
>
> I spoke to Keith, and he thinks we do need an add_ce_ref there. I'll fix
> that on the reroll. Duy, do you agree?
--
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/1] merge: fix cache_entry use-after-free
+Duy Nguyen, who knows the split index better.
On Thu, 2015-10-08 at 13:00 -0700, Junio C Hamano wrote:
> David Turner writes:
>
> > From: Keith McGuigan
> >
> > During merges, we would previously free entries that we no longer need
> > in the destination index. But those entries might also be stored in
> > the dir_entry cache, and when a later call to add_to_index found them,
> > they would be used after being freed.
> >
> > To prevent this, add a ref count for struct cache_entry. Whenever
> > a cache entry is added to a data structure, the ref count is incremented;
> > when it is removed from the data structure, it is decremented. When
> > it hits zero, the cache_entry is freed.
> >
> > Signed-off-by: David Turner
> > Signed-off-by: Keith McGuigan
> > ---
>
> Thanks.
>
> > @@ -213,6 +214,32 @@ struct cache_entry {
> > struct pathspec;
> >
> > /*
> > + * Increment the cache_entry reference count. Should be called
> > + * whenever a pointer to a cache_entry is retained in a data structure,
> > + * thus marking it as alive.
> > + */
> > +static inline void add_ce_ref(struct cache_entry *ce)
> > +{
> > + assert(ce != NULL && ce->ref_count >= 0);
> > + ++ce->ref_count;
> > +}
>
> We tend to prefer post-increment when the distinction between pre-
> or post- does not make any difference, which is the case here.
Will fix.
> > diff --git a/name-hash.c b/name-hash.c
> > index 702cd05..f12c919 100644
> > --- a/name-hash.c
> > +++ b/name-hash.c
> > @@ -92,7 +93,9 @@ static void remove_dir_entry(struct index_state *istate,
> > struct cache_entry *ce)
> > struct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));
> > while (dir && !(--dir->nr)) {
> > struct dir_entry *parent = dir->parent;
> > - hashmap_remove(&istate->dir_hash, dir, NULL);
> > + struct dir_entry *removed = hashmap_remove(&istate->dir_hash,
> > dir, NULL);
> > + assert(removed == dir);
> > + drop_ce_ref(dir->ce);
>
> This is curious. In remove_name_hash() you do not have the
> corresponding assert. Why is it necessary here (or is it
> unnecessary over there)?
It is unnecessary in any case: it's an assert rather than a check for an
expected (or even unexpected) case. That just happens to be where Keith
first managed to track down the use-after free, so that's where he
happened to put the assert while trying to debug it. I'm in general in
favor of more asserts rather than fewer, so I would prefer to keep it
in. But I can remove it if you like.
> > @@ -147,6 +151,7 @@ void remove_name_hash(struct index_state *istate,
> > struct cache_entry *ce)
> > return;
> > ce->ce_flags &= ~CE_HASHED;
> > hashmap_remove(&istate->name_hash, ce, ce);
> > + drop_ce_ref(ce);
> >
> > if (ignore_case)
> > remove_dir_entry(istate, ce);
> > diff --git a/read-cache.c b/read-cache.c
> > index 87204a5..442de34 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -53,6 +53,7 @@ static const char *alternate_index_output;
> > static void set_index_entry(struct index_state *istate, int nr, struct
> > cache_entry *ce)
> > {
> > istate->cache[nr] = ce;
> > + add_ce_ref(ce);
> > add_name_hash(istate, ce);
> > }
>
> What happens to the existing entry that used to be istate->cache[nr],
> which may or may not be 'ce' that is replacing it?
>
> It turns out that all three calling sites are safe, but it is not
> immediately obvious why. Perhaps some comment in front of the
> function is in order, to warn those who may have to add a new caller
> or restructure the existing calling chain, that istate->cache[nr] is
> expected not to hold a live reference when the function is called,
> or something?
Happy to add it if you want, but to me it was clear without because if
there were a value in istate->cache[nr], that old value would be leaked.
Let me know.
> > @@ -314,8 +314,10 @@ void replace_index_entry_in_base(struct index_state
> > *istate,
> > istate->split_index->base &&
> > old->index <= istate->split_index->base->cache_nr) {
> > new->index = old->index;
> > - if (old != istate->split_index->base->cache[new->index - 1])
> > - free(istate->split_index->base->cache[new->index - 1]);
> > + if (old != istate->split_index->base->cache[new->index - 1]) {
> > + struct cache_entry *ce =
> > istate->split_index->base->cache[new->index - 1];
> > + drop_ce_ref(ce);
> > + }
> > istate->split_index->base->cache[new->index - 1] = new;
>
> Does 'new' already have the right refcount at this point?
I spoke to Keith, and he thinks we do need an add_ce_ref there. I'll fix
that on the reroll. Duy, do you agree?
--
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] t0027: Improve test for not-normalized files
Torsten Bögershausen writes:
This patch is seriously broken and I do not know how you managed to
do so. Notice how "+create_NNO_files" is indented but no other
added lines in the same hunk, for example.
I tried to hand-munge, but gave up.
> +commit_chk_wrnNNO () {
Squashing warn into wrn or (check into chk) does not make it any
easier to read or type.
> + crlf=$1
> + attr=$2
> + lfwarn=$3
> + crlfwarn=$4
> + lfmixcrlf=$5
> + lfmixcr=$6
> + crlfnul=$7
> + pfx=NNO_${crlf}_attr_${attr}
> + #Commit files on top of existing file
> + create_gitattributes "$attr" &&
> + for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> + do
> + fname=${pfx}_$f.txt &&
> + cp $f $fname &&
> + git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> + git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname
> >"${pfx}_$f.err" 2>&1
> + done
> +
> + test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
> + check_warning "$lfwarn" ${pfx}_LF.err
> + '
> + test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF" '
> + check_warning "$crlfwarn" ${pfx}_CRLF.err
> + '
> +
> + test_expect_success "commit NNO files crlf=$crlf attr=$attr
> CRLF_mix_LF" '
> + check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
> + '
> +
> + test_expect_success "commit NNO files crlf=$crlf attr=$attr LF_mix_cr" '
> + check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
> + '
> +
> + test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF_nul" '
> + check_warning "$crlfnul" ${pfx}_CRLF_nul.err
> + '
> +}
> +
> check_files_in_repo () {
> crlf=$1
> attr=$2
> @@ -115,6 +165,31 @@ check_files_in_repo () {
> compare_files $crlfnul ${pfx}CRLF_nul.txt
> }
> +check_in_repo_NNO () {
> + crlf=$1
> + attr=$2
> + lfname=$3
> + crlfname=$4
> + lfmixcrlf=$5
> + lfmixcr=$6
> + crlfnul=$7
> + pfx=NNO_${crlf}_attr_${attr}_
> + test_expect_success "compare_files $lfname ${pfx}LF.txt" '
> + compare_files $lfname ${pfx}LF.txt
> + '
> + test_expect_success "compare_files $crlfname ${pfx}CRLF.txt" '
> + compare_files $crlfname ${pfx}CRLF.txt
> + '
> + test_expect_success "compare_files $lfmixcrlf ${pfx}CRLF_mix_LF.txt" '
> + compare_files $lfmixcrlf ${pfx}CRLF_mix_LF.txt
> + '
> + test_expect_success "compare_files $lfmixcr ${pfx}LF_mix_CR.txt" '
> + compare_files $lfmixcr ${pfx}LF_mix_CR.txt
> + '
> + test_expect_success "compare_files $crlfnul ${pfx}CRLF_nul.txt" '
> + compare_files $crlfnul ${pfx}CRLF_nul.txt
> + '
> +}
> checkout_files () {
> eol=$1
> @@ -169,7 +244,11 @@ test_expect_success 'setup master' '
> printf "line1\nline2\rline3" >LF_mix_CR &&
> printf "line1\r\nline2\rline3" >CRLF_mix_CR &&
> printf "line1Q\r\nline2\r\nline3" | q_to_nul >CRLF_nul &&
> - printf "line1Q\nline2\nline3" | q_to_nul >LF_nul
> + printf "line1Q\nline2\nline3" | q_to_nul >LF_nul &&
> + create_NNO_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF
> CRLF_mix_LF &&
> + git -c core.autocrlf=false add NNO_*.txt &&
> + git commit -m "mixed line endings" &&
> + test_tick
> '
> @@ -191,46 +270,72 @@ else
> WAMIX=CRLF_LF
> fi
> -# attr LFCRLF repoMIX CRLFmixLF
> LFmixCR CRLFNUL
> +# attr LFCRLF CRLFmixLF LFmixCR
> CRLFNUL
> test_expect_success 'commit files empty attr' '
> - commit_check_warn false "" """"""""
> ""
>"" &&
> - commit_check_warn true "" "LF_CRLF" """LF_CRLF" "LF_CRLF"
> ""
>"" &&
> - commit_check_warn input "" """CRLF_LF" "CRLF_LF" "CRLF_LF"
> ""
>""
> + commit_check_warn false "" """"""""
> "" &&
> + commit_check_warn true "" "LF_CRLF" """LF_CRLF" ""
> "" &&
> + commit_check_warn input "" """CRLF_LF" "CRLF_LF" ""
> ""
> '
> test_expect_success 'commit files attr=auto' '
> - commit_check_warn false "auto" "$WILC" "$WICL" "$WAMIX" "$WAMIX"
> ""
>"" &&
> - commit_check_warn true "auto" "LF_CRLF" """LF_CRLF" "LF_CRLF"
> ""
>"" &&
> - commit_check_warn input "auto" """CRLF_LF" "CRLF_LF" "CRLF_LF"
> ""
>""
> + commit_check_warn false "auto" "$WILC" "$WICL" "$WAMIX" ""
> "" &&
> + commit_check_warn true "auto" "LF_CRLF" """LF_CRLF" ""
> "" &&
> + commit_check_warn input "auto" """CRLF_LF" "CRLF_LF" ""
> ""
> '
> test_expect_success 'commit files attr=text' '
> - commit_check_warn false "text" "$WILC" "$WICL" "$WAMIX" "$WAMIX"
> "$WILC"
> "$WICL" &&
> - commit_c
Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails
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
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(&cp.env_array, "GITHEAD_%s=%.*s",
+sha1_to_hex(his_tree), linelen(state->msg),
state->msg);
+ if (state->quiet)
+ argv_array_push(&cp.env_array, "GIT_MERGE_VERBOSITY=0");
+
+ argv_array_push(&cp.args, "merge-recursive");
+ argv_array_push(&cp.args, sha1_to_hex(orig_tree));
+ argv_array_push(&cp.args, "--");
+ argv_array_push(&cp.args, sha1_to_hex(our_tree));
+ argv_array_push(&cp.args, sha1_to_hex(his_tree));
+
+ status = run_command(&cp) ? (-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);
-
- 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(&o, our_tree, his_tree, 1, bases, &result))
{
+ 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 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
Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails
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 2/2] pull --rebase: reinstate helpful message on abort
Johannes Schindelin writes: > When calling `git pull --rebase`, things can go wrong. In such a case, > we want to tell the user about the most common ways out of this fix: > ... > builtin/am.c | 1 + > 1 file changed, 1 insertion(+) It is strange to see a patch to am that does not talk anything about it, though. And looking at the codepath, the issue does not have much to do with "pull --rebase". It doesn't even have much to do with "rebase". This is purely about "am -3" fallback codepath. Because fall-back-threeway wants to react to an error (i.e. calls merge_recursive_generic() and wants to use its return value), but merge_recursive_generic() can die, it fails to do so. It would not even run rerere(), for example. -- 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 v2 00/10] port branch.c to use ref-filter's printing options
Matthieu Moy writes:
> Junio C Hamano writes:
>
>> Then used_atom[] could become something like
>>
>> struct {
>> const char *str; /* e.g. "align:position=left,32" */
>> struct {
>> const char *part0; /* everything before '=' */
>> const char *part1; /* optional */
>> } *modifier;
>> int modifier_nr;
>> } *used_atom;
>
> If the goal is to prepare as much as possible when parsing the format
> string, I'd even push it one step further and have stg like
>
> struct {
> const char *str; /* e.g. "align:position=left,32" */
> union {
> struct {
> int position;
> enum { left, right, center } kind;
> } align;
> struct {
> ;
> } objectname;
> int modifier_nr;
> } *used_atom;
>
> Just a thought, I'm not sure how useful this would be, and this may be
> too much change for this series (so it may deserve a separate topic).
Yes, if we are willing to enrich the element of valid_atom[] array
with a type-specific parsing functions, we could even do that. Then
populate_value() would not have to do any parsing and just do the
filling.
I was shooting for a middle ground, but certainly with an eye
towards such an endgame state in the future.
--
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 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 v2] filter-branch: remove multi-line headers in msg filter
Michael J Gruber writes: >> Set IFS to an empty string for the “read” call, thus disabling the word >> splitting, which causes $header_line to be set to the non-empty value ' >> '. This allows the loop to fully consume the header lines before >> emitting the original, intact commit message. >> >> Signed-off-by: James McCoy >> --- > > Thanks for hanging in :) > > Reviewed-by: Michael J Gruber As long as you are fine with giving authorship to James, I am fine with that. I'll amend what is queued with your reviewed-by above and will merge to 'next'. 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
Hello
Chevrolet auto mobile company selected your email for the electronic Balloting. 900,000.00 USD was issued to you as winner send name,address and mobile for claim: chev.cla...@yandex.com --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- 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 1/2] test-path-utils.c: remove incorrect assumption
On Fri, Oct 9, 2015 at 2:05 AM, Junio C Hamano wrote: > I'll squash this in as part of your first patch that removes the > test from test-path-utils.c. That makes it clearer why it is the > right thing to remove the test, I'd think. > Great, many thanks! > 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
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
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 v2 00/10] port branch.c to use ref-filter's printing options
Junio C Hamano writes:
> Then used_atom[] could become something like
>
> struct {
> const char *str; /* e.g. "align:position=left,32" */
> struct {
> const char *part0; /* everything before '=' */
> const char *part1; /* optional */
> } *modifier;
> int modifier_nr;
> } *used_atom;
If the goal is to prepare as much as possible when parsing the format
string, I'd even push it one step further and have stg like
struct {
const char *str; /* e.g. "align:position=left,32" */
union {
struct {
int position;
enum { left, right, center } kind;
} align;
struct {
;
} objectname;
int modifier_nr;
} *used_atom;
Just a thought, I'm not sure how useful this would be, and this may be
too much change for this series (so it may deserve a separate topic).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2] filter-branch: remove multi-line headers in msg filter
James McCoy venit, vidit, dixit 09.10.2015 02:21: > df062010 (filter-branch: avoid passing commit message through sed) > introduced a regression when filtering commits with multi-line headers, > if the header contains a blank line. An example of this is a gpg-signed > commit: > > $ git cat-file commit signed-commit > tree 3d4038e029712da9fc59a72afbfcc90418451630 > parent 110eac945dc1713b27bdf49e74e5805db66971f0 > author A U Thor 1112912413 -0700 > committer C O Mitter 1112912413 -0700 > gpgsig -BEGIN PGP SIGNATURE- >Version: GnuPG v1 > >iEYEABECAAYFAlYXADwACgkQE7b1Hs3eQw23CACgldB/InRyDgQwyiFyMMm3zFpj >pUsAnA+f3aMUsd9mNroloSmlOgL6jIMO >=0Hgm >-END PGP SIGNATURE- > > Adding gpg > > As a consequence, "filter-branch --msg-filter cat" (which should leave the > commit message unchanged) spills the signature (after the internal blank > line) into the original commit message. > > The reason is that although the signature is indented, making the line a > whitespace only line, the “read” call is splitting the line based on > the shell's IFS, which defaults to . The leading > space is consumed and $header_line is empty, causing the “skip header > lines” loop to exit. > > The rest of the commit object is then re-used as the rewritten commit > message, causing the new message to include the signature of the > original commit. > > Set IFS to an empty string for the “read” call, thus disabling the word > splitting, which causes $header_line to be set to the non-empty value ' > '. This allows the loop to fully consume the header lines before > emitting the original, intact commit message. > > Signed-off-by: James McCoy > --- Thanks for hanging in :) Reviewed-by: Michael J Gruber -- 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
