Git development
 help / color / mirror / Atom feed
* Re: In-body from headers ignored
From: Junio C Hamano @ 2017-02-05 23:43 UTC (permalink / raw)
  To: Liam Breck; +Cc: git
In-Reply-To: <CAKvHMgQLKccm2LcL4LGhz0afVthaS2gvEcLtoHX2TcDnr1npbw@mail.gmail.com>

Liam Breck <liam@networkimprov.net> writes:

> git format-patch & send-email generate the in-body From header.
>
> git am recognizes it.
>
> git commit & format-patch & send-email ignore it. (The latter two will
> add a new header above an extant one.) Is there a rationale for this?

I may be misunderstanding you, but I am guessing that you are
expecting

	$ GIT_AUTHOR_NAME="Liam Breck" GIT_AUTHOR_EMAIL='liam@n.net' \
	git commit -m 'The title of the patch

	From: Somebody Else <somebody@else.xz>
	Subject: The real title of the patch

	This is the (true) first line of the body of the message.'

to record a commit object that would give

	$ git cat-file commit HEAD
	tree ....
	parent ....
	author Somebody Else <somebody@else.xz> ....
	committer ...

	The real title of the patch

	This is the (true) first line of the body of the message.

and seeing that the real author is still you, the title is "The real
title of the patch", and the first paragraph of the body consists of
these two lines that begin with "From: and "Subject:".

This is very much deliberate.  "git commit" does not care if the
second paragraph in the body of the message resembles e-mail
headers, because it is a command that can be used by people who do
not even e-mail patches.

"git format-patch" and "git am" are all about passing the commit
between people over e-mail, and they know that the second paragraph
can have "From:", "Subject:" or "Date:" to override what "am"
obtains from the header lines of the e-mail that carried the
message, because the e-mail headers can be different (e.g. you may
be forwarding somebody else's e-mail but you may not be able to
forge the real "From:" line to your MUA/MTA) from what you really
want to use.  On the receiving end, "am" tells "mailinfo" program to
inspect the message body with "--inbody-headers" option.  After
"mailinfo" inspects the message, "am" invokes the underlying
machinery to actually make the commit (which corresponds to "git
commit"), but at that point the invoked "git commit" does not even
see these in-body header lines.  That is how division of labor
between these layers of the commands are structured.


^ permalink raw reply

* Re: [PATCH] tag: generate useful reflog message
From: Junio C Hamano @ 2017-02-05 23:25 UTC (permalink / raw)
  To: cornelius.weig; +Cc: git, bitte.keine.werbung.einwerfen, karthik.188, peff
In-Reply-To: <20170205214254.24560-1-cornelius.weig@tngtech.com>

cornelius.weig@tngtech.com writes:

> Now, a reflog message is generated when creating a tag. The message
> follows the pattern "commit: <subject>" where the subject is taken from
> the commit the tag points to. For example:
> "6e3a7b3 refs/tags/tag_with_reflog@{0}: commit: Git 2.12-rc0"

Because the reflog records the actions, shouldn't it be saying that
you "tagged"?  The reflog for HEAD says things like "reset: moving
to...", "am: $subject", and the reflog for a branch says things like
"branch: created from master", "am: $subject", "rebase -i (finish)".

For a tag, I would imagine something like "tag: tagged 4e59582ff7
("Seventh batch for 2.12", 2017-01-23)" would be more appropriate.

> Notes:
>     While playing around with tag reflogs I also found a bug that was present
>     before this patch. It manifests itself when the sha1-ref in the reflog does not
>     point to a commit object but something else.

The underlying machinery for "log" and "rev-list" is about showing a
stream of commits, and most of the reflog entries point at commits.

On the other hand, the "walking reflogs to and show the sequence of
the tip of refs", and there is no reason to expect the tip of refs
will always be commits, but an ancient design mistake bolted the
latter on top of the former (perhaps because in practice the tip of
refs are almost always commits); "reflog" aka "log -g" and "rev-list
--walk-reflogs" share the same issue coming from that misdesign,
which needs to be corrected to solve this issue.

The exact same design mistake also makes "git reflog" to accept
options like "--topo-order", even though many of the options that
make sense for the "commit DAG walking" (which is what "log" and
"rev-list" are about) do not make any sense when walking a reflog.
And the command would give nonsense output when given such an
option, because a reflog is a single strand of pearl of objects (not
necessarily commits) and the order in which these objects appear in
the reflog does not have anything to do with the underlying commit
DAG topology.  Fixing the ancient misdesign would fix this issue,
too.

I think the fix would involve first ripping out the "reflog walking"
code that was bolted on and stop allowing it to inject the entries
taken from the reflog into the "walk the commit DAG" machinery.
Then "reflog walking" code needs to be taught to have its own "now
we got a single object to show, show it (using the helper functions
to show a single object that is already used by 'git show')" code,
instead of piggy-backing on the output codepath used by "log" and
"rev-list".


^ permalink raw reply

* [PATCH v2] tag: generate useful reflog message
From: cornelius.weig @ 2017-02-05 23:18 UTC (permalink / raw)
  To: git; +Cc: bitte.keine.werbung.einwerfen, Cornelius Weig, karthik.188, peff
In-Reply-To: <20170205214254.24560-1-cornelius.weig@tngtech.com>

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option
`core.logAllRefUpdates` set to 'always', a reflog is created for them.
So far, the description of reflog entries for tags was empty, making the
reflog hard to understand. For example:
"6e3a7b3 refs/tags/test@{0}:"

Now, a reflog message is generated when creating a tag, following the
pattern "<action>: <description>". Here, action is the command line with
all arguments, or the value of GIT_REFLOG_ACTION if it is set. The
description is the commit subject, if the tag points to a commit. For
example:
"6e3a7b3 refs/tags/test@{0}: tag --create-reflog test: Git 2.12-rc0"
If the tag points to a tree/blob/tag object, the following static
strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---

Notes:
    *This patch supersedes the version submitted a few hours earlier.*
    
    Sorry for the messup, but I realized that the pattern for the reflog message
    from my first draft did not comply to standard git behavior.
    
    Please also note my remarks from v1 (repeated here):
    
    While playing around with tag reflogs I also found a bug that was present
    before this patch. It manifests itself when the sha1-ref in the reflog does not
    point to a commit object but something else.
    
    For example,
    
     - when the referenced sha1 is a tag object:
    	$ git tag --create-reflog -f -m'annotated tag' tag_with_reflog
     - when the referenced sha1 is a blob object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD:<filename>
     - when the referenced sha1 is a tree object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD^{tree}
    
    In each case, a proper reflog entry is generated, but
    	$ git reflog tag_with_reflog
    will sometimes segfault (if it does, it does so consistently), or only show the
    first few entries. The tree/blob cases are IMHO not so important, but the
    broken reflog for annotated tags I find quite severe.
    
    I guess it's because the reflog is funneled through the log.c code, where every
    reflog-entry is assumed to be a commit object? If this is the case, a fix would
    probably be quite involved.

 builtin/tag.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..3d9e105 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 static int force_sign_annotate;
+static struct strbuf default_rla = STRBUF_INIT;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -302,6 +303,48 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *object, struct strbuf *sb)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (!rla)
+		rla = default_rla.buf;
+
+	strbuf_addf(sb, "%s: ", rla ? rla : default_rla.buf);
+
+	type = sha1_object_info(object, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, "internal object");
+		break;
+	case OBJ_COMMIT:
+		buf = read_sha1_file(object, &type, &size);
+		if (buf) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, sb->len, subject_start, subject_len);
+			free(buf);
+		} else {
+			die("commit object %s could not be read",
+				sha1_to_hex(object));
+		}
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, "tree object");
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, "blob object");
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, "other tag object");
+		break;
+	}
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +378,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -349,7 +393,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	const char *format = NULL;
-	int icase = 0;
+	int icase = 0, i;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -391,6 +435,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_tag_config, sorting_tail);
 
+	/* Record the command line for the reflog */
+	strbuf_addstr(&default_rla, "tag");
+	for (i = 1; i < argc; i++)
+		strbuf_addf(&default_rla, " %s", argv[i]);
+
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
 	filter.lines = -1;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..9c80bc9 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+git log -1 > expected \
