[PATCH 0/5] checkout --orphan improvements

[PATCH 0/5] checkout --orphan improvements

[Erick Mattos]

These series of patches are improvements to 'git checkout --orphan'.

The main reason for them is a corner case which is not being solved by
actual implementation.  As it is a quite improbable situation and as it was
necessary to do more extensive changes to support it then its development
was held to be presented in a new developing cycle.

When someone set core.logAllRefUpdates to false reflogs are not created
automatically.  This behavior is superseeded by -l option.  Actually this is
not allowed with --orphan by current implementation.  Those new patches are
made to fix that.

There are also two other patches for configuring completion in bash and to
enhance documentation.

To be completely honest I don't see a point of not having the reflogs
created and deleted automatically so I see no reason for -l and
core.logAllRefUpdates at all.  But I do not like to do anything partially
thus these new patches.  If someone could show me a case please do it.  ;-)

[PATCH 1/5] Documentation: alter checkout --orphan description

This one improves documentation text by late corrections from previous
threads.

[PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

Prepare the field by separating the logic to set up the reflog from the
reflog writing action.

[PATCH 3/5] checkout --orphan: respect -l option always

This is the actual actor.

[PATCH 4/5] t3200: test -l with core.logAllRefUpdates options

Adjusting scripts to test everything extensively.

[PATCH 5/5] bash completion: add --orphan to 'git checkout'

Just do that git change.

[PATCH 1/5] Documentation: alter checkout --orphan description

[Erick Mattos]

The present text is a try to enhance description accuracy.  It is a
merge of the rewritten text made by native english speaker Chris Johnsen
and further changes of Junio.  It came from the last thread messages of
--orphan patch.
---
 Documentation/git-checkout.txt |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 4505eb6..b84ec26 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -91,22 +91,29 @@ explicitly give a name with '-b' in such a case.
 	details.
 
 --orphan::
-	Create a new branch named <new_branch>, unparented to any other
-	branch.  The new branch you switch to does not have any commit
-	and after the first one it will become the root of a new history
-	completely unconnected from all the other branches.
+	Create a new 'orphan' branch, named <new_branch>, started from
+	<start_point> and switch to it.  The first commit made on this
+	new branch will have no parents and it will be the root of a new
+	history totally disconnected from all the other branches and
+	commits.
 +
-When you use "--orphan", the index and the working tree are kept intact.
-This allows you to start a new history that records set of paths similar
-to that of the start-point commit, which is useful when you want to keep
-different branches for different audiences you are working to like when
-you have an open source and commercial versions of a software, for example.
+The index and the working tree are adjusted as if you had previously run
+"git checkout <start_point>".  This allows you to start a new history
+that records a set of paths similar to <start_point> by easily running
+"git commit -a" to make the root commit.
 +
-If you want to start a disconnected history that records set of paths
-totally different from the original branch, you may want to first clear
-the index and the working tree, by running "git rm -rf ." from the
-top-level of the working tree, before preparing your files (by copying
-from elsewhere, extracting a tarball, etc.) in the working tree.
+This can be useful when you want to publish the tree from a commit
+without exposing its full history. You might want to do this to publish
+an open source branch of a project whose current tree is "clean", but
+whose full history contains proprietary or otherwise encumbered bits of
+code.
++
+If you want to start a disconnected history that records a set of paths
+that is totally different from the one of <start_point>, then you should
+clear the index and the working tree right after creating the orphan
+branch by running "git rm -rf ." from the top level of the working tree.
+Afterwards you will be ready to prepare your new files, repopulating the
+working tree, by copying them from elsewhere, extracting a tarball, etc.
 
 -m::
 --merge::
-- 
1.7.1.231.g0687c.dirty

[PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

[Erick Mattos]

Separation of the logic for testing and preparing the reflogs from
function log_ref_write to a new non static new function: log_ref_setup.

This allows to be performed from outside the first all reasonable checks
and procedures for writing reflogs.
---
 refs.c |   57 ++++++++++++++++++++++++++++++++++++---------------------
 refs.h |    3 +++
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index d3db15a..1161c2d 100644
--- a/refs.c
+++ b/refs.c
@@ -1258,52 +1258,67 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
-static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
-			 const unsigned char *new_sha1, const char *msg)
+int log_ref_setup(const char *ref_name, char **log_file)
 {
-	int logfd, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
-	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
-
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
-
-	git_snpath(log_file, sizeof(log_file), "logs/%s", ref_name);
+	int logfd, oflags = O_APPEND | O_WRONLY;
+	char logfile[PATH_MAX];
 
+	git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
+	*log_file = logfile;
 	if (log_all_ref_updates &&
 	    (!prefixcmp(ref_name, "refs/heads/") ||
 	     !prefixcmp(ref_name, "refs/remotes/") ||
 	     !prefixcmp(ref_name, "refs/notes/") ||
 	     !strcmp(ref_name, "HEAD"))) {
-		if (safe_create_leading_directories(log_file) < 0)
+		if (safe_create_leading_directories(*log_file) < 0)
 			return error("unable to create directory for %s",
-				     log_file);
+				     *log_file);
 		oflags |= O_CREAT;
 	}
 
-	logfd = open(log_file, oflags, 0666);
+	logfd = open(*log_file, oflags, 0666);
 	if (logfd < 0) {
 		if (!(oflags & O_CREAT) && errno == ENOENT)
 			return 0;
 
 		if ((oflags & O_CREAT) && errno == EISDIR) {
-			if (remove_empty_directories(log_file)) {
+			if (remove_empty_directories(*log_file)) {
 				return error("There are still logs under '%s'",
-					     log_file);
+					     *log_file);
 			}
-			logfd = open(log_file, oflags, 0666);
+			logfd = open(*log_file, oflags, 0666);
 		}
 
 		if (logfd < 0)
 			return error("Unable to append to %s: %s",
-				     log_file, strerror(errno));
+				     *log_file, strerror(errno));
 	}
 
-	adjust_shared_perm(log_file);
+	adjust_shared_perm(*log_file);
+	close(logfd);
+	return 0;
+}
 
+static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
+			 const unsigned char *new_sha1, const char *msg)
+{
+	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
+	unsigned maxlen, len;
+	int msglen;
+	char *log_file;
+	char *logrec;
+	const char *committer;
+
+	if (log_all_ref_updates < 0)
+		log_all_ref_updates = !is_bare_repository();
+
+	result = log_ref_setup(ref_name, &log_file);
+	if (result)
+		return result;
+
+	logfd = open(log_file, oflags);
+	if (logfd < 0)
+		return 0;
 	msglen = msg ? strlen(msg) : 0;
 	committer = git_committer_info(0);
 	maxlen = strlen(committer) + msglen + 100;
diff --git a/refs.h b/refs.h
index 4a18b08..594c9d9 100644
--- a/refs.h
+++ b/refs.h
@@ -68,6 +68,9 @@ extern void unlock_ref(struct ref_lock *lock);
 /** Writes sha1 into the ref specified by the lock. **/
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
+/** Setup reflog before using. **/
+int log_ref_setup(const char *ref_name, char **log_file);
+
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);
 
-- 
1.7.1.231.g0687c.dirty

[PATCH 3/5] checkout --orphan: respect -l option always

[Erick Mattos]

Added changes to satisfy a corner case: creating reflogs by using -l
when core.logAllRefUpdates is set to false.
---
 builtin/checkout.c         |   31 ++++++++++++++++--
 t/t2017-checkout-orphan.sh |   78 ++++++++++++++++++++++++++++++++------------
 2 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c382521..024c936 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -493,7 +493,24 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	struct strbuf msg = STRBUF_INIT;
 	const char *old_desc;
 	if (opts->new_branch) {
-		if (!opts->new_orphan_branch)
+		if (opts->new_orphan_branch) {
+			if (opts->new_branch_log && !log_all_ref_updates) {
+				int temp;
+				char *log_file;
+				char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
+
+				temp = log_all_ref_updates;
+				log_all_ref_updates = 1;
+				if (log_ref_setup(ref_name, &log_file)) {
+					fprintf(stderr, "Can not do reflog for '%s'\n",
+					    opts->new_orphan_branch);
+					log_all_ref_updates = temp;
+					return;
+				}
+				log_all_ref_updates = temp;
+			}
+		}
+		else
 			create_branch(old->name, opts->new_branch, new->name, 0,
 				      opts->new_branch_log, opts->track);
 		new->name = opts->new_branch;
@@ -517,6 +534,14 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 					opts->new_branch ? " a new" : "",
 					new->name);
 		}
+		if (old->path && old->name) {
+			char log_file[PATH_MAX], ref_file[PATH_MAX];
+
+			git_snpath(log_file, sizeof(log_file), "logs/%s", old->path);
+			git_snpath(ref_file, sizeof(ref_file), "%s", old->path);
+			if (!file_exists(ref_file) && file_exists(log_file))
+				remove_path(log_file);
+		}
 	} else if (strcmp(new->name, "HEAD")) {
 		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
 			   REF_NODEREF, DIE_ON_ERR);
@@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_orphan_branch) {
 		if (opts.new_branch)
 			die("--orphan and -b are mutually exclusive");
-		if (opts.track > 0 || opts.new_branch_log)
-			die("--orphan cannot be used with -t or -l");
+		if (opts.track > 0)
+			die("--orphan should not be used with -t");
 		opts.new_branch = opts.new_orphan_branch;
 	}
 
diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
index a8297c6..be88d4b 100755
--- a/t/t2017-checkout-orphan.sh
+++ b/t/t2017-checkout-orphan.sh
@@ -49,6 +49,62 @@ test_expect_success '--orphan must be rejected with -b' '
 	test refs/heads/master = "$(git symbolic-ref HEAD)"
 '
 
+test_expect_success '--orphan must be rejected with -t' '
+	git checkout master &&
+	test_must_fail git checkout --orphan new -t master &&
+	test refs/heads/master = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success '--orphan ignores branch.autosetupmerge' '
+	git checkout master &&
+	git config branch.autosetupmerge always &&
+	git checkout --orphan gamma &&
+	test -z "$(git config branch.gamma.merge)" &&
+	test refs/heads/gamma = "$(git symbolic-ref HEAD)" &&
+	test_must_fail git rev-parse --verify HEAD^
+'
+
+test_expect_success '--orphan makes reflog by default' '
+	git checkout master &&
+	git config --unset core.logAllRefUpdates &&
+	git checkout --orphan delta &&
+	! test -f .git/logs/refs/heads/delta &&
+	test_must_fail PAGER= git reflog show delta &&
+	git commit -m Delta &&
+	test -f .git/logs/refs/heads/delta &&
+	PAGER= git reflog show delta
+'
+
+test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '
+	git checkout master &&
+	git config core.logAllRefUpdates false &&
+	git checkout --orphan epsilon &&
+	! test -f .git/logs/refs/heads/epsilon &&
+	test_must_fail PAGER= git reflog show epsilon &&
+	git commit -m Epsilon &&
+	! test -f .git/logs/refs/heads/epsilon &&
+	test_must_fail PAGER= git reflog show epsilon
+'
+
+test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '
+	git checkout master &&
+	git checkout -l --orphan zeta &&
+	test -f .git/logs/refs/heads/zeta &&
+	test_must_fail PAGER= git reflog show zeta &&
+	git commit -m Zeta &&
+	PAGER= git reflog show zeta
+'
+
+test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
+	git checkout master &&
+	git checkout -l --orphan eta &&
+	test -f .git/logs/refs/heads/eta &&
+	test_must_fail PAGER= git reflog show eta &&
+	git checkout master &&
+	! test -f .git/logs/refs/heads/eta &&
+	test_must_fail PAGER= git reflog show eta
+'
+
 test_expect_success '--orphan is rejected with an existing name' '
 	git checkout master &&
 	test_must_fail git checkout --orphan master &&
@@ -60,31 +116,11 @@ test_expect_success '--orphan refuses to switch if a merge is needed' '
 	git reset --hard &&
 	echo local >>"$TEST_FILE" &&
 	cat "$TEST_FILE" >"$TEST_FILE.saved" &&
-	test_must_fail git checkout --orphan gamma master^ &&
+	test_must_fail git checkout --orphan new master^ &&
 	test refs/heads/master = "$(git symbolic-ref HEAD)" &&
 	test_cmp "$TEST_FILE" "$TEST_FILE.saved" &&
 	git diff-index --quiet --cached HEAD &&
 	git reset --hard
 '
 
-test_expect_success '--orphan does not mix well with -t' '
-	git checkout master &&
-	test_must_fail git checkout -t master --orphan gamma &&
-	test refs/heads/master = "$(git symbolic-ref HEAD)"
-'
-
-test_expect_success '--orphan ignores branch.autosetupmerge' '
-	git checkout -f master &&
-	git config branch.autosetupmerge always &&
-	git checkout --orphan delta &&
-	test -z "$(git config branch.delta.merge)" &&
-	test refs/heads/delta = "$(git symbolic-ref HEAD)" &&
-	test_must_fail git rev-parse --verify HEAD^
-'
-
-test_expect_success '--orphan does not mix well with -l' '
-	git checkout -f master &&
-	test_must_fail git checkout -l --orphan gamma
-'
-
 test_done
-- 
1.7.1.231.g0687c.dirty

[PATCH 4/5] t3200: test -l with core.logAllRefUpdates options

[Erick Mattos]

By default reflogs are always created for new local branches by
"checkout -b".  But by setting core.logAllRefUpdates to false this will
not be true anymore.

In that case you only create the reflogs when you use -l switch with
"checkout -b".

Added missing tests to check expected behaviors.
---
 t/t3200-branch.sh |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e0b7605..9d2c06e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -224,6 +224,30 @@ test_expect_success \
 	 test -f .git/logs/refs/heads/g/h/i &&
 	 diff expect .git/logs/refs/heads/g/h/i'
 
