[PATCH] git-rebase.sh: Update USAGE string

[PATCH] git-rebase.sh: Update USAGE string

[Jari Aalto]

Present the options in -s|--long (short, long) order.
Mention merge and new whitespace option.

Signed-off-by: Jari Aalto <jari.aalto AT cante.net>
---
 git-rebase.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..9c8cf90 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Junio C Hamano.
 #
 
-USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
+USAGE='[-i|--interactive] [-v|--verbose] [--whitespace={nowarn|warn|error|error-all|strip}] [-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
 LONG_USAGE='git-rebase replaces <branch> with a new branch of the
 same name.  When the --onto option is provided the new branch starts
 out with a HEAD equal to <newbase>, otherwise it is equal to <upstream>
-- 
1.5.4-rc5.GIT-dirty



-- 
Welcome to FOSS revolution: we fix and modify until it shines

Re: [PATCH] git-rebase.sh: Update USAGE string

[Jakub Narebski]

Jari Aalto <jari.aalto@cante.net> writes:

> Present the options in -s|--long (short, long) order.

This is a good change (but might not make sense without the following
one)...

> Mention merge and new whitespace option.

...and this is not; usage string is now too long.

Please either use placeholder ([<whitespace>]), or enumerate
only short version of commands ([-i] [-v] [-m]) in synopsis,
or break synopsis into lines 80-column lines max.

> -USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
> +USAGE='[-i|--interactive] [-v|--verbose] [--whitespace={nowarn|warn|error|error-all|strip}] [-m|--merge] [--onto <newbase>] <upstream> [<branch>]'

-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jari Aalto]

Present the options in -s|--long (short, long) order.
Mention merge and new whitespace option.

ed-off-by: Jari Aalto <jari.aalto AT cante.net>
---
 ***********************************************************************
 Reworked patch (No. 1)

 [Jakub Narebski]
 > or break synopsis into lines 80-column lines max.

 ***********************************************************************

 git-rebase.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..6805742 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -3,7 +3,10 @@
 # Copyright (c) 2005 Junio C Hamano.
 #
 
-USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
+USAGE='[-i|--interactive] [-v|--verbose]
+[--whitespace={nowarn|warn|error|error-all|strip}]
+[-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
+
 LONG_USAGE='git-rebase replaces <branch> with a new branch of the
 same name.  When the --onto option is provided the new branch starts
 out with a HEAD equal to <newbase>, otherwise it is equal to <upstream>
-- 
1.5.4-rc5.GIT-dirty


-- 
Welcome to FOSS revolution: we fix and modify until it shines

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Johannes Schindelin]

Hi,

what's this "(No. 1)" business?

On Mon, 4 Feb 2008, Jari Aalto wrote:

> ed-off-by: Jari Aalto <jari.aalto AT cante.net>

ed-by: me... ;-)

> Welcome to FOSS revolution: we fix and modify until it shines

/me grins

Ciao,
Dscho

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jakub Narebski]

Jari Aalto <jari.aalto@cante.net> writes:

> Subject: Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

What's with the "(No. 1)" here? If you want to add some comment that
should *not* be part of commit message, put it in brackets beside
PATCH, for example "[PATCH/RFC]", "[PATCH (resend)]", 
"[PATCH (amend)]", "[PATCH v2]" etc.

> Present the options in -s|--long (short, long) order.
> Mention merge and new whitespace option.
> 
> ed-off-by: Jari Aalto <jari.aalto AT cante.net>

I guess that it is copy'n'paste error, and it should be
Signed-off-by: Jari Aalto <jari.aalto@cante.net>

>  
> -USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
> +USAGE='[-i|--interactive] [-v|--verbose]
> +[--whitespace={nowarn|warn|error|error-all|strip}]

I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
instead of introducing yet not agreed upon notation (this has the
advantage of shortening synopisis, which should be short IMHO).

Besides here the mutually exclusive options are naturelly delimited,
so you can say just (I think):
   [--whitespace=nowarn|warn|error|error-all|strip]

> +[-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
> +

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jari Aalto]

* Sun 2008-02-03 Jakub Narebski <jnareb@gmail.com> INBOX
> I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
> instead of introducing yet not agreed upon notation (this has the
> advantage of shortening synopisis, which should be short IMHO).

