Re: [PATCH v5 4/9] status: collect per-file data for --porcelain=v2

[Junio C Hamano]

Eric Wong  writes:

>> > Not a big deal (no need to resend for this one alone), but let's
>> > make the above properly formatted, i.e.
>> > 
>> >if (ce_stage(ce)) {
>> >...
>> >} else {
>> >...
>> >}
>> 
>> Do I understand correctly that your objections is against having the curly
>> brace before the "else" on its own line?
>> 
>> If so, when did our coding style change? I vividly remember that we
>> strongly favored putting the "else" on a new line after a closing brace,
>> to make diffs nicer in case the braces were removed or added.
>
> AFAIK, Linux kernel CodingStyle has always been what Junio
> suggested (just w/o the trailing spaces :),
> and we inherit from that.

What Eric said.

While I admit that I sometimes break line between "}" and "else {"
by inertia when I am being careless and get caught by checkpatch.pl
myself, I do not recall trying to justify it; you probably may
remember somebody else saying that, but I don't recall anybody
making that argument on the list, and more importantly I don't
recall us making that our style based on that argument.

The only two and half kinds of warnings we knowingly ignore from
scripts/checkpatch.pl in the Linux kernel source tree are:

 * "Avoid typedefs."  We do avoid making graduitous use of typedef to
   hide a structure behind a type and pretty much limit ourselves to
   use it for (callback) function types, though.

 * "We've never heard of Helped-by/Mentored-by footers"; well,
   kernel folks may not, but we have ;-)

 * "No spaces for bitfield width".  This may not be justifyable, but
   the majority of our bitfield widths are defined in the way not
   blessed by checkpatch.pl checker.
--
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 v5 4/9] status: collect per-file data for --porcelain=v2

[Eric Wong]

Johannes Schindelin  wrote:
> On Fri, 5 Aug 2016, Junio C Hamano wrote:
> > Jeff Hostetler  writes:
> > >   }
> > > - else
> > > + else {
> > >   d->index_status = DIFF_STATUS_ADDED;
> > > + /* Leave {mode,oid}_head zero for adds. */
> > > + d->mode_index = ce->ce_mode;
> > > + hashcpy(d->oid_index.hash, ce->sha1);
> > > + }
> > 
> > Not a big deal (no need to resend for this one alone), but let's
> > make the above properly formatted, i.e.
> > 
> > if (ce_stage(ce)) {
> > ...
> > } else {
> > ...
> > }
> 
> Do I understand correctly that your objections is against having the curly
> brace before the "else" on its own line?
> 
> If so, when did our coding style change? I vividly remember that we
> strongly favored putting the "else" on a new line after a closing brace,
> to make diffs nicer in case the braces were removed or added.

AFAIK, Linux kernel CodingStyle has always been what Junio
suggested (just w/o the trailing spaces :),
and we inherit from that.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/Documentation/CodingStyle

> BTW your suggestion has 24 extra spaces after the final closing brace ;-)
--
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 v5 4/9] status: collect per-file data for --porcelain=v2

[Johannes Schindelin]

Hi Junio,

On Fri, 5 Aug 2016, Junio C Hamano wrote:

> Jeff Hostetler  writes:
> 
> > if (ce_stage(ce)) {
> > d->index_status = DIFF_STATUS_UNMERGED;
> > d->stagemask |= (1 << (ce_stage(ce) - 1));
> > +   /*
> > +* Don't bother setting {mode,oid}_{head,index} since 
> > the print
> > +* code will output the stage values directly and not 
> > use the
> > +* values in these fields.
> > +*/
> > }
> > -   else
> > +   else {
> > d->index_status = DIFF_STATUS_ADDED;
> > +   /* Leave {mode,oid}_head zero for adds. */
> > +   d->mode_index = ce->ce_mode;
> > +   hashcpy(d->oid_index.hash, ce->sha1);
> > +   }
> 
> Not a big deal (no need to resend for this one alone), but let's
> make the above properly formatted, i.e.
> 
>   if (ce_stage(ce)) {
>   ...
>   } else {
>   ...
>   }

Do I understand correctly that your objections is against having the curly
brace before the "else" on its own line?

If so, when did our coding style change? I vividly remember that we
strongly favored putting the "else" on a new line after a closing brace,
to make diffs nicer in case the braces were removed or added.

BTW your suggestion has 24 extra spaces after the final closing brace ;-)

Ciao,
Dscho
--
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 v5 4/9] status: collect per-file data for --porcelain=v2

[Junio C Hamano]

Jeff Hostetler  writes:

>   if (ce_stage(ce)) {
>   d->index_status = DIFF_STATUS_UNMERGED;
>   d->stagemask |= (1 << (ce_stage(ce) - 1));
> + /*
> +  * Don't bother setting {mode,oid}_{head,index} since 
> the print
> +  * code will output the stage values directly and not 
> use the
> +  * values in these fields.
> +  */
>   }
> - else
> + else {
>   d->index_status = DIFF_STATUS_ADDED;
> + /* Leave {mode,oid}_head zero for adds. */
> + d->mode_index = ce->ce_mode;
> + hashcpy(d->oid_index.hash, ce->sha1);
> + }

Not a big deal (no need to resend for this one alone), but let's
make the above properly formatted, i.e.

if (ce_stage(ce)) {
...
} else {
...
}

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

[PATCH v5 4/9] status: collect per-file data for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

Collect extra per-file data for porcelain V2 format.

The output of `git status --porcelain` leaves out many
details about the current status that clients might like
to have.  This can force them to be less efficient as they
may need to launch secondary commands (and try to match
the logic within git) to accumulate this extra information.
For example, a GUI IDE might want the file mode to display
the correct icon for a changed item (without having to stat
it afterwards).

Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  3 +++
 wt-status.c  | 63 +++-
 wt-status.h  |  4 
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 185ac35..3d222d3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, 
const char *arg, int un
*value = STATUS_FORMAT_PORCELAIN;
else if (!strcmp(arg, "v1") || !strcmp(arg, "1"))
*value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v2") || !strcmp(arg, "2"))
+   *value = STATUS_FORMAT_PORCELAIN_V2;
else
die("unsupported porcelain version '%s'", arg);
 
@@ -1104,6 +1106,7 @@ static struct status_deferred_config {
 static void finalize_deferred_config(struct wt_status *s)
 {
int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+  status_format != STATUS_FORMAT_PORCELAIN_V2 
&&
   !s->null_termination);
 
if (s->null_termination) {
diff --git a/wt-status.c b/wt-status.c
index a9031e4..ca8023e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -434,6 +434,31 @@ static void wt_status_collect_changed_cb(struct 
diff_queue_struct *q,
if (S_ISGITLINK(p->two->mode))
d->new_submodule_commits = !!oidcmp(>one->oid,
>two->oid);
+
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   die("BUG: worktree status add???");
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->mode_index = p->one->mode;
+   oidcpy(>oid_index, >one->oid);
+   /* mode_worktree is zero for a delete. */
+   break;
+
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_UNMERGED:
+   d->mode_index = p->one->mode;
+   d->mode_worktree = p->two->mode;
+   oidcpy(>oid_index, >one->oid);
+   break;
+
+   case DIFF_STATUS_UNKNOWN:
+   die("BUG: worktree status unknown???");
+   break;
+   }
+
}
 }
 
@@ -479,12 +504,36 @@ static void wt_status_collect_updated_cb(struct 
diff_queue_struct *q,
if (!d->index_status)
d->index_status = p->status;
switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   /* Leave {mode,oid}_head zero for an add. */
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_index, >two->oid);
+   break;
+   case DIFF_STATUS_DELETED:
+   d->mode_head = p->one->mode;
+   oidcpy(>oid_head, >one->oid);
+   /* Leave {mode,oid}_index zero for a delete. */
+   break;
+
case DIFF_STATUS_COPIED:
case DIFF_STATUS_RENAMED:
d->head_path = xstrdup(p->one->path);
+   d->score = p->score * 100 / MAX_SCORE;
+   /* fallthru */
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   d->mode_head = p->one->mode;
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_head, >one->oid);
+   oidcpy(>oid_index, >two->oid);
break;
case DIFF_STATUS_UNMERGED:
d->stagemask = unmerged_mask(p->two->path);
+   /*
+* Don't bother setting {mode,oid}_{head,index} since 
the print
+* code will output the stage values directly and not 
use the
+* values in these fields.
+*/
break;
}
}
@@ -565,9 +614,18 @@ static void wt_status_collect_changes_initial(struct 
wt_status *s)
if (ce_stage(ce)) {
d->index_status = DIFF_STATUS_UNMERGED;