Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler  writes:

>>> +   case DIFF_STATUS_DELETED:
>>> +   d->porcelain_v2.mode_index = p->one->mode;
>>> +   hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
>>> +   break;
>>> +
>>> +   case DIFF_STATUS_COPIED:
>>> +   case DIFF_STATUS_MODIFIED:
>>> +   case DIFF_STATUS_RENAMED:
>>
>> Can RENAMED or COPIED happen here?
>
> If we don't report renamed or copied for unstaged changes, then no.

The question was "do we report such cases"?

>>> +   if (!d->index_status) {
>>> +   if (d->worktree_status == DIFF_STATUS_MODIFIED ||
>>> +   d->worktree_status == DIFF_STATUS_DELETED) {
>>> +   /* X=' ' Y=[MD]
>>> +* The item did not change in head-vs-index scan so the 
>>> head
>>> +* column was never set. (The index column was set 
>>> during the
>>> +* index-vs-worktree scan.)
>>> +* Force set the head column to make the output 
>>> complete.
>>> +*/
>>> +   d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
>>> +   hashcpy(d->porcelain_v2.sha1_head, 
>>> d->porcelain_v2.sha1_index);
>>> +   }
>>
>> Can there be "else" clause for this inner "if"?  When
>> d->index_status is not set, but worktree_status is not one of these
>> two, what does it indicate?  The same for the next one.
>
> For a normal entry (not unmerged) when X is ' ', Y can only
> be 'M' or 'D'.  The only other value for Y is ' ', but then if
> both were ' ' the entry would not be in the list.  So I think
> we're OK, but I'll document that here.  And below.

Good.

>>> +   memset(stages, 0, sizeof(stages));
>>> +   pos = cache_name_pos(it->string, strlen(it->string));
>>> +   assert(pos < 0);
>>> +   pos = -pos-1;
>>> +   while (pos < active_nr) {
>>> +   ce = active_cache[pos++];
>>> +   stage = ce_stage(ce);
>>> +   if (strcmp(ce->name, it->string) || !stage)
>>> +   break;
>>> +   stages[stage - 1].mode = ce->ce_mode;
>>> +   hashcpy(stages[stage - 1].sha1, ce->sha1);
>>> +   }
>>
>> You did assert(pos < 0) to make sure that the path the caller told
>> you is unmerged indeed is unmerged, which is a very good check.  In
>> the same spirit, you would want to make sure that you got at least
>> one result from the above loop, to diagnose a bug where the path did
>> not even exist at all in the index.
>
> Would that be possible for a conflict/unmerged entry?

It is possible to exactly the same degree as it is possible you
would get !(pos < 0) answer from cache_name_pos() here, I would
think.  The assert() you have up above is protecting us from either
a broken caller to this function that gives an it that points at a
merged path (in which case the assert is violated) or a breakage in
cache_name_pos().  Making sure the loop finds at least one unmerged
entry protects us from similar kind of breakage that violates the
expectation this code has from the other parts of the code.


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 v1 3/6] Per-file output for Porcelain Status V2

[Jeff Hostetler]

On 07/20/2016 05:31 PM, Junio C Hamano wrote:


The code seems to assume that d->porcelain_v2.* fields are
initialized earlier in the callchain to reasonable values
(e.g. STATUS_ADDED case does not clear .mode_head to "missing"); I
am not sure if that is easier to read or fill in all the values
explicitly in this function.


The structure is initially zeroed when it allocated.
I'm only setting values for the side the we know are
actually defined (such as in an ADD we know there is
no head values). I'll look at making this more clear.


+}
+
+/* Copy info for both sides of an index-vs-worktree change
+ * into the very verbose porcelain data.
+ */
+static void porcelain_v2_changed_entry(
+   struct wt_status_change_data *d,
+   const struct diff_filepair *p)
+{
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   d->porcelain_v2.mode_worktree = p->two->mode;
+   /* don't bother with worktree sha, since it is almost always 
zero. */
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->porcelain_v2.mode_index = p->one->mode;
+   hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
+   break;
+
+   case DIFF_STATUS_COPIED:
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_RENAMED:


Can RENAMED or COPIED happen here?


If we don't report renamed or copied for unstaged changes, then no.



The code would be simpler and we'll catch bugs more uniformly if
"collect" phase is made oblivious to s->status_format, I would
suspect, by making unconditional call to porcelain_v2_*_entry()
functions from all of these "collect" functions.


ok. will do.




+/* Convert various submodule status values into a
+ * string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+   struct wt_status_change_data *d,
+   char sub[5])
+{
+   int k = 0;
+
+   if (S_ISGITLINK(d->porcelain_v2.mode_head) ||
+   S_ISGITLINK(d->porcelain_v2.mode_index) ||
+   S_ISGITLINK(d->porcelain_v2.mode_worktree)) {
+   /* We have a submodule */
+   sub[k++] = 'S';
+
+   /* Sub-flags for each type of dirt */
+   if (d->new_submodule_commits)
+   sub[k++] = 'C';
+   if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+   sub[k++] = 'M';
+   if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+   sub[k++] = 'U';
+   } else {
+   /* Not a submodule */
+   sub[k++] = 'N';
+   }
+
+   sub[k] = 0;
+}


