* [PATCH v4 3/6] wt-status: make the require_clean_work_tree() function reusable
From: Johannes Schindelin @ 2016-10-07 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475856491.git.johannes.schindelin@gmx.de>
The function used by "git pull" to stop the user when the working
tree has changes is useful in other places.
Let's move it into a more prominent (and into an actually reusable)
spot: wt-status.[ch].
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 77 +---------------------------------------------------------
wt-status.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
wt-status.h | 3 +++
3 files changed, 80 insertions(+), 76 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 58fc176..01b6465 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
#include "revision.h"
#include "tempfile.h"
#include "lockfile.h"
+#include "wt-status.h"
enum rebase_type {
REBASE_INVALID = -1,
@@ -326,82 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
}
/**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
- struct rev_info rev_info;
- int result;
-
- init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
- DIFF_OPT_SET(&rev_info.diffopt, QUICK);
- diff_setup_done(&rev_info.diffopt);
- result = run_diff_files(&rev_info, 0);
- return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
- struct rev_info rev_info;
- int result;
-
- if (is_cache_unborn())
- return 0;
-
- init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
- DIFF_OPT_SET(&rev_info.diffopt, QUICK);
- add_head_to_pending(&rev_info);
- diff_setup_done(&rev_info.diffopt);
- result = run_diff_index(&rev_info, 1);
- return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
- int gently)
-{
- struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
- int err = 0;
-
- hold_locked_index(lock_file, 0);
- refresh_cache(REFRESH_QUIET);
- update_index_if_able(&the_index, lock_file);
- rollback_lock_file(lock_file);
-
- if (has_unstaged_changes()) {
- /* TRANSLATORS: the action is e.g. "pull with rebase" */
- error(_("Cannot %s: You have unstaged changes."), _(action));
- err = 1;
- }
-
- if (has_uncommitted_changes()) {
- if (err)
- error(_("Additionally, your index contains uncommitted changes."));
- else
- error(_("Cannot %s: Your index contains uncommitted changes."),
- _(action));
- err = 1;
- }
-
- if (err) {
- if (hint)
- error("%s", hint);
- if (!gently)
- exit(128);
- }
-
- return err;
-}
-
-/**
* Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
* into merge_heads.
*/
diff --git a/wt-status.c b/wt-status.c
index 99d1b0a..89475f1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
#include "strbuf.h"
#include "utf8.h"
#include "worktree.h"
+#include "lockfile.h"
static const char cut_line[] =
"------------------------ >8 ------------------------\n";
@@ -2208,3 +2209,78 @@ void wt_status_print(struct wt_status *s)
break;
}
}
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+ struct rev_info rev_info;
+ int result;
+
+ init_revisions(&rev_info, NULL);
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+ diff_setup_done(&rev_info.diffopt);
+ result = run_diff_files(&rev_info, 0);
+ return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+ struct rev_info rev_info;
+ int result;
+
+ if (is_cache_unborn())
+ return 0;
+
+ init_revisions(&rev_info, NULL);
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+ add_head_to_pending(&rev_info);
+ diff_setup_done(&rev_info.diffopt);
+ result = run_diff_index(&rev_info, 1);
+ return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+ struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+ int err = 0;
+
+ hold_locked_index(lock_file, 0);
+ refresh_cache(REFRESH_QUIET);
+ update_index_if_able(&the_index, lock_file);
+ rollback_lock_file(lock_file);
+
+ if (has_unstaged_changes()) {
+ /* TRANSLATORS: the action is e.g. "pull with rebase" */
+ error(_("Cannot %s: You have unstaged changes."), _(action));
+ err = 1;
+ }
+
+ if (has_uncommitted_changes()) {
+ if (err)
+ error(_("Additionally, your index contains uncommitted changes."));
+ else
+ error(_("Cannot %s: Your index contains uncommitted changes."),
+ _(action));
+ err = 1;
+ }
+
+ if (err) {
+ if (hint)
+ error("%s", hint);
+ if (!gently)
+ exit(128);
+ }
+
+ return err;
+}
diff --git a/wt-status.h b/wt-status.h
index e401837..68b4709 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,4 +128,7 @@ void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, .
__attribute__((format (printf, 3, 4)))
void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+/* The following function expects that the caller took care of reading the index. */
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
#endif /* STATUS_H */
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v4 2/6] pull: make code more similar to the shell script again
From: Johannes Schindelin @ 2016-10-07 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475856491.git.johannes.schindelin@gmx.de>
When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.
This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.
Note: when the hint "Please commit or stash them" was introduced first,
Git did not have the convention of continuing error messages in lower
case, but now we do have that convention, therefore we reintroduce this
hint down-cased, obeying said convention.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..58fc176 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
* If the work tree has unstaged or uncommitted changes, dies with the
* appropriate message.
*/
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+ int gently)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
- int do_die = 0;
+ int err = 0;
hold_locked_index(lock_file, 0);
refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,28 @@ static void die_on_unclean_work_tree(void)
rollback_lock_file(lock_file);
if (has_unstaged_changes()) {
- error(_("Cannot pull with rebase: You have unstaged changes."));
- do_die = 1;
+ /* TRANSLATORS: the action is e.g. "pull with rebase" */
+ error(_("Cannot %s: You have unstaged changes."), _(action));
+ err = 1;
}
if (has_uncommitted_changes()) {
- if (do_die)
+ if (err)
error(_("Additionally, your index contains uncommitted changes."));
else
- error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
- do_die = 1;
+ error(_("Cannot %s: Your index contains uncommitted changes."),
+ _(action));
+ err = 1;
}
- if (do_die)
- exit(1);
+ if (err) {
+ if (hint)
+ error("%s", hint);
+ if (!gently)
+ exit(128);
+ }
+
+ return err;
}
/**
@@ -875,7 +884,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
die(_("Updating an unborn branch with changes added to the index."));
if (!autostash)
- die_on_unclean_work_tree();
+ require_clean_work_tree(N_("pull with rebase"),
+ _("please commit or stash them."), 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
From: Johannes Schindelin @ 2016-10-07 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.
Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.
Changes since v3:
- reworded 3/5's commit message according to Junio's suggestion.
- fixed a tyop in 4/5's commit message, pointed out by Jakub.
- marked the hint "please commit or stash them" (reintroduced from the
original git-pull.sh script) as translatable.
- changed the exit code to 128 (emulating a die()) if
require_clean_work-tree() was asked to be non-gentle.
- fixed a tyop in 3/6 (which was replaced in 4/6, but it is good not to
introduce bugs only to fix them right away).
- prefixed the commit message of 4/6 with the "wt-status:" prefix,
replicating Junio's commit message in the `pu` branch.
Johannes Schindelin (6):
pull: drop confusing prefix parameter of die_on_unclean_work_tree()
pull: make code more similar to the shell script again
wt-status: make the require_clean_work_tree() function reusable
wt-status: export also the has_un{staged,committed}_changes()
functions
wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
wt-status: begin error messages with lower-case
builtin/pull.c | 71 +++-------------------------------------------------
wt-status.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
wt-status.h | 6 +++++
3 files changed, 87 insertions(+), 68 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v4
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v4
Interdiff vs v3:
diff --git a/builtin/pull.c b/builtin/pull.c
index 0bf9802..d6e46ee 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (!autostash)
require_clean_work_tree(N_("pull with rebase"),
- "please commit or stash them.", 1, 0);
+ _("please commit or stash them."), 1, 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index ef67593..e8e5de4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2281,7 +2281,7 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
if (hint)
error("%s", hint);
if (!gently)
- exit(err);
+ exit(128);
}
return err;
--
2.10.0.windows.1.325.ge6089c1
base-commit: a23ca1b8dc42ffd4de2ef30d67ce1e21ded29886
^ permalink raw reply
* [PATCH v4 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()
From: Johannes Schindelin @ 2016-10-07 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475856491.git.johannes.schindelin@gmx.de>
In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.
The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.
The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.
That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
/**
* Returns 1 if there are unstaged changes, 0 otherwise.
*/
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
{
struct rev_info rev_info;
int result;
- init_revisions(&rev_info, prefix);
+ init_revisions(&rev_info, NULL);
DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
/**
* Returns 1 if there are uncommitted changes, 0 otherwise.
*/
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
{
struct rev_info rev_info;
int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
if (is_cache_unborn())
return 0;
- init_revisions(&rev_info, prefix);
+ init_revisions(&rev_info, NULL);
DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
* If the work tree has unstaged or uncommitted changes, dies with the
* appropriate message.
*/
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
update_index_if_able(&the_index, lock_file);
rollback_lock_file(lock_file);
- if (has_unstaged_changes(prefix)) {
+ if (has_unstaged_changes()) {
error(_("Cannot pull with rebase: You have unstaged changes."));
do_die = 1;
}
- if (has_uncommitted_changes(prefix)) {
+ if (has_uncommitted_changes()) {
if (do_die)
error(_("Additionally, your index contains uncommitted changes."));
else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
die(_("Updating an unborn branch with changes added to the index."));
if (!autostash)
- die_on_unclean_work_tree(prefix);
+ die_on_unclean_work_tree();
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Jeff King @ 2016-10-07 16:06 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <4ac0ce84-ca6c-3650-ef5e-e13c54c60504@connectinternetsolutions.com>
On Fri, Oct 07, 2016 at 04:45:08PM +0100, Richard Lloyd wrote:
> On 06/10/16 20:11, Jeff King wrote:
> > Junio mentioned the NO_REGEX knob in the Makefile. If that works for
> > you, the next step is probably to add a line to the HP-UX section of
> > config.mak.uname, so that it just works out of the box.
>
> This doesn't work because the check in git-compat-util.h only looks
> for REG_STARTEND being defined (if it isn't, it #error's out).
>
> That define is not mentioned anywhere else other than in the
> compat/regex tree, which is why I used -Icompat/regex to pick up
> <regex.h> from there - this was the "easiest" solution for me on
> HP-UX 11.
I'm confused. Setting NO_REGEX in the Makefile will add -Icompat/regex
to your compiler invocation. So git-compat-util.h should pick up our
compat regex routines, which _do_ have REG_STARTEND.
How are you building? Doing:
make NO_REGEX=1
is supposed to work, and if it doesn't, there's a bug.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jakub Narębski @ 2016-10-07 15:55 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <alpine.DEB.2.20.1610071616390.35196@virtualbox>
W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze:
> On Fri, 7 Oct 2016, Jakub Narębski wrote:
>> Note that we would have to teach git completion about new syntax;
>> or new configuration variable if we go that route.
>
> Why would we? Git's completion does not expand aliases, it only completes
> the aliases' names, not their values.
Because Git completion finds out which _options_ and _arguments_
to complete for an alias based on its expansion.
Yes, this is nice bit of trickery...
--
Jakub Narębski
^ permalink raw reply
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Richard Lloyd @ 2016-10-07 15:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161006191127.2vjtmxl7ygjeqcbk@sigill.intra.peff.net>
On 06/10/16 20:11, Jeff King wrote:
> Junio mentioned the NO_REGEX knob in the Makefile. If that works for
> you, the next step is probably to add a line to the HP-UX section of
> config.mak.uname, so that it just works out of the box.
This doesn't work because the check in git-compat-util.h only looks
for REG_STARTEND being defined (if it isn't, it #error's out).
That define is not mentioned anywhere else other than in the
compat/regex tree, which is why I used -Icompat/regex to pick up
<regex.h> from there - this was the "easiest" solution for me on
HP-UX 11.
Note that with this inclusion change, the source compiled and linked
fine on HP-UX 11 and git passed its tests, including the regex-based ones.
Richard K. Lloyd, E-mail: richard.lloyd@connectinternetsolutions.com
Connect Internet Solutions, WWW: https://www.connectinternetsolutions.com/
4th Floor, New Barratt House,
47, North John Street,
Liverpool,
Merseyside, UK. L2 6SG
>
> -Peff
>
--
This e-mail (and any attachments) is private and confidential. If you have
received it in error, please notify the sender immediately and delete it
from your system. Do not use, copy or disclose the information in any way
nor act in reliance on it.
Any views expressed in this message are those of the individual sender,
except where the sender specifically states them to be the views of Connect
Internet Solutions Ltd. This e-mail and any attachments are believed to be
virus free but it is the recipient's responsibility to ensure that they are.
Connect Internet Solutions Ltd
(A company registered in England No: 04424350)
Registered Office: 4th Floor, New Barratt House, 47 North John Street,
Liverpool, L2 6SG
Telephone: +44 (0) 151 282 4321
VAT registration number: 758 2838 85
^ permalink raw reply
* [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
We run a command for each sha1 change in a submodule. This is
unnecessary since we can simply batch all sha1's we want to check into
one command. Lets do it so we can speedup the check when many submodule
changes are in need of checking.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 63 +++++++++++++++++++++++++++++++++----------------------------
1 file changed, 34 insertions(+), 29 deletions(-)
diff --git a/submodule.c b/submodule.c
index 5044afc2f8..a05c2a34b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data)
return 0;
}
-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
+static int check_has_hash(const unsigned char sha1[20], void *data)
{
- if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ int *has_hash = (int *) data;
+
+ if (!lookup_commit_reference(sha1))
+ *has_hash = 0;
+
+ return 0;
+}
+
+static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
+{
+ int has_hash = 1;
+
+ if (add_submodule_odb(path))
+ return 0;
+
+ sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
+ return has_hash;
+}
+
+static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
+{
+ if (!submodule_has_hashes(path, hashes))
return 0;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
struct strbuf buf = STRBUF_INIT;
int needs_pushing = 0;
- argv[1] = sha1_to_hex(sha1);
- cp.argv = argv;
+ argv_array_push(&cp.args, "rev-list");
+ sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);
+ argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
- sha1_to_hex(sha1), path);
+ die("Could not run 'git rev-list <hashes> --not --remotes -n 1' command in submodule %s",
+ path);
if (strbuf_read(&buf, cp.out, 41))
needs_pushing = 1;
finish_command(&cp);
@@ -604,21 +626,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
-struct collect_submodule_from_sha1s_data {
- char *submodule_path;
- struct string_list *needs_pushing;
-};
-
-static void collect_submodules_from_sha1s(const unsigned char sha1[20],
- void *data)
-{
- struct collect_submodule_from_sha1s_data *me =
- (struct collect_submodule_from_sha1s_data *) data;
-
- if (submodule_needs_pushing(me->submodule_path, sha1))
- string_list_insert(me->needs_pushing, me->submodule_path);
-}
-
static void free_submodules_sha1s(struct string_list *submodules)
{
int i;
@@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes,
argv_array_clear(&argv);
for (i = 0; i < submodules.nr; i++) {
- struct string_list_item *item = &submodules.items[i];
- struct collect_submodule_from_sha1s_data data;
- data.submodule_path = item->string;
- data.needs_pushing = needs_pushing;
- sha1_array_for_each_unique((struct sha1_array *) item->util,
- collect_submodules_from_sha1s,
- &data);
+ struct string_list_item *submodule = &submodules.items[i];
+ struct sha1_array *hashes = (struct sha1_array *) submodule->util;
+
+ if (submodule_needs_pushing(submodule->string, hashes))
+ string_list_insert(needs_pushing, submodule->string);
}
free_submodules_sha1s(&submodules);
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* [PATCH v2 0/3] Speedup finding of unpushed submodules
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
You can find the first iteration of this series as part of this thread:
http://public-inbox.org/git/%3C20160914173124.GA7613@sandbox%3E/
All mentioned issues should be fixed. I dropped the last patch which was
the cause of the broken tests.
This should optimize every part of this test to a nice speed if you are
pushing to a remote. The only case that is still broken/slow as hell is
when calling push with a direct url.
I am thinking whether we should maybe error out with a "not implemented"
message or something and mention that --recurse-submoules does not work
with direct urls? But we might want to have another look at performance
with this patch included. Maybe it is actually useable with the last
patch included which was not yet on pu.
Cheers Heiko
Heiko Voigt (3):
serialize collection of changed submodules
serialize collection of refs that contain submodule changes
batch check whether submodule needs pushing into one call
submodule.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++--------------
submodule.h | 5 +--
transport.c | 29 ++++++++++-----
3 files changed, 114 insertions(+), 36 deletions(-)
--
2.10.1.637.g09b28c5
^ permalink raw reply
* [PATCH v2 2/3] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
We are iterating over each pushed ref and want to check whether it
contains changes to submodules. Instead of immediately checking each ref
lets first collect them and then do the check for all of them in one
revision walk.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 36 +++++++++++++++++++++---------------
submodule.h | 5 +++--
transport.c | 29 +++++++++++++++++++++--------
3 files changed, 45 insertions(+), 25 deletions(-)
diff --git a/submodule.c b/submodule.c
index 59c9d15905..5044afc2f8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,
return 1;
}
+static int append_hash_to_argv(const unsigned char sha1[20], void *data)
+{
+ struct argv_array *argv = (struct argv_array *) data;
+ argv_array_push(argv, sha1_to_hex(sha1));
+ return 0;
+}
+
static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
{
if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
@@ -623,24 +630,24 @@ static void free_submodules_sha1s(struct string_list *submodules)
string_list_clear(submodules, 1);
}
-int find_unpushed_submodules(unsigned char new_sha1[20],
+int find_unpushed_submodules(struct sha1_array *hashes,
const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
- const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
- int argc = ARRAY_SIZE(argv) - 1, i;
- char *sha1_copy;
+ int i;
struct string_list submodules = STRING_LIST_INIT_DUP;
+ struct argv_array argv = ARGV_ARRAY_INIT;
- struct strbuf remotes_arg = STRBUF_INIT;
-
- strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
init_revisions(&rev, NULL);
- sha1_copy = xstrdup(sha1_to_hex(new_sha1));
- argv[1] = sha1_copy;
- argv[3] = remotes_arg.buf;
- setup_revisions(argc, argv, &rev, NULL);
+
+ /* argv.argv[0] will be ignored by setup_revisions */
+ argv_array_push(&argv, "find_unpushed_submodules");
+ sha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);
+ argv_array_push(&argv, "--not");
+ argv_array_pushf(&argv, "--remotes=%s", remotes_name);
+
+ setup_revisions(argv.argc, argv.argv, &rev, NULL);
if (prepare_revision_walk(&rev))
die("revision walk setup failed");
@@ -648,8 +655,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
- free(sha1_copy);
- strbuf_release(&remotes_arg);
+ argv_array_clear(&argv);
for (i = 0; i < submodules.nr; i++) {
struct string_list_item *item = &submodules.items[i];
@@ -687,12 +693,12 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing))
+ if (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing))
return 1;
for (i = 0; i < needs_pushing.nr; i++) {
diff --git a/submodule.h b/submodule.h
index d9e197a948..065b2f0a2a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@
struct diff_options;
struct argv_array;
+struct sha1_array;
enum {
RECURSE_SUBMODULES_CHECK = -4,
@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);
int ok_to_remove_submodule(const char *path);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
+int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
diff --git a/transport.c b/transport.c
index 94d6dc3725..05f2ce83f1 100644
--- a/transport.c
+++ b/transport.c
@@ -903,23 +903,36 @@ int transport_push(struct transport *transport,
if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
struct ref *ref = remote_refs;
+ struct sha1_array hashes = SHA1_ARRAY_INIT;
+
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- !push_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name))
- die ("Failed to push all needed submodules!");
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&hashes, ref->new_oid.hash);
+
+ if (!push_unpushed_submodules(&hashes, transport->remote->name)) {
+ sha1_array_clear(&hashes);
+ die ("Failed to push all needed submodules!");
+ }
+ sha1_array_clear(&hashes);
}
if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+ struct sha1_array hashes = SHA1_ARRAY_INIT;
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- find_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name, &needs_pushing))
- die_with_unpushed_submodules(&needs_pushing);
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&hashes, ref->new_oid.hash);
+
+ if (find_unpushed_submodules(&hashes, transport->remote->name,
+ &needs_pushing)) {
+ sha1_array_clear(&hashes);
+ die_with_unpushed_submodules(&needs_pushing);
+ }
+ string_list_clear(&needs_pushing, 0);
+ sha1_array_clear(&hashes);
}
push_ret = transport->push_refs(transport, remote_refs, flags);
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
To check whether a submodule needs to be pushed we need to collect all
changed submodules. Lets collect them first and then execute the
possibly expensive test whether certain revisions are already pushed
only once per submodule.
There is further potential for optimization since we can assemble one
command and only issued that instead of one call for each remote ref in
the submodule.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 58 insertions(+), 5 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2de06a3351..59c9d15905 100644
--- a/submodule.c
+++ b/submodule.c
@@ -554,19 +554,34 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20
return 0;
}
+static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
+ const char *path)
+{
+ struct string_list_item *item;
+
+ item = string_list_insert(submodules, path);
+ if (item->util)
+ return (struct sha1_array *) item->util;
+
+ /* NEEDSWORK: should we have sha1_array_init()? */
+ item->util = xcalloc(1, sizeof(struct sha1_array));
+ return (struct sha1_array *) item->util;
+}
+
static void collect_submodules_from_diff(struct diff_queue_struct *q,
struct diff_options *options,
void *data)
{
int i;
- struct string_list *needs_pushing = data;
+ struct string_list *submodules = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
+ struct sha1_array *hashes;
if (!S_ISGITLINK(p->two->mode))
continue;
- if (submodule_needs_pushing(p->two->path, p->two->oid.hash))
- string_list_insert(needs_pushing, p->two->path);
+ hashes = get_sha1s_from_list(submodules, p->two->path);
+ sha1_array_append(hashes, p->two->oid.hash);
}
}
@@ -582,14 +597,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
+struct collect_submodule_from_sha1s_data {
+ char *submodule_path;
+ struct string_list *needs_pushing;
+};
+
+static void collect_submodules_from_sha1s(const unsigned char sha1[20],
+ void *data)
+{
+ struct collect_submodule_from_sha1s_data *me =
+ (struct collect_submodule_from_sha1s_data *) data;
+
+ if (submodule_needs_pushing(me->submodule_path, sha1))
+ string_list_insert(me->needs_pushing, me->submodule_path);
+}
+
+static void free_submodules_sha1s(struct string_list *submodules)
+{
+ int i;
+ for (i = 0; i < submodules->nr; i++) {
+ struct string_list_item *item = &submodules->items[i];
+ struct sha1_array *hashes = (struct sha1_array *) item->util;
+ sha1_array_clear(hashes);
+ }
+ string_list_clear(submodules, 1);
+}
+
int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
- int argc = ARRAY_SIZE(argv) - 1;
+ int argc = ARRAY_SIZE(argv) - 1, i;
char *sha1_copy;
+ struct string_list submodules = STRING_LIST_INIT_DUP;
struct strbuf remotes_arg = STRBUF_INIT;
@@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
die("revision walk setup failed");
while ((commit = get_revision(&rev)) != NULL)
- find_unpushed_submodule_commits(commit, needs_pushing);
+ find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
+ for (i = 0; i < submodules.nr; i++) {
+ struct string_list_item *item = &submodules.items[i];
+ struct collect_submodule_from_sha1s_data data;
+ data.submodule_path = item->string;
+ data.needs_pushing = needs_pushing;
+ sha1_array_for_each_unique((struct sha1_array *) item->util,
+ collect_submodules_from_sha1s,
+ &data);
+ }
+ free_submodules_sha1s(&submodules);
+
return needs_pushing->nr;
}
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Matthieu Moy @ 2016-10-07 14:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071611550.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Matthieu,
>
> On Fri, 7 Oct 2016, Matthieu Moy wrote:
>
>> Another possibility: !(nocd), which leaves room
>> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
>> with the :(word) syntax of pathspecs.
>
> But is this backwards-compatible? Don't we execute everything that comes
> after the exclamation mark as a command-line via shell, where the
> parentheses mean "open a subshell"?
Good point. I can imagine someone already having
[alias]
foo = !(cd bar && $something) && $something_else
Your proposed (keyword)! is better.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] clean up confusing suggestion for commit references
From: Jeff King @ 2016-10-07 14:32 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git
In-Reply-To: <20161007095638.GA55445@book.hvoigt.net>
On Fri, Oct 07, 2016 at 11:56:38AM +0200, Heiko Voigt wrote:
> The description for referencing commits looks as if it is contradicting
> the example, since it is itself enclosed in double quotes. Lets use
> single quotes around the description and include the double quotes in
> the description so it matches the example.
> ---
> Sorry for opening this up again but I just looked up the format and was
> like: "Umm, which one is now the correct one..."
>
> For this makes more sense. What do others think?
Looking over the threads, I wasn't sure there was consensus[1,2]. So it would
be equally correct to drop the quotes from the example.
I dunno. I am in favor of no-quotes, myself, so maybe I am just
manufacturing dissent in my mind. :)
-Peff
[1] http://public-inbox.org/git/a9731f60-5c30-0bc6-f73a-f7ffb7bd4231@kdbg.org/
[2] http://public-inbox.org/git/20160829183015.2uqnfezekjfa3ott@sigill.intra.peff.net/
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-07 14:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071519450.35196@virtualbox>
On Fri, Oct 07, 2016 at 04:11:34PM +0200, Johannes Schindelin wrote:
> Possibly a better idea would be to use *another* special symbol, one that
> makes intuitive sense as a modifier, such as:
>
> [alias]
> # This works as before
> xyz = !pwd
> # As does this
> stat = -p status
> # This, however, is different:
> duy = (nocd)!pwd
>
> This is backwards compatible as "(" is not a part of any Git command, nor
> of a valid alias, nor is it commonly used as part of a git-*
> executable/script.
>
> It is also kind of a bit more intuitive, I'd wager, and it is also
> extensible to future options we may want to introduce.
I like this much better (like you, I am concerned about things like
"!(foo)" as conflicting with the shell). And I think your "(nocd)!pwd"
example is quite readable.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:19 UTC (permalink / raw)
To: Jakub Narębski
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <e1b432bc-97b4-15be-aa44-71921c64cd15@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
Hi Kuba,
On Fri, 7 Oct 2016, Jakub Narębski wrote:
> W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze:
> > On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> >>
> >>> Throwing something at the mailing list to see if anybody is
> >>> interested.
> >>>
> >>> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> >>> handling path arguments hard because they are relative to the original
> >>> cwd. We set GIT_PREFIX to work around it, but I still think it's more
> >>> natural to keep cwd where it is.
> >>>
> >>> We have a way to do that now after 441981b (git: simplify environment
> >>> save/restore logic - 2016-01-26). It's just a matter of choosing the
> >>> right syntax. I'm going with '!!'. I'm not very happy with it. But I
> >>> do like this type of alias.
> >>
> >> I do not know why you are not happy with the syntax, but I
> >> personally think it brilliant, both the idea and the preliminary
> >> clean-up that made this possible with a simple patch like this.
> >
> > I guess he is not happy with it because "!!" is quite unintuitive a
> > construct. I know that *I* would have been puzzled by it, asking "What the
> > heck does this do?".
>
> Well, "!" as a prefix is not intuitive either.
You do not use vi, do you? :-P
In vi, if you enter command mode (typing a colon) and then want to
execute, say, `pwd`, you type !pwd<Enter>
> Perhaps "!.", because "." is current directory, and the "." command
> (that is, alias to "source") doesn't make sense in git aliases.
If you want to execute, say, `pwd` in the current directory, that would
mean you want to write
!.pwd
But that already means "execute `.pwd`"...
> Note that we would have to teach git completion about new syntax;
> or new configuration variable if we go that route.
Why would we? Git's completion does not expand aliases, it only completes
the aliases' names, not their values.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <vpqmvig5ns6.fsf@anie.imag.fr>
[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]
Hi Matthieu,
On Fri, 7 Oct 2016, Matthieu Moy wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >> Hi Junio,
> >>
> >> On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >>
> >>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> >>>
> >>> > Throwing something at the mailing list to see if anybody is
> >>> > interested.
> >>> >
> >>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could
> >>> > make
> >>> > handling path arguments hard because they are relative to the
> >>> > original
> >>> > cwd. We set GIT_PREFIX to work around it, but I still think it's
> >>> > more
> >>> > natural to keep cwd where it is.
> >>> >
> >>> > We have a way to do that now after 441981b (git: simplify
> >>> > environment
> >>> > save/restore logic - 2016-01-26). It's just a matter of choosing
> >>> > the
> >>> > right syntax. I'm going with '!!'. I'm not very happy with it.
> >>> > But I
> >>> > do like this type of alias.
> >>>
> >>> I do not know why you are not happy with the syntax, but I
> >>> personally think it brilliant, both the idea and the preliminary
> >>> clean-up that made this possible with a simple patch like this.
> >>
> >> I guess he is not happy with it because "!!" is quite unintuitive a
> >> construct. I know that *I* would have been puzzled by it, asking
> >> "What the
> >> heck does this do?".
> >
> > Yep. And I wouldn't want to set a tradition for the next alias type
> > '!!!'. There's no good choice to represent a new alias type with a
> > leading symbol. This just occurred to me, however, what do you think
> > about a new config group for it? With can have something like
> > externalAlias.* (or some other name) that lives in parallel with
> > alias.*. Then we don't need '!' (or '!!') at all.
>
> Another possibility: !(nocd), which leaves room
> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
> with the :(word) syntax of pathspecs.
But is this backwards-compatible? Don't we execute everything that comes
after the exclamation mark as a command-line via shell, where the
parentheses mean "open a subshell"?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:11 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8ASc7Fxm5XDHFiX9E+bQ8s1MtmEHfc7bZY4C-_GEQr0og@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]
Hi Duy,
On Fri, 7 Oct 2016, Duy Nguyen wrote:
> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >
> >> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> >>
> >> > Throwing something at the mailing list to see if anybody is
> >> > interested.
> >> >
> >> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> >> > handling path arguments hard because they are relative to the original
> >> > cwd. We set GIT_PREFIX to work around it, but I still think it's more
> >> > natural to keep cwd where it is.
> >> >
> >> > We have a way to do that now after 441981b (git: simplify environment
> >> > save/restore logic - 2016-01-26). It's just a matter of choosing the
> >> > right syntax. I'm going with '!!'. I'm not very happy with it. But I
> >> > do like this type of alias.
> >>
> >> I do not know why you are not happy with the syntax, but I
> >> personally think it brilliant, both the idea and the preliminary
> >> clean-up that made this possible with a simple patch like this.
> >
> > I guess he is not happy with it because "!!" is quite unintuitive a
> > construct. I know that *I* would have been puzzled by it, asking "What the
> > heck does this do?".
>
> Yep. And I wouldn't want to set a tradition for the next alias type
> '!!!'. There's no good choice to represent a new alias type with a
> leading symbol. This just occurred to me, however, what do you think
> about a new config group for it? With can have something like
> externalAlias.* (or some other name) that lives in parallel with
> alias.*. Then we don't need '!' (or '!!') at all.
But what would the precedence be? externalAlias.xyz wins over alias.xyz?
And we still would need '!' support: tons of people (including myself)
rely on it.
Possibly a better idea would be to use *another* special symbol, one that
makes intuitive sense as a modifier, such as:
[alias]
# This works as before
xyz = !pwd
# As does this
stat = -p status
# This, however, is different:
duy = (nocd)!pwd
This is backwards compatible as "(" is not a part of any Git command, nor
of a valid alias, nor is it commonly used as part of a git-*
executable/script.
It is also kind of a bit more intuitive, I'd wager, and it is also
extensible to future options we may want to introduce.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jakub Narębski @ 2016-10-07 13:31 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <alpine.DEB.2.20.1610071319520.35196@virtualbox>
Hello, Johannes
W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze:
> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> Throwing something at the mailing list to see if anybody is
>>> interested.
>>>
>>> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
>>> handling path arguments hard because they are relative to the original
>>> cwd. We set GIT_PREFIX to work around it, but I still think it's more
>>> natural to keep cwd where it is.
>>>
>>> We have a way to do that now after 441981b (git: simplify environment
>>> save/restore logic - 2016-01-26). It's just a matter of choosing the
>>> right syntax. I'm going with '!!'. I'm not very happy with it. But I
>>> do like this type of alias.
>>
>> I do not know why you are not happy with the syntax, but I
>> personally think it brilliant, both the idea and the preliminary
>> clean-up that made this possible with a simple patch like this.
>
> I guess he is not happy with it because "!!" is quite unintuitive a
> construct. I know that *I* would have been puzzled by it, asking "What the
> heck does this do?".
Well, "!" as a prefix is not intuitive either.
Perhaps "!.", because "." is current directory, and the "." command
(that is, alias to "source") doesn't make sense in git aliases.
Note that we would have to teach git completion about new syntax;
or new configuration variable if we go that route.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Matthieu Moy @ 2016-10-07 12:47 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8ASc7Fxm5XDHFiX9E+bQ8s1MtmEHfc7bZY4C-_GEQr0og@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Junio,
>>
>> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>>
>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>
>>> > Throwing something at the mailing list to see if anybody is
>>> > interested.
>>> >
>>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could
>>> > make
>>> > handling path arguments hard because they are relative to the
>>> > original
>>> > cwd. We set GIT_PREFIX to work around it, but I still think it's
>>> > more
>>> > natural to keep cwd where it is.
>>> >
>>> > We have a way to do that now after 441981b (git: simplify
>>> > environment
>>> > save/restore logic - 2016-01-26). It's just a matter of choosing
>>> > the
>>> > right syntax. I'm going with '!!'. I'm not very happy with it.
>>> > But I
>>> > do like this type of alias.
>>>
>>> I do not know why you are not happy with the syntax, but I
>>> personally think it brilliant, both the idea and the preliminary
>>> clean-up that made this possible with a simple patch like this.
>>
>> I guess he is not happy with it because "!!" is quite unintuitive a
>> construct. I know that *I* would have been puzzled by it, asking
>> "What the
>> heck does this do?".
>
> Yep. And I wouldn't want to set a tradition for the next alias type
> '!!!'. There's no good choice to represent a new alias type with a
> leading symbol. This just occurred to me, however, what do you think
> about a new config group for it? With can have something like
> externalAlias.* (or some other name) that lives in parallel with
> alias.*. Then we don't need '!' (or '!!') at all.
Another possibility: !(nocd), which leaves room
for !(keyword1,keyword2,...) if needed later. Also, it is consistent
with the :(word) syntax of pathspecs.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Sergey Organov @ 2016-10-07 13:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqshs9l69h.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>>> Last, if "reference" is not good enough and we get to internals anyway,
>>>> why not say SHA1 then?
>>>
>>> Because that is still colloquial? I think s/name/object name/ is a
>>> sensible change, but not s/name/reference/.
>>
>> No, "reference" is more sensible here than any of "name", "object name",
>> or "SHA-1", the same way as here:
>>
>> $ git help glossary
>> [...]
>> chain
>> A list of objects, where each object in the list contains a
>> reference to its successor (for example, the successor of a
>> commit could be one of its parents).
>> [...]
>
> The entry for "chain" and the description under discussion have
> stress on different aspect, though. The description of "chain" is
> more general: an object refers to another object by referring to it,
> by unspecified means. The reason why it is left unspecified is
> because the way a tree object refers to blobs and trees is different
> from the way a commit object refers to its parents (the former has
> object names of blobs and trees in the tree entries; the latter uses
> "parent" entries in the object header part to record object names of
> parent commits). It wants to stress more on the fact that there is
> some mechanism to associate one object to others, than how that
> association/linkage is expressed.
>
> The way the resulting commit is described in the original text of
> "git merge" description stresses more on "how" by being a lot more
> specific to commit objects. It does not just say "refers to parents
> (by unspecified means)"; instead it tries to say what exactly are
> recorded, i.e. the parents are referred to by recording the object
> names of them in a new commit object. It stresses more on "how"
> (because it can afford to be more specific, unlike the description
> of more general concept of a "chain").
That's were our disagreement actually is, and that's what I've tried to
fix with s/name/reference/, and that's why I'm against s/name/object
name/.
Rather than being more (and more) specific at every opportunity, one
needs a good reason to get more specific. In this particular case,
general DAG terminology seems to be enough to describe git-merge
semantics, thus using GIT specifics is unfounded.
> It may be debatable if we want to give the description of what is
> exactly recorded at that point of the document,
Exactly. My point in this particular discussion is that details of
recording of references to parents don't belong here, even though to
tell the truth I think they don't belong to git _user_ documentation at
all.
> but I personally
> think that the users deserve a chance to learn how a merge is
> recorded in "git merge" documentation.
I doubt a user will gain anything from this sacred knowledge suddenly
being thrown on him when what she is looking for is understanding of
basic merge semantics in GIT.
That said, if you still disagree, please feel free to just drop the
patch.
-- Sergey
^ permalink raw reply
* Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Duy Nguyen @ 2016-10-07 12:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqwphljnlj.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 7, 2016 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>>
>>>> We don't use it internally _yet_. I need to go through all the
>>>> external diff code and see --shift-ita should be there. The end goal
>>>> is still changing the default behavior and getting rid of --shift-ita,
>>>
>>> I do not agree with that endgame, and quite honestly I do not want
>>> to waste time reviewing such a series.
>
> I definitely shouldn't have said that, especially "waste". Many
> issues around i-t-a and diff make my head hurt when I think about
> them [*1*], but not wanting to spend time that gets my
> head hurt and not wanting to waste time are totally different
> things. My apologies.
No problem. I do appreciate a straight shoot down though. Many of my
topics have been going on for months (ones not in 'pu') and seeing it
rejected near the end is worse than stopping working on them early.
> I missed something curious in your statement above, i.e. "external
> diff". I thought we have pretty much got rid of all the invocation
> of "git diff" via the run_command() interface and we do not need the
> command line option (we only need the options->shift_ita so that
> callers like "git status" can seletively ask for it when making
> internal calls), and that is why I didn't want to see it.
I don't know if we have had any external diff calls in our shell-based
commands. I don't read them often. Regardless, people do use "git
diff" and it should show the "right thing" (I know it's subjective).
Or at least be consistent with both git-commit and git-status.
> [Footnote]
>
> Here is one of the things around i-t-a and diff. If you make "git
> diff" (between the index and the working tree) report "new" file, it
> would imply that "git apply" run without "--index" should create an
Off topic. This reminds me of an old patch about apply and ita [1] but
that one is not the same here
> ita entry in the index for symmetry, wouldn't it? That by itself
> can be seen as an improvement (we no longer would have to say that
> "git apply patchfile && git commit -a" that is run in a clean state
> will forget new files the patchfile creates), but it also means we
> now need a repository in order to run "git apply" (without "--index"),
> which is a problem, as "git apply" is often used as a better "patch".
We could detect "no repo available" and ignore the index, I guess.
> "git apply --cached" may also become "interesting". A patch that
> would apply cleanly to HEAD should apply cleanly if you did this:
>
> $ git read-tree HEAD
> $ git apply --cached <patch
>
> no matter what the working tree state is. Should a patch that
> creates a "new" file add contents to the index, or just an i-t-a
> entry? I could argue it both ways, but either is quite satisfactory
> and makes my head hurt.
--cached tells you to put new contents in the index. I-ta entries,
being a reminder to add stuff, don't really fit in here because you
want to add contents _now_, i think. After a successful "git apply
--cached", a "git commit" should contain exactly what the applied
patch has. If new files are i-t-a entries instead, then the new commit
would not be the same as the patch.
[1] https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclouds@gmail.com/
--
Duy
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-07 13:07 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <vpqmvig5ns6.fsf@anie.imag.fr>
On Fri, Oct 7, 2016 at 7:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Junio,
>>>
>>> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>>>
>>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>>
>>>> > Throwing something at the mailing list to see if anybody is
>>>> > interested.
>>>> >
>>>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could
>>>> > make
>>>> > handling path arguments hard because they are relative to the
>>>> > original
>>>> > cwd. We set GIT_PREFIX to work around it, but I still think it's
>>>> > more
>>>> > natural to keep cwd where it is.
>>>> >
>>>> > We have a way to do that now after 441981b (git: simplify
>>>> > environment
>>>> > save/restore logic - 2016-01-26). It's just a matter of choosing
>>>> > the
>>>> > right syntax. I'm going with '!!'. I'm not very happy with it.
>>>> > But I
>>>> > do like this type of alias.
>>>>
>>>> I do not know why you are not happy with the syntax, but I
>>>> personally think it brilliant, both the idea and the preliminary
>>>> clean-up that made this possible with a simple patch like this.
>>>
>>> I guess he is not happy with it because "!!" is quite unintuitive a
>>> construct. I know that *I* would have been puzzled by it, asking
>>> "What the
>>> heck does this do?".
>>
>> Yep. And I wouldn't want to set a tradition for the next alias type
>> '!!!'. There's no good choice to represent a new alias type with a
>> leading symbol. This just occurred to me, however, what do you think
>> about a new config group for it? With can have something like
>> externalAlias.* (or some other name) that lives in parallel with
>> alias.*. Then we don't need '!' (or '!!') at all.
>
> Another possibility: !(nocd), which leaves room
> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
> with the :(word) syntax of pathspecs.
This seems to solve my problem nicely.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Heiko Voigt @ 2016-10-07 12:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git, torvalds, peff
In-Reply-To: <xmqqshs9i6fr.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 06, 2016 at 01:11:20PM -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> > Currently the force flag in `git submodule add` takes care of possibly
> > ignored files or when a name collision occurs.
> >
> > However there is another situation where submodule add comes in handy:
> > When you already have a gitlink recorded, but no configuration was
> > done (i.e. no .gitmodules file nor any entry in .git/config) and you
> > want to generate these config entries. For this situation allow
> > `git submodule add` to proceed if there is already a submodule at the
> > given path in the index.
Is it important that the submodule is in the index? How about worktree?
From the index entry alone we can not deduce the values anyway. So I
would say the submodule has to be in the worktree, no matter what is in
the index. If its not in the index we can also add it.
BTW, that is the way I imagined submodules would work in the first
place: just clone and add them, like I described here[1].
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
>
> Yup, the goal makes perfect sense.
>
> I vaguely recall discussing this exact issue of "git submodule add"
> that refuses to add a path that already is a gitlink (via "git add"
> that has previously been run) elsewhere on this list some time ago.
Yes there was a discussion, see the link.
Cheers Heiko
[1] http://public-inbox.org/git/%3C20160916141143.GA47240@book.hvoigt.net%3E/
^ permalink raw reply
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist
From: Heiko Voigt @ 2016-10-07 12:41 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Linus Torvalds
In-Reply-To: <CAGZ79ka=bVX51wrSiz4u7xOy+54t+v7a-iU5dHtQoNnB7-NAkw@mail.gmail.com>
On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote:
> On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:
> >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
> >> > Jeff,
> >> > thanks for the suggestions, both git_path(..) as well as checking the config,
> >> > this seems quite readable to me:
> >>
> >> When reading the discussion I thought the same: What about the
> >> "old-style" repositories. I like this one. Checking both locations
> >> is nice.
> >
> > BTW, since it seems we all agree on the direction. Should we add some
> > tests?
> >
>
> Good call. What do we want to test for?
> * Correctness in case of submodules? (just push and get rejected)
> I think that is easy to do.
> * Performance with [no, lots of] submodules? That seems harder to me.
>
> I'll add a test for the correctness part and resend.
Well I though about the following tests:
* Without submodules: Make sure submodule processing is disabled by
default
* With new-style submodules: Make sure submodules are processed by
default
* With old-style submodules: Make sure submodules are processed by
default
But I have to admit that I did not think about the "how can we do that".
But performance seems to be the only thing that is exposing the
processing when we have no submodules, so it seems we can only easily do
the tests with submodules.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-07 12:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071319520.35196@virtualbox>
On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>> > Throwing something at the mailing list to see if anybody is
>> > interested.
>> >
>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
>> > handling path arguments hard because they are relative to the original
>> > cwd. We set GIT_PREFIX to work around it, but I still think it's more
>> > natural to keep cwd where it is.
>> >
>> > We have a way to do that now after 441981b (git: simplify environment
>> > save/restore logic - 2016-01-26). It's just a matter of choosing the
>> > right syntax. I'm going with '!!'. I'm not very happy with it. But I
>> > do like this type of alias.
>>
>> I do not know why you are not happy with the syntax, but I
>> personally think it brilliant, both the idea and the preliminary
>> clean-up that made this possible with a simple patch like this.
>
> I guess he is not happy with it because "!!" is quite unintuitive a
> construct. I know that *I* would have been puzzled by it, asking "What the
> heck does this do?".
Yep. And I wouldn't want to set a tradition for the next alias type
'!!!'. There's no good choice to represent a new alias type with a
leading symbol. This just occurred to me, however, what do you think
about a new config group for it? With can have something like
externalAlias.* (or some other name) that lives in parallel with
alias.*. Then we don't need '!' (or '!!') at all.
--
Duy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox