* [bug?] checkout -m doesn't work without a base version @ 2011-12-04 22:31 Pete Harlan 2011-12-05 18:58 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Pete Harlan @ 2011-12-04 22:31 UTC (permalink / raw) To: git Hi, If during a merge I've resolved conflicts in foo.c but want to start over with foo.c to resolve them differently, I can say "git checkout -m foo.c" to restore it to its un-resolved state. But this only works if there's a base version; if foo.c was added in each branch, we get: error: path 'foo.c' does not have all three versions Git didn't need all three versions to create the original conflicted file, so why would it need them to recreate it? (The message is the same if I explicitly tell Git I don't want diff3 via "git checkout --conflict=merge foo.c".) If this is considered a bug worth fixing I'll write a test that it fails; if it's expected behavior I think the docs should mention that. Thanks, Pete Harlan pgit@pcharlan.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-04 22:31 [bug?] checkout -m doesn't work without a base version Pete Harlan @ 2011-12-05 18:58 ` Junio C Hamano 2011-12-07 7:30 ` Pete Harlan 2011-12-14 10:19 ` [bug?] checkout -m doesn't work without a base version Michael Schubert 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2011-12-05 18:58 UTC (permalink / raw) To: Pete Harlan; +Cc: git Pete Harlan <pgit@pcharlan.com> writes: > But this only works if there's a base version; if foo.c was added in > each branch, we get: > > error: path 'foo.c' does not have all three versions > > Git didn't need all three versions to create the original conflicted > file, so why would it need them to recreate it? Because the original "merge" was a bit more carefully written but "checkout -m" was written without worrying too much about "both sides added differently" corner case and still being defensive about not doing random thing upon getting an unexpected input state. IOW, being lazy ;-) How does this look? -- >8 -- checkout -m: no need to insist on having all 3 stages The content level merge machinery ll_merge() is prepared to merge correctly in "both sides added differently" case by using an empty blob as if it were the common ancestor. "checkout -m" could do the same, but didn't bother supporting it and instead insisted on having all three stages. Reported-by: Pete Harlan Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/checkout.c | 60 +++++++++++++++++++++++++++++++-------------------- 1 files changed, 36 insertions(+), 24 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2a80772..923d040 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -114,16 +114,21 @@ static int check_stage(int stage, struct cache_entry *ce, int pos) return error(_("path '%s' does not have their version"), ce->name); } -static int check_all_stages(struct cache_entry *ce, int pos) +static int check_stages(unsigned stages, struct cache_entry *ce, int pos) { - if (ce_stage(ce) != 1 || - active_nr <= pos + 2 || - strcmp(active_cache[pos+1]->name, ce->name) || - ce_stage(active_cache[pos+1]) != 2 || - strcmp(active_cache[pos+2]->name, ce->name) || - ce_stage(active_cache[pos+2]) != 3) - return error(_("path '%s' does not have all three versions"), - ce->name); + unsigned seen = 0; + const char *name = ce->name; + + while (pos < active_nr) { + ce = active_cache[pos]; + if (strcmp(name, ce->name)) + break; + seen |= (1 << ce_stage(ce)); + pos++; + } + if ((stages & seen) != stages) + return error(_("path '%s' does not have all necessary versions"), + name); return 0; } @@ -150,18 +155,27 @@ static int checkout_merged(int pos, struct checkout *state) int status; unsigned char sha1[20]; mmbuffer_t result_buf; + unsigned char threeway[3][20]; + unsigned mode; + + memset(threeway, 0, sizeof(threeway)); + while (pos < active_nr) { + int stage; + stage = ce_stage(ce); + if (!stage || strcmp(path, ce->name)) + break; + hashcpy(threeway[stage - 1], ce->sha1); + if (stage == 2) + mode = create_ce_mode(ce->ce_mode); + pos++; + ce = active_cache[pos]; + } + if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2])) + return error(_("path '%s' does not have necessary versions"), path); - if (ce_stage(ce) != 1 || - active_nr <= pos + 2 || - strcmp(active_cache[pos+1]->name, path) || - ce_stage(active_cache[pos+1]) != 2 || - strcmp(active_cache[pos+2]->name, path) || - ce_stage(active_cache[pos+2]) != 3) - return error(_("path '%s' does not have all 3 versions"), path); - - read_mmblob(&ancestor, active_cache[pos]->sha1); - read_mmblob(&ours, active_cache[pos+1]->sha1); - read_mmblob(&theirs, active_cache[pos+2]->sha1); + read_mmblob(&ancestor, threeway[0]); + read_mmblob(&ours, threeway[1]); + read_mmblob(&theirs, threeway[2]); /* * NEEDSWORK: re-create conflicts from merges with @@ -192,9 +206,7 @@ static int checkout_merged(int pos, struct checkout *state) if (write_sha1_file(result_buf.ptr, result_buf.size, blob_type, sha1)) die(_("Unable to add merge result for '%s'"), path); - ce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode), - sha1, - path, 2, 0); + ce = make_cache_entry(mode, sha1, path, 2, 0); if (!ce) die(_("make_cache_entry failed for path '%s'"), path); status = checkout_entry(ce, state, NULL); @@ -252,7 +264,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec, } else if (stage) { errs |= check_stage(stage, ce, pos); } else if (opts->merge) { - errs |= check_all_stages(ce, pos); + errs |= check_stages((1<<2) | (1<<3), ce, pos); } else { errs = 1; error(_("path '%s' is unmerged"), ce->name); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-05 18:58 ` Junio C Hamano @ 2011-12-07 7:30 ` Pete Harlan 2011-12-08 18:27 ` Junio C Hamano 2011-12-20 20:37 ` [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail Ævar Arnfjörð Bjarmason 2011-12-14 10:19 ` [bug?] checkout -m doesn't work without a base version Michael Schubert 1 sibling, 2 replies; 15+ messages in thread From: Pete Harlan @ 2011-12-07 7:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 12/05/2011 10:58 AM, Junio C Hamano wrote: > Pete Harlan <pgit@pcharlan.com> writes: > >> But this only works if there's a base version; if foo.c was added in >> each branch, we get: >> >> error: path 'foo.c' does not have all three versions >> >> Git didn't need all three versions to create the original conflicted >> file, so why would it need them to recreate it? > > Because the original "merge" was a bit more carefully written but > "checkout -m" was written without worrying too much about "both sides > added differently" corner case and still being defensive about not doing > random thing upon getting an unexpected input state. > > IOW, being lazy ;-) > > How does this look? Wow, thanks for the quick response! That does indeed let me checkout the file as expected. I wrote a test (below) to be folded in with your patch, but the test fails because it expects the restored file to be the same as the originally-conflicted file, but the conflict-line labels change from "HEAD" and "master": <<<<<<< HEAD in_topic ======= in_master >>>>>>> master to "ours" and "theirs". (The same thing happens in the 3-way merge case.) If the label change is expected then I can rewrite the test to ignore labels, or to expect "ours" and "theirs", whichever you think is best. If the label change is unexpected, then I guess the test is good :) --Pete -- >8 -- Test 'checkout -m -- path' when path is a 2-way merge Signed-off-by: Pete Harlan <pgit@pcharlan.com> --- t/t2023-checkout-m-twoway.sh | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-) create mode 100755 t/t2023-checkout-m-twoway.sh diff --git a/t/t2023-checkout-m-twoway.sh b/t/t2023-checkout-m-twoway.sh new file mode 100755 index 0000000..5b50360 --- /dev/null +++ b/t/t2023-checkout-m-twoway.sh @@ -0,0 +1,29 @@ +#!/bin/sh + +test_description='checkout -m -- conflicted/path/with/2-way/merge + +Ensures that checkout -m on a resolved file restores the conflicted file +when it conflicted with a 2-way merge.' + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + test_commit initial_commit && + git branch topic && + test_commit added_in_master file.txt in_master && + git checkout topic && + test_commit added_in_topic file.txt in_topic +' + +test_must_fail git merge master + +test_expect_success '-m restores 2-way conflicted+resolved file' ' + cp file.txt file.txt.conflicted && + echo resolved >file.txt && + git add file.txt && + git checkout -m -- file.txt && + test_cmp file.txt.conflicted file.txt +' + +test_done ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-07 7:30 ` Pete Harlan @ 2011-12-08 18:27 ` Junio C Hamano 2011-12-12 1:48 ` Pete Harlan 2011-12-20 20:37 ` [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2011-12-08 18:27 UTC (permalink / raw) To: Pete Harlan; +Cc: git Pete Harlan <pgit@pcharlan.com> writes: > to "ours" and "theirs". (The same thing happens in the 3-way merge > case.) That is entirely expected. "checkout -m" does not know or care _how_ the index came to the conflicted state and reproduces the three-way conflicted state in the file in the working tree solely from the contents of the index which records only what the common thing looked like (stage #1), what one side looked like (stage #2) and what the other side looked like (stage #3) before the mergy operation began, so there is no way for it to say "the other side came from foo/blah branch". There are even cases where the conflict originally came not from any branch (think "am -3"). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-08 18:27 ` Junio C Hamano @ 2011-12-12 1:48 ` Pete Harlan 2011-12-12 5:29 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Pete Harlan @ 2011-12-12 1:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 12/08/2011 10:27 AM, Junio C Hamano wrote: > Pete Harlan <pgit@pcharlan.com> writes: > >> to "ours" and "theirs". (The same thing happens in the 3-way merge >> case.) > > That is entirely expected. "checkout -m" does not know or care _how_ the > index came to the conflicted state and reproduces the three-way conflicted > state in the file in the working tree solely from the contents of the > index which records only what the common thing looked like (stage #1), > what one side looked like (stage #2) and what the other side looked like > (stage #3) before the mergy operation began, so there is no way for it to > say "the other side came from foo/blah branch". There are even cases where > the conflict originally came not from any branch (think "am -3"). Thanks for taking the time to explain this. Here's a test that strips branch info off the conflict lines before verifying checkout -m, and it tests both the 2-way and 3-way cases. The 3-way works before and after your patch; the 2-way fails before your patch but passes after. If the you think the test is worth including feel free to fold it in to your patch. --Pete ------------------- >From a4522e06515231034c1ada65e1e91614a6c4809e Mon Sep 17 00:00:00 2001 From: Pete Harlan <pgit@pcharlan.com> Date: Tue, 6 Dec 2011 23:01:28 -0800 Subject: [PATCH] Test 'checkout -m -- path' Signed-off-by: Pete Harlan <pgit@pcharlan.com> --- t/t2023-checkout-m.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-) create mode 100755 t/t2023-checkout-m.sh diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh new file mode 100755 index 0000000..1a40ce0 --- /dev/null +++ b/t/t2023-checkout-m.sh @@ -0,0 +1,47 @@ +#!/bin/sh + +test_description='checkout -m -- <conflicted path> + +Ensures that checkout -m on a resolved file restores the conflicted file' + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + test_commit both.txt both.txt initial && + git branch topic && + test_commit modified_in_master both.txt in_master && + test_commit added_in_master each.txt in_master && + git checkout topic && + test_commit modified_in_topic both.txt in_topic && + test_commit added_in_topic each.txt in_topic +' + +test_must_fail git merge master + +clean_branchnames () { + # Remove branch names after conflict lines + sed 's/^\([<>]\{5,\}\) .*$/\1/' +} + +test_expect_success '-m restores 2-way conflicted+resolved file' ' + cp each.txt each.txt.conflicted && + echo resolved >each.txt && + git add each.txt && + git checkout -m -- each.txt && + clean_branchnames <each.txt >each.txt.cleaned && + clean_branchnames <each.txt.conflicted >each.txt.conflicted.cleaned && + test_cmp each.txt.conflicted.cleaned each.txt.cleaned +' + +test_expect_success '-m restores 3-way conflicted+resolved file' ' + cp both.txt both.txt.conflicted && + echo resolved >both.txt && + git add both.txt && + git checkout -m -- both.txt && + clean_branchnames <both.txt >both.txt.cleaned && + clean_branchnames <both.txt.conflicted >both.txt.conflicted.cleaned && + test_cmp both.txt.conflicted.cleaned both.txt.cleaned +' + +test_done -- 1.7.8 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-12 1:48 ` Pete Harlan @ 2011-12-12 5:29 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2011-12-12 5:29 UTC (permalink / raw) To: Pete Harlan; +Cc: git Thanks, will queue. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail 2011-12-07 7:30 ` Pete Harlan 2011-12-08 18:27 ` Junio C Hamano @ 2011-12-20 20:37 ` Ævar Arnfjörð Bjarmason 2011-12-20 21:23 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2011-12-20 20:37 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Pete Harlan, Ævar Arnfjörð Bjarmason Change an invocation of test_must_fail() to be inside a test_expect_success() as is our usual pattern. Having it outside caused our tests to fail under prove(1) since we wouldn't print a newline before TAP output: CONFLICT (content): Merge conflict in both.txt # GETTEXT POISON #ok 2 - -m restores 2-way conflicted+resolved file Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t2023-checkout-m.sh | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh index 1a40ce0..7e18985 100755 --- a/t/t2023-checkout-m.sh +++ b/t/t2023-checkout-m.sh @@ -17,7 +17,9 @@ test_expect_success setup ' test_commit added_in_topic each.txt in_topic ' -test_must_fail git merge master +test_expect_success 'git merge master' ' + test_must_fail git merge master +' clean_branchnames () { # Remove branch names after conflict lines -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail 2011-12-20 20:37 ` [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail Ævar Arnfjörð Bjarmason @ 2011-12-20 21:23 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2011-12-20 21:23 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Pete Harlan Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change an invocation of test_must_fail() to be inside a > test_expect_success() as is our usual pattern. Having it outside > caused our tests to fail under prove(1) since we wouldn't print a > newline before TAP output: > > CONFLICT (content): Merge conflict in both.txt > # GETTEXT POISON #ok 2 - -m restores 2-way conflicted+resolved file > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Thanks. > --- > t/t2023-checkout-m.sh | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh > index 1a40ce0..7e18985 100755 > --- a/t/t2023-checkout-m.sh > +++ b/t/t2023-checkout-m.sh > @@ -17,7 +17,9 @@ test_expect_success setup ' > test_commit added_in_topic each.txt in_topic > ' > > -test_must_fail git merge master > +test_expect_success 'git merge master' ' > + test_must_fail git merge master > +' > > clean_branchnames () { > # Remove branch names after conflict lines ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-05 18:58 ` Junio C Hamano 2011-12-07 7:30 ` Pete Harlan @ 2011-12-14 10:19 ` Michael Schubert 2011-12-14 17:54 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Michael Schubert @ 2011-12-14 10:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Pete Harlan, git On 12/05/2011 07:58 PM, Junio C Hamano wrote: > @@ -150,18 +155,27 @@ static int checkout_merged(int pos, struct checkout *state) > int status; > unsigned char sha1[20]; > mmbuffer_t result_buf; > + unsigned char threeway[3][20]; > + unsigned mode; > + > + memset(threeway, 0, sizeof(threeway)); > + while (pos < active_nr) { > + int stage; > + stage = ce_stage(ce); > + if (!stage || strcmp(path, ce->name)) > + break; > + hashcpy(threeway[stage - 1], ce->sha1); > + if (stage == 2) > + mode = create_ce_mode(ce->ce_mode); > + pos++; > + ce = active_cache[pos]; > + } > + if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2])) > + return error(_("path '%s' does not have necessary versions"), path); > > - if (ce_stage(ce) != 1 || > - active_nr <= pos + 2 || > - strcmp(active_cache[pos+1]->name, path) || > - ce_stage(active_cache[pos+1]) != 2 || > - strcmp(active_cache[pos+2]->name, path) || > - ce_stage(active_cache[pos+2]) != 3) > - return error(_("path '%s' does not have all 3 versions"), path); > - > - read_mmblob(&ancestor, active_cache[pos]->sha1); > - read_mmblob(&ours, active_cache[pos+1]->sha1); > - read_mmblob(&theirs, active_cache[pos+2]->sha1); > + read_mmblob(&ancestor, threeway[0]); > + read_mmblob(&ours, threeway[1]); > + read_mmblob(&theirs, threeway[2]); > > /* > * NEEDSWORK: re-create conflicts from merges with > @@ -192,9 +206,7 @@ static int checkout_merged(int pos, struct checkout *state) > if (write_sha1_file(result_buf.ptr, result_buf.size, > blob_type, sha1)) > die(_("Unable to add merge result for '%s'"), path); > - ce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode), > - sha1, > - path, 2, 0); > + ce = make_cache_entry(mode, sha1, path, 2, 0); > if (!ce) > die(_("make_cache_entry failed for path '%s'"), path); > status = checkout_entry(ce, state, NULL); gcc 4.6.2: builtin/checkout.c: In function ‘cmd_checkout’: builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized in this function [-Wuninitialized] builtin/checkout.c:160:11: note: ‘mode’ was declared here ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-14 10:19 ` [bug?] checkout -m doesn't work without a base version Michael Schubert @ 2011-12-14 17:54 ` Junio C Hamano 2011-12-15 4:20 ` Miles Bader 2011-12-15 10:42 ` Andreas Schwab 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2011-12-14 17:54 UTC (permalink / raw) To: Michael Schubert; +Cc: Pete Harlan, git Michael Schubert <mschub@elegosoft.com> writes: >> ... >> + memset(threeway, 0, sizeof(threeway)); >> + while (pos < active_nr) { >> + int stage; >> + stage = ce_stage(ce); >> + if (!stage || strcmp(path, ce->name)) >> + break; >> + hashcpy(threeway[stage - 1], ce->sha1); >> + if (stage == 2) >> + mode = create_ce_mode(ce->ce_mode); >> + pos++; >> + ce = active_cache[pos]; >> + } >> + if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2])) >> + return error(_("path '%s' does not have necessary versions"), path); >> ... >> + ce = make_cache_entry(mode, sha1, path, 2, 0); >> if (!ce) >> die(_("make_cache_entry failed for path '%s'"), path); >> status = checkout_entry(ce, state, NULL); > > gcc 4.6.2: > > builtin/checkout.c: In function ‘cmd_checkout’: > builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized in this function [-Wuninitialized] > builtin/checkout.c:160:11: note: ‘mode’ was declared here Isn't this just your gcc being overly cautious (aka "silly")? The variable "mode" is assigned to when we see an stage #2 entry in the loop, and we should have updated threeway[1] immediately before doing so. If threeway[1] is not updated, we would have already returned before using the variable in make_cache_entry(). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-14 17:54 ` Junio C Hamano @ 2011-12-15 4:20 ` Miles Bader 2011-12-15 10:11 ` Michael Schubert 2011-12-15 10:42 ` Andreas Schwab 1 sibling, 1 reply; 15+ messages in thread From: Miles Bader @ 2011-12-15 4:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Michael Schubert, Pete Harlan, git Junio C Hamano <gitster@pobox.com> writes: >> builtin/checkout.c: In function ‘cmd_checkout’: >> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized >> in this function [-Wuninitialized] >> builtin/checkout.c:160:11: note: ‘mode’ was declared here > > Isn't this just your gcc being overly cautious (aka "silly")? > > The variable "mode" is assigned to when we see an stage #2 entry in the > loop, and we should have updated threeway[1] immediately before doing so. > If threeway[1] is not updated, we would have already returned before using > the variable in make_cache_entry(). Maybe that is actually guaranteed (I dunno), but it's certainly not obvious from the code here, even to a human... any guarantee would have to come from external invariants that the compiler doesn't know about. Given that, I think it's a fair warning, certainly not "silly." This aspect of the code doesn't seem easy to understand... -Miles -- `To alcohol! The cause of, and solution to, all of life's problems' --Homer J. Simpson ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-15 4:20 ` Miles Bader @ 2011-12-15 10:11 ` Michael Schubert 0 siblings, 0 replies; 15+ messages in thread From: Michael Schubert @ 2011-12-15 10:11 UTC (permalink / raw) To: Miles Bader; +Cc: Junio C Hamano, Pete Harlan, git On 12/15/2011 05:20 AM, Miles Bader wrote: > Junio C Hamano <gitster@pobox.com> writes: >>> builtin/checkout.c: In function ‘cmd_checkout’: >>> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized >>> in this function [-Wuninitialized] >>> builtin/checkout.c:160:11: note: ‘mode’ was declared here >> >> Isn't this just your gcc being overly cautious (aka "silly")? >> >> The variable "mode" is assigned to when we see an stage #2 entry in the >> loop, and we should have updated threeway[1] immediately before doing so. >> If threeway[1] is not updated, we would have already returned before using >> the variable in make_cache_entry(). > > Maybe that is actually guaranteed (I dunno), but it's certainly not > obvious from the code here, even to a human... any guarantee would > have to come from external invariants that the compiler doesn't know > about. > > Given that, I think it's a fair warning, certainly not "silly." This > aspect of the code doesn't seem easy to understand... Silly or not: That's what gcc 4.6.x is warning about with -Wuninitialized (-Wall) set - I didn't set any additional options, just plain `make all`. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-14 17:54 ` Junio C Hamano 2011-12-15 4:20 ` Miles Bader @ 2011-12-15 10:42 ` Andreas Schwab 2011-12-15 17:36 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2011-12-15 10:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: Michael Schubert, Pete Harlan, git Junio C Hamano <gitster@pobox.com> writes: > The variable "mode" is assigned to when we see an stage #2 entry in the > loop, and we should have updated threeway[1] immediately before doing so. > If threeway[1] is not updated, we would have already returned before using > the variable in make_cache_entry(). How can you be sure that ce_stage(ce) ever returns 2? Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-15 10:42 ` Andreas Schwab @ 2011-12-15 17:36 ` Junio C Hamano 2011-12-16 22:38 ` Ramsay Jones 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2011-12-15 17:36 UTC (permalink / raw) To: Andreas Schwab; +Cc: Michael Schubert, Pete Harlan, git Andreas Schwab <schwab@linux-m68k.org> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> The variable "mode" is assigned to when we see an stage #2 entry in the >> loop, and we should have updated threeway[1] immediately before doing so. >> If threeway[1] is not updated, we would have already returned before using >> the variable in make_cache_entry(). > > How can you be sure that ce_stage(ce) ever returns 2? You cannot and and there are cases where you exit the loop without finding a stage #2 entry. But in that case threeway[1] stays 0{40} and control is returned to the caller without ever getting to the place where the variable is used. You could do the usual "unnecessary initialization" trick, though. builtin/checkout.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 31aa248..064e7a1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -152,7 +152,7 @@ static int checkout_merged(int pos, struct checkout *state) unsigned char sha1[20]; mmbuffer_t result_buf; unsigned char threeway[3][20]; - unsigned mode; + unsigned mode = 0; memset(threeway, 0, sizeof(threeway)); while (pos < active_nr) { ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [bug?] checkout -m doesn't work without a base version 2011-12-15 17:36 ` Junio C Hamano @ 2011-12-16 22:38 ` Ramsay Jones 0 siblings, 0 replies; 15+ messages in thread From: Ramsay Jones @ 2011-12-16 22:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Andreas Schwab, Michael Schubert, Pete Harlan, git Junio C Hamano wrote: > You could do the usual "unnecessary initialization" trick, though. Yes, I wrote exactly this patch some days ago (when it first appeared in pu), but haven't found the time to send it ... :-D ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-12-20 21:24 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-04 22:31 [bug?] checkout -m doesn't work without a base version Pete Harlan 2011-12-05 18:58 ` Junio C Hamano 2011-12-07 7:30 ` Pete Harlan 2011-12-08 18:27 ` Junio C Hamano 2011-12-12 1:48 ` Pete Harlan 2011-12-12 5:29 ` Junio C Hamano 2011-12-20 20:37 ` [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail Ævar Arnfjörð Bjarmason 2011-12-20 21:23 ` Junio C Hamano 2011-12-14 10:19 ` [bug?] checkout -m doesn't work without a base version Michael Schubert 2011-12-14 17:54 ` Junio C Hamano 2011-12-15 4:20 ` Miles Bader 2011-12-15 10:11 ` Michael Schubert 2011-12-15 10:42 ` Andreas Schwab 2011-12-15 17:36 ` Junio C Hamano 2011-12-16 22:38 ` Ramsay Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).