Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Fri, Aug 24, 2018 at 7:42 AM Duy Nguyen wrote: > > On Fri, Aug 24, 2018 at 5:02 AM Jacob Keller wrote: > > > > On Thu, Aug 23, 2018 at 9:28 AM Junio C Hamano wrote: > > > I think the above example forgets "-a" on the final "git commit" > > > step. With it added, I can understand the concern (and I am sure > > > you would, too). > > > > > > The user is trying to add everything done in the working tree, and > > > "commit -a" would catch all changes to paths that were already > > > tracked, but a separate "add" is necessary for newly created paths. > > > But adding a new path means the index no longer matches HEAD, and > > > the "commit -a" at the final step sweeps the changes to already > > > tracked paths---failing that because there "already is something > > > staged" will break the workflow. > > > > Right. I think this would need to be able to understand the case of > > "different only by new files". > > OK so the rules I'm going to try to implement is, if the version in > the index is not the same as one in HEAD or one in worktree, then "git > commit -a" fails. > > The unwritten line is, if the path in question does not exist in HEAD, > then the first condition "as one in HEAD" is dropped, naturally. This > addresses the "git add new-file" problem, but if you have made more > changes in new-file in worktree after "git add" and do "git commit -a" > then you still get rejected because it fails the second condition. > > File removal should be considered as well. But I don't foresee any > problem there. Resolving merges, replacing higher stage entries with > stage 0 will be accepted at "git commit -a" as usual. > > Let me know if we should tweak these rules (and how). > -- > Duy This seems reasonable to me. It might trigger a few issues with people doing "git add new_file ; $EDITOR new_file ; git commit -a" but... I know I've accidentally done "git commit -a" and had problems. What about "git commit "? That's another case where I've accidentally lost some work for a similar reason. Thanks, Jake
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Fri, Aug 24, 2018 at 5:02 AM Jacob Keller wrote: > > On Thu, Aug 23, 2018 at 9:28 AM Junio C Hamano wrote: > > I think the above example forgets "-a" on the final "git commit" > > step. With it added, I can understand the concern (and I am sure > > you would, too). > > > > The user is trying to add everything done in the working tree, and > > "commit -a" would catch all changes to paths that were already > > tracked, but a separate "add" is necessary for newly created paths. > > But adding a new path means the index no longer matches HEAD, and > > the "commit -a" at the final step sweeps the changes to already > > tracked paths---failing that because there "already is something > > staged" will break the workflow. > > Right. I think this would need to be able to understand the case of > "different only by new files". OK so the rules I'm going to try to implement is, if the version in the index is not the same as one in HEAD or one in worktree, then "git commit -a" fails. The unwritten line is, if the path in question does not exist in HEAD, then the first condition "as one in HEAD" is dropped, naturally. This addresses the "git add new-file" problem, but if you have made more changes in new-file in worktree after "git add" and do "git commit -a" then you still get rejected because it fails the second condition. File removal should be considered as well. But I don't foresee any problem there. Resolving merges, replacing higher stage entries with stage 0 will be accepted at "git commit -a" as usual. Let me know if we should tweak these rules (and how). -- Duy
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Thu, Aug 23, 2018 at 9:28 AM Junio C Hamano wrote: > I think the above example forgets "-a" on the final "git commit" > step. With it added, I can understand the concern (and I am sure > you would, too). > > The user is trying to add everything done in the working tree, and > "commit -a" would catch all changes to paths that were already > tracked, but a separate "add" is necessary for newly created paths. > But adding a new path means the index no longer matches HEAD, and > the "commit -a" at the final step sweeps the changes to already > tracked paths---failing that because there "already is something > staged" will break the workflow. Right. I think this would need to be able to understand the case of "different only by new files". Thanks, Jake
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Mon, Aug 20, 2018 at 8:43 AM Nguyễn Thái Ngọc Duy wrote: > Instead, let's handle just this problem for now. This new option > commit.preciousDirtyIndex, if set to false, will not allow `commit -a` > to continue if the final index is different from the existing one. I > don't think this can be achieved with hooks because the hooks don't > know about "-a" or different commit modes. > Here you say setting it to "false" enables this check but... > +commit.preciousDirtyIndex:: > + If some changes are partially staged in the index (i.e. > + "git commit -a" and "git commit" commit different changes), reject > + "git commit -a". > + Here, sounds like it is enabled by setting it to true. Thanks, Jake
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
Duy Nguyen writes: > On Thu, Aug 23, 2018 at 4:15 AM Jonathan Nieder wrote: >> > The remaining question becomes scripts. A script might do >> > >> > ... modify old-file and create new-file ... >> > git add new-file >> > git commit -m "some great message" >> > >> > which would trip this error. For that matter, humans might do that, >> > too. Could the check detect this case (where the only changes in the >> > index are additions of new files) and treat it as non-destructive? >> >> (where the only changes in the index are additions of new files *that >> match the worktree*) > > I don't think my change would affect this case. If "git commit" does > not change the index by itself, there's no point in stopping it. I think the above example forgets "-a" on the final "git commit" step. With it added, I can understand the concern (and I am sure you would, too). The user is trying to add everything done in the working tree, and "commit -a" would catch all changes to paths that were already tracked, but a separate "add" is necessary for newly created paths. But adding a new path means the index no longer matches HEAD, and the "commit -a" at the final step sweeps the changes to already tracked paths---failing that because there "already is something staged" will break the workflow.
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Thu, Aug 23, 2018 at 4:15 AM Jonathan Nieder wrote: > > The remaining question becomes scripts. A script might do > > > > ... modify old-file and create new-file ... > > git add new-file > > git commit -m "some great message" > > > > which would trip this error. For that matter, humans might do that, > > too. Could the check detect this case (where the only changes in the > > index are additions of new files) and treat it as non-destructive? > > (where the only changes in the index are additions of new files *that > match the worktree*) I don't think my change would affect this case. If "git commit" does not change the index by itself, there's no point in stopping it. -- Duy
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
A few quick corrections: Jonathan Nieder wrote: > I'm starting to lean toward having this on unconditionally, with a > message that points the user who really doesn't want to clobber their ... who really *does* want to clobber their index ... > index toward "git add -u", as a good idea. I think that for humans, > that would be okay and that configuration doesn't really help much > for this. > > The remaining question becomes scripts. A script might do > > ... modify old-file and create new-file ... > git add new-file > git commit -m "some great message" > > which would trip this error. For that matter, humans might do that, > too. Could the check detect this case (where the only changes in the > index are additions of new files) and treat it as non-destructive? (where the only changes in the index are additions of new files *that match the worktree*) Sorry for the noise, Jonathan
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
Duy Nguyen wrote: > On Mon, Aug 20, 2018 at 9:30 PM Jonathan Nieder wrote: >> I frequently use "git commit -a" this way intentionally, so I would be >> unlikely to turn this config on. That leads me to suspect it's not a >> good candidate for configuration: >> >> - it's not configuration for the sake of a transition period, since some >> people would keep it on forever >> >> - it's not configuration based on different project needs, either >> >> So configuration doesn't feel like a good fit. > > I think it falls under personal preference (yes some people like me > will keep it on forever in fear of losing staged changes). Sorry for the lack of clarity. I meant "some people would keep it off forever". >> That said, I lean toward your initial thought, that this is papering >> over a missing undo feature. Can you say more about how you'd imagine >> undo working? [...] > [1] > https://public-inbox.org/git/1375597720-13236-1-git-send-email-pclo...@gmail.com/ > [2] > https://public-inbox.org/git/1375966270-10968-1-git-send-email-pclo...@gmail.com/ Thanks for the links! That's very helpful. I'm starting to lean toward having this on unconditionally, with a message that points the user who really doesn't want to clobber their index toward "git add -u", as a good idea. I think that for humans, that would be okay and that configuration doesn't really help much for this. The remaining question becomes scripts. A script might do ... modify old-file and create new-file ... git add new-file git commit -m "some great message" which would trip this error. For that matter, humans might do that, too. Could the check detect this case (where the only changes in the index are additions of new files) and treat it as non-destructive? Thanks, Jonathan
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Mon, Aug 20, 2018 at 9:30 PM Jonathan Nieder wrote: > > Hi, > > Nguyễn Thái Ngọc Duy wrote: > > > So many times I have carefully cherry picked changes to the index with > > `git add -p` then accidentally did `git commit -am ` (usually by > > retrieving a command from history and pressing Enter too quickly) > > which destroyed beautiful index. > > > > One way to deal with this is some form of `git undo` that allows me to > > retrieve the old index. > > Yes, I would love such an undo feature! > > How would you imagine that this information would get stored? We can > start with adding that and a low-level (plumbing) interface to it, to > avoid being blocked on getting the user-facing (porcelain) "git undo" > interface right. (Or we can go straight for the porcelain. It's fine > for it to start minimal and gain switches later.) Yeah I'd love to see "git undo" too, but unfortunately I don't have a clear model how it should operate. Which is why I still hesitate to implement one. :P All I have is something close to how undo is done in an editor, where we could save a list of actions and corresponding undo ones, but editors are wysiwyg and we can't just let the user undo, see the result elsewhere and redo if they're not happy. > > Instead, let's handle just this problem for now. This new option > > commit.preciousDirtyIndex, if set to false, will not allow `commit -a` > > to continue if the final index is different from the existing one. I > > don't think this can be achieved with hooks because the hooks don't > > know about "-a" or different commit modes. > > I frequently use "git commit -a" this way intentionally, so I would be > unlikely to turn this config on. That leads me to suspect it's not a > good candidate for configuration: > > - it's not configuration for the sake of a transition period, since some > people would keep it on forever > > - it's not configuration based on different project needs, either > > So configuration doesn't feel like a good fit. I think it falls under personal preference (yes some people like me will keep it on forever in fear of losing staged changes). > That said, I lean toward your initial thought, that this is papering > over a missing undo feature. Can you say more about how you'd imagine > undo working? There is not much to say. But at least to be able to undo changes in the index, I made two attempts in the past [1] [2] (and forgot about them until I got bitten by the lack of undo again this time). I guess I could push for one of those approaches again because it will help me and also lay the foundation for any git-undo in the future. Once we have can restore index, undoing "git reset --hard" is also possible (by hashing everything and putting them in a temporary index first). [1] https://public-inbox.org/git/1375597720-13236-1-git-send-email-pclo...@gmail.com/ [2] https://public-inbox.org/git/1375966270-10968-1-git-send-email-pclo...@gmail.com/ -- Duy
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
Hi, Nguyễn Thái Ngọc Duy wrote: > So many times I have carefully cherry picked changes to the index with > `git add -p` then accidentally did `git commit -am ` (usually by > retrieving a command from history and pressing Enter too quickly) > which destroyed beautiful index. > > One way to deal with this is some form of `git undo` that allows me to > retrieve the old index. Yes, I would love such an undo feature! How would you imagine that this information would get stored? We can start with adding that and a low-level (plumbing) interface to it, to avoid being blocked on getting the user-facing (porcelain) "git undo" interface right. (Or we can go straight for the porcelain. It's fine for it to start minimal and gain switches later.) [...] > Instead, let's handle just this problem for now. This new option > commit.preciousDirtyIndex, if set to false, will not allow `commit -a` > to continue if the final index is different from the existing one. I > don't think this can be achieved with hooks because the hooks don't > know about "-a" or different commit modes. I frequently use "git commit -a" this way intentionally, so I would be unlikely to turn this config on. That leads me to suspect it's not a good candidate for configuration: - it's not configuration for the sake of a transition period, since some people would keep it on forever - it's not configuration based on different project needs, either So configuration doesn't feel like a good fit. It might be that I can switch my muscle memory to "git add -u && git commit", in which case this could work as a new default without configuration (or with configuration for a transition period). A separate commandline option "git commit -a --no-clobber-index" might make sense as well, since then I could pass --clobber-index to keep my current workflow. That would also follow the usual progression: introduce commandline option first, then config, then change default. That said, I lean toward your initial thought, that this is papering over a missing undo feature. Can you say more about how you'd imagine undo working? Thanks, Jonathan
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
On Mon, Aug 20, 2018 at 11:41 AM Nguyễn Thái Ngọc Duy wrote: > One way to deal with this is some form of `git undo` that allows me to > retrieve the old index. That's not a lot of work by itself. The problem > is designing that `git undo` interface because there are more undo > options that this. s/that/than/
Re: [PATCH/RFC] commit: new option to abort -a something is already staged
Nguyễn Thái Ngọc Duy writes:
> +commit.preciousDirtyIndex::
> + If some changes are partially staged in the index (i.e.
> + "git commit -a" and "git commit" commit different changes), reject
> + "git commit -a".
Or perhaps better yet, go into yes/no interaction to confirm if you
have access to interactive terminal at fd #0/#1, perhaps?
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 213fca2d8e..489e4b9f50 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -98,6 +98,7 @@ static const char *author_message, *author_message_buffer;
> static char *edit_message, *use_message;
> static char *fixup_message, *squash_message;
> static int all, also, interactive, patch_interactive, only, amend, signoff;
> +static int allow_dirty_index = 1;
> static int edit_flag = -1; /* unspecified */
> static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> static int config_commit_verbose = -1; /* unspecified */
> @@ -385,10 +386,24 @@ static const char *prepare_index(int argc, const char
> **argv, const char *prefix
>* (B) on failure, rollback the real index.
>*/
> if (all || (also && pathspec.nr)) {
> + int compare_oid = all && !allow_dirty_index;
> + struct object_id previous_oid;
> +
> + if (compare_oid) {
> + if (update_main_cache_tree(0) || !the_index.cache_tree)
> + die(_("error building trees"));
Hmph, when we conclude a conflicted merge with "commit -a", wouldn't
we fail to compute the "before" picture, with higher-stage entries
stil in the index?
> + if (the_index.cache_tree->entry_count >= 0)
> + oidcpy(_oid,
> _index.cache_tree->oid);
> + else
> + oidclr(_oid);
The cache-tree covers no entry, meaning the index has no cache
entries? Shouldn't we setting EMPTY_TREE_SHA1_BIN or something
instead?
> + }
> hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
> add_files_to_cache(also ? prefix : NULL, , 0);
> refresh_cache_or_die(refresh_flags);
> update_main_cache_tree(WRITE_TREE_SILENT);
> + if (compare_oid && the_index.cache_tree &&
> + oidcmp(_oid, _index.cache_tree->oid))
> + die(_("staged content is different, aborting"));
I was hoping that it is an easy change to teach add_files_to_cache()
to report how many paths it actually has "added" (modifications and
removals are of course also counted), which allows us not to waste
computing a throw-away tree object once more.
There only are three existing callers to the function, and only one
of them rely on the current "non-zero is error, zero is good"
semantics, so updating that caller (and perhaps vetting the other
callers to see if they also _should_ pay attention to the return
value, at least to detect errors of not number of paths updated)
shouldn't be that much more effort, and would be a good update to
the API regardless of this new feature, I would think.
> if (write_locked_index(_index, _lock, 0))
> die(_("unable to write new_index file"));
> commit_style = COMMIT_NORMAL;
> @@ -1413,6 +1428,10 @@ static int git_commit_config(const char *k, const char
> *v, void *cb)
> config_commit_verbose = git_config_bool_or_int(k, v, _bool);
> return 0;
> }
> + if (!strcmp(k, "commit.preciousdirtyindex")) {
> + allow_dirty_index = !git_config_bool(k, v);
> + return 0;
> + }
>
> status = git_gpg_config(k, v, NULL);
> if (status)