+	--format="format:tag --create-reflog tag_with_reflog: %s%n"
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+git log -1 > expected \
+	--format='format:tag -m annotated tag --create-reflog tag_with_reflog: %s%n'
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2


^ permalink raw reply related

* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Jacob Keller @ 2017-02-05 22:45 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Jeff King, Git mailing list
In-Reply-To: <CACsJy8CTRcDuRiNeutG82iAj8qQFp+z2nadFfdkkHQGk6GKppA@mail.gmail.com>

On Sun, Feb 5, 2017 at 2:39 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jan 21, 2017 at 2:16 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> I would be interested in the code for this.. I'm curious if I can
>> adapt it to my use of tmux.
>
> I stumbled upon this which does mention about git SHA-1. Maybe you'll
> find it useful. Haven't tried it out though.
>
> https://github.com/morantron/tmux-fingers
>
> --
> Duy

This sounds almost exactly like what I want! :)

Thanks,
Jake

^ permalink raw reply

* In-body from headers ignored
From: Liam Breck @ 2017-02-05 21:45 UTC (permalink / raw)
  To: git

git format-patch & send-email generate the in-body From header.

git am recognizes it.

git commit & format-patch & send-email ignore it. (The latter two will
add a new header above an extant one.) Is there a rationale for this?

If not, maybe this is a bug?

^ permalink raw reply

* [PATCH] tag: generate useful reflog message
From: cornelius.weig @ 2017-02-05 21:42 UTC (permalink / raw)
  To: git; +Cc: bitte.keine.werbung.einwerfen, Cornelius Weig, karthik.188, peff

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option
`core.logAllRefUpdates` set to 'always', a reflog is created for them.
So far, the description of reflog entries for tags was empty, making the
reflog hard to understand. For example:
"6e3a7b3 refs/tags/tag_with_reflog@{0}:"

Now, a reflog message is generated when creating a tag. The message
follows the pattern "commit: <subject>" where the subject is taken from
the commit the tag points to. For example:
"6e3a7b3 refs/tags/tag_with_reflog@{0}: commit: Git 2.12-rc0"
If the tag points to a tree/blob/tag object, the following static
messages are used instead:

 - "tree object"
 - "blob object"
 - "other tag object"

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---

Notes:
    While playing around with tag reflogs I also found a bug that was present
    before this patch. It manifests itself when the sha1-ref in the reflog does not
    point to a commit object but something else.
    
    For example,
    
     - when the referenced sha1 is a tag object:
    	$ git tag --create-reflog -f -m'annotated tag' tag_with_reflog
     - when the referenced sha1 is a blob object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD:<filename>
     - when the referenced sha1 is a tree object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD^{tree}
    
    In each case, a proper reflog entry is generated, but
    	$ git reflog tag_with_reflog
    will sometimes segfault (if it does, it does so consistently), or only show the
    first few entries. The tree/blob cases are IMHO not so important, but the
    broken reflog for annotated tags I find quite severe.
    
    I guess it's because the reflog is funneled through the log.c code, where every
    reflog-entry is assumed to be a commit object? If this is the case, a fix would
    probably be quite involved.

 builtin/tag.c  | 43 ++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 14 +++++++++++++-
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..c0d9478 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,43 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *object, struct strbuf *sb)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	type = sha1_object_info(object, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, "internal object");
+		break;
+	case OBJ_COMMIT:
+		strbuf_addstr(sb, "commit: ");
+		buf = read_sha1_file(object, &type, &size);
+		if (buf) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, 8, subject_start, subject_len);
+			free(buf);
+		} else {
+			die("commit object %s could not be read",
+				sha1_to_hex(repl));
+		}
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, "tree object");
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, "blob object");
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, "other tag object");
+		break;
+	}
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +372,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +532,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +544,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +554,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..0a92b2c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -83,7 +83,19 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	git log -1 --format="format:commit: %s%n" > expected &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	git log -1 --format="format:commit: %s%n" > expected &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2


^ permalink raw reply related

* [PATCH] difftool: fix bug when printing usage
From: David Aguilar @ 2017-02-05 21:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git ML, Johannes Schindelin, Denton Liu
In-Reply-To: <20170205201751.z4rfmy5xxaqg472l@gmail.com>

"git difftool -h" reports an error:

	fatal: BUG: setup_git_env called without repository

Defer repository setup so that the help option processing happens before
the repository is initialized.

Add tests to ensure that the basic usage works inside and outside of a
repository.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This bug exists in both "master" and "next".
This patch has been tested on both branches.

 builtin/difftool.c  |  8 ++++----
 t/t7800-difftool.sh | 13 +++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index b5e85ab079..d13350ce83 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	/* NEEDSWORK: once we no longer spawn anything, remove this */
-	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
-	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
-
 	git_config(difftool_config, NULL);
 	symlinks = has_symlinks;
 
@@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (tool_help)
 		return print_tool_help();
 
+	/* NEEDSWORK: once we no longer spawn anything, remove this */
+	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index aa0ef02597..21e2ac4ad6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,19 @@ prompt_given ()
 	test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
+test_expect_success 'basic usage requires no repo' '
+	lines=$(git difftool -h | grep ^usage: | wc -l) &&
+	test "$lines" -eq 1 &&
+	# create a ceiling directory to prevent Git from finding a repo
+	mkdir -p not/repo &&
+	ceiling="$PWD/not" &&
+	lines=$(cd not/repo &&
+		GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
+		grep ^usage: | wc -l) &&
+	test "$lines" -eq 1 &&
+	rmdir -p not/repo
+'
+
 # Create a file on master and change it on branch
 test_expect_success 'setup' '
 	echo master >file &&
-- 
2.12.0.rc0.209.g3d1e53e462


^ permalink raw reply related

