Stash #796

Merged
merged 11 commits into from Oct 29, 2012

4 participants

@nulltoken
libgit2 member
  • git_stash_save()

    • GIT_STASH_KEEP_INDEX
    • GIT_STASH_INCLUDE_UNTRACKED
    • GIT_STASH_INCLUDE_IGNORED
  • git_stash_foreach()

  • git_stash_drop()
@travisbot

This pull request fails (merged 0f8ef914 into cbc02c1).

@travisbot

This pull request fails (merged 6dbe1578 into e560aa8).

@travisbot

This pull request fails (merged 806f4ff9 into e560aa8).

@travisbot

This pull request fails (merged 6a93e443 into e560aa8).

@travisbot

This pull request fails (merged 4b91735c into e560aa8).

@travisbot

This pull request fails (merged 2711e54c into e560aa8).

@travisbot

This pull request fails (merged 7354671d into b00e921).

@travisbot

This pull request fails (merged c91e864c into b00e921).

@travisbot

This pull request fails (merged 705e90c1 into b00e921).

@travisbot

This pull request fails (merged afc956be into b00e921).

@travisbot

This pull request fails (merged cea4a09a into b00e921).

@travisbot

This pull request fails (merged d08c3ea6 into b00e921).

@travisbot

This pull request fails (merged d46fec0b into b00e921).

@travisbot

This pull request fails (merged 78c30737 into b00e921).

@travisbot

This pull request fails (merged ffc9a96c into b00e921).

@travisbot

This pull request fails (merged 5173d10b into b00e921).

@travisbot

This pull request fails (merged a902e0e3 into b00e921).

@travisbot

This pull request fails (merged a2666366 into b00e921).

@travisbot

This pull request fails (merged 44fdcc4c into b00e921).

@travisbot

This pull request fails (merged d457abf6 into b00e921).

@travisbot

This pull request fails (merged 3f1de0c4 into b00e921).

@travisbot

This pull request fails (merged a46eac4d into b00e921).

@travisbot

This pull request fails (merged a2b6558e into b00e921).

@travisbot

This pull request fails (merged d6d0bad9 into b00e921).

@travisbot

This pull request fails (merged dfbf1d6c into b00e921).

@travisbot

This pull request fails (merged 0b4f2ddc into b00e921).

@travisbot

This pull request fails (merged d8beb4e8 into b00e921).

@travisbot

This pull request fails (merged 87d11053 into b00e921).

@travisbot

This pull request fails (merged 703e70a9 into b00e921).

@travisbot

This pull request fails (merged 2107107b into 111ee3f).

@travisbot

This pull request fails (merged 613cf29f into dd4345b).

@travisbot

This pull request fails (merged ab5ef3ac into 9f99c5d).

@travisbot

This pull request fails (merged 33fe7b11 into d1af70b).

@travisbot

This pull request fails (merged 5e2fa0ad into d4b5735).

@travisbot

This pull request fails (merged 1bbe8586 into 5a8204f).

@travisbot

This pull request fails (merged f31d8f26 into f98c32f).

@nulltoken
libgit2 member

Ok. This needs some valgrinding, but git_stash_save() and git_stash_foreach() tests pass :-) It was about time!

@nulltoken
libgit2 member

BTW, although it's not done yet, I 'd be really happy to benefit from some early feedback.

Please? Pretty please?

@nulltoken
libgit2 member

Ok. This needs some valgrinding

Done

@nulltoken
libgit2 member

@vmg Ready for review!

@nulltoken nulltoken referenced this pull request in libgit2/libgit2sharp Oct 17, 2012
Closed

Add support for Stashes #143

@nulltoken
libgit2 member

Rebased to cope with latest @arrbee's changes.

@vmg vmg and 1 other commented on an outdated diff Oct 26, 2012
include/git2/stash.h
+ *
+ * If the callback returns a non-zero value, this will stop looping.
+ *
+ * @param repo Repository where to find the stash.
+ *
+ * @param stash_cb Callback to invoke per found stashed state. The most recent
+ * stash state will be enumerated first.
+ *
+ * @param payload Extra parameter to callback function.
+ *
+ * @return 0 on success, GIT_EUSER on non-zero callback, or error code
+ */
+GIT_EXTERN(int) git_stash_foreach(
+ git_repository *repo,
+ int (*stash_cb)(
+ /* The position within the stash list.
@vmg
libgit2 member
vmg added a note Oct 26, 2012

This is gonna choke our documentation parser. You should get it out of here, typedef it and document it there.

@nulltoken
libgit2 member

Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vmg vmg commented on the diff Oct 26, 2012
src/stash.c
+ const char *message,
+ const git_commit *parent)
+{
+ git_tree *i_tree = NULL;
+ git_oid i_commit_oid;
+ git_buf msg = GIT_BUF_INIT;
+ int error = -1;
+
+ if (build_tree_from_index(&i_tree, index) < 0)
+ goto cleanup;
+
+ if (git_buf_printf(&msg, "index on %s\n", message) < 0)
+ goto cleanup;
+
+ if (git_commit_create(
+ &i_commit_oid,
@vmg
libgit2 member
vmg added a note Oct 26, 2012

Yeah, ok, let's not go this far. You can have more than one argument per line. :p

@nulltoken
libgit2 member

Yeah, ok, let's not go this far. You can have more than one argument per line. :p

Ok. I was just trying to make sure you could review this from your shiny phone ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken
libgit2 member

Rebased as well...

@arrbee arrbee commented on the diff Oct 29, 2012
include/git2/index.h
@@ -347,6 +347,14 @@ enum {
*/
GIT_EXTERN(int) git_index_read_tree(git_index *index, git_tree *tree);
+/**
+ * Get the repository this index relates to
+ *
@arrbee
libgit2 member
arrbee added a note Oct 29, 2012

Should probably note that is it possible for this to return NULL if this index object was just created by git_index_open() and not retrieved from a git_repository...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arrbee arrbee commented on the diff Oct 29, 2012
src/stash.c
+ delta->status);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int build_untracked_tree(
+ git_tree **tree_out,
+ git_index *index,
+ git_commit *i_commit,
+ uint32_t flags)
+{
+ git_tree *i_tree = NULL;
+ git_diff_list *diff = NULL;
+ git_diff_options opts = {0};
@arrbee
libgit2 member
arrbee added a note Oct 29, 2012

It is a somewhat minor optimization, but you may want to always set the diff flag GIT_DIFF_SKIP_BINARY_CHECK. Stash doesn't care and it will prevent diff from reading file contents to decide if the file is binary or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arrbee
libgit2 member

Ack. Ignore this comment. I realize my misunderstanding now...

@arrbee arrbee commented on the diff Oct 29, 2012
src/stash.c
+ goto cleanup;
+
+ max = git_reflog_entrycount(reflog);
+ for (i = 0; i < max; i++) {
+ entry = git_reflog_entry_byindex(reflog, max - i - 1);
+
+ if (callback(i,
+ git_reflog_entry_msg(entry),
+ git_reflog_entry_oidnew(entry),
+ payload)) {
+ error = GIT_EUSER;
+ goto cleanup;
+ }
+ }
+
+ error = 0;
@arrbee
libgit2 member
arrbee added a note Oct 29, 2012

I don't think there is a code path where we would get to this point and have error be non-zero. Can this line be removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arrbee
libgit2 member

Okay, well, I've looked over my comments and apart from a temporary misunderstanding I had about the uselessness of returning an "index" value from git_stash_save(), none of them are actually significant or need to be addressed right now.

I'd say we can merge this as is and if we need to do little cleanups down the line, they are easily incremental to the work that is already completely here.

@vmg
libgit2 member

@arrbee: I was wondering the same thing while reading your comment. Can you explain the misunderstanding to me?

@phkelley phkelley commented on the diff Oct 29, 2012
src/stash.c
+{
+ git_reference *head = NULL;
+ int error;
+
+ if ((error = retrieve_head(&head, repo)) < 0)
+ return error;
+
+ error = -1;
+
+ if (strcmp("HEAD", git_reference_name(head)) == 0)
+ git_buf_puts(stash_message, "(no branch): ");
+ else
+ git_buf_printf(
+ stash_message,
+ "%s: ",
+ git_reference_name(head) + strlen(GIT_REFS_HEADS_DIR));
@phkelley
libgit2 member

Should we add a length check here for git_reference_name's result to avoid running off the end of the buffer if it doesn't start with /refs/heads/ for some reason?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vmg
libgit2 member

Oh, you mean that save return an OID and no index because the stash is always saved at the end of the list?

@arrbee
libgit2 member

@vmg Exactly. There is no point in returning the index since it will always be 0...

@vmg
libgit2 member

Makes sense. Boat it.

@vmg vmg merged commit 8a1479a into libgit2:development Oct 29, 2012

1 check passed

Details default The Travis build passed
@phkelley phkelley commented on the diff Oct 29, 2012
src/stash.c
+ repo,
+ (flags & GIT_STASH_INCLUDE_UNTRACKED) == GIT_STASH_INCLUDE_UNTRACKED,
+ (flags & GIT_STASH_INCLUDE_IGNORED) == GIT_STASH_INCLUDE_IGNORED)) < 0)
+ goto cleanup;
+
+ error = -1;
+
+ if (git_repository_index(&index, repo) < 0)
+ goto cleanup;
+
+ if (commit_index(&i_commit, index, stasher, git_buf_cstr(&msg), b_commit) < 0)
+ goto cleanup;
+
+ if ((flags & GIT_STASH_INCLUDE_UNTRACKED || flags & GIT_STASH_INCLUDE_IGNORED)
+ && commit_untracked(&u_commit, index, stasher, git_buf_cstr(&msg), i_commit, flags) < 0)
+ goto cleanup;
@phkelley
libgit2 member

What happens in the scenario when one of these two flags is specified, but there isn't anything in the working copy that matches these criteria -- no ignored files to include, no untracked files to include... does commit_untracked fail in that case, or is there such a thing as an empty commit? it seems like we would not want to get an error code here and goto cleanup if the answer is that there are no items in either of these categories.

@vmg
libgit2 member
vmg added a note Oct 29, 2012

Hm, even if there are no untracked or ignored files to stash, there could be actual changes in tracked files that will get stashed, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@phkelley phkelley commented on the diff Oct 29, 2012
src/stash.c
+ && commit_untracked(&u_commit, index, stasher, git_buf_cstr(&msg), i_commit, flags) < 0)
+ goto cleanup;
+
+ if (prepare_worktree_commit_message(&msg, message) < 0)
+ goto cleanup;
+
+ if (commit_worktree(out, index, stasher, git_buf_cstr(&msg), i_commit, b_commit, u_commit) < 0)
+ goto cleanup;
+
+ git_buf_rtrim(&msg);
+ if (update_reflog(out, repo, stasher, git_buf_cstr(&msg)) < 0)
+ goto cleanup;
+
+ if (reset_index_and_workdir(
+ repo,
+ ((flags & GIT_STASH_KEEP_INDEX) == GIT_STASH_KEEP_INDEX) ?
@phkelley
libgit2 member

I see bitmask conditions written this way in quite a few places -- it's necessary to check for equality when the constant represents a mask of bits and you want to make sure all of them are set instead of just one, but your enumeration doesn't have any masks, just single bits, so flags & GIT_STASH_KEEP_INDEX suffices -- or maybe this is just personal preference. It's a nitpick, for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken
libgit2 member

@arrbee @phkelley @vmg Thanks a lot for the thorough review. ❤️
I'll soon push some updates to take them into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment