stash-p broken?

stash-p broken?

[Michael Migdol]

I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,
"git stash -p" stopped working.  (It apparently is ignoring the -p
parameter).  I'm not sure what version I was using previously, but after
some experimentation, I see that:

version 1.7.12.2 : stash -p DOES work
version 1.9.3 : doesn't work
version 2.0.3 : doesn't work

Osse on the IRC channel was helping out with this and saw some very odd
behavior while trying to bisect where the regression occurred;  he said
he'd comment further tomorrow.  But I thought I would send a note to the
list for archival...

Cheers,
Michael

Re: stash-p broken?

[Michael Migdol]

Sorry for error -- I meant: git stash list -p, not git stash -p.

On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:
> I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,
> "git stash -p" stopped working.  (It apparently is ignoring the -p
> parameter).  I'm not sure what version I was using previously, but after
> some experimentation, I see that:
>
> version 1.7.12.2 : stash -p DOES work
> version 1.9.3 : doesn't work
> version 2.0.3 : doesn't work
>
> Osse on the IRC channel was helping out with this and saw some very odd
> behavior while trying to bisect where the regression occurred;  he said
> he'd comment further tomorrow.  But I thought I would send a note to the
> list for archival...
>
> Cheers,
> Michael

Re: stash-p broken?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:
> Sorry for error -- I meant: git stash list -p, not git stash -p.
> 
> On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:
> > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,
> > "git stash -p" stopped working.  (It apparently is ignoring the -p
> > parameter).  I'm not sure what version I was using previously, but after
> > some experimentation, I see that:
> >
> > version 1.7.12.2 : stash -p DOES work
> > version 1.9.3 : doesn't work
> > version 2.0.3 : doesn't work

Under the hood, we do:

  git log --format="%gd: %gs" -g "$@" refs/stash --

But it looks like git log ignores -p if -g is provided.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: stash-p broken?

[Øystein Walle]

brian m. carlson <sandals <at> crustytoothpaste.net> writes:

> 
> On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:
> > Sorry for error -- I meant: git stash list -p, not git stash -p.
> > 
> > On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam <at>
migdol.net> wrote:
> > > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,
> > > "git stash -p" stopped working.  (It apparently is ignoring the -p
> > > parameter).  I'm not sure what version I was using previously, but after
> > > some experimentation, I see that:
> > >
> > > version 1.7.12.2 : stash -p DOES work
> > > version 1.9.3 : doesn't work
> > > version 2.0.3 : doesn't work
> 
> Under the hood, we do:
> 
>   git log --format="%gd: %gs" -g "$ <at> " refs/stash --
> 
> But it looks like git log ignores -p if -g is provided.
> 

Unfortunately I don't have much to add...

I have a weird issue where `git stash list -p` does not show a diff, but
`~/usr/bin/git stash list -p` does, for the exact same version of Git.
However, the underlying command does not show a diff in either case,
even though I can tell that is the one being executed (with set -x
inserted into .../libexec/git-stash).

This is so weird that I think it's my system's fault. I have to do some
more digging.

Øsse

Re: stash-p broken?

[Jeff King]

On Tue, Jul 29, 2014 at 02:59:40AM +0000, brian m. carlson wrote:

> On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:
> > Sorry for error -- I meant: git stash list -p, not git stash -p.
> > 
> > On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:
> > > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,
> > > "git stash -p" stopped working.  (It apparently is ignoring the -p
> > > parameter).  I'm not sure what version I was using previously, but after
> > > some experimentation, I see that:
> > >
> > > version 1.7.12.2 : stash -p DOES work
> > > version 1.9.3 : doesn't work
> > > version 2.0.3 : doesn't work
> 
> Under the hood, we do:
> 
>   git log --format="%gd: %gs" -g "$@" refs/stash --
> 
> But it looks like git log ignores -p if -g is provided.

Almost. It's not ignored, but rather it is a noop on merge commits
without "-c" or "--cc". And every stash is a merge commit (between the
base commit and the index content).

So why did it work in v1.7.12.2? It didn't. It just looked like it
worked. :)

When we do a reflog walk, we replace the parents of each commit with the
prior reflog entry, simulating a linear string of development, moving
from one reflog entry to another.  Prior to 838f9a1 (log: use true
parents for diff when walking reflogs, 2013-08-03), we used those "fake"
parents when diffing, too. But that doesn't make much sense. It will
show the diff between two adjacent reflog entries, which may not even be
related (especially for stashes!). What the user is generally after is
to walk the reflog, but to show each commit in isolation as a diff
against its parents.

So v1.7.12.2 showed _a_ diff, but it was the diff against the prior
stash, which was generally nonsensical.  After 838f9a1, we actually show
the diff against the real parents, but you need "-p --cc" to see
anything, because each stash is a merge.

