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

From: Brian Lyles <brianmlyles@gmail.com>
Date: Mar 25, 2024, 16:12 UTC
Message-ID: <17c00de527e3a0c4.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
In-Reply-To: <79c4d09f-fc1b-40d1-9655-3a9e166918cb@gmail.com>

Hi Phillip

On Mon, Mar 25, 2024 at 9:38 AM <phillip.wood123@gmail.com> wrote:

Show 16 quoted lines

>      @@ sequencer.c: static struct object_id *get_cache_tree_oid(struct index_state *ist
>       -	 */
>       -	if (repo_parse_commit(r, head_commit))
>       -		return -1;
>      ++	const char *head_name;
>      ++
>       +	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
>       +		/*
>      -+		 * Treat an error reading HEAD as an unborn branch.
>      ++		 * Check to see if this is an unborn branch
>       +		 */
> 
> You are only editing an existing comment here and it is not worth a
> re-roll on its own, but for one line comments we prefer
> 
> 	/* Check to see if this is an unborn branch */

Existing in v3, but new with this series. Since it sounds like a re-roll is desired anyway to address the line length issue below, I can go ahead and fix this in v5 as well.

Show 6 quoted lines

>      ++		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))
> 
> While we don't mind the occasional line that is a little over 80
> characters these really are rather long.
>

You're right, these got a little long. I wasn't able to identify a definitive wrapping style for these cases, so I'll include my proposed update here just to avoid another re-roll. Does the following diff from v4 to a proposed v5 work for you?

@@ -776,11 +776,13 @@ static int is_index_unchanged(struct repository *r)
 	const char *head_name;
 
 	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 {

> Apart from the minor style issues this all looks good to me, thanks
> for working on it, it will be a useful addition to be able to drop
> cherry-picks that become empty.

Thanks, I really appreciate your help with this series!

-- 
Thank you,
Brian Lyles