Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:43 PM, Stefan Beller wrote: > cov-build --dir cov-int make For this you need to download their analytic tool and put it in your $PATH. Let me see if I need to update the tool so it enables finding more potential issues. So that was an initial hurdle. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:14 PM, Jeff King wrote: > > Unfortunately it is hard for me to test a one-off, as running it locally > is a complete pain. Stefan set it up long ago to pull "pu" and email out > the results from their central servers, so I just scan those emails for > things that look like real issues. > I am glad this provides actual value. :) (I tend to ignore the emails nowadays as I am focusing on other stuff. I'll get back to down the number issues eventually, I promise ;) Running one offs is "relatively" easy. I did that for other projects once upon a time. Here is my script to run the build: #!/bin/bash . .profile cd coverity/git git clean -dfx git fetch --all git checkout origin/pu git am ../git_strbuf_stop_out_of_bounds_coverity.patch descrip="scripted upload scanning github.com/gitster/git pu" name=$(git describe) cov-build --dir cov-int make tar czvf git-${name}.tgz cov-int curl --form project=git \ --form token= \ --form email= \ --form file=@git-${name}.tgz \ --form version="${name}" \ --form description="${descrip}" \ https://scan.coverity.com/builds?project=git git clean -dfx You can get a token if you look around, e.g. at https://scan.coverity.com/projects/git/builds/new and the email address is the one you signed up with coverity (or the one from Github, if signed up via Github) I am currently applying one patch on top of pu, (id: 1434536209-31350-1-git-send-email-pclo...@gmail.com I can resend if you don't have that.) I am running this script as a cron job, but it can be run "as is" as well, you'd just need to toss in the one-off test patch. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On 08/05/2016 05:02 PM, Jeff King wrote: On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote: +static void wt_porcelain_v2_print_unmerged_entry( + struct string_list_item *it, + struct wt_status *s) +{ + struct wt_status_change_data *d = it->util; + const struct cache_entry *ce; + struct strbuf buf_current = STRBUF_INIT; + const char *path_current = NULL; + int pos, stage, sum; + struct { + int mode; + struct object_id oid; + } stages[3]; + char *key; [...] + switch (d->stagemask) { + case 1: key = "DD"; break; /* both deleted */ + case 2: key = "AU"; break; /* added by us */ + case 3: key = "UD"; break; /* deleted by them */ + case 4: key = "UA"; break; /* added by them */ + case 5: key = "DU"; break; /* deleted by us */ + case 6: key = "AA"; break; /* both added */ + case 7: key = "UU"; break; /* both modified */ + } [...] + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c", + unmerged_prefix, key, submodule_token, Coverity complains that "key" can be uninitialized here. I think it's wrong, and just doesn't know that d->stagemask is constrained to 1-7. But perhaps it is worth adding a: default: die("BUG: unhandled unmerged status %x", d->stagemask); to the end of the switch. That would shut up Coverity, and give us a better indication if our constraint is violated. -Peff got it. thanks! Jeff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:14 PM, Jeff King wrote: > > Unfortunately it is hard for me to test a one-off, as running it locally > is a complete pain. Stefan set it up long ago to pull "pu" and email out > the results from their central servers, so I just scan those emails for > things that look like real issues. Ah, running check on 'pu' is an excellent way to catch issues early, before they hit 'next'. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 05, 2016 at 02:09:48PM -0700, Junio C Hamano wrote: > On Fri, Aug 5, 2016 at 2:02 PM, Jeff King wrote: > > On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote: > >> + switch (d->stagemask) { > >> + case 1: key = "DD"; break; /* both deleted */ > >> + ... > >> + case 7: key = "UU"; break; /* both modified */ > >> + } > >> [...] > >> + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c", > >> + unmerged_prefix, key, submodule_token, > > > > Coverity complains that "key" can be uninitialized here. I think it's > > wrong, and just doesn't know that d->stagemask is constrained to 1-7. > > > > But perhaps it is worth adding a: > > > > default: > > die("BUG: unhandled unmerged status %x", d->stagemask); > > > > to the end of the switch. That would shut up Coverity, and give us a > > better indication if our constraint is violated. > > This is pure curiosity but I wonder if Coverity shuts up if we > instead switched on (d->stagemask & 07). Your "default: BUG" > suggestion is what we should use as a real fix, of course. I suspect yes. It's pretty clever about analyzing the flow and placing constraints on values (though in this case "& 07" includes "0", which is not handled in the switch). Unfortunately it is hard for me to test a one-off, as running it locally is a complete pain. Stefan set it up long ago to pull "pu" and email out the results from their central servers, so I just scan those emails for things that look like real issues. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:02 PM, Jeff King wrote: > On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote: >> + switch (d->stagemask) { >> + case 1: key = "DD"; break; /* both deleted */ >> + ... >> + case 7: key = "UU"; break; /* both modified */ >> + } >> [...] >> + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c", >> + unmerged_prefix, key, submodule_token, > > Coverity complains that "key" can be uninitialized here. I think it's > wrong, and just doesn't know that d->stagemask is constrained to 1-7. > > But perhaps it is worth adding a: > > default: > die("BUG: unhandled unmerged status %x", d->stagemask); > > to the end of the switch. That would shut up Coverity, and give us a > better indication if our constraint is violated. This is pure curiosity but I wonder if Coverity shuts up if we instead switched on (d->stagemask & 07). Your "default: BUG" suggestion is what we should use as a real fix, of course. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote: > +static void wt_porcelain_v2_print_unmerged_entry( > + struct string_list_item *it, > + struct wt_status *s) > +{ > + struct wt_status_change_data *d = it->util; > + const struct cache_entry *ce; > + struct strbuf buf_current = STRBUF_INIT; > + const char *path_current = NULL; > + int pos, stage, sum; > + struct { > + int mode; > + struct object_id oid; > + } stages[3]; > + char *key; > [...] > + switch (d->stagemask) { > + case 1: key = "DD"; break; /* both deleted */ > + case 2: key = "AU"; break; /* added by us */ > + case 3: key = "UD"; break; /* deleted by them */ > + case 4: key = "UA"; break; /* added by them */ > + case 5: key = "DU"; break; /* deleted by us */ > + case 6: key = "AA"; break; /* both added */ > + case 7: key = "UU"; break; /* both modified */ > + } > [...] > + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c", > + unmerged_prefix, key, submodule_token, Coverity complains that "key" can be uninitialized here. I think it's wrong, and just doesn't know that d->stagemask is constrained to 1-7. But perhaps it is worth adding a: default: die("BUG: unhandled unmerged status %x", d->stagemask); to the end of the switch. That would shut up Coverity, and give us a better indication if our constraint is violated. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html