Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase

> 
>> It is not yet clear to me if we want to _always_ discard a note from
>> a commit that would become "empty" during a rebase session (in other
>> words, a commit that becomes empty during a rebase is _always_ a
>> sign that the change it brings in is _already_ in the new base of
>> the rebase
> 
> Yeah, or in a patch that was picked before.
> 
>> and the necessary information the note wanted to carry to
>> the target branch is there without need to _duplicate_ it by copying
>> the note).  But assuming that we want the behaviour, the code change
>> to sequencer.c looks very reasonable to me, except for one thing that
>> I am not clear about.
> 
> I think given the commit goes away, it's natural that the note goes
> away, too. And to come back to your question above: I think it doesn't
> need documentation, that if a commit disappears its notes go away, too.
> But that might be subjective?!

>>> diff --git a/sequencer.c b/sequencer.c
>>> index 57855b0066ac..da2185a37c5d 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> ...
>>> @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,
>>>   		return error_with_patch(r, commit,
>>>   					arg, item->arg_len, opts, res, !res);
>>>   	}
>>> -	if (is_rebase_i(opts) && !res)
>>> +	if (is_rebase_i(opts) && !res && !dropped_commit)
>>>   		record_in_rewritten(&item->commit->object.oid,
>>>   				    peek_command(todo_list, 1));
>>
>> If we have a sequence of commits where a commit that was *not*
>> dropped is followed by a fixup commit that *is* dropped (e.g.,
>> because it became empty/redundant), wouldn't it prevent the
>> previously pending commit from being flushed to skip
>> `record_in_rewritten` entirely for the dropped fixup commit?

> Huh, sounds possible. I wonder if that makes the change so complicated
> that my time isn't well spend working on that given that I'm not used to
> git's source code and it's better addressed by someone with deeper
> knowledge. Sounds as if we need a state signaling "Current commit is
> done".

>> Wouldn't it map the note for `X` to rewritten `C`?
>>
>>> diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh
>>> new file mode 100755
>>> index 000000000000..0eddde7f9961
>>> --- /dev/null
>>> +++ b/t/t3322-notes-rebase.sh
>>> @@ -0,0 +1,37 @@
>>> +#!/bin/sh
>>> +
>>> +test_description='Test notes on rebase'
>>> +
>>> +. ./test-lib.sh
>>> +
>>> +test_expect_success setup '
>>> +	git init &&
>>> +	git config notes.rewriteRef refs/notes/commits &&
>>> +	git version > version &&
>>> +	echo A > A &&
>>
>> Style.  In our codebase, redirection operator sticks to the
>> redirection target without SP in between, i.e.
>>
>> 	git version >version &&
>> 	echo A >A &&
>>
>>> +	git notes add -m "This is B" @ &&
>>
>> '@' is hard to read; when you refer to HEAD, please write HEAD.
>>
>>
>>> +test_expect_success 'rebase B + C on top of BD' '
>>> +	git rebase @ master
>>> +'
>>> +
>>> +test_expect_success 'assert there is no note on BD' '
>>> +	if git notes list branch >/tmp/lalaa; then return 1; fi
>>> +'
>>
>> Do not step outside of $TRASH_DIRECTORY without a good reason.
> 
> Oh, that is a debug thing that shouldn't have made it into the patch.
>   
>> Style.  In our codebase, shell scripts do not use ';' and written
>> more like
>>
>> 	if git notes list branch >notes-list
>> 	then
>> 		return 1
>> 	fi
>>
>> But more importantly, if you want to make sure the command makes a
>> controlled exit (not crash), use
>>
>> 	test_must_fail git notes list branch
> 
> Ah, I really wondered if I'm missing something because it should be
> easier to say "this command should fail".
> 
> Best regards
> Uwe