Some project conventions have been added to CodingGuidelines.
* ps/doc-more-c-coding-guidelines:
Documentation: consistently use spaces inside initializers
Documentation: document idiomatic function names
Documentation: document naming schema for structs and their functions
Documentation: clarify indentation style for C preprocessor directives
clang-format: fix indentation width for preprocessor directives
"git grep -W" omits blank lines that follow the found function at
the end of the file, just like it omits blank lines before the next
function.
* rs/grep-omit-blank-lines-after-function-at-eof:
grep: -W: skip trailing empty lines at EOF, too
"git notes add -m '' --allow-empty" and friends that take prepared
data to create notes should not invoke an editor, but it started
doing so since Git 2.42, which has been corrected.
* dd/notes-empty-no-edit-by-default:
notes: do not trigger editor when adding an empty note
Test script linter has been updated to catch an attempt to use
one-shot export construct "VAR=VAL func" for shell functions (which
does not work for some shells) better.
* es/shell-check-updates:
check-non-portable-shell: improve `VAR=val shell-func` detection
check-non-portable-shell: suggest alternative for `VAR=val shell-func`
check-non-portable-shell: loosen one-shot assignment error message
t4034: fix use of one-shot variable assignment with shell function
t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
A 'P' command to "git add -p" that passes the patch hunk to the
pager has been added.
* rj/add-p-pager:
add-patch: render hunks through the pager
pager: introduce wait_for_pager
pager: do not close fd 2 unnecessarily
add-patch: test for 'p' command
The reviewing guidelines document now explicitly encourages people
to give positive reviews and how.
* jc/doc-reviewing-guidelines-positive-reviews:
ReviewingGuidelines: encourage positive reviews more
"git rebase --help" referred to "offset" (the difference between
the location a change was taken from and the change gets replaced)
incorrectly and called it "fuzz", which has been corrected.
* jc/doc-rebase-fuzz-vs-offset-fix:
doc: difference in location to apply is "offset", not "fuzz"
It has been documented that we avoid "VAR=VAL shell_func" and why.
* jc/doc-one-shot-export-with-shell-func:
CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"
Another reftable test has been ported to use the unit test framework.
* cp/unit-test-reftable-merged:
t-reftable-merged: add test for REFTABLE_FORMAT_ERROR
t-reftable-merged: use reftable_ref_record_equal to compare ref records
t-reftable-merged: add tests for reftable_merged_table_max_update_index
t-reftable-merged: improve the const-correctness of helper functions
t-reftable-merged: improve the test t_merged_single_record()
t: harmonize t-reftable-merged.c with coding guidelines
t: move reftable/merged_test.c to the unit testing framework
A CI job that use clang-format to check coding style issues in new
code has been added.
* kn/ci-clang-format:
ci/style-check: add `RemoveBracesLLVM` in CI job
check-whitespace: detect if no base_commit is provided
ci: run style check on GitHub and GitLab
clang-format: formalize some of the spacing rules
clang-format: avoid spacing around bitfield colon
clang-format: indent preprocessor directives after hash
"git checkout --ours" (no other arguments) complained that the
option is incompatible with branch switching, which is technically
correct, but found confusing by some users. It now says that the
user needs to give pathspec to specify what paths to checkout.
* jc/checkout-no-op-switch-errors:
checkout: special case error messages during noop switching
"git add -p" by users with diff.suppressBlankEmpty set to true
failed to parse the patch that represents an unmodified empty line
with an empty line (not a line with a single space on it), which
has been corrected.
* pw/add-patch-with-suppress-blank-empty:
add-patch: use normalize_marker() when recounting edited hunk
add-patch: handle splitting hunks with diff.suppressBlankEmpty
A build tweak knob has been simplified by not setting the value
that is already the default; another unused one has been removed.
* rj/make-cleanup:
config.mak.uname: remove unused uname_P variable
Makefile: drop -Wno-universal-initializer from SP_EXTRA_FLAGS
Many Porcelain commands that internally use the merge machinery
were taught to consistently honor the diff.algorithm configuration.
* ad/merge-with-diff-algorithm:
merge-recursive: honor diff.algorithm
Unit test clean-up.
* rs/t-strvec-use-test-msg:
t-strvec: fix type mismatch in check_strvec
t-strvec: improve check_strvec() output
t-strvec: use test_msg()
Our coding guide is inconsistent with how it uses spaces inside of
initializers (`struct foo bar = { something }`). While we mostly carry
the space between open and closing braces and the initialized members,
in one case we don't.
Fix this one instance such that we consistently carry the space. This is
also consistent with how clang-format formats such initializers.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We semi-regularly have discussions around whether a function shall be
named `S_release()`, `S_clear()` or `S_free()`. Indeed, it may not be
obvious which of these is preferable as we never really defined what
each of these variants means exactly.
Carve out a space where we can add idiomatic names for common functions
in our coding guidelines and define each of those functions. Like this,
we can get to a shared understanding of their respective semantics and
can easily point towards our style guide in future discussions such that
our codebase becomes more consistent over time.
Note that the intent is not to rename all functions which violate these
semantics right away. Rather, the intent is to slowly converge towards a
common style over time.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We nowadays have a proper mishmash of struct-related functions that are
called `<verb>_<struct>` (e.g. `clear_prio_queue()`) versus functions
that are called `<struct>_<verb>` (e.g. `strbuf_clear()`). While the
former style may be easier to tie into a spoken conversation, most of
our communication happens in text anyway. Furthermore, prefixing
functions with the name of the structure they operate on makes it way
easier to group them together, see which functions are related, and will
also help folks who are using code completion.
Let's thus settle on one style, namely the one where functions start
with the name of the structure they operate on.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commit, we have settled on using a single space per
nesting level to indent preprocessor directives. Clarify our coding
guidelines accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In [1], we have improved our clang-format configuration to also specify
the style for how to indent preprocessor directives. But while we have
settled the question of where to put the indentation, either before or
after the hash sign, we didn't specify exactly how to indent.
With the current configuration, clang-format uses tabs to indent each
level of nested preprocessor directives, which is in fact unintentional
and never done in our codebase. Instead, we use a mixture of indenting
by either one or two spaces, where using a single space is somewhat more
common.
Adapt our clang-format configuration accordingly by specifying an
indentation width of one space.
[1]: <20240708092317.267915-1-karthik.188@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* kn/ci-clang-format:
ci/style-check: add `RemoveBracesLLVM` in CI job
check-whitespace: detect if no base_commit is provided
ci: run style check on GitHub and GitLab
clang-format: formalize some of the spacing rules
clang-format: avoid spacing around bitfield colon
clang-format: indent preprocessor directives after hash
4aa2c4753d (grep: -W: don't extend context to trailing empty lines,
2016-05-28) stopped showing empty lines at the end of function context
when using -W. Do the same for trailing empty lines at the end of
files, for consistency -- it doesn't matter whether a function section
is ended by the next function or the end of the file.
Test it by adding a trailing empty line to the file used by the test
"grep -W" and leave its expected output the same.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With "git notes add -C $blob", the given blob contents are to be made
into a note without involving an editor. But when "--allow-empty" is
given, the editor is invoked, which can cause problems for
non-interactive callers[1].
This behaviour started with 90bc19b3ae (notes.c: introduce
'--separator=<paragraph-break>' option, 2023-05-27), which changed
editor invocation logic to check for a zero length note_data buffer.
Restore the original behaviour of "git note" that takes the contents
given via the "-m", "-C", "-F" options without invoking an editor, by
checking for any prior parameter callbacks, indicated by a non-zero
note_data.msg_nr. Remove the now-unneeded note_data.given flag.
Add a test for this regression by checking whether GIT_EDITOR is
invoked alongside "git notes add -C $empty_blob --allow-empty"
[1] https://github.com/ddiss/icyci/issues/12
Signed-off-by: David Disseldorp <ddiss@suse.de>
[jc: enhanced the test with -m/-F options]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The comment surrounding check_pointer_eq() should explain about what
this function does instead of explaining check_int(). Correct this.
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.
As such, check-non-portable-shell.pl warns when it detects such usage.
However, a limitation of the check is that it only detects such
invocations when variable assignment (i.e. `VAR=val`) is the first thing
on the line. Thus, it can easily be fooled by an invocation such as:
echo X | VAR=val shell-func
Address this shortcoming by loosening the check so that the variable
assignment can be recognized even when not at the beginning of the line.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Most problems reported by check-non-portable-shell are accompanied by
advice suggesting how the test author can repair the problem. For
instance:
error: egrep/fgrep obsolescent (use grep -E/-F)
However, when one-shot variable assignment is detected when calling a
shell function (i.e. `VAR=val shell-func`), the problem is reported, but
no advice is given. The lack of advice is particularly egregious since
neither the problem nor the workaround are likely well-known by
newcomers to the project writing tests for the first time. Address this
shortcoming by recommending the use of `test_env` which is tailor made
for this specific use-case.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a0a630192d (t/check-non-portable-shell: detect "FOO=bar
shell_func", 2018-07-13) added the check for one-shot environment
variable assignment for shell functions, the primary reason given for
avoiding them was that, under some shells, the assignment outlives the
invocation of the shell function, thus could potentially negatively
impact subsequent commands in the same test, as well as subsequent
tests.
However, it has recently become apparent that this is not the only
potential problem with one-shot assignments and shell functions. Another
problem is that some shells do not actually export the variable to
commands which the function invokes[1]. More significantly, however, the
behavior of one-shot assignments with shell functions is not specified
by POSIX[2].
Given this new understanding, the presented error message ("assignment
extends beyond 'shell_func'") is too specific and potentially
misleading. Address this by emitting a less specific error message.
(Note that the wording "is not portable" is chosen over the more
specific "behavior not specified by POSIX" for consistency with almost
all other error message issued by this "lint" script.)
[1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/
[2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell. One such problematic behavior
is that, with some shells, the assignment will outlive the invocation of
the function, thus may potentially impact subsequent commands in the
test, as well as subsequent tests. A common way to work around the
problem is to wrap a subshell around the one-shot assignment, thus
ensuring that the assignment is short-lived.
In this test, the subshell is employed precisely for this purpose; other
side-effects of the subshell, such as losing the effect of `test_tick`
which is invoked by `test_commit`, are immaterial.
These days, we can take advantage of `test_commit --author` to more
clearly convey that the test is interested only in overriding the author
of the commit.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano