{"thread":{"id":"23872","subject":"[PATCH 0/5] checkout --orphan improvements","startedAt":"2010-05-22T00:28:34Z","lastAt":"2010-06-03T16:28:48Z","messageCount":18,"participants":["Erick Mattos","Junio C Hamano","Erik Faye-Lund","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"142071","messageId":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":null,"subject":"[PATCH 0/5] checkout --orphan improvements","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:28:34Z","receivedAt":"2010-05-22T00:28:34Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"These series of patches are improvements to 'git checkout --orphan'.\n\nThe main reason for them is a corner case which is not being solved by\nactual implementation.  As it is a quite improbable situation and as it was\nnecessary to do more extensive changes to support it then its development\nwas held to be presented in a new developing cycle.\n\nWhen someone set core.logAllRefUpdates to false reflogs are not created\nautomatically.  This behavior is superseeded by -l option.  Actually this is\nnot allowed with --orphan by current implementation.  Those new patches are\nmade to fix that.\n\nThere are also two other patches for configuring completion in bash and to\nenhance documentation.\n\nTo be completely honest I don't see a point of not having the reflogs\ncreated and deleted automatically so I see no reason for -l and\ncore.logAllRefUpdates at all.  But I do not like to do anything partially\nthus these new patches.  If someone could show me a case please do it.  ;-)\n\n[PATCH 1/5] Documentation: alter checkout --orphan description\n\nThis one improves documentation text by late corrections from previous\nthreads.\n\n[PATCH 2/5] refs: split log_ref_write logic into log_ref_setup\n\nPrepare the field by separating the logic to set up the reflog from the\nreflog writing action.\n\n[PATCH 3/5] checkout --orphan: respect -l option always\n\nThis is the actual actor.\n\n[PATCH 4/5] t3200: test -l with core.logAllRefUpdates options\n\nAdjusting scripts to test everything extensively.\n\n[PATCH 5/5] bash completion: add --orphan to 'git checkout'\n\nJust do that git change.\n"},{"id":"142072","messageId":"1274488119-6989-2-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","subject":"[PATCH 1/5] Documentation: alter checkout --orphan description","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:28:35Z","receivedAt":"2010-05-22T00:28:35Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"The present text is a try to enhance description accuracy.  It is a\nmerge of the rewritten text made by native english speaker Chris Johnsen\nand further changes of Junio.  It came from the last thread messages of\n--orphan patch.\n---\n Documentation/git-checkout.txt |   35 +++++++++++++++++++++--------------\n 1 files changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex 4505eb6..b84ec26 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -91,22 +91,29 @@ explicitly give a name with '-b' in such a case.\n \tdetails.\n \n --orphan::\n-\tCreate a new branch named <new_branch>, unparented to any other\n-\tbranch.  The new branch you switch to does not have any commit\n-\tand after the first one it will become the root of a new history\n-\tcompletely unconnected from all the other branches.\n+\tCreate a new 'orphan' branch, named <new_branch>, started from\n+\t<start_point> and switch to it.  The first commit made on this\n+\tnew branch will have no parents and it will be the root of a new\n+\thistory totally disconnected from all the other branches and\n+\tcommits.\n +\n-When you use \"--orphan\", the index and the working tree are kept intact.\n-This allows you to start a new history that records set of paths similar\n-to that of the start-point commit, which is useful when you want to keep\n-different branches for different audiences you are working to like when\n-you have an open source and commercial versions of a software, for example.\n+The index and the working tree are adjusted as if you had previously run\n+\"git checkout <start_point>\".  This allows you to start a new history\n+that records a set of paths similar to <start_point> by easily running\n+\"git commit -a\" to make the root commit.\n +\n-If you want to start a disconnected history that records set of paths\n-totally different from the original branch, you may want to first clear\n-the index and the working tree, by running \"git rm -rf .\" from the\n-top-level of the working tree, before preparing your files (by copying\n-from elsewhere, extracting a tarball, etc.) in the working tree.\n+This can be useful when you want to publish the tree from a commit\n+without exposing its full history. You might want to do this to publish\n+an open source branch of a project whose current tree is \"clean\", but\n+whose full history contains proprietary or otherwise encumbered bits of\n+code.\n++\n+If you want to start a disconnected history that records a set of paths\n+that is totally different from the one of <start_point>, then you should\n+clear the index and the working tree right after creating the orphan\n+branch by running \"git rm -rf .\" from the top level of the working tree.\n+Afterwards you will be ready to prepare your new files, repopulating the\n+working tree, by copying them from elsewhere, extracting a tarball, etc.\n \n -m::\n --merge::\n-- \n1.7.1.231.g0687c.dirty\n"},{"id":"142073","messageId":"1274488119-6989-3-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","subject":"[PATCH 2/5] refs: split log_ref_write logic into log_ref_setup","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:28:36Z","receivedAt":"2010-05-22T00:28:36Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Separation of the logic for testing and preparing the reflogs from\nfunction log_ref_write to a new non static new function: log_ref_setup.\n\nThis allows to be performed from outside the first all reasonable checks\nand procedures for writing reflogs.\n---\n refs.c |   57 ++++++++++++++++++++++++++++++++++++---------------------\n refs.h |    3 +++\n 2 files changed, 39 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d3db15a..1161c2d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1258,52 +1258,67 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n-static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n-\t\t\t const unsigned char *new_sha1, const char *msg)\n+int log_ref_setup(const char *ref_name, char **log_file)\n {\n-\tint logfd, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n-\tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n-\n-\tif (log_all_ref_updates < 0)\n-\t\tlog_all_ref_updates = !is_bare_repository();\n-\n-\tgit_snpath(log_file, sizeof(log_file), \"logs/%s\", ref_name);\n+\tint logfd, oflags = O_APPEND | O_WRONLY;\n+\tchar logfile[PATH_MAX];\n \n+\tgit_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n+\t*log_file = logfile;\n \tif (log_all_ref_updates &&\n \t    (!prefixcmp(ref_name, \"refs/heads/\") ||\n \t     !prefixcmp(ref_name, \"refs/remotes/\") ||\n \t     !prefixcmp(ref_name, \"refs/notes/\") ||\n \t     !strcmp(ref_name, \"HEAD\"))) {\n-\t\tif (safe_create_leading_directories(log_file) < 0)\n+\t\tif (safe_create_leading_directories(*log_file) < 0)\n \t\t\treturn error(\"unable to create directory for %s\",\n-\t\t\t\t     log_file);\n+\t\t\t\t     *log_file);\n \t\toflags |= O_CREAT;\n \t}\n \n-\tlogfd = open(log_file, oflags, 0666);\n+\tlogfd = open(*log_file, oflags, 0666);\n \tif (logfd < 0) {\n \t\tif (!(oflags & O_CREAT) && errno == ENOENT)\n \t\t\treturn 0;\n \n \t\tif ((oflags & O_CREAT) && errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(log_file)) {\n+\t\t\tif (remove_empty_directories(*log_file)) {\n \t\t\t\treturn error(\"There are still logs under '%s'\",\n-\t\t\t\t\t     log_file);\n+\t\t\t\t\t     *log_file);\n \t\t\t}\n-\t\t\tlogfd = open(log_file, oflags, 0666);\n+\t\t\tlogfd = open(*log_file, oflags, 0666);\n \t\t}\n \n \t\tif (logfd < 0)\n \t\t\treturn error(\"Unable to append to %s: %s\",\n-\t\t\t\t     log_file, strerror(errno));\n+\t\t\t\t     *log_file, strerror(errno));\n \t}\n \n-\tadjust_shared_perm(log_file);\n+\tadjust_shared_perm(*log_file);\n+\tclose(logfd);\n+\treturn 0;\n+}\n \n+static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1, const char *msg)\n+{\n+\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n+\tunsigned maxlen, len;\n+\tint msglen;\n+\tchar *log_file;\n+\tchar *logrec;\n+\tconst char *committer;\n+\n+\tif (log_all_ref_updates < 0)\n+\t\tlog_all_ref_updates = !is_bare_repository();\n+\n+\tresult = log_ref_setup(ref_name, &log_file);\n+\tif (result)\n+\t\treturn result;\n+\n+\tlogfd = open(log_file, oflags);\n+\tif (logfd < 0)\n+\t\treturn 0;\n \tmsglen = msg ? strlen(msg) : 0;\n \tcommitter = git_committer_info(0);\n \tmaxlen = strlen(committer) + msglen + 100;\ndiff --git a/refs.h b/refs.h\nindex 4a18b08..594c9d9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -68,6 +68,9 @@ extern void unlock_ref(struct ref_lock *lock);\n /** Writes sha1 into the ref specified by the lock. **/\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n+/** Setup reflog before using. **/\n+int log_ref_setup(const char *ref_name, char **log_file);\n+\n /** Reads log for the value of ref during at_time. **/\n 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);\n \n-- \n1.7.1.231.g0687c.dirty\n"},{"id":"142074","messageId":"1274488119-6989-4-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","subject":"[PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:28:37Z","receivedAt":"2010-05-22T00:28:37Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Added changes to satisfy a corner case: creating reflogs by using -l\nwhen core.logAllRefUpdates is set to false.\n---\n builtin/checkout.c         |   31 ++++++++++++++++--\n t/t2017-checkout-orphan.sh |   78 ++++++++++++++++++++++++++++++++------------\n 2 files changed, 85 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c382521..024c936 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -493,7 +493,24 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *old_desc;\n \tif (opts->new_branch) {\n-\t\tif (!opts->new_orphan_branch)\n+\t\tif (opts->new_orphan_branch) {\n+\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n+\t\t\t\tint temp;\n+\t\t\t\tchar *log_file;\n+\t\t\t\tchar *ref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n+\n+\t\t\t\ttemp = log_all_ref_updates;\n+\t\t\t\tlog_all_ref_updates = 1;\n+\t\t\t\tif (log_ref_setup(ref_name, &log_file)) {\n+\t\t\t\t\tfprintf(stderr, \"Can not do reflog for '%s'\\n\",\n+\t\t\t\t\t    opts->new_orphan_branch);\n+\t\t\t\t\tlog_all_ref_updates = temp;\n+\t\t\t\t\treturn;\n+\t\t\t\t}\n+\t\t\t\tlog_all_ref_updates = temp;\n+\t\t\t}\n+\t\t}\n+\t\telse\n \t\t\tcreate_branch(old->name, opts->new_branch, new->name, 0,\n \t\t\t\t      opts->new_branch_log, opts->track);\n \t\tnew->name = opts->new_branch;\n@@ -517,6 +534,14 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \t\t\t\t\topts->new_branch ? \" a new\" : \"\",\n \t\t\t\t\tnew->name);\n \t\t}\n+\t\tif (old->path && old->name) {\n+\t\t\tchar log_file[PATH_MAX], ref_file[PATH_MAX];\n+\n+\t\t\tgit_snpath(log_file, sizeof(log_file), \"logs/%s\", old->path);\n+\t\t\tgit_snpath(ref_file, sizeof(ref_file), \"%s\", old->path);\n+\t\t\tif (!file_exists(ref_file) && file_exists(log_file))\n+\t\t\t\tremove_path(log_file);\n+\t\t}\n \t} else if (strcmp(new->name, \"HEAD\")) {\n \t\tupdate_ref(msg.buf, \"HEAD\", new->commit->object.sha1, NULL,\n \t\t\t   REF_NODEREF, DIE_ON_ERR);\n@@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.new_orphan_branch) {\n \t\tif (opts.new_branch)\n \t\t\tdie(\"--orphan and -b are mutually exclusive\");\n-\t\tif (opts.track > 0 || opts.new_branch_log)\n-\t\t\tdie(\"--orphan cannot be used with -t or -l\");\n+\t\tif (opts.track > 0)\n+\t\t\tdie(\"--orphan should not be used with -t\");\n \t\topts.new_branch = opts.new_orphan_branch;\n \t}\n \ndiff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh\nindex a8297c6..be88d4b 100755\n--- a/t/t2017-checkout-orphan.sh\n+++ b/t/t2017-checkout-orphan.sh\n@@ -49,6 +49,62 @@ test_expect_success '--orphan must be rejected with -b' '\n \ttest refs/heads/master = \"$(git symbolic-ref HEAD)\"\n '\n \n+test_expect_success '--orphan must be rejected with -t' '\n+\tgit checkout master &&\n+\ttest_must_fail git checkout --orphan new -t master &&\n+\ttest refs/heads/master = \"$(git symbolic-ref HEAD)\"\n+'\n+\n+test_expect_success '--orphan ignores branch.autosetupmerge' '\n+\tgit checkout master &&\n+\tgit config branch.autosetupmerge always &&\n+\tgit checkout --orphan gamma &&\n+\ttest -z \"$(git config branch.gamma.merge)\" &&\n+\ttest refs/heads/gamma = \"$(git symbolic-ref HEAD)\" &&\n+\ttest_must_fail git rev-parse --verify HEAD^\n+'\n+\n+test_expect_success '--orphan makes reflog by default' '\n+\tgit checkout master &&\n+\tgit config --unset core.logAllRefUpdates &&\n+\tgit checkout --orphan delta &&\n+\t! test -f .git/logs/refs/heads/delta &&\n+\ttest_must_fail PAGER= git reflog show delta &&\n+\tgit commit -m Delta &&\n+\ttest -f .git/logs/refs/heads/delta &&\n+\tPAGER= git reflog show delta\n+'\n+\n+test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '\n+\tgit checkout master &&\n+\tgit config core.logAllRefUpdates false &&\n+\tgit checkout --orphan epsilon &&\n+\t! test -f .git/logs/refs/heads/epsilon &&\n+\ttest_must_fail PAGER= git reflog show epsilon &&\n+\tgit commit -m Epsilon &&\n+\t! test -f .git/logs/refs/heads/epsilon &&\n+\ttest_must_fail PAGER= git reflog show epsilon\n+'\n+\n+test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '\n+\tgit checkout master &&\n+\tgit checkout -l --orphan zeta &&\n+\ttest -f .git/logs/refs/heads/zeta &&\n+\ttest_must_fail PAGER= git reflog show zeta &&\n+\tgit commit -m Zeta &&\n+\tPAGER= git reflog show zeta\n+'\n+\n+test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '\n+\tgit checkout master &&\n+\tgit checkout -l --orphan eta &&\n+\ttest -f .git/logs/refs/heads/eta &&\n+\ttest_must_fail PAGER= git reflog show eta &&\n+\tgit checkout master &&\n+\t! test -f .git/logs/refs/heads/eta &&\n+\ttest_must_fail PAGER= git reflog show eta\n+'\n+\n test_expect_success '--orphan is rejected with an existing name' '\n \tgit checkout master &&\n \ttest_must_fail git checkout --orphan master &&\n@@ -60,31 +116,11 @@ test_expect_success '--orphan refuses to switch if a merge is needed' '\n \tgit reset --hard &&\n \techo local >>\"$TEST_FILE\" &&\n \tcat \"$TEST_FILE\" >\"$TEST_FILE.saved\" &&\n-\ttest_must_fail git checkout --orphan gamma master^ &&\n+\ttest_must_fail git checkout --orphan new master^ &&\n \ttest refs/heads/master = \"$(git symbolic-ref HEAD)\" &&\n \ttest_cmp \"$TEST_FILE\" \"$TEST_FILE.saved\" &&\n \tgit diff-index --quiet --cached HEAD &&\n \tgit reset --hard\n '\n \n-test_expect_success '--orphan does not mix well with -t' '\n-\tgit checkout master &&\n-\ttest_must_fail git checkout -t master --orphan gamma &&\n-\ttest refs/heads/master = \"$(git symbolic-ref HEAD)\"\n-'\n-\n-test_expect_success '--orphan ignores branch.autosetupmerge' '\n-\tgit checkout -f master &&\n-\tgit config branch.autosetupmerge always &&\n-\tgit checkout --orphan delta &&\n-\ttest -z \"$(git config branch.delta.merge)\" &&\n-\ttest refs/heads/delta = \"$(git symbolic-ref HEAD)\" &&\n-\ttest_must_fail git rev-parse --verify HEAD^\n-'\n-\n-test_expect_success '--orphan does not mix well with -l' '\n-\tgit checkout -f master &&\n-\ttest_must_fail git checkout -l --orphan gamma\n-'\n-\n test_done\n-- \n1.7.1.231.g0687c.dirty\n"},{"id":"142075","messageId":"1274488119-6989-5-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","subject":"[PATCH 4/5] t3200: test -l with core.logAllRefUpdates options","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:28:38Z","receivedAt":"2010-05-22T00:28:38Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"By default reflogs are always created for new local branches by\n\"checkout -b\".  But by setting core.logAllRefUpdates to false this will\nnot be true anymore.\n\nIn that case you only create the reflogs when you use -l switch with\n\"checkout -b\".\n\nAdded missing tests to check expected behaviors.\n---\n t/t3200-branch.sh |   24 ++++++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e0b7605..9d2c06e 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -224,6 +224,30 @@ test_expect_success \\\n \t test -f .git/logs/refs/heads/g/h/i &&\n \t diff expect .git/logs/refs/heads/g/h/i'\n \n+test_expect_success 'checkout -b makes reflog by default' '\n+\tgit checkout master &&\n+\tgit config --unset core.logAllRefUpdates &&\n+\tgit checkout -b alpha &&\n+\ttest -f .git/logs/refs/heads/alpha &&\n+\tPAGER= git reflog show alpha\n+'\n+\n+test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates = false' '\n+\tgit checkout master &&\n+\tgit config core.logAllRefUpdates false &&\n+\tgit checkout -b beta &&\n+\t! test -f .git/logs/refs/heads/beta &&\n+\ttest_must_fail PAGER= git reflog show beta\n+'\n+\n+test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '\n+\tgit checkout master &&\n+\tgit checkout -lb gamma &&\n+\tgit config --unset core.logAllRefUpdates &&\n+\ttest -f .git/logs/refs/heads/gamma &&\n+\tPAGER= git reflog show gamma\n+'\n+\n test_expect_success 'avoid ambiguous track' '\n \tgit config branch.autosetupmerge true &&\n \tgit config remote.ambi1.url lalala &&\n-- \n1.7.1.231.g0687c.dirty\n"},{"id":"142076","messageId":"1274488119-6989-6-git-send-email-erick.mattos@gmail.com","threadId":"23872","inReplyTo":"1274488119-6989-1-git-send-email-erick.mattos@gmail.com","subject":"[PATCH 5/5] bash completion: add --orphan to 'git checkout'","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-22T00:43:52Z","receivedAt":"2010-05-22T00:43:52Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Updating git-completion.bash with new --orphan option to 'git checkout'.\n---\n contrib/completion/git-completion.bash |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 545bd4b..b70cca1 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -841,7 +841,7 @@ _git_checkout ()\n \t--*)\n \t\t__gitcomp \"\n \t\t\t--quiet --ours --theirs --track --no-track --merge\n-\t\t\t--conflict= --patch\n+\t\t\t--conflict= --orphan --patch\n \t\t\t\"\n \t\t;;\n \t*)\n-- \n1.7.1.231.g0687c.dirty\n"},{"id":"142304","messageId":"7v632bs13c.fsf@alter.siamese.dyndns.org","threadId":"23872","inReplyTo":"1274488119-6989-3-git-send-email-erick.mattos@gmail.com","subject":"Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-26T05:07:03Z","receivedAt":"2010-05-26T05:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erick Mattos <erick.mattos@gmail.com> writes:\n\n> -static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n> -\t\t\t const unsigned char *new_sha1, const char *msg)\n> +int log_ref_setup(const char *ref_name, char **log_file)\n>  {\n> -\tint logfd, written, oflags = O_APPEND | O_WRONLY;\n> -\tunsigned maxlen, len;\n> -\tint msglen;\n> -\tchar log_file[PATH_MAX];\n> -\tchar *logrec;\n> -\tconst char *committer;\n> -\n> -\tif (log_all_ref_updates < 0)\n> -\t\tlog_all_ref_updates = !is_bare_repository();\n> -\n> -\tgit_snpath(log_file, sizeof(log_file), \"logs/%s\", ref_name);\n> +\tint logfd, oflags = O_APPEND | O_WRONLY;\n> +\tchar logfile[PATH_MAX];\n> +\tgit_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n> +\t*log_file = logfile;\n> ...\n\nI have a slight suspicion that it would have made the patch smaller and\neasier to read if you kept the name of the on-stack log_file[] as-is, and\nnamed the retval parameter logfile_p or soemthing.  Also you would need to\nmake this buffer \"static char log_file[]\", no?  Otherwise you would be\nreturning a pointer to a dead buffer to the caller.\n\n> +static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n> +\t\t\t const unsigned char *new_sha1, const char *msg)\n> +{\n> + ...\n> +\tresult = log_ref_setup(ref_name, &log_file);\n> +\tif (result)\n> +\t\treturn result;\n> +\n> +\tlogfd = open(log_file, oflags);\n\nYuck, the caller needs to call \"setup\" which discards the file descriptor\nopened for writing and then open it again itself?\n"},{"id":"142305","messageId":"7vzkznqmir.fsf@alter.siamese.dyndns.org","threadId":"23872","inReplyTo":"1274488119-6989-4-git-send-email-erick.mattos@gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-26T05:07:08Z","receivedAt":"2010-05-26T05:07:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erick Mattos <erick.mattos@gmail.com> writes:\n\n> +\t\t\tgit_snpath(ref_file, sizeof(ref_file), \"%s\", old->path);\n\n???\n\n> @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>  \tif (opts.new_orphan_branch) {\n>  \t\tif (opts.new_branch)\n>  \t\t\tdie(\"--orphan and -b are mutually exclusive\");\n> -\t\tif (opts.track > 0 || opts.new_branch_log)\n> -\t\t\tdie(\"--orphan cannot be used with -t or -l\");\n> +\t\tif (opts.track > 0)\n> +\t\t\tdie(\"--orphan should not be used with -t\");\n\nWhy s/cannot/should not/?  Just being curious.\n\n> +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '\n> +\tgit checkout master &&\n> +\tgit checkout -l --orphan eta &&\n> +\ttest -f .git/logs/refs/heads/eta &&\n> +\ttest_must_fail PAGER= git reflog show eta &&\n> +\tgit checkout master &&\n> +\t! test -f .git/logs/refs/heads/eta &&\n> +\ttest_must_fail PAGER= git reflog show eta\n> +'\n\nI don't quite understand the title of this test, nor am I convinced that\ntesting for .git/logs/refs/heads/eta is necessarily a good thing to do\nhere.  \"eta\" branch is first prepared in an unborn state with the working\ntree and the index prepared to commit what is in 'master', and the first\n\"git reflog\" would fail because there is no eta branch at that point yet.\nMoving to 'master' from that state would still leave \"eta\" branch unborn\nand we will not see \"git reflog\" for that branch (we will fail \"git log\neta\" too for that matter).  Perhaps two \"test -f .git/logs/refs/heads/eta\"\nshouldn't be there?  It feels that it is testing a bit too low level an\nimplementation detail.\n"},{"id":"142373","messageId":"AANLkTimT3sI3yuM8RZai-eWDk8Z5Rmc28RLGOx_i-RXa@mail.gmail.com","threadId":"23872","inReplyTo":"7vzkznqmir.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-26T14:52:03Z","receivedAt":"2010-05-26T14:52:03Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/5/26 Junio C Hamano <gitster@pobox.com>\n>\n> Erick Mattos <erick.mattos@gmail.com> writes:\n> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n> >       if (opts.new_orphan_branch) {\n> >               if (opts.new_branch)\n> >                       die(\"--orphan and -b are mutually exclusive\");\n> > -             if (opts.track > 0 || opts.new_branch_log)\n> > -                     die(\"--orphan cannot be used with -t or -l\");\n> > +             if (opts.track > 0)\n> > +                     die(\"--orphan should not be used with -t\");\n>\n> Why s/cannot/should not/?  Just being curious.\n\nI have typed that text, not changed the original so this is not a fix\nto your text.  Anyway for me \"should not\" is more polite, like \"you\nshould not yell\" meaning you really can not do it.  Or \"you should not\ndisrespect the captain\".\n\nBut that is not a fix.\n\n> > +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '\n> > +     git checkout master &&\n> > +     git checkout -l --orphan eta &&\n> > +     test -f .git/logs/refs/heads/eta &&\n> > +     test_must_fail PAGER= git reflog show eta &&\n> > +     git checkout master &&\n> > +     ! test -f .git/logs/refs/heads/eta &&\n> > +     test_must_fail PAGER= git reflog show eta\n> > +'\n>\n> I don't quite understand the title of this test, nor am I convinced that\n> testing for .git/logs/refs/heads/eta is necessarily a good thing to do\n> here.  \"eta\" branch is first prepared in an unborn state with the working\n> tree and the index prepared to commit what is in 'master', and the first\n> \"git reflog\" would fail because there is no eta branch at that point yet.\n> Moving to 'master' from that state would still leave \"eta\" branch unborn\n> and we will not see \"git reflog\" for that branch (we will fail \"git log\n> eta\" too for that matter).  Perhaps two \"test -f .git/logs/refs/heads/eta\"\n> shouldn't be there?  It feels that it is testing a bit too low level an\n> implementation detail.\n\nSo I need to explain the solution:\n\nWhen config core.logAllRefUpdates is set to false what really happens\nis that the reflog is not created and any reflog change is saved only\nwhen you have an existent reflog.\n\nWhat I did was to make a \"touch reflog\".  Creating it, when the new\nbranch get eventually saved then the reflog would be written normally.\n But in case somebody give up this new branch before the first save,\nmoving back to a regular branch would leave a ghost reflog.\n\nI have coded the cleaning commands for that and the test is just a\ncheck of this behavior.\n\nThe first \"test -f .git/logs/refs/heads/eta\" tests if reflog was\ncreated and the second if it was deleted.  No big deal.\n\nRegards\n"},{"id":"142374","messageId":"AANLkTikKAkwHYj6OvfEJM1YE8w2TZL2oeMBrj28V3CwX@mail.gmail.com","threadId":"23872","inReplyTo":"AANLkTimT3sI3yuM8RZai-eWDk8Z5Rmc28RLGOx_i-RXa@mail.gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-05-26T15:13:02Z","receivedAt":"2010-05-26T15:13:02Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, May 26, 2010 at 4:52 PM, Erick Mattos <erick.mattos@gmail.com> wrote:\n> Hi,\n>\n> 2010/5/26 Junio C Hamano <gitster@pobox.com>\n>>\n>> Erick Mattos <erick.mattos@gmail.com> writes:\n>> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>> >       if (opts.new_orphan_branch) {\n>> >               if (opts.new_branch)\n>> >                       die(\"--orphan and -b are mutually exclusive\");\n>> > -             if (opts.track > 0 || opts.new_branch_log)\n>> > -                     die(\"--orphan cannot be used with -t or -l\");\n>> > +             if (opts.track > 0)\n>> > +                     die(\"--orphan should not be used with -t\");\n>>\n>> Why s/cannot/should not/?  Just being curious.\n>\n> I have typed that text, not changed the original so this is not a fix\n> to your text.  Anyway for me \"should not\" is more polite, like \"you\n> should not yell\" meaning you really can not do it.  Or \"you should not\n> disrespect the captain\".\n\nI don't think it makes sense to try and be polite when we're actually\nrefusing... \"should not\" implies that it possible but not recommended.\nAnd in this case it's impossible, because we die()...\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"142375","messageId":"4BFD3ED3.3000709@drmicha.warpmail.net","threadId":"23872","inReplyTo":"AANLkTimT3sI3yuM8RZai-eWDk8Z5Rmc28RLGOx_i-RXa@mail.gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-05-26T15:31:31Z","receivedAt":"2010-05-26T15:31:31Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Erick Mattos venit, vidit, dixit 26.05.2010 16:52:\n> Hi,\n> \n> 2010/5/26 Junio C Hamano <gitster@pobox.com>\n>>\n>> Erick Mattos <erick.mattos@gmail.com> writes:\n>>> @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>>>       if (opts.new_orphan_branch) {\n>>>               if (opts.new_branch)\n>>>                       die(\"--orphan and -b are mutually exclusive\");\n>>> -             if (opts.track > 0 || opts.new_branch_log)\n>>> -                     die(\"--orphan cannot be used with -t or -l\");\n>>> +             if (opts.track > 0)\n>>> +                     die(\"--orphan should not be used with -t\");\n>>\n>> Why s/cannot/should not/?  Just being curious.\n> \n> I have typed that text, not changed the original so this is not a fix\n> to your text.  Anyway for me \"should not\" is more polite, like \"you\n> should not yell\" meaning you really can not do it.  Or \"you should not\n> disrespect the captain\".\n\n\"should not\" means you can but you should not. \"die\" certainly means you\ncannot. This is not a matter of politeness but of correctness.\n\n> \n> But that is not a fix.\n\nThere's a \"-\" line with \"cannot\" and a \"+\" line with \"should not\". So\nyou certainly changed what was there before.\n\n> \n>>> +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '\n\nReally long line here ;)\n\n>>> +     git checkout master &&\n>>> +     git checkout -l --orphan eta &&\n>>> +     test -f .git/logs/refs/heads/eta &&\n>>> +     test_must_fail PAGER= git reflog show eta &&\n>>> +     git checkout master &&\n>>> +     ! test -f .git/logs/refs/heads/eta &&\n>>> +     test_must_fail PAGER= git reflog show eta\n>>> +'\n>>\n>> I don't quite understand the title of this test, nor am I convinced that\n>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do\n>> here.  \"eta\" branch is first prepared in an unborn state with the working\n>> tree and the index prepared to commit what is in 'master', and the first\n>> \"git reflog\" would fail because there is no eta branch at that point yet.\n>> Moving to 'master' from that state would still leave \"eta\" branch unborn\n>> and we will not see \"git reflog\" for that branch (we will fail \"git log\n>> eta\" too for that matter).  Perhaps two \"test -f .git/logs/refs/heads/eta\"\n>> shouldn't be there?  It feels that it is testing a bit too low level an\n>> implementation detail.\n> \n> So I need to explain the solution:\n> \n> When config core.logAllRefUpdates is set to false what really happens\n> is that the reflog is not created and any reflog change is saved only\n> when you have an existent reflog.\n> \n> What I did was to make a \"touch reflog\".  Creating it, when the new\n\nYou mean checkout -l --orphan does that touch? There is none in the\ntest. Does ordinary checkout with -l does that, too?\n\n> branch get eventually saved then the reflog would be written normally.\n>  But in case somebody give up this new branch before the first save,\n> moving back to a regular branch would leave a ghost reflog.\n\nThe touched entry (is left), not a reflog, I assume, otherwise the\nreflog command should not fail.\n\n> \n> I have coded the cleaning commands for that and the test is just a\n> check of this behavior.\n\nWhich command does the cleaning? \"reflog show\" or \"checkout master\"?\n\n> \n> The first \"test -f .git/logs/refs/heads/eta\" tests if reflog was\n> created and the second if it was deleted.  No big deal.\n> \n> Regards\n\nI haven't followed this series due to earlier worries about --orphan but\nI'm wondering about this cleaning up behind the back. Maybe it's just a\nmatter of explanations, though.\n\nMichael\n"},{"id":"142378","messageId":"AANLkTincuWkXqFybDwq2Mh9MI1eC2JjjGwLy17iEP9u5@mail.gmail.com","threadId":"23872","inReplyTo":"AANLkTikKAkwHYj6OvfEJM1YE8w2TZL2oeMBrj28V3CwX@mail.gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-26T16:01:50Z","receivedAt":"2010-05-26T16:01:50Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/5/26 Erik Faye-Lund <kusmabite@googlemail.com>:\n> I don't think it makes sense to try and be polite when we're actually\n> refusing... \"should not\" implies that it possible but not recommended.\n> And in this case it's impossible, because we die()...\n\nRight then!  As I told it was not a fix.  So let's 's/should not/cannot/' then.\n\nRegards\n"},{"id":"142384","messageId":"AANLkTiksYeRzqNTdOMxb3oliuVna6kAxbHM8nxx6gNCO@mail.gmail.com","threadId":"23872","inReplyTo":"4BFD3ED3.3000709@drmicha.warpmail.net","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-26T18:04:33Z","receivedAt":"2010-05-26T18:04:33Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/5/26 Michael J Gruber <git@drmicha.warpmail.net>:\n>> But that is not a fix.\n>\n> There's a \"-\" line with \"cannot\" and a \"+\" line with \"should not\". So\n> you certainly changed what was there before.\n\nEverybody know what a minus or a plus sign means in a diff. ;-)\n\nWhat I have meant was that I had typed the whole line myself after\nsome previous removal while I was making the changes during\n\"deletion/moving lines\" actions.  No big deal, just a mistake.\n\nThe real message change here is from blocking -t an -l to blocking\nonly -t.  As I had told I have not realized the 'should not/cannot'\nissue.\n\n>>>> +     git checkout master &&\n>>>> +     git checkout -l --orphan eta &&\n>>>> +     test -f .git/logs/refs/heads/eta &&\n>>>> +     test_must_fail PAGER= git reflog show eta &&\n>>>> +     git checkout master &&\n>>>> +     ! test -f .git/logs/refs/heads/eta &&\n>>>> +     test_must_fail PAGER= git reflog show eta\n>>>> +'\n>>>\n>>> I don't quite understand the title of this test, nor am I convinced that\n>>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do\n>>> here.  \"eta\" branch is first prepared in an unborn state with the working\n>>> tree and the index prepared to commit what is in 'master', and the first\n>>> \"git reflog\" would fail because there is no eta branch at that point yet.\n>>> Moving to 'master' from that state would still leave \"eta\" branch unborn\n>>> and we will not see \"git reflog\" for that branch (we will fail \"git log\n>>> eta\" too for that matter).  Perhaps two \"test -f .git/logs/refs/heads/eta\"\n>>> shouldn't be there?  It feels that it is testing a bit too low level an\n>>> implementation detail.\n>>\n>> So I need to explain the solution:\n>>\n>> When config core.logAllRefUpdates is set to false what really happens\n>> is that the reflog is not created and any reflog change is saved only\n>> when you have an existent reflog.\n>>\n>> What I did was to make a \"touch reflog\".  Creating it, when the new\n>\n> You mean checkout -l --orphan does that touch? There is none in the\n> test. Does ordinary checkout with -l does that, too?\n\nThis is not done by a test.  It is part of the whole implementation.\nIt is done only when needed: on that special corner case.\n\nPlease read the patches mainly the 2/5 and 3/5.\n\n>> branch get eventually saved then the reflog would be written normally.\n>>  But in case somebody give up this new branch before the first save,\n>> moving back to a regular branch would leave a ghost reflog.\n>\n> The touched entry (is left), not a reflog, I assume, otherwise the\n> reflog command should not fail.\n>\n>>\n>> I have coded the cleaning commands for that and the test is just a\n>> check of this behavior.\n>\n> Which command does the cleaning? \"reflog show\" or \"checkout master\"?\n>\n>>\n>> The first \"test -f .git/logs/refs/heads/eta\" tests if reflog was\n>> created and the second if it was deleted.  No big deal.\n>>\n>> Regards\n>\n> I haven't followed this series due to earlier worries about --orphan but\n> I'm wondering about this cleaning up behind the back. Maybe it's just a\n> matter of explanations, though.\n>\n> Michael\n>\n\nYour questions are too unaware of the code.  ;-)  As I don't think you\nare asking me to explain each single line then I imagine you have not\nread the patches, just the chat.  Please read the patch series.  I\nwill be very glad to answer any further questions then.\n\nBest regards\n"},{"id":"142385","messageId":"AANLkTikPypcmGB6NuTl-SQZR3lnIvdmVG5E8wjVAlIej@mail.gmail.com","threadId":"23872","inReplyTo":"7v632bs13c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-05-26T18:11:05Z","receivedAt":"2010-05-26T18:11:05Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi there,\n\n2010/5/26 Junio C Hamano <gitster@pobox.com>:\n> I have a slight suspicion that it would have made the patch smaller and\n> easier to read if you kept the name of the on-stack log_file[] as-is, and\n> named the retval parameter logfile_p or soemthing.\n\nThe size of the patch is indeed by the split/insertion which\ncomplicates the diff's life.\nIf you compare both blobs you see it is not a hard change.  But we can\nnot hope for computer's intelligence during this lifetime.  ;-D\n\n>  Also you would need to\n> make this buffer \"static char log_file[]\", no?  Otherwise you would be\n> returning a pointer to a dead buffer to the caller.\n\nNot really.  git_snpath() is taking care of setting up the buffer\ndynamically in the heap.  The calling function presents its buffer by\nreference thus only the pointer's address which its content is later\nchanged to point to the dynamic one.\n\n>> +static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n>> +                      const unsigned char *new_sha1, const char *msg)\n>> +{\n>> + ...\n>> +     result = log_ref_setup(ref_name, &log_file);\n>> +     if (result)\n>> +             return result;\n>> +\n>> +     logfd = open(log_file, oflags);\n>\n> Yuck, the caller needs to call \"setup\" which discards the file descriptor\n> opened for writing and then open it again itself?\n\nThe separation of logic of setup from writing of the reflog and the\nconsequently created log_ref_setup was meant to just prepare the\nreflog file.  This way it can be used consistently on different\nfunctions.\n\nAt the moment It is being used on log_ref_write() and in\nupdate_refs_for_switch().  In the first case it is interesting that\nthe reflog keeps opened to be used.  On the later case it is not.  So,\none of the calling functions would have to do something.\n\nWe have two approaches to that:\n1. keeping the reflog opened and making sure the calling function close it.\n2. closing it and making the calling function open it or not as needed.\n\nI have chosen 2 because of:\n* I think it is safer to have any function closing open files,\ncleaning variables or\n  resources used by it whenever possible.\n* It is more elegant that the function does what it is meant to do, in this case\n  setting up the file only.\n* It possibly keeps the code cleaner because only one 'close' for this function\n  needs to be done and in the same place it happened the correspondent 'open'.\n* No approach was going to cost any resources more.\n\nNow just a question, Junio:\n\nI forgot to sign-off those patches, should I have to send them again?\n\nRegards\n"},{"id":"142401","messageId":"4BFE2461.5080501@drmicha.warpmail.net","threadId":"23872","inReplyTo":"AANLkTiksYeRzqNTdOMxb3oliuVna6kAxbHM8nxx6gNCO@mail.gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-05-27T07:50:57Z","receivedAt":"2010-05-27T07:50:57Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Erick Mattos venit, vidit, dixit 26.05.2010 20:04:\n> Hi,\n> \n> 2010/5/26 Michael J Gruber <git@drmicha.warpmail.net>:\n>>> But that is not a fix.\n>>\n>> There's a \"-\" line with \"cannot\" and a \"+\" line with \"should not\". So\n>> you certainly changed what was there before.\n> \n> Everybody know what a minus or a plus sign means in a diff. ;-)\n> \n> What I have meant was that I had typed the whole line myself after\n> some previous removal while I was making the changes during\n> \"deletion/moving lines\" actions.  No big deal, just a mistake.\n> \n> The real message change here is from blocking -t an -l to blocking\n> only -t.  As I had told I have not realized the 'should not/cannot'\n> issue.\n> \n>>>>> +     git checkout master &&\n>>>>> +     git checkout -l --orphan eta &&\n>>>>> +     test -f .git/logs/refs/heads/eta &&\n>>>>> +     test_must_fail PAGER= git reflog show eta &&\n>>>>> +     git checkout master &&\n>>>>> +     ! test -f .git/logs/refs/heads/eta &&\n>>>>> +     test_must_fail PAGER= git reflog show eta\n>>>>> +'\n>>>>\n>>>> I don't quite understand the title of this test, nor am I convinced that\n>>>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do\n>>>> here.  \"eta\" branch is first prepared in an unborn state with the working\n>>>> tree and the index prepared to commit what is in 'master', and the first\n>>>> \"git reflog\" would fail because there is no eta branch at that point yet.\n>>>> Moving to 'master' from that state would still leave \"eta\" branch unborn\n>>>> and we will not see \"git reflog\" for that branch (we will fail \"git log\n>>>> eta\" too for that matter).  Perhaps two \"test -f .git/logs/refs/heads/eta\"\n>>>> shouldn't be there?  It feels that it is testing a bit too low level an\n>>>> implementation detail.\n>>>\n>>> So I need to explain the solution:\n>>>\n>>> When config core.logAllRefUpdates is set to false what really happens\n>>> is that the reflog is not created and any reflog change is saved only\n>>> when you have an existent reflog.\n>>>\n>>> What I did was to make a \"touch reflog\".  Creating it, when the new\n>>\n>> You mean checkout -l --orphan does that touch? There is none in the\n>> test. Does ordinary checkout with -l does that, too?\n> \n> This is not done by a test.  It is part of the whole implementation.\n> It is done only when needed: on that special corner case.\n> \n> Please read the patches mainly the 2/5 and 3/5.\n> \n>>> branch get eventually saved then the reflog would be written normally.\n>>>  But in case somebody give up this new branch before the first save,\n>>> moving back to a regular branch would leave a ghost reflog.\n>>\n>> The touched entry (is left), not a reflog, I assume, otherwise the\n>> reflog command should not fail.\n>>\n>>>\n>>> I have coded the cleaning commands for that and the test is just a\n>>> check of this behavior.\n>>\n>> Which command does the cleaning? \"reflog show\" or \"checkout master\"?\n>>\n>>>\n>>> The first \"test -f .git/logs/refs/heads/eta\" tests if reflog was\n>>> created and the second if it was deleted.  No big deal.\n>>>\n>>> Regards\n>>\n>> I haven't followed this series due to earlier worries about --orphan but\n>> I'm wondering about this cleaning up behind the back. Maybe it's just a\n>> matter of explanations, though.\n>>\n>> Michael\n>>\n> \n> Your questions are too unaware of the code.  ;-)  As I don't think you\n> are asking me to explain each single line then I imagine you have not\n> read the patches, just the chat.  Please read the patch series.  I\n> will be very glad to answer any further questions then.\n\nI'm not asking you to explain your code but your intentions: What is it\nsupposed to do? If I have to read the code to figure that out then your\ncommit messages and on-list explanations (or my understanding thereof)\nare suboptimal. You're cleaning up some files in logs/refs and I'm wondering\n\n- which command does that automatically (after your series) and\n- in which circumstances (only --orphan or more) superfluous files were\nleft there before.\n\nIf you're not willing to answer that I'm simply not reading the code.\nOne reviewer less.\n\nMichael\n"},{"id":"142834","messageId":"7vzkzdcneh.fsf@alter.siamese.dyndns.org","threadId":"23872","inReplyTo":"AANLkTikPypcmGB6NuTl-SQZR3lnIvdmVG5E8wjVAlIej@mail.gmail.com","subject":"Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-02T18:14:30Z","receivedAt":"2010-06-02T18:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erick Mattos <erick.mattos@gmail.com> writes:\n\n> Now just a question, Junio:\n>\n> I forgot to sign-off those patches, should I have to send them again?\n\nI would have appreciated a whole re-send while I was too distracted by\nnon-git stuff during the past few weeks, but now I am more or less settled\nin, it's Ok to just send a separate \"Signed-off-by:\" like this one:\n\n    http://article.gmane.org/gmane.comp.version-control.git/148230/raw\n"},{"id":"142842","messageId":"AANLkTikcQny9Es_cMJTg94qTJZD2s7T37h_Hur9Lt-Lv@mail.gmail.com","threadId":"23872","inReplyTo":"7vzkzdcneh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-06-02T23:16:30Z","receivedAt":"2010-06-02T23:16:30Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/6/2 Junio C Hamano <gitster@pobox.com>:\n> Erick Mattos <erick.mattos@gmail.com> writes:\n>\n>> Now just a question, Junio:\n>>\n>> I forgot to sign-off those patches, should I have to send them again?\n>\n> I would have appreciated a whole re-send while I was too distracted by\n> non-git stuff during the past few weeks, but now I am more or less settled\n> in, it's Ok to just send a separate \"Signed-off-by:\" like this one:\n>\n>    http://article.gmane.org/gmane.comp.version-control.git/148230/raw\n>\n\nAll right, following then:\n\n2010/5/21 Erick Mattos <erick.mattos@gmail.com>:\n> These series of patches are improvements to 'git checkout --orphan'.\n>\n> The main reason for them is a corner case which is not being solved by\n> actual implementation.  As it is a quite improbable situation and as it was\n> necessary to do more extensive changes to support it then its development\n> was held to be presented in a new developing cycle.\n>\n> When someone set core.logAllRefUpdates to false reflogs are not created\n> automatically.  This behavior is superseeded by -l option.  Actually this is\n> not allowed with --orphan by current implementation.  Those new patches are\n> made to fix that.\n>\n> There are also two other patches for configuring completion in bash and to\n> enhance documentation.\n>\n> To be completely honest I don't see a point of not having the reflogs\n> created and deleted automatically so I see no reason for -l and\n> core.logAllRefUpdates at all.  But I do not like to do anything partially\n> thus these new patches.  If someone could show me a case please do it.  ;-)\n>\n> [PATCH 1/5] Documentation: alter checkout --orphan description\n>\n> This one improves documentation text by late corrections from previous\n> threads.\n>\n> [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup\n>\n> Prepare the field by separating the logic to set up the reflog from the\n> reflog writing action.\n>\n> [PATCH 3/5] checkout --orphan: respect -l option always\n>\n> This is the actual actor.\n>\n> [PATCH 4/5] t3200: test -l with core.logAllRefUpdates options\n>\n> Adjusting scripts to test everything extensively.\n>\n> [PATCH 5/5] bash completion: add --orphan to 'git checkout'\n>\n> Just do that git change.\n\nSigned-off-by: Erick Mattos <erick.mattos@gmail.com>\n"},{"id":"142909","messageId":"AANLkTikUpyH6nmTfB4XbjQqJHHJOZiea4hn61tIH2ulR@mail.gmail.com","threadId":"23872","inReplyTo":"AANLkTikKAkwHYj6OvfEJM1YE8w2TZL2oeMBrj28V3CwX@mail.gmail.com","subject":"Re: [PATCH 3/5] checkout --orphan: respect -l option always","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-06-03T16:28:48Z","receivedAt":"2010-06-03T16:28:48Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi Junio,\n\nJust a small fix...\n\n2010/5/26 Erik Faye-Lund <kusmabite@googlemail.com>:\n> On Wed, May 26, 2010 at 4:52 PM, Erick Mattos <erick.mattos@gmail.com> wrote:\n>> Hi,\n>>\n>> 2010/5/26 Junio C Hamano <gitster@pobox.com>\n>>>\n>>> Erick Mattos <erick.mattos@gmail.com> writes:\n>>> > @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>>> >       if (opts.new_orphan_branch) {\n>>> >               if (opts.new_branch)\n>>> >                       die(\"--orphan and -b are mutually exclusive\");\n>>> > -             if (opts.track > 0 || opts.new_branch_log)\n>>> > -                     die(\"--orphan cannot be used with -t or -l\");\n>>> > +             if (opts.track > 0)\n>>> > +                     die(\"--orphan should not be used with -t\");\n>>>\n>>> Why s/cannot/should not/?  Just being curious.\n>>\n>> I have typed that text, not changed the original so this is not a fix\n>> to your text.  Anyway for me \"should not\" is more polite, like \"you\n>> should not yell\" meaning you really can not do it.  Or \"you should not\n>> disrespect the captain\".\n>\n> I don't think it makes sense to try and be polite when we're actually\n> refusing... \"should not\" implies that it possible but not recommended.\n> And in this case it's impossible, because we die()...\n\nIf you agree, please do that 's/should not/cannot/' on pu.\n\nRegards\n"}]}