Re: branch --contains / tag --merged inconsistency
SZEDER Gábor writes: > The commit pointed to by the tag Pacemaker-0.6.1 and its parent have a > serious clock skew, i.e. they are a few months older than their parents: > [...] > (branch|tag|describe|...) (--contains|--merged) use the commit timestamp > information as a heuristic to avoid traversing parts of the history, > which makes these operations, especially on big histories, an order of > magnitude or two faster. Yeah, commit timestamps can't always be > trusted, but skewed commits are rare, and skewed commits with this much > skew are even rarer. Szia Gábor, Thank you very much for the explanation! Good to know there's no serious problem or misunderstanding here, but a conscious algorithmic choice being fooled by clock skew. I wonder if there's a way such clock skew can appear without actual committer clocks being off by months... > FWIW, much work is being done on a cached commit graph including commit > generation numbers, which will solve this issue both correctly and more > efficiently. Perhaps it will already be included in the next release. Wonderful news, thanks for sharing it! -- Regards, Feri
Re: [RFC PATCH] checkout: Force matching mtime between files
On Fri, Apr 27, 2018 at 11:08 PM, Marc Branchaud wrote:
>> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud
>> This is a limitation of the current post-checkout hook. $3==0 from the
>> hook lets us know this is not a branch switch, but it does not really
>> tell you the affected paths. If it somehow passes all the given
>> pathspec to you, then you should be able to do "git ls-files --
>> $pathspec" which gives you the exact same set of paths that
>> git-checkout updates. We could do this by setting $4 to "--" and put
>> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
>> the above example.
>>
>> There is third case here, if you do "git checkout --
>> path/to/file" then it cannot be covered by the current design. I guess
>> we could set $3 to '2' (retrieve from a tree) to indicate this in
>> addition to 0 (from index) and 1 (from switching branch) and then $1
>> could be the tree in question (pathspecs are passed the same way
>> above)
>>
>> [1] I wonder if we could have a more generic approach to pass
>> pathspecs via environment, which could work for more than just this
>> one hook. Not sure if it's a good idea though.
>
>
> I think there needs to be something other than listing all the paths in the
> command is viable, because it's too easy to hit some command-line-length
> limit.
I send pathspecs, not paths. If you type "git checkout -- foo/" then I
send exactly "foo/" not every paths in it. You can always figure that
out with git-ls-files.
Sure this can still hit command length limit when you do "git checkout
-- foo/*" and have lots of files in foo just one more param from
hitting the limit, then the hook may hit the limit because we need
more command line arguments. But this is the corner case I don't think
we should really need to care about.
> It would also be good if hook authors didn't have to re-invent the
> wheel of determining the changed paths for every corner-case.
Flexibility vs convenience I guess. A sample hook as template should
help the reinvention.
> My first instinct is to write them one-per-line on the hook's stdin. That's
> probably not generic enough for most hooks, but it seems like a good
> approach for this proposal.
>
> Throwing them into a temporary file with a known name is also good ---
> better, I think, than stuffing them into an environment variable.
This goes back to my post-checkout-modified proposal. If you're
writing to file, might as well reuse the index format. Then you can
read it with ls-files (which lets you decide path separator or even
quoting, I'm not sure) and it also provides some more info like file
hashes, access time...
--
Duy
Re: [RFC PATCH] checkout: Force matching mtime between files
On Fri, Apr 27, 2018 at 11:08 PM, Elijah Newren wrote:
> On Fri, Apr 27, 2018 at 10:03 AM, Duy Nguyen wrote:
>> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud wrote:
>>>
>>> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
>>> identical so the above loop does nothing. Offhand I'm not even sure how a
>>> hook might get the right files in this case.
>>
>> This is a limitation of the current post-checkout hook. $3==0 from the
>> hook lets us know this is not a branch switch, but it does not really
>> tell you the affected paths. If it somehow passes all the given
>> pathspec to you, then you should be able to do "git ls-files --
>> $pathspec" which gives you the exact same set of paths that
>> git-checkout updates. We could do this by setting $4 to "--" and put
>> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
>> the above example.
>>
>> There is third case here, if you do "git checkout --
>> path/to/file" then it cannot be covered by the current design. I guess
>> we could set $3 to '2' (retrieve from a tree) to indicate this in
>> addition to 0 (from index) and 1 (from switching branch) and then $1
>> could be the tree in question (pathspecs are passed the same way
>> above)
>>
>> [1] I wonder if we could have a more generic approach to pass
>> pathspecs via environment, which could work for more than just this
>> one hook. Not sure if it's a good idea though.
>
> Here's a crazy idea -- maybe instead of a list of pathspecs you just
> provide the timestamp of when git checkout started. Then the hook
> could walk the tree, find all files with modification times at least
> that late, and modify them all back to the the timestamp of when the
> git checkout started.
>
> Would that be enough? Is that too crazy?
For this use case? Probably (assuming that timestamp precision does
not cause problems).
I'm more concerned about what info post-checkout hook should provide
but does not. Giving hook writer a way to get the list of updated
files lets them do more fancy stuff while providing checkout start
time probably only helps just this one case.
Providing start time in general for all hooks sounds like a good thing
though (and simple enough to implement).
--
Duy
Re: [PATCH 18/41] index-pack: abstract away hash function constant
On Fri, Apr 27, 2018 at 11:08 PM, brian m. carlson wrote: > On Thu, Apr 26, 2018 at 05:46:28PM +0200, Duy Nguyen wrote: >> On Wed, Apr 25, 2018 at 8:49 PM, Martin Ågren wrote: >> > Once that is accomplished, I sort of suspect that this code will want to >> > be updated to not always blindly use the_hash_algo, but to always work >> > with SHA-1 sizes. Or rather, this would turn into more generic code to >> > handle both "v2 with SHA-1" and "v3 with some hash function(s)". This >> > commit might be a good first step in that direction. >> >> I also have an uneasy feeling when things this close to on-disk file >> format get hash-agnostic treatment. I think we would need to start >> adding new file formats soon, from bottom up with simple things like >> loose object files (cat-file and hash-object should be enough to test >> blobs...), then moving up to pack files and more. This is when we can >> really decide where to use the new hash and whether we should keep >> some hashes as sha-1. > > I agree that this is work which needs to be done soon. There are > basically a couple of pieces we need to handle NewHash: > > * Remove the dependencies on SHA-1 as much as possible. > * Get the tests to pass with a different hash (almost done for 160-bit > hash; in progress for 256-bit hashes). This step sounds good on paper but realistically could be a nightmare for you. I tried to implement a simple cat-file/hash-object combination with my imaginary newhash, which sounded straightforward to me since you have done a lot of heavylifting. To my surprise I hit a lot more problems. My point is, when I concentrate on just a few simple cases like this, I have a smaller scope to work with and could quickly identify problems. When you work on the scale of the test suite, it's really hard to know where the problem is (and you don't even know what areas are newhash-safe). Anyway my cat-file/hash-object modification could be found here. I probably will polish and send out a few good patches from it. https://github.com/pclouds/git/commits/new-hash -- Duy
Re: [PATCH v6 06/11] Add a test for `git replace --convert-graft-file`
Hallo Johannes, > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh > index c630aba657e..bed86a0af3d 100755 > --- a/t/t6050-replace.sh > +++ b/t/t6050-replace.sh > @@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a > mergetag' ' Note the GPG prereq of the previous test. > git replace -d $HASH10 > ' > > +test_expect_success '--convert-graft-file' ' > + : add and convert graft file && > + printf "%s\n%s %s\n\n# comment\n%s\n" \ > + $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \ This test relies on the piece of history that was created two tests earlier in 'set up a merge commit with a mergetag', and that test, too, has the GPG prereq. So if there is no GPG installed, then that test won't be executed, the history needed by this test won't be created, HEAD won't have a second parent, and I'm sure you know where this is going: ok 32 # skip set up a merge commit with a mergetag (missing GPG) <... snip ...> ok 33 # skip --graft on a commit with a mergetag (missing GPG) <... snip ...> ++git log --graph --oneline * ffccc9d hello: again 3 more lines * 14ac020 hello: 2 more lines * 093e41a hello: BUG fixed * 40237c8 hello: 1 more line * 8fc2a8e hello: 2 more lines * 4217adb hello: 4 more lines with a BUG * 00ad688 hello: 4 lines ++: add and convert graft file +++git rev-parse 'HEAD^^' 'HEAD^' 'HEAD^^' 'HEAD^2' fatal: ambiguous argument 'HEAD^2': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' ++printf '%s\n%s %s\n\n# comment\n%s\n' 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c 14ac020163ea60a9d683ce68e36c946f31ecc856 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c 'HEAD^2' ++cat .git/info/grafts 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c 14ac020163ea60a9d683ce68e36c946f31ecc856 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c # comment HEAD^2 ++git replace --convert-graft-file hint: Support for /info/grafts is deprecated hint: and will be removed in a future Git version. hint: hint: Please use "git replace --convert-graft-file" hint: to convert the grafts into replace refs. hint: hint: Turn this message off by running hint: "git config advice.graftFileDeprecated false" error: bad graft data: HEAD^2 error: new commit is the same as the old one: '14ac020163ea60a9d683ce68e36c946f31ecc856' error: Not a valid object name: 'HEAD^2' warning: could not convert the following graft(s): 14ac020163ea60a9d683ce68e36c946f31ecc856 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c HEAD^2 error: last command exited with $?=1 not ok 34 - --convert-graft-file > + >.git/info/grafts && > + git replace --convert-graft-file && > + test_path_is_missing .git/info/grafts && > + > + : verify that the history is now "grafted" && > + git rev-list HEAD >out && > + test_line_count = 4 out && > + > + : create invalid graft file and verify that it is not deleted && > + test_when_finished "rm -f .git/info/grafts" && > + echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts && > + test_must_fail git replace --convert-graft-file 2>err && > + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" err && > + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts > +' > + > test_done > -- > 2.17.0.windows.1.33.gfcbb1fa0445
Branch deletion question / possible bug?
Hi, I discovered that I was able to delete the feature branch I was in, due to some fat fingering on my part and case insensitivity. I never realized this could be done before. A quick google search did not give me a whole lot to work with... Steps to reproduce: 1. Create a feature branch, "editCss" 2. git checkout master 3. git checkout editCSS 4. git checkout editCss 5. git branch -d editCSS Normally, it should have been impossible for a user to delete the branch they're on. And the deletion left me in a weird state that took a while to dig out of. I know this was a user error, but I was also wondering if this was a bug. Thanks, Pik Tang
Re: git merge banch w/ different submodule revision
Hi,
On Fri, Apr 27, 2018 at 3:37 AM, Middelschulte, Leif
wrote:
> Am Donnerstag, den 26.04.2018, 17:19 -0700 schrieb Elijah Newren:
>> On Thu, Apr 26, 2018 at 3:49 AM, Middelschulte, Leif
>> wrote:
>> > Problem case: Merge either branch into the other
>> >
>> > Expected behavior: Merge conflict.
>> >
>> > Actual behavior: Auto merge without conflicts.
>> >
>> > Note 1: A merge conflict does occur, if the sourced revisions do *not*
>> > have a linear history
Let me just note that I don't actually use submodules myself, and
rarely run across them, so as far as users expect submodules should
behave I may have to defer to others. But it was particularly this
sentence of yours that caught my attention and got me to respond. I
may have misunderstood which repository had the non-linear history,
but...
>> But, there is some further smarts in that if either A or B point at
>> commits that contain the other in their history and both contain the
>> commit that O points at, then you can just do a fast-forward update to
>> the newest.
This particular paragraph, is relevant to your example; more details below.
>> You didn't tell us how the merge-base (cd5e1a5 from the diagram you
>> gave) differed in your example here between the two repositories. In
>> fact, the non-linear case could have several merge-bases, in which
>> case they all become potentially relevant (as does their merge-bases
>> since at that point you'll trigger the recursive portion of
>> merge-recursive). Giving us that info might help us point out what
>> happened, though if either the fast-forward logic comes into play or
>> the recursive logic gets in the mix, then we may need you to provide a
>> testcase (or access to the repo in question) in order to explain it
>> and/or determine if you've found a bug.
>
> I placed two reositories here:
> https://gitlab.com/foss-contributions/git-examples/network/develop
> The access should be public w/o login.
>
> If you prefer the examples to be placed somewhere else, let me know.
So the only thing I see here is a single repository, which contains a
submodule with linear history. (unless I was grabbing it wrong; I
just tried `git clone --recurse-submodules
https://gitlab.com/foss-contributions/git-examples`) Do you also have
an example with non-linear history demonstrating your claim that it
behaves differently, for comparison?
Anyway, in this case you had both branches updating the submodule to
something newer (to a fast-forward update of what it previously was),
but one side advanced it further than the other side did (in
particular, to what turned out to be a fast-forward update of what the
other branch used). That means the whole fast-forwarding logic of
commit 68d03e4a6e44 ("Implement automatic fast-forward merge for
submodules", 2010-07-07)) came into play.
I would expect that a different example involving non-linear history
would behave the same, if both sides update the submodule in a fashion
that is just fast-forwarding and one commit contains the other in its
history. I'm curious if you have a counter example.
[PATCH 6/6] rebase --rebase-merges: root commits can be cousins, too
Reported by Wink Saville: when rebasing with no-rebase-cousins, we
will want to refrain from rebasing all of them, even when they are
root commits.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 3 ++-
t/t3430-rebase-merges.sh | 25 +
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index ad5ff2709a6..2bcd13e1fc6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3883,7 +3883,8 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
}
if (!commit)
- fprintf(out, "%s onto\n", cmd_reset);
+ fprintf(out, "%s %s\n", cmd_reset,
+ rebase_cousins ? "onto" : "[new root]");
else {
const char *to = NULL;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 5543f1d5a34..ce6de6f491e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -287,5 +287,30 @@ test_expect_success 'a "merge" into a root commit is a
fast-forward' '
test_cmp_rev HEAD $head
'
+test_expect_success 'A root commit can be a cousin, treat it that way' '
+ git checkout --orphan khnum &&
+ test_commit yama &&
+ git checkout -b asherah master &&
+ test_commit shamkat &&
+ git merge --allow-unrelated-histories khnum &&
+ test_tick &&
+ git rebase -f -r HEAD^ &&
+ ! test_cmp_rev HEAD^2 khnum &&
+ test_cmp_graph HEAD^.. <<-\EOF &&
+ * Merge branch '\''khnum'\'' into asherah
+ |\
+ | * yama
+ o shamkat
+ EOF
+ test_tick &&
+ git rebase --rebase-merges=rebase-cousins HEAD^ &&
+ test_cmp_graph HEAD^.. <<-\EOF
+ * Merge branch '\''khnum'\'' into asherah
+ |\
+ | * yama
+ |/
+ o shamkat
+ EOF
+'
test_done
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward
When a user provides a todo list containing something like
reset [new root]
merge my-branch
let's do the same as if pulling into an orphan branch: simply
fast-forward.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 12
t/t3430-rebase-merges.sh | 13 +
2 files changed, 25 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index d10ebd62520..ad5ff2709a6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2850,6 +2850,18 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
goto leave_merge;
}
+ if (opts->have_squash_onto &&
+ !oidcmp(&head_commit->object.oid, &opts->squash_onto)) {
+ /*
+* When the user tells us to "merge" something into a
+* "[new root]", let's simply fast-forward to the merge head.
+*/
+ rollback_lock_file(&lock);
+ ret = fast_forward_to(&merge_commit->object.oid,
+ &head_commit->object.oid, 0, opts);
+ goto leave_merge;
+ }
+
if (commit) {
const char *message = get_commit_buffer(commit, NULL);
const char *body;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 35260862fcb..5543f1d5a34 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -275,4 +275,17 @@ test_expect_success 'root commits' '
test_cmp_rev HEAD $before
'
+test_expect_success 'a "merge" into a root commit is a fast-forward' '
+ head=$(git rev-parse HEAD) &&
+ cat >script-from-scratch <<-EOF &&
+ reset [new root]
+ merge $head
+ EOF
+ test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+ test_tick &&
+ git rebase -i -r HEAD^ &&
+ test_cmp_rev HEAD $head
+'
+
+
test_done
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part
In this developer's earlier attempt to accelerate interactive rebases by
converting large parts from Unix shell script into portable, performant
C, the --root handling was specifically excluded (to simplify the task a
little bit; it still took over a year to get that reduced set of patches
into Git proper).
This patch ties up that loose end: now only --preserve-merges uses the
slow Unix shell script implementation to perform the interactive rebase.
As the rebase--helper reports progress to stderr (unlike the scripted
interactive rebase, which reports it to stdout, of all places), we have
to adjust a couple of tests that did not expect that for `git rebase -i
--root`.
This patch fixes -- at long last! -- the really old bug reported in
6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
--root *always* rewrote the root commit, even if there were no changes.
The bug still persists in --preserve-merges mode, of course, but that
mode will be deprecated as soon as the new --rebase-merges mode
stabilizes, anyway.
Signed-off-by: Johannes Schindelin
---
git-rebase--interactive.sh| 4 +++-
t/t3404-rebase-interactive.sh | 19 +--
t/t3421-rebase-topology-linear.sh | 6 +++---
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cbf44f86482..2f4941d0fc9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
else
revisions=$onto...$orig_head
shortrevisions=$shorthead
+ test -z "$squash_onto" ||
+ echo "$squash_onto" >"$state_dir"/squash-onto
fi
}
@@ -948,7 +950,7 @@ EOF
die "Could not skip unnecessary pick commands"
checkout_onto
- if test -z "$rebase_root" && test ! -d "$rewritten"
+ if test ! -d "$rewritten"
then
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff}
$allow_empty_message \
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540e5..c65826ddace 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1204,10 +1204,6 @@ test_expect_success 'drop' '
test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
'
-cat >expect expect actual.2 &&
+ cr_to_nl actual &&
test_i18ncmp expect actual &&
test D = $(git cat-file commit HEAD | sed -ne \$p)
'
diff --git a/t/t3421-rebase-topology-linear.sh
b/t/t3421-rebase-topology-linear.sh
index e7438ad06ac..99b2aac9219 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -328,9 +328,9 @@ test_run_rebase () {
test_cmp_rev c HEAD
"
}
-test_run_rebase failure ''
-test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
test_run_rebase failure -p
test_run_rebase () {
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH 2/6] sequencer: learn about the special "fake root commit" handling
When an interactive rebase wants to recreate a root commit, it
- first creates a new, empty root commit,
- checks it out,
- converts the next `pick` command so that it amends the empty root
commit
Introduce support in the sequencer to handle such an empty root commit,
by looking for the file /rebase-merge/squash-onto; if it exists
and contains a commit name, the sequencer will compare the HEAD to said
root commit, and if identical, a new root commit will be created.
While converting scripted code into proper, portable C, we also do away
with the old "amend with an empty commit message, then cherry-pick
without committing, then amend again" dance and replace it with code
that uses the internal API properly to do exactly what we want: create a
new root commit.
To keep the implementation simple, we always spawn `git commit` to create
new root commits.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 104 ++--
sequencer.h | 4 ++
2 files changed, 105 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 90c8218aa9a..fc124596b53 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list,
"rebase-merge/rewritten-list")
static GIT_PATH_FUNC(rebase_path_rewritten_pending,
"rebase-merge/rewritten-pending")
+/*
+ * The path of the file containig the OID of the "squash onto" commit, i.e.
+ * the dummy commit used for `reset [new root]`.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
+
/*
* The path of the file listing refs that need to be deleted after the rebase
* finishes. This is used by the `label` command to record the need for
cleanup.
@@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to,
const struct object_id *f
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, "HEAD",
- to, unborn ? &null_oid : from,
+ to, unborn && !is_rebase_i(opts) ?
+ &null_oid : from,
0, sb.buf, &err) ||
ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
@@ -692,6 +699,42 @@ static char *get_author(const char *message)
return NULL;
}
+static const char *read_author_ident(struct strbuf *buf)
+{
+ char *p, *p2;
+
+ if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
+ return NULL;
+
+ for (p = buf->buf; *p; p++)
+ if (skip_prefix(p, "'''", (const char **)&p2))
+ strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
+ else if (*p == '\'')
+ strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
+
+ if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
+ strbuf_splice(buf, 0, p - buf->buf, "", 0);
+ p = strchr(buf->buf, '\n');
+ if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
+ strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
+ p = strchr(p, '\n');
+ if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
+ (const char **)&p2)) {
+ strbuf_splice(buf, p - buf->buf, p2 - p,
+ "> ", 2);
+ p = strchr(p, '\n');
+ if (p) {
+ strbuf_setlen(buf, p - buf->buf);
+ return buf->buf;
+ }
+ }
+ }
+ }
+
+ warning(_("could not parse '%s'"), rebase_path_author_script());
+ return NULL;
+}
+
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"
@@ -711,6 +754,7 @@ N_("you have staged changes in your working tree\n"
#define AMEND_MSG (1<<2)
#define CLEANUP_MSG (1<<3)
#define VERIFY_MSG (1<<4)
+#define CREATE_ROOT_COMMIT (1<<5)
/*
* If we are cherry-pick, and if the merge did not result in
@@ -730,6 +774,40 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
struct child_process cmd = CHILD_PROCESS_INIT;
const char *value;
+ if (flags & CREATE_ROOT_COMMIT) {
+ struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
+ const char *author = is_rebase_i(opts) ?
+ read_author_ident(&script) : NULL;
+ struct object_id root_commit, *cache_tree_oid;
+ int res = 0;
+
+ if (!defmsg)
+ BUG("root commit without message");
+
+ i
[PATCH 1/6] sequencer: extract helper to update active_cache_tree
This patch extracts the code from is_index_unchanged() to initialize or
update the index' cache tree (i.e. a tree object reflecting the current
index' top-level tree).
The new helper will be used in the upcoming code to support `git rebase
-i --root` via the sequencer.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e2f83942843..90c8218aa9a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -562,9 +562,23 @@ static int do_recursive_merge(struct commit *base, struct
commit *next,
return !clean;
}
+static struct object_id *get_cache_tree_oid(void)
+{
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree))
+ if (cache_tree_update(&the_index, 0)) {
+ error(_("unable to update cache tree"));
+ return NULL;
+ }
+
+ return &active_cache_tree->oid;
+}
+
static int is_index_unchanged(void)
{
- struct object_id head_oid;
+ struct object_id head_oid, *cache_tree_oid;
struct commit *head_commit;
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
@@ -583,15 +597,10 @@ static int is_index_unchanged(void)
if (parse_commit(head_commit))
return -1;
- if (!active_cache_tree)
- active_cache_tree = cache_tree();
-
- if (!cache_tree_fully_valid(active_cache_tree))
- if (cache_tree_update(&the_index, 0))
- return error(_("unable to update cache tree"));
+ if (!(cache_tree_oid = get_cache_tree_oid()))
+ return -1;
- return !oidcmp(&active_cache_tree->oid,
- &head_commit->tree->object.oid);
+ return !oidcmp(cache_tree_oid, &head_commit->tree->object.oid);
}
static int write_author_script(const char *message)
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH 4/6] sequencer: allow introducing new root commits
In the context of the new --rebase-merges mode, which was designed
specifically to allow for changing the existing branch topology
liberally, a user may want to extract commits into a completely fresh
branch that starts with a newly-created root commit.
This is now possible by inserting the command `reset [new root]` before
`pick`ing the commit that wants to become a root commit. Example:
reset [new root]
pick 012345 a commit that is about to become a root commit
pick 234567 this commit will have the previous one as parent
This does not conflict with other uses of the `reset` command because
`[new root]` is not (part of) a valid ref name: both the opening bracket
as well as the space are illegal in ref names.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 40
t/t3430-rebase-merges.sh | 34 ++
2 files changed, 62 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fc124596b53..d10ebd62520 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2727,18 +2727,34 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
return -1;
- /* Determine the length of the label */
- for (i = 0; i < len; i++)
- if (isspace(name[i]))
- len = i;
-
- strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
- if (get_oid(ref_name.buf, &oid) &&
- get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
- error(_("could not read '%s'"), ref_name.buf);
- rollback_lock_file(&lock);
- strbuf_release(&ref_name);
- return -1;
+ if (len == 10 && !strncmp("[new root]", name, len)) {
+ if (!opts->have_squash_onto) {
+ const char *hex;
+ if (commit_tree("", 0, the_hash_algo->empty_tree,
+ NULL, &opts->squash_onto,
+ NULL, NULL))
+ return error(_("writing fake root commit"));
+ opts->have_squash_onto = 1;
+ hex = oid_to_hex(&opts->squash_onto);
+ if (write_message(hex, strlen(hex),
+ rebase_path_squash_onto(), 0))
+ return error(_("writing squash-onto"));
+ }
+ oidcpy(&oid, &opts->squash_onto);
+ } else {
+ /* Determine the length of the label */
+ for (i = 0; i < len; i++)
+ if (isspace(name[i]))
+ len = i;
+
+ strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
+ if (get_oid(ref_name.buf, &oid) &&
+ get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
+ error(_("could not read '%s'"), ref_name.buf);
+ rollback_lock_file(&lock);
+ strbuf_release(&ref_name);
+ return -1;
+ }
}
memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7bec..35260862fcb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -241,4 +241,38 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
test_cmp_rev HEAD $before
'
+test_expect_success 'root commits' '
+ git checkout --orphan unrelated &&
+ (GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="r...@example.com" \
+test_commit second-root) &&
+ test_commit third-root &&
+ cat >script-from-scratch <<-\EOF &&
+ pick third-root
+ label first-branch
+ reset [new root]
+ pick second-root
+ merge first-branch # Merge the 3rd root
+ EOF
+ test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+ test_tick &&
+ git rebase -i --force --root -r &&
+ test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
+ test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
+ test $(git rev-parse second-root:second-root.t) = \
+ $(git rev-parse HEAD^:second-root.t) &&
+ test_cmp_graph HEAD <<-\EOF &&
+ * Merge the 3rd root
+ |\
+ | * third-root
+ * second-root
+ EOF
+
+ : fast forward if possible &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_might_fail git config --unset sequence.editor &&
+ test_tick &&
+ git rebase -i --root -r &&
+ test_cmp_rev HEAD $before
+'
+
test_done
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH 0/6] Let the sequencer handle `git rebase -i --root`
When I reimplemented the most performance-critical bits of the interactive rebase in the sequencer, to speed up `git rebase -i` particularly on Windows (even if the benefits are still quite notable on Linux or macOS), I punted on the --root part. I had always hoped that some other contributor (or I myself) would come back later to address the --root part in the sequencer, too, with the idea that this would move the last remaining complicated code from git-rebase--interactive.sh into sequencer.c, to facilitate converting the rest of git-rebase--interactive.sh. When I say "the last remaining complicated code", of course I neglect the --preserve-merges code, but as I worked hard on the --rebase-merges patch series with the intention to eventually deprecate and maybe even remove the --preserve-merges mode, I always implicitly assume that the --preserve-merges code will be moved into its own shell script (git-rebase--preserve-merges.sh, maybe?) and never be converted. So here goes: the patches to move the handling of --root into the sequencer. After two preparatory patches, the real conversion takes place in the third patch. After that, we take care of the --root related concerns that arise in conjunction with the --rebase-merges mode. As the --rebase-merges/--root patches overlap quite a bit (not so much in the code itself as in philosophical considerations such as "what should happen if you try to merge a branch into a new root", or the fact that the label/reset/merge commands make it desirable to be able to create a new root commit in the middle of a todo list), I had to consider in which order to contribute them. In the end, I decided to go with --rebase-merges first, so the --root patches are based on the --rebase-merges patch series. I consider this patch series a critical prerequisite for Alban's Google Summer of Code project to convert rebase -i into a builtin. Johannes Schindelin (6): sequencer: extract helper to update active_cache_tree sequencer: learn about the special "fake root commit" handling rebase -i --root: let the sequencer handle even the initial part sequencer: allow introducing new root commits rebase --rebase-merges: a "merge" into a new root is a fast-forward rebase --rebase-merges: root commits can be cousins, too git-rebase--interactive.sh| 4 +- sequencer.c | 186 ++ sequencer.h | 4 + t/t3404-rebase-interactive.sh | 19 ++- t/t3421-rebase-topology-linear.sh | 6 +- t/t3430-rebase-merges.sh | 72 6 files changed, 256 insertions(+), 35 deletions(-) base-commit: 673fb9cb8b5c7d57cb560b6ade45e419c8dd09fc Based-On: recreate-merges at https://github.com/dscho/git Fetch-Base-Via: git fetch https://github.com/dscho/git recreate-merges Published-As: https://github.com/dscho/git/releases/tag/sequencer-and-root-commits-v1 Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-and-root-commits-v1 -- 2.17.0.windows.1.33.gfcbb1fa0445
Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)
On 2018-04-26 19:23, Elijah Newren wrote: On Thu, Apr 26, 2018 at 10:13 AM, Torsten Bögershausen wrote: Hm, thanks for the report. I don't have a high sierra box, but I can probably get one. t0050 -should- pass automagically, so I feel that I can do something. Unless someone is faster of course. Sweet, thanks for taking a look. Is it possible that you run debug=t verbose=t ./t0050-filesystem.sh and send the output to me ? Sure. First, though, note that I can make it pass (or at least "not ok...TODO known breakage") with the following patch (may be whitespace-damaged by gmail): diff --git a/t/test-lib.sh b/t/test-lib.sh index 483c8d6d7..770b91f8c 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1106,12 +1106,7 @@ test_lazy_prereq UTF8_NFD_TO_NFC ' auml=$(printf "\303\244") aumlcdiar=$(printf "\141\314\210") >"$auml" && - case "$(echo *)" in - "$aumlcdiar") - true ;; - *) - false ;; - esac + stat "$aumlcdiar" >/dev/null 2>/dev/null Nicely analyzed and improved. The "stat" statement is technically correct. I think that a more git-style fix would be [] --- + test -r "$aumlcdiar" instead of the stat. I looked into the 2 known breakages. In short: they test use cases which are not sooo important for a user in practice, but do a good test if the code is broken. IOW: I can't see a need for immediate action. As you already did all the analyzes: Do you want to send a patch ?
[PATCH v6 10/11] technical/shallow: describe why shallow cannot use replace refs
It is tempting to do away with commit_graft altogether (in the long haul), now that grafts are deprecated. However, the shallow feature needs a couple of things that the replace refs cannot fulfill. Let's point that out in the documentation. Signed-off-by: Johannes Schindelin --- Documentation/technical/shallow.txt | 7 +++ 1 file changed, 7 insertions(+) diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt index 4ec721335d2..01dedfe9ffe 100644 --- a/Documentation/technical/shallow.txt +++ b/Documentation/technical/shallow.txt @@ -17,6 +17,13 @@ Each line contains exactly one SHA-1. When read, a commit_graft will be constructed, which has nr_parent < 0 to make it easier to discern from user provided grafts. +Note that the shallow feature could not be changed easily to +use replace refs: a commit containing a `mergetag` is not allowed +to be replaced, not even by a root commit. Such a commit can be +made shallow, though. Also, having a `shallow` file explicitly +listing all the commits made shallow makes it a *lot* easier to +do shallow-specific things such as to deepen the history. + Since fsck-objects relies on the library to read the objects, it honours shallow commits automatically. -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 11/11] Remove obsolete script to convert grafts to replace refs
The functionality is now implemented as `git replace
--convert-graft-file`.
Signed-off-by: Johannes Schindelin
---
contrib/convert-grafts-to-replace-refs.sh | 28 ---
1 file changed, 28 deletions(-)
delete mode 100755 contrib/convert-grafts-to-replace-refs.sh
diff --git a/contrib/convert-grafts-to-replace-refs.sh
b/contrib/convert-grafts-to-replace-refs.sh
deleted file mode 100755
index 0cbc917b8cf..000
--- a/contrib/convert-grafts-to-replace-refs.sh
+++ /dev/null
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# You should execute this script in the repository where you
-# want to convert grafts to replace refs.
-
-GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
-
-. $(git --exec-path)/git-sh-setup
-
-test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
-
-grep '^[^# ]' "$GRAFTS_FILE" |
-while read definition
-do
- if test -n "$definition"
- then
- echo "Converting: $definition"
- git replace --graft $definition ||
- die "Conversion failed for: $definition"
- fi
-done
-
-mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
- die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
-
-echo "Success!"
-echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!"
-echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 09/11] technical/shallow: stop referring to grafts
Now that grafts are deprecated, we should start to assume that readers have no idea what grafts are. So it makes more sense to make the description of the "shallow" feature stand on its own. Suggested-by: Eric Sunshine Helped-by: Junio Hamano Signed-off-by: Johannes Schindelin --- Documentation/technical/shallow.txt | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt index 5183b154229..4ec721335d2 100644 --- a/Documentation/technical/shallow.txt +++ b/Documentation/technical/shallow.txt @@ -8,15 +8,10 @@ repo, and therefore grafts are introduced pretending that these commits have no parents. * -The basic idea is to write the SHA-1s of shallow commits into -$GIT_DIR/shallow, and handle its contents like the contents -of $GIT_DIR/info/grafts (with the difference that shallow -cannot contain parent information). - -This information is stored in a new file instead of grafts, or -even the config, since the user should not touch that file -at all (even throughout development of the shallow clone, it -was never manually edited!). +$GIT_DIR/shallow lists commit object names and tells Git to +pretend as if they are root commits (e.g. "git log" traversal +stops after showing them; "git fsck" does not complain saying +the commits listed on their "parent" lines do not exist). Each line contains exactly one SHA-1. When read, a commit_graft will be constructed, which has nr_parent < 0 to make it easier -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 07/11] Deprecate support for .git/info/grafts
The grafts feature was a convenient way to "stitch together" ancient
history to the fresh start of linux.git.
Its implementation is, however, not up to Git's standards, as there are
too many ways where it can lead to surprising and unwelcome behavior.
For example, when pushing from a repository with active grafts, it is
possible to miss commits that have been "grafted out", resulting in a
broken state on the other side.
Also, the grafts feature is limited to "rewriting" commits' list of
parents, it cannot replace anything else.
The much younger feature implemented as `git replace` set out to remedy
those limitations and dangerous bugs.
Seeing as `git replace` is pretty mature by now (since 4228e8bc98
(replace: add --graft option, 2014-07-19) it can perform the graft
file's duties), it is time to deprecate support for the graft file, and
to retire it eventually.
Signed-off-by: Johannes Schindelin
Reviewed-by: Stefan Beller
---
advice.c | 2 ++
advice.h | 1 +
commit.c | 10 ++
t/t6001-rev-list-graft.sh | 9 +
4 files changed, 22 insertions(+)
diff --git a/advice.c b/advice.c
index 406efc183ba..4411704fd45 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,7 @@ int advice_rm_hints = 1;
int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
+int advice_graft_file_deprecated = 1;
static struct {
const char *name;
@@ -42,6 +43,7 @@ static struct {
{ "addembeddedrepo", &advice_add_embedded_repo },
{ "ignoredhook", &advice_ignored_hook },
{ "waitingforeditor", &advice_waiting_for_editor },
+ { "graftfiledeprecated", &advice_graft_file_deprecated },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 70568fa7922..9f5064e82a8 100644
--- a/advice.h
+++ b/advice.h
@@ -21,6 +21,7 @@ extern int advice_rm_hints;
extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
+extern int advice_graft_file_deprecated;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/commit.c b/commit.c
index 2952ec987c5..451d3ce8dfe 100644
--- a/commit.c
+++ b/commit.c
@@ -12,6 +12,7 @@
#include "prio-queue.h"
#include "sha1-lookup.h"
#include "wt-status.h"
+#include "advice.h"
static struct commit_extra_header *read_commit_extra_header_lines(const char
*buf, size_t len, const char **);
@@ -176,6 +177,15 @@ static int read_graft_file(const char *graft_file)
struct strbuf buf = STRBUF_INIT;
if (!fp)
return -1;
+ if (advice_graft_file_deprecated)
+ advise(_("Support for /info/grafts is deprecated\n"
+"and will be removed in a future Git version.\n"
+"\n"
+"Please use \"git replace --convert-graft-file\"\n"
+"to convert the grafts into replace refs.\n"
+"\n"
+"Turn this message off by running\n"
+"\"git config advice.graftFileDeprecated false\""));
while (!strbuf_getwholeline(&buf, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line(&buf);
diff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh
index 05ddc69cf2a..7504ba47511 100755
--- a/t/t6001-rev-list-graft.sh
+++ b/t/t6001-rev-list-graft.sh
@@ -110,4 +110,13 @@ do
"
done
+
+test_expect_success 'show advice that grafts are deprecated' '
+ git show HEAD 2>err &&
+ test_i18ngrep "git replace" err &&
+ test_config advice.graftFileDeprecated false &&
+ git show HEAD 2>err &&
+ test_i18ngrep ! "git replace" err
+'
+
test_done
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 08/11] filter-branch: stop suggesting to use grafts
The graft file is deprecated now, so let's use replace refs in the example in filter-branch's man page instead. Suggested-by: Eric Sunshine Signed-off-by: Johannes Schindelin --- Documentation/git-filter-branch.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt index b634043183b..1d4d2f86045 100644 --- a/Documentation/git-filter-branch.txt +++ b/Documentation/git-filter-branch.txt @@ -288,7 +288,7 @@ git filter-branch --parent-filter \ or even simpler: --- -echo "$commit-id $graft-id" >> .git/info/grafts +git replace --graft $commit-id $graft-id git filter-branch $graft-id..HEAD --- -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 05/11] replace: introduce --convert-graft-file
This option is intended to help with the transition away from the
now-deprecated graft file.
Signed-off-by: Johannes Schindelin
---
Documentation/git-replace.txt | 11 ++---
builtin/replace.c | 44 ++-
2 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index e5c57ae6ef4..246dc9943c2 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -11,6 +11,7 @@ SYNOPSIS
'git replace' [-f]
'git replace' [-f] --edit
'git replace' [-f] --graft [...]
+'git replace' [-f] --convert-graft-file
'git replace' -d ...
'git replace' [--format=] [-l []]
@@ -87,9 +88,13 @@ OPTIONS
content as except that its parents will be
[...] instead of 's parents. A replacement ref
is then created to replace with the newly created
- commit. See contrib/convert-grafts-to-replace-refs.sh for an
- example script based on this option that can convert grafts to
- replace refs.
+ commit. Use `--convert-graft-file` to convert a
+ `$GIT_DIR/info/grafts` file and use replace refs instead.
+
+--convert-graft-file::
+ Creates graft commits for all entries in `$GIT_DIR/info/grafts`
+ and deletes that file upon success. The purpose is to help users
+ with transitioning off of the now-deprecated graft file.
-l ::
--list ::
diff --git a/builtin/replace.c b/builtin/replace.c
index e57d3d187ed..35394ec1874 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -20,6 +20,7 @@ static const char * const git_replace_usage[] = {
N_("git replace [-f] "),
N_("git replace [-f] --edit "),
N_("git replace [-f] --graft [...]"),
+ N_("git replace [-f] --convert-graft-file"),
N_("git replace -d ..."),
N_("git replace [--format=] [-l []]"),
NULL
@@ -476,6 +477,38 @@ static int create_graft(int argc, const char **argv, int
force)
return replace_object_oid(old_ref, &old_oid, "replacement", &new_oid,
force);
}
+static int convert_graft_file(int force)
+{
+ const char *graft_file = get_graft_file();
+ FILE *fp = fopen_or_warn(graft_file, "r");
+ struct strbuf buf = STRBUF_INIT, err = STRBUF_INIT;
+ struct argv_array args = ARGV_ARRAY_INIT;
+
+ if (!fp)
+ return -1;
+
+ while (strbuf_getline(&buf, fp) != EOF) {
+ if (*buf.buf == '#')
+ continue;
+
+ argv_array_split(&args, buf.buf);
+ if (args.argc && create_graft(args.argc, args.argv, force))
+ strbuf_addf(&err, "\n\t%s", buf.buf);
+ argv_array_clear(&args);
+ }
+ fclose(fp);
+
+ strbuf_release(&buf);
+
+ if (!err.len)
+ return unlink_or_warn(graft_file);
+
+ warning(_("could not convert the following graft(s):\n%s"), err.buf);
+ strbuf_release(&err);
+
+ return -1;
+}
+
int cmd_replace(int argc, const char **argv, const char *prefix)
{
int force = 0;
@@ -487,6 +520,7 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
MODE_DELETE,
MODE_EDIT,
MODE_GRAFT,
+ MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
@@ -494,6 +528,7 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"),
MODE_DELETE),
OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"),
MODE_EDIT),
OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's
parents"), MODE_GRAFT),
+ OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert
existing graft file"), MODE_CONVERT_GRAFT_FILE),
OPT_BOOL_F('f', "force", &force, N_("replace the ref if it
exists"),
PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for
--edit")),
@@ -516,7 +551,8 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
if (force &&
cmdmode != MODE_REPLACE &&
cmdmode != MODE_EDIT &&
- cmdmode != MODE_GRAFT)
+ cmdmode != MODE_GRAFT &&
+ cmdmode != MODE_CONVERT_GRAFT_FILE)
usage_msg_opt("-f only makes sense when writing a replacement",
git_replace_usage, options);
@@ -549,6 +585,12 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
git_replace_usage, options);
return create_graft(argc, argv, force);
+ case MODE_CONVERT_GRAFT_FILE:
+ if (argc != 0)
+ usage_msg_opt("--convert-graft-file takes no argument",
+ git_replace_u
[PATCH v6 01/11] argv_array: offer to split a string by whitespace
This is a simple function that will interpret a string as a whitespace
delimited list of values, and add those values into the array.
Note: this function does not (yet) offer to split by arbitrary delimiters,
or keep empty values in case of runs of whitespace, or de-quote Unix shell
style. All fo this functionality can be added later, when and if needed.
Signed-off-by: Johannes Schindelin
---
argv-array.c | 20
argv-array.h | 2 ++
2 files changed, 22 insertions(+)
diff --git a/argv-array.c b/argv-array.c
index 5d370fa3366..cb5bcd2c064 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
array->argc--;
}
+void argv_array_split(struct argv_array *array, const char *to_split)
+{
+ while (isspace(*to_split))
+ to_split++;
+ for (;;) {
+ const char *p = to_split;
+
+ if (!*p)
+ break;
+
+ while (*p && !isspace(*p))
+ p++;
+ argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
+
+ while (isspace(*p))
+ p++;
+ to_split = p;
+ }
+}
+
void argv_array_clear(struct argv_array *array)
{
if (array->argv != empty_argv) {
diff --git a/argv-array.h b/argv-array.h
index 29056e49a12..750c30d2f2c 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,6 +19,8 @@ LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pushv(struct argv_array *, const char **);
void argv_array_pop(struct argv_array *);
+/* Splits by whitespace; does not handle quoted arguments! */
+void argv_array_split(struct argv_array *, const char *);
void argv_array_clear(struct argv_array *);
const char **argv_array_detach(struct argv_array *);
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 02/11] commit: Let the callback of for_each_mergetag return on error
This is yet another patch to be filed under the keyword "libification".
There is one subtle change in behavior here, where a `git log` that has
been asked to show the mergetags would now stop reporting the mergetags
upon the first failure, whereas previously, it would have continued to the
next mergetag, if any.
In practice, that change should not matter, as it is 1) uncommon to
perform octopus merges using multiple tags as merge heads, and 2) when the
user asks to be shown those tags, they really should be there.
Signed-off-by: Johannes Schindelin
---
builtin/replace.c | 8
commit.c | 8 +---
commit.h | 4 ++--
log-tree.c| 13 +++--
4 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 935647be6bd..245d3f4164e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -345,7 +345,7 @@ struct check_mergetag_data {
const char **argv;
};
-static void check_one_mergetag(struct commit *commit,
+static int check_one_mergetag(struct commit *commit,
struct commit_extra_header *extra,
void *data)
{
@@ -368,20 +368,20 @@ static void check_one_mergetag(struct commit *commit,
if (get_oid(mergetag_data->argv[i], &oid) < 0)
die(_("Not a valid object name: '%s'"),
mergetag_data->argv[i]);
if (!oidcmp(&tag->tagged->oid, &oid))
- return; /* found */
+ return 0; /* found */
}
die(_("original commit '%s' contains mergetag '%s' that is discarded; "
"use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
}
-static void check_mergetags(struct commit *commit, int argc, const char **argv)
+static int check_mergetags(struct commit *commit, int argc, const char **argv)
{
struct check_mergetag_data mergetag_data;
mergetag_data.argc = argc;
mergetag_data.argv = argv;
- for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+ return for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
}
static int create_graft(int argc, const char **argv, int force)
diff --git a/commit.c b/commit.c
index ca474a7c112..2952ec987c5 100644
--- a/commit.c
+++ b/commit.c
@@ -1288,17 +1288,19 @@ struct commit_extra_header
*read_commit_extra_headers(struct commit *commit,
return extra;
}
-void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
+int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
{
struct commit_extra_header *extra, *to_free;
+ int res = 0;
to_free = read_commit_extra_headers(commit, NULL);
- for (extra = to_free; extra; extra = extra->next) {
+ for (extra = to_free; !res && extra; extra = extra->next) {
if (strcmp(extra->key, "mergetag"))
continue; /* not a merge tag */
- fn(commit, extra, data);
+ res = fn(commit, extra, data);
}
free_commit_extra_headers(to_free);
+ return res;
}
static inline int standard_header_field(const char *field, size_t len)
diff --git a/commit.h b/commit.h
index 0fb8271665c..9000895ad91 100644
--- a/commit.h
+++ b/commit.h
@@ -291,10 +291,10 @@ extern const char *find_commit_header(const char *msg,
const char *key,
/* Find the end of the log message, the right place for a new trailer. */
extern int ignore_non_trailer(const char *buf, size_t len);
-typedef void (*each_mergetag_fn)(struct commit *commit, struct
commit_extra_header *extra,
+typedef int (*each_mergetag_fn)(struct commit *commit, struct
commit_extra_header *extra,
void *cb_data);
-extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void
*data);
+extern int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void
*data);
struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
diff --git a/log-tree.c b/log-tree.c
index d1c0bedf244..f3a51a6e726 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -488,9 +488,9 @@ static int is_common_merge(const struct commit *commit)
&& !commit->parents->next->next);
}
-static void show_one_mergetag(struct commit *commit,
- struct commit_extra_header *extra,
- void *data)
+static int show_one_mergetag(struct commit *commit,
+struct commit_extra_header *extra,
+void *data)
{
struct rev_info *opt = (struct rev_info *)data;
struct object_id oid;
@@ -502,7 +502,7 @@ static void show_one_mergetag(struct commit *commit,
hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid);
tag = lookup_tag(&oid);
if (!tag)
- return; /* error message already given */
+
[PATCH v6 04/11] replace: "libify" create_graft() and callees
File this away as yet another patch in the "libification" category.
As with all useful functions, in the next commit we want to use
create_graft() from a higher-level function where it would be
inconvenient if the called function simply die()s: if there is a
problem, we want to let the user know how to proceed, and the callee
simply has no way of knowing what to say.
Signed-off-by: Johannes Schindelin
---
builtin/replace.c | 169 ++
1 file changed, 112 insertions(+), 57 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index e345a5a0f1c..e57d3d187ed 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char
*format)
else if (!strcmp(format, "long"))
data.format = REPLACE_FORMAT_LONG;
else
- die("invalid replace format '%s'\n"
- "valid formats are 'short', 'medium' and 'long'\n",
- format);
+ return error("invalid replace format '%s'\n"
+"valid formats are 'short', 'medium' and 'long'\n",
+format);
for_each_replace_ref(show_reference, (void *)&data);
@@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char
*ref,
return 0;
}
-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
struct object_id *prev,
struct strbuf *ref,
int force)
@@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object,
strbuf_reset(ref);
strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object));
if (check_refname_format(ref->buf, 0))
- die("'%s' is not a valid ref name.", ref->buf);
+ return error("'%s' is not a valid ref name.", ref->buf);
if (read_ref(ref->buf, prev))
oidclr(prev);
else if (!force)
- die("replace ref '%s' already exists", ref->buf);
+ return error("replace ref '%s' already exists", ref->buf);
+ return 0;
}
static int replace_object_oid(const char *object_ref,
@@ -161,28 +162,33 @@ static int replace_object_oid(const char *object_ref,
struct strbuf ref = STRBUF_INIT;
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+ int res = 0;
obj_type = oid_object_info(object, NULL);
repl_type = oid_object_info(repl, NULL);
if (!force && obj_type != repl_type)
- die("Objects must be of the same type.\n"
- "'%s' points to a replaced object of type '%s'\n"
- "while '%s' points to a replacement object of type '%s'.",
- object_ref, type_name(obj_type),
- replace_ref, type_name(repl_type));
-
- check_ref_valid(object, &prev, &ref, force);
+ return error("Objects must be of the same type.\n"
+"'%s' points to a replaced object of type '%s'\n"
+"while '%s' points to a replacement object of "
+"type '%s'.",
+object_ref, type_name(obj_type),
+replace_ref, type_name(repl_type));
+
+ if (check_ref_valid(object, &prev, &ref, force)) {
+ strbuf_release(&ref);
+ return -1;
+ }
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, repl, &prev,
0, NULL, &err) ||
ref_transaction_commit(transaction, &err))
- die("%s", err.buf);
+ res = error("%s", err.buf);
ref_transaction_free(transaction);
strbuf_release(&ref);
- return 0;
+ return res;
}
static int replace_object(const char *object_ref, const char *replace_ref, int
force)
@@ -190,9 +196,11 @@ static int replace_object(const char *object_ref, const
char *replace_ref, int f
struct object_id object, repl;
if (get_oid(object_ref, &object))
- die("Failed to resolve '%s' as a valid ref.", object_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+object_ref);
if (get_oid(replace_ref, &repl))
- die("Failed to resolve '%s' as a valid ref.", replace_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+replace_ref);
return replace_object_oid(object_ref, &object, replace_ref, &repl,
force);
}
@@ -202,7 +210,7 @@ static int replace_object(const char *object_ref, const
char *replace_ref, int f
* If "raw" is true, then the object's raw contents are printed according to
* "type
[PATCH v6 00/11] Deprecate .git/info/grafts
It is fragile, as there is no way for the revision machinery to say "but
now I want to traverse the graph ignoring the graft file" e.g. when
pushing commits to a remote repository (which, as a consequence, can
miss commits).
And we already have a better solution with `git replace --graft
[...]`.
Changes since v5:
- Disentangled the lumped-together conditional blocks in
edit_and_replace() again.
- Moved fixup (a superfluous argv_array_clear()) from the patch that
adds a test for --convert-graft-file back to the patch that actually
introduces that option.
Johannes Schindelin (11):
argv_array: offer to split a string by whitespace
commit: Let the callback of for_each_mergetag return on error
replace: avoid using die() to indicate a bug
replace: "libify" create_graft() and callees
replace: introduce --convert-graft-file
Add a test for `git replace --convert-graft-file`
Deprecate support for .git/info/grafts
filter-branch: stop suggesting to use grafts
technical/shallow: stop referring to grafts
technical/shallow: describe why shallow cannot use replace refs
Remove obsolete script to convert grafts to replace refs
Documentation/git-filter-branch.txt | 2 +-
Documentation/git-replace.txt | 11 +-
Documentation/technical/shallow.txt | 20 +-
advice.c | 2 +
advice.h | 1 +
argv-array.c | 20 ++
argv-array.h | 2 +
builtin/replace.c | 223 --
commit.c | 18 +-
commit.h | 4 +-
contrib/convert-grafts-to-replace-refs.sh | 28 ---
log-tree.c| 13 +-
t/t6001-rev-list-graft.sh | 9 +
t/t6050-replace.sh| 20 ++
14 files changed, 258 insertions(+), 115 deletions(-)
delete mode 100755 contrib/convert-grafts-to-replace-refs.sh
base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v6
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v6
Interdiff vs v5:
diff --git a/builtin/replace.c b/builtin/replace.c
index acd30e3d824..35394ec1874 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -326,10 +326,15 @@ static int edit_and_replace(const char *object_ref, int
force, int raw)
strbuf_release(&ref);
tmpfile = git_pathdup("REPLACE_EDITOBJ");
- if (export_object(&old_oid, type, raw, tmpfile) ||
- (launch_editor(tmpfile, NULL, NULL) < 0 &&
- error("editing object file failed")) ||
- import_object(&new_oid, type, raw, tmpfile)) {
+ if (export_object(&old_oid, type, raw, tmpfile)) {
+ free(tmpfile);
+ return -1;
+ }
+ if (launch_editor(tmpfile, NULL, NULL) < 0) {
+ free(tmpfile);
+ return error("editing object file failed");
+ }
+ if (import_object(&new_oid, type, raw, tmpfile)) {
free(tmpfile);
return -1;
}
--
2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 06/11] Add a test for `git replace --convert-graft-file`
The proof, as the saying goes, lies in the pudding. So here is a regression test that not only demonstrates what the option is supposed to accomplish, but also demonstrates that it does accomplish it. Signed-off-by: Johannes Schindelin --- t/t6050-replace.sh | 20 1 file changed, 20 insertions(+) diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index c630aba657e..bed86a0af3d 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a mergetag' ' git replace -d $HASH10 ' +test_expect_success '--convert-graft-file' ' + : add and convert graft file && + printf "%s\n%s %s\n\n# comment\n%s\n" \ + $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \ + >.git/info/grafts && + git replace --convert-graft-file && + test_path_is_missing .git/info/grafts && + + : verify that the history is now "grafted" && + git rev-list HEAD >out && + test_line_count = 4 out && + + : create invalid graft file and verify that it is not deleted && + test_when_finished "rm -f .git/info/grafts" && + echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts && + test_must_fail git replace --convert-graft-file 2>err && + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" err && + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts +' + test_done -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v6 03/11] replace: avoid using die() to indicate a bug
We have the BUG() macro for that purpose.
Signed-off-by: Johannes Schindelin
---
builtin/replace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
return list_replace_refs(argv[0], format);
default:
- die("BUG: invalid cmdmode %d", (int)cmdmode);
+ BUG("invalid cmdmode %d", (int)cmdmode);
}
}
--
2.17.0.windows.1.33.gfcbb1fa0445
Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash
Hi Phillip,
On Sat, 21 Apr 2018, Phillip Wood wrote:
> On 20/04/18 13:18, Johannes Schindelin wrote:
> >
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> >
> > However, if the last of these fixup/squash commands fails with merge
> > conflicts, and if the user then decides to skip it (or resolve it to a
> > clean worktree and then continue the rebase), the current code fails to
> > clean up the commit message.
>
> Thanks for taking the time to track this down and fix it.
It was on my mind, but since I got caught twice by this bug within a week,
I figured it was about time.
> > diff --git a/sequencer.c b/sequencer.c
> > index a9c3bc26f84..f067b7b24c5 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2781,17 +2781,12 @@ static int continue_single_pick(void)
> >
> > static int commit_staged_changes(struct replay_opts *opts)
> > {
> > - unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> > + unsigned int flags = ALLOW_EMPTY | EDIT_MSG, is_fixup = 0, is_clean;
> >
> >if (has_unstaged_changes(1))
> > return error(_("cannot rebase: You have unstaged changes."));
> > - if (!has_uncommitted_changes(0)) {
> > - const char *cherry_pick_head = git_path_cherry_pick_head();
> > - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> > - return error(_("could not remove CHERRY_PICK_HEAD"));
> > - return 0;
> > - }
> > + is_clean = !has_uncommitted_changes(0);
> >
> >if (file_exists(rebase_path_amend())) {
> > struct strbuf rev = STRBUF_INIT;
> > @@ -2804,16 +2799,41 @@ static int commit_staged_changes(struct replay_opts
> > *opts)
> > if (get_oid_hex(rev.buf, &to_amend))
> > return error(_("invalid contents: '%s'"),
> > rebase_path_amend());
> > - if (oidcmp(&head, &to_amend))
> > + if (!is_clean && oidcmp(&head, &to_amend))
> > return error(_("\nYou have uncommitted changes in your "
> > "working tree. Please, commit them\n"
> > "first and then run 'git rebase "
> > "--continue' again."));
> > + if (is_clean && !oidcmp(&head, &to_amend)) {
>
> Looking at pick_commits() it only writes to rebase_path_amend() if there are
> conflicts, not if the command has been rescheduled so this is safe.
This is indeed the intent of that file.
> > + strbuf_reset(&rev);
> > + /*
> > +* Clean tree, but we may need to finalize a
> > +* fixup/squash chain. A failed fixup/squash leaves
> > the
> > +* file amend-type in rebase-merge/; It is okay if
> > that
> > +* file is missing, in which case there is no such
> > +* chain to finalize.
> > +*/
> > + read_oneliner(&rev, rebase_path_amend_type(), 0);
> > + if (!strcmp("squash", rev.buf))
> > + is_fixup = TODO_SQUASH;
> > + else if (!strcmp("fixup", rev.buf)) {
> > + is_fixup = TODO_FIXUP;
> > + flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;
>
> I was going to say this should probably be (flags & ~(EDIT_MSG | VERIFY_MSG))
> but for some reason VERIFY_MSG isn't set here - I wonder if it should be as I
> think it's set elsewhere when we edit the message.
As this patch series is purely about the bug fix where interrupted
fixup/squash series can lead to incorrect commit messages, I would say
that if this is a bug, it should be fixed in a separate patch series.
The name of that option is actually a little bit unfortunate: it bypasses
the pre-commit/commit-msg hooks. I am not sure why they are bypassed in
the commit_staged_changes() function.
*clickety-click*
It would appear that I simply copied this from
https://github.com/git/git/blob/v2.17.0/git-rebase--interactive.sh#L794-L808
So where does that come from? Let's use `git log -L
794,808:git-rebase--interactive.sh v2.17.0` to find out.
*clickety-click*
>From that log, it looks as if this was added in 2147f844ed1 (rebase -i: handle
fixup of root commit correctly, 2012-07-24). But that is incorrect: the
--no-verify invocation was only split into two by said commit, and moved
into the conditional. So we need to look a little further, with a larger
line range (I extended it to 810 for the purpose of this analysis).
*clickety-click*
So it goes all the way back to c5b09feb786 (Avoid update hook during
git-rebase --interactive, 2007-12-19). From that commit message, you can
see that the rationale for the --no-verify flag was as following: the `git
commit` might fail when continuing with staged changes, due to a check in
a hook, and since there is inadequate error c
Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash
On Fri, Apr 27, 2018 at 1:48 PM, Johannes Schindelin wrote: > During a series of fixup/squash commands, the interactive rebase builds > up a commit message with comments. This will be presented to the user in > the editor if at least one of those commands was a `squash`. This sounds as if the whole series will be presented to the user, i.e. pick A squash B fixup C would present A+B+C in the editor. I always assumed the sequencer to be linear, i.e. pick A+B, open editor and then fixup C into the previous result? No need to resend it reworded, I just realize that I never tested my potentially wrong assumption. > The diff is best viewed with --color-moved. ... and web pages are "best viewed with IE 6.0" ;-) I found this so funny that I had to download the patches and actually look at them using the move detection only to find out that only very few lines are moved, as there are only very few deleted lines.
Re: [PATCH 18/41] index-pack: abstract away hash function constant
On Thu, Apr 26, 2018 at 05:46:28PM +0200, Duy Nguyen wrote: > On Wed, Apr 25, 2018 at 8:49 PM, Martin Ågren wrote: > > Once that is accomplished, I sort of suspect that this code will want to > > be updated to not always blindly use the_hash_algo, but to always work > > with SHA-1 sizes. Or rather, this would turn into more generic code to > > handle both "v2 with SHA-1" and "v3 with some hash function(s)". This > > commit might be a good first step in that direction. > > I also have an uneasy feeling when things this close to on-disk file > format get hash-agnostic treatment. I think we would need to start > adding new file formats soon, from bottom up with simple things like > loose object files (cat-file and hash-object should be enough to test > blobs...), then moving up to pack files and more. This is when we can > really decide where to use the new hash and whether we should keep > some hashes as sha-1. I agree that this is work which needs to be done soon. There are basically a couple of pieces we need to handle NewHash: * Remove the dependencies on SHA-1 as much as possible. * Get the tests to pass with a different hash (almost done for 160-bit hash; in progress for 256-bit hashes). * Write pack code. * Write loose object index code. * Write read-as-SHA-1 code. * Force the codebase to always use SHA-1 when dealing with fetch/push. * Distinguish between code which needs to use compatObjectFormat and code which needs to use objectFormat. * Decide on NewHash. I'm working on the top two bullet points right now. Others are welcome to pick up other pieces, or I'll get to them eventually. As much as I'm dreading having the bikeshedding discussion over what we're going to pick for NewHash, some of these pieces require knowing what algorithm it will be. For example, we have some tests which either need to be completely rewritten or have a translation table written for them (think the ones that use colliding short names). In order for those tests to have the translation table written, we need to be able to compute colliding values. I'm annotating these with prerequisites, but there are quite a few tests which are skipped. I expect writing the pack, loose object index, and read-as-SHA-1 code is going to require having some code for NewHash or stand-in present in order for it to compile and be tested. It's possible that others could come up with more imaginative solutions that don't require that, but I have my doubts. > For trailing hashes for example, there's no need to move to a new hash > which only costs us more cycles. We just use it as a fancy checksum to > avoid bit flips. But then my assumption about cost may be completely > wrong without experimenting. I would argue that consistency is helpful. Also, do we really want people to be able to (eventually) create colliding packs that contain different data? That doesn't seem like a good idea. But also, some of the candidates we're considering for NewHash are actually faster than SHA-1. So for performance reasons alone, it might be useful to adopt a consistent scheme. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [RFC PATCH] checkout: Force matching mtime between files
On 2018-04-27 01:03 PM, Duy Nguyen wrote:
On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud wrote:
The best approach to do so is to have those people do the "touch"
thing in their own post-checkout hook. People who use Git as the
source control system won't have to pay runtime cost of doing the
touch thing, and we do not have to maintain such a hook script.
Only those who use the "feature" would.
The post-checkout hook approach is not exactly straightforward.
I am revisiting this because I'm not even happy with my
post-checkout-modified hook suggestion, so..
Naively, it's simply
for F in `git diff --name-only $1 $2`; do touch "$F"; done
But consider:
* Symlinks can cause the wrong file to be touched. (Granted, Michał's
proposed patch also doesn't deal with symlinks.) Let's assume that a hook
can be crafted will all possible sophistication. There are still some
fundamental problems:
OK so this one could be tricky to get right, but it's possible.
* In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
identical so the above loop does nothing. Offhand I'm not even sure how a
hook might get the right files in this case.
This is a limitation of the current post-checkout hook. $3==0 from the
hook lets us know this is not a branch switch, but it does not really
tell you the affected paths. If it somehow passes all the given
pathspec to you, then you should be able to do "git ls-files --
$pathspec" which gives you the exact same set of paths that
git-checkout updates. We could do this by setting $4 to "--" and put
all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
the above example.
There is third case here, if you do "git checkout --
path/to/file" then it cannot be covered by the current design. I guess
we could set $3 to '2' (retrieve from a tree) to indicate this in
addition to 0 (from index) and 1 (from switching branch) and then $1
could be the tree in question (pathspecs are passed the same way
above)
[1] I wonder if we could have a more generic approach to pass
pathspecs via environment, which could work for more than just this
one hook. Not sure if it's a good idea though.
I think there needs to be something other than listing all the paths in
the command is viable, because it's too easy to hit some
command-line-length limit. It would also be good if hook authors didn't
have to re-invent the wheel of determining the changed paths for every
corner-case.
My first instinct is to write them one-per-line on the hook's stdin.
That's probably not generic enough for most hooks, but it seems like a
good approach for this proposal.
Throwing them into a temporary file with a known name is also good ---
better, I think, than stuffing them into an environment variable.
M.
* The hook has to be set up in every repo and submodule (at least until
something like Ævar's experiments come to fruition).
Either --template or core.hooksPath would solve this, or I'll try to
get my "hooks.* config" patch in. I think that one is a good thing to
do anyway because it allows more flexible hook management (and it
could allow multiple hooks of the same name too). With this, we could
avoid adding more command-specific config like core.fsmonitor or
uploadpack.packObjectsHook which to me are hooks.
Another option is extend core.hooksPath for searching multiple places
instead of just one like AEvar suggested.
* A fresh clone can't run the hook. This is especially important when
dealing with submodules. (In one case where we were bit by this, make
though that half of a fresh submodule clone's files were stale, and decided
to re-autoconf the entire thing.)
Both --template and config-based hooks should let you handle this case.
So, I think if we improve the hook system to give more information
(which is something we definitely should do), we could do this without
adding special cases in git. I'm not saying that we should never add
special cases, but at least this lets us play with different kinds of
post-checkout activities before we decide which one would be best
implemented in git.
Re: [RFC PATCH] checkout: Force matching mtime between files
On Fri, Apr 27, 2018 at 10:03 AM, Duy Nguyen wrote:
> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud wrote:
>>
>> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
>> identical so the above loop does nothing. Offhand I'm not even sure how a
>> hook might get the right files in this case.
>
> This is a limitation of the current post-checkout hook. $3==0 from the
> hook lets us know this is not a branch switch, but it does not really
> tell you the affected paths. If it somehow passes all the given
> pathspec to you, then you should be able to do "git ls-files --
> $pathspec" which gives you the exact same set of paths that
> git-checkout updates. We could do this by setting $4 to "--" and put
> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
> the above example.
>
> There is third case here, if you do "git checkout --
> path/to/file" then it cannot be covered by the current design. I guess
> we could set $3 to '2' (retrieve from a tree) to indicate this in
> addition to 0 (from index) and 1 (from switching branch) and then $1
> could be the tree in question (pathspecs are passed the same way
> above)
>
> [1] I wonder if we could have a more generic approach to pass
> pathspecs via environment, which could work for more than just this
> one hook. Not sure if it's a good idea though.
Here's a crazy idea -- maybe instead of a list of pathspecs you just
provide the timestamp of when git checkout started. Then the hook
could walk the tree, find all files with modification times at least
that late, and modify them all back to the the timestamp of when the
git checkout started.
Would that be enough? Is that too crazy?
Sure, people could concurrently edit a file or run another program
that modified files, but if you're doing that you're already playing
race games with whether your next incremental build is going to be
able to be correct. (Some (annoying) IDEs explicitly lock you out
from editing files during a build to attempt to avoid this very
problem.)
That does leave one other caveat: If people intentionally do really
weird stuff with having files with modification timestamps far in the
future. However, it seems likely that the group of people doing that,
if non-zero in number, is likely to be dis-joint with the group of
folks that want this special
uniform-timestamp-across-files-in-a-checkout behavior.
Re: [PATCH v5 00/11] Deprecate .git/info/grafts
Hi Junio,
On Thu, 26 Apr 2018, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > - if (export_object(&old_oid, type, raw, tmpfile))
> > - return -1;
> > - if (launch_editor(tmpfile, NULL, NULL) < 0)
> > - return error("editing object file failed");
> > - if (import_object(&new_oid, type, raw, tmpfile))
> > + tmpfile = git_pathdup("REPLACE_EDITOBJ");
> > + if (export_object(&old_oid, type, raw, tmpfile) ||
> > + (launch_editor(tmpfile, NULL, NULL) < 0 &&
> > + error("editing object file failed")) ||
> > + import_object(&new_oid, type, raw, tmpfile)) {
> > + free(tmpfile);
> > return -1;
> > -
> > + }
>
> I know the above is to avoid leaking tmpfile, but a single if ()
> condition that makes multiple calls to functions primarily for their
> side effects is too ugly to live.
I changed it back to individual conditional blocks, with every single one
of them having their own free(tmpfile). That is at least clearer.
Ciao,
Dscho
[PATCH v4 1/4] rebase -i: demonstrate bugs with fixup!/squash! commit messages
When multiple fixup/squash commands are processed and the last one causes merge conflicts and is skipped, we leave the "This is a combination of ..." comments in the commit message. Noticed by Eric Sunshine. This regression test also demonstrates that we rely on the localized version of # This is a combination of commits to contain the in ASCII, which breaks under GETTEXT_POISON. Signed-off-by: Johannes Schindelin --- t/t3418-rebase-continue.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 9214d0bb511..3874f187246 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -88,6 +88,28 @@ test_expect_success 'rebase passes merge strategy options correctly' ' git rebase --continue ' +test_expect_failure '--skip after failed fixup cleans commit message' ' + test_when_finished "test_might_fail git rebase --abort" && + git checkout -b with-conflicting-fixup && + test_commit wants-fixup && + test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 && + test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 && + test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 && + test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \ + git rebase -i HEAD~4 && + + : now there is a conflict, and comments in the commit message && + git show HEAD >out && + grep "fixup! wants-fixup" out && + + : skip and continue && + git rebase --skip && + + : now the comments in the commit message should have been cleaned up && + git show HEAD >out && + ! grep "fixup! wants-fixup" out +' + test_expect_success 'setup rerere database' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v4 0/4] rebase -i: avoid stale "# This is a combination of" in commit messages
Eric Sunshine pointed out that I had such a commit message in https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/ and I went on a hunt to figure out how the heck this happened. Turns out that if there is a fixup/squash chain where the *last* command fails with merge conflicts, and we either --skip ahead or resolve the conflict to a clean tree and then --continue, our code does not do a final cleanup. Contrary to my initial gut feeling, this bug was not introduced by my rewrite in C of the core parts of rebase -i, but it looks to me as if that bug was with us for a very long time (at least the --skip part). The developer (read: user of rebase -i) in me says that we would want to fast-track this, but the author of rebase -i in me says that we should be cautious and cook this in `next` for a while. Fixes since v3 (thanks, Phillip, for the really fruitful discussion!): - We now avoid using the commit message prepared for the skipped fixup/squash. - Replaced the "rebase -i: Handle "combination of commits" with GETTEXT_POISON" patch by a *real* fix instead of a work-around: Instead of parsing the first line of the commit message and punting when it is missing an ASCII-encoded number, we determine separately (independent from any localized text). - Fixed quite a couple more corner cases, using the `current-fixups` file introduced for the GETTEXT_POISON fix: * we only need to re-commit if this was the final fixup/squash in the fixup/squash chain, * we only need to commit interactively if there was *any* non-skipped squash, * if the fixup/squash chain continues, the was incorrect in the "This is a combination of commits" comment in the intermediate commit message (it included the now-skipped commits), and * even if a filed fixup/squash in the middle of a fixup/squash chain failed, and its merge conflicts were resolved and committed, the "This is a combination of commits" comment was incorrect: we had already deleted message-fixup and message-squash, so the next update_squash_message() would mistakenly assume that we were starting afresh. Worse: if only fixup commands were remaining, but there had been a squash command, we would retain the "squash!" line in the commit message and not give the user a chance to clean things up in the final fixup! Johannes Schindelin (4): rebase -i: demonstrate bugs with fixup!/squash! commit messages rebase -i: Handle "combination of commits" with GETTEXT_POISON sequencer: always commit without editing when asked for rebase --skip: clean up commit message after a failed fixup/squash sequencer.c| 193 - sequencer.h| 6 +- t/t3418-rebase-continue.sh | 49 ++ 3 files changed, 200 insertions(+), 48 deletions(-) base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d Published-As: https://github.com/dscho/git/releases/tag/clean-msg-after-fixup-continue-v4 Fetch-It-Via: git fetch https://github.com/dscho/git clean-msg-after-fixup-continue-v4 Interdiff vs v3: diff --git a/sequencer.c b/sequencer.c index e1efb0ebf31..cec180714ef 100644 --- a/sequencer.c +++ b/sequencer.c @@ -74,13 +74,6 @@ static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message") * previous commit and from the first squash/fixup commit are written * to it. The commit message for each subsequent squash/fixup commit * is appended to the file as it is processed. - * - * The first line of the file is of the form - * # This is a combination of $count commits. - * where $count is the number of commits whose messages have been - * written to the file so far (including the initial "pick" commit). - * Each time that a commit message is processed, this line is read and - * updated. It is deleted just before the combined commit is made. */ static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash") /* @@ -91,6 +84,11 @@ static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash") * commit without opening the editor.) */ static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup") +/* + * This file contains the list fixup/squash commands that have been + * accumulated into message-fixup or message-squash so far. + */ +static GIT_PATH_FUNC(rebase_path_current_fixups, "rebase-merge/current-fixups") /* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and * GIT_AUTHOR_DATE that will be used for the commit that is currently @@ -106,13 +104,6 @@ static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script") * command is processed, this file is deleted. */ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") -/* - * If there was a merge conflict in a fixup/squash series, we need to - * record the type so that a `git rebase --skip` can clean up the commit - * message as app
[PATCH v4 3/4] sequencer: always commit without editing when asked for
Previously, we only called run_git_commit() without EDIT_MSG when we also passed in a default message. However, an upcoming caller will want to commit without EDIT_MSG and *without* a default message: to clean up fixup/squash comments in HEAD's commit message. Let's prepare for that. Signed-off-by: Johannes Schindelin --- sequencer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sequencer.c b/sequencer.c index d2e6f33023d..56166b0d6c7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -717,6 +717,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign); if (defmsg) argv_array_pushl(&cmd.args, "-F", defmsg, NULL); + else if (!(flags & EDIT_MSG)) + argv_array_pushl(&cmd.args, "-C", "HEAD", NULL); if ((flags & CLEANUP_MSG)) argv_array_push(&cmd.args, "--cleanup=strip"); if ((flags & EDIT_MSG)) -- 2.17.0.windows.1.33.gfcbb1fa0445
[PATCH v4 2/4] rebase -i: Handle "combination of commits" with GETTEXT_POISON
We previously relied on the localized versions of
# This is a combination of commits
(which we write into the commit messages during fixup/squash chains)
to contain encoded in ASCII.
This is not true in general, and certainly not true when compiled with
GETTEXT_POISON=TryToKillMe, as demonstrated by the regression test we
just introduced in t3418.
So let's decouple keeping track of the count from the (localized) commit
messages by introducing a new file called 'current-fixups' that keeps
track of the current fixup/squash chain. This file contains a bit more
than just the count (it contains a list of "fixup "/"squash
" lines). This is done on purpose, as it will come in handy for
a fix for the bug where `git rebase --skip` on a final fixup/squash will
leave the commit message in limbo.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 78 ++---
sequencer.h | 6 -
2 files changed, 49 insertions(+), 35 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5e3a50fafc9..d2e6f33023d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -74,13 +74,6 @@ static GIT_PATH_FUNC(rebase_path_message,
"rebase-merge/message")
* previous commit and from the first squash/fixup commit are written
* to it. The commit message for each subsequent squash/fixup commit
* is appended to the file as it is processed.
- *
- * The first line of the file is of the form
- * # This is a combination of $count commits.
- * where $count is the number of commits whose messages have been
- * written to the file so far (including the initial "pick" commit).
- * Each time that a commit message is processed, this line is read and
- * updated. It is deleted just before the combined commit is made.
*/
static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
/*
@@ -91,6 +84,11 @@ static GIT_PATH_FUNC(rebase_path_squash_msg,
"rebase-merge/message-squash")
* commit without opening the editor.)
*/
static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
+/*
+ * This file contains the list fixup/squash commands that have been
+ * accumulated into message-fixup or message-squash so far.
+ */
+static GIT_PATH_FUNC(rebase_path_current_fixups, "rebase-merge/current-fixups")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
@@ -253,6 +251,7 @@ int sequencer_remove_state(struct replay_opts *opts)
for (i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
free(opts->xopts);
+ strbuf_release(&opts->current_fixups);
strbuf_addstr(&dir, get_dir(opts));
remove_dir_recursively(&dir, 0);
@@ -1329,34 +1328,23 @@ static int update_squash_messages(enum todo_command
command,
struct commit *commit, struct replay_opts *opts)
{
struct strbuf buf = STRBUF_INIT;
- int count, res;
+ int res;
const char *message, *body;
- if (file_exists(rebase_path_squash_msg())) {
+ if (opts->current_fixup_count > 0) {
struct strbuf header = STRBUF_INIT;
- char *eol, *p;
+ char *eol;
- if (strbuf_read_file(&buf, rebase_path_squash_msg(), 2048) <= 0)
+ if (strbuf_read_file(&buf, rebase_path_squash_msg(), 9) <= 0)
return error(_("could not read '%s'"),
rebase_path_squash_msg());
- p = buf.buf + 1;
- eol = strchrnul(buf.buf, '\n');
- if (buf.buf[0] != comment_line_char ||
- (p += strcspn(p, "0123456789\n")) == eol)
- return error(_("unexpected 1st line of squash message:"
- "\n\n\t%.*s"),
-(int)(eol - buf.buf), buf.buf);
- count = strtol(p, NULL, 10);
-
- if (count < 1)
- return error(_("invalid 1st line of squash message:\n"
- "\n\t%.*s"),
-(int)(eol - buf.buf), buf.buf);
+ eol = buf.buf[0] != comment_line_char ?
+ buf.buf : strchrnul(buf.buf, '\n');
strbuf_addf(&header, "%c ", comment_line_char);
- strbuf_addf(&header,
- _("This is a combination of %d commits."), ++count);
+ strbuf_addf(&header, _("This is a combination of %d commits."),
+ opts->current_fixup_count + 2);
strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
strbuf_release(&header);
} else {
@@ -1379,10 +1367,8 @@ static int update_squash_messages(enum todo_command
command,
rebase_path_fixup_msg());
}
- count = 2;
strbuf_addf(&buf, "%c
[PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash
During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.
In any case, the commit message will be cleaned up eventually, removing
all those intermediate comments, in the final step of such a
fixup/squash chain.
However, if the last fixup/squash command in such a chain fails with
merge conflicts, and if the user then decides to skip it (or resolve it
to a clean worktree and then continue the rebase), the current code
fails to clean up the commit message.
This commit fixes that behavior.
The fix is quite a bit more involved than meets the eye because it is
not only about the question whether we are `git rebase --skip`ing a
fixup or squash. It is also about removing the skipped fixup/squash's
commit message from the accumulated commit message. And it is also about
the question whether we should let the user edit the final commit
message or not ("Was there a squash in the chain *that was not
skipped*?").
For example, in this case we will want to fix the commit message, but
not open it in an editor:
pick<- succeeds
fixup <- succeeds
squash <- fails, will be skipped
This is where the newly-introduced `current-fixups` file comes in real
handy. A quick look and we can determine whether there was a non-skipped
squash. We only need to make sure to keep it up to date with respect to
skipped fixup/squash commands. As a bonus, we can even avoid committing
unnecessarily, e.g. when there was only one fixup, and it failed, and
was skipped.
To fix only the bug where the final commit message was not cleaned up
properly, but without fixing the rest, would have been more complicated
than fixing it all in one go, hence this commit lumps together more than
a single concern.
For the same reason, this commit also adds a bit more to the existing
test case for the regression we just fixed.
The diff is best viewed with --color-moved.
Signed-off-by: Johannes Schindelin
---
sequencer.c| 113 -
t/t3418-rebase-continue.sh | 35 ++--
2 files changed, 131 insertions(+), 17 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 56166b0d6c7..cec180714ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
}
-static int commit_staged_changes(struct replay_opts *opts)
+static int commit_staged_changes(struct replay_opts *opts,
+struct todo_list *todo_list)
{
unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
+ unsigned int final_fixup = 0, is_clean;
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
- if (!has_uncommitted_changes(0)) {
- const char *cherry_pick_head = git_path_cherry_pick_head();
- if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
- return error(_("could not remove CHERRY_PICK_HEAD"));
- return 0;
- }
+ is_clean = !has_uncommitted_changes(0);
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
@@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts
*opts)
if (get_oid_hex(rev.buf, &to_amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
- if (oidcmp(&head, &to_amend))
+ if (!is_clean && oidcmp(&head, &to_amend))
return error(_("\nYou have uncommitted changes in your "
"working tree. Please, commit them\n"
"first and then run 'git rebase "
"--continue' again."));
+ /*
+* When skipping a failed fixup/squash, we need to edit the
+* commit message, the current fixup list and count, and if it
+* was the last fixup/squash in the chain, we need to clean up
+* the commit message and if there was a squash, let the user
+* edit it.
+*/
+ if (is_clean && !oidcmp(&head, &to_amend) &&
+ opts->current_fixup_count > 0 &&
+ file_exists(rebase_path_stopped_sha())) {
+ const char *p = opts->current_fixups.buf;
+ int len = opts->current_fixups.len;
+
+ opts->current_fixup_count--;
+ if (!len)
+ BUG("Incorrect current_fixups:\n%s", p);
+ while (len && p[len - 1] != '\n')
+ len--;
+ strbuf_setlen(&opts->current_fixups, le
Re: [PATCH v3 2/3] merge: Add merge.renames config setting
On Fri, Apr 27, 2018 at 11:37 AM, Eckhard Maaß wrote: > On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote: >> I think demoting from copy to rename-only is a good idea, at least >> for now, because I do not believe we have figured out what we want >> to happen when we detect copied files are involved in a merge. > > Does anyone know some threads concerning that topic? I tried to search > for it, but somehow "merge copy detection" did not find me useful > threads so far. I would be interested in that topic anyway, so I would > like to know what the ideas are that floated so far. > I doubt it has ever been discussed before this thread. But, if you're curious, I'll try to dump a few thoughts. Let's say we have branches A and B, and: A: modifies file z B: copies z to y Should the modifications to z done in A propagate to both z and y? If not, what good is copy detection? If so, then there are several ramifications... - If B not only copied z but also first modified it, then do we have potential conflicts with both z and y -- possibly the exact same conflicts, making the user resolve them repeatedly? - What if A copied z to x? Do changes to z propagate to all three of z and x and y? Do changes to either x or y affect z? Do they affect each other? - If A deleted z, does that give us a copy/delete conflict for y? Do we also have to worry about copy/add conflicts? copy/add/delete? rename/copy (multiple variants)? copy/copy? - Extra degrees of freedom may mean new conflict types: - The extra degrees of freedom from renames introduced multiple new conflict types (e.g. rename/add, rename/rename(1to2), rename/rename(2to1)). - Directory rename detection added more (rename/rename/rename(1to3), rename/rename(Nto1), add/add/add, directory/file/file, n-fold transitive rename, etc.), -- and forced us to specify various rules to limit the possibility space so that conflicts could be representable in the index and understandable by the user. - I suspect adding copies would add new types of conflicts we haven't thought of yet. The more I think about it, the more I think that attempting to detect copies in a merge algorithm just doesn't make sense. Anything I can think of that someone might attempt to use detected copies for would just surprise users in a bad way...and it'd open up a big can of edge and corner case problems as well.
Re: [RFC PATCH] checkout: Force matching mtime between files
On Fri, Apr 27, 2018 at 7:18 PM, Michał Górny wrote:
> W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
> napisał:
>> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
>> > "Robin H. Johnson" writes:
>> >
>> > > In the thread from 6 years ago, you asked about tar's behavior for
>> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
>> > > ordering after restore would be the same, and would only rebuild if the
>> > > original source happened to be dirty.
>> > >
>> > > This behavior is already non-deterministic in Git, and would be improved
>> > > by the patch.
>> >
>> > But Git is not an archiver (tar), but is a source code control
>> > system, so I do not think we should spend any extra cycles to
>> > "improve" its behaviour wrt the relative ordering, at least for the
>> > default case. Only those who rely on having build artifact *and*
>> > source should pay the runtime (and preferrably also the
>> > maintainance) cost.
>>
>> Anyone who uses "make" or some other mtime-based tool is affected by
>> this. I agree that it's not "Everyone" but it sure is a lot of people.
>>
>> Are we all that sure that the performance hit is that drastic? After
>> all, we've just done write_entry(). Calling utime() at that point
>> should just hit the filesystem cache.
>>
>> > The best approach to do so is to have those people do the "touch"
>> > thing in their own post-checkout hook. People who use Git as the
>> > source control system won't have to pay runtime cost of doing the
>> > touch thing, and we do not have to maintain such a hook script.
>> > Only those who use the "feature" would.
>>
>> The post-checkout hook approach is not exactly straightforward.
>>
>> Naively, it's simply
>>
>> for F in `git diff --name-only $1 $2`; do touch "$F"; done
>>
>> But consider:
>>
>> * Symlinks can cause the wrong file to be touched. (Granted, Michał's
>> proposed patch also doesn't deal with symlinks.) Let's assume that a
>> hook can be crafted will all possible sophistication. There are still
>> some fundamental problems:
>>
>> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
>> identical so the above loop does nothing. Offhand I'm not even sure how
>> a hook might get the right files in this case.
>>
>> * The hook has to be set up in every repo and submodule (at least until
>> something like Ævar's experiments come to fruition).
>>
>> * A fresh clone can't run the hook. This is especially important when
>> dealing with submodules. (In one case where we were bit by this, make
>> though that half of a fresh submodule clone's files were stale, and
>> decided to re-autoconf the entire thing.)
>>
>>
>> I just don't think the hook approach can completely solve the problem.
>>
>
> There's also the performance aspect. If we deal with checkouts that
> include 1000+ files on a busy system (i.e. when mtimes really become
> relevant), calling utime() instantly has a good chance of hitting warm
> cache. On the other hand, post-checkout hook has a greater risk of
> running cold cache, i.e. writing to all inodes twice.
The FS cache is evicted on a LRU basis. What you're saying is true,
but in the two different implementations there's maybe a 2-3 second
gap between what git is doing and the post-checkout hook is doing. If
the system is under such memory pressure that you've evicted the pages
you just touched you're probably screwed anyway. Maybe I've missed
something here, but this point seems moot.
There's certainly other good arguments against using the current hook
implementation for this, e.g. not being able to do this on clone as
noted upthread.
I think patches that made this configurable in some way in git would
be worth looking at, and due to the subject matter it might make sense
to have it in the core distribution as a non-hook, but I think the
default behavior should always be what it is now, since almost nobody
cares about these edge case,s and users should have to opt-in to use
behavior to work around them.
Re: Fetching tags overwrites existing tags
On Tue, Apr 24, 2018 at 9:57 PM, Wink Saville wrote: > If have a repository with a tag "v1.0.0" and I add a remote repository > which also has a tag "v1.0.0" tag is overwritten. I feel like this thread has gotten somewhat side-tracked by the valid discussion about whether we should have remote tracking tags, but the much easier thing to fix is that the "+" prefix for refs/tags/* means nothing. I noticed this when working on fetch.pruneTags in the last release, but didn't dig further. I.e. if you clone git.git and update "master" and a tag: $ git fetch origin 'refs/heads/*:refs/heads/*' --dry-run >From github.com:git/git ! [rejected] master -> master (non-fast-forward) $ git fetch origin '+refs/heads/*:refs/heads/*' --dry-run >From github.com:git/git + 969e05fae2...1f1cddd558 master -> master (forced update) Here "+" does the right thing, but then: $ git fetch origin 'refs/tags/*:refs/tags/*' --dry-run >From github.com:git/git t [tag update]v2.17.0-> v2.17.0 $ git fetch origin '+refs/tags/*:refs/tags/*' --dry-run >From github.com:git/git t [tag update]v2.17.0-> v2.17.0 Here the former shouldn't be clobbering the existing tag.
[PATCH v2 7/6] doc: normalize [--options] to [options] in git-diff
SYNOPSIS and other manuals use [options] but DESCRIPTION used [--options]. Signed-off-by: Andreas Heiduk --- Documentation/git-diff.txt | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt index 6593b58299..7c2c442700 100644 --- a/Documentation/git-diff.txt +++ b/Documentation/git-diff.txt @@ -21,7 +21,7 @@ Show changes between the working tree and the index or a tree, changes between the index and a tree, changes between two trees, changes between two blob objects, or changes between two files on disk. -'git diff' [--options] [--] [...]:: +'git diff' [options] [--] [...]:: This form is to view the changes you made relative to the index (staging area for the next commit). In other @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. further add to the index but you still haven't. You can stage these changes by using linkgit:git-add[1]. -'git diff' [--options] --no-index [--] :: +'git diff' [options] --no-index [--] :: This form is to compare the given two paths on the filesystem. You can omit the `--no-index` option when @@ -38,7 +38,7 @@ two blob objects, or changes between two files on disk. or when running the command outside a working tree controlled by Git. -'git diff' [--options] --cached [] [--] [...]:: +'git diff' [options] --cached [] [--] [...]:: This form is to view the changes you staged for the next commit relative to the named . Typically you @@ -48,7 +48,7 @@ two blob objects, or changes between two files on disk. is not given, it shows all staged changes. --staged is a synonym of --cached. -'git diff' [--options] [--] [...]:: +'git diff' [options] [--] [...]:: This form is to view the changes you have in your working tree relative to the named . You can @@ -56,18 +56,18 @@ two blob objects, or changes between two files on disk. branch name to compare with the tip of a different branch. -'git diff' [--options] [--] [...]:: +'git diff' [options] [--] [...]:: This is to view the changes between two arbitrary . -'git diff' [--options] .. [--] [...]:: +'git diff' [options] .. [--] [...]:: This is synonymous to the previous form. If on one side is omitted, it will have the same effect as using HEAD instead. -'git diff' [--options] \... [--] [...]:: +'git diff' [options] \... [--] [...]:: This form is to view the changes on the branch containing and up to the second , starting at a common ancestor -- 2.16.2
Re: Fetching tags overwrites existing tags
On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville wrote: > > The other change was rather than using > ""+refs/tags/*:refs/remote-tags/$name/*" > I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems > cleaner. > Again, if remote-tags is preferred I'll change it back. >From looking at the code, it looks like you mean "+refs/tags/*:refs/remotes/tags/$name/*". The issue with that approach is that it collides with a remote named "tags". "refs/remote-tags", on the other hand, represents a new-to-Git path, one that won't already be in use by any other standard functionality. That seems like a better approach than hoping no one out there will call one of their remotes "tags". Bryan
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
On Fri, Apr 27, 2018 at 2:40 PM, Andreas Heiduk wrote: > Am 27.04.2018 um 19:33 schrieb Eric Sunshine: >> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: >>> @@ -13,7 +13,7 @@ SYNOPSIS >>> -'git diff' [options] [--no-index] [--] >>> +'git diff' [options] --no-index [--] >>> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. >>> -'git diff' --no-index [--options] [--] [...]:: >>> +'git diff' [--options] --no-index [--] :: >> >> Not a problem introduced by this patch, but shouldn't this say >> "[options]" rather than "[--options]"? Since the aim of this patch >> series is to clean up botches and normalize documentation, perhaps it >> could also fix this oddness(?). > > Well, in the SYNOPSIS it is always `[options]` for all variants but in > the DESCRIPTION it is always `[--options]` for all variants. Fixing the > other variants would stretch the "subject" line of the patch a little > bit to far ;-) I wasn't suggesting that this patch should fix that issue (it shouldn't) but that it could/should be done by a separate new patch since it's a distinct change. (That's why I was careful to say "aim of this patch _series_".)
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
Am 27.04.2018 um 20:45 schrieb Martin Ågren: > On 27 April 2018 at 20:28, Andreas Heiduk wrote: >> Am 27.04.2018 um 19:18 schrieb Martin Ågren: >>> On 27 April 2018 at 19:04, Andreas Heiduk wrote: The two '' parameters are not optional but the option '--no-index' is. Also move the `--options` part to the same place where the other variants show them. >>> >>> Should this commit message be updated after the changes you did to >>> address Junio's comment? This text suggests you want to place --no-index >>> in [] (and you did in v1) but you do not do that below. >>> All three items are already correct in the synopsis. >>> >>> Same here, now you actually do change things there. >>> Signed-off-by: Andreas Heiduk Reviewed-by: Martin Ågren >>> >>> Strictly speaking, my Reviewed-by was on another patch. I do find this >> >> Sorry, I've added that trailer after reading "The diff LGTM.", then >> applied Junio's changes and forgot to remove the trailer. >> >>> one better though thanks to Junio's suggestion (except the mismatch with >>> the commit message). >> >> I'll fix that with this: >> >> doc: align 'diff --no-index' in text with synopsis > > s/with/and/ since they both change? It's not that the first changes to > match the second, but they actually both change to match each other (and > to be correct, obviously). Corrected > >> Make the two '' parameters in DESCRIPTION mandatory and >> move the `--options` part to the same place where the other >> variants show them. And finally make `--no-index` in SYNOPSIS >> as mandatory as in DESCRIPTION. > > Great! Junio had some good reasoning about how --no-index is > sometimes optional, but not always. Not sure if it's worth spelling that > out. (Although one could argue that it already did trip us up once. :-)) The post-context already explains that. > Eric's point about "--options" vs "options" seemed right to me. If you > address that, note that this message says "--options".
Re: Fetching tags overwrites existing tags
On Thu, Apr 26, 2018 at 4:24 PM, Junio C Hamano wrote: > Junio C Hamano writes: > > > Hence (1) we should detect and error out when --prefix-tags is used > with mirror fetch near where we do the same for track used without > mirror fetch already, (2) detect and error out when --prefix-tags is > used with track, and (3) add "+refs/tags/*:refs/remote-tags/$name/*" > just once without paying attention to track here. We may not even > want add_remote_tags() helper function if we go that route. > I've replied to the thread using format-email/send-email with the subject: "[RFC PATCH v2] Teach remote add the --prefix-tags option", but I misspelled Junio's email address :( I've tried to address the issues pointed out by Junio. But I've choosen not to do "(2) detect and error out when --prefix-tags is used with track". My thinking is tags are independent of tracking and it seems reasonable that they sould be included if requested. If I'm wrong I'll certainly fix it. The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*" I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner. Again, if remote-tags is preferred I'll change it back. One other question, I'm not sure "--prefix-tags" is the best name for the option, maybe "--sub-tags" or "--nested-tags" or ... -- Wink
[RFC PATCH v2] Teach remote add the --prefix-tags option
When --prefix-tags is passed to `git remote add` the tagopt is set to
--prefix-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.
For example:
$ git remote add -f --prefix-tags gbenchmark g...@github.com:google/benchmark
Updating gbenchmark
warning: no common commits
remote: Counting objects: 4406, done.
remote: Compressing objects: 100% (18/18), done.
remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
Resolving deltas: 100% (2865/2865), done.
From github.com:google/benchmark
* [new branch] clangtidy -> gbenchmark/clangtidy
* [new branch] iter_report -> gbenchmark/iter_report
* [new branch] master -> gbenchmark/master
* [new branch] releasing -> gbenchmark/releasing
* [new branch] reportercleanup -> gbenchmark/reportercleanup
* [new branch] rmheaders -> gbenchmark/rmheaders
* [new branch] v2 -> gbenchmark/v2
* [new tag] v0.0.9 -> tags/gbenchmark/v0.0.9
* [new tag] v0.1.0 -> tags/gbenchmark/v0.1.0
* [new tag] v1.0.0 -> tags/gbenchmark/v1.0.0
* [new tag] v1.1.0 -> tags/gbenchmark/v1.1.0
* [new tag] v1.2.0 -> tags/gbenchmark/v1.2.0
* [new tag] v1.3.0 -> tags/gbenchmark/v1.3.0
* [new tag] v1.4.0 -> tags/gbenchmark/v1.4.0
And the .git/config remote "gbenchmark" section looks like:
[remote "gbenchmark"]
url = g...@github.com:google/benchmark
fetch = +refs/heads/*:refs/remotes/gbenchmark/*
fetch = +refs/tags/*:refs/remotes/tags/gbenchmark/*
tagopt = --prefix-tags
Based on a solution proposed by Junio on the email list [1]
[1]:
https://public-inbox.org/git/xmqqbme51rgn@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e
Helped-by: Junio C Hamano
Helped-by: Jacob Keller
Signed-off-by: Wink Saville
---
Documentation/git-remote.txt | 8 +--
builtin/remote.c | 42
remote.c | 2 ++
3 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..c97bf29d46 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
[verse]
'git remote' [-v | --verbose]
-'git remote add' [-t ] [-m ] [-f] [--[no-]tags]
[--mirror=]
+'git remote add' [-t ] [-m ] [-f] [--prefix-tags | --tags |
--no-tags] [--mirror=]
'git remote rename'
'git remote remove'
'git remote set-head' (-a | --auto | -d | --delete | )
@@ -54,7 +54,11 @@ With `-f` option, `git fetch ` is run immediately after
the remote information is set up.
+
With `--tags` option, `git fetch ` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --prefix-tags to import them
+to refs/remotes/tags//.
++
+With `--prefix-tags` option, `git fetch ` imports every tag from the
+remote repository to refs/remotes/tags//.
+
With `--no-tags` option, `git fetch ` does not import tags from
the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..39de50bdd6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
static const char * const builtin_remote_usage[] = {
N_("git remote [-v | --verbose]"),
- N_("git remote add [-t ] [-m ] [-f] [--tags |
--no-tags] [--mirror=] "),
+ N_("git remote add [-t ] [-m ] [-f] [--prefix-tags |
--tags | --no-tags] [--mirror=] "),
N_("git remote rename "),
N_("git remote remove "),
N_("git remote set-head (-a | --auto | -d | --delete |
)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
enum {
TAGS_UNSET = 0,
TAGS_DEFAULT = 1,
- TAGS_SET = 2
+ TAGS_SET = 2,
+ TAGS_SET_PREFIX = 3
};
#define MIRROR_NONE 0
@@ -123,6 +124,14 @@ static void add_branch(const char *key, const char
*branchname,
git_config_set_multivar(key, tmp->buf, "^$", 0);
}
+static void add_remote_tags(const char *key, const char *remotename,
+ struct strbuf *tmp)
+{
+ strbuf_reset(tmp);
+ strbuf_addf(tmp, "+refs/tags/*:refs/remotes/tags/%s/*", remotename);
+ git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
static const char mirror_advice[] =
N_("--mirror is dangerous and deprecated; please\n"
"\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +170,9 @@ static int add(int argc, const char **argv)
OPT_SET_INT(0, "tags", &fetch_tags,
N_("import all tags and associated objects when
fetching"),
TAGS_SET),
+ OPT_SET_INT(0, "prefix-tags", &fetch_tags,
+ N_("import all tags and associated ob
Re: [PATCH v2 1/6] doc: improve formatting in githooks.txt
On 27 April 2018 at 19:04, Andreas Heiduk wrote: > Typeset commands and similar things with as `git foo` instead of > 'git foo' or 'git-foo' and add linkgit to the commands which run > the hooks. > > Signed-off-by: Andreas Heiduk > Reviewed-by: Martin Ågren Indeed. The difference between last time (the original patch and the two fixups) and this patch is precisely the small tweaks that I suggested. Thanks Martin
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
On 27 April 2018 at 20:28, Andreas Heiduk wrote: > Am 27.04.2018 um 19:18 schrieb Martin Ågren: >> On 27 April 2018 at 19:04, Andreas Heiduk wrote: >>> The two '' parameters are not optional but the option >>> '--no-index' is. Also move the `--options` part to the same >>> place where the other variants show them. >> >> Should this commit message be updated after the changes you did to >> address Junio's comment? This text suggests you want to place --no-index >> in [] (and you did in v1) but you do not do that below. >> >>> All three items are already correct in the synopsis. >> >> Same here, now you actually do change things there. >> >>> Signed-off-by: Andreas Heiduk >>> Reviewed-by: Martin Ågren >> >> Strictly speaking, my Reviewed-by was on another patch. I do find this > > Sorry, I've added that trailer after reading "The diff LGTM.", then > applied Junio's changes and forgot to remove the trailer. > >> one better though thanks to Junio's suggestion (except the mismatch with >> the commit message). > > I'll fix that with this: > > doc: align 'diff --no-index' in text with synopsis s/with/and/ since they both change? It's not that the first changes to match the second, but they actually both change to match each other (and to be correct, obviously). > Make the two '' parameters in DESCRIPTION mandatory and > move the `--options` part to the same place where the other > variants show them. And finally make `--no-index` in SYNOPSIS > as mandatory as in DESCRIPTION. Great! Junio had some good reasoning about how --no-index is sometimes optional, but not always. Not sure if it's worth spelling that out. (Although one could argue that it already did trip us up once. :-)) Eric's point about "--options" vs "options" seemed right to me. If you address that, note that this message says "--options".
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
On 27 April 2018 at 20:40, Andreas Heiduk wrote: > Am 27.04.2018 um 19:33 schrieb Eric Sunshine: >> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: >>> The two '' parameters are not optional but the option >>> '--no-index' is. Also move the `--options` part to the same >>> place where the other variants show them. >>> >>> All three items are already correct in the synopsis. >>> >>> Signed-off-by: Andreas Heiduk >>> --- >>> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt >>> @@ -13,7 +13,7 @@ SYNOPSIS >>> -'git diff' [options] [--no-index] [--] >>> +'git diff' [options] --no-index [--] >>> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. >>> -'git diff' --no-index [--options] [--] [...]:: >>> +'git diff' [--options] --no-index [--] :: >> >> Not a problem introduced by this patch, but shouldn't this say >> "[options]" rather than "[--options]"? Since the aim of this patch >> series is to clean up botches and normalize documentation, perhaps it >> could also fix this oddness(?). >> > > Well, in the SYNOPSIS it is always `[options]` for all variants but in > the DESCRIPTION it is always `[--options]` for all variants. Fixing the > other variants would stretch the "subject" line of the patch a little > bit to far ;-) Hmm, I do not think it's always though. It's pretty consistent in its inconsistency, but "git diff [options] " goes the other way. Maybe that's patch 1/7...
Re: [PATCH v2 6/6] doc: add note about shell quoting to revision.txt
Am 27.04.2018 um 19:36 schrieb Eric Sunshine: > On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: >> Signed-off-by: Andreas Heiduk >> Reviewed-by: Junio C Hamano >> --- >> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt >> @@ -186,6 +190,8 @@ existing tag object. >> + Depending on the given text the shell's word splitting rules might >> + require additional quoting. > > s/text/&,/ > Fixed, Thanks
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
Am 27.04.2018 um 19:33 schrieb Eric Sunshine: > On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: >> The two '' parameters are not optional but the option >> '--no-index' is. Also move the `--options` part to the same >> place where the other variants show them. >> >> All three items are already correct in the synopsis. >> >> Signed-off-by: Andreas Heiduk >> --- >> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt >> @@ -13,7 +13,7 @@ SYNOPSIS >> -'git diff' [options] [--no-index] [--] >> +'git diff' [options] --no-index [--] >> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. >> -'git diff' --no-index [--options] [--] [...]:: >> +'git diff' [--options] --no-index [--] :: > > Not a problem introduced by this patch, but shouldn't this say > "[options]" rather than "[--options]"? Since the aim of this patch > series is to clean up botches and normalize documentation, perhaps it > could also fix this oddness(?). > Well, in the SYNOPSIS it is always `[options]` for all variants but in the DESCRIPTION it is always `[--options]` for all variants. Fixing the other variants would stretch the "subject" line of the patch a little bit to far ;-)
Re: [PATCH v3 2/3] merge: Add merge.renames config setting
On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote: > I think demoting from copy to rename-only is a good idea, at least > for now, because I do not believe we have figured out what we want > to happen when we detect copied files are involved in a merge. Does anyone know some threads concerning that topic? I tried to search for it, but somehow "merge copy detection" did not find me useful threads so far. I would be interested in that topic anyway, so I would like to know what the ideas are that floated so far. Greetings, Eckhard
[no subject]
From: Elijah Newren On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote: > Can you write the documentation that clearly explains the exact behavior you > want? That would kill two birds with one stone... :) Sure, something like the following is what I envision, and I've tried to include the suggestion from Junio to document the copy behavior in the merge-recursive documentation. -- 8< -- Subject: [PATCH] fixup! merge: Add merge.renames config setting --- Documentation/merge-config.txt | 3 +-- Documentation/merge-strategies.txt | 5 +++-- merge-recursive.c | 8 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt index 59848e5634..662c2713ca 100644 --- a/Documentation/merge-config.txt +++ b/Documentation/merge-config.txt @@ -41,8 +41,7 @@ merge.renameLimit:: merge.renames:: Whether and how Git detects renames. If set to "false", rename detection is disabled. If set to "true", basic rename - detection is enabled. If set to "copies" or "copy", Git will - detect copies, as well. Defaults to the value of diff.renames. + detection is enabled. Defaults to the value of diff.renames. merge.renormalize:: Tell Git that canonical representation of files in the diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index 1e0728aa12..aa66cbe41e 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -23,8 +23,9 @@ recursive:: causing mismerges by tests done on actual merge commits taken from Linux 2.6 kernel development history. Additionally this can detect and handle merges involving - renames. This is the default merge strategy when - pulling or merging one branch. + renames, but currently cannot make use of detected + copies. This is the default merge strategy when pulling + or merging one branch. + The 'recursive' strategy can take the following options: diff --git a/merge-recursive.c b/merge-recursive.c index 6cc4404144..b618f134d2 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -564,6 +564,14 @@ static struct string_list *get_renames(struct merge_options *o, opts.flags.recursive = 1; opts.flags.rename_empty = 0; opts.detect_rename = merge_detect_rename(o); + /* +* We do not have logic to handle the detection of copies. In +* fact, it may not even make sense to add such logic: would we +* really want a change to a base file to be propagated through +* multiple other files by a merge? +*/ + if (opts.detect_rename > DIFF_DETECT_RENAME) + opts.detect_rename = DIFF_DETECT_RENAME; opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit : o->diff_rename_limit >= 0 ? o->diff_rename_limit : 1000; -- 2.17.0.294.g8e3b83c0e3
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
Am 27.04.2018 um 19:18 schrieb Martin Ågren: > On 27 April 2018 at 19:04, Andreas Heiduk wrote: >> The two '' parameters are not optional but the option >> '--no-index' is. Also move the `--options` part to the same >> place where the other variants show them. > > Should this commit message be updated after the changes you did to > address Junio's comment? This text suggests you want to place --no-index > in [] (and you did in v1) but you do not do that below. > >> All three items are already correct in the synopsis. > > Same here, now you actually do change things there. > >> Signed-off-by: Andreas Heiduk >> Reviewed-by: Martin Ågren > > Strictly speaking, my Reviewed-by was on another patch. I do find this Sorry, I've added that trailer after reading "The diff LGTM.", then applied Junio's changes and forgot to remove the trailer. > one better though thanks to Junio's suggestion (except the mismatch with > the commit message). I'll fix that with this: doc: align 'diff --no-index' in text with synopsis Make the two '' parameters in DESCRIPTION mandatory and move the `--options` part to the same place where the other variants show them. And finally make `--no-index` in SYNOPSIS as mandatory as in DESCRIPTION.
Confusing documentation for git apply -p
https://git-scm.com/docs/git-apply#git-apply--pltngt > -p > Remove leading slashes from traditional diff paths. The default is 1. This suggests to me the following outcomes: 1) home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo 2) home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo 3) /home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo 4) /home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo 5) //home/user/repos/myrepo with -p1 becomes /home/user/repos/myrepo 6) //home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo `Remove leading slashes`...That's not really what's happening. What seems to actually happen is that it is removing directories from the path: 1) home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo 2) home/user/repos/myrepo with -p2 becomes user/repos/myrepo This argument seems to be removing folders from the path, not slashes. I'm sure it's doing both removing slashes and folders, but at different times, which makes it even more confusing to anyone trying to wrap their head around it. -p1= /home/a -> home/a -p1= home/a -> home/a ?? Does this happen, or does it actually remove `home`??? -p2=/home/a -> home/a OR a ??? I have no idea after reading the docs and seeing different behavior. I literally have to experiment with it to know what it does in each scenario. -p2=home/a -> a ?? folder and slash removed. All of this doesn't have to be so complicated, it's just being expressed in a complicated way. Break this out into two separate functions maybe, or update the documentation so it's clearer how this behaves. > -p - number of folders in path to remove from the beginning of the path. Not great, but clearer than what's there. Thanks, Kelly Elton http://www.kellyelton.com
Re: [PATCH v2 6/6] doc: add note about shell quoting to revision.txt
On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: > Signed-off-by: Andreas Heiduk > Reviewed-by: Junio C Hamano > --- > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt > @@ -186,6 +190,8 @@ existing tag object. > + Depending on the given text the shell's word splitting rules might > + require additional quoting. s/text/&,/
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk wrote: > The two '' parameters are not optional but the option > '--no-index' is. Also move the `--options` part to the same > place where the other variants show them. > > All three items are already correct in the synopsis. > > Signed-off-by: Andreas Heiduk > --- > diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt > @@ -13,7 +13,7 @@ SYNOPSIS > -'git diff' [options] [--no-index] [--] > +'git diff' [options] --no-index [--] > @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. > -'git diff' --no-index [--options] [--] [...]:: > +'git diff' [--options] --no-index [--] :: Not a problem introduced by this patch, but shouldn't this say "[options]" rather than "[--options]"? Since the aim of this patch series is to clean up botches and normalize documentation, perhaps it could also fix this oddness(?).
Missing --relative documentation
git format-patch is missing documentation for --relative. There is also no auto complete(or tab complete, whatever it's called) for the --relative switch/argument. https://stackoverflow.com/a/16309416/222054 Thanks, Kelly Elton http://www.kellyelton.com
Re: [RFC PATCH] checkout: Force matching mtime between files
W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
napisał:
> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
> > "Robin H. Johnson" writes:
> >
> > > In the thread from 6 years ago, you asked about tar's behavior for
> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
> > > ordering after restore would be the same, and would only rebuild if the
> > > original source happened to be dirty.
> > >
> > > This behavior is already non-deterministic in Git, and would be improved
> > > by the patch.
> >
> > But Git is not an archiver (tar), but is a source code control
> > system, so I do not think we should spend any extra cycles to
> > "improve" its behaviour wrt the relative ordering, at least for the
> > default case. Only those who rely on having build artifact *and*
> > source should pay the runtime (and preferrably also the
> > maintainance) cost.
>
> Anyone who uses "make" or some other mtime-based tool is affected by
> this. I agree that it's not "Everyone" but it sure is a lot of people.
>
> Are we all that sure that the performance hit is that drastic? After
> all, we've just done write_entry(). Calling utime() at that point
> should just hit the filesystem cache.
>
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook. People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
>
> The post-checkout hook approach is not exactly straightforward.
>
> Naively, it's simply
>
> for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched. (Granted, Michał's
> proposed patch also doesn't deal with symlinks.) Let's assume that a
> hook can be crafted will all possible sophistication. There are still
> some fundamental problems:
>
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
> identical so the above loop does nothing. Offhand I'm not even sure how
> a hook might get the right files in this case.
>
> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).
>
> * A fresh clone can't run the hook. This is especially important when
> dealing with submodules. (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and
> decided to re-autoconf the entire thing.)
>
>
> I just don't think the hook approach can completely solve the problem.
>
There's also the performance aspect. If we deal with checkouts that
include 1000+ files on a busy system (i.e. when mtimes really become
relevant), calling utime() instantly has a good chance of hitting warm
cache. On the other hand, post-checkout hook has a greater risk of
running cold cache, i.e. writing to all inodes twice.
--
Best regards,
Michał Górny
Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
On 27 April 2018 at 19:04, Andreas Heiduk wrote: > The two '' parameters are not optional but the option > '--no-index' is. Also move the `--options` part to the same > place where the other variants show them. Should this commit message be updated after the changes you did to address Junio's comment? This text suggests you want to place --no-index in [] (and you did in v1) but you do not do that below. > All three items are already correct in the synopsis. Same here, now you actually do change things there. > Signed-off-by: Andreas Heiduk > Reviewed-by: Martin Ågren Strictly speaking, my Reviewed-by was on another patch. I do find this one better though thanks to Junio's suggestion (except the mismatch with the commit message). Thanks for continuing with this series. Martin > --- > Documentation/git-diff.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt > index b0c1bb95c8..6593b58299 100644 > --- a/Documentation/git-diff.txt > +++ b/Documentation/git-diff.txt > @@ -13,7 +13,7 @@ SYNOPSIS > 'git diff' [options] --cached [] [--] [...] > 'git diff' [options] [--] [...] > 'git diff' [options] > -'git diff' [options] [--no-index] [--] > +'git diff' [options] --no-index [--] > > DESCRIPTION > --- > @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. > further add to the index but you still haven't. You can > stage these changes by using linkgit:git-add[1]. > > -'git diff' --no-index [--options] [--] [...]:: > +'git diff' [--options] --no-index [--] :: > > This form is to compare the given two paths on the > filesystem. You can omit the `--no-index` option when
Re: In some rebases, `exec git -C ...` has wrong working directory
Hi Johannes, Thanks for your reply. Part of my confusion was regarding the interaction between `-C` and `--git-dir`. For instance, we have $ git --git-dir target -C /tmp/tmp.Cl4aXMSVis init Initialized empty Git repository in /tmp/tmp.Cl4aXMSVis/target/ which makes sense and is what I expected: the `-C` and `--git-dir` values are joined, as suggested by the docs for `-C` in git(1). But with $ git --git-dir /tmp/tmp.Cl4aXMSVis/repo -C /tmp/tmp.Cl4aXMSVis init Initialized empty Git repository in /tmp/tmp.Cl4aXMSVis/repo/ it appears that the `-C` argument is ignored entirely. Is this because running `git -C foo ...` is equivalent to running `cd foo; git ...` in a shell, so when the `--git-dir` is an absolute path the value of `-C` has no effect? (Assuming that `GIT_WORK_TREE` is unset.) In your example: >exec git -C /somewhere/else show HEAD:some-file >some-other-file isn't the behavior to copy the version of `some-file` in the repository being rebased to `some-other-file` in the current working directory, such that the `-C` has no effect, because the shell redirect is not affected by the `-C`? (This is what happens for me.) If so, why include the `-C` in the command? > I do not think that we can sensibly *remove* GIT_DIR from the environment > variables passed to the exec'ed command, as we have been doing that for > ages, and some scripts (as demonstrated above) started relying on that > behavior. So if we changed it, we would break backwards-compatibility, > which is something we try to avoid very much in Git. This makes sense; understood and agreed. Do you know why `rebase --root` does not set `GIT_DIR`? > Maybe you could a contribute a patch to the documentation? Sure; I'd be happy to do that once I understand this a bit better. :-) Best, WC
[PATCH v2 4/6] doc: add '-d' and '-o' for 'git push'
Add the missing `-o` shortcut for `--push-option` to the synopsis. Add the missing `-d` shortcut for `--delete` in the main section. Signed-off-by: Andreas Heiduk Reviewed-by: Martin Ågren --- Documentation/git-push.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 5b08302fc2..f2bbda6e32 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=] [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | --verbose] - [-u | --set-upstream] [--push-option=] + [-u | --set-upstream] [-o | --push-option=] [--[no-]signed|--signed=(true|false|if-asked)] [--force-with-lease[=[:]]] [--no-verify] [ [...]] @@ -123,6 +123,7 @@ already exists on the remote side. will be tab-separated and sent to stdout instead of stderr. The full symbolic names of the refs will be given. +-d:: --delete:: All listed refs are deleted from the remote repository. This is the same as prefixing all refs with a colon. -- 2.16.2
[PATCH v2 5/6] git-svn: remove ''--add-author-from' for 'commit-diff'
The subcommand 'commit-diff' does not support the option '--add-author-from'. Signed-off-by: Andreas Heiduk Signed-off-by: Eric Wong --- Documentation/git-svn.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index d59379ee23..e9615951d2 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -707,7 +707,7 @@ creating the branch or tag. config key: svn.useLogAuthor --add-author-from:: - When committing to svn from Git (as part of 'commit-diff', 'set-tree' or 'dcommit' + When committing to svn from Git (as part of 'set-tree' or 'dcommit' operations), if the existing log message doesn't already have a `From:` or `Signed-off-by:` line, append a `From:` line based on the Git commit's author string. If you use this, then `--use-log-author` -- 2.16.2
[PATCH v2 3/6] doc: clarify ignore rules for git ls-files
Explain that `git ls-files --ignored` requires at least one of the `--exclude*` options to do its job. Signed-off-by: Andreas Heiduk --- Documentation/git-ls-files.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt index 3ac3e3a77d..f3474b2ede 100644 --- a/Documentation/git-ls-files.txt +++ b/Documentation/git-ls-files.txt @@ -53,7 +53,8 @@ OPTIONS Show only ignored files in the output. When showing files in the index, print only those matched by an exclude pattern. When showing "other" files, show only those matched by an exclude - pattern. + pattern. Standard ignore rules are not automatically activated, + therefore at least one of the `--exclude*` options is required. -s:: --stage:: -- 2.16.2
[PATCH v2 6/6] doc: add note about shell quoting to revision.txt
Signed-off-by: Andreas Heiduk Reviewed-by: Junio C Hamano --- Documentation/revisions.txt | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index dfcc49c72c..c1d3a40a90 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -7,6 +7,10 @@ syntax. Here are various ways to spell object names. The ones listed near the end of this list name trees and blobs contained in a commit. +NOTE: This document shows the "raw" syntax as seen by git. The shell +and other UIs might require additional quoting to protect special +characters and to avoid word splitting. + '', e.g. 'dae86e1950b1277e545cee180551750029cfe735', 'dae86e':: The full SHA-1 object name (40-byte hexadecimal string), or a leading substring that is unique within the repository. @@ -186,6 +190,8 @@ existing tag object. is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a literal '!' character, followed by 'foo'. Any other sequence beginning with ':/!' is reserved for now. + Depending on the given text the shell's word splitting rules might + require additional quoting. ':', e.g. 'HEAD:README', ':README', 'master:./README':: A suffix ':' followed by a path names the blob or tree -- 2.16.2
[PATCH v2 1/6] doc: improve formatting in githooks.txt
Typeset commands and similar things with as `git foo` instead of 'git foo' or 'git-foo' and add linkgit to the commands which run the hooks. Signed-off-by: Andreas Heiduk Reviewed-by: Martin Ågren --- Documentation/githooks.txt | 115 +++-- 1 file changed, 58 insertions(+), 57 deletions(-) diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index f877f7b7cd..e3c283a174 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -31,7 +31,7 @@ Hooks can get their arguments via the environment, command-line arguments, and stdin. See the documentation for each hook below for details. -'git init' may copy hooks to the new repository, depending on its +`git init` may copy hooks to the new repository, depending on its configuration. See the "TEMPLATE DIRECTORY" section in linkgit:git-init[1] for details. When the rest of this document refers to "default hooks" it's talking about the default template shipped @@ -45,9 +45,9 @@ HOOKS applypatch-msg ~~ -This hook is invoked by 'git am'. It takes a single +This hook is invoked by linkgit:git-am[1]. It takes a single parameter, the name of the file that holds the proposed commit -log message. Exiting with a non-zero status causes 'git am' to abort +log message. Exiting with a non-zero status causes `git am` to abort before applying the patch. The hook is allowed to edit the message file in place, and can @@ -61,7 +61,7 @@ The default 'applypatch-msg' hook, when enabled, runs the pre-applypatch ~~ -This hook is invoked by 'git am'. It takes no parameter, and is +This hook is invoked by linkgit:git-am[1]. It takes no parameter, and is invoked after the patch is applied, but before a commit is made. If it exits with non-zero status, then the working tree will not be @@ -76,33 +76,33 @@ The default 'pre-applypatch' hook, when enabled, runs the post-applypatch ~~~ -This hook is invoked by 'git am'. It takes no parameter, +This hook is invoked by linkgit:git-am[1]. It takes no parameter, and is invoked after the patch is applied and a commit is made. This hook is meant primarily for notification, and cannot affect -the outcome of 'git am'. +the outcome of `git am`. pre-commit ~~ -This hook is invoked by 'git commit', and can be bypassed +This hook is invoked by linkgit:git-commit[1], and can be bypassed with the `--no-verify` option. It takes no parameters, and is invoked before obtaining the proposed commit log message and making a commit. Exiting with a non-zero status from this script -causes the 'git commit' command to abort before creating a commit. +causes the `git commit` command to abort before creating a commit. The default 'pre-commit' hook, when enabled, catches introduction of lines with trailing whitespaces and aborts the commit when such a line is found. -All the 'git commit' hooks are invoked with the environment +All the `git commit` hooks are invoked with the environment variable `GIT_EDITOR=:` if the command will not bring up an editor to modify the commit message. prepare-commit-msg ~~ -This hook is invoked by 'git commit' right after preparing the +This hook is invoked by linkgit:git-commit[1] right after preparing the default log message, and before the editor is started. It takes one to three parameters. The first is the name of the file @@ -114,7 +114,7 @@ commit is a merge or a `.git/MERGE_MSG` file exists); `squash` (if a `.git/SQUASH_MSG` file exists); or `commit`, followed by a commit SHA-1 (if a `-c`, `-C` or `--amend` option was given). -If the exit status is non-zero, 'git commit' will abort. +If the exit status is non-zero, `git commit` will abort. The purpose of the hook is to edit the message file in place, and it is not suppressed by the `--no-verify` option. A non-zero exit @@ -127,7 +127,7 @@ help message found in the commented portion of the commit template. commit-msg ~~ -This hook is invoked by 'git commit' and 'git merge', and can be +This hook is invoked by linkgit:git-commit[1] and linkgit:git-merge[1], and can be bypassed with the `--no-verify` option. It takes a single parameter, the name of the file that holds the proposed commit log message. Exiting with a non-zero status causes the command to abort. @@ -143,16 +143,16 @@ The default 'commit-msg' hook, when enabled, detects duplicate post-commit ~~~ -This hook is invoked by 'git commit'. It takes no parameters, and is +This hook is invoked by linkgit:git-commit[1]. It takes no parameters, and is invoked after a commit is made. This hook is meant primarily for notification, and cannot affect -the outcome of 'git commit'. +the outcome of `git commit`. pre-rebase ~~ -This hook is called by 'git rebase' and can be used to prevent a +This hook is called by linkgit:git-rebase[1] and can be used to prevent a branch from
[PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis
The two '' parameters are not optional but the option '--no-index' is. Also move the `--options` part to the same place where the other variants show them. All three items are already correct in the synopsis. Signed-off-by: Andreas Heiduk Reviewed-by: Martin Ågren --- Documentation/git-diff.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt index b0c1bb95c8..6593b58299 100644 --- a/Documentation/git-diff.txt +++ b/Documentation/git-diff.txt @@ -13,7 +13,7 @@ SYNOPSIS 'git diff' [options] --cached [] [--] [...] 'git diff' [options] [--] [...] 'git diff' [options] -'git diff' [options] [--no-index] [--] +'git diff' [options] --no-index [--] DESCRIPTION --- @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk. further add to the index but you still haven't. You can stage these changes by using linkgit:git-add[1]. -'git diff' --no-index [--options] [--] [...]:: +'git diff' [--options] --no-index [--] :: This form is to compare the given two paths on the filesystem. You can omit the `--no-index` option when -- 2.16.2
[PATCH v2 0/6] Some doc-fixes
This reroll incorporates the comments of the first version, including a "large scale" rewrtie of githooks.txt. I'v added "Reviewed-by" and "Signed-off-by" as appropriate. Andreas Heiduk (6): doc: improve formatting in githooks.txt doc: align 'diff --no-index' in text with synopsis doc: clarify ignore rules for git ls-files doc: add '-d' and '-o' for 'git push' git-svn: remove ''--add-author-from' for 'commit-diff' doc: add note about shell quoting to revision.txt Documentation/git-diff.txt | 4 +- Documentation/git-ls-files.txt | 3 +- Documentation/git-push.txt | 3 +- Documentation/git-svn.txt | 2 +- Documentation/githooks.txt | 115 + Documentation/revisions.txt| 6 +++ 6 files changed, 71 insertions(+), 62 deletions(-) -- 2.16.2
Re: [RFC PATCH] checkout: Force matching mtime between files
On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud wrote:
>> The best approach to do so is to have those people do the "touch"
>> thing in their own post-checkout hook. People who use Git as the
>> source control system won't have to pay runtime cost of doing the
>> touch thing, and we do not have to maintain such a hook script.
>> Only those who use the "feature" would.
>
>
> The post-checkout hook approach is not exactly straightforward.
I am revisiting this because I'm not even happy with my
post-checkout-modified hook suggestion, so..
>
> Naively, it's simply
>
> for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched. (Granted, Michał's
> proposed patch also doesn't deal with symlinks.) Let's assume that a hook
> can be crafted will all possible sophistication. There are still some
> fundamental problems:
OK so this one could be tricky to get right, but it's possible.
>
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
> identical so the above loop does nothing. Offhand I'm not even sure how a
> hook might get the right files in this case.
This is a limitation of the current post-checkout hook. $3==0 from the
hook lets us know this is not a branch switch, but it does not really
tell you the affected paths. If it somehow passes all the given
pathspec to you, then you should be able to do "git ls-files --
$pathspec" which gives you the exact same set of paths that
git-checkout updates. We could do this by setting $4 to "--" and put
all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
the above example.
There is third case here, if you do "git checkout --
path/to/file" then it cannot be covered by the current design. I guess
we could set $3 to '2' (retrieve from a tree) to indicate this in
addition to 0 (from index) and 1 (from switching branch) and then $1
could be the tree in question (pathspecs are passed the same way
above)
[1] I wonder if we could have a more generic approach to pass
pathspecs via environment, which could work for more than just this
one hook. Not sure if it's a good idea though.
> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).
Either --template or core.hooksPath would solve this, or I'll try to
get my "hooks.* config" patch in. I think that one is a good thing to
do anyway because it allows more flexible hook management (and it
could allow multiple hooks of the same name too). With this, we could
avoid adding more command-specific config like core.fsmonitor or
uploadpack.packObjectsHook which to me are hooks.
Another option is extend core.hooksPath for searching multiple places
instead of just one like AEvar suggested.
> * A fresh clone can't run the hook. This is especially important when
> dealing with submodules. (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and decided
> to re-autoconf the entire thing.)
Both --template and config-based hooks should let you handle this case.
So, I think if we improve the hook system to give more information
(which is something we definitely should do), we could do this without
adding special cases in git. I'm not saying that we should never add
special cases, but at least this lets us play with different kinds of
post-checkout activities before we decide which one would be best
implemented in git.
--
Duy
Re: Great Investment Offer
Hello In my search for a business partner i got your contact in google search. My client is willing to invest $10 Million to $500 million but my client said he need a trusted partner who he can have a meeting at the point of releasing his funds. I told my client that you have a good profile with your company which i got details about you on my search on google lookup. Can we trust you. Can we make a plan for a long term business relationship. Please reply. Regards, Gagum Melvin Sikze Kakha Tel: 703197576
Re: [PATCH 5/6] git-svn: commit-diff does not support --add-author-from
Am 17.04.2018 um 08:18 schrieb Eric Wong: > Andreas Heiduk wrote: >> Signed-off-by: Andreas Heiduk > > Thanks. > Signed-off-by: Eric Wong > > And pushed for Junio: [...] I'd like to keep the patches together, so I borrow your 'Signed-off-By' from the commit and do a complete reroll of this mini-series.
Re: fixup! [PATCH 1/6] doc: fix formatting inconsistency in githooks.txt
Am 12.04.2018 um 21:36 schrieb Martin Ågren: > On 11 April 2018 at 23:08, Andreas Heiduk wrote: >> - reflow some paragraphs >> --- >> Documentation/githooks.txt | 14 +++--- >> 1 file changed, 7 insertions(+), 7 deletions(-) > > I have reviewed the resulting githooks.txt. See the diff below for two > more instances that I found. For the second hunk, I have difficulties > parsing that paragraph, but I still claim those should be backticks and > *git* read-tree... > > Martin I've added the fixes for the next reroll and put you into a Reviewed-by Trailer :-) Andreas
Re: branch --contains / tag --merged inconsistency
Szia Feri, > I'm moving the IRC discussion here, because this might be a bug report > in the end. So, kindly try these steps (103 MB free space required): > > $ git clone https://github.com/ClusterLabs/pacemaker.git && cd pacemaker > [...] > $ git branch --contains Pacemaker-0.6.1 > * master > $ git tag --merged master | fgrep Pacemaker-0.6 > Pacemaker-0.6.0 > Pacemaker-0.6.2 > Pacemaker-0.6.3 > Pacemaker-0.6.4 > Pacemaker-0.6.5 > Pacemaker-0.6.6 > > Notice that Pacemaker-0.6.1 is missing from the output. Kind people on > IRC didn't find a quick explanation, and we all had to go eventually. > Is this expected behavior? Reproduced with git 2.11.0 and 2.17.0. The commit pointed to by the tag Pacemaker-0.6.1 and its parent have a serious clock skew, i.e. they are a few months older than their parents: $ git log --format='%h %ad %cd%d%n%s' --date=short Pacemaker-0.6.1^..47a8ef4c 47a8ef4ce 2008-02-15 2008-02-15 Low: TE: Logging - display the op's magic field for unexpected and foreign events b9cfcd6b4 2007-12-10 2007-12-10 (tag: Pacemaker-0.6.2) haresources2cib.py: set default-action-timeout to the default (20s) 52e7793e0 2007-12-10 2007-12-10 haresources2cib.py: update ra parameters lists dea277271 2008-02-14 2008-02-14 Medium: Build: Turn on snmp support in rpm packages (patch from MATSUDA, Daiki) f418742fe 2008-02-14 2008-02-14 Low: Build: Update the .spec file with the one used by build service ccfa716a5 2008-02-14 2008-02-14 Medium: SNMP: Allow the snmp subagent to be built (patch from MATSUDA, Daiki) 50f0ade2d 2008-02-14 2008-02-14 Low: Build: Update last release number 90f11667f 2008-02-14 2008-02-14 Medium: Tools: Make sure the autoconf variables in haresources2cib are expanded 9d2383c46 2008-02-11 2008-02-11 (tag: Pacemaker-0.6.1) High: cib: Ensure the archived file hits the disk before returning (branch|tag|describe|...) (--contains|--merged) use the commit timestamp information as a heuristic to avoid traversing parts of the history, which makes these operations, especially on big histories, an order of magnitude or two faster. Yeah, commit timestamps can't always be trusted, but skewed commits are rare, and skewed commits with this much skew are even rarer. I'm not sure how (or if it's at all possible) to turn off this timestamp-based optimisation. FWIW, much work is being done on a cached commit graph including commit generation numbers, which will solve this issue both correctly and more efficiently. Perhaps it will already be included in the next release.
Hello My Dear Friend,
I have a business proposal in the tune of $10.2 Million USD for you to handle with me. I have opportunity to transfer this abandon fund to your bank account in your country which belongs to our client. I am inviting you in this transaction where this money can be shared between us at ratio of 60/40% and help the needy around us don’t be afraid of anything I am with you I will instruct you what you will do to maintain this fund. Please kindly contact me with your information's if you are interested in this transaction for more details. Your Name:.. Your Bank Name:. Your Account Number:... Your Telephone Number: Your Country And Address: Your Age And Sex:... Thanks Mrs.Zainab Ahmed,
branch --contains / tag --merged inconsistency
Hi, I'm moving the IRC discussion here, because this might be a bug report in the end. So, kindly try these steps (103 MB free space required): $ git clone https://github.com/ClusterLabs/pacemaker.git && cd pacemaker [...] $ git branch --contains Pacemaker-0.6.1 * master $ git tag --merged master | fgrep Pacemaker-0.6 Pacemaker-0.6.0 Pacemaker-0.6.2 Pacemaker-0.6.3 Pacemaker-0.6.4 Pacemaker-0.6.5 Pacemaker-0.6.6 Notice that Pacemaker-0.6.1 is missing from the output. Kind people on IRC didn't find a quick explanation, and we all had to go eventually. Is this expected behavior? Reproduced with git 2.11.0 and 2.17.0. -- Thanks, Feri
Re: [PATCH v3 2/3] merge: Add merge.renames config setting
Hi Dsco,
On Fri, Apr 27, 2018 at 12:23 AM, Johannes Schindelin
wrote:
> Hi,
>
> Guys, you argued long and hard that one config setting (diff.renames)
> should magically imply another one (merge.renames), on the basis that they
> essentially do the same.
I apologize for any frustration; I've probably caused a good deal of
it by repeatedly catching or noticing things after one review and then
bringing them up later. I just didn't catch it all at first.
Your restatement of the basis of the argument doesn't match my
rationale, though. My reasoning was that (1) the configuration
options exist to allow the user to specify how much of a performance
cost they're willing to pay, (2) the two options are separate because
some users might want to configure one more loosely or tightly than
the other, but (3) many users will want to just specify once how much
they are willing to pay without being forced to repeat themselves for
different parts of git (diff, merge, possible future commands).
I'll add to my former basis by stating that I think the diffstat at
the end of the merge should be controlled by these variables, even if
that's not implemented as part of Ben's series. And both of those
configuration options clearly feed into whether that diffstat should
be doing rename or copy or no detection. While they could be kept
separate options, forcing us to somehow decide which one overrides
which, I think it's much more natural if we already have merge.renames
inheriting from and overriding diff.renames.
> And now you are suggesting that they *cannot* be essentially the same? Are
> you agreeing on such a violation of the Law of Least Surprise?
>
> Please, make up your mind. Inheriting merge.renames from diff.renames is
> "magic" enough to be puzzling. Introducing that auto-demoting is just
> simply creating a bad user experience. Please don't.
>From my view, resolve and octopus merges auto-demote diff.renames and
merge.renames to false. In fact, they optimize the work involved in
that demotion by not even checking the value. I think demoting "copy"
to "true" in the recursive merge machinery is no more surprising than
that is. You can find more details to this argument in the portion of
my email that you responded to but snipped out.
But to add to that argument, if auto-demotion is a violation of the
Law of Least Surprise, as you claim, then naming this option
merge.renames is also a violation of the Law of Least Surprise. You
would instead need to rename it to something like
merge.recursive.renames. (And then when a new merge strategy comes
along that also does renames, we'll need to add a merge.ort.renames.)
> It is fine, of course, to admit at this stage that the whole magic idea of
> letting merge.renames inherit from diff.renames was only half thought
> through (we're in reviewing stage right now for the purpose of weighing
> alternative approaches and trying to come up with the best solution after
> all) and that we should go with Ben's original approach, which has the
> rather huge and dramatic advantage of being very, very clear and easy to
> understand to the user. We could even document that (and why) the 'copy'
> value in merge.renames is not allowed for the time being.
It was only half thought through, yes, at least by me. But the more I
think through it, the more I like the inheritance personally. I see
no problem with the demotion and think the inheritance has the
advantage of being easier to understand, because I see your proposal
as causing questions like:
- "Why does merge.renameLimit inherit from diff.renameLimit but
merge.renames doesn't from diff.renames?"
- "Why can't I just specify in one place that I {am, am not} willing
to pay for {full, both copy and} rename detection wherever it makes
sense?"
- "How do I control the detection for the diffstat at the end of the merge?"
Hope that helps,
Elijah
[PATCH v1] perf/bisect_run_script: disable codespeed
When bisecting a performance regression using a config file,
`./bisect_regression --config my_perf.conf` for example, the
config file can contain Codespeed configuration which would
instruct the 'aggregate.perl' script called by the 'run'
script to output results in the Codespeed format and maybe
to try to send this output to a Codespeed server.
This in unfortunate because the 'bisect_run_script' relies
on the regular output from 'aggregate.perl' to mesure
performance, so let's disable Codespeed output and sending
results to a Codespeed server.
Signed-off-by: Christian Couder
---
t/perf/bisect_run_script | 6 ++
1 file changed, 6 insertions(+)
diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script
index 038255df4b..3ebaf15521 100755
--- a/t/perf/bisect_run_script
+++ b/t/perf/bisect_run_script
@@ -24,6 +24,12 @@
result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt"
GIT_PERF_DIRS_OR_REVS="$bisect_head"
export GIT_PERF_DIRS_OR_REVS
+# Don't use codespeed
+GIT_PERF_CODESPEED_OUTPUT=
+GIT_PERF_SEND_TO_CODESPEED=
+export GIT_PERF_CODESPEED_OUTPUT
+export GIT_PERF_SEND_TO_CODESPEED
+
./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'"
rtime=$(sed -n
"s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p"
"$result_file")
--
2.17.0.257.g28b659db43
[ANNOUNCE] Gitwin: Git Server for Windows with SSH/HTTP(S) transport and Gitweb
Hi all, This is a ONE-TIME announcement of Gitwin - a Git Server for Windows: Gitwin is a packaging of Git, OpenSSH, Nginx and many other related tools to make it a ready-to-use solution as a secure git repository on Windows. It supports SSH and HTTP(S) transports as well as Gitweb with Highlighter and Gravatar support. A free edition is available. Please visit https://itefix.net/gitwin for more information Kind regards Tevfik Karagulle Itefix.net
Re: git merge banch w/ different submodule revision
Hi, firstofall: thank all of you for your feedback. Am Donnerstag, den 26.04.2018, 17:19 -0700 schrieb Elijah Newren: > On Thu, Apr 26, 2018 at 3:49 AM, Middelschulte, Leif > wrote: > > Hi, > > > > we're using git-flow as a basic development workflow. However, doing so > > revealed unexpected merge-behavior by git. > > > > Assume the following setup: > > > > - Repository `S` is sourced by repository `p` as submodule `s` > > - Repository `p` has two branches: `feature_x` and `develop` > > - The revisions sourced via the submodule have a linear history > > > > > > * 1c1d38f (feature_x) update submodule revision to b17e9d9 > > > * 3290e69 (HEAD -> develop) update submodule revision to 0598394 > > > / > > > > * cd5e1a5 initial submodule revision > > > > > > Problem case: Merge either branch into the other > > > > Expected behavior: Merge conflict. > > > > Actual behavior: Auto merge without conflicts. > > > > Note 1: A merge conflict does occur, if the sourced revisions do *not* have > > a linear history > > > > Did I get something wrong about how git resolves merges? Shouldn't git be > > like: "hey, you're trying to merge two different contents for the same > > line" (the submodule's revision) > > Hard to say without saying what commit was referenced for the > submodule in the merge-bases for the two repositories you have. In > the basic case.. > > If branch A and branch B have different commits checked out in the > submodule, say: >A: deadbeef >B: ba5eba11 > > then it's not clear whether there's a conflict or not. The merge-base > (the common point of history) matters. So, for example if the > original version (which I'll refer to as 'O") had: > O: deadbeef > > then you would say, "Oh, branch A made no change to this submodule but > B did. So let's go with what B has." Conversely, of O had ba5eba11, > then you'd go the other way. > > But, there is some further smarts in that if either A or B point at > commits that contain the other in their history and both contain the > commit that O points at, then you can just do a fast-forward update to > the newest. > > > You didn't tell us how the merge-base (cd5e1a5 from the diagram you > gave) differed in your example here between the two repositories. In > fact, the non-linear case could have several merge-bases, in which > case they all become potentially relevant (as does their merge-bases > since at that point you'll trigger the recursive portion of > merge-recursive). Giving us that info might help us point out what > happened, though if either the fast-forward logic comes into play or > the recursive logic gets in the mix, then we may need you to provide a > testcase (or access to the repo in question) in order to explain it > and/or determine if you've found a bug. I placed two reositories here: https://gitlab.com/foss-contributions/git-examples/network/develop The access should be public w/o login. If you prefer the examples to be placed somewhere else, let me know. > > Does that help? I guess it's somehow understandable that it tries to be more smart about things wrt submodules. However, I believe that there should be some kind of choice here. Not giving *any* notice, makes testing feature-branches hell. I hope the provided example exhibits the challenge. BR, Leif > > Elijah >
Re: In some rebases, `exec git -C ...` has wrong working directory
Hi William,
On Thu, 26 Apr 2018, William Chargin wrote:
> Here is a repro script:
>
> #!/bin/sh
> set -eux
> git --version
> tmpdir="$(mktemp -d)"
> cd "${tmpdir}"
> mkdir target repo
> cd repo
> git init
> touch file; git add file
> git commit -m "Initial commit"
> git rebase HEAD --exec "git -C ${tmpdir}/target init"
We do take pains to pass the GIT_DIR to the exec'ed command, and it is the
GIT_DIR of the worktree in which the rebase runs.
> The end of this script prints something like
>
> Executing: git -C /tmp/tmp.gd2q51jO93/target init
> Reinitialized existing Git repository in /tmp/tmp.gd2q51jO93/repo/.git/
> Successfully rebased and updated refs/heads/master.
So that is actually what I expected.
I might even have one or two scripts relying on that feature, where
something like
exec git -C /somewhere/else show HEAD:some-file >some-other-file
is executed, and works.
> But this is wrong: the repository should be initialized in `target`, not
> reinitialized in `repo`.
>
> Notes:
>
> - This propagates to subprocesses: if you run `exec make test` and
> your test suite ends up calling `git -C`, then the same problem
> occurs.
>
> - Substituting `rebase --root` for `rebase HEAD` causes the problem to
> go away.
>
> - The `rebase HEAD` exec context adds the `GIT_DIR` environment
> variable, and this is sufficient to reproduce the problem:
> running `GIT_DIR="$PWD" git -C /tmp/target init` puts the repo in
> the current working directory. The `rebase --root` context adds no
> such environment variable. (You can use `--exec 'env >tempfile'` to
> verify these.)
>
> My `git --version` is 2.16.2.
Even `pu` has the same behavior.
I do not think that we can sensibly *remove* GIT_DIR from the environment
variables passed to the exec'ed command, as we have been doing that for
ages, and some scripts (as demonstrated above) started relying on that
behavior. So if we changed it, we would break backwards-compatibility,
which is something we try to avoid very much in Git.
Maybe you could a contribute a patch to the documentation? Something that
tells people who use the `--exec` option that they want to prefix their
command with `unset GIT_DIR; ` when their command wants to work with a
different Git repository than the current one?
Ciao,
Johannes
Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)
On Wed, Apr 25, 2018 at 4:37 AM, Junio C Hamano wrote: > * tg/worktree-add-existing-branch (2018-04-25) 4 commits > - worktree: teach "add" to check out existing branches > - worktree: factor out dwim_branch function > - worktree: improve message when creating a new worktree > - worktree: remove extra members from struct add_opts > > "git worktree add" learned to check out an existing branch. > > Is this ready for 'next'? I just gave v9 (which you have queued) a final full review and found it satisfactory (and gave my Reviewed-by:), so yes, it seems ready for 'next'. Thanks.
Re: [PATCH v9 0/4] worktree: teach "add" to check out existing branches
On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer wrote: > Thanks Eric for the review and the suggestions on the previous round. > > Changes since the previous round: > > - UNLEAK new_branch after it was xstrndup'd > - update the commit message of 2/4 according to Eric's suggestions > - make force_new_branch a boolean flag in > print_preparing_worktree_line, instead of using it as the branch > name. Instead use new_branch as the new branch name everywhere in > that function. > - get rid of the ckeckout_existing_branch flag Thanks. I did another full review of all the patches and played around with the new functionality again for good measure. Everything looked good, and I think the patches are now ready to graduate. For what it's worth, the entire series is: Reviewed-by: Eric Sunshine
Re: [PATCH v3 2/3] merge: Add merge.renames config setting
Hi, On Thu, 26 Apr 2018, Elijah Newren wrote: > On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano wrote: > > Ben Peart writes: > > > >> Color me puzzled. :) The consensus was that the default value for > >> merge.renames come from diff.renames. diff.renames supports copy > >> detection which means that merge.renames will inherit that value. My > >> assumption was that is what was intended so when I reimplemented it, I > >> fully implemented it that way. > >> > >> Are you now requesting to only use diff.renames as the default if the > >> value is true or false but not if it is copy? What should happen if > >> diff.renames is actually set to copy? Should merge silently change > >> that to true, display a warning, error out, or something else? Do you > >> have some other behavior for how to handle copy being inherited from > >> diff.renames you'd like to see? > >> > >> Can you write the documentation that clearly explains the exact > >> behavior you want? That would kill two birds with one stone... :) > > > > I think demoting from copy to rename-only is a good idea, at least > > for now, because I do not believe we have figured out what we want > > to happen when we detect copied files are involved in a merge. > > > > But I am not sure if we even want to fail merge.renames=copy as an > > invalid configuration. So my gut feeling of the best solution to > > the above is to do something like: > > > > - whether the configuration comes from diff.renames or > >merge.renames, turn *.renames=copy to true inside the merge > >recursive machinery. > > > > - document the fact in "git merge-recursive" documentation (or "git > >merge" documentation) to say "_currently_ asking for rename > >detection to find copies and renames will do the same > >thing---copies are ignored", impliying "this might change in the > >future", in the BUGS section. > > Yes, I agree. Guys, you argued long and hard that one config setting (diff.renames) should magically imply another one (merge.renames), on the basis that they essentially do the same. And now you are suggesting that they *cannot* be essentially the same? Are you agreeing on such a violation of the Law of Least Surprise? Please, make up your mind. Inheriting merge.renames from diff.renames is "magic" enough to be puzzling. Introducing that auto-demoting is just simply creating a bad user experience. Please don't. It is fine, of course, to admit at this stage that the whole magic idea of letting merge.renames inherit from diff.renames was only half thought through (we're in reviewing stage right now for the purpose of weighing alternative approaches and trying to come up with the best solution after all) and that we should go with Ben's original approach, which has the rather huge and dramatic advantage of being very, very clear and easy to understand to the user. We could even document that (and why) the 'copy' value in merge.renames is not allowed for the time being. Ciao, Dscho
Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)
Hi, On Thu, 26 Apr 2018, Derrick Stolee wrote: > On 4/25/2018 1:43 PM, Brandon Williams wrote: > > On 04/25, Ævar Arnfjörð Bjarmason wrote: > > > > * bw/protocol-v2 (2018-03-15) 35 commits > > > > (merged to 'next' on 2018-04-11 at 23ee234a2c) [... snip ...] > > > > (this branch is used by bw/server-options.) > > > > > > > > The beginning of the next-gen transfer protocol. > > > > > > > > Will cook in 'next'. > > > > > > With a month & 10 days of no updates & this looking stable it would > > > be great to have it in master sooner than later to build on top of > > > it in the 2.18 window. > > > > I personally think that this series is ready to graduate to master but > > I mentioned to Junio off-list that it might be a good idea to wait > > until it has undergone a little more stress testing. We've been in > > the process of trying to get this rolled out to our internal server > > but due to a few roadblocks and people being out of the office its > > taken a bit longer than I would have liked to get it up and running. > > I'm hoping in another few days/a week I'll have some more data on live > > traffic. At that point I think I'll be more convinced that it will be > > safe to merge it. > > > > I may be overly cautions but I'm hoping that we can get this right > > without needing to do another protocol version bump for a very long > > time. Technically using v2 is hidden behind an "experimental" config > > flag so we should still be able to tweak it after the fact if we > > absolutely needed to, but I'd like to avoid that if necessary. > > There's no testing better than production. I think if we have an > opportunity to test with realistic traffic, we should take advantage. > > But I also agree that this series looks stable. > > I realize it's hard to communicate both "this series is ready to merge" and "I > appreciate your caution." To add to Stolee's comment: in our (day job) team, we are huge fans of "Feature Flags". We blog plenty about it, GitHub uses it in their Scientist approach, and even here in the Git project, we managed to use the paradigm: remember the transition from the scripted version of `difftool` to the builtin version? That was also done via a Feature Flag. In the same vein, I would treat your config setting as a Feature Flag, and I would be very much in favor of having the code in production, with that feature flag defaulting to "off", and with a description that tells people very clearly that not only the flag is experimental, but that we might need to change the protocol, still, before v2 goes final. I would then add an entry to Git for Windows' installer's "Experimental Options" section, to give it a bit more exposure (mainly to verify that the v2 client <-> v1 server connection works flawlessly), also as opt-in. How does that sound? Ciao, Dscho
