stash: partial stash specific files #10

gitkraken-jacobw · 2022-06-29T17:18:57Z

Copy-pasted from main libgit2 repo:

There's this concept called partial stashing in Git where users are able to stash desired files without having to stash and also reset the entire workdir. I based my implementation on tiennou's suggestion from libgit2#4725. This introduces options to git_stash_save where developers are able to supply the paths that they want to have stashed from the workdir. I could have gone further and have exposed a git_index* option as tiennou suggested, but I chose not to at this moment. Let me know if I should expose a git_index* option so that users can supply an arbitrary index to be stashed.

I also introduced GIT_STASH_KEEP_ALL, which prevents resetting the index and workdir once a stash has been completed.

AlexaXs

The code looks very good, just maybe it would help with the review if you could add different commits, each one explaining the changes applied. I can think of these two:

a commit explaining the fact that code of git_stash_save() has been moved inside the new function git_stash_save_with_opts() and that this function handles two different cases, case when no paths are specified and case with paths for partial stashing.
a commit explaining the changes in commit_worktree()

AlexaXs · 2022-07-13T14:44:38Z

include/git2/stash.h

+ * or error code.
+ */
+GIT_EXTERN(int) git_stash_save_with_opts(
+	git_oid *out, git_repository *repo, git_stash_save_options *opts);


opts should be const: const git_stash_save_options *opts

we should keep the format here of other functions: \n and \t for each argument of the function, same way as for example git_stash_apply

AlexaXs · 2022-07-13T14:48:51Z

src/stash.c

+static int stash_update_index_from_paths(
+	git_repository *repo,
+	git_index *index,
+	git_strarray *paths)


const git_strarray *paths

AlexaXs · 2022-07-13T14:49:33Z

src/stash.c

 	git_oid *w_commit_oid,
 	git_repository *repo,
 	const git_signature *stasher,
 	const char *message,
 	git_commit *i_commit,
 	git_commit *b_commit,
-	git_commit *u_commit)
+	git_commit *u_commit,
+	git_tree *tree)


const git_tree *tree

AlexaXs · 2022-07-13T14:50:09Z

src/stash.c

+static int ensure_there_are_changes_to_stash_paths(
+	git_repository *repo,
+	uint32_t flags,
+	git_strarray *paths)


const git_strarray *paths

AlexaXs · 2022-07-13T14:50:41Z

src/stash.c

+}
+
+int git_stash_save_with_opts(
+	git_oid *out, git_repository *repo, git_stash_save_options *opts)


const git_stash_save_options *opts

AlexaXs · 2022-07-13T15:02:26Z

src/stash.c

 	int error;

 	GIT_ASSERT_ARG(out);
 	GIT_ASSERT_ARG(repo);
-	GIT_ASSERT_ARG(stasher);



We also need to check if opts is NULL, and if so provide a default one. Similar to what normalize_apply_options does at the beginning.

Since it appears opts.stasher is required, should we instead do something like:

GIT_ASSERT_ARG(opts); GIT_ASSERT_ARG(opts->stasher); // not sure if this is the best way to validate properties

It seems GIT_ASSERT_ARG is used when NULL is not allowed for the argument, and checking for NULL and providing a default value is used when NULL is allowed. So I agree we can use GIT_ASSERT_ARG(opts) inside git_stash_save_with_opts.

The problem is when the user calls git_stash_save, GIT_STASH_SAVE_OPTIONS_INIT initializes stasher with NULL, in which case if we don't allow stasher to be NULL inside git_stash_save_with_opts it won't work.

I think a possible solution could be GIT_ASSERT_ARG(stasher) inside git_stash_save, and GIT_ASSERT_ARG(opts && opts->stasher) inside `git_stash_save_with_opts

AlexaXs · 2022-07-13T15:05:20Z

include/git2/stash.h

+ * @param version The struct version; pass `GIT_STASH_SAVE_OPTIONS_VERSION`.
+ * @return Zero on success; -1 on failure.
+ */
+GIT_EXTERN(int) git_stash_save_options_init(


would be good to add a test in tests/core/structinit.c similar to this one:

/* stash apply */ CHECK_MACRO_FUNC_INIT_EQUAL( \ git_stash_apply_options, GIT_STASH_APPLY_OPTIONS_VERSION, \ GIT_STASH_APPLY_OPTIONS_INIT, git_stash_apply_options_init);

AlexaXs · 2022-07-13T15:09:43Z

src/stash.c


 	if ((error = git_repository__ensure_not_bare(repo, "stash save")) < 0)
 		return error;

 	if ((error = retrieve_base_commit_and_message(&b_commit, &msg, repo)) < 0)
 		goto cleanup;

-	if ((error = ensure_there_are_changes_to_stash(repo, flags)) < 0)
+	if (opts->paths.count == 0 &&


Probably it could clear the code a bit if we added a flag like bool hasPaths assigning it the value of opts->paths.count > 0, and use that flag when testing this condition in the code: if (!hasPaths)...

AlexaXs · 2022-07-13T15:10:43Z

src/stash.c

-	if ((error = commit_worktree(out, repo, stasher, git_buf_cstr(&msg),
-				     i_commit, b_commit, u_commit)) < 0)
-		goto cleanup;
+	if (opts->paths.count == 0) {


if (!hasPaths)...

AlexaXs · 2022-07-13T15:22:02Z

src/stash.c

@@ -593,7 +754,10 @@ int git_stash_save(
 	git_commit_free(i_commit);
 	git_commit_free(b_commit);
 	git_commit_free(u_commit);
+	git_tree_free(tree);


_free() functions are safe with NULL, but since we are mixing code here for cases with paths and without paths, it'd be good to do this here too:

if (hasPaths) { git_tree_free(tree); git_reference_free(head); git_index_free(paths_index); }

we want to build a partial stash from an index in the future, so some functions have been reworked to allow for this

code has been moved from `git_stash_save` into `git_stash_save_with_opts` in order to handle cases where we want to partially stash using options or stash normally without partially stashed paths specified

AlexaXs suggested changes Jul 13, 2022

View reviewed changes

gitkraken-jacobw added 6 commits July 14, 2022 10:18

stash: introduce build_stash_commit_from_index

d6554d0

we want to build a partial stash from an index in the future, so some functions have been reworked to allow for this

stash: implement partial stashing by path

8280bb0

code has been moved from `git_stash_save` into `git_stash_save_with_opts` in order to handle cases where we want to partially stash using options or stash normally without partially stashed paths specified

stash: implement CI testing

65210e9

stash: better option validation for stash save

ec67f95

stash: add const to arguments

f2befe8

stash: test save options init

98faa3e

gitkraken-jacobw force-pushed the partialstashing branch from 7d7c7f2 to 98faa3e Compare July 14, 2022 17:19

gitkraken-jacobw mentioned this pull request Jul 27, 2022

stash: partial stash specific files Axosoft/libgit2#1

Merged

ianhattendorf merged commit 0ac7af7 into nodegit:libgit-next Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

stash: partial stash specific files #10

stash: partial stash specific files #10

Uh oh!

gitkraken-jacobw commented Jun 29, 2022

Uh oh!

AlexaXs left a comment

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

gitkraken-jacobw Jul 13, 2022

Uh oh!

AlexaXs Jul 14, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022

Uh oh!

AlexaXs Jul 13, 2022 •

edited

Loading

Uh oh!

Uh oh!

stash: partial stash specific files #10

stash: partial stash specific files #10

Uh oh!

Conversation

gitkraken-jacobw commented Jun 29, 2022

Uh oh!

AlexaXs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexaXs Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexaXs Jul 13, 2022 •

edited

Loading