* [PATCH RFC] commit: allow to commit even if there are intent-to-add entries @ 2012-01-11 6:01 Nguyễn Thái Ngọc Duy 2012-01-11 8:08 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-11 6:01 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy This patch replaces the approach in 331fcb5 (git add --intent-to-add: do not let an empty blob be committed by accident) regarding i-t-a entries: instead of forbidding i-t-a entries at commit time, we can simply ignore them. We already ignore CE_REMOVE entries while updating cache-tree. Putting CE_INTENT_TO_ADD ones in the same category should not cause any negative effects regarding cache-tree. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- On the few chances I have to use "git add -N" it does not fit well with "git add -p; git diff --cached; git commit -m foo" style. I think this may be a good thing to do. builtin/commit.c | 2 +- builtin/write-tree.c | 2 +- cache-tree.c | 14 +++++--------- t/t2203-add-intent.sh | 10 +++++++++- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index eba1377..767b78a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -871,7 +871,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, discard_cache(); read_cache_from(index_file); if (update_main_cache_tree(0)) { - error(_("Error building trees")); + error(_("Error building trees; the index is unmerged?")); return 0; } diff --git a/builtin/write-tree.c b/builtin/write-tree.c index b223af4..68baa24 100644 --- a/builtin/write-tree.c +++ b/builtin/write-tree.c @@ -46,7 +46,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix) die("%s: error reading the index", me); break; case WRITE_TREE_UNMERGED_INDEX: - die("%s: error building trees", me); + die("%s: error building trees; the index is unmerged?", me); break; case WRITE_TREE_PREFIX_ERROR: die("%s: prefix %s not found", me, prefix); diff --git a/cache-tree.c b/cache-tree.c index 8de3959..47defd1 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -158,19 +158,15 @@ static int verify_cache(struct cache_entry **cache, funny = 0; for (i = 0; i < entries; i++) { struct cache_entry *ce = cache[i]; - if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) { + if (ce_stage(ce)) { if (silent) return -1; if (10 < ++funny) { fprintf(stderr, "...\n"); break; } - if (ce_stage(ce)) - fprintf(stderr, "%s: unmerged (%s)\n", - ce->name, sha1_to_hex(ce->sha1)); - else - fprintf(stderr, "%s: not added yet\n", - ce->name); + fprintf(stderr, "%s: unmerged (%s)\n", + ce->name, sha1_to_hex(ce->sha1)); } } if (funny) @@ -338,8 +334,8 @@ static int update_one(struct cache_tree *it, mode, sha1_to_hex(sha1), entlen+baselen, path); } - if (ce->ce_flags & CE_REMOVE) - continue; /* entry being removed */ + if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD)) + continue; /* entry being removed or just placeholder */ strbuf_grow(&buffer, entlen + 100); strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0'); diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 2543529..65430e4 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -41,7 +41,15 @@ test_expect_success 'cannot commit with i-t-a entry' ' echo frotz >nitfol && git add rezrov && git add -N nitfol && - test_must_fail git commit -m initial + git commit -m initial && + git ls-tree -r HEAD >actual && + cat >expected <<EOF && +100644 blob ce013625030ba8dba906f756967f9e9ca394464a elif +100644 blob ce013625030ba8dba906f756967f9e9ca394464a file +100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4 rezrov +EOF + test_cmp expected actual && + git reset HEAD^ ' test_expect_success 'can commit with an unrelated i-t-a entry in index' ' -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-11 6:01 [PATCH RFC] commit: allow to commit even if there are intent-to-add entries Nguyễn Thái Ngọc Duy @ 2012-01-11 8:08 ` Junio C Hamano 2012-01-11 11:02 ` Jonathan Nieder 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2012-01-11 8:08 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > This patch replaces the approach in 331fcb5 (git add --intent-to-add: > do not let an empty blob be committed by accident) regarding i-t-a > entries: instead of forbidding i-t-a entries at commit time, we can > simply ignore them. I have a mild suspicion that in earlier incarnation of the patch we used to let empty blobs committed, and then we used to instead not commit anything at all for such a path, and the real users were bitten by either of these approaches, forgetting to add the contents to the final commit. So I am not sure if this is such a good idea. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-11 8:08 ` Junio C Hamano @ 2012-01-11 11:02 ` Jonathan Nieder 2012-01-11 21:08 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2012-01-11 11:02 UTC (permalink / raw) To: Junio C Hamano Cc: Nguyễn Thái Ngọc Duy, git, Jeff King, Will Palmer Hi, Junio C Hamano wrote: > I have a mild suspicion that in earlier incarnation of the patch we used > to let empty blobs committed, and then we used to instead not commit > anything at all for such a path, and the real users were bitten by either > of these approaches, forgetting to add the contents to the final commit. I remember the empty blob era. :) I don't think I ever saw something like this patch, though, and a quick search finds that the first iteration of the bugfix to stop commiting empty blobs was the one that was used: http://thread.gmane.org/gmane.comp.version-control.git/101881/focus=101894 I suspect that at the time, introducing an intent-to-add flag (which was always the right thing to do) provided enough momentum to avoid any worries about smaller details like whether to error out or skip those entries on commit, which could always be changed later (today). > So I am not sure if this is such a good idea. My first reaction was the same, but on reflection, I think this is a good idea as long as the "git status" output in the editor says something appropriate. The response Duy mentioned[1] to a report about the unenlightening message "error building trees" was also memorable: > When running "commit" and "status" with files marked with "intent to add", > I think there are three possible interpretations of what the user > wants to do. [ (1) thanks for stopping me, I had forgotten about that file; (2) I changed my mind, please leave that file out; or (3) please dwim and add the file ] I think (3) was a trick --- no one that does not use the "-a" option would want that. :) At the time, I did not understand what (2) meant. Now I see why --- in interpretation (2), the user did not change her mind at all. She said "I will be adding this file at some point, so please keep track of it along with the others for the sake of commands like 'git diff' and 'git add -u', but that does not mean "I will be adding this file at some point _before the next commit_". So at the time I thought (1) was the only sensible behavior but kept my mouth shut; and now I see that (1) and (2) both fit into reasonable workflows. However. A person using "git diff" to review remaining changes and "git add" to mark files once they have reached their final form would benefit even more from a switch for "git commit" to error out if _any_ files in the worktree do not match the index. So if we are to take this workflow seriously, treating "git add -N" as a special case is not helping. What we currently provide for this workflow is a reminder in the status area of changes that were not marked with "git add". I suspect no longer erroring out might feel eerie for a period for people that were relying on "git add -N" as a reminder but that as long as the "git status" output is sensible enough, Duy's proposed behavior would end up seeming just as natural. (2) makes intent-to-add entries just like any other tracked index entry with some un-added content. It is conceptually pleasant and fits well in all workflows I can imagine[2]. Hope that helps, and sorry for the ramble, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/170651/focus=170658 [2] Ok, that is a small lie. In "git stash", a commit is used to save the state of the index, so the user would want the presence of the intent-to-add entry to be stored somehow in the commit, and none of (1), (2), or (3) will make her happy. Using "git commit" this way is not going to work when there are unmerged entries, for example, either, so I think it is okay to ignore that problem here. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-11 11:02 ` Jonathan Nieder @ 2012-01-11 21:08 ` Junio C Hamano 2012-01-12 2:53 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2012-01-11 21:08 UTC (permalink / raw) To: Jonathan Nieder Cc: Nguyễn Thái Ngọc Duy, git, Jeff King, Will Palmer Jonathan Nieder <jrnieder@gmail.com> writes: >> When running "commit" and "status" with files marked with "intent to add", >> I think there are three possible interpretations of what the user >> wants to do. > [ (1) thanks for stopping me, I had forgotten about that file; > (2) I changed my mind, please leave that file out; or (3) please > dwim and add the file ] > > I think (3) was a trick --- no one that does not use the "-a" option > would want that. :) I really wish it were the case, but I doubt it. People from other VCS background seem to still think that "commit" without paths should commit everything; after getting told that "what you add to the index is what you will commit", I can easily see this commint: but but but I told Git that I _want_ to add with -N! Why aren't you committing it? > At the time, I did not understand what (2) meant. Now I see why --- > in interpretation (2), the user did not change her mind at all. You are correct. "I still cannot make up my mind" is what is happening in that situation. The user explicitly said "I cannot decide about this path right now" when she said "add -N". And we haven't heard from the user what should happen to the path. Now we have to make a commit so somebody needs to decide. > She > said "I will be adding this file at some point, so please keep track > of it along with the others for the sake of commands like 'git diff' > and 'git add -u', but that does not mean "I will be adding this file > at some point _before the next commit_". Correct. She only said "I cannot decide right now" when she said "add -N" and hasn't gave us any more hint as to what should happen now we have to make a commit. It is _wrong_ for us to unilaterally decide for the user that she does not want the path in the commit. The last we heard from her was that she does not know what should happen to the path. > (2) makes intent-to-add entries just like any other tracked index > entry with some un-added content. You are comparing files edited in the working tree without the user telling anything about them to Git (both tracked and untracked) and files the user explicitly told Git that the user hasn't made up her mind about. Why is it a good thing to make the latter behave "just like any other"? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-11 21:08 ` Junio C Hamano @ 2012-01-12 2:53 ` Nguyen Thai Ngoc Duy 2012-01-12 3:05 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Nguyen Thai Ngoc Duy @ 2012-01-12 2:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git, Jeff King, Will Palmer 2012/1/12 Junio C Hamano <gitster@pobox.com>: > Jonathan Nieder <jrnieder@gmail.com> writes: > >>> When running "commit" and "status" with files marked with "intent to add", >>> I think there are three possible interpretations of what the user >>> wants to do. >> [ (1) thanks for stopping me, I had forgotten about that file; >> (2) I changed my mind, please leave that file out; or (3) please >> dwim and add the file ] >> >> I think (3) was a trick --- no one that does not use the "-a" option >> would want that. :) > > I really wish it were the case, but I doubt it. > > People from other VCS background seem to still think that "commit" without > paths should commit everything; after getting told that "what you add to > the index is what you will commit", I can easily see this commint: but but > but I told Git that I _want_ to add with -N! Why aren't you committing it? I see "-N" just as an indication, not really an "add" operation. Perhaps update-index is a better place for it. >> (2) makes intent-to-add entries just like any other tracked index >> entry with some un-added content. > > You are comparing files edited in the working tree without the user > telling anything about them to Git (both tracked and untracked) and files > the user explicitly told Git that the user hasn't made up her mind > about. Why is it a good thing to make the latter behave "just like any > other"? The way I see this flag is "include these files in my diff in addition to tracked files", and therefore should not have any effects at commit time. I might turn some of those extra files to tracked some time if I want to commit. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-12 2:53 ` Nguyen Thai Ngoc Duy @ 2012-01-12 3:05 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2012-01-12 3:05 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: Jonathan Nieder, git, Jeff King, Will Palmer Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes: >> I really wish it were the case, but I doubt it. >> >> People from other VCS background seem to still think that "commit" without >> paths should commit everything; after getting told that "what you add to >> the index is what you will commit", I can easily see this commint: but but >> but I told Git that I _want_ to add with -N! Why aren't you committing it? > > I see "-N" just as an indication, not really an "add" operation. But the thing is, what _you_, who knows a bit more than these new people about Git, see does not matter, because they would not listen to you. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries 2012-01-11 6:01 [PATCH RFC] commit: allow to commit even if there are intent-to-add entries Nguyễn Thái Ngọc Duy 2012-01-11 8:08 ` Junio C Hamano @ 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy 2012-01-11 9:59 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy 2012-01-11 9:59 ` [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index Nguyễn Thái Ngọc Duy 3 siblings, 0 replies; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-11 9:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy 2012/1/11 Junio C Hamano <gitster@pobox.com>: > I have a mild suspicion that in earlier incarnation of the patch we used > to let empty blobs committed, and then we used to instead not commit > anything at all for such a path, and the real users were bitten by either > of these approaches, forgetting to add the contents to the final commit. > > So I am not sure if this is such a good idea. I found your elaborate writing [1] about it. These are the interpretations listed in that post: -- 8< -- When running "commit" and "status" with files marked with "intent to add", I think there are three possible interpretations of what the user wants to do. (1) I earlier said "I'll decide the exact contents to be committed for these paths and tell you by running 'git add' later." when I said 'git add -N'. But I forgot to do so before running "git commit". Thanks for catching this mistake and erroring out. (2) I said "I'll decide the exact content to be committed ... later." when I said 'git add -N'. I am running "git commit" now, but I still don't know what the final contents for this path should be. I changed my mind, and I do not want to include the addition of these paths in the commit I am making. Please do not error out, but just ignore the earlier 'add -N' for now. (3) I said "I'll decide the exact content to be committed ... later." when I said 'git add -N'. I am running "git commit" now, without explicitly telling you with 'git add' about the final contents for these paths. Please take it as an implicit hint that I am happy with the contents in the working tree and commit them as if I said 'git add' on these paths, but do leave modifications to already tracked paths that I haven't added with 'git add'. -- 8< -- So (1) may be the safe and sane interpretation and should be the default. But perhaps we should allow (2) also, for example with --skip-intent-to-add option? It's really frustrating to remove all i-t-a, commit (I don't do "commit <path>"), then add them back. [1] http://article.gmane.org/gmane.comp.version-control.git/170658 Nguyễn Thái Ngọc Duy (2): cache-tree: update API to take abitrary flags commit: add --skip-intent-to-add to allow commit with i-t-a entries in index builtin/commit.c | 10 +++++++--- cache-tree.c | 35 +++++++++++++++++------------------ cache-tree.h | 5 ++++- merge-recursive.c | 2 +- t/t2203-add-intent.sh | 17 +++++++++++++++++ test-dump-cache-tree.c | 2 +- 6 files changed, 47 insertions(+), 24 deletions(-) -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] cache-tree: update API to take abitrary flags 2012-01-11 6:01 [PATCH RFC] commit: allow to commit even if there are intent-to-add entries Nguyễn Thái Ngọc Duy 2012-01-11 8:08 ` Junio C Hamano 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy @ 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy 2012-01-11 23:48 ` Junio C Hamano 2012-01-11 9:59 ` [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index Nguyễn Thái Ngọc Duy 3 siblings, 1 reply; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-11 9:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy --- builtin/commit.c | 4 ++-- cache-tree.c | 27 ++++++++++++--------------- cache-tree.h | 4 +++- merge-recursive.c | 2 +- test-dump-cache-tree.c | 2 +- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index eba1377..bf42bb3 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -400,7 +400,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); add_files_to_cache(also ? prefix : NULL, pathspec, 0); refresh_cache_or_die(refresh_flags); - update_main_cache_tree(1); + update_main_cache_tree(WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || close_lock_file(&index_lock)) die(_("unable to write new_index file")); @@ -421,7 +421,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); refresh_cache_or_die(refresh_flags); if (active_cache_changed) { - update_main_cache_tree(1); + update_main_cache_tree(WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || commit_locked_index(&index_lock)) die(_("unable to write new_index file")); diff --git a/cache-tree.c b/cache-tree.c index 8de3959..16355d6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -150,9 +150,10 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path) } static int verify_cache(struct cache_entry **cache, - int entries, int silent) + int entries, int flags) { int i, funny; + int silent = flags & WRITE_TREE_SILENT; /* Verify that the tree is merged */ funny = 0; @@ -241,10 +242,11 @@ static int update_one(struct cache_tree *it, int entries, const char *base, int baselen, - int missing_ok, - int dryrun) + int flags) { struct strbuf buffer; + int missing_ok = flags & WRITE_TREE_MISSING_OK; + int dryrun = flags & WRITE_TREE_DRY_RUN; int i; if (0 <= it->entry_count && has_sha1_file(it->sha1)) @@ -288,8 +290,7 @@ static int update_one(struct cache_tree *it, cache + i, entries - i, path, baselen + sublen + 1, - missing_ok, - dryrun); + flags); if (subcnt < 0) return subcnt; i += subcnt - 1; @@ -371,15 +372,13 @@ static int update_one(struct cache_tree *it, int cache_tree_update(struct cache_tree *it, struct cache_entry **cache, int entries, - int missing_ok, - int dryrun, - int silent) + int flags) { int i; - i = verify_cache(cache, entries, silent); + i = verify_cache(cache, entries, flags); if (i) return i; - i = update_one(it, cache, entries, "", 0, missing_ok, dryrun); + i = update_one(it, cache, entries, "", 0, flags); if (i < 0) return i; return 0; @@ -572,11 +571,9 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) was_valid = cache_tree_fully_valid(active_cache_tree); if (!was_valid) { - int missing_ok = flags & WRITE_TREE_MISSING_OK; - if (cache_tree_update(active_cache_tree, active_cache, active_nr, - missing_ok, 0, 0) < 0) + flags) < 0) return WRITE_TREE_UNMERGED_INDEX; if (0 <= newfd) { if (!write_cache(newfd, active_cache, active_nr) && @@ -672,10 +669,10 @@ int cache_tree_matches_traversal(struct cache_tree *root, return 0; } -int update_main_cache_tree (int silent) +int update_main_cache_tree (int flags) { if (!the_index.cache_tree) the_index.cache_tree = cache_tree(); return cache_tree_update(the_index.cache_tree, - the_index.cache, the_index.cache_nr, 0, 0, silent); + the_index.cache, the_index.cache_nr, flags); } diff --git a/cache-tree.h b/cache-tree.h index 0ec0b2a..d8cb2e9 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -29,13 +29,15 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root); struct cache_tree *cache_tree_read(const char *buffer, unsigned long size); int cache_tree_fully_valid(struct cache_tree *); -int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int, int); +int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int); int update_main_cache_tree(int); /* bitmasks to write_cache_as_tree flags */ #define WRITE_TREE_MISSING_OK 1 #define WRITE_TREE_IGNORE_CACHE_TREE 2 +#define WRITE_TREE_DRY_RUN 4 +#define WRITE_TREE_SILENT 8 /* error return codes */ #define WRITE_TREE_UNREADABLE_INDEX (-1) diff --git a/merge-recursive.c b/merge-recursive.c index d83cd6c..6479a60 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -264,7 +264,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) if (!cache_tree_fully_valid(active_cache_tree) && cache_tree_update(active_cache_tree, - active_cache, active_nr, 0, 0, 0) < 0) + active_cache, active_nr, 0) < 0) die("error building trees"); result = lookup_tree(active_cache_tree->sha1); diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c index e6c2923..a6ffdf3 100644 --- a/test-dump-cache-tree.c +++ b/test-dump-cache-tree.c @@ -59,6 +59,6 @@ int main(int ac, char **av) struct cache_tree *another = cache_tree(); if (read_cache() < 0) die("unable to read index file"); - cache_tree_update(another, active_cache, active_nr, 0, 1, 0); + cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN); return dump_cache_tree(active_cache_tree, another, ""); } -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] cache-tree: update API to take abitrary flags 2012-01-11 9:59 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy @ 2012-01-11 23:48 ` Junio C Hamano 2012-01-12 1:20 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2012-01-11 23:48 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > --- Thanks; this one looks very sensible regardless of what follows (or does not follow). Forgot to sign-off? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] cache-tree: update API to take abitrary flags 2012-01-11 23:48 ` Junio C Hamano @ 2012-01-12 1:20 ` Nguyen Thai Ngoc Duy 0 siblings, 0 replies; 13+ messages in thread From: Nguyen Thai Ngoc Duy @ 2012-01-12 1:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 2012/1/12 Junio C Hamano <gitster@pobox.com>: > Thanks; this one looks very sensible regardless of what follows (or does > not follow). Forgot to sign-off? Deliberately to stop you from using it because I did not test it carefully. It was created as material for the discussion only. Will resubmit later. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index 2012-01-11 6:01 [PATCH RFC] commit: allow to commit even if there are intent-to-add entries Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2012-01-11 9:59 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy @ 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy 2012-01-11 23:55 ` Junio C Hamano 3 siblings, 1 reply; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-11 9:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy --- builtin/commit.c | 10 +++++++--- cache-tree.c | 8 +++++--- cache-tree.h | 1 + t/t2203-add-intent.sh | 17 +++++++++++++++++ 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index bf42bb3..021206e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; static int no_post_rewrite, allow_empty_message; +static int cache_tree_flags, skip_intent_to_add; static char *untracked_files_arg, *force_date, *ignore_submodule_arg; static char *sign_commit; @@ -170,6 +171,7 @@ static struct option builtin_commit_options[] = { OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"), OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"), { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" }, + OPT_BOOL(0, "skip-intent-to-add", &skip_intent_to_add, "allow intent-to-add entries in index"), /* end commit contents options */ { OPTION_BOOLEAN, 0, "allow-empty", &allow_empty, NULL, @@ -400,7 +402,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); add_files_to_cache(also ? prefix : NULL, pathspec, 0); refresh_cache_or_die(refresh_flags); - update_main_cache_tree(WRITE_TREE_SILENT); + update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || close_lock_file(&index_lock)) die(_("unable to write new_index file")); @@ -421,7 +423,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); refresh_cache_or_die(refresh_flags); if (active_cache_changed) { - update_main_cache_tree(WRITE_TREE_SILENT); + update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || commit_locked_index(&index_lock)) die(_("unable to write new_index file")); @@ -870,7 +872,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, */ discard_cache(); read_cache_from(index_file); - if (update_main_cache_tree(0)) { + if (update_main_cache_tree(cache_tree_flags)) { error(_("Error building trees")); return 0; } @@ -1088,6 +1090,8 @@ static int parse_and_validate_options(int argc, const char *argv[], cleanup_mode = CLEANUP_ALL; else die(_("Invalid cleanup mode %s"), cleanup_arg); + if (skip_intent_to_add) + cache_tree_flags = WRITE_TREE_INTENT_TO_ADD_OK; handle_untracked_files_arg(s); diff --git a/cache-tree.c b/cache-tree.c index 16355d6..b8593e0 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -159,7 +159,9 @@ static int verify_cache(struct cache_entry **cache, funny = 0; for (i = 0; i < entries; i++) { struct cache_entry *ce = cache[i]; - if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) { + if (ce_stage(ce) || + ((flags & WRITE_TREE_INTENT_TO_ADD_OK) == 0 && + (ce->ce_flags & CE_INTENT_TO_ADD))) { if (silent) return -1; if (10 < ++funny) { @@ -339,8 +341,8 @@ static int update_one(struct cache_tree *it, mode, sha1_to_hex(sha1), entlen+baselen, path); } - if (ce->ce_flags & CE_REMOVE) - continue; /* entry being removed */ + if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD)) + continue; /* entry being removed or placeholder */ strbuf_grow(&buffer, entlen + 100); strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0'); diff --git a/cache-tree.h b/cache-tree.h index d8cb2e9..865733c 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -38,6 +38,7 @@ int update_main_cache_tree(int); #define WRITE_TREE_IGNORE_CACHE_TREE 2 #define WRITE_TREE_DRY_RUN 4 #define WRITE_TREE_SILENT 8 +#define WRITE_TREE_INTENT_TO_ADD_OK 16 /* error return codes */ #define WRITE_TREE_UNREADABLE_INDEX (-1) diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 2543529..990c765 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -44,6 +44,23 @@ test_expect_success 'cannot commit with i-t-a entry' ' test_must_fail git commit -m initial ' +test_expect_success 'can commit with i-t-a entry' ' + git reset --hard && + echo xyzzy >rezrov && + echo frotz >nitfol && + git add rezrov && + git add -N nitfol && + git commit --skip-intent-to-add -m initial && + git ls-tree -r HEAD >actual && + cat >expected <<EOF && +100644 blob ce013625030ba8dba906f756967f9e9ca394464a elif +100644 blob ce013625030ba8dba906f756967f9e9ca394464a file +100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4 rezrov +EOF + test_cmp expected actual && + git reset HEAD^ +' + test_expect_success 'can commit with an unrelated i-t-a entry in index' ' git reset --hard && echo xyzzy >rezrov && -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index 2012-01-11 9:59 ` [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index Nguyễn Thái Ngọc Duy @ 2012-01-11 23:55 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2012-01-11 23:55 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > --- > builtin/commit.c | 10 +++++++--- > cache-tree.c | 8 +++++--- > cache-tree.h | 1 + > t/t2203-add-intent.sh | 17 +++++++++++++++++ > 4 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index bf42bb3..021206e 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff; > static int edit_flag = -1; /* unspecified */ > static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; > static int no_post_rewrite, allow_empty_message; > +static int cache_tree_flags, skip_intent_to_add; > static char *untracked_files_arg, *force_date, *ignore_submodule_arg; > static char *sign_commit; > > @@ -170,6 +171,7 @@ static struct option builtin_commit_options[] = { > OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"), > OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"), > { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" }, > + OPT_BOOL(0, "skip-intent-to-add", &skip_intent_to_add, "allow intent-to-add entries in index"), This is more like "ignore", not "allow", from end user's point of view, no? The user earlier said "I cannot decide what contents to put in the commit yet for this path", and normally we catch it and remind the user that she needs to decide. This option gives her a quick way to say "I decide that I do not want to add this path at all to this commit I am creating, so please ignore it in the meantime." > @@ -1088,6 +1090,8 @@ static int parse_and_validate_options(int argc, const char *argv[], > cleanup_mode = CLEANUP_ALL; > else > die(_("Invalid cleanup mode %s"), cleanup_arg); > + if (skip_intent_to_add) > + cache_tree_flags = WRITE_TREE_INTENT_TO_ADD_OK; The name WRITE_TREE_INTENT_TO_ADD_OK says "it is OK to call write-tree with i-t-a entries in the index, please do not barf", but I think "when writing a tree, ignore i-t-a entries" would be a more appropriate way to say the same thing, i.e. WRITE_TREE_IGNORE_INTENT_TO_ADD. Other than that, I do not see an issue in the implementation of the patch. It is a separate design level issue if we want to worsen proliferation of the options, though. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] nd/commit-ignore-i-t-a replacement @ 2012-01-16 2:36 Nguyễn Thái Ngọc Duy 2012-01-16 2:36 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-16 2:36 UTC (permalink / raw) To: git Cc: Jonathan Nieder, Junio C Hamano, Nguyễn Thái Ngọc Duy This replaces current topic branch in pu with a more sensible approach using config key. write-tree also learns about this. Nguyễn Thái Ngọc Duy (2): cache-tree: update API to take abitrary flags commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees Documentation/config.txt | 5 +++++ Documentation/git-add.txt | 12 ++++++++++-- Documentation/git-write-tree.txt | 8 +++++++- builtin/commit.c | 13 ++++++++++--- builtin/write-tree.c | 2 ++ cache-tree.c | 35 +++++++++++++++++------------------ cache-tree.h | 5 ++++- merge-recursive.c | 2 +- t/t2203-add-intent.sh | 30 ++++++++++++++++++++++++++++++ test-dump-cache-tree.c | 2 +- 10 files changed, 87 insertions(+), 27 deletions(-) -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] cache-tree: update API to take abitrary flags 2012-01-16 2:36 [PATCH 0/2] nd/commit-ignore-i-t-a replacement Nguyễn Thái Ngọc Duy @ 2012-01-16 2:36 ` Nguyễn Thái Ngọc Duy 0 siblings, 0 replies; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-01-16 2:36 UTC (permalink / raw) To: git Cc: Jonathan Nieder, Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/commit.c | 4 ++-- cache-tree.c | 27 ++++++++++++--------------- cache-tree.h | 4 +++- merge-recursive.c | 2 +- test-dump-cache-tree.c | 2 +- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index eba1377..bf42bb3 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -400,7 +400,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); add_files_to_cache(also ? prefix : NULL, pathspec, 0); refresh_cache_or_die(refresh_flags); - update_main_cache_tree(1); + update_main_cache_tree(WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || close_lock_file(&index_lock)) die(_("unable to write new_index file")); @@ -421,7 +421,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); refresh_cache_or_die(refresh_flags); if (active_cache_changed) { - update_main_cache_tree(1); + update_main_cache_tree(WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || commit_locked_index(&index_lock)) die(_("unable to write new_index file")); diff --git a/cache-tree.c b/cache-tree.c index 8de3959..16355d6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -150,9 +150,10 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path) } static int verify_cache(struct cache_entry **cache, - int entries, int silent) + int entries, int flags) { int i, funny; + int silent = flags & WRITE_TREE_SILENT; /* Verify that the tree is merged */ funny = 0; @@ -241,10 +242,11 @@ static int update_one(struct cache_tree *it, int entries, const char *base, int baselen, - int missing_ok, - int dryrun) + int flags) { struct strbuf buffer; + int missing_ok = flags & WRITE_TREE_MISSING_OK; + int dryrun = flags & WRITE_TREE_DRY_RUN; int i; if (0 <= it->entry_count && has_sha1_file(it->sha1)) @@ -288,8 +290,7 @@ static int update_one(struct cache_tree *it, cache + i, entries - i, path, baselen + sublen + 1, - missing_ok, - dryrun); + flags); if (subcnt < 0) return subcnt; i += subcnt - 1; @@ -371,15 +372,13 @@ static int update_one(struct cache_tree *it, int cache_tree_update(struct cache_tree *it, struct cache_entry **cache, int entries, - int missing_ok, - int dryrun, - int silent) + int flags) { int i; - i = verify_cache(cache, entries, silent); + i = verify_cache(cache, entries, flags); if (i) return i; - i = update_one(it, cache, entries, "", 0, missing_ok, dryrun); + i = update_one(it, cache, entries, "", 0, flags); if (i < 0) return i; return 0; @@ -572,11 +571,9 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) was_valid = cache_tree_fully_valid(active_cache_tree); if (!was_valid) { - int missing_ok = flags & WRITE_TREE_MISSING_OK; - if (cache_tree_update(active_cache_tree, active_cache, active_nr, - missing_ok, 0, 0) < 0) + flags) < 0) return WRITE_TREE_UNMERGED_INDEX; if (0 <= newfd) { if (!write_cache(newfd, active_cache, active_nr) && @@ -672,10 +669,10 @@ int cache_tree_matches_traversal(struct cache_tree *root, return 0; } -int update_main_cache_tree (int silent) +int update_main_cache_tree (int flags) { if (!the_index.cache_tree) the_index.cache_tree = cache_tree(); return cache_tree_update(the_index.cache_tree, - the_index.cache, the_index.cache_nr, 0, 0, silent); + the_index.cache, the_index.cache_nr, flags); } diff --git a/cache-tree.h b/cache-tree.h index 0ec0b2a..d8cb2e9 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -29,13 +29,15 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root); struct cache_tree *cache_tree_read(const char *buffer, unsigned long size); int cache_tree_fully_valid(struct cache_tree *); -int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int, int); +int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int); int update_main_cache_tree(int); /* bitmasks to write_cache_as_tree flags */ #define WRITE_TREE_MISSING_OK 1 #define WRITE_TREE_IGNORE_CACHE_TREE 2 +#define WRITE_TREE_DRY_RUN 4 +#define WRITE_TREE_SILENT 8 /* error return codes */ #define WRITE_TREE_UNREADABLE_INDEX (-1) diff --git a/merge-recursive.c b/merge-recursive.c index d83cd6c..6479a60 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -264,7 +264,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) if (!cache_tree_fully_valid(active_cache_tree) && cache_tree_update(active_cache_tree, - active_cache, active_nr, 0, 0, 0) < 0) + active_cache, active_nr, 0) < 0) die("error building trees"); result = lookup_tree(active_cache_tree->sha1); diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c index e6c2923..a6ffdf3 100644 --- a/test-dump-cache-tree.c +++ b/test-dump-cache-tree.c @@ -59,6 +59,6 @@ int main(int ac, char **av) struct cache_tree *another = cache_tree(); if (read_cache() < 0) die("unable to read index file"); - cache_tree_update(another, active_cache, active_nr, 0, 1, 0); + cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN); return dump_cache_tree(active_cache_tree, another, ""); } -- 1.7.3.1.256.g2539c.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-01-16 2:37 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-11 6:01 [PATCH RFC] commit: allow to commit even if there are intent-to-add entries Nguyễn Thái Ngọc Duy 2012-01-11 8:08 ` Junio C Hamano 2012-01-11 11:02 ` Jonathan Nieder 2012-01-11 21:08 ` Junio C Hamano 2012-01-12 2:53 ` Nguyen Thai Ngoc Duy 2012-01-12 3:05 ` Junio C Hamano 2012-01-11 9:59 ` Nguyễn Thái Ngọc Duy 2012-01-11 9:59 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy 2012-01-11 23:48 ` Junio C Hamano 2012-01-12 1:20 ` Nguyen Thai Ngoc Duy 2012-01-11 9:59 ` [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index Nguyễn Thái Ngọc Duy 2012-01-11 23:55 ` Junio C Hamano 2012-01-16 2:36 [PATCH 0/2] nd/commit-ignore-i-t-a replacement Nguyễn Thái Ngọc Duy 2012-01-16 2:36 ` [PATCH 1/2] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).