Re: [PATCHv2] git-status: show short sequencer state

2012-10-29 Thread Phil Hord 
On Thu, Oct 25, 2012 at 12:05 PM, Phil Hord ho...@cisco.com wrote:

 Jeff King wrote:
 On Tue, Oct 23, 2012 at 04:02:54PM -0400, Phil Hord wrote:

 Teach git-status to report the sequencer state in short form
 using a new --sequencer (-S) switch.  Output zero or more
 simple state token strings indicating the deduced state of the
 git sequencer.

 Introduce a common function to determine the current sequencer
 state so the regular status function and this short version can
 share common code.

 Add a substate to wt_status_state to track more detailed
 information about a state, such as conflicted or resolved.
 Move the am_empty_patch flage out of wt_status_state and into
 This patch ended up quite long. It might be a little easier to review
 if it were broken into refactoring steps (I have not looked at it too
 closely yet, but it seems like the three paragraphs above could each be
 their own commit).

I'm currently splitting this out into a series and reconsidering some
of it along the way.  I need some guidance.

I want to support these two modes:

  A.  'git status --short' with sequence tokens added:
   ## conflicted
   ## merge
   ?? untracked-workdir-file
   etc.

  B.  Same as (A) but without workdir status:
   ## conflicted
   ## merge

The user who wants 'A' would initiate it like this:
git status --sequencer
  or
git status -S

How do I spell the options for 'B'?  I have come up with these three
possibilities:
git --sequencer-only   # Another switch
git --sequencer=only   # An OPTARG parser
git -S -S   # like git-diff -C -C, an OPT_COUNTUP

The first one is easy but weird, imho.
The second seems silly for just one type of option.
The last one is cheap to implement, but harder to explain in Documentation/

Any opinions?

Phil
--
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: [PATCHv2] git-status: show short sequencer state

2012-10-29 Thread Phil Hord 
On Mon, Oct 29, 2012 at 5:41 PM, Jeff King p...@peff.net wrote:
 On Mon, Oct 29, 2012 at 02:05:14PM -0400, Phil Hord wrote:

 I'm currently splitting this out into a series and reconsidering some
 of it along the way.  I need some guidance.

 I want to support these two modes:

   A.  'git status --short' with sequence tokens added:
## conflicted
## merge
?? untracked-workdir-file
etc.

   B.  Same as (A) but without workdir status:
