Re: [RFC 2/3] am: semi working --cover-at-tip

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2017, 06:00 UTC
Message-ID: <xmqqbmk54cy3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <948b19c2-9f2d-de9d-1e0a-6681dc9317a9@suse.de>

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

>  	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
>  		state->sign_commit = gpgsign ? "" : NULL;
> +
>  }

Please give at least a cursory proof-reading before sending things out.

Show 12 quoted lines

> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
>  
>  	oidclr(&state->orig_commit);
>  	unlink(am_path(state, "original-commit"));
> -
> -	if (!get_oid("HEAD", &head))
> -		write_state_text(state, "abort-safety", oid_to_hex(&head));
> -	else
> -		write_state_text(state, "abort-safety", "");
> -
> -	state->cur++;
> -	write_state_count(state, "next", state->cur);

Moving these lines to a later part of the source file is fine, but can you do so as a separate preparatory patch that does not change anything else? That would unclutter the main patch that adds the feature, allowing better reviews from reviewers.

The hunk below...

Show 25 quoted lines

> +/**
> + * Increments the patch pointer, and cleans am_state for the application of the
> + * next patch.
> + */
> +static void am_next(struct am_state *state)
> +{
> +	struct object_id head;
> +
> +	/* Flush the cover letter if needed */
> +	if (state->cover_at_tip == 1 &&
> +	    state->series_len > 0 &&
> +	    state->series_id == state->series_len &&
> +	    state->cover_id > 0)
> +		do_apply_cover(state);
> +
> +	am_clean(state);
> +
> +	if (!get_oid("HEAD", &head))
> +		write_state_text(state, "abort-safety", oid_to_hex(&head));
> +	else
> +		write_state_text(state, "abort-safety", "");
> +
> +	state->cur++;
> +	write_state_count(state, "next", state->cur);
> +}

... if you followed that "separate preparatory step" approach, would show clearly that you added the logic to call do_apply_cover() when we transition after applying the Nth patch of a series with N patches, as all the existing lines will show only as unchanged context lines.

By the way, don't we want to sanity check state->last (which we learn by running "git mailsplit" that splits the incoming mbox into pieces and counts the number of messages) against state->series_len? Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover letter, and then follow-up with [PATCH 7/6]. For somebody like me, it would be more convenient if the above code (more-or-less) ignored series_len and called do_apply_cover() after applying the last patch (which would be [PATCH 7/6]) based on what state->last says.

Thanks.