Re: [PATCH v5 13/17] read-tree: show progress by default
Hi Stolee On 21/10/2019 14:56, Derrick Stolee via GitGitGadget wrote: From: Derrick Stolee The read-tree builtin has a --verbose option that signals to show progress and other data while updating the index. Update this to be on by default when stderr is a terminal window. This will help tools like 'git sparse-checkout' to automatically benefit from progress indicators when a user runs these commands. Signed-off-by: Derrick Stolee --- builtin/read-tree.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/read-tree.c b/builtin/read-tree.c index ca5e655d2f..69963d83dc 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -162,6 +162,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) opts.head_idx = -1; opts.src_index = &the_index; opts.dst_index = &the_index; + opts.verbose_update = isatty(2); I'm slightly wary of changing the output of plumbing commands like this. If a script wants progress output it can already get it by passing --verbose. With this change a script that does not want that output now has to pass --no-verbose. If 'git sparse-checkout' wants progress indicators why not put the isatty() check there and pass the appropriate option to read-tree? Best Wishes Phillip git_config(git_read_tree_config, NULL);
Re: [RFC PATCH 7/7] merge: teach --autostash option
Hi Denton
On 16/10/2019 18:26, Denton Liu wrote:
In rebase, one can pass the `--autostash` option to cause the worktree
to be automatically stashed before continuing with the rebase. This
option is missing in merge, however.
It might be helpful to say why this option is useful. I can see one
might want to stash before doing `git pull` in the same way as one might
before a rebase but what's the motivation for doing it before a normal
merge?
Implement the `--autostash` option and corresponding `merge.autoStash`
option in merge which stashes before merging and then pops after.
Reported-by: Alban Gruin
Signed-off-by: Denton Liu
---
builtin/merge.c | 13 +
builtin/pull.c | 9 +
t/t5520-pull.sh | 8
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 062e911441..d1a5eaad0d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -40,6 +40,7 @@
#include "branch.h"
#include "commit-reach.h"
#include "wt-status.h"
+#include "autostash.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -58,6 +59,8 @@ static const char * const builtin_merge_usage[] = {
NULL
};
+static GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
+
static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = -1;
static int option_edit = -1;
@@ -81,6 +84,7 @@ static int show_progress = -1;
static int default_to_upstream = 1;
static int signoff;
static const char *sign_commit;
+static int autostash;
static int no_verify;
static struct strategy all_strategy[] = {
@@ -285,6 +289,8 @@ static struct option builtin_merge_options[] = {
OPT_SET_INT(0, "progress", &show_progress, N_("force progress
reporting"), 1),
{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+ OPT_BOOL(0, "autostash", &autostash,
+ N_("automatically stash/stash pop before and after")),
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files
(default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and
commit-msg hooks")),
@@ -440,6 +446,7 @@ static void finish(struct commit *head_commit,
strbuf_addf(&reflog_message, "%s: %s",
getenv("GIT_REFLOG_ACTION"), msg);
}
+ apply_autostash(merge_autostash());
if (squash) {
squash_message(head_commit, remoteheads);
} else {
@@ -631,6 +638,9 @@ static int git_merge_config(const char *k, const char *v,
void *cb)
} else if (!strcmp(k, "commit.gpgsign")) {
sign_commit = git_config_bool(k, v) ? "" : NULL;
return 0;
+ } else if (!strcmp(k, "merge.autostash")) {
+ autostash = git_config_bool(k, v);
+ return 0;
}
status = fmt_merge_msg_config(k, v, cb);
@@ -724,6 +734,8 @@ static int try_merge_strategy(const char *strategy, struct
commit_list *common,
for (j = common; j; j = j->next)
commit_list_insert(j->item, &reversed);
+ if (autostash)
+ perform_autostash(merge_autostash());
hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
clean = merge_recursive(&o, head,
remoteheads->item, reversed, &result);
@@ -1288,6 +1300,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
/* Invoke 'git reset --merge' */
ret = cmd_reset(nargc, nargv, prefix);
+ apply_autostash(merge_autostash());
goto done;
}
diff --git a/builtin/pull.c b/builtin/pull.c
index d25ff13a60..ee186781ae 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -183,7 +183,7 @@ static struct option pull_options[] = {
N_("verify that the named commit has a valid GPG signature"),
PARSE_OPT_NOARG),
OPT_BOOL(0, "autostash", &opt_autostash,
- N_("automatically stash/stash pop before and after rebase")),
+ N_("automatically stash/stash pop before and after")),
I've not looked closely at the code in this patch but noticed this. I
think it would read better if it said
automatically stash/stash pop before and after pulling
Best Wishes
Phillip
OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy&quo
Re: [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS
Hi Denton
On 16/10/2019 18:26, Denton Liu wrote:
Since autostash.c was recently introduced, we should avoid
USE_THE_INDEX_COMPATIBILITY_MACROS since we are trying to move away from
this in the rest of the codebase. Rewrite the autostash code to not need
it and remove its definition.
Signed-off-by: Denton Liu
---
autostash.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/autostash.c b/autostash.c
index 722cf78b12..0a1f00d2e5 100644
--- a/autostash.c
+++ b/autostash.c
@@ -1,5 +1,3 @@
-#define USE_THE_INDEX_COMPATIBILITY_MACROS
-
#include "git-compat-util.h"
#include "autostash.h"
#include "cache-tree.h"
@@ -46,7 +44,7 @@ int reset_head(struct object_id *oid, const char *action,
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch);
- if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+ if (!refs_only && repo_hold_locked_index(the_repository, &lock,
LOCK_REPORT_ON_ERROR) < 0) {
As I understand it the reason for moving away from hold_locked_index()
is that it relies on a global variable. While replacing it with an
explicit reference the the_repository removes the implicit dependency on
global state, it does not move us away from depending on a global
variable. I think it would be nicer to change these functions to take a
`struct repository*` before moving them.
Best Wishes
Phillip
ret = -1;
goto leave_reset_head;
}
@@ -157,8 +155,8 @@ void perform_autostash(const char *path)
struct lock_file lock_file = LOCK_INIT;
int fd;
- fd = hold_locked_index(&lock_file, 0);
- refresh_cache(REFRESH_QUIET);
+ fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+ refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
if (0 <= fd)
repo_update_index_if_able(the_repository, &lock_file);
rollback_lock_file(&lock_file);
Re: [RFC PATCH 4/7] autostash: extract reset_head() from rebase
Hi Denton
It's great to see this being libified, I've had it in mind to do this so
we can avoid forking 'git checkout' in sequencer.c
On 16/10/2019 18:26, Denton Liu wrote:
Begin the process of lib-ifying the autostash code. In a future commit,
This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.
this will be used to implement `--autostash` in other builtins.
Signed-off-by: Denton Liu
---
autostash.c | 137 +++
autostash.h | 12 +
builtin/rebase.c | 137 ---
3 files changed, 149 insertions(+), 137 deletions(-)
diff --git a/autostash.c b/autostash.c
index 62ec7a7c80..eb58e0c8a4 100644
--- a/autostash.c
+++ b/autostash.c
@@ -1,9 +1,17 @@
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
It might be nicer to have a preparatory step that fixes this by adding a
'struct repository *r' argument to the function in builtin/rebase.c
before moving the function. You could also do the same for next patch
and then move both functions together.
Best Wishes
Phillip
+
#include "git-compat-util.h"
#include "autostash.h"
+#include "cache-tree.h"
#include "dir.h"
#include "gettext.h"
+#include "lockfile.h"
+#include "refs.h"
#include "run-command.h"
#include "strbuf.h"
+#include "tree-walk.h"
+#include "tree.h"
+#include "unpack-trees.h"
int read_one(const char *path, struct strbuf *buf)
{
@@ -13,6 +21,135 @@ int read_one(const char *path, struct strbuf *buf)
return 0;
}
+int reset_head(struct object_id *oid, const char *action,
+ const char *switch_to_branch, unsigned flags,
+ const char *reflog_orig_head, const char *reflog_head)
+{
+ unsigned detach_head = flags & RESET_HEAD_DETACH;
+ unsigned reset_hard = flags & RESET_HEAD_HARD;
+ unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
+ unsigned update_orig_head = flags & RESET_ORIG_HEAD;
+ struct object_id head_oid;
+ struct tree_desc desc[2] = { { NULL }, { NULL } };
+ struct lock_file lock = LOCK_INIT;
+ struct unpack_trees_options unpack_tree_opts;
+ struct tree *tree;
+ const char *reflog_action;
+ struct strbuf msg = STRBUF_INIT;
+ size_t prefix_len;
+ struct object_id *orig = NULL, oid_orig,
+ *old_orig = NULL, oid_old_orig;
+ int ret = 0, nr = 0;
+
+ if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+ BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+ if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+ ret = -1;
+ goto leave_reset_head;
+ }
+
+ if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
+ ret = error(_("could not determine HEAD revision"));
+ goto leave_reset_head;
+ }
+
+ if (!oid)
+ oid = &head_oid;
+
+ if (refs_only)
+ goto reset_head_refs;
+
+ memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+ setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+ unpack_tree_opts.head_idx = 1;
+ unpack_tree_opts.src_index = the_repository->index;
+ unpack_tree_opts.dst_index = the_repository->index;
+ unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
+ unpack_tree_opts.update = 1;
+ unpack_tree_opts.merge = 1;
+ if (!detach_head)
+ unpack_tree_opts.reset = 1;
+
+ if (repo_read_index_unmerged(the_repository) < 0) {
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
+ }
+
+ if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++],
&head_oid)) {
+ ret = error(_("failed to find tree of %s"),
+ oid_to_hex(&head_oid));
+ goto leave_reset_head;
+ }
+
+ if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) {
+ ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ goto leave_reset_head;
+ }
+
+ if (unpack_trees(nr, desc, &unpack_tree_opts)) {
+ ret = -1;
+ goto leave_reset_head;
+ }
+
+ tree = parse_tree_indirect(oid);
+ prime_cache_tree(the_repository, the_repository->index, tree);
+
+ if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
+ ret = error(_("co
Re: [RFC PATCH 2/7] autostash: extract read_one() from rebase
Hi Denton
On 16/10/2019 18:26, Denton Liu wrote:
Begin the process of lib-ifying the autostash code. In a future commit,
this will be used to implement `--autostash` in other builtins.
This patch is best viewed with `--color-moved` and
`--color-moved-ws=allow-indentation-change`.
Signed-off-by: Denton Liu
---
autostash.c | 12
autostash.h | 9 +
builtin/rebase.c | 10 +-
3 files changed, 22 insertions(+), 9 deletions(-)
create mode 100644 autostash.c
create mode 100644 autostash.h
diff --git a/autostash.c b/autostash.c
new file mode 100644
index 00..a6898e0fda
--- /dev/null
+++ b/autostash.c
@@ -0,0 +1,12 @@
+#include "git-compat-util.h"
+#include "autostash.h"
+#include "gettext.h"
+#include "strbuf.h"
+
+int read_one(const char *path, struct strbuf *buf)
+{
+ if (strbuf_read_file(buf, path, 0) < 0)
+ return error_errno(_("could not read '%s'"), path);
+ strbuf_trim_trailing_newline(buf);
+ return 0;
+}
This looks like it's doing a similar job to read_oneliner() in
sequencer.c, is it possible to make that public and use it instead?
(There may be a difference if the file is missing but that function
already takes a flag so it could probably be modified easily enough.)
Best Wishes
Phillip
diff --git a/autostash.h b/autostash.h
new file mode 100644
index 00..4a8f504f12
--- /dev/null
+++ b/autostash.h
@@ -0,0 +1,9 @@
+#ifndef AUTOSTASH_H
+#define AUTOSTASH_H
+
+#include "strbuf.h"
+
+/* Read one file, then strip line endings */
+int read_one(const char *path, struct strbuf *buf);
+
+#endif
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a20582e72..9fd7de6b2f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -27,6 +27,7 @@
#include "branch.h"
#include "sequencer.h"
#include "rebase-interactive.h"
+#include "autostash.h"
static char const * const builtin_rebase_usage[] = {
N_("git rebase [-i] [options] [--exec ] "
@@ -561,15 +562,6 @@ static const char *state_dir_path(const char *filename,
struct rebase_options *o
return path.buf;
}
-/* Read one file, then strip line endings */
-static int read_one(const char *path, struct strbuf *buf)
-{
- if (strbuf_read_file(buf, path, 0) < 0)
- return error_errno(_("could not read '%s'"), path);
- strbuf_trim_trailing_newline(buf);
- return 0;
-}
-
/* Initialize the rebase options from the state directory. */
static int read_basic_state(struct rebase_options *opts)
{
[PATCH v2 5/6] move run_commit_hook() to libgit and use it there
From: Phillip Wood
This function was declared in commit.h but was implemented in
builtin/commit.c so was not part of libgit. Move it to libgit so we can
use it in the sequencer. This simplifies the implementation of
run_prepare_commit_msg_hook() and will be used in the next commit.
Signed-off-by: Phillip Wood
---
builtin/commit.c | 22 --
commit.c | 24
sequencer.c | 23 ++-
3 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1921401117..d898a57f5d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1443,28 +1443,6 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
return git_status_config(k, v, s);
}
-int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...)
-{
- struct argv_array hook_env = ARGV_ARRAY_INIT;
- va_list args;
- int ret;
-
- argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
-
- /*
-* Let the hook know that no editor will be launched.
-*/
- if (!editor_is_used)
- argv_array_push(&hook_env, "GIT_EDITOR=:");
-
- va_start(args, name);
- ret = run_hook_ve(hook_env.argv,name, args);
- va_end(args);
- argv_array_clear(&hook_env);
-
- return ret;
-}
-
int cmd_commit(int argc, const char **argv, const char *prefix)
{
const char *argv_gc_auto[] = {"gc", "--auto", NULL};
diff --git a/commit.c b/commit.c
index 26ce0770f6..7ca8d12174 100644
--- a/commit.c
+++ b/commit.c
@@ -19,6 +19,7 @@
#include "advice.h"
#include "refs.h"
#include "commit-reach.h"
+#include "run-command.h"
static struct commit_extra_header *read_commit_extra_header_lines(const char
*buf, size_t len, const char **);
@@ -1581,3 +1582,26 @@ size_t ignore_non_trailer(const char *buf, size_t len)
}
return boc ? len - boc : len - cutoff;
}
+
+int run_commit_hook(int editor_is_used, const char *index_file,
+ const char *name, ...)
+{
+ struct argv_array hook_env = ARGV_ARRAY_INIT;
+ va_list args;
+ int ret;
+
+ argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+
+ /*
+* Let the hook know that no editor will be launched.
+*/
+ if (!editor_is_used)
+ argv_array_push(&hook_env, "GIT_EDITOR=:");
+
+ va_start(args, name);
+ ret = run_hook_ve(hook_env.argv,name, args);
+ va_end(args);
+ argv_array_clear(&hook_env);
+
+ return ret;
+}
diff --git a/sequencer.c b/sequencer.c
index 2adcf5a639..cdc0d1dfba 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,25 +1127,22 @@ static int run_prepare_commit_msg_hook(struct
repository *r,
struct strbuf *msg,
const char *commit)
{
- struct argv_array hook_env = ARGV_ARRAY_INIT;
- int ret;
- const char *name;
+ int ret = 0;
+ const char *name, *arg1 = NULL, *arg2 = NULL;
name = git_path_commit_editmsg();
if (write_message(msg->buf, msg->len, name, 0))
return -1;
- argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", r->index_file);
- argv_array_push(&hook_env, "GIT_EDITOR=:");
- if (commit)
- ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
- "commit", commit, NULL);
- else
- ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
- "message", NULL);
- if (ret)
+ if (commit) {
+ arg1 = "commit";
+ arg2 = commit;
+ } else {
+ arg1 = "message";
+ }
+ if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
+ arg1, arg2, NULL))
ret = error(_("'prepare-commit-msg' hook failed"));
- argv_array_clear(&hook_env);
return ret;
}
--
gitgitgadget
[PATCH v2 4/6] sequencer.h fix placement of #endif
From: Phillip Wood
Commit 65850686cf ("rebase -i: rewrite write_basic_state() in C",
2018-08-28) accidentially added new function declarations after
the #endif at the end of the include guard.
Signed-off-by: Phillip Wood
---
sequencer.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sequencer.h b/sequencer.h
index 0c494b83d4..ac66892d71 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -195,11 +195,10 @@ void print_commit_summary(struct repository *repo,
int read_author_script(const char *path, char **name, char **email, char
**date,
int allow_missing);
-#endif
-
void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
int write_basic_state(struct replay_opts *opts, const char *head_name,
struct commit *onto, const char *orig_head);
void sequencer_post_commit_cleanup(struct repository *r);
int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
+#endif /* SEQUENCER_H */
--
gitgitgadget
[PATCH v2 2/6] t3404: set $EDITOR in subshell
From: Phillip Wood
As $EDITOR is exported setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell. This commit leaves
20 calls to set_fake_editor that are not in subshells as they can
safely be removed in the next commit once all the other editor setting
is done inside subshells.
I have moved the call to set_fake_editor in some tests so it comes
immediately before the call to 'git rebase' to avoid moving unrelated
commands into the subshell. In one case ('rebase -ix with
--autosquash') the call to set_fake_editor is moved past an invocation
of 'git rebase'. This is safe as that invocation of 'git rebase'
requires EDITOR=: or EDITOR=fake-editor.sh without FAKE_LINES being
set which will be the case as the preceding tests either set their
editor in a subshell or call set_fake_editor without setting FAKE_LINES.
In a one test ('auto-amend only edited commits after "edit"') a call
to test_tick are now in a subshell. I think this is OK as it is there
to set the date for the next commit which is executed in the same
subshell rather than updating GIT_COMMITTER_DATE for later tests (the
next test calls test_tick before doing anything else).
Signed-off-by: Phillip Wood
---
t/t3404-rebase-interactive.sh | 546 +-
1 file changed, 342 insertions(+), 204 deletions(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c26b3362ef..cb9b21 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -79,8 +79,11 @@ test_expect_success 'rebase -i with empty HEAD' '
cat >expect <<-\EOF &&
error: nothing to do
EOF
- set_fake_editor &&
- test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual
2>&1 &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="1 exec_true" \
+ git rebase -i HEAD^ >actual 2>&1
+ ) &&
test_i18ncmp expect actual
'
@@ -139,8 +142,11 @@ test_expect_success 'rebase -i sets work tree properly' '
test_expect_success 'rebase -i with the exec command checks tree cleanness' '
git checkout master &&
- set_fake_editor &&
- test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i
HEAD^ &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" \
+ git rebase -i HEAD^
+ ) &&
test_cmp_rev master^ HEAD &&
git reset --hard &&
git rebase --continue
@@ -168,9 +174,11 @@ test_expect_success 'rebase -x with newline in command
fails' '
test_expect_success 'rebase -i with exec of inexistent command' '
git checkout master &&
test_when_finished "git rebase --abort" &&
- set_fake_editor &&
- test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
- git rebase -i HEAD^ >actual 2>&1 &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="exec_this-command-does-not-exist
1" \
+ git rebase -i HEAD^ >actual 2>&1
+ ) &&
! grep "Maybe git-rebase is broken" actual
'
@@ -230,8 +238,10 @@ test_expect_success 'reflog for the branch shows correct
finish message' '
'
test_expect_success 'exchange two commits' '
- set_fake_editor &&
- FAKE_LINES="2 1" git rebase -i HEAD~2 &&
+ (
+ set_fake_editor &&
+ FAKE_LINES="2 1" git rebase -i HEAD~2
+ ) &&
test H = $(git cat-file commit HEAD^ | sed -ne \$p) &&
test G = $(git cat-file commit HEAD | sed -ne \$p)
'
@@ -332,9 +342,11 @@ test_expect_success 'squash' '
test_tick &&
GIT_AUTHOR_NAME="Nitfol" git commit -m "nitfol" file7 &&
echo "**" &&
- set_fake_editor &&
- FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=2 \
- git rebase -i --onto master HEAD~2 &&
+ (
+ set_fake_editor &&
+ FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=2 \
+ git rebase -i --onto master HEAD~2
+ ) &&
test B = $(cat file7) &&
test $(git rev-parse HEAD^) = $(git rev-parse master)
'
@@ -355,8 +367,10 @@ test_expect_success REBA
[PATCH v2 0/6] sequencer: start running post-commit hook again
When I converted the sequencer to avoid forking git commit i forgot about
the post-commit hook. These patches are based on
pw/rebase-i-show-HEAD-to-reword, otherwise the new test fails as that branch
changes the number of commits we make.
Thanks to Dscho & Junio for their comments on V1. I've made the following
changes:
Patches 1-3 These are new patches in response to Dscho's request to set
$EDITOR in a subshell. There were ~80 other tests that weren't doing that
and they are fixed in these patches. Patch 2 contains the main action,
unfortunately due to a combination of having to remove the trailing ' &&'
from the last line moved into the subshell and re-wrapping some lines due to
the increased indentation --color-moved and --color-moved-ws are of limited
use when viewing this patch.
Patch 4 (was 1) Unchanged
Patch 5 (was 2) I've moved the function definition to commit.c rather than
sequencer.c as suggested. I've also removed an unused struct argv_array from
run_prepare_commit_msg_hook() (There wasn't a compiler warning as it was
still calling argv_array_clear() at the end of the function) and reworded
the commit message.
Patch 6 (was 3) I've tided up the test and removed the wrapper function for
running the post-commit hook as suggested.
Phillip Wood (6):
t3404: remove unnecessary subshell
t3404: set $EDITOR in subshell
t3404: remove uneeded calls to set_fake_editor
sequencer.h fix placement of #endif
move run_commit_hook() to libgit and use it there
sequencer: run post-commit hook
builtin/commit.c | 22 --
commit.c | 24 ++
sequencer.c | 24 +-
sequencer.h | 3 +-
t/lib-rebase.sh | 28 ++
t/t3404-rebase-interactive.sh | 596 +-
6 files changed, 432 insertions(+), 265 deletions(-)
base-commit: b0a3186140dbc7bd64cbc6ef733386a0f1eb6a4d
Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-388%2Fphillipwood%2Fwip%2Frebase-commit-hooks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-388/phillipwood/wip/rebase-commit-hooks-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/388
Range-diff vs v1:
-: -- > 1: b9689e85e5 t3404: remove unnecessary subshell
-: -- > 2: 96432cd0f0 t3404: set $EDITOR in subshell
-: -- > 3: 09857dee78 t3404: remove uneeded calls to set_fake_editor
1: 7305f8d8e8 = 4: 0eac3ede02 sequencer.h fix placement of #endif
2: 420ecf442c ! 5: f394a0e163 sequencer: use run_commit_hook()
@@ -1,9 +1,11 @@
Author: Phillip Wood
-sequencer: use run_commit_hook()
+move run_commit_hook() to libgit and use it there
-This simplifies the implementation of run_prepare_commit_msg_hook()
and
-will be used in the next commit.
+This function was declared in commit.h but was implemented in
+builtin/commit.c so was not part of libgit. Move it to libgit so we
can
+use it in the sequencer. This simplifies the implementation of
+run_prepare_commit_msg_hook() and will be used in the next commit.
Signed-off-by: Phillip Wood
@@ -40,25 +42,22 @@
{
const char *argv_gc_auto[] = {"gc", "--auto", NULL};
- diff --git a/commit.h b/commit.h
- --- a/commit.h
- +++ b/commit.h
+ diff --git a/commit.c b/commit.c
+ --- a/commit.c
+ +++ b/commit.c
@@
- int compare_commits_by_commit_date(const void *a_, const void *b_, void
*unused);
- int compare_commits_by_gen_then_commit_date(const void *a_, const void
*b_, void *unused);
+ #include "advice.h"
+ #include "refs.h"
+ #include "commit-reach.h"
++#include "run-command.h"
+
+ static struct commit_extra_header *read_commit_extra_header_lines(const
char *buf, size_t len, const char **);
--LAST_ARG_MUST_BE_NULL
--int run_commit_hook(int editor_is_used, const char *index_file, const
char *name, ...);
--
- #endif /* COMMIT_H */
-
- diff --git a/sequencer.c b/sequencer.c
- --- a/sequencer.c
- +++ b/sequencer.c
@@
- run_rewrite_hook(&old_head->object.oid, new_head);
+ }
+ return boc ? len - boc : len - cutoff;
}
-
++
+int run_commit_hook(int editor_is_used, const char *index_file,
+ const char *name, ...)
+{
@@ -81,12 +80,15 @@
+
+ return ret;
+}
-+
- static int run_prepare_commit_msg_hook(struct repository *r,
+
+ diff --git a/sequencer.c b/sequencer.c
+ --- a/sequencer.c
+ +++ b/sequencer.c
+@@
struct strbuf *msg,
const char *commit)
{
- struct a
[PATCH v2 1/6] t3404: remove unnecessary subshell
From: Phillip Wood Neither of the commands executed in the subshell change any shell variables or the current directory so there is no need for them to be executed in a subshell. Signed-off-by: Phillip Wood --- t/t3404-rebase-interactive.sh | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index d2f1d5bd23..c26b3362ef 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -945,10 +945,8 @@ test_expect_success C_LOCALE_OUTPUT 'rebase -ix with --autosquash' ' git add bis.txt && git commit -m "fixup! two_exec" && set_fake_editor && - ( - git checkout -b autosquash_actual && - git rebase -i --exec "git show HEAD" --autosquash HEAD~4 >actual - ) && + git checkout -b autosquash_actual && + git rebase -i --exec "git show HEAD" --autosquash HEAD~4 >actual && git checkout autosquash && ( git checkout -b autosquash_expected && -- gitgitgadget
[PATCH v2 6/6] sequencer: run post-commit hook
From: Phillip Wood
Prior to commit 356ee4659b ("sequencer: try to commit without forking
'git commit'", 2017-11-24) the sequencer would always run the
post-commit hook after each pick or revert as it forked `git commit` to
create the commit. The conversion to committing without forking `git
commit` omitted to call the post-commit hook after creating the commit.
Signed-off-by: Phillip Wood
---
sequencer.c | 1 +
t/t3404-rebase-interactive.sh | 19 +++
2 files changed, 20 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cdc0d1dfba..da2decbd3a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1401,6 +1401,7 @@ static int try_to_commit(struct repository *r,
goto out;
}
+ run_commit_hook(0, r->index_file, "post-commit", NULL);
if (flags & AMEND_MSG)
commit_post_rewrite(r, current_head, oid);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c5d0326825..c573c99069 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1583,6 +1583,25 @@ test_expect_success 'valid author header when author
contains single quote' '
test_cmp expected actual
'
+test_expect_success 'post-commit hook is called' '
+ test_when_finished "rm -f .git/hooks/post-commit" &&
+ >actual &&
+ mkdir -p .git/hooks &&
+ write_script .git/hooks/post-commit <<-\EOS &&
+ git rev-parse HEAD >>actual
+ EOS
+ (
+ set_fake_editor &&
+ FAKE_LINES="edit 4 1 reword 2 fixup 3" git rebase -i A E &&
+ echo x>file3 &&
+ git add file3 &&
+ FAKE_COMMIT_MESSAGE=edited git rebase --continue
+ ) &&
+ git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
+ >expect &&
+ test_cmp expect actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
--
gitgitgadget
[PATCH v2 3/6] t3404: remove uneeded calls to set_fake_editor
From: Phillip Wood
Some tests were calling set_fake_editor to ensure they had a sane no-op
editor set. Now that all the editor setting is done in subshells these
tests can rely on EDITOR=: and so do not need to call set_fake_editor.
Also add a test at the end to detect any future additions messing with
the exported value of $EDITOR.
Signed-off-by: Phillip Wood
---
t/lib-rebase.sh | 28
t/t3404-rebase-interactive.sh | 25 +
2 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 7ea30e5006..e4554db85e 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -118,3 +118,31 @@ make_empty () {
git commit --allow-empty -m "$1" &&
git tag "$1"
}
+
+# Call this (inside test_expect_success) at the end of a test file to
+# check that no tests have changed editor related environment
+# variables or config settings
+test_editor_unchanged () {
+ # We're only interested in exported variables hence 'sh -c'
+ sh -c 'cat >actual <<-EOF
+ EDITOR=$EDITOR
+ FAKE_COMMIT_AMEND=$FAKE_COMMIT_AMEND
+ FAKE_COMMIT_MESSAGE=$FAKE_COMMIT_MESSAGE
+ FAKE_LINES=$FAKE_LINES
+ GIT_EDITOR=$GIT_EDITOR
+ GIT_SEQUENCE_EDITOR=$GIT_SEQUENCE_EDITOR
+ core.editor=$(git config core.editor)
+ sequence.editor=$(git config sequence.editor)
+ EOF'
+ cat >expect <<-\EOF
+ EDITOR=:
+ FAKE_COMMIT_AMEND=
+ FAKE_COMMIT_MESSAGE=
+ FAKE_LINES=
+ GIT_EDITOR=
+ GIT_SEQUENCE_EDITOR=
+ core.editor=
+ sequence.editor=
+ EOF
+ test_cmp expect actual
+}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index cb9b21..c5d0326825 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -189,7 +189,6 @@ test_expect_success 'implicit interactive rebase does not
invoke sequence editor
test_expect_success 'no changes are a nop' '
git checkout branch2 &&
- set_fake_editor &&
git rebase -i F &&
test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
test $(git rev-parse I) = $(git rev-parse HEAD)
@@ -199,7 +198,6 @@ test_expect_success 'test the [branch] option' '
git checkout -b dead-end &&
git rm file6 &&
git commit -m "stop here" &&
- set_fake_editor &&
git rebase -i F branch2 &&
test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
test $(git rev-parse I) = $(git rev-parse branch2) &&
@@ -208,7 +206,6 @@ test_expect_success 'test the [branch] option' '
test_expect_success 'test --onto ' '
git checkout -b test-onto branch2 &&
- set_fake_editor &&
git rebase -i --onto branch1 F &&
test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
@@ -218,7 +215,6 @@ test_expect_success 'test --onto ' '
test_expect_success 'rebase on top of a non-conflicting commit' '
git checkout branch1 &&
git tag original-branch1 &&
- set_fake_editor &&
git rebase -i branch2 &&
test file6 = $(git diff --name-only original-branch1) &&
test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
@@ -264,7 +260,6 @@ test_expect_success 'stop on conflicting pick' '
>>>>>>> 5d18e54... G
EOF
git tag new-branch1 &&
- set_fake_editor &&
test_must_fail git rebase -i master &&
test "$(git rev-parse HEAD~3)" = "$(git rev-parse master)" &&
test_cmp expect .git/rebase-merge/patch &&
@@ -293,7 +288,6 @@ test_expect_success 'abort' '
test_expect_success 'abort with error when new base cannot be checked out' '
git rm --cached file1 &&
git commit -m "remove file in base" &&
- set_fake_editor &&
test_must_fail git rebase -i master > output 2>&1 &&
test_i18ngrep "The following untracked working tree files would be
overwritten by checkout:" \
output &&
@@ -308,7 +302,6 @@ test_expect_success 'retain authorship' '
test_tick &&
GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
git tag twerp &&
- set_fake_editor &&
Re: [PATCH 2/3] sequencer: use run_commit_hook()
Hi Dscho & Junio On 11/10/2019 05:24, Junio C Hamano wrote: > Johannes Schindelin writes: > >>> builtin/commit.c | 22 -- >>> commit.h | 3 --- >>> sequencer.c | 45 ++--- >>> sequencer.h | 2 ++ >>> 4 files changed, 36 insertions(+), 36 deletions(-) >> >> Hmm. I would have thought that `commit.c` would be a more logical home >> for that function (and that the declaration could remain in `commit.h`)? > > Good correction. There are some other public commit related functions in sequencer.c - print_commit_summary(), commit_post_rewrite(), rest_is_empty(), cleanup_message(), message_is_empty(), template_untouched(), update_head_with_reflog() . Would you like to see them moved to commit.c (probably as a separate series)? Best Wishes Phillip > > Thanks. >
Re: [PATCH 3/3] sequencer: run post-commit hook
Hi Dscho
On 10/10/2019 22:31, Johannes Schindelin wrote:
Hi Phillip,
On Thu, 10 Oct 2019, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood
Prior to commit 356ee4659b ("sequencer: try to commit without forking
'git commit'", 2017-11-24) the sequencer would always run the
post-commit hook after each pick or revert as it forked `git commit` to
create the commit. The conversion to committing without forking `git
commit` omitted to call the post-commit hook after creating the commit.
Signed-off-by: Phillip Wood
Makes sense.
---
builtin/commit.c | 2 +-
sequencer.c | 5 +
sequencer.h | 1 +
t/t3404-rebase-interactive.sh | 17 +
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d898a57f5d..adb8c89c60 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
repo_rerere(the_repository, 0);
run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
- run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
+ run_post_commit_hook(use_editor, get_index_file());
Does it really make sense to abstract the hook name away? It adds a lot
of churn for just two callers...
I'll drop the new function in the reroll
if (amend && !no_post_rewrite) {
commit_post_rewrite(the_repository, current_head, &oid);
}
diff --git a/sequencer.c b/sequencer.c
index 3ce578c40b..b4947f6969 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1173,6 +1173,10 @@ static int run_prepare_commit_msg_hook(struct repository
*r,
return ret;
}
+void run_post_commit_hook(int editor_is_used, const char *index_file) {
+ run_commit_hook(editor_is_used, index_file, "post-commit", NULL);
+}
+
If we must have a separate `run_post_commit_hook()`, then it should be
an `inline` function, defined in the header. Or even a macro to begin
with.
static const char implicit_ident_advice_noconfig[] =
N_("Your name and email address were configured automatically based\n"
"on your username and hostname. Please check that they are accurate.\n"
@@ -1427,6 +1431,7 @@ static int try_to_commit(struct repository *r,
goto out;
}
+ run_post_commit_hook(0, r->index_file);
So this is the _actual_ change of this patch.
if (flags & AMEND_MSG)
commit_post_rewrite(r, current_head, oid);
diff --git a/sequencer.h b/sequencer.h
index b0419d6ddb..e3e73c5635 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -203,4 +203,5 @@ int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
LAST_ARG_MUST_BE_NULL
int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...);
+void run_post_commit_hook(int editor_is_used, const char *index_file);
#endif /* SEQUENCER_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2f1d5bd23..d9217235b6 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author
contains single quote' '
test_cmp expected actual
'
+test_expect_success 'post-commit hook is called' '
+ test_when_finished "rm -f .git/hooks/post-commit commits" &&
+ mkdir -p .git/hooks &&
+ write_script .git/hooks/post-commit <<-\EOS &&
+ git rev-parse HEAD >>commits
Should `commits` be initialized before this script is written, e.g.
using
>commits &&
Good point, especially if it is renamed to actual as Junio suggests
+ EOS
+ set_fake_editor &&
The `set_fake_editor` function sets a global environment variable, and
therefore needs to be run in a subshell. Therefore, this line (as well
as the next one) need to be enclosed in `( ... )`.
There are ~80 instances of
set_fake_editor/test_set_editor/set_cat_todo_editor in that file that
are not in subshells. I've converted them in a preparatory patch (that
was fun), removing about 20 that can now safely rely on EDITOR=:
(hopefully that will ameliorate the performance hit of ~60 extra
subshells a little)
+ FAKE_LINES="edit 4 1 reword 2 fixup 3" git rebase -i A E &&
+ echo x>file3 &&
We usually leave no space after the `>`, but we _do_ leave a space
_before_ the `>`.
+ git add file3 &&
+ FAKE_COMMIT_MESSAGE=edited git rebase --continue &&
+ # rev-list does not support -g --reverse
+ git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
+ HEAD@{1} HEAD >expected &&
Wouldn't this be better
[PATCH 3/3] sequencer: run post-commit hook
From: Phillip Wood
Prior to commit 356ee4659b ("sequencer: try to commit without forking
'git commit'", 2017-11-24) the sequencer would always run the
post-commit hook after each pick or revert as it forked `git commit` to
create the commit. The conversion to committing without forking `git
commit` omitted to call the post-commit hook after creating the commit.
Signed-off-by: Phillip Wood
---
builtin/commit.c | 2 +-
sequencer.c | 5 +
sequencer.h | 1 +
t/t3404-rebase-interactive.sh | 17 +
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d898a57f5d..adb8c89c60 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
repo_rerere(the_repository, 0);
run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
- run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
+ run_post_commit_hook(use_editor, get_index_file());
if (amend && !no_post_rewrite) {
commit_post_rewrite(the_repository, current_head, &oid);
}
diff --git a/sequencer.c b/sequencer.c
index 3ce578c40b..b4947f6969 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1173,6 +1173,10 @@ static int run_prepare_commit_msg_hook(struct repository
*r,
return ret;
}
+void run_post_commit_hook(int editor_is_used, const char *index_file) {
+ run_commit_hook(editor_is_used, index_file, "post-commit", NULL);
+}
+
static const char implicit_ident_advice_noconfig[] =
N_("Your name and email address were configured automatically based\n"
"on your username and hostname. Please check that they are accurate.\n"
@@ -1427,6 +1431,7 @@ static int try_to_commit(struct repository *r,
goto out;
}
+ run_post_commit_hook(0, r->index_file);
if (flags & AMEND_MSG)
commit_post_rewrite(r, current_head, oid);
diff --git a/sequencer.h b/sequencer.h
index b0419d6ddb..e3e73c5635 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -203,4 +203,5 @@ int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
LAST_ARG_MUST_BE_NULL
int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...);
+void run_post_commit_hook(int editor_is_used, const char *index_file);
#endif /* SEQUENCER_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2f1d5bd23..d9217235b6 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author
contains single quote' '
test_cmp expected actual
'
+test_expect_success 'post-commit hook is called' '
+ test_when_finished "rm -f .git/hooks/post-commit commits" &&
+ mkdir -p .git/hooks &&
+ write_script .git/hooks/post-commit <<-\EOS &&
+ git rev-parse HEAD >>commits
+ EOS
+ set_fake_editor &&
+ FAKE_LINES="edit 4 1 reword 2 fixup 3" git rebase -i A E &&
+ echo x>file3 &&
+ git add file3 &&
+ FAKE_COMMIT_MESSAGE=edited git rebase --continue &&
+ # rev-list does not support -g --reverse
+ git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
+ HEAD@{1} HEAD >expected &&
+ test_cmp expected commits
+'
+
test_done
--
gitgitgadget
[PATCH 1/3] sequencer.h fix placement of #endif
From: Phillip Wood
Commit 65850686cf ("rebase -i: rewrite write_basic_state() in C",
2018-08-28) accidentially added new function declarations after
the #endif at the end of the include guard.
Signed-off-by: Phillip Wood
---
sequencer.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sequencer.h b/sequencer.h
index 0c494b83d4..ac66892d71 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -195,11 +195,10 @@ void print_commit_summary(struct repository *repo,
int read_author_script(const char *path, char **name, char **email, char
**date,
int allow_missing);
-#endif
-
void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
int write_basic_state(struct replay_opts *opts, const char *head_name,
struct commit *onto, const char *orig_head);
void sequencer_post_commit_cleanup(struct repository *r);
int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
+#endif /* SEQUENCER_H */
--
gitgitgadget
[PATCH 2/3] sequencer: use run_commit_hook()
From: Phillip Wood
This simplifies the implementation of run_prepare_commit_msg_hook() and
will be used in the next commit.
Signed-off-by: Phillip Wood
---
builtin/commit.c | 22 --
commit.h | 3 ---
sequencer.c | 45 ++---
sequencer.h | 2 ++
4 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1921401117..d898a57f5d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1443,28 +1443,6 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
return git_status_config(k, v, s);
}
-int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...)
-{
- struct argv_array hook_env = ARGV_ARRAY_INIT;
- va_list args;
- int ret;
-
- argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
-
- /*
-* Let the hook know that no editor will be launched.
-*/
- if (!editor_is_used)
- argv_array_push(&hook_env, "GIT_EDITOR=:");
-
- va_start(args, name);
- ret = run_hook_ve(hook_env.argv,name, args);
- va_end(args);
- argv_array_clear(&hook_env);
-
- return ret;
-}
-
int cmd_commit(int argc, const char **argv, const char *prefix)
{
const char *argv_gc_auto[] = {"gc", "--auto", NULL};
diff --git a/commit.h b/commit.h
index f5295ca7f3..37684a35f0 100644
--- a/commit.h
+++ b/commit.h
@@ -389,7 +389,4 @@ void verify_merge_signature(struct commit *commit, int
verbose);
int compare_commits_by_commit_date(const void *a_, const void *b_, void
*unused);
int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_,
void *unused);
-LAST_ARG_MUST_BE_NULL
-int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...);
-
#endif /* COMMIT_H */
diff --git a/sequencer.c b/sequencer.c
index 2adcf5a639..3ce578c40b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1123,28 +1123,51 @@ void commit_post_rewrite(struct repository *r,
run_rewrite_hook(&old_head->object.oid, new_head);
}
+int run_commit_hook(int editor_is_used, const char *index_file,
+ const char *name, ...)
+{
+ struct argv_array hook_env = ARGV_ARRAY_INIT;
+ va_list args;
+ int ret;
+
+ argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+
+ /*
+* Let the hook know that no editor will be launched.
+*/
+ if (!editor_is_used)
+ argv_array_push(&hook_env, "GIT_EDITOR=:");
+
+ va_start(args, name);
+ ret = run_hook_ve(hook_env.argv,name, args);
+ va_end(args);
+ argv_array_clear(&hook_env);
+
+ return ret;
+}
+
static int run_prepare_commit_msg_hook(struct repository *r,
struct strbuf *msg,
const char *commit)
{
struct argv_array hook_env = ARGV_ARRAY_INIT;
- int ret;
- const char *name;
+ int ret = 0;
+ const char *name, *arg1 = NULL, *arg2 = NULL;
name = git_path_commit_editmsg();
if (write_message(msg->buf, msg->len, name, 0))
return -1;
- argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", r->index_file);
- argv_array_push(&hook_env, "GIT_EDITOR=:");
- if (commit)
- ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
- "commit", commit, NULL);
- else
- ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
- "message", NULL);
- if (ret)
+ if (commit) {
+ arg1 = "commit";
+ arg2 = commit;
+ } else {
+ arg1 = "message";
+ }
+ if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
+ arg1, arg2, NULL))
ret = error(_("'prepare-commit-msg' hook failed"));
+
argv_array_clear(&hook_env);
return ret;
diff --git a/sequencer.h b/sequencer.h
index ac66892d71..b0419d6ddb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -201,4 +201,6 @@ int write_basic_state(struct replay_opts *opts, const char
*head_name,
void sequencer_post_commit_cleanup(struct repository *r);
int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
+LAST_ARG_MUST_BE_NULL
+int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...);
#endif /* SEQUENCER_H */
--
gitgitgadget
[PATCH 0/3] sequencer: start running post-commit hook again
When I converted the sequencer to avoid forking git commit i forgot about the post-commit hook. These patches are based on pw/rebase-i-show-HEAD-to-reword, otherwise the new test fails as that branch changes the number of commits we make. Phillip Wood (3): sequencer.h fix placement of #endif sequencer: use run_commit_hook() sequencer: run post-commit hook builtin/commit.c | 24 + commit.h | 3 --- sequencer.c | 50 +++ sequencer.h | 6 +++-- t/t3404-rebase-interactive.sh | 17 5 files changed, 61 insertions(+), 39 deletions(-) base-commit: b0a3186140dbc7bd64cbc6ef733386a0f1eb6a4d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-388%2Fphillipwood%2Fwip%2Frebase-commit-hooks-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-388/phillipwood/wip/rebase-commit-hooks-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/388 -- gitgitgadget
Re: Raise your hand to Ack jk/code-of-conduct if your Ack fell thru cracks
Hi Junio On 09/10/2019 01:14, Junio C Hamano wrote: Johannes Schindelin writes: In other words, the commit message can be augmented by this: Acked-by: Johannes Schindelin Acked-by: Derrick Stolee Acked-by: Garima Singh Acked-by: Jonathan Tan Acked-by: Thomas Gummerer Acked-by: brian m. carlson Acked-by: Elijah Newren Junio, would you mind picking it up, please? I trust you enough that I won't go back to the cited messages to double check that these acks are real, but I'd still wait for a few days for people who expressed their Acks but your scan missed, or those who wanted to give their Acks but forgot to do so, to raise their hands on this thread. I forgot to add Acked-by: Phillip Wood to my original reply in this thread, could you add it please Thanks Phillip Thanks for starting the concluding move on this topic. For reference, here is the CoC the patch wants to add (there is no such topic yet locally, nor a single patch that can be made into such a topic, so there isn't exactly something people can Ack on yet. So here is a "preview" of what we would see once such a series lands). CODE_OF_CONDUCT.md | 93 ++ # Git Code of Conduct This code of conduct outlines our expectations for participants within the Git community, as well as steps for reporting unacceptable behavior. We are committed to providing a welcoming and inspiring community for all and expect our code of conduct to be honored. Anyone who violates this code of conduct may be banned from the community. ## Our Pledge In the interest of fostering an open and welcoming environment, we as contributors and maintainers pledge to make participation in our project and our community a harassment-free experience for everyone, regardless of age, body size, disability, ethnicity, sex characteristics, gender identity and expression, level of experience, education, socio-economic status, nationality, personal appearance, race, religion, or sexual identity and orientation. ## Our Standards Examples of behavior that contributes to creating a positive environment include: * Using welcoming and inclusive language * Being respectful of differing viewpoints and experiences * Gracefully accepting constructive criticism * Focusing on what is best for the community * Showing empathy towards other community members Examples of unacceptable behavior by participants include: * The use of sexualized language or imagery and unwelcome sexual attention or advances * Trolling, insulting/derogatory comments, and personal or political attacks * Public or private harassment * Publishing others' private information, such as a physical or electronic address, without explicit permission * Other conduct which could reasonably be considered inappropriate in a professional setting ## Our Responsibilities Project maintainers are responsible for clarifying the standards of acceptable behavior and are expected to take appropriate and fair corrective action in response to any instances of unacceptable behavior. Project maintainers have the right and responsibility to remove, edit, or reject comments, commits, code, wiki edits, issues, and other contributions that are not aligned to this Code of Conduct, or to ban temporarily or permanently any contributor for other behaviors that they deem inappropriate, threatening, offensive, or harmful. ## Scope This Code of Conduct applies within all project spaces, and it also applies when an individual is representing the project or its community in public spaces. Examples of representing a project or community include using an official project e-mail address, posting via an official social media account, or acting as an appointed representative at an online or offline event. Representation of a project may be further defined and clarified by project maintainers. ## Enforcement Instances of abusive, harassing, or otherwise unacceptable behavior may be reported by contacting the project team at g...@sfconservancy.org. All complaints will be reviewed and investigated and will result in a response that is deemed necessary and appropriate to the circumstances. The project team is obligated to maintain confidentiality with regard to the reporter of an incident. Further details of specific enforcement policies may be posted separately. Project maintainers who do not follow or enforce the Code of Conduct in good faith may face temporary or permanent repercussions as determined by other members of the project's leadership. The project leadership team can be contacted by email as a whole at g...@sfconservancy.org, or individually: - Ævar Arnfjörð Bjarmason - Christian Couder - Jeff King - Junio C Hamano ## Attribution This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4, available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
Re: [PATCH 1/1] commit: add support to provide --coauthor
On 08/10/2019 11:11, Phillip Wood wrote:
> Hi Toon & Zeger-Jan
>
> On 08/10/2019 08:49, Toon Claes wrote:
>> Add support to provide the Co-author when committing. For each
>> co-author provided with --coauthor=, a line is added at the
>> bottom of the commit message, like this:
>>
>> Co-authored-by:
>>
>> It's a common practice use when pairing up with other people and both
>> authors want to in the commit message.
>
> Thanks for the patch, it's looking good. I can see this being useful to
> some people - I like the way the patch itself is co-authored.
> [...]
>> @@ -803,6 +805,10 @@ static int prepare_to_commit(const char
>> *index_file, const char *prefix,
>> if (clean_message_contents)
>> strbuf_stripspace(&sb, 0);
>>
>> + for (i = 0; i < coauthors.nr; i++) {
>> + append_coauthor(&sb, coauthors.items[i].string);
>
> If you look at the existing code that's calling append_signoff() it does
> append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0)
> The purpose of ignore_non_trailer is to ignore comment and conflicts
> messages at the end of the commit message (there's more detail in a
> comment above it's definition in builtin/commit.c). I think we need to
> pass this offset when adding co-authors as well.
>
> One question - what is the desired de-duplication behavior of
> Co-authored-by:? What happens if there is already a matching
> Co-authored-by: footer? (It is also possible for the trailers code to
> only ignore an existing footer if it's the last one.) What happens if
> the same Co-authored-by: is duplicated on the command line? It would be
> nice to have this defined and tests to check it's enforced.
I should give a bit more detail here. git-interpret-trailers gives more
control over handling of duplicates that is configurable via 'git
config' than 'commit --signoff' does. The reason for this is that
'commit --signoff' predates the interpret-trailers stuff. As we're
adding a new footer command we should decide if we want it to act like
--signoff or give the user the ability to configure the de-duplication
behavior by using the interpret-trailers code path instead. (I think
format-patch --signoff respects the interpret-trailers config variables
but 'am --signoff' and 'cherry-pick --signoff' do not.
>
> Another useful addition would be to check that the footer value looks
> sane but that could come later
Looking at the way commit handles --author (grep for force_author in
builtin/commit.c) it should be simple to add these checks - just call
split_ident() and check it returns zero. --author also checks if the
string contains '<' and if it doesn't it uses the string given on the
command line to lookup a matching author in the commit log - that could
be a nice feature to use here too (see the code that calls
find_author_by_nickname()).
Best Wishes
Phillip
- I don't think we do that for any other
> footers at the moment (though I haven't checked to see if that's really
> true)
>
>> + }
>> +
>> if (signoff)
>> append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
>>
>> @@ -1504,6 +1510,7 @@ int cmd_commit(int argc, const char **argv,
>> const char *prefix)
>> OPT_STRING(0, "squash", &squash_message, N_("commit"),
>> N_("use autosquash formatted message to squash specified commit")),
>> OPT_BOOL(0, "reset-author", &renew_authorship, N_("the
>> commit is authored by me now (used with -C/-c/--amend)")),
>> OPT_BOOL('s', "signoff", &signoff, N_("add Signed-off-by:")),
>> + OPT_STRING_LIST(0, "coauthor", &coauthors, N_("co-author"),
>> N_("add Co-authored-by:")),
>> OPT_FILENAME('t', "template", &template_file, N_("use
>> specified template file")),
>> OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
>> OPT_CLEANUP(&cleanup_arg),
>> diff --git a/sequencer.c b/sequencer.c
>> index d648aaf416..8958a22470 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -36,6 +36,7 @@
>> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>
>> static const char sign_off_header[] = "Signed-off-by: ";
>> +static const char coauthor_header[] = "Co-authored-by: ";
>> static const char cherry_picked_prefix[] = "(cherry picked from
>&g
Re: [PATCH 1/1] commit: add support to provide --coauthor
ader[] = "Signed-off-by: ";
+static const char coauthor_header[] = "Co-authored-by: ";
static const char cherry_picked_prefix[] = "(cherry picked from commit ";
GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
@@ -4385,15 +4386,9 @@ int sequencer_pick_revisions(struct repository *r,
return res;
}
-void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
+static void append_footer(struct strbuf *msgbuf, struct strbuf* sob, size_t
ignore_footer, size_t no_dup_sob)
{
- unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
- struct strbuf sob = STRBUF_INIT;
- int has_footer;
-
- strbuf_addstr(&sob, sign_off_header);
- strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
- strbuf_addch(&sob, '\n');
+ size_t has_footer;
if (!ignore_footer)
strbuf_complete_line(msgbuf);
@@ -4402,11 +4397,11 @@ void append_signoff(struct strbuf *msgbuf, size_t
ignore_footer, unsigned flag)
* If the whole message buffer is equal to the sob, pretend that we
* found a conforming footer with a matching sob
*/
- if (msgbuf->len - ignore_footer == sob.len &&
- !strncmp(msgbuf->buf, sob.buf, sob.len))
+ if (msgbuf->len - ignore_footer == sob->len &&
+ !strncmp(msgbuf->buf, sob->buf, sob->len))
has_footer = 3;
else
- has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
+ has_footer = has_conforming_footer(msgbuf, sob, ignore_footer);
if (!has_footer) {
const char *append_newlines = NULL;
@@ -4440,7 +4435,32 @@ void append_signoff(struct strbuf *msgbuf, size_t
ignore_footer, unsigned flag)
if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
- sob.buf, sob.len);
+ sob->buf, sob->len);
+}
+
+void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
+{
+ unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
+ struct strbuf sob = STRBUF_INIT;
+
+ strbuf_addstr(&sob, sign_off_header);
+ strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
+ strbuf_addch(&sob, '\n');
+
+ append_footer(msgbuf, &sob, ignore_footer, no_dup_sob);
+
+ strbuf_release(&sob);
+}
+
+void append_coauthor(struct strbuf *msgbuf, const char *coauthor)
+{
+ struct strbuf sob = STRBUF_INIT;
+
+ strbuf_addstr(&sob, coauthor_header);
+ strbuf_addstr(&sob, coauthor);
+ strbuf_addch(&sob, '\n');
+
+ append_footer(msgbuf, &sob, 0, 1);
As we have a constant for APPEND_SIGNOFF_DEDUP can we use it here please
rather than '1' which does not covey the same meaning to the author.
Also as I said above I think you want to pass in a real value for
ignore_footer not zero
strbuf_release(&sob);
}
diff --git a/sequencer.h b/sequencer.h
index 574260f621..e36489fce7 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -170,6 +170,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);
*/
void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned
flag);
+void append_coauthor(struct strbuf *msgbuf, const char* co_author);
+
void append_conflicts_hint(struct index_state *istate,
struct strbuf *msgbuf, enum commit_msg_cleanup_mode
cleanup_mode);
enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 14c92e4c25..5ed6735cf4 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -138,6 +138,17 @@ test_expect_success 'partial removal' '
'
+test_expect_success 'co-author' '
+
+ >coauthor &&
+ git add coauthor &&
+ git commit -m "thank you" --co-author="Author " &&
+ git cat-file commit HEAD >commit.msg &&
+ sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
+ echo "Author " >expected &&
+ test_cmp expected actual
+'
This is fine as far as it goes but it would be nice to test the
de-duplication behavior once that's defined
Best Wishes
Phillip
test_expect_success 'sign off' '
>positive &&
--
2.22.0.rc3
Re: What's cooking in git.git (Oct 2019, #01; Thu, 3)
Hi Elijah
On 05/10/2019 01:40, Elijah Newren wrote:
On Fri, Oct 4, 2019 at 4:49 AM Phillip Wood wrote:
Hi Junio
On 03/10/2019 06:04, Junio C Hamano wrote:
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'. The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.
[...]
* pw/rebase-i-show-HEAD-to-reword (2019-08-19) 3 commits
- sequencer: simplify root commit creation
- rebase -i: check for updated todo after squash and reword
- rebase -i: always update HEAD before rewording
(this branch is used by ra/rebase-i-more-options.)
"git rebase -i" showed a wrong HEAD while "reword" open the editor.
Will merge to 'next'.
That's great, thanks
* ra/rebase-i-more-options (2019-09-09) 6 commits
- rebase: add --reset-author-date
- rebase -i: support --ignore-date
- sequencer: rename amend_author to author_to_rename
- rebase -i: support --committer-date-is-author-date
- sequencer: allow callers of read_author_script() to ignore fields
- rebase -i: add --ignore-whitespace flag
(this branch uses pw/rebase-i-show-HEAD-to-reword.)
"git rebase -i" learned a few options that are known by "git
rebase" proper.
Is this ready for 'next'.
Nearly, but not quite I think cf [1]. Also I'm still not convinced that
having different behaviors for --ignore-whitespace depending on the
backend is going to be helpful but maybe they are close enough not to
matter too much in practice [2].
Sorry I should have chimed in sooner; I can speak to the second point.
I would say that in practice it doesn't matter a lot; in most cases
the two overlap. Both am's --ignore-whitespace and merge's
-Xignore-space-change are buggy (in different ways) and should be
fixed, but I'd consider them both to be buggy in edge cases. I
recommended earlier this summer that Rohit submit the patches without
first attempting to fix apply or xdiff, and kept in my TODO list
that'd I'd go in and fix xdiff later if Rohit didn't have extra time
for it. I did a little digging back then to find out the differences
and suggested some text to use to explain them and to argue that they
shouldn't block this feature:
"""
am's --ignore-space-change (an alias for am's --ignore-whitespace; see
git-apply's description of those two flags) not only share the same
name with diff's --ignore-space-change and merge's
-Xignore-space-change, but the similarity in naming appears to have
been intentional with am's --ignore-space-change and merge's
-Xignore-space-change being designed to have the same functionality
(see e.g. the commit messages for f008cef4abb2 ("Merge branch
'jc/apply-ignore-whitespace'", 2014-06-03) and 4e5dd044c62f
("merge-recursive: options to ignore whitespace changes",
2010-08-26)). For the most part, these options do provide the same
behavior. However, there are some edge cases where both apply's
--ignore-space-change and merge's -Xignore-space-change fall short of
optimal behavior, and in different ways. In particular,
--ignore-space-change for apply will handle whitespace changes in the
context region but not in the region the other side modified, and
-Xignore-space-change will delete whitespace changes even when the
other side had no changes (thus treating both sides as unmodified).
Fixing these differences in edge cases is left for future work; this
patch simply wires interactive rebase to also understand
--ignore-whitespace by translating it to -Xignore-space-change.
"""
I've got another email with even more detail if folks need it.
Thanks for clarifying that, it sounds like it shouldn't be a problem in
practice then (especially if you're planning some improvements)
Best Wishes
Phillip
Re: What's cooking in git.git (Oct 2019, #01; Thu, 3)
Hi Junio On 03/10/2019 06:04, Junio C Hamano wrote: Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. [...] * pw/rebase-i-show-HEAD-to-reword (2019-08-19) 3 commits - sequencer: simplify root commit creation - rebase -i: check for updated todo after squash and reword - rebase -i: always update HEAD before rewording (this branch is used by ra/rebase-i-more-options.) "git rebase -i" showed a wrong HEAD while "reword" open the editor. Will merge to 'next'. That's great, thanks * ra/rebase-i-more-options (2019-09-09) 6 commits - rebase: add --reset-author-date - rebase -i: support --ignore-date - sequencer: rename amend_author to author_to_rename - rebase -i: support --committer-date-is-author-date - sequencer: allow callers of read_author_script() to ignore fields - rebase -i: add --ignore-whitespace flag (this branch uses pw/rebase-i-show-HEAD-to-reword.) "git rebase -i" learned a few options that are known by "git rebase" proper. Is this ready for 'next'. Nearly, but not quite I think cf [1]. Also I'm still not convinced that having different behaviors for --ignore-whitespace depending on the backend is going to be helpful but maybe they are close enough not to matter too much in practice [2]. [1] https://public-inbox.org/git/20190806173638.17510-1-rohit.ashiwal...@gmail.com/T/#m965ce1f09d1d1b8010c04db0eabd4b19ce99fe82 [2] https://public-inbox.org/git/20190806173638.17510-1-rohit.ashiwal...@gmail.com/T/#m94e059c18b7bbcc721d12b190bd4aaecc5e8d591
Re: [PATCH v4 3/6] rebase -i: support --committer-date-is-author-date
thor_date)
+ options.flags |= REBASE_FORCE;
+
for (i = 0; i < options.git_am_opts.argc; i++) {
const char *option = options.git_am_opts.argv[i], *p;
- if (!strcmp(option, "--committer-date-is-author-date") ||
- !strcmp(option, "--ignore-date") ||
+ if (!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
diff --git a/sequencer.c b/sequencer.c
index adeff2561e..fd925f2d72 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,6 +147,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
* command-line.
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
@@ -879,6 +880,17 @@ static char *get_author(const char *message)
return NULL;
}
+/* Returns a "date" string that needs to be free()'d by the caller */
+static char *read_author_date_or_null(void)
+{
+ char *date;
+
+ if (read_author_script(rebase_path_author_script(),
+ NULL, NULL, &date, 0))
+ return NULL;
+ return date;
+}
+
static const char staged_changes_advice[] =
N_("you have staged changes in your working tree\n"
"If these changes are meant to be squashed into the previous commit, run:\n"
@@ -938,6 +950,24 @@ static int run_git_commit(struct repository *r,
cmd.git_cmd = 1;
+if (opts->committer_date_is_author_date) {
+ int res = -1;
+ struct strbuf datebuf = STRBUF_INIT;
+ char *date = read_author_date_or_null();
+
+ if (!date)
+ return -1;
+
+ strbuf_addf(&datebuf, "@%s", date);
+ res = setenv("GIT_COMMITTER_DATE", datebuf.buf, 1);
+
+ strbuf_release(&datebuf);
+ free(date);
+
+ if (res)
+ return -1;
+}
+
if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
@@ -1331,7 +1361,6 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
-
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
const char *out_enc = get_commit_output_encoding();
@@ -1359,6 +1388,26 @@ static int try_to_commit(struct repository *r,
commit_list_insert(current_head, &parents);
}
+ if (opts->committer_date_is_author_date) {
+ int len = strlen(author);
+ struct ident_split ident;
+ struct strbuf date = STRBUF_INIT;
+
+ if (split_ident_line(&ident, author, len) < 0)
+ return error(_("malformed ident line"));
Here (and just below) we return if there is an error but later on we
`goto out` should we be doing that here to clean something up?
Best Wishes
Phillip
+ if (!ident.date_begin)
+ return error(_("corrupted author without date
information"));
+
+ strbuf_addf(&date, "@%.*s %.*s",
+ (int)(ident.date_end - ident.date_begin),
ident.date_begin,
+ (int)(ident.tz_end - ident.tz_begin),
ident.tz_begin);
+ res = setenv("GIT_COMMITTER_DATE", date.buf, 1);
+ strbuf_release(&date);
+
+ if (res)
+ goto out;
+ }
+
if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
res = error(_("git write-tree failed to write a tree"));
goto out;
@@ -2473,6 +2522,11 @@ static int read_populate_opts(struct replay_opts *opts)
opts->signoff = 1;
}
+ if (file_exists(rebase_path_cdate_is_adate())) {
+ opts->allow_ff = 0;
+ opts->committer_date_is_author_date = 1;
+ }
+
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;
@@ -2555,6 +2609,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
write_file
Re: [PATCH v4 1/6] rebase -i: add --ignore-whitespace flag
t")),
- OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
- NULL, N_("passed to 'git am'"),
- PARSE_OPT_NOARG),
OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
&options.git_am_opts, NULL,
N_("passed to 'git am'"), PARSE_OPT_NOARG),
@@ -1417,6 +1426,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
N_("passed to 'git am'"), PARSE_OPT_NOARG),
OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
N_("passed to 'git apply'"), 0),
+ OPT_BOOL(0, "ignore-whitespace", &options.ignore_whitespace,
+N_("ignore changes in whitespace")),
OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
N_("action"), N_("passed to 'git apply'"), 0),
OPT_BIT('f', "force-rebase", &options.flags,
@@ -1834,6 +1845,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
if (options.rebase_merges) {
+ if (options.ignore_whitespace)
+ die(_("cannot combine '--rebase-merges' with "
+ "'--ignore-whitespace'"));
--rebase-merges now supports --strategy_option so we should fix this.
if (strategy_options.nr)
die(_("cannot combine '--rebase-merges' with "
"'--strategy-option'"));
diff --git a/t/t3422-rebase-incompatible-options.sh
b/t/t3422-rebase-incompatible-options.sh
index a5868ea152..4342f79eea 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -61,7 +61,6 @@ test_rebase_am_only () {
}
test_rebase_am_only --whitespace=fix
-test_rebase_am_only --ignore-whitespace
test_rebase_am_only --committer-date-is-author-date
test_rebase_am_only -C4
diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
new file mode 100755
index 00..2e16e00a9d
--- /dev/null
+++ b/t/t3433-rebase-options-compatibility.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+#
+# Copyright (c) 2019 Rohit Ashiwal
+#
+
+test_description='tests to ensure compatibility between am and interactive
backends'
+
+. ./test-lib.sh
+
+# This is a special case in which both am and interactive backends
+# provide the same output.
I'm still concerned that we have to construct a special test case to get
both backends to generate the same output. If this series is about
supporting the behavior of the am backend in the sequencer then I'm
worried that having the same option name have two different behaviors
will create more problems than it solves. Do you have a feeling for how
often the two backends give different results?
Best Wishes
Phillip
It was done intentionally because
+# both the backends fall short of optimal behaviour.
+test_expect_success 'setup' '
+ git checkout -b topic &&
+ q_to_tab >file <<-\EOF &&
+ line 1
+ Qline 2
+ line 3
+ EOF
+ git add file &&
+ git commit -m "add file" &&
+ cat >file <<-\EOF &&
+ line 1
+ new line 2
+ line 3
+ EOF
+ git commit -am "update file" &&
+ git tag side &&
+
+ git checkout --orphan master &&
+ sed -e "s/^|//" >file <<-\EOF &&
+ |line 1
+ |line 2
+ |line 3
+ EOF
+ git add file &&
+ git commit -m "add file" &&
+ git tag main
+'
+
+test_expect_success '--ignore-whitespace works with am backend' '
+ cat >expect <<-\EOF &&
+ line 1
+ new line 2
+ line 3
+ EOF
+ test_must_fail git rebase main side &&
+ git rebase --abort &&
+ git rebase --ignore-whitespace main side &&
+ test_cmp expect file
+'
+
+test_expect_success '--ignore-whitespace works with interactive backend' '
+ cat >expect <<-\EOF &&
+ line 1
+ new line 2
+ line 3
+ EOF
+ test_must_fail git rebase --merge main side &&
+ git rebase --abort &&
+ git rebase --merge --ignore-whitespace main side &&
+ test_cmp expect file
+'
+
+test_done
Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
Hi Dscho
On 02/10/2019 09:16, Johannes Schindelin wrote:
Hi,
On Fri, 27 Sep 2019, Phillip Wood wrote:
Hi Alban
On 25/09/2019 21:13, Alban Gruin wrote:
>>> [...]
builtin/rebase.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct
rebase_options *opts)
if (opts->strategy_opts)
parse_strategy_opts(&replay, opts->strategy_opts);
+if (opts->squash_onto) {
I guess it does not matter much, but shouldn't this be guarded against
the case where `replay.squash_onto` was already initialized? Like, `if
(opts->squash_onto && !replay.have_squash_onto)`?
replay is uninitialized as this function takes a `struct rebase_opitons`
and returns a new `struct replay_options` based on that.
Best Wishes
Phillip
Other than that, this patch makes sense to me.
Ciao,
Dscho
+ oidcpy(&replay.squash_onto, opts->squash_onto);
+ replay.have_squash_onto = 1;
+ }
+
return replay;
}
Re: [PATCH v1 0/5] Use complete_action’s todo list to do the rebase
Hi Alban On 25/09/2019 21:13, Alban Gruin wrote: This can be seen as a continuation of ag/reduce-rewriting-todo. Currently, complete_action() releases its todo list before calling sequencer_continue(), which reloads the todo list from the disk. This series removes this useless round trip. Patches 1, 2, and 3 originally come from a series meaning to improve rebase.missingCommitsCheck[0]. In the original series, I wanted to check for missing commits in read_populate_todo(), so a warning could be issued after a `rebase --continue' or an `exec' commands. But, in the case of the initial edit, it is already checked in complete_action(), and would be checked a second time in sequencer_continue() (a caller of read_populate_todo()). So I hacked up sequencer_continue() to accept a pointer to a todo list, and if not null, would skip the call to read_populate_todo(). (This was really ugly, tbh.) Some issues arose with git-prompt.sh[1], hence 1, 2 and 3. Patch 5 is a new approach to what I did first. Instead of bolting a new parameter to sequencer_continue(), this makes complete_action() calling directly pick_commits(). Thanks for working on this, these patches all look fine to me modulo the confusing wording in the commit message on patch 4 Best Wishes Phillip This is based on master (4c86140027, "Third batch"). The tip of this series is tagged as "reduce-todo-list-cont-v1" at https://github.com/agrn/git. [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gr...@gmail.com/ [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/ Alban Gruin (5): sequencer: update `total_nr' when adding an item to a todo list sequencer: update `done_nr' when skipping commands in a todo list sequencer: move the code writing total_nr on the disk to a new function rebase: fill `squash_onto' in get_replay_opts() sequencer: directly call pick_commits() from complete_action() builtin/rebase.c | 5 + sequencer.c | 26 ++ 2 files changed, 23 insertions(+), 8 deletions(-)
Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
Hi Alban
On 25/09/2019 21:13, Alban Gruin wrote:
get_replay_opts() did not fill `squash_onto' if possible, meaning that
I'm not sure what you mean by 'if possible' here, I think the sentance
makes sense without that.
this field should be read from the disk by the sequencer through
read_populate_opts(). Without this, calling `pick_commits()' directly
will result in incorrect results with `rebase --root'.
Let’s change that.
Good catch
Best Wishes
Phillip
Signed-off-by: Alban Gruin
---
builtin/rebase.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct
rebase_options *opts)
if (opts->strategy_opts)
parse_strategy_opts(&replay, opts->strategy_opts);
+ if (opts->squash_onto) {
+ oidcpy(&replay.squash_onto, opts->squash_onto);
+ replay.have_squash_onto = 1;
+ }
+
return replay;
}
Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
Hi Alban
Thanks for removing some more unnecessary work reloading the the todo list.
On 25/09/2019 21:13, Alban Gruin wrote:
Currently, complete_action() calls sequencer_continue() to do the
rebase. Even though the former already has the todo list, the latter
loads it from the disk and parses it. Calling directly pick_commits()
from complete_action() avoids this unnecessary round trip.
Signed-off-by: Alban Gruin
---
sequencer.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ec7ea8d9e5..b395dd6e11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct
replay_opts *opts, unsigned fla
return error_errno(_("could not write '%s'"), todo_file);
}
- todo_list_release(&new_todo);
-
if (checkout_onto(r, opts, onto_name, &oid, orig_head))
return -1;
if (require_clean_work_tree(r, "rebase", "", 1, 1))
return -1;
- return sequencer_continue(r, opts);
sequencer_continue does a number of things before calling pick_commits(). It
- calls read_and_refresh_cache() - this is unnecessary here as we've
just called require_clean_work_tree()
- calls read_populate_opts() - this is unnecessary as we're staring a
new rebase so opts is fully populated
- loads the todo list - this is unnecessary as we've just populated
the todo list
- commits any staged changes - this is unnecessary as we're staring a
new rebase so there are no staged changes
- calls record_in_rewritten() - this is unnecessary as we're starting
a new rebase
So I agree that this patch is correct.
Thanks
Phillip
+ todo_list_write_total_nr(&new_todo);
+ res = pick_commits(r, &new_todo, opts);
+ todo_list_release(&new_todo);
+
+ return res;
}
struct subject2item_entry {
Re: [PATCH v4 5/6] rebase -i: support --ignore-date
-signoff\n");
if (opts->committer_date_is_author_date)
write_file(rebase_path_cdate_is_adate(), "%s", "");
+ if (opts->ignore_date)
+ write_file(rebase_path_ignore_date(), "%s", "");
if (opts->reschedule_failed_exec)
write_file(rebase_path_reschedule_failed_exec(), "%s", "");
@@ -3536,6 +3584,8 @@ static int do_merge(struct repository *r,
argv_array_push(&cmd.args, git_path_merge_msg(r));
if (opts->gpg_sign)
argv_array_push(&cmd.args, opts->gpg_sign);
+ if (opts->ignore_date)
+ push_dates(&cmd);
/* Add the tips to be merged */
for (j = to_merge; j; j = j->next)
@@ -3809,7 +3859,8 @@ static int pick_commits(struct repository *r,
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit ||
- opts->committer_date_is_author_date));
+ opts->committer_date_is_author_date ||
+ opts->ignore_date));
if (read_and_refresh_cache(r, opts))
return -1;
diff --git a/sequencer.h b/sequencer.h
index e3881e9275..bf5a79afdb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,7 @@ struct replay_opts {
int quiet;
int reschedule_failed_exec;
int committer_date_is_author_date;
+ int ignore_date;
int mainline;
diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
index 6c1fbab4d8..0ad7bfc6a0 100755
--- a/t/t3433-rebase-options-compatibility.sh
+++ b/t/t3433-rebase-options-compatibility.sh
@@ -99,4 +99,39 @@ test_expect_success '--committer-date-is-author-date works
with rebase -r' '
done
+# Checking for + in author time is enough since default
+# timezone is UTC, but the timezone used while committing
+# sets to +0530.
+test_expect_success '--ignore-date works with am backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ git show HEAD --pretty="format:%ci" >committertime &&
+ grep "+" authortime &&
+ grep "+" committertime
+'
+
+test_expect_success '--ignore-date works with interactive backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date -i HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ git show HEAD --pretty="format:%ci" >committertime &&
+ grep "+" authortime &&
+ grep "+" committertime
+'
+
+test_expect_success '--ignore-date works with rebase -r' '
+ git checkout side &&
+ git merge commit3 &&
+ git rebase -r --root --ignore-date &&
+ git rev-list HEAD >rev_list &&
+ while read HASH
+ do
+ git show $HASH --pretty="format:%ai" >authortime
+ git show $HASH --pretty="format:%ci" >committertime
+ grep "+" authortime
+ grep "+" committertime
+ done
It's good to see the new test. Did you see Stolee's email [4] about the
test coverage of the previous version of this series? You should check
that this series tests all the untested non-error handling lines.
[4]
https://public-inbox.org/git/1ed86989-9ba2-0cd7-b6f7-654d1943b...@gmail.com/
Best Wishes
Phillip
test_done
Re: [PATCH v2 00/19] hashmap bug/safety/ease-of-use fixes
Hi Eric
On 24/09/2019 02:03, Eric Wong wrote:
Patches 1-11 are largely unchanged from the original series with the
exception of 2, which is new and posted at:
https://public-inbox.org/git/20190908074953.kux7zz4y7iolqko4@whir/
12-17 take further steps to get us away from hashmap_entry being
the first element, but they're also a bit ugly because __typeof__
isn't portable
18-19 finally brings me to the APIs I want to expose without
relying on __typeof :)
Looking at the overall diff for this series looks a lot nicer with the
extra patches that eliminate most of the explicit calls to
container_of(). Thanks for the improved api and the cocci-check patch as
well.
I've only had time for a quick look through but the patches seem well
ordered and easy to follow. I think there are some line folding issues
where you have wrapped a line when you added the type parameter and then
removed it in a later path without re-flowing the line. Apart from that
the only thing I noticed is that hashmap.h still starts with
* struct long2string {
* struct hashmap_entry ent; // must be the first member!
Is that still the case now that hashmap_{get,put,remove}_entry() use
container_of() and hashmap_init() takes a struct hashmap_entry? That
comment is in a lot of our structure definitions as well.
Best Wishes
Phillip
Apologies for the delays, been busy with other stuff...
Previous discussion starts at:
https://public-inbox.org/git/20190826024332.3403-...@80x24.org/
The following changes since commit 745f6812895b31c02b29bdfe4ae8e5498f776c26:
First batch after Git 2.23 (2019-08-22 12:41:04 -0700)
are available in the Git repository at:
https://80x24.org/git-svn.git hashmap-wip-v2
for you to fetch changes up to 212a596edd99169b284912b7b70b6280e2fb90e6:
hashmap: remove type arg from hashmap_{get,put,remove}_entry (2019-09-24
00:42:22 +)
Eric Wong (19):
diff: use hashmap_entry_init on moved_entry.ent
coccicheck: detect hashmap_entry.hash assignment
packfile: use hashmap_entry in delta_base_cache_entry
hashmap_entry_init takes "struct hashmap_entry *"
hashmap_get_next takes "const struct hashmap_entry *"
hashmap_add takes "struct hashmap_entry *"
hashmap_get takes "const struct hashmap_entry *"
hashmap_remove takes "const struct hashmap_entry *"
hashmap_put takes "struct hashmap_entry *"
introduce container_of macro
hashmap_get_next returns "struct hashmap_entry *"
hashmap: use *_entry APIs to wrap container_of
hashmap_get{,_from_hash} return "struct hashmap_entry *"
hashmap_cmp_fn takes hashmap_entry params
hashmap: use *_entry APIs for iteration
hashmap: hashmap_{put,remove} return hashmap_entry *
hashmap: introduce hashmap_free_entries
OFFSETOF_VAR macro to simplify hashmap iterators
hashmap: remove type arg from hashmap_{get,put,remove}_entry
attr.c | 22 ++---
blame.c | 25 +++---
builtin/describe.c | 21 +++--
builtin/difftool.c | 56 ++--
builtin/fast-export.c | 15 ++--
builtin/fetch.c | 30 ---
config.c| 24 +++---
contrib/coccinelle/hashmap.cocci| 16
diff.c | 31 ---
diffcore-rename.c | 15 ++--
git-compat-util.h | 33
hashmap.c | 58 -
hashmap.h | 164 +---
merge-recursive.c | 87 ++-
name-hash.c | 57 +++--
oidmap.c| 20 +++--
oidmap.h| 6 +-
packfile.c | 22 +++--
patch-ids.c | 18 ++--
range-diff.c| 10 +--
ref-filter.c| 31 ---
refs.c | 23 +++--
remote.c| 21 +++--
revision.c | 28 +++---
sequencer.c | 44 ++
sub-process.c | 20 +++--
sub-process.h | 4 +-
submodule-config.c | 52 +++-
t/helper/test-hashmap.c | 49 ++-
t/helper/test-lazy-init-name-hash.c | 12 +--
30 files changed, 647 insertions(+), 367 deletions(-)
create mode 100644 contrib/coccinelle/hashmap.cocci
Eric Wong (19):
diff: use hashmap_entry_init on moved_entry.ent
coccicheck: detect hashmap_entry.hash assignment
packfile: use hashmap_entry in delta_base_cache_ent
Re: [PATCH] add a Code of Conduct document
Hi Gábor On 24/09/2019 10:01, SZEDER Gábor wrote: On Tue, Sep 24, 2019 at 02:44:54AM -0400, Jeff King wrote: [...] After some poking around at various CoC options, this one seemed like the best fit to me. But I'm open to suggestions or more discussion. It seems to me that the important piece is having _some_ CoC, and picking something standard-ish seems a safe bet. We are decent people, and know how to behave properly and treat each other with respect. It is my fundamental assumption that all future contributors are decent and respectful human beings as well. History suggests that almost all will be decent but there will be one or two who aren't and cause trouble. A CoC like this, which is "explicit about the behavior we want to model" (quoting the original discussion starter) inherently insinuates that we aren't decent, and can't behave without being told how to do so. Frankly, I find this borderline insulting to me, to my parents, to all fellow contributors, and to future contributors as well. I don't find it insulting, to me it just sets out that the project welcomes contributors who behave decently. There are locations, nationalities and cultures, where the avarage wide-spread CoCs, like Contributor Covenant and its derivatives, are perceived as (paraphrasing) too "American", politically overcorrect, corporate BS, etc., which are forced upon open-source projects. Consequently, such CoCs are often found rather discouraging, and announcements about their adoption in open-source projects generally get negative reaction. Less is more. Much-much more. A concise CoC that treats its readers as responsible, well-behaved human beings is met with much approval. I was pleasantly surprised at how short the proposed CoC is, I don't think it's too long at all. It only takes a couple of minutes to read and is quite clear. Best Wishes Phillip Take, for example, the TrueOS Rules of Conduct, which in just a few short sentences covers everything that's worth covering: https://www.trueos.org/rulesofconduct/ If diversity and inclusion of other cultures is indeed a priority, then we should carefully consider that some potential contributors will rather choose not to contribute because of a CoC like this. If people are on board with this direction, it might be fun to pick up a bunch of "Acked-by" trailers from people in the community who agree with it. It might give it more weight if many members have publicly endorsed it. Because of the above I'm leaning towards NACK.
Re: [PATCH] add a Code of Conduct document
Hi Peff On 24/09/2019 07:44, Jeff King wrote: We've never had a formally written Code of Conduct document. Though it has been discussed off and on over the years, for the most part the behavior on the mailing list has been good enough that nobody felt the need to push one forward. However, even if there aren't specific problems now, it's a good idea to have a document: - it puts everybody on the same page with respect to expectations. This might avoid poor behavior, but also makes it easier to handle it if it does happen. - it publicly advertises that good conduct is important to us and will be enforced, which may make some people more comfortable with joining our community - it may be a good time to cement our expectations when things are quiet, since it gives everybody some distance rather than focusing on a current contentious issue I think these are all good points, it is definitely better to discuss this when there isn't a pressing problem to resolve. This patch adapts the Contributor Covenant Code of Conduct. As opposed to writing our own from scratch, this uses common and well-accepted language, and strikes a good balance between illustrating expectations and avoiding a laundry list of behaviors. It's also the same document used by the Git for Windows project. The text is taken mostly verbatim from: https://www.contributor-covenant.org/version/1/4/code-of-conduct.html I also stole a very nice introductory paragraph from the Git for Windows version of the file. Using an existing text makes sense, even more so if it is being successfully used by git for windows There are a few subtle points, though: - the document refers to "the project maintainers". For the code, we generally only consider there to be one maintainer: Junio C Hamano. But for dealing with community issues, it makes sense to involve more people to spread the responsibility. I've listed the project committee address of g...@sfconservancy.org as the contact point. - the document mentions banning from the community, both in the intro paragraph and in "Our Responsibilities". The exact mechanism here is left vague. I can imagine it might start with social enforcement (not accepting patches, ignoring emails) and could escalate to technical measures if necessary (asking vger admins to block an address). It probably make sense _not_ to get too specific at this point, and deal with specifics as they come up. I think this is a sensible approach - it needs to be clear that there is a mechanism to deal with violations otherwise there's no point to having a CoC but we don't want to get bogged down by a whole sequence of what if someone does this that or the other. I think the text below does a good job of setting out expectations without being too long Best Wishes Phillip Signed-off-by: Jeff King --- Obviously related to the discussion in: https://public-inbox.org/git/71fba9e7-6314-6ef9-9959-6ae06843d...@gmail.com/ After some poking around at various CoC options, this one seemed like the best fit to me. But I'm open to suggestions or more discussion. It seems to me that the important piece is having _some_ CoC, and picking something standard-ish seems a safe bet. I did find this nice set of guidelines in an old discussion: https://github.com/mhagger/git/commit/c6e6196be8fab3d48b12c4e42eceae6937538dee I think it's missing some things that are "standard" in more modern CoCs (in particular, there's not much discussion of enforcement or responsibilities, and I think those are important for the "making people comfortable" goal). But maybe there are bits we'd like to pick out for other documents; not so much "_what_ we expect" as "here are some tips on _how_". If people are on board with this direction, it might be fun to pick up a bunch of "Acked-by" trailers from people in the community who agree with it. It might give it more weight if many members have publicly endorsed it. I've cc'd g...@sfconservancy.org here, because I think it's important for all of the project committee members to endorse it (and because the document puts us on the hook for enforcing it!). CODE_OF_CONDUCT.md | 85 ++ 1 file changed, 85 insertions(+) create mode 100644 CODE_OF_CONDUCT.md diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md new file mode 100644 index 00..b94f72b0b8 --- /dev/null +++ b/CODE_OF_CONDUCT.md @@ -0,0 +1,85 @@ +# Git Code of Conduct + +This code of conduct outlines our expectations for participants within +the Git community, as well as steps for reporting unacceptable behavior. +We are committed to providing a welcoming and inspiring community for +all and expect our code of conduct to be honored. A
Re: [PATCH 0/1] git-config --add allows values from stdin
Hi Taylor and ZJ On 22/09/2019 04:11, Taylor Blau wrote: Hi ZJ, On Tue, Sep 17, 2019 at 03:31:34PM +0200, Zeger-Jan van de Weg wrote: When adding or updating configuration values using git-config, the values could all be observed by different processes as these are passed as arguments. In some environments all commands executed are also all logged. When the value contains secrets, this is a side effect that would be great to avoid. How much extra security does this actually add? - do the processes that can observe the command line arguments also have read access to the git config file? At GitLab we use Rugged/libgit2 to circumvent this property[1]. The following patch allows a value to be set through stdin when the user passes a `--stdin` flag. Interesting. I had thought some time ago about making an interactive line-oriented 'mode' for using 'git-config(1)', which would allow callers to add/delete/fetch multiple variables using only a single process. This would satisfy a more general use-case than yours: particularly my idea was grown out of wanting to specify or read many configuration entries at once when using a tool built around Git, such as Git LFS. I had not considered tying '--stdin' to the '--add' (implicit or not) mode of 'git-config(1)'. It is an interesting idea to be sure. On the one hand, it lends itself to other modes, such as '--get' combined with '--stdin', or '--unset' in the same fashion. One could imagine that each of these would take either a key/value-pair (in the case of '--add') or a set of key(s) (in the remaining cases). The most desirable aspect is that this would allow for a clear path to this series being picked up. It would be great to be able to --get multiple values and I can see people wanting to be able to --unset them as well. On the other hand, tying '--stdin' to a particular mode of using 'git conifg' seems overly restrictive to me. If I am building a tool that wants to fetch some values in the configuration, and then add/unset others based on the results using only a single process, I don't think that a mode-based '--stdin' flag gets the job done. That's true but I don't know how common it is compared to a script wanting to read a bunch of config variables at startup (i.e. does it warrant the extra complexity) Best Wishes Phillip One happy medium that comes to mind is a new '--interactive' mode, which implies '--stdin' and would allow the above use-case, e.g.: $ git config --interactive <<\EOF get core.myval set core.foo bar unset core.baz EOF (An off-topic note is that it would be interesting to allow more fanciful options than 'get', e.g., 'get' with a '--type' specifier, or some such). I'm not sure if anyone actually wants to use 'git-config(1)' in this way, but I figured that I would at least share some things that I was thinking about when initially considering this proposal. [1]: https://gitlab.com/gitlab-org/gitaly/blob/8ab5bd595984678838f3f09a96798b149e68a939/ruby/lib/gitlab/git/http_auth.rb#L14-15 Zeger-Jan van de Weg (1): Git config allows value setting from stdin Documentation/git-config.txt | 5 - builtin/config.c | 23 +-- t/t1300-config.sh| 11 +++ 3 files changed, 36 insertions(+), 3 deletions(-) -- 2.23.0 Thanks, Taylor
Re: [PATCH v2] rebase: introduce --update-branches option
Hi
On 09/09/2019 15:13, Johannes Schindelin wrote:
Hi,
On Mon, 9 Sep 2019, Phillip Wood wrote:
On 08/09/2019 00:44, Warren He wrote:
Everyone in this thread, thanks for your support and encouragement.
[...]
It should not really imply `--interactive`, but `--rebase-merges`.
`imply_interactive` doesn't fully switch on `--interactive`, i.e., causing
the
editor to open. It only selects the backend, which I think we're saying is
the
right thing. I've dropped the `-i` from the test description.
And we don't really have to imply --rebase-merges, in case someone would
prefer
to linearize things, which who knows? Running that non-rebase-merges command
in
the example scenario from my original post should give something like this:
I think it would probably be less confusing to default to preserving merges
s/preserving/rebasing/?
and having an option to turn that off - people are going to be surprised if
their history is linearized.
I don't think it makes any sense to linearize the history while updating
branches, as the commits will be all jumbled up. Imagine this history:
- A - B - C - D -
\ /
E - F
Typically, it does not elicit any "bug" reports, but this can easily be
linearized to
- A' - B' - E' - C' - F' -
In my mind, it makes no sense to update any local branches that pointed
to C and F to point to C' and F', respectively.
I agree
[...]
* * *
And then there's the discussion about using `exec git branch -f`. To
summarize
the issues collected from the entire thread:
1. the changes aren't atomically applied at the end of the rebase
2. it fails when the branch is checked out in a worktree
3. it clobbers the branch if anything else updates it during the rebase
4. the way we prepare the unprefixed branch doesn't work right some exotic
cases
5. the reflog message it leaves is uninformative
For #4, I think we've lucked out actually. The `load_ref_decorations`
routine we
use determines that a ref is `DECORATION_REF_LOCAL` under the condition
`starts_with(refname, "refs/heads/")` (log-tree.c:114, add_ref_decoration),
so
`prettify_refname` will find the prefix and skip it. But that's an invariant
maintained by two pieces of code pretty far away from each other.
For #5, for the convenience of readers, the reflog entry it leaves looks
like this:
```
00873f2 feat-e@{0}: branch: Reset to HEAD
```
Not great.
I haven't made any changes to this yet, but I've thought about what I want.
My
favorite so far is to add a new todo command that just does everything
right. It
would make a temparary ref `refs/rewritten-heads/xxx` (or something), and
update
`refs/heads/xxx` at the end.
I think that's the best way to do it. If we had a command like 'branch
' that creates a ref to remember the current HEAD and saves the
current branch head. Then at the end rebase can update the branches to point
to the saved commits if the branch is unchanged. If the rebase is aborted then
we don't end up with some branches updated and others not.
I'd avoid cluttering the space with more commands. For `branch`, for
example, the natural short command would be `b`, but that already means
`break`.
We could just not have a short name, after all --update-branch is hardly
a short alternative
In contrast, I would think that
label --update-branch my-side-track
would make for a nicer read than
label my-side-track
branch my-side-track
I agree it would be nice to do both on a single line, my argument was
mainly against using 'exec branch ...' so that we can defer the branch
updates until the very end of the rebase. The branch command could set a
label as well or we could add an option to label I'm not that bothered
either way at the moment. Another possibility which we probably don't
want is to have labels starting refs/ imply --update-branch
Of course, it would be a lot harder to bring back the safety of `git
update-ref`'s `` safe-guard, in either forms.
Is it that difficult to write the current branch HEAD to a file when we
label it and then read those back at the end when we update the refs? or
are you thinking of calling 'git branch' instead of
ref_transaction_update()? I'm not sure what the advantage of 'git
branch' is though.
Best Wishes
Phillip
And of course, the first form would _require_ the logic to be moved to
`make_script_with_merges()` because we could not otherwise guarantee
that the labels corresponding to local branch names aren't already in
use, for different commits.
Side Note
I'd avoid creating another worktree local ref refs/rewritten-heads/.
Either store them under refs/rewritten/ or refs/worktree/
Yep.
Ciao,
Dscho
Re: [PATCH] name-rev: avoid cutoff timestamp underflow
On 23/09/2019 09:37, SZEDER Gábor wrote:
On Sun, Sep 22, 2019 at 11:01:26PM +0200, Johannes Sixt wrote:
Am 22.09.19 um 21:53 schrieb SZEDER Gábor:
On Sun, Sep 22, 2019 at 07:57:36PM +0100, Phillip Wood wrote:
On 22/09/2019 19:01, SZEDER Gábor wrote:
+/*
+ * One day. See the 'name a rev close to epoch' test in t6120 when
+ * changing this value
+ */
+#define CUTOFF_DATE_SLOP 86400
typedef struct rev_name {
const char *tip_name;
@@ -481,8 +485,13 @@ int cmd_name_rev(int argc, const char **argv, const char
*prefix)
add_object_array(object, *argv, &revs);
}
- if (cutoff)
- cutoff = cutoff - CUTOFF_DATE_SLOP;
+ if (cutoff) {
+ /* check for undeflow */
+ if (cutoff - CUTOFF_DATE_SLOP < cutoff)
Nice catch but wouldn't this be clearer as
if (cutoff > CUTOFF_DATE_SLOP) ?
It would only be clearer now, with an unsigned 'timestamp_t'. I
tried to future-proof for a signed 'timestamp_t' and a cutoff date
before the UNIX epoch.
Huh? For signed cutoff and positive CUTOFF_DATE_SLOP,
cutoff - CUTOFF_DATE_SLOP < cutoff is ALWAYS true. Signed interger
underflow is undefined behavior and signed integer arithmetic does not
wrap around!
IOW, the new condition makes only sense today, because cutoff is an
unsigned type, but breaks down should we switch to a signed type.
Yeah, that's what I meant with worrying about signed underflow in the
commit message. As long as the cutoff is at least a day later than
the minimum value of our future signed 'timestamp_t', the condition
does the right thing. And considering that oldest time a signed 64
bit timestamp can represent far exceeds the age of the universe, and
the oldest value of even a signed 32 bit timestamp is almost half the
age of the Earth, I wasn't too worried.
It's still going to trip up static analysers though isn't it? Also it
will confuse readers who have to reason that it does not overflow in
practice as we (probably?) never use very negative values
Best Wishes
Phillip
You need this on top:
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a4d8d312ab..2d83c2b172 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -487,10 +487,10 @@ int cmd_name_rev(int argc, const char **argv, const char
*prefix)
if (cutoff) {
/* check for undeflow */
- if (cutoff - CUTOFF_DATE_SLOP < cutoff)
+ if (cutoff > TIME_MIN + CUTOFF_DATE_SLOP)
cutoff = cutoff - CUTOFF_DATE_SLOP;
else
- cutoff = 0;
+ cutoff = TIME_MIN;
}
for_each_ref(name_ref, &data);
diff --git a/git-compat-util.h b/git-compat-util.h
index c68c61d07c..1bdc21a069 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -344,6 +344,7 @@ typedef uintmax_t timestamp_t;
#define PRItime PRIuMAX
#define parse_timestamp strtoumax
#define TIME_MAX UINTMAX_MAX
+#define TIME_MIN 0
#ifndef PATH_SEP
#define PATH_SEP ':'
Re: [PATCH] name-rev: avoid cutoff timestamp underflow
Hi Gábor
On 22/09/2019 19:01, SZEDER Gábor wrote:
When 'git name-rev' is invoked with commit-ish parameters, it tries to
save some work, and doesn't visit commits older than the committer
date of the oldest given commit minus a one day worth of slop. Since
our 'timestamp_t' is an unsigned type, this leads to a timestamp
underflow when the committer date of the oldest given commit is within
a day of the UNIX epoch. As a result the cutoff timestamp ends up
far-far in the future, and 'git name-rev' doesn't visit any commits,
and names each given commit as 'undefined'.
Check whether substacting the slop from the oldest committer date
would lead to an underflow, and use a 0 as cutoff in that case. This
way it will handle commits shortly after the epoch even if we were to
switch to a signed 'timestamp_t' (but then we'll have to worry about
signed underflow for very old commits).
Note that the type of the cutoff timestamp variable used to be signed
before 5589e87fd8 (name-rev: change a "long" variable to timestamp_t,
2017-05-20). The behavior was still the same even back then, but the
underflow didn't happen when substracting the slop from the oldest
committer date, but when comparing the signed cutoff timestamp with
unsigned committer dates in name_rev(). IOW, this underflow bug is as
old as 'git name-rev' itself.
Signed-off-by: SZEDER Gábor
---
This patch adds a test at the end of 't6120-describe.sh', so it will
conflict with my non-recursive name-rev patch series, which adds a
test there as well, but the conflict should be wasy to resolve.
https://public-inbox.org/git/20190919214712.7348-7-szeder@gmail.com/
builtin/name-rev.c | 15 ---
t/t6120-describe.sh | 15 +++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c785fe16ba..a4d8d312ab 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -9,7 +9,11 @@
#include "sha1-lookup.h"
#include "commit-slab.h"
-#define CUTOFF_DATE_SLOP 86400 /* one day */
+/*
+ * One day. See the 'name a rev close to epoch' test in t6120 when
+ * changing this value
+ */
+#define CUTOFF_DATE_SLOP 86400
typedef struct rev_name {
const char *tip_name;
@@ -481,8 +485,13 @@ int cmd_name_rev(int argc, const char **argv, const char
*prefix)
add_object_array(object, *argv, &revs);
}
- if (cutoff)
- cutoff = cutoff - CUTOFF_DATE_SLOP;
+ if (cutoff) {
+ /* check for undeflow */
+ if (cutoff - CUTOFF_DATE_SLOP < cutoff)
Nice catch but wouldn't this be clearer as
if (cutoff > CUTOFF_DATE_SLOP) ?
Best Wishes
Phillip
+ cutoff = cutoff - CUTOFF_DATE_SLOP;
+ else
+ cutoff = 0;
+ }
for_each_ref(name_ref, &data);
if (transform_stdin) {
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 2b883d8174..965e633c32 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -424,4 +424,19 @@ test_expect_success 'describe complains about missing
object' '
test_must_fail git describe $ZERO_OID
'
+test_expect_success 'name-rev a rev shortly after epoch' '
+ test_when_finished "git checkout master" &&
+
+ git checkout --orphan no-timestamp-underflow &&
+ # Any date closer to epoch than the CUTOFF_DATE_SLOP constant
+ # in builtin/name-rev.c.
+ GIT_COMMITTER_DATE="@1234 +" \
+ git commit -m "committer date shortly after epoch" &&
+ near_commit_oid=$(git rev-parse HEAD) &&
+
+ echo "$near_commit_oid no-timestamp-underflow" >expect &&
+ git name-rev $near_commit_oid >actual &&
+ test_cmp expect actual
+'
+
test_done
Re: [PATCH v4 0/6] rebase -i: support more options
On 09/09/2019 19:02, Junio C Hamano wrote: Rohit Ashiwal writes: Following the suggestion of Phillip I've rebased my patch on master (745f681289) and cherry-picking b0a3186140. Sorry, but that's horrible. The latter does not even cleanly apply on the former. Yes I had assumed that the cherry pick would become the first patch of this series and be dropped from pw/rebase-i-show-HEAD-to-reword. I should have been more explicit about that. Let me see if I can find time to whip this into a reasonable shape. As pw/rebase-i-show-HEAD-to-reword is slated for next perhaps these could build on that. The first patch needs am -3 to apply to that branch but the result looks ok and the rest apply as is. Best Wishes Phillip Thanks.
Re: [PATCH v2] rebase: introduce --update-branches option
Hi Warren
On 08/09/2019 00:44, Warren He wrote:
Everyone in this thread, thanks for your support and encouragement.
[...]
It should not really imply `--interactive`, but `--rebase-merges`.
`imply_interactive` doesn't fully switch on `--interactive`, i.e., causing the
editor to open. It only selects the backend, which I think we're saying is the
right thing. I've dropped the `-i` from the test description.
And we don't really have to imply --rebase-merges, in case someone would prefer
to linearize things, which who knows? Running that non-rebase-merges command in
the example scenario from my original post should give something like this:
I think it would probably be less confusing to default to preserving
merges and having an option to turn that off - people are going to be
surprised if their history is linearized.
```
A - B (master)
\
F (feat-f)
\
E (feat-e)
\
H (my-dev)
```
So for now I haven't moved the implementation into `make_script_with_merges`.
> [...]>
[handling `fixup`/`squash` chains]
I've moved `todo_list_add_branch_updates` to run before
`todo_list_rearrange_squash`. The rearranging pulls fixups out, causing the
branch update to "fall" onto the items before, and reinserts them between a
commit and its branch update, casing them to be included in the updated branch.
which is my opinion of the right thing to do. I've added a test about this with
the following scenario:
```
A - B (master)
\
I - J - fixup! I (fixup-early)
\
K - fixup! J (fixup-late)
```
which results in the following todo list with `--autosquash`:
```
pick 9eadc32 I
fixup 265fa32 fixup! I
pick a0754fc J
fixup e7d1999 fixup! J
exec git branch -f fixup-early
pick c8bc4af K
```
That makes sense I think
I'd like to suggest [`test_cmp_rev`] instead
I've updated the test to use `test_cmp_rev`. It's not with your suggested
invocation though. We don't update the `C` tag. I've referred to the rebased `C`
with `test_cmp_rev linear-early HEAD^` and similar for the other checks.
That sounds right
* * *
And then there's the discussion about using `exec git branch -f`. To summarize
the issues collected from the entire thread:
1. the changes aren't atomically applied at the end of the rebase
2. it fails when the branch is checked out in a worktree
3. it clobbers the branch if anything else updates it during the rebase
4. the way we prepare the unprefixed branch doesn't work right some exotic cases
5. the reflog message it leaves is uninformative
For #4, I think we've lucked out actually. The `load_ref_decorations` routine we
use determines that a ref is `DECORATION_REF_LOCAL` under the condition
`starts_with(refname, "refs/heads/")` (log-tree.c:114, add_ref_decoration), so
`prettify_refname` will find the prefix and skip it. But that's an invariant
maintained by two pieces of code pretty far away from each other.
For #5, for the convenience of readers, the reflog entry it leaves looks like
this:
```
00873f2 feat-e@{0}: branch: Reset to HEAD
```
Not great.
I haven't made any changes to this yet, but I've thought about what I want. My
favorite so far is to add a new todo command that just does everything right. It
would make a temparary ref `refs/rewritten-heads/xxx` (or something), and update
`refs/heads/xxx` at the end.
I think that's the best way to do it. If we had a command like 'branch
' that creates a ref to remember the current HEAD and saves
the current branch head. Then at the end rebase can update the branches
to point to the saved commits if the branch is unchanged. If the rebase
is aborted then we don't end up with some branches updated and others not.
Side Note
I'd avoid creating another worktree local ref refs/rewritten-heads/.
Either store them under refs/rewritten/ or refs/worktree/
Best Wishes
Phillip
I agree that requiring a separate update-ref step at the end of the todo list is
unfriendly. Manually putting in some branch update commands and then realizing
that they weren't applied would be extremely frustrating. I don't see the option
of using existing tools as the easiest-to-use solution.
I'm reluctant to combine this with the existing `label` command. So far it
sounds like we generally want to be more willing to skip branch updates while
performing the rebase, with aforementioned scenarios where something else
updates the branch before we do, or if the branch becomes checked out in a
worktree. We don't want to mess up the structure of a `rebase -r` as a result of
skipping some branch updates. I think it would be conceptually simpler and
implementation-wise less tricky if we didn't combine it with the `label
Re: [PATCH] rebase: introduce --update-branches option
Hi Warren On 03/09/2019 00:41, Warren He wrote: Sometimes people have to rebase multiple related branches. One way to do that quickly, when there are branches pointing to ancestors of a later branch (which happens a lot if you try hard to pad your PR count on GitHub--I mean if you try to make small, logically separate changes), is to rebase that later branch and then reset ancestor branches to the rewritten commits. You just have to work out which branches correspond to which of the new commits. Here's an automated way to update those ancestor branches. I think this would be really useful, but as it is implemented here it only updates branch heads that are in the todo list. This means that if I have a branch structure like A - B (master) | C - E (topic-2) | D (topic-1) and I do `git rebase --update-branches master topic1` Then topic-2 will not be updated and if I do `git rebase --update-branches master topic2` then topic-1 will not be updated even though C is updated in both cases and is a common ancestor of topic-1 and topic-2. Conceptually to update all the branches descended from a rewritten commit would require using `git for-each-ref --contains $(git merge-base )` and then using `git rev-list ..` on each of those refs to get the commits to generate the todo list. Another case is applying fixup commits. In the example above if I squash a fixup for C from either branch I probably want to update both the branches descended from it. One other thing is that if the user aborts the rebase then ideally we don't want to update all the branches so it would be better to store the updated heads as we go along and then update them all at the end of the rebase. Worktrees add another complication as if one of the branches that is to be updated is checked out in another worktree then we need some way to deal with that. If there are no local changes in that worktree then we could just update the local HEAD and working copy. Best Wishes Phillip It's implemented as a function that processes a todo list, modeled after `todo_list_add_exec_commands`. Currently steps are added as `exec git branch -f `, which comes with the caveat that they're not applied atomically when it finishes rebasing. If you were to use this feature to rebase `my-dev` onto `master` in this situation: ``` A - B (master) |\ | E (feat-e) \ \ F | (feat-f) \| G (interim) \ H (my-dev) ``` you'd get a todo list like this: ``` label onto # Branch G reset onto pick 65945ab E exec git branch -f feat-e label G reset onto pick 4f0b0ad F exec git branch -f feat-f merge -C e50066c G # G exec git branch -f interim pick 539e556 H ``` Warren He (1): rebase: introduce --update-branches option Documentation/git-rebase.txt | 8 + builtin/rebase.c | 13 ++-- sequencer.c | 68 ++- sequencer.h | 6 ++-- t/t3431-rebase-update-branches.sh | 64 5 files changed, 154 insertions(+), 5 deletions(-) create mode 100755 t/t3431-rebase-update-branches.sh
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Hi Matt This is definitely worth fixing, I've got a couple of comments below On 02/09/2019 15:01, Matt R via GitGitGadget wrote: From: Matt R The `label` todo command in interactive rebases creates temporary refs in the `refs/rewritten/` namespace. These refs are stored as loose refs, i.e. as files in `.git/refs/rewritten/`, therefore they have to conform with file name limitations on the current filesystem. This poses a problem in particular on NTFS/FAT, where e.g. the colon character is not a valid part of a file name. Being picking I'll point out that ':' is not a valid in refs either. Looking at https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I think only " and | are not allowed on NTFS/FAT but are valid in refs (see the man page for git check-ref-format for all the details). So the main limitation is actually what git allows in refs. Let's safeguard against this by replacing not only white-space characters by dashes, but all non-alpha-numeric ones. However, we exempt non-ASCII UTF-8 characters from that, as it should be quite possible to reflect branch names such as `↯↯↯` in refs/file names. Signed-off-by: Matthew Rogers Signed-off-by: Johannes Schindelin --- sequencer.c | 12 +++- t/t3430-rebase-merges.sh | 5 + 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 34ebf8ed94..23f4a0876a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp, else strbuf_addbuf(&label, &oneline); + /* +* Sanitize labels by replacing non-alpha-numeric characters +* (including white-space ones) by dashes, as they might be +* illegal in file names (and hence in ref names). +* +* Note that we retain non-ASCII UTF-8 characters (identified +* via the most significant bit). They should be all acceptable +* in file names. We do not validate the UTF-8 here, that's not +* the job of this function. +*/ for (p1 = label.buf; *p1; p1++) - if (isspace(*p1)) + if (!(*p1 & 0x80) && !isalnum(*p1)) *(char *)p1 = '-'; I'm sightly concerned that this opens the possibility for unexpected effects if two different labels get sanitized to the same string. I suspect it's unlikely to happen in practice but doing something like percent encoding non-alphanumeric characters would avoid the problem entirely. Best Wishes Phillip strbuf_reset(&buf); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 7b6c4847ad..737396f944 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' ' test_path_is_missing .git/MERGE_HEAD ' +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' ' + git checkout -b colon-in-label E && + git merge -m "colon: this should work" G && + git rebase --rebase-merges --force-rebase E +' test_done
Re: git-diff passes : args to GIT_EXTERNAL_DIFF incorrectly?
On 30/08/2019 14:23, Dmitry Nikulin wrote: On Fri, 30 Aug 2019 at 13:16, Phillip Wood wrote: I'm not sure why the last argument is being split in your example. It is not split in the example below I have replicated the splitting issue on my small demo repo [1]: $ env GIT_EXTERNAL_DIFF=./print_argv.py git diff -M origin/branch1 origin/branch1-mv -- file1.txt file1-mv.txt ['./print_argv.py', 'file1.txt', '/tmp/EWaCSc_file1.txt', '2bef330804cb3f6962e45a72a12a3071ee9b5888', '100644', '/tmp/mtEiSc_file1-mv.txt', 'f8fd6737fbe5a45c97ba9c9de495dc46ff11eccd', '100644', 'file1-mv.txt', 'similarity index 90%\n' 'rename from file1.txt\n' 'rename to file1-mv.txt\n' 'index 2bef330..f8fd673 100644\n'] That's strange - What OS are you using? Does python do any pre-processing of arguments with newlines in them? This is, however, tangential to the original problem: documenting the external diff CLI interface for diffing two blobs. Here is what I am seeing: $ env GIT_EXTERNAL_DIFF=./print_argv.py git diff origin/branch1:file1.txt origin/branch1-mv:file1-mv.txt ['./print_argv.py', 'file1.txt', '/tmp/n9USvy_file1.txt', '2bef330804cb3f6962e45a72a12a3071ee9b5888', '100644', '/tmp/Zst0uy_file1-mv.txt', 'f8fd6737fbe5a45c97ba9c9de495dc46ff11eccd', '100644', 'file1-mv.txt', 'index 2bef330..f8fd673 100644\n'] The meaning and origin of the last arg remains mysterious, and the other args do not conform to the published documentation[2], which states that the args should be: The documentation is incomplete it should document the extra fields passed when it detects renames. path old-file old-hex old-mode new-file new-hex new-mode Instead the args that are passed are: path old-filename old-file old-hex old-mode new-file new-hex new-mode new-filename something I think what is happening is that because you're passing different filenames in to get the blobs the 'pass rename information' code is being triggered so it shows the new filename as the name used to get the second blob and then passes the header which only has an index line as there isn't any real rename information. Best Wishes Phillip [1]: https://github.com/dniku/git-external-diff-argv [2]: https://www.git-scm.com/docs/git#Documentation/git.txt-codeGITEXTERNALDIFFcode
Re: git-diff passes : args to GIT_EXTERNAL_DIFF incorrectly?
Hi Dmitry On 29/08/2019 15:36, Dmitry Nikulin wrote: Thank you for the reply. [...] However, for the original repository where I first faced this problem (https://github.com/yandexdataschool/Practical_RL), Git passes a very weird set of args to the external diff: $ env GIT_EXTERNAL_DIFF=./print_argv.py git diff -M master coursera -- week02_value_based/seminar_vi.ipynb week2_model_based/practice_vi.ipynb ['./print_argv.py', 'week02_value_based/seminar_vi.ipynb', '/tmp/amudWz_seminar_vi.ipynb', '8f8016963c888b7dd8dd20f60b7d6fdb41b26c1d', '100644', '/tmp/Ub7zPz_practice_vi.ipynb', '21db80f53b632d975a9af0acbaf397eb717cde2c', '100644', 'week2_model_based/practice_vi.ipynb', 'similarity index 82%\n' 'rename from week02_value_based/seminar_vi.ipynb\n' 'rename to week2_model_based/practice_vi.ipynb\n' 'index 8f80169..21db80f 100644\n'] I would guess that this is a bug. There can clearly be a hotfix (after all, Git passes all of the information to the external that it should per the spec, that is, -path, -hex, -mode; adding, however, some garbage). When git detects a rename it adds two parameters to the end of the argument list for the external diff program. The first extra argument is the new name and the second is the header with the similarity information[1]. I'm not sure why the last argument is being split in your example. It is not split in the example below $git mv git.c renamed.c $env GIT_EXTERNAL_DIFF='printf "|%s|\\n"' git diff HEAD |git.c| |/tmp/lMQpP8_git.c| |c1ee7124edcfb0417539134d50212e997dc71c1f| |100644| |renamed.c| |c1ee7124edcfb0417539134d50212e997dc71c1f| |100644| |renamed.c| |similarity index 100% rename from git.c rename to renamed.c | Best Wishes Phillip [1] https://github.com/git/git/blob/master/diff.c#L4204 I do not know though to what extent this information is correct. You say that this information is lost when I use the : notation; however, Git seems to pass paths and hexes correctly. This only leaves open the question of file mode. Perhaps it could be preserved at least for some cases, such as when the blob is retrieved from a path in a tree?
Re: [PATCH 1/1] checkout: add simple check for 'git checkout -b'
Hi Stolee
On 29/08/2019 18:01, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee
The 'git switch' command was created to separate half of the
behavior of 'git checkout'. It specifically has the mode to
do nothing with the index and working directory if the user
only specifies to create a new branch and change HEAD to that
branch. This is also the behavior most users expect from
'git checkout -b', but for historical reasons it also performs
an index update by scanning the working directory. This can be
slow for even moderately-sized repos.
A performance fix for 'git checkout -b' was introduced by
fa655d8411 (checkout: optimize "git checkout -b "
2018-08-16). That change includes details about the config
setting checkout.optimizeNewBranch when the sparse-checkout
feature is required. The way this change detected if this
behavior change is safe was through the skip_merge_working_tree()
method. This method was complex and needed to be updated
as new options were introduced.
This behavior was essentially reverted by 65f099b ("switch:
no worktree status unless real branch switch happens"
2019-03-29). Instead, two members of the checkout_opts struct
were used to distinguish between 'git checkout' and 'git switch':
* switch_branch_doing_nothing_is_ok
* only_merge_on_switching_branches
These settings have opposite values depending on if we start
in cmd_checkout or cmd_switch.
The message for 64f099b includes "Users of big repos are
encouraged to move to switch." Making this change while
'git switch' is still experimental is too aggressive.
Create a happy medium between these two options by making
'git checkout -b ' behave just like 'git switch',
but only if we read exactly those arguments. This must
be done in cmd_checkout to avoid the arguments being
consumed by the option parsing logic.
This differs from the previous change by fa644d8 in that
the config option checkout.optimizeNewBranch remains
deleted. This means that 'git checkout -b' will ignore
the index merge even if we have a sparse-checkout file.
While this is a behavior change for 'git checkout -b',
it matches the behavior of 'git switch -c'.
Signed-off-by: Derrick Stolee
---
builtin/checkout.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6123f732a2..116200cf90 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1713,6 +1713,15 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
opts.overlay_mode = -1;
opts.checkout_index = -2;/* default on */
opts.checkout_worktree = -2; /* default on */
+
+ if (argc == 3 && !strcmp(argv[1], "-b")) {
+ /*
+* User ran 'git checkout -b ' and expects
What if the user ran 'git checkout -b'? Then argc == 2.
Best Wishes
Phillip
+* the same behavior as 'git switch -c '.
+*/
+ opts.switch_branch_doing_nothing_is_ok = 0;
+ opts.only_merge_on_switching_branches = 1;
+ }
options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
Re: error: cannot cherry-pick during a revert
Hi Mike
On 29/08/2019 00:25, Mike Hommey wrote:
Hi,
This just happened to me while cherry-pick'ing:
$ git cherry-pick HEAD@{1}
error: could not apply 614fe5e629b84... try
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'
Recorded preimage for 'taskcluster/ci/build/linux.yml'
(... this is where I fix my conflict ...)
$ git add -u
$ git cherry-pick --continue
error: cannot cherry-pick during a revert.
fatal: cherry-pick failed
Oh dear that's not good
So apparently, cherry-pick thinks it was doing a revert when it hit a
conflict?
(This is with git 2.23)
I wondered if this was due to some of the recent changes adding --skip
to cherry-pick and revert but I can't see anything obvious at the
moment. To get that error the sequencer has loaded a todo file (in
read_populate_todo()) which starts with a revert command. Is it possible
you were reverting a sequence of commits before you ran the cherry-pick?
(a single pick or revert does not create a todo list). It could be that
there was an old todo list left over from a while ago - historically the
sequencer hasn't been all that good at cleaning up after itself if the
user committed the final pick or revert with 'git commit' and forgot to
run 'cherry-pick/revert --continue' afterwards.
Best Wishes
Phillip
Mike
Re: [PATCH 10/11] introduce container_of macro
On 27/08/2019 15:49, Derrick Stolee wrote: On 8/25/2019 10:43 PM, Eric Wong wrote: This macro is popular within the Linux kernel for supporting intrusive data structures such as linked lists, red-black trees, and chained hash tables while allowing the compiler to do type checking. I intend to use this to remove the limitation of "hashmap_entry" being location-dependent and to allow more compile-time type checking. This macro already exists in our source as "list_entry" in list.h and making "list_entry" an alias to "container_of" as the Linux kernel has done is a possibility. [snip] +/* + * container_of - Get the address of an object containing a field. + * + * @ptr: pointer to the field. + * @type: type of the object. + * @member: name of the field within the object. + */ +#define container_of(ptr, type, member) \ + ((type *) ((char *)(ptr) - offsetof(type, member))) + #endif I think it would be good to include at least one use of this macro in this patch. As it stands, I need to look at the next patch to make sense of what this is doing. It took me a little while to parse what is happening here. 'ptr' is a pointer to the generic struct (in our case, 'struct hashmap_entry *'), while 'type' is the parent type, and 'member' is the name of the member in 'type' that is of type typeof(*ptr). It took me a couple of minutes to figure it out as well. The rest of this patch series adds some very welcome type safety changes, at first sight this patch threatens to undermine that as there is no check (and no compiler independent way to check) that type == typeof(*ptr). It would also be helpful if the commit message could explain how this can be used to improve type safety. Best Wishes Phillip Perhaps this is easier to grok for others than it was for me. -Stolee
Re: [PATCH 04/11] hashmap_entry: detect improper initialization
Hi Eric On 27/08/2019 10:49, Eric Wong wrote: Johannes Schindelin wrote: Hi Eric, On Mon, 26 Aug 2019, Eric Wong wrote: By renaming the "hash" field to "_hash", it's easy to spot improper initialization of hashmap_entry structs which can leave "hashmap_entry.next" uninitialized. Would you mind elaborating a bit? This explanation does not enlighten me, sadly, all I see is that it makes it (slightly) harder for me to maintain Git for Windows' patches on top of `pu`, as the FSCache patches access that field directly (so even if they rebase cleanly, the build breaks). I renamed it to intentionally break my build. That way I could easily spot if there were any other improper initializations of the .hash field. It's fine to revert, actually, it could be more of a "showing my work" patch. I'm still a bit confused as the changed initializations already used hashmap_entry_init() and so presumably were already initializing hashmap_entry.next correctly. Is there a way to get 'make coccicheck' detect incorrect initializations, this renaming wont prevent bad code being added in the future. Best Wishes Phillip (AFAIK, it's a pretty common practice, but maybe not here :x) I've also pondered adding a "hashmap_entry_hash(const struct hashmap_entry *)" accessor method for reading the field value (but not setting it), but it's a bit verbose... I'm also wondering where/if hashmap offers a real benefit over khash nowadays; the latter ought to have better locality. Would like benchmark at some point in the future; but safety fixes first :)
Re: [PATCH v5 0/2] Honor .gitattributes with rebase --am
Hi Brian
On 26/08/2019 00:33, brian m. carlson wrote:
This series makes rebase --am honor the .gitattributes file for
subsequent patches when a patch changes it.
Note that there are two places we load attributes in ll-merge.c, but
this code only handles the one that git am uses. The two cannot be
unified because the one in ll_merge_marker_size intentionally doesn't
load the merge attribute, since it wants to always use the recursive
strategy. Loading it anyway causes t4017 to fail.
Changes from v4:
* Wrap lines in apply.c.
* Handle merge and conflict-marker-size attributes.
* Add tests for am and am -3 in addition to rebase.
I've only had time for a quick look at these but the changes look good
to me. Thanks for taking the time to add the tests for am and the code
for handling the merge attributes
Best Wishes
Phillip
Changes from v3:
* Check for both addition and removal of .gitattributes files.
* Switch from "test_config" to "git config".
Changes from v2:
* Rename has_path_suffix to ends_with_path_components.
Changes from v1:
* Add has_path_suffix in a separate commit.
brian m. carlson (2):
path: add a function to check for path suffix
am: reload .gitattributes after patching it
apply.c | 11 ++
convert.c | 11 +-
convert.h | 6 ++
ll-merge.c| 19 +
ll-merge.h| 1 +
path.c| 39 +++
path.h| 3 +++
t/t3400-rebase.sh | 36
t/t4150-am.sh | 52 +++
9 files changed, 164 insertions(+), 14 deletions(-)
Range-diff against v4:
1: fa825e4b40 ! 1: 2077a0829e apply: reload .gitattributes after patching it
@@ Metadata
Author: brian m. carlson
## Commit message ##
-apply: reload .gitattributes after patching it
+am: reload .gitattributes after patching it
When applying multiple patches with git am, or when rebasing using the
am backend, it's possible that one of our patches has updated a
@@ Commit message
To ensure we write the correct data into the working tree, expire the
cache after each patch that touches a path ending in ".gitattributes".
+Since we load these attributes in multiple separate files, we must
+expire them accordingly.
+
+Verify that both the am and rebase code paths work correctly,
including
+the conflict marker size with am -3.
Signed-off-by: brian m. carlson
@@ apply.c: static int apply_patch(struct apply_state *state,
*listp = patch;
listp = &patch->next;
+
-+ if ((patch->new_name &&
ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
-+ (patch->old_name &&
ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
++ if ((patch->new_name &&
++ ends_with_path_components(patch->new_name,
++GITATTRIBUTES_FILE)) ||
++ (patch->old_name &&
++ ends_with_path_components(patch->old_name,
++GITATTRIBUTES_FILE)))
+ flush_attributes = 1;
}
else {
@@ apply.c: static int apply_patch(struct apply_state *state,
strbuf_release(&buf);
## convert.c ##
+@@
+ #include "pkt-line.h"
+ #include "sub-process.h"
+ #include "utf8.h"
++#include "ll-merge.h"
+
+ /*
+ * convert.c - convert a file when checking it out and checking it in.
@@ convert.c: struct conv_attrs {
const char *working_tree_encoding; /* Supported encoding or default
encoding if NULL */
};
@@ convert.c: static void convert_attrs(const struct index_state *istate,
+{
+ attr_check_free(check);
+ check = NULL;
++ reset_merge_attributes();
+}
+
int would_convert_to_git_filter_fd(const struct index_state *istate,
const char *path)
@@ convert.h: void convert_to_git_filter_fd(const struct index_state
*istate,
*
* Streaming conversion support
+ ## ll-merge.c ##
+@@ ll-merge.c: struct ll_merge_driver {
+ char *cmdline;
+ };
+
++static struct attr_check *merge_attributes;
++static struct attr_check *load_merge_attributes(void)
++{
++ if (!merge_attributes)
++ merge_attributes = attr_check_initl("merge",
"conflict-marker-size", NULL);
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
On 20/08/2019 19:32, Phillip Wood wrote: On 20/08/2019 19:24, Junio C Hamano wrote: Phillip Wood writes: Do you know why -m and -i aren't affected? I had to look, but I believe the answer is because they use the sequencer, and the sequencer calls git merge-recursive as a separate process, and so the writing of the tree is only done in a subprocess, which can't persist state. The sequencer has been running in a single process for a while now. We do fork for 'git merge' sometimes when processing 'merge' commands but 'pick' commands are all done in a single process by calling do_recursive_merge(). Best Wishes Phillip Should we move the merge-recursive code into the main process, we'll likely have the same problem there. So we actually have the same issue already? I don't think so, I modified Brian's test to call 'rebase -i' and it passes but no one seems to know why. I spent some time digging into this and the attributes are reloaded before each pick. This is because check_updates() called by unpack_trees() calls git_attr_set_direction(GIT_ATTR_CHECKOUT) at the start of the function and git_attr_set_direction(GIT_ATTR_CHECKIN) at the end of the function. This has the effect of dropping all loaded attributes as git_attr_set_direction() calls drop_all_attr_stacks() when the direction is changed. Best Wishes Phillip Best Wishes Phillip
Re: [PATCH 3/3] sequencer: simplify root commit creation
On 19/08/2019 17:09, Eric Sunshine wrote: On Mon, Aug 19, 2019 at 5:18 AM Phillip Wood via GitGitGadget wrote: Adapt try_to_commit() to create a new root commit rather than special casing this in run_git_commit(). The significantly reduces the amount of s/The/This/ Thanks Eric - well spotted as ever. Thanks Junio for fixing this up in pu. Best Wishes Phillip special case code for creating the root commit and reduces the number of commit code paths we have to worry about. Signed-off-by: Phillip Wood
Re: [PATCH v3 1/6] rebase -i: add --ignore-whitespace flag
Hi Rohit
On 20/08/2019 04:45, Rohit Ashiwal wrote:
There are two backends available for rebasing, viz, the am and the
interactive. Naturally, there shall be some features that are
implemented in one but not in the other. One such flag is
--ignore-whitespace which indicates merge mechanism to treat lines
with only whitespace changes as unchanged. Wire the interactive
rebase to also understand the --ignore-whitespace flag by
translating it to -Xignore-space-change.
Signed-off-by: Rohit Ashiwal
---
Documentation/git-rebase.txt| 13 -
builtin/rebase.c| 22 +++--
t/t3422-rebase-incompatible-options.sh | 1 -
t/t3433-rebase-options-compatibility.sh | 65 +
4 files changed, 94 insertions(+), 7 deletions(-)
create mode 100755 t/t3433-rebase-options-compatibility.sh
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6156609cf7..873eb5768c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -371,8 +371,16 @@ If either or --root is given on the command
line, then the
default is `--no-fork-point`, otherwise the default is `--fork-point`.
--ignore-whitespace::
+ Behaves differently depending on which backend is selected.
++
+'am' backend: When applying a patch, ignore changes in whitespace in
+context lines if necessary.
++
+'interactive' backend: Treat lines with only whitespace changes as
+unchanged for the sake of a three-way merge.
+
Thanks for spelling out exactly what this does. I had not appreciated
the difference before. Does this mean that if I have a branch with some
whitespace cleanups I'll get different results if I rebase it with the
sequencer compared to am? (I suspect from the description that the
sequencer will simply ignore all my whitespace changes)
I think this is ready if we can live with the difference - I'm not
entirely convinced that adding an option with the same name and a
different behavior is going to improve things though.
Best Wishes
Phillip
--whitespace=::
- These flag are passed to the 'git apply' program
+ This flag is passed to the 'git apply' program
(see linkgit:git-apply[1]) that applies the patch.
+
See also INCOMPATIBLE OPTIONS below.
@@ -520,7 +528,6 @@ The following options:
* --committer-date-is-author-date
* --ignore-date
* --whitespace
- * --ignore-whitespace
* -C
are incompatible with the following options:
@@ -543,6 +550,8 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
+ * --preserve-merges and --ignore-whitespace
+ * --rebase-merges and --ignore-whitespace
* --rebase-merges and --strategy
* --rebase-merges and --strategy-option
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 670096c065..f8a618d54c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -79,6 +79,7 @@ struct rebase_options {
int allow_rerere_autoupdate;
int keep_empty;
int autosquash;
+ int ignore_whitespace;
char *gpg_sign_opt;
int autostash;
char *cmd;
@@ -99,6 +100,7 @@ struct rebase_options {
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
{
+ struct strbuf strategy_buf = STRBUF_INIT;
struct replay_opts replay = REPLAY_OPTS_INIT;
replay.action = REPLAY_INTERACTIVE_REBASE;
@@ -114,9 +116,15 @@ static struct replay_opts get_replay_opts(const struct
rebase_options *opts)
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
replay.strategy = opts->strategy;
+
if (opts->strategy_opts)
- parse_strategy_opts(&replay, opts->strategy_opts);
+ strbuf_addstr(&strategy_buf, opts->strategy_opts);
+ if (opts->ignore_whitespace)
+ strbuf_addstr(&strategy_buf, " --ignore-space-change");
+ if (strategy_buf.len)
+ parse_strategy_opts(&replay, strategy_buf.buf);
+ strbuf_release(&strategy_buf);
return replay;
}
@@ -511,6 +519,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options,
builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
+ opts.strategy_opts = xstrdup_or_null(opts.strategy_opts);
+
if (!is_null_oid(&squash_onto))
opts.squash_onto = &squash_onto;
@@ -964,6 +974,8 @@ static int run_am(struct rebase_options *opts)
am.git_cmd = 1;
argv_array_push(&am.args, "am");
+ if (opts->ignore_whitespace)
+ argv_array_push(&am.args, "--igno
Re: [PATCH v3 0/6] rebase -i: support more options
On 20/08/2019 18:53, Junio C Hamano wrote:
Phillip Wood writes:
Hi Rohit
On 20/08/2019 04:45, Rohit Ashiwal wrote:
I've tries to incorporated all the suggestions.
It is helpful if you can list the changes to remind us all what we
said. (as a patch author I find composing that is helpful to remind me
if there's anything I've forgotten to address)
Also there are a couple of things that were discussed such as
splitting up the author and passing it round as a tuple and testing a non-default timezone which aren't included -
that's fine but it helps if you take a moment to explain why in the
cover letter.
Some points:
- According to v2.0.0's git-am.sh, ignore-date should override
committer-date-is-author-date. Ergo, we are not barfing out
when both flags are provided.
- Should the 'const' qualifier be removed[2]? Since it is leaving
a false impression that author should not be free()'d.
The author returned by read_author_ident() is owned by the strbuf that
you pass to read_author_ident() which is confusing.
Best Wishes
Phillip
I've looked at this round, but will push out v2 for today's
integration cycle, mostly due to lack of time, but there do not seem
to be great difference between the iterations.
The "ignore-date" step conflicts semantically with b0a31861
("sequencer: simplify root commit creation", 2019-08-19) but in a
good way. Without the clean-up b0a31861 makes, we need to munge the
timestamp in two places, but with it, there is only one place that
needs to modify the timestamp for the feature (in try_to_commit()).
That was my hope
You may want to see if these "more options" topic can take advantage
of the "simplify root commit creation" by building on top of some
form of it (I do not know offhand if b0a31861 ("sequencer: simplify
root commit creation", 2019-08-19) must build on other two patches
to fix "rebase -i" or it can be split out as an independent
clean-up), and if it is feasible, work with your student to make it
happen, perhaps?
If it is separated out from the first two there will be a minor textual
conflict as the first patch in that series changes the root commit
creation to stop it writing CHERRY_PICK_HEAD and then b0a31861 deletes
the modified version but it shouldn't be a problem to resolve manually.
Rohit do you want to try cherry-picking b0a31861 onto master and then
rebasing your patches on top of it? - it would be nice to simplify things.
Best Wishes
Phillip
Thanks.
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
On 20/08/2019 19:24, Junio C Hamano wrote: Phillip Wood writes: Do you know why -m and -i aren't affected? I had to look, but I believe the answer is because they use the sequencer, and the sequencer calls git merge-recursive as a separate process, and so the writing of the tree is only done in a subprocess, which can't persist state. The sequencer has been running in a single process for a while now. We do fork for 'git merge' sometimes when processing 'merge' commands but 'pick' commands are all done in a single process by calling do_recursive_merge(). Best Wishes Phillip Should we move the merge-recursive code into the main process, we'll likely have the same problem there. So we actually have the same issue already? I don't think so, I modified Brian's test to call 'rebase -i' and it passes but no one seems to know why. Best Wishes Phillip
Re: [PATCH v3 5/6] rebase -i: support --ignore-date
On 20/08/2019 18:42, Junio C Hamano wrote:
Rohit Ashiwal writes:
+/* Construct a free()able author string with current time as the author date */
+static char *ignore_author_date(const char *author)
+{
+ int len = strlen(author);
Mental note: ignore_author_date() would not allow author==NULL as its input.
@@ -1020,10 +1047,20 @@ static int run_git_commit(struct repository *r,
if (res <= 0)
res = error_errno(_("could not read '%s'"), defmsg);
- else
+ else {
+ if (opts->ignore_date) {
+ char *new_author = ignore_author_date(author);
+ if (!author)
+ BUG("ignore-date can only be used with "
+ "rebase, which must set the author "
+ "before committing the tree");
Yet, author is used and then checked for NULL-ness, which is
backwards. Before we have a chance to issue this BUG(), we would
already have segfaulted inside ignore_author_date().
Good catch!
Re: [PATCH v3 0/6] rebase -i: support more options
Hi Rohit
On 20/08/2019 04:45, Rohit Ashiwal wrote:
I've tries to incorporated all the suggestions.
It is helpful if you can list the changes to remind us all what we said.
(as a patch author I find composing that is helpful to remind me if
there's anything I've forgotten to address)
Also there are a couple of things that were discussed such as splitting
up the author and passing it round as a tuple
and testing a non-default timezone which aren't included - that's fine
but it helps if you take a moment to explain why in the cover letter.
Some points:
- According to v2.0.0's git-am.sh, ignore-date should override
committer-date-is-author-date. Ergo, we are not barfing out
when both flags are provided.
- Should the 'const' qualifier be removed[2]? Since it is leaving
a false impression that author should not be free()'d.
The author returned by read_author_ident() is owned by the strbuf that
you pass to read_author_ident() which is confusing.
Best Wishes
Phillip
[1]: git show v2.0.0:git-am.sh
[2]: https://github.com/git/git/blob/v2.23.0/sequencer.c#L959
Rohit Ashiwal (6):
rebase -i: add --ignore-whitespace flag
sequencer: add NULL checks under read_author_script
rebase -i: support --committer-date-is-author-date
sequencer: rename amend_author to author_to_rename
rebase -i: support --ignore-date
rebase: add --reset-author-date
Documentation/git-rebase.txt| 26 +++--
builtin/rebase.c| 53 +++---
sequencer.c | 135 ++--
sequencer.h | 2 +
t/t3422-rebase-incompatible-options.sh | 2 -
t/t3433-rebase-options-compatibility.sh | 100 ++
6 files changed, 289 insertions(+), 29 deletions(-)
create mode 100755 t/t3433-rebase-options-compatibility.sh
Range-diff:
1: 4cd0aa3084 ! 1: e82ed8cad5 rebase -i: add --ignore-whitespace flag
@@ -19,10 +19,13 @@
default is `--no-fork-point`, otherwise the default is `--fork-point`.
--ignore-whitespace::
-+ This flag is either passed to the 'git apply' program
-+ (see linkgit:git-apply[1]), or to 'git merge' program
-+ (see linkgit:git-merge[1]) as `-Xignore-space-change`,
-+ depending on which backend is selected by other options.
++ Behaves differently depending on which backend is selected.
+++
++'am' backend: When applying a patch, ignore changes in whitespace in
++context lines if necessary.
+++
++'interactive' backend: Treat lines with only whitespace changes as
++unchanged for the sake of a three-way merge.
+
--whitespace=::
- These flag are passed to the 'git apply' program
@@ -63,7 +66,7 @@
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
{
-+ char *strategy_opts = opts->strategy_opts;
++ struct strbuf strategy_buf = STRBUF_INIT;
struct replay_opts replay = REPLAY_OPTS_INIT;
replay.action = REPLAY_INTERACTIVE_REBASE;
@@ -71,24 +74,19 @@
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
replay.strategy = opts->strategy;
-- if (opts->strategy_opts)
-- parse_strategy_opts(&replay, opts->strategy_opts);
-+
-+ if (opts->ignore_whitespace) {
-+ struct strbuf buf = STRBUF_INIT;
-+
-+ if (strategy_opts)
-+ strbuf_addstr(&buf, strategy_opts);
+
-+ strbuf_addstr(&buf, " --ignore-space-change");
-+ free(strategy_opts);
-+ strategy_opts = strbuf_detach(&buf, NULL);
-+ }
-+ if (strategy_opts)
-+ parse_strategy_opts(&replay, strategy_opts);
+ if (opts->strategy_opts)
+- parse_strategy_opts(&replay, opts->strategy_opts);
++ strbuf_addstr(&strategy_buf, opts->strategy_opts);
++ if (opts->ignore_whitespace)
++ strbuf_addstr(&strategy_buf, " --ignore-space-change");
++ if (strategy_buf.len)
++ parse_strategy_opts(&replay, strategy_buf.buf);
++ strbuf_release(&strategy_buf);
return replay;
}
+
@@
argc = parse_options(argc, argv, prefix, options,
builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
2: e2c0304587 = 2: 209057b361 sequencer: add NULL checks under
read_author_script
3: 6aed57ae2e ! 3: a4e6644ef8 rebase -i: support
--committer-date-is-author-date
@@ -21,10 +21,12 @@
+
--ignore-date::
- These flags are passed to 'git am' to easily change the dates
+- of the rebased comm
Re: [PATCH v3 5/6] rebase -i: support --ignore-date
"));
@@ -2592,6 +2636,11 @@ static int read_populate_opts(struct replay_opts *opts)
opts->committer_date_is_author_date = 1;
}
+ if (file_exists(rebase_path_ignore_date())) {
+ opts->allow_ff = 0;
+ opts->ignore_date = 1;
+ }
+
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;
@@ -2676,6 +2725,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
write_file(rebase_path_signoff(), "--signoff\n");
if (opts->committer_date_is_author_date)
write_file(rebase_path_cdate_is_adate(), "%s", "");
+ if (opts->ignore_date)
+ write_file(rebase_path_ignore_date(), "%s", "");
if (opts->reschedule_failed_exec)
write_file(rebase_path_reschedule_failed_exec(), "%s", "");
@@ -3601,6 +3652,8 @@ static int do_merge(struct repository *r,
argv_array_push(&cmd.args, git_path_merge_msg(r));
if (opts->gpg_sign)
argv_array_push(&cmd.args, opts->gpg_sign);
+ if (opts->ignore_date)
+ push_dates(&cmd);
/* Add the tips to be merged */
for (j = to_merge; j; j = j->next)
@@ -3874,7 +3927,8 @@ static int pick_commits(struct repository *r,
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit ||
- opts->committer_date_is_author_date));
+ opts->committer_date_is_author_date ||
+ opts->ignore_date));
if (read_and_refresh_cache(r, opts))
return -1;
diff --git a/sequencer.h b/sequencer.h
index e3881e9275..bf5a79afdb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,7 @@ struct replay_opts {
int quiet;
int reschedule_failed_exec;
int committer_date_is_author_date;
+ int ignore_date;
int mainline;
diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
index b2419a2b75..c060fcd10b 100755
--- a/t/t3433-rebase-options-compatibility.sh
+++ b/t/t3433-rebase-options-compatibility.sh
@@ -81,4 +81,20 @@ test_expect_success '--committer-date-is-author-date works
with interactive back
test_cmp authortime committertime
'
+# Checking for + in author time is enough since default
+# timezone is UTC, but the timezone used while committing
+# sets to +0530.
I'm still worried that this will hide some breakage in the future. Can
we add a test for rebase -r as well to check that code please.
Best Wishes
Phillip
+test_expect_success '--ignore-date works with am backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ grep "+" authortime
+'
+
+test_expect_success '--ignore-date works with interactive backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date -i HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ grep "+" authortime
+'
test_done
Re: [PATCH v3 3/6] rebase -i: support --committer-date-is-author-date
}
+ if (options.committer_date_is_author_date)
+ options.flags |= REBASE_FORCE;
+
for (i = 0; i < options.git_am_opts.argc; i++) {
const char *option = options.git_am_opts.argv[i], *p;
- if (!strcmp(option, "--committer-date-is-author-date") ||
- !strcmp(option, "--ignore-date") ||
+ if (!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
diff --git a/sequencer.c b/sequencer.c
index 30d77c2682..29b67bc1ae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,6 +147,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
* command-line.
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
@@ -879,6 +880,17 @@ static char *get_author(const char *message)
return NULL;
}
+/* Returns a "date" string that needs to be free()'d by the caller */
+static char *read_author_date_or_null(void)
+{
+ char *date;
+
+ if (read_author_script(rebase_path_author_script(),
+ NULL, NULL, &date, 0))
+ return NULL;
+ return date;
+}
+
/* Read author-script and return an ident line (author timestamp) */
static const char *read_author_ident(struct strbuf *buf)
{
@@ -964,6 +976,25 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
+ if (opts->committer_date_is_author_date) {
+ size_t len;
+ int res = -1;
+ struct strbuf datebuf = STRBUF_INIT;
+ char *date = read_author_date_or_null();
+
+ if (!date)
+ return -1;
It is good to this this new error checking
+
+ strbuf_addf(&datebuf, "@%s", date);
+ free(date);
+
+ date = strbuf_detach(&datebuf, &len);
+ res = setenv("GIT_COMMITTER_DATE", date, 1);
+ free(date);
This could be simplified slightly by doing
res = setenv("GIT_COMMITTER_DATE", datebuf.buf. 1);
strbuf_release(&datebuf);
+
+ if (res)
+ return -1;
+ }
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
const char *author = NULL;
@@ -1400,7 +1431,6 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
-
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
const char *out_enc = get_commit_output_encoding();
@@ -1427,6 +1457,21 @@ static int try_to_commit(struct repository *r,
commit_list_insert(current_head, &parents);
}
+ if (opts->committer_date_is_author_date) {
+ int len = strlen(author);
+ struct ident_split ident;
+ struct strbuf date = STRBUF_INIT;
+
+ split_ident_line(&ident, author, len);
+
+ if (!ident.date_begin)
+ return error(_("corrupted author without date
information"));
I had to read the source of split_ident_line() to check this was safe, I
think it is better to use the return value to check for errors - a
quick greps shows that this is what all the other callers do.
+
+ strbuf_addf(&date, "@%s", ident.date_begin);
I still think this will pass any junk after a corrupted timezone, it
would be better to do
strbuf_addf(&date, "@%.*s %.*s", ident.date_begin, ident.data_end -
ident.date_begin, ident.tz_begin, ident.tz_end - ident.tz_begin);
+ setenv("GIT_COMMITTER_DATE", date.buf, 1);
The code above (in run_git_commit()) checks the return value of setenv()
and returns an error if it failed should this be doing the same?
Best Wishes
Phillip
+ strbuf_release(&date);
+ }
+
if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
res = error(_("git write-tree failed to write a tree"));
goto out;
@@ -2542,6 +2587,11 @@ static int read_populate_opts(struct replay_opts *opts)
opts->signoff = 1;
}
+
Re: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing
Hi Ben
I need to have a longer look at this (I don't understand why we're
calling reset --hard after we've stashed the changes) but I notice that
the test lines you're changing predate the switch to the builtin rebase
so those changes are not related to the branch switching problem.
Best Wishes
Phillip
On 18/08/2019 10:53, Ben Wijen wrote:
The rebase --autostash incorrectly moved the current branch to orig_head, where
orig_head -- commit object name of tip of the branch before rebasing
It seems this was incorrectly taken over from git-legacy-rebase.sh
Signed-off-by: Ben Wijen
---
builtin/rebase.c| 18 ++
t/t3420-rebase-autostash.sh | 4
2 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 670096c065..a928f44941 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
state_dir_path("autostash", &options);
struct child_process stash = CHILD_PROCESS_INIT;
struct object_id oid;
- struct commit *head =
- lookup_commit_reference(the_repository,
- &options.orig_head);
argv_array_pushl(&stash.args,
"stash", "create", "autostash", NULL);
@@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
options.state_dir);
write_file(autostash, "%s", oid_to_hex(&oid));
printf(_("Created autostash: %s\n"), buf.buf);
- if (reset_head(&head->object.oid, "reset --hard",
+
+ /*
+* We might not be on orig_head yet:
+* Make sure to reset w/o switching branches...
+*/
+ if (reset_head(NULL, "reset --hard",
NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
die(_("could not reset --hard"));
- printf(_("HEAD is now at %s"),
- find_unique_abbrev(&head->object.oid,
- DEFAULT_ABBREV));
- strbuf_reset(&buf);
- pp_commit_easy(CMIT_FMT_ONELINE, head, &buf);
- if (buf.len > 0)
- printf(" %s", buf.buf);
- putchar('\n');
if (discard_index(the_repository->index) < 0 ||
repo_read_index(the_repository) < 0)
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 867e4e0b17..2ea1909881 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -37,7 +37,6 @@ test_expect_success setup '
create_expected_success_am () {
cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
First, rewinding head to replay your work on top of it...
Applying: second commit
Applying: third commit
@@ -48,7 +47,6 @@ create_expected_success_am () {
create_expected_success_interactive () {
q_to_cr >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
Applied autostash.
Successfully rebased and updated refs/heads/rebased-feature-branch.
EOF
@@ -57,7 +55,6 @@ create_expected_success_interactive () {
create_expected_failure_am () {
cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
First, rewinding head to replay your work on top of it...
Applying: second commit
Applying: third commit
@@ -70,7 +67,6 @@ create_expected_failure_am () {
create_expected_failure_interactive () {
cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
Applying autostash resulted in conflicts.
Your changes are safe in the stash.
You can run "git stash pop" or "git stash drop" at any time.
Re: [PATCH 1/2] t3420: never change upstream branch
Hi Ben On 18/08/2019 10:53, Ben Wijen wrote: When using `git rebase --autostash ` and the workarea is dirty, the active branch is incorrectly reset to the rebase branch. This test will check for such behavior. Signed-off-by: Ben Wijen --- t/t3420-rebase-autostash.sh | 9 + 1 file changed, 9 insertions(+) diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index b8f4d03467..867e4e0b17 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -306,4 +306,13 @@ test_expect_success 'branch is left alone when possible' ' test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" ' +test_expect_success 'never change upstream branch' ' + test_when_finished "git reset --hard && git branch -D upstream" && + git checkout -b upstream unrelated-onto-branch && + echo changed >file0 && + git add file0 && + git rebase --autostash upstream feature-branch && + test $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch) In addition to Junio's suggestions I'd add using test_cmp_rev upstream unrelated-onto-branch for the last line. Best Wishes Phillip +' + test_done
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
On 20/08/2019 04:05, brian m. carlson wrote:
On 2019-08-19 at 09:55:27, Phillip Wood wrote:
On 19/08/2019 10:41, Phillip Wood wrote:
[...]
diff --git a/convert.c b/convert.c
index 94ff837649..030e9b81b9 100644
--- a/convert.c
+++ b/convert.c
@@ -1293,10 +1293,11 @@ struct conv_attrs {
const char *working_tree_encoding; /* Supported encoding or
default encoding if NULL */
};
+static struct attr_check *check;
I was concerned about the impact adding a file global if we ever want to
multi-thread this for submodules, but looking through the file there are
a couple of others already so this isn't creating a new problem.
Doh, I've just realized it was static already - ignore that.
And I just realized that I didn't read the entire thread before
responding. Sorry about that.
One thing did occur to me though - does this patch reset attributes like the
merge marker length (they're less critical though if there is a conflict
after an attribute change it would be nice to have the correct length) or
just the ones for filtering files?
It resets "crlf", "ident", "filter", "eol", "text", and
"working-tree-encoding". Things it doesn't reset include "whitespace",
"export-ignore", "export-subst", "merge", and "conflict-marker-size".
Of these, I think only the latter two are relevant.
I'll update that in v5.
Thanks, one other thought I had was that this is really a bug in 'am'
rather than 'rebase'. There has been some talk of switching the default
rabase backend to the sequencer so maybe it would be better to test 'am'
rather than 'rebase' to ensure we still check 'am' is working properly
after any switch in the rebase backend. (perhaps we should be testing
rebase and rebase --interactive separately as well to prevent any future
regressions I'm not sure)
Best Wishes
Phillip
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
Hi Brian
On 20/08/2019 03:45, brian m. carlson wrote:
On 2019-08-19 at 09:41:42, Phillip Wood wrote:
Hi Brian
On 18/08/2019 19:44, brian m. carlson wrote:
When applying multiple patches with git am, or when rebasing using the
am backend, it's possible that one of our patches has updated a
gitattributes file. Currently, we cache this information, so if a
file in a subsequent patch has attributes applied, the file will be
written out with the attributes in place as of the time we started the
rebase or am operation, not with the attributes applied by the previous
patch. This problem does not occur when using the -m or -i flags to
rebase.
Do you know why -m and -i aren't affected?
I had to look, but I believe the answer is because they use the
sequencer, and the sequencer calls git merge-recursive as a separate
process, and so the writing of the tree is only done in a subprocess,
which can't persist state.
The sequencer has been running in a single process for a while now. We
do fork for 'git merge' sometimes when processing 'merge' commands but
'pick' commands are all done in a single process by calling
do_recursive_merge().
Best Wishes
Phillip
Should we move the merge-recursive code into the main process, we'll
likely have the same problem there.
diff --git a/apply.c b/apply.c
index cde95369bb..d57bc635e4 100644
--- a/apply.c
+++ b/apply.c
@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
struct patch *list = NULL, **listp = &list;
int skipped_patch = 0;
int res = 0;
+ int flush_attributes = 0;
state->patch_input_file = filename;
if (read_patch_file(&buf, fd) < 0)
@@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,
patch_stats(state, patch);
*listp = patch;
listp = &patch->next;
+
+ if ((patch->new_name &&
ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
+ (patch->old_name &&
ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
+ flush_attributes = 1;
style nit - these lines are very long compared to 80 characters
They are. They aren't two different from other lines in the file, and I
thought that leaving them that way would preserve the similarities, but
I can also wrap them. I'll send out a v5 with wrapped lines.
diff --git a/convert.c b/convert.c
index 94ff837649..030e9b81b9 100644
--- a/convert.c
+++ b/convert.c
@@ -1293,10 +1293,11 @@ struct conv_attrs {
const char *working_tree_encoding; /* Supported encoding or default
encoding if NULL */
};
+static struct attr_check *check;
I was concerned about the impact adding a file global if we ever want to
multi-thread this for submodules, but looking through the file there are a
couple of others already so this isn't creating a new problem.
+
static void convert_attrs(const struct index_state *istate,
struct conv_attrs *ca, const char *path)
{
- static struct attr_check *check;
struct attr_check_item *ccheck = NULL;
if (!check) {
@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state
*istate,
ca->crlf_action = CRLF_AUTO_INPUT;
}
The portion I've included above demonstrates that this was already a
static variable, just one hidden in a function. So yeah, this is no
worse than it already was.
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
On 19/08/2019 10:41, Phillip Wood wrote:
[...]
diff --git a/convert.c b/convert.c
index 94ff837649..030e9b81b9 100644
--- a/convert.c
+++ b/convert.c
@@ -1293,10 +1293,11 @@ struct conv_attrs {
const char *working_tree_encoding; /* Supported encoding or
default encoding if NULL */
};
+static struct attr_check *check;
I was concerned about the impact adding a file global if we ever want to
multi-thread this for submodules, but looking through the file there are
a couple of others already so this isn't creating a new problem.
Doh, I've just realized it was static already - ignore that.
One thing did occur to me though - does this patch reset attributes like
the merge marker length (they're less critical though if there is a
conflict after an attribute change it would be nice to have the correct
length) or just the ones for filtering files?
Best Wishes
Phillip
Best Wishes
Phillip
+
static void convert_attrs(const struct index_state *istate,
struct conv_attrs *ca, const char *path)
{
- static struct attr_check *check;
struct attr_check_item *ccheck = NULL;
if (!check) {
@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct
index_state *istate,
ca->crlf_action = CRLF_AUTO_INPUT;
}
+void reset_parsed_attributes(void)
+{
+ attr_check_free(check);
+ check = NULL;
+}
+
int would_convert_to_git_filter_fd(const struct index_state *istate,
const char *path)
{
struct conv_attrs ca;
diff --git a/convert.h b/convert.h
index 831559f10d..3710969d43 100644
--- a/convert.h
+++ b/convert.h
@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct
index_state *istate,
int would_convert_to_git_filter_fd(const struct index_state *istate,
const char *path);
+/*
+ * Reset the internal list of attributes used by convert_to_git and
+ * convert_to_working_tree.
+ */
+void reset_parsed_attributes(void);
+
/*
*
* Streaming conversion support
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80b23fd326..23469cc789 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -301,6 +301,42 @@ test_expect_success 'rebase --am and
--show-current-patch' '
)
'
+test_expect_success 'rebase --am and .gitattributes' '
+ test_create_repo attributes &&
+ (
+ cd attributes &&
+ test_commit init &&
+ git config filter.test.clean "sed -e
'\''s/smudged/clean/g'\''" &&
+ git config filter.test.smudge "sed -e
'\''s/clean/smudged/g'\''" &&
+
+ test_commit second &&
+ git checkout -b test HEAD^ &&
+
+ echo "*.txt filter=test" >.gitattributes &&
+ git add .gitattributes &&
+ test_commit third &&
+
+ echo "This text is smudged." >a.txt &&
+ git add a.txt &&
+ test_commit fourth &&
+
+ git checkout -b removal HEAD^ &&
+ git rm .gitattributes &&
+ git add -u &&
+ test_commit fifth &&
+ git cherry-pick test &&
+
+ git checkout test &&
+ git rebase master &&
+ grep "smudged" a.txt &&
+
+ git checkout removal &&
+ git reset --hard &&
+ git rebase master &&
+ grep "clean" a.txt
+ )
+'
+
test_expect_success 'rebase--merge.sh and --show-current-patch' '
test_create_repo conflict-merge &&
(
Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it
Hi Brian
On 18/08/2019 19:44, brian m. carlson wrote:
When applying multiple patches with git am, or when rebasing using the
am backend, it's possible that one of our patches has updated a
gitattributes file. Currently, we cache this information, so if a
file in a subsequent patch has attributes applied, the file will be
written out with the attributes in place as of the time we started the
rebase or am operation, not with the attributes applied by the previous
patch. This problem does not occur when using the -m or -i flags to
rebase.
Do you know why -m and -i aren't affected?
To ensure we write the correct data into the working tree, expire the
cache after each patch that touches a path ending in ".gitattributes".
Signed-off-by: brian m. carlson
---
apply.c | 7 +++
convert.c | 9 -
convert.h | 6 ++
t/t3400-rebase.sh | 36
4 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/apply.c b/apply.c
index cde95369bb..d57bc635e4 100644
--- a/apply.c
+++ b/apply.c
@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
struct patch *list = NULL, **listp = &list;
int skipped_patch = 0;
int res = 0;
+ int flush_attributes = 0;
state->patch_input_file = filename;
if (read_patch_file(&buf, fd) < 0)
@@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,
patch_stats(state, patch);
*listp = patch;
listp = &patch->next;
+
+ if ((patch->new_name &&
ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
+ (patch->old_name &&
ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
+ flush_attributes = 1;
style nit - these lines are very long compared to 80 characters
}
else {
if (state->apply_verbosity > verbosity_normal)
@@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,
if (state->summary && state->apply_verbosity > verbosity_silent)
summary_patch_list(list);
+ if (flush_attributes)
+ reset_parsed_attributes();
end:
free_patch_list(list);
strbuf_release(&buf);
diff --git a/convert.c b/convert.c
index 94ff837649..030e9b81b9 100644
--- a/convert.c
+++ b/convert.c
@@ -1293,10 +1293,11 @@ struct conv_attrs {
const char *working_tree_encoding; /* Supported encoding or default
encoding if NULL */
};
+static struct attr_check *check;
I was concerned about the impact adding a file global if we ever want to
multi-thread this for submodules, but looking through the file there are
a couple of others already so this isn't creating a new problem.
Best Wishes
Phillip
+
static void convert_attrs(const struct index_state *istate,
struct conv_attrs *ca, const char *path)
{
- static struct attr_check *check;
struct attr_check_item *ccheck = NULL;
if (!check) {
@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state
*istate,
ca->crlf_action = CRLF_AUTO_INPUT;
}
+void reset_parsed_attributes(void)
+{
+ attr_check_free(check);
+ check = NULL;
+}
+
int would_convert_to_git_filter_fd(const struct index_state *istate, const
char *path)
{
struct conv_attrs ca;
diff --git a/convert.h b/convert.h
index 831559f10d..3710969d43 100644
--- a/convert.h
+++ b/convert.h
@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state
*istate,
int would_convert_to_git_filter_fd(const struct index_state *istate,
const char *path);
+/*
+ * Reset the internal list of attributes used by convert_to_git and
+ * convert_to_working_tree.
+ */
+void reset_parsed_attributes(void);
+
/*
*
* Streaming conversion support
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80b23fd326..23469cc789 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -301,6 +301,42 @@ test_expect_success 'rebase --am and --show-current-patch'
'
)
'
+test_expect_success 'rebase --am and .gitattributes' '
+ test_create_repo attributes &&
+ (
+ cd attributes &&
+ test_commit init &&
+ git config filter.test.clean "sed -e '\''s/smudged/clean/g'\''"
&&
+ git config filter.test.smudge "sed -e '\''s/clean/smudged/g'\''"
&&
+
+ test_commit second &&
+
Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.
Hi Ben On 18/08/2019 10:53, Ben Wijen wrote: I found an issue with git rebase --autostash with an dirty worktree/index. It seems the currently active branch is moved, which is not correct. I'm not sure I understand what you mean by "the currently active branch is moved". 'git rebase --autostash ' should checkout (presumably stashing any unstaged and uncommitted changes first) and then do rebase - what's it doing instead? Best Wishes Phillip The following patches contain both a test and a fix. Ben Wijen (2): t3420: never change upstream branch rebase.c: make sure current branch isn't moved when autostashing builtin/rebase.c| 18 ++ t/t3420-rebase-autostash.sh | 13 + 2 files changed, 15 insertions(+), 16 deletions(-)
[PATCH 0/3] rebase -i: always update HEAD before rewording
This series contains a couple of patches to make the C version of rebase --interactive behave more like the scripted version. The third patch is not strictly related to the first two but is included here to avoid merge conflicts. Phillip Wood (3): rebase -i: always update HEAD before rewording rebase -i: check for updated todo after squash and reword sequencer: simplify root commit creation sequencer.c| 130 + t/t3404-rebase-interactive.sh | 16 +++- t/t3429-rebase-edit-todo.sh| 21 - t/t7505-prepare-commit-msg-hook.sh | 8 +- t/t7505/expected-rebase-i | 3 +- 5 files changed, 75 insertions(+), 103 deletions(-) base-commit: ff66981f4593aec0f3b3eeace0eacb7dbe44fd8c Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-312%2Fphillipwood%2Fwip%2Frebase-reword-update-head-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-312/phillipwood/wip/rebase-reword-update-head-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/312 -- gitgitgadget
[PATCH 1/3] rebase -i: always update HEAD before rewording
From: Phillip Wood
If the user runs git log while rewording a commit it is confusing if
sometimes we're amending the commit that's being reworded and at other
times we're creating a new commit depending on whether we could
fast-forward or not[1]. Fix this inconsistency by always committing the
picked commit and then running 'git commit --amend' to do the reword.
The first commit is performed by the sequencer without forking git
commit and does not impact on the speed of rebase. In a test rewording
100 commits with
GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i s/pick/reword/' \
../bin-wrappers/git rebase -i --root
and taking the best of three runs the current master took
957ms and with this patch it took 961ms.
This change fixes rewording the new root commit when rearranging commits
with --root.
Note that the new code no longer updates CHERRY_PICK_HEAD after creating
a root commit - I'm not sure why the old code was that creating that ref
after a successful commit, everywhere else it is removed after a
successful commit.
[1]
https://public-inbox.org/git/xmqqlfvu4be3@gitster-ct.c.googlers.com/T/#m133009cb91cf0917bcf667300f061178be56680a
Reported-by: SZEDER Gábor
Signed-off-by: Phillip Wood
---
sequencer.c| 19 +++
t/t3404-rebase-interactive.sh | 16 +---
t/t7505-prepare-commit-msg-hook.sh | 8 +---
t/t7505/expected-rebase-i | 3 ++-
4 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 34ebf8ed94..085777f4a1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -986,12 +986,10 @@ static int run_git_commit(struct repository *r,
strbuf_release(&msg);
strbuf_release(&script);
- if (!res) {
- update_ref(NULL, "CHERRY_PICK_HEAD", &root_commit, NULL,
- REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);
+ if (!res)
res = update_ref(NULL, "HEAD", &root_commit, NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
- }
+
return res < 0 ? error(_("writing root commit")) : 0;
}
@@ -1785,7 +1783,7 @@ static int do_pick_commit(struct repository *r,
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
- int res, unborn = 0, allow;
+ int res, unborn = 0, reword = 0, allow;
if (opts->no_commit) {
/*
@@ -1855,7 +1853,7 @@ static int do_pick_commit(struct repository *r,
opts);
if (res || command != TODO_REWORD)
goto leave;
- flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG;
+ reword = 1;
msg_file = NULL;
goto fast_forward_edit;
}
@@ -1913,7 +1911,7 @@ static int do_pick_commit(struct repository *r,
}
if (command == TODO_REWORD)
- flags |= EDIT_MSG | VERIFY_MSG;
+ reword = 1;
else if (is_fixup(command)) {
if (update_squash_messages(r, command, commit, opts))
return -1;
@@ -1997,13 +1995,18 @@ static int do_pick_commit(struct repository *r,
} else if (allow)
flags |= ALLOW_EMPTY;
if (!opts->no_commit) {
-fast_forward_edit:
if (author || command == TODO_REVERT || (flags & AMEND_MSG))
res = do_commit(r, msg_file, author, opts, flags);
else
res = error(_("unable to parse commit author"));
+ if (!res && reword)
+fast_forward_edit:
+ res = run_git_commit(r, NULL, opts, EDIT_MSG |
+VERIFY_MSG | AMEND_MSG |
+(flags & ALLOW_EMPTY));
}
+
if (!res && final_fixup) {
unlink(rebase_path_fixup_msg());
unlink(rebase_path_squash_msg());
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ff..d2f1d5bd23 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1016,9 +1016,9 @@ test_expect_success 'rebase -i --root fixup root commit' '
test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
'
-test_expect_success 'rebase -i --root reword root commit' '
+test_expect_success 'rebase -i --root reword original root commit' '
test_when_finished "test_might_fail git rebase --abort" &&
- git checkout -b reword-root-branch master &&
+ git checkout -b reword-original-root-branch master &&
set_fake_editor &a
[PATCH 3/3] sequencer: simplify root commit creation
From: Phillip Wood
Adapt try_to_commit() to create a new root commit rather than special
casing this in run_git_commit(). The significantly reduces the amount of
special case code for creating the root commit and reduces the number of
commit code paths we have to worry about.
Signed-off-by: Phillip Wood
---
sequencer.c | 75 +++--
1 file changed, 4 insertions(+), 71 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index a13e1a7466..758c780790 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -869,34 +869,6 @@ static char *get_author(const char *message)
return NULL;
}
-/* Read author-script and return an ident line (author timestamp) */
-static const char *read_author_ident(struct strbuf *buf)
-{
- struct strbuf out = STRBUF_INIT;
- char *name, *email, *date;
-
- if (read_author_script(rebase_path_author_script(),
- &name, &email, &date, 0))
- return NULL;
-
- /* validate date since fmt_ident() will die() on bad value */
- if (parse_date(date, &out)){
- warning(_("invalid date format '%s' in '%s'"),
- date, rebase_path_author_script());
- strbuf_release(&out);
- return NULL;
- }
-
- strbuf_reset(&out);
- strbuf_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, 0));
- strbuf_swap(buf, &out);
- strbuf_release(&out);
- free(name);
- free(email);
- free(date);
- return buf->buf;
-}
-
static const char staged_changes_advice[] =
N_("you have staged changes in your working tree\n"
"If these changes are meant to be squashed into the previous commit, run:\n"
@@ -954,45 +926,6 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
- if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
- struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
- const char *author = NULL;
- struct object_id root_commit, *cache_tree_oid;
- int res = 0;
-
- if (is_rebase_i(opts)) {
- author = read_author_ident(&script);
- if (!author) {
- strbuf_release(&script);
- return -1;
- }
- }
-
- if (!defmsg)
- BUG("root commit without message");
-
- if (!(cache_tree_oid = get_cache_tree_oid(r->index)))
- res = -1;
-
- if (!res)
- res = strbuf_read_file(&msg, defmsg, 0);
-
- if (res <= 0)
- res = error_errno(_("could not read '%s'"), defmsg);
- else
- res = commit_tree(msg.buf, msg.len, cache_tree_oid,
- NULL, &root_commit, author,
- opts->gpg_sign);
-
- strbuf_release(&msg);
- strbuf_release(&script);
- if (!res)
- res = update_ref(NULL, "HEAD", &root_commit, NULL, 0,
-UPDATE_REFS_MSG_ON_ERR);
-
- return res < 0 ? error(_("writing root commit")) : 0;
- }
-
cmd.git_cmd = 1;
if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
@@ -1376,7 +1309,7 @@ static int try_to_commit(struct repository *r,
struct object_id *oid)
{
struct object_id tree;
- struct commit *current_head;
+ struct commit *current_head = NULL;
struct commit_list *parents = NULL;
struct commit_extra_header *extra = NULL;
struct strbuf err = STRBUF_INIT;
@@ -1411,7 +1344,8 @@ static int try_to_commit(struct repository *r,
}
parents = copy_commit_list(current_head->parents);
extra = read_commit_extra_headers(current_head, exclude_gpgsig);
- } else if (current_head) {
+ } else if (current_head &&
+ (!(flags & CREATE_ROOT_COMMIT) || (flags & AMEND_MSG))) {
commit_list_insert(current_head, &parents);
}
@@ -1488,8 +1422,7 @@ static int do_commit(struct repository *r,
{
int res = 1;
- if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG) &&
- !(flags & CREATE_ROOT_COMMIT)) {
+ if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG)) {
struct object_id oid;
struct strbuf sb = STRBUF_INIT;
--
gitgitgadget
[PATCH 2/3] rebase -i: check for updated todo after squash and reword
From: Phillip Wood
While a rebase is stopped for the user to edit a commit message it can
be convenient for them to also edit the todo list. The scripted version
of rebase supported this but the C version does not. We already check to
see if the todo list has been updated by an exec command so extend this
to rewords and squashes. It only costs a single stat call to do this so
it should not affect the speed of the rebase (especially as it has just
stopped for the user to edit a message)
Note that for squashes the editor may be opened on a different pick to
the squash itself as we edit the message at the end of a chain fixups
and squashes.
Signed-off-by: Phillip Wood
---
sequencer.c | 42 -
t/t3429-rebase-edit-todo.sh | 21 ++-
2 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 085777f4a1..a13e1a7466 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1773,7 +1773,7 @@ static int do_pick_commit(struct repository *r,
enum todo_command command,
struct commit *commit,
struct replay_opts *opts,
- int final_fixup)
+ int final_fixup, int *check_todo)
{
unsigned int flags = opts->edit ? EDIT_MSG : 0;
const char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);
@@ -1999,11 +1999,14 @@ static int do_pick_commit(struct repository *r,
res = do_commit(r, msg_file, author, opts, flags);
else
res = error(_("unable to parse commit author"));
- if (!res && reword)
+ *check_todo = !!(flags & EDIT_MSG);
+ if (!res && reword) {
fast_forward_edit:
res = run_git_commit(r, NULL, opts, EDIT_MSG |
VERIFY_MSG | AMEND_MSG |
(flags & ALLOW_EMPTY));
+ *check_todo = 1;
+ }
}
@@ -3821,6 +3824,7 @@ static int pick_commits(struct repository *r,
while (todo_list->current < todo_list->nr) {
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
+ int check_todo = 0;
if (save_todo(todo_list, opts))
return -1;
@@ -3859,7 +3863,8 @@ static int pick_commits(struct repository *r,
command_to_string(item->command), NULL),
1);
res = do_pick_commit(r, item->command, item->commit,
- opts, is_final_fixup(todo_list));
+opts, is_final_fixup(todo_list),
+&check_todo);
if (is_rebase_i(opts) && res < 0) {
/* Reschedule */
advise(_(rescheduled_advice),
@@ -3916,7 +3921,6 @@ static int pick_commits(struct repository *r,
} else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(arg + item->arg_len);
int saved = *end_of_arg;
- struct stat st;
if (!opts->verbose)
term_clear_line();
@@ -3927,17 +3931,8 @@ static int pick_commits(struct repository *r,
if (res) {
if (opts->reschedule_failed_exec)
reschedule = 1;
- } else if (stat(get_todo_path(opts), &st))
- res = error_errno(_("could not stat '%s'"),
- get_todo_path(opts));
- else if (match_stat_data(&todo_list->stat, &st)) {
- /* Reread the todo file if it has changed. */
- todo_list_release(todo_list);
- if (read_populate_todo(r, todo_list, opts))
- res = -1; /* message was printed */
- /* `current` will be incremented below */
- todo_list->current = -1;
}
+ check_todo = 1;
} else if (item->command == TODO_LABEL) {
if ((res = do_label(r, arg, item->arg_len)))
reschedule = 1;
@@ -3973,6 +3968,20 @@ static int pick_commits(struct repository *r,
item->commit,
Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
On 14/08/2019 20:33, Junio C Hamano wrote: Phillip Wood writes: That's an important distinction, particularly if GIT_COMMITTER_DATE is set in the environment - are we aiming to have the author and committer dates match or are we just resetting the author date to now? Rohit - do you know which --ignore-date does in the am based rebase? The purpose "am --ignore-date" was to ignore "Date:" that came from the patch message, overriding it with the current date. It might have become harder to read in the C version, but "git show v2.0.0:git-am.sh" would be an easier way to read how "--ignore-date" wanted to behave. Thanks for the pointer, looking at the code I agree that --reset-author-date would be a better alias. If GIT_COMMITTER_DATE is not set then the author and committer dates match, otherwise the author date is reset to the current time. The code also shows how we should be combining --ignore-date and --committer-date-is-author-date. Best Wishes Phillip
Re: minor interactive rebase regression: HEAD points to wrong commit while rewording
On 14/08/2019 22:20, SZEDER Gábor wrote: On Mon, Aug 12, 2019 at 09:28:52PM +0100, Phillip Wood wrote: Save the updated commit message, and after the editor opens up the third commit's log message, check again where HEAD is pointing to now: ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG third ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1 c3db735 (HEAD) second - updated As second has been updated the sequencer cannot fast-forward to third so it cherry-picks third and then passes --edit when it runs 'git commit' to commit the cherry-pick. HEAD is updated once the reworded commit has been created. I think the scripted rebase always ran cherry-pick and then ran 'commit --amend' afterwards if the commit was being reworded. The C implementation is more efficient as it avoids creating an redundant commit but has the side effect that HEAD is not updated before the reword which was surprising here. I'm not sure about this more efficient thing. I mean, 'git rebase' is about to launch the editor to let the user type something... Compared to that the time spent in creating an extra commit object is surely negligible. I changed the sequencer to always commit the cherry-pick and then run 'git commit --amend' for rewords [1]. Running time env GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i s/pick/reword/' ../bin-wrappers/git rebase -i --root over 100 commits I cannot see any real difference in the timings between master and that branch. Any difference is within the variation of the times of multiple runs. The change also fixes a bug when rewording a re-arranged root commit. Best Wishes Phillip [1] https://github.com/phillipwood/git/commits/wip/rebase-reword-update-head
Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
On 13/08/2019 22:45, Junio C Hamano wrote: Rohit Ashiwal writes: --ignore-date:: - This flag is passed to 'git am' to change the author date - of the rebased commits (see linkgit:git-am[1]). + Instead of using the given author date, re-set it to the value + same as committer (current) date. This implies --force-rebase. s/to the value same as .* date\./the current time./; The more important thing is that we record the current timestamp as the author date; that timestamp being very close to the committer date of the resulting commit is a mere consequence of the fact that we use the current time for committer date and much less important. That's an important distinction, particularly if GIT_COMMITTER_DATE is set in the environment - are we aiming to have the author and committer dates match or are we just resetting the author date to now? Rohit - do you know which --ignore-date does in the am based rebase? Best Wishes Phillip Again, I think reset-author-date would be a better synonym to this one.
Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
Hi Junio & Rohit
On 13/08/2019 18:21, Junio C Hamano wrote:
Phillip Wood writes:
+static void push_dates(struct child_process *child)
+{
+ time_t now = time(NULL);
+ struct strbuf date = STRBUF_INIT;
+
+ strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
+ argv_array_pushf(&child->args, "--date=%s", date.buf);
it doesn't matter but it might have been nicer to set both dates the
same way in the environment.
+ argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
We can see that this date string lacks timezone information, which
would likely fall back to whatever timezone the user is in. Is that
what we want? I am guessing it is, as we are dealing with "now"
timestamp, but wanted to double check.
I think we probably want to use the local timezone as you suggest
+ if (opts->ignore_date) {
+ if (!author)
+ BUG("ignore-date can only be used with "
+ "rebase, which must set the author "
+ "before committing the tree");
+ ignore_author_date(&author);
Is this leaking the old author? I'd rather see
tmp_author = ignore_author_date(author);
free(author);
author = tmp_author;
Or make sure ignore_author_date() does not leak the original, when
it rewrites its parameter.
But I have a larger question at the higher design level. Why are we
passing a single string "author" around, instead of parsed and split
fields, like tuple? That would allow us
to replace only the time part a lot more easily. Would it make the
other parts of the code more cumbersome (I didn't check---and if
that is the case, then that is a valid reason why we want to stick
to the current "a single string 'author' keeps the necessary info
for the 4-tuple" design).
It's a bit of a mess at the moment. There are places where we want a
single author data string when calling commit_tree_extended(), and other
places where we want to set the name, email and date in the environment
when running 'git commit'. For the latter case we use a file which we
read and write as the C version just follows what the shell script did.
I think carrying round a tuple would be the
sensible way to go and we can build the author string when we need it.
Doing that would hopefully eliminate having to read and write the author
file so much. I haven't looked at how difficult it would be to change
the existing code to do that. We should also really carry the commit
message around in a variable and only write it to a file if a pick fails
or we are editing the message and running 'git commit'. If we're just
using commit_tree_extended() there is no need to be writing the message
to a file and then reading it back in again later.
Best Wishes
Phillip
+ }
res = commit_tree(msg.buf, msg.len, cache_tree_oid,
NULL, &root_commit, author,
opts->gpg_sign);
+ }
strbuf_release(&msg);
strbuf_release(&script);
@@ -1053,6 +1087,8 @@ static int run_git_commit(struct repository *r,
argv_array_push(&cmd.args, "--amend");
if (opts->gpg_sign)
argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
+ if (opts->ignore_date)
+ push_dates(&cmd);
if (defmsg)
argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
else if (!(flags & EDIT_MSG))
@@ -1515,6 +1551,11 @@ static int try_to_commit(struct repository *r,
reset_ident_date();
+if (opts->ignore_date) {
+ ignore_author_date(&author);
+ free(author_to_free);
Where is author_to_free set? We should always free the old author, see
above.
Or require callers to pass a free()able memory to ignore_author_date()
and have the callee free the original?
Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
On 13/08/2019 18:06, Junio C Hamano wrote: Phillip Wood writes: [...] + + strbuf_addf(&date, "@%s",ident.date_begin); I think we should use %s.* and ident.date_end to be sure we getting what we want. Your version is OK if the author is formatted correctly but I'm uneasy about relying on that when we can get the verified end from ident. If the author line is not formatted correctly, split_ident_line() would notice and return NULL in these fields, I think (in other words, my take on the call to split_ident_line() above is not necessarily done in order to "split", but primarily to validate that the line is formatted correctly---and find the beginning of the timestamp field). I just had a read through split_ident_line() and it looks to me like it will ignore any junk after the timezone. So long as it sees '+' or '-' followed by at least one digit it will use that for the time zone and return success regardless of what follows it so I think we want to pay attention to the end data it returns for the date and timezone. But your "pay attention to date_end" raises an interesting point. The string that follows ident.date_begin would be a large integer (i.e. number of seconds since epoch), a SP, a positive or negative sign (i.e. east or west of GMT), 4 digits (i.e. timezone offset), so if you want to leave something like "@1544328981" in the buffer, you need to stop at .date_end to omit the timezone information. On the other hand, if you do want the timezone information as well (which I think is the case for this codepath), you should not stop at there and have something like "@1544328981 +0900", the code as written would give better result. Good point, I had forgotten that split_ident_line() returned separate fields for the date and timezone. I agree that we want the timezone here too. I I think it would be a good idea to beef up the tests to use a non default timezone to check that we are actually setting it correctly. Best Wishes Phillip
Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
On 12/08/2019 20:42, Rohit Ashiwal wrote:
rebase am already has this flag to "lie" about the committer date
by changing it to the author date. Let's add the same for
interactive machinery.
Signed-off-by: Rohit Ashiwal
---
Documentation/git-rebase.txt| 8 +++-
builtin/rebase.c| 20 ++---
sequencer.c | 57 -
sequencer.h | 1 +
t/t3422-rebase-incompatible-options.sh | 1 -
t/t3433-rebase-options-compatibility.sh | 19 +
6 files changed, 96 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 28e5e08a83..697ce8e6ff 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -383,8 +383,12 @@ default is `--no-fork-point`, otherwise the default is
`--fork-point`.
See also INCOMPATIBLE OPTIONS below.
--committer-date-is-author-date::
+ Instead of recording the time the rebased commits are
+ created as the committer date, reuse the author date
+ as the committer date. This implies --force-rebase.
+
--ignore-date::
- These flags are passed to 'git am' to easily change the dates
+ This flag is passed to 'git am' to change the author date
Very minor nit-pick. This has lost the plural for 'dates'. I'm not sure
this is correct as there will be more than one date because the commits
will have different dates. If it had said 'change the date of each
rebased commit' then the singular would definitely be right but as it
stands I'm not sure this is an improvement.
Best Wishes
Phillip
of the rebased commits (see linkgit:git-am[1]).
+
See also INCOMPATIBLE OPTIONS below.
@@ -522,7 +526,6 @@ INCOMPATIBLE OPTIONS
The following options:
- * --committer-date-is-author-date
* --ignore-date
* --whitespace
* -C
@@ -548,6 +551,7 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
* --preserve-merges and --ignore-whitespace
+ * --preserve-merges and --committer-date-is-author-date
* --rebase-merges and --ignore-whitespace
* --rebase-merges and --strategy
* --rebase-merges and --strategy-option
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ab1bbb78ee..b1039f8db0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -82,6 +82,7 @@ struct rebase_options {
int ignore_whitespace;
char *gpg_sign_opt;
int autostash;
+ int committer_date_is_author_date;
char *cmd;
int allow_empty_message;
int rebase_merges, rebase_cousins;
@@ -114,6 +115,8 @@ static struct replay_opts get_replay_opts(const struct
rebase_options *opts)
replay.allow_empty_message = opts->allow_empty_message;
replay.verbose = opts->flags & REBASE_VERBOSE;
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
+ replay.committer_date_is_author_date =
+ opts->committer_date_is_author_date;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
replay.strategy = opts->strategy;
@@ -533,6 +536,9 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
warning(_("--[no-]rebase-cousins has no effect without "
"--rebase-merges"));
+ if (opts.committer_date_is_author_date)
+ opts.flags |= REBASE_FORCE;
+
return !!run_rebase_interactive(&opts, command);
}
@@ -981,6 +987,8 @@ static int run_am(struct rebase_options *opts)
if (opts->ignore_whitespace)
argv_array_push(&am.args, "--ignore-whitespace");
+ if (opts->committer_date_is_author_date)
+ argv_array_push(&opts->git_am_opts,
"--committer-date-is-author-date");
if (opts->action && !strcmp("continue", opts->action)) {
argv_array_push(&am.args, "--resolved");
argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1424,9 +1432,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
OPT_BOOL(0, "signoff", &options.signoff,
N_("add a Signed-off-by: line to each commit")),
- OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
- &options.git_am_opts, NULL,
- N_("passed to 'git am'"), PARSE_OPT_NOARG),
+ OPT_BOOL(0, "committer-date-is-author-date",
+&options.committer_date_is_author_date,
Re: [GSoC][PATCH v2 4/6] sequencer: rename amend_author to author_to_rename
Hi Rohit
On 12/08/2019 20:42, Rohit Ashiwal wrote:
The purpose of amend_author was to free() the malloc()'d string
obtained from get_author() while amending a commit. But we can
also use the variable to free() the author at our convenience.
Rename it to convey this meaning.
Thanks for rewording this
Best Wishes
Phillip
Signed-off-by: Rohit Ashiwal
---
sequencer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fbc0ed0cad..e186136ccc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1424,7 +1424,7 @@ static int try_to_commit(struct repository *r,
struct commit_extra_header *extra = NULL;
struct strbuf err = STRBUF_INIT;
struct strbuf commit_msg = STRBUF_INIT;
- char *amend_author = NULL;
+ char *author_to_free = NULL;
const char *hook_commit = NULL;
enum commit_msg_cleanup_mode cleanup;
int res = 0;
@@ -1445,7 +1445,7 @@ static int try_to_commit(struct repository *r,
strbuf_addstr(msg, orig_message);
hook_commit = "HEAD";
}
- author = amend_author = get_author(message);
+ author = author_to_free = get_author(message);
unuse_commit_buffer(current_head, message);
if (!author) {
res = error(_("unable to parse commit author"));
@@ -1534,7 +1534,7 @@ static int try_to_commit(struct repository *r,
free_commit_extra_headers(extra);
strbuf_release(&err);
strbuf_release(&commit_msg);
- free(amend_author);
+ free(author_to_free);
return res;
}
Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
L(0, "ignore-whitespace", &options.ignore_whitespace,
@@ -1705,13 +1712,15 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
state_dir_base, cmd_live_rebase, buf.buf);
}
- if (options.committer_date_is_author_date)
+ if (options.ignore_date)
+ options.committer_date_is_author_date = 0;
We should probably print an error if the user gives
--committer-date-is-author-date and --author-date-is-committer-date (and
in cmd_rebase__interactive() if we keep this option there)
+ if (options.committer_date_is_author_date ||
+ options.ignore_date)
options.flags |= REBASE_FORCE;
for (i = 0; i < options.git_am_opts.argc; i++) {
const char *option = options.git_am_opts.argv[i], *p;
- if (!strcmp(option, "--ignore-date") ||
- !strcmp(option, "--whitespace=fix") ||
+ if (!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
else if (skip_prefix(option, "-C", &p)) {
diff --git a/sequencer.c b/sequencer.c
index e186136ccc..aecd9b4ad8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -148,6 +148,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
static GIT_PATH_FUNC(rebase_path_cdate_is_adate,
"rebase-merge/cdate_is_adate")
+static GIT_PATH_FUNC(rebase_path_ignore_date, "rebase-merge/ignore_date")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
@@ -919,6 +920,31 @@ static const char *read_author_ident(struct strbuf *buf)
return buf->buf;
}
+static void ignore_author_date(const char **author)
+{
+ int len = strlen(*author);
+ struct ident_split ident;
+ struct strbuf new_author = STRBUF_INIT;
+
+ split_ident_line(&ident, *author, len);
+ len = ident.mail_end - ident.name_begin + 1;
+
+ strbuf_addf(&new_author, "%.*s", len, *author);
+ datestamp(&new_author);
+ *author = strbuf_detach(&new_author, NULL);
+}
It's good to see this using the indet api. I think this would be nicer
if it took a char* returned the new author rather than changing the
function argument, particularly as it does not free the string that is
passed in so the ownership is unclear.
+
+static void push_dates(struct child_process *child)
+{
+ time_t now = time(NULL);
+ struct strbuf date = STRBUF_INIT;
+
+ strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
+ argv_array_pushf(&child->args, "--date=%s", date.buf);
it doesn't matter but it might have been nicer to set both dates the
same way in the environment.
+ argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
+ strbuf_release(&date);
+}
+
static const char staged_changes_advice[] =
N_("you have staged changes in your working tree\n"
"If these changes are meant to be squashed into the previous commit, run:\n"
@@ -1020,10 +1046,18 @@ static int run_git_commit(struct repository *r,
if (res <= 0)
res = error_errno(_("could not read '%s'"), defmsg);
- else
+ else {
+ if (opts->ignore_date) {
+ if (!author)
+ BUG("ignore-date can only be used with "
+ "rebase, which must set the author "
+ "before committing the tree");
+ ignore_author_date(&author);
Is this leaking the old author? I'd rather see
tmp_author = ignore_author_date(author);
free(author);
author = tmp_author;
+ }
res = commit_tree(msg.buf, msg.len, cache_tree_oid,
NULL, &root_commit, author,
opts->gpg_sign);
+ }
strbuf_release(&msg);
strbuf_release(&script);
@@ -1053,6 +1087,8 @@ static int run_git_commit(struct repository *r,
argv_array_push(&cmd.args, "--amend");
if (opts->gpg_sign)
argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
+ if (opts->ignore_date)
+ push_dates(&cmd);
if (
Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
On 13/08/2019 11:38, Phillip Wood wrote:
Hi Rohit
[...]
@@ -964,6 +976,25 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
+ if (opts->committer_date_is_author_date) {
+ size_t len;
+ int res = -1;
+ struct strbuf datebuf = STRBUF_INIT;
+ char *date = read_author_date_or_null();
You must always check the return value of functions that might return
NULL. In this case we should return an error as you do in try_to
_commit() later
+
+ strbuf_addf(&datebuf, "@%s", date);
GNU printf() will add something like '(null)' to the buffer if you pass
a NULL pointer so I don't think we can be sure that this will not
increase the length of the buffer if date is NULL.
I should have added that passing NULL to snprintf() and friends is going
to be undefined behavior anyway so you shouldn't do it for that reason
alone.
Best Wishes
Phillip
An explicit check
above would be much clearer as well rather than checking len later. What
happens if you don't add the '@' at the beginning? (I'm don't know much
about git's date handling)
+ free(date);
+
+ date = strbuf_detach(&datebuf, &len);
+
+ if (len > 1)
+ res = setenv("GIT_COMMITTER_DATE", date, 1);
+
+ free(date);
+
+ if (res)
+ return -1;
+ }
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
const char *author = NULL;
@@ -1400,7 +1431,6 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
-
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
const char *out_enc = get_commit_output_encoding();
@@ -1427,6 +1457,21 @@ static int try_to_commit(struct repository *r,
commit_list_insert(current_head, &parents);
}
+ if (opts->committer_date_is_author_date) {
+ int len = strlen(author);
+ struct ident_split ident;
+ struct strbuf date = STRBUF_INIT;
+
+ split_ident_line(&ident, author, len);
+
+ if (!ident.date_begin)
+ return error(_("corrupted author without date
information"));
We return an error if we cannot get the date - this is exactly what we
should be doing above. It is also great to see a single version of this
being used whether or not we are amending.
+
+ strbuf_addf(&date, "@%s",ident.date_begin);
I think we should use %s.* and ident.date_end to be sure we getting what
we want. Your version is OK if the author is formatted correctly but I'm
uneasy about relying on that when we can get the verified end from ident.
Best Wishes
Phillip
+ setenv("GIT_COMMITTER_DATE", date.buf, 1);
+ strbuf_release(&date);
+ }
+
if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
res = error(_("git write-tree failed to write a tree"));
goto out;
@@ -2542,6 +2587,11 @@ static int read_populate_opts(struct
replay_opts *opts)
opts->signoff = 1;
}
+ if (file_exists(rebase_path_cdate_is_adate())) {
+ opts->allow_ff = 0;
+ opts->committer_date_is_author_date = 1;
+ }
+
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;
@@ -2624,6 +2674,8 @@ int write_basic_state(struct replay_opts *opts,
const char *head_name,
write_file(rebase_path_gpg_sign_opt(), "-S%s\n",
opts->gpg_sign);
if (opts->signoff)
write_file(rebase_path_signoff(), "--signoff\n");
+ if (opts->committer_date_is_author_date)
+ write_file(rebase_path_cdate_is_adate(), "%s", "");
if (opts->reschedule_failed_exec)
write_file(rebase_path_reschedule_failed_exec(), "%s", "");
@@ -3821,7 +3873,8 @@ static int pick_commits(struct repository *r,
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
- opts->record_origin || opts->edit));
+ opts->record_origin || opts->edit ||
+ opts->committer_date_is_author_date));
if (read_and_refresh_cache(r, opts))
return -1;
diff --git a/sequencer.h b/sequencer.h
index 6704acbb9c..e3881e9275 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -43,6 +43,7 @@ struct replay_opts {
int verbose;
int quiet;
int reschedule_failed_exec;
+ int committer_date_is_author_date;
int mainline;
diff --git a/t/t3422-rebase-incompatible-options.sh
b/t/t3422-rebase-incompatible-op
Re: [GSoC][PATCH v2 1/6] rebase -i: add --ignore-whitespace flag
Hi Rohit
Thanks for the re-roll
On 12/08/2019 20:42, Rohit Ashiwal wrote:
There are two backends available for rebasing, viz, the am and the
interactive. Naturally, there shall be some features that are
implemented in one but not in the other. One such flag is
--ignore-whitespace which indicates merge mechanism to treat lines
with only whitespace changes as unchanged. Wire the interactive
rebase to also understand the --ignore-whitespace flag by
translating it to -Xignore-space-change.
Signed-off-by: Rohit Ashiwal
---
Documentation/git-rebase.txt| 10 +++-
builtin/rebase.c| 29 +--
t/t3422-rebase-incompatible-options.sh | 1 -
t/t3433-rebase-options-compatibility.sh | 65 +
4 files changed, 97 insertions(+), 8 deletions(-)
create mode 100755 t/t3433-rebase-options-compatibility.sh
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6156609cf7..28e5e08a83 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -371,8 +371,13 @@ If either or --root is given on the command
line, then the
default is `--no-fork-point`, otherwise the default is `--fork-point`.
--ignore-whitespace::
+ This flag is either passed to the 'git apply' program
+ (see linkgit:git-apply[1]), or to 'git merge' program
+ (see linkgit:git-merge[1]) as `-Xignore-space-change`,
+ depending on which backend is selected by other options.
I still think this should document what it does rather than how it is
implemented - see my previous comments.
+
--whitespace=::
- These flag are passed to the 'git apply' program
+ This flag is passed to the 'git apply' program
(see linkgit:git-apply[1]) that applies the patch.
+
See also INCOMPATIBLE OPTIONS below.
@@ -520,7 +525,6 @@ The following options:
* --committer-date-is-author-date
* --ignore-date
* --whitespace
- * --ignore-whitespace
* -C
are incompatible with the following options:
@@ -543,6 +547,8 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
+ * --preserve-merges and --ignore-whitespace
+ * --rebase-merges and --ignore-whitespace
* --rebase-merges and --strategy
* --rebase-merges and --strategy-option
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 670096c065..ab1bbb78ee 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -79,6 +79,7 @@ struct rebase_options {
int allow_rerere_autoupdate;
int keep_empty;
int autosquash;
+ int ignore_whitespace;
char *gpg_sign_opt;
int autostash;
char *cmd;
@@ -99,6 +100,7 @@ struct rebase_options {
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
{
+ char *strategy_opts = opts->strategy_opts;
strategy_opts is a shallow copy of opts->strategy_opts - this will cause
problems later
struct replay_opts replay = REPLAY_OPTS_INIT;
replay.action = REPLAY_INTERACTIVE_REBASE;
@@ -114,8 +116,19 @@ static struct replay_opts get_replay_opts(const struct
rebase_options *opts)
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
replay.strategy = opts->strategy;
- if (opts->strategy_opts)
- parse_strategy_opts(&replay, opts->strategy_opts);
+
+ if (opts->ignore_whitespace) {
+ struct strbuf buf = STRBUF_INIT;
+
+ if (strategy_opts)
+ strbuf_addstr(&buf, strategy_opts);
I'd rewrite this as
if (opts->strategy_opts)
strbuf_addstr(&buf, opts->strategy_opts);
without the outer if testing opts->ignore_whitespace
+
then add
if (opts->ignore_whitespace)
here
+ strbuf_addstr(&buf, " --ignore-space-change");
+ free(strategy_opts);
and then you don't need this free() or strategy_opts. At the moment this
has freed opts->stragety_opts which is what we were trying to avoid, in
fact it's slightly worse than the last version because we don't change
opts->strategy_opts to reflect the fact it has been freed. Also the
caller cannot know if it has been freed as it depends what other options
are set.
+ strategy_opts = strbuf_detach(&buf, NULL);
+ }
+ if (strategy_opts)
+ parse_strategy_opts(&replay, strategy_opts);
replace this withe
if (buf.len)
parse_strategy_opts(&replay, buf.buf);
strbuf_release(&buf);
This way we never change opts->strategy_opts and we always release the
temporary copy.
Best Wishes
Phillip
return replay;
}
@@ -511
Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
;, NULL, &options.git_am_opts, N_("n"),
@@ -1697,10 +1705,12 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
state_dir_base, cmd_live_rebase, buf.buf);
}
+ if (options.committer_date_is_author_date)
+ options.flags |= REBASE_FORCE;
+
for (i = 0; i < options.git_am_opts.argc; i++) {
const char *option = options.git_am_opts.argv[i], *p;
- if (!strcmp(option, "--committer-date-is-author-date") ||
- !strcmp(option, "--ignore-date") ||
+ if (!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
diff --git a/sequencer.c b/sequencer.c
index 30d77c2682..fbc0ed0cad 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,6 +147,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
* command-line.
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
@@ -879,6 +880,17 @@ static char *get_author(const char *message)
return NULL;
}
+/* Returns a "date" string that needs to be free()'d by the caller */
+static char *read_author_date_or_null(void)
+{
+ char *date;
+
+ if (read_author_script(rebase_path_author_script(),
+ NULL, NULL, &date, 0))
+ return NULL;
+ return date;
+}
+
/* Read author-script and return an ident line (author timestamp) */
static const char *read_author_ident(struct strbuf *buf)
{
@@ -964,6 +976,25 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
+ if (opts->committer_date_is_author_date) {
+ size_t len;
+ int res = -1;
+ struct strbuf datebuf = STRBUF_INIT;
+ char *date = read_author_date_or_null();
You must always check the return value of functions that might return
NULL. In this case we should return an error as you do in try_to
_commit() later
+
+ strbuf_addf(&datebuf, "@%s", date);
GNU printf() will add something like '(null)' to the buffer if you pass
a NULL pointer so I don't think we can be sure that this will not
increase the length of the buffer if date is NULL. An explicit check
above would be much clearer as well rather than checking len later. What
happens if you don't add the '@' at the beginning? (I'm don't know much
about git's date handling)
+ free(date);
+
+ date = strbuf_detach(&datebuf, &len);
+
+ if (len > 1)
+ res = setenv("GIT_COMMITTER_DATE", date, 1);
+
+ free(date);
+
+ if (res)
+ return -1;
+ }
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
const char *author = NULL;
@@ -1400,7 +1431,6 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
-
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
const char *out_enc = get_commit_output_encoding();
@@ -1427,6 +1457,21 @@ static int try_to_commit(struct repository *r,
commit_list_insert(current_head, &parents);
}
+ if (opts->committer_date_is_author_date) {
+ int len = strlen(author);
+ struct ident_split ident;
+ struct strbuf date = STRBUF_INIT;
+
+ split_ident_line(&ident, author, len);
+
+ if (!ident.date_begin)
+ return error(_("corrupted author without date
information"));
We return an error if we cannot get the date - this is exactly what we
should be doing above. It is also great to see a single version of this
being used whether or not we are amending.
+
+ strbuf_addf(&date, "@%s",ident.date_begin);
I think we should use %s.* and ident.date_end to be sure we getting what
we want. Your version is OK if the author is formatted correctly but I'm
uneasy about relying on that when we can get the verified end from ident.
Best Wishes
Phillip
+ setenv("GIT_COMMITTER_DAT
Re: minor interactive rebase regression: HEAD points to wrong commit while rewording
On 12/08/2019 18:50, SZEDER Gábor wrote: When running interactive rebase to reword a commit message, I would expect that the commit whose message I'm rewording is checked out. This is not quite the case when rewording multiple subsequent commit messages. Let's start with four commits, and start an interactive rebase from the first commit: $ git log --oneline 5835aa1 (HEAD -> master) fourth 64ecc64 third d5fad83 second 384b86f first $ git rebase -i 384b86f Update the instruction sheet to edit the log messages of two subsequent commits: r d5fad83 second r 64ecc64 third pick 5835aa1 fourth Now, after the editor opens up the second commit's log message, start a new terminal and check where HEAD is pointing to: ~/tmp/reword (master|REBASE-i 1/3)$ head -n1 .git/COMMIT_EDITMSG second ~/tmp/reword (master|REBASE-i 1/3)$ git log --oneline -1 d5fad83 (HEAD) second So far so good. Because the sequencer can fast-forwarded to second from first it does that and then run 'commit --amend' to do the reword. Save the updated commit message, and after the editor opens up the third commit's log message, check again where HEAD is pointing to now: ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG third ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1 c3db735 (HEAD) second - updated As second has been updated the sequencer cannot fast-forward to third so it cherry-picks third and then passes --edit when it runs 'git commit' to commit the cherry-pick. HEAD is updated once the reworded commit has been created. I think the scripted rebase always ran cherry-pick and then ran 'commit --amend' afterwards if the commit was being reworded. The C implementation is more efficient as it avoids creating an redundant commit but has the side effect that HEAD is not updated before the reword which was surprising here. I don't think I've ever looked at HEAD while rewording, my shell prompt gets the current pick from .git/rebase-merge/done so does not look at HEAD. While it might seem odd if the user looks at HEAD it's quite nice not to create a new commit only to amend it straight away when it's reworded. We have REBASE_HEAD which always points to the current pick - HEAD is also an unreliable indicator of the current pick if there are conflicts. Best Wishes Phillip As you can see, HEAD still points to the (now rewritten) second commit. It's only HEAD, though: notice the '+' in the git prompt, indicating that both the worktree and index are dirty. And indeed, they both already match the state of the currently reworded, i.e. third, commit: ~/tmp/reword (master +|REBASE-i 2/3)$ cat file third This is good, because even though HEAD has not been updated yet, it already allows users to take a look at the "big picture", i.e. actual file contents, in case the diff included in the commit message template doesn't show enough context. This behavior changed in commit 18633e1a22 (rebase -i: use the rebase--helper builtin, 2017-02-09); prior to that HEAD pointed to the third commit while editing its log message. It's important to reword subsequent commits. When rewording multiple, but non subsequent commits (e.g. reword, pick, reword in the instruction sheet), then HEAD is pointing to the right commits during both rewords.
Re: Incorrect behavior from git checkout --patch
Hi Dave
On 11/08/2019 10:54, Phillip Wood wrote:
Hi Dave
Thanks for the bug report. I've tried to reproduce it on the latest
master, version 2.22.0 and 2.20.1 and am unable to do so.
Doh, I was doing 'git checkout -p HEAD', when I checkout from the index
I can reproduce the bug and it is fixed by the patch I linked to earlier
which will be in git 2.23.0. That will be released soon (we're at
2.23.0.rc2 at the moment)
Thanks again for taking the time to report the bug
Best Wishes
Phillip
I thought it
might be related to a bug I recently fixed[1] but it does not appear to
be. I've appended the files I used below just in case I made a mistake
translating your patches to files that can be used with git checkout.
Best Wishes
Phillip
[1]
https://github.com/git/git/commit/1b074e15d0f976be2bc14f9528874a841c055213#diff-588be9a03d1f7e33db12f186aad5fde9
pre-image
block_one {
line 1
line 2
line 3
2 4 6 8
line 4
line 5
line 6
2 4 6 8
line 7
line 8
line 9
2 4 6 8
line 10
line 11
line 12
}
block_two {
line 1
line 2
line 3
2 5 6 9
line 4
line 5
line 6
2 5 6 9
line 7
line 8
line 9
2 5 6 9
line 10
line 11
line 12
}
post-image
block_one {
line 1
line 1.5
line 2
line 2.5
line 3
2 4 6 8
line 4
line 4.5
line 5
line 5.5
line 6
2 4 6 8
line 7
line 7.5
line 8
line 8.5
line 9
2 4 6 8
line 10
line 10.5
line 11
line 11.5
line 12
}
block_two {
line 1
line 1.5
line 2
line 2.5
line 3
2 5 6 9
line 4
line 4.5
line 5
line 5.5
line 6
2 5 6 9
line 7
line 7.5
line 8
line 8.5
line 9
2 5 6 9
line 10
line 10.5
line 11
line 11.5
line 12
}
On 10/08/2019 21:12, Dave Kaminski wrote:
I am observing git checkout --patch making changes to the wrong lines
of a file.
This is with a clean install of git version 2.20.1 on a debian docker
container (image tag 10.0 which is also "latest" as of this writing).
With a diff that looks like the following:
diff --git a/file.txt b/file.txt
index 868aa22..ea4d786 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
-2 5 6 9
+2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
-2 5 6 9
+2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 5 6 9
+2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
doing a `git checkout --patch -- ./file.txt`, splitting the diff into
hunks, and discarding all of the hunks that begin with numbers, e.g.
@@ -22,3 +32,3 @@
line 3
-2 5 6 9
+2 4 6 8
line 4
the expected state of the file in the working directory is this:
diff --git a/file.txt b/file.txt
index 868aa22..9ab67a1 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 5 6 9
line 4
+line 4.5
line 5
+line 5.5
line 6
2 5 6 9
line 7
+line 7.5
line 8
+line 8.5
line 9
2 5 6 9
line 10
+line 10.5
line 11
+line 11.5
line 12
}
but instead the actual state of the file is this:
diff --git a/file.txt b/file.txt
index 868aa22..76fe65d 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 4 6 8
+2 5 6 9
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 5 6 9
line 4
+line 4.5
line 5
+line 5.5
line 6
2 5 6 9
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 5 6 9
+2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
See the changes between "line 9" and "line 10" in both blocks that are
not correct.
Re: Incorrect behavior from git checkout --patch
Hi Dave
Thanks for the bug report. I've tried to reproduce it on the latest
master, version 2.22.0 and 2.20.1 and am unable to do so. I thought it
might be related to a bug I recently fixed[1] but it does not appear to
be. I've appended the files I used below just in case I made a mistake
translating your patches to files that can be used with git checkout.
Best Wishes
Phillip
[1]
https://github.com/git/git/commit/1b074e15d0f976be2bc14f9528874a841c055213#diff-588be9a03d1f7e33db12f186aad5fde9
pre-image
block_one {
line 1
line 2
line 3
2 4 6 8
line 4
line 5
line 6
2 4 6 8
line 7
line 8
line 9
2 4 6 8
line 10
line 11
line 12
}
block_two {
line 1
line 2
line 3
2 5 6 9
line 4
line 5
line 6
2 5 6 9
line 7
line 8
line 9
2 5 6 9
line 10
line 11
line 12
}
post-image
block_one {
line 1
line 1.5
line 2
line 2.5
line 3
2 4 6 8
line 4
line 4.5
line 5
line 5.5
line 6
2 4 6 8
line 7
line 7.5
line 8
line 8.5
line 9
2 4 6 8
line 10
line 10.5
line 11
line 11.5
line 12
}
block_two {
line 1
line 1.5
line 2
line 2.5
line 3
2 5 6 9
line 4
line 4.5
line 5
line 5.5
line 6
2 5 6 9
line 7
line 7.5
line 8
line 8.5
line 9
2 5 6 9
line 10
line 10.5
line 11
line 11.5
line 12
}
On 10/08/2019 21:12, Dave Kaminski wrote:
I am observing git checkout --patch making changes to the wrong lines of a file.
This is with a clean install of git version 2.20.1 on a debian docker
container (image tag 10.0 which is also "latest" as of this writing).
With a diff that looks like the following:
diff --git a/file.txt b/file.txt
index 868aa22..ea4d786 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
-2 5 6 9
+2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
-2 5 6 9
+2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 5 6 9
+2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
doing a `git checkout --patch -- ./file.txt`, splitting the diff into
hunks, and discarding all of the hunks that begin with numbers, e.g.
@@ -22,3 +32,3 @@
line 3
-2 5 6 9
+2 4 6 8
line 4
the expected state of the file in the working directory is this:
diff --git a/file.txt b/file.txt
index 868aa22..9ab67a1 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 5 6 9
line 4
+line 4.5
line 5
+line 5.5
line 6
2 5 6 9
line 7
+line 7.5
line 8
+line 8.5
line 9
2 5 6 9
line 10
+line 10.5
line 11
+line 11.5
line 12
}
but instead the actual state of the file is this:
diff --git a/file.txt b/file.txt
index 868aa22..76fe65d 100644
--- a/file.txt
+++ b/file.txt
@@ -1,35 +1,51 @@
block_one {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 4 6 8
line 4
+line 4.5
line 5
+line 5.5
line 6
2 4 6 8
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 4 6 8
+2 5 6 9
line 10
+line 10.5
line 11
+line 11.5
line 12
}
block_two {
line 1
+line 1.5
line 2
+line 2.5
line 3
2 5 6 9
line 4
+line 4.5
line 5
+line 5.5
line 6
2 5 6 9
line 7
+line 7.5
line 8
+line 8.5
line 9
-2 5 6 9
+2 4 6 8
line 10
+line 10.5
line 11
+line 11.5
line 12
}
See the changes between "line 9" and "line 10" in both blocks that are
not correct.
Re: [GSoC][PATCHl 5/6] rebase -i: support --ignore-date
On 08/08/2019 12:42, Phillip Wood wrote:
On 06/08/2019 18:36, Rohit Ashiwal wrote:
rebase am already has this flag to "lie" about the author date
by changing it to the committer (current) date. Let's add the same
for interactive machinery.
Signed-off-by: Rohit Ashiwal
---
>> [...[
static const char staged_changes_advice[] =
N_("you have staged changes in your working tree\n"
"If these changes are meant to be squashed into the previous commit,
run:\n"
@@ -985,7 +997,7 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
- if (opts->committer_date_is_author_date &&
+ if (opts->committer_date_is_author_date && !opts->ignore_date &&
setenv_committer_date_to_author_date())
return 1;
We read the author script again just below, can set the committer date
there by parsing the author string, that would mean you could use the
same function that works on an author string in try_to_commit() (this
comment should be on patch 3 I think)
It's a bit more complicated as you've done this to avoid the duplication
in the previous version. I think it should be possible to do it by
reading the author identity upfront (protected by 'if
is_rebase_i(opts)') and processing it appropriately in the two code
paths. (You might need to refactor some of the functions like
read_env_script() to do this.) An alternative would be to refactor
try_to_commit() so that it can create the root commit and delete the
code here that does that.
Best Wishes
Phillip
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
@@ -1013,10 +1025,18 @@ static int run_git_commit(struct repository *r,
if (res <= 0)
res = error_errno(_("could not read '%s'"), defmsg);
- else
+ else {
+ if (opts->ignore_date) {
+ if (!author)
+ BUG("ignore-date can only be used with "
+ "rebase -i, which must set the "
I know it's only a bug message but it's not just 'rebase -i' but 'rebase
-k', 'rebase -m' ... that use this code path, it would be better just to
say 'rebase'
+ "author before committing the tree");
+ ignore_author_date(&author);
+ }
res = commit_tree(msg.buf, msg.len, cache_tree_oid,
NULL, &root_commit, author,
opts->gpg_sign);
+ }
strbuf_release(&msg);
strbuf_release(&script);
@@ -1046,6 +1066,8 @@ static int run_git_commit(struct repository *r,
argv_array_push(&cmd.args, "--amend");
if (opts->gpg_sign)
argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
+ if (opts->ignore_date)
+ argv_array_pushf(&cmd.args, "--date=%ld", time(NULL));
I think this is racy as it only sets the author date, the committer date
may end up being different.
if (defmsg)
argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
else if (!(flags & EDIT_MSG))
@@ -1425,7 +1447,7 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
if (!(flags & AMEND_MSG) && opts->committer_date_is_author_date &&
- setenv_committer_date_to_author_date())
+ !opts->ignore_date && setenv_committer_date_to_author_date())
return -1;
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
@@ -1447,7 +1469,7 @@ static int try_to_commit(struct repository *r,
res = error(_("unable to parse commit author"));
goto out;
}
- if (opts->committer_date_is_author_date) {
+ if (opts->committer_date_is_author_date && !opts->ignore_date) {
If we only handled committer_date_is_author_date in a single place it
wouldn't need to be changed twice in this patch
char *date;
int len = strlen(author);
char *idx = memchr(author, '>', len);
@@ -1507,6 +1529,11 @@ static int try_to_commit(struct repository *r,
reset_ident_date();
+ if (opts->ignore_date) {
+ ignore_author_date(&author);
+ free(author_to_free);
+ author_to_free = (char *)author;
+ }
if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
oid, author, opts->gpg_sign, extra)) {
res = error(_("failed to write commit object"));
@@ -2583,6 +2610,11 @@ static int read_populate_opts(struct
replay_opts *opts)
opts->committer_date_is_
Re: [GSoC][PATCHl 1/6] rebase -i: add --ignore-whitespace flag
Hi Rohit
On 06/08/2019 18:36, Rohit Ashiwal wrote:
There are two backends available for rebasing, viz, the am and the
interactive. Naturally, there shall be some features that are
implemented in one but not in the other. One such flag is
--ignore-whitespace which indicates merge mechanism to treat lines
with only whitespace changes as unchanged. Wire the interactive
rebase to also understand the --ignore-whitespace flag by
translating it to -Xignore-space-change.
Signed-off-by: Rohit Ashiwal
---
Documentation/git-rebase.txt| 10 +++-
builtin/rebase.c| 26 --
t/t3422-rebase-incompatible-options.sh | 1 -
t/t3433-rebase-options-compatibility.sh | 65 +
4 files changed, 95 insertions(+), 7 deletions(-)
create mode 100755 t/t3433-rebase-options-compatibility.sh
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 5e4e927647..85404fea52 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -371,8 +371,13 @@ If either or --root is given on the command
line, then the
default is `--no-fork-point`, otherwise the default is `--fork-point`.
--ignore-whitespace::
+ This flag is either passed to the 'git apply' program
+ (see linkgit:git-apply[1]), or to 'git merge' program
+ (see linkgit:git-merge[1]) as `-Xignore-space-change`,
+ depending on which backend is selected by other options.
I think it would be better to document the effect of this option rather
than the implementation detail. It is confusing at the moment as it
talks about 'git merge' but we don't allow this option with merges.
+
--whitespace=::
- These flag are passed to the 'git apply' program
+ This flag is passed to the 'git apply' program
(see linkgit:git-apply[1]) that applies the patch.
+
See also INCOMPATIBLE OPTIONS below.
@@ -520,7 +525,6 @@ The following options:
* --committer-date-is-author-date
* --ignore-date
* --whitespace
- * --ignore-whitespace
* -C
are incompatible with the following options:
@@ -543,6 +547,8 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
+ * --preserve-merges and --ignore-whitespace
+ * --rebase-merges and --ignore-whitespace
* --rebase-merges and --strategy
* --rebase-merges and --strategy-option
diff --git a/builtin/rebase.c b/builtin/rebase.c
index db6ca9bd7d..3c195ddc73 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -79,6 +79,7 @@ struct rebase_options {
int allow_rerere_autoupdate;
int keep_empty;
int autosquash;
+ int ignore_whitespace;
char *gpg_sign_opt;
int autostash;
char *cmd;
@@ -97,7 +98,7 @@ struct rebase_options {
.git_format_patch_opt = STRBUF_INIT \
}
-static struct replay_opts get_replay_opts(const struct rebase_options *opts)
+static struct replay_opts get_replay_opts(struct rebase_options *opts)
{
struct replay_opts replay = REPLAY_OPTS_INIT;
@@ -114,6 +115,17 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
It's a shame this changes the rebase_options that are passed in, this
function should ideally not modify what is passed in.
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
replay.strategy = opts->strategy;
+
+ if (opts->ignore_whitespace) {
+ struct strbuf buf = STRBUF_INIT;
+
+ if (opts->strategy_opts)
+ strbuf_addstr(&buf, opts->strategy_opts);
+
+ strbuf_addstr(&buf, " --ignore-space-change");
+ free(opts->strategy_opts);
+ opts->strategy_opts = strbuf_detach(&buf, NULL);
+ }
Instead of modifying opts->strategy_opts perhaps we could just use a
temporary variable
Best Wishes
Phillip
if (opts->strategy_opts)
parse_strategy_opts(&replay, opts->strategy_opts);
@@ -511,6 +523,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
+ opts.strategy_opts = xstrdup_or_null(opts.strategy_opts);
+
if (!is_null_oid(&squash_onto))
opts.squash_onto = &squash_onto;
@@ -954,6 +968,8 @@ static int run_am(struct rebase_options *opts)
am.git_cmd = 1;
argv_array_push(&am.args, "am");
+ if (opts->ignore_whitespace)
+ argv_array_push(&am.args, "--ignore-whitespace");
if (opts->ac
Re: [GSoC][PATCHl 6/6] rebase: add --author-date-is-committer-date
Hi Rohit
On 06/08/2019 18:36, Rohit Ashiwal wrote:
The previous commit introduced --ignore-date flag to interactive
rebase, but the name is actually very vague in context of rebase -i
since there are two dates we can work with. Add an alias to convey
the precise purpose.
That's an excellent idea
Best Wishes
Phillip
Signed-off-by: Rohit Ashiwal
---
Documentation/git-rebase.txt | 1 +
builtin/rebase.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a5cdf8518b..bb60426911 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -388,6 +388,7 @@ See also INCOMPATIBLE OPTIONS below.
as the committer date. This implies --force-rebase.
--ignore-date::
+--author-date-is-committer-date::
Lie about the author date by re-setting it to the value
same as committer (current) date. This implies --force-rebase.
+
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7f464fc9ba..a9a42f9ee4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1433,6 +1433,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
OPT_BOOL(0, "committer-date-is-author-date",
&options.committer_date_is_author_date,
N_("make committer date match author date")),
+ OPT_BOOL(0, "author-date-is-committer-date",
&options.ignore_date,
+"ignore author date and use current date"),
OPT_BOOL(0, "ignore-date", &options.ignore_date,
"ignore author date and use current date"),
OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
Re: [GSoC][PATCHl 5/6] rebase -i: support --ignore-date
-1;
if (!(flags & AMEND_MSG) && opts->committer_date_is_author_date &&
- setenv_committer_date_to_author_date())
+ !opts->ignore_date && setenv_committer_date_to_author_date())
return -1;
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
@@ -1447,7 +1469,7 @@ static int try_to_commit(struct repository *r,
res = error(_("unable to parse commit author"));
goto out;
}
- if (opts->committer_date_is_author_date) {
+ if (opts->committer_date_is_author_date && !opts->ignore_date) {
If we only handled committer_date_is_author_date in a single place it
wouldn't need to be changed twice in this patch
char *date;
int len = strlen(author);
char *idx = memchr(author, '>', len);
@@ -1507,6 +1529,11 @@ static int try_to_commit(struct repository *r,
reset_ident_date();
+ if (opts->ignore_date) {
+ ignore_author_date(&author);
+ free(author_to_free);
+ author_to_free = (char *)author;
+ }
if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
oid, author, opts->gpg_sign, extra)) {
res = error(_("failed to write commit object"));
@@ -2583,6 +2610,11 @@ static int read_populate_opts(struct replay_opts *opts)
opts->committer_date_is_author_date = 1;
}
+ if (file_exists(rebase_path_ignore_date())) {
+ opts->allow_ff = 0;
+ opts->ignore_date = 1;
+ }
+
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;
@@ -2667,6 +2699,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
write_file(rebase_path_signoff(), "--signoff\n");
if (opts->committer_date_is_author_date)
write_file(rebase_path_cdate_is_adate(), "%s", "");
+ if (opts->ignore_date)
+ write_file(rebase_path_ignore_date(), "%s", "");
if (opts->reschedule_failed_exec)
write_file(rebase_path_reschedule_failed_exec(), "%s", "");
@@ -3484,6 +3518,9 @@ static int do_merge(struct repository *r,
argv_array_push(&cmd.args, git_path_merge_msg(r));
if (opts->gpg_sign)
argv_array_push(&cmd.args, opts->gpg_sign);
+ if (opts->ignore_date)
+ argv_array_pushf(&cmd.args,
+"GIT_AUTHOR_DATE=%ld", time(NULL));
/* Add the tips to be merged */
for (j = to_merge; j; j = j->next)
@@ -3756,7 +3793,8 @@ static int pick_commits(struct repository *r,
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit ||
- opts->committer_date_is_author_date));
+ opts->committer_date_is_author_date ||
+ opts->ignore_date));
if (read_and_refresh_cache(r, opts))
return -1;
diff --git a/sequencer.h b/sequencer.h
index e6cba468db..73d0515a3e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,7 @@ struct replay_opts {
int quiet;
int reschedule_failed_exec;
int committer_date_is_author_date;
+ int ignore_date;
int mainline;
diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
index ceab48a831..95d99c4b7b 100755
--- a/t/t3433-rebase-options-compatibility.sh
+++ b/t/t3433-rebase-options-compatibility.sh
@@ -81,4 +81,20 @@ test_expect_success '--committer-date-is-author-date works
with interactive back
test_cmp authortime committertime
'
+# Checking for + in author time is enough since default
+# timezone is UTC, but the timezone used while committing
+# sets to +0530.
That sounds potentially fragile but I guess the timezone is unlikely to
change in the future
Best Wishes
Phillip
+test_expect_success '--ignore-date works with am backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ grep "+" authortime
+'
+
+test_expect_success '--ignore-date works with interactive backend' '
+ git commit --amend --date="$GIT_AUTHOR_DATE" &&
+ git rebase --ignore-date -i HEAD^ &&
+ git show HEAD --pretty="format:%ai" >authortime &&
+ grep "+" authortime
+'
test_done
Re: [GSoC][PATCHl 4/6] sequencer: rename amend_author to author_to_rename
Hi Rohit
On 06/08/2019 18:36, Rohit Ashiwal wrote:
The purpose of amend_author was to free() the malloc()'d string
obtained from get_author(). But the name does not actually convey
this purpose. Rename it to something meaningful.
The name was intended to covey that it was only used when amending a
commit, I'm fine with the rename though.
Best Wishes
Phillip
Signed-off-by: Rohit Ashiwal
---
sequencer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 65adf79222..d24a6fd585 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1417,7 +1417,7 @@ static int try_to_commit(struct repository *r,
struct commit_extra_header *extra = NULL;
struct strbuf err = STRBUF_INIT;
struct strbuf commit_msg = STRBUF_INIT;
- char *amend_author = NULL;
+ char *author_to_free = NULL;
const char *hook_commit = NULL;
enum commit_msg_cleanup_mode cleanup;
int res = 0;
@@ -1441,7 +1441,7 @@ static int try_to_commit(struct repository *r,
strbuf_addstr(msg, orig_message);
hook_commit = "HEAD";
}
- author = amend_author = get_author(message);
+ author = author_to_free = get_author(message);
unuse_commit_buffer(current_head, message);
if (!author) {
res = error(_("unable to parse commit author"));
@@ -1526,7 +1526,7 @@ static int try_to_commit(struct repository *r,
free_commit_extra_headers(extra);
strbuf_release(&err);
strbuf_release(&commit_msg);
- free(amend_author);
+ free(author_to_free);
return res;
}
Re: [GSoC][PATCHl 3/6] rebase -i: support --committer-date-is-author-date
@ -1686,10 +1694,12 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
state_dir_base, cmd_live_rebase, buf.buf);
}
+ if (options.committer_date_is_author_date)
+ options.flags |= REBASE_FORCE;
+
for (i = 0; i < options.git_am_opts.argc; i++) {
const char *option = options.git_am_opts.argv[i], *p;
- if (!strcmp(option, "--committer-date-is-author-date") ||
- !strcmp(option, "--ignore-date") ||
+ if (!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
diff --git a/sequencer.c b/sequencer.c
index a2d7b0925e..65adf79222 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,6 +147,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
* command-line.
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
@@ -876,6 +877,17 @@ static char *get_author(const char *message)
return NULL;
}
+/* Returns a "date" string that needs to be free()'d by the caller */
+static char *read_author_date_or_null(void)
+{
+ char *date;
+
+ if (read_author_script(rebase_path_author_script(),
+ NULL, NULL, &date, 0))
+ return NULL;
+ return date;
+}
+
/* Read author-script and return an ident line (author timestamp) */
static const char *read_author_ident(struct strbuf *buf)
{
@@ -904,6 +916,18 @@ static const char *read_author_ident(struct strbuf *buf)
return buf->buf;
}
+static int setenv_committer_date_to_author_date(void)
+{
+ int res = 1;
In the git codebase it is conventional to return -1 for an error
+ char *date = read_author_date_or_null();
+
+ if (date)
+ res = setenv("GIT_COMMITTER_DATE", date, 1);
+
+ free(date);
+ return res;
+}
+
static const char staged_changes_advice[] =
N_("you have staged changes in your working tree\n"
"If these changes are meant to be squashed into the previous commit, run:\n"
@@ -961,6 +985,9 @@ static int run_git_commit(struct repository *r,
{
struct child_process cmd = CHILD_PROCESS_INIT;
+ if (opts->committer_date_is_author_date &&
+ setenv_committer_date_to_author_date())
+ return 1;
-1 for errors please, as seen in the surrounding code.
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
const char *author = NULL;
@@ -1397,7 +1424,9 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, ¤t_head))
return -1;
-
+ if (!(flags & AMEND_MSG) && opts->committer_date_is_author_date &&
+ setenv_committer_date_to_author_date())
+ return -1;
I think it would be better to handle committer_date_is_author_date in a
single place in this function, not have two implementations one of which
is untested as shown in Stolee's latest test email. This function
receives the author as a string so it should work with the author string
once we have checked for amend.
if (flags & AMEND_MSG) {
const char *exclude_gpgsig[] = { "gpgsig", NULL };
const char *out_enc = get_commit_output_encoding();
@@ -1418,6 +1447,17 @@ static int try_to_commit(struct repository *r,
res = error(_("unable to parse commit author"));
goto out;
}
+ if (opts->committer_date_is_author_date) {
+ char *date;
+ int len = strlen(author);
+ char *idx = memchr(author, '>', len);
what happens if there is a corrupted author without an email? - We end
up with undefined behavior.
+
+ ++idx;
+ date = malloc(author + len - idx);
+ memcpy(date, idx, author + len - idx);
+ setenv("GIT_COMMITTER_DATE", date, 1);
+ free(date);
git has a high level api for manipulation author/committer information
in ident.c, it would be best to use that. In any case code like this
should be using strbuf's rather than malloc
+
Re: amend warnings with no changes staged
On 06/08/2019 04:53, Junio C Hamano wrote:
Junio C Hamano writes:
Jonathan Nieder writes:
Some non-judgemental descriptive output like
$ git commit --amend --no-edit
No changes.
$
would address this case, without bothering people who are doing it
intentionally. So I think there's room for a simple improvement here.
I do that to refresh the committer timestamp.
I do, too. The proposal is, paraphrasing,
$ git commit --amend --no-edit
Ah, I see that you want me to refresh the committer timestamp.
Done, as requested.
$
Ah, OK then. I somehow misread "No changes." as an error message.
Well, on second thought, I think "fatal: no changes" that exits with
non-zero, with "--force" as an escape hatch for those who want to
refresh the committer timestamp, would probably be more in line with
the expectation Lukas had when this thread was started, and I further
suspect that it might be a bit more end-user friendly.
I agree that this would be the must user friendly way of doing it. I
think refreshing the timestamp is probably a niche use of '--amend' (out
of interest what the the motivation for doing that?) although it does
seem to popular with the other contributors to this thread. We could
always have a transition period with an opt-in config variable. I find
the current behavior quite annoying when I've forgotten to stage
anything as there is no indication the the commit's content is
unchanged. I've been using a wrapper script that errors out if there are
no new changes staged with --amend --no-edit and found it very helpful
(as is a proper --reword option rather than having to give --amend --only)
Best Wishes
Phillip
It is a backward incompatible behaviour, but I suspect that if I
were inventing "commit --amend" today, unlike 8588452c ("git-commit
--amend: allow empty commit.", 2006-03-04), I probably would design
it that way. After all, failing and stopping is always a safer
option than going ahead with or without a report.
I am not sure which one between "go ahead anyway but report" and
"fail by default but allow forcing" I would prefer more. At least
not yet. But I won't rule the latter out at this point.
Thanks.
Re: Support for --stdin-paths in commit, add, etc
On 01/08/2019 14:25, Alexandr Miloslavskiy wrote: On 31.07.2019 19:19, Jeff King wrote: I don't have any real objection to adding stdin support for more commands. Bu tin the specific case you're discussing, it seems like using "git update-index" might already solve your problem. It's the intended plumbing for scripted index updates, and it already supports receiving paths from stdin. I have now studied which git commands already use commandline splitting in our application. For some of them, I didn't find comparable plumbing; for others, I feel that a lot of new edge cases will arise, and it will need a lot of testing to make sure things work as expected. Therefore, for us it would be best if high-level commands also accepted --stdin-paths. If I develop good enough patches for that, will you accept them? We're interested in these high-level commands: 1) git add 2) git rm 3) git checkout 4) git reset 5) git stash 6) git commit Here's the list of detailed commands and plumbings I found: 01) git add 'git update-index' doesn't seem to be able to skip ignored files. No but it only takes paths not pathspecs, can you filter out the ignored paths first? From a UI point of view it would be better not to allow users to select ignored files if you don't want to be able to add them. If you want to use a pathspec then you can do 'git ls-files --exclude-standard -o -z | git update-index --add -z --stdin' git check-ignore --stdin should help if you have a list of paths and you want to remove the ignored ones (it's not great as it tells you which ones are ignored rather than filtering the list for you) 02) git add --force Probably 'git update-index --add --stdin'. 03) git checkout Probably 'git checkout-index --stdin' 04) git checkout HEAD Didn't find a plumbing to only affect named paths. You can use a temporary index and do GIT_INDEX_FILE=$tmp_index git read-tree HEAD && GIT_INDEX_FILE=$tmp_index git checkout-index --stdin && rm $tmp_index 05) git checkout --ours Probably 'git checkout-index --stage=2 --force --stdin' 06) git checkout --theirs Probably 'git checkout-index --stage=3 --force --stdin' 07) git rm [--force] [--cached] Probably 'git update-index --force-remove' Didn't find how to delete files from working tree. You have to delete them yourself. 08) git reset -q HEAD Didn't find a plumbing to only affect named paths. It's a bit of a pain but you can use 'git update-index -q --unmerged --refresh && git diff-index --cached -z HEAD' to get a list of paths in the index that differ from HEAD. The list contains the old oid for each path (see the man page for the format) and you can feed those into 'git update-index --index-info' to update the index. 09) git add --update Probably 'git update-index --again --add --stdin' Not sure that --again is good replacement. or something like 'git update-index -q --refresh && git diff-files --name-only -z | git update-index -z --stdin' 10) git stash push [--keep-index] [--include-untracked] [--message] Didn't find plumbing for stashes. stashes are just commits really so there are no plumbing commands specifically related to them. 11) git commit [--allow-empty] [--amend] [--signoff] [--no-verify] --file=CommitMessage.txt -o Didn't find a plumbing to only affect named staged paths. You can use a temporary index, add the files you want to commit with update-index --stdin and then run 'git commit' When I've been scripting I've sometimes wished that diff-index and diff-files had a --stdin option. Best Wishes Phillip
Re: [BUG]: Destructive behaviour of git revert
Hi Andreas On 30/07/2019 18:24, Andreas Wiesinger wrote: Hello, git revert for merges will mark merged files as deleted and commit them as if they would have been deleted, but that is for sure never what anybody would expect and has deleted many files unintentionally and unrecognized in our repo shortly. I have reproduced this issue in a very small example repo here: https://gitlab.com/electrocnic/git-revert-destructive-behaviour/tree/master Expected behaviour would be: A reverted merge should never mark any files or directories as "deleted", and merging the branch where the other merge has been reverted should not lead to the deletion of those files. Thanks for creating an example. I've had a quick look at it and it appears that you have a merge that adds a new file from the branch being merged, then when you revert the merge that file is deleted. Could you explain what you want to happen when you revert that merge? The idea of revert is to undo the changes introduced by the commit that is being reverted, I'm struggling to understand why you think revert should not do this for files that are added. Best Wishes Phillip I classify this behaviour as really really destructive and clearly a bug, even if this behaviour is by intention. It should not be by intention then.. Best regards, Andreas
Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.
Hi Alban On 25/07/2019 21:26, Alban Gruin wrote: Hi Phillip, Le 24/07/2019 à 15:29, Phillip Wood a écrit : Hi Alban Thanks for working on this, it's great to see you back on the list and I think it would be a useful addition to rebase. Unfortunately I'm not sure about this implementation though (although the early bug fix patches are useful in their own right) On 17/07/2019 15:39, Alban Gruin wrote: To prevent mistakes when editing a branch, rebase features a knob, rebase.missingCommitsCheck, to warn the user if a commit was dropped. Unfortunately, this check is only effective for the initial edit, which means that if you edit the todo list at a later point of the rebase and dropped a commit, no warnings or errors would be issued. This adds the ability to check if commits were dropped when resuming a rebase (with `rebase --continue'), when editing the todo list (with `rebase --edit-todo'), or when reloading the todo list after an `exec' command. I'm not sure if we really need to check the todo list when continuing or after an exec command. In which case I don’t really understand why there is an `error' mode if one can completely bypass it with `--continue'. That's an interesting point about `--continue`. Perhaps if `--edit-todo` detects deleted lines in error mode it should write a file to stop `--continue` continuing rather than having to validate the entire list each time we continue a rebase. Alternatively we could annotate the todo list with a message the dropped commits commented out and reopen the editor for the user to fix the problem, but that would cause scripted editors to enter a infinite loop as they're unlikely to fix the problem the second time round. A third possibility is to keep your code validating the list each time we run continue, but update the backup file with each edit so it detects added commits that are deleted in a later edit. This would also provide some protection for users who edit git-rebase-todo directly, though if they are using a script that deletes lines in git-rebase-todo directly it will suddenly stop working with this change if they have rebase.missingCommitsCheck set to error. Having said all that we could decide that the existing error message is enough and allow the user to skip re-editing the list if they really did mean to remove those lines. It would be annoying to have to re-edit the list when one had intended to delete those lines. The official way to edit the todo list is to run 'git rebase --edit-todo' and I'm not sure if we support scripts writing to .git/rebase-merge/git-rebase-todo directly. If we only support the check after --edit-todo then I think the implementation can be simplified as we can just write a copy of the file before it is edited and don't need to check .git/rebase-merge/done. Additionally that would catch commits that are added by the user and then deleted in a later edit. They wont be in the original list so I don't think this series will detect their deletion. True -- but with this solution, if a bad command is introduced, there will be false negatives. Given the pitfall of my solution, this should be an acceptable trade-off. We could detect a bad commit by checking the oid and not complaining if it is not valid. That's slightly complicated by labels, but we could fairly easily keep of list of the labels defined so far as we scan the list. That would also open the possibility of detecting errors where the user references an undefined label in `merge` or `reset` commands but that's a separate problem. Best Wishes Phillip At the extreme I have a script around rebase that runs 'rebase -i HEAD' and then fills in the todo list with a fake editor that adds 'reset ...' as the first line to set the starting point of the rebase. I think dscho's garden-shears script does something similar. Under the proposed scheme if I subsequently edit the todo list it will not catch any deleted commits as the original list is empty. Best Wishes Phillip Cheers, Alban
Re: [GSoC][PATCH v2 2/2] rebase -i: support --committer-date-is-author-date
Hi Rohit
On 23/07/2019 20:57, Rohit Ashiwal wrote:
Hi Phillip
On Sat, 20 Jul 2019 15:56:50 +0100 Phillip Wood
wrote:
[...]
@@ -467,6 +470,9 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
OPT_BOOL(0, "autosquash", &opts.autosquash,
N_("move commits that begin with squash!/fixup!")),
OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
+ OPT_BOOL(0, "committer-date-is-author-date",
+&opts.committer_date_is_author_date,
+N_("make committer date match author date")),
I guess it's good to do this for completeness but does
rebase--preserver-merges.sh support --committer-date-is-author-date? It
is the only caller of rebase--interactive I think so would be the only
user of this code.
Oh! Yes, I did it for the completeness. Let's add the flag while we
still have that _rebase--interactive_ command hanging out with us.
[...]
+ if (read_author_script(rebase_path_author_script(),
+ NULL, NULL, &date, 0))
+ die(_("failed to read author date"));
Can we have this return an error please - we try quite hard in the
sequencer not to die in library code.
Yes, we can through an error and continue, but then the user will
see the unchanged author date which is against his / her will but
it will not crash the program at least.
I dont think it should continue, the error should be propagated up to
cmd_rebase() (In your patch I think one of the context lines in
run_git_commit() shows this happening)
[...]
+ if (opts->committer_date_is_author_date) {
+ char *date = read_author_date_or_die();
+ argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s", date);
+ free(date);
+ }
It's a shame to be doing this twice is slightly different ways in the
same function (and again in try_to_commit() but I don't think that can
be avoided as not all callers of run_git_commit() go through
try_to_commit()). As I think the child inherits the current environment
modified by cmd.env_array we could just call setenv() at the top of the
function. It would be worth looking to see if it would be simpler to do
the setenv() call in the loop that picks the commits, then we would
avoid having to do it in do_merge() and try_to_commit() separately.
Ok, I'll have to change the code according to what Junio suggested.
Let's see how this area will look after that.
[...]
+ if (file_exists(rebase_path_cdate_is_adate())) {
+ opts->allow_ff = 0;
This is safe as we don't save the state of allow_ff for rebases so it
wont be overridden later. It would be an idea to add to the checks in
the assert() at the beginning of pick_commits() no we have another
option that implies --force-rebase.
Are you suggesting to modify this assert() call (in pick_commits())?
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit));
Yes I think it should check for opts->committer_date_is_author_date here
Best Wishes
Phillip
Thanks
Rohit
Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.
Hi Alban Thanks for working on this, it's great to see you back on the list and I think it would be a useful addition to rebase. Unfortunately I'm not sure about this implementation though (although the early bug fix patches are useful in their own right) On 17/07/2019 15:39, Alban Gruin wrote: To prevent mistakes when editing a branch, rebase features a knob, rebase.missingCommitsCheck, to warn the user if a commit was dropped. Unfortunately, this check is only effective for the initial edit, which means that if you edit the todo list at a later point of the rebase and dropped a commit, no warnings or errors would be issued. This adds the ability to check if commits were dropped when resuming a rebase (with `rebase --continue'), when editing the todo list (with `rebase --edit-todo'), or when reloading the todo list after an `exec' command. I'm not sure if we really need to check the todo list when continuing or after an exec command. The official way to edit the todo list is to run 'git rebase --edit-todo' and I'm not sure if we support scripts writing to .git/rebase-merge/git-rebase-todo directly. If we only support the check after --edit-todo then I think the implementation can be simplified as we can just write a copy of the file before it is edited and don't need to check .git/rebase-merge/done. Additionally that would catch commits that are added by the user and then deleted in a later edit. They wont be in the original list so I don't think this series will detect their deletion. At the extreme I have a script around rebase that runs 'rebase -i HEAD' and then fills in the todo list with a fake editor that adds 'reset ...' as the first line to set the starting point of the rebase. I think dscho's garden-shears script does something similar. Under the proposed scheme if I subsequently edit the todo list it will not catch any deleted commits as the original list is empty. Best Wishes Phillip The idea to extend this feature was suggested to me more than a year ago by Phillip Wood, if I'm not mistaken. I postponed this until four month ago, when ag/sequencer-reduce-rewriting-todo finally hit master, but I had to stop because of other obligations. I could go back to work one month ago, when I did the bulk of this series, but I lacked time to polish it, so it waited a bit more. Now, I think it is in a good shape to be sent, although it is still RFC-quality to me. The advertised functionality should work well, but perhaps there is some flaws I missed. The first two patches are new tests, demonstrating that after the initial edit, the check is not done. The next four are what could be qualified as omissions from ag/sequencer-reduce-rewriting-todo, but they are quite important (IMHO) for the rest of the series. The last three actually extend rebase.missingCommitsCheck. This is based on master (9d418600f4, "The fifth batch"). The tip of this series is tagged as "edit-todo-drop-rfc" in https://github.com/agrn/git. Alban Gruin (9): t3404: demonstrate that --edit-todo does not check for dropped commits t3429: demonstrate that rebase exec does not check for dropped commits sequencer: update `total_nr' when adding an item to a todo list sequencer: update `done_nr' when skipping commands in a todo list sequencer: move the code writing total_nr on the disk to a new function sequencer: add a parameter to sequencer_continue() to accept a todo list rebase-interactive: todo_list_check() also uses the done list rebase-interactive: warn if commit is dropped with --edit-todo sequencer: have read_populate_todo() check for dropped commits builtin/rebase.c | 2 +- builtin/revert.c | 2 +- rebase-interactive.c | 67 +++- rebase-interactive.h | 6 ++- sequencer.c | 53 ++ sequencer.h | 3 +- t/t3404-rebase-interactive.sh | 82 +++ t/t3429-rebase-edit-todo.sh | 44 ++- 8 files changed, 224 insertions(+), 35 deletions(-)
Re: [GSoC][PATCH v2 1/1] rebase -i: add --ignore-whitespace flag
parse_options(argc, argv, NULL, options,
> builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
>
> + opts.strategy_opts = xstrdup_or_null(opts.strategy_opts);
> +
> if (!is_null_oid(&squash_onto))
> opts.squash_onto = &squash_onto;
>
> @@ -954,6 +970,8 @@ static int run_am(struct rebase_options *opts)
> am.git_cmd = 1;
> argv_array_push(&am.args, "am");
>
> + if (opts->ignore_whitespace)
> + argv_array_push(&am.args, "--ignore-whitespace");
> if (opts->action && !strcmp("continue", opts->action)) {
> argv_array_push(&am.args, "--resolved");
> argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> @@ -1401,9 +1419,6 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
> OPT_BOOL(0, "signoff", &options.signoff,
>N_("add a Signed-off-by: line to each commit")),
> - OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
> - NULL, N_("passed to 'git am'"),
> - PARSE_OPT_NOARG),
> OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
> &options.git_am_opts, NULL,
> N_("passed to 'git am'"), PARSE_OPT_NOARG),
> @@ -1411,6 +1426,8 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> N_("passed to 'git am'"), PARSE_OPT_NOARG),
> OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
> N_("passed to 'git apply'"), 0),
> + OPT_BOOL(0, "ignore-whitespace", &options.ignore_whitespace,
> + N_("ignore changes in whitespace")),
> OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
> N_("action"), N_("passed to 'git apply'"), 0),
> OPT_BIT('f', "force-rebase", &options.flags,
> @@ -1821,6 +1838,9 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> }
>
> if (options.rebase_merges) {
> + if (options.ignore_whitespace)
> + die(_("cannot combine '--rebase-merges' with "
> + "'--ignore-whitespace'"));
I was going to ask why we cannot just pass -Xignore-space-change when
making the merge but the lines below seem to show we don't support merge
options yet.
> if (strategy_options.nr)
> die(_("cannot combine '--rebase-merges' with "
> "'--strategy-option'"));
> diff --git a/sequencer.h b/sequencer.h
> index 0c494b83d4..303047a133 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -43,6 +43,7 @@ struct replay_opts {
> int verbose;
> int quiet;
> int reschedule_failed_exec;
> + int ignore_whitespace;
Is this new field used anywhere - we add -Xignore-space-change to
replay_opts.xopts so why do we need this as well?
Best Wishes
Phillip
>
> int mainline;
>
> diff --git a/t/t3422-rebase-incompatible-options.sh
> b/t/t3422-rebase-incompatible-options.sh
> index a5868ea152..4342f79eea 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -61,7 +61,6 @@ test_rebase_am_only () {
> }
>
> test_rebase_am_only --whitespace=fix
> -test_rebase_am_only --ignore-whitespace
> test_rebase_am_only --committer-date-is-author-date
> test_rebase_am_only -C4
>
> diff --git a/t/t3431-rebase-options-compatibility.sh
> b/t/t3431-rebase-options-compatibility.sh
> new file mode 100755
> index 00..f38ae6f5fc
> --- /dev/null
> +++ b/t/t3431-rebase-options-compatibility.sh
> @@ -0,0 +1,66 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2019 Rohit Ashiwal
> +#
> +
> +test_description='tests to ensure compatibility between am and interactive
> backends'
> +
> +. ./test-lib.sh
> +
> +# This is a special case in which both am and interactive backends
> +# provide the same outputs. It was done intentionally because
> +# --ignore-whitespace both the backends fall short of optimal
> +# behaviour.
> +test_expect_s
Re: [GSoC][PATCH v2 2/2] rebase -i: support --committer-date-is-author-date
EDIT_MSG))
@@ -1467,6 +1492,12 @@ static int try_to_commit(struct repository *r,
reset_ident_date();
+ if (opts->committer_date_is_author_date) {
+ char *date = read_author_date_or_die();
+ setenv("GIT_COMMITTER_DATE", date, 1);
+ free(date);
+ }
+
if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
oid, author, opts->gpg_sign, extra)) {
res = error(_("failed to write commit object"));
@@ -2538,6 +2569,11 @@ static int read_populate_opts(struct replay_opts *opts)
opts->signoff = 1;
}
+ if (file_exists(rebase_path_cdate_is_adate())) {
+ opts->allow_ff = 0;
This is safe as we don't save the state of allow_ff for rebases so it
wont be overridden later. It would be an idea to add to the checks in
the assert() at the beginning of pick_commits() no we have another
option that implies --force-rebase.
Best Wishes
Phillip
+ opts->committer_date_is_author_date = 1;
+ }
+
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;
@@ -2620,6 +2656,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
write_file(rebase_path_gpg_sign_opt(), "-S%s\n",
opts->gpg_sign);
if (opts->signoff)
write_file(rebase_path_signoff(), "--signoff\n");
+ if (opts->committer_date_is_author_date)
+ write_file(rebase_path_cdate_is_adate(), "%s", "");
if (opts->reschedule_failed_exec)
write_file(rebase_path_reschedule_failed_exec(), "%s", "");
@@ -3437,6 +3475,12 @@ static int do_merge(struct repository *r,
argv_array_push(&cmd.args, git_path_merge_msg(r));
if (opts->gpg_sign)
argv_array_push(&cmd.args, opts->gpg_sign);
+ if (opts->committer_date_is_author_date) {
+ char *date = read_author_date_or_die();
+ argv_array_pushf(&cmd.env_array,
+"GIT_COMMITTER_DATE=%s", date);
+ free(date);
+ }
/* Add the tips to be merged */
for (j = to_merge; j; j = j->next)
diff --git a/sequencer.h b/sequencer.h
index 303047a133..0cfe184fc2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,7 @@ struct replay_opts {
int quiet;
int reschedule_failed_exec;
int ignore_whitespace;
+ int committer_date_is_author_date;
int mainline;
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 4342f79eea..7402f7e3da 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -61,7 +61,6 @@ test_rebase_am_only () {
}
test_rebase_am_only --whitespace=fix
-test_rebase_am_only --committer-date-is-author-date
test_rebase_am_only -C4
test_expect_success REBASE_P '--preserve-merges incompatible with --signoff' '
diff --git a/t/t3431-rebase-options-compatibility.sh
b/t/t3431-rebase-options-compatibility.sh
index f38ae6f5fc..c3f7f6d5d0 100755
--- a/t/t3431-rebase-options-compatibility.sh
+++ b/t/t3431-rebase-options-compatibility.sh
@@ -7,6 +7,9 @@ test_description='tests to ensure compatibility between am and
interactive backe
. ./test-lib.sh
+GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
+export GIT_AUTHOR_DATE
+
# This is a special case in which both am and interactive backends
# provide the same outputs. It was done intentionally because
# --ignore-whitespace both the backends fall short of optimal
@@ -63,4 +66,22 @@ test_expect_success '--ignore-whitespace works with
interactive backend' '
test_cmp expect file
'
+test_expect_success '--committer-date-is-author-date works with am backend' '
+ git rebase -f HEAD^ &&
+ git rebase --committer-date-is-author-date HEAD^ &&
+ git cat-file commit HEAD | sed -e "/^\$/q" >head &&
+ sed -ne "/^author /s/.*> //p" head >authortime &&
+ sed -ne "/^committer /s/.*> //p" head >committertime &&
+ test_cmp authortime committertime
+'
+
+test_expect_success '--committer-date-is-author-date works with interactive
backend' '
+ git rebase -f HEAD^ &&
+ git rebase -i --committer-date-is-author-date HEAD^ &&
+ git cat-file commit HEAD | sed -e "/^\$/q" >head &&
+ sed -ne "/^author /s/.*> //p" head >authortime &&
+ sed -ne "/^committer /s/.*> //p" head >committertime &&
+ test_cmp authortime committertime
+'
+
test_done
Re: What's cooking in git.git (Jul 2019, #01; Wed, 3)
On 08/07/2019 23:02, Junio C Hamano wrote: > Phillip Wood writes: > >>> * pw/rebase-progress-test-cleanup (2019-07-01) 1 commit >>> - t3420: remove progress lines before comparing output >>> (this branch uses sg/rebase-progress.) >>> >>> Test cleanup. >>> >>> Will merge to 'next'. >> >> I've just posted an update to this which avoids the repeated printf calls > > Thanks. Picked up the one from "Date: Thu Jul 4 02:47:02 2019 -0700". Yes, that's the one - sorry I forgot to add a link to my original email Best Wishes Phillip
Re: What's cooking in git.git (Jul 2019, #01; Wed, 3)
On 03/07/2019 23:28, Junio C Hamano wrote: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. The ones marked with '.' do not appear in any of > the integration branches, but I am still holding onto them. > > The third batch of topics post 2.22 are now in 'master', and the tip > of 'next' has been rewound. > > You can find the changes described here in the integration branches > of the repositories listed at > > http://git-blame.blogspot.com/p/git-public-repositories.html > > -- > [New Topics] > > * pw/rebase-progress-test-cleanup (2019-07-01) 1 commit > - t3420: remove progress lines before comparing output > (this branch uses sg/rebase-progress.) > > Test cleanup. > > Will merge to 'next'. I've just posted an update to this which avoids the repeated printf calls Best Wishes Phillip
Re: [PATCH 1/1] t3420: remove progress lines before comparing output
On 02/07/2019 18:23, Junio C Hamano wrote: > Phillip Wood writes: > >>> As long as sed implementation used here does not do anything funny >>> to CR, I think the approach to strip everything before the last CR >>> on the line is sensible. As I am not familiar with how Windows port >>> of sed wants to treat a CR byte in the pattern, I am not sure about >>> the precondition of the above statement, though. >> >> I wondered about that too, but it passes the CI tests under windows. > > Hopefully Git for Windows, MinGW, and CygWin would all behave > similarly. > >>> I also have to wonder if we can/want to do this without an extra >>> printf process every time we sanitize the output, though I do not >>> think I care too deeply about it. >> >> I could add 're="$(printf ...)"' to the setup at the top of the file >> if you want > > As I do not care too deeply about it, we recently saw a lot about > reducing number of processes in the tests, so apparently some folks > care and I presume they want to see something like that to happen. > I do not think $re is a good name for such a variable, though ;-) Yes, $re was just a place holder - naming is hard ... Best Wishes Phillip
[PATCH v2 1/1] t3420: remove progress lines before comparing output
From: Phillip Wood
Some of the tests check the output of rebase is what we expect. These
were added after a regression that added unwanted stash output when
using --autostash. They are useful as they prevent unintended changes to
the output of the various rebase commands. However they also include all
the progress output which is less useful as it only tests what would be
written to a dumb terminal which is not the normal use case. The recent
changes to fix clearing the line when printing progress necessarily
meant making an ugly change to these tests. Address this my removing the
progress output before comparing it to the expected output. We do this
by removing everything before the final "\r" on each line as we don't
care about the progress indicator, but we do care about what is printed
immediately after it.
Signed-off-by: Phillip Wood
---
t/t3420-rebase-autostash.sh | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 9186e90127..b8f4d03467 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -30,7 +30,8 @@ test_expect_success setup '
echo conflicting-change >file2 &&
git add . &&
test_tick &&
- git commit -m "related commit"
+ git commit -m "related commit" &&
+ remove_progress_re="$(printf "s/.*\\r//")"
'
create_expected_success_am () {
@@ -48,8 +49,8 @@ create_expected_success_interactive () {
q_to_cr >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
HEAD is now at $(git rev-parse --short feature-branch) third commit
- Rebasing (1/2)QRebasing (2/2)QApplied autostash.
- Q
QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
+ Applied autostash.
+ Successfully rebased and updated refs/heads/rebased-feature-branch.
EOF
}
@@ -67,13 +68,13 @@ create_expected_failure_am () {
}
create_expected_failure_interactive () {
- q_to_cr >expected <<-EOF
+ cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
HEAD is now at $(git rev-parse --short feature-branch) third commit
- Rebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.
+ Applying autostash resulted in conflicts.
Your changes are safe in the stash.
You can run "git stash pop" or "git stash drop" at any time.
- Q
QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
+ Successfully rebased and updated refs/heads/rebased-feature-branch.
EOF
}
@@ -109,7 +110,8 @@ testrebase () {
suffix=interactive
fi &&
create_expected_success_$suffix &&
- test_i18ncmp expected actual
+ sed "$remove_progress_re" actual2 &&
+ test_i18ncmp expected actual2
'
test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
@@ -209,7 +211,8 @@ testrebase () {
suffix=interactive
fi &&
create_expected_failure_$suffix &&
- test_i18ncmp expected actual
+ sed "$remove_progress_re" actual2 &&
+ test_i18ncmp expected actual2
'
}
--
gitgitgadget