+test_expect_success 'checkout -b makes reflog by default' '
+	git checkout master &&
+	git config --unset core.logAllRefUpdates &&
+	git checkout -b alpha &&
+	test -f .git/logs/refs/heads/alpha &&
+	PAGER= git reflog show alpha
+'
+
+test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates = false' '
+	git checkout master &&
+	git config core.logAllRefUpdates false &&
+	git checkout -b beta &&
+	! test -f .git/logs/refs/heads/beta &&
+	test_must_fail PAGER= git reflog show beta
+'
+
+test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '
+	git checkout master &&
+	git checkout -lb gamma &&
+	git config --unset core.logAllRefUpdates &&
+	test -f .git/logs/refs/heads/gamma &&
+	PAGER= git reflog show gamma
+'
+
 test_expect_success 'avoid ambiguous track' '
 	git config branch.autosetupmerge true &&
 	git config remote.ambi1.url lalala &&
-- 
1.7.1.231.g0687c.dirty

[PATCH 5/5] bash completion: add --orphan to 'git checkout'

[Erick Mattos]

Updating git-completion.bash with new --orphan option to 'git checkout'.
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 545bd4b..b70cca1 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -841,7 +841,7 @@ _git_checkout ()
 	--*)
 		__gitcomp "
 			--quiet --ours --theirs --track --no-track --merge
-			--conflict= --patch
+			--conflict= --orphan --patch
 			"
 		;;
 	*)
