* Re: [PATCH 4/5] sequencer: handle single commit pick separately
From: Jonathan Nieder @ 2011-11-06 0:35 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Christian Couder
In-Reply-To: <1320510586-3940-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already
> contains this information. However, '.git/sequencer/opts' and
> '.git/sequencer/head' are required to support '--reset' and
> '--continue' operations.
This is meant as a signal to later "git cherry-pick" commands that it
is okay to forget about the cherry-pick, right? How is the reader
supposed to know that? Say so!
By the way, it's not clear to me yet whether the resulting UI would be
more pleasant or not. What is the expected calling sequence? Any odd
corners of behavior changing? What happens if I do
git cherry-pick foo; # conflicts!
git cherry-pick bar; # just ignore them
or
git cherry-pick foo; # conflicts! but resolved in index by rerere
git checkout something-else
Is there any potential downside to the change?
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,
> opts->record_origin || opts->edit));
> read_and_refresh_cache(opts);
>
> + /*
> + * Backward compatibility hack: when only a single commit is
> + * picked, don't save_todo(), because CHERRY_PICK_HEAD will
> + * contain this information anyway.
> + */
How does saving disk space by avoiding saving redundant information
affect backward compatibility? I'm not sure what this comment is
trying to say.
> + if (opts->subcommand == REPLAY_NONE &&
> + todo_list->next == NULL && todo_list->action == REPLAY_PICK)
> + return do_pick_commit(todo_list->operand, REPLAY_PICK, opts);
> +
> for (cur = todo_list; cur; cur = cur->next) {
^ permalink raw reply
* Re: [PATCH] Add abbreviated commit hash to rebase conflict message
From: Sverre Rabbelier @ 2011-11-06 0:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð, Jonas Flodén, Eric Herman,
Fernando Vezzosi, Git List
In-Reply-To: <7v39e2852t.fsf@alter.siamese.dyndns.org>
Heya,
On Sun, Nov 6, 2011 at 01:31, Junio C Hamano <gitster@pobox.com> wrote:
> We can guess that 7-hexdigit is an abbreviated commit object name but the
> above description and the title do not tell the most important thing. What
> commit are you trying to describe, and why is it a good idea to show it?
The same commit that the title and number are already being displayed
for. It's a good idea to show that as that's a lot more convenient way
to look up the commit that failed to apply than just a rather
arbitrary number and the title.
>> Reviewed-by: Eric Herman <eric@freesa.org>
>> Reviewed-by: Fernando Vezzosi <buccia@repnz.net>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
>
> I wouldn't have issues if these were Helped-by or Asked-by or something,
> but a patch with Reviewed-by for which I do not see any trace of
> discussion on this list triggers some WTF at least for me.
>
> Where did these reviews take place? What were their inputs and how was the
> patch improved based on them? Why I should trust the judgements of these
> people?
We had a little Git hackathon in Amsterdam today, the review was done
IRL. In this case it consisted of Fernando pointing out that we should
stick to the git cherry-pick format of displaying the hash/title (with
the hash in square brackets before the title), rather than in
parenthesis after the title like I had before. I wanted to give credit
to their offline review somehow. If you'd prefer the "Helped-by" nomer
for this case I'm fine with that.
> What happens when threeway is not enabled, and especially when "git am" is
> used for applying patches, not within rebase?
The same thing that already happens. I'm not sure what it is, but
whatever title/number is shown, the matching hash is now shown as
well. This patch does not change that behavior.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 5/5] sequencer: revert d3f4628e
From: Jonathan Nieder @ 2011-11-06 0:42 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Christian Couder
In-Reply-To: <1320510586-3940-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Revert d3f4628e (revert: Remove sequencer state when no commits are
> pending, 2011-06-06), because this is not the right approach. Instead
> of increasing coupling between the sequencer and 'git commit', a
> unified '--continue' that invokes 'git commit' on behalf of the
> end-user is preferred.
Forgive me for forgetting: what is the problem that d3f4628e was going
to resolve (i.e., right approach to what)? What is this increased
coupling, and why do we want to avoid it? Is "to prefer" another word
for "to implement"? Who is being united by this new --continue
switch?
Is this patch just reverting a previous patch? If so, why doesn't the
commit message use the usual format
Revert "<commit message>"
This reverts commit <unabbreviated object name>.
<explanation>
?
> sequencer.c | 12 +-----------
> t/t3510-cherry-pick-sequence.sh | 24 ------------------------
> 2 files changed, 1 insertions(+), 35 deletions(-)
When changing behavior, it's more comforting to modify tests to describe
the new behavior than to just get rid of them. :)
To sum matters up: with a new commit message, patch 1 seems likely to
be ready. Patches 2-5 seem to need more work --- it's not clear to me
yet what they are supposed to do.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [git patches] libata updates, GPG signed (but see admin notes)
From: Linus Torvalds @ 2011-11-06 0:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ted Ts'o, Shawn Pearce, git, James Bottomley, Jeff Garzik,
Andrew Morton, linux-ide, LKML
In-Reply-To: <7vsjm2870k.fsf@alter.siamese.dyndns.org>
On Sat, Nov 5, 2011 at 4:49 PM, Junio C Hamano <junio@pobox.com> wrote:
>
> You do not have to resort to NUL; we could just stuff whatever you do not
> need to see but needs to be left *intact* in the new header fields just
> like the embedded GPG signatures are stored in signed commits.
Agreed, [ details removed ] that sounds perfect. And makes it easy to
get at if you want to with just "git cat-file commit" - without ever
really being visible to people who don't care. And having it visible
in the editor with '#' means that the user who does the merge gets to
see what actually ended up being put in there, along with the fact
that yes, it verified correctly.
So I think I really like that approach - it seems to solve all problems.
Linus
^ permalink raw reply
* Re: How do I get a squashed diff for review
From: Alexander Usov @ 2011-11-06 2:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git, Roland Kaufmann
In-Reply-To: <7vobwq86tx.fsf@alter.siamese.dyndns.org>
On 5 November 2011 23:53, Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Usov <a.s.usov@gmail.com> writes:
>
>> Consider the following history:
>>
>> master: A---B---D---F
>> \ \
>> branch: .-C--E--G
>>
>> ...
>> - "git diff D..branch" would do a trick, but I'm not sure how to
>> correctly determine
>> D (if I'm to write a script).
>
> D is the merge-base between F and G. So "git diff $(git merge-base F G) G"
> would compare D and G.
>
> Modern git lets you write it as "git diff F...G" as this is a fairly
> useful short-hand.
Strange.
I definitely remember reading about "git diff F...G" and trying it out.
And somehow the result wasn't really correct -- as if commit B was
chosen to be merge-base.
I can't reproduce it right now on a sample repo -- will try it once
more back in the office.
Thanks for help.
--
Best regards,
Alexander.
^ permalink raw reply
* Re: [PATCH] fsck: print progress
From: Nguyen Thai Ngoc Duy @ 2011-11-06 2:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git, Jeff King
In-Reply-To: <7vk47e86j5.fsf@alter.siamese.dyndns.org>
2011/11/6 Junio C Hamano <gitster@pobox.com>:
> Stephen Boyd <bebarino@gmail.com> writes:
>
>> What progress isn't shown? How about
>>
>> If --verbose is used with --progress the progress status
>> will not be shown.
>
> When I ask for verbose output, I do not get progress eye-candy?
>
> Why?
Because in verbose mode, the screen is filled with "checking ..."
lines for evevry object, there'll be no wonder why it stops for so
long. It'd be good to show how many percent complete, but with current
progress.c support, the progress line would be lost in "checking..."
flood.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/1] gc --auto: warn gc will soon run, give users a chance to run manually
From: Nguyen Thai Ngoc Duy @ 2011-11-06 2:47 UTC (permalink / raw)
To: Fernando Vezzosi; +Cc: git
In-Reply-To: <20111105151225.EE3869004A@inscatolati.net>
On Sat, Nov 5, 2011 at 5:33 PM, Fernando Vezzosi <buccia@repnz.net> wrote:
> Signed-off-by: Fernando Vezzosi <buccia@repnz.net>
> ---
>
> Rebased Nguyễn's patch on top of mine.
I think when gc.autowarnonly is true, my patch should be no-op because
you'll get warnings eventually when you hit the thresholds.
--
Duy
^ permalink raw reply
* Re: [PATCH] Introduce commit.verbose config option
From: Nguyen Thai Ngoc Duy @ 2011-11-06 2:51 UTC (permalink / raw)
To: Fernando Vezzosi; +Cc: git
In-Reply-To: <20111105182339.27C069004A@inscatolati.net>
On Sun, Nov 6, 2011 at 1:07 AM, Fernando Vezzosi <buccia@repnz.net> wrote:
> Enabling commit.verbose will make git commit behave as if --verbose was
> passed on the command line.
Is it necessary? If you want short command, you would use alias
instead of typing "commit". Adding --verbose to the alias to make the
command even shorter is straightforward.
--
Duy
^ permalink raw reply
* [ANNOUNCE] git-cola v1.7.0 tagged and released
From: David Aguilar @ 2011-11-06 4:11 UTC (permalink / raw)
To: git-cola, git
The latest feature release of git-cola is available on github:
http://git-cola.github.com/
git-cola is a sleek and powerful git GUI.
It's fast, featureful, and fun to use.
Development is active after almost 4 years of development
so checkout our github repo for the latest and greatest:
https://github.com/git-cola/git-cola
New features since the last release
-----------------------------------
* A graphical DAG visualizer is available as `git dag`.
* The 'stash' widget includes a preview pane for instantly
showing the contents of a stash.
* Usability improvements
`git-dag` is probably the most interesting new feature.
It visualizes DAGs using a 2D canvas which allows for unique
visualizations over a project's history.
`git-cola` has long had advanced interactive staging /
index-manipulation capabilities, difftool integration,
and much more. It's written in python, portable to all
platforms where git is available, and GPL2 licensed.
Have fun,
--
David @ https://github.com/davvid
^ permalink raw reply
* Re: [PATCH] Add abbreviated commit hash to rebase conflict message
From: Junio C Hamano @ 2011-11-06 4:14 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Ævar Arnfjörð, Jonas Flodén, Eric Herman,
Fernando Vezzosi, Git List
In-Reply-To: <CAGdFq_hw1630ELQP3+AEaCmUTEjYq7K1j8ZB-n0_rb1VN=wQgA@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> We had a little Git hackathon in Amsterdam today,...
Ah, that was the context I was missing.
>> What happens when threeway is not enabled, and especially when "git am" is
>> used for applying patches, not within rebase?
>
> The same thing that already happens. I'm not sure what it is, but
> whatever title/number is shown, the matching hash is now shown as
> well. This patch does not change that behavior.
I am puzzled, but that cannot be true. The existing message uses $msgnum
and $FIRSTLINE but does not use $commit because it does not necessarily
exist.
What a value would the variable contain when I am applying your original
patch message using "git am -s" (or "git am -s3")?
^ permalink raw reply
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly
From: Jonathan Nieder @ 2011-11-06 4:31 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow,
Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Eric Herman,
Fernando Vezzosi
In-Reply-To: <1320535407-4933-2-git-send-email-srabbelier@gmail.com>
Sverre Rabbelier wrote:
> This happens only when the corresponding commits are not exported in
> the current fast-export run. This can happen either when the relevant
> commit is already marked, or when the commit is explicitly marked
> as UNINTERESTING with a negative ref by another argument.
The above "This" has no antecedent. I guess you mean that
fast-export writes no output when passed a range of the form A..A.
> This breaks fast-export based remote helpers,
Makes sense.
> as they use marks
> files to store which commits have already been seen. The call graph
> is something as follows:
>
> $ # push master to remote repo
> $ git fast-export --{im,ex}port-marks=marksfile master
> $ # make a commit on master and push it to remote
> $ git fast-export --{im,ex}port-marks=marksfile master
> $ # run `git branch foo` and push it to remote
> $ git fast-export --{im,ex}port-marks=marksfile foo
>
> When fast-export imports the marksfile and sees that all commits in
> foo are marked as UNINTERESTING
Hmm, I didn't know about this behavior. Would it be possible to add
a test for it, too?
> t/t9350-fast-export.sh | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
With or without the change suggested above, this new test seems to me
like a good thing, even though in the longer term it might be nicer to
teach fast-export to understand a syntax like
git fast-import ^master master:master
Put another way, the possibility of something nicer later shouldn't
stop us from adding an incremental refinement that improves things
today.
Thanks for working on this,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3] fast-export: do not refer to non-existing marks
From: Jonathan Nieder @ 2011-11-06 4:45 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow,
Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Eric Herman,
Fernando Vezzosi
In-Reply-To: <1320535407-4933-3-git-send-email-srabbelier@gmail.com>
Hi,
Sverre Rabbelier wrote:
> When calling `git fast-export a..a b` when a and b refer to the same
> commit, nothing would be exported, and an incorrect reset line would
> be printed for b ('from :0').
Hm, seems problematic indeed.
> Extract a handle_reset function that deals with this, which can then
> be re-used in a later commit.
So, does this patch drop the confusing behavior and add one that is
more intuitive for remote helpers? It's not clear from this
description what sort of deal the patch makes and whether it is a good
or bad one.
[...]
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -529,9 +529,20 @@ static void get_tags_and_duplicates(struct object_array *pending,
> }
> }
>
> +static void handle_reset(const char *name, struct object *object)
Nit: the other handle_* functions are about acting on objects
encountered during revision traversal from the object store. In other
words, the things being handled are the git objects.
By contrast, this function is about writing a "reset" command to the
fast-import stream. I'd be tempted to call it reset_ref() or
something like that.
> +{
> + int mark = get_object_mark(object);
> +
> - commit = (struct commit *)object;
> - printf("reset %s\nfrom :%d\n\n", name,
> - get_object_mark(&commit->object));
> + if (mark)
> + printf("reset %s\nfrom :%d\n\n", name,
> + get_object_mark(object));
> + else
> + printf("reset %s\nfrom %s\n\n", name,
> + sha1_to_hex(object->sha1));
Ah --- the functional change is to use a sha1 when there is no mark
corresponding to the object.
Why is this codepath being run at all when b is excluded by the
revision range (a..a a = ^a a a)? Is this the same bug tested
for in patch 1/3 or something separate?
^ permalink raw reply
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs
From: Jonathan Nieder @ 2011-11-06 5:01 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow,
Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Eric Herman,
Fernando Vezzosi
In-Reply-To: <1320535407-4933-4-git-send-email-srabbelier@gmail.com>
Sverre Rabbelier wrote:
> When a revision is specified on the commandline we explicitly output
> a 'reset' command for it if it was not handled already. This allows
> for example the remote-helper protocol to use fast-export to create
> branches that point to a commit that has already been exported.
>
> Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
Thanks. I'd suggest squashing in the test from patch 1/3 for easy
reference (since each patch makes the other easier to understand).
> ---
> The if statement dealing with tag_of_filtered_mode is not as
> elegant as either me or Dscho would have liked, but we couldn't
> find a better way to determine if a ref is a tag at this point
> in the code.
>
> Additionally, the elem->whence != REV_CMD_RIGHT case should really
> check if REV_CMD_RIGHT_REF, but as this is not provided by the
> ref_info structure this is left as is. A result of this is that
> incorrect input will result in incorrect output, rather than an
> error message. That is: `git fast-export a..<sha1>` will
> incorrectly generate a `reset <sha1>` statement in the fast-export
> stream.
>
> The dwim_ref bit is a double work (it has already been done by the
> caller of this function), but I decided it would be more work to
> pass this information along than to recompute it for the few
> commandline refs that were relevant.
These details seem like good details for the commit message, so the
next puzzled person looking at the code can see what behavior is
deliberate and what are the incidental side-effects.
The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds
worth a test.
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -18,6 +18,8 @@
> #include "parse-options.h"
> #include "quote.h"
>
> +#define REF_HANDLED (ALL_REV_FLAGS + 1)
Could TMP_MARK be used for this?
[...]
> @@ -541,10 +543,34 @@ static void handle_reset(const char *name, struct object *object)
> sha1_to_hex(object->sha1));
> }
>
> -static void handle_tags_and_duplicates(struct string_list *extra_refs)
> +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs)
> {
> int i;
>
> + /* even if no commits were exported, we need to export the ref */
> + for (i = 0; i < revs->cmdline.nr; i++) {
Might be clearer in a new function.
> + struct rev_cmdline_entry *elem = &revs->cmdline.rev[i];
> +
> + if (elem->flags & UNINTERESTING)
> + continue;
> +
> + if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)
> + continue;
Oh, neat.
> +
> + char *full_name;
declaration-after-statement
> + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);
> +
> + if (!prefixcmp(full_name, "refs/tags/") &&
What happens if dwim_ref fails, perhaps because a ref was deleted in
the meantime?
> + (tag_of_filtered_mode != REWRITE ||
> + !get_object_mark(elem->item)))
> + continue;
Style nit: this would be easier to read if the "if" condition doesn't
line up with the code below it:
if (!prefixcmp(full_name, "refs/tags/")) {
if (tag_of_filtered_mode != REWRITE ||
!get_object_mark(elem->item))
continue;
}
If tag_of_filtered_mode == ABORT, we are going to die() soon, right?
So this seems to be about tag_of_filtered_mode == DROP --- makes
sense.
When does the !get_object_mark() case come up?
> +
> + if (!(elem->flags & REF_HANDLED)) {
> + handle_reset(full_name, elem->item);
> + elem->flags |= REF_HANDLED;
> + }
Just curious: is the REF_HANDLED handling actually needed? What
would happen if fast-export included the redundant resets?
> + }
[...]
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -446,7 +446,7 @@ from $(git rev-parse master)
>
> EOF
>
> -test_expect_failure 'refs are updated even if no commits need to be exported' '
> +test_expect_success 'refs are updated even if no commits need to be exported' '
> git fast-export master..master > actual &&
> test_cmp expected actual
> '
Thanks for a pleasant read.
^ permalink raw reply
* Re: [PATCH 09/10] fmt-merge-msg: Add contents of merged tag in the merge message
From: Junio C Hamano @ 2011-11-06 6:03 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <4EB4F74D.7090004@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Since the tag signature depends on the tag message, including all spelin
> errors, the integrator must not change the text so that third parties
> can repeat the verification. (Correct?) I assume this is the reason that
> you put 'tag:' in front of the tag message, to discourage any changes.
Yes, but after reading response from Linus in the other thread, I think we
came up with even a better plan.
> What if the tag is not signed?
Thanks for raising a good point. See
http://thread.gmane.org/gmane.linux.ide/50518/focus=1211568
for the revised plan.
^ permalink raw reply
* Re: [PATCH] fsck: print progress
From: Junio C Hamano @ 2011-11-06 6:05 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Stephen Boyd, git, Jeff King
In-Reply-To: <CACsJy8DDKLrtmA3rGdW6xghwSs_e+zi1o2LzPDbmryH+XufpvQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> Because in verbose mode, the screen is filled with "checking ..."
> lines for evevry object, there'll be no wonder why it stops for so
> long.
Ah, OK. It makes sense then, but the fact that you needed to explain it
would suggest that explanation deserves to be in the proposed commit log
message.
I was wondering if it was a typo of either "If --quiet is used, progress
is not shown", or "If --verbose=no is used, ..."
^ permalink raw reply
* Re: [PATCH na/strtoimax] Compatibility: declare strtoimax() under NO_STRTOUMAX
From: Junio C Hamano @ 2011-11-06 6:10 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Nick Alcock, Git Mailing List
In-Reply-To: <4EB5583E.2030306@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Commit f696543d (Add strtoimax() compatibility function) introduced an
> implementation of the function, but forgot to add a declaration.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
Thanks, but I think f696543d is v1.7.6 and not that patch. I hope you do
not mind if I just squashed this in, instead of leaving it as a separate
patch.
> git-compat-util.h | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index feb6f8e..4efef46 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -354,6 +354,8 @@ extern size_t gitstrlcpy(char *, const char *, size_t);
> #ifdef NO_STRTOUMAX
> #define strtoumax gitstrtoumax
> extern uintmax_t gitstrtoumax(const char *, char **, int);
> +#define strtoimax gitstrtoimax
> +extern intmax_t gitstrtoimax(const char *, char **, int);
> #endif
>
> #ifdef NO_STRTOK_R
^ permalink raw reply
* Re: [PATCH] pull: introduce a pull.rebase option to enable --rebase
From: Junio C Hamano @ 2011-11-06 7:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Eric Herman, Sverre Rabbelier, Fernando Vezzosi,
Johannes Schindelin
In-Reply-To: <1320507358-3407-1-git-send-email-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> This option will be considered at a lower priority than
> branch.<name>.rebase, i.e. we could set pull.rebase=true and
> branch.<name>.rebase=false and the latter configuration option would
> win.
>
> Reviewed-by: Sverre Rabbelier <srabbelier@gmail.com>
> Reviewed-by: Fernando Vezzosi <buccia@repnz.net>
> Reviewed-by: Eric Herman <eric@freesa.org>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
I see many reviewed-by lines, but what kind of review did this patch have,
exactly? It seem to break its own test (branch.to-rebase.rebase should
override pull.rebase).
I think I've queued most (if not all) of the patches in flight except for
Ram's sequencer reroll to 'pu' and pu^ passes the test but the tip of pu
does not due to this topic.
Thanks for other topics from the Amsterdam together, by the way. I did not
comment on them individually but they looked mostly reasonable from a
quick glance.
^ permalink raw reply
* Re: [PATCH na/strtoimax] Compatibility: declare strtoimax() under NO_STRTOUMAX
From: Johannes Sixt @ 2011-11-06 8:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nick Alcock, Git Mailing List
In-Reply-To: <7vlirt7pdk.fsf@alter.siamese.dyndns.org>
Am 06.11.2011 07:10, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Commit f696543d (Add strtoimax() compatibility function) introduced an
>> implementation of the function, but forgot to add a declaration.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>
> Thanks, but I think f696543d is v1.7.6 and not that patch.
Oops, sorry for that.
> I hope you do
> not mind if I just squashed this in, instead of leaving it as a separate
> patch.
I do not mind at all.
-- Hannes
^ permalink raw reply
* Re: [PATCH] pull: introduce a pull.rebase option to enable --rebase
From: Sverre Rabbelier @ 2011-11-06 9:23 UTC (permalink / raw)
To: Ævar Arnfjörð, Junio C Hamano
Cc: git, Eric Herman, Fernando Vezzosi, Johannes Schindelin
In-Reply-To: <7v8vnt7kvd.fsf@alter.siamese.dyndns.org>
Heya,
On Sun, Nov 6, 2011 at 08:47, Junio C Hamano <gitster@pobox.com> wrote:
> I see many reviewed-by lines, but what kind of review did this patch have,
> exactly? It seem to break its own test (branch.to-rebase.rebase should
> override pull.rebase).
We looked at the patch the same way one would on list, and I assumed
that the tests passed. Ævar, did you sent a wrong version by accident
perhaps?
> Thanks for other topics from the Amsterdam together, by the way. I did not
> comment on them individually but they looked mostly reasonable from a
> quick glance.
Thanks.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* [PATCH v2] pull: introduce a pull.rebase option to enable --rebase
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 9:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Eric Herman, Sverre Rabbelier, Fernando Vezzosi,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
In-Reply-To: <7v8vnt7kvd.fsf@alter.siamese.dyndns.org>
Currently we either need to set branch.<name>.rebase for existing
branches if we'd like "git pull" to mean "git pull --rebase", or have
the forethought of setting "branch.autosetuprebase" before we create
the branch.
But there's no way to globally configure "git pull" to mean "git pull
--rebase" for existing branches, introduce a "pull.rebase" option to
do that.
This option will be considered at a lower priority than
branch.<name>.rebase, i.e. we could set pull.rebase=true and
branch.<name>.rebase=false and the latter configuration option would
win.
Reviewed-by: Sverre Rabbelier <srabbelier@gmail.com>
Reviewed-by: Fernando Vezzosi <buccia@repnz.net>
Reviewed-by: Eric Herman <eric@freesa.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
On Sun, Nov 6, 2011 at 08:47, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This option will be considered at a lower priority than
>> branch.<name>.rebase, i.e. we could set pull.rebase=true and
>> branch.<name>.rebase=false and the latter configuration option would
>> win.
>>
>> Reviewed-by: Sverre Rabbelier <srabbelier@gmail.com>
>> Reviewed-by: Fernando Vezzosi <buccia@repnz.net>
>> Reviewed-by: Eric Herman <eric@freesa.org>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> I see many reviewed-by lines, but what kind of review did this patch have,
> exactly? It seem to break its own test (branch.to-rebase.rebase should
> override pull.rebase).
We all stood behind my laptop while I explained what it did and
why. Sverre pointed out that I should use the test_when_finished()
function for unsetting the config variables, Eric and Fernando looked
it over as well.
> I think I've queued most (if not all) of the patches in flight except for
> Ram's sequencer reroll to 'pu' and pu^ passes the test but the tip of pu
> does not due to this topic.
Due to a trivial error of mine. I sent out the wrong patch as Sverre
thought, the problem was that I was trying to --unset a config
variable twice, that's now fixed and the tests pass with this patch.
Documentation/config.txt | 14 +++++++++++++-
Documentation/git-pull.txt | 2 +-
git-pull.sh | 4 ++++
t/t5520-pull.sh | 23 +++++++++++++++++++++--
4 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5a841da..b2d7d92 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -677,7 +677,9 @@ branch.<name>.mergeoptions::
branch.<name>.rebase::
When true, rebase the branch <name> on top of the fetched branch,
instead of merging the default branch from the default remote when
- "git pull" is run.
+ "git pull" is run. See "pull.rebase" for doing this in a non
+ branch-specific manner.
+
*NOTE*: this is a possibly dangerous operation; do *not* use
it unless you understand the implications (see linkgit:git-rebase[1]
for details).
@@ -1590,6 +1592,16 @@ pretty.<name>::
Note that an alias with the same name as a built-in format
will be silently ignored.
+pull.rebase::
+ When true, rebase branches on top of the fetched branch, instead
+ of merging the default branch from the default remote when "git
+ pull" is run. See "branch.<name>.rebase" for setting this on a
+ per-branch basis.
+
+ *NOTE*: this is a possibly dangerous operation; do *not* use
+ it unless you understand the implications (see linkgit:git-rebase[1]
+ for details).
+
pull.octopus::
The default merge strategy to use when pulling multiple branches
at once.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index e1da468..0f18ec8 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -108,7 +108,7 @@ include::merge-options.txt[]
fetched, the rebase uses that information to avoid rebasing
non-local changes.
+
-See `branch.<name>.rebase` and `branch.autosetuprebase` in
+See `pull.rebase`, `branch.<name>.rebase` and `branch.autosetuprebase` in
linkgit:git-config[1] if you want to make `git pull` always use
`{litdd}rebase` instead of merging.
+
diff --git a/git-pull.sh b/git-pull.sh
index 9868a0b..24b6b7c 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -44,6 +44,10 @@ merge_args=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short="${curr_branch#refs/heads/}"
rebase=$(git config --bool branch.$curr_branch_short.rebase)
+if test -z "$rebase"
+then
+ rebase=$(git config --bool pull.rebase)
+fi
dry_run=
while :
do
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 0e5eb67..35304b4 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -94,16 +94,35 @@ test_expect_success '--rebase' '
test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
test new = $(git show HEAD:file2)
'
+test_expect_success 'pull.rebase' '
+ git reset --hard before-rebase &&
+ git config --bool pull.rebase true &&
+ test_when_finished "git config --unset pull.rebase" &&
+ git pull . copy &&
+ test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
+ test new = $(git show HEAD:file2)
+'
test_expect_success 'branch.to-rebase.rebase' '
git reset --hard before-rebase &&
- git config branch.to-rebase.rebase 1 &&
+ git config --bool branch.to-rebase.rebase true &&
+ test_when_finished "git config --unset branch.to-rebase.rebase" &&
git pull . copy &&
- git config branch.to-rebase.rebase 0 &&
test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
test new = $(git show HEAD:file2)
'
+test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
+ git reset --hard before-rebase &&
+ git config --bool pull.rebase true &&
+ test_when_finished "git config --unset pull.rebase" &&
+ git config --bool branch.to-rebase.rebase false &&
+ test_when_finished "git config --unset branch.to-rebase.rebase" &&
+ git pull . copy &&
+ test $(git rev-parse HEAD^) != $(git rev-parse copy) &&
+ test new = $(git show HEAD:file2)
+'
+
test_expect_success '--rebase with rebased upstream' '
git remote add -f me . &&
--
1.7.6.3
^ permalink raw reply related
* [PATCH 0/3] Fix code issues spotted by clang
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jim Meyering, Fredrik Gustafsson,
Ævar Arnfjörð Bjarmason
This series fixes some of the code issues raised by clang. This leaves
the following warnings that I haven't addressed:
revision.c:766:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
[-Wconstant-conversion]
p->item->object.flags &= ~TMP_MARK;
^ ~~~~~~~~~
revision.c:768:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
[-Wconstant-conversion]
p->item->object.flags &= ~TMP_MARK;
^ ~~~~~~~~~
revision.c:1875:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
[-Wconstant-conversion]
p->item->object.flags &= ~TMP_MARK;
^ ~~~~~~~~~
revision.c:2202:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967158 to 134217590
[-Wconstant-conversion]
commit->object.flags &= ~(ADDED | SEEN | SHOWN);
^ ~~~~~~~~~~~~~~~~~~~~~~~
upload-pack.c:115:12: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
[-Wconstant-conversion]
o->flags &= ~UNINTERESTING;
^ ~~~~~~~~~~~~~~
upload-pack.c:689:19: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294705151 to 133955583
[-Wconstant-conversion]
object->flags &= ~CLIENT_SHALLOW;
^ ~~~~~~~~~~~~~~~
builtin/checkout.c:676:16: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
[-Wconstant-conversion]
object->flags &= ~UNINTERESTING;
^ ~~~~~~~~~~~~~~
builtin/reflog.c:173:32: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294965247 to 134215679
[-Wconstant-conversion]
found.objects[i].item->flags &= ~STUDYING;
^ ~~~~~~~~~
builtin/reflog.c:232:31: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294963199 to 134213631
[-Wconstant-conversion]
pending->item->object.flags &= ~REACHABLE;
^ ~~~~~~~~~~
bisect.c:66:24: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294901759 to 134152191
[-Wconstant-conversion]
commit->object.flags &= ~COUNTED;
^ ~~~~~~~~
Ævar Arnfjörð Bjarmason (3):
apply: get rid of useless x < 0 comparison on a size_t type
diff/apply: cast variable in call to free()
grep: get rid of useless x < 0 comparison on an enum member
builtin/apply.c | 3 ---
builtin/diff.c | 2 +-
grep.c | 2 +-
submodule.c | 2 +-
4 files changed, 3 insertions(+), 6 deletions(-)
--
1.7.6.3
^ permalink raw reply
* [PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jim Meyering, Fredrik Gustafsson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <1320581184-4557-1-git-send-email-avarab@gmail.com>
According to the C standard size_t is always unsigned, therefore the
comparison "n1 < 0 || n2 < 0" when n1 and n2 are size_t will always be
false.
This was raised by clang 2.9 which throws this warning when compiling
apply.c:
builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
if (n1 < 0 || n2 < 0)
~~ ^ ~
builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
if (n1 < 0 || n2 < 0)
~~ ^ ~
This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta
while adding an option to git-apply to ignore whitespace differences.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/apply.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..b3b59db 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
const char *last2 = s2 + n2 - 1;
int result = 0;
- if (n1 < 0 || n2 < 0)
- return 0;
-
/* ignore line endings */
while ((*last1 == '\r') || (*last1 == '\n'))
last1--;
--
1.7.6.3
^ permalink raw reply related
* [PATCH 2/3] diff/apply: cast variable in call to free()
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jim Meyering, Fredrik Gustafsson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <1320581184-4557-1-git-send-email-avarab@gmail.com>
Both of these free() calls are freeing a "const unsigned char (*)[20]"
type while free() expects a "void *". This results in the following
warning under clang 2.9:
builtin/diff.c:185:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
free(parent);
^~~~~~
submodule.c:394:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
free(parents);
^~~~~~~
This free()-ing without a cast was added by Jim Meyering to
builtin/diff.c in v1.7.6-rc3~4 and later by Fredrik Gustafsson in
submodule.c in v1.7.7-rc1~25^2.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/diff.c | 2 +-
submodule.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 1118689..0fe638f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -182,7 +182,7 @@ static int builtin_diff_combined(struct rev_info *revs,
hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
diff_tree_combined(parent[0], parent + 1, ents - 1,
revs->dense_combined_merges, revs);
- free(parent);
+ free((void *)parent);
return 0;
}
diff --git a/submodule.c b/submodule.c
index 0fd10a0..52cdcc6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -391,7 +391,7 @@ static void commit_need_pushing(struct commit *commit, struct commit_list *paren
rev.diffopt.format_callback_data = needs_pushing;
diff_tree_combined(commit->object.sha1, parents, n, 1, &rev);
- free(parents);
+ free((void *)parents);
}
int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
--
1.7.6.3
^ permalink raw reply related
* [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jim Meyering, Fredrik Gustafsson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <1320581184-4557-1-git-send-email-avarab@gmail.com>
Remove an "p->field < 0" comparison in grep.c that'll always be
false. In this case "p" is a "grep_pat" where "field" is defined as:
enum grep_header_field field;
And grep_header_field is in turn defined as:
enum grep_header_field {
GREP_HEADER_AUTHOR = 0,
GREP_HEADER_COMMITTER
};
Meaning that this comparison will always be false. This was spotted by
clang 2.9 which produced the following warning while compiling grep.c:
grep.c:330:16: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
~~~~~~~~ ^ ~
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
grep.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/grep.c b/grep.c
index b29d09c..025d3f8 100644
--- a/grep.c
+++ b/grep.c
@@ -327,7 +327,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
for (p = opt->header_list; p; p = p->next) {
if (p->token != GREP_PATTERN_HEAD)
die("bug: a non-header pattern in grep header list.");
- if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
+ if (GREP_HEADER_FIELD_MAX <= p->field)
die("bug: unknown header field %d", p->field);
compile_regexp(p, opt);
}
--
1.7.6.3
^ permalink raw reply related
* Re: [PATCH 0/3] Fix code issues spotted by clang
From: Ævar Arnfjörð Bjarmason @ 2011-11-06 12:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jim Meyering, Fredrik Gustafsson,
Ævar Arnfjörð Bjarmason, Martin Waitz,
Elijah Newren
In-Reply-To: <1320581184-4557-1-git-send-email-avarab@gmail.com>
On Sun, Nov 6, 2011 at 13:06, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> This series fixes some of the code issues raised by clang.
Out of curiosity I also compiled git on "Sun Studio 12 Update 1" on
ee6dfb2d83ba1b057943e705f707fa27e34e47f9 with this patch series
applied and it emits the following warnings, quoted below along with
my comments:
"pack-write.c", line 76: warning: name redefined by pragma
redefine_extname declared static: tmpfile
Presumably this means that we've imported the function tmpfile() and
Sun's CC doesn't like that we're creating a variable with that name.
"read-cache.c", line 761: warning: statement not reached
SunCC's brain is melting on this particularly clever piece of code:
goto inside;
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
inside:
"sha1_file.c", line 2453: warning: name redefined by pragma
redefine_extname declared static: tmpfile
same tempfile issue.
"compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS
"compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS
Seems we're clashing with this:
$ grep -r '#define.*_FILE_OFFSET_BITS' /usr/include/
/usr/include/sys/feature_tests.h:#define _FILE_OFFSET_BITS 64
/usr/include/sys/feature_tests.h:#define _FILE_OFFSET_BITS 32
"xdiff/xutils.c", line 194: warning: statement not reached
This was added in b97e911643341cb31e6b97029b9ffd96fc675b1d as a
workaround on systems using glibc, should this even be run on Sun
libc? Martin?
Another clever bit of code blowing SunCC's brain:
if (flags & XDF_IGNORE_WHITESPACE) {
goto skip_ws;
while (i1 < s1 && i2 < s2) {
if (l1[i1++] != l2[i2++])
return 0;
skip_ws:
while (i1 < s1 && XDL_ISSPACE(l1[i1]))
i1++;
while (i2 < s2 && XDL_ISSPACE(l2[i2]))
i2++;
}
}
"fast-import.c", line 858: warning: name redefined by pragma
redefine_extname declared static: tmpfile
ditto tempfile issue.
"builtin/fast-export.c", line 54: warning: enum type mismatch: op "="
This seems to me to be a legitimate issue we introduced in
2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?
We're defining an enum like this:
static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;
And then doing:
if (unset || !strcmp(arg, "abort"))
tag_of_filtered_mode = ABORT; <-- Line 54
else if (!strcmp(arg, "drop"))
tag_of_filtered_mode = DROP;
else if (!strcmp(arg, "rewrite"))
tag_of_filtered_mode = REWRITE;
Presumably that assignment should be "= ERROR".
"builtin/index-pack.c", line 175: warning: name redefined by
pragma redefine_extname declared static: tmpfile
ditto tempfile.
"vcs-svn/string_pool.c", line 11: warning: initializer will be
sign-extended: -1
"vcs-svn/string_pool.c", line 81: warning: initializer will be
sign-extended: -1
"vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
"vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
"test-treap.c", line 34: warning: initializer will be sign-extended: -1
All of these come down to assigning ~0 to an uint32_t variable, e.g.:
uint32_t token = ~0;
^ 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