Re: [PATCH v2] send-email: export patch counters in validate environment

From: Robin Jarry <robin@jarry.cc>
Date: Apr 12, 2023, 18:33 UTC
Message-ID: <CRUZR9IO75B2.3DTTR2N12SQRL@ringo>
In-Reply-To: <xmqqfs957zs4.fsf@gitster.g>

Hi Junio,

Junio C Hamano, Apr 12, 2023 at 19:53:

> See Documentation/CodingGuidelines, look for "For shell scripts
> specifically" and follow what is in the section.  There may be style
> violations of other kinds in the file.

I had missed that one. I'll have a look.

Show 5 quoted lines

> > +	# Ensure that the patch applies without conflicts to the latest
> > +	# upstream version.
>
> That comment is true only for the first one.  The second patch needs
> to apply to the upstream plus the first patch, and so on.

Will adjust this as well.

Show 12 quoted lines

> Style-wise, it is better to get rid of get_upstream_url and write
> the above more like
>
> 	workdir=$1 &&
> 	url=$(git config remote.originurl) &&
> 	rm -r -- "$workdir" &&
> 	git clone ... ||
> 	die "failed to ..."
>
> and that would be less error prone (e.g. you will catch failure from
> "rm" yourself, instead of relying on "git clone" to catch it for
> you).

I have added set -e at the beginning of the script, specifically to avoid the chained && commands which make the code hard to read. If any command returns/exits with a non-zero status which is not handled by an if or by a ||, the shell script will exit.

I can probably get rid of the explicit die statements because of this.

Show 10 quoted lines

> In any case, I would avoid network traffic and extra disk usage if I
> were showing an example for readers to follow, and would not
> recommend you to use "clone" here, even if it were a shallow one.
>
> It would make much more sense to create a secondary worktree based
> on this repository, with its HEAD detached at the copy of the target
> branch (e.g. refs/remotes/origin/HEAD), and use that secondary
> worktree, as the necessary objects for "am -3" to fall back on are
> more likely to be found in such a setting, compared to a shallow
> clone that only can have the blobs at the tip.

I have never used secondary worktrees. My original thinking was that the local repository may not be up to date compared to the upstream and running git fetch on the local repo seemed like a bad idea. Would there be a proper way to do this with secondary worktree?

There may not be an elegant generic solution here. $(git config remote.origin.url) may not even contain the proper upstream url...

Also, if I understand how worktrees function, applying patches in a detached HEAD will create blobs in the current git dir. These will eventually be garbage collected but I wonder if that could be a problem.

Show 16 quoted lines

> It is a good discipline to always set GIT_DIR and GIT_WORK_TREE as a
> pair.  Working in a subdirectory of a working tree becomes awkward,
> because the presence of the former without the latter signals that
> your $(cwd) is at the top of the working tree.
>
> But that is more or less moot, because I am suggesting not to use
> "git clone" to prepare the playground and instead use a secondary
> worktree that is attached to the same current repository, so GIT_DIR
> would be the same as the current one.
>
> And because you are "cd"ing there anyway, it probably is much
> simpler to just
>
>     unset GIT_DIR GIT_WORK_TREE
>
> to let the repository discovery mechanism take care of it.

Depending on whether I use a worktree or not, I will unset these variables.

Show 7 quoted lines

> It is uneven that validate_patch and validate_cover_letter are
> responsible for dying when problem is found, but validate_series is
> not and the caller is made responsible for that.
>
> I would make the caller responsible for dying with message for all
> three by removing the calls to "die" or "exit" from the former two,
> if I were showing an example for readers to follow.

Agreed, this is inconsistent. My original intent was to provide more explicit error messages but it is probably not necessary.

As explained above, `set -e` will force early exit if any command fails without being explicitly handled. I will remove die/exit calls.

> Overall, a very well crafted patch, even though little details and
> some design choices can be improved.

Thanks for the careful review!