[BUG] git stash refuses to save after "add -N"

[BUG] git stash refuses to save after "add -N"

[Yann Dirson]

$ echo foo > bar
$ git add -N bar
$ ./git --exec-path=$PWD stash save
bar: not added yet
fatal: git-write-tree: error building trees
Cannot save the current index state


Maybe it would require some magic in git-stash to detect/save/restore that
particular state, or "just" to cause "add -N" to insert an empty file
instead ?

I suspect that second solution would not be particularly popular, but I
don't find the 1st one very appealing, it just looks like breaking the
whole idea behind git-stash :)

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Fri, Aug 28, 2009 at 01:02:23PM +0200, Yann Dirson wrote:

> $ echo foo > bar
> $ git add -N bar
> $ ./git --exec-path=$PWD stash save
> bar: not added yet
> fatal: git-write-tree: error building trees
> Cannot save the current index state
> 
> Maybe it would require some magic in git-stash to detect/save/restore that
> particular state, or "just" to cause "add -N" to insert an empty file
> instead ?

Yes, there needs to be some magic in git-stash to handle this. There are
actually two calls to write-tree: one to save the index and one to save
the working tree. I think the working tree one should be OK, because we
"git add -u" right beforehand, which means "intent-to-add" files will
be saved properly.

For the index case, we unfortunately cannot represent the situation in
the index using a tree, which means we cannot have a stash that doesn't
lose information. So we have to choose either dropping those index
entries, inserting them as blank files, or inserting them with
working-tree contents.

When you apply the stash, if they were:

  - dropped, then you may be surprised to find that those files are now
    untracked

  - inserted as working-tree content, then you may not realize that you
    had not _actually_ added that content to the index earlier, and just
    commit it

  - inserted as blank files, then you may be a bit surprised by the fact
    that it looks like you added a blank version, but at least you will
    still see a diff against the working tree file, alerting you to the
    fact that maybe they weren't entirely ready for commit.

So I think of the three, the last one is the least surprising. The other
option is to die and force the user to resolve the issue, which is what
we do now. It does actually tell you the problem "bar: not added yet",
though we could perhaps improve on that message a bit. I think that
would require a new 'ls-files' flag to list intent-to-add files.

-Peff

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Fri, Aug 28, 2009 at 03:05:31PM -0400, Jeff King wrote:

> For the index case, we unfortunately cannot represent the situation in
> the index using a tree, which means we cannot have a stash that doesn't
> lose information. So we have to choose either dropping those index
> entries, inserting them as blank files, or inserting them with
> working-tree contents.

Actually, you _could_ try representing the information by shoe-horning
it into the tree. For example, by allocating a bit of the mode as
"intent-to-add". That seems pretty ugly to me, though.

You could also try to stick the information in the stash's commit
message and recreate it during "stash apply". But again, that feels kind
of ugly.

-Peff

Re: [BUG] git stash refuses to save after "add -N"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   - inserted as blank files, then you may be a bit surprised by the fact
>     that it looks like you added a blank version, but at least you will
>     still see a diff against the working tree file, alerting you to the
>     fact that maybe they weren't entirely ready for commit.
>
> So I think of the three, the last one is the least surprising. The other
> option is to die and force the user to resolve the issue, which is what
> we do now. It does actually tell you the problem "bar: not added yet",
> though we could perhaps improve on that message a bit.

I am slightly in favor of leaving the things as they are, as the error
message is quite clear.