-- 
1.7.1.231.g0687c.dirty

Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> -static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
> -			 const unsigned char *new_sha1, const char *msg)
> +int log_ref_setup(const char *ref_name, char **log_file)
>  {
> -	int logfd, written, oflags = O_APPEND | O_WRONLY;
> -	unsigned maxlen, len;
> -	int msglen;
> -	char log_file[PATH_MAX];
> -	char *logrec;
> -	const char *committer;
> -
> -	if (log_all_ref_updates < 0)
> -		log_all_ref_updates = !is_bare_repository();
> -
> -	git_snpath(log_file, sizeof(log_file), "logs/%s", ref_name);
> +	int logfd, oflags = O_APPEND | O_WRONLY;
> +	char logfile[PATH_MAX];
> +	git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
> +	*log_file = logfile;
> ...

I have a slight suspicion that it would have made the patch smaller and
easier to read if you kept the name of the on-stack log_file[] as-is, and
named the retval parameter logfile_p or soemthing.  Also you would need to
make this buffer "static char log_file[]", no?  Otherwise you would be
returning a pointer to a dead buffer to the caller.

> +static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
> +			 const unsigned char *new_sha1, const char *msg)
> +{
> + ...
> +	result = log_ref_setup(ref_name, &log_file);
> +	if (result)
> +		return result;
> +
> +	logfd = open(log_file, oflags);

Yuck, the caller needs to call "setup" which discards the file descriptor
opened for writing and then open it again itself?

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> +			git_snpath(ref_file, sizeof(ref_file), "%s", old->path);

???

> @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	if (opts.new_orphan_branch) {
>  		if (opts.new_branch)
>  			die("--orphan and -b are mutually exclusive");
> -		if (opts.track > 0 || opts.new_branch_log)
> -			die("--orphan cannot be used with -t or -l");
> +		if (opts.track > 0)
> +			die("--orphan should not be used with -t");

Why s/cannot/should not/?  Just being curious.

> +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
> +	git checkout master &&
> +	git checkout -l --orphan eta &&
> +	test -f .git/logs/refs/heads/eta &&
> +	test_must_fail PAGER= git reflog show eta &&
> +	git checkout master &&
> +	! test -f .git/logs/refs/heads/eta &&
> +	test_must_fail PAGER= git reflog show eta
> +'

I don't quite understand the title of this test, nor am I convinced that
testing for .git/logs/refs/heads/eta is necessarily a good thing to do
here.  "eta" branch is first prepared in an unborn state with the working
tree and the index prepared to commit what is in 'master', and the first
"git reflog" would fail because there is no eta branch at that point yet.
Moving to 'master' from that state would still leave "eta" branch unborn
and we will not see "git reflog" for that branch (we will fail "git log
eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
shouldn't be there?  It feels that it is testing a bit too low level an
implementation detail.

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Erick Mattos]

Hi,

2010/5/26 Junio C Hamano <gitster@pobox.com>
>
> Erick Mattos <erick.mattos@gmail.com> writes:
> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> >       if (opts.new_orphan_branch) {
> >               if (opts.new_branch)
> >                       die("--orphan and -b are mutually exclusive");
> > -             if (opts.track > 0 || opts.new_branch_log)
> > -                     die("--orphan cannot be used with -t or -l");
> > +             if (opts.track > 0)
> > +                     die("--orphan should not be used with -t");
>
> Why s/cannot/should not/?  Just being curious.

I have typed that text, not changed the original so this is not a fix
to your text.  Anyway for me "should not" is more polite, like "you
should not yell" meaning you really can not do it.  Or "you should not
disrespect the captain".

But that is not a fix.

> > +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
> > +     git checkout master &&
> > +     git checkout -l --orphan eta &&
> > +     test -f .git/logs/refs/heads/eta &&
> > +     test_must_fail PAGER= git reflog show eta &&
> > +     git checkout master &&
> > +     ! test -f .git/logs/refs/heads/eta &&
> > +     test_must_fail PAGER= git reflog show eta
> > +'
>
> I don't quite understand the title of this test, nor am I convinced that
> testing for .git/logs/refs/heads/eta is necessarily a good thing to do
> here.  "eta" branch is first prepared in an unborn state with the working
> tree and the index prepared to commit what is in 'master', and the first
> "git reflog" would fail because there is no eta branch at that point yet.
> Moving to 'master' from that state would still leave "eta" branch unborn
> and we will not see "git reflog" for that branch (we will fail "git log
> eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
> shouldn't be there?  It feels that it is testing a bit too low level an
> implementation detail.

So I need to explain the solution:

When config core.logAllRefUpdates is set to false what really happens
is that the reflog is not created and any reflog change is saved only
when you have an existent reflog.

What I did was to make a "touch reflog".  Creating it, when the new
branch get eventually saved then the reflog would be written normally.
 But in case somebody give up this new branch before the first save,
moving back to a regular branch would leave a ghost reflog.

I have coded the cleaning commands for that and the test is just a
check of this behavior.

The first "test -f .git/logs/refs/heads/eta" tests if reflog was
created and the second if it was deleted.  No big deal.

Regards

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Erik Faye-Lund]

On Wed, May 26, 2010 at 4:52 PM, Erick Mattos <erick.mattos@gmail.com> wrote:
> Hi,
>
> 2010/5/26 Junio C Hamano <gitster@pobox.com>
>>
>> Erick Mattos <erick.mattos@gmail.com> writes:
>> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>> >       if (opts.new_orphan_branch) {
>> >               if (opts.new_branch)
>> >                       die("--orphan and -b are mutually exclusive");
>> > -             if (opts.track > 0 || opts.new_branch_log)
>> > -                     die("--orphan cannot be used with -t or -l");
>> > +             if (opts.track > 0)
>> > +                     die("--orphan should not be used with -t");
>>
>> Why s/cannot/should not/?  Just being curious.
>
> I have typed that text, not changed the original so this is not a fix
> to your text.  Anyway for me "should not" is more polite, like "you
> should not yell" meaning you really can not do it.  Or "you should not
> disrespect the captain".

I don't think it makes sense to try and be polite when we're actually
refusing... "should not" implies that it possible but not recommended.
And in this case it's impossible, because we die()...

-- 
Erik "kusma" Faye-Lund

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Michael J Gruber]

Erick Mattos venit, vidit, dixit 26.05.2010 16:52:
> Hi,
> 
> 2010/5/26 Junio C Hamano <gitster@pobox.com>
>>
>> Erick Mattos <erick.mattos@gmail.com> writes:
>>> @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>>       if (opts.new_orphan_branch) {
>>>               if (opts.new_branch)
>>>                       die("--orphan and -b are mutually exclusive");
>>> -             if (opts.track > 0 || opts.new_branch_log)
>>> -                     die("--orphan cannot be used with -t or -l");
>>> +             if (opts.track > 0)
>>> +                     die("--orphan should not be used with -t");
>>
>> Why s/cannot/should not/?  Just being curious.
> 
> I have typed that text, not changed the original so this is not a fix
> to your text.  Anyway for me "should not" is more polite, like "you
> should not yell" meaning you really can not do it.  Or "you should not
> disrespect the captain".

"should not" means you can but you should not. "die" certainly means you
cannot. This is not a matter of politeness but of correctness.

> 
> But that is not a fix.

There's a "-" line with "cannot" and a "+" line with "should not". So
you certainly changed what was there before.

> 
>>> +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '

Really long line here ;)

>>> +     git checkout master &&
>>> +     git checkout -l --orphan eta &&
>>> +     test -f .git/logs/refs/heads/eta &&
>>> +     test_must_fail PAGER= git reflog show eta &&
>>> +     git checkout master &&
>>> +     ! test -f .git/logs/refs/heads/eta &&
>>> +     test_must_fail PAGER= git reflog show eta
>>> +'
>>
>> I don't quite understand the title of this test, nor am I convinced that
>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do
>> here.  "eta" branch is first prepared in an unborn state with the working
>> tree and the index prepared to commit what is in 'master', and the first
>> "git reflog" would fail because there is no eta branch at that point yet.
>> Moving to 'master' from that state would still leave "eta" branch unborn
>> and we will not see "git reflog" for that branch (we will fail "git log
>> eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
>> shouldn't be there?  It feels that it is testing a bit too low level an
>> implementation detail.
> 
> So I need to explain the solution:
> 
> When config core.logAllRefUpdates is set to false what really happens
> is that the reflog is not created and any reflog change is saved only
> when you have an existent reflog.
> 
> What I did was to make a "touch reflog".  Creating it, when the new

You mean checkout -l --orphan does that touch? There is none in the
test. Does ordinary checkout with -l does that, too?

> branch get eventually saved then the reflog would be written normally.
>  But in case somebody give up this new branch before the first save,
> moving back to a regular branch would leave a ghost reflog.

The touched entry (is left), not a reflog, I assume, otherwise the
reflog command should not fail.

> 
> I have coded the cleaning commands for that and the test is just a
> check of this behavior.

Which command does the cleaning? "reflog show" or "checkout master"?

> 
> The first "test -f .git/logs/refs/heads/eta" tests if reflog was
> created and the second if it was deleted.  No big deal.
> 
> Regards

I haven't followed this series due to earlier worries about --orphan but
I'm wondering about this cleaning up behind the back. Maybe it's just a
matter of explanations, though.

Michael

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Erick Mattos]

Hi,

2010/5/26 Erik Faye-Lund <kusmabite@googlemail.com>:
> I don't think it makes sense to try and be polite when we're actually
> refusing... "should not" implies that it possible but not recommended.
> And in this case it's impossible, because we die()...

Right then!  As I told it was not a fix.  So let's 's/should not/cannot/' then.

Regards

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Erick Mattos]

Hi,

2010/5/26 Michael J Gruber <git@drmicha.warpmail.net>:
>> But that is not a fix.
>
> There's a "-" line with "cannot" and a "+" line with "should not". So
> you certainly changed what was there before.

Everybody know what a minus or a plus sign means in a diff. ;-)

What I have meant was that I had typed the whole line myself after
some previous removal while I was making the changes during
"deletion/moving lines" actions.  No big deal, just a mistake.

The real message change here is from blocking -t an -l to blocking
only -t.  As I had told I have not realized the 'should not/cannot'
issue.

>>>> +     git checkout master &&
>>>> +     git checkout -l --orphan eta &&
>>>> +     test -f .git/logs/refs/heads/eta &&
>>>> +     test_must_fail PAGER= git reflog show eta &&
>>>> +     git checkout master &&
>>>> +     ! test -f .git/logs/refs/heads/eta &&
>>>> +     test_must_fail PAGER= git reflog show eta
>>>> +'
>>>
>>> I don't quite understand the title of this test, nor am I convinced that
>>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do
>>> here.  "eta" branch is first prepared in an unborn state with the working
>>> tree and the index prepared to commit what is in 'master', and the first
>>> "git reflog" would fail because there is no eta branch at that point yet.
>>> Moving to 'master' from that state would still leave "eta" branch unborn
>>> and we will not see "git reflog" for that branch (we will fail "git log
>>> eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
>>> shouldn't be there?  It feels that it is testing a bit too low level an
>>> implementation detail.
>>
>> So I need to explain the solution:
>>
>> When config core.logAllRefUpdates is set to false what really happens
>> is that the reflog is not created and any reflog change is saved only
>> when you have an existent reflog.
>>
>> What I did was to make a "touch reflog".  Creating it, when the new
>
> You mean checkout -l --orphan does that touch? There is none in the
> test. Does ordinary checkout with -l does that, too?

This is not done by a test.  It is part of the whole implementation.
It is done only when needed: on that special corner case.

Please read the patches mainly the 2/5 and 3/5.

>> branch get eventually saved then the reflog would be written normally.
>>  But in case somebody give up this new branch before the first save,
>> moving back to a regular branch would leave a ghost reflog.
>
> The touched entry (is left), not a reflog, I assume, otherwise the
> reflog command should not fail.
>
>>
>> I have coded the cleaning commands for that and the test is just a
>> check of this behavior.
>
> Which command does the cleaning? "reflog show" or "checkout master"?
>
>>
>> The first "test -f .git/logs/refs/heads/eta" tests if reflog was
>> created and the second if it was deleted.  No big deal.
>>
>> Regards
>
> I haven't followed this series due to earlier worries about --orphan but
> I'm wondering about this cleaning up behind the back. Maybe it's just a
> matter of explanations, though.
>
> Michael
>

Your questions are too unaware of the code.  ;-)  As I don't think you
are asking me to explain each single line then I imagine you have not
read the patches, just the chat.  Please read the patch series.  I
will be very glad to answer any further questions then.

Best regards

Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

[Erick Mattos]

Hi there,

2010/5/26 Junio C Hamano <gitster@pobox.com>:
> I have a slight suspicion that it would have made the patch smaller and
> easier to read if you kept the name of the on-stack log_file[] as-is, and
> named the retval parameter logfile_p or soemthing.

The size of the patch is indeed by the split/insertion which
complicates the diff's life.
If you compare both blobs you see it is not a hard change.  But we can
not hope for computer's intelligence during this lifetime.  ;-D

>  Also you would need to
> make this buffer "static char log_file[]", no?  Otherwise you would be
> returning a pointer to a dead buffer to the caller.

Not really.  git_snpath() is taking care of setting up the buffer
dynamically in the heap.  The calling function presents its buffer by
reference thus only the pointer's address which its content is later
changed to point to the dynamic one.

>> +static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
>> +                      const unsigned char *new_sha1, const char *msg)
>> +{
>> + ...
>> +     result = log_ref_setup(ref_name, &log_file);
>> +     if (result)
>> +             return result;
>> +
>> +     logfd = open(log_file, oflags);
>
> Yuck, the caller needs to call "setup" which discards the file descriptor
> opened for writing and then open it again itself?

The separation of logic of setup from writing of the reflog and the
consequently created log_ref_setup was meant to just prepare the
reflog file.  This way it can be used consistently on different
functions.

At the moment It is being used on log_ref_write() and in
update_refs_for_switch().  In the first case it is interesting that
the reflog keeps opened to be used.  On the later case it is not.  So,
one of the calling functions would have to do something.

We have two approaches to that:
1. keeping the reflog opened and making sure the calling function close it.
2. closing it and making the calling function open it or not as needed.

I have chosen 2 because of:
* I think it is safer to have any function closing open files,
cleaning variables or
  resources used by it whenever possible.
* It is more elegant that the function does what it is meant to do, in this case
  setting up the file only.
* It possibly keeps the code cleaner because only one 'close' for this function
  needs to be done and in the same place it happened the correspondent 'open'.
* No approach was going to cost any resources more.

Now just a question, Junio:

I forgot to sign-off those patches, should I have to send them again?

Regards

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Michael J Gruber]

