| Message ID | 325d64e9-8a31-6ba0-73f2-5e9d67b8682f@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] branch: allow deleting dangling branches with --force | expand |
René Scharfe <l.s.r@web.de> writes: > + hash=$(git rev-parse HEAD) && > + objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") && > + git branch --no-track dangling && > + test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" && Do we need test -f here? > + mv $objpath $objpath.x && > + git branch --delete --force dangling && > + test -z "$(git for-each-ref refs/heads/dangling)" It is not wrong per-se, but maybe git show-ref --quiet refs/heads/dangling is more straight-forward. Thanks.
On Thu, Aug 26 2021, René Scharfe wrote: > - Added Reported-by and Helped-by. Thanks, this whole thing looks good to me. > - Made test independent of ref store. Also thanks. Just my 0.02: I think even with v1 this patch is fine to go in (but thanks for the re-roll!). I.e. under a full run of the testsuite with reftable a bunch of things are broken currently. It's not really that much more effort to just fix up code like in the v1 of this patch when we get to fixing those with the reftable integration, and putting the onus on patch authors on testing that topic in "seen" with their tests is probably not a good time investment overall v.s. just fixing them in bulk later. Particularly since in this case we can make it refstore independent, since it's about a disappearing loose object, but in some other cases it's either the whole test that needs to be skipped, or we'd be better off with some helpers to produce the corruption in one way under REFFILES, and in another way under !REFFILES....
Am 26.08.21 um 21:05 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> + hash=$(git rev-parse HEAD) && >> + objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") && >> + git branch --no-track dangling && >> + test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" && > > Do we need test -f here? If the mv in the next line fails, then test in the cleanup prevents it from adding another confusing error. So it's not really needed, but kinda nice to have. >> + mv $objpath $objpath.x && >> + git branch --delete --force dangling && > >> + test -z "$(git for-each-ref refs/heads/dangling)" > > It is not wrong per-se, but maybe > > git show-ref --quiet refs/heads/dangling > > is more straight-forward. Actually it *is* wrong, because that check passes even if the dangling ref still exists due to for-each-ref checking if the ref target exists and just erroring out if it doesn't. I somehow assumed it wouldn't do this needless verification. So we'd need to check its return value: git for-each-ref refs/heads/dangling >actual && test_must_be_empty actual git show-ref fails both if the ref is missing and if it's dangling, so we'd need to check its stderr to distinguish between those cases: test_must_fail git show-ref --quiet refs/heads/dangling 2>err && test_must_be_empty err To avoid these complications we could ask git branch itself: test -z $(git branch --list dangling) René
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index 94dc9a54f2..5449767121 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -118,7 +118,8 @@ OPTIONS Reset <branchname> to <startpoint>, even if <branchname> exists already. Without `-f`, 'git branch' refuses to change an existing branch. In combination with `-d` (or `--delete`), allow deleting the - branch irrespective of its merged status. In combination with + branch irrespective of its merged status, or whether it even + points to a valid commit. In combination with `-m` (or `--move`), allow renaming the branch even if the new branch name already exists, the same applies for `-c` (or `--copy`). diff --git a/builtin/branch.c b/builtin/branch.c index b23b1d1752..03c7b7253a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname, int kinds, int force) { struct commit *rev = lookup_commit_reference(the_repository, oid); - if (!rev) { + if (!force && !rev) { error(_("Couldn't look up commit object for '%s'"), refname); return -1; } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cc4b10236e..d0d28c8ea7 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1272,6 +1272,18 @@ test_expect_success 'attempt to delete a branch merged to its base' ' test_must_fail git branch -d my10 ' +test_expect_success 'branch --delete --force removes dangling branch' ' + git checkout main && + test_commit unstable && + hash=$(git rev-parse HEAD) && + objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") && + git branch --no-track dangling && + test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" && + mv $objpath $objpath.x && + git branch --delete --force dangling && + test -z "$(git for-each-ref refs/heads/dangling)" +' + test_expect_success 'use --edit-description' ' write_script editor <<-\EOF && echo "New contents" >"$1"
git branch only allows deleting branches that point to valid commits. Skip that check if --force is given, as the caller is indicating with it that they know what they are doing and accept the consequences. This allows deleting dangling branches, which previously had to be reset to a valid start-point using --force first. Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- Changes since v1: - Added Reported-by and Helped-by. - Made test independent of ref store. Documentation/git-branch.txt | 3 ++- builtin/branch.c | 2 +- t/t3200-branch.sh | 12 ++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) -- 2.33.0