The third option would probably loo like this, which is not too bad,
though.

 builtin-commit.c       |    2 +-
 builtin-write-tree.c   |    3 +++
 cache-tree.c           |   29 ++++++++++++++---------------
 cache-tree.h           |    4 +++-
 git-stash.sh           |    2 +-
 merge-recursive.c      |    2 +-
 test-dump-cache-tree.c |    2 +-
 7 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..fc97d54 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -595,7 +595,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (!active_cache_tree)
 		active_cache_tree = cache_tree();
 	if (cache_tree_update(active_cache_tree,
-			      active_cache, active_nr, 0, 0) < 0) {
+			      active_cache, active_nr, 0) < 0) {
 		error("Error building trees");
 		return 0;
 	}
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index b223af4..6fce25e 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -23,6 +23,9 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	struct option write_tree_options[] = {
 		OPT_BIT(0, "missing-ok", &flags, "allow missing objects",
 			WRITE_TREE_MISSING_OK),
+		OPT_BIT(0, "intent-as-empty", &flags,
+			"write intent-to-add entries as empty blobs",
+			WRITE_TREE_ADD_INTENT_AS_EMPTY),
 		{ OPTION_STRING, 0, "prefix", &prefix, "<prefix>/",
 		  "write tree object for a subdirectory <prefix>" ,
 		  PARSE_OPT_LITERAL_ARGHELP },
diff --git a/cache-tree.c b/cache-tree.c
index d917437..7377066 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -148,7 +148,7 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path)
 }
 
 static int verify_cache(struct cache_entry **cache,
-			int entries)
+			int entries, int flags)
 {
 	int i, funny;
 
@@ -156,7 +156,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_ADD_INTENT_AS_EMPTY) &&
+		     (ce->ce_flags & CE_INTENT_TO_ADD))) {
 			if (10 < ++funny) {
 				fprintf(stderr, "...\n");
 				break;
@@ -237,8 +239,7 @@ 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 i;
@@ -284,8 +285,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;
@@ -328,7 +328,9 @@ static int update_one(struct cache_tree *it,
 			mode = ce->ce_mode;
 			entlen = pathlen - baselen;
 		}
-		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
+		if (mode != S_IFGITLINK &&
+		    !(flags & WRITE_TREE_MISSING_OK) &&
+		    !has_sha1_file(sha1))
 			return error("invalid object %06o %s for '%.*s'",
 				mode, sha1_to_hex(sha1), entlen+baselen, path);
 
@@ -345,7 +347,7 @@ static int update_one(struct cache_tree *it,
 #endif
 	}
 
-	if (dryrun)
+	if (flags & WRITE_TREE_DRY_RUN)
 		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
 	else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {
 		strbuf_release(&buffer);
@@ -365,14 +367,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 flags)
 {
 	int i;
-	i = verify_cache(cache, entries);
+	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;
@@ -565,11 +566,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)
+				      flags) < 0)
 			return WRITE_TREE_UNMERGED_INDEX;
 		if (0 <= newfd) {
 			if (!write_cache(newfd, active_cache, active_nr) &&
diff --git a/cache-tree.h b/cache-tree.h
index 3df641f..4ee03bb 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -29,11 +29,13 @@ 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 cache_tree_update(struct cache_tree *, struct cache_entry **, int, int);
 
 /* bitmasks to write_cache_as_tree flags */
 #define WRITE_TREE_MISSING_OK 1
 #define WRITE_TREE_IGNORE_CACHE_TREE 2
+#define WRITE_TREE_ADD_INTENT_AS_EMPTY 4
+#define WRITE_TREE_DRY_RUN 8
 
 /* error return codes */
 #define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/git-stash.sh b/git-stash.sh
index d61c9d0..735e511 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -63,7 +63,7 @@ create_stash () {
 	msg=$(printf '%s: %s' "$branch" "$head")
 
 	# state of the index
-	i_tree=$(git write-tree) &&
+	i_tree=$(git write-tree --intent-as-empty) &&
 	i_commit=$(printf 'index on %s\n' "$msg" |
 		git commit-tree $i_tree -p $b_commit) ||
 		die "Cannot save the current index state"
diff --git a/merge-recursive.c b/merge-recursive.c
index 10d7913..dea5400 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -211,7 +211,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)
+			      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 1f73f1e..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);
+	cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN);
 	return dump_cache_tree(active_cache_tree, another, "");
 }

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Sat, Aug 29, 2009 at 03:34:45PM -0700, Junio C Hamano wrote:

> I am slightly in favor of leaving the things as they are, as the error
> message is quite clear.

Hmm. Thinking about it a bit more, I think "add as empty content" is
probably the best. It scares me a little because it is losing
information during the stash, but consider it from the user's
perspective.

