Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > On Fri, Aug 31 2018, Stephen P. Smith wrote: > >> In an update to fix a bug with "commit --dry-run" it was found that >> the commitable flag was broken. The update was, at the time, >> accepted as it was better than the previous version. > > What update is this? I.e. git.git commit id? See the "or this invocation > of `git show`" part of SubmittingPatches for how to quote it in the > commit message. > >> Since the set of the flag had been done in wt_longstatus_print_updated, >> set the flag in wt_status_collect_updated_cb. >> >> Set the commitable flag in wt_status_collect_changes_initial to keep >> from introducing a rebase regression. >> >> 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. 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 that is expeted from the code gets violated if we change the code in show_merge_in_progress() function. 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; 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. Thanks for working on this.