The above seems to do "up to 5"; if these come early in the output,
it may instead be easier to read if "fixed 5 columns, many of them
may be SP, and some may be C or M or U", which would align on fixed
column terminal people use for coding.


I debated fixed 4 columns vs a variable width token.
The latter seemed easier to parse, but does look more ugly
on the terminal.  I'll revisit this.  Perhaps make it fixed
4 columns with a '.' instead of SPACE, so both parsing
techniques work.




+/* Various fix-up steps before we start printing an item.
+ */
+static void wt_porcelain_v2_fix_up_status(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+
+   if (!d->index_status) {
+   if (d->worktree_status == DIFF_STATUS_MODIFIED ||
+   d->worktree_status == DIFF_STATUS_DELETED) {
+   /* X=' ' Y=[MD]
+* The item did not change in head-vs-index scan so the 
head
+* column was never set. (The index column was set 
during the
+* index-vs-worktree scan.)
+* Force set the head column to make the output 
complete.
+*/
+   d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
+   hashcpy(d->porcelain_v2.sha1_head, 
d->porcelain_v2.sha1_index);
+   }


Can there be "else" clause for this inner "if"?  When
d->index_status is not set, but worktree_status is not one of these
two, what does it indicate?  The same for the next one.


For a normal entry (not unmerged) when X is ' ', Y can only
be 'M' or 'D'.  The only other value for Y is ' ', but then if
both were ' ' the entry would not be in the list.  So I think
we're OK, but I'll document that here.  And below.



+/*
+ * Define a single format for tracked entries. This includes:
+ * normal changes, rename changes, and unmerged changes.
+ *
+ * The meanings of modes_[abcd] and sha_[abc] depends on the
+ * change type, but are always present.
+ *
+ * Path(s) are C-Quoted if necessary. Current path is ALWAYS
+ * first. The rename source path is only present when necessary.
+ * A single TAB separates them (because paths can contain spaces
+ * and C-Quoting converts actual tabs in pathnames to 

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Johannes Schindelin]

Hi,

On Wed, 20 Jul 2016, Junio C Hamano wrote:

> Jeff Hostetler <jeffh...@microsoft.com> writes:
> 
> Just to avoid later headaches...  please look at your commit titles
> and imagine how they will look when listed among 400+ other changes
> when they are included in a future release in "git shortlog" output.
> 
> > Subject: Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2
> 
> Subject: status: per-file output for --porcelain=v2
> 
> or something like that, perhaps?
> 
> > This commit sets up version 2 porcelain status and
> > defines the format of detail lines.  This includes
> > the usual XY and pathname fields.  It adds the various
> > file modes and SHAs and the rename score.  For regular
> > entries these values reflect the head, index and
> > worktree. For unmerged entries these values reflect
> > the stage 1, 2, and 3 values.
> 
> Also, we usually do not say "This commit does this and that".
> 
> See Documentation/SubmittingPatches for more details regarding the
> above two points (and more).

Maybe something like this:

-- snipsnap --
status: per-file output for --porcelain=2

The output of `git status --porcelain` leaves out many details that,
say, an IDE would need to know about the current status.

Let's introduce version porcelain status v2 that adds the various file
modes and SHAs and the rename score. For regular entries these values
reflect the head, index and worktree. For unmerged entries these values
reflect the stage 1, 2, and 3 values.
--
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 v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler  writes:

> diff --git a/wt-status.c b/wt-status.c
> index c19b52c..2d4829e 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -406,6 +406,89 @@ static void wt_status_print_change_data(struct wt_status 
> *s,
>   strbuf_release();
>  }
>  
> +
> +/* Copy info for both sides of a head-vs-index change
> + * into the Porcelain V2 data.
> + */

/* 
 * Please don't use unbalanced/asymmetric comment.
 * Our multi-line comment blocks look more like
 * this.
 */

> +static void porcelain_v2_updated_entry(
> + struct wt_status_change_data *d,
> + struct diff_filepair *p)
> +{
> + switch (p->status) {
> + case DIFF_STATUS_ADDED:
> + d->porcelain_v2.mode_index = p->two->mode;
> + hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
> + break;
> +
> + case DIFF_STATUS_DELETED:
> + d->porcelain_v2.mode_head = p->one->mode;
> + hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
> + break;
> +
> + case DIFF_STATUS_RENAMED:
> + d->porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;
> + case DIFF_STATUS_COPIED:
> + case DIFF_STATUS_MODIFIED:
> + case DIFF_STATUS_TYPE_CHANGED:
> + case DIFF_STATUS_UNMERGED:
> + d->porcelain_v2.mode_head = p->one->mode;
> + d->porcelain_v2.mode_index = p->two->mode;
> + hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
> + hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
> + break;
> +
> + case DIFF_STATUS_UNKNOWN:
> + /* This should never happen. */
> + break;
> + }

You may want to have

die("BUG: status unknown???");

if you know "This should never happen.".

The code seems to assume that d->porcelain_v2.* fields are
initialized earlier in the callchain to reasonable values
(e.g. STATUS_ADDED case does not clear .mode_head to "missing"); I
am not sure if that is easier to read or fill in all the values
explicitly in this function.

> +}
> +
> +/* Copy info for both sides of an index-vs-worktree change
> + * into the very verbose porcelain data.
> + */
> +static void porcelain_v2_changed_entry(
> + struct wt_status_change_data *d,
> + const struct diff_filepair *p)
> +{
> + switch (p->status) {
> + case DIFF_STATUS_ADDED:
> + d->porcelain_v2.mode_worktree = p->two->mode;
> + /* don't bother with worktree sha, since it is almost always 
> zero. */
> + break;
> +
> + case DIFF_STATUS_DELETED:
> + d->porcelain_v2.mode_index = p->one->mode;
> + hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
> + break;
> +
> + case DIFF_STATUS_COPIED:
> + case DIFF_STATUS_MODIFIED:
> + case DIFF_STATUS_RENAMED:

Can RENAMED or COPIED happen here?

> + case DIFF_STATUS_TYPE_CHANGED:
> + d->porcelain_v2.mode_index = p->one->mode;
> + d->porcelain_v2.mode_worktree = p->two->mode;
> + hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
> + /* don't bother with worktree sha, since it is almost always 
> zero. */
> + break;
> +
> + case DIFF_STATUS_UNKNOWN:
> + /* This should never happen. */
> + break;
> +
> + case DIFF_STATUS_UNMERGED:
> + /* This should never happen. */
> + break;
> + }
> +}

Other than that, the same comments as the ones for _updated_ apply
to this _changed_ function.

> +static void porcelain_v2_added_initial_entry(
> + struct wt_status_change_data *d,
> + const struct cache_entry *ce)
> +{
> + d->porcelain_v2.mode_index = ce->ce_mode;
> + hashcpy(d->porcelain_v2.sha1_index, ce->sha1);
> +}
> +
>  static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>struct diff_options *options,
>void *data)
> @@ -433,6 +516,9 @@ static void wt_status_collect_changed_cb(struct 
> diff_queue_struct *q,
>   d->dirty_submodule = p->two->dirty_submodule;
>   if (S_ISGITLINK(p->two->mode))
>   d->new_submodule_commits = !!hashcmp(p->one->sha1, 
> p->two->sha1);
> +
> + if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
> + porcelain_v2_changed_entry(d, p);
>   }
>  }
>  
> @@ -486,6 +572,9 @@ static void wt_status_collect_updated_cb(struct 
> diff_queue_struct *q,
>   d->stagemask = unmerged_mask(p->two->path);
>   break;
>   }
> +
> + if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
> + porcelain_v2_updated_entry(d, p);
>   }
>  }
> @@ -565,8 +654,12 @@ static void wt_status_collect_changes_initial(struct 
> wt_status *s)
>   d->index_status = DIFF_STATUS_UNMERGED;
>  

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler <jeffh...@microsoft.com> writes:

Just to avoid later headaches...  please look at your commit titles
and imagine how they will look when listed among 400+ other changes
when they are included in a future release in "git shortlog" output.

> Subject: Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

Subject: status: per-file output for --porcelain=v2

or something like that, perhaps?

> This commit sets up version 2 porcelain status and
> defines the format of detail lines.  This includes
> the usual XY and pathname fields.  It adds the various
> file modes and SHAs and the rename score.  For regular
> entries these values reflect the head, index and
> worktree. For unmerged entries these values reflect
> the stage 1, 2, and 3 values.

Also, we usually do not say "This commit does this and that".

See Documentation/SubmittingPatches for more details regarding the
above two points (and more).

--
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