Their work-in-progress is being interrupted, so they need to stash. They
try "git stash" and the current version comes back with an error. Now
what? If they know what to do, they can manually "git rm --cached" each
of the offending files (and I say manually because there isn't a
parseable list of them anywhere). But they probably don't know what to
do, which means trying to find the information in the documentation.

And all of this while they are trying to quickly switch contexts to
whatever it was that caused them to stash in the first place. So I
expect the most useful thing would be a "git stash -f" that adds them as
empty. And it's reasonably safe, because we're not losing information in
the transition from index to stash tree without the user first having
been notified.

On the other hand, it may be sufficient to just do the transformation
with a "-f", which will save users even more time, and we can put a note
in the documentation about how stash interacts with -N. I don't know
whether people will actually care or not (and your patch already does
the unconditional form, so it's less work :) ).

-Peff

Re: [BUG] git stash refuses to save after "add -N"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Aug 29, 2009 at 03:34:45PM -0700, Junio C Hamano wrote:
>
>> I am slightly in favor of leaving the things as they are, as the error
>> message is quite clear.
>
> Hmm. Thinking about it a bit more, I think "add as empty content" is
> probably the best. It scares me a little because it is losing
> information during the stash, but consider it from the user's
> perspective.
> ...
> And all of this while they are trying to quickly switch contexts to
> whatever it was that caused them to stash in the first place.

Ok, then probably the "how about" patch would be a part of the right
solution.

One thing I noticed was that while unstashing without --index, we add full
contents to the index of new files.  I think it is because back then when
stash was written there was no other way, but now we have intent-to-add
and a way to stash such an entry, I think we should add only the intent to
add them in that codepath.

Of course we will not do this when unstashing with --index.

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Sun, Aug 30, 2009 at 01:01:11PM -0700, Junio C Hamano wrote:

> > And all of this while they are trying to quickly switch contexts to
> > whatever it was that caused them to stash in the first place.
> 
> Ok, then probably the "how about" patch would be a part of the right
> solution.

And then something like this on top (assuming you cut the change to
git-stash.sh from yours).

My concerns are:

  - "-f" is kind of vague. Would people expect it to force aspects of
    the stash? Should it be "--intent-as-empty"?

  - the error message is still a bit muddled, because you get the "not
    yet added" files _first_, then some failure cruft from write-tree,
    and _then_ the trying-to-be-helpful message

I dunno. Honestly I am a bit lukewarm about this whole thing, as it
seems like something that just wouldn't come up that often, and while
the current error message is a bit disorganized, I think a user who has
used "git add -N" can figure out that it is related (the only report we
have is from Yann, who _did_ figure it out, but wanted to know how to
make git handle the situation better).

diff --git a/git-stash.sh b/git-stash.sh
index d61c9d0..963cad0 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -20,6 +20,7 @@ TMP="$GIT_DIR/.git-stash.$$"
 trap 'rm -f "$TMP-*"' 0
 
 ref_stash=refs/stash
+force=
 
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
@@ -63,7 +64,14 @@ create_stash () {
 	msg=$(printf '%s: %s' "$branch" "$head")
 
 	# state of the index
-	i_tree=$(git write-tree) &&
+	if ! i_tree=$(git write-tree ${force:+--intent-as-empty}); then
+		case "$force" in
+		t) die 'Cannot save the current index state';;
+		*) echo >&2 'fatal: unable to create tree; if some files are marked as'
+		   echo >&2 '"not added yet", you may override with "git stash save -f"'
+		   exit 1
+		esac
+	fi
 	i_commit=$(printf 'index on %s\n' "$msg" |
 		git commit-tree $i_tree -p $b_commit) ||
 		die "Cannot save the current index state"
@@ -104,6 +112,9 @@ save_stash () {
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
+		-f|--force)
+			force=t
+			;;
 		*)
 			break
 			;;
diff --git a/t/t3904-stash-intent.sh b/t/t3904-stash-intent.sh
new file mode 100755
index 0000000..ec7dd12
--- /dev/null
+++ b/t/t3904-stash-intent.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='stash with intent-to-add index entries'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo content >base &&
+	git add base &&
+	git commit -m base &&
+	echo foo content >foo &&
+	echo bar content >bar &&
+	git add foo &&
+	git add -N bar
+'
+
+test_expect_success 'stash save refuses intent-to-add entry' '
+	test_must_fail git stash save
+'
+
+test_expect_success 'stash save -f allows intent-to-add' '
+	git stash save -f &&
+	git show stash^2:foo >foo.stash &&
+	echo foo content >expect &&
+	test_cmp expect foo.stash &&
+	>expect &&
+	git show stash^2:bar >bar.stash &&
+	test_cmp expect bar.stash
+'
+
+test_done

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Sun, Aug 30, 2009 at 01:01:11PM -0700, Junio C Hamano wrote:

> One thing I noticed was that while unstashing without --index, we add full
> contents to the index of new files.  I think it is because back then when
> stash was written there was no other way, but now we have intent-to-add
> and a way to stash such an entry, I think we should add only the intent to
> add them in that codepath.
> 
> Of course we will not do this when unstashing with --index.

And btw, I think your suggestion is reasonable, though I don't feel
strongly either way. I don't know that the current behavior is really
bothering anybody.

-Peff

Re: [BUG] git stash refuses to save after "add -N"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My concerns are:
>
>   - "-f" is kind of vague. Would people expect it to force aspects of
>     the stash? Should it be "--intent-as-empty"?
>
>   - the error message is still a bit muddled, because you get the "not
>     yet added" files _first_, then some failure cruft from write-tree,
>     and _then_ the trying-to-be-helpful message
>
> I dunno. Honestly I am a bit lukewarm about this whole thing, as it
> seems like something that just wouldn't come up that often, and while
> the current error message is a bit disorganized, I think a user who has
> used "git add -N" can figure out that it is related (the only report we
> have is from Yann, who _did_ figure it out, but wanted to know how to
> make git handle the situation better).

I am not sure if asking for positive confirmation with "-f" is even worth
it.  As you pointed out in your earlier message, which prompted me to
respond with a patch, when this codepath is exercised, the user is in a
rush, and I do not see what else the user would want to do other than
including it in the stash by rerunning with -f.

Re: [BUG] git stash refuses to save after "add -N"

[Jeff King]

On Sun, Aug 30, 2009 at 10:03:07PM -0700, Junio C Hamano wrote:

> >   - "-f" is kind of vague. Would people expect it to force aspects of
> >     the stash? Should it be "--intent-as-empty"?
> 
> I am not sure if asking for positive confirmation with "-f" is even worth
> it.  As you pointed out in your earlier message, which prompted me to
> respond with a patch, when this codepath is exercised, the user is in a
> rush, and I do not see what else the user would want to do other than
> including it in the stash by rerunning with -f.

I guess it was just to mitigate my fear that we are somehow creating a
stash that will confuse people when they apply it. But really that fear
is probably unjustified.

-Peff

Re: [BUG] git stash refuses to save after "add -N"

[Yann Dirson]

> On Sun, Aug 30, 2009 at 10:03:07PM -0700, Junio C Hamano wrote:
>
>> >   - "-f" is kind of vague. Would people expect it to force aspects of
>> >     the stash? Should it be "--intent-as-empty"?
>>
>> I am not sure if asking for positive confirmation with "-f" is even
>> worth
>> it.  As you pointed out in your earlier message, which prompted me to
>> respond with a patch, when this codepath is exercised, the user is in a
>> rush, and I do not see what else the user would want to do other than
>> including it in the stash by rerunning with -f.
>
> I guess it was just to mitigate my fear that we are somehow creating a
> stash that will confuse people when they apply it. But really that fear
> is probably unjustified.

Well, indeed each time I use "add -N" it is mostly to get "git diff" show
me the contents of the new file as part of my WIP, so it would not make
much difference to me if "add -N" would just add the file as empty to the
index in the first place.

Out of curiosity, does anyone make any use of the current difference
between not-added-yet and added-as-empty ?