• Move the check for -DNO_CLEAN (-n), so this value is reflected when exporting the variable

• Add missing local variable declaration

• Use a compact switch statement

I'd be tempted to remove NANO_PMAKE from embedded/common, rescue/common, and dhcpd/common.

In D47399#1081457, @christos wrote:

LGTM, feel free to propose the patch for ai2s and davbus. :)

• Update with suggestions

Rebase.

In D47399#1081457, @christos wrote:

LGTM, feel free to propose the patch for ai2s and davbus. :)

The metadriver is also missing snd_emu10k1.
PowerPC has ai2s and davbus which can also be included in the snd_driver metadriver with similar guards, effectively replicating sys/modules/sound/driver/Makefile.

Thank you for the detailed explanation.
I'll wait for the MoM.
By the way, I have updated my local prepare-commit-msg hook, but maybe it would be useful to update tools/tools/git/hooks/prepare-commit-message as well?

FreeBSD-telepathy?
I just committed a fix and then realized the preferred style is the one described here (glancing over the commit history and the preferred style of my mentor).
Should I just remove the word "canonical" and use "preferred", or wait until the srcmgr call minutes are published?

Sorry, thinking more about it, this is essentially an rc patch.
The start action will always restart if there is an ipfilter_optionlist defined. Two questions:

1. Can ipfilter rely more on rc.subr?
2. If i understand name wouldn't this want an extra action?

I would have just added:

if [ -n "${ifilter_optionlist}" ]; then
	if ${ipfilter_program:-/sbin/ipf} -V | grep -q 'Running: yes'; then
		${ipfilter_program:-/sbin/ipf} -D
	fi
	${ipfilter_program:-/sbin/ipf} -T "${ipfilter_optionlist}"
fi

But the current code does the job well and it's arguably more explicit.

If I understood both suggestions correctly, the issue is that tmp is used in gitarc__{list,patch,stage}, and shadowing of the variable (tmp is a local variable)/overwriting of the file contents could accidentally occur in a future refactor of this script.
Suggestions:

1. One suggestions is to randomize the temporary file every time with the use of a subroutine xmktemp which nests the random file name under an already randomized GITARC_TMPDIR.
2. A consistent file naming scheme is desired, so each tmp file name would have to be changed to GITARC_TMPDIR/tmp_{list,patch,stage}, for example.

Both are valid, IMO I would also prefer option 2.
However, it is possible that in the future, someone™ extends this script, and by the virtues of copy/pasting, this nomenclature is lost or not easily followed. Given that this someone could very well be me, let's avoid a possible foot-shooting and implement 1[^1].

Just a minor observation.

This iteration attempts to reach a middle ground between all valid suggestions.

In D47289#1079383, @des wrote:

lgtm but I would suggest waiting for @markj to approve as well

In D46781#1079099, @emaste wrote:

I do not yet have the appropriate permissions to do it.

I believe everything should be set now.

Please, feel free to commit the changes from that branch if you should consider so:
https://github.com/jlduran/freebsd-src/tree/fix-git-arc-consolidated

• Use only $tmp

In D47302#1078661, @imp wrote:

Woot

• Minor iteration

And a more detailed explanation (I hope):
It was suggested in D46767 to use a trap to remove the temporary files created with mktemp. This is almost always considered a good practice, as it reduces the chances of leaving stray files, which may contain sensitive information.
The suggested line was:

trap 'rc=$?; rm -f "$review_data" "$user_data" "$diff_data" "$tmp"; trap - 0; exit $rc' 0 1 2 3 5 10 15

Meaning that whenever a signal of EXIT HUP INT QUIT TRAP USR1 TERM (0 1 2 3 5 10 15), without the SIG prefix is triggered, the temporary file is removed. The value of $? after the trap action completes shall be the value it had before the trap action was executed. Finally, if the action is -, the shell shall reset each condition to the default value.
It was also suggested to store {review,user,dif,tmp}_data under a directory, in order to just:

trap 'rc=$?; rm -fr "$tmp_data_dir"; trap - 0; exit $rc' 0 1 2 3 5 10 15

The code was changed to use a directory.
There are also other places in this file where mktemp is used—there's even one function where the temporary file is not being removed ($tmp under gitarc__stage())—so a global cleanup() makes sense. However, depending on where the trap is triggered, the local variable containing the temporary file or directory may not exist (i.e., outside the function). It is for this reason that these temporary variables must not be local variables, that is why I removed them from their respective local declarations.

