{"thread":{"id":"37236","subject":"stash-p broken?","startedAt":"2014-07-29T00:38:10Z","lastAt":"2014-07-31T16:49:41Z","messageCount":12,"participants":["Michael Migdol","brian m. carlson","Øystein Walle","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"246866","messageId":"CAJWZfo6j2E2qu1n6JTmtPw109tAjnEfWBvKxwGfTrKSF6MZ2Uw@mail.gmail.com","threadId":"37236","inReplyTo":null,"subject":"stash-p broken?","fromName":"Michael Migdol","fromEmail":"michael-spam@migdol.net","sentAt":"2014-07-29T00:38:10Z","receivedAt":"2014-07-29T00:38:10Z","isPatch":false,"sender":{"key":"michael-spam@migdol.net","avatar":null},"body":"I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,\n\"git stash -p\" stopped working.  (It apparently is ignoring the -p\nparameter).  I'm not sure what version I was using previously, but after\nsome experimentation, I see that:\n\nversion 1.7.12.2 : stash -p DOES work\nversion 1.9.3 : doesn't work\nversion 2.0.3 : doesn't work\n\nOsse on the IRC channel was helping out with this and saw some very odd\nbehavior while trying to bisect where the regression occurred;  he said\nhe'd comment further tomorrow.  But I thought I would send a note to the\nlist for archival...\n\nCheers,\nMichael\n"},{"id":"246867","messageId":"CAJWZfo741u8KqZax4ReTEg8hy1BKWR2_PofxAQtE4wU=ikRsPw@mail.gmail.com","threadId":"37236","inReplyTo":"CAJWZfo6j2E2qu1n6JTmtPw109tAjnEfWBvKxwGfTrKSF6MZ2Uw@mail.gmail.com","subject":"Re: stash-p broken?","fromName":"Michael Migdol","fromEmail":"michael-spam@migdol.net","sentAt":"2014-07-29T00:56:28Z","receivedAt":"2014-07-29T00:56:28Z","isPatch":false,"sender":{"key":"michael-spam@migdol.net","avatar":null},"body":"Sorry for error -- I meant: git stash list -p, not git stash -p.\n\nOn Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:\n> I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,\n> \"git stash -p\" stopped working.  (It apparently is ignoring the -p\n> parameter).  I'm not sure what version I was using previously, but after\n> some experimentation, I see that:\n>\n> version 1.7.12.2 : stash -p DOES work\n> version 1.9.3 : doesn't work\n> version 2.0.3 : doesn't work\n>\n> Osse on the IRC channel was helping out with this and saw some very odd\n> behavior while trying to bisect where the regression occurred;  he said\n> he'd comment further tomorrow.  But I thought I would send a note to the\n> list for archival...\n>\n> Cheers,\n> Michael\n"},{"id":"246869","messageId":"20140729025940.GA8640@vauxhall.crustytoothpaste.net","threadId":"37236","inReplyTo":"CAJWZfo741u8KqZax4ReTEg8hy1BKWR2_PofxAQtE4wU=ikRsPw@mail.gmail.com","subject":"Re: stash-p broken?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-07-29T02:59:40Z","receivedAt":"2014-07-29T02:59:40Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:\n> Sorry for error -- I meant: git stash list -p, not git stash -p.\n> \n> On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:\n> > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,\n> > \"git stash -p\" stopped working.  (It apparently is ignoring the -p\n> > parameter).  I'm not sure what version I was using previously, but after\n> > some experimentation, I see that:\n> >\n> > version 1.7.12.2 : stash -p DOES work\n> > version 1.9.3 : doesn't work\n> > version 2.0.3 : doesn't work\n\nUnder the hood, we do:\n\n  git log --format=\"%gd: %gs\" -g \"$@\" refs/stash --\n\nBut it looks like git log ignores -p if -g is provided.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"246874","messageId":"loom.20140729T084120-450@post.gmane.org","threadId":"37236","inReplyTo":"20140729025940.GA8640@vauxhall.crustytoothpaste.net","subject":"Re: stash-p broken?","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-07-29T06:45:24Z","receivedAt":"2014-07-29T06:45:24Z","isPatch":false,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"brian m. carlson <sandals <at> crustytoothpaste.net> writes:\n\n> \n> On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:\n> > Sorry for error -- I meant: git stash list -p, not git stash -p.\n> > \n> > On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam <at>\nmigdol.net> wrote:\n> > > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,\n> > > \"git stash -p\" stopped working.  (It apparently is ignoring the -p\n> > > parameter).  I'm not sure what version I was using previously, but after\n> > > some experimentation, I see that:\n> > >\n> > > version 1.7.12.2 : stash -p DOES work\n> > > version 1.9.3 : doesn't work\n> > > version 2.0.3 : doesn't work\n> \n> Under the hood, we do:\n> \n>   git log --format=\"%gd: %gs\" -g \"$ <at> \" refs/stash --\n> \n> But it looks like git log ignores -p if -g is provided.\n> \n\nUnfortunately I don't have much to add...\n\nI have a weird issue where `git stash list -p` does not show a diff, but\n`~/usr/bin/git stash list -p` does, for the exact same version of Git.\nHowever, the underlying command does not show a diff in either case,\neven though I can tell that is the one being executed (with set -x\ninserted into .../libexec/git-stash).\n\nThis is so weird that I think it's my system's fault. I have to do some\nmore digging.\n\nØsse\n"},{"id":"246884","messageId":"20140729092301.GA13134@peff.net","threadId":"37236","inReplyTo":"20140729025940.GA8640@vauxhall.crustytoothpaste.net","subject":"Re: stash-p broken?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T09:23:02Z","receivedAt":"2014-07-29T09:23:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 02:59:40AM +0000, brian m. carlson wrote:\n\n> On Mon, Jul 28, 2014 at 05:56:28PM -0700, Michael Migdol wrote:\n> > Sorry for error -- I meant: git stash list -p, not git stash -p.\n> > \n> > On Mon, Jul 28, 2014 at 5:38 PM, Michael Migdol <michael-spam@migdol.net> wrote:\n> > > I recently upgraded from Ubuntu 13.10 to Ubuntu 14.04.  After doing so,\n> > > \"git stash -p\" stopped working.  (It apparently is ignoring the -p\n> > > parameter).  I'm not sure what version I was using previously, but after\n> > > some experimentation, I see that:\n> > >\n> > > version 1.7.12.2 : stash -p DOES work\n> > > version 1.9.3 : doesn't work\n> > > version 2.0.3 : doesn't work\n> \n> Under the hood, we do:\n> \n>   git log --format=\"%gd: %gs\" -g \"$@\" refs/stash --\n> \n> But it looks like git log ignores -p if -g is provided.\n\nAlmost. It's not ignored, but rather it is a noop on merge commits\nwithout \"-c\" or \"--cc\". And every stash is a merge commit (between the\nbase commit and the index content).\n\nSo why did it work in v1.7.12.2? It didn't. It just looked like it\nworked. :)\n\nWhen we do a reflog walk, we replace the parents of each commit with the\nprior reflog entry, simulating a linear string of development, moving\nfrom one reflog entry to another.  Prior to 838f9a1 (log: use true\nparents for diff when walking reflogs, 2013-08-03), we used those \"fake\"\nparents when diffing, too. But that doesn't make much sense. It will\nshow the diff between two adjacent reflog entries, which may not even be\nrelated (especially for stashes!). What the user is generally after is\nto walk the reflog, but to show each commit in isolation as a diff\nagainst its parents.\n\nSo v1.7.12.2 showed _a_ diff, but it was the diff against the prior\nstash, which was generally nonsensical.  After 838f9a1, we actually show\nthe diff against the real parents, but you need \"-p --cc\" to see\nanything, because each stash is a merge.\n\nUnfortunately, the resulting diff is often a bit uglier than necessary.\nHere's an example:\n\n    git init\n    echo one >file && git add file && git commit -m one\n    echo two >>file\n    git stash\n    git stash list -p --cc\n\nyields:\n\n    stash@{0}: WIP on master: d0e237c one\n    \n    diff --cc file\n    index 5626abf,5626abf..814f4a4\n    --- a/file\n    +++ b/file\n    @@@ -1,1 -1,1 +1,2 @@@\n      one\n    ++two\n\nIt's a combined-diff, because we are showing the changes from both the\noriginal base commit and the index. But the index was the same as the\nbase, so each changed line gets two \"++\" markers (and two \"--\", and two\nspaces for context lines). And in many cases, I'd guess the index is\nuntouched (in fact, \"git stash show\" explicitly shows _only_ the diff\nbetween the working tree and the base!).\n\nWhen we create the stash, we could notice that the trees for the index\nand base are identical, and just skip making the index commit (and\ntherefore make the stash itself a regular commit rather than a merge).\nThat might break later users of the stash, though, who would want to\ncheck \"stash@{0}\" against \"stash@{0}^2\" to see what was in the index\n(they could infer from the lack of a second parent that it was empty,\nbut that is not backwards compatible).\n\nSo perhaps we could do something better on the viewing side, and\n\"simplify\" combined merges for files with all-identical parents. With\nthe patch below, these commands (on top of the earlier ones):\n\n   echo two >>file && git add file\n   echo three >>file\n   git stash\n   git stash list -p --cc --simplify-combined-diff\n\nyields:\n\n    stash@{0}: WIP on master: d0e237c one\n    \n    diff --cc file\n    index 5626abf,814f4a4..4cb29ea\n    --- a/file\n    +++ b/file\n    @@@ -1,1 -1,2 +1,3 @@@\n      one\n    + two\n    ++three\n    stash@{1}: WIP on master: d0e237c one\n    \n    diff --git file\n    index 5626abf..814f4a4\n    --- a/file\n    +++ b/file\n    @@ -1,1 +1,2 @@\n     one\n    +two\n\n\nI.e., we show the combined diff when it is relevant (in the first stash,\nwhere the index actually had something interesting), and a normal diff\notherwise. And then we could make \"--cc --simplify-combined-diff\" the default\nfor \"stash list\" (which would only trigger if you provided \"--raw\" or\n\"-p\" yourself).\n\nThe patch for --simplify-combined-diff is below. Note that it either\nsimplifies to a single parent, or does not simplification at all. In\ntheory we could simplify an octopus merge of \"A,B,A\" into \"A,B\", but I\nthink that just ends up being more confusing (and ambiguous; you don't\nknow which parent was dropped).\n\n-Peff\n\n---\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 6c00c4b..6a27436 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -900,7 +900,11 @@ static void show_combined_header(struct combine_diff_path *elem,\n \tif (rev->loginfo && !rev->no_commit_id)\n \t\tshow_log(rev);\n \n-\tdump_quoted_path(dense ? \"diff --cc \" : \"diff --combined \",\n+\tdump_quoted_path(num_parent == 1 ?\n+\t\t\t   \"diff --git \" :\n+\t\t\t   dense ?\n+\t\t\t     \"diff --cc \" :\n+\t\t\t     \"diff --combined \",\n \t\t\t \"\", elem->path, line_prefix, c_meta, c_reset);\n \tprintf(\"%s%sindex \", line_prefix, c_meta);\n \tfor (i = 0; i < num_parent; i++) {\n@@ -955,6 +959,15 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t line_prefix, c_meta, c_reset);\n }\n \n+static int simplify_parents(struct combine_diff_path *p, int nr)\n+{\n+\tint i;\n+\tfor (i = 1; i < nr; i++)\n+\t\tif (hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))\n+\t\t\treturn nr;\n+\treturn 1;\n+}\n+\n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t    int dense, int working_tree_file,\n \t\t\t    struct rev_info *rev)\n@@ -979,6 +992,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))\n \t\ttextconv = userdiff_get_textconv(userdiff);\n \n+\tif (rev->simplify_combined_diffs)\n+\t\tnum_parent = simplify_parents(elem, num_parent);\n+\n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n \t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\n@@ -1181,6 +1197,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \tif (rev->loginfo && !rev->no_commit_id)\n \t\tshow_log(rev);\n \n+\tif (rev->simplify_combined_diffs)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n \n \tif (opt->output_format & DIFF_FORMAT_RAW) {\n \t\tprintf(\"%s\", line_prefix);\n@@ -1247,12 +1265,16 @@ static void free_combined_pair(struct diff_filepair *pair)\n  * but currently nobody uses it, so this should suffice for now.\n  */\n static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n-\t\t\t\t\t   int num_parent)\n+\t\t\t\t\t   int num_parent,\n+\t\t\t\t\t   struct rev_info *rev)\n {\n \tint i;\n \tstruct diff_filepair *pair;\n \tstruct diff_filespec *pool;\n \n+\tif (rev->simplify_combined_diffs)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n+\n \tpair = xcalloc(1, sizeof(*pair));\n \tpool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));\n \tpair->one = pool + 1;\n@@ -1277,7 +1299,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n static void handle_combined_callback(struct diff_options *opt,\n \t\t\t\t     struct combine_diff_path *paths,\n \t\t\t\t     int num_parent,\n-\t\t\t\t     int num_paths)\n+\t\t\t\t     int num_paths,\n+\t\t\t\t     struct rev_info *rev)\n {\n \tstruct combine_diff_path *p;\n \tstruct diff_queue_struct q;\n@@ -1287,7 +1310,8 @@ static void handle_combined_callback(struct diff_options *opt,\n \tq.alloc = num_paths;\n \tq.nr = num_paths;\n \tfor (i = 0, p = paths; p; p = p->next)\n-\t\tq.queue[i++] = combined_pair(p, num_parent);\n+\t\tq.queue[i++] = combined_pair(p, num_parent, rev);\n+\n \topt->format_callback(&q, opt, opt->format_callback_data);\n \tfor (i = 0; i < num_paths; i++)\n \t\tfree_combined_pair(q.queue[i]);\n@@ -1498,7 +1522,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\t (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))\n \t\t\tneedsep = 1;\n \t\telse if (opt->output_format & DIFF_FORMAT_CALLBACK)\n-\t\t\thandle_combined_callback(opt, paths, num_parent, num_paths);\n+\t\t\thandle_combined_callback(opt, paths, num_parent,\n+\t\t\t\t\t\t num_paths, rev);\n \n \t\tif (opt->output_format & DIFF_FORMAT_PATCH) {\n \t\t\tif (needsep)\ndiff --git a/revision.c b/revision.c\nindex b8b0ea3..27eba77 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1820,6 +1820,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 1;\n \t\trevs->combine_merges = 1;\n+\t} else if (!strcmp(arg, \"--simplify-combined-diffs\")) {\n+\t\trevs->simplify_combined_diffs = 1;\n \t} else if (!strcmp(arg, \"-v\")) {\n \t\trevs->verbose_header = 1;\n \t} else if (!strcmp(arg, \"--pretty\")) {\ndiff --git a/revision.h b/revision.h\nindex a620530..29689a1 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -122,6 +122,7 @@ struct rev_info {\n \t\t\tignore_merges:1,\n \t\t\tcombine_merges:1,\n \t\t\tdense_combined_merges:1,\n+\t\t\tsimplify_combined_diffs:1,\n \t\t\talways_show_header:1;\n \n \t/* Format info */\n"},{"id":"246923","messageId":"xmqq61ig3xsp.fsf@gitster.dls.corp.google.com","threadId":"37236","inReplyTo":"20140729092301.GA13134@peff.net","subject":"Re: stash-p broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T17:00:22Z","receivedAt":"2014-07-29T17:00:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So perhaps we could do something better on the viewing side, and\n> \"simplify\" combined merges for files with all-identical parents.\n\nOr you can ask to show the difference with the first parent, no?\n\nI do not think giving a single-parent diff when --cc/-c is asked\nfor, which is a clear indication that the caller is aware that the\ncommit in question is a merge, is such a good idea.\n"},{"id":"246924","messageId":"20140729174357.GA20042@peff.net","threadId":"37236","inReplyTo":"xmqq61ig3xsp.fsf@gitster.dls.corp.google.com","subject":"Re: stash-p broken?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:43:57Z","receivedAt":"2014-07-29T17:43:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So perhaps we could do something better on the viewing side, and\n> > \"simplify\" combined merges for files with all-identical parents.\n> \n> Or you can ask to show the difference with the first parent, no?\n\nYeah, though there is no way to do the other thing (--second-parent to\nshow only the index side). I doubt anybody wants that, though. Just\npassing --first-parent would probably be fine (that would match \"git\nstash show\", too, though like that \"stash show\" it is impossible to\noverride to see the index portion then).\n\nBut...\n\n> I do not think giving a single-parent diff when --cc/-c is asked\n> for, which is a clear indication that the caller is aware that the\n> commit in question is a merge, is such a good idea.\n\nI think that is my point, though. The user is _not_ aware that the\ncommit in question is a merge. They stashed some stuff, and now they\nwant to see the result. I'd like to show them a vanilla commit if the\nindex is irrelevant, and otherwise show them something more interesting.\n\nI definitely don't think that should be the default behavior, but I\ndon't see a problem with it being part of stash. We pick apart the stash\nalready in git-stash.sh, so for \"git stash show\" we can do it ourselves.\nBut during a traversal, we need support from \"git log\", since the answer\nis different for each stash (and even for each path).\n\n-Peff\n"},{"id":"246935","messageId":"xmqqwqaw2fe3.fsf@gitster.dls.corp.google.com","threadId":"37236","inReplyTo":"20140729174357.GA20042@peff.net","subject":"Re: stash-p broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T18:23:16Z","receivedAt":"2014-07-29T18:23:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 29, 2014 at 10:00:22AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > So perhaps we could do something better on the viewing side, and\n>> > \"simplify\" combined merges for files with all-identical parents.\n>> \n>> Or you can ask to show the difference with the first parent, no?\n>\n> Yeah, though there is no way to do the other thing (--second-parent to\n> show only the index side). I doubt anybody wants that, though. Just\n> passing --first-parent would probably be fine (that would match \"git\n> stash show\", too, though like that \"stash show\" it is impossible to\n> override to see the index portion then).\n>\n> But...\n>\n>> I do not think giving a single-parent diff when --cc/-c is asked\n>> for, which is a clear indication that the caller is aware that the\n>> commit in question is a merge, is such a good idea.\n>\n> I think that is my point, though. The user is _not_ aware that the\n> commit in question is a merge.\n\nOh, don't get me wrong.  I am not opposed to give better output from\n\"stash show -p\".  I do not want to see us change what \"git log --cc\" \ndoes without being asked to do something different.\n\nI see you added --simplify-combined-diffs to avoid breaking \"log\",\nso that is not too bad, but I am still unsure what should happen\nwhen the first parent and the result is the same and only the second\nparent is different (i.e. you have changes in the index and your\nworking tree changes have been reverted---then you create a stash).\nShould it show as \"single source diff, with all changes reverted\"?\nShould it show as a normal \"combined diff for a merge\"?  Should it\nshow as \"no changes whatsoever\"?\n"},{"id":"246937","messageId":"xmqqlhrc2cfo.fsf@gitster.dls.corp.google.com","threadId":"37236","inReplyTo":"20140729174357.GA20042@peff.net","subject":"Re: stash-p broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T19:27:07Z","receivedAt":"2014-07-29T19:27:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think that is my point, though. The user is _not_ aware that the\n> commit in question is a merge. They stashed some stuff, and now they\n> want to see the result. I'd like to show them a vanilla commit if the\n> index is irrelevant, and otherwise show them something more interesting.\n\nI actually think \"git stash list\" users should be made very aware\nthat they are looking at merge commits, and also what two states\neach of these merge commits represents.  \"git stash --help\" says\nthat \"list\" takes any random option that you can give to \"git log\",\nand at that point you need to be very well aware what you are\ntelling \"log\" to show and how.\n\nIt probably *was* a UI mistake to make \"list\" just delegate to \"log\"\nand it was a graver UI mistake that it was documented as such;\notherwise we could have limited option set \"git stash list\" would\ntake, which can be totally independent from what \"git log\" takes,\nand we could just have said \"git stash list -p\" is a way to view\neach entry with patch in a reasonable representation.  But that\nideal world no longer exists X-<.\n"},{"id":"247007","messageId":"20140731001627.GB22297@peff.net","threadId":"37236","inReplyTo":"xmqqlhrc2cfo.fsf@gitster.dls.corp.google.com","subject":"Re: stash-p broken?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-31T00:16:27Z","receivedAt":"2014-07-31T00:16:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 12:27:07PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I think that is my point, though. The user is _not_ aware that the\n> > commit in question is a merge. They stashed some stuff, and now they\n> > want to see the result. I'd like to show them a vanilla commit if the\n> > index is irrelevant, and otherwise show them something more interesting.\n> \n> I actually think \"git stash list\" users should be made very aware\n> that they are looking at merge commits, and also what two states\n> each of these merge commits represents.\n\nYeah, I kind of agree, but I also am not optimistic about most users\nunderstanding that. I was trying to gamble on the fact that:\n\n  1. Naive users who do not understand the index will not stage files\n     and then stash in the first place.  To them, the stash would be a\n     simple diff between two states, the commit and the working tree.\n\n  2. Advanced users would see the more complicated diff, but only\n     because they were doing more interesting things with the index.\n     They know what the index is, and therefore know that stash must be\n     saving it somehow.\n\nOf course that breaks down when the naive user somehow ends up with\nstaged content in the index (e.g., they are resolving a merge and try to\nstash). Then they are thrust into the more complicated situation either\nway.\n\nI dunno. I suspect that gamble would work _most_ of the time, and makes\nan OK default. On the other hand, \"git stash list\" was completely\nuseless for showing diffs for many years, and since becoming useful has\nrequired \"--cc\" to show anything good. And this is the first complaint\nwe've seen on the list. Maybe people don't actually care, and educating\npeople to use \"--cc -p\" (or \"--first-parent -p\") is fine.\n\n-Peff\n"},{"id":"247009","messageId":"20140731002321.GD22297@peff.net","threadId":"37236","inReplyTo":"xmqqwqaw2fe3.fsf@gitster.dls.corp.google.com","subject":"Re: stash-p broken?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-31T00:23:21Z","receivedAt":"2014-07-31T00:23:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 11:23:16AM -0700, Junio C Hamano wrote:\n\n> I see you added --simplify-combined-diffs to avoid breaking \"log\",\n> so that is not too bad, but I am still unsure what should happen\n> when the first parent and the result is the same and only the second\n> parent is different (i.e. you have changes in the index and your\n> working tree changes have been reverted---then you create a stash).\n> Should it show as \"single source diff, with all changes reverted\"?\n> Should it show as a normal \"combined diff for a merge\"?  Should it\n> show as \"no changes whatsoever\"?\n\nInteresting question. I think right now we would show nothing in the\ncombined diff, as we simply took one side. This is really showing the\nweakness of the \"stash is a merge\" storage format. If we use it as a\nstorage format, and pick it apart in \"git stash\", that's fine. But as\nsoon as we start treating it like a real merge and expecting \"git log\"\nto behave, the cracks begin to show.\n\nHmph. To be honest, I am starting to wonder if implying \"--first-parent\"\nis a more sensible option for \"stash list\". It matches \"stash show\", at\nleast, and it is not unreasonable to simply present the changes in the\nworking tree by default, and ignore the index. People who are more\nclueful can pick apart the commits using \"git log\" themselves.\n\n-Peff\n"},{"id":"247065","messageId":"xmqqbns5xyl6.fsf@gitster.dls.corp.google.com","threadId":"37236","inReplyTo":"20140731002321.GD22297@peff.net","subject":"Re: stash-p broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-31T16:49:41Z","receivedAt":"2014-07-31T16:49:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmph. To be honest, I am starting to wonder if implying \"--first-parent\"\n> is a more sensible option for \"stash list\". It matches \"stash show\", at\n> least, and it is not unreasonable to simply present the changes in the\n> working tree by default, and ignore the index. People who are more\n> clueful can pick apart the commits using \"git log\" themselves.\n\nYup, I tend to agree.\n\nAfter thinking about this a lot, I really wish that we did not have\n\"stash apply\" and \"stash pop\", but just \"stash save\" and a single\n\"stash restore\" that either checks out the original branch (if its\nhead have not moved) or detaches the HEAD at the original commit\n(otherwise), and then restores the state of the index and the\nworking tree (with or without untracked) and nothing else, but the\ndetails are too long to fit in this topic ;-)\n"}]}