Erick Mattos venit, vidit, dixit 26.05.2010 20:04:
> Hi,
> 
> 2010/5/26 Michael J Gruber <git@drmicha.warpmail.net>:
>>> But that is not a fix.
>>
>> There's a "-" line with "cannot" and a "+" line with "should not". So
>> you certainly changed what was there before.
> 
> Everybody know what a minus or a plus sign means in a diff. ;-)
> 
> What I have meant was that I had typed the whole line myself after
> some previous removal while I was making the changes during
> "deletion/moving lines" actions.  No big deal, just a mistake.
> 
> The real message change here is from blocking -t an -l to blocking
> only -t.  As I had told I have not realized the 'should not/cannot'
> issue.
> 
>>>>> +     git checkout master &&
>>>>> +     git checkout -l --orphan eta &&
>>>>> +     test -f .git/logs/refs/heads/eta &&
>>>>> +     test_must_fail PAGER= git reflog show eta &&
>>>>> +     git checkout master &&
>>>>> +     ! test -f .git/logs/refs/heads/eta &&
>>>>> +     test_must_fail PAGER= git reflog show eta
>>>>> +'
>>>>
>>>> I don't quite understand the title of this test, nor am I convinced that
>>>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do
>>>> here.  "eta" branch is first prepared in an unborn state with the working
>>>> tree and the index prepared to commit what is in 'master', and the first
>>>> "git reflog" would fail because there is no eta branch at that point yet.
>>>> Moving to 'master' from that state would still leave "eta" branch unborn
>>>> and we will not see "git reflog" for that branch (we will fail "git log
>>>> eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
>>>> shouldn't be there?  It feels that it is testing a bit too low level an
>>>> implementation detail.
>>>
>>> So I need to explain the solution:
>>>
>>> When config core.logAllRefUpdates is set to false what really happens
>>> is that the reflog is not created and any reflog change is saved only
>>> when you have an existent reflog.
>>>
>>> What I did was to make a "touch reflog".  Creating it, when the new
>>
>> You mean checkout -l --orphan does that touch? There is none in the
>> test. Does ordinary checkout with -l does that, too?
> 
> This is not done by a test.  It is part of the whole implementation.
> It is done only when needed: on that special corner case.
> 
> Please read the patches mainly the 2/5 and 3/5.
> 
>>> branch get eventually saved then the reflog would be written normally.
>>>  But in case somebody give up this new branch before the first save,
>>> moving back to a regular branch would leave a ghost reflog.
>>
>> The touched entry (is left), not a reflog, I assume, otherwise the
>> reflog command should not fail.
>>
>>>
>>> I have coded the cleaning commands for that and the test is just a
>>> check of this behavior.
>>
>> Which command does the cleaning? "reflog show" or "checkout master"?
>>
>>>
>>> The first "test -f .git/logs/refs/heads/eta" tests if reflog was
>>> created and the second if it was deleted.  No big deal.
>>>
>>> Regards
>>
>> I haven't followed this series due to earlier worries about --orphan but
>> I'm wondering about this cleaning up behind the back. Maybe it's just a
>> matter of explanations, though.
>>
>> Michael
>>
> 
> Your questions are too unaware of the code.  ;-)  As I don't think you
> are asking me to explain each single line then I imagine you have not
> read the patches, just the chat.  Please read the patch series.  I
> will be very glad to answer any further questions then.