Sorry, I originally had one revision where the temp file/directory was passed to the cleanup() function. The previously submitted one was a non-working hybrid.

Note to self: (Feature requeAdd a -T option to pass a test plan, similar to arcgit's.

Thank you for making this tool more portable. This is a cousin commit of D46781 and D46767. I agree, echo (without -n is portable) and IMO, more readable than printf, because there is no need to add a new line at the end. The issue is only with echo -n.

A few (almost) irrelevant suggestions.
I have tested a bit more with the minimal image. For the use case described here, it works quite well. However, when compiling something from source, I ran into a few issues:

1. Missing /usr/lib/libprocstat.so.1 from FreeBSD-utilities (this package pulls in a few dependencies).
2. Missing /usr/lib/crt1.o from FreeBSD-clibs-dev (this package is in the top 20 sorted by size).
3. Missing /sys symbolic link from pkgbase (irrelevant for this revision).

Overall, the minimal image is good and a welcomed addition towards delivering jails containers straight from the FreeBSD project (hopefully from a fully-owned registry as well as containerized ports packages).
Thanks again!

@kp has fixed the ping test in 2926c2594249f64fecbbdcb0b0b9a3591931ab04. It was not related to this commit.

The unk-40 below also has an unknown option, but is not tested in this file (tested on the atf-sh based test).
There are a couple other minor fixes I would like to add to this file, but this is just about making the tests pass.
Thanks for the heads up!

Thank you!

Sorry, forgot to actually include my doubts in the discussion.

Thank you, this looks good.

1. Build a list of commit ids (reversed).
2. Get a list of open revisions with colors (arc list --ansi).
3. For each commit:
   1. Print the commit short hash.
   2. Look for "Differential Revision:" in the commit body (log2diff).
   3. If there is only one result, print the Differential ID, if any ( diff2status). This may happen when a reviewer uses git arc patch -c to download the review and commit locally, as this text is added automatically by git-arc:
      1. Sanity check for the Differential Revision ID.
      2. Capture the output of echo '{"names":["D12345"]}' | arc call-conduit -- phid.lookup. To check online, visit https://reviews.freebsd.org/conduit/method/phid.lookup/ and type an array of the Differential Review IDs ["D12345"] [^1].
      3. Grab the Phabricator ID. This string must exist, but check just in case.
      4. Grab the "detailed status" (printf "%s" '{"constraints": {"phids": ["PHID-DREV-bxzw4f4pkmzlyha4gosr"]}}' | arc call-conduit -- differential.revision.search | jq -r '.response.data[0].fields.status').
      5. Pretty-print the "detailed status" and the title.
      6. Stop.
   4. Check if the title of the commit matches one from the list obtained in step 2. This can yield one of three outcomes:
      1. No Review : <title>.
      2. Ambiguous Reviews: D12345, D54321. I wonder if it should also print the title
      3. The output of the line matched in step 2, without the initial * (note that in my terminal, this also removes the bold font and coloring of the Differential Revision ID).
      4. Stop.

In D46804#1067197, @markj wrote:

If I understand correctly, "when a review exists" means that the commit log already has a "Differential Revision" tag?

Minor fixes

Final patch in the series.

% git arc list main..fix-git-arc-consolidated
33118f4d8db4 No Review             : git-arc.sh: Fix typo s/Truning/Turning/
775a91cc76f6 Accepted        D46767: git-arc: Do not echo unescaped literals to jq
c305f7eb5215 Accepted        D46781: git-arc: Make patch with reviewers more portable
a75479fe401f Accepted        D46789: git-arc: Fix find_author() for external users
23587ac312ed Needs Review    D46804: git-arc: Improve 'list' when a review exists

I'm not really good at painting in the terminal. I hope there is a shorter way to achieve the same result.

I think I know how to achieve the same result using printf, and escaping literals with %s.
Regardless, I only have one more patch for git-arc (git arc list) that may benefit of review_data being in a file. If it turns out otherwise, I can "revert" to using variables.
I'm done trying out this great article on the topic.
Thank you for the reviews!

Fix other occurrences of "echo -n" while here.

Tested against a Kea server, with a simplified version of the following example:
https://github.com/isc-projects/kea/blob/master/doc/examples/kea4/vivso.json

Thank you.. Hmm I am pretty sure I included @emaste in the review. Well, still learning git-arc.

I'm not a big fan of making temporary files, but I don't know how else to fix it.