Re: [PATCH v5 0/7] cherry-pick: add `--empty` for more robust redundant commit handling

[phillip.wood123@xxxxxxxxx]

Hi Brian

On 25/03/2024 23:16, Brian Lyles wrote:
> This is the final planned re-roll of this series, addressing two minor
> style concerns with commit 4/7 as noted in this [2] thread. All other
> commits are left unchanged.
>
> [2]: https://lore.kernel.org/git/xmqqa5mmhvx5.fsf@gitster.g
>
> Range-diff from v4:
>
> 1:  f6b8a655cd = 1:  f6b8a655cd docs: address inaccurate `--empty` default with `--exec`
> 2:  401de76c0b = 2:  401de76c0b docs: clean up `--empty` formatting in git-rebase(1) and git-am(1)
> 3:  031b3bb7bb = 3:  031b3bb7bb rebase: update `--empty=ask` to `--empty=stop`
> 4:  fd53c39482 ! 4:  d3bfe41819 sequencer: handle unborn branch with `--allow-empty`
>      @@ sequencer.c: static struct object_id *get_cache_tree_oid(struct index_state *ist
>        	struct commit *head_commit;
>        	struct index_state *istate = r->index;
>       +	const char *head_name;
>      -
>      --	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
>      --		return error(_("could not resolve HEAD commit"));
>      ++
>       +	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
>      -+		/*
>      -+		 * Check to see if this is an unborn branch
>      -+		 */
>      -+		head_name = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, &head_oid, NULL);
>      -+		if (!head_name || !starts_with(head_name, "refs/heads/") || !is_null_oid(&head_oid))
>      ++		/* Check to see if this is an unborn branch */
>      ++		head_name = resolve_ref_unsafe("HEAD",
>      ++			RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>      ++			&head_oid, NULL);
>      ++		if (!head_name ||
>      ++			!starts_with(head_name, "refs/heads/") ||
>      ++			!is_null_oid(&head_oid))
>       +			return error(_("could not resolve HEAD commit"));
>       +		head_tree_oid = the_hash_algo->empty_tree;
>       +	} else {
>       +		head_commit = lookup_commit(r, &head_oid);

This version is definitely more readable, thanks

>      +-	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
>      +-		return error(_("could not resolve HEAD commit"));
>      +-
>       -	head_commit = lookup_commit(r, &head_oid);
>       +		/*
>       +		 * If head_commit is NULL, check_commit, called from

This looks strange, but if I do a range-diff locally from v4 to v5 I 
only see the line wrapping changes above so I don't think it is anything 
to worry about.

Thanks for working on this

Phillip

> 5:  90dca45c12 = 5:  5e690bca6e sequencer: do not require `allow_empty` for redundant commit options
> 6:  ab3b6afc97 = 6:  ed03908e9e cherry-pick: enforce `--keep-redundant-commits` incompatibility
> 7:  0e2577ea56 = 7:  d3cf068c45 cherry-pick: add `--empty` for more robust redundant commit handling
>
>
> Brian Lyles (7):
>    docs: address inaccurate `--empty` default with `--exec`
>    docs: clean up `--empty` formatting in git-rebase(1) and git-am(1)
>    rebase: update `--empty=ask` to `--empty=stop`
>    sequencer: handle unborn branch with `--allow-empty`
>    sequencer: do not require `allow_empty` for redundant commit options
>    cherry-pick: enforce `--keep-redundant-commits` incompatibility
>    cherry-pick: add `--empty` for more robust redundant commit handling
>
>   Documentation/git-am.txt          | 20 ++++++---
>   Documentation/git-cherry-pick.txt | 30 ++++++++++---
>   Documentation/git-rebase.txt      | 26 +++++++----
>   builtin/rebase.c                  | 16 ++++---
>   builtin/revert.c                  | 38 +++++++++++++++-
>   sequencer.c                       | 72 ++++++++++++++++++-------------
>   t/t3424-rebase-empty.sh           | 55 +++++++++++++++++++++--
>   t/t3501-revert-cherry-pick.sh     | 14 ++++--
>   t/t3505-cherry-pick-empty.sh      | 51 +++++++++++++++++++++-
>   t/t3510-cherry-pick-sequence.sh   | 32 ++++++++++++++
>   10 files changed, 286 insertions(+), 68 deletions(-)