## conflicted
## merge

 The user who wants 'A' would initiate it like this:
 git status --sequencer
   or
 git status -S

 How do I spell the options for 'B'?  I have come up with these three
 possibilities:
 git --sequencer-only   # Another switch
 git --sequencer=only   # An OPTARG parser
 git -S -S   # like git-diff -C -C, an OPT_COUNTUP

 Might it be easier to spell 'A' as:

   git status --short -S

 and B as:

   git status -S

 this is sort of like how -b works (except you cannot currently ask for
 it separately, but arguably you could). If we have a proliferation of
 such options, then we might need config to help turn them on all the
 time (I'd guess people are probably already using aliases to do this).

I think I like this path.

I expect a common idiom to be 'git status -S --porcelain --null', and
both --porcelain and --null imply --short.  I think I can still do The
Right Thing, but the code is starting to spaghettify.  I'll take a
crack at it.

Thanks.

P
--
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: [PATCHv2] git-status: show short sequencer state

2012-10-25 Thread Jeff King 
On Tue, Oct 23, 2012 at 04:02:54PM -0400, Phil Hord wrote:

 Teach git-status to report the sequencer state in short form
 using a new --sequencer (-S) switch.  Output zero or more
 simple state token strings indicating the deduced state of the
 git sequencer.
 
 Introduce a common function to determine the current sequencer
 state so the regular status function and this short version can
 share common code.
 
 Add a substate to wt_status_state to track more detailed
 information about a state, such as conflicted or resolved.
 Move the am_empty_patch flage out of wt_status_state and into

This patch ended up quite long. It might be a little easier to review
if it were broken into refactoring steps (I have not looked at it too
closely yet, but it seems like the three paragraphs above could each be
their own commit).

 State token strings which may be emitted and their meanings:
 merge  a git-merge is in progress
 am a git-am is in progress
 rebase a git-rebase is in progress
 rebase-interactive a git-rebase--interactive is in progress

A minor nit, but you might want to update this list from the one in the
documentation.

 diff --git a/builtin/commit.c b/builtin/commit.c
 index a17a5df..9706ed9 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -114,7 +114,8 @@ static struct strbuf message = STRBUF_INIT;
  static enum {
   STATUS_FORMAT_LONG,
   STATUS_FORMAT_SHORT,
 - STATUS_FORMAT_PORCELAIN
 + STATUS_FORMAT_PORCELAIN,
 + STATUS_FORMAT_SEQUENCER
  } status_format = STATUS_FORMAT_LONG;

Hmm. So the new format is its own distinct output format. I could not
say I would like to see short status, and by the way, show me the
sequencer state, as you can with -b. Is it possible to do this (or
even desirable; getting the sequencer state should be way cheaper, so
conflating the two may not be what some callers want).

Not complaining, just wondering about the intended use cases.

Also, does there need to be a --porcelain version of this output? It
seems like we can have multiple words (e.g., in a merge, with conflicted
entries). If there is no arbitrary data, we do not have to worry about
delimiters and quoting.  But I wonder if we would ever want to expand
the information to include arbitrary strings, at which point we would
want NUL delimiters; should we start with that now?

 + // Determine main sequencer activity

Please avoid C99 comments (there are others in the patch, too).

 +void wt_sequencer_print(struct wt_status *s)
 +{
 + struct wt_status_state state;
 +
 + wt_status_get_state(s, state);
 +
 + if (state.merge_in_progress)
 + wt_print_token(s, merge);
 + if (state.am_in_progress)
 + wt_print_token(s, am);
 + if (state.rebase_in_progress)
 + wt_print_token(s, rebase);
 + if (state.rebase_interactive_in_progress)
 + wt_print_token(s, rebase-interactive);
 + if (state.cherry_pick_in_progress)
 + wt_print_token(s, cherry-pick);
 + if (state.bisect_in_progress)
 + wt_print_token(s, bisect);
 +
 + switch (state.substate) {
 + case WT_SUBSTATE_NOMINAL:
 + break;
 + case WT_SUBSTATE_CONFLICTED:
 + wt_print_token(s, conflicted);
 + break;
 + case WT_SUBSTATE_RESOLVED:
 + wt_print_token(s, resolved);
 + break;
 + case WT_SUBSTATE_EDITED:
 + wt_print_token(s, edited);
 + break;
 + case WT_SUBSTATE_EDITING:
 + wt_print_token(s, editing);
 + break;
 + case WT_SUBSTATE_SPLITTING:
 + wt_print_token(s, splitting);
 + break;
 + case WT_SUBSTATE_AM_EMPTY:
 + wt_print_token(s, am-empty);
 + break;
 + }
 +}

It is clear from this code that some tokens can happen together, and
some are mutually exclusive. Should the documentation talk about that,
or do we want to literally keep it as a list of tags?

-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: [PATCHv2] git-status: show short sequencer state

2012-10-25 Thread Phil Hord 

Jeff King wrote:
 On Tue, Oct 23, 2012 at 04:02:54PM -0400, Phil Hord wrote:

 Teach git-status to report the sequencer state in short form
 using a new --sequencer (-S) switch.  Output zero or more
 simple state token strings indicating the deduced state of the
 git sequencer.

 Introduce a common function to determine the current sequencer
 state so the regular status function and this short version can
 share common code.

 Add a substate to wt_status_state to track more detailed
 information about a state, such as conflicted or resolved.
 Move the am_empty_patch flage out of wt_status_state and into
 This patch ended up quite long. It might be a little easier to review
 if it were broken into refactoring steps (I have not looked at it too
 closely yet, but it seems like the three paragraphs above could each be
 their own commit).

I can do that.

 State token strings which may be emitted and their meanings:
 merge  a git-merge is in progress
 am a git-am is in progress
 rebase a git-rebase is in progress
 rebase-interactive a git-rebase--interactive is in progress
 A minor nit, but you might want to update this list from the one in the
 documentation.

I considered it, but I also considered the audience; then I took the
easier path.  I'll look again.

 diff --git a/builtin/commit.c b/builtin/commit.c
 index a17a5df..9706ed9 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -114,7 +114,8 @@ static struct strbuf message = STRBUF_INIT;
  static enum {
  STATUS_FORMAT_LONG,
  STATUS_FORMAT_SHORT,
 -STATUS_FORMAT_PORCELAIN
 +STATUS_FORMAT_PORCELAIN,
 +STATUS_FORMAT_SEQUENCER
  } status_format = STATUS_FORMAT_LONG;
 Hmm. So the new format is its own distinct output format. I could not
 say I would like to see short status, and by the way, show me the
 sequencer state, as you can with -b. Is it possible to do this (or
 even desirable; getting the sequencer state should be way cheaper, so
 conflating the two may not be what some callers want).

 Not complaining, just wondering about the intended use cases.

Originally I did place this output in the short-format display.  I
particularly want this info to be available for scripts. But it feels
right to include it with the short status, since the long-form is
already available with 'git status --no-short'.

Since there may be more than one line reported, I did not feel there was
a simple way to contain these details in with the short status.  For
branch this is a simple decision as the reported branch will take
exactly one line.

  git status -b -s
  ## master
   M Foo

But for sequencer state, this seemed like it could be too convoluted or
might encourage too much reliance line-counting:

  git status -b -s -S
  ## master
  ## merge
  ## conflicted
   M Foo

Maybe a different line-prefix would help make this clearer:

  git status -b -s -S
  ## master
  #! merge
  #! conflicted
   M Foo

It seemed to me this was someone else's itch and I might not scratch it
properly.  But I am willing to try if you think this is more useful than
distracting.

 Also, does there need to be a --porcelain version of this output? It
 seems like we can have multiple words (e.g., in a merge, with conflicted
 entries). If there is no arbitrary data, we do not have to worry about
 delimiters and quoting.  But I wonder if we would ever want to expand
 the information to include arbitrary strings, at which point we would
 want NUL delimiters; should we start with that now?

This works, though I haven't tested it on v2:

   git status -S -z


 +// Determine main sequencer activity
 Please avoid C99 comments (there are others in the patch, too).

Thanks.

 +void wt_sequencer_print(struct wt_status *s)
 +{
 +struct wt_status_state state;
 +
 +wt_status_get_state(s, state);
 +
 +if (state.merge_in_progress)
 +wt_print_token(s, merge);
 +if (state.am_in_progress)
 +wt_print_token(s, am);
 +if (state.rebase_in_progress)
 +wt_print_token(s, rebase);
 +if (state.rebase_interactive_in_progress)
 +wt_print_token(s, rebase-interactive);
 +if (state.cherry_pick_in_progress)
 +wt_print_token(s, cherry-pick);
 +if (state.bisect_in_progress)
 +wt_print_token(s, bisect);
 +
 +switch (state.substate) {
 +case WT_SUBSTATE_NOMINAL:
 +break;
 +case WT_SUBSTATE_CONFLICTED:
 +wt_print_token(s, conflicted);
 +break;
 +case WT_SUBSTATE_RESOLVED:
 +wt_print_token(s, resolved);
 +break;
 +case WT_SUBSTATE_EDITED:
 +wt_print_token(s, edited);
 +break;
 +case WT_SUBSTATE_EDITING:
 +wt_print_token(s, editing);
 +break;
 +case WT_SUBSTATE_SPLITTING:
 +wt_print_token(s, splitting);
 +break;
 +case WT_SUBSTATE_AM_EMPTY:
 +wt_print_token(s, am-empty);
 +