git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [RFC PATCH 0/7] merge: learn --autostash
@ 2019-10-16 17:26 Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 1/7] Makefile: alphabetically sort += lists Denton Liu
                   ` (6 more replies)
  0 siblings, 7 replies; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Alban reported[1] that he expected merge to have an --autostash option,
just like rebase. Since there's not really any reason why merge can't
have it, let's implement it in this patchset.

The actual logic isn't too bad. That change can be found in the last
patch. The remainder is refactoring so that the change can be made
easily. One thing I would like some attention on is if apply_autostash()
is called in the right place. It's called somewhere above
close_object_store() and also in the --abort case, just like in rebase,
but I'm not quite sure if that's right.

Since this is an RFC, we're missing tests and documentation but I've
tested it and it seems to work reasonably well.

[1]: https://github.com/gitgitgadget/git/issues/394

Denton Liu (7):
  Makefile: alphabetically sort += lists
  autostash: extract read_one() from rebase
  autostash: extract apply_autostash() from rebase
  autostash: extract reset_head() from rebase
  autostash: extract perform_autostash() from rebase
  autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS
  merge: teach --autostash option

 Makefile         |  75 +++++++--------
 autostash.c      | 239 ++++++++++++++++++++++++++++++++++++++++++++++
 autostash.h      |  24 +++++
 builtin/merge.c  |  13 +++
 builtin/pull.c   |   9 +-
 builtin/rebase.c | 240 +----------------------------------------------
 t/t5520-pull.sh  |   8 --
 7 files changed, 323 insertions(+), 285 deletions(-)
 create mode 100644 autostash.c
 create mode 100644 autostash.h

-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
@ 2019-10-16 17:26 ` Denton Liu
  2019-10-17 18:07   ` Junio C Hamano
  2019-10-16 17:26 ` [RFC PATCH 2/7] autostash: extract read_one() from rebase Denton Liu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

There are many += lists in the Makefile and, over time, they have gotten
slightly out of order, alphabetically. Alphabetically sort all += lists
to bring them back in order.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Makefile | 75 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/Makefile b/Makefile
index de60c8e7aa..268a273df5 100644
--- a/Makefile
+++ b/Makefile
@@ -604,12 +604,12 @@ unexport CDPATH
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
+SCRIPT_SH += git-legacy-stash.sh
 SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-legacy-stash.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
@@ -617,8 +617,8 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--preserve-merges
-SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
+SCRIPT_LIB += git-sh-setup
 
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-archimport.perl
@@ -686,9 +686,9 @@ PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += imap-send.o
+PROGRAM_OBJS += remote-testsvn.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
-PROGRAM_OBJS += remote-testsvn.o
 
 # Binary suffix, set to .exe for Windows builds
 X =
@@ -709,9 +709,9 @@ TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-genzeros.o
+TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
-TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
 TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
@@ -735,8 +735,8 @@ TEST_BUILTINS_OBJS += test-revision-walking.o
 TEST_BUILTINS_OBJS += test-run-command.o
 TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
 TEST_BUILTINS_OBJS += test-serve-v2.o
-TEST_BUILTINS_OBJS += test-sha1.o
 TEST_BUILTINS_OBJS += test-sha1-array.o
+TEST_BUILTINS_OBJS += test-sha1.o
 TEST_BUILTINS_OBJS += test-sha256.o
 TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
@@ -746,10 +746,10 @@ TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
 TEST_BUILTINS_OBJS += test-trace2.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
-TEST_BUILTINS_OBJS += test-xml-encode.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
 TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
+TEST_BUILTINS_OBJS += test-xml-encode.o
 
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.
@@ -786,10 +786,10 @@ OTHER_PROGRAMS = git$X
 
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
-BINDIR_PROGRAMS_NEED_X += git-upload-pack
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
-BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-shell
+BINDIR_PROGRAMS_NEED_X += git-upload-archive
+BINDIR_PROGRAMS_NEED_X += git-upload-pack
 
 BINDIR_PROGRAMS_NO_X += git-cvsserver
 
@@ -827,11 +827,12 @@ LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
 LIB_OBJS += apply.o
-LIB_OBJS += archive.o
 LIB_OBJS += archive-tar.o
 LIB_OBJS += archive-zip.o
+LIB_OBJS += archive.o
 LIB_OBJS += argv-array.o
 LIB_OBJS += attr.o
+LIB_OBJS += autostash.o
 LIB_OBJS += base85.o
 LIB_OBJS += bisect.o
 LIB_OBJS += blame.o
@@ -845,9 +846,9 @@ LIB_OBJS += checkout.o
 LIB_OBJS += color.o
 LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
-LIB_OBJS += commit.o
 LIB_OBJS += commit-graph.o
 LIB_OBJS += commit-reach.o
+LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
@@ -861,17 +862,17 @@ LIB_OBJS += ctype.o
 LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += delta-islands.o
+LIB_OBJS += diff-delta.o
+LIB_OBJS += diff-lib.o
+LIB_OBJS += diff-no-index.o
+LIB_OBJS += diff.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
-LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-lib.o
-LIB_OBJS += diff-no-index.o
-LIB_OBJS += diff.o
-LIB_OBJS += dir.o
 LIB_OBJS += dir-iterator.o
+LIB_OBJS += dir.o
 LIB_OBJS += editor.o
 LIB_OBJS += entry.o
 LIB_OBJS += environment.o
@@ -889,7 +890,6 @@ LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hashmap.o
-LIB_OBJS += linear-assignment.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
@@ -899,9 +899,10 @@ LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
 LIB_OBJS += line-log.o
 LIB_OBJS += line-range.o
-LIB_OBJS += list-objects.o
-LIB_OBJS += list-objects-filter.o
+LIB_OBJS += linear-assignment.o
 LIB_OBJS += list-objects-filter-options.o
+LIB_OBJS += list-objects-filter.o
+LIB_OBJS += list-objects.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
 LIB_OBJS += log-tree.o
@@ -910,31 +911,31 @@ LIB_OBJS += mailinfo.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += mem-pool.o
-LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
+LIB_OBJS += merge.o
 LIB_OBJS += mergesort.o
 LIB_OBJS += midx.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += negotiator/default.o
 LIB_OBJS += negotiator/skipping.o
-LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
 LIB_OBJS += notes-utils.o
+LIB_OBJS += notes.o
 LIB_OBJS += object.o
 LIB_OBJS += oidmap.o
 LIB_OBJS += oidset.o
-LIB_OBJS += packfile.o
-LIB_OBJS += pack-bitmap.o
 LIB_OBJS += pack-bitmap-write.o
+LIB_OBJS += pack-bitmap.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-objects.o
 LIB_OBJS += pack-revindex.o
 LIB_OBJS += pack-write.o
+LIB_OBJS += packfile.o
 LIB_OBJS += pager.o
-LIB_OBJS += parse-options.o
 LIB_OBJS += parse-options-cb.o
+LIB_OBJS += parse-options.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
 LIB_OBJS += path.o
@@ -952,6 +953,7 @@ LIB_OBJS += range-diff.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
 LIB_OBJS += rebase-interactive.o
+LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
@@ -959,7 +961,6 @@ LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
 LIB_OBJS += refspec.o
-LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
 LIB_OBJS += replace-object.o
 LIB_OBJS += repo-settings.o
@@ -974,8 +975,8 @@ LIB_OBJS += serve.o
 LIB_OBJS += server-info.o
 LIB_OBJS += setup.o
 LIB_OBJS += sha1-array.o
-LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1-file.o
+LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1-name.o
 LIB_OBJS += shallow.o
 LIB_OBJS += sideband.o
@@ -985,9 +986,9 @@ LIB_OBJS += stable-qsort.o
 LIB_OBJS += strbuf.o
 LIB_OBJS += streaming.o
 LIB_OBJS += string-list.o
-LIB_OBJS += submodule.o
-LIB_OBJS += submodule-config.o
 LIB_OBJS += sub-process.o
+LIB_OBJS += submodule-config.o
+LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
@@ -1006,11 +1007,11 @@ LIB_OBJS += trace2/tr2_tgt_normal.o
 LIB_OBJS += trace2/tr2_tgt_perf.o
 LIB_OBJS += trace2/tr2_tls.o
 LIB_OBJS += trailer.o
-LIB_OBJS += transport.o
 LIB_OBJS += transport-helper.o
+LIB_OBJS += transport.o
 LIB_OBJS += tree-diff.o
-LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
+LIB_OBJS += tree.o
 LIB_OBJS += unpack-trees.o
 LIB_OBJS += upload-pack.o
 LIB_OBJS += url.o
@@ -1050,9 +1051,9 @@ BUILTIN_OBJS += builtin/checkout.o
 BUILTIN_OBJS += builtin/clean.o
 BUILTIN_OBJS += builtin/clone.o
 BUILTIN_OBJS += builtin/column.o
+BUILTIN_OBJS += builtin/commit-graph.o
 BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
-BUILTIN_OBJS += builtin/commit-graph.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
 BUILTIN_OBJS += builtin/credential.o
@@ -1083,13 +1084,13 @@ BUILTIN_OBJS += builtin/ls-remote.o
 BUILTIN_OBJS += builtin/ls-tree.o
 BUILTIN_OBJS += builtin/mailinfo.o
 BUILTIN_OBJS += builtin/mailsplit.o
-BUILTIN_OBJS += builtin/merge.o
 BUILTIN_OBJS += builtin/merge-base.o
 BUILTIN_OBJS += builtin/merge-file.o
 BUILTIN_OBJS += builtin/merge-index.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
 BUILTIN_OBJS += builtin/merge-tree.o
+BUILTIN_OBJS += builtin/merge.o
 BUILTIN_OBJS += builtin/mktag.o
 BUILTIN_OBJS += builtin/mktree.o
 BUILTIN_OBJS += builtin/multi-pack-index.o
@@ -1109,9 +1110,9 @@ BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
-BUILTIN_OBJS += builtin/remote.o
 BUILTIN_OBJS += builtin/remote-ext.o
 BUILTIN_OBJS += builtin/remote-fd.o
+BUILTIN_OBJS += builtin/remote.o
 BUILTIN_OBJS += builtin/repack.o
 BUILTIN_OBJS += builtin/replace.o
 BUILTIN_OBJS += builtin/rerere.o
@@ -2325,16 +2326,16 @@ reconfigure config.mak.autogen: config.status
 endif
 
 XDIFF_OBJS += xdiff/xdiffi.o
-XDIFF_OBJS += xdiff/xprepare.o
-XDIFF_OBJS += xdiff/xutils.o
 XDIFF_OBJS += xdiff/xemit.o
+XDIFF_OBJS += xdiff/xhistogram.o
 XDIFF_OBJS += xdiff/xmerge.o
 XDIFF_OBJS += xdiff/xpatience.o
-XDIFF_OBJS += xdiff/xhistogram.o
+XDIFF_OBJS += xdiff/xprepare.o
+XDIFF_OBJS += xdiff/xutils.o
 
+VCSSVN_OBJS += vcs-svn/fast_export.o
 VCSSVN_OBJS += vcs-svn/line_buffer.o
 VCSSVN_OBJS += vcs-svn/sliding_window.o
-VCSSVN_OBJS += vcs-svn/fast_export.o
 VCSSVN_OBJS += vcs-svn/svndiff.o
 VCSSVN_OBJS += vcs-svn/svndump.o
 
@@ -3143,9 +3144,9 @@ endif
 #
 ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
 ALL_COMMANDS += git
+ALL_COMMANDS += git-gui git-citool
 ALL_COMMANDS += gitk
 ALL_COMMANDS += gitweb
-ALL_COMMANDS += git-gui git-citool
 
 .PHONY: check-docs
 check-docs::
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 2/7] autostash: extract read_one() from rebase
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 1/7] Makefile: alphabetically sort += lists Denton Liu
@ 2019-10-16 17:26 ` Denton Liu
  2019-10-18  9:04   ` Phillip Wood
  2019-10-16 17:26 ` [RFC PATCH 3/7] autostash: extract apply_autostash() " Denton Liu
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Begin the process of lib-ifying the autostash code. In a future commit,
this will be used to implement `--autostash` in other builtins.

This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 autostash.c      | 12 ++++++++++++
 autostash.h      |  9 +++++++++
 builtin/rebase.c | 10 +---------
 3 files changed, 22 insertions(+), 9 deletions(-)
 create mode 100644 autostash.c
 create mode 100644 autostash.h

diff --git a/autostash.c b/autostash.c
new file mode 100644
index 0000000000..a6898e0fda
--- /dev/null
+++ b/autostash.c
@@ -0,0 +1,12 @@
+#include "git-compat-util.h"
+#include "autostash.h"
+#include "gettext.h"
+#include "strbuf.h"
+
+int read_one(const char *path, struct strbuf *buf)
+{
+	if (strbuf_read_file(buf, path, 0) < 0)
+		return error_errno(_("could not read '%s'"), path);
+	strbuf_trim_trailing_newline(buf);
+	return 0;
+}
diff --git a/autostash.h b/autostash.h
new file mode 100644
index 0000000000..4a8f504f12
--- /dev/null
+++ b/autostash.h
@@ -0,0 +1,9 @@
+#ifndef AUTOSTASH_H
+#define AUTOSTASH_H
+
+#include "strbuf.h"
+
+/* Read one file, then strip line endings */
+int read_one(const char *path, struct strbuf *buf);
+
+#endif
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a20582e72..9fd7de6b2f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -27,6 +27,7 @@
 #include "branch.h"
 #include "sequencer.h"
 #include "rebase-interactive.h"
+#include "autostash.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
@@ -561,15 +562,6 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o
 	return path.buf;
 }
 
-/* Read one file, then strip line endings */
-static int read_one(const char *path, struct strbuf *buf)
-{
-	if (strbuf_read_file(buf, path, 0) < 0)
-		return error_errno(_("could not read '%s'"), path);
-	strbuf_trim_trailing_newline(buf);
-	return 0;
-}
-
 /* Initialize the rebase options from the state directory. */
 static int read_basic_state(struct rebase_options *opts)
 {
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 3/7] autostash: extract apply_autostash() from rebase
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 1/7] Makefile: alphabetically sort += lists Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 2/7] autostash: extract read_one() from rebase Denton Liu
@ 2019-10-16 17:26 ` " Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 4/7] autostash: extract reset_head() " Denton Liu
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Continue the process of lib-ifying the autostash code. In a future
commit, this will be used to implement `--autostash` in other builtins.

This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 autostash.c      | 46 +++++++++++++++++++++++++++++++++++++++++++++
 autostash.h      |  2 ++
 builtin/rebase.c | 49 ++----------------------------------------------
 3 files changed, 50 insertions(+), 47 deletions(-)

diff --git a/autostash.c b/autostash.c
index a6898e0fda..62ec7a7c80 100644
--- a/autostash.c
+++ b/autostash.c
@@ -1,6 +1,8 @@
 #include "git-compat-util.h"
 #include "autostash.h"
+#include "dir.h"
 #include "gettext.h"
+#include "run-command.h"
 #include "strbuf.h"
 
 int read_one(const char *path, struct strbuf *buf)
@@ -10,3 +12,47 @@ int read_one(const char *path, struct strbuf *buf)
 	strbuf_trim_trailing_newline(buf);
 	return 0;
 }
+
+int apply_autostash(const char *path)
+{
+	struct strbuf autostash = STRBUF_INIT;
+	struct child_process stash_apply = CHILD_PROCESS_INIT;
+
+	if (!file_exists(path))
+		return 0;
+
+	if (read_one(path, &autostash))
+		return error(_("Could not read '%s'"), path);
+	/* Ensure that the hash is not mistaken for a number */
+	strbuf_addstr(&autostash, "^0");
+	argv_array_pushl(&stash_apply.args,
+			 "stash", "apply", autostash.buf, NULL);
+	stash_apply.git_cmd = 1;
+	stash_apply.no_stderr = stash_apply.no_stdout =
+		stash_apply.no_stdin = 1;
+	if (!run_command(&stash_apply))
+		printf(_("Applied autostash.\n"));
+	else {
+		struct argv_array args = ARGV_ARRAY_INIT;
+		int res = 0;
+
+		argv_array_pushl(&args,
+				 "stash", "store", "-m", "autostash", "-q",
+				 autostash.buf, NULL);
+		if (run_command_v_opt(args.argv, RUN_GIT_CMD))
+			res = error(_("Cannot store %s"), autostash.buf);
+		argv_array_clear(&args);
+		strbuf_release(&autostash);
+		if (res)
+			return res;
+
+		fprintf(stderr,
+			_("Applying autostash resulted in conflicts.\n"
+			  "Your changes are safe in the stash.\n"
+			  "You can run \"git stash pop\" or \"git stash drop\" "
+			  "at any time.\n"));
+	}
+
+	strbuf_release(&autostash);
+	return 0;
+}
diff --git a/autostash.h b/autostash.h
index 4a8f504f12..5f4e4bd22c 100644
--- a/autostash.h
+++ b/autostash.h
@@ -6,4 +6,6 @@
 /* Read one file, then strip line endings */
 int read_one(const char *path, struct strbuf *buf);
 
+int apply_autostash(const char *path);
+
 #endif
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9fd7de6b2f..661928d427 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -682,51 +682,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	return 0;
 }
 
-static int apply_autostash(struct rebase_options *opts)
-{
-	const char *path = state_dir_path("autostash", opts);
-	struct strbuf autostash = STRBUF_INIT;
-	struct child_process stash_apply = CHILD_PROCESS_INIT;
-
-	if (!file_exists(path))
-		return 0;
-
-	if (read_one(path, &autostash))
-		return error(_("Could not read '%s'"), path);
-	/* Ensure that the hash is not mistaken for a number */
-	strbuf_addstr(&autostash, "^0");
-	argv_array_pushl(&stash_apply.args,
-			 "stash", "apply", autostash.buf, NULL);
-	stash_apply.git_cmd = 1;
-	stash_apply.no_stderr = stash_apply.no_stdout =
-		stash_apply.no_stdin = 1;
-	if (!run_command(&stash_apply))
-		printf(_("Applied autostash.\n"));
-	else {
-		struct argv_array args = ARGV_ARRAY_INIT;
-		int res = 0;
-
-		argv_array_pushl(&args,
-				 "stash", "store", "-m", "autostash", "-q",
-				 autostash.buf, NULL);
-		if (run_command_v_opt(args.argv, RUN_GIT_CMD))
-			res = error(_("Cannot store %s"), autostash.buf);
-		argv_array_clear(&args);
-		strbuf_release(&autostash);
-		if (res)
-			return res;
-
-		fprintf(stderr,
-			_("Applying autostash resulted in conflicts.\n"
-			  "Your changes are safe in the stash.\n"
-			  "You can run \"git stash pop\" or \"git stash drop\" "
-			  "at any time.\n"));
-	}
-
-	strbuf_release(&autostash);
-	return 0;
-}
-
 static int finish_rebase(struct rebase_options *opts)
 {
 	struct strbuf dir = STRBUF_INIT;
@@ -734,7 +689,7 @@ static int finish_rebase(struct rebase_options *opts)
 	int ret = 0;
 
 	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
-	apply_autostash(opts);
+	apply_autostash(state_dir_path("autostash", opts));
 	close_object_store(the_repository->objects);
 	/*
 	 * We ignore errors in 'gc --auto', since the
@@ -1181,7 +1136,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	} else if (status == 2) {
 		struct strbuf dir = STRBUF_INIT;
 
-		apply_autostash(opts);
+		apply_autostash(state_dir_path("autostash", opts));
 		strbuf_addstr(&dir, opts->state_dir);
 		remove_dir_recursively(&dir, 0);
 		strbuf_release(&dir);
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 4/7] autostash: extract reset_head() from rebase
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
                   ` (2 preceding siblings ...)
  2019-10-16 17:26 ` [RFC PATCH 3/7] autostash: extract apply_autostash() " Denton Liu
@ 2019-10-16 17:26 ` " Denton Liu
  2019-10-18  9:37   ` Phillip Wood
  2019-10-16 17:26 ` [RFC PATCH 5/7] autostash: extract perform_autostash() " Denton Liu
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Begin the process of lib-ifying the autostash code. In a future commit,

This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.
this will be used to implement `--autostash` in other builtins.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 autostash.c      | 137 +++++++++++++++++++++++++++++++++++++++++++++++
 autostash.h      |  12 +++++
 builtin/rebase.c | 137 -----------------------------------------------
 3 files changed, 149 insertions(+), 137 deletions(-)

diff --git a/autostash.c b/autostash.c
index 62ec7a7c80..eb58e0c8a4 100644
--- a/autostash.c
+++ b/autostash.c
@@ -1,9 +1,17 @@
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
+
 #include "git-compat-util.h"
 #include "autostash.h"
+#include "cache-tree.h"
 #include "dir.h"
 #include "gettext.h"
+#include "lockfile.h"
+#include "refs.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "tree-walk.h"
+#include "tree.h"
+#include "unpack-trees.h"
 
 int read_one(const char *path, struct strbuf *buf)
 {
@@ -13,6 +21,135 @@ int read_one(const char *path, struct strbuf *buf)
 	return 0;
 }
 
+int reset_head(struct object_id *oid, const char *action,
+	       const char *switch_to_branch, unsigned flags,
+	       const char *reflog_orig_head, const char *reflog_head)
+{
+	unsigned detach_head = flags & RESET_HEAD_DETACH;
+	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
+	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
+	struct object_id head_oid;
+	struct tree_desc desc[2] = { { NULL }, { NULL } };
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	const char *reflog_action;
+	struct strbuf msg = STRBUF_INIT;
+	size_t prefix_len;
+	struct object_id *orig = NULL, oid_orig,
+		*old_orig = NULL, oid_old_orig;
+	int ret = 0, nr = 0;
+
+	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
+		ret = error(_("could not determine HEAD revision"));
+		goto leave_reset_head;
+	}
+
+	if (!oid)
+		oid = &head_oid;
+
+	if (refs_only)
+		goto reset_head_refs;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = the_repository->index;
+	unpack_tree_opts.dst_index = the_repository->index;
+	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
+	unpack_tree_opts.update = 1;
+	unpack_tree_opts.merge = 1;
+	if (!detach_head)
+		unpack_tree_opts.reset = 1;
+
+	if (repo_read_index_unmerged(the_repository) < 0) {
+		ret = error(_("could not read index"));
+		goto leave_reset_head;
+	}
+
+	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) {
+		ret = error(_("failed to find tree of %s"),
+			    oid_to_hex(&head_oid));
+		goto leave_reset_head;
+	}
+
+	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
+		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
+	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(the_repository, the_repository->index, tree);
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
+		ret = error(_("could not write index"));
+		goto leave_reset_head;
+	}
+
+reset_head_refs:
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
+	prefix_len = msg.len;
+
+	if (update_orig_head) {
+		if (!get_oid("ORIG_HEAD", &oid_old_orig))
+			old_orig = &oid_old_orig;
+		if (!get_oid("HEAD", &oid_orig)) {
+			orig = &oid_orig;
+			if (!reflog_orig_head) {
+				strbuf_addstr(&msg, "updating ORIG_HEAD");
+				reflog_orig_head = msg.buf;
+			}
+			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
+				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
+		} else if (old_orig)
+			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
+	}
+
+	if (!reflog_head) {
+		strbuf_setlen(&msg, prefix_len);
+		strbuf_addstr(&msg, "updating HEAD");
+		reflog_head = msg.buf;
+	}
+	if (!switch_to_branch)
+		ret = update_ref(reflog_head, "HEAD", oid, orig,
+				 detach_head ? REF_NO_DEREF : 0,
+				 UPDATE_REFS_MSG_ON_ERR);
+	else {
+		ret = update_ref(reflog_head, switch_to_branch, oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+		if (!ret)
+			ret = create_symref("HEAD", switch_to_branch,
+					    reflog_head);
+	}
+	if (run_hook)
+		run_hook_le(NULL, "post-checkout",
+			    oid_to_hex(orig ? orig : &null_oid),
+			    oid_to_hex(oid), "1", NULL);
+
+leave_reset_head:
+	strbuf_release(&msg);
+	rollback_lock_file(&lock);
+	while (nr)
+		free((void *)desc[--nr].buffer);
+	return ret;
+}
+
 int apply_autostash(const char *path)
 {
 	struct strbuf autostash = STRBUF_INIT;
diff --git a/autostash.h b/autostash.h
index 5f4e4bd22c..1406638166 100644
--- a/autostash.h
+++ b/autostash.h
@@ -6,6 +6,18 @@
 /* Read one file, then strip line endings */
 int read_one(const char *path, struct strbuf *buf);
 
+#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
+#define RESET_HEAD_DETACH (1<<0)
+#define RESET_HEAD_HARD (1<<1)
+#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
+#define RESET_HEAD_REFS_ONLY (1<<3)
+#define RESET_ORIG_HEAD (1<<4)
+
+int reset_head(struct object_id *oid, const char *action,
+	       const char *switch_to_branch, unsigned flags,
+	       const char *reflog_orig_head, const char *reflog_head);
+
 int apply_autostash(const char *path);
 
 #endif
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 661928d427..c3165896cc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -734,143 +734,6 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
-#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
-
-#define RESET_HEAD_DETACH (1<<0)
-#define RESET_HEAD_HARD (1<<1)
-#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
-#define RESET_HEAD_REFS_ONLY (1<<3)
-#define RESET_ORIG_HEAD (1<<4)
-
-static int reset_head(struct object_id *oid, const char *action,
-		      const char *switch_to_branch, unsigned flags,
-		      const char *reflog_orig_head, const char *reflog_head)
-{
-	unsigned detach_head = flags & RESET_HEAD_DETACH;
-	unsigned reset_hard = flags & RESET_HEAD_HARD;
-	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
-	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
-	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
-	struct object_id head_oid;
-	struct tree_desc desc[2] = { { NULL }, { NULL } };
-	struct lock_file lock = LOCK_INIT;
-	struct unpack_trees_options unpack_tree_opts;
-	struct tree *tree;
-	const char *reflog_action;
-	struct strbuf msg = STRBUF_INIT;
-	size_t prefix_len;
-	struct object_id *orig = NULL, oid_orig,
-		*old_orig = NULL, oid_old_orig;
-	int ret = 0, nr = 0;
-
-	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
-		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
-
-	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
-		ret = error(_("could not determine HEAD revision"));
-		goto leave_reset_head;
-	}
-
-	if (!oid)
-		oid = &head_oid;
-
-	if (refs_only)
-		goto reset_head_refs;
-
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = the_repository->index;
-	unpack_tree_opts.dst_index = the_repository->index;
-	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
-	unpack_tree_opts.update = 1;
-	unpack_tree_opts.merge = 1;
-	if (!detach_head)
-		unpack_tree_opts.reset = 1;
-
-	if (repo_read_index_unmerged(the_repository) < 0) {
-		ret = error(_("could not read index"));
-		goto leave_reset_head;
-	}
-
-	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) {
-		ret = error(_("failed to find tree of %s"),
-			    oid_to_hex(&head_oid));
-		goto leave_reset_head;
-	}
-
-	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
-		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
-		goto leave_reset_head;
-	}
-
-	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	tree = parse_tree_indirect(oid);
-	prime_cache_tree(the_repository, the_repository->index, tree);
-
-	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
-		ret = error(_("could not write index"));
-		goto leave_reset_head;
-	}
-
-reset_head_refs:
-	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
-	prefix_len = msg.len;
-
-	if (update_orig_head) {
-		if (!get_oid("ORIG_HEAD", &oid_old_orig))
-			old_orig = &oid_old_orig;
-		if (!get_oid("HEAD", &oid_orig)) {
-			orig = &oid_orig;
-			if (!reflog_orig_head) {
-				strbuf_addstr(&msg, "updating ORIG_HEAD");
-				reflog_orig_head = msg.buf;
-			}
-			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
-				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
-		} else if (old_orig)
-			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
-	}
-
-	if (!reflog_head) {
-		strbuf_setlen(&msg, prefix_len);
-		strbuf_addstr(&msg, "updating HEAD");
-		reflog_head = msg.buf;
-	}
-	if (!switch_to_branch)
-		ret = update_ref(reflog_head, "HEAD", oid, orig,
-				 detach_head ? REF_NO_DEREF : 0,
-				 UPDATE_REFS_MSG_ON_ERR);
-	else {
-		ret = update_ref(reflog_head, switch_to_branch, oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-		if (!ret)
-			ret = create_symref("HEAD", switch_to_branch,
-					    reflog_head);
-	}
-	if (run_hook)
-		run_hook_le(NULL, "post-checkout",
-			    oid_to_hex(orig ? orig : &null_oid),
-			    oid_to_hex(oid), "1", NULL);
-
-leave_reset_head:
-	strbuf_release(&msg);
-	rollback_lock_file(&lock);
-	while (nr)
-		free((void *)desc[--nr].buffer);
-	return ret;
-}
-
 static int move_to_original_branch(struct rebase_options *opts)
 {
 	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 5/7] autostash: extract perform_autostash() from rebase
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
                   ` (3 preceding siblings ...)
  2019-10-16 17:26 ` [RFC PATCH 4/7] autostash: extract reset_head() " Denton Liu
@ 2019-10-16 17:26 ` " Denton Liu
  2019-10-21 19:00   ` Johannes Schindelin
  2019-10-16 17:26 ` [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS Denton Liu
  2019-10-16 17:26 ` [RFC PATCH 7/7] merge: teach --autostash option Denton Liu
  6 siblings, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Continue the process of lib-ifying the autostash code. In a future
commit, this will be used to implement `--autostash` in other builtins.

This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 autostash.c      | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 autostash.h      |  1 +
 builtin/rebase.c | 44 +-------------------------------------------
 3 files changed, 48 insertions(+), 43 deletions(-)

diff --git a/autostash.c b/autostash.c
index eb58e0c8a4..722cf78b12 100644
--- a/autostash.c
+++ b/autostash.c
@@ -12,6 +12,7 @@
 #include "tree-walk.h"
 #include "tree.h"
 #include "unpack-trees.h"
+#include "wt-status.h"
 
 int read_one(const char *path, struct strbuf *buf)
 {
@@ -150,6 +151,51 @@ int reset_head(struct object_id *oid, const char *action,
 	return ret;
 }
 
+void perform_autostash(const char *path)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
+
+	fd = hold_locked_index(&lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
+	if (has_unstaged_changes(the_repository, 1) ||
+	    has_uncommitted_changes(the_repository, 1)) {
+		struct child_process stash = CHILD_PROCESS_INIT;
+		struct object_id oid;
+
+		argv_array_pushl(&stash.args,
+				 "stash", "create", "autostash", NULL);
+		stash.git_cmd = 1;
+		stash.no_stdin = 1;
+		if (capture_command(&stash, &buf, GIT_MAX_HEXSZ))
+			die(_("Cannot autostash"));
+		strbuf_trim_trailing_newline(&buf);
+		if (get_oid(buf.buf, &oid))
+			die(_("Unexpected stash response: '%s'"),
+			    buf.buf);
+		strbuf_reset(&buf);
+		strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
+
+		if (safe_create_leading_directories_const(path))
+			die(_("Could not create directory for '%s'"),
+			    path);
+		write_file(path, "%s", oid_to_hex(&oid));
+		printf(_("Created autostash: %s\n"), buf.buf);
+		if (reset_head(NULL, "reset --hard",
+			       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
+			die(_("could not reset --hard"));
+
+		if (discard_index(the_repository->index) < 0 ||
+			repo_read_index(the_repository) < 0)
+			die(_("could not read index"));
+	}
+}
+
 int apply_autostash(const char *path)
 {
 	struct strbuf autostash = STRBUF_INIT;
diff --git a/autostash.h b/autostash.h
index 1406638166..e08ccb9881 100644
--- a/autostash.h
+++ b/autostash.h
@@ -18,6 +18,7 @@ int reset_head(struct object_id *oid, const char *action,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head);
 
+void perform_autostash(const char *path);
 int apply_autostash(const char *path);
 
 #endif
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c3165896cc..c4decdfb5b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1797,49 +1797,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		die(_("could not read index"));
 
 	if (options.autostash) {
-		struct lock_file lock_file = LOCK_INIT;
-		int fd;
-
-		fd = hold_locked_index(&lock_file, 0);
-		refresh_cache(REFRESH_QUIET);
-		if (0 <= fd)
-			repo_update_index_if_able(the_repository, &lock_file);
-		rollback_lock_file(&lock_file);
-
-		if (has_unstaged_changes(the_repository, 1) ||
-		    has_uncommitted_changes(the_repository, 1)) {
-			const char *autostash =
-				state_dir_path("autostash", &options);
-			struct child_process stash = CHILD_PROCESS_INIT;
-			struct object_id oid;
-
-			argv_array_pushl(&stash.args,
-					 "stash", "create", "autostash", NULL);
-			stash.git_cmd = 1;
-			stash.no_stdin = 1;
-			strbuf_reset(&buf);
-			if (capture_command(&stash, &buf, GIT_MAX_HEXSZ))
-				die(_("Cannot autostash"));
-			strbuf_trim_trailing_newline(&buf);
-			if (get_oid(buf.buf, &oid))
-				die(_("Unexpected stash response: '%s'"),
-				    buf.buf);
-			strbuf_reset(&buf);
-			strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
-
-			if (safe_create_leading_directories_const(autostash))
-				die(_("Could not create directory for '%s'"),
-				    options.state_dir);
-			write_file(autostash, "%s", oid_to_hex(&oid));
-			printf(_("Created autostash: %s\n"), buf.buf);
-			if (reset_head(NULL, "reset --hard",
-				       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
-				die(_("could not reset --hard"));
-
-			if (discard_index(the_repository->index) < 0 ||
-				repo_read_index(the_repository) < 0)
-				die(_("could not read index"));
-		}
+		perform_autostash(state_dir_path("autostash", &options));
 	}
 
 	if (require_clean_work_tree(the_repository, "rebase",
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
                   ` (4 preceding siblings ...)
  2019-10-16 17:26 ` [RFC PATCH 5/7] autostash: extract perform_autostash() " Denton Liu
@ 2019-10-16 17:26 ` Denton Liu
  2019-10-18  9:41   ` Phillip Wood
  2019-10-16 17:26 ` [RFC PATCH 7/7] merge: teach --autostash option Denton Liu
  6 siblings, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Since autostash.c was recently introduced, we should avoid
USE_THE_INDEX_COMPATIBILITY_MACROS since we are trying to move away from
this in the rest of the codebase. Rewrite the autostash code to not need
it and remove its definition.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 autostash.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/autostash.c b/autostash.c
index 722cf78b12..0a1f00d2e5 100644
--- a/autostash.c
+++ b/autostash.c
@@ -1,5 +1,3 @@
-#define USE_THE_INDEX_COMPATIBILITY_MACROS
-
 #include "git-compat-util.h"
 #include "autostash.h"
 #include "cache-tree.h"
@@ -46,7 +44,7 @@ int reset_head(struct object_id *oid, const char *action,
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
 		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
 
-	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+	if (!refs_only && repo_hold_locked_index(the_repository, &lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;
 	}
@@ -157,8 +155,8 @@ void perform_autostash(const char *path)
 	struct lock_file lock_file = LOCK_INIT;
 	int fd;
 
-	fd = hold_locked_index(&lock_file, 0);
-	refresh_cache(REFRESH_QUIET);
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
 	if (0 <= fd)
 		repo_update_index_if_able(the_repository, &lock_file);
 	rollback_lock_file(&lock_file);
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 7/7] merge: teach --autostash option
  2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
                   ` (5 preceding siblings ...)
  2019-10-16 17:26 ` [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS Denton Liu
@ 2019-10-16 17:26 ` Denton Liu
  2019-10-18  9:46   ` Phillip Wood
  2019-10-21 19:10   ` Johannes Schindelin
  6 siblings, 2 replies; 23+ messages in thread
From: Denton Liu @ 2019-10-16 17:26 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

In rebase, one can pass the `--autostash` option to cause the worktree
to be automatically stashed before continuing with the rebase. This
option is missing in merge, however.

Implement the `--autostash` option and corresponding `merge.autoStash`
option in merge which stashes before merging and then pops after.

Reported-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/merge.c | 13 +++++++++++++
 builtin/pull.c  |  9 +++++----
 t/t5520-pull.sh |  8 --------
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 062e911441..d1a5eaad0d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -40,6 +40,7 @@
 #include "branch.h"
 #include "commit-reach.h"
 #include "wt-status.h"
+#include "autostash.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -58,6 +59,8 @@ static const char * const builtin_merge_usage[] = {
 	NULL
 };
 
+static GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
+
 static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = -1;
 static int option_edit = -1;
@@ -81,6 +84,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
+static int autostash;
 static int no_verify;
 
 static struct strategy all_strategy[] = {
@@ -285,6 +289,8 @@ static struct option builtin_merge_options[] = {
 	OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+	OPT_BOOL(0, "autostash", &autostash,
+	      N_("automatically stash/stash pop before and after")),
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
 	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
@@ -440,6 +446,7 @@ static void finish(struct commit *head_commit,
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
+	apply_autostash(merge_autostash());
 	if (squash) {
 		squash_message(head_commit, remoteheads);
 	} else {
@@ -631,6 +638,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	} else if (!strcmp(k, "commit.gpgsign")) {
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
+	} else if (!strcmp(k, "merge.autostash")) {
+		autostash = git_config_bool(k, v);
+		return 0;
 	}
 
 	status = fmt_merge_msg_config(k, v, cb);
@@ -724,6 +734,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		for (j = common; j; j = j->next)
 			commit_list_insert(j->item, &reversed);
 
+		if (autostash)
+			perform_autostash(merge_autostash());
 		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
@@ -1288,6 +1300,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		/* Invoke 'git reset --merge' */
 		ret = cmd_reset(nargc, nargv, prefix);
+		apply_autostash(merge_autostash());
 		goto done;
 	}
 
diff --git a/builtin/pull.c b/builtin/pull.c
index d25ff13a60..ee186781ae 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -183,7 +183,7 @@ static struct option pull_options[] = {
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
 	OPT_BOOL(0, "autostash", &opt_autostash,
-		N_("automatically stash/stash pop before and after rebase")),
+		N_("automatically stash/stash pop before and after")),
 	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
 		N_("merge strategy to use"),
 		0),
@@ -671,6 +671,10 @@ static int run_merge(void)
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	if (opt_autostash == 0)
+		argv_array_push(&args, "--no-autostash");
+	else if (opt_autostash == 1)
+		argv_array_push(&args, "--autostash");
 	if (opt_allow_unrelated_histories > 0)
 		argv_array_push(&args, "--allow-unrelated-histories");
 
@@ -918,9 +922,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	if (!opt_rebase && opt_autostash != -1)
-		die(_("--[no-]autostash option is only valid with --rebase."));
-
 	autostash = config_autostash;
 	if (opt_rebase) {
 		if (opt_autostash != -1)
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index cf4cc32fd0..75f162495a 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -365,14 +365,6 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 	test_pull_autostash_fail --rebase --no-autostash
 '
 
-for i in --autostash --no-autostash
-do
-	test_expect_success "pull $i (without --rebase) is illegal" '
-		test_must_fail git pull $i . copy 2>err &&
-		test_i18ngrep "only valid with --rebase" err
-	'
-done
-
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
-- 
2.23.0.897.g0a19638b1e


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-16 17:26 ` [RFC PATCH 1/7] Makefile: alphabetically sort += lists Denton Liu
@ 2019-10-17 18:07   ` Junio C Hamano
  2019-10-21 18:44     ` Johannes Schindelin
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2019-10-17 18:07 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Alban Gruin, Johannes Schindelin

Denton Liu <liu.denton@gmail.com> writes:

> There are many += lists in the Makefile and, over time, they have gotten
> slightly out of order, alphabetically. Alphabetically sort all += lists
> to bring them back in order.
> ...

Hmm.  I like the general thrust, but ...

>  LIB_OBJS += combine-diff.o
> -LIB_OBJS += commit.o
>  LIB_OBJS += commit-graph.o
>  LIB_OBJS += commit-reach.o
> +LIB_OBJS += commit.o

... I do not particularly see this change (there may be similar
ones) desirable.  I'd find it it be much more natural to sort
"commit-anything" after "commit", and that is true with or without
the common extension ".o" added to these entries.

In short, flipping these entries because '.' sorts later than '-' is
making the result look "less sorted", at least to me.

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 2/7] autostash: extract read_one() from rebase
  2019-10-16 17:26 ` [RFC PATCH 2/7] autostash: extract read_one() from rebase Denton Liu
@ 2019-10-18  9:04   ` Phillip Wood
  2019-10-21 18:46     ` Johannes Schindelin
  0 siblings, 1 reply; 23+ messages in thread
From: Phillip Wood @ 2019-10-18  9:04 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List
  Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Hi Denton

On 16/10/2019 18:26, Denton Liu wrote:
> Begin the process of lib-ifying the autostash code. In a future commit,
> this will be used to implement `--autostash` in other builtins.
> 
> This patch is best viewed with `--color-moved` and
> `--color-moved-ws=allow-indentation-change`.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   autostash.c      | 12 ++++++++++++
>   autostash.h      |  9 +++++++++
>   builtin/rebase.c | 10 +---------
>   3 files changed, 22 insertions(+), 9 deletions(-)
>   create mode 100644 autostash.c
>   create mode 100644 autostash.h
> 
> diff --git a/autostash.c b/autostash.c
> new file mode 100644
> index 0000000000..a6898e0fda
> --- /dev/null
> +++ b/autostash.c
> @@ -0,0 +1,12 @@
> +#include "git-compat-util.h"
> +#include "autostash.h"
> +#include "gettext.h"
> +#include "strbuf.h"
> +
> +int read_one(const char *path, struct strbuf *buf)
> +{
> +	if (strbuf_read_file(buf, path, 0) < 0)
> +		return error_errno(_("could not read '%s'"), path);
> +	strbuf_trim_trailing_newline(buf);
> +	return 0;
> +}

This looks like it's doing a similar job to read_oneliner() in 
sequencer.c, is it possible to make that public and use it instead? 
(There may be a difference if the file is missing but that function 
already takes a flag so it could probably be modified easily enough.)

Best Wishes

Phillip


> diff --git a/autostash.h b/autostash.h
> new file mode 100644
> index 0000000000..4a8f504f12
> --- /dev/null
> +++ b/autostash.h
> @@ -0,0 +1,9 @@
> +#ifndef AUTOSTASH_H
> +#define AUTOSTASH_H
> +
> +#include "strbuf.h"
> +
> +/* Read one file, then strip line endings */
> +int read_one(const char *path, struct strbuf *buf);
> +
> +#endif
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4a20582e72..9fd7de6b2f 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -27,6 +27,7 @@
>   #include "branch.h"
>   #include "sequencer.h"
>   #include "rebase-interactive.h"
> +#include "autostash.h"
>   
>   static char const * const builtin_rebase_usage[] = {
>   	N_("git rebase [-i] [options] [--exec <cmd>] "
> @@ -561,15 +562,6 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o
>   	return path.buf;
>   }
>   
> -/* Read one file, then strip line endings */
> -static int read_one(const char *path, struct strbuf *buf)
> -{
> -	if (strbuf_read_file(buf, path, 0) < 0)
> -		return error_errno(_("could not read '%s'"), path);
> -	strbuf_trim_trailing_newline(buf);
> -	return 0;
> -}
> -
>   /* Initialize the rebase options from the state directory. */
>   static int read_basic_state(struct rebase_options *opts)
>   {
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 4/7] autostash: extract reset_head() from rebase
  2019-10-16 17:26 ` [RFC PATCH 4/7] autostash: extract reset_head() " Denton Liu
@ 2019-10-18  9:37   ` Phillip Wood
  2019-10-21 18:56     ` Johannes Schindelin
  0 siblings, 1 reply; 23+ messages in thread
From: Phillip Wood @ 2019-10-18  9:37 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List
  Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Hi Denton

It's great to see this being libified, I've had it in mind to do this so 
we can avoid forking 'git checkout' in sequencer.c

On 16/10/2019 18:26, Denton Liu wrote:
> Begin the process of lib-ifying the autostash code. In a future commit,
> 
> This patch is best viewed with `--color-moved` and
> `--color-moved-ws=allow-indentation-change`.
> this will be used to implement `--autostash` in other builtins.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   autostash.c      | 137 +++++++++++++++++++++++++++++++++++++++++++++++
>   autostash.h      |  12 +++++
>   builtin/rebase.c | 137 -----------------------------------------------
>   3 files changed, 149 insertions(+), 137 deletions(-)
> 
> diff --git a/autostash.c b/autostash.c
> index 62ec7a7c80..eb58e0c8a4 100644
> --- a/autostash.c
> +++ b/autostash.c
> @@ -1,9 +1,17 @@
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS

It might be nicer to have a preparatory step that fixes this by adding a 
'struct repository *r' argument to the function in builtin/rebase.c 
before moving the function. You could also do the same for next patch 
and then move both functions together.

Best Wishes

Phillip

> +
>   #include "git-compat-util.h"
>   #include "autostash.h"
> +#include "cache-tree.h"
>   #include "dir.h"
>   #include "gettext.h"
> +#include "lockfile.h"
> +#include "refs.h"
>   #include "run-command.h"
>   #include "strbuf.h"
> +#include "tree-walk.h"
> +#include "tree.h"
> +#include "unpack-trees.h"
>   
>   int read_one(const char *path, struct strbuf *buf)
>   {
> @@ -13,6 +21,135 @@ int read_one(const char *path, struct strbuf *buf)
>   	return 0;
>   }
>   
> +int reset_head(struct object_id *oid, const char *action,
> +	       const char *switch_to_branch, unsigned flags,
> +	       const char *reflog_orig_head, const char *reflog_head)
> +{
> +	unsigned detach_head = flags & RESET_HEAD_DETACH;
> +	unsigned reset_hard = flags & RESET_HEAD_HARD;
> +	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> +	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> +	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
> +	struct object_id head_oid;
> +	struct tree_desc desc[2] = { { NULL }, { NULL } };
> +	struct lock_file lock = LOCK_INIT;
> +	struct unpack_trees_options unpack_tree_opts;
> +	struct tree *tree;
> +	const char *reflog_action;
> +	struct strbuf msg = STRBUF_INIT;
> +	size_t prefix_len;
> +	struct object_id *orig = NULL, oid_orig,
> +		*old_orig = NULL, oid_old_orig;
> +	int ret = 0, nr = 0;
> +
> +	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> +		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
> +
> +	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> +		ret = -1;
> +		goto leave_reset_head;
> +	}
> +
> +	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
> +		ret = error(_("could not determine HEAD revision"));
> +		goto leave_reset_head;
> +	}
> +
> +	if (!oid)
> +		oid = &head_oid;
> +
> +	if (refs_only)
> +		goto reset_head_refs;
> +
> +	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> +	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> +	unpack_tree_opts.head_idx = 1;
> +	unpack_tree_opts.src_index = the_repository->index;
> +	unpack_tree_opts.dst_index = the_repository->index;
> +	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
> +	unpack_tree_opts.update = 1;
> +	unpack_tree_opts.merge = 1;
> +	if (!detach_head)
> +		unpack_tree_opts.reset = 1;
> +
> +	if (repo_read_index_unmerged(the_repository) < 0) {
> +		ret = error(_("could not read index"));
> +		goto leave_reset_head;
> +	}
> +
> +	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) {
> +		ret = error(_("failed to find tree of %s"),
> +			    oid_to_hex(&head_oid));
> +		goto leave_reset_head;
> +	}
> +
> +	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
> +		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> +		goto leave_reset_head;
> +	}
> +
> +	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
> +		ret = -1;
> +		goto leave_reset_head;
> +	}
> +
> +	tree = parse_tree_indirect(oid);
> +	prime_cache_tree(the_repository, the_repository->index, tree);
> +
> +	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
> +		ret = error(_("could not write index"));
> +		goto leave_reset_head;
> +	}
> +
> +reset_head_refs:
> +	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> +	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> +	prefix_len = msg.len;
> +
> +	if (update_orig_head) {
> +		if (!get_oid("ORIG_HEAD", &oid_old_orig))
> +			old_orig = &oid_old_orig;
> +		if (!get_oid("HEAD", &oid_orig)) {
> +			orig = &oid_orig;
> +			if (!reflog_orig_head) {
> +				strbuf_addstr(&msg, "updating ORIG_HEAD");
> +				reflog_orig_head = msg.buf;
> +			}
> +			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
> +				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
> +		} else if (old_orig)
> +			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
> +	}
> +
> +	if (!reflog_head) {
> +		strbuf_setlen(&msg, prefix_len);
> +		strbuf_addstr(&msg, "updating HEAD");
> +		reflog_head = msg.buf;
> +	}
> +	if (!switch_to_branch)
> +		ret = update_ref(reflog_head, "HEAD", oid, orig,
> +				 detach_head ? REF_NO_DEREF : 0,
> +				 UPDATE_REFS_MSG_ON_ERR);
> +	else {
> +		ret = update_ref(reflog_head, switch_to_branch, oid,
> +				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +		if (!ret)
> +			ret = create_symref("HEAD", switch_to_branch,
> +					    reflog_head);
> +	}
> +	if (run_hook)
> +		run_hook_le(NULL, "post-checkout",
> +			    oid_to_hex(orig ? orig : &null_oid),
> +			    oid_to_hex(oid), "1", NULL);
> +
> +leave_reset_head:
> +	strbuf_release(&msg);
> +	rollback_lock_file(&lock);
> +	while (nr)
> +		free((void *)desc[--nr].buffer);
> +	return ret;
> +}
> +
>   int apply_autostash(const char *path)
>   {
>   	struct strbuf autostash = STRBUF_INIT;
> diff --git a/autostash.h b/autostash.h
> index 5f4e4bd22c..1406638166 100644
> --- a/autostash.h
> +++ b/autostash.h
> @@ -6,6 +6,18 @@
>   /* Read one file, then strip line endings */
>   int read_one(const char *path, struct strbuf *buf);
>   
> +#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
> +
> +#define RESET_HEAD_DETACH (1<<0)
> +#define RESET_HEAD_HARD (1<<1)
> +#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
> +#define RESET_HEAD_REFS_ONLY (1<<3)
> +#define RESET_ORIG_HEAD (1<<4)
> +
> +int reset_head(struct object_id *oid, const char *action,
> +	       const char *switch_to_branch, unsigned flags,
> +	       const char *reflog_orig_head, const char *reflog_head);
> +
>   int apply_autostash(const char *path);
>   
>   #endif
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 661928d427..c3165896cc 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -734,143 +734,6 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
>   	}
>   }
>   
> -#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
> -
> -#define RESET_HEAD_DETACH (1<<0)
> -#define RESET_HEAD_HARD (1<<1)
> -#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
> -#define RESET_HEAD_REFS_ONLY (1<<3)
> -#define RESET_ORIG_HEAD (1<<4)
> -
> -static int reset_head(struct object_id *oid, const char *action,
> -		      const char *switch_to_branch, unsigned flags,
> -		      const char *reflog_orig_head, const char *reflog_head)
> -{
> -	unsigned detach_head = flags & RESET_HEAD_DETACH;
> -	unsigned reset_hard = flags & RESET_HEAD_HARD;
> -	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> -	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> -	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
> -	struct object_id head_oid;
> -	struct tree_desc desc[2] = { { NULL }, { NULL } };
> -	struct lock_file lock = LOCK_INIT;
> -	struct unpack_trees_options unpack_tree_opts;
> -	struct tree *tree;
> -	const char *reflog_action;
> -	struct strbuf msg = STRBUF_INIT;
> -	size_t prefix_len;
> -	struct object_id *orig = NULL, oid_orig,
> -		*old_orig = NULL, oid_old_orig;
> -	int ret = 0, nr = 0;
> -
> -	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> -		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
> -
> -	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> -		ret = -1;
> -		goto leave_reset_head;
> -	}
> -
> -	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
> -		ret = error(_("could not determine HEAD revision"));
> -		goto leave_reset_head;
> -	}
> -
> -	if (!oid)
> -		oid = &head_oid;
> -
> -	if (refs_only)
> -		goto reset_head_refs;
> -
> -	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> -	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> -	unpack_tree_opts.head_idx = 1;
> -	unpack_tree_opts.src_index = the_repository->index;
> -	unpack_tree_opts.dst_index = the_repository->index;
> -	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
> -	unpack_tree_opts.update = 1;
> -	unpack_tree_opts.merge = 1;
> -	if (!detach_head)
> -		unpack_tree_opts.reset = 1;
> -
> -	if (repo_read_index_unmerged(the_repository) < 0) {
> -		ret = error(_("could not read index"));
> -		goto leave_reset_head;
> -	}
> -
> -	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) {
> -		ret = error(_("failed to find tree of %s"),
> -			    oid_to_hex(&head_oid));
> -		goto leave_reset_head;
> -	}
> -
> -	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
> -		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> -		goto leave_reset_head;
> -	}
> -
> -	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
> -		ret = -1;
> -		goto leave_reset_head;
> -	}
> -
> -	tree = parse_tree_indirect(oid);
> -	prime_cache_tree(the_repository, the_repository->index, tree);
> -
> -	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
> -		ret = error(_("could not write index"));
> -		goto leave_reset_head;
> -	}
> -
> -reset_head_refs:
> -	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> -	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> -	prefix_len = msg.len;
> -
> -	if (update_orig_head) {
> -		if (!get_oid("ORIG_HEAD", &oid_old_orig))
> -			old_orig = &oid_old_orig;
> -		if (!get_oid("HEAD", &oid_orig)) {
> -			orig = &oid_orig;
> -			if (!reflog_orig_head) {
> -				strbuf_addstr(&msg, "updating ORIG_HEAD");
> -				reflog_orig_head = msg.buf;
> -			}
> -			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
> -				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
> -		} else if (old_orig)
> -			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
> -	}
> -
> -	if (!reflog_head) {
> -		strbuf_setlen(&msg, prefix_len);
> -		strbuf_addstr(&msg, "updating HEAD");
> -		reflog_head = msg.buf;
> -	}
> -	if (!switch_to_branch)
> -		ret = update_ref(reflog_head, "HEAD", oid, orig,
> -				 detach_head ? REF_NO_DEREF : 0,
> -				 UPDATE_REFS_MSG_ON_ERR);
> -	else {
> -		ret = update_ref(reflog_head, switch_to_branch, oid,
> -				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> -		if (!ret)
> -			ret = create_symref("HEAD", switch_to_branch,
> -					    reflog_head);
> -	}
> -	if (run_hook)
> -		run_hook_le(NULL, "post-checkout",
> -			    oid_to_hex(orig ? orig : &null_oid),
> -			    oid_to_hex(oid), "1", NULL);
> -
> -leave_reset_head:
> -	strbuf_release(&msg);
> -	rollback_lock_file(&lock);
> -	while (nr)
> -		free((void *)desc[--nr].buffer);
> -	return ret;
> -}
> -
>   static int move_to_original_branch(struct rebase_options *opts)
>   {
>   	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS
  2019-10-16 17:26 ` [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS Denton Liu
@ 2019-10-18  9:41   ` Phillip Wood
  0 siblings, 0 replies; 23+ messages in thread
From: Phillip Wood @ 2019-10-18  9:41 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List
  Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Hi Denton

On 16/10/2019 18:26, Denton Liu wrote:
> Since autostash.c was recently introduced, we should avoid
> USE_THE_INDEX_COMPATIBILITY_MACROS since we are trying to move away from
> this in the rest of the codebase. Rewrite the autostash code to not need
> it and remove its definition.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   autostash.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/autostash.c b/autostash.c
> index 722cf78b12..0a1f00d2e5 100644
> --- a/autostash.c
> +++ b/autostash.c
> @@ -1,5 +1,3 @@
> -#define USE_THE_INDEX_COMPATIBILITY_MACROS
> -
>   #include "git-compat-util.h"
>   #include "autostash.h"
>   #include "cache-tree.h"
> @@ -46,7 +44,7 @@ int reset_head(struct object_id *oid, const char *action,
>   	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
>   		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
>   
> -	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> +	if (!refs_only && repo_hold_locked_index(the_repository, &lock, LOCK_REPORT_ON_ERROR) < 0) {

As I understand it the reason for moving away from hold_locked_index() 
is that it relies on a global variable. While replacing it with an 
explicit reference the the_repository removes the implicit dependency on 
global state, it does not move us away from depending on a global 
variable. I think it would be nicer to change these functions to take a 
`struct repository*` before moving them.

Best Wishes

Phillip
>   		ret = -1;
>   		goto leave_reset_head;
>   	}
> @@ -157,8 +155,8 @@ void perform_autostash(const char *path)
>   	struct lock_file lock_file = LOCK_INIT;
>   	int fd;
>   
> -	fd = hold_locked_index(&lock_file, 0);
> -	refresh_cache(REFRESH_QUIET);
> +	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
> +	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
>   	if (0 <= fd)
>   		repo_update_index_if_able(the_repository, &lock_file);
>   	rollback_lock_file(&lock_file);
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 7/7] merge: teach --autostash option
  2019-10-16 17:26 ` [RFC PATCH 7/7] merge: teach --autostash option Denton Liu
@ 2019-10-18  9:46   ` Phillip Wood
  2019-10-21 19:10   ` Johannes Schindelin
  1 sibling, 0 replies; 23+ messages in thread
From: Phillip Wood @ 2019-10-18  9:46 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List
  Cc: Alban Gruin, Johannes Schindelin, Junio C Hamano

Hi Denton

On 16/10/2019 18:26, Denton Liu wrote:
> In rebase, one can pass the `--autostash` option to cause the worktree
> to be automatically stashed before continuing with the rebase. This
> option is missing in merge, however.

It might be helpful to say why this option is useful. I can see one 
might want to stash before doing `git pull` in the same way as one might 
before a rebase but what's the motivation for doing it before a normal 
merge?

> 
> Implement the `--autostash` option and corresponding `merge.autoStash`
> option in merge which stashes before merging and then pops after.
> 
> Reported-by: Alban Gruin <alban.gruin@gmail.com>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   builtin/merge.c | 13 +++++++++++++
>   builtin/pull.c  |  9 +++++----
>   t/t5520-pull.sh |  8 --------
>   3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 062e911441..d1a5eaad0d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -40,6 +40,7 @@
>   #include "branch.h"
>   #include "commit-reach.h"
>   #include "wt-status.h"
> +#include "autostash.h"
>   
>   #define DEFAULT_TWOHEAD (1<<0)
>   #define DEFAULT_OCTOPUS (1<<1)
> @@ -58,6 +59,8 @@ static const char * const builtin_merge_usage[] = {
>   	NULL
>   };
>   
> +static GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
> +
>   static int show_diffstat = 1, shortlog_len = -1, squash;
>   static int option_commit = -1;
>   static int option_edit = -1;
> @@ -81,6 +84,7 @@ static int show_progress = -1;
>   static int default_to_upstream = 1;
>   static int signoff;
>   static const char *sign_commit;
> +static int autostash;
>   static int no_verify;
>   
>   static struct strategy all_strategy[] = {
> @@ -285,6 +289,8 @@ static struct option builtin_merge_options[] = {
>   	OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
>   	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>   	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> +	OPT_BOOL(0, "autostash", &autostash,
> +	      N_("automatically stash/stash pop before and after")),
>   	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
>   	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
>   	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
> @@ -440,6 +446,7 @@ static void finish(struct commit *head_commit,
>   		strbuf_addf(&reflog_message, "%s: %s",
>   			getenv("GIT_REFLOG_ACTION"), msg);
>   	}
> +	apply_autostash(merge_autostash());
>   	if (squash) {
>   		squash_message(head_commit, remoteheads);
>   	} else {
> @@ -631,6 +638,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>   	} else if (!strcmp(k, "commit.gpgsign")) {
>   		sign_commit = git_config_bool(k, v) ? "" : NULL;
>   		return 0;
> +	} else if (!strcmp(k, "merge.autostash")) {
> +		autostash = git_config_bool(k, v);
> +		return 0;
>   	}
>   
>   	status = fmt_merge_msg_config(k, v, cb);
> @@ -724,6 +734,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>   		for (j = common; j; j = j->next)
>   			commit_list_insert(j->item, &reversed);
>   
> +		if (autostash)
> +			perform_autostash(merge_autostash());
>   		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>   		clean = merge_recursive(&o, head,
>   				remoteheads->item, reversed, &result);
> @@ -1288,6 +1300,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>   
>   		/* Invoke 'git reset --merge' */
>   		ret = cmd_reset(nargc, nargv, prefix);
> +		apply_autostash(merge_autostash());
>   		goto done;
>   	}
>   
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d25ff13a60..ee186781ae 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -183,7 +183,7 @@ static struct option pull_options[] = {
>   		N_("verify that the named commit has a valid GPG signature"),
>   		PARSE_OPT_NOARG),
>   	OPT_BOOL(0, "autostash", &opt_autostash,
> -		N_("automatically stash/stash pop before and after rebase")),
> +		N_("automatically stash/stash pop before and after")),

I've not looked closely at the code in this patch but noticed this. I 
think it would read better if it said

     automatically stash/stash pop before and after pulling

Best Wishes

Phillip

>   	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
>   		N_("merge strategy to use"),
>   		0),
> @@ -671,6 +671,10 @@ static int run_merge(void)
>   	argv_array_pushv(&args, opt_strategy_opts.argv);
>   	if (opt_gpg_sign)
>   		argv_array_push(&args, opt_gpg_sign);
> +	if (opt_autostash == 0)
> +		argv_array_push(&args, "--no-autostash");
> +	else if (opt_autostash == 1)
> +		argv_array_push(&args, "--autostash");
>   	if (opt_allow_unrelated_histories > 0)
>   		argv_array_push(&args, "--allow-unrelated-histories");
>   
> @@ -918,9 +922,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   	if (get_oid("HEAD", &orig_head))
>   		oidclr(&orig_head);
>   
> -	if (!opt_rebase && opt_autostash != -1)
> -		die(_("--[no-]autostash option is only valid with --rebase."));
> -
>   	autostash = config_autostash;
>   	if (opt_rebase) {
>   		if (opt_autostash != -1)
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index cf4cc32fd0..75f162495a 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -365,14 +365,6 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
>   	test_pull_autostash_fail --rebase --no-autostash
>   '
>   
> -for i in --autostash --no-autostash
> -do
> -	test_expect_success "pull $i (without --rebase) is illegal" '
> -		test_must_fail git pull $i . copy 2>err &&
> -		test_i18ngrep "only valid with --rebase" err
> -	'
> -done
> -
>   test_expect_success 'pull.rebase' '
>   	git reset --hard before-rebase &&
>   	test_config pull.rebase true &&
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-17 18:07   ` Junio C Hamano
@ 2019-10-21 18:44     ` Johannes Schindelin
  2019-10-21 18:53       ` Denton Liu
  2019-10-21 19:49       ` Junio C Hamano
  0 siblings, 2 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-21 18:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List, Alban Gruin

Hi Junio,

On Fri, 18 Oct 2019, Junio C Hamano wrote:

> Denton Liu <liu.denton@gmail.com> writes:
>
> > There are many += lists in the Makefile and, over time, they have gotten
> > slightly out of order, alphabetically. Alphabetically sort all += lists
> > to bring them back in order.
> > ...
>
> Hmm.  I like the general thrust, but ...
>
> >  LIB_OBJS += combine-diff.o
> > -LIB_OBJS += commit.o
> >  LIB_OBJS += commit-graph.o
> >  LIB_OBJS += commit-reach.o
> > +LIB_OBJS += commit.o
>
> ... I do not particularly see this change (there may be similar
> ones) desirable.  I'd find it it be much more natural to sort
> "commit-anything" after "commit", and that is true with or without
> the common extension ".o" added to these entries.
>
> In short, flipping these entries because '.' sorts later than '-' is
> making the result look "less sorted", at least to me.

The problem with this argument is that it disagrees with ASCII, as `-`
has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically
_larger_.

So Denton's patch does the correct thing.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 2/7] autostash: extract read_one() from rebase
  2019-10-18  9:04   ` Phillip Wood
@ 2019-10-21 18:46     ` Johannes Schindelin
  0 siblings, 0 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-21 18:46 UTC (permalink / raw)
  To: phillip.wood; +Cc: Denton Liu, Git Mailing List, Alban Gruin, Junio C Hamano

Hi,

On Fri, 18 Oct 2019, Phillip Wood wrote:

> Hi Denton
>
> On 16/10/2019 18:26, Denton Liu wrote:
> > Begin the process of lib-ifying the autostash code. In a future commit,
> > this will be used to implement `--autostash` in other builtins.
> >
> > This patch is best viewed with `--color-moved` and
> > `--color-moved-ws=allow-indentation-change`.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >   autostash.c      | 12 ++++++++++++
> >   autostash.h      |  9 +++++++++
> >   builtin/rebase.c | 10 +---------
> >   3 files changed, 22 insertions(+), 9 deletions(-)
> >   create mode 100644 autostash.c
> >   create mode 100644 autostash.h
> >
> > diff --git a/autostash.c b/autostash.c
> > new file mode 100644
> > index 0000000000..a6898e0fda
> > --- /dev/null
> > +++ b/autostash.c
> > @@ -0,0 +1,12 @@
> > +#include "git-compat-util.h"
> > +#include "autostash.h"
> > +#include "gettext.h"
> > +#include "strbuf.h"
> > +
> > +int read_one(const char *path, struct strbuf *buf)
> > +{
> > +	if (strbuf_read_file(buf, path, 0) < 0)
> > +		return error_errno(_("could not read '%s'"), path);
> > +	strbuf_trim_trailing_newline(buf);
> > +	return 0;
> > +}
>
> This looks like it's doing a similar job to read_oneliner() in sequencer.c, is
> it possible to make that public and use it instead? (There may be a difference
> if the file is missing but that function already takes a flag so it could
> probably be modified easily enough.)

Oh, I would _love_ to see those two functions reconciled.

Thanks,
Dscho

>
> Best Wishes
>
> Phillip
>
>
> > diff --git a/autostash.h b/autostash.h
> > new file mode 100644
> > index 0000000000..4a8f504f12
> > --- /dev/null
> > +++ b/autostash.h
> > @@ -0,0 +1,9 @@
> > +#ifndef AUTOSTASH_H
> > +#define AUTOSTASH_H
> > +
> > +#include "strbuf.h"
> > +
> > +/* Read one file, then strip line endings */
> > +int read_one(const char *path, struct strbuf *buf);
> > +
> > +#endif
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 4a20582e72..9fd7de6b2f 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -27,6 +27,7 @@
> >   #include "branch.h"
> >   #include "sequencer.h"
> >   #include "rebase-interactive.h"
> > +#include "autostash.h"
> >
> >   static char const * const builtin_rebase_usage[] = {
> >   	N_("git rebase [-i] [options] [--exec <cmd>] "
> > @@ -561,15 +562,6 @@ static const char *state_dir_path(const char *filename,
> > struct rebase_options *o
> >   	return path.buf;
> >   }
> >
> > -/* Read one file, then strip line endings */
> > -static int read_one(const char *path, struct strbuf *buf)
> > -{
> > -	if (strbuf_read_file(buf, path, 0) < 0)
> > -		return error_errno(_("could not read '%s'"), path);
> > -	strbuf_trim_trailing_newline(buf);
> > -	return 0;
> > -}
> > -
> >   /* Initialize the rebase options from the state directory. */
> >   static int read_basic_state(struct rebase_options *opts)
> >   {
> >
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-21 18:44     ` Johannes Schindelin
@ 2019-10-21 18:53       ` Denton Liu
  2019-10-22 23:33         ` Johannes Schindelin
  2019-10-21 19:49       ` Junio C Hamano
  1 sibling, 1 reply; 23+ messages in thread
From: Denton Liu @ 2019-10-21 18:53 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List, Alban Gruin

Hi Johannes,

On Mon, Oct 21, 2019 at 08:44:40PM +0200, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Fri, 18 Oct 2019, Junio C Hamano wrote:
> 
> > Denton Liu <liu.denton@gmail.com> writes:
> >
> > > There are many += lists in the Makefile and, over time, they have gotten
> > > slightly out of order, alphabetically. Alphabetically sort all += lists
> > > to bring them back in order.
> > > ...
> >
> > Hmm.  I like the general thrust, but ...
> >
> > >  LIB_OBJS += combine-diff.o
> > > -LIB_OBJS += commit.o
> > >  LIB_OBJS += commit-graph.o
> > >  LIB_OBJS += commit-reach.o
> > > +LIB_OBJS += commit.o
> >
> > ... I do not particularly see this change (there may be similar
> > ones) desirable.  I'd find it it be much more natural to sort
> > "commit-anything" after "commit", and that is true with or without
> > the common extension ".o" added to these entries.
> >
> > In short, flipping these entries because '.' sorts later than '-' is
> > making the result look "less sorted", at least to me.
> 
> The problem with this argument is that it disagrees with ASCII, as `-`
> has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically
> _larger_.
> 
> So Denton's patch does the correct thing.

I actually agree with Junio on this one. Without the prefixes, "commit"
would go before "commit-graph" so I think it would make more sense to
order with the prefixes removed instead of taking the naive ordering by
just sorting each block.

Thanks,

Denton

> 
> Ciao,
> Dscho

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 4/7] autostash: extract reset_head() from rebase
  2019-10-18  9:37   ` Phillip Wood
@ 2019-10-21 18:56     ` Johannes Schindelin
  0 siblings, 0 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-21 18:56 UTC (permalink / raw)
  To: phillip.wood; +Cc: Denton Liu, Git Mailing List, Alban Gruin, Junio C Hamano

Hi,

[sorry, Phillip, my reply-all fu deserts me today, apparently.]


On Fri, 18 Oct 2019, Phillip Wood wrote:

> Hi Denton
>
> It's great to see this being libified, I've had it in mind to do this so we
> can avoid forking 'git checkout' in sequencer.c
>
> On 16/10/2019 18:26, Denton Liu wrote:
> > Begin the process of lib-ifying the autostash code. In a future commit,
> >
> > This patch is best viewed with `--color-moved` and
> > `--color-moved-ws=allow-indentation-change`.
> > this will be used to implement `--autostash` in other builtins.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >   autostash.c      | 137 +++++++++++++++++++++++++++++++++++++++++++++++
> >   autostash.h      |  12 +++++
> >   builtin/rebase.c | 137 -----------------------------------------------
> >   3 files changed, 149 insertions(+), 137 deletions(-)
> >
> > diff --git a/autostash.c b/autostash.c
> > index 62ec7a7c80..eb58e0c8a4 100644
> > --- a/autostash.c
> > +++ b/autostash.c
> > @@ -1,9 +1,17 @@
> > +#define USE_THE_INDEX_COMPATIBILITY_MACROS
>
> It might be nicer to have a preparatory step that fixes this by adding a
> 'struct repository *r' argument to the function in builtin/rebase.c before
> moving the function. You could also do the same for next patch and then move
> both functions together.

In addition to that, I think that `reset_head()`

- should live in its own file, not be hidden in `autostash.c`,

- its default reflog action should _not_ be "rebase".

- ideally be made the working horse of `builtin/reset.c`,

- in addition to that `struct repository *r`, it should probably accept
  a `struct index_state *index` and a `const char *worktree_directory`,
  but that can easily come in the future, as needed.

Thanks,
Dscho


>
> Best Wishes
>
> Phillip
>
> > +
> >   #include "git-compat-util.h"
> >   #include "autostash.h"
> > +#include "cache-tree.h"
> >   #include "dir.h"
> >   #include "gettext.h"
> > +#include "lockfile.h"
> > +#include "refs.h"
> >   #include "run-command.h"
> >   #include "strbuf.h"
> > +#include "tree-walk.h"
> > +#include "tree.h"
> > +#include "unpack-trees.h"
> >
> >   int read_one(const char *path, struct strbuf *buf)
> >   {
> > @@ -13,6 +21,135 @@ int read_one(const char *path, struct strbuf *buf)
> >   	return 0;
> >   }
> >
> > +int reset_head(struct object_id *oid, const char *action,
> > +	       const char *switch_to_branch, unsigned flags,
> > +	       const char *reflog_orig_head, const char *reflog_head)
> > +{
> > +	unsigned detach_head = flags & RESET_HEAD_DETACH;
> > +	unsigned reset_hard = flags & RESET_HEAD_HARD;
> > +	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> > +	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> > +	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
> > +	struct object_id head_oid;
> > +	struct tree_desc desc[2] = { { NULL }, { NULL } };
> > +	struct lock_file lock = LOCK_INIT;
> > +	struct unpack_trees_options unpack_tree_opts;
> > +	struct tree *tree;
> > +	const char *reflog_action;
> > +	struct strbuf msg = STRBUF_INIT;
> > +	size_t prefix_len;
> > +	struct object_id *orig = NULL, oid_orig,
> > +		*old_orig = NULL, oid_old_orig;
> > +	int ret = 0, nr = 0;
> > +
> > +	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> > +		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
> > +
> > +	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > {
> > +		ret = -1;
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
> > +		ret = error(_("could not determine HEAD revision"));
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	if (!oid)
> > +		oid = &head_oid;
> > +
> > +	if (refs_only)
> > +		goto reset_head_refs;
> > +
> > +	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> > +	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> > +	unpack_tree_opts.head_idx = 1;
> > +	unpack_tree_opts.src_index = the_repository->index;
> > +	unpack_tree_opts.dst_index = the_repository->index;
> > +	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
> > +	unpack_tree_opts.update = 1;
> > +	unpack_tree_opts.merge = 1;
> > +	if (!detach_head)
> > +		unpack_tree_opts.reset = 1;
> > +
> > +	if (repo_read_index_unmerged(the_repository) < 0) {
> > +		ret = error(_("could not read index"));
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++],
> > &head_oid)) {
> > +		ret = error(_("failed to find tree of %s"),
> > +			    oid_to_hex(&head_oid));
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
> > +		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
> > +		ret = -1;
> > +		goto leave_reset_head;
> > +	}
> > +
> > +	tree = parse_tree_indirect(oid);
> > +	prime_cache_tree(the_repository, the_repository->index, tree);
> > +
> > +	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
> > {
> > +		ret = error(_("could not write index"));
> > +		goto leave_reset_head;
> > +	}
> > +
> > +reset_head_refs:
> > +	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> > +	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> > +	prefix_len = msg.len;
> > +
> > +	if (update_orig_head) {
> > +		if (!get_oid("ORIG_HEAD", &oid_old_orig))
> > +			old_orig = &oid_old_orig;
> > +		if (!get_oid("HEAD", &oid_orig)) {
> > +			orig = &oid_orig;
> > +			if (!reflog_orig_head) {
> > +				strbuf_addstr(&msg, "updating ORIG_HEAD");
> > +				reflog_orig_head = msg.buf;
> > +			}
> > +			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
> > +				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
> > +		} else if (old_orig)
> > +			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
> > +	}
> > +
> > +	if (!reflog_head) {
> > +		strbuf_setlen(&msg, prefix_len);
> > +		strbuf_addstr(&msg, "updating HEAD");
> > +		reflog_head = msg.buf;
> > +	}
> > +	if (!switch_to_branch)
> > +		ret = update_ref(reflog_head, "HEAD", oid, orig,
> > +				 detach_head ? REF_NO_DEREF : 0,
> > +				 UPDATE_REFS_MSG_ON_ERR);
> > +	else {
> > +		ret = update_ref(reflog_head, switch_to_branch, oid,
> > +				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> > +		if (!ret)
> > +			ret = create_symref("HEAD", switch_to_branch,
> > +					    reflog_head);
> > +	}
> > +	if (run_hook)
> > +		run_hook_le(NULL, "post-checkout",
> > +			    oid_to_hex(orig ? orig : &null_oid),
> > +			    oid_to_hex(oid), "1", NULL);
> > +
> > +leave_reset_head:
> > +	strbuf_release(&msg);
> > +	rollback_lock_file(&lock);
> > +	while (nr)
> > +		free((void *)desc[--nr].buffer);
> > +	return ret;
> > +}
> > +
> >   int apply_autostash(const char *path)
> >   {
> >   	struct strbuf autostash = STRBUF_INIT;
> > diff --git a/autostash.h b/autostash.h
> > index 5f4e4bd22c..1406638166 100644
> > --- a/autostash.h
> > +++ b/autostash.h
> > @@ -6,6 +6,18 @@
> >   /* Read one file, then strip line endings */
> >   int read_one(const char *path, struct strbuf *buf);
> >
> > +#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
> > +
> > +#define RESET_HEAD_DETACH (1<<0)
> > +#define RESET_HEAD_HARD (1<<1)
> > +#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
> > +#define RESET_HEAD_REFS_ONLY (1<<3)
> > +#define RESET_ORIG_HEAD (1<<4)
> > +
> > +int reset_head(struct object_id *oid, const char *action,
> > +	       const char *switch_to_branch, unsigned flags,
> > +	       const char *reflog_orig_head, const char *reflog_head);
> > +
> >   int apply_autostash(const char *path);
> >
> >   #endif
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 661928d427..c3165896cc 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -734,143 +734,6 @@ static void add_var(struct strbuf *buf, const char
> > *name, const char *value)
> >   	}
> >   }
> >
> > -#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
> > -
> > -#define RESET_HEAD_DETACH (1<<0)
> > -#define RESET_HEAD_HARD (1<<1)
> > -#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
> > -#define RESET_HEAD_REFS_ONLY (1<<3)
> > -#define RESET_ORIG_HEAD (1<<4)
> > -
> > -static int reset_head(struct object_id *oid, const char *action,
> > -		      const char *switch_to_branch, unsigned flags,
> > -		      const char *reflog_orig_head, const char *reflog_head)
> > -{
> > -	unsigned detach_head = flags & RESET_HEAD_DETACH;
> > -	unsigned reset_hard = flags & RESET_HEAD_HARD;
> > -	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> > -	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> > -	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
> > -	struct object_id head_oid;
> > -	struct tree_desc desc[2] = { { NULL }, { NULL } };
> > -	struct lock_file lock = LOCK_INIT;
> > -	struct unpack_trees_options unpack_tree_opts;
> > -	struct tree *tree;
> > -	const char *reflog_action;
> > -	struct strbuf msg = STRBUF_INIT;
> > -	size_t prefix_len;
> > -	struct object_id *orig = NULL, oid_orig,
> > -		*old_orig = NULL, oid_old_orig;
> > -	int ret = 0, nr = 0;
> > -
> > -	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> > -		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
> > -
> > -	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > {
> > -		ret = -1;
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
> > -		ret = error(_("could not determine HEAD revision"));
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	if (!oid)
> > -		oid = &head_oid;
> > -
> > -	if (refs_only)
> > -		goto reset_head_refs;
> > -
> > -	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> > -	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> > -	unpack_tree_opts.head_idx = 1;
> > -	unpack_tree_opts.src_index = the_repository->index;
> > -	unpack_tree_opts.dst_index = the_repository->index;
> > -	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
> > -	unpack_tree_opts.update = 1;
> > -	unpack_tree_opts.merge = 1;
> > -	if (!detach_head)
> > -		unpack_tree_opts.reset = 1;
> > -
> > -	if (repo_read_index_unmerged(the_repository) < 0) {
> > -		ret = error(_("could not read index"));
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++],
> > &head_oid)) {
> > -		ret = error(_("failed to find tree of %s"),
> > -			    oid_to_hex(&head_oid));
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
> > -		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
> > -		ret = -1;
> > -		goto leave_reset_head;
> > -	}
> > -
> > -	tree = parse_tree_indirect(oid);
> > -	prime_cache_tree(the_repository, the_repository->index, tree);
> > -
> > -	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
> > {
> > -		ret = error(_("could not write index"));
> > -		goto leave_reset_head;
> > -	}
> > -
> > -reset_head_refs:
> > -	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> > -	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> > -	prefix_len = msg.len;
> > -
> > -	if (update_orig_head) {
> > -		if (!get_oid("ORIG_HEAD", &oid_old_orig))
> > -			old_orig = &oid_old_orig;
> > -		if (!get_oid("HEAD", &oid_orig)) {
> > -			orig = &oid_orig;
> > -			if (!reflog_orig_head) {
> > -				strbuf_addstr(&msg, "updating ORIG_HEAD");
> > -				reflog_orig_head = msg.buf;
> > -			}
> > -			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
> > -				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
> > -		} else if (old_orig)
> > -			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
> > -	}
> > -
> > -	if (!reflog_head) {
> > -		strbuf_setlen(&msg, prefix_len);
> > -		strbuf_addstr(&msg, "updating HEAD");
> > -		reflog_head = msg.buf;
> > -	}
> > -	if (!switch_to_branch)
> > -		ret = update_ref(reflog_head, "HEAD", oid, orig,
> > -				 detach_head ? REF_NO_DEREF : 0,
> > -				 UPDATE_REFS_MSG_ON_ERR);
> > -	else {
> > -		ret = update_ref(reflog_head, switch_to_branch, oid,
> > -				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> > -		if (!ret)
> > -			ret = create_symref("HEAD", switch_to_branch,
> > -					    reflog_head);
> > -	}
> > -	if (run_hook)
> > -		run_hook_le(NULL, "post-checkout",
> > -			    oid_to_hex(orig ? orig : &null_oid),
> > -			    oid_to_hex(oid), "1", NULL);
> > -
> > -leave_reset_head:
> > -	strbuf_release(&msg);
> > -	rollback_lock_file(&lock);
> > -	while (nr)
> > -		free((void *)desc[--nr].buffer);
> > -	return ret;
> > -}
> > -
> >   static int move_to_original_branch(struct rebase_options *opts)
> >   {
> >    struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> >
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 5/7] autostash: extract perform_autostash() from rebase
  2019-10-16 17:26 ` [RFC PATCH 5/7] autostash: extract perform_autostash() " Denton Liu
@ 2019-10-21 19:00   ` Johannes Schindelin
  0 siblings, 0 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-21 19:00 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Alban Gruin, Junio C Hamano

Hi Denton,

On Wed, 16 Oct 2019, Denton Liu wrote:

> Continue the process of lib-ifying the autostash code. In a future
> commit, this will be used to implement `--autostash` in other builtins.
>
> This patch is best viewed with `--color-moved` and
> `--color-moved-ws=allow-indentation-change`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  autostash.c      | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  autostash.h      |  1 +
>  builtin/rebase.c | 44 +-------------------------------------------
>  3 files changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/autostash.c b/autostash.c
> index eb58e0c8a4..722cf78b12 100644
> --- a/autostash.c
> +++ b/autostash.c
> @@ -12,6 +12,7 @@
>  #include "tree-walk.h"
>  #include "tree.h"
>  #include "unpack-trees.h"
> +#include "wt-status.h"
>
>  int read_one(const char *path, struct strbuf *buf)
>  {
> @@ -150,6 +151,51 @@ int reset_head(struct object_id *oid, const char *action,
>  	return ret;
>  }
>
> +void perform_autostash(const char *path)

Maybe we can find a better name than "perform_autostash"? It is not
clear whether this is the "saving" or "applying" part of the autostash,
I think.

Maybe `save_autostash()`? And maybe instead of `path`, the parameter
could be `save_hash_to_path` or something similar?

Now that I think of it, I forgot to mention in a reply to an earlier
patch in this series that `reset_head()` might be too generic a name to
be a global function...

Ciao,
Dscho

> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct lock_file lock_file = LOCK_INIT;
> +	int fd;
> +
> +	fd = hold_locked_index(&lock_file, 0);
> +	refresh_cache(REFRESH_QUIET);
> +	if (0 <= fd)
> +		repo_update_index_if_able(the_repository, &lock_file);
> +	rollback_lock_file(&lock_file);
> +
> +	if (has_unstaged_changes(the_repository, 1) ||
> +	    has_uncommitted_changes(the_repository, 1)) {
> +		struct child_process stash = CHILD_PROCESS_INIT;
> +		struct object_id oid;
> +
> +		argv_array_pushl(&stash.args,
> +				 "stash", "create", "autostash", NULL);
> +		stash.git_cmd = 1;
> +		stash.no_stdin = 1;
> +		if (capture_command(&stash, &buf, GIT_MAX_HEXSZ))
> +			die(_("Cannot autostash"));
> +		strbuf_trim_trailing_newline(&buf);
> +		if (get_oid(buf.buf, &oid))
> +			die(_("Unexpected stash response: '%s'"),
> +			    buf.buf);
> +		strbuf_reset(&buf);
> +		strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
> +
> +		if (safe_create_leading_directories_const(path))
> +			die(_("Could not create directory for '%s'"),
> +			    path);
> +		write_file(path, "%s", oid_to_hex(&oid));
> +		printf(_("Created autostash: %s\n"), buf.buf);
> +		if (reset_head(NULL, "reset --hard",
> +			       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
> +			die(_("could not reset --hard"));
> +
> +		if (discard_index(the_repository->index) < 0 ||
> +			repo_read_index(the_repository) < 0)
> +			die(_("could not read index"));
> +	}
> +}
> +
>  int apply_autostash(const char *path)
>  {
>  	struct strbuf autostash = STRBUF_INIT;
> diff --git a/autostash.h b/autostash.h
> index 1406638166..e08ccb9881 100644
> --- a/autostash.h
> +++ b/autostash.h
> @@ -18,6 +18,7 @@ int reset_head(struct object_id *oid, const char *action,
>  	       const char *switch_to_branch, unsigned flags,
>  	       const char *reflog_orig_head, const char *reflog_head);
>
> +void perform_autostash(const char *path);
>  int apply_autostash(const char *path);
>
>  #endif
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index c3165896cc..c4decdfb5b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1797,49 +1797,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		die(_("could not read index"));
>
>  	if (options.autostash) {
> -		struct lock_file lock_file = LOCK_INIT;
> -		int fd;
> -
> -		fd = hold_locked_index(&lock_file, 0);
> -		refresh_cache(REFRESH_QUIET);
> -		if (0 <= fd)
> -			repo_update_index_if_able(the_repository, &lock_file);
> -		rollback_lock_file(&lock_file);
> -
> -		if (has_unstaged_changes(the_repository, 1) ||
> -		    has_uncommitted_changes(the_repository, 1)) {
> -			const char *autostash =
> -				state_dir_path("autostash", &options);
> -			struct child_process stash = CHILD_PROCESS_INIT;
> -			struct object_id oid;
> -
> -			argv_array_pushl(&stash.args,
> -					 "stash", "create", "autostash", NULL);
> -			stash.git_cmd = 1;
> -			stash.no_stdin = 1;
> -			strbuf_reset(&buf);
> -			if (capture_command(&stash, &buf, GIT_MAX_HEXSZ))
> -				die(_("Cannot autostash"));
> -			strbuf_trim_trailing_newline(&buf);
> -			if (get_oid(buf.buf, &oid))
> -				die(_("Unexpected stash response: '%s'"),
> -				    buf.buf);
> -			strbuf_reset(&buf);
> -			strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
> -
> -			if (safe_create_leading_directories_const(autostash))
> -				die(_("Could not create directory for '%s'"),
> -				    options.state_dir);
> -			write_file(autostash, "%s", oid_to_hex(&oid));
> -			printf(_("Created autostash: %s\n"), buf.buf);
> -			if (reset_head(NULL, "reset --hard",
> -				       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
> -				die(_("could not reset --hard"));
> -
> -			if (discard_index(the_repository->index) < 0 ||
> -				repo_read_index(the_repository) < 0)
> -				die(_("could not read index"));
> -		}
> +		perform_autostash(state_dir_path("autostash", &options));
>  	}
>
>  	if (require_clean_work_tree(the_repository, "rebase",
> --
> 2.23.0.897.g0a19638b1e
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 7/7] merge: teach --autostash option
  2019-10-16 17:26 ` [RFC PATCH 7/7] merge: teach --autostash option Denton Liu
  2019-10-18  9:46   ` Phillip Wood
@ 2019-10-21 19:10   ` Johannes Schindelin
  1 sibling, 0 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-21 19:10 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Alban Gruin, Junio C Hamano

Hi Denton,

On Wed, 16 Oct 2019, Denton Liu wrote:

> In rebase, one can pass the `--autostash` option to cause the worktree
> to be automatically stashed before continuing with the rebase. This
> option is missing in merge, however.
>
> Implement the `--autostash` option and corresponding `merge.autoStash`
> option in merge which stashes before merging and then pops after.
>
> Reported-by: Alban Gruin <alban.gruin@gmail.com>

Maybe "Suggested-by" would be more accurate, it is not like this feature
request was a bug report...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  builtin/merge.c | 13 +++++++++++++
>  builtin/pull.c  |  9 +++++----
>  t/t5520-pull.sh |  8 --------
>  3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 062e911441..d1a5eaad0d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -40,6 +40,7 @@
>  #include "branch.h"
>  #include "commit-reach.h"
>  #include "wt-status.h"
> +#include "autostash.h"
>
>  #define DEFAULT_TWOHEAD (1<<0)
>  #define DEFAULT_OCTOPUS (1<<1)
> @@ -58,6 +59,8 @@ static const char * const builtin_merge_usage[] = {
>  	NULL
>  };
>
> +static GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
> +
>  static int show_diffstat = 1, shortlog_len = -1, squash;
>  static int option_commit = -1;
>  static int option_edit = -1;
> @@ -81,6 +84,7 @@ static int show_progress = -1;
>  static int default_to_upstream = 1;
>  static int signoff;
>  static const char *sign_commit;
> +static int autostash;
>  static int no_verify;
>
>  static struct strategy all_strategy[] = {
> @@ -285,6 +289,8 @@ static struct option builtin_merge_options[] = {
>  	OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
>  	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>  	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> +	OPT_BOOL(0, "autostash", &autostash,
> +	      N_("automatically stash/stash pop before and after")),
>  	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
>  	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
>  	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
> @@ -440,6 +446,7 @@ static void finish(struct commit *head_commit,
>  		strbuf_addf(&reflog_message, "%s: %s",
>  			getenv("GIT_REFLOG_ACTION"), msg);
>  	}
> +	apply_autostash(merge_autostash());

Should this not be guarded by `if (autostash)`?

>  	if (squash) {
>  		squash_message(head_commit, remoteheads);
>  	} else {
> @@ -631,6 +638,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  	} else if (!strcmp(k, "commit.gpgsign")) {
>  		sign_commit = git_config_bool(k, v) ? "" : NULL;
>  		return 0;
> +	} else if (!strcmp(k, "merge.autostash")) {
> +		autostash = git_config_bool(k, v);
> +		return 0;
>  	}
>
>  	status = fmt_merge_msg_config(k, v, cb);
> @@ -724,6 +734,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  		for (j = common; j; j = j->next)
>  			commit_list_insert(j->item, &reversed);
>
> +		if (autostash)
> +			perform_autostash(merge_autostash());
>  		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>  		clean = merge_recursive(&o, head,
>  				remoteheads->item, reversed, &result);
> @@ -1288,6 +1300,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>
>  		/* Invoke 'git reset --merge' */
>  		ret = cmd_reset(nargc, nargv, prefix);
> +		apply_autostash(merge_autostash());

Again, this should be guarded by `if (autostash)`, methinks.

>  		goto done;
>  	}
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d25ff13a60..ee186781ae 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -183,7 +183,7 @@ static struct option pull_options[] = {
>  		N_("verify that the named commit has a valid GPG signature"),
>  		PARSE_OPT_NOARG),
>  	OPT_BOOL(0, "autostash", &opt_autostash,
> -		N_("automatically stash/stash pop before and after rebase")),
> +		N_("automatically stash/stash pop before and after")),

Makes sense; this is now shared between the rebase and the merge modes.

>  	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
>  		N_("merge strategy to use"),
>  		0),
> @@ -671,6 +671,10 @@ static int run_merge(void)
>  	argv_array_pushv(&args, opt_strategy_opts.argv);
>  	if (opt_gpg_sign)
>  		argv_array_push(&args, opt_gpg_sign);
> +	if (opt_autostash == 0)
> +		argv_array_push(&args, "--no-autostash");
> +	else if (opt_autostash == 1)
> +		argv_array_push(&args, "--autostash");

Or shorter:

	argv_array_pushf(&args, "%s-autostash", opt_autostash ? "-" : "--no");

Ah, but that would mishandle `-1`. I bet I will be puzzled by this
again. Maybe it would make sense to mention in a code comment that it
can be `-1` in which case we leave it to `rebase` to use the config
settings to determine whether or not to autostash.

>  	if (opt_allow_unrelated_histories > 0)
>  		argv_array_push(&args, "--allow-unrelated-histories");
>
> @@ -918,9 +922,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  	if (get_oid("HEAD", &orig_head))
>  		oidclr(&orig_head);
>
> -	if (!opt_rebase && opt_autostash != -1)
> -		die(_("--[no-]autostash option is only valid with --rebase."));
> -
>  	autostash = config_autostash;
>  	if (opt_rebase) {
>  		if (opt_autostash != -1)
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index cf4cc32fd0..75f162495a 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -365,14 +365,6 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
>  	test_pull_autostash_fail --rebase --no-autostash
>  '
>
> -for i in --autostash --no-autostash
> -do
> -	test_expect_success "pull $i (without --rebase) is illegal" '
> -		test_must_fail git pull $i . copy 2>err &&
> -		test_i18ngrep "only valid with --rebase" err
> -	'
> -done
> -

Nice!
Dscho

>  test_expect_success 'pull.rebase' '
>  	git reset --hard before-rebase &&
>  	test_config pull.rebase true &&
> --
> 2.23.0.897.g0a19638b1e
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-21 18:44     ` Johannes Schindelin
  2019-10-21 18:53       ` Denton Liu
@ 2019-10-21 19:49       ` Junio C Hamano
  2019-10-21 19:54         ` Jeff King
  1 sibling, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2019-10-21 19:49 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Denton Liu, Git Mailing List, Alban Gruin

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> ... I do not particularly see this change (there may be similar
>> ones) desirable.  I'd find it it be much more natural to sort
>> "commit-anything" after "commit", and that is true with or without
>> the common extension ".o" added to these entries.
>>
>> In short, flipping these entries because '.' sorts later than '-' is
>> making the result look "less sorted", at least to me.
>
> The problem with this argument is that it disagrees with ASCII, as `-`
> has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically
> _larger_.

I am saying that sorting these in ASCII order did not produce result
that is easy to the eyes.

You are saying that Denton's patch sorted these lines in ASCII order.

I agree with you that it did correctly sort them in ASCII order.

That does not make the patch right ;-)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-21 19:49       ` Junio C Hamano
@ 2019-10-21 19:54         ` Jeff King
  2019-10-22 11:34           ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2019-10-21 19:54 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Denton Liu, Git Mailing List, Alban Gruin

On Tue, Oct 22, 2019 at 04:49:19AM +0900, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> ... I do not particularly see this change (there may be similar
> >> ones) desirable.  I'd find it it be much more natural to sort
> >> "commit-anything" after "commit", and that is true with or without
> >> the common extension ".o" added to these entries.
> >>
> >> In short, flipping these entries because '.' sorts later than '-' is
> >> making the result look "less sorted", at least to me.
> >
> > The problem with this argument is that it disagrees with ASCII, as `-`
> > has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically
> > _larger_.
> 
> I am saying that sorting these in ASCII order did not produce result
> that is easy to the eyes.
> 
> You are saying that Denton's patch sorted these lines in ASCII order.
> 
> I agree with you that it did correctly sort them in ASCII order.
> 
> That does not make the patch right ;-)

What's the purpose of sorting them, though? I thought it was less for
aesthetics and more to to keep lines deterministic (to avoid two
branches adding the same line in different places, thus causing
weirdness when the two are merged). In that case, I think we care less
about the exact order and more that anybody can easily reproduce the
same sort (by running "10:!sort" or whatever you weird emacs-types would
type).

-Peff

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-21 19:54         ` Jeff King
@ 2019-10-22 11:34           ` Junio C Hamano
  0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2019-10-22 11:34 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, Denton Liu, Git Mailing List, Alban Gruin

Jeff King <peff@peff.net> writes:

>> ...
>> I agree with you that it did correctly sort them in ASCII order.
>
> What's the purpose of sorting them, though? I thought it was less for
> aesthetics and more to to keep lines deterministic (to avoid two
> branches adding the same line in different places, thus causing
> weirdness when the two are merged). In that case, I think we care less
> about the exact order and more that anybody can easily reproduce the
> same sort (by running "10:!sort" or whatever you weird emacs-types would
> type).

In the ideal world, "sort" would have a handy option we can tell it
to reshuffle the ASCII table in such a way that all punctuations
come before alphanumeric, making sure "/" and "." are the first two
letters in the alphabet, and everybody can use it to sort the lines
reproducibly and also readably.  But I do not know of such a widely
used implementation of "sort", so...

If we had known better, we would have used such a custom sort order
to sort the index entries, making sure that slash sorts before any
other byte ;-)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
  2019-10-21 18:53       ` Denton Liu
@ 2019-10-22 23:33         ` Johannes Schindelin
  0 siblings, 0 replies; 23+ messages in thread
From: Johannes Schindelin @ 2019-10-22 23:33 UTC (permalink / raw)
  To: Denton Liu; +Cc: Junio C Hamano, Git Mailing List, Alban Gruin

Hi,

On Mon, 21 Oct 2019, Denton Liu wrote:

> Hi Johannes,
>
> On Mon, Oct 21, 2019 at 08:44:40PM +0200, Johannes Schindelin wrote:
> > Hi Junio,
> >
> > On Fri, 18 Oct 2019, Junio C Hamano wrote:
> >
> > > Denton Liu <liu.denton@gmail.com> writes:
> > >
> > > > There are many += lists in the Makefile and, over time, they have gotten
> > > > slightly out of order, alphabetically. Alphabetically sort all += lists
> > > > to bring them back in order.
> > > > ...
> > >
> > > Hmm.  I like the general thrust, but ...
> > >
> > > >  LIB_OBJS += combine-diff.o
> > > > -LIB_OBJS += commit.o
> > > >  LIB_OBJS += commit-graph.o
> > > >  LIB_OBJS += commit-reach.o
> > > > +LIB_OBJS += commit.o
> > >
> > > ... I do not particularly see this change (there may be similar
> > > ones) desirable.  I'd find it it be much more natural to sort
> > > "commit-anything" after "commit", and that is true with or without
> > > the common extension ".o" added to these entries.
> > >
> > > In short, flipping these entries because '.' sorts later than '-' is
> > > making the result look "less sorted", at least to me.
> >
> > The problem with this argument is that it disagrees with ASCII, as `-`
> > has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically
> > _larger_.
> >
> > So Denton's patch does the correct thing.
>
> I actually agree with Junio on this one. Without the prefixes, "commit"
> would go before "commit-graph" so I think it would make more sense to
> order with the prefixes removed instead of taking the naive ordering by
> just sorting each block.

That will make it harder on other contributors like me, who prefer to
mark the lines in `vim` and then call `:sort` on them, and then not care
about it any further.

Any decision that makes automating tedious tasks harder puts more burden
on human beings. I don't like that.

Ciao,
Dscho

>
> Thanks,
>
> Denton
>
> >
> > Ciao,
> > Dscho
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-16 17:26 [RFC PATCH 0/7] merge: learn --autostash Denton Liu
2019-10-16 17:26 ` [RFC PATCH 1/7] Makefile: alphabetically sort += lists Denton Liu
2019-10-17 18:07   ` Junio C Hamano
2019-10-21 18:44     ` Johannes Schindelin
2019-10-21 18:53       ` Denton Liu
2019-10-22 23:33         ` Johannes Schindelin
2019-10-21 19:49       ` Junio C Hamano
2019-10-21 19:54         ` Jeff King
2019-10-22 11:34           ` Junio C Hamano
2019-10-16 17:26 ` [RFC PATCH 2/7] autostash: extract read_one() from rebase Denton Liu
2019-10-18  9:04   ` Phillip Wood
2019-10-21 18:46     ` Johannes Schindelin
2019-10-16 17:26 ` [RFC PATCH 3/7] autostash: extract apply_autostash() " Denton Liu
2019-10-16 17:26 ` [RFC PATCH 4/7] autostash: extract reset_head() " Denton Liu
2019-10-18  9:37   ` Phillip Wood
2019-10-21 18:56     ` Johannes Schindelin
2019-10-16 17:26 ` [RFC PATCH 5/7] autostash: extract perform_autostash() " Denton Liu
2019-10-21 19:00   ` Johannes Schindelin
2019-10-16 17:26 ` [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS Denton Liu
2019-10-18  9:41   ` Phillip Wood
2019-10-16 17:26 ` [RFC PATCH 7/7] merge: teach --autostash option Denton Liu
2019-10-18  9:46   ` Phillip Wood
2019-10-21 19:10   ` Johannes Schindelin

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git