I'm not asking you to explain your code but your intentions: What is it
supposed to do? If I have to read the code to figure that out then your
commit messages and on-list explanations (or my understanding thereof)
are suboptimal. You're cleaning up some files in logs/refs and I'm wondering

- which command does that automatically (after your series) and
- in which circumstances (only --orphan or more) superfluous files were
left there before.

If you're not willing to answer that I'm simply not reading the code.
One reviewer less.

Michael

Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> Now just a question, Junio:
>
> I forgot to sign-off those patches, should I have to send them again?

I would have appreciated a whole re-send while I was too distracted by
non-git stuff during the past few weeks, but now I am more or less settled
in, it's Ok to just send a separate "Signed-off-by:" like this one:

    http://article.gmane.org/gmane.comp.version-control.git/148230/raw

Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

[Erick Mattos]

Hi,

2010/6/2 Junio C Hamano <gitster@pobox.com>:
> Erick Mattos <erick.mattos@gmail.com> writes:
>
>> Now just a question, Junio:
>>
>> I forgot to sign-off those patches, should I have to send them again?
>
> I would have appreciated a whole re-send while I was too distracted by
> non-git stuff during the past few weeks, but now I am more or less settled
> in, it's Ok to just send a separate "Signed-off-by:" like this one:
>
>    http://article.gmane.org/gmane.comp.version-control.git/148230/raw
>

