{"thread":{"id":"42536","subject":"[PATCH/RFC 0/4] i-t-a entries again...","startedAt":"2016-06-06T11:16:39Z","lastAt":"2016-09-27T12:28:24Z","messageCount":15,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","stefan.naewe@atlas-elektronik.com","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"288418","messageId":"20160606111643.7122-1-pclouds@gmail.com","threadId":"42536","inReplyTo":null,"subject":"[PATCH/RFC 0/4] i-t-a entries again...","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-06T11:16:39Z","receivedAt":"2016-06-06T11:16:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The first two patches bring back a patch does fixes \"git status\" when\ni-t-a entries are present (i.e. \"git diff\" should show new files while\n\"git diff --cached\" should show no changes).\n\nThe third commit fixes \"git commit\" creating empty commits when i-t-a\nentries are the only changes compared to HEAD.\n\nThe last one brings back the old behavior before 3f6d56d (commit:\nignore intent-to-add entries instead of refusing - 2012-02-07). I\nstill believe that current behavior has its uses. So instead of\nreverting the behavior unconditonally, the user can choose to switch\nback to the old behavior via config key.\n\nThe last two do not have have proper tests added because I just\ncreated them in the last hours. This is mainly to bring back the old\ndiscussion to see what should be the way forward.\n\nNguyễn Thái Ngọc Duy (4):\n  diff.h: extend \"flags\" field to 64 bits because we're out of bits\n  Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"\n  commit: don't count i-t-a entries when checking if the new commit is empty\n  commit: reinstate commit behavior before 3f6d56d via a config option\n\n Documentation/diff-options.txt |  6 ++++++\n builtin/commit.c               | 14 ++++++++++----\n cache-tree.c                   | 14 +++++++++++---\n diff-lib.c                     | 18 ++++++++++++++++--\n diff.c                         |  4 +++-\n diff.h                         |  7 ++++---\n t/t2203-add-intent.sh          | 20 ++++++++++++++++++--\n wt-status.c                    |  5 +++++\n 8 files changed, 73 insertions(+), 15 deletions(-)\n\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288419","messageId":"20160606111643.7122-2-pclouds@gmail.com","threadId":"42536","inReplyTo":"20160606111643.7122-1-pclouds@gmail.com","subject":"[PATCH 1/4] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-06T11:16:40Z","receivedAt":"2016-06-06T11:16:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current flags field is 32-bits, all used except one bit and we need one\nmore bit is needed for to toggle i-t-a behavior. The 9th bit could be\nreused for this, but we could just extend it to 64 bits now to give room\nfor more future flags.\n\ngcc -Wconversion is used to catch assignments that truncate bits. No new\nwarning was introduced (in fact one in index_differs_from() was\neliminated),\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/commit.c | 2 +-\n diff-lib.c       | 4 ++--\n diff.c           | 2 +-\n diff.h           | 6 +++---\n 4 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 443ff91..fcfaa2b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -906,7 +906,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t * submodules which were manually staged, which would\n \t\t\t * be really confusing.\n \t\t\t */\n-\t\t\tint diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n+\t\t\tuint64_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \t\t\tif (ignore_submodule_arg &&\n \t\t\t    !strcmp(ignore_submodule_arg, \"all\"))\n \t\t\t\tdiff_flags |= DIFF_OPT_IGNORE_SUBMODULES;\ndiff --git a/diff-lib.c b/diff-lib.c\nindex bc49c70..27887d0 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n {\n \tint changed = ce_match_stat(ce, st, ce_option);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n-\t\tunsigned orig_flags = diffopt->flags;\n+\t\tuint64_t orig_flags = diffopt->flags;\n \t\tif (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))\n \t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n \t\tif (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))\n@@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n \treturn 0;\n }\n \n-int index_differs_from(const char *def, int diff_flags)\n+int index_differs_from(const char *def, uint64_t diff_flags)\n {\n \tstruct rev_info rev;\n \tstruct setup_revision_opt opt;\ndiff --git a/diff.c b/diff.c\nindex d3734d3..f70425f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4936,7 +4936,7 @@ int diff_can_quit_early(struct diff_options *opt)\n static int is_submodule_ignored(const char *path, struct diff_options *options)\n {\n \tint ignored = 0;\n-\tunsigned orig_flags = options->flags;\n+\tuint64_t orig_flags = options->flags;\n \tif (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))\n \t\tset_diffopt_flags_from_submodule_config(options, path);\n \tif (DIFF_OPT_TST(options, IGNORE_SUBMODULES))\ndiff --git a/diff.h b/diff.h\nindex 125447b..b497078 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -115,8 +115,8 @@ struct diff_options {\n \tconst char *pickaxe;\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n-\tunsigned flags;\n-\tunsigned touched_flags;\n+\tuint64_t flags;\n+\tuint64_t touched_flags;\n \n \t/* diff-filter bits */\n \tunsigned int filter;\n@@ -348,7 +348,7 @@ extern int diff_result_code(struct diff_options *, int);\n \n extern void diff_no_index(struct rev_info *, int, const char **);\n \n-extern int index_differs_from(const char *def, int diff_flags);\n+extern int index_differs_from(const char *def, uint64_t diff_flags);\n \n /*\n  * Fill the contents of the filespec \"df\", respecting any textconv defined by\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288421","messageId":"20160606111643.7122-3-pclouds@gmail.com","threadId":"42536","inReplyTo":"20160606111643.7122-1-pclouds@gmail.com","subject":"[PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-06T11:16:41Z","receivedAt":"2016-06-06T11:16:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The original commit d95d728aba06a34394d15466045cbdabdada58a2 was\nreverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we\nwere (and still are) not ready for a new world order. A lot more\ninvestigation must be done to see what is impacted. See the 78cc1a5 for\ndetails.\n\nThis patch takes a smaller and safer step. The new behavior is\ncontrolled by SHIFT_INTENT_TO_ADD flag. We can gradually move more diff\nusers to the new behavior after we are sure it's safe to do so. This\nflag is exposed to outside as \"--shift-ita\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/diff-options.txt |  6 ++++++\n diff-lib.c                     | 14 ++++++++++++++\n diff.c                         |  2 ++\n diff.h                         |  1 +\n t/t2203-add-intent.sh          | 20 ++++++++++++++++++--\n wt-status.c                    |  5 +++++\n 6 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3cb3015..904438b 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -559,5 +559,11 @@ endif::git-format-patch[]\n --no-prefix::\n \tDo not show any source or destination prefix.\n \n+--shift-ita::\n+\tBy default entries added by \"git add -N\" appear as an existing\n+\tempty file in \"git diff\" and a new file in \"git diff --cached\".\n+\tThis option makes the entry appear as a new file in \"git diff\"\n+\tand non-existent in \"git diff --cached\".\n+\n For more detailed explanation on these common options, see also\n linkgit:gitdiffcore[7].\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 27887d0..1248970 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -212,6 +212,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t       ce->sha1, !is_null_sha1(ce->sha1),\n \t\t\t\t\t       ce->name, 0);\n \t\t\t\tcontinue;\n+\t\t\t} else if (DIFF_OPT_TST(&revs->diffopt, SHIFT_INTENT_TO_ADD) &&\n+\t\t\t\t   ce_intent_to_add(ce)) {\n+\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n+\t\t\t\t\t       EMPTY_BLOB_SHA1_BIN, 0,\n+\t\t\t\t\t       ce->name, 0);\n+\t\t\t\tcontinue;\n \t\t\t}\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n@@ -376,6 +382,14 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \tstruct rev_info *revs = o->unpack_data;\n \tint match_missing, cached;\n \n+\t/* i-t-a entries do not actually exist in the index */\n+\tif (DIFF_OPT_TST(&revs->diffopt, SHIFT_INTENT_TO_ADD) &&\n+\t    idx && ce_intent_to_add(idx)) {\n+\t\tidx = NULL;\n+\t\tif (!tree)\n+\t\t\treturn;\t/* nothing to diff.. */\n+\t}\n+\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\ndiff --git a/diff.c b/diff.c\nindex f70425f..b624e64 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3910,6 +3910,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\n \t\treturn parse_ws_error_highlight(options, arg);\n+\telse if (!strcmp(arg, \"--shift-ita\"))\n+\t\tDIFF_OPT_SET(options, SHIFT_INTENT_TO_ADD);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/diff.h b/diff.h\nindex b497078..9e42556 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_FUNCCONTEXT         (1 << 29)\n #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)\n #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)\n+#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)\n \n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 2a4a749..4ae98f4 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -5,10 +5,24 @@ test_description='Intent to add'\n . ./test-lib.sh\n \n test_expect_success 'intent to add' '\n+\ttest_commit 1 &&\n+\tgit rm 1.t &&\n+\techo hello >1.t &&\n \techo hello >file &&\n \techo hello >elif &&\n \tgit add -N file &&\n-\tgit add elif\n+\tgit add elif &&\n+\tgit add -N 1.t\n+'\n+\n+test_expect_success 'git status' '\n+\tgit status --porcelain | grep -v actual >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tDA 1.t\n+\tA  elif\n+\t A file\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'check result of \"add -N\"' '\n@@ -43,7 +57,9 @@ test_expect_success 'i-t-a entry is simply ignored' '\n \tgit add -N nitfol &&\n \tgit commit -m second &&\n \ttest $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&\n-\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 1\n+\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&\n+\ttest $(git diff --name-only --shift-ita HEAD -- nitfol | wc -l) = 0 &&\n+\ttest $(git diff --name-only --shift-ita -- nitfol | wc -l) = 1\n '\n \n test_expect_success 'can commit with an unrelated i-t-a entry in index' '\ndiff --git a/wt-status.c b/wt-status.c\nindex 4f27bd6..cb89005 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -497,6 +497,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, NULL);\n \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \tDIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);\n+\tDIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);\n \tif (!s->show_untracked_files)\n \t\tDIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);\n \tif (s->ignore_submodule_arg) {\n@@ -520,6 +521,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, &opt);\n \n \tDIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);\n+\tDIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);\n \tif (s->ignore_submodule_arg) {\n \t\thandle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);\n \t} else {\n@@ -555,6 +557,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \n \t\tif (!ce_path_match(ce, &s->pathspec, NULL))\n \t\t\tcontinue;\n+\t\tif (ce_intent_to_add(ce))\n+\t\t\tcontinue;\n \t\tit = string_list_insert(&s->change, ce->name);\n \t\td = it->util;\n \t\tif (!d) {\n@@ -853,6 +857,7 @@ static void wt_status_print_verbose(struct wt_status *s)\n \n \tinit_revisions(&rev, NULL);\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n+\tDIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);\n \n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288420","messageId":"20160606111643.7122-4-pclouds@gmail.com","threadId":"42536","inReplyTo":"20160606111643.7122-1-pclouds@gmail.com","subject":"[PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-06T11:16:42Z","receivedAt":"2016-06-06T11:16:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"i-t-a entries are excluded from tree building. Relying solely on active_nr\n(or diff without --shift-ita) may lead to empty commits sometimes, when\ni-t-a entries are the only ones \"changed\" in the index.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/commit.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fcfaa2b..e98ca8a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tif (amend)\n \t\t\tparent = \"HEAD^1\";\n \n-\t\tif (get_sha1(parent, sha1))\n-\t\t\tcommitable = !!active_nr;\n-\t\telse {\n+\t\tif (get_sha1(parent, sha1)) {\n+\t\t\tint i, ita_nr = 0;\n+\n+\t\t\tfor (i = 0; i < active_nr; i++)\n+\t\t\t\tif (ce_intent_to_add(active_cache[i]))\n+\t\t\t\t\tita_nr++;\n+\t\t\tcommitable = active_nr - ita_nr == 0;\n+\t\t} else {\n \t\t\t/*\n \t\t\t * Unless the user did explicitly request a submodule\n \t\t\t * ignore mode by passing a command line option we do\n@@ -910,6 +915,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tif (ignore_submodule_arg &&\n \t\t\t    !strcmp(ignore_submodule_arg, \"all\"))\n \t\t\t\tdiff_flags |= DIFF_OPT_IGNORE_SUBMODULES;\n+\t\t\tdiff_flags |= DIFF_OPT_SHIFT_INTENT_TO_ADD;\n \t\t\tcommitable = index_differs_from(parent, diff_flags);\n \t\t}\n \t}\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288422","messageId":"20160606111643.7122-5-pclouds@gmail.com","threadId":"42536","inReplyTo":"20160606111643.7122-1-pclouds@gmail.com","subject":"[PATCH/RFC 4/4] commit: reinstate commit behavior before 3f6d56d via a config option","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-06T11:16:43Z","receivedAt":"2016-06-06T11:16:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There are two groups of users with regards to \"git add -N\". One uses\ni-t-a as a reminder that certain untracked files must be added before\nthe next commit. Erroring when i-t-a entries are still present\n(i.e. real content not added) out gives the user a chance to add real\ncontent.\n\nThe other group that sees i-t-a entries as a way to make Git aware of\nsome untracked paths so that \"git diff\" and \"git status\" show them\ncorrectly. If they decide that some content should be staged for the\nnext commit, they \"git add\" again for real. If content in these paths is\nnever added, the paths are simply ignored.\n\n3f6d56d (commit: ignore intent-to-add entries instead of refusing -\n2012-02-07) changes the behavior from the former to the latter. This\npatch brings bach the former behavior if core.errorCommitIta is true.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n cache-tree.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex ddf0cc9..b87bd3c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -155,20 +155,28 @@ static int verify_cache(struct cache_entry **cache,\n {\n \tint i, funny;\n \tint silent = flags & WRITE_TREE_SILENT;\n+\tint core_error_commit_ita = 0;\n+\n+\tgit_config_get_bool(\"core.errorcommitita\", &core_error_commit_ita);\n \n \t/* Verify that the tree is merged */\n \tfunny = 0;\n \tfor (i = 0; i < entries; i++) {\n \t\tconst struct cache_entry *ce = cache[i];\n-\t\tif (ce_stage(ce)) {\n+\t\tif (ce_stage(ce) ||\n+\t\t    (core_error_commit_ita && ce_intent_to_add(ce))) {\n \t\t\tif (silent)\n \t\t\t\treturn -1;\n \t\t\tif (10 < ++funny) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\",\n-\t\t\t\tce->name, sha1_to_hex(ce->sha1));\n+\t\t\tif (ce_stage(ce))\n+\t\t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\",\n+\t\t\t\t\tce->name, sha1_to_hex(ce->sha1));\n+\t\t\telse\n+\t\t\t\tfprintf(stderr, \"%s: not added yet\\n\",\n+\t\t\t\t\tce->name);\n \t\t}\n \t}\n \tif (funny)\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288479","messageId":"xmqqfusqw17r.fsf@gitster.mtv.corp.google.com","threadId":"42536","inReplyTo":"20160606111643.7122-2-pclouds@gmail.com","subject":"Re: [PATCH 1/4] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T19:45:28Z","receivedAt":"2016-06-06T19:45:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Current flags field is 32-bits, all used except one bit and we need one\n> more bit is needed for to toggle i-t-a behavior. The 9th bit could be\n> reused for this, but we could just extend it to 64 bits now to give room\n> for more future flags.\n\nIsn't it a better option to add new things as separate words (or a\nseparate bit:1 field)?  Is this 32nd thing envisioned to be added\ngoing to be used everywhere like the existing flag bits, or can it\nbe handled like \"use_color\", \"break_opt\", \"show_rename_progress\"?\n"},{"id":"288480","messageId":"xmqqbn3ew0mf.fsf@gitster.mtv.corp.google.com","threadId":"42536","inReplyTo":"20160606111643.7122-4-pclouds@gmail.com","subject":"Re: [PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T19:58:16Z","receivedAt":"2016-06-06T19:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> i-t-a entries are excluded from tree building. Relying solely on active_nr\n> (or diff without --shift-ita) may lead to empty commits sometimes, when\n> i-t-a entries are the only ones \"changed\" in the index.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/commit.c | 12 +++++++++---\n>  1 file changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index fcfaa2b..e98ca8a 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\tif (amend)\n>  \t\t\tparent = \"HEAD^1\";\n>  \n> -\t\tif (get_sha1(parent, sha1))\n> -\t\t\tcommitable = !!active_nr;\n> -\t\telse {\n> +\t\tif (get_sha1(parent, sha1)) {\n> +\t\t\tint i, ita_nr = 0;\n> +\n> +\t\t\tfor (i = 0; i < active_nr; i++)\n> +\t\t\t\tif (ce_intent_to_add(active_cache[i]))\n> +\t\t\t\t\tita_nr++;\n> +\t\t\tcommitable = active_nr - ita_nr == 0;\n> +\t\t} else {\n>  \t\t\t/*\n>  \t\t\t * Unless the user did explicitly request a submodule\n>  \t\t\t * ignore mode by passing a command line option we do\n> @@ -910,6 +915,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\t\tif (ignore_submodule_arg &&\n>  \t\t\t    !strcmp(ignore_submodule_arg, \"all\"))\n>  \t\t\t\tdiff_flags |= DIFF_OPT_IGNORE_SUBMODULES;\n> +\t\t\tdiff_flags |= DIFF_OPT_SHIFT_INTENT_TO_ADD;\n>  \t\t\tcommitable = index_differs_from(parent, diff_flags);\n\nThis feels like an ugly hack.  Doesn't wt-status do a separate\n\"diff\" that enumerates what's different between the index and the\nHEAD?\n\nI am wondering if this \"we do not include status and do not ask\nrun_status() about commitable bit\" codepath should share more with\nthe other side, which you do not touch at all with this series,\nwhich in turn must be doing the right thing already, apparently\nwithout having to know anything about the SHIFT_INTENT_TO_ADD\nhackery.\n\nOr does \"git commit\" that uses run_status() still allow an empty\ncommit after this patch?\n"},{"id":"288483","messageId":"xmqqy46iukn6.fsf@gitster.mtv.corp.google.com","threadId":"42536","inReplyTo":"xmqqbn3ew0mf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T20:28:45Z","receivedAt":"2016-06-06T20:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am wondering if this \"we do not include status and do not ask\n> run_status() about commitable bit\" codepath should share more with\n> the other side, which you do not touch at all with this series,\n> which in turn must be doing the right thing already, apparently\n> without having to know anything about the SHIFT_INTENT_TO_ADD\n> hackery.\n>\n> Or does \"git commit\" that uses run_status() still allow an empty\n> commit after this patch?\n\nAh, I didn't see what 2/4 does.  The duplication still does make\nthis look an ugly hack, but I can see that the series is at least\ninternally consistent.\n"},{"id":"288486","messageId":"xmqqtwh6uk0k.fsf@gitster.mtv.corp.google.com","threadId":"42536","inReplyTo":"20160606111643.7122-3-pclouds@gmail.com","subject":"Re: [PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T20:42:19Z","receivedAt":"2016-06-06T20:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> +--shift-ita::\n> +\tBy default entries added by \"git add -N\" appear as an existing\n> +\tempty file in \"git diff\" and a new file in \"git diff --cached\".\n> +\tThis option makes the entry appear as a new file in \"git diff\"\n> +\tand non-existent in \"git diff --cached\".\n\nI do not think this should exist at the UI level, even though the\nuse of it in wt-status.c (below) makes a very good sense at least\nas a temporary band-aid.\n\nAt the philosophical level, I however think this \"I-T-A does not\nlogically exist in the index (yet)\" is a mistake, and \"an option\ncontrols if I-T-A does or does not exist depending on who calls it\"\nis even worse; it is a road to insanity.\n\nFor example, because I-T-A does not logically exist in the index,\n\"git reset --hard\" should not remove it but make it untracked again\n(but I do not think it does).  After \"git add -N foo\", because \"foo\"\ndoes not exist in the index, \"git clean\" should remove it for the\ndefinition of what's in the index to be logically consistent, but\nthe whole intent of \"add -N\" is that the user meant it is worth\nchecking into sometime in the future, which contradicts with its\nremoval upon \"clean\".\n\nSo, I dunno.\n"},{"id":"288502","messageId":"CACsJy8AVzWBsDhzj+UJo9AbSi0f=jrAyrLZ9sU=VBP3woFXr0A@mail.gmail.com","threadId":"42536","inReplyTo":"xmqqfusqw17r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-07T00:40:18Z","receivedAt":"2016-06-07T00:40:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 7, 2016 at 2:45 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Current flags field is 32-bits, all used except one bit and we need one\n>> more bit is needed for to toggle i-t-a behavior. The 9th bit could be\n>> reused for this, but we could just extend it to 64 bits now to give room\n>> for more future flags.\n>\n> Isn't it a better option to add new things as separate words (or a\n> separate bit:1 field)?  Is this 32nd thing envisioned to be added\n> going to be used everywhere like the existing flag bits, or can it\n> be handled like \"use_color\", \"break_opt\", \"show_rename_progress\"?\n\nIt could probably live as a separate bitfield, or an int. What scares\nme is \"flags\" sometimes is backed up then restored, but it's probably\nok. But we need to extend this \"flags\" eventually, don't we? I don't\nthink people will stop adding new  DIFF_OPT_ flags.\n-- \nDuy\n"},{"id":"288525","messageId":"40fabe80-7e68-bb9a-4b3d-c88c598cc5b8@atlas-elektronik.com","threadId":"42536","inReplyTo":"20160606111643.7122-2-pclouds@gmail.com","subject":"Re: [PATCH 1/4] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2016-06-07T06:40:41Z","receivedAt":"2016-06-07T06:40:41Z","isPatch":true,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Am 06.06.2016 um 13:16 schrieb Nguyễn Thái Ngọc Duy:\n> Current flags field is 32-bits, all used except one bit and we need one\n> more bit is needed for to toggle i-t-a behavior. The 9th bit could be\n\nSomething's wrong here. Either drop the \"is needed\" or the \"we need\".\n\n> reused for this, but we could just extend it to 64 bits now to give room\n> for more future flags.\n> \n> gcc -Wconversion is used to catch assignments that truncate bits. No new\n> warning was introduced (in fact one in index_differs_from() was\n> eliminated),\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/commit.c | 2 +-\n>  diff-lib.c       | 4 ++--\n>  diff.c           | 2 +-\n>  diff.h           | 6 +++---\n>  4 files changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 443ff91..fcfaa2b 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -906,7 +906,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\t\t * submodules which were manually staged, which would\n>  \t\t\t * be really confusing.\n>  \t\t\t */\n> -\t\t\tint diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> +\t\t\tuint64_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n>  \t\t\tif (ignore_submodule_arg &&\n>  \t\t\t    !strcmp(ignore_submodule_arg, \"all\"))\n>  \t\t\t\tdiff_flags |= DIFF_OPT_IGNORE_SUBMODULES;\n> diff --git a/diff-lib.c b/diff-lib.c\n> index bc49c70..27887d0 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n>  {\n>  \tint changed = ce_match_stat(ce, st, ce_option);\n>  \tif (S_ISGITLINK(ce->ce_mode)) {\n> -\t\tunsigned orig_flags = diffopt->flags;\n> +\t\tuint64_t orig_flags = diffopt->flags;\n>  \t\tif (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))\n>  \t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n>  \t\tif (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))\n> @@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n>  \treturn 0;\n>  }\n>  \n> -int index_differs_from(const char *def, int diff_flags)\n> +int index_differs_from(const char *def, uint64_t diff_flags)\n>  {\n>  \tstruct rev_info rev;\n>  \tstruct setup_revision_opt opt;\n> diff --git a/diff.c b/diff.c\n> index d3734d3..f70425f 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4936,7 +4936,7 @@ int diff_can_quit_early(struct diff_options *opt)\n>  static int is_submodule_ignored(const char *path, struct diff_options *options)\n>  {\n>  \tint ignored = 0;\n> -\tunsigned orig_flags = options->flags;\n> +\tuint64_t orig_flags = options->flags;\n>  \tif (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))\n>  \t\tset_diffopt_flags_from_submodule_config(options, path);\n>  \tif (DIFF_OPT_TST(options, IGNORE_SUBMODULES))\n> diff --git a/diff.h b/diff.h\n> index 125447b..b497078 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -115,8 +115,8 @@ struct diff_options {\n>  \tconst char *pickaxe;\n>  \tconst char *single_follow;\n>  \tconst char *a_prefix, *b_prefix;\n> -\tunsigned flags;\n> -\tunsigned touched_flags;\n> +\tuint64_t flags;\n> +\tuint64_t touched_flags;\n>  \n>  \t/* diff-filter bits */\n>  \tunsigned int filter;\n> @@ -348,7 +348,7 @@ extern int diff_result_code(struct diff_options *, int);\n>  \n>  extern void diff_no_index(struct rev_info *, int, const char **);\n>  \n> -extern int index_differs_from(const char *def, int diff_flags);\n> +extern int index_differs_from(const char *def, uint64_t diff_flags);\n>  \n>  /*\n>   * Fill the contents of the filespec \"df\", respecting any textconv defined by\n> \n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: Make Headlines..use a corduroy pillow....\npython -c \"print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')\" \nGPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF"},{"id":"288588","messageId":"CACsJy8DO+KU59LbQ0ZtTax6DZKqYmZVT6MHLmNK73GkO9qHf_w@mail.gmail.com","threadId":"42536","inReplyTo":"xmqqtwh6uk0k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-07T12:04:26Z","receivedAt":"2016-06-07T12:04:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 7, 2016 at 3:42 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> +--shift-ita::\n>> +     By default entries added by \"git add -N\" appear as an existing\n>> +     empty file in \"git diff\" and a new file in \"git diff --cached\".\n>> +     This option makes the entry appear as a new file in \"git diff\"\n>> +     and non-existent in \"git diff --cached\".\n>\n> I do not think this should exist at the UI level,\n\nI need it. I do \"git diff --stat\" and \"git diff --cached --stat\" a lot\nmore often than \"git status\". Without this option, I'm stuck with the\nold behavior.\n\n> even though the\n> use of it in wt-status.c (below) makes a very good sense at least\n> as a temporary band-aid.\n>\n> At the philosophical level, I however think this \"I-T-A does not\n> logically exist in the index (yet)\" is a mistake, and \"an option\n> controls if I-T-A does or does not exist depending on who calls it\"\n> is even worse; it is a road to insanity.\n\ni-t-a entries have dual personality (perhaps because it's implemented\nas an index entry). Although I think the \"does not exist\" aspect will\nwin in most cases. The intention behind the revert is we have more\ntime to examine case by case and gradually convert them. Maybe in the\nend one behavior wins and we no longer need another. A thought of\nkeeping i-t-a entries in an index extension instead crossed my mind.\nIt may simplify things a bit (e.g. there's no \"ghost\" entries any more\nand active_nr in 3/4 can remain \"the number of _real_ entries\"). The\nfew parts that do need to know about i-t-a entries need explict\nmodification (probably git-reset and git-diff). But I don't know yet\nif it would just lead to another nightmare.\n\n> For example, because I-T-A does not logically exist in the index,\n> \"git reset --hard\" should not remove it but make it untracked again\n> (but I do not think it does). After \"git add -N foo\", because \"foo\"\n> does not exist in the index, \"git clean\" should remove it for the\n> definition of what's in the index to be logically consistent, but\n> the whole intent of \"add -N\" is that the user meant it is worth\n> checking into sometime in the future, which contradicts with its\n> removal upon \"clean\".\n\nI think we should fix them. I started that and so far only 4d55200\n(grep: make it clear i-t-a entries are ignored - 2015-12-27) has made\nit to 'master'.\n\n> So, I dunno.\n\nI just remembered why the old behavior (abort to commit if i-t-a\nentries are present) bugged me: it does not work well with splitting\nchanges in worktree into multiple commits (e.g. with \"git add -p\").\nEven though I want git remind me to commit an i-t-a entry in the end,\nit does not necessarily mean I have to do it in the next commit, which\nmay cover a bunch of files except that i-t-a file. I don't see any way\naround that except ignoring i-t-a entries at commit time. If there's\nanother way, I'm all ears.\n-- \nDuy\n"},{"id":"288821","messageId":"alpine.DEB.2.20.1606091815310.2680@virtualbox","threadId":"42536","inReplyTo":"20160606111643.7122-3-pclouds@gmail.com","subject":"Re: [PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-09T16:18:15Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/diff.h b/diff.h\n> index b497078..9e42556 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n>  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)\n>  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)\n>  #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)\n> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)\n\nI am afraid that this is not enough. On Windows, sizeof(unsigned long) is\n4 (and it is perfectly legal). That means that you would have to use at\nleast (1ULL << 32), but then you also have to change the type of xdl_opts\nto uint64_t, which in turn means that you will have to change the\ndefinition of xpparam_t's \"flags\" field from unsigned long to uint64_t.\n\nMaybe this can be avoided?\n\nCiao,\nJohannes"},{"id":"302694","messageId":"CACsJy8A_CWgcS5za-Dha6Khgd6HqHk9UWHq7qqAeR-kns-syXg@mail.gmail.com","threadId":"42536","inReplyTo":"alpine.DEB.2.20.1606091815310.2680@virtualbox","subject":"Re: [PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-09-27T10:58:39Z","receivedAt":"2016-09-27T10:59:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(sorry for a very late reply, I'm just picking this series up again)\n\nOn Thu, Jun 9, 2016 at 11:18 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Duy,\n>\n> On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote:\n>\n>> diff --git a/diff.h b/diff.h\n>> index b497078..9e42556 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n>>  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)\n>>  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)\n>>  #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)\n>> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)\n>\n> I am afraid that this is not enough. On Windows, sizeof(unsigned long) is\n> 4 (and it is perfectly legal). That means that you would have to use at\n> least (1ULL << 32),\n\nOK.\n\n> but then you also have to change the type of xdl_opts\n> to uint64_t, which in turn means that you will have to change the\n> definition of xpparam_t's \"flags\" field from unsigned long to uint64_t.\n\nI miss a connection here. This new flag is intended to be used in\n\"flags\" field in struct diff_options. Is there any chance it can be\nset on xdl_opts (of the same struct, I assume)?\n\n> Maybe this can be avoided?\n\nI don't see a good way to avoid it. We normally enable or disable diff\nfeatures as bit flags and now we run out of bits. Adding something\nlike \"flags2\" works, but not pretty. Any suggestion is welcome.\n-- \nDuy\n"},{"id":"302697","messageId":"CACsJy8CsfPqOXReM6tzBDyVob-oSAKinoA6C6TKjSyy6Y9WuJQ@mail.gmail.com","threadId":"42536","inReplyTo":"CACsJy8A_CWgcS5za-Dha6Khgd6HqHk9UWHq7qqAeR-kns-syXg@mail.gmail.com","subject":"Re: [PATCH 2/4] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-09-27T12:27:42Z","receivedAt":"2016-09-27T12:28:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 27, 2016 at 5:58 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> but then you also have to change the type of xdl_opts\n>> to uint64_t, which in turn means that you will have to change the\n>> definition of xpparam_t's \"flags\" field from unsigned long to uint64_t.\n>\n> I miss a connection here. This new flag is intended to be used in\n> \"flags\" field in struct diff_options. Is there any chance it can be\n> set on xdl_opts (of the same struct, I assume)?\n>\n>> Maybe this can be avoided?\n>\n> I don't see a good way to avoid it. We normally enable or disable diff\n> features as bit flags and now we run out of bits. Adding something\n> like \"flags2\" works, but not pretty. Any suggestion is welcome.\n\nNever mind. I think I found some way that does not look particularly bad.\n-- \nDuy\n"}]}