patchgit-rebase.sh: Update USAGE string

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

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

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

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

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

Hi,

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

On Mon, 4 Feb 2008, Jari Aalto wrote:

ed-by: me... ;-)

/me grins

Ciao, Dscho

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

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.

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

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]

* Sun 2008-02-03 Jakub Narebski <jnareb@gmail.com> INBOX

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

Hi,

On Mon, 4 Feb 2008, Jari Aalto wrote:

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

Ciao, Dscho

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

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

Junio C Hamano wrote:

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

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

All true.

Well, we sould have to document this in CodingStyle, I think, then.
 

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

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

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

On Mon, 4 Feb 2008, Mike Hommey wrote:

The above was re-added by me.

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 <jnareb@gmail.com> writes:

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.

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

Quoting Jakub Narebski <jnareb@gmail.com>:

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?

Hi,

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

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

W poniedziałek 4. lutego 2008 16:06, Johannes Schindelin napisał:

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?

Hi,

On Mon, 4 Feb 2008, Jakub Narebski wrote:

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

Jakub Narebski <jnareb@gmail.com> writes:

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.

Hi,

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

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

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

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