* [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone.  Unfortunately
git currently offers no such option.  git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.

Allow 'git stash push' to take pathspec to specify which paths to stash.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-stash.txt        | 11 ++++++++++
 git-stash.sh                       | 30 ++++++++++++++++++++-------
 t/t3903-stash.sh                   | 42 ++++++++++++++++++++++++++++++++++++++
 t/t3905-stash-include-untracked.sh | 26 +++++++++++++++++++++++
 4 files changed, 102 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index a138ed6a24..be55e456fa 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
 'git stash' branch <branchname> [<stash>]
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
+	     [--] [<pathspec>...]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
+	     [-- <paths>...]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
 -------
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
 
 	Save your local modifications to a new 'stash' and roll them
 	back to HEAD (in the working tree and in the index).
@@ -56,6 +61,12 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
 	only <message> does not trigger this action to prevent a misspelled
 	subcommand from making an unwanted stash.
 +
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
 +
diff --git a/git-stash.sh b/git-stash.sh
index 33b2d0384c..46367d40aa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
 untracked_files () {
 	excl_opt=--exclude-standard
 	test "$untracked" = "all" && excl_opt=
-	git ls-files -o -z $excl_opt
+	git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -72,6 +72,11 @@ create_stash () {
 			untracked="$1"
 			new_style=t
 			;;
+		--)
+			shift
+			new_style=t
+			break
+			;;
 		*)
 			if test -n "$new_style"
 			then
@@ -99,6 +104,10 @@ create_stash () {
 	if test -z "$new_style"
 	then
 		stash_msg="$*"
+		while test $# != 0
+		do
+			shift
+		done
 	fi
 
 	git update-index -q --refresh
@@ -134,7 +143,7 @@ create_stash () {
 		# Untracked files are stored by themselves in a parentless commit, for
 		# ease of unpacking later.
 		u_commit=$(
-			untracked_files | (
+			untracked_files "$@" | (
 				GIT_INDEX_FILE="$TMPindex" &&
 				export GIT_INDEX_FILE &&
 				rm -f "$TMPindex" &&
@@ -157,7 +166,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
@@ -171,7 +180,7 @@ create_stash () {
 
 		# find out what the user wants
 		GIT_INDEX_FILE="$TMP-index" \
-			git add--interactive --patch=stash -- &&
+			git add--interactive --patch=stash -- "$@" &&
 
 		# state of the working tree
 		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -307,18 +316,25 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash -m "$stash_msg" -u "$untracked"
+	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
 	store_stash -m "$stash_msg" -q $w_commit ||
 	die "$(gettext "Cannot save the current status")"
 	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
 
 	if test -z "$patch_mode"
 	then
-		git reset --hard ${GIT_QUIET:+-q}
+		if test $# != 0
+		then
+			git ls-files -z -- "$@" | xargs -0 git reset --
+			git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
+			git ls-files -z --others -- "$@" | xargs -0 git clean --force --
+		else
+			git reset --hard ${GIT_QUIET:+-q}
+		fi
 		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
 		if test -n "$untracked"
 		then
-			git clean --force --quiet -d $CLEAN_X_OPTION
+			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
 		if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b859e51086..461fe46045 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -811,4 +811,46 @@ test_expect_success 'new style stash create stores correct message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stash -- <filename> stashes and restores the file' '
+	>foo &&
+	>bar &&
+	git add foo bar &&
+	git stash push -- foo &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+	>foo &&
+	>bar &&
+	>extra &&
+	git add foo bar extra &&
+	git stash push -- foo bar &&
+	test_path_is_missing bar &&
+	test_path_is_missing foo &&
+	test_path_is_file extra &&
+	git stash pop &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+	>"foo	bar" &&
+	>foo &&
+	>untracked &&
+	git add foo* &&
+	git stash push -- foo* &&
+	test_path_is_missing "foo	bar" &&
+	test_path_is_missing foo &&
+	test_path_is_file untracked &&
+	git stash pop &&
+	test_path_is_file "foo	bar" &&
+	test_path_is_file foo &&
+	test_path_is_file untracked
+'
+
 test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..9a98e31a3e 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
 	test -s .gitignore
 '
 
+test_expect_success 'stash push --include-untracked with pathspec' '
+	>foo &&
+	>bar &&
+	git stash push --include-untracked -- foo &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file bar &&
+	test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+	>"foo	bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash push --include-untracked -- foo* &&
+	test_path_is_missing "foo	bar" &&
+	test_path_is_missing foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo	bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
 test_done
-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

Don't mention git reset --hard in the documentation for git stash save.
It's an implementation detail that doesn't matter to the end user and
thus shouldn't be exposed to them.  In addition it's not quite true for
git stash -p, and will not be true when a filename argument to limit the
stash to a few files is introduced.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-stash.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..2e9e344cd7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -47,8 +47,9 @@ OPTIONS
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
 
-	Save your local modifications to a new 'stash', and run `git reset
-	--hard` to revert them.  The <message> part is optional and gives
+	Save your local modifications to a new 'stash' and roll them
+	back to HEAD (in the working tree and in the index).
+	The <message> part is optional and gives
 	the description along with the stashed state.  For quickly making
 	a snapshot, you can omit _both_ "save" and <message>, but giving
 	only <message> does not trigger this action to prevent a misspelled
-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [PATCH v3 3/5] stash: add test for the create command line arguments
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

Currently there is no test showing the expected behaviour of git stash
create's command line arguments.  Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t3903-stash.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0171b824c9..21cb70dc74 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create stores correct message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create "create test message") &&
+	echo "On master: create test message" >expect &&
+	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create test untracked) &&
+	echo "On master: test untracked" >expect &&
+	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [PATCH v3 0/5] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170129201604.30445-1-t.gummerer@gmail.com>

Thanks Junio for the review in the previous round.

Changes since v2:

- $IFS should now be supported by using "$@" everywhere instead of using
  a $files variable.
- Added a new patch showing the old behaviour of git stash create is
  preserved.
- Rephrased the documentation
- Simplified the option parsing in save_stash, by leaving the
  actual parsing to push_stash instead.

Interdiff below:

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 8306bac397..be55e456fa 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,7 +17,7 @@ SYNOPSIS
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
-	     [--] [<paths>...]
+	     [--] [<pathspec>...]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
@@ -51,19 +51,21 @@ OPTIONS
 -------
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
-push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<paths>...]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
 
 	Save your local modifications to a new 'stash' and roll them
-	back both in the working tree and in the index.
+	back to HEAD (in the working tree and in the index).
 	The <message> part is optional and gives
 	the description along with the stashed state.  For quickly making
 	a snapshot, you can omit _both_ "save" and <message>, but giving
 	only <message> does not trigger this action to prevent a misspelled
 	subcommand from making an unwanted stash.
 +
-If the paths argument is given in 'git stash push', only these files
-are put in the new 'stash'.  In addition only the indicated files are
-changed in the working tree to match the index.
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
 +
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
diff --git a/git-stash.sh b/git-stash.sh
index 0072a38b4c..46367d40aa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
 untracked_files () {
 	excl_opt=--exclude-standard
 	test "$untracked" = "all" && excl_opt=
-	git ls-files -o -z $excl_opt -- $1
+	git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -59,13 +59,12 @@ create_stash () {
 	stash_msg=
 	untracked=
 	new_style=
-	files=
 	while test $# != 0
 	do
 		case "$1" in
 		-m|--message)
 			shift
-			stash_msg="$1"
+			stash_msg=${1?"-m needs an argument"}
 			new_style=t
 			;;
 		-u|--include-untracked)
@@ -75,7 +74,6 @@ create_stash () {
 			;;
 		--)
 			shift
-			files="$@"
 			new_style=t
 			break
 			;;
@@ -106,6 +104,10 @@ create_stash () {
 	if test -z "$new_style"
 	then
 		stash_msg="$*"
+		while test $# != 0
+		do
+			shift
+		done
 	fi
 
 	git update-index -q --refresh
@@ -141,7 +143,7 @@ create_stash () {
 		# Untracked files are stored by themselves in a parentless commit, for
 		# ease of unpacking later.
 		u_commit=$(
-			untracked_files $files | (
+			untracked_files "$@" | (
 				GIT_INDEX_FILE="$TMPindex" &&
 				export GIT_INDEX_FILE &&
 				rm -f "$TMPindex" &&
@@ -164,7 +166,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff-index --name-only -z HEAD -- $files >"$TMP-stagenames" &&
+			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
@@ -178,7 +180,7 @@ create_stash () {
 
 		# find out what the user wants
 		GIT_INDEX_FILE="$TMP-index" \
-			git add--interactive --patch=stash -- $files &&
+			git add--interactive --patch=stash -- "$@" &&
 
 		# state of the working tree
 		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -268,7 +270,7 @@ push_stash () {
 			;;
 		-m|--message)
 			shift
-			stash_msg=$1
+			stash_msg=${1?"-m needs an argument"}
 			;;
 		--help)
 			show_help
@@ -300,8 +302,6 @@ push_stash () {
 		shift
 	done
 
-	files="$*"
-
 	if test -n "$patch_mode" && test -n "$untracked"
 	then
 		die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
@@ -316,14 +316,14 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash -m "$stash_msg" -u "$untracked" -- $files
+	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
 	store_stash -m "$stash_msg" -q $w_commit ||
 	die "$(gettext "Cannot save the current status")"
 	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
 
 	if test -z "$patch_mode"
 	then
-		if test -n "$files"
+		if test $# != 0
 		then
 			git ls-files -z -- "$@" | xargs -0 git reset --
 			git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
@@ -334,7 +334,7 @@ push_stash () {
 		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
 		if test -n "$untracked"
 		then
-			git clean --force --quiet -d $CLEAN_X_OPTION -- $files
+			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
 		if test "$keep_index" = "t" && test -n "$i_tree"
@@ -357,24 +357,6 @@ save_stash () {
 	while test $# != 0
 	do
 		case "$1" in
-		-k|--keep-index)
-			push_options="-k $push_options"
-			;;
-		--no-keep-index)
-			push_options="--no-keep-index $push_options"
-			;;
-		-p|--patch)
-			push_options="-p $push_options"
-			;;
-		-q|--quiet)
-			push_options="-q $push_options"
-			;;
-		-u|--include-untracked)
-			push_options="-u $push_options"
-			;;
-		-a|--all)
-			push_options="-a $push_options"
-			;;
 		--help)
 			show_help
 			;;
@@ -383,20 +365,8 @@ save_stash () {
 			break
 			;;
 		-*)
-			option="$1"
-			# TRANSLATORS: $option is an invalid option, like
-			# `--blah-blah'. The 7 spaces at the beginning of the
-			# second line correspond to "error: ". So you should line
-			# up the second line with however many characters the
-			# translation of "error: " takes in your language. E.g. in
-			# English this is:
-			#
-			#    $ git stash save --blah-blah 2>&1 | head -n 2
-			#    error: unknown option for 'stash save': --blah-blah
-			#           To provide a message, use git stash save -- '--blah-blah'
-			eval_gettextln "error: unknown option for 'stash save': \$option
-       To provide a message, use git stash save -- '\$option'"
-			usage
+			# pass all options through to push_stash
+			push_options="$push_options $1"
 			;;
 		*)
 			break
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ca4c44aa9c..461fe46045 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,7 +784,7 @@ test_expect_success 'push -m shows right message' '
 	test_cmp expect actual
 '
 
-test_expect_success 'deprecated version of stash create stores correct message' '
+test_expect_success 'create stores correct message' '
 	>foo &&
 	git add foo &&
 	STASH_ID=$(git stash create "create test message") &&
@@ -793,6 +793,15 @@ test_expect_success 'deprecated version of stash create stores correct message'
 	test_cmp expect actual
 '
 
+test_expect_success 'create with multiple arguments for the message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create test untracked) &&
+	echo "On master: test untracked" >expect &&
+	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'new style stash create stores correct message' '
 	>foo &&
 	git add foo &&
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..9a98e31a3e 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
 	test -s .gitignore
 '
 
+test_expect_success 'stash push --include-untracked with pathspec' '
+	>foo &&
+	>bar &&
+	git stash push --include-untracked -- foo &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file bar &&
+	test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+	>"foo	bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash push --include-untracked -- foo* &&
+	test_path_is_missing "foo	bar" &&
+	test_path_is_missing foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo	bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
 test_done

Thomas Gummerer (5):
  Documentation/stash: remove mention of git reset --hard
  stash: introduce push verb
  stash: add test for the create command line arguments
  stash: introduce new format create
  stash: teach 'push' (and 'create') to honor pathspec

 Documentation/git-stash.txt        |  17 ++++-
 git-stash.sh                       | 124 +++++++++++++++++++++++++++++++++----
 t/t3903-stash.sh                   |  78 +++++++++++++++++++++++
 t/t3905-stash-include-untracked.sh |  26 ++++++++
 4 files changed, 230 insertions(+), 15 deletions(-)

-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [PATCH v3 4/5] stash: introduce new format create
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

git stash create currently supports a positional argument for adding a
message.  This is not quite in line with how git commands usually take
comments (using a -m flag).

Add a new syntax for adding a message to git stash create using a -m
flag.  This is with the goal of deprecating the old style git stash
create with positional arguments.

This also adds a -u argument, for untracked files.  This is already used
internally as another positional argument, but can now be used from the
command line.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-stash.txt |  1 +
 git-stash.sh                | 50 +++++++++++++++++++++++++++++++++++++++++----
 t/t3903-stash.sh            |  9 ++++++++
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..a138ed6a24 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' clear
 'git stash' create [<message>]
+'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index bf7863a846..33b2d0384c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -56,8 +56,50 @@ clear_stash () {
 }
 
 create_stash () {
-	stash_msg="$1"
-	untracked="$2"
+	stash_msg=
+	untracked=
+	new_style=
+	while test $# != 0
+	do
+		case "$1" in
+		-m|--message)
+			shift
+			stash_msg=${1?"-m needs an argument"}
+			new_style=t
+			;;
+		-u|--include-untracked)
+			shift
+			untracked="$1"
+			new_style=t
+			;;
+		*)
+			if test -n "$new_style"
+			then
+				echo "invalid argument"
+				option="$1"
+				# TRANSLATORS: $option is an invalid option, like
+				# `--blah-blah'. The 7 spaces at the beginning of the
+				# second line correspond to "error: ". So you should line
+				# up the second line with however many characters the
+				# translation of "error: " takes in your language. E.g. in
+				# English this is:
+				#
+				#    $ git stash save --blah-blah 2>&1 | head -n 2
+				#    error: unknown option for 'stash save': --blah-blah
+				#           To provide a message, use git stash save -- '--blah-blah'
+				eval_gettextln "error: unknown option for 'stash create': \$option"
+				usage
+			fi
+			break
+			;;
+		esac
+		shift
+	done
+
+	if test -z "$new_style"
+	then
+		stash_msg="$*"
+	fi
 
 	git update-index -q --refresh
 	if no_changes
@@ -265,7 +307,7 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash "$stash_msg" $untracked
+	create_stash -m "$stash_msg" -u "$untracked"
 	store_stash -m "$stash_msg" -q $w_commit ||
 	die "$(gettext "Cannot save the current status")"
 	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
@@ -667,7 +709,7 @@ clear)
 	;;
 create)
 	shift
