* [PATCH v2 01/16] t2018: remove trailing space from test description
From: Denton Liu @ 2020-01-07 4:52 UTC (permalink / raw)
To: Git Mailing List
Cc: Eric Sunshine, Jakub Narebski, Johannes Sixt, Junio C Hamano
In-Reply-To: <cover.1578372682.git.liu.denton@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t2018-checkout-branch.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 822381dd9d..e18abce3d2 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='checkout '
+test_description='checkout'
. ./test-lib.sh
--
2.25.0.rc1.180.g49a268d3eb
^ permalink raw reply related
* [PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 2)
From: Denton Liu @ 2020-01-07 4:52 UTC (permalink / raw)
To: Git Mailing List
Cc: Eric Sunshine, Jakub Narebski, Johannes Sixt, Junio C Hamano
In-Reply-To: <cover.1577454401.git.liu.denton@gmail.com>
The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].
This is the second part. It focuses on t[234]*.sh. The first part can be
found here[2].
Changes since v1:
* Rewrite commit messages to be shorter and use the present tense when
referring to the present state
* Clean up as suggested by Eric
* Replace 12/16 with J6t's patch
* Drop "let sed open its own files"
[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/
Denton Liu (15):
t2018: remove trailing space from test description
t2018: add space between function name and ()
t2018: improve style of if-statement
t2018: use test_expect_code for failing git commands
t2018: teach do_checkout() to accept `!` arg
t2018: don't lose return code of git commands
t2018: replace "sha" with "oid"
t3030: use test_path_is_missing()
t3310: extract common notes_merge_files_gone()
t3415: stop losing return codes of git commands
t3415: increase granularity of test_auto_{fixup,squash}()
t3419: stop losing return code of git command
t3507: fix indentation
t3507: use test_path_is_missing()
t4124: only mark git command with test_must_fail
Johannes Sixt (1):
t3504: do check for conflict marker after failed cherry-pick
t/t2018-checkout-branch.sh | 77 ++++++++-----
t/t3030-merge-recursive.sh | 2 +-
t/t3310-notes-merge-manual-resolve.sh | 22 ++--
t/t3415-rebase-autosquash.sh | 153 +++++++++++++++++++-------
t/t3419-rebase-patch-id.sh | 3 +-
t/t3504-cherry-pick-rerere.sh | 6 +-
t/t3507-cherry-pick-conflict.sh | 28 ++---
t/t4124-apply-ws-rule.sh | 10 +-
8 files changed, 205 insertions(+), 96 deletions(-)
Range-diff against v1:
1: 7517159cc1 = 1: a0199f1e48 t2018: remove trailing space from test description
2: 0674eee69e ! 2: f0f541b520 t2018: add space between function name and ()
@@ Metadata
## Commit message ##
t2018: add space between function name and ()
+ Add a space between the function name and () which brings the style in
+ line with Git's coding guidelines.
+
## t/t2018-checkout-branch.sh ##
@@ t/t2018-checkout-branch.sh: test_description='checkout'
# 2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
-: ---------- > 3: 501eb147c3 t2018: improve style of if-statement
3: 2451bff3af ! 4: 587e53053f t2018: use test_must_fail for failing git commands
@@ Metadata
Author: Denton Liu <liu.denton@gmail.com>
## Commit message ##
- t2018: use test_must_fail for failing git commands
+ t2018: use test_expect_code for failing git commands
- Before, when we expected `git diff` to fail, we negated its status with
+ When we are expecting `git diff` to fail, we negate its status with
`!`. However, if git ever exits unexpectedly, say due to a segfault, we
would not be able to tell the difference between that and a controlled
- failure. Use `test_must_fail git diff` instead so that if an unepxected
- failure occurs, we can catch it.
+ failure. Use `test_expect_code 1 git diff` instead so that if an
+ unexpected failure occurs, we can catch it.
- We had some instances of `test_must_fail test_dirty_{un,}mergable`.
- However, `test_must_fail` should only be used on git commands. Teach
- test_dirty_{un,}mergable() to accept `!` as a potential first argument
- which will run the git command without test_must_fail(). This prevents
- the double-negation where we were effectively running
- `test_must_fail test_must_fail git diff ...`.
+ We have some instances of `test_must_fail test_dirty_{un,}mergable`.
+ However, `test_must_fail` should only be used on git commands. Convert
+ these instances to use the `test_dirty_{un,}mergeable_discards_changes`
+ helper functions so that we remove the double-negation.
While we're at it, remove redirections to /dev/null since output is
- silenced when running without `-v` and debugging output is useful with
- `-v`, remove redirections to /dev/null as it is not useful.
+ silenced when running without `-v` anyway.
## t/t2018-checkout-branch.sh ##
@@ t/t2018-checkout-branch.sh: do_checkout () {
@@ t/t2018-checkout-branch.sh: do_checkout () {
test_dirty_unmergeable () {
- ! git diff --exit-code >/dev/null
-+ should_fail="test_expect_code 1" &&
-+ if test "x$1" = "x!"
-+ then
-+ should_fail=
-+ fi &&
-+ $should_fail git diff --exit-code
++ test_expect_code 1 git diff --exit-code
++}
++
++test_dirty_unmergeable_discards_changes () {
++ git diff --exit-code
}
setup_dirty_unmergeable () {
@@ t/t2018-checkout-branch.sh: setup_dirty_unmergeable () {
test_dirty_mergeable () {
- ! git diff --cached --exit-code >/dev/null
-+ should_fail="test_expect_code 1" &&
-+ if test "x$1" = "x!"
-+ then
-+ should_fail=
-+ fi &&
-+ $should_fail git diff --cached --exit-code
++ test_expect_code 1 git diff --cached --exit-code
++}
++
++test_dirty_mergeable_discards_changes () {
++ git diff --cached --exit-code
}
setup_dirty_mergeable () {
@@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch
# still dirty and on branch1
do_checkout branch2 $HEAD1 "-f -b" &&
- test_must_fail test_dirty_unmergeable
-+ test_dirty_unmergeable !
++ test_dirty_unmergeable_discards_changes
'
test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
@@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch
setup_dirty_mergeable &&
do_checkout branch2 $HEAD1 "-f -b" &&
- test_must_fail test_dirty_mergeable
-+ test_dirty_mergeable !
++ test_dirty_mergeable_discards_changes
'
test_expect_success 'checkout -b to an existing branch fails' '
@@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -B to an existing bran
# still dirty and on branch1
do_checkout branch2 $HEAD1 "-f -B" &&
- test_must_fail test_dirty_unmergeable
-+ test_dirty_unmergeable !
++ test_dirty_unmergeable_discards_changes
'
test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
@@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -B to an existing b
setup_dirty_mergeable &&
do_checkout branch2 $HEAD1 "-f -B" &&
- test_must_fail test_dirty_mergeable
-+ test_dirty_mergeable !
++ test_dirty_mergeable_discards_changes
'
test_expect_success 'checkout -b <describe>' '
4: bc6330557e ! 5: c43c11b912 t2018: teach do_checkout() to accept `!` arg
@@ Metadata
## Commit message ##
t2018: teach do_checkout() to accept `!` arg
- Before, we were running `test_must_fail do_checkout`. However,
+ We are running `test_must_fail do_checkout`. However,
`test_must_fail` should only be used on git commands. Teach
do_checkout() to accept `!` as a potential first argument which will
- prepend `test_must_fail` to the enclosed git command and skips the
- remainder of the function.
+ cause the function to expect the "git checkout" to fail.
This increases the granularity of the test as, instead of blindly
checking that do_checkout() failed, we check that only the specific
@@ Commit message
## t/t2018-checkout-branch.sh ##
@@ t/t2018-checkout-branch.sh: test_description='checkout'
+
+ . ./test-lib.sh
+
+-# Arguments: <branch> <sha> [<checkout options>]
++# Arguments: [!] <branch> <sha> [<checkout options>]
+ #
+ # Runs "git checkout" to switch to <branch>, testing that
+ #
+@@ t/t2018-checkout-branch.sh: test_description='checkout'
+ # 2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
#
# If <checkout options> is not specified, "git checkout" is run with -b.
++#
++# If the first argument is `!`, "git checkout" is expected to fail when
++# it is run.
do_checkout () {
+ should_fail= &&
+ if test "x$1" = "x!"
+ then
-+ should_fail=test_must_fail &&
++ should_fail=yes &&
+ shift
+ fi &&
exp_branch=$1 &&
@@ t/t2018-checkout-branch.sh: do_checkout () {
fi
- git checkout $opts $exp_branch $exp_sha &&
-+ $should_fail git checkout $opts $exp_branch $exp_sha &&
-
+-
- test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
- test $exp_sha = $(git rev-parse --verify HEAD)
-+ if test -z "$should_fail"
++ if test -n "$should_fail"
+ then
++ test_must_fail git checkout $opts $exp_branch $exp_sha
++ else
++ git checkout $opts $exp_branch $exp_sha &&
+ test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
+ test $exp_sha = $(git rev-parse --verify HEAD)
+ fi
5: fb2b865e6a ! 6: e09a70f6ca t2018: don't lose return code of git commands
@@ Metadata
## Commit message ##
t2018: don't lose return code of git commands
- We had some git commands wrapped in non-assignment command substitutions
- which would result in their return codes to be lost. Rewrite these
- instances so that their return codes are now checked.
+ Fix invocations of git commands so their exit codes are not lost
+ within non-assignment command substitutions.
## t/t2018-checkout-branch.sh ##
@@ t/t2018-checkout-branch.sh: do_checkout () {
- exp_ref="refs/heads/$exp_branch" &&
-
- # if <sha> is not specified, use HEAD.
-- exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
-+ head=$(git rev-parse --verify HEAD) &&
-+ exp_sha=${2:-$head} &&
-
- # default options for git checkout: -b
- if [ -z "$3" ]; then
-@@ t/t2018-checkout-branch.sh: do_checkout () {
-
- if test -z "$should_fail"
- then
+ test_must_fail git checkout $opts $exp_branch $exp_sha
+ else
+ git checkout $opts $exp_branch $exp_sha &&
- test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
- test $exp_sha = $(git rev-parse --verify HEAD)
+ echo "$exp_ref" >ref.expect &&
6: 7c26b921c3 ! 7: 71f84811c7 t2018: replace "sha" with "oid"
@@ t/t2018-checkout-branch.sh: test_description='checkout'
. ./test-lib.sh
--# Arguments: <branch> <sha> [<checkout options>]
-+# Arguments: <branch> <oid> [<checkout options>]
+-# Arguments: [!] <branch> <sha> [<checkout options>]
++# Arguments: [!] <branch> <oid> [<checkout options>]
#
# Runs "git checkout" to switch to <branch>, testing that
#
@@ t/t2018-checkout-branch.sh: test_description='checkout'
+# 2) HEAD is <oid>; if <oid> is not specified, the old HEAD is used.
#
# If <checkout options> is not specified, "git checkout" is run with -b.
- do_checkout () {
+ #
@@ t/t2018-checkout-branch.sh: do_checkout () {
exp_branch=$1 &&
exp_ref="refs/heads/$exp_branch" &&
- # if <sha> is not specified, use HEAD.
+- exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
+ # if <oid> is not specified, use HEAD.
- head=$(git rev-parse --verify HEAD) &&
-- exp_sha=${2:-$head} &&
-+ exp_oid=${2:-$head} &&
++ exp_oid=${2:-$(git rev-parse --verify HEAD)} &&
# default options for git checkout: -b
- if [ -z "$3" ]; then
+ if test -z "$3"
@@ t/t2018-checkout-branch.sh: do_checkout () {
- opts="$3"
- fi
-- $should_fail git checkout $opts $exp_branch $exp_sha &&
-+ $should_fail git checkout $opts $exp_branch $exp_oid &&
-
- if test -z "$should_fail"
+ if test -n "$should_fail"
then
+- test_must_fail git checkout $opts $exp_branch $exp_sha
++ test_must_fail git checkout $opts $exp_branch $exp_oid
+ else
+- git checkout $opts $exp_branch $exp_sha &&
++ git checkout $opts $exp_branch $exp_oid &&
echo "$exp_ref" >ref.expect &&
git rev-parse --symbolic-full-name HEAD >ref.actual &&
test_cmp ref.expect ref.actual &&
7: 9e37358f38 ! 8: f0da1d6350 t3030: use test_path_is_missing()
@@ Metadata
## Commit message ##
t3030: use test_path_is_missing()
- Previously, we would use `test_must_fail test -d` to ensure that the
- directory is removed. However, test_must_fail() should only be used for
- git commands. Use test_path_is_missing() instead to check that the
- directory has been removed.
+ We use `test_must_fail test -d` to ensure that the directory is removed.
+ However, test_must_fail() should only be used for git commands. Use
+ test_path_is_missing() instead to check that the directory has been
+ removed.
## t/t3030-merge-recursive.sh ##
@@ t/t3030-merge-recursive.sh: test_expect_success 'merge removes empty directories' '
8: 9705769841 ! 9: 46fe82b856 t3310: extract common no_notes_merge_left()
@@ Metadata
Author: Denton Liu <liu.denton@gmail.com>
## Commit message ##
- t3310: extract common no_notes_merge_left()
+ t3310: extract common notes_merge_files_gone()
- We had many statements which were duplicated. Extract and replace these
- duplicate statements with no_notes_merge_left().
+ We have many statements which are duplicated. Extract and replace these
+ duplicate statements with notes_merge_files_gone().
While we're at it, replace the test_might_fail(), which should only be
- used on git commands, with a compound command that always returns 0,
- even if the underlying ls fails.
+ used on git commands.
Also, remove the redirection from stderr to /dev/null. This is because
the test scripts automatically suppress output already. Otherwise, if a
@@ t/t3310-notes-merge-manual-resolve.sh: verify_notes () {
test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
}
-+no_notes_merge_left () {
++notes_merge_files_gone () {
++ # No .git/NOTES_MERGE_* files left
+ { ls .git/NOTES_MERGE_* >output || :; } &&
+ test_must_be_empty output
+}
@@ t/t3310-notes-merge-manual-resolve.sh: EOF
- # No .git/NOTES_MERGE_* files left
- test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
- test_must_be_empty output &&
-+ no_notes_merge_left &&
++ notes_merge_files_gone &&
# Merge commit has pre-merge y and pre-merge z as parents
test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
@@ t/t3310-notes-merge-manual-resolve.sh: test_expect_success 'redo merge of z into
- # No .git/NOTES_MERGE_* files left
- test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
- test_must_be_empty output &&
-+ no_notes_merge_left &&
++ notes_merge_files_gone &&
# m has not moved (still == y)
test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
# Verify that other notes refs has not changed (w, x, y and z)
@@ t/t3310-notes-merge-manual-resolve.sh: EOF
- # No .git/NOTES_MERGE_* files left
- test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
- test_must_be_empty output &&
-+ no_notes_merge_left &&
++ notes_merge_files_gone &&
# Merge commit has pre-merge y and pre-merge z as parents
test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
@@ t/t3310-notes-merge-manual-resolve.sh: EOF
- # No .git/NOTES_MERGE_* files left
- test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
- test_must_be_empty output &&
-+ no_notes_merge_left &&
++ notes_merge_files_gone &&
# m has not moved (still == w)
test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
# Verify that other notes refs has not changed (w, x, y and z)
9: b6d8f6a6e1 ! 10: 32d2051b31 t3415: stop losing return codes of git commands
@@ Metadata
## Commit message ##
t3415: stop losing return codes of git commands
- When a command is in a non-assignment subshell, the return code will be
- lost in favour of the surrounding command's. Rewrite instances of this
- such that the return code of git commands is no longer lost.
+ Fix invocations of git commands so their exit codes are not lost
+ within non-assignment command substitutions.
## t/t3415-rebase-autosquash.sh ##
@@ t/t3415-rebase-autosquash.sh: test_auto_fixup () {
10: e2818d1761 ! 11: 8b716c6a81 t3415: increase granularity of test_auto_{fixup,squash}()
@@ Metadata
## Commit message ##
t3415: increase granularity of test_auto_{fixup,squash}()
- We used to use `test_must_fail test_auto_{fixup,squash}` which would
+ We are using `test_must_fail test_auto_{fixup,squash}` which would
ensure that the function failed. However, this is a little ham-fisted as
there is no way of ensuring that the expected part of the function
actually fails.
11: 0357bb8533 ! 12: ea36028d53 t3419: stop losing return code of git command
@@ Metadata
## Commit message ##
t3419: stop losing return code of git command
- We had an instance of a git command in a non-assignment command
- substitution. Its return code was lost so we would not be able to detect
- if the command failed for some reason. Since we were testing to make
- sure the output of the command was empty, rewrite it in a more canonical
- way.
+ Fix invocation of git command so its exit codes is not lost within
+ a non-assignment command substitution.
## t/t3419-rebase-patch-id.sh ##
@@ t/t3419-rebase-patch-id.sh: do_tests () {
12: 35e32f21e1 < -: ---------- t3504: don't use `test_must_fail test_cmp`
-: ---------- > 13: 88134bb6d1 t3504: do check for conflict marker after failed cherry-pick
13: 1c38a5b3f7 ! 14: 96310b7d28 t3507: fix indentation
@@ Metadata
## Commit message ##
t3507: fix indentation
- We had some test cases which were indented 7-spaces instead of a tab.
- Fix this by reindenting with a tab instead.
+ We have some test cases which are indented 7-spaces instead of a tab.
+ Reindent with a tab instead.
This patch should appear empty with `--ignore-all-space`.
14: 7fa886809e = 15: 69125b8e2f t3507: use test_path_is_missing()
15: e214e1a667 ! 16: b93ebc0e42 t4124: only mark git command with test_must_fail
@@ t/t4124-apply-ws-rule.sh: prepare_test_file () {
}
apply_patch () {
-+ should_fail= &&
++ cmd_prefix= &&
+ if test "x$1" = 'x!'
+ then
-+ should_fail=test_must_fail &&
++ cmd_prefix=test_must_fail &&
+ shift
+ fi &&
>target &&
sed -e "s|\([ab]\)/file|\1/target|" <patch |
- git apply "$@"
-+ $should_fail git apply "$@"
++ $cmd_prefix git apply "$@"
}
test_fix () {
16: 31aa0c7d15 < -: ---------- t4124: let sed open its own files
--
2.25.0.rc1.180.g49a268d3eb
^ permalink raw reply
* Re: [PATCH 1/1] add: use advise function to display hints
From: Heba Waly @ 2020-01-07 4:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Heba Waly via GitGitGadget, Git Mailing List
In-Reply-To: <xmqqpng1eisc.fsf@gitster-ct.c.googlers.com>
On Fri, Jan 3, 2020 at 8:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
>
> Use of advise() function is good for giving hints not just due to
> its yellow coloring (which by the way I find not very readable,
> perhaps because I use black ink on white paper). One good thing in
> using the advise() API is that the messages can also be squelched
> with advice.* configuration variables.
>
Got it, thanks.
> And these two hints in "git add" are good chandidates to make
> customizable (perhaps with "advice.addNothing"), so I tend to agree
> with you that it makes sense to move these two messages to advise().
> Unfortunately this patch goes only halfway and stops (see below).
>
> If there are many other places that calls to advise() are made
> without getting guarded by the toggles defined in advice.c, we
> should fix them, I think.
>
Ok, we can address that in a separate patch.
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> > builtin/add.c | 4 ++--
> > t/t3700-add.sh | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 4c38aff419..eebf8d772b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
> > fprintf(stderr, _(ignore_error));
> > for (i = 0; i < dir->ignored_nr; i++)
> > fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > - fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > + advise(_("Use -f if you really want to add them.\n"));
> > exit_status = 1;
> > }
> >
> > @@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >
> > if (require_pathspec && pathspec.nr == 0) {
> > fprintf(stderr, _("Nothing specified, nothing added.\n"));
> > - fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> > + advise( _("Maybe you wanted to say 'git add .'?\n"));
> > return 0;
> > }
>
> The final code for the above part would look like:
>
> if (advice_add_nothing)
> advise(_("Use -f if you really want to add them."));
> ...
> if (advice_add_nothing)
> advise( _("Maybe you wanted to say 'git add .'?"));
>
> and then you would
>
> * add defn of advice_add_nothing to advice.h
> * add decl of the same, initialized to 1(true), to advice.c
> * map "addNothing" to &advice_add_nothing in advice.c::advice_config[]
>
> to complete the other half of this patch, if the config we choose to
> use is named "advice.addNothing".
>
Understood.
> By the way, notice that the single-liner advise() messages do not
> end with LF? This is another difference between printf() family and
> advise(). advise() cuts its message at LF and prefixes each piece
> with "hint:" but after the final LF there is nothing but NUL, which
> means the final LF is optional.
>
> The warning()/error()/die() family is different from advise() in
> that they do not chop the incoming message at LF. This behaviour is
> less i18n friendly, and it would be nice to eventually change them
> to behave similarly to advise().
>
Thank you for the extra tip.
> Thanks.
>
>
Heba
^ permalink raw reply
* [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
From: Heba Waly via GitGitGadget @ 2020-01-07 4:10 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano, Heba Waly
In-Reply-To: <pull.507.v2.git.1578370226.gitgitgadget@gmail.com>
From: Heba Waly <heba.waly@gmail.com>
Display a hint to the user when attempting to delete a checked out
branch.
Currently the user gets an error message saying: "error: Cannot delete
branch <branch> checked out at <path>". The hint will be displayed
after the error message.
Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
advice.c | 4 +++-
advice.h | 1 +
builtin/branch.c | 14 ++++++++++++++
t/t3200-branch.sh | 6 ++++--
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/advice.c b/advice.c
index 249c60dcf3..0a8fd2f68e 100644
--- a/advice.c
+++ b/advice.c
@@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;
int advice_checkout_ambiguous_remote_branch_name = 1;
int advice_nested_tag = 1;
int advice_submodule_alternate_error_strategy_die = 1;
+int advice_delete_checkedout_branch = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -91,7 +92,8 @@ static struct {
{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
{ "nestedTag", &advice_nested_tag },
{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-
+ { "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
+
/* make this an alias for backward compatibility */
{ "pushNonFastForward", &advice_push_update_rejected }
};
diff --git a/advice.h b/advice.h
index b706780614..e75c5ee33c 100644
--- a/advice.h
+++ b/advice.h
@@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;
extern int advice_checkout_ambiguous_remote_branch_name;
extern int advice_nested_tag;
extern int advice_submodule_alternate_error_strategy_die;
+extern int advice_delete_checkedout_branch;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..d1a9443e36 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
error(_("Cannot delete branch '%s' "
"checked out at '%s'"),
bname.buf, wt->path);
+ if (advice_delete_checkedout_branch) {
+ if (wt->is_current) {
+ advise(_("The branch you are trying to delete is already "
+ "checked out, run the following command to "
+ "checkout a different branch then try again:\n"
+ "git switch <branch>"));
+ }
+ else {
+ advise(_("The branch you are trying to delete is checked "
+ "out on another worktree, run the following command "
+ "to checkout a different branch then try again:\n"
+ "git -C %s switch <branch>"), wt->path);
+ }
+ }
ret = 1;
continue;
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..edb01bee45 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '
test_expect_success 'deleting currently checked out branch fails' '
git worktree add -b my7 my7 &&
- test_must_fail git -C my7 branch -d my7 &&
- test_must_fail git branch -d my7 &&
+ test_must_fail git -C my7 branch -d my7 2>output1.err &&
+ test_must_fail git branch -d my7 2>output2.err &&
+ test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err &&
+ test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err &&
rm -r my7 &&
git worktree prune
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting
From: Heba Waly via GitGitGadget @ 2020-01-07 4:10 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano
In-Reply-To: <pull.507.git.1577933387.gitgitgadget@gmail.com>
When a user attempts to delete a checked out branch, an error message is
displayed saying: "error: Cannot delete branch checked out at ". This patch
suggests displaying a hint after the error message advising the user to
checkout another branch first using "git checkout ".
Heba Waly (1):
branch: advise the user to checkout a different branch before deleting
advice.c | 4 +++-
advice.h | 1 +
builtin/branch.c | 14 ++++++++++++++
t/t3200-branch.sh | 6 ++++--
4 files changed, 22 insertions(+), 3 deletions(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/507
Range-diff vs v1:
1: 82bf24ce53 ! 1: 19a7cc1889 branch: advise the user to checkout a different branch before deleting
@@ -3,15 +3,48 @@
branch: advise the user to checkout a different branch before deleting
Display a hint to the user when attempting to delete a checked out
- branch saying "Checkout another branch before deleting this one:
- git checkout <branch_name>".
+ branch.
Currently the user gets an error message saying: "error: Cannot delete
- branch <branch_name> checked out at <path>". The hint will be displayed
+ branch <branch> checked out at <path>". The hint will be displayed
after the error message.
Signed-off-by: Heba Waly <heba.waly@gmail.com>
+ diff --git a/advice.c b/advice.c
+ --- a/advice.c
+ +++ b/advice.c
+@@
+ int advice_checkout_ambiguous_remote_branch_name = 1;
+ int advice_nested_tag = 1;
+ int advice_submodule_alternate_error_strategy_die = 1;
++int advice_delete_checkedout_branch = 1;
+
+ static int advice_use_color = -1;
+ static char advice_colors[][COLOR_MAXLEN] = {
+@@
+ { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
+ { "nestedTag", &advice_nested_tag },
+ { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
+-
++ { "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
++
+ /* make this an alias for backward compatibility */
+ { "pushNonFastForward", &advice_push_update_rejected }
+ };
+
+ diff --git a/advice.h b/advice.h
+ --- a/advice.h
+ +++ b/advice.h
+@@
+ extern int advice_checkout_ambiguous_remote_branch_name;
+ extern int advice_nested_tag;
+ extern int advice_submodule_alternate_error_strategy_die;
++extern int advice_delete_checkedout_branch;
+
+ int git_default_advice_config(const char *var, const char *value);
+ __attribute__((format (printf, 1, 2)))
+
diff --git a/builtin/branch.c b/builtin/branch.c
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -19,8 +52,20 @@
error(_("Cannot delete branch '%s' "
"checked out at '%s'"),
bname.buf, wt->path);
-+ advise(_("Checkout another branch before deleting this "
-+ "one: git checkout <branch_name>"));
++ if (advice_delete_checkedout_branch) {
++ if (wt->is_current) {
++ advise(_("The branch you are trying to delete is already "
++ "checked out, run the following command to "
++ "checkout a different branch then try again:\n"
++ "git switch <branch>"));
++ }
++ else {
++ advise(_("The branch you are trying to delete is checked "
++ "out on another worktree, run the following command "
++ "to checkout a different branch then try again:\n"
++ "git -C %s switch <branch>"), wt->path);
++ }
++ }
ret = 1;
continue;
}
@@ -29,12 +74,15 @@
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@
+
test_expect_success 'deleting currently checked out branch fails' '
git worktree add -b my7 my7 &&
- test_must_fail git -C my7 branch -d my7 &&
+- test_must_fail git -C my7 branch -d my7 &&
- test_must_fail git branch -d my7 &&
-+ test_must_fail git branch -d my7 >actual.out 2>actual.err &&
-+ test_i18ngrep "hint: Checkout another branch" actual.err &&
++ test_must_fail git -C my7 branch -d my7 2>output1.err &&
++ test_must_fail git branch -d my7 2>output2.err &&
++ test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err &&
++ test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err &&
rm -r my7 &&
git worktree prune
'
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones
From: Hans Jerry Illikainen @ 2020-01-07 4:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq36ctbis8.fsf@gitster-ct.c.googlers.com>
On Sun, Jan 05 2020, Junio C Hamano wrote:
> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> And finally, signature verification is added to the clone builtin. It
>> obeys --(no-)verify-signatures, clone.verifySignatures and
>> gpg.verifySignatures (in decreasing order of significance).
>
> I am not sure what it should mean to verify signature on clone. I'd
> assume that our threat model and verification policy are consistent
> with what we use for a merge/pull, in that we trust all history
> behind a commit that has a trusted signature, so it is clear that
> you would want the tip commit of the default branch (or if you are
> naming a single branch to clone, then the tip of that branch) to
> carry a trusted signature.
Yes, that's how it's implemented in v0 -- the tip of the branch/tag that
is to be checked out is verified.
> But what about the commits that are reachable from other branches and
> tags that are *not* contained in the branch that is initially checked
> out?
I thought about that and figured that adding signature verification for
the checkout builtin would be the next step after this series. That
thought should probably have been mentioned in the cover letter for
criticism -- because now I'm not sure if that's a reasonable approach
anymore.
> Later in the proposed log message of 5/5 you allude to risk of
> merely checking out a potentially untrustworthy contents to the
> working tree. This is far stricter than the usual threat model we
> tend to think about as the developers of source code management
> system, where checking out is perfectly OK but running "make" or its
> equivalent is the first contact between the victim's system with
> malicious contents.
Modern desktop environments perform enough magic that I think it makes
sense to assume that simply having malicious content on disk is enough
for a compromise -- even though no explicit user interaction takes place
with that content. The NSF bug in GStreamer that made headlines a few
years back is a good example of that [1]. And the numerous AV bugs
published by taviso [2].
> Verifying the tip of the default/sole branch upon cloning before the
> tree of the commit is checked out certainly would cover that single
> case (i.e. the initial checkout after cloning), but I am not sure if
> it is the best way, and I am reasonably certain it is insufficient
> against your threat model. After such a clone is made, when the
> user checks out another branch obtained from the "origin" remote,
> there is no mechanism that protects the user from the same risk you
> just covered with the new signature verification mechanism upon
> cloning, without validating the tip of that other branch, somewhere
> between the time the clone is made and that other branch gets
> checked out.
I agree. Again, I should have mentioned my thoughts on adding signature
verification to the checkout builtin in the cover letter.
> It almost makes me suspect that what you are trying to do with the
> "verification upon cloning" may be much better done as a "verify the
> tree for trustworthyness before checking it out to the working tree"
> mechanism, where the trustworthyness of a tree-ish object may be
> defined (and possibly made customizable by the policy of the project
> the user is working on) like so:
>
> - A tree is trustworthy if it is the tree of a trustworthy commit.
>
> - A commit is trustworthy if
>
> . it carries a trusted signature, or
> . it is pointed by a tag that carries a trusted signature, or
> . it is reachable from a trustworthy commit.
>
> Now, it is prohibitively expensive to compute the trusttworthiness
> of a tree, defined like the above, when checking it out, UNLESS you
> force each and every commit to carry a trusted signature, which is
> not necessarily practical in the real world.
Even though you mention that it's computationally expensive, I kind of
like this approach. It seems generalized enough that it doesn't need to
be tied to a single operation like 'clone'.
> Another approach to ensure that any and all checkout would work only
> on a trustworthy tree might be to make sure that tips of *all* the
> refs are trustworthy commits immediately after cloning (instead of
> verifying only the branch you are going to checkout, which is what
> your patch does IIUC). That way, any subsequent checkout in the
> repository would either be checking out a commit that was
>
> - originally cloned from the remote, and is reachable from some ref
> that was validated back when the repository was cloned, or
>
> - created locally in this repository, which we trust, or
>
> - fetched from somewhere else, and is reachable from the commit
> that was validated back when "git pull" validated what was about
> to be integrated into the history of *this* repository.
Wouldn't that still forgo signature verification when doing something
like:
$ git fetch
$ git checkout -b foo origin/branch-with-previously-unseen-commits
unless either fetch or checkout is equipped with signature verification?
Also, in case of a revoked key, this approach would require that tags
signed with that key be deleted on origin. Maybe that's to be
considered best practice anyway, but distro maintainers might not
appreciate disappearing release tags.
Also, an interesting observation (but probably a "not our problem" as
far as Git is concerned) -- I have noticed that certain git forges might
create branches unexpectedly. A few of my repos has dependabot/...
branches created by a GitHub bot that is enabled by default:
"""
Automated security updates are opened by Dependabot on behalf of
GitHub. The Dependabot GitHub App is automatically installed on every
repository where automated security updates are enabled.
"""
I can foresee a scenario where validating the tip of every ref on a
fresh clone would result in a DoS for lot of automated CI/CD-type
workflows when bots on GitHub (and other hosts) creates unexpected
branches in the users repos.
$ git branch -r
origin/HEAD -> origin/master
origin/dependabot/pip/requirements/typed-ast-1.3.2
origin/master
> Hmm?
I appreciate you taking the time to write your thoughts! Unfortunately
there doesn't seem to be an obvious approach that is significantly
preferable to the alternatives. I will experiment with the ideas you
mentioned above and see if I can come up with something that makes
sense.
Would you prefer that I break off the series before 5/5 in a new series
to fix the comments you made on the other patches that seem
almost-mergeable?
Thanks!
[1] https://scarybeastsecurity.blogspot.com/2016/11/0day-exploit-compromising-linux-desktop.html
[2] https://bugs.chromium.org/p/project-zero/issues/detail?id=769
--
hji
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Bryan Turner @ 2020-01-07 3:41 UTC (permalink / raw)
To: Jonathan Nieder
Cc: brian m. carlson, Git Users, Jeff King, Junio C Hamano, Miriam R.
In-Reply-To: <CAGyf7-GpCSaBauuKunDxBcZbcHU2sYkrt3+BpdF20dN8ZuPZgw@mail.gmail.com>
On Mon, Jan 6, 2020 at 7:40 PM Bryan Turner <bturner@atlassian.com> wrote:
>
> On Mon, Jan 6, 2020 at 6:04 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> >
> > brian m. carlson wrote:
> >
> > > In this function, we free the pointer we get from locate_in_PATH and
> > > then check whether it's NULL. However, this is undefined behavior if
> > > the pointer is non-NULL, since the C standard no longer permits us to
> > > use a valid pointer after freeing it.
> > [...]
> > > Noticed-by: Miriam R. <mirucam@gmail.com>
> > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > > ---
> > > run-command.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> >
> > This API that forces the caller to deal with the allocated result when
> > they never asked for it seems a bit inconvenient. Should we clean it up
> > a little? Something like this (on top):
> >
> > -- >8 --
> > Subject: run-command: let caller pass in buffer to locate_in_PATH
> >
> > Instead of returning a buffer that the caller is responsible for
> > freeing, use a strbuf output parameter to record the path to the
> > searched-for program.
> >
> > This makes ownership a little easier to reason about, since the owning
> > code declares the buffer. It's a good habit to follow because it
> > allows buffer reuse when calling such a function in a loop.
> >
> > It also allows the caller exists_in_PATH that does not care about the
> > path to the command to be slightly simplified, by allowing a NULL
> > output parameter that means that locate_in_PATH should take care of
> > allocating and freeing its temporary buffer.
> >
> > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> > ---
> > run-command.c | 51 +++++++++++++++++++++++++++++----------------------
> > 1 file changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git i/run-command.c w/run-command.c
> > index f5e1149f9b..a6dc38396a 100644
> > --- i/run-command.c
> > +++ w/run-command.c
> > @@ -170,52 +170,57 @@ int is_executable(const char *name)
> > * The caller should ensure that file contains no directory
> > * separators.
> > *
> > - * Returns the path to the command, as found in $PATH or NULL if the
> > - * command could not be found. The caller inherits ownership of the memory
> > - * used to store the resultant path.
> > + * Returns a boolean indicating whether the command was found in $PATH.
> > + * The path to the command is recorded in the strbuf 'out', if supplied.
> > *
> > * This should not be used on Windows, where the $PATH search rules
> > * are more complicated (e.g., a search for "foo" should find
> > * "foo.exe").
> > */
> > -static char *locate_in_PATH(const char *file)
> > +static int locate_in_PATH(const char *file, struct strbuf *out)
> > {
> > const char *p = getenv("PATH");
> > struct strbuf buf = STRBUF_INIT;
> >
> > - if (!p || !*p)
> > - return NULL;
> > + if (!out)
> > + out = &buf;
> > +
> > + if (!p || !*p) {
> > + strbuf_reset(out);
>
> Since the while loop and this block both call strbuf_reset(out);, is
> there a reason not to do:
> if (out)
> strbuf_reset(out);
> } else {
> out = &buf;
> }
> above? The loop below would still need its own reset, but it could do
> that when it decides to loop.
Ignore my incorrect braces; force of habit. I meant:
if (out)
strbuf_reset(out);
else
out = &buf;
>
> > + strbuf_release(&buf);
> > + return 0;
> > + }
> >
> > while (1) {
> > const char *end = strchrnul(p, ':');
> >
> > - strbuf_reset(&buf);
> > + strbuf_reset(out);
>
> This reset would be removed
>
> >
> > /* POSIX specifies an empty entry as the current directory. */
> > if (end != p) {
> > - strbuf_add(&buf, p, end - p);
> > - strbuf_addch(&buf, '/');
> > + strbuf_add(out, p, end - p);
> > + strbuf_addch(out, '/');
> > }
> > - strbuf_addstr(&buf, file);
> > + strbuf_addstr(out, file);
> >
> > - if (is_executable(buf.buf))
> > - return strbuf_detach(&buf, NULL);
> > + if (is_executable(out->buf)) {
> > + strbuf_release(&buf);
> > + return 1;
> > + }
> >
>
> We'd call strbuf_reset(out); here instead, before we break or loop.
>
> > if (!*end)
> > break;
> > p = end + 1
> > }
> >
> > + strbuf_reset(out);
>
> And we could leave this one out; if we make it here, we'd know the
> buffer was already reset.
>
> > strbuf_release(&buf);
> > - return NULL;
> > + return 0;
> > }
> >
> > static int exists_in_PATH(const char *file)
> > {
> > - char *r = locate_in_PATH(file);
> > - int found = r != NULL;
> > - free(r);
> > - return found;
> > + return locate_in_PATH(file, NULL);
> > }
> >
> > int sane_execvp(const char *file, char * const argv[])
> > @@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
> > * directly.
> > */
> > if (!strchr(out->argv[1], '/')) {
> > - char *program = locate_in_PATH(out->argv[1]);
> > - if (program) {
> > - free((char *)out->argv[1]);
> > - out->argv[1] = program;
> > - } else {
> > + struct strbuf program = STRBUF_INIT;
> > +
> > + if (!locate_in_PATH(out->argv[1], &program)) {
> > argv_array_clear(out);
> > + strbuf_release(&program);
> > errno = ENOENT;
> > return -1;
> > }
> > +
> > + free((char *)out->argv[1]);
> > + out->argv[1] = strbuf_detach(&program, NULL);
> > }
> >
> > return 0;
>
> Just a thought. (Pardon the noise from the peanut gallery!)
>
> Best regards,
> Bryan Turner
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Bryan Turner @ 2020-01-07 3:40 UTC (permalink / raw)
To: Jonathan Nieder
Cc: brian m. carlson, Git Users, Jeff King, Junio C Hamano, Miriam R.
In-Reply-To: <20200107020425.GG92456@google.com>
On Mon, Jan 6, 2020 at 6:04 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> brian m. carlson wrote:
>
> > In this function, we free the pointer we get from locate_in_PATH and
> > then check whether it's NULL. However, this is undefined behavior if
> > the pointer is non-NULL, since the C standard no longer permits us to
> > use a valid pointer after freeing it.
> [...]
> > Noticed-by: Miriam R. <mirucam@gmail.com>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > run-command.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> This API that forces the caller to deal with the allocated result when
> they never asked for it seems a bit inconvenient. Should we clean it up
> a little? Something like this (on top):
>
> -- >8 --
> Subject: run-command: let caller pass in buffer to locate_in_PATH
>
> Instead of returning a buffer that the caller is responsible for
> freeing, use a strbuf output parameter to record the path to the
> searched-for program.
>
> This makes ownership a little easier to reason about, since the owning
> code declares the buffer. It's a good habit to follow because it
> allows buffer reuse when calling such a function in a loop.
>
> It also allows the caller exists_in_PATH that does not care about the
> path to the command to be slightly simplified, by allowing a NULL
> output parameter that means that locate_in_PATH should take care of
> allocating and freeing its temporary buffer.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> run-command.c | 51 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git i/run-command.c w/run-command.c
> index f5e1149f9b..a6dc38396a 100644
> --- i/run-command.c
> +++ w/run-command.c
> @@ -170,52 +170,57 @@ int is_executable(const char *name)
> * The caller should ensure that file contains no directory
> * separators.
> *
> - * Returns the path to the command, as found in $PATH or NULL if the
> - * command could not be found. The caller inherits ownership of the memory
> - * used to store the resultant path.
> + * Returns a boolean indicating whether the command was found in $PATH.
> + * The path to the command is recorded in the strbuf 'out', if supplied.
> *
> * This should not be used on Windows, where the $PATH search rules
> * are more complicated (e.g., a search for "foo" should find
> * "foo.exe").
> */
> -static char *locate_in_PATH(const char *file)
> +static int locate_in_PATH(const char *file, struct strbuf *out)
> {
> const char *p = getenv("PATH");
> struct strbuf buf = STRBUF_INIT;
>
> - if (!p || !*p)
> - return NULL;
> + if (!out)
> + out = &buf;
> +
> + if (!p || !*p) {
> + strbuf_reset(out);
Since the while loop and this block both call strbuf_reset(out);, is
there a reason not to do:
if (out)
strbuf_reset(out);
} else {
out = &buf;
}
above? The loop below would still need its own reset, but it could do
that when it decides to loop.
> + strbuf_release(&buf);
> + return 0;
> + }
>
> while (1) {
> const char *end = strchrnul(p, ':');
>
> - strbuf_reset(&buf);
> + strbuf_reset(out);
This reset would be removed
>
> /* POSIX specifies an empty entry as the current directory. */
> if (end != p) {
> - strbuf_add(&buf, p, end - p);
> - strbuf_addch(&buf, '/');
> + strbuf_add(out, p, end - p);
> + strbuf_addch(out, '/');
> }
> - strbuf_addstr(&buf, file);
> + strbuf_addstr(out, file);
>
> - if (is_executable(buf.buf))
> - return strbuf_detach(&buf, NULL);
> + if (is_executable(out->buf)) {
> + strbuf_release(&buf);
> + return 1;
> + }
>
We'd call strbuf_reset(out); here instead, before we break or loop.
> if (!*end)
> break;
> p = end + 1
> }
>
> + strbuf_reset(out);
And we could leave this one out; if we make it here, we'd know the
buffer was already reset.
> strbuf_release(&buf);
> - return NULL;
> + return 0;
> }
>
> static int exists_in_PATH(const char *file)
> {
> - char *r = locate_in_PATH(file);
> - int found = r != NULL;
> - free(r);
> - return found;
> + return locate_in_PATH(file, NULL);
> }
>
> int sane_execvp(const char *file, char * const argv[])
> @@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
> * directly.
> */
> if (!strchr(out->argv[1], '/')) {
> - char *program = locate_in_PATH(out->argv[1]);
> - if (program) {
> - free((char *)out->argv[1]);
> - out->argv[1] = program;
> - } else {
> + struct strbuf program = STRBUF_INIT;
> +
> + if (!locate_in_PATH(out->argv[1], &program)) {
> argv_array_clear(out);
> + strbuf_release(&program);
> errno = ENOENT;
> return -1;
> }
> +
> + free((char *)out->argv[1]);
> + out->argv[1] = strbuf_detach(&program, NULL);
> }
>
> return 0;
Just a thought. (Pardon the noise from the peanut gallery!)
Best regards,
Bryan Turner
^ permalink raw reply
* Re: [PATCH 0/1] sequencer: comment out the 'squash!' line
From: Jonathan Nieder @ 2020-01-07 3:36 UTC (permalink / raw)
To: Jeff King
Cc: Mike Rappazzo, Junio C Hamano, Michael Rappazzo via GitGitGadget,
Git List
In-Reply-To: <20200106193253.GA971477@coredump.intra.peff.net>
Jeff King wrote:
> But I thought that was the point of "squash" versus "fixup"? One
> includes the commit message, and the other does not.
>
> I do think "commit --squash" is mostly useless for that reason, and I
> suspect we could do a better job in the documentation about pushing
> people to "--fixup".
>
> But --squash _can_ be useful with other options to populate the commit
> message (e.g., "--edit", which just pre-populates the subject with the
> right "squash!" line but lets you otherwise write a normal commit
> message). If that's the workflow you're using, then I'm sympathetic to
> auto-removing just a "squash!" line, as it's automated garbage that is
> only meant as a signal for --autosquash.
It's a signal for --autosquash and it gives a visual signal to humans
of where the squashed commit came from.
--squash already implies --edit, supporting this kind of workflow.
If we could turn back time and start over, would we want something
like the following?
1. if someone leaves the squash! message as is, include it as is in
the commit message without commenting out
2. if someone edits the squash! commit message to include a body
describing what is being squashed in, include the squash! line as
part of the commented marker
3. if someone leaves the (uncommented) squash! message in after being
presented with an editor at --autosquash time, reopen the editor
with some text verifying they really meant to do that
It's rare that concatenated commit messages make sense to be used as
is, especially when trailers (sign-offs, Fixes, etc) are involved. I
suspect that (3) is more important than (2) here --- we're using the
same space in the editor for input and output, and the result is a
kind of error-prone process of getting the output right.
Since we can't turn back time, one possibility would be to make tools
like "git show --check" notice the squash! lines. Would that be
useful?
One nice thing about (2) is that it's unlikely to affect scripted use.
Thoughts?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: brian m. carlson @ 2020-01-07 2:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano, Miriam R.
In-Reply-To: <20200107020425.GG92456@google.com>
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
On 2020-01-07 at 02:04:25, Jonathan Nieder wrote:
> brian m. carlson wrote:
>
> > In this function, we free the pointer we get from locate_in_PATH and
> > then check whether it's NULL. However, this is undefined behavior if
> > the pointer is non-NULL, since the C standard no longer permits us to
> > use a valid pointer after freeing it.
> [...]
> > Noticed-by: Miriam R. <mirucam@gmail.com>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > run-command.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> This API that forces the caller to deal with the allocated result when
> they never asked for it seems a bit inconvenient. Should we clean it up
> a little? Something like this (on top):
>
> -- >8 --
> Subject: run-command: let caller pass in buffer to locate_in_PATH
>
> Instead of returning a buffer that the caller is responsible for
> freeing, use a strbuf output parameter to record the path to the
> searched-for program.
>
> This makes ownership a little easier to reason about, since the owning
> code declares the buffer. It's a good habit to follow because it
> allows buffer reuse when calling such a function in a loop.
>
> It also allows the caller exists_in_PATH that does not care about the
> path to the command to be slightly simplified, by allowing a NULL
> output parameter that means that locate_in_PATH should take care of
> allocating and freeing its temporary buffer.
Sure, I think this is a nice improvement. The user can reuse the buffer
in a loop if they want by resetting it, which as you point out would be
convenient (and potentially more efficient). And we're already using
one internally, so there's no reason not to just pass one in.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Jonathan Nieder @ 2020-01-07 2:04 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King, Junio C Hamano, Miriam R.
In-Reply-To: <20200107013640.1821227-1-sandals@crustytoothpaste.net>
brian m. carlson wrote:
> In this function, we free the pointer we get from locate_in_PATH and
> then check whether it's NULL. However, this is undefined behavior if
> the pointer is non-NULL, since the C standard no longer permits us to
> use a valid pointer after freeing it.
[...]
> Noticed-by: Miriam R. <mirucam@gmail.com>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> run-command.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
This API that forces the caller to deal with the allocated result when
they never asked for it seems a bit inconvenient. Should we clean it up
a little? Something like this (on top):
-- >8 --
Subject: run-command: let caller pass in buffer to locate_in_PATH
Instead of returning a buffer that the caller is responsible for
freeing, use a strbuf output parameter to record the path to the
searched-for program.
This makes ownership a little easier to reason about, since the owning
code declares the buffer. It's a good habit to follow because it
allows buffer reuse when calling such a function in a loop.
It also allows the caller exists_in_PATH that does not care about the
path to the command to be slightly simplified, by allowing a NULL
output parameter that means that locate_in_PATH should take care of
allocating and freeing its temporary buffer.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
run-command.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git i/run-command.c w/run-command.c
index f5e1149f9b..a6dc38396a 100644
--- i/run-command.c
+++ w/run-command.c
@@ -170,52 +170,57 @@ int is_executable(const char *name)
* The caller should ensure that file contains no directory
* separators.
*
- * Returns the path to the command, as found in $PATH or NULL if the
- * command could not be found. The caller inherits ownership of the memory
- * used to store the resultant path.
+ * Returns a boolean indicating whether the command was found in $PATH.
+ * The path to the command is recorded in the strbuf 'out', if supplied.
*
* This should not be used on Windows, where the $PATH search rules
* are more complicated (e.g., a search for "foo" should find
* "foo.exe").
*/
-static char *locate_in_PATH(const char *file)
+static int locate_in_PATH(const char *file, struct strbuf *out)
{
const char *p = getenv("PATH");
struct strbuf buf = STRBUF_INIT;
- if (!p || !*p)
- return NULL;
+ if (!out)
+ out = &buf;
+
+ if (!p || !*p) {
+ strbuf_reset(out);
+ strbuf_release(&buf);
+ return 0;
+ }
while (1) {
const char *end = strchrnul(p, ':');
- strbuf_reset(&buf);
+ strbuf_reset(out);
/* POSIX specifies an empty entry as the current directory. */
if (end != p) {
- strbuf_add(&buf, p, end - p);
- strbuf_addch(&buf, '/');
+ strbuf_add(out, p, end - p);
+ strbuf_addch(out, '/');
}
- strbuf_addstr(&buf, file);
+ strbuf_addstr(out, file);
- if (is_executable(buf.buf))
- return strbuf_detach(&buf, NULL);
+ if (is_executable(out->buf)) {
+ strbuf_release(&buf);
+ return 1;
+ }
if (!*end)
break;
p = end + 1;
}
+ strbuf_reset(out);
strbuf_release(&buf);
- return NULL;
+ return 0;
}
static int exists_in_PATH(const char *file)
{
- char *r = locate_in_PATH(file);
- int found = r != NULL;
- free(r);
- return found;
+ return locate_in_PATH(file, NULL);
}
int sane_execvp(const char *file, char * const argv[])
@@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
* directly.
*/
if (!strchr(out->argv[1], '/')) {
- char *program = locate_in_PATH(out->argv[1]);
- if (program) {
- free((char *)out->argv[1]);
- out->argv[1] = program;
- } else {
+ struct strbuf program = STRBUF_INIT;
+
+ if (!locate_in_PATH(out->argv[1], &program)) {
argv_array_clear(out);
+ strbuf_release(&program);
errno = ENOENT;
return -1;
}
+
+ free((char *)out->argv[1]);
+ out->argv[1] = strbuf_detach(&program, NULL);
}
return 0;
^ permalink raw reply related
* Re: [Outreachy] Return value before or after free()?
From: brian m. carlson @ 2020-01-07 1:58 UTC (permalink / raw)
To: Jeff King, Miriam R., git
In-Reply-To: <20200107010809.GH6570@camp.crustytoothpaste.net>
[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]
On 2020-01-07 at 01:08:09, brian m. carlson wrote:
> Unfortunately, compilers have gotten much more aggressive about assuming
> that undefined behavior never occurs and rewriting code based on that.
> clang is not as bad about doing that, but GCC is very aggressive about
> it. There are multiple instances where NULL pointer checks have been
> optimized out because the compiler exploited undefined behavior to
> assume a pointer was never NULL.
>
> In this case, the only case in which we can safely assume that this
> behavior is acceptable is that r is NULL, in which case C11 tells us
> that "no action occurs" due to the free. So the compiler could just
> optimize this out to a "return 0". Just because it doesn't now doesn't
> mean we can assume it won't in the future, so we do need to fix this.
>
> I'll send a patch.
Oof, I just realized that you had tagged this with "[Outreachy]", which
means that you were probably planning on sending a patch to fix this,
and then I went and did it instead, so let me apologize for doing that.
I sent it because oftentimes we say "we should fix this thing" and then
never do it because nobody sends a patch, but in this case I should have
paid more attention and waited for you to respond and send one instead.
Again, sorry about that.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Junio C Hamano @ 2020-01-07 1:42 UTC (permalink / raw)
To: Stephen Oberholtzer; +Cc: git
In-Reply-To: <CAD_xR9dGNKdVNzFgFUaZCgJetpW5tXxb8wERovdjbc=1jS-KxA@mail.gmail.com>
Stephen Oberholtzer <stevie@qrpff.net> writes:
> ... a clean, portable way to create a single case statement that
> handles the final condition.
Hmm, don't we want to say (1) original 0 (success) maps to 1 (2)
original 125 maps to 125 (3) anything below 128 in the original maps
to 0 and (4) everything else is left intact? Perhaps something as
simple like this, taking advantage of the fact that $? won't have
anything other than digits?
... do something ...
status=$?
case $status in
0) status=1 ;;
125) : as-is ;;
? | ?? | 1[01]? | 12[0-7]) status=0;;
*) : as-is ;;
esac
I am still unclear where the need for {SUCCESS_FAIL}_TERM magic
comes from, though.
^ permalink raw reply
* [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: brian m. carlson @ 2020-01-07 1:36 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Miriam R.
In this function, we free the pointer we get from locate_in_PATH and
then check whether it's NULL. However, this is undefined behavior if
the pointer is non-NULL, since the C standard no longer permits us to
use a valid pointer after freeing it.
The only case in which the C standard would permit this to be defined
behavior is if r were NULL, since it states that in such a case "no
action occurs" as a result of calling free.
It's easy to suggest that this is not likely to be a problem, but we
know that GCC does aggressively exploit the fact that undefined
behavior can never occur to optimize and rewrite code, even when that's
contrary to the expectations of the programmer. It is, in fact, very
common for it to omit NULL pointer checks, just as we have here.
Since it's easy to fix, let's do so, and avoid a potential headache in
the future.
Noticed-by: Miriam R. <mirucam@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
run-command.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index 9942f120a9..f5e1149f9b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -213,8 +213,9 @@ static char *locate_in_PATH(const char *file)
static int exists_in_PATH(const char *file)
{
char *r = locate_in_PATH(file);
+ int found = r != NULL;
free(r);
- return r != NULL;
+ return found;
}
int sane_execvp(const char *file, char * const argv[])
^ permalink raw reply related
* Re: [PATCH] bisect run: allow '--' as an options terminator
From: Junio C Hamano @ 2020-01-07 1:34 UTC (permalink / raw)
To: Stephen Oberholtzer; +Cc: git, Christian Couder, Jonathan Nieder
In-Reply-To: <CAD_xR9faremhsGP4G65vHj=cvskUbK1rpPrQisa2_4AD2AGNYw@mail.gmail.com>
Stephen Oberholtzer <stevie@qrpff.net> writes:
>> I do not think I have seen enough to justify addition of "--",
>
> That's fine; I was just trying to be thorough (also, it was easy to
> test.) I was taught: if you accept any -options, honor -- as well. If
> you're not concerned about that, that's fine with me.
Ahh, that indeed is the crucial piece of information that was
missing. My review was about "Do we really *need* it?", but if you
are doing so from following a rule/dogma/principle, that changes the
equation quite a bit.
I do not think this project officially subscribes to the "anywhere
-option is taken should accept '--' as the end of options marker"
school, but because most modern command line processors use
parse_options() API which gets "--" for free, we can consider
ourselves practically accepting it already.
And adopting such a rule/dogma/principle frees us from having to
think about each individual case and helps us being consistent, so
it is not necessarily a bad thing as long as the cost of following
the rule/dogma/principle is not too onerous. At that point, what
needs to be reviewed instead becomes to "does this new code follow
the rule, and is it not bending backwards to do so?"
So, I do not have strong objection against "bisect run -- <cmd>".
It was, as I said in the original review, that it was so unclear
why double-dash is a logical consequence of accepting options,
because it was left unsaid. It would have been an easy sell if
this were a part of a patch that actually adds new option(s) and
explained perhaps like so:
Teach 'bisect run' to take --foo and --bar options, that
tell the command to do Foo and Bar. While at it, teach it
the common convention that "--" is used to terminate the
list of options. Nobody sane may try to use <cmd> that
begins with a dash, but supporting "--" everywhere we accept
a dashed option is good for consistency.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/1] sequencer: comment out the 'squash!' line
From: brian m. carlson @ 2020-01-07 1:34 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: Junio C Hamano, Michael Rappazzo via GitGitGadget, Git List
In-Reply-To: <CANoM8SV=pT3sFrfnEqWc2xBn_c2rES0qSMsdptF0DgcxgYL94w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]
On 2020-01-06 at 19:20:09, Mike Rappazzo wrote:
> On Mon, Jan 6, 2020 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Michael Rappazzo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > Since this change what the expected post-rebase commit comment would look
> > > like, related test expectations are adjusted to reflect the the new
> > > expectation. A new test is added for the new expectation.
> >
> > Doesn't that mean automated tools people may have written require
> > similar adjustment to continue working correctly if this change is
> > applied?
> >
> > Can you tell us more about your expected use case? I am imagining
> > that most people use the log messages from both/all commits being
> > squashed when manually editing to perfect the final log message (as
> > opposed to mechanically processing the concatenated message), so it
> > shouldn't matter if the squash! title is untouched or commented out
> > to them, and those (probably minority) who are mechanical processing
> > will be hurt with this change, so I do not quite see the point of
> > this patch.
>
> This change isn't removing the subject line from the commit message
> during the edit phase, it is only commenting it out. With the subject being
> commented out, it can minimize the effort to edit during the squash.
>
> Furthermore, it can help to eliminate accidental inclusion in the final
> message. Ultimately, the accidental inclusion is my motivation for
> submitting this.
I think this series would be useful. I've occasionally included the
"squash!" line in my commit even after I've edited the rest of the
commit message. It's not super frequent, but it is a hassle to have to
delete it, and it does happen occasionally. Usually I catch it before I
send out the series for review.
I can see the argument that this makes it a little harder for mechanical
processing across versions, but I suspect most of that looks something
like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be
affected. We do make occasional slightly incompatible changes across
versions in order to improve user experience, and I think a lot of folks
who use squash commits will find this a pleasant improvement.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [Outreachy] Return value before or after free()?
From: brian m. carlson @ 2020-01-07 1:08 UTC (permalink / raw)
To: Jeff King; +Cc: Miriam R., git
In-Reply-To: <20200106213051.GD980197@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]
On 2020-01-06 at 21:30:51, Jeff King wrote:
> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:
>
> > in run-command.c file `exists_in_PATH()` function does this:
> >
> > static int exists_in_PATH(const char *file)
> > {
> > char *r = locate_in_PATH(file);
> > free(r);
> > return r != NULL;
> > }
> >
> > I wonder if it is correct to do return r != NULL; after free(r);
>
> It is technically undefined behavior according to the C standard, but I
> think it would be hard to find an implementation where it was not
> perfectly fine in practice.
>
> Ref: http://c-faq.com/malloc/ptrafterfree.html
>
> I'd probably leave it alone unless it is causing a problem (e.g., a
> static analyzer complaining).
Unfortunately, compilers have gotten much more aggressive about assuming
that undefined behavior never occurs and rewriting code based on that.
clang is not as bad about doing that, but GCC is very aggressive about
it. There are multiple instances where NULL pointer checks have been
optimized out because the compiler exploited undefined behavior to
assume a pointer was never NULL.
In this case, the only case in which we can safely assume that this
behavior is acceptable is that r is NULL, in which case C11 tells us
that "no action occurs" due to the free. So the compiler could just
optimize this out to a "return 0". Just because it doesn't now doesn't
mean we can assume it won't in the future, so we do need to fix this.
I'll send a patch.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Stephen Oberholtzer @ 2020-01-07 0:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy2ul9yg0.fsf@gitster-ct.c.googlers.com>
Junio,
On Sun, Jan 5, 2020 at 8:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Yeah, I know. I didn't mean to give you a perfect solution and that
> was why I said "along the line of...". I know I ignored the 128 and
> above, as I usually trust that our contributors are competent enough
> to be able to fill in the missing details given an outline.
>
> The key takeaway I wanted you to notice was that a single case
> statement that maps the exit code external command would give us
> would look sufficient, without any of the {SUCCESS,FAIL}_TERM magic
> you had in your version, which indicates that there is more than the
> simple "using a run script to find where a bug was fixed can be done
> by swapping exit code" going on. And it is quite unclear why that
> is needed either from the patch or the text that accompanied the
> patch.
In this particular instance at least, I'm not competent enough to come
up with a clean, portable way to create a single case statement that
handles the final condition.
The issue I'm having is that case-esac blocks do string matching, not
integer value matching, so I don't know how to replicate the current
behavior without a case pattern that looks like
[12]|1[3-9]|1[01][0-9]|12[0-467]|1[3-9]|2[0-9]|[3-9].
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
^ permalink raw reply
* Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option
From: Jonathan Nieder @ 2020-01-07 0:05 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: git
In-Reply-To: <CAMxuvayT8FtovVnWU4bjQCP26drN37yuPG2+G2jAUsm0Ns_AYA@mail.gmail.com>
Marc-André Lureau wrote:
> On Sat, Jan 4, 2020 at 4:45 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Marc-André Lureau wrote:
>>> One of my most frequent mistake is to commit undesired submodules
>>> changes when doing "commit -a", and I have seen a number of people doing
>>> the same mistake in various projects. I wish there would be a config to
>>> change this default behaviour.
>>
>> Can you say more about the overall workflow this is part of? What
>> causes the submodules to change state in the first place here?
>
> The most common case is, I guess, when you work on different branches
> that have different (compatible) versions of the submodules.
Ah! This is because "git checkout" defaults to --no-recurse-submodules,
which is a terrible default.
Does "git config submodule.recurse true" help? If so, we can look
into which it would take to flip that default.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Nieder @ 2020-01-06 23:47 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20200106211730.GB980197@coredump.intra.peff.net>
Hi,
Jeff King wrote:
> On Fri, Jan 03, 2020 at 04:13:31PM -0800, Jonathan Nieder wrote:
>> To follow up on Junio's hint in his review: callers can inject
>> additional cached objects by using pretend_object_file. Junio
>> described how this would make sense as a mechanism for building
>> the virtual ancestor object, but we don't do that. In fact, the
>> only caller is fake_working_tree_commit in "git blame", a read-only
>> code path. *phew*
>>
>> -- >8 --
>> Subject: sha1-file: document how to use pretend_object_file
>> [...]
>> +/*
>> + * Add an object file to the in-memory object store, without writing it
>> + * to disk.
>> + *
>> + * Callers are responsible for calling write_object_file to record the
>> + * object in persistent storage before writing any other new objects
>> + * that reference it.
>> + */
>> int pretend_object_file(void *, unsigned long, enum object_type,
>> struct object_id *oid);
>
> I think this is an improvement over the status quo, but it's still a
> potential trap for code which happens to run in the same process (see my
> other email in the thread).
>
> Should the message perhaps be even more scary?
A pet peeve of mine is warning volume escalation: if it becomes common
for us to say
* Warning: callers are reponsible for [...]
then new warnings trying to stand out might say
* WARNING: callers are responsible for [...]
and then after we are desensitized to that, we may switch to
* WARNING WARNING WARNING, not the usual blah-blah: callers are
and so on. The main way I have found to counteract that is to make
the "dangerous curve" markers context-specific enough that people
don't learn to ignore them. After all, sometimes a concurrency
warning is important to me, at other times warnings about clarity may
be what attract my interest, and so on.
I don't have a good suggestion here. Perhaps "Callers are responsible
for" is too slow and something more terse would help?
/*
* Adds an object to the in-memory object store, without writing it
* to disk.
*
* Use with caution! Unless you call write_object_file to record the
* in-memory object to persistent storage, any other new objects that
* reference it will point to a missing (in memory only) object,
* resulting in a corrupt repository.
*/
It would be even better if we have some automated way to catch this
kind of issue. Should tests run "git fsck" after each test? Should
write_object_file have a paranoid mode that checks integrity?
I don't know an efficient way to do that. Ultimately I am comfortable
counting on reviewers to be aware of this kind of pitfall. While
nonlocal invariants are always hard to maintain, this pitfall is
inherent in the semantics of the function, so I am not too worried
that reviewers will overlook it.
A less error-prone interface would tie the result of
pretend_object_file to a short-lived overlay on the_repository without
affecting global state. We could even enforce read-only access in
that overlay. I don't think the "struct repository" interface and
callers are ready for that yet, though.
Thanks,
Jonathan
^ permalink raw reply
* Re: [Outreachy] Return value before or after free()?
From: Andreas Schwab @ 2020-01-06 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Miriam R., git
In-Reply-To: <20200106213051.GD980197@coredump.intra.peff.net>
On Jan 06 2020, Jeff King wrote:
> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:
>
>> in run-command.c file `exists_in_PATH()` function does this:
>>
>> static int exists_in_PATH(const char *file)
>> {
>> char *r = locate_in_PATH(file);
>> free(r);
>> return r != NULL;
>> }
>>
>> I wonder if it is correct to do return r != NULL; after free(r);
>
> It is technically undefined behavior according to the C standard, but I
> think it would be hard to find an implementation where it was not
> perfectly fine in practice.
Compilers get constantly better at exploiting undefined behaviour, so I
would not count on it.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: John Paul Adrian Glaubitz @ 2020-01-06 23:33 UTC (permalink / raw)
To: Thomas Braun, brian m. carlson, git
In-Reply-To: <2ce6a111-73ac-d8ac-e3a9-25286423535f@virtuell-zuhause.de>
On 1/7/20 12:31 AM, Thomas Braun wrote:
> On 04.01.2020 21:58, brian m. carlson wrote:
>
>> I got rid of my UltraSPARC hardware when I last moved, so I won't be
>> looking into this further, but it seems unlikely to me that the problem
>> is in Git when the only non-Windows C change between the two versions is
>> a one-line file name change.
>>
>
> Just in case someone is interested. Last year I've created a QEMU image
> using MIPS big-endian hardware [1]. It is still on debian stretch and
> like **slow** but does not require additional hardware.
FWIW, the GCC compile farm is supposed to get a new beefy MIPS server
soon as well so I have been told.
But as mentioned earlier, there is already a SPARC T5 in the GCC compile
farm which is open to use for any open-source developer [1].
Adrian
> [1] https://gcc.gnu.org/wiki/CompileFarm
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: Thomas Braun @ 2020-01-06 23:31 UTC (permalink / raw)
To: brian m. carlson, John Paul Adrian Glaubitz, git
In-Reply-To: <20200104205803.GE6570@camp.crustytoothpaste.net>
On 04.01.2020 21:58, brian m. carlson wrote:
> I got rid of my UltraSPARC hardware when I last moved, so I won't be
> looking into this further, but it seems unlikely to me that the problem
> is in Git when the only non-Windows C change between the two versions is
> a one-line file name change.
>
Just in case someone is interested. Last year I've created a QEMU image
using MIPS big-endian hardware [1]. It is still on debian stretch and
like **slow** but does not require additional hardware.
[1]: https://github.com/t-b/debian-mips-qemu
^ permalink raw reply
* Re: [PATCH] bisect run: allow '--' as an options terminator
From: Stephen Oberholtzer @ 2020-01-06 23:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Nieder
In-Reply-To: <xmqq1rsfddkq.fsf@gitster-ct.c.googlers.com>
Junio,
Sorry about that sloppy test script; that'll teach me to try cleaning
up a patch late at night.
On Fri, Jan 3, 2020 at 11:56 PM Junio C Hamano <gitster@pobox.com> wrote:
> I do not think I have seen enough to justify addition of "--",
That's fine; I was just trying to be thorough (also, it was easy to
test.) I was taught: if you accept any -options, honor -- as well. If
you're not concerned about that, that's fine with me.
> but
> addition of tests by itself may have a value, and I'd prefer to see
> it as its own patch. But I see hits for
>
> git grep 'git bisect run' t/
>
> already in t6030, so "adds an actual test script", while it is not
> telling a lie, may be giving a false impression that it is adding
> something new that has been missing.
Mmh, I didn't see those 'bisect run' tests in there; clearly, I didn't
look hard enough. I was distracted by several factors, including the
filename (t/t6030-bisect-porcelain.sh: I associated "porcelain" with
"output intended to be parsed". Since run doesn't have any meaningful
parse-able output, I did not expect to find it there.) and the fact
that bisect apparently has all of the test cases in one single script,
when (for example) rev-list currently has 23 different test scripts.
> > +exec </dev/null
>
> All of our test scripts are designed to be used unattended with the
> use of test_expect_* harness helpers. Can you tell us why this new
> file alone needs to dissociate the input to _all_ its subproesses by
> doing this, while others do not have to?
Ironically, that line was copied straight out of the top of t6030.
>
> > +. ./test-lib.sh
> > +
> > +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
>
> Yuck. I have to say this is trying to be too clever and cute than
> its worth. Doesn't it defeat test-lint, for example?
Actually, it doesn't. test-lint is too devious :-o
I was trying to adapt the script that I initially wrote to set up a
repo for manual testing. A heredoc was the easiest way to achieve
that without having to worry too much about escaping.
<more advice on readability and portability snipped>
> Thanks.
No, thank you, for putting up with me :)
Anyway, it sounds like: (a) the feature my patch adds isn't wanted or
needed, and (b) the functionality I *thought* I was adding test
coverage for already has test coverage, so we can kill this particular
thread entirely.
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
^ permalink raw reply
* Re: [PATCH 1/1] add: use advise function to display hints
From: Junio C Hamano @ 2020-01-06 23:18 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Heba Waly via GitGitGadget, git, Heba Waly
In-Reply-To: <20200106231327.GB181522@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
> Hm, I guess this answers my question above about them being 1:1. But I
> suppose it doesn't necessarily preclude advise() from associating a
> single config with multiple advice messages.
... and probably the other message in the thread from me would
answer any remaining question you may have, I guess ;-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox