patchrebase --autosquash: fix a potential segfault

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`.

The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item.

To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `last[i]`.

The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain.

However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain.

Example:

	* 0192a To fixup
	* 02f12 fixup! To fixup
	* 03763 fixup! To fixup
	* 04ecb fixup! 02f12

Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit.

The good news is that it is easy to fix this: we can detect the situation by looking at `last[i2]` (which will be `-1` if `i2` is actually in the middle of a fixup chain), and in that case we simply need to squeeze the current item into the middle of the `next` chain, without touching `last` (i.e. leaving the end index of the fixup chain alone).

Reported-by: Paul Ganssle <paul@ganssle.io>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    rebase --autosquash: fix a potential segfault
    
    This patch is in response to 
    https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/
    .

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/625

 sequencer.c                  | 11 ++++++++++-
 t/t3415-rebase-autosquash.sh | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

OK, this would catch the case even when fixing up a fix-up of antoher fix-up, so we won't need further "else if" in the future ;-)

I suspect that this breakage is as old as 2.14, introduced by c44a4c65 (rebase -i: rearrange fixup/squash lines using the rebase--helper, 2017-07-14), but perhaps we won't need to backmerge the fix that far. We don't even backport security fixes beyond 2.17 (which is two years old).

Just in case I'll queue this immediately on top of f2a04904 (sequencer: refactor rearrange_squash() to work on a todo_list, 2019-03-05); that would give us a potential to cover as far back to 2.20 series.

Thanks.

On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote:

s/last/tail/, I think? (and below)

OK, good. I definitely had figured out how to detect the case, but wasn't quite sure how to manipulate next.

But your fix here makes sense:

I do have one question, though. What happens if we add a second fixup-of-a-fixup?

We'd see its "next" slot filled, but now pointing to the first fixup-of-a-fixup. And we'd add ourselves at the front of that list. So I think:

  1234 foo
  5678 !fixup foo
  abcd !fixup 5678
  dbaf !fixup 5678

would end up reordering abcd and dbaf (putting dbaf first), wouldn't it?

But when I tested it doesn't seem to:

  git init
  git commit -m base --allow-empty
  git commit --squash HEAD -m 'this is the first squash' --allow-empty
  s=$(git rev-parse HEAD)
  git commit -m "squash! $s" -m 'this is the second squash' --allow-empty
  git commit -m "squash! $s" -m 'this is the third squash' --allow-empty
  git rebase -ki --autosquash --root

So I think there's something I don't quite understand about how the chain of "next" works. If you can enlighten me, I'd be grateful.

But your patch does seem to work as advertised. It might be worth adding the double-squash-of-squash to the test.

-Peff

On Mon, May 04, 2020 at 05:33:26PM -0400, Jeff King wrote:

Thanks for asking this question, I was a little curious about it, too.

Ditto.

Yes, I think that this is a good, worthwhile addition to the patch. Sorry Johannes for suggesting that you do more work on an already-great patch. No good deed goes unpunished, I guess ;).

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Interesting that three people looked at the same patch and asked the same question in different ways ;-)

Hi Junio,

On Tue, 5 May 2020, Junio C Hamano wrote:

Indeed!

I am very grateful, as I had missed that, and it helped me figure out a better way to do it, and v2 looks a lot nicer, too.

Ciao, Dscho

On Wed, May 06, 2020 at 11:35:48PM +0200, Johannes Schindelin wrote:

OK, so your v2 addresses that. Does that mean it was broken in v1? If so, then why didn't my test reveal it?

I'm not really doubting that your v2 works so much as trying to un-confuse myself about the whole situation (which in turn might lead to a more intelligent review).

-Peff

Hi Peff,

On Thu, 7 May 2020, Jeff King wrote:

Yes.

Let's disect this:

 i  hash oneline
#0  1234 foo
#1  5678 !fixup foo
#2  abcd !fixup 5678
#3  dbaf !fixup 5678

Let's follow the original code, i.e. before my v1:

When #1 is processed, i.e. when `i == 1`, it finds `i2 == 0` as target. So it sets `next[0]` as well as `tail[0]` to 1.

Then #2 is processed, i.e. `i == 2`, and it finds `i2 == 1` as target. It sets `next[1]` as well as `tail[1]` to 2.

Now #3 is processed, i.e. it also finds `i2 == 1` as target, so it looks at next[1], sees that it is already non-negative, so it sets `next[tail[1]]`, i.e. `next[2]` to 3. It also sets `tail[1]` to 3, but nobody cares about that because there is no further todo command.

Now, let's follow the code with my v1:

It actually does the same as before! Why, you ask? Because at no stage is there any non-negative `next[j]` whose corresponding `tail[j]` is negative. (Except after #3 was processed, at that stage, `next[2]` is non-negative but `tail[2]` still is negative, but as I said, noone cares because there are no remaining todo commands.)

So the crucial part to trigger this bug is to have a regular `fixup! <oneline>` _between_ the `fixup! <oneline>` and the `fixup! <hash>` targeting the latter. So I think I can modify your example accordingly:

	1234 foo
	5678 fixup! foo
	90ab fixup! foo
	abcd fixup! 5678
	dbaf fixup! 5678

Or using your actual shell commands:

  git commit -m base --allow-empty
  git commit --squash HEAD -m 'this is the first squash' --allow-empty
  s=$(git rev-parse HEAD)
  git commit --fixup HEAD^ --allow-empty # This is the crucial command
  git commit -m "squash! $s" -m 'this is the second squash' --allow-empty
  git commit -m "squash! $s" -m 'this is the third squash' --allow-empty
  git rebase -ki --autosquash --root

Note the cricual command `git commit --fixup HEAD^`. When processing that, `i == 2` and `i2 == 0` (as for `i == 1`), and before v1, this would have set `next[1]` but `tail[0]`! With v1, this would have led to #4 and #5 being exchanged. With v2, the role of `tail` would have been extended to not only talk about the beginning of a fixup/squash chain, but about _any_ target of a fixup/squash, even if it is in the middle of a chain.

So why does this work? Why does it still do the right thing _even after_ inserting a fixup in the middle of a chain?

That's the beauty: if I insert anything in the middle of it, the `tail` of the actual beginning of the fixup/squash chain won't need to be changed. It still points to the end of that chain.

All I need to ensure is that item `i` is not just appended to the "chain" starting at `i2`, but that it is _inserted_ at the end of that chain in case that it is actually part of a larger chain, i.e. that its `next[i]` is set correctly before making it the immediate successor of the target commit. Since all of the elements in `next` and `tail` are initialized to `-1` (i.e. "no next fixup/squash item after this"), it will even do the right thing when it should actually append: it will set `next[i]` to `-1` in that case.

I wish I was quicker in my responses because I think that this is really helpful a conversation. By "forcing my hand" on a thorough explanation, you really help me get clarity for myself about the actual underlying issues. So even if I still think that v2 is correct after writing up the above explanation, the degree of my confidence increased substantially.

Thanks, Dscho

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`.

The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item.

To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `tail[i]`.

The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain.

However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain.

Example:

	* 0192a To fixup
	* 02f12 fixup! To fixup
	* 03763 fixup! To fixup
	* 04ecb fixup! 02f12

Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit.

The good news is that it is easy to fix this: we use the correct condition (we now possibly set `tail[i2]` even for fixups in the middle) and we _also_ have to ensure that we _insert_ the item rather than _append_ it, i.e. we need to set `next[i2]` accordingly (it might still be set to `-1` if it was actually appended).

Reported-by: Paul Ganssle <paul@ganssle.io>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    rebase --autosquash: fix a potential segfault
    
    This patch is in response to 
    https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/
    .
    
    Changes since v1:
    
     * Fixed the order of two or more fixups-of-fixups (oddly enough, this
       simplified the patch...)
     * Extended the test to have two fixups-of-fixups, ensuring their order
       is preserved.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/625

Range-diff vs v1:

 1:  bb820acc342 ! 1:  de029422324 rebase --autosquash: fix a potential segfault
     @@ Commit message
          index of the item that should be rearranged just after the `i`th item.
      
          To avoid having to walk the entire `next` chain when appending another
     -    fixup/squash, we also store the end of the `next` chain in `last[i]`.
     +    fixup/squash, we also store the end of the `next` chain in `tail[i]`.
      
          The logic we currently use to update these array items is based on the
          assumption that given a fixup/squash item at index `i`, we just found
     @@ Commit message
          Note how the fourth commit targets the second commit, which is already a
          fixup that targets the first commit.
      
     -    The good news is that it is easy to fix this: we can detect the
     -    situation by looking at `last[i2]` (which will be `-1` if `i2` is
     -    actually in the middle of a fixup chain), and in that case we simply
     -    need to squeeze the current item into the middle of the `next` chain,
     -    without touching `last` (i.e. leaving the end index of the fixup chain
     -    alone).
     +    The good news is that it is easy to fix this: we use the correct
     +    condition (we now possibly set `tail[i2]` even for fixups in the middle)
     +    and we _also_ have to ensure that we _insert_ the item rather than
     +    _append_ it, i.e. we need to set `next[i2]` accordingly (it might still
     +    be set to `-1` if it was actually appended).
      
          Reported-by: Paul Ganssle <paul@ganssle.io>
          Helped-by: Jeff King <peff@peff.net>
     @@ Commit message
      
       ## sequencer.c ##
      @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
     + 			todo_list->items[i].command =
     + 				starts_with(subject, "fixup!") ?
       				TODO_FIXUP : TODO_SQUASH;
     - 			if (next[i2] < 0)
     +-			if (next[i2] < 0)
     ++			if (tail[i2] < 0) {
     ++				next[i] = next[i2];
       				next[i2] = i;
      -			else
     -+			else if (tail[i2] >= 0)
     ++			} else {
     ++				next[i] = next[tail[i2]];
       				next[tail[i2]] = i;
     -+			else {
     -+				/*
     -+				 * i2 refers to a fixup commit in the middle of
     -+				 * a fixup chain
     -+				 */
     -+				next[i] = next[i2];
     -+				next[i2] = i;
     -+				continue;
      +			}
       			tail[i2] = i;
       		} else if (!hashmap_get_from_hash(&subject2item,
     @@ t/t3415-rebase-autosquash.sh: test_expect_success 'abort last squash' '
      +	git commit --squash HEAD^ -m Y --allow-empty &&
      +	test_tick &&
      +	git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty &&
     -+	git rebase -ki --autosquash HEAD~4 &&
     -+	test XZY = $(git show | tr -cd X-Z)
     ++	test_tick &&
     ++	git commit -m "squash! $(git rev-parse HEAD^^)" -m W --allow-empty &&
     ++	git rebase -ki --autosquash HEAD~5 &&
     ++	test XZWY = $(git show | tr -cd W-Z)
      +'
      +
       test_done

 sequencer.c                  |  7 +++++--
 t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`.

The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item.

To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `tail[i]`.

The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain.

However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain.

Example:

	* 0192a To fixup
	* 02f12 fixup! To fixup
	* 03763 fixup! To fixup
	* 04ecb fixup! 02f12

Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit.

Previously, we would update `next` and `tail` under our assumption that every `fixup!` commit would find the start of the `fixup!`/`squash!` chain. This would lead to a segmentation fault because we would actually end up with a `next[i]` pointing to a `fixup!` but the corresponding `tail[i]` pointing nowhere, which would the lead to a segmentation fault.

Let's fix this by _inserting_, rather than _appending_, the item. In other words, if we make a given line successor of another line, we do not simply forget any previously set successor of the latter, but make it a successor of the former.

In the above example, at the point when we insert 04ecb just after 02f12, 03763 would already be recorded as a successor of 04ecb, and we now "squeeze in" 04ecb.

To complete the idea, we now no longer assume that `next[i]` pointing to a line means that `last[i]` points to a line, too. Instead, we extend the concept of `last` to cover also partial `fixup!`/`squash!` chains, i.e. chains starting in the middle of a larger such chain.

In the above example, after processing all lines, `last[0]` (corresponding to 0192a) would point to 03763, which indeed is the end of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12) would point to 04ecb (which is the last `fixup!` targeting 02f12, but it has 03763 as successor, i.e. it is not the end of overall `fixup!` chain).

Reported-by: Paul Ganssle <paul@ganssle.io>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    rebase --autosquash: fix a potential segfault
    
    This patch is in response to 
    https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/
    .
    
    Changes since v2:
    
     * Explained the fix more verbosely in the commit message.
    
    Changes since v1:
    
     * Fixed the order of two or more fixups-of-fixups (oddly enough, this
       simplified the patch...)
     * Extended the test to have two fixups-of-fixups, ensuring their order
       is preserved.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/625

Range-diff vs v2:

 1:  de029422324 ! 1:  c1c4607da0e rebase --autosquash: fix a potential segfault
     @@ Commit message
          Note how the fourth commit targets the second commit, which is already a
          fixup that targets the first commit.
      
     -    The good news is that it is easy to fix this: we use the correct
     -    condition (we now possibly set `tail[i2]` even for fixups in the middle)
     -    and we _also_ have to ensure that we _insert_ the item rather than
     -    _append_ it, i.e. we need to set `next[i2]` accordingly (it might still
     -    be set to `-1` if it was actually appended).
     +    Previously, we would update `next` and `tail` under our assumption that
     +    every `fixup!` commit would find the start of the `fixup!`/`squash!`
     +    chain. This would lead to a segmentation fault because we would actually
     +    end up with a `next[i]` pointing to a `fixup!` but the corresponding
     +    `tail[i]` pointing nowhere, which would the lead to a segmentation
     +    fault.
     +
     +    Let's fix this by _inserting_, rather than _appending_, the item. In
     +    other words, if we make a given line successor of another line, we do
     +    not simply forget any previously set successor of the latter, but make
     +    it a successor of the former.
     +
     +    In the above example, at the point when we insert 04ecb just after
     +    02f12, 03763 would already be recorded as a successor of 04ecb, and we
     +    now "squeeze in" 04ecb.
     +
     +    To complete the idea, we now no longer assume that `next[i]` pointing to
     +    a line means that `last[i]` points to a line, too. Instead, we extend
     +    the concept of `last` to cover also partial `fixup!`/`squash!` chains,
     +    i.e. chains starting in the middle of a larger such chain.
     +
     +    In the above example, after processing all lines, `last[0]`
     +    (corresponding to 0192a) would point to 03763, which indeed is the end
     +    of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12)
     +    would point to 04ecb (which is the last `fixup!` targeting 02f12, but it
     +    has 03763 as successor, i.e. it is not the end of overall `fixup!`
     +    chain).
      
          Reported-by: Paul Ganssle <paul@ganssle.io>
          Helped-by: Jeff King <peff@peff.net>

 sequencer.c                  |  7 +++++--
 t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote:

Could you please clarify if I'm understanding this correctly: does this affect the fixups-of-a-fixup which were created by

	git commit --fixup=<pointer to previous fixup! commit>

? For example:

	* 0192a To fixup	
	* 02f12 fixup! To fixup	
	* 03763 fixup! To fixup	
	* 04ecb fixup! fixup! To fixup

Where 04ecb was created by pointing option --fixup at 02f12.

Hi Andrei,

On Wed, 6 May 2020, Andrei Rybak wrote:

No, it only affects commits whose oneline (i.e. the first line of the commit message) is `fixup! <commit-hash>`.

Ciao, Johannes

On 07/05/2020 15:27, Johannes Schindelin wrote:

Is this ability to have a commit message `fixup! <commit-hash>` documented?

I've looked a few times in the past and didn't find it. The docs for `git commit --fixup=` doesn't put the oid in the commit's subject line, rather it puts the subject of the referent commit after the "fixup! ".

Searching from a different direction I've just seen it is mentioned in the v1.7.4 release notes.

Would a doc fix to clarify this be appropriate or have I missed something?

Philip

On 2020-05-08 18:43, Philip Oakley wrote:

Yes, it's documented in description of --autosquash: "A commit matches the `...` if the commit subject matches, or if the `...` refers to the commit's hash."

Hi Andrei, On 08/05/2020 17:57, Andrei Rybak wrote:

I've never read it that way, especially given the `git commit fixup=` description. That one strongly suggests that one starts with the subject and then finds the commit id from that.

We do see in the side discussion that the todo list uses the oid, rather than the subject, which is given verbatim, but the docs are rather opaque on the question of oid vs subject. 

If it is only the second guessing of the meaning of those three dots then maybe the docs do need a  bit of clarification.

Philip

Hi all,

On 08/05/2020 17:57, Andrei Rybak wrote:

The docs don't  clarify if a full oid has is required, or a unique abbreviation within the repository, or just unique within the rebase instruction sheet.

I'd presume that all it needed was the latter, but could easily expect the former (full oid) to be 'matching', while the mid ground may be fairly simple to code...

Philip

Hi Philip,

On Mon, 18 May 2020, Philip Oakley wrote:

It's even worse: _any_ valid reference will do. As you can see from https://github.com/git/git/blob/efcab5b7a3d2/sequencer.c#L5359-L5381, the search goes like this:

  - For the remainder of the `fixup! <remainder>` line:

	1. If it is identical to the oneline of any commit mentioned in a
	   previously-seen `pick` line, pick that as target.

	2. Otherwise, if the remainder can be looked up as a commit
	   (think: `fixup! master~3`) _and_ that commit was mentioned in
	   a previously-seen `pick` line, pick that as target.

	3. If all else fails, and if the remainder is the _start_ of a
	   oneline of a commit previously seen in a `pick` line, pick that
	   as target (if multiple lines match, use the first one).

Do feel free to put that into a native-speaker form of a patch to improve the documentation.

Ciao, Dscho

Hi Dscho,

On 18/05/2020 04:27, Johannes Schindelin wrote:

Sorry for the delay on a reply to this one.  I do have a small couple of patches to slightly improve the docs. Hope to send soon.

I'm thinking that for the longer term it may need a section covering fixup/squash, so as to cover all the different user interaction stages, e.g. the commit options, and commit message; and then the initial interactive instruction sheet, and on-going edits of the instruction sheet when the rebase pauses.

In particular, the (assuming proper understanding) the interjection between the almost identical 1 & 3  [identical vs start of the oneline of a commit in the `pick` (insn) list...], of the ability to specify an almost arbitrary rev. I still have to check the code to see what it does/tries to do.

Philip 

It's easy to think that all fixup! commit messages must match a previous subject line based on commit's --fixup option. It had this developer confused for many years.

Lets at least do a some small changes to make the use of the commit hash in the fixup message more obvious.

Philip

In-Reply-To: 9a9e7432-7a74-f46e-9a77-b8acaa9a974f@iee.email
To: git@vger.kernel.org
cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
cc: Andrei Rybak <rybak.a.v@gmail.com>
cc: Paul Ganssle <paul@ganssle.io>
cc: Jeff King <peff@peff.net>

Philip Oakley (2):
  doc: fixup/squash: clarify use of <oid-hash> in subject line
  doc: fixup/squash: remove ellipsis marks, use <line> for clarify

 Documentation/git-rebase.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Ellipsis marks fail to hint at the typoe or style of the missing content.

Tell the reader what is missing, for easier comprehension.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---

The fixup/squash process could probably benefit from its own section
as there are many places for user interaction with the process.
This is a minimal shift toward such an improvement.
---
 Documentation/git-rebase.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

The option to use the oid hash is buried deep within the fixup/squash documenation. Split the paragraph so that the option choice is more obvious.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
The use of ellision `...` isn't great, as it gives no hint or clue,
leaving the subsequent test with a difficult explanation.

Clarify if a full oid has is required, or a unique abbreviation within the respository, or just uniques within the rebase instruction?

This is a minimal change that sidesteps the chance to rewrite/clarify
the potential wider confusions over specifying the <commit> being
referred to in the fixup/squash process.
---
 Documentation/git-rebase.txt | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Philip Oakley <philipoakley@iee.email> writes:

True. If you are planning to correct it in 2/2, then I think it makes more sense to squash that in to have a single patch.

Puzzled. You must know the answer to "do we need a full object name, or is it sufficient to have anything that gives us a unique commit object name?" so why not write it in the patch instead of asking the question here? Or do you not know the answer and this is a RFC/WIP patch????

Hmph. So this step cannot be reviewed to judge if it is a good change by itself?

Let me locally recreate a squashed single patch and review _that_ instead.

Overall it looks much better than the original.

The original did not even attempt to define what is a "match" for the purpose of this option, so the ellipses may have been OK, but once we need to refer to what is there, we need a name to refer to it and ellipses no longer are sufficient, and using the step 1/2 alone would not make any sense. We definitely should take the step 2/2 together with it.

"A commit matches the <line> if the commit subject matches" is not a great definition of what a "match" is, though. The readers are left in the same darkness about what constitutes a "match" of <line> against "the commit subject". If you define this "subject matches" as a substring match, for example, you do not even have to say "as a fall-back"---it is by (the updated version of your) definition that how the commit subject and <line> matches so there is no need to allow any fall-back involved.

On 27/05/2020 18:35, Junio C Hamano wrote:

OK

This was a left over note about deeper questions outside of this patch series.

I was working on 'small incremental steps' here.

I'd taken the idea of being able to name the thing as step 1, to get past the Newspeak problem.

The fall back does include the commit hash, and (not yet in this series) is the extra information that Dscho provided at [1], so it's not a simple substring match, nor partial string match. Part of this reader confusion comes out of the `commit --fixup` option that effectively directs the reader away from using a hash, to using the target's full commit message for the fixup! line.

At this stage, the aim is to make the option for the use of the commit hash a bit more visible within the text. Even after many years of reading, it still didn't stand out in the old 'wall of text', hence the all important paragraph break

I'll combine the two patches at this stage.

Philip

[1] https://public-inbox.org/git/nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet/     "It's even worse"