-	create_stash "$*" && echo "$w_commit"
+	create_stash "$@" && echo "$w_commit"
 	;;
 store)
 	shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 21cb70dc74..b859e51086 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,13 @@ test_expect_success 'create with multiple arguments for the message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new style stash create stores correct message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create -m "create test message new style") &&
+	echo "On master: create test message new style" >expect &&
+	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [PATCH v3 2/5] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Thomas Gummerer
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

Introduce a new git stash push verb in addition to git stash save.  The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     | 46 +++++++++++++++++++++++++++++++++++++++++++---
 t/t3903-stash.sh |  9 +++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..bf7863a846 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -189,10 +189,11 @@ store_stash () {
 	return $ret
 }
 
-save_stash () {
+push_stash () {
 	keep_index=
 	patch_mode=
 	untracked=
+	stash_msg=
 	while test $# != 0
 	do
 		case "$1" in
@@ -216,6 +217,10 @@ save_stash () {
 		-a|--all)
 			untracked=all
 			;;
+		-m|--message)
+			shift
+			stash_msg=${1?"-m needs an argument"}
+			;;
 		--help)
 			show_help
 			;;
@@ -251,8 +256,6 @@ save_stash () {
 		die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
 	fi
 
-	stash_msg="$*"
-
 	git update-index -q --refresh
 	if no_changes
 	then
@@ -291,6 +294,39 @@ save_stash () {
 	fi
 }
 
+save_stash () {
+	push_options=
+	while test $# != 0
+	do
+		case "$1" in
+		--help)
+			show_help
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			# pass all options through to push_stash
+			push_options="$push_options $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	stash_msg="$*"
+
+	if test -z "$stash_msg"
+	then
+		push_stash $push_options
+	else
+		push_stash $push_options -m "$stash_msg"
+	fi
+}
+
 have_stash () {
 	git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +653,10 @@ save)
 	shift
 	save_stash "$@"
 	;;
+push)
+	shift
+	push_stash "$@"
+	;;
 apply)
 	shift
 	apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..0171b824c9 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' '
 	test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+	>foo &&
+	git add foo &&
+	git stash push -m "test message" &&
+	echo "stash@{0}: On master: test message" >expect &&
+	git stash list | head -n 1 >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.12.0.rc0.208.g81c5d00b20.dirty


^ permalink raw reply related

* [BUG] was: Re: [PATCH] Remove --no-gui option from difftool usage string
From: David Aguilar @ 2017-02-05 20:17 UTC (permalink / raw)
  To: Denton Liu, Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <20170204025617.GA9221@arch-attack.localdomain>


On Fri, Feb 03, 2017 at 06:56:17PM -0800, Denton Liu wrote:
> The --no-gui option not documented in the manpage, nor is it actually
> used in the source code. This change removes it from the usage help
> that's printed.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  git-difftool.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

[Dscho, I found a bug, see below]

I forgot to mention that the scripted difftool has been
rewritten in C and will be going away soon.
builtin/difftool.c is already in "next".

New patches against difftool should target the builtin.

Regarding removing it from the usage string, IMO that can be
considered a good change if the rationale were instead that
we never expect users to ever type "--no-gui" in practice.

Wasting the short usage string screen space with a useless to
99.99% users option is bad for usability.  From that perspective
we shouldn't mention it there, so reframing the commit message
towards that argument would make for a better motivation.

Removing the mention from the usage string and adding it to the
manpage would be the a good change from that perspective as well.


BTW, in "next", it seems that the builtin difftool crashes when
doing "git difftool -h", so we should probably add a test
for that too...

From the tip of next:

$ git difftool -h
fatal: BUG: setup_git_env called without repository
-- 
David

^ permalink raw reply

* Re: git-scm.com status report
From: Pranit Bauva @ 2017-02-05 20:11 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List
In-Reply-To: <20170202023349.7fopb3a6pc6dkcmd@sigill.intra.peff.net>

Hey Peff,

On Thu, Feb 2, 2017 at 8:03 AM, Jeff King <peff@peff.net> wrote:
> ## What's on the site
>
> We have the domains git-scm.com and git-scm.org (the latter we've had
> for a while). They both point to the same website, which has general
> information about Git, including:

Since we have an "official" control over the website, shouldn't we be
using the .org domain more because we are more of an organization?
What I mean is that in many places, we have referred to git-scm.com,
which was perfectly fine because it was done by github which is a
company but now I think it would be more appropriate to use
git-scm.org domain. We can forward all .com requests to .org and try
to move all reference we know about, to .org. What do you all think?

Regards,
Pranit Bauva

^ permalink raw reply

* Re: Git clonebundles
From: Christian Couder @ 2017-02-05 16:37 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Stefan Saasen, Git Mailing List
In-Reply-To: <CAJo=hJsS6FmL9iNScaXqkWJumALfGr8Od5QkbfZ+ZG3osxkp7Q@mail.gmail.com>

On Sat, Feb 4, 2017 at 6:39 PM, Shawn Pearce <spearce@spearce.org> wrote:
> On Mon, Jan 30, 2017 at 11:00 PM, Stefan Saasen <ssaasen@atlassian.com> wrote:
>>
>> Bitbucket recently added support for Mercurial’s clonebundle extension
>> (http://gregoryszorc.com/blog/2015/10/22/cloning-improvements-in-mercurial-3.6/).
>> Mercurial’s clone bundles allow the Mercurial client to seed a repository using
>> a bundle file instead of dynamically generating a bundle for the client.
> ...
>> Prior art
>> ~~~~~~~~~
>>
>> Our proof-of-concept is built on top of ideas that have been
>> circulating for a while. We are aware of a number of proposed changes
>> in this space:
>>
>>
>> * Jeff King's work on network bundles:
>> https://github.com/peff/git/commit/17e2409df37edd0c49ef7d35f47a7695f9608900
>> * Nguyễn Thái Ngọc Duy's work on "[PATCH 0/8] Resumable clone
>> revisited, proof of concept":
>> https://www.spinics.net/lists/git/msg267260.html
>> * Resumable clone work by Kevin Wern:
>> https://public-inbox.org/git/1473984742-12516-1-git-send-email-kevin.m.wern@gmail.com/
>
> I think you missed the most common deployment of prior art, which is
> Android using the git-repo tool[1]. The git-repo tool has had
> clone.bundle support since Sep 2011[2] and the Android Git servers
> have been answering /clone.bundle requests[3] since just before that.
> The bundle files are generated with `git bundle create` on a regular
> schedule by cron.
>
> [1] https://gerrit.googlesource.com/git-repo/+/04071c1c72437a930db017bd4c562ad06087986a/project.py#2091
> [2] https://gerrit.googlesource.com/git-repo/+/f322b9abb4cadc67b991baf6ba1b9f2fbd5d7812
> [3] https://android.googlesource.com/platform/frameworks/base/clone.bundle

There is also Junio's work on Bundle v3 that was unfortunately
recently discarded.
Look for "jc/bundle" in:

http://public-inbox.org/git/xmqq4m0cry60.fsf@gitster.mtv.corp.google.com/

and previous "What's cooking in git.git" emails.

I am also working on adding external object database support using
previous work by Peff:

http://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/

that could be extended to support clone bundles.

[...]

^ permalink raw reply

* Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
From: Pranit Bauva @ 2017-02-05 14:55 UTC (permalink / raw)
  To: Siddharth Kannan
  Cc: Git List, Matthieu.Moy, Jeff King, Junio C Hamano, Duy Nguyen,
	Brian M. Carlson
In-Reply-To: <1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com>

Hey Siddharth,

On Sun, Feb 5, 2017 at 6:27 PM, Siddharth Kannan
<kannan.siddharth12@gmail.com> wrote:
> Search and replace "-" (written in the context of a branch name) in the argument
> list with "@{-1}". As per the help text of git rev-list, this includes the following four
> cases:
>
>   a. "-"
>   b. "^-"
>   c. "-..other-branch-name" or "other-branch-name..-"
>   d. "-...other-branch-name" or "other-branch-name...-"
>
> (a) and (b) have been implemented as in the previous implementations of
> this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
> docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
> abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
> short-hand for "previous branch", 2011-04-07)
>
> (c) and (d) have been implemented by using the strbuf API, growing it to the
> right size and placing "@{-1}" instead of "-"
>
> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
> ---
> This is a patch for one of the microprojects of SoC 2017. [1]
>
> I have implemented this using multiple methods, that I have re-written again and
> again for better versions ([2]). The present version I feel is the closest that
> I could get to the existing code in the repository. This patch only uses
> functions that are commonly used in the rest of the codebase.
>
> I still have to write tests, as well as update documentation as done in 696acf45
> (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).
>
> I request your comments on this patch. Also, I have the following questions
> regarding this patch:
>
> 1. Is the approach that I have used to solve this problem fine?
> 2. Is the code I am writing in the right function? (I have put it right
> before the revisions data structure is setup, thus these changes affect only
> git-log)
>
> [1]: https://git.github.io/SoC-2017-Microprojects/
> [2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
> strbufs for the starting 4 characters, and last 4 characters and compares those
> to the appropriate strings for case (c) and case (d). I edited this patch to use
> strstr instead, which avoids all the strbuf declarations)
>
>  builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 55d20cc..a5aac99 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>                          struct rev_info *rev, struct setup_revision_opt *opt)
>  {
>         struct userformat_want w;
> -       int quiet = 0, source = 0, mailmap = 0;
> +       int quiet = 0, source = 0, mailmap = 0, i = 0;
>         static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
>
>         const struct option builtin_log_options[] = {
> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>
>         if (quiet)
>                 rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> +
> +       /*
> +        * Check if any argument has a "-" in it, which has been referred to as a
> +        * shorthand for @{-1}.  Handles methods that might be used to list commits
> +        * as mentioned in git rev-list --help
> +        */
> +
> +       for(i = 0; i < argc; ++i) {
> +               if (!strcmp(argv[i], "-")) {
> +                       argv[i] = "@{-1}";
> +               } else if (!strcmp(argv[i], "^-")) {
> +                       argv[i] = "^@{-1}";
> +               } else if (strlen(argv[i]) >= 4) {
> +
> +                       if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, "@{-1}");
> +                               strbuf_addstr(&changed_argument, argv[i] + 1);
> +
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +
> +                       /*
> +                        * Find the first occurence, and add the size to it and proceed if
> +                        * the resulting value is NULL
> +                        */
> +                       if (!(strstr(argv[i], "...-") + 4)  ||
> +                                       !(strstr(argv[i], "..-") + 3)) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, argv[i]);
> +
> +                               strbuf_grow(&changed_argument, strlen(argv[i]) + 4);
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +               }
> +       }
> +
>         argc = setup_revisions(argc, argv, rev, opt);
>
>         /* Any arguments at this point are not recognized */
> --


It is highly recommended to follow the pre existing style of code and
commits. In the micro project list, I think it is mentioned that this
similar thing is implemented in git-merge so you should try and dig
the commit history of that file to find the similar change.

If you do this, then you will find out that there is a very short and
sweet way to do it. I won't directly point out the commit.

strbuf API should be used when you need to modify the contents of the
string. I think you have a little confusion.

If you declare the string as,

const char *str = "foo";

then, you can also do,

str = "bar";

But you can't do,

str[1] = 'z';

I hope you get what I am saying, if not, search for it.

Regards,
Pranit Bauva

^ permalink raw reply

* Re: [RFC] Add support for downloading blobs on demand
From: Christian Couder @ 2017-02-05 14:03 UTC (permalink / raw)
  To: Ben Peart; +Cc: Jeff King, git, Johannes Schindelin
In-Reply-To: <002601d2710b$c3715890$4a5409b0$@gmail.com>

(Sorry for the late reply and thanks to Dscho for pointing me to this thread.)

On Tue, Jan 17, 2017 at 10:50 PM, Ben Peart <peartben@gmail.com> wrote:
>> From: Jeff King [mailto:peff@peff.net]
>> On Fri, Jan 13, 2017 at 10:52:53AM -0500, Ben Peart wrote:
>>
>> > Clone and fetch will pass a  --lazy-clone  flag (open to a better name
>> > here) similar to  --depth  that instructs the server to only return
>> > commits and trees and to ignore blobs.
>> >
>> > Later during git operations like checkout, when a blob cannot be found
>> > after checking all the regular places (loose, pack, alternates, etc),
>> > git will download the missing object and place it into the local
>> > object store (currently as a loose object) then resume the operation.
>>
>> Have you looked at the "external odb" patches I wrote a while ago, and
>> which Christian has been trying to resurrect?
>>
>>   https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpublic-
>> inbox.org%2Fgit%2F20161130210420.15982-1-
>> chriscool%40tuxfamily.org%2F&data=02%7C01%7CBen.Peart%40microsoft.c
>> om%7C9596d3bf32564f123e0c08d43f08a9e1%7C72f988bf86f141af91ab2d7c
>> d011db47%7C1%7C0%7C636202753822020527&sdata=a6%2BGOAQoRhjFoxS
>> vftY8JZAVUssmrXuDZ9OBy3xqNZk%3D&reserved=0
>>
>> This is a similar approach, though I pushed the policy for "how do you get the
>> objects" out into an external script. One advantage there is that large objects
>> could easily be fetched from another source entirely (e.g., S3 or equivalent)
>> rather than the repo itself.
>>
>> The downside is that it makes things more complicated, because a push or a
>> fetch now involves three parties (server, client, and the alternate object
>> store). So questions like "do I have all the objects I need" are hard to reason
>> about.
>>
>> If you assume that there's going to be _some_ central Git repo which has all
>> of the objects, you might as well fetch from there (and do it over normal git
>> protocols). And that simplifies things a bit, at the cost of being less flexible.
>
> We looked quite a bit at the external odb patches, as well as lfs and
> even using alternates.  They all share a common downside that you must
> maintain a separate service that contains _some_ of the files.

Pushing the policy for "how do you get the objects" out into an
external helper doesn't mean that the external helper cannot use the
main service.
The external helper is still free to do whatever it wants including
calling the main service if it thinks it's better.

> These
> files must also be versioned, replicated, backed up and the service
> itself scaled out to handle the load.  As you mentioned, having multiple
> services involved increases flexability but it also increases the
> complexity and decreases the reliability of the overall version control
> service.

About reliability, I think it depends a lot on the use case. If you
want to get very big files over an unreliable connection, it can
better if you send those big files over a restartable protocol and
service like HTTP/S on a regular web server.

> For operational simplicity, we opted to go with a design that uses a
> single, central git repo which has _all_ the objects and to focus on
> enhancing it to handle large numbers of files efficiently.  This allows
> us to focus our efforts on a great git service and to avoid having to
> build out these other services.

Ok, but I don't think it prevents you from using at least some of the
same mechanisms that the external odb series is using.
And reducing the number of mechanisms in Git itself is great for its
maintainability and simplicity.

>> > To prevent git from accidentally downloading all missing blobs, some
>> > git operations are updated to be aware of the potential for missing blobs.
>> > The most obvious being check_connected which will return success as if
>> > everything in the requested commits is available locally.
>>
>> Actually, Git is pretty good about trying not to access blobs when it doesn't
>> need to. The important thing is that you know enough about the blobs to
>> fulfill has_sha1_file() and sha1_object_info() requests without actually
>> fetching the data.
>>
>> So the client definitely needs to have some list of which objects exist, and
>> which it _could_ get if it needed to.

Yeah, and the external odb series handles that already, thanks to
Peff's initial work.

>> The one place you'd probably want to tweak things is in the diff code, as a
>> single "git log -Sfoo" would fault in all of the blobs.
>
> It is an interesting idea to explore how we could be smarter about
> preventing blobs from faulting in if we had enough info to fulfill
> has_sha1_file() and sha1_object_info().  Given we also heavily prune the
> working directory using sparse-checkout, this hasn't been our top focus
> but it is certainly something worth looking into.

The external odb series doesn't handle preventing blobs from faulting
in yet, so this could be a common problem.

[...]

>> One big hurdle to this approach, no matter the protocol, is how you are
>> going to handle deltas. Right now, a git client tells the server "I have this
>> commit, but I want this other one". And the server knows which objects the
>> client has from the first, and which it needs from the second. Moreover, it
>> knows that it can send objects in delta form directly from disk if the other
>> side has the delta base.
>>
>> So what happens in this system? We know we don't need to send any blobs
>> in a regular fetch, because the whole idea is that we only send blobs on
>> demand. So we wait for the client to ask us for blob A. But then what do we
>> send? If we send the whole blob without deltas, we're going to waste a lot of
>> bandwidth.
>>
>> The on-disk size of all of the blobs in linux.git is ~500MB. The actual data size
>> is ~48GB. Some of that is from zlib, which you get even for non-deltas. But
>> the rest of it is from the delta compression. I don't think it's feasible to give
>> that up, at least not for "normal" source repos like linux.git (more on that in
>> a minute).
>>
>> So ideally you do want to send deltas. But how do you know which objects
>> the other side already has, which you can use as a delta base? Sending the
>> list of "here are the blobs I have" doesn't scale. Just the sha1s start to add
>> up, especially when you are doing incremental fetches.

To initialize some paths that the client wants, it could perhaps just
ask for some pack files, or maybe bundle files, related to these
paths.
Those packs or bundles could be downloaded either directly from the
main server or from other web or proxy servers.

>> I think this sort of things performs a lot better when you just focus on large
>> objects. Because they don't tend to delta well anyway, and the savings are
>> much bigger by avoiding ones you don't want. So a directive like "don't
>> bother sending blobs larger than 1MB" avoids a lot of these issues. In other
>> words, you have some quick shorthand to communicate between the client
>> and server: this what I have, and what I don't.
>> Normal git relies on commit reachability for that, but there are obviously
>> other dimensions. The key thing is that both sides be able to express the
>> filters succinctly, and apply them efficiently.
>
> Our challenge has been more the sheer _number_ of files that exist in
> the repo rather than the _size_ of the files in the repo.  With >3M
> source files and any typical developer only needing a small percentage
> of those files to do their job, our focus has been pruning the tree as
> much as possible such that they only pay the cost for the files they
> actually need.  With typical text source files being 10K - 20K in size,
> the overhead of the round trip is a significant part of the overall
> transfer time so deltas don't help as much.  I agree that large files
> are also a problem but it isn't my top focus at this point in time.

Ok, but it would be nice if both problems could be solved using some
common mechanisms.
This way it could probably work better in situations where there are
both a large number of files _and_ some big files.
And from what I am seeing, there could be no real downside from using
some common mechanisms.

>> If most of your benefits are not from avoiding blobs in general, but rather
>> just from sparsely populating the tree, then it sounds like sparse clone might
>> be an easier path forward. The general idea is to restrict not just the
>> checkout, but the actual object transfer and reachability (in the tree
>> dimension, the way shallow clone limits it in the time dimension, which will
>> require cooperation between the client and server).
>>
>> So that's another dimension of filtering, which should be expressed pretty
>> succinctly: "I'm interested in these paths, and not these other ones." It's
>> pretty easy to compute on the server side during graph traversal (though it
>> interacts badly with reachability bitmaps, so there would need to be some
>> hacks there).
>>
>> It's an idea that's been talked about many times, but I don't recall that there
>> were ever working patches. You might dig around in the list archive under
>> the name "sparse clone" or possibly "narrow clone".
>
> While a sparse/narrow clone would work with this proposal, it isn't
> required.  You'd still probably want all the commits and trees but the
> clone would also bring down the specified blobs.  Combined with using
> "depth" you could further limit it to those blobs at tip.
>
> We did run into problems with this model however as our usage patterns
> are such that our working directories often contain very sparse trees
> and as a result, we can end up with thousands of entries in the sparse
> checkout file.  This makes it difficult for users to manually specify a
> sparse-checkout before they even do a clone.  We have implemented a
> hashmap based sparse-checkout to deal with the performance issues of
> having that many entries but that's a different RFC/PATCH.  In short, we
> found that a "lazy-clone" and downloading blobs on demand provided a
> better developer experience.

I think both ways are possible using the external odb mechanism.

>> > Future Work
>> > ~~~~~~~~~~~
>> >
>> > The current prototype calls a new hook proc in
>> > sha1_object_info_extended and read_object, to download each missing
>> > blob.  A better solution would be to implement this via a long running
>> > process that is spawned on the first download and listens for requests
>> > to download additional objects until it terminates when the parent git
>> > operation exits (similar to the recent long running smudge and clean filter
>> work).
>>
>> Yeah, see the external-odb discussion. Those prototypes use a process per
>> object, but I think we all agree after seeing how the git-lfs interface has
>> scaled that this is a non-starter. Recent versions of git-lfs do the single-
>> process thing, and I think any sort of external-odb hook should be modeled
>> on that protocol.

I agree that the git-lfs scaling work is great, but I think it's not
necessary in the external odb work to have the same kind of
single-process protocol from the beginning (though it should be
possible and easy to add it).
For example if the external odb work can be used or extended to handle
restartable clone by downloading a single bundle when cloning, this
would not need that kind of protocol.

> I'm looking into this now and plan to re-implement it this way before
> sending out the first patch series.  Glad to hear you think it is a good
> protocol to model it on.

Yeah, for your use case on Windows, it looks really worth it to use
this kind of protocol.

>> > Need to investigate an alternate batching scheme where we can make a
>> > single request for a set of "related" blobs and receive single a
>> > packfile (especially during checkout).
>>
>> I think this sort of batching is going to be the really hard part to retrofit onto
>> git. Because you're throwing out the procedural notion that you can loop
>> over a set of objects and ask for each individually.
>> You have to start deferring computation until answers are ready. Some
>> operations can do that reasonably well (e.g., checkout), but something like
>> "git log -p" is constantly digging down into history. I suppose you could just
>> perform the skeleton of the operation _twice_, once to find the list of objects
>> to fault in, and the second time to actually do it.

In my opinion, perhaps we can just prevent "git log -p" from faulting
in blobs and have it show a warning saying that it was performed only
on a subset of all the blobs.

[...]

^ permalink raw reply

* [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
From: Siddharth Kannan @ 2017-02-05 12:57 UTC (permalink / raw)
  To: git
  Cc: pranit.bauva, Matthieu.Moy, peff, gitster, pclouds, sandals,
	Siddharth Kannan

Search and replace "-" (written in the context of a branch name) in the argument
list with "@{-1}". As per the help text of git rev-list, this includes the following four
cases:

  a. "-"
  b. "^-"
  c. "-..other-branch-name" or "other-branch-name..-"
  d. "-...other-branch-name" or "other-branch-name...-"

(a) and (b) have been implemented as in the previous implementations of
this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
short-hand for "previous branch", 2011-04-07)

(c) and (d) have been implemented by using the strbuf API, growing it to the
right size and placing "@{-1}" instead of "-"

Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
This is a patch for one of the microprojects of SoC 2017. [1]

I have implemented this using multiple methods, that I have re-written again and
again for better versions ([2]). The present version I feel is the closest that
I could get to the existing code in the repository. This patch only uses
functions that are commonly used in the rest of the codebase.

I still have to write tests, as well as update documentation as done in 696acf45
(checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).

I request your comments on this patch. Also, I have the following questions
regarding this patch:

1. Is the approach that I have used to solve this problem fine?
2. Is the code I am writing in the right function? (I have put it right
before the revisions data structure is setup, thus these changes affect only
git-log)

[1]: https://git.github.io/SoC-2017-Microprojects/
[2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
strbufs for the starting 4 characters, and last 4 characters and compares those
to the appropriate strings for case (c) and case (d). I edited this patch to use
strstr instead, which avoids all the strbuf declarations)

 builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..a5aac99 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
 	struct userformat_want w;
-	int quiet = 0, source = 0, mailmap = 0;
+	int quiet = 0, source = 0, mailmap = 0, i = 0;
 	static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};

 	const struct option builtin_log_options[] = {
@@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,

 	if (quiet)
 		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
+
+	/*
+	 * Check if any argument has a "-" in it, which has been referred to as a
+	 * shorthand for @{-1}.  Handles methods that might be used to list commits
+	 * as mentioned in git rev-list --help
+	 */
+
+	for(i = 0; i < argc; ++i) {
+		if (!strcmp(argv[i], "-")) {
+			argv[i] = "@{-1}";
+		} else if (!strcmp(argv[i], "^-")) {
+			argv[i] = "^@{-1}";
+		} else if (strlen(argv[i]) >= 4) {
+
+			if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) {
+				struct strbuf changed_argument = STRBUF_INIT;
+
+				strbuf_addstr(&changed_argument, "@{-1}");
+				strbuf_addstr(&changed_argument, argv[i] + 1);
+
+				strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
+
+				argv[i] = strbuf_detach(&changed_argument, NULL);
+			}
+
+			/*
+			 * Find the first occurence, and add the size to it and proceed if
+			 * the resulting value is NULL
+			 */
+			if (!(strstr(argv[i], "...-") + 4)  ||
+					!(strstr(argv[i], "..-") + 3)) {
+				struct strbuf changed_argument = STRBUF_INIT;
+
+				strbuf_addstr(&changed_argument, argv[i]);
+
+				strbuf_grow(&changed_argument, strlen(argv[i]) + 4);
+				strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
+
+				strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5);
+
+				argv[i] = strbuf_detach(&changed_argument, NULL);
+			}
+		}
+	}
+
 	argc = setup_revisions(argc, argv, rev, opt);

 	/* Any arguments at this point are not recognized */
--
2.1.4


^ permalink raw reply related

* Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
From: Thomas Gummerer @ 2017-02-05 12:13 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
	Øyvind A. Holm, Jakub Narębski
In-Reply-To: <xmqq7f5cuu8l.fsf@gitster.mtv.corp.google.com>

On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Don't mention git reset --hard in the documentation for git stash save.
> > It's an implementation detail that doesn't matter to the end user and
> > thus shouldn't be exposed to them.
> 
> Everybody understands what "reset --hard" does; it can be an
> easier-to-read "short-hand" description to say "reset --hard"
> instead of giving a lengthy description of what happens.

While this is definitely true for experienced git users, it might not
be for some people relatively new to git, which are probably the ones
that need the description most.

> Because of that, I do not necessarily agree with the above
> justification.  I'll read the remainder of the series before
> commenting further on the above.
> 
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index 2e9cef06e6..0fc23c25ee 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -47,8 +47,9 @@ OPTIONS
> >  
> >  save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
> >  
> > -	Save your local modifications to a new 'stash', and run `git reset
> > -	--hard` to revert them.  The <message> part is optional and gives
> > +	Save your local modifications to a new 'stash' and roll them
> > +	back both in the working tree and in the index.
> 
> "... roll them back both ..." is unclear where they are rolled back
> to.  
> 
> Perhaps "roll them back ... to HEAD" or something?

Yeah that makes sense, thanks.

-- 
Thomas

^ permalink raw reply

* [PATCH] p5302: create repositories for index-pack results explicitly
From: René Scharfe @ 2017-02-05 11:43 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Git List

Before 7176a314 (index-pack: complain when --stdin is used outside of a
repo) index-pack silently created a non-existing target directory; now
the command refuses to work unless it's used against a valid repository.
That causes p5302 to fail, which relies on the former behavior.  Fix it
by setting up the destinations for its performance tests using git init.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/perf/p5302-pack-index.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh
index 5ee9211f98..99bdb16c85 100755
--- a/t/perf/p5302-pack-index.sh
+++ b/t/perf/p5302-pack-index.sh
@@ -13,6 +13,13 @@ test_expect_success 'repack' '
 	export PACK
 '
 
+test_expect_success 'create target repositories' '
+	for repo in t1 t2 t3 t4 t5 t6
+	do
+		git init --bare $repo
+	done
+'
+
 test_perf 'index-pack 0 threads' '
 	GIT_DIR=t1 git index-pack --threads=1 --stdin < $PACK
 '
-- 
2.11.1


^ permalink raw reply related

* Re: [PATCH v2 4/4] stash: support filename argument
From: Thomas Gummerer @ 2017-02-05 11:02 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
	Øyvind A. Holm, Jakub Narębski
In-Reply-To: <xmqqa8a8uuc9.fsf@gitster.mtv.corp.google.com>

On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Add an optional filename argument to git stash push, which allows for
> > stashing a single (or multiple) files.
> 
> You can give pathspec with one or more elements, so "an optional
> argument" sounds too limiting.  
> 
>     Allow 'git stash push' to take pathspec to specify which paths
>     to stash.
> 
> Also retitle
> 
> 	stash: teach 'push' (and 'create') to honor pathspec
> 
> or something.
> 
> > @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> >  	only <message> does not trigger this action to prevent a misspelled
> >  	subcommand from making an unwanted stash.
> >  +
> > +If the paths argument is given in 'git stash push', only these files
> > +are put in the new 'stash'.  In addition only the indicated files are
> > +changed in the working tree to match the index.
> 
> Actually the stash contains "all paths".  You could say that you are
> placing _modifications_ to these paths in stash, even though that is
> not how Git's world model works (i.e. everything is a snapshot, and
> modifications are merely difference between two successive
> snapshots).  A technically correct version may be:
> 
> 	When pathspec is given to 'git stash push', the new stash
> 	records the modified states only for the files that match
> 	the pathspec.  The index entries and working tree files are
> 	then rolled back to the state in HEAD only for these files,
> 	too, leaving files that do not match the pathspec intact.
> 
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 5f08b43967..0072a38b4c 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -41,7 +41,7 @@ no_changes () {
> >  untracked_files () {
> >  	excl_opt=--exclude-standard
> >  	test "$untracked" = "all" && excl_opt=
> > -	git ls-files -o -z $excl_opt
> > +	git ls-files -o -z $excl_opt -- $1
> 
> Hmph, why "$1" is spelled without dq, implying that it is split at
> $IFS boundary?  This line alone makes me suspect that this is not
> prepared to deal correctly with $IFS.  Let's read on...
> 
> > @@ -59,6 +59,7 @@ create_stash () {
> >  	stash_msg=
> >  	untracked=
> >  	new_style=
> > +	files=
> >  	while test $# != 0
> >  	do
> >  		case "$1" in
> > @@ -72,6 +73,12 @@ create_stash () {
> >  			untracked="$1"
> >  			new_style=t
> >  			;;
> > +		--)
> > +			shift
> > +			files="$@"
> 
> Isn't this the same as writing files="$*", i.e. concatenate the
> multiple arguments with the first whitespace in $IFS in between?
> 
> > @@ -134,7 +141,7 @@ create_stash () {
> >  		# Untracked files are stored by themselves in a parentless commit, for
> >  		# ease of unpacking later.
> >  		u_commit=$(
> > -			untracked_files | (
> > +			untracked_files $files | (
> 
> ... and this lets it split at $IFS again when passing it down to the
> helper.  But the helper looks only at $1 so the second and subsequent
> ones will be ignored altogether.
> 
> This cannot be correct, and any hunk in the remainder of the patch
> that mentions $files will be incorrect for the same reason.
> 
> Is it possible to carry what the caller (and the end user) gave you
> in "$@" without molesting it at all?  That would mean you do not
> need to introduce $files variable at all, and then the places that
> do things like this:
> 
> > -	create_stash -m "$stash_msg" -u "$untracked"
> > +	create_stash -m "$stash_msg" -u "$untracked" -- $files
> 
> can instead do
> 
> 	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
> 
> That would allow you to work correctly with pathspec with $IFS
> whitespaces in them.

Thanks for taking the time for this explanation!  It cleared quite a
few things up in my head.  I'll make these fixes in the re-roll.

-- 
Thomas

^ permalink raw reply

* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Duy Nguyen @ 2017-02-05 10:39 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xoMTX5n_h+5MytZwVqKjqa0wdNKCeDtH29A_+WSfr6gTQ@mail.gmail.com>

On Sat, Jan 21, 2017 at 2:16 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> I would be interested in the code for this.. I'm curious if I can
> adapt it to my use of tmux.

I stumbled upon this which does mention about git SHA-1. Maybe you'll
find it useful. Haven't tried it out though.

https://github.com/morantron/tmux-fingers

-- 
Duy

^ permalink raw reply

* Re: [PATCH] Remove --no-gui option from difftool usage string
From: David Aguilar @ 2017-02-05 10:22 UTC (permalink / raw)
  To: Denton Liu; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <20170204062351.GA11277@arch-attack.localdomain>

On Fri, Feb 03, 2017 at 10:23:51PM -0800, Denton Liu wrote:
> On Fri, Feb 03, 2017 at 09:58:09PM -0800, Jacob Keller wrote:
> > On Fri, Feb 3, 2017 at 6:56 PM, Denton Liu <liu.denton@gmail.com> wrote:
> > > The --no-gui option not documented in the manpage, nor is it actually
> > > used in the source code. This change removes it from the usage help
> > > that's printed.
> > >
> > > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > > ---
> > >  git-difftool.perl | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/git-difftool.perl b/git-difftool.perl
> > > index a5790d03a..657d5622d 100755
> > > --- a/git-difftool.perl
> > > +++ b/git-difftool.perl
> > > @@ -29,8 +29,8 @@ sub usage
> > >         print << 'USAGE';
> > >  usage: git difftool [-t|--tool=<tool>] [--tool-help]
> > >                      [-x|--extcmd=<cmd>]
> > > -                    [-g|--gui] [--no-gui]
> > > -                    [--prompt] [-y|--no-prompt]
> > > +                    [-g|--gui] [--prompt]
> > > +                    [-y|--no-prompt]
> > >                      [-d|--dir-diff]
> > >                      ['git diff' options]
> > >  USAGE
> > > --
> > > 2.11.0
> > >
> > 
> > Aren't "--no-foo" options automatically created for booleans when
> > using our option parsing code?
> > 
> > Thanks,
> > Jake
> 
> Sorry, I guess I didn't notice that. Would it make sense to document it
> in the manpage then?

The general "--no-*" convention is mentioned in "git help cli",
but the manpages and usage strings across all commands are
inconsistent about mentioning the "--no-*" variants; some do,
some don't.

IMO it probably wouldn't hurt to document --no-gui in the manpage.

cheers,
-- 
David

^ permalink raw reply

* Re: [PATCH] Add --gui option to mergetool
From: David Aguilar @ 2017-02-05 10:18 UTC (permalink / raw)
  To: Denton Liu; +Cc: git
In-Reply-To: <20170204064303.GA14990@arch-attack.localdomain>

On Fri, Feb 03, 2017 at 10:43:03PM -0800, Denton Liu wrote:
> * fix the discrepancy between difftool and mergetool where
>   the former has the --gui flag and the latter does not by adding the
>   functionality to mergetool

Please avoid bullet points in commit messages when a simple
paragraph will suffice.

The implementation of this feature seems ok, but tests are
needed in t/t7610-mergetool.sh.

> * make difftool read 'merge.guitool' as a fallback, in accordance to the
>   manpage for difftool: "git difftool falls back to git mergetool
>   config variables when the difftool equivalents have not been defined"

I did not spot this change in the code.

Nonetheless, this should be split off as a separate patch, and
tests should be added.

> * add guitool-related completions

This should be split off as a separate patch as well.

Generally, 3 bullet points suggests there should be 3 patches in
this series.

Thanks,
-- 
David

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox