Re: stash-p broken?
Jeff King p...@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 ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
On Tue, Jul 29, 2014 at 12:27:07PM -0700, Junio C Hamano wrote: Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
On Tue, Jul 29, 2014 at 02:59:40AM +, 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-s...@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,
Re: stash-p broken?
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote: Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
Jeff King p...@peff.net writes: On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote: Jeff King p...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
Jeff King p...@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-. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
stash-p broken?
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
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-s...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stash-p broken?
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-s...@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 signature.asc Description: Digital signature
