{"thread":{"id":"50888","subject":"[GSoC][RFC] discussion about stashing with conflicts","startedAt":"2019-04-07T13:08:27Z","lastAt":"2019-04-08T21:55:41Z","messageCount":8,"participants":["Kapil Jain","Thomas Gummerer","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"373350","messageId":"CAMknYEN1x5zDPn4vaZmw3ch-Oy2=NQ=cfF9YmXumcbZGWvTToQ@mail.gmail.com","threadId":"50888","inReplyTo":null,"subject":"[GSoC][RFC] discussion about stashing with conflicts","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-07T13:08:13Z","receivedAt":"2019-04-07T13:08:27Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"below is my understanding from reading the parts of code as suggested on IRC.\n\nwhat is the use of ce_stage macro ?\ntells about stage of an index entry.\nif ce_stage says, stage #0 i.e staging area, then that index entry is\nin staging area\nand nothing needs to be done.\nelse a temporary index entry is created and repo_read_index_unmerged()\ncalls other function and tries to add it to index.\nif it fails, it issues an error.\n\nis this correct interpretation ?\n\n1) in repo_read_index_unmerged(), why don't we make the value of\n`unmerged` 0, if adding index entry is successful; as the entry is no\nlonger unmerged ?\n\n2) what is ADD_CACHE_SKIP_DFCHECK ?\ni am unsure if i get its meaning, cache.h says that it means \"Ok to\nskip DF conflict checks\"\nwhat are DF conflict checks ? something about diffing to check for\nconflicts ? if so why are we skipping it this entry had conflicts in\nthe past maybe it will create again.\n\n3) what is cache_nr variable in index_state struct ? what is its use ?\n\n\nNow, about add_index_entry_with_check(), i don't fully understand this\nfunction but concentrating on the part pointed by dscho.\nhttps://github.com/git/git/blob/v2.21.0/read-cache.c#L1284-L1294\n\n    /*\n     * Inserting a merged entry (\"stage 0\") into the index\n     * will always replace all non-merged entries..\n     */\n\nso this is the part we need to play with for the project\nhttps://git.github.io/SoC-2019-Ideas/#teach-git-stash-to-handle-unmerged-index-entries\ntry and change this in some-way to not replace those unmerged entries\nor store them some place else ?\n"},{"id":"373353","messageId":"20190407183857.GA32487@hank.intra.tgummerer.com","threadId":"50888","inReplyTo":"CAMknYEN1x5zDPn4vaZmw3ch-Oy2=NQ=cfF9YmXumcbZGWvTToQ@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-07T18:38:57Z","receivedAt":"2019-04-07T18:39:05Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/07, Kapil Jain wrote:\n> below is my understanding from reading the parts of code as suggested on IRC.\n> \n> what is the use of ce_stage macro ?\n> tells about stage of an index entry.\n> if ce_stage says, stage #0 i.e staging area, then that index entry is\n> in staging area\n> and nothing needs to be done.\n\nI don't quite understand what you mean with \"nothing needst to be\ndone\" here.  In the context of teaching 'git stash' to handle unmerged\nindex entries, nothing that is not already being done needs to be done\nwith an index entry that is at stage #0.  The current implementation\nalready handles that correctly.\n\n> else a temporary index entry is created and repo_read_index_unmerged()\n> calls other function and tries to add it to index.\n> if it fails, it issues an error.\n\nNot sure what you mean here.  Index entries with higher stages are not\ntemporary, they are written out to the index file, and can then be\nread back with 'repo_read_index()' for example.\n\nrepo_read_index_unmerged() drops entries to stage #0, and marks them\nas CE_CONFLICTED, which callers can then use to distinguish index\nentries that are conflicted and those that are not.\n\nIn the context of the GSoC project, this function is probably not\nuseful, because we need to separate out entries of different stages,\nso we can create commits of those entries, as described in the thread\nthat's also linked from the project page [*1*].  Once\nrepo_read_index_unmerged() is called, we no longer know which entry is\nat which stage (they are all at stage 0).\n\n*1*: https://public-inbox.org/git/nycvar.QRO.7.76.6.1902072023250.41@tvgsbejvaqbjf.bet/\n\n> is this correct interpretation ?\n> \n> 1) in repo_read_index_unmerged(), why don't we make the value of\n> `unmerged` 0, if adding index entry is successful; as the entry is no\n> longer unmerged ?\n\nBecause the caller often wants to know if the index is unmerged in the\nfirst place, and would refuse to operate on such an index.  Read the\ncomment documenting the function again, that explains this very\nnicely.  Then see how some callers actually use the function, and\nyou'll see that they actually don't care about dropping the entry to\nstage 0, but they care about knowing whether the index is unmerged or\nnot.\n\n> 2) what is ADD_CACHE_SKIP_DFCHECK ?\n> i am unsure if i get its meaning, cache.h says that it means \"Ok to\n> skip DF conflict checks\"\n> what are DF conflict checks ? something about diffing to check for\n> conflicts ? if so why are we skipping it this entry had conflicts in\n> the past maybe it will create again.\n\nA DF conflict is a directory/file conflict, where one side of the\nmerge has created a file that has the same name as a directory created\non the other side of the merge.  From a bit of digging around in\ngit.git, t/t1000-read-tree-m-3way.sh seems to have some good\nexplanation on the various types of conflicts, not sure if it's\ndocumented somewhere in Documentation/ in detail as well.\n\nIn this function we just care about creating an index that doesn't\ncontain any conflicts, so we are skipping the checks here.\n\n> 3) what is cache_nr variable in index_state struct ? what is its use ?\n\nIf you look for cache_nr in the codebase you'll see that it's often\nused when iterating through the whole index.  That could give you a\nhint what it is.  Alternatively you could go have a look where it is\nassigned, and what it is assigned to.  Looking through read-cache.c I\ncan find this line [*2*].  hdr_entries is in 'struct cache_header',\nwhich is the header of the index file (cache is another name for the\nindex that's used in the codebase).\n\nNow if you look at Documentation/technical/index-format.txt, you can\nsee that it's the number of entries that are in the index.\n\n*2*: https://github.com/gitster/git/blob/041f5ea1cf987a4068ef5f39ba0a09be85952064/read-cache.c#L2163\n\n> Now, about add_index_entry_with_check(), i don't fully understand this\n> function but concentrating on the part pointed by dscho.\n> https://github.com/git/git/blob/v2.21.0/read-cache.c#L1284-L1294\n> \n>     /*\n>      * Inserting a merged entry (\"stage 0\") into the index\n>      * will always replace all non-merged entries..\n>      */\n> \n> so this is the part we need to play with for the project\n> https://git.github.io/SoC-2019-Ideas/#teach-git-stash-to-handle-unmerged-index-entries\n> try and change this in some-way to not replace those unmerged entries\n> or store them some place else ?\n\nIt would be useful to repeat what the context is in which Dscho gave\nyou this code to read.  Looking back at the IRC logs, I can see:\n\n    11:55:15 dscho | Also, here is some code that is helpful to understand\n    what happens when unmerged entries are collapsed into a merged entry:\n    https://github.com/git/git/blob/v2.21.0/read-cache.c#L1284-L1294\n\nAnd the whole conversation was about trying to understand index\nentries better, which is a fundamental part of the project, rather\nthan code that needs to be changed for the project itself.  Most\nlikely you will not change any functions in read-cache.c, but you'll\nneed an understanding of how they do things.\n\nSo the question is, did you read this function in depth and understand\nwhat it does?  If you want to validate your understanding of the\nfunction, try to repeat what it does in your own words, and ask for us\nto correct you.\n"},{"id":"373395","messageId":"CAMknYEOAg1S8cNYdLPgJemxgikisNpmeuw74T0w+7PUo93stVg@mail.gmail.com","threadId":"50888","inReplyTo":"20190407183857.GA32487@hank.intra.tgummerer.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-08T05:48:23Z","receivedAt":"2019-04-08T05:48:38Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"On Mon, Apr 8, 2019 at 12:09 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:\n>\n> On 04/07, Kapil Jain wrote:\n> >\n> > what is the use of ce_stage macro ?\n> > tells about stage of an index entry.\n> > if ce_stage says, stage #0 i.e staging area, then that index entry is\n> > in staging area\n> > and nothing needs to be done.\n>\n> I don't quite understand what you mean with \"nothing needst to be\n> done\" here.  In the context of teaching 'git stash' to handle unmerged\n> index entries, nothing that is not already being done needs to be done\n> with an index entry that is at stage #0.  The current implementation\n> already handles that correctly.\n>\n> > else a temporary index entry is created and repo_read_index_unmerged()\n> > calls other function and tries to add it to index.\n> > if it fails, it issues an error.\n>\n> Not sure what you mean here.  Index entries with higher stages are not\n> temporary, they are written out to the index file, and can then be\n> read back with 'repo_read_index()' for example.\n\nsorry, i failed to provide detailed explanation. below is what i meant.\n\nin repo_read_index_merged(),\nif ce_stage() macro says that this cache_entry is in stage #0 i.e.\nalready merged,\nthen the function doesn't try to add that entry into index.\n\nif (!ce_stage(ce))\n    continue;\n\nbut when it is not in stage #0; the function, creates a temporary cache_entry,\n\nstruct cache_entry *new_ce;\nnew_ce = make_empty_cache_entry(istate, len);\n\nand tries to add it to index file.\n\nif (add_index_entry(istate, new_ce, ADD_CACHE_SKIP_DFCHECK))\n    return error(_(\"%s: cannot drop to stage #0\"),\n             new_ce->name);\n\nnow if this try of adding index entry is successful, then that entry\nis no longer unmerged, right ?\nso can we make `unmerged` variable 0.\n\n> >\n> > 1) in repo_read_index_unmerged(), why don't we make the value of\n> > `unmerged` 0, if adding index entry is successful; as the entry is no\n> > longer unmerged ?\n>\n> Because the caller often wants to know if the index is unmerged in the\n> first place, and would refuse to operate on such an index.  Read the\n> comment documenting the function again, that explains this very\n> nicely.  Then see how some callers actually use the function, and\n> you'll see that they actually don't care about dropping the entry to\n> stage 0,\n\n>but they care about knowing whether the index is unmerged or\n> not.\n\nif they care about whether the index *is* unmerged, and that call to\nadd_index_entry()\nis successful, then index is no longer unmerged (at least because of\nthat index_entry).\nis it possible that they care about if index *was* unmerged ?\n\n\n> So the question is, did you read this function in depth and understand\n> what it does?  If you want to validate your understanding of the\n> function, try to repeat what it does in your own words, and ask for us\n> to correct you.\n\nupcoming mail will do so.\n"},{"id":"373411","messageId":"CACsJy8Dc3fuXWOOO-hNJqGNomufP7bffoHVf5hHLTubHQvq9vA@mail.gmail.com","threadId":"50888","inReplyTo":"CAMknYEOAg1S8cNYdLPgJemxgikisNpmeuw74T0w+7PUo93stVg@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-08T10:31:56Z","receivedAt":"2019-04-08T10:32:25Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 8, 2019 at 12:49 PM Kapil Jain <jkapil.cs@gmail.com> wrote:\n>\n> On Mon, Apr 8, 2019 at 12:09 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> >\n> > On 04/07, Kapil Jain wrote:\n> > >\n> > > what is the use of ce_stage macro ?\n> > > tells about stage of an index entry.\n> > > if ce_stage says, stage #0 i.e staging area, then that index entry is\n> > > in staging area\n> > > and nothing needs to be done.\n> >\n> > I don't quite understand what you mean with \"nothing needst to be\n> > done\" here.  In the context of teaching 'git stash' to handle unmerged\n> > index entries, nothing that is not already being done needs to be done\n> > with an index entry that is at stage #0.  The current implementation\n> > already handles that correctly.\n> >\n> > > else a temporary index entry is created and repo_read_index_unmerged()\n> > > calls other function and tries to add it to index.\n> > > if it fails, it issues an error.\n> >\n> > Not sure what you mean here.  Index entries with higher stages are not\n> > temporary, they are written out to the index file, and can then be\n> > read back with 'repo_read_index()' for example.\n>\n> sorry, i failed to provide detailed explanation. below is what i meant.\n>\n> in repo_read_index_merged(),\n> if ce_stage() macro says that this cache_entry is in stage #0 i.e.\n> already merged,\n> then the function doesn't try to add that entry into index.\n>\n> if (!ce_stage(ce))\n>     continue;\n>\n> but when it is not in stage #0; the function, creates a temporary cache_entry,\n>\n> struct cache_entry *new_ce;\n> new_ce = make_empty_cache_entry(istate, len);\n>\n> and tries to add it to index file.\n>\n> if (add_index_entry(istate, new_ce, ADD_CACHE_SKIP_DFCHECK))\n>     return error(_(\"%s: cannot drop to stage #0\"),\n>              new_ce->name);\n>\n> now if this try of adding index entry is successful, then that entry\n> is no longer unmerged, right ?\n> so can we make `unmerged` variable 0.\n\nThere's a big block of comment on top saying return true if unmerged.\nAs you noticed, after the function returns, there will be no unmerged\nentries left anyway. Always returning zero then does not even make\nsense.\n\nThe main purpose of this function is probably just checking that there\nare unmerged entries or not (and return 1 if so). The\nadd_index_entry() there is probably just to do some more validation.\n\nSometimes when I don't understand what some code does, I look at \"git\nlog --patch\". In this case, there a big explanation in ad3762042a\n(read-cache: fix directory/file conflict handling in\nread_index_unmerged(), 2018-07-31) that might help you.\n\nGoing further back to d147e501f3 (Builtin git-read-tree., 2006-05-23),\nyou can see that this function has two separate uses. One about the\nreturn code, one about collapsing unmerged entries back to stage zero.\n-- \nDuy\n"},{"id":"373412","messageId":"CAMknYENqH==YcAfU-B16Jytc4Pts4viNQn9deTFag++zZVb1+Q@mail.gmail.com","threadId":"50888","inReplyTo":"CACsJy8Dc3fuXWOOO-hNJqGNomufP7bffoHVf5hHLTubHQvq9vA@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-08T11:00:46Z","receivedAt":"2019-04-08T11:01:01Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"On Mon, Apr 8, 2019 at 4:02 PM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> Sometimes when I don't understand what some code does, I look at \"git\n> log --patch\". In this case, there a big explanation in ad3762042a\n> (read-cache: fix directory/file conflict handling in\n> read_index_unmerged(), 2018-07-31) that might help you.\n>\n\ni got my reason from the commit message,\n\n\"The _only_ reason we want to keep a previously unmerged entry in the\nindex at stage #0 is so that we don't forget the fact that we have\ncorresponding file in the work tree in order to be able to remove it\nwhen the tree we are resetting to does not have the path.\"\n\nand now that i got back to reading that comment, it makes sense.\n\nfor finding that commit message did you do something like this:\ngit log -L :repo_read_index_unmerged:read-cache.c\n\nThanks.\n"},{"id":"373414","messageId":"CACsJy8D7mQZgJhZ4H8Z929EnjYe1Wp7idrbXqkUsmL1Ypn83dg@mail.gmail.com","threadId":"50888","inReplyTo":"CAMknYENqH==YcAfU-B16Jytc4Pts4viNQn9deTFag++zZVb1+Q@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-08T11:09:10Z","receivedAt":"2019-04-08T11:09:39Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 8, 2019 at 6:00 PM Kapil Jain <jkapil.cs@gmail.com> wrote:\n> for finding that commit message did you do something like this:\n> git log -L :repo_read_index_unmerged:read-cache.c\n\nNo. I'm old fashioned. I just do \"git log --patch --follow <path>\"\nthen search read_index_unmerged, which should at least appear in the\n@@ line. But yeah log -L or \"tig blame\" may be better to dig in\nhistory.\n-- \nDuy\n"},{"id":"373423","messageId":"CAMknYENGZBQ6-8sJ5UrKS_ZLLoz79K8FYUHFQqSpV2JrkSuf6w@mail.gmail.com","threadId":"50888","inReplyTo":"CACsJy8D7mQZgJhZ4H8Z929EnjYe1Wp7idrbXqkUsmL1Ypn83dg@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-08T14:27:44Z","receivedAt":"2019-04-08T14:27:58Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"while understanding add_index_entry_with_check()\ni got to do_invalidate_path().\n\nthe commit hash for do_invalidate_path() function is\n`749864627c2d3c33bbc56d7ba0b5542af698cc40`\n\nin the commit message it is explained that, cache-tree is used to\nstore object names of index file objects and it is kept separate from\nindex file because adding it would change index file format.\n\nthe part i couldn't understand is:\n\"During various index manupulations, we selectively invalidate\nthe parts so that the next write-tree can bypass regenerating\ntree objects for unchanged parts of the directory hierarchy.\"\n\nwhat exactly does invalidating means here ?\n"},{"id":"373443","messageId":"20190408215535.GB32487@hank.intra.tgummerer.com","threadId":"50888","inReplyTo":"CAMknYENGZBQ6-8sJ5UrKS_ZLLoz79K8FYUHFQqSpV2JrkSuf6w@mail.gmail.com","subject":"Re: [GSoC][RFC] discussion about stashing with conflicts","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-08T21:55:35Z","receivedAt":"2019-04-08T21:55:41Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/08, Kapil Jain wrote:\n> while understanding add_index_entry_with_check()\n> i got to do_invalidate_path().\n> \n> the commit hash for do_invalidate_path() function is\n> `749864627c2d3c33bbc56d7ba0b5542af698cc40`\n> \n> in the commit message it is explained that, cache-tree is used to\n> store object names of index file objects and it is kept separate from\n> index file because adding it would change index file format.\n> \n> the part i couldn't understand is:\n> \"During various index manupulations, we selectively invalidate\n> the parts so that the next write-tree can bypass regenerating\n> tree objects for unchanged parts of the directory hierarchy.\"\n> \n> what exactly does invalidating means here ?\n\nFWIW, I don't think you need to understand cache-tree for the stash\nGSoC project.  Your time is probably better spent taking what you\nlearned, and trying to make that into a proposal, as the application\nperiod is coming to an end.\n\nThat said, since we are talking about a cache here, invalidating means\nsimply making part of the cache invalid, which means the caches\ncontents need to be re-calculated next time they are needed.\n\nFor the cache-tree in particular that means that we need to\nre-calculate tree objects that have been invalidated, while we can\njust re-use the ones that have not.\n\nIf you want to have a look at the cache-tree, you can use\n't/helper/test-tool dump-cache-tree .git/index' from your locally\nbuilt git, which will dump the cache-tree that can be found\n'.git/index'.  Compare the output of that command just after you did\n'git reset --hard' on your repository (of course it needs some\ncontents), and after you modified some file, and added it to the index\nusing 'git add'.  In the latter case you will notice some directories\nthat are marked as 'invalid'.\n"}]}