Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
On Fri, Mar 03, 2017 at 02:56:05AM -0500, Jeff King wrote: > On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote: > > > +test_perf 'noop prune-empty' ' > > + git checkout --detach tip && > > + git filter-branch -f --prune-empty base..HEAD > > +' > > I don't mind adding this, but of curiosity, does it show anything > interesting? > > -Peff Nothing surprising; the overhead for the change was minimal. I wasn't sure what the practice is for adding unit/perf tests, so I erred on the side of covering everything. I will leave this as the last commit in the series so that it can be dropped or merged as you see fit. -- <><
Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
On Fri, Mar 03, 2017 at 02:55:35AM -0500, Jeff King wrote: > The only objectionable thing I noticed in the test additions is that > the early ones should be marked test_expect_failure until the fix from > 3/4 flips them to "success". Otherwise it breaks bisectability. > > -Peff Good point. Will fix in a v2 set then. -- <><
Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote: > "Devin J. Pohly" <djpo...@gmail.com> writes: > > > I think your point is interesting too, though. If a commit is also > > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems > > reasonable that someone might want to leave it in the filtered branch as > > an empty commit while pruning empt*ied* commits. I would imagine that > > as another option (--prune-newly-empty?). > > I was hoping to hear from others who may care about filter-branch to > comment on this topic to help me decide, but I haven't heard > anything, so here is my tentative thinking. > > I am leaning to: > > * Take your series as-is, which would mean --prune-empty will >change the behaviour to unconditionally lose the empty root. > > * Then, people who care deeply about it can add a new option that >prunes commits that become empty while keeping the originally >empty ones. > > Thoughts? Sounds good to me. I would be willing to work on a new option if needed (to "atone" for changing existing behavior), so you can loop me in if there are any complaints. -- <><
Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
On Thu, Feb 23, 2017 at 01:17:49PM -0800, Junio C Hamano wrote:
> "Devin J. Pohly" <djpo...@gmail.com> writes:
>
> > Previously, the git_commit_non_empty_tree function would always pass any
> > commit with no parents to git-commit-tree, regardless of whether the
> > tree was nonempty. The new commit would then be recorded in the
> > filter-branch revision map, and subsequent commits which leave the tree
> > untouched would be correctly filtered.
> >
> > With this change, parentless commits with an empty tree are correctly
> > pruned, and an empty file is recorded in the revision map, signifying
> > that it was rewritten to "no commits." This works naturally with the
> > parent mapping for subsequent commits.
> >
> > Signed-off-by: Devin J. Pohly <djpo...@gmail.com>
> > ---
>
> I am not sure if a root that records an empty tree should be pruned
> with --prune-empty to begin with.
>
> When we are pruning consecutive commits in the other parts of the
> history because they have identical (presumably non-empty) trees,
> should an empty root that the original history wanted to create be
> pruned because before the commit it was void, after the commit it is
> empty? Should "void" (lack of any tree) and "empty" (the tree is
> there, but it does not have anything in it) be treated the same?
> Shouldn't root be treated as a bit more special thing?
>
The case I had in mind was a filter which happened to remove all changes
from any parentless commit (see the testcase added to t7003). It would
not necessarily have been an empty commit in the original history.
Use case/motivation: I am splitting my dotfiles repo to migrate to vcsh,
and the original first commit (which only touches a few files) appears
in every branch. In the branches which do not include those files, the
commit is empty but still present.
I think your point is interesting too, though. If a commit is also
TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
reasonable that someone might want to leave it in the filtered branch as
an empty commit while pruning empt*ied* commits. I would imagine that
as another option (--prune-newly-empty?).
>
> I myself do not have a good answer to the above questions.
>
> I think the updated code makes sense, provided if we decide that
> void to empty is just like transitioning between two identical
> (presumably non-empty) trees. The updated documentation is a lot
> more readable as well.
>
> Comments from those who have been involved in filter-branch?
>
> > Documentation/git-filter-branch.txt | 14 ++
> > git-filter-branch.sh| 2 ++
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/git-filter-branch.txt
> > b/Documentation/git-filter-branch.txt
> > index 0a09698c0..6e4bb0220 100644
> > --- a/Documentation/git-filter-branch.txt
> > +++ b/Documentation/git-filter-branch.txt
> > @@ -167,14 +167,12 @@ to other tags will be rewritten to point to the
> > underlying commit.
> > project root. Implies <>.
> >
> > --prune-empty::
> > - Some kind of filters will generate empty commits, that left the tree
> > - untouched. This switch allow git-filter-branch to ignore such
> > - commits. Though, this switch only applies for commits that have one
> > - and only one parent, it will hence keep merges points. Also, this
> > - option is not compatible with the use of `--commit-filter`. Though you
> > - just need to use the function 'git_commit_non_empty_tree "$@"' instead
> > - of the `git commit-tree "$@"` idiom in your commit filter to make that
> > - happen.
> > + Some filters will generate empty commits that leave the tree untouched.
> > + This option instructs git-filter-branch to remove such commits if they
> > + have exactly one or zero non-pruned parents; merge commits will
> > + therefore remain intact. This option cannot be used together with
> > + `--commit-filter`, though the same effect can be achieved by using the
> > + provided `git_commit_non_empty_tree` function in a commit filter.
> >
> > --original ::
> > Use this option to set the namespace where the original commits
> > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > index 86b2ff1e0..2b8cdba15 100755
> > --- a/git-filter-branch.sh
> > +++ b/git-filter-branch.sh
> > @@ -46,6 +46,8 @@ git_commit_non_empty_tree()
> > {
> > if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
> > map "$3"
> > + elif test $# = 1 && test "$1" =
> > 4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
> > + :
> > else
> > git commit-tree "$@"
> > fi
--
<><
[PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
Previously, the git_commit_non_empty_tree function would always pass any
commit with no parents to git-commit-tree, regardless of whether the
tree was nonempty. The new commit would then be recorded in the
filter-branch revision map, and subsequent commits which leave the tree
untouched would be correctly filtered.
With this change, parentless commits with an empty tree are correctly
pruned, and an empty file is recorded in the revision map, signifying
that it was rewritten to "no commits." This works naturally with the
parent mapping for subsequent commits.
Signed-off-by: Devin J. Pohly <djpo...@gmail.com>
---
Documentation/git-filter-branch.txt | 14 ++
git-filter-branch.sh| 2 ++
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-filter-branch.txt
b/Documentation/git-filter-branch.txt
index 0a09698c0..6e4bb0220 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -167,14 +167,12 @@ to other tags will be rewritten to point to the
underlying commit.
project root. Implies <>.
--prune-empty::
- Some kind of filters will generate empty commits, that left the tree
- untouched. This switch allow git-filter-branch to ignore such
- commits. Though, this switch only applies for commits that have one
- and only one parent, it will hence keep merges points. Also, this
- option is not compatible with the use of `--commit-filter`. Though you
- just need to use the function 'git_commit_non_empty_tree "$@"' instead
- of the `git commit-tree "$@"` idiom in your commit filter to make that
- happen.
+ Some filters will generate empty commits that leave the tree untouched.
+ This option instructs git-filter-branch to remove such commits if they
+ have exactly one or zero non-pruned parents; merge commits will
+ therefore remain intact. This option cannot be used together with
+ `--commit-filter`, though the same effect can be achieved by using the
+ provided `git_commit_non_empty_tree` function in a commit filter.
--original ::
Use this option to set the namespace where the original commits
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86b2ff1e0..2b8cdba15 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -46,6 +46,8 @@ git_commit_non_empty_tree()
{
if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
map "$3"
+ elif test $# = 1 && test "$1" =
4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
+ :
else
git commit-tree "$@"
fi
--
2.11.1
[PATCH 4/4] p7000: add test for filter-branch with --prune-empty
Signed-off-by: Devin J. Pohly <djpo...@gmail.com> --- t/perf/p7000-filter-branch.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/perf/p7000-filter-branch.sh b/t/perf/p7000-filter-branch.sh index 15ee5d1d5..b029586cc 100755 --- a/t/perf/p7000-filter-branch.sh +++ b/t/perf/p7000-filter-branch.sh @@ -16,4 +16,9 @@ test_perf 'noop filter' ' git filter-branch -f base..HEAD ' +test_perf 'noop prune-empty' ' + git checkout --detach tip && + git filter-branch -f --prune-empty base..HEAD +' + test_done -- 2.11.1
[PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable
Sanity check before changing the logic in git_commit_non_empty_tree. Signed-off-by: Devin J. Pohly <djpo...@gmail.com> --- t/t7003-filter-branch.sh | 7 +++ 1 file changed, 7 insertions(+) diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index 2dfe46250..7cb60799b 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh @@ -371,6 +371,13 @@ test_expect_success '--prune-empty is able to prune root commit' ' test_cmp expect actual ' +test_expect_success '--prune-empty is able to prune entire branch' ' + git branch prune-entire B && + git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire && + test_path_is_missing .git/refs/heads/prune-entire && + test_must_fail git reflog exists refs/heads/prune-entire +' + test_expect_success '--remap-to-ancestor with filename filters' ' git checkout master && git reset --hard A && -- 2.11.1
[PATCH 1/4] t7003: ensure --prune-empty can prune root commit
New test to expose a bug in filter-branch whereby the root commit is never pruned, even though its tree is empty and --prune-empty is given. The setup isn't exactly pretty, but I couldn't think of a simpler way to create a parallel commit graph sans the first commit. Signed-off-by: Devin J. Pohly <djpo...@gmail.com> --- t/t7003-filter-branch.sh | 30 ++ 1 file changed, 30 insertions(+) diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index cb8fbd8e5..2dfe46250 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh @@ -313,6 +313,27 @@ test_expect_success 'Tag name filtering allows slashes in tag names' ' git cat-file tag X/2 > actual && test_cmp expect actual ' +test_expect_success 'setup --prune-empty comparisons' ' + git checkout --orphan master-no-a && + git rm -rf . && + unset test_tick && + test_tick && + GIT_COMMITTER_DATE="@0 +" GIT_AUTHOR_DATE="@0 +" && + test_commit --notick B B.t B Bx && + git checkout -b branch-no-a Bx && + test_commit D D.t D Dx && + mkdir dir && + test_commit dir/D dir/D.t dir/D dir/Dx && + test_commit E E.t E Ex && + git checkout master-no-a && + test_commit C C.t C Cx && + git checkout branch-no-a && + git merge Cx -m "Merge tag '\''C'\'' into branch" && + git tag Fx && + test_commit G G.t G Gx && + test_commit H H.t H Hx && + git checkout branch +' test_expect_success 'Prune empty commits' ' git rev-list HEAD > expect && @@ -341,6 +362,15 @@ test_expect_success 'prune empty works even without index/tree filters' ' test_cmp expect actual ' +test_expect_success '--prune-empty is able to prune root commit' ' + git rev-list branch-no-a >expect && + git branch testing H && + git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing && + git rev-list testing >actual && + git branch -D testing && + test_cmp expect actual +' + test_expect_success '--remap-to-ancestor with filename filters' ' git checkout master && git reset --hard A && -- 2.11.1
Re: [PATCH/RFC] remote-helper: allow fetch to discover sha1 later
On Thu, Sep 13, 2012 at 11:10:26PM -0700, Junio C Hamano wrote: I do not think it is a good idea to allow such a helper to claim that it supports fetch capability, for at least two reasons: * Being able to list is essential for fetch based helpers. think, far from arbitrary. ... Oh, I don't mean to change the semantics of the list command at all. What I thought seemed arbitrary was limiting the *fetch* command to refs with pre-known sha1s. Any list-time optimization in place or possible for such refs wouldn't be affected. (In light of this, specifying a new sha1 for a ref which already had one in the list should probably be an error rather than a warning. I'll update this in the next version and make it clear that a ref message must only be issued for refs reported without a sha1.) * Existing versions of git that can drive remote helpers that claim to have fetch capability are not prepared to accept an unknown refs protocol message from the helper, so a helper written for your new semantics of fetch capability will not work with them. ... I see. I didn't realize that new helper + old Git client is a supported combination. Still, hear me out. 1. A new helper must only send a ref message when git asks for a ref without a known sha1. 2. Asking for a ref without a known sha1 is unsupported, according to git-remote-helpers.txt: Only objects which were reported in the ref list with a sha1 may be fetched [with fetch]. 3. Furthermore, asking for a ref without a known sha1 *already* breaks in existing versions of git with current handlers: $ git fetch testfetch::../git1 foo fatal: bad object error: testfetch::../git1 did not send all necessary objects Compare - existing version of git, with a new handler: $ git fetch testref::../git1 foo warning: testref unexpectedly said: 'ref 0f31snip refs/heads/foo' fatal: bad object error: testref::../git1 did not send all necessary objects So the proposed change doesn't break anything that isn't already broken. :) That said, if you're still not sold, a new capability is fine. Though I think it can be done without it, I'm certainly not opposed to adding one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC] remote-helper: allow fetch to discover sha1 later
From: Devin J. Pohly djpo...@gmail.com
Allow a fetch-style remote helper to issue the notification
ref sha1 name
to specify a ref's value during the fetch operation.
Currently, a remote helper with the fetch capability cannot fetch a
ref unless its sha1 was known when the list command was executed.
This limitation is arbitrary, as the helper may eventually be able to
determine the correct sha1 as it fetches objects.
Signed-off-by: Devin J. Pohly djpo...@gmail.com
---
Soliciting general opinions - first git patch. :)
The fetch command can be a lot more convenient than import if you're working
with a remote that doesn't give you a commit and the associated objects at the
same time. And there's no apparent reason that something like this isn't
possible.
How it works: the old_sha1 field is currently set when the output from list
is parsed, then not used again until after the fetch command completes, just
before updating refs. Changing it during the fetch only affects the final
value of the ref. (Forgetting to resolve a ref will result in exactly the
same behavior as before: an error from check_everything_connected.)
Not sure if either or both of the warning()s should be a die() instead.
Documentation/git-remote-helpers.txt | 8 ++--
transport-helper.c | 24
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-remote-helpers.txt
b/Documentation/git-remote-helpers.txt
index f5836e4..fb4240f 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -229,8 +229,12 @@ Supported if the helper has the option capability.
to the database. Fetch commands are sent in a batch, one
per line, terminated with a blank line.
Outputs a single blank line when all fetch commands in the
- same batch are complete. Only objects which were reported
- in the ref list with a sha1 may be fetched this way.
+ same batch are complete.
++
+If the named ref was reported in the ref list without a sha1, must
+output a 'ref sha1 name' line once the sha1 is known. This is
+also required for a symref if its target did not have a sha1 in the
+list.
+
Optionally may output a 'lock file' line indicating a file under
GIT_DIR/objects/pack which is keeping a pack until refs can be
diff --git a/transport-helper.c b/transport-helper.c
index cfe0988..6fce419 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -363,6 +363,30 @@ static int fetch_with_fetch(struct transport *transport,
warning(%s also locked %s, data-name, name);
else
transport-pack_lockfile = xstrdup(name);
+ } else if (!prefixcmp(buf.buf, ref )) {
+ const char *rest = buf.buf + 4;
+ char *eov, *eon;
+ struct ref *posn;
+
+ eov = strchr(rest, ' ');
+ if (!eov)
+ die(Malformed ref command: %s, buf.buf);
+ eon = strchr(eov + 1, ' ');
+ *eov = '\0';
+ if (eon)
+ *eon = '\0';
+ for (i = 0; i nr_heads; i++) {
+ posn = to_fetch[i];
+ if (!strcmp(eov + 1, posn-name))
+ break;
+ }
+ if (i == nr_heads || (posn-status
REF_STATUS_UPTODATE)) {
+ warning(Ref %s is not being fetched, eov + 1);
+ continue;
+ }
+ if (!is_null_sha1(posn-old_sha1))
+ warning(Ref %s is being overwritten, eov + 1);
+ get_sha1_hex(rest, posn-old_sha1);
}
else if (!buf.len)
break;
--
1.7.12
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