The "{a|b|c}" is a used syntax. See cpio(1).
 
    cpio  {-o|--create} [-0acvABLV] ...

The information about values to --whitespace is buried under 2 manual
pages. A help that displays the values immediately is more helpful. The
--whitespace option is used very frequently.

Jari

-- 
Welcome to FOSS revolution: we fix and modify until it shines

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Jari Aalto wrote:

> * Sun 2008-02-03 Jakub Narebski <jnareb@gmail.com> INBOX
> > I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
> > instead of introducing yet not agreed upon notation (this has the
> > advantage of shortening synopisis, which should be short IMHO).
> 
> The "{a|b|c}" is a used syntax. See cpio(1).

By this reasoning, "(a|b|c)" is also a used syntax.  See git-branch(1).

Ciao,
Dscho

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Junio C Hamano]

Jari Aalto <jari.aalto@cante.net> writes:

> * Sun 2008-02-03 Jakub Narebski <jnareb@gmail.com> INBOX
>> I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
>> instead of introducing yet not agreed upon notation (this has the
>> advantage of shortening synopisis, which should be short IMHO).
>
> The "{a|b|c}" is a used syntax. See cpio(1).
>  
>     cpio  {-o|--create} [-0acvABLV] ...

I do not think using {} for grouping is incorrect, and I think
there is at least a consensus on the list that it is Ok as long
as we consistently do so.

Unfortunately, the majority of, if not all of, our existing
documents use () instead for that purpose.

So pros-and-cons are that (1) changing all of them to use {} is
more politically correct (pro); (2) our use of (), as we
consistently use them, does not hurt readability (neutral); and
(3) it is a thankless makework to replace them all to {} _and
make sure the conversion is correct_ (large con).  (4) also if
other people make changes to documentation at the same time,
that would add more work in conflict resolution (slight con).

As you seem to have volunteered to do the conversion and
validation, I do not see much problem to move from () to {} in
principle.

About the part your patch touches, [--whitespace={a|b|c}] is
more precise than [--whitespace=a|b|c] Jakub suggested, but I
suspect most sane people would not misinterpret the latter as
"this part can be omitted but you could write '--whitespace=a',
'b' or 'c' here", so...

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jakub Narebski]

Junio C Hamano wrote:
> Jari Aalto <jari.aalto@cante.net> writes:
> 
>> * Sun 2008-02-03 Jakub Narebski <jnareb@gmail.com> INBOX
>>> I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
>>> instead of introducing yet not agreed upon notation (this has the
>>> advantage of shortening synopisis, which should be short IMHO).
>>
>> The "{a|b|c}" is a used syntax. See cpio(1).
>>  
>>     cpio  {-o|--create} [-0acvABLV] ...

BTW. it is not only cpio and rpm, but also combinediff (and friends from 
patchutils), flex, gimp, jpegtopnm (and other from netpbm), ps2pdf, 
run, tail, xmlto, ip, losetup, netstat, sudo and sudoedit,...

> I do not think using {} for grouping is incorrect, and I think
> there is at least a consensus on the list that it is Ok as long
> as we consistently do so.
> 
> Unfortunately, the majority of, if not all of, our existing
> documents use () instead for that purpose.
> 
> So pros-and-cons are that (1) changing all of them to use {} is
> more politically correct (pro); (2) our use of (), as we
> consistently use them, does not hurt readability (neutral); and

Actually we are not entirely consistent here.  git-init(1) has 
  --shared[={false|true|umask|group|all|world|everybody}]
in the option description, git-rev-list(1) has
  [ --date={local|relative|default|iso|rfc|short} ]
in its longish synopsis (although in second case we could omit
the curlies, it is in separate line so reducing line length does
not matter in this case).

> (3) it is a thankless makework to replace them all to {} _and
> make sure the conversion is correct_ (large con).  

All true.

> (4) also if 
> other people make changes to documentation at the same time,
> that would add more work in conflict resolution (slight con).

Well, we sould have to document this in CodingStyle, I think, then.
 
> About the part your patch touches, [--whitespace={a|b|c}] is
> more precise than [--whitespace=a|b|c] Jakub suggested, but I
> suspect most sane people would not misinterpret the latter as
> "this part can be omitted but you could write '--whitespace=a',
> 'b' or 'c' here", so...

It probably also depends if we deperately want to reduce line length as 
not to split synopsis into yet another line (the shorter synopsis is, 
usually the better).

-- 
Jakub Narebski
Poland

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Mike Hommey]

On Mon, Feb 04, 2008 at 11:15:20AM +0100, Jakub Narebski <jnareb@gmail.com> wrote:
> Actually we are not entirely consistent here.  git-init(1) has 
>   --shared[={false|true|umask|group|all|world|everybody}]
> in the option description, git-rev-list(1) has
>   [ --date={local|relative|default|iso|rfc|short} ]

What is inconsistent here ? The first tells you you can use --shared without
an argument. And it's [--shared[=<permissions>]] in the synopsis, so you
may omit --shared, or use in alone, or specify permissions. The second tells
you can't use --date without an additional argument.

Mike

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jakub Narebski]

On Mon, 4 Feb 2008, Mike Hommey wrote:
> On Mon, Feb 04, 2008 at 11:15:20AM +0100, Jakub Narebski <jnareb@gmail.com> wrote:
>> Junio C Hamano wrote:

>>> Unfortunately, the majority of, if not all of, our existing
>>> documents use () instead for that purpose.
>>>
>>> So pros-and-cons are that [...] (2) our use of (), as we
>>> consistently use them, does not hurt readability (neutral); and

The above was re-added by me.

>> Actually we are not entirely consistent here.  git-init(1) has 
>>   --shared[={false|true|umask|group|all|world|everybody}]
>> in the option description, git-rev-list(1) has
>>   [ --date={local|relative|default|iso|rfc|short} ]
> 
> What is inconsistent here ? The first tells you you can use --shared without
> an argument. And it's [--shared[=<permissions>]] in the synopsis, so you
> may omit --shared, or use in alone, or specify permissions. The second tells
> you can't use --date without an additional argument.

You have cut a bit too much when quoting. The inconsistency is between
git-branch(1) using () to delimit mutually exclusive options:

   git-branch (-m | -M) [<oldbranch>] <newbranch>

and git-init(1) (and git-rev-list(1)) using {} for that:

   --shared[={false|true|umask|group|all|world|everybody}]

-- 
Jakub Narebski
Poland

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Besides here the mutually exclusive options are naturelly delimited,
> so you can say just (I think):
>    [--whitespace=nowarn|warn|error|error-all|strip]

I think that is probably the right thing to do in this case.  It
is clear where the alternatives end, without adding extra
grouping, so it keeps the description easy to read.

It may fail with a sick reader who would complain "but 'git
rebase --whitespace=nowarn' is correct while 'git rebase warn'
is not!", but personally I do not think we deeply care about
pleasing such a pedantic nitpicker.

[PATCH v3] git-rebase.sh: Update USAGE string

[Jari Aalto]

Present the options in -s|--long (short, long) order.
Mention merge and new whitespace option.

Signed-off-by: Jari Aalto <jari.aalto AT cante.net>
---
 ***********************************************************************
 reworked patch (No. 3)
 ***********************************************************************

 [Junio C Hamano}
 >>    [--whitespace=nowarn|warn|error|error-all|strip]
 >
 > I think that is probably the right thing to do in this case.  It
 > is clear where the alternatives end, without adding extra
 > grouping, so it keeps the description easy to read.

 git-rebase.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..6128455 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -3,7 +3,10 @@
 # Copyright (c) 2005 Junio C Hamano.
 #
 
-USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
+USAGE='[-i|--interactive] [-v|--verbose]
+[--whitespace=nowarn|warn|error|error-all|strip]
+[-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
+
 LONG_USAGE='git-rebase replaces <branch> with a new branch of the
 same name.  When the --onto option is provided the new branch starts
 out with a HEAD equal to <newbase>, otherwise it is equal to <upstream>
-- 
1.5.4-rc5.GIT-dirty

Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[しらいしななこ]

Quoting Jakub Narebski <jnareb@gmail.com>:

> Jari Aalto <jari.aalto@cante.net> writes:
>>  
>> -USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
>> +USAGE='[-i|--interactive] [-v|--verbose]
>> +[--whitespace={nowarn|warn|error|error-all|strip}]
>
> I would say "[--whitespace=<option>]" or "[--whitespace=<action>]"
> instead of introducing yet not agreed upon notation (this has the
> advantage of shortening synopisis, which should be short IMHO).
>
> Besides here the mutually exclusive options are naturelly delimited,
> so you can say just (I think):
>    [--whitespace=nowarn|warn|error|error-all|strip]
>
>> +[-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
>> +

I tried to run "git rebase --interactive --whitespace=strip" but it does not seem to strip blank characters at the end of my lines.  Did I find a bug?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Find out how you can get spam free email.
http://www.bluebottle.com/tag/3

rebase -i and --whitespace, was Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 503 bytes --]

Hi,

On Mon, 4 Feb 2008, しらいしななこ wrote:

> I tried to run "git rebase --interactive --whitespace=strip" but it does 
> not seem to strip blank characters at the end of my lines.  Did I find a 
> bug?

Yes.  Interactive rebase never bothered with --whitespace options, since 
it works purely with cherry-pick (the --merge option to non-interactive 
rebase).  Therefore, the operation is not patch based, and does not call 
git-apply at all (which would handle the whitespace).

Ciao,
Dscho

Re: rebase -i and --whitespace, was Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Jakub Narebski]

W poniedziałek 4. lutego 2008 16:06, Johannes Schindelin napisał:
> On Mon, 4 Feb 2008, しらいしななこ wrote:
> 
> > I tried to run "git rebase --interactive --whitespace=strip" but it does 
> > not seem to strip blank characters at the end of my lines.  Did I find a 
> > bug?
> 
> Yes.  Interactive rebase never bothered with --whitespace options, since 
> it works purely with cherry-pick (the --merge option to non-interactive 
> rebase).  Therefore, the operation is not patch based, and does not call 
> git-apply at all (which would handle the whitespace).

So it means that synopsis should, instead of current (pre-patch)

'git-rebase' [-i | --interactive] [-v | --verbose] [-m | --merge]
        [-C<n>] [ --whitespace=<option>] [-p | --preserve-merges]
        [--onto <newbase>] <upstream> [<branch>]

should read

'git-rebase' [-v | --verbose]  [-p | --preserve-merges]
        [{-i | --interactive} | [-C<n>] [ --whitespace=<option>] [-m | --merge]]
        [--onto <newbase>] <upstream> [<branch>]

or perhaps even separated into interactive / non-interactive merge?

-- 
Jakub Narebski
Poland

Re: rebase -i and --whitespace, was Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1425 bytes --]

Hi,

On Mon, 4 Feb 2008, Jakub Narebski wrote:

> W poniedziałek 4. lutego 2008 16:06, Johannes Schindelin napisał:
> > On Mon, 4 Feb 2008, しらいしななこ wrote:
> > 
> > > I tried to run "git rebase --interactive --whitespace=strip" but it does 
> > > not seem to strip blank characters at the end of my lines.  Did I find a 
> > > bug?
> > 
> > Yes.  Interactive rebase never bothered with --whitespace options, since 
> > it works purely with cherry-pick (the --merge option to non-interactive 
> > rebase).  Therefore, the operation is not patch based, and does not call 
> > git-apply at all (which would handle the whitespace).
> 
> So it means that synopsis should, instead of current (pre-patch)
> 
> 'git-rebase' [-i | --interactive] [-v | --verbose] [-m | --merge]
>         [-C<n>] [ --whitespace=<option>] [-p | --preserve-merges]
>         [--onto <newbase>] <upstream> [<branch>]
> 
> should read
> 
> 'git-rebase' [-v | --verbose]  [-p | --preserve-merges]
>         [{-i | --interactive} | [-C<n>] [ --whitespace=<option>] [-m | --merge]]
>         [--onto <newbase>] <upstream> [<branch>]
> 
> or perhaps even separated into interactive / non-interactive merge?

Or the --merge code path should check if there was any --whitespace 
option, and re-apply each commit using something like

	git reset --hard HEAD^ &&
	git diff-tree HEAD@{1}..HEAD |
	git apply --index <whitespace options>

Ciao,
Dscho

Re: rebase -i and --whitespace, was Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> So it means that synopsis should, instead of current (pre-patch)
>
> 'git-rebase' [-i | --interactive] [-v | --verbose] [-m | --merge]
>         [-C<n>] [ --whitespace=<option>] [-p | --preserve-merges]
>         [--onto <newbase>] <upstream> [<branch>]
>
> should read
>
> 'git-rebase' [-v | --verbose]  [-p | --preserve-merges]
>         [{-i | --interactive} | [-C<n>] [ --whitespace=<option>] [-m | --merge]]
>         [--onto <newbase>] <upstream> [<branch>]
>
> or perhaps even separated into interactive / non-interactive merge?

I think the reality is:

 * -i ignores the lack of -m (i.e. do not use the slow "merge");

 * Rebase with -m cannot use -C<n> and --whitespace (hence -i
   because it forces -m);

 * -p is only meaningful when using -m;

Three possible courses of actions are:

 (1) fix merge codepath (this is involved --- we would need to
     teach xdl_merge() to honor --whitespace={warn|error|fix}
     and -C<n>); or

 (2) fix -i so that it does not force -m; or

 (3) adjust the description to reality.

Obviously the easiest would be to document the behaviour as-is,
but I suspect (2) would be the best practical solution if we
wanted to have any improvement compared to the current
situation.

Re: rebase -i and --whitespace, was Re: [PATCH] git-rebase.sh: Update USAGE string (No. 1)

[Johannes Schindelin]

Hi,

On Tue, 5 Feb 2008, Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > So it means that synopsis should, instead of current (pre-patch)
> >
> > 'git-rebase' [-i | --interactive] [-v | --verbose] [-m | --merge]
> >         [-C<n>] [ --whitespace=<option>] [-p | --preserve-merges]
> >         [--onto <newbase>] <upstream> [<branch>]
> >
> > should read
> >
> > 'git-rebase' [-v | --verbose]  [-p | --preserve-merges]
> >         [{-i | --interactive} | [-C<n>] [ --whitespace=<option>] [-m | --merge]]
> >         [--onto <newbase>] <upstream> [<branch>]
> >
> > or perhaps even separated into interactive / non-interactive merge?
> 
> I think the reality is:
> 
>  * -i ignores the lack of -m (i.e. do not use the slow "merge");
> 
>  * Rebase with -m cannot use -C<n> and --whitespace (hence -i
>    because it forces -m);
> 
>  * -p is only meaningful when using -m;

Even worse, AFAIR -p only works with -i.

> Three possible courses of actions are:
> 
>  (1) fix merge codepath (this is involved --- we would need to
>      teach xdl_merge() to honor --whitespace={warn|error|fix}
>      and -C<n>); or
> 
>  (2) fix -i so that it does not force -m; or
> 
>  (3) adjust the description to reality.
> 
> Obviously the easiest would be to document the behaviour as-is,
> but I suspect (2) would be the best practical solution if we
> wanted to have any improvement compared to the current
> situation.

Granted.  A long time ago, I started making a builtin from rebase/am, but 
I got sidetracked pretty early.

However, I think that (2) and builtinification are pretty related, and can 
be done in one go.

Ciao,
Dscho

[PATCH v2] git-rebase.sh: Update USAGE string

[Jari Aalto]

Present the options in -s|--long (short, long) order.
Mention merge and new whitespace option.

Signed-off-by: Jari Aalto <jari.aalto AT cante.net>
---
 ***********************************************************************
 Reworked patch (No. 2)

 * [Jakub Narebski]
 > break synopsis into lines 80-column lines max.

 * Fix "ed-off-by"

 ***********************************************************************

 git-rebase.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..6805742 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -3,7 +3,10 @@
 # Copyright (c) 2005 Junio C Hamano.
 #
 
-USAGE='[--interactive | -i] [-v] [--onto <newbase>] <upstream> [<branch>]'
+USAGE='[-i|--interactive] [-v|--verbose]
+[--whitespace={nowarn|warn|error|error-all|strip}]
+[-m|--merge] [--onto <newbase>] <upstream> [<branch>]'
+
 LONG_USAGE='git-rebase replaces <branch> with a new branch of the
 same name.  When the --onto option is provided the new branch starts
 out with a HEAD equal to <newbase>, otherwise it is equal to <upstream>
-- 
1.5.4-rc5.GIT-dirty



-- 
Welcome to FOSS revolution: we fix and modify until it shines