On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Leave the setting of the commitable flag in show_merge_in_progress. If > >> a check for merged commits is moved to the collect phase then other > >> --dry-run tests fail. > > "Some tests fail" is not a good explanation why you keep the setting > of commitable bit in the "show" codepath. The test coverage may not > be thorough, and the tests that fail might be expecting wrong things. > I didn't figure it was, but I haven't yet figured out how to explain what what I saw last evening. I wanted to send something out to get feedback from someone who may know the code far better than I. > The change in this patch makes the internal "diff-index" invocation > responsible for setting the commitable bit. This is better for non > merge commits than the current code because the current code only > sets the commitable bit when longstatus is asked for (and the code > to show the longstatus detects changes to be committed), so the > short-form will not have chance to set the bit, but the internal > "diff-index" is what determines if the resulting commit would have > difference relative to the HEAD, so it is a better place to make > that decision. > > Merge commits need to be allowed even when the resulting commit > records a tree that is identical to that of the current HEAD > (i.e. we merged a side branch, but we already had all the necessary > changes done on it). So it is insufficient to let "diff-index" > invocation to set the committable bit. Is that why "other --dry-run > tests fail"? What I am getting at is to have a reasonable "because > ..." to explain why "other --dry-run tests fail" after it, to make > it clear to the readers that the failure is not because tests are > checking wrong things but because a specific condition thatwt_status_collect(), isYes > expeted from the code gets violated if we change the code in > show_merge_in_progress() function. Agreed. I'm just green at this code base, and so don't quite know what I should see as opposed to what I do see. > > That brings us to another point. Is there a case where we want to > see s->commitable bit set correctly but we do not make any call to > show_merge_in_progress() function? It is conceivable to have a new > "git commit --dry-run --quiet [[--] <pathspec>]" mode that is > totally silent but reports if what we have is committable with the > exit status, and for that we would not want to call any show_* > functions. That leads me to suspect that ideally we would want to > see wt_status_collect_changes_index() to be the one that is setting > the commitable bit. Or even its caller wt_status_collect(), which > would give us a better chance of being correct even during the > initial commit. For the "during merge" case, we would need to say > something like > > if (state->merge_in_progress && !has_unmerged(s)) > s->commitable = 1; > I placed the following in wt_status_collect() last evening, and received errors from three early tests in 7501-commit.sh. Thanks for a hint. if (!has_unmerged(s)) s->commitable = 1; Maybe the missing first condition was what I needed. Which leads me to asking: Do you want a preparatory patch moving has_unmerged() further up in the file before adding a reference to has_unmerged() in wt_status_collect(). > but the "state" thing is passed around only among the "print/show" > level of functions in the current code. We might need to merge that > into the wt_status structure to pass it down to the "collect" phase > at the lower level before/while doing so. I dunno. Would you explain what you are thinking for passing moving the "stat" think into wt_status. I haven't figured out how the "collect" sequence, relates to the "print/show" squence. > > Thanks for working on this. I decided sometime back to work on something I didn't know using a process I don't normally use to broaden my experience. I'm enjoying this and hope you don't mind lots of questions from someone new. sps