Unfortunately, the resulting diff is often a bit uglier than necessary.
Here's an example:

    git init
    echo one >file && git add file && git commit -m one
    echo two >>file
    git stash
    git stash list -p --cc

yields:

    stash@{0}: WIP on master: d0e237c one
    
    diff --cc file
    index 5626abf,5626abf..814f4a4
    --- a/file
    +++ b/file
    @@@ -1,1 -1,1 +1,2 @@@
      one
    ++two

It's a combined-diff, because we are showing the changes from both the
original base commit and the index. But the index was the same as the
base, so each changed line gets two "++" markers (and two "--", and two
spaces for context lines). And in many cases, I'd guess the index is
untouched (in fact, "git stash show" explicitly shows _only_ the diff
between the working tree and the base!).

When we create the stash, we could notice that the trees for the index
and base are identical, and just skip making the index commit (and
therefore make the stash itself a regular commit rather than a merge).
That might break later users of the stash, though, who would want to
check "stash@{0}" against "stash@{0}^2" to see what was in the index
(they could infer from the lack of a second parent that it was empty,
but that is not backwards compatible).

So perhaps we could do something better on the viewing side, and
"simplify" combined merges for files with all-identical parents. With
the patch below, these commands (on top of the earlier ones):

   echo two >>file && git add file
   echo three >>file
   git stash
   git stash list -p --cc --simplify-combined-diff

yields:

    stash@{0}: WIP on master: d0e237c one
    
    diff --cc file
    index 5626abf,814f4a4..4cb29ea
    --- a/file
    +++ b/file
    @@@ -1,1 -1,2 +1,3 @@@
      one
    + two
    ++three
    stash@{1}: WIP on master: d0e237c one
    
    diff --git file
    index 5626abf..814f4a4
    --- a/file
    +++ b/file
    @@ -1,1 +1,2 @@
     one
    +two


I.e., we show the combined diff when it is relevant (in the first stash,
where the index actually had something interesting), and a normal diff
otherwise. And then we could make "--cc --simplify-combined-diff" the default
for "stash list" (which would only trigger if you provided "--raw" or
"-p" yourself).

The patch for --simplify-combined-diff is below. Note that it either
simplifies to a single parent, or does not simplification at all. In
theory we could simplify an octopus merge of "A,B,A" into "A,B", but I
think that just ends up being more confusing (and ambiguous; you don't
know which parent was dropped).

-Peff

---
diff --git a/combine-diff.c b/combine-diff.c
index 6c00c4b..6a27436 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -900,7 +900,11 @@ static void show_combined_header(struct combine_diff_path *elem,
 	if (rev->loginfo && !rev->no_commit_id)
 		show_log(rev);
 
-	dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
+	dump_quoted_path(num_parent == 1 ?
+			   "diff --git " :
+			   dense ?
+			     "diff --cc " :
+			     "diff --combined ",
 			 "", elem->path, line_prefix, c_meta, c_reset);
 	printf("%s%sindex ", line_prefix, c_meta);
 	for (i = 0; i < num_parent; i++) {
@@ -955,6 +959,15 @@ static void show_combined_header(struct combine_diff_path *elem,
 				 line_prefix, c_meta, c_reset);
 }
 
+static int simplify_parents(struct combine_diff_path *p, int nr)
+{
+	int i;
+	for (i = 1; i < nr; i++)
+		if (hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))
+			return nr;
+	return 1;
+}
+
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			    int dense, int working_tree_file,
 			    struct rev_info *rev)
@@ -979,6 +992,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))
 		textconv = userdiff_get_textconv(userdiff);
 
+	if (rev->simplify_combined_diffs)
+		num_parent = simplify_parents(elem, num_parent);
+
 	/* Read the result of merge first */
 	if (!working_tree_file)
 		result = grab_blob(elem->sha1, elem->mode, &result_size,
@@ -1181,6 +1197,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 	if (rev->loginfo && !rev->no_commit_id)
 		show_log(rev);
 
+	if (rev->simplify_combined_diffs)
+		num_parent = simplify_parents(p, num_parent);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
 		printf("%s", line_prefix);
@@ -1247,12 +1265,16 @@ static void free_combined_pair(struct diff_filepair *pair)
  * but currently nobody uses it, so this should suffice for now.
  */
 static struct diff_filepair *combined_pair(struct combine_diff_path *p,
-					   int num_parent)
+					   int num_parent,
+					   struct rev_info *rev)
 {
 	int i;
 	struct diff_filepair *pair;
 	struct diff_filespec *pool;
 
+	if (rev->simplify_combined_diffs)
+		num_parent = simplify_parents(p, num_parent);
+
 	pair = xcalloc(1, sizeof(*pair));
 	pool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));
 	pair->one = pool + 1;
@@ -1277,7 +1299,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,
 static void handle_combined_callback(struct diff_options *opt,
 				     struct combine_diff_path *paths,
 				     int num_parent,
-				     int num_paths)
+				     int num_paths,
+				     struct rev_info *rev)
 {
 	struct combine_diff_path *p;
 	struct diff_queue_struct q;
@@ -1287,7 +1310,8 @@ static void handle_combined_callback(struct diff_options *opt,
 	q.alloc = num_paths;
 	q.nr = num_paths;
 	for (i = 0, p = paths; p; p = p->next)
-		q.queue[i++] = combined_pair(p, num_parent);
+		q.queue[i++] = combined_pair(p, num_parent, rev);
+
 	opt->format_callback(&q, opt, opt->format_callback_data);
 	for (i = 0; i < num_paths; i++)
 		free_combined_pair(q.queue[i]);
@@ -1498,7 +1522,8 @@ void diff_tree_combined(const unsigned char *sha1,
 			 (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))
 			needsep = 1;
 		else if (opt->output_format & DIFF_FORMAT_CALLBACK)
-			handle_combined_callback(opt, paths, num_parent, num_paths);
+			handle_combined_callback(opt, paths, num_parent,
+						 num_paths, rev);
 
 		if (opt->output_format & DIFF_FORMAT_PATCH) {
 			if (needsep)
diff --git a/revision.c b/revision.c
index b8b0ea3..27eba77 100644
--- a/revision.c
+++ b/revision.c
@@ -1820,6 +1820,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->diff = 1;
 		revs->dense_combined_merges = 1;
 		revs->combine_merges = 1;
+	} else if (!strcmp(arg, "--simplify-combined-diffs")) {
+		revs->simplify_combined_diffs = 1;
 	} else if (!strcmp(arg, "-v")) {
 		revs->verbose_header = 1;
 	} else if (!strcmp(arg, "--pretty")) {
diff --git a/revision.h b/revision.h
index a620530..29689a1 100644
--- a/revision.h
+++ b/revision.h
@@ -122,6 +122,7 @@ struct rev_info {
 			ignore_merges:1,
 			combine_merges:1,
 			dense_combined_merges:1,
+			simplify_combined_diffs:1,
 			always_show_header:1;
 
 	/* Format info */

Re: stash-p broken?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So perhaps we could do something better on the viewing side, and
> "simplify" combined merges for files with all-identical parents.

Or you can ask to show the difference with the first parent, no?

I do not think giving a single-parent diff when --cc/-c is asked
for, which is a clear indication that the caller is aware that the
commit in question is a merge, is such a good idea.

Re: stash-p broken?

[Jeff King]

On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So perhaps we could do something better on the viewing side, and
> > "simplify" combined merges for files with all-identical parents.
> 
> Or you can ask to show the difference with the first parent, no?

Yeah, though there is no way to do the other thing (--second-parent to
show only the index side). I doubt anybody wants that, though. Just
passing --first-parent would probably be fine (that would match "git
stash show", too, though like that "stash show" it is impossible to
override to see the index portion then).

But...

> I do not think giving a single-parent diff when --cc/-c is asked
> for, which is a clear indication that the caller is aware that the
> commit in question is a merge, is such a good idea.

I think that is my point, though. The user is _not_ aware that the
commit in question is a merge. They stashed some stuff, and now they
want to see the result. I'd like to show them a vanilla commit if the
index is irrelevant, and otherwise show them something more interesting.

I definitely don't think that should be the default behavior, but I
don't see a problem with it being part of stash. We pick apart the stash
already in git-stash.sh, so for "git stash show" we can do it ourselves.
But during a traversal, we need support from "git log", since the answer
is different for each stash (and even for each path).

-Peff

Re: stash-p broken?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > So perhaps we could do something better on the viewing side, and
>> > "simplify" combined merges for files with all-identical parents.
>> 
>> Or you can ask to show the difference with the first parent, no?
>
> Yeah, though there is no way to do the other thing (--second-parent to
> show only the index side). I doubt anybody wants that, though. Just
> passing --first-parent would probably be fine (that would match "git
> stash show", too, though like that "stash show" it is impossible to
> override to see the index portion then).
>
> But...
>
>> I do not think giving a single-parent diff when --cc/-c is asked
>> for, which is a clear indication that the caller is aware that the
>> commit in question is a merge, is such a good idea.
>
> I think that is my point, though. The user is _not_ aware that the
> commit in question is a merge.

Oh, don't get me wrong.  I am not opposed to give better output from
"stash show -p".  I do not want to see us change what "git log --cc" 
does without being asked to do something different.

I see you added --simplify-combined-diffs to avoid breaking "log",
so that is not too bad, but I am still unsure what should happen
when the first parent and the result is the same and only the second
parent is different (i.e. you have changes in the index and your
working tree changes have been reverted---then you create a stash).
Should it show as "single source diff, with all changes reverted"?
Should it show as a normal "combined diff for a merge"?  Should it
show as "no changes whatsoever"?

Re: stash-p broken?

[Jeff King]

On Tue, Jul 29, 2014 at 11:23:16AM -0700, Junio C Hamano wrote:

> I see you added --simplify-combined-diffs to avoid breaking "log",
> so that is not too bad, but I am still unsure what should happen
> when the first parent and the result is the same and only the second
> parent is different (i.e. you have changes in the index and your
> working tree changes have been reverted---then you create a stash).
> Should it show as "single source diff, with all changes reverted"?
> Should it show as a normal "combined diff for a merge"?  Should it
> show as "no changes whatsoever"?

Interesting question. I think right now we would show nothing in the
combined diff, as we simply took one side. This is really showing the
weakness of the "stash is a merge" storage format. If we use it as a
storage format, and pick it apart in "git stash", that's fine. But as
soon as we start treating it like a real merge and expecting "git log"
to behave, the cracks begin to show.

Hmph. To be honest, I am starting to wonder if implying "--first-parent"
is a more sensible option for "stash list". It matches "stash show", at
least, and it is not unreasonable to simply present the changes in the
working tree by default, and ignore the index. People who are more
clueful can pick apart the commits using "git log" themselves.

-Peff

Re: stash-p broken?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmph. To be honest, I am starting to wonder if implying "--first-parent"
> is a more sensible option for "stash list". It matches "stash show", at
> least, and it is not unreasonable to simply present the changes in the
> working tree by default, and ignore the index. People who are more
> clueful can pick apart the commits using "git log" themselves.

Yup, I tend to agree.

After thinking about this a lot, I really wish that we did not have
"stash apply" and "stash pop", but just "stash save" and a single
"stash restore" that either checks out the original branch (if its
head have not moved) or detaches the HEAD at the original commit
(otherwise), and then restores the state of the index and the
working tree (with or without untracked) and nothing else, but the
details are too long to fit in this topic ;-)

Re: stash-p broken?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think that is my point, though. The user is _not_ aware that the
> commit in question is a merge. They stashed some stuff, and now they
> want to see the result. I'd like to show them a vanilla commit if the
> index is irrelevant, and otherwise show them something more interesting.

I actually think "git stash list" users should be made very aware
that they are looking at merge commits, and also what two states
each of these merge commits represents.  "git stash --help" says
that "list" takes any random option that you can give to "git log",
and at that point you need to be very well aware what you are
telling "log" to show and how.

It probably *was* a UI mistake to make "list" just delegate to "log"
and it was a graver UI mistake that it was documented as such;
otherwise we could have limited option set "git stash list" would
take, which can be totally independent from what "git log" takes,
and we could just have said "git stash list -p" is a way to view
each entry with patch in a reasonable representation.  But that
ideal world no longer exists X-<.

Re: stash-p broken?

[Jeff King]

On Tue, Jul 29, 2014 at 12:27:07PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think that is my point, though. The user is _not_ aware that the
> > commit in question is a merge. They stashed some stuff, and now they
> > want to see the result. I'd like to show them a vanilla commit if the
> > index is irrelevant, and otherwise show them something more interesting.
> 
> I actually think "git stash list" users should be made very aware
> that they are looking at merge commits, and also what two states
> each of these merge commits represents.

Yeah, I kind of agree, but I also am not optimistic about most users
understanding that. I was trying to gamble on the fact that:

  1. Naive users who do not understand the index will not stage files
     and then stash in the first place.  To them, the stash would be a
     simple diff between two states, the commit and the working tree.

  2. Advanced users would see the more complicated diff, but only
     because they were doing more interesting things with the index.
     They know what the index is, and therefore know that stash must be
     saving it somehow.

Of course that breaks down when the naive user somehow ends up with
staged content in the index (e.g., they are resolving a merge and try to
stash). Then they are thrust into the more complicated situation either
way.

I dunno. I suspect that gamble would work _most_ of the time, and makes
an OK default. On the other hand, "git stash list" was completely
useless for showing diffs for many years, and since becoming useful has
required "--cc" to show anything good. And this is the first complaint
we've seen on the list. Maybe people don't actually care, and educating
people to use "--cc -p" (or "--first-parent -p") is fine.

-Peff