On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote: > "Stephen P. Smith" <ischis2@xxxxxxx> writes: > > > The 'commit --dry-run' and commit return values differed if a > > conflicted merge had been resolved and the commit would be the same as > > the parent. > > > > Update show_merge_in_progress to set the commitable bit if conflicts > > have been resolved and a merge is in progress. > > > > Signed-off-by: Stephen P. Smith <ischis2@xxxxxxx> > > --- > > I think I mislead you into a slightly wrong direction. While the > single liner does improve the situation, I think this is merely a > band-aid upon closer inspection. For example, if you changed your > "commit --dry-run" in your test to "commit --dry-run --short", you > would notice that the test would fail. > > In fact, "commit --dry-run" is already broken without this "a merge > ends up in a no-op" corner case. The management of s->commitable > flag and dry_run_commit() that uses it are unfortunately more broken > than I originally thought. > > If we check for places where s->committable is set, we notice that > there is only one location: wt_status_print_updated(). This function > runs an equivalent of "diff-index --cached" and flips s->committable > on when it sees any difference. > > This function is only called from wt_status_print(), which in turn > is only called from run_status() in commit.c when the status format > is unspecified or set to STATUS_FORMAT_LONG. > > So if you do this: > > $ git reset --hard HEAD > $ >a-new-file && git add a-new-file > $ git commit --dry-run --short; echo $? > > you'd get "No, there is nothing interesting to commit", which is > clearly bogus. > > I said s->committable is flipped on only when there is any change in > "diff-index --cached". There is nothing that flips it off, by > noticing that there are unmerged paths, for example. This is > another brokenness around "git commit --dry-run". Imagine that you > are in a middle of a conflicted cherry-pick. You did "git add" on a > resolved path and you still have another path whose conflict has not > been resolved. If you run a "git commit --dry-run", you will hear > "Yes, you can make a meaningful commit", which again is clearly > bogus. > > These things need to be eventually fixed, and I think the fix will > involve revamping how we compute s->committable flag. Most likely, > we won't be doing any of that in any wt_status function whose name > has "print" or "show" in it. As the original designer of the wt_* > suite (before these multiple output formats are added), I would say > everything should happen inside the "collect" phase, if we wanted to > make s->committable bit usable. > > So in the sense, eventually the code updated by this patch will have > to be discarded when we fix the "commit --dry-run" in the right way, > but in the meantime, the patch does not make things worse, so let's > think about queuing it as-is for now as a stop-gap measure. > Hmmmm.... that makes sense. Let me fix the commit message per what I told Philip and then I will start working on a rework of the commitable bit with this in mind as a follow on patch. > Thanks. > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html