All right, following then:

2010/5/21 Erick Mattos <erick.mattos@gmail.com>:
> These series of patches are improvements to 'git checkout --orphan'.
>
> The main reason for them is a corner case which is not being solved by
> actual implementation.  As it is a quite improbable situation and as it was
> necessary to do more extensive changes to support it then its development
> was held to be presented in a new developing cycle.
>
> When someone set core.logAllRefUpdates to false reflogs are not created
> automatically.  This behavior is superseeded by -l option.  Actually this is
> not allowed with --orphan by current implementation.  Those new patches are
> made to fix that.
>
> There are also two other patches for configuring completion in bash and to
> enhance documentation.
>
> To be completely honest I don't see a point of not having the reflogs
> created and deleted automatically so I see no reason for -l and
> core.logAllRefUpdates at all.  But I do not like to do anything partially
> thus these new patches.  If someone could show me a case please do it.  ;-)
>
> [PATCH 1/5] Documentation: alter checkout --orphan description
>
> This one improves documentation text by late corrections from previous
> threads.
>
> [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup
>
> Prepare the field by separating the logic to set up the reflog from the
> reflog writing action.
>
> [PATCH 3/5] checkout --orphan: respect -l option always
>
> This is the actual actor.
>
> [PATCH 4/5] t3200: test -l with core.logAllRefUpdates options
>
> Adjusting scripts to test everything extensively.
>
> [PATCH 5/5] bash completion: add --orphan to 'git checkout'
>
> Just do that git change.

Signed-off-by: Erick Mattos <erick.mattos@gmail.com>

Re: [PATCH 3/5] checkout --orphan: respect -l option always

[Erick Mattos]

Hi Junio,

Just a small fix...

2010/5/26 Erik Faye-Lund <kusmabite@googlemail.com>:
> On Wed, May 26, 2010 at 4:52 PM, Erick Mattos <erick.mattos@gmail.com> wrote:
>> Hi,
>>
>> 2010/5/26 Junio C Hamano <gitster@pobox.com>
>>>
>>> Erick Mattos <erick.mattos@gmail.com> writes:
>>> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>> >       if (opts.new_orphan_branch) {
>>> >               if (opts.new_branch)
>>> >                       die("--orphan and -b are mutually exclusive");
>>> > -             if (opts.track > 0 || opts.new_branch_log)
>>> > -                     die("--orphan cannot be used with -t or -l");
>>> > +             if (opts.track > 0)
>>> > +                     die("--orphan should not be used with -t");
>>>
>>> Why s/cannot/should not/?  Just being curious.
>>
>> I have typed that text, not changed the original so this is not a fix
>> to your text.  Anyway for me "should not" is more polite, like "you
>> should not yell" meaning you really can not do it.  Or "you should not
>> disrespect the captain".
>
> I don't think it makes sense to try and be polite when we're actually
> refusing... "should not" implies that it possible but not recommended.
> And in this case it's impossible, because we die()...

If you agree, please do that 's/should not/cannot/' on pu.

Regards