On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote: > > --- > > 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. > I understand. > 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. > Makes sense. > 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. Tonight I started work on a patch to remove the two locations where committable was set in the *print* and *show* functions. I believe that what you mean by the "collect" phase is the set of functions that are in wt_status.c and have collect in the function name. > > 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. > I'm happy with moveing the patch from pu (where it is now) to next. I've re-started my work on this. > 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
