* [PATCH v2 0/2] fix data corruption in fast-import
From: Dmitry Ivankov @ 2011-08-14 18:32 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Dmitry Ivankov
In-Reply-To: <1313145170-24471-1-git-send-email-divanorama@gmail.com>
It turns out the bug is older than "M 040000.." command.
Managed to reproduce with just "C .." command from tags/v1.5.3-rc2~6^2
b6f3481b.. Teach fast-import to recursively copy files/directories (Jul 15 2007)
And even better, there is no need to add a explicit check for sha1 mismatch (we
may still want to have this, but it can go separately).
Basically the test does:
Fill two distinct directories old/a, old/b
Commit them
(necessary to make trees have sha1 computed and thus become potential delta bases)
C old new
C old/a new/b
M ... new/b/new_file
new/b is stored as a delta against old/a, but with delta base pointing to old/b.
And so ls-tree new/b fails, fsck fails both with "failed to apply delta".
Dmitry Ivankov (2):
fast-import: add a test for tree delta base corruption
fast-import: prevent producing bad delta
fast-import.c | 35 ++++++++++++++++++++++++++++++-----
t/t9300-fast-import.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+), 5 deletions(-)
--
1.7.3.4
^ permalink raw reply
* Re: [PATCH v4 0/2] push limitations
From: Jens Lehmann @ 2011-08-14 17:29 UTC (permalink / raw)
To: git; +Cc: Fredrik Gustafsson, gitster, hvoigt
In-Reply-To: <1313312262-2460-1-git-send-email-iveqy@iveqy.com>
Please ignore this series, it slipped through by accident.
Am 14.08.2011 10:57, schrieb Fredrik Gustafsson:
> The first iteration of this patch series can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327
>
> The second iteration of this patch series can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/177992
>
> The third iteration of this patch series can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/179037/focus=179048
>
>
> Fredrik Gustafsson (2):
> rev-parse: add option --is-well-formed-git-dir [path]
> push: Don't push a repository with unpushed submodules
>
> Documentation/git-push.txt | 6 ++
> Documentation/git-rev-parse.txt | 4 ++
> builtin/push.c | 17 ++++++
> builtin/rev-parse.c | 8 +++
> cache.h | 1 +
> combine-diff.c | 2 +-
> setup.c | 7 +++
> submodule.c | 108 +++++++++++++++++++++++++++++++++++++++
> submodule.h | 1 +
> t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++++++++++++++
> t/t7400-submodule-basic.sh | 4 +-
> t/t7403-submodule-sync.sh | 5 +-
> t/t7407-submodule-foreach.sh | 97 ++++++++++++++++++-----------------
> transport.c | 9 +++
> transport.h | 1 +
> 15 files changed, 312 insertions(+), 52 deletions(-)
>
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Jonathan Nieder @ 2011-08-14 16:48 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0m0KyZkOqVWJET=_Xy5GxwVfpPkB0OO1b2hJ8aN_x7hGA@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Putting an "int aggressive = 1" and passing the variable instead of
> the literal is a little inelegant.
What about a flag word, like for example read_sha1_file_extended()
takes?
[...]
>> Do you think there might
>> be scripts out there relying on being able to use "git read-tree
>> --reset -u HEAD" to clear away a failed cherry-pick before trying
>> again, and if so, can we do something about it?
>
> I'm not sure we can do anything about it -- we should probably put
> some kind of warning in the commit message?
That's necessary at a minimum.
I actually _don't_ think there might be many scripts relying on "git
read-tree --reset -u HEAD" to clear away a failed cherry-pick, simply
because very few people seem to use plumbing these days :) and those
who do might be of a mindset to use "git apply" instead of
cherry-pick. But it would be prudent to run a code search to check
and to think carefully about the effect on people who did use the
"read-tree --reset && cherry-pick again" idiom.
Hope that helps.
Jonathan
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Ramkumar Ramachandra @ 2011-08-14 16:37 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814160440.GK18466@elie.gateway.2wire.net>
Hi again,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> [...]
>
> The function name (..._notify_commit()) does not seem very intuitive.
> Based on the name, I expect it to use the sequencer to print a message
> to the user about the commit in progress.
For the record, I'm not too happy with the name either. I was hoping
that someone would suggest a better name during the review :P
> .git/index
> - afterward, because when writing the index fails, I (the user)
> might want to react by running "git cherry-pick --abort".
Very subtle point: will fix. I'd originally put it along with the
other state-removing functions.
> Not about this patch: I keep on forgetting what the argument to
> remove_sequencer_state means. Would it be possible to make it
> a flag, which would both make the meaning more obvious and mean
> it is easy to support additional flags in the future?
I don't want to create a struct and populate it because I highly doubt
remove_sequencer_state will take any more arguments in the future.
Putting an "int aggressive = 1" and passing the variable instead of
the literal is a little inelegant. Actually making any change at this
stage is inelegant because of the other remove_sequencer_state() calls
:| My call: we can fix it if and when the function needs more
arguments.
>> --- a/t/t3032-merge-recursive-options.sh
>> +++ b/t/t3032-merge-recursive-options.sh
>> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
>> git read-tree --reset -u HEAD &&
>> test_must_fail git cherry-pick --no-commit remote &&
>> git read-tree --reset -u HEAD &&
>> + git cherry-pick --reset &&
>> test_must_fail git cherry-pick remote &&
>> test_must_fail git update-index --refresh &&
>> + git cherry-pick --reset &&
>> grep "<<<<<<" text.txt
>> '
>
> What is this about? Maybe it would be clearer to change the "git
> read-tree ..." to "git reset --hard", so the test assertion would not
> rely on new cherry-pick features (and to mention the change in the
> commit message!).
Okay, I can do this instead (don't check whitespace! I just typed out
the patch):
@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
git read-tree --reset -u HEAD &&
test_must_fail git cherry-pick --no-commit remote &&
- git read-tree --reset -u HEAD &&
+ git reset --hard &&
test_must_fail git cherry-pick remote &&
test_must_fail git update-index --refresh &&
+ git reset --hard &&
grep "<<<<<<" text.txt
'
> Doesn't this point to a risk in the patch? Do you think there might
> be scripts out there relying on being able to use "git read-tree
> --reset -u HEAD" to clear away a failed cherry-pick before trying
> again, and if so, can we do something about it?
I'm not sure we can do anything about it -- we should probably put
some kind of warning in the commit message?
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '
>> test_path_is_missing .git/sequencer
>> '
>>
>> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
>> +test_expect_success 'final commit cleans up sequencer state' '
>> pristine_detach initial &&
>> test_must_fail git cherry-pick base..picked &&
>> - test_path_is_missing .git/sequencer &&
>> echo "resolved" >foo &&
>> git add foo &&
>> git commit &&
>> + test_path_is_missing .git/sequencer &&
>> {
>
> It would also be nice to check "test_path_is_dir" before the final
> commit, so people working on this code in the future know both aspects
> of the patch are intentional.
Good point. Fixed.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional
From: Ramkumar Ramachandra @ 2011-08-14 16:21 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814160923.GL18466@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> If I understand correctly, the current code makes it perfectly possible
> to use commands like
>
> pick origin/master^^2~5
I was thinking exactly the same thing a few hours ago! I was
wondering what to do about such a tree'ish specification, and I came
to the conclusion that we should discourage this atleast for normal
users. Why? Because the tree'ish has to be invariant over the entire
sequencer operation*: something like "pick HEAD~3" is clearly flawed
since the picking operation updates HEAD.
* Some of us might find some really clever uses for a tree'ish that
refers to different commits during the course of the sequencer
operation, and I'm alright with that. It might just be something
that's too complicated for many users to grok, and we can protect them
by not advertising this tree'ish operand feature.
> when that is more convenient to type. So I wonder whether 40 is actually
> a good limit. Another possibility would be to do something like
>
> static struct strbuf cmit = STRBUF_INIT;
I don't like this; the sequencer will just end up drinking a lot of
memory for no reason if the insn sheet is horribly corrupted. I think
40 is a very reasonable limit, even for the most complex tree'ish I
can dream of.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 1/7] revert: Free memory after get_message call
From: Jonathan Nieder @ 2011-08-14 16:15 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> The format_todo function leaks memory because it forgets to call
> free_message after get_message. It is a potentially big leak, because
> fresh memory is allocated to store the commit message message for each
> commit. Fix this.
Reading this, I think, "why does formatting an insn list require all
these heavy memory allocations?". So while the patch looks good to
me, I start to wonder if something using find_commit_subject()
directly would not be easier.
^ permalink raw reply
* Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional
From: Jonathan Nieder @ 2011-08-14 16:09 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -697,26 +697,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
> unsigned char commit_sha1[20];
> char sha1_abbrev[40];
[...]
> + q = p + strcspn(p, " \n");
> + if (q - p + 1 > sizeof(sha1_abbrev))
> return NULL;
If I understand correctly, the current code makes it perfectly possible
to use commands like
pick origin/master^^2~5
when that is more convenient to type. So I wonder whether 40 is actually
a good limit. Another possibility would be to do something like
static struct strbuf cmit = STRBUF_INIT;
strbuf_reset(&cmit);
...
strbuf_add(&cmit, p, q - p);
to avoid having to worry about the buffer size altogether.
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Jonathan Nieder @ 2011-08-14 16:04 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-8-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Since d3f4628e (revert: Remove sequencer state when no commits are
> pending, 2011-07-06), the sequencer removes the sequencer state before
> the final commit is actually completed. This design is inherently
> flawed, as it will not allow the user to abort the sequencer operation
> at that stage. Instead, make 'git commit' notify the sequencer after
> every successful commit; the sequencer then removes the state if no
> more instructions are pending.
Sorry, I'm getting lost in all the words. I suspect you are saying
“d3f4628e was trying to solve such-and-such problem, but its fix was
problematic because it removes the data that a hypothetical "git
cherry-pick --abort" command would need to work. Back out that
change and adopt the following instead.”
In particular, the above does not make it clear to me:
- as a user, what effect will I notice after this change?
- what problem does it solve?
- does it have any downsides?
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -26,6 +26,7 @@
> #include "unpack-trees.h"
> #include "quote.h"
> #include "submodule.h"
> +#include "sequencer.h"
>
> static const char * const builtin_commit_usage[] = {
> "git commit [options] [--] <filepattern>...",
> @@ -1521,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> unlink(git_path("MERGE_MODE"));
> unlink(git_path("SQUASH_MSG"));
>
> + /*
> + * Notify the sequencer that we're committing. The routine
> + * removes the sequencer state if our commit just completed
> + * the last instruction.
> + */
> + sequencer_notify_commit();
> +
> if (commit_index_files())
> die (_("Repository has been updated, but unable to write\n"
> "new_index file. Check that disk is not full or quota is\n"
The function name (..._notify_commit()) does not seem very intuitive.
Based on the name, I expect it to use the sequencer to print a message
to the user about the commit in progress.
What happens if writing to .git/index fails? I can think of reasons
to remove the sequencer file before or afterward:
- before, because once .git/index has been removed, the index is not
locked any more and further commands could take place in parallel.
- afterward, because when writing the index fails, I (the user)
might want to react by running "git cherry-pick --abort".
The latter seems slightly more compelling to me --- after all, any
further command wanting to touch the sequencer directory is going
to check whether it exists --- but more importantly, the former
reminds me that we haven't thought carefully about what concurrent
operations using the sequencer are and aren't allowed. Though I
doubt that it would come up much in practice. :)
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
> die(_("Unusable instruction sheet: %s"), todo_file);
> }
>
> +void sequencer_notify_commit(void)
> +{
> + struct replay_insn_list *todo_list = NULL;
> +
> + if (!file_exists(git_path(SEQ_TODO_FILE)))
> + return;
> + read_populate_todo(&todo_list);
> + if (!todo_list->next)
> + remove_sequencer_state(1);
> +}
Not about this patch: I keep on forgetting what the argument to
remove_sequencer_state means. Would it be possible to make it
a flag, which would both make the meaning more obvious and mean
it is easy to support additional flags in the future?
> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
> git read-tree --reset -u HEAD &&
> test_must_fail git cherry-pick --no-commit remote &&
> git read-tree --reset -u HEAD &&
> + git cherry-pick --reset &&
> test_must_fail git cherry-pick remote &&
> test_must_fail git update-index --refresh &&
> + git cherry-pick --reset &&
> grep "<<<<<<" text.txt
> '
What is this about? Maybe it would be clearer to change the "git
read-tree ..." to "git reset --hard", so the test assertion would not
rely on new cherry-pick features (and to mention the change in the
commit message!).
Doesn't this point to a risk in the patch? Do you think there might
be scripts out there relying on being able to use "git read-tree
--reset -u HEAD" to clear away a failed cherry-pick before trying
again, and if so, can we do something about it?
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '
> test_path_is_missing .git/sequencer
> '
>
> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
> +test_expect_success 'final commit cleans up sequencer state' '
> pristine_detach initial &&
> test_must_fail git cherry-pick base..picked &&
> - test_path_is_missing .git/sequencer &&
> echo "resolved" >foo &&
> git add foo &&
> git commit &&
> + test_path_is_missing .git/sequencer &&
> {
It would also be nice to check "test_path_is_dir" before the final
commit, so people working on this code in the future know both aspects
of the patch are intentional.
Thanks, I'm glad to see this patch.
^ permalink raw reply
* Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery
From: Jonathan Nieder @ 2011-08-14 15:22 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0=zqyvL8zo9wvBGUXyf3RWSZB7dY=WaC9TN6YXnThag0Q@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:
>> git_path() calls vsnprintf which clobbers errno, so depending on the
>> platform this can print messages like
>>
>> fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
[...]
> Ugh, yet another "bugfix patch" to queue near the beginning of the
> series. Thanks for catching this.
It's not a bug you introduced, and ideally it would be a separate
patch on its own, against the js/cherry-pick-usability branch.
>> Ramkumar Ramachandra wrote:
>>> +static struct tree *empty_tree(void)
>>> [...]
>>
>> This tree is leaked (for example if you cherry-pick a sequence of
>> root commits).
>
> This is not something I introduced -- it can wait until later, no?
Yep, it's Christian's fault (for introducing multiple-cherry-pick).
Patch below. The things I do... :)
[...]
> It is a simple code movement. Is there something I can do to help?
Part of the review was about examples where it was not simple code
movement. Maybe if there is another round the thing to do would be to
send a patch generated with -B -M to allow others to more easily check
it.
Thanks.
-- >8 --
Subject: revert: plug memory leak in "cherry-pick root commit" codepath
The empty tree passed as common ancestor to merge_trees() when
cherry-picking a parentless commit is allocated on the heap and never
freed. Leaking such a small one-time allocation is not a very big
problem, but now that "git cherry-pick" can cherry-pick multiple
commits it can start to add up.
Avoid the leak by storing the fake tree exactly once in the BSS
section (i.e., use a static). While at it, let's add a test to make
sure cherry-picking multiple parentless commits continues to work.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Patch is against cc/cherry-pick-series (86c7bb47).
builtin/revert.c | 10 +++++-----
t/t3503-cherry-pick-root.sh | 27 ++++++++++++++++++++++++++-
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 853e9e40..c1b0fb3d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -273,12 +273,12 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
static struct tree *empty_tree(void)
{
- struct tree *tree = xcalloc(1, sizeof(struct tree));
+ static struct tree tree;
- tree->object.parsed = 1;
- tree->object.type = OBJ_TREE;
- pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
- return tree;
+ tree.object.parsed = 1;
+ tree.object.type = OBJ_TREE;
+ pretend_sha1_file(NULL, 0, OBJ_TREE, tree.object.sha1);
+ return &tree;
}
static NORETURN void die_dirty_index(const char *me)
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index b0faa299..472e5b80 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -16,15 +16,40 @@ test_expect_success setup '
echo second > file2 &&
git add file2 &&
test_tick &&
- git commit -m "second"
+ git commit -m "second" &&
+
+ git symbolic-ref HEAD refs/heads/third &&
+ rm .git/index file2 &&
+ echo third > file3 &&
+ git add file3 &&
+ test_tick &&
+ git commit -m "third"
'
test_expect_success 'cherry-pick a root commit' '
+ git checkout second^0 &&
git cherry-pick master &&
test first = $(cat file1)
'
+test_expect_success 'cherry-pick two root commits' '
+
+ echo first >expect.file1 &&
+ echo second >expect.file2 &&
+ echo third >expect.file3 &&
+
+ git checkout second^0 &&
+ git cherry-pick master third &&
+
+ test_cmp expect.file1 file1 &&
+ test_cmp expect.file2 file2 &&
+ test_cmp expect.file3 file3 &&
+ git rev-parse --verify HEAD^^ &&
+ test_must_fail git rev-parse --verify HEAD^^^
+
+'
+
test_done
--
1.7.6
^ permalink raw reply related
* [PATCH] Utilize config variable pager.stash in stash list command
From: Ingo Brückl @ 2011-08-14 14:31 UTC (permalink / raw)
To: git
Signed-off-by: Ingo Brückl <ib@wupperonline.de>
---
By now stash list ignores it.
git-stash.sh | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index f4e6f05..7bb0856 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,7 +264,8 @@ have_stash () {
list_stash () {
have_stash || return 0
- git log --format="%gd: %gs" -g "$@" $ref_stash --
+ test "$(git config --get pager.stash)" = "false" && no_pager=--no-pager
+ git $no_pager log --format="%gd: %gs" -g "$@" $ref_stash --
}
show_stash () {
--
1.7.6
^ permalink raw reply related
* Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-08-14 14:28 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0=sggexB0HDN-6LsoLz2RO-ftABTnAYp7q-jVO0oCJu2A@mail.gmail.com>
Ramkumar Ramachandra wrote:
> int len = strchrnul(p, '\n') - p;
> if (len > 255 || len - strchrnul(p, '\n') + p != 0)
> len = 255;
Yuck. Why not just use a size_t?
> Shouldn't it be part of error() atleast in the long term?
The default implementation of error() is (practically speaking) a thin
wrapper around fprintf. What are the new semantics you are proposing
--- truncating %s arguments to one short line? I don't think that
would be a good idea, since for example, some current callers might be
passing "\n" explicitly as a %s argument.
^ permalink raw reply
* Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser
From: Ramkumar Ramachandra @ 2011-08-14 14:07 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814115821.GC18466@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '
>> [...]
>> + sed "s/pick \([0-9a-f]\+\)\(.*\)/pick \1\1\1\1\1\1\1\1\2/" .git/sequencer/todo >new_sheet &&
>
> This construct (\+ in sed regexes) is not portable. See the note on
> grep in Documentation/CodingGuidelines
I see. Could you show me how to do it right? My regex-foo is pretty
weak, and I don't know what else to do.
> (maybe it should be tweaked to
> say "grep and sed").
Okay. Do you like this?
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
-- 8< --
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index fe1c1e5..0a843ea 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -69,8 +69,8 @@ For shell scripts specifically (not exhaustive):
- We do not write the noiseword "function" in front of shell
functions.
- - As to use of grep, stick to a subset of BRE (namely, no \{m,n\},
- [::], [==], nor [..]) for portability.
+ - As to use of grep and sed, stick to a subset of BRE (namely, no
+ \{m,n\}, [::], [==], nor [..]) for portability.
- We do not use \{m,n\};
-- Ram
^ permalink raw reply related
* Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-08-14 14:06 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814121244.GD18466@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> + int len = strchrnul(p, '\n') - p;
>> + if (len > 255)
>> + len = 255;
>> + return error(_("Unrecognized action: %.*s"), len, p);
>
> What happens if the current line has more than INT_MAX characters?
I don't know! I don't think this behavior is defined by the C89
standard, so I'd expect every compiler to do something different. I'd
expect to be able to fix it like this:
int len = strchrnul(p, '\n') - p;
if (len > 255 || len - strchrnul(p, '\n') + p != 0)
len = 255;
> Maybe it would make sense to factor out a function for this
> computation, for brevity and so there is just one place to tweak.
Shouldn't it be part of error() atleast in the long term? I'll write
a quick private helper for now.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery
From: Ramkumar Ramachandra @ 2011-08-14 13:57 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814131303.GF18466@elie.gateway.2wire.net>
Hi again,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Move code from builtin/revert.c to sequencer.c and expose a public API
>> without making any functional changes. Although it is useful only to
>> existing callers of cherry-pick and revert now, this patch lays the
>> foundation for future expansion.
>
> :) It sounds like you are running a business.
*laughs* Now that you mention it, it certainly does :)
>> +++ b/sequencer.c
>> @@ -1,13 +1,656 @@
>> [...]
>> +#define COMMIT_MESSAGE_INIT { NULL, NULL, NULL, NULL, NULL };
I'll get rid of this.
>> +static const char *action_keyword(const struct replay_opts *opts)
>> [...]
>
> Another (non-functional) change. Probably (?) this renaming has a
> good reason to be part of this patch, but it should definitely be
> mentioned in the commit message.
Yes, I want the rename to be part of this patch (see Daniel's comment
and my agreement). Will clarify in the commit message.
> git_path() calls vsnprintf which clobbers errno, so depending on the
> platform this can print messages like
>
> fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
>
> The natural fix would be to add a local for it (as a separate patch).
> Sorry I missed this when the code first arrived.
Ugh, yet another "bugfix patch" to queue near the beginning of the
series. Thanks for catching this.
>> +static struct tree *empty_tree(void)
>> [...]
>
> This tree is leaked (for example if you cherry-pick a sequence of
> root commits).
This is not something I introduced -- it can wait until later, no?
>> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)
>> [...]
>
> The exit code here violates the usual "exit with status 128 for
> errors other than conflicts" rule. Perhaps it should be changed to
> "return -1" in a separate patch (to accompany the patch that returns
> error() instead of die()-ing so often to allow callers to give
> additional context to errors from this machinery).
Great catch! Will fix.
>> --- a/sequencer.h
>> +++ b/sequencer.h
> [...]
>> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
>
> I thought sequencer_parse_args() wasn't being exposed.
Rebase fail, sorry. There is no sequencer_parse_args().
> Except where noted above, I hope this is just simple code movement,
> but I haven't checked. If I could be sure, it would be easier to
> review.
It is a simple code movement. Is there something I can do to help?
-- Ram
^ permalink raw reply
* Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions
From: Ramkumar Ramachandra @ 2011-08-14 13:43 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110814125223.GE18466@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> Since I wrote the patch modulo the commit message and small
> improvements, shouldn't I be blamed as the author? (git log
> --grep="[Oo]riginal patch" and git log --grep=jc: give some examples.)
> You can have my sign-off if you'd like.
I'd initially blamed you for writing it, but then I was worried that I
didn't have your signoff -- so I quickly changed it before sending it
out. Will fix in the next round. Thanks for the clarification and
signoff :)
-- Ram
^ permalink raw reply
* Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery
From: Jonathan Nieder @ 2011-08-14 13:13 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-7-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Move code from builtin/revert.c to sequencer.c and expose a public API
> without making any functional changes. Although it is useful only to
> existing callers of cherry-pick and revert now, this patch lays the
> foundation for future expansion.
:) It sounds like you are running a business.
I guess I would suggest clarifying that you mean exposing further
functionality through this API for more callers, rather than just
generally bloating git further.
[...]
> +++ b/sequencer.c
> @@ -1,13 +1,656 @@
[...]
> +#define COMMIT_MESSAGE_INIT { NULL, NULL, NULL, NULL, NULL };
As much as I like this change (and I do, modulo the stray semicolon),
it does not belong in a patch where it can be unnoticed in the code
movement.
> +static const char *action_keyword(const struct replay_opts *opts)
> +{
> + return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
> +}
Another (non-functional) change. Probably (?) this renaming has a
good reason to be part of this patch, but it should definitely be
mentioned in the commit message.
> +static void write_cherry_pick_head(struct commit *commit)
> +{
> + int fd;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +
> + fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"),
> + git_path("CHERRY_PICK_HEAD"));
git_path() calls vsnprintf which clobbers errno, so depending on the
platform this can print messages like
fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
The natural fix would be to add a local for it (as a separate patch).
Sorry I missed this when the code first arrived.
> +static struct tree *empty_tree(void)
> +{
> + struct tree *tree = xcalloc(1, sizeof(struct tree));
> +
> + tree->object.parsed = 1;
> + tree->object.type = OBJ_TREE;
> + pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
> + return tree;
This tree is leaked (for example if you cherry-pick a sequence of
root commits).
> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)
> +{
> + struct ref_lock *ref_lock;
> +
> + read_cache();
> + if (checkout_fast_forward(from, to))
> + exit(1); /* the callee should have complained already */
> + ref_lock = lock_any_ref_for_update("HEAD", from, 0);
> + return write_ref_sha1(ref_lock, to, "cherry-pick");
> +}
The exit code here violates the usual "exit with status 128 for
errors other than conflicts" rule. Perhaps it should be changed to
"return -1" in a separate patch (to accompany the patch that returns
error() instead of die()-ing so often to allow callers to give
additional context to errors from this machinery).
> void remove_sequencer_state(int aggressive)
> {
> struct strbuf seq_dir = STRBUF_INIT;
> struct strbuf seq_old_dir = STRBUF_INIT;
> -
> strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
Unrelated change snuck in?
> --- a/sequencer.h
> +++ b/sequencer.h
[...]
> @@ -25,4 +49,7 @@ struct replay_insn_list {
> */
> void remove_sequencer_state(int aggressive);
>
> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
> +int sequencer_pick_revisions(struct replay_opts *opts);
> +
I thought sequencer_parse_args() wasn't being exposed.
Except where noted above, I hope this is just simple code movement,
but I haven't checked. If I could be sure, it would be easier to
review.
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions
From: Jonathan Nieder @ 2011-08-14 12:52 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Currently, prepare_revs calls setup_revisions with the (argc, argv) in
> the opts structure filled in by parse_args. As a result, prepare_revs
> has to take up the responsibility of erroring out and printing usage
> information when (argc, argv) is malformed. Since parse_args is doing
> this for other kinds of parse errors anyway, give it the task of
> calling setup_revisions as well.
I'm having trouble understanding the above. Are you saying that we
want to concentrate all usage() calls in a single function for some
reason and prepare_revs() goes against this? Why would I (the reader)
care about that?
Presumably a simpler explanation is that it makes my life easier in
two ways:
1. If I want to understand what commandline arguments are accepted
by cherry-pick/revert, after this patch there is just one function
to look at.
2. If I want to ask the replay machinery to do something different,
as a caller I can set my options in a "struct rev_info" instead
of forging commandline arguments with the same effect.
> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Since I wrote the patch modulo the commit message and small
improvements, shouldn't I be blamed as the author? (git log
--grep="[Oo]riginal patch" and git log --grep=jc: give some examples.)
You can have my sign-off if you'd like.
[...]
> + if (opts->subcommand == REPLAY_NONE) {
> + opts->revs = xmalloc(sizeof(*opts->revs));
My fault: this never gets freed. As long as this is private to the
cherry-pick/revert builtin, it's a one-time tiny leak cleaned up by
_exit --- no harm done, but probably worth a comment.
^ permalink raw reply
* Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-08-14 12:12 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> if (!prefixcmp(start, "pick ")) {
> - action = CHERRY_PICK;
> + item->action = REPLAY_PICK;
> p += strlen("pick ");
> } else if (!prefixcmp(start, "revert ")) {
> - action = REVERT;
> + item->action = REPLAY_REVERT;
> p += strlen("revert ");
> - } else
> - return NULL;
> + } else {
> + int len = strchrnul(p, '\n') - p;
> + if (len > 255)
> + len = 255;
> + return error(_("Unrecognized action: %.*s"), len, p);
What happens if the current line has more than INT_MAX characters?
Maybe it would make sense to factor out a function for this
computation, for brevity and so there is just one place to tweak.
^ permalink raw reply
* Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser
From: Jonathan Nieder @ 2011-08-14 11:58 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1313310789-10216-3-git-send-email-artagnon@gmail.com>
Hi,
Ramkumar Ramachandra wrote:
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '
> test_must_fail git cherry-pick --continue
> '
>
> +test_expect_success 'malformed instruction sheet 3' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
> + echo "resolved" >foo &&
> + git add foo &&
> + git commit &&
> + sed "s/pick \([0-9a-f]\+\)\(.*\)/pick \1\1\1\1\1\1\1\1\2/" .git/sequencer/todo >new_sheet &&
This construct (\+ in sed regexes) is not portable. See the note on
grep in Documentation/CodingGuidelines (maybe it should be tweaked to
say "grep and sed").
^ permalink raw reply
* "git apply --check" successes but git am says "does not match index"
From: Zemacsh @ 2011-08-14 9:36 UTC (permalink / raw)
To: git
Hi, all,
I met a problem with git am.
Before applying a mbox patch, "git apply --check" reports OK. Then, I run 'git
am', however, it complains "does not match index". Actually, both working tree
and index are clean. what might be the problem?
If I run "git am --abort" now, and re-turn "git am". To my surprise, everything
goes well.
Best
-- zemacsh
^ permalink raw reply
* [PATCH v4 2/2] push: Don't push a repository with unpushed submodules
From: Fredrik Gustafsson @ 2011-08-14 9:03 UTC (permalink / raw)
To: gitster; +Cc: git, jens.lehmann, hvoigt, iveqy
When working with submodules it is easy to forget to push a
submodule to the server but pushing a super-project that
contains a commit for that submodule. The result is that the
superproject points at a submodule commit that is not available
on the server.
This adds the option --recurse-submodules=check to push. When
using this option git will check that all submodule commits that
are about to be pushed are present on a remote of the submodule.
To be able to use a combined diff, disabling a diff callback has
been removed from combined-diff.c.
Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
I am not sure whether the change required for the diff callback
has other side effects. Another pair of eyes appreciated.
Documentation/git-push.txt | 6 ++
builtin/push.c | 17 ++++++
combine-diff.c | 2 +-
submodule.c | 108 ++++++++++++++++++++++++++++++++++++++++
submodule.h | 1 +
t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++++++++++++++
transport.c | 9 +++
transport.h | 1 +
8 files changed, 237 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 88acfcd..6ae6ba3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -162,6 +162,12 @@ useful if you write an alias or script around 'git push'.
is specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.
+--recurse-submodules=check::
+ Check whether all submodule commits used by the revisions to be
+ pushed are available on a remote tracking branch. Otherwise the
+ push will be aborted and the command will exit with non-zero status.
+
+
include::urls-remotes.txt[]
OUTPUT
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..23ba365 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -8,6 +8,7 @@
#include "remote.h"
#include "transport.h"
#include "parse-options.h"
+#include "submodule.h"
static const char * const push_usage[] = {
"git push [<options>] [<repository> [<refspec>...]]",
@@ -219,6 +220,19 @@ static int do_push(const char *repo, int flags)
return !!errs;
}
+static int option_parse_recurse_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *flags = opt->value;
+ if (arg) {
+ if (!strcmp(arg, "check"))
+ *flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+ else
+ die("bad %s argument: %s", opt->long_name, arg);
+ }
+ return 0;
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -236,6 +250,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
OPT_BIT( 0, "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
+ { OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check",
+ "controls recursive pushing of submodules",
+ PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
diff --git a/combine-diff.c b/combine-diff.c
index be67cfc..e5a0d84 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1068,7 +1068,7 @@ void diff_tree_combined(const unsigned char *sha1,
* when doing combined diff.
*/
int stat_opt = (opt->output_format &
- (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));
+ (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT|DIFF_FORMAT_CALLBACK));
if (i == 0 && stat_opt)
diffopts.output_format = stat_opt;
else
diff --git a/submodule.c b/submodule.c
index 1ba9646..45f508c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,114 @@ void set_config_fetch_recurse_submodules(int value)
config_fetch_recurse_submodules = value;
}
+static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+ return 1;
+}
+
+static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
+{
+ if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ return 0;
+
+ if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
+ struct child_process cp;
+ const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
+ struct strbuf buf = STRBUF_INIT;
+ int needs_pushing = 0;
+
+ argv[1] = sha1_to_hex(sha1);
+ memset(&cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.out = -1;
+ cp.dir = path;
+ if (start_command(&cp))
+ die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
+ sha1_to_hex(sha1), path);
+ if (strbuf_read(&buf, cp.out, 41))
+ needs_pushing = 1;
+ finish_command(&cp);
+ close(cp.out);
+ strbuf_release(&buf);
+ return needs_pushing;
+ }
+
+ return 0;
+}
+
+static void collect_submodules_from_diff(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
+{
+ int i;
+ int *needs_pushing = data;
+
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (!S_ISGITLINK(p->two->mode))
+ continue;
+ if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
+ *needs_pushing = 1;
+ break;
+ }
+ }
+}
+
+
+static void commit_need_pushing(struct commit *commit, struct commit_list *parent, int *needs_pushing)
+{
+ const unsigned char (*parents)[20];
+ unsigned int i, n;
+ struct rev_info rev;
+
+ n = commit_list_count(parent);
+ parents = xmalloc(n * sizeof(*parents));
+
+ for (i = 0; i < n; i++) {
+ hashcpy((unsigned char *)(parents + i), parent->item->object.sha1);
+ parent = parent->next;
+ }
+
+ init_revisions(&rev, NULL);
+ rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+ rev.diffopt.format_callback = collect_submodules_from_diff;
+ rev.diffopt.format_callback_data = needs_pushing;
+ diff_tree_combined(commit->object.sha1, parents, n, 1, &rev);
+
+ free(parents);
+}
+
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
+{
+ struct rev_info rev;
+ struct commit *commit;
+ const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
+ int argc = ARRAY_SIZE(argv) - 1;
+ char *sha1_copy;
+ int needs_pushing = 0;
+ struct strbuf remotes_arg = STRBUF_INIT;
+
+ strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
+ init_revisions(&rev, NULL);
+ sha1_copy = xstrdup(sha1_to_hex(new_sha1));
+ argv[1] = sha1_copy;
+ argv[3] = remotes_arg.buf;
+ setup_revisions(argc, argv, &rev, NULL);
+ if (prepare_revision_walk(&rev))
+ die("revision walk setup failed");
+
+ while ((commit = get_revision(&rev)) && !needs_pushing)
+ commit_need_pushing(commit, commit->parents, &needs_pushing);
+
+ free(sha1_copy);
+ strbuf_release(&remotes_arg);
+
+ return needs_pushing;
+}
+
static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
{
int is_present = 0;
diff --git a/submodule.h b/submodule.h
index 5350b0d..799c22d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,5 +29,6 @@ int fetch_populated_submodules(int num_options, const char **options,
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20]);
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);
#endif
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index faa2e96..4dda8b8 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -32,4 +32,98 @@ test_expect_success push '
)
'
+test_expect_success 'push if submodule has no remote' '
+ (
+ cd work/gar/bage &&
+ >junk2 &&
+ git add junk2 &&
+ git commit -m "Second junk"
+ ) &&
+ (
+ cd work &&
+ git add gar/bage &&
+ git commit -m "Second commit for gar/bage" &&
+ git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push fails if submodule commit not on remote' '
+ (
+ cd work/gar &&
+ git clone --bare bage ../../submodule.git &&
+ cd bage &&
+ git remote add origin ../../../submodule.git &&
+ git fetch &&
+ >junk3 &&
+ git add junk3 &&
+ git commit -m "Third junk"
+ ) &&
+ (
+ cd work &&
+ git add gar/bage &&
+ git commit -m "Third commit for gar/bage" &&
+ test_must_fail git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push succeeds after commit was pushed to remote' '
+ (
+ cd work/gar/bage &&
+ git push origin master
+ ) &&
+ (
+ cd work &&
+ git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push fails when commit on multiple branches if one branch has no remote' '
+ (
+ cd work/gar/bage &&
+ >junk4 &&
+ git add junk4 &&
+ git commit -m "Fourth junk"
+ ) &&
+ (
+ cd work &&
+ git branch branch2 &&
+ git add gar/bage &&
+ git commit -m "Fourth commit for gar/bage" &&
+ git checkout branch2 &&
+ (
+ cd gar/bage &&
+ git checkout HEAD~1
+ ) &&
+ >junk1 &&
+ git add junk1 &&
+ git commit -m "First junk" &&
+ test_must_fail git push --recurse-submodules=check ../pub.git
+ )
+'
+
+test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '
+ git init --bare a
+ git clone a a1 &&
+ (
+ cd a1 &&
+ git init b
+ (
+ cd b &&
+ >junk &&
+ git add junk &&
+ git commit -m "initial"
+ ) &&
+ git add b &&
+ git commit -m "added submodule" &&
+ git push --recurse-submodule=check origin master
+ )
+'
+
+test_expect_success 'push succeeds when --no-recurse-submodules is used' '
+ (
+ cd work &&
+ git push ../pub.git --no-recurse-submodules
+ )
+'
+
test_done
diff --git a/transport.c b/transport.c
index c9c8056..fa279d5 100644
--- a/transport.c
+++ b/transport.c
@@ -10,6 +10,7 @@
#include "refs.h"
#include "branch.h"
#include "url.h"
+#include "submodule.h"
/* rsync support */
@@ -1041,6 +1042,14 @@ int transport_push(struct transport *transport,
flags & TRANSPORT_PUSH_MIRROR,
flags & TRANSPORT_PUSH_FORCE);
+ if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
+ struct ref *ref = remote_refs;
+ for (; ref; ref = ref->next)
+ if (!is_null_sha1(ref->new_sha1) &&
+ check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))
+ die("There are unpushed submodules, aborting.");
+ }
+
push_ret = transport->push_refs(transport, remote_refs, flags);
err = push_had_errors(remote_refs);
ret = push_ret | err;
diff --git a/transport.h b/transport.h
index 161d724..059b330 100644
--- a/transport.h
+++ b/transport.h
@@ -101,6 +101,7 @@ struct transport {
#define TRANSPORT_PUSH_MIRROR 8
#define TRANSPORT_PUSH_PORCELAIN 16
#define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
1.7.6.398.g64865.dirty
^ permalink raw reply related
* [PATCH v4 2/2] push: Don't push a repository with unpushed submodules
From: Fredrik Gustafsson @ 2011-08-14 8:57 UTC (permalink / raw)
To: gitster; +Cc: git, jens.lehmann, hvoigt, iveqy
In-Reply-To: <1313312262-2460-1-git-send-email-iveqy@iveqy.com>
When working with submodules it is easy to forget to push a
submodule to the server but pushing a super-project that
contains a commit for that submodule. The result is that the
superproject points at a submodule commit that is not available
on the server.
This adds the option --recurse-submodules=check to push. When
using this option git will check that all submodule commits that
are about to be pushed are present on a remote of the submodule.
To be able to use a combined diff, disabling a diff callback has
been removed from combined-diff.c.
Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
I am not sure whether the change required for the diff callback
has other side effects. Another pair of eyes appreciated.
Documentation/git-push.txt | 6 ++
builtin/push.c | 17 ++++++
combine-diff.c | 2 +-
submodule.c | 108 ++++++++++++++++++++++++++++++++++++++++
submodule.h | 1 +
t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++++++++++++++
transport.c | 9 +++
transport.h | 1 +
8 files changed, 237 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 88acfcd..6ae6ba3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -162,6 +162,12 @@ useful if you write an alias or script around 'git push'.
is specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.
+--recurse-submodules=check::
+ Check whether all submodule commits used by the revisions to be
+ pushed are available on a remote tracking branch. Otherwise the
+ push will be aborted and the command will exit with non-zero status.
+
+
include::urls-remotes.txt[]
OUTPUT
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..23ba365 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -8,6 +8,7 @@
#include "remote.h"
#include "transport.h"
#include "parse-options.h"
+#include "submodule.h"
static const char * const push_usage[] = {
"git push [<options>] [<repository> [<refspec>...]]",
@@ -219,6 +220,19 @@ static int do_push(const char *repo, int flags)
return !!errs;
}
+static int option_parse_recurse_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *flags = opt->value;
+ if (arg) {
+ if (!strcmp(arg, "check"))
+ *flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+ else
+ die("bad %s argument: %s", opt->long_name, arg);
+ }
+ return 0;
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -236,6 +250,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
OPT_BIT( 0, "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
+ { OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check",
+ "controls recursive pushing of submodules",
+ PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
diff --git a/combine-diff.c b/combine-diff.c
index be67cfc..e5a0d84 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1068,7 +1068,7 @@ void diff_tree_combined(const unsigned char *sha1,
* when doing combined diff.
*/
int stat_opt = (opt->output_format &
- (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));
+ (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT|DIFF_FORMAT_CALLBACK));
if (i == 0 && stat_opt)
diffopts.output_format = stat_opt;
else
diff --git a/submodule.c b/submodule.c
index 1ba9646..45f508c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,114 @@ void set_config_fetch_recurse_submodules(int value)
config_fetch_recurse_submodules = value;
}
+static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+ return 1;
+}
+
+static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
+{
+ if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ return 0;
+
+ if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
+ struct child_process cp;
+ const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
+ struct strbuf buf = STRBUF_INIT;
+ int needs_pushing = 0;
+
+ argv[1] = sha1_to_hex(sha1);
+ memset(&cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.out = -1;
+ cp.dir = path;
+ if (start_command(&cp))
+ die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
+ sha1_to_hex(sha1), path);
+ if (strbuf_read(&buf, cp.out, 41))
+ needs_pushing = 1;
+ finish_command(&cp);
+ close(cp.out);
+ strbuf_release(&buf);
+ return needs_pushing;
+ }
+
+ return 0;
+}
+
+static void collect_submodules_from_diff(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
+{
+ int i;
+ int *needs_pushing = data;
+
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (!S_ISGITLINK(p->two->mode))
+ continue;
+ if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
+ *needs_pushing = 1;
+ break;
+ }
+ }
+}
+
+
+static void commit_need_pushing(struct commit *commit, struct commit_list *parent, int *needs_pushing)
+{
+ const unsigned char (*parents)[20];
+ unsigned int i, n;
+ struct rev_info rev;
+
+ n = commit_list_count(parent);
+ parents = xmalloc(n * sizeof(*parents));
+
+ for (i = 0; i < n; i++) {
+ hashcpy((unsigned char *)(parents + i), parent->item->object.sha1);
+ parent = parent->next;
+ }
+
+ init_revisions(&rev, NULL);
+ rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+ rev.diffopt.format_callback = collect_submodules_from_diff;
+ rev.diffopt.format_callback_data = needs_pushing;
+ diff_tree_combined(commit->object.sha1, parents, n, 1, &rev);
+
+ free(parents);
+}
+
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
+{
+ struct rev_info rev;
+ struct commit *commit;
+ const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
+ int argc = ARRAY_SIZE(argv) - 1;
+ char *sha1_copy;
+ int needs_pushing = 0;
+ struct strbuf remotes_arg = STRBUF_INIT;
+
+ strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
+ init_revisions(&rev, NULL);
+ sha1_copy = xstrdup(sha1_to_hex(new_sha1));
+ argv[1] = sha1_copy;
+ argv[3] = remotes_arg.buf;
+ setup_revisions(argc, argv, &rev, NULL);
+ if (prepare_revision_walk(&rev))
+ die("revision walk setup failed");
+
+ while ((commit = get_revision(&rev)) && !needs_pushing)
+ commit_need_pushing(commit, commit->parents, &needs_pushing);
+
+ free(sha1_copy);
+ strbuf_release(&remotes_arg);
+
+ return needs_pushing;
+}
+
static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
{
int is_present = 0;
diff --git a/submodule.h b/submodule.h
index 5350b0d..799c22d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,5 +29,6 @@ int fetch_populated_submodules(int num_options, const char **options,
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20]);
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);
#endif
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index faa2e96..4dda8b8 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -32,4 +32,98 @@ test_expect_success push '
)
'
+test_expect_success 'push if submodule has no remote' '
+ (
+ cd work/gar/bage &&
+ >junk2 &&
+ git add junk2 &&
+ git commit -m "Second junk"
+ ) &&
+ (
+ cd work &&
+ git add gar/bage &&
+ git commit -m "Second commit for gar/bage" &&
+ git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push fails if submodule commit not on remote' '
+ (
+ cd work/gar &&
+ git clone --bare bage ../../submodule.git &&
+ cd bage &&
+ git remote add origin ../../../submodule.git &&
+ git fetch &&
+ >junk3 &&
+ git add junk3 &&
+ git commit -m "Third junk"
+ ) &&
+ (
+ cd work &&
+ git add gar/bage &&
+ git commit -m "Third commit for gar/bage" &&
+ test_must_fail git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push succeeds after commit was pushed to remote' '
+ (
+ cd work/gar/bage &&
+ git push origin master
+ ) &&
+ (
+ cd work &&
+ git push --recurse-submodules=check ../pub.git master
+ )
+'
+
+test_expect_success 'push fails when commit on multiple branches if one branch has no remote' '
+ (
+ cd work/gar/bage &&
+ >junk4 &&
+ git add junk4 &&
+ git commit -m "Fourth junk"
+ ) &&
+ (
+ cd work &&
+ git branch branch2 &&
+ git add gar/bage &&
+ git commit -m "Fourth commit for gar/bage" &&
+ git checkout branch2 &&
+ (
+ cd gar/bage &&
+ git checkout HEAD~1
+ ) &&
+ >junk1 &&
+ git add junk1 &&
+ git commit -m "First junk" &&
+ test_must_fail git push --recurse-submodules=check ../pub.git
+ )
+'
+
+test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '
+ git init --bare a
+ git clone a a1 &&
+ (
+ cd a1 &&
+ git init b
+ (
+ cd b &&
+ >junk &&
+ git add junk &&
+ git commit -m "initial"
+ ) &&
+ git add b &&
+ git commit -m "added submodule" &&
+ git push --recurse-submodule=check origin master
+ )
+'
+
+test_expect_success 'push succeeds when --no-recurse-submodules is used' '
+ (
+ cd work &&
+ git push ../pub.git --no-recurse-submodules
+ )
+'
+
test_done
diff --git a/transport.c b/transport.c
index c9c8056..fa279d5 100644
--- a/transport.c
+++ b/transport.c
@@ -10,6 +10,7 @@
#include "refs.h"
#include "branch.h"
#include "url.h"
+#include "submodule.h"
/* rsync support */
@@ -1041,6 +1042,14 @@ int transport_push(struct transport *transport,
flags & TRANSPORT_PUSH_MIRROR,
flags & TRANSPORT_PUSH_FORCE);
+ if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
+ struct ref *ref = remote_refs;
+ for (; ref; ref = ref->next)
+ if (!is_null_sha1(ref->new_sha1) &&
+ check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))
+ die("There are unpushed submodules, aborting.");
+ }
+
push_ret = transport->push_refs(transport, remote_refs, flags);
err = push_had_errors(remote_refs);
ret = push_ret | err;
diff --git a/transport.h b/transport.h
index 161d724..059b330 100644
--- a/transport.h
+++ b/transport.h
@@ -101,6 +101,7 @@ struct transport {
#define TRANSPORT_PUSH_MIRROR 8
#define TRANSPORT_PUSH_PORCELAIN 16
#define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
1.7.6.398.g64865.dirty
^ permalink raw reply related
* [PATCH v4 0/2] push limitations
From: Fredrik Gustafsson @ 2011-08-14 8:57 UTC (permalink / raw)
To: gitster; +Cc: git, jens.lehmann, hvoigt, iveqy
The first iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327
The second iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/177992
The third iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/179037/focus=179048
Fredrik Gustafsson (2):
rev-parse: add option --is-well-formed-git-dir [path]
push: Don't push a repository with unpushed submodules
Documentation/git-push.txt | 6 ++
Documentation/git-rev-parse.txt | 4 ++
builtin/push.c | 17 ++++++
builtin/rev-parse.c | 8 +++
cache.h | 1 +
combine-diff.c | 2 +-
setup.c | 7 +++
submodule.c | 108 +++++++++++++++++++++++++++++++++++++++
submodule.h | 1 +
t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++++++++++++++
t/t7400-submodule-basic.sh | 4 +-
t/t7403-submodule-sync.sh | 5 +-
t/t7407-submodule-foreach.sh | 97 ++++++++++++++++++-----------------
transport.c | 9 +++
transport.h | 1 +
15 files changed, 312 insertions(+), 52 deletions(-)
--
1.7.6.398.g64865.dirty
^ permalink raw reply
* Re: [feature wishlist] add commit subcommand to git add -i
From: Ramkumar Ramachandra @ 2011-08-14 8:56 UTC (permalink / raw)
To: Jim Cromie; +Cc: git, Conrad Irwin
In-Reply-To: <CAJfuBxwW8Dyp8FTS13uPOBKZGL9JOEqaSOhGN+zBJ_8BHpJE3g@mail.gmail.com>
Hi Jim,
Jim Cromie writes:
> when using git add -i, it would be handy to have a [c]ommit option.
I can't personally comment on this because I use Magit for staging/
unstaging and committing. It's quite an awesome application- do check
it out if you use Emacs.
> going further, if git rebase -i had ability to "back" a fixup patch
> back to where it should have been, and adjust the intervening patches
> where conflict would normally happen, that would be awesome.
> Simplistically, this would just shift the patch 1 step back iteratively,
> until it wouldnt apply properly, and then --abort, stopping at the last
> clean rebase.
Hm, I'm not sure if I understand fully: is the idea about moving a
commit backwards iteratively so we have to resolve several simpler and
smaller conflicts? I have to admit that I work around this problem by
running 'rebase -i' several times, moving the commit back in the
sequence little-by-little.
-- Ram
^ permalink raw reply
* Re: [feature wishlist] add commit subcommand to git add -i
From: Conrad Irwin @ 2011-08-14 8:48 UTC (permalink / raw)
To: Jim Cromie; +Cc: git
In-Reply-To: <CAJfuBxwW8Dyp8FTS13uPOBKZGL9JOEqaSOhGN+zBJ_8BHpJE3g@mail.gmail.com>
On Sun, Aug 14, 2011 at 1:38 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> going further, if git rebase -i had ability to "back" a fixup patch
> back to where it should have been, and adjust the intervening patches
> where conflict would normally happen, that would be awesome.
> Simplistically, this would just shift the patch 1 step back iteratively,
> until it wouldnt apply properly, and then --abort, stopping at the last
> clean rebase.
>
> apologies if this is too hair-brained, or already done.
It sounds like you're looking for several git commit
(-p|--interactive) --fixup <commit>, followed by a git rebase -i
--autosquash. It's not quite as automatic as you describe, but I think
that automating it would be pretty hard to do correctly.
Conrad
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox