* Re: [PATCH] fsck: print progress
From: Junio C Hamano @ 2011-11-05 23:59 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Nguyễn Thái Ngọc Duy, git, Jeff King
In-Reply-To: <4EB4EB70.40801@gmail.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?
^ permalink raw reply
* Re: [PATCH 1/5] sequencer: factor code out of revert builtin
From: Jonathan Nieder @ 2011-11-06 0:12 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Christian Couder
In-Reply-To: <1320510586-3940-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Start building the generalized sequencer by moving code from revert.c
> into sequencer.c and sequencer.h. Make the builtin responsible only
> for command-line parsing, and expose a new sequencer_pick_revisions()
> to do the actual work of sequencing commits.
>
> This is intended to be almost a pure code movement patch with no
> functional changes. Check with:
Do I understand correctly that the purpose of this patch is to expose
some functions through the "sequencer.h" API, which patches later in
the series will use? Which functions? What is this generalized
sequencer which we are starting to build? Why should I be happy about
(or care about, for that matter) code having moved from one source
file to another?
Rule of thumb for commit messages: after reading a commit message, I
should be able to predict what the patch will do, without reading the
patch.
I am guessing the above description started sane and then went through
a few revisions without a person reading it all the way through again.
Please consider just rewriting it.
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1,19 +1,9 @@
> #include "cache.h"
> #include "builtin.h"
> -#include "object.h"
> -#include "commit.h"
> -#include "tag.h"
> -#include "run-command.h"
> -#include "exec_cmd.h"
> -#include "utf8.h"
> #include "parse-options.h"
> -#include "cache-tree.h"
> #include "diff.h"
> #include "revision.h"
> #include "rerere.h"
> -#include "merge-recursive.h"
> -#include "refs.h"
> -#include "dir.h"
> #include "sequencer.h"
Hoorah!
[snipping lots of deletion of code from builtin/revert.c]
> @@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
> opts.action = REPLAY_REVERT;
> git_config(git_default_config, NULL);
> parse_args(argc, argv, &opts);
> - res = pick_revisions(&opts);
> + res = sequencer_pick_revisions(&opts);
The new sequencer_pick_revisions is just a new name for the old
pick_revisions. Sane, but probably worth mentioning in the log
message.
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,7 +1,27 @@
> #include "cache.h"
> +#include "object.h"
> +#include "commit.h"
> +#include "tag.h"
> +#include "run-command.h"
> +#include "exec_cmd.h"
> +#include "utf8.h"
> +#include "cache-tree.h"
> +#include "diff.h"
> +#include "revision.h"
> +#include "rerere.h"
> +#include "merge-recursive.h"
> +#include "refs.h"
> -#include "sequencer.h"
> -#include "strbuf.h"
> #include "dir.h"
> +#include "sequencer.h"
Why did sequencer.h move to after dir.h? Wow, we use a lot of headers
here --- I wonder if there are some pieces that could be split out
(that's not due to your patch, though).
[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -8,6 +8,30 @@
> #define SEQ_OPTS_FILE "sequencer/opts"
>
> enum replay_action { REPLAY_REVERT, REPLAY_PICK };
> +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
> +
> +struct replay_opts {
[...]
> @@ -25,4 +49,6 @@ struct replay_insn_list {
> */
> void remove_sequencer_state(int aggressive);
>
> +int sequencer_pick_revisions(struct replay_opts *opts);
Ah, so this moves most of the logic of "git cherry-pick" to the sequencer
but the only new API that needs to be exposed is pick_revisions(). The
calling sequence looks like this:
memset(&opts, o, sizeof(opts));
opts.action = REPLAY_PICK;
opts.revs = xmalloc(sizeof(*opts.revs));
init_revisions(opts.revs);
add_pending_object / setup_revisions / etc
sequencer_pick_revisions(&opts);
The small exposed interface makes this a relatively uninvasive patch,
and the immediate advantage is that we plan to reuse some of the
functionality used in pick_revisions() in other, new APIs to be used
by commands other than cherry-pick. No functional change yet intended.
Except for the commit message, looks reasonable (though I haven't
tried the "git blame" magic to check the code movement part). Thanks.
^ permalink raw reply
* Re: [PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state
From: Jonathan Nieder @ 2011-11-06 0:15 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Christian Couder
In-Reply-To: <1320510586-3940-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when
> invoked aggressively, since we want to treat it as part of the
> sequencer state now. While at it, make some minor improvements to the
> function.
What does it mean to invoke a function aggressively? What is the
nature of these minor improvements (are they behavior changes or just
cleanups)? (Remember, the reader hasn't seen the patch yet.)
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -25,17 +25,22 @@ static char *get_encoding(const char *message);
>
> void remove_sequencer_state(int aggressive)
> {
> + const char *seq_dir = git_path(SEQ_DIR);
> + const char *seq_old_dir = git_path(SEQ_OLD_DIR);
> + const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
If there were just two more like this, the behavior would change
completely. Scary. Are these temporary variables needed?
^ permalink raw reply
* Re: [PATCH 2/1] gc --auto: warn gc will soon run, give users a chance to run manually
From: Junio C Hamano @ 2011-11-06 0:19 UTC (permalink / raw)
To: Fernando Vezzosi; +Cc: git
In-Reply-To: <20111105154411.079F69004A@inscatolati.net>
Fernando Vezzosi <buccia@repnz.net> writes:
> Signed-off-by: Fernando Vezzosi <buccia@repnz.net>
> ---
>
> Rebased Nguyễn's patch on top of mine.
You don't have to do this.
^ permalink raw reply
* Re: [PATCH 3/5] sequencer: sequencer state is useless without todo
From: Jonathan Nieder @ 2011-11-06 0:26 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Christian Couder
In-Reply-To: <1320510586-3940-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Later in the series, we will not write '.git/sequencer/todo' for a
> single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains
> this information. So, stomp the sequencer state in create_seq_state()
> unless the todo file is present.
What problem does this solve? How does it solve it? What does it
mean to stomp?
The usual commit-message debugging strategy applies here: imagine you
are a BIOS clone manufacturer, and for legal reasons you are not
allowed to read this part of the git implementation embedded in the
standard BIOS. However, you are allowed to read the commit message,
and if that message is clear enough, it will explain the purpose and
behavior of that code and you will be able to implement a compatible
implementation addressing the same problem without scratching your
head too much.
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
>
> static int create_seq_dir(void)
> {
> + const char *todo_file = git_path(SEQ_TODO_FILE);
> const char *seq_dir = git_path(SEQ_DIR);
Scary idiom.
> - if (file_exists(seq_dir))
> - return error(_("%s already exists."), seq_dir);
> - else if (mkdir(seq_dir, 0777) < 0)
> + if (file_exists(todo_file))
> + return error(_("%s already exists."), todo_file);
> +
> + /* If todo_file doesn't exist, discard sequencer state */
> + remove_sequencer_state(1);
> + if (mkdir(seq_dir, 0777) < 0)
> die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
I guess this patch would make more sense after patch 4.
^ permalink raw reply
* Re: [PATCH] Add abbreviated commit hash to rebase conflict message
From: Junio C Hamano @ 2011-11-06 0:31 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Ævar Arnfjörð Bjarmason, Jonas Flodén,
Junio C Hamano, Eric Herman, Fernando Vezzosi, Git List
In-Reply-To: <1320501759-27236-1-git-send-email-srabbelier@gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Also move the $msgnum to a more sensible location.
>
> Before:
> Patch failed at 0001 msg
> After:
> Patch 0001 failed at [da65151] msg
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?
> 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?
What happens when threeway is not enabled, and especially when "git am" is
used for applying patches, not within rebase?
> ---
> git-am.sh | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 9042432..9d70588 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -837,7 +837,8 @@ did you forget to use 'git add'?"
> fi
> if test $apply_status != 0
> then
> - eval_gettextln 'Patch failed at $msgnum $FIRSTLINE'
> + abbrev_commit=$(git log -1 --pretty=%h $commit)
> + eval_gettextln 'Patch $msgnum failed at [$abbrev_commit] $FIRSTLINE'
^ permalink raw reply
* 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
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