From: greened@obbligato•org (David A. Greene)
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org, sunshine@sunshineco•com,
davidw@realtimegenomics•com
Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits
Date: Tue, 19 Jan 2016 22:10:51 -0600 [thread overview]
Message-ID: <87wpr4ubzo.fsf@waller.obbligato.org> (raw)
In-Reply-To: <xmqqsi1tbh68.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Tue, 19 Jan 2016 09:41:35 -0800")
Junio C Hamano <gitster@pobox•com> writes:
> Thanks, both. Will queue.
>
> I have a couple of questions and comments, though.
>
>> +test_expect_success 'setup' '
>> + test_commit README &&
>> +...
>> + tree=$(git write-tree) &&
>> + head=$(git rev-parse HEAD) &&
>> + rev=$(git rev-parse --verify files-master^0) &&
>> + commit=$(git commit-tree -p $head -p $rev -m "Add subproject master" $tree) &&
>
> I think at this point, your index and working tree states match that
> of $commit. So the next command ...
>
>> + git reset $commit &&
>
> ... made me wonder what its significance was. I think you are doing
> this solely to move the HEAD pointer to point at $commit, but then
> it would be much better and more readable to use update-ref, i.e.
> making this line to:
>
> git update-ref HEAD $commit &&
>
> instead, as "write-tree && commit-tree && update-ref" is a familiar
> pattern to reimplement "git commit" using the plumbing. Ending that
> three-command sequence with "reset" breaks the pattern.
Ok, that makes sense.
>> + (
>> + cd files_subtree &&
>> + test_commit master4
>> + ) &&
>> + test_commit files_subtree/master5
>
> I understand that you are creating these two commits both in the
> top-level repository (the one the history initially created in
> "files" repository gets merged into), but you are creating them
> slightly differently. Is that significant? I am not complaining
> about the style of writing tests, but I am wondering if having these
> two commits created differently has any effect on the bug you
> observed, which may be a good starting point for anybody who wants
> to fix it to start digging from. IOW, would the resulting history
> different if you did this instead?
>
> test_commit files_subtree/master4 &&
> test_commit files_subtree/master5
That is a good question. I originally created the test to see if making
these two commits differently would cause any problems with
git-subtree's split command. Obviously, I didn't get that far. :)
So I think it makes sense for me to at least test and see what happens.
I will add another test to this set if it makes a difference.
> I also notice that files_subtree/master4 does not appear in any of
> the verification in the three tests that use the history being
> prepared here, i.e. if master4 is silently dropped while master5 is
> kept, such a bug won't be caught by them.
Ah, good catch. I should add a test for that.
Let me do a re-roll of this since I think you bring up some excellent
points. Might be a few days due to work obbligations.
-David
next prev parent reply other threads:[~2016-01-20 4:11 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-05 4:40 [PATCH] Test rebase -Xsubtree David Greene
2016-01-05 4:40 ` [PATCH] Add a test for subtree rebase that loses commits David Greene
2016-01-05 8:47 ` Torsten Bögershausen
2016-01-05 9:57 ` Dennis Kaarsemaker
2016-01-05 11:18 ` Torsten Bögershausen
2016-01-05 20:34 ` Eric Sunshine
2016-01-05 21:14 ` David A. Greene
2016-01-10 23:08 ` [PATCH v2] Test rebase -Xsubtree David Greene
2016-01-10 23:08 ` [PATCH] Add a test for subtree rebase that loses commits David Greene
2016-01-15 1:19 ` Eric Sunshine
2016-01-17 22:50 ` David A. Greene
2016-01-17 23:13 ` [PATCH v3 contrib/subtree 1/1] " David Greene
2016-01-17 23:32 ` Eric Sunshine
2016-01-17 23:36 ` David A. Greene
2016-01-17 23:43 ` [PATCH v4 1/1] contrib/subtree: " David Greene
2016-01-18 18:10 ` Eric Sunshine
2016-01-19 2:53 ` David A. Greene
2016-01-19 2:59 ` [PATCH v5 " David Greene
2016-01-19 4:21 ` Eric Sunshine
2016-01-19 17:41 ` Junio C Hamano
2016-01-20 4:10 ` David A. Greene [this message]
2016-04-12 23:27 ` Junio C Hamano
2016-06-28 10:55 ` David A. Greene
2016-06-28 10:54 ` [PATCH v6 " David Greene
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wpr4ubzo.fsf@waller.obbligato.org \
--to=greened@obbligato$(echo .)org \
--cc=davidw@realtimegenomics$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox