Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data

[Stefan Beller]

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

[Stefan Beller]

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

[Jeff Hostetler]

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

[Junio C Hamano]

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

[Jeff King]

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

[Junio C Hamano]

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

[Jeff King]

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

[PATCH v4 5/8] status: print per-file porcelain v2 status data

[Jeff Hostetler]

From: Jeff Hostetler 

Print per-file information in porcelain v2 format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 wt-status.c | 283 +++-
 1 file changed, 282 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 15d3349..46061d4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1813,6 +1813,287 @@ static void wt_porcelain_print(struct wt_status *s)
wt_shortstatus_print(s);
 }
 
+/*
+ * Convert various submodule status values into a
+ * fixed-length string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+   struct wt_status_change_data *d,
+   char sub[5])
+{
+   if (S_ISGITLINK(d->mode_head) ||
+   S_ISGITLINK(d->mode_index) ||
+   S_ISGITLINK(d->mode_worktree)) {
+   sub[0] = 'S';
+   sub[1] = d->new_submodule_commits ? 'C' : '.';
+   sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' 
: '.';
+   sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' 
: '.';
+   } else {
+   sub[0] = 'N';
+   sub[1] = '.';
+   sub[2] = '.';
+   sub[3] = '.';
+   }
+   sub[4] = 0;
+}
+
+/*
+ * Fix-up changed entries before we print them.
+ */
+static void wt_porcelain_v2_fix_up_changed(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+
+   if (!d->index_status) {
+   /*
+* This entry is unchanged in the index (relative to the head).
+* Therefore, the collect_updated_cb was never called for this
+* entry (during the head-vs-index scan) and so the head column
+* fields were never set.
+*
+* We must have data for the index column (from the
+* index-vs-worktree scan (otherwise, this entry should not be
+* in the list of changes)).
+*
+* Copy index column fields to the head column, so that our
+* output looks complete.
+*/
+   assert(d->mode_head == 0);
+   d->mode_head = d->mode_index;
+   oidcpy(>oid_head, >oid_index);
+   }
+
+   if (!d->worktree_status) {
+   /*
+* This entry is unchanged in the worktree (relative to the 
index).
+* Therefore, the collect_changed_cb was never called for this 
entry
+* (during the index-vs-worktree scan) and so the worktree 
column
+* fields were never set.
+*
+* We must have data for the index column (from the 
head-vs-index
+* scan).
+*
+* Copy the index column fields to the worktree column so that
+* our output looks complete.
+*
+* Note that we only have a mode field in the worktree column
+* because the scan code tries really hard to not have to 
compute it.
+*/
+   assert(d->mode_worktree == 0);
+   d->mode_worktree = d->mode_index;
+   }
+}
+
+/*
+ * Print porcelain v2 info for tracked entries with changes.
+ */
+static void wt_porcelain_v2_print_changed_entry(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+   struct strbuf buf_current = STRBUF_INIT;
+   struct strbuf buf_src = STRBUF_INIT;
+   const char *path_current = NULL;
+   const char *path_src = NULL;
+   char key[3];
+   char submodule_token[5];
+   char sep_char, eol_char;
+
+   wt_porcelain_v2_fix_up_changed(it, s);
+   wt_porcelain_v2_submodule_state(d, submodule_token);
+
+   key[0] = d->index_status ? d->index_status : '.';
+   key[1] = d->worktree_status ? d->worktree_status : '.';
+   key[2] = 0;
+
+   if (s->null_termination) {
+   /*
+* In -z mode, we DO NOT C-Quote pathnames.  Current path is 
ALWAYS first.
+* A single NUL character separates them.
+*/
+   sep_char = '\0';
+   eol_char = '\0';
+   path_current = it->string;
+   path_src = d->head_path;
+   } else {
+   /*
+* Path(s) are C-Quoted if necessary. Current path is ALWAYS 
first.
+* The source path is only present when necessary.
+* A single TAB separates them (because paths can contain spaces
+* which are not escaped and C-Quoting does escape TAB 
characters).
+*/
+   sep_char = '\t';
+   eol_char = '\n';
+   path_current =