git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v1 0/3] fixup fixup documenation
@ 2016-08-14 21:46 Philip Oakley
  2016-08-14 21:46 ` [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision Philip Oakley
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 21:46 UTC (permalink / raw)
  To: GitList

With the review of the list's workflow, and failings in my personal
workflow, I asked a couple of questions about fixup and discovered
that my reading of the man pages hadn't provided the illumination
they hope to give. 

Here's three little documenation patches, and some queries to go
with them.

Philip Oakley (3):
  doc: commit: --fixup/--squash can take a commit revision
  doc: rebase: fixup! can take an object name
  doc: rebase: clarify fixup! fixup! constraint

 Documentation/git-commit.txt | 8 ++++----
 Documentation/git-rebase.txt | 9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.9.0.windows.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 21:46 [PATCH v1 0/3] fixup fixup documenation Philip Oakley
@ 2016-08-14 21:46 ` Philip Oakley
  2016-08-14 22:09   ` Junio C Hamano
  2016-08-14 21:46 ` [PATCH v1 2/3] doc: rebase: fixup! can take an object name Philip Oakley
  2016-08-14 21:46 ` [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint Philip Oakley
  2 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 21:46 UTC (permalink / raw)
  To: GitList

Be clearer that the --fixup/--squash options can take any of the
gitrevisions methods of specifying a commit, not just a 'hash'.

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
v1
It's not immediately obvious what different forms the <commit>
option can take. Spell out, and refer to the git revisions guide,
that any of the revision methods will work.

On a side note, if one looks at the glossary, a <commit> links to
<commit object> which links to <object> (and only tangentially
"about" revisions) which then says "uniquely identified by the SHA-1",
so it is easy to think one should use the sha1 here.

I only discovered this misunderstanding while following up
other parts of this series!
---
 Documentation/git-commit.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index e704953..3600929 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -81,15 +81,15 @@ OPTIONS
 --fixup=<commit>::
 	Construct a commit message for use with `rebase --autosquash`.
 	The commit message will be the subject line from the specified
-	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
-	for details.
+	commit revision with a prefix of "fixup! ".  See linkgit:git-rebase[1]
+	and linkgit:gitrevisions[7] for details.
 
 --squash=<commit>::
 	Construct a commit message for use with `rebase --autosquash`.
 	The commit message subject line is taken from the specified
-	commit with a prefix of "squash! ".  Can be used with additional
+	commit revision with a prefix of "squash! ".  Can be used with additional
 	commit message options (`-m`/`-c`/`-C`/`-F`). See
-	linkgit:git-rebase[1] for details.
+	linkgit:git-rebase[1] and linkgit:gitrevisions[7] for details.
 
 --reset-author::
 	When used with -C/-c/--amend options, or when committing after a
-- 
2.9.0.windows.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 2/3] doc: rebase: fixup! can take an object name
  2016-08-14 21:46 [PATCH v1 0/3] fixup fixup documenation Philip Oakley
  2016-08-14 21:46 ` [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision Philip Oakley
@ 2016-08-14 21:46 ` Philip Oakley
  2016-08-14 22:11   ` Junio C Hamano
  2016-08-14 21:46 ` [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint Philip Oakley
  2 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 21:46 UTC (permalink / raw)
  To: GitList

Since 68d5d03 (rebase: teach --autosquash to match on sha1 in addition
to message, 2010-11-04) the commit subject can refer directly to the
destination object hash as a single word.)...

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
v1
This is about the actual commit subject line, rather than the --fixup
options to the commit command.

This came out of https://public-inbox.org/git/FAE9116880074D6FA421942CCAEC368F@PhilipOakley/
where I was expecting to be able to say 'fixup! <sha1> my message', but
I can't (which would be another day's patch - fixup! <rev>! my message').

One question is whether 'standalone' is clear enough, or needs to say
'single word revision'? (which would mean it's not 'object name')

Further, with more digging, I think that any
rev specifier that has no spaces should work [1], despite the 68d5d's
title. Though maybe during the relevant phase of rebase -i some of the
rev specifiers may not work because of the series being rewound - dunno.

[1] https://github.com/git/git/blame/v2.9.2/git-rebase--interactive.sh#L790
---
 Documentation/git-rebase.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0387b40..66b789a 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -421,7 +421,8 @@ without an explicit `--interactive`.
 --no-autosquash::
 	When the commit log message begins with "squash! ..." (or
 	"fixup! ..."), and there is a commit whose title begins with
-	the same ..., automatically modify the todo list of rebase -i
+	the same "..." message, or a commit object name (standalone),
+	automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
 	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
-- 
2.9.0.windows.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint
  2016-08-14 21:46 [PATCH v1 0/3] fixup fixup documenation Philip Oakley
  2016-08-14 21:46 ` [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision Philip Oakley
  2016-08-14 21:46 ` [PATCH v1 2/3] doc: rebase: fixup! can take an object name Philip Oakley
@ 2016-08-14 21:46 ` Philip Oakley
  2016-08-14 22:20   ` Junio C Hamano
  2 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 21:46 UTC (permalink / raw)
  To: GitList

22c5b13 (rebase -i: handle fixup! fixup! in --autosquash, 2013-06-27)

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
v1
the historical discussion about this is here
https://public-inbox.org/git/20130611180530.GA18488%40oinkpad.pimlott.net/

I certainly misunderstood what this meant. It sounded like only one fixup! was
allowed per commit (i.e. one mistake) - fixing two mistakes wouldn't be
allowed. Hindsight is a wonderful thing.

Also, does 'earliest commit requiring fixup/squash' fully convey that
its the one to fix.
---
 Documentation/git-rebase.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 66b789a..91eb107 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -425,9 +425,9 @@ without an explicit `--interactive`.
 	automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
-	"fixup! " or "squash! " after the first, in case you referred to an
-	earlier fixup/squash with `git commit --fixup/--squash`.
+	commit from `pick` to `squash` (or `fixup`).  Commits with repeated
+	"fixup! " or "squash! " in the subject line are considered to refer
+	to the earliest commit requiring fixup/squash.
 +
 This option is only valid when the '--interactive' option is used.
 +
-- 
2.9.0.windows.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 21:46 ` [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision Philip Oakley
@ 2016-08-14 22:09   ` Junio C Hamano
  2016-08-14 22:45     ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2016-08-14 22:09 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

Philip Oakley <philipoakley@iee.org> writes:
> Be clearer that the --fixup/--squash options can take any of the
> gitrevisions methods of specifying a commit, not just a 'hash'.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.org>
> ---
> ...
> @@ -81,15 +81,15 @@ OPTIONS
>  --fixup=<commit>::
>  	Construct a commit message for use with `rebase --autosquash`.
>  	The commit message will be the subject line from the specified
> -	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
> -	for details.
> +	commit revision with a prefix of "fixup! ".  See linkgit:git-rebase[1]
> +	and linkgit:gitrevisions[7] for details.

The same comment applies to the other hunk, but rephrasing "commit"
with "commit revision" (the latter is not even in the glossary) does
not make it clearer at all.  Especially when discussing rebases and
anything that rewrites commits, it can easily be mistaken as if you
are talking about v2 of the commit by fixing up the original, but
that is not the impression you want to give.

"The specified commit" is clear enough.  It may be debatable if we
want to talk about "how" to specify the commit, though.  I think the
use of "commit" in an angle-bracket-pair in the label for the
section, i.e. "--fixup=<commit>", has been considered to be clear
enough to tell that you can use usual extended SHA-1 syntax to
specify the commit you want to talk about, but if so, that is not
limited to this entry, and I do not think this description or the
other one for the "--squash" option are particularly worse than
those for the "-c" and "-C" options.  The description for "-c" does
say "Take an existing commit object", but that's like "the specified
commit" used here.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/3] doc: rebase: fixup! can take an object name
  2016-08-14 21:46 ` [PATCH v1 2/3] doc: rebase: fixup! can take an object name Philip Oakley
@ 2016-08-14 22:11   ` Junio C Hamano
  2016-08-14 23:00     ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2016-08-14 22:11 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

Philip Oakley <philipoakley@iee.org> writes:

> Since 68d5d03 (rebase: teach --autosquash to match on sha1 in addition
> to message, 2010-11-04) the commit subject can refer directly to the
> destination object hash as a single word.)...

That's not an object hash but an object name (see glossary); you got
it right in the actual patch text, though ;-).

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 0387b40..66b789a 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -421,7 +421,8 @@ without an explicit `--interactive`.
>  --no-autosquash::
>  	When the commit log message begins with "squash! ..." (or
>  	"fixup! ..."), and there is a commit whose title begins with
> -	the same ..., automatically modify the todo list of rebase -i
> +	the same "..." message, or a commit object name (standalone),
> +	automatically modify the todo list of rebase -i

What's "(standalone)"?  I can understand the updated text and agree
that it is better than the original without that part, though.

>  	so that the commit marked for squashing comes right after the
>  	commit to be modified, and change the action of the moved
>  	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint
  2016-08-14 21:46 ` [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint Philip Oakley
@ 2016-08-14 22:20   ` Junio C Hamano
  2016-08-14 23:23     ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2016-08-14 22:20 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

Philip Oakley <philipoakley@iee.org> writes:

> 22c5b13 (rebase -i: handle fixup! fixup! in --autosquash, 2013-06-27)

That's a noun that names a commit object.  What about it?

> I certainly misunderstood what this meant. It sounded like only one fixup! was
> allowed per commit (i.e. one mistake) - fixing two mistakes wouldn't be
> allowed. Hindsight is a wonderful thing.

I think this is about

	git commit -m foo
	... some other commits
        git commit -m 'fixup! foo'
	... even more other commits
        git commit -m 'fixup! fixup! foo'

The last one is technically a fixup of the second one, but as long
as the second one is to be applied _and_ the last one comes after
the second one, we could say that both of them are fix-ups for the
very first one.  And that is what the original text says.

I have a feeling that if the editor session to edit the todo drops
the second one and leaves this:

	pick foo
        fixup fixup! fixup! foo

the command _should_ notice that the second one that is required to
apply the last one is gone and error out, but probably the code does
not do so, and if that is the case, I think "they fix the very first
commit that invited these fixups" is a more reasonable description
than the more technically stringent "fixup! fixup! is to fix what
the fixup! did", which is not what the code implements.

> Also, does 'earliest commit requiring fixup/squash' fully convey that
> its the one to fix.

I cannot tell if that a question or a statement?

> ---
>  Documentation/git-rebase.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 66b789a..91eb107 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -425,9 +425,9 @@ without an explicit `--interactive`.
>  	automatically modify the todo list of rebase -i
>  	so that the commit marked for squashing comes right after the
>  	commit to be modified, and change the action of the moved
> -	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
> -	"fixup! " or "squash! " after the first, in case you referred to an
> -	earlier fixup/squash with `git commit --fixup/--squash`.
> +	commit from `pick` to `squash` (or `fixup`).  Commits with repeated
> +	"fixup! " or "squash! " in the subject line are considered to refer
> +	to the earliest commit requiring fixup/squash.
>  +
>  This option is only valid when the '--interactive' option is used.
>  +

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 22:09   ` Junio C Hamano
@ 2016-08-14 22:45     ` Philip Oakley
  2016-08-14 22:55       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 22:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: GitList

From: "Junio C Hamano" <gitster@pobox.com>
> Philip Oakley <philipoakley@iee.org> writes:
>> Be clearer that the --fixup/--squash options can take any of the
>> gitrevisions methods of specifying a commit, not just a 'hash'.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.org>
>> ---
>> ...
>> @@ -81,15 +81,15 @@ OPTIONS
>>  --fixup=<commit>::
>>  Construct a commit message for use with `rebase --autosquash`.
>>  The commit message will be the subject line from the specified
>> - commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
>> - for details.
>> + commit revision with a prefix of "fixup! ".  See linkgit:git-rebase[1]
>> + and linkgit:gitrevisions[7] for details.
>
> The same comment applies to the other hunk, but rephrasing "commit"
> with "commit revision" (the latter is not even in the glossary) does
> not make it clearer at all.  Especially when discussing rebases and
> anything that rewrites commits, it can easily be mistaken as if you
> are talking about v2 of the commit by fixing up the original, but
> that is not the impression you want to give.

Hmm, had to read that a few times before I saw what you meant regarding 'v2' 
as the revised commit.

>
> "The specified commit" is clear enough.  It may be debatable if we
> want to talk about "how" to specify the commit, though.

Exactly. The latter.

> I think the
> use of "commit" in an angle-bracket-pair in the label for the
> section, i.e. "--fixup=<commit>", has been considered to be clear
> enough to tell that you can use usual extended SHA-1 syntax to
> specify the commit you want to talk about,

I certainly hadn't picked up on that ability to use the extended sha1 syntax 
(specifying revisions...) here.

Part of the issue is that the whole fixup/squash capability is buried within 
just two documents as asides [1], and in the place it's spelt out (in 
rebase) it talks about the commit message being used, which is just part of 
the confusion.

>  but if so, that is not
> limited to this entry, and I do not think this description or the
> other one for the "--squash" option are particularly worse than
> those for the "-c" and "-C" options.  The description for "-c" does
> say "Take an existing commit object", but that's like "the specified
> commit" used here.
>
OK
--
Philip
[1] just looked at the new Progit version and fixup/squash is not even 
mentioned. 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 22:45     ` Philip Oakley
@ 2016-08-14 22:55       ` Junio C Hamano
  2016-08-14 23:29         ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2016-08-14 22:55 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

"Philip Oakley" <philipoakley@iee.org> writes:

>> I think the
>> use of "commit" in an angle-bracket-pair in the label for the
>> section, i.e. "--fixup=<commit>", has been considered to be clear
>> enough to tell that you can use usual extended SHA-1 syntax to
>> specify the commit you want to talk about,
>
> I certainly hadn't picked up on that ability to use the extended sha1
> syntax (specifying revisions...) here.

By "has been considered", I meant that the documentation text is
still open for improvement.  I just didn't find rewording "commit"
with "commit revision" is that improvement we need there.

Perhaps we need to have somewhere central a section that explains
various notations used in the documentation set.  I think it is safe
to say something like "unless otherwise qualified, <commit> (or any
object type in an angle-bracket-pair) is used as a placeholder to
take any acceptable way to spell object names (cf. gitrevisions for
details)" these days [*1*].


[Footnote]

*1* In ancient days I think some plumbing commands only took 40-hex
object names, but as far as I know they've all been updated.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/3] doc: rebase: fixup! can take an object name
  2016-08-14 22:11   ` Junio C Hamano
@ 2016-08-14 23:00     ` Philip Oakley
  2016-08-14 23:02       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 23:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: GitList

From: "Junio C Hamano" <gitster@pobox.com>
> Philip Oakley <philipoakley@iee.org> writes:
>
>> Since 68d5d03 (rebase: teach --autosquash to match on sha1 in addition
>> to message, 2010-11-04) the commit subject can refer directly to the
>> destination object hash as a single word.)...
>
> That's not an object hash but an object name (see glossary); you got
> it right in the actual patch text, though ;-).

As noted in 1/3, the glossary also needs a fix (for those who read it) to 
make sure the name-value confusion is distinguished, with the potential 
'object names' (colloquial)  being via (git) 'revisions', while  the 
cannonical object name is the (oid) sha1 hash. Somehow folk need pointing at 
the (broad) ways of spelling commit names rather than just the narrow sha. A 
bit of a finger and the moon problem.

>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 0387b40..66b789a 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -421,7 +421,8 @@ without an explicit `--interactive`.
>>  --no-autosquash::
>>  When the commit log message begins with "squash! ..." (or
>>  "fixup! ..."), and there is a commit whose title begins with
>> - the same ..., automatically modify the todo list of rebase -i
>> + the same "..." message, or a commit object name (standalone),
>> + automatically modify the todo list of rebase -i
>
> What's "(standalone)"?  I can understand the updated text and agree
> that it is better than the original without that part, though.

the 'standalone' is that it must be a single (standalone) word on the 
subject line immediately after the "fixup! "(s).

communicationg that one cannot have any extra textual notes after that word 
was the issue that 'standalone' tried to address.

--
Philip 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/3] doc: rebase: fixup! can take an object name
  2016-08-14 23:00     ` Philip Oakley
@ 2016-08-14 23:02       ` Junio C Hamano
  2016-08-14 23:30         ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2016-08-14 23:02 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

"Philip Oakley" <philipoakley@iee.org> writes:

> the 'standalone' is that it must be a single (standalone) word on the
> subject line immediately after the "fixup! "(s).
>
> communicationg that one cannot have any extra textual notes after that
> word was the issue that 'standalone' tried to address.

Perhaps I am slow but did you mean the same thing as "it must be a
single word and nothing else on the remainder of the line"?


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint
  2016-08-14 22:20   ` Junio C Hamano
@ 2016-08-14 23:23     ` Philip Oakley
  2016-08-15 15:42       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 23:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: GitList

From: "Junio C Hamano" <gitster@pobox.com>
> Philip Oakley <philipoakley@iee.org> writes:
>
>> 22c5b13 (rebase -i: handle fixup! fixup! in --autosquash, 2013-06-27)
>
> That's a noun that names a commit object.  What about it?

It's the commit that introduced the original change that's being clarified. 
Is a paragraph needed or simply "Ref commit: .."

>
>> I certainly misunderstood what this meant. It sounded like only one 
>> fixup! was
>> allowed per commit (i.e. one mistake) - fixing two mistakes wouldn't be
>> allowed. Hindsight is a wonderful thing.
>
> I think this is about

Yes, but the original wording didn't make me think that.

>
> git commit -m foo
> ... some other commits
>        git commit -m 'fixup! foo'
> ... even more other commits
>        git commit -m 'fixup! fixup! foo'
>
> The last one is technically a fixup of the second one, but as long
> as the second one is to be applied _and_ the last one comes after
> the second one, we could say that both of them are fix-ups for the
> very first one.  And that is what the original text says.

No. To me it said "You can only have one fixup (commit) for any given 
commit". So that's one original, and one fixup. After that the autosquash 
would leave the remaining fixup commits in their original positions in the 
commit sequence. Hence the patch.

>
> I have a feeling that if the editor session to edit the todo drops
> the second one and leaves this:
>
> pick foo
>        fixup fixup! fixup! foo
>
> the command _should_ notice that the second one that is required to
> apply the last one is gone and error out, but probably the code does
> not do so, and if that is the case, I think "they fix the very first
> commit that invited these fixups" is a more reasonable description
> than the more technically stringent "fixup! fixup! is to fix what
> the fixup! did", which is not what the code implements.
>
>> Also, does 'earliest commit requiring fixup/squash' fully convey that
>> its the one to fix.
>
> I cannot tell if that a question or a statement?

It's a question. In your prior para you offer "they fix the very first
> commit that invited these fixups" as an alternate.
It's when a users mental model is that they got their first fixup wrong and 
it's that fixup they are correcting, and later they add different fixups to 
the orignal that it all gets hairy.
(diffs must have the right sequence, while snapshots don't care - so if we 
keep the diff sequence, we don't care about the user's mental model as the 
end results are the same).

The writeup needs to cope with the mental model rather than the end result.

It maybe that the whole 'fixup/squash' capability should be brought out as a 
small separate section to give space to these points and the user 
understanding.
>
>> ---
>>  Documentation/git-rebase.txt | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 66b789a..91eb107 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -425,9 +425,9 @@ without an explicit `--interactive`.
>>  automatically modify the todo list of rebase -i
>>  so that the commit marked for squashing comes right after the
>>  commit to be modified, and change the action of the moved
>> - commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
>> - "fixup! " or "squash! " after the first, in case you referred to an
>> - earlier fixup/squash with `git commit --fixup/--squash`.
>> + commit from `pick` to `squash` (or `fixup`).  Commits with repeated
>> + "fixup! " or "squash! " in the subject line are considered to refer
>> + to the earliest commit requiring fixup/squash.
>>  +
>>  This option is only valid when the '--interactive' option is used.
>>  +
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 22:55       ` Junio C Hamano
@ 2016-08-14 23:29         ` Philip Oakley
  2016-08-16 22:11           ` Philip Oakley
  0 siblings, 1 reply; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 23:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: GitList

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>> I think the
>>> use of "commit" in an angle-bracket-pair in the label for the
>>> section, i.e. "--fixup=<commit>", has been considered to be clear
>>> enough to tell that you can use usual extended SHA-1 syntax to
>>> specify the commit you want to talk about,
>>
>> I certainly hadn't picked up on that ability to use the extended sha1
>> syntax (specifying revisions...) here.
>
> By "has been considered", I meant that the documentation text is
> still open for improvement.  I just didn't find rewording "commit"
> with "commit revision" is that improvement we need there.
>
> Perhaps we need to have somewhere central a section that explains
> various notations used in the documentation set.  I think it is safe
> to say something like "unless otherwise qualified, <commit> (or any
> object type in an angle-bracket-pair) is used as a placeholder to
> take any acceptable way to spell object names (cf. gitrevisions for
> details)" these days [*1*].

True. I'm cautious that we may accidentally still hide it in another 
document that the user doesn't see when reading "this" (or any other)  man 
page.

Your sentence is short enough to be added to those few key pages that users 
refer to to get them started in the right direction.

> *1* In ancient days I think some plumbing commands only took 40-hex
> object names, but as far as I know they've all been updated.
> --
Philip 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/3] doc: rebase: fixup! can take an object name
  2016-08-14 23:02       ` Junio C Hamano
@ 2016-08-14 23:30         ` Philip Oakley
  0 siblings, 0 replies; 16+ messages in thread
From: Philip Oakley @ 2016-08-14 23:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: GitList

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
> 
>> the 'standalone' is that it must be a single (standalone) word on the
>> subject line immediately after the "fixup! "(s).
>>
>> communicationg that one cannot have any extra textual notes after that
>> word was the issue that 'standalone' tried to address.
> 
> Perhaps I am slow but did you mean the same thing as "it must be a
> single word and nothing else on the remainder of the line"?

Yes.

--
Philip

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint
  2016-08-14 23:23     ` Philip Oakley
@ 2016-08-15 15:42       ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2016-08-15 15:42 UTC (permalink / raw)
  To: Philip Oakley; +Cc: GitList

"Philip Oakley" <philipoakley@iee.org> writes:

>> I think this is about
>
> Yes, but the original wording didn't make me think that.

Yeah, it is very plausible that it is not limited to you, and I
agree that it is worthwhile to update the description around here.

>>> Also, does 'earliest commit requiring fixup/squash' fully convey that
>>> its the one to fix.
>>
>> I cannot tell if that a question or a statement?
>
> It's a question. In your prior para you offer "they fix the very first
> commit that invited these fixups" as an alternate.

I think both are equally understandable to me (but I am not the
primary target audience).

> It's when a users mental model is that they got their first fixup
> wrong and it's that fixup they are correcting, and later they add
> different fixups to the orignal that it all gets hairy.
> (diffs must have the right sequence, while snapshots don't care - so
> if we keep the diff sequence, we don't care about the user's mental
> model as the end results are the same).
>
> The writeup needs to cope with the mental model rather than the end result.

Yes, yes.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision
  2016-08-14 23:29         ` Philip Oakley
@ 2016-08-16 22:11           ` Philip Oakley
  0 siblings, 0 replies; 16+ messages in thread
From: Philip Oakley @ 2016-08-16 22:11 UTC (permalink / raw)
  To: Philip Oakley, Junio C Hamano; +Cc: GitList

From: "Philip Oakley" <philipoakley@iee.org>
> From: "Junio C Hamano" <gitster@pobox.com>
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>>>> I think the
>>>> use of "commit" in an angle-bracket-pair in the label for the
>>>> section, i.e. "--fixup=<commit>", has been considered to be clear
>>>> enough to tell that you can use usual extended SHA-1 syntax to
>>>> specify the commit you want to talk about,
>>>
>>> I certainly hadn't picked up on that ability to use the extended sha1
>>> syntax (specifying revisions...) here.
>>
>> By "has been considered", I meant that the documentation text is
>> still open for improvement.  I just didn't find rewording "commit"
>> with "commit revision" is that improvement we need there.
>>
>> Perhaps we need to have somewhere central a section that explains
>> various notations used in the documentation set.  I think it is safe
>> to say something like "unless otherwise qualified, <commit> (or any
>> object type in an angle-bracket-pair) is used as a placeholder to
>> take any acceptable way to spell object names (cf. gitrevisions for
>> details)" these days [*1*].
>
> True. I'm cautious that we may accidentally still hide it in another 
> document that the user doesn't see when reading "this" (or any other)  man 
> page.
>
> Your sentence is short enough to be added to those few key pages that 
> users refer to to get them started in the right direction.
>
>> *1* In ancient days I think some plumbing commands only took 40-hex
>> object names, but as far as I know they've all been updated.
>> --

I'll take the patch series away and have a rework over the coming week or 
so.

Philip 


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-08-16 22:11 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-14 21:46 [PATCH v1 0/3] fixup fixup documenation Philip Oakley
2016-08-14 21:46 ` [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision Philip Oakley
2016-08-14 22:09   ` Junio C Hamano
2016-08-14 22:45     ` Philip Oakley
2016-08-14 22:55       ` Junio C Hamano
2016-08-14 23:29         ` Philip Oakley
2016-08-16 22:11           ` Philip Oakley
2016-08-14 21:46 ` [PATCH v1 2/3] doc: rebase: fixup! can take an object name Philip Oakley
2016-08-14 22:11   ` Junio C Hamano
2016-08-14 23:00     ` Philip Oakley
2016-08-14 23:02       ` Junio C Hamano
2016-08-14 23:30         ` Philip Oakley
2016-08-14 21:46 ` [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint Philip Oakley
2016-08-14 22:20   ` Junio C Hamano
2016-08-14 23:23     ` Philip Oakley
2016-08-15 15:42       ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).