Erick Mattos <erick.mattos@xxxxxxxxx> writes:
> @@ -25,6 +26,10 @@ linkgit:git-branch[1] were called; in this case you can
> use the --track or --no-track options, which will be passed to `git
> branch`. As a convenience, --track without `-b` implies branch
> creation; see the description of --track below.
> +When using -b, it is possible to use the option -o to set the new branch
> +as unparented thus unrelated to the previous branch. The new code will
> +be committed by using 'git add' and 'git commit' as if it was an initial
> +commit.
This says what the option does, but it is hard to guess why it would be a
good thing to do in the first place from the above description.
The use case in your commit log message wasn't convincing either. If such
a new branch will be merged later, especially if the trees of the commits
in newly rooted history resemble the trees in the original history (I am
guessing that is the intended use case, as you do not seem to be removing
anything from the working tree---how is the user expected to use them by
the way?), not having a common merge base will make the merge harder, not
easier, and later examination of the history (think "bisect") also
harder.
This looks like a "because we can" feeping creaturism, without any
"because it is beneficial if users can do this for this reason"
justification.
And what it can do "because we can" doesn't look very useful, safe, nor
sane either, with this particular implementation.
> @@ -509,8 +510,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,
> struct strbuf msg = STRBUF_INIT;
> const char *old_desc;
> if (opts->new_branch) {
> - create_branch(old->name, opts->new_branch, new->name, 0,
> - opts->new_branch_log, opts->track);
> + if (opts->new_branch_orphan) {
> + discard_cache();
> + remove_path(get_index_file());
I don't think we want to see "remove_path()" here. The function is about
the files in the work tree, and not about the files under .git/.
Currently the codepath to create and write out the index is abstracted
like this:
fd = hold_locked_index();
... populate the_index structure ...
write_cache(fd);
commit_locked_index();
and by only changing the implementation of these three functions, we could
store the index somewhere other than on the filesystem (say, database or
memcache). By using remove_path() on the return value of get_index_file()
in a random codepath like this one, you are pre-seeding a bug for other
people who may ant to make changes like that in the future.
But the above is just an advice on the coding, assuming that what is being
coded is sane, which unfortunately is not. You are nuking the index but
without doing anything to the working tree files. Why? The user manually
has to remove them? Or re-add them? I don't think you would want to call
discard_cache() _nor_ remove the index file here. This is probably even
more so if you think about a case where the user is using a sparse
checkout. Some random set of files are still in the working tree but
other files aren't, and the index used to keep track of which is what, but
you lost that information by discarding the cache.
If you are going to leave the files in the working tree intact, you should
make it the user's responsibility to run "git rm -f" after "checkout -o",
if the user wants to start from an empty index. That would also make it
safer; a mistaken "checkout -o" would be easier to recover from by running
reset without --hard, if it does not touch the index.
By the way, the worst part of this patch is that I didn't see any safety
checks tested in the test script. What prevents the users from typing an
extra -o by mistake, while having some changes to the index and/or the
files in the working tree? Even if you change it not to touch the index,
it would probably make sense to make sure this "feature" is a lot harder
to invoke by mistake. In a sane workflow you wouldn't be creating root
commits left and right. Perhaps by not giving it a '-o' shorthand would
be a good start.
In the above I assumed (by guessing from the fact that you are not
touching files in the working tree) that the tree eventually committed by
the user on this newly rooted history would have some resemblance to the
trees in the original history. But if -o is intended to be used for
really "starting from void" with an empty tree, then I think the option
should instead do this:
- run an equivalent of "git rm .", which includes:
- make sure no change is in the index relative to HEAD; otherwise error
out.
- make sure no change is in the working tree relative to the index;
otherwise error out.
- discard_cache() and write that empty index out;
- make the HEAD dangling like you did.
That would be safe and halfway useful _if_ somebody wants to start a truly
new history from scratch, even though if you want to have an unrelated
history in your repository it would be more natural do so by fetching such
an unrelated history from an unrelated repository.
The way the patch implements is not suited for neither "starting from
void" case, nor "continuing the tree but with disjoint history" case, I
think.
> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
> new file mode 100755
> index 0000000..7170641
> --- /dev/null
> +++ b/t/t2017-checkout-orphan.sh
> @@ -0,0 +1,50 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2010 Erick Mattos
> +#
> +
> +test_description='git checkout -b
> +
> +Tests for -o functionality.'
> +
> +. ./test-lib.sh
> +
> +TEST_FILE=foo
> +
> +test_expect_success 'Setup' '
> + echo "initial" > "$TEST_FILE" &&
This is just style, but I find it easier to read if you have one SP before
redirect and no SP between redirect and the filename, i.e.:
echo initial >"$TEST_FILE" &&
> + test "alpha" = "$(git branch | sed -n "/*/s/\* //p")" &&
Don't read from "git branch" in scripts; use symbolic-ref on HEAD.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html