* Re: [PATCH 3/5] revert: simplify getting commit subject in format_todo()
From: Jonathan Nieder @ 2011-12-07 7:06 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> format_todo() calls get_message(), but uses only the subject line of
> the commit message. Save work and unnecessary memory allocations by
> using find_commit_subject() instead. Also, remove the spurious check
> on cur->item: the previous patch makes sure that instruction sheets
> with missing commit subjects are parsable.
So, what effect will this patch actually have? Is it an optimization
with no other observable effect?
^ permalink raw reply
* Re: [bug?] checkout -m doesn't work without a base version
From: Pete Harlan @ 2011-12-07 7:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbormn8vk.fsf@alter.siamese.dyndns.org>
On 12/05/2011 10:58 AM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
>
>> But this only works if there's a base version; if foo.c was added in
>> each branch, we get:
>>
>> error: path 'foo.c' does not have all three versions
>>
>> Git didn't need all three versions to create the original conflicted
>> file, so why would it need them to recreate it?
>
> Because the original "merge" was a bit more carefully written but
> "checkout -m" was written without worrying too much about "both sides
> added differently" corner case and still being defensive about not doing
> random thing upon getting an unexpected input state.
>
> IOW, being lazy ;-)
>
> How does this look?
Wow, thanks for the quick response! That does indeed let me checkout
the file as expected.
I wrote a test (below) to be folded in with your patch, but the test
fails because it expects the restored file to be the same as the
originally-conflicted file, but the conflict-line labels change from
"HEAD" and "master":
<<<<<<< HEAD
in_topic
=======
in_master
>>>>>>> master
to "ours" and "theirs". (The same thing happens in the 3-way merge
case.)
If the label change is expected then I can rewrite the test to ignore
labels, or to expect "ours" and "theirs", whichever you think is best.
If the label change is unexpected, then I guess the test is good :)
--Pete
-- >8 --
Test 'checkout -m -- path' when path is a 2-way merge
Signed-off-by: Pete Harlan <pgit@pcharlan.com>
---
t/t2023-checkout-m-twoway.sh | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
create mode 100755 t/t2023-checkout-m-twoway.sh
diff --git a/t/t2023-checkout-m-twoway.sh b/t/t2023-checkout-m-twoway.sh
new file mode 100755
index 0000000..5b50360
--- /dev/null
+++ b/t/t2023-checkout-m-twoway.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='checkout -m -- conflicted/path/with/2-way/merge
+
+Ensures that checkout -m on a resolved file restores the conflicted file
+when it conflicted with a 2-way merge.'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_tick &&
+ test_commit initial_commit &&
+ git branch topic &&
+ test_commit added_in_master file.txt in_master &&
+ git checkout topic &&
+ test_commit added_in_topic file.txt in_topic
+'
+
+test_must_fail git merge master
+
+test_expect_success '-m restores 2-way conflicted+resolved file' '
+ cp file.txt file.txt.conflicted &&
+ echo resolved >file.txt &&
+ git add file.txt &&
+ git checkout -m -- file.txt &&
+ test_cmp file.txt.conflicted file.txt
+'
+
+test_done
^ permalink raw reply related
* Re: How to make devs write better commit messages
From: Thomas Koch @ 2011-12-07 7:06 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
Joseph Huttner:
> The bottom line is that good commit messages are really important, so
> we should make it as easy as possible for developers to go ahead and
> write a perfect commit message every time they commit code.
I recently started to work with the code review system Gerrit[1]. First I did
not pay attention to it, but later I was amazed that the commit message can
(and should) be reviewed just like the code changes.
So if you're using a code review prozess (you should!) then also include the
commit message in the review.
[1] http://en.wikipedia.org/wiki/Gerrit_%28software%29
Thomas Koch, http://www.koch.ro
^ permalink raw reply
* Re: [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-12-07 7:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all instructions use
> the same action. Now you can do:
>
> pick fdc0b12 picked
> revert 965fed4 anotherpick
>
> For cherry-pick and revert, this means that a 'git cherry-pick
> --continue' can continue an ongoing revert operation and viceversa.
Good idea.
> This patch lays the foundation for extending the parser to support
> more actions so 'git rebase -i' can reuse this machinery in the
> future. While at it, also improve the error messages reported by the
> insn sheet parser.
I don't know what this part is about...
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
> NULL
> };
>
> -enum replay_action { REVERT, CHERRY_PICK };
Removing the enum, to expose it in sequencer.h. What does this have
to do with the stated goal of the patch?
> enum replay_subcommand {
> REPLAY_NONE,
> REPLAY_REMOVE_STATE,
> @@ -73,14 +72,14 @@ struct replay_opts {
>
> static const char *action_name(const struct replay_opts *opts)
> {
> - return opts->action == REVERT ? "revert" : "cherry-pick";
> + return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
[... lots of similar changes snipped ...]
> @@ -467,7 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> return run_command_v_opt(args, RUN_GIT_CMD);
> }
>
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +static int do_pick_commit(struct commit *commit, enum replay_action action,
> + struct replay_opts *opts)
Ok, now we're at the title feature.
Passing "action" as a separate parameter in addition to "opts" makes
sense since it can vary from item to item on the todo list. It
unfortunately makes reviewers' lives hard since it is easy to miss
additional unconverted uses of opts->action in the function and
compilers will not warn about that.
[...]
> if (index_differs_from("HEAD", 0))
> return error_dirty_index(opts);
[...]
> if (parent && parse_commit(parent) < 0)
> /* TRANSLATORS: The first %s will be "revert" or
> "cherry-pick", the second %s a SHA1 */
> return error(_("%s: cannot parse parent commit %s"),
> action_name(opts), sha1_to_hex(parent->object.sha1));
[...]
> res = do_recursive_merge(base, next, base_label, next_label,
> head, &msgbuf, opts);
Makes sense. This is the command name passed on the command line, not
the action name from the todo list, so it is left alone.
[...]
> @@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
[...]
> - error(opts->action == REVERT
> + error(action == REPLAY_REVERT
> ? _("could not revert %s... %s")
> : _("could not apply %s... %s"),
... and that's the last use of "action" in this function on the
current master branch. Good.
[... more distracting s/REVERT/REPLAY_REVERT/ stuff snipped...]
> @@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
[...]
> -static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> - struct replay_opts *opts)
> +static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
Looks good.
[...]
> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
[...]
> - } else
> - return NULL;
> + } else {
> + size_t len = strchrnul(bol, '\n') - bol;
> + if (len > 255)
> + len = 255;
> + return error(_("Line %d: Unrecognized action: %.*s"),
> + lineno, (int)len, bol);
> + }
A new error message when an insn line does not start with "pick" or
"revert". Looks like a good change, but what does it have to do with
the subject of the patch?
[...]
> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
[...]
> status = get_sha1(bol, commit_sha1);
> *end_of_object_name = saved;
[...]
> if (status < 0)
> - return NULL;
> + return error(_("Line %d: Malformed object name: %s"), lineno, bol);
(Not about this patch, but an earlier one) What is this idiom about?
Why not
if (get_sha1(bol, commit_sha1))
return error(_(...));
?
[...]
> -static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> - struct replay_opts *opts)
> +static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
Nice to see.
> {
> - struct commit_list **next = todo_list;
> - struct commit *commit;
> + struct replay_insn_list **next = todo_list;
> + struct replay_insn_list item = {0, NULL, NULL};
Style: elsewhere in this file, initializers look like "{ 0, NULL, NULL }"
(with space between the braces and values).
[...]
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
Thanks for writing these.
> test_line_count = 4 commits
> '
>
> +test_expect_success 'revert --continue continues after cherry-pick' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
Stops in "picked", asking user to resolve conflicts.
> + echo "c" >foo &&
> + git add foo &&
> + git commit &&
User resolves conflicts, as in the previous test.
> + git revert --continue &&
And asks to continue.
> + test_path_is_missing .git/sequencer &&
> + {
> + git rev-list HEAD |
> + git diff-tree --root --stdin |
> + sed "s/$_x40/OBJID/g"
> + } >actual &&
> + cat >expect <<-\EOF &&
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M unrelated
> + OBJID
> + :000000 100644 OBJID OBJID A foo
> + :000000 100644 OBJID OBJID A unrelated
> + EOF
> + test_cmp expect actual
I wonder if there is some simpler way to check the result of resuming
a cherry-pick sequence, though that doesn't have much to do with the
patch. I guess I'd find
echo d >expect &&
... &&
# index is clean
git update-index --refresh &&
git rev-list HEAD >commits &&
test_line_count = 4 commits &&
test_cmp expect foo &&
test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
a more convincing demonstration that the cherry-pick finished without
incident.
> +'
> +
> +test_expect_success 'mixed pick and revert instructions' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
> + echo "c" >foo &&
> + git add foo &&
> + git commit &&
Same setup.
> + oldsha=`git rev-parse --short HEAD~1` &&
> + echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
Adding to the insn sheet. Makes sense.
Summing up: I quite like the idea of this patch, but I would be a lot
happier if changes unrelated to its goal were split off to be
considered separately.
> Acked-by: Jonathan Nieder <jrnider@gmail.com>
No, not acked.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH 5/5] reset: update cache-tree data when appropriate
From: Thomas Rast @ 2011-12-07 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Carlos Martín Nieto
In-Reply-To: <7v62hte1k7.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > @@ -84,6 +85,12 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> > return error(_("Failed to find tree of %s."), sha1_to_hex(sha1));
> > if (unpack_trees(nr, desc, &opts))
> > return -1;
> > +
> > + if (reset_type == MIXED || reset_type == HARD) {
> > + tree = parse_tree_indirect(sha1);
> > + prime_cache_tree(&active_cache_tree, tree);
> > + }
>
> The basic idea that MIXED or HARD should result in a cache-tree that match
> the tree we just read is sound, but how expensive is prime_cache_tree()? I
> think it reads the same tree once again. Admittedly, the data needed to
> reconstruct the tree is likely to be hot in core, but it may be necessary
> to measure before deciding if this is a good change.
Oh, I just didn't bother with timings because it's the same strategy
that read-tree follows, so I assumed if it was worth it back then, it
should again be a win.
For linux-2.6 as usual and best-of-10:
git reset before: 0:00.25real 0.12user 0.11system
after: 0:00.28real 0.14user 0.12system
git reset --hard before: 0:00.21real 0.13user 0.06system
after: 0:00.18real 0.10user 0.07system
So we spend ~30ms of extra user time, at a possible gain of 30% in
future git-commit invocations, as mentioned in the cover letter. I
think that's worth it :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Thomas Rast @ 2011-12-07 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vfwgxflkv.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > Actually (sadly) I'll have to revise it. It doesn't match much of C++
> > either, and I haven't yet come up with a reasonable regex that
> > matches, say,
> >
> > foo::Bar<int>::t& Baz::operator<<(
> >
> > which I would call ludicrous, but it's valid C++.
>
> Heh, I'd rather not see us go that route, which would either end up
> implementing a C++ parser or reverting the heuristics back to "non-blank
> at the beginning of the line" that was already reasonably useful.
Well, there are many things that we deliberately do not match right
now and for which that's a good thing:
label:
public:
void declaration_only(...);
int global_variable;
At some point I was wondering whether it would be better to just
declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and
otherwise match all '^[A-Za-z].*\(' but I may be missing something.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 5/5] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-07 8:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> From: Jonathan Nieder <jrnieder@gmail.com>
>
> Currently, command-line arguments are communicated using (argc, argv)
> until a prepare_revs() turns it into a terse structure. However,
> since we plan to expose the cherry-picking machinery through a public
> API in the future, we want callers to be able to call in with a
> filled-in structure. For the revert builtin, this means that the
> chief argument parser, parse_args(), should parse into such a
> structure. Make this change.
Fine. I guess it can be said more clearly while emphasizing public
interfaces over implementation details:
Since the "multiple cherry-pick" feature was introduced (commit
7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track
of the set of commits to be cherry-picked or reverted using
commit_argc and commit_argv variables storing the relevant
command-line parameters.
Future callers as other commands are built in (am, rebase,
sequencer) may find it easier to pass rev-list options to this
machinery in already-parsed form, though. So teach
cmd_cherry_pick and cmd_revert to parse the rev-list arguments
in advance and pass the commit set to pick_revisions() as a
value of type "struct rev_info".
[...]
> @@ -222,7 +220,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> "-x", opts->record_origin,
> "--edit", opts->edit,
> NULL);
> - opts->commit_argv = argv;
> +
> + if (opts->subcommand == REPLAY_NONE) {
> + opts->revs = xmalloc(sizeof(*opts->revs));
> + init_revisions(opts->revs, NULL);
> + opts->revs->no_walk = 1;
> + if (argc < 2)
> + usage_with_options(usage_str, options);
> + argc = setup_revisions(argc, argv, opts->revs, NULL);
> + } else
> + opts->revs = NULL;
> +
> + /* Forbid stray command-line arguments */
> + if (argc > 1)
> + usage_with_options(usage_str, options);
> }
Looks good.
Tests? What happens if I try "git cherry-pick --all"? How about "git
cherry-pick HEAD..HEAD"?
FWIW, with the commit message clarified and with or without new tests,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: Thomas Rast @ 2011-12-07 8:11 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
René Scharfe wrote:
> Am 02.12.2011 17:15, schrieb René Scharfe:
> > How about adding a parameter to control the number of threads
> > (--threads?) instead that defaults to eight (or five) for the worktree
> > and one for the rest? That would also make benchmarking easier.
>
> Like this:
>
> -- >8 --
> Subject: grep: add parameter --threads
>
> Allow the number of threads to be specified by the user. This makes
> benchmarking the performance impact of different numbers of threads
> much easier.
Sounds good, though in the end we would also want to have a config
variable for the poor OS X users who have to tune their threads
*down*... :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: Thomas Rast @ 2011-12-07 8:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9ED1.8010502@lsrfire.ath.cx>
René Scharfe wrote:
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 47ac188..e981a9b 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -228,8 +228,9 @@ OPTIONS
> there is a match and with non-zero status when there isn't.
>
> --threads <n>::
> + Run <n> search threads in parallel. Default is 8 when searching
> + the worktree and 0 otherwise. This option is ignored if git was
> + built without support for POSIX threads.
[...]
> - nr_threads = (online_cpus() > 1) ? THREADS : 0;
> + nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
It would be more consistent to stick to the pack.threads convention
where 0 means "all of my cores", so to disable threading the user
would set the number of threads to 1. Or were you trying to measure
the contention between the worker thread and the add_work() thread?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:17 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
> ($gname/186365) implemented.
Thanks for this. I am worried that some of my comments in previous
reviews (especially about change descriptions) were not addressed,
which could mean one of a few things:
- you didn't notice them
- they were wrong
- you noticed them and they were describing real problems, but it
wasn't worth the time to fix them
Fine. But I would like to know which case they fell into, so I can
learn in order to do a better job reviewing the future and know my
time is well spent.
The series makes various changes, all essentially good, which leaves
me all the more motivated to figure out how to get this sort of thing
in smoothly without making life difficult for people in the future
tracking down regressions.
Ciao,
Jonathan
^ permalink raw reply
* Re: Re: Git Submodule Problem - Bug?
From: Manuel Koller @ 2011-12-07 8:21 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Jens Lehmann, Fredrik Gustafsson, Thomas Rast, git
In-Reply-To: <20111129220303.GA2812@sandbox-rc.fritz.box>
> How about this:
>
> The user issues 'git submodule add foo' and we discover that there is
> already a local clone under the name foo. Git then asks something like
> this
>
> Error when adding: There is already a local submodule under the
> name 'foo'.
>
> You can either rename the submodule to be added to a different
> name or manually remove the local clone underneath
> .git/modules/foo. If you want to remove the local clone please
> quit now.
>
> We strongly suggest that you give each submodule a unique name.
> Note: This name is independent from the path it is bound to.
>
> What do you want me to do ([r]ename it, [Q]uit) ?
>
> When the user chooses 'rename' git will prompt for a new name.
>
> If we are going to support the remove use case with add we additionally
> need some logic to deal with it during update (which is not supported
> yet AFAIK). But we probably need this support anyway since between
> removal and adding a new submodule under the same can be a long time.
> If users switch between such ancient history and the new history we
> would have the same conflict.
>
> We could of course just error out and tell the user that he has to give
> the submodule an uniqe name. If the user does not do so leave it to him
> to deal with the situation manually.
>
> What do you think?
>
> Cheers Heiko
Prompt to choose another name would be fine I guess - but it solves
the problem only if the submodule has been initialized already. There
could be a submodule of the same name in another branch, which I
haven't checked out yet, for example. The user would have to be forced
choose a unique name for every submodule.
Anyway, it seems impossible to handle a name clash automatically,
since there are good reasons to have different urls for the same
submodule. Having read the thread linked by Junio, the only way out
seems to be a kind of url rewrite scheme and using the url as name.
Doesn't it solve all the problems?
- the url is more or less unique (there are problems now if we have to
different submodules at the same path, which is much more likely to
happen than a different repository at the same url some time in the
future)
- after a change of the submodule's url, we can still check out old
commits in a comfortable way
- we could have the same submodule at different paths, but downloaded only once
- the user is not forced to do anything, but the .gitmodule config can
still be overruled if necessary
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Ramkumar Ramachandra @ 2011-12-07 8:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Hi Jonathan,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this. I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
> - you didn't notice them
> - they were wrong
> - you noticed them and they were describing real problems, but it
> wasn't worth the time to fix them
>
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.
I suppose they were a combination of all three. I'm sorry- I'll try
to be more careful and communicative in the future.
Thanks for this round of reviews!
-- Ram
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
^in
> time is well spent.
Sorry for the nonsense. :) And now that I look back over previous
revisions of the patches, I see that I _hadn't_ clearly complained
about the same aspects of these particular commit messages before.
So what am I talking about here?
I guess it is just a pattern: commit messages I have looked at in the
sequencer series lately seem to focus too much on implementation
details and not enough on the "big picture" of what the user or
internal API user will experience. One symptom is that I get lost in
reading the commit message without looking at the patch. Another
symptom is that the commit messages tend to mention the particular
(private) functions that were changed, instead of the more prominent
(often public) callers that the reader might have cause to call.
Hoping that clarifies a little,
Jonathan
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-07 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
Thanks for the review. There's a lot of things you mention that I
either didn't see (staring blind, you know) or that I didn't know of.
On Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> + struct stat s;
>> + trace_printf("checking '%s'\n", path);
>> +
>> + if (stat(path, &s) < 0) {
>> + ...
>> + /* check world permissions */
>> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> + return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?
Probably. I'll use access instead in a reroll.
> I also think that your permission check is incorrectly implemented.
>
> $ cd /var/tmp && date >j && chmod 044 j && ls -l j
> ----r--r-- 1 junio junio 29 Dec 6 14:32 j
> $ cat j
> cat: j: Permission denied
> $ su pogo
> Password:
> $ cat j
> Tue Dec 6 14:32:23 PST 2011
>
> That's a world-readable but unreadable-only-to-me file.
Hmm, this is a case that didn't fit my expectations. Thanks for catching.
>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> +{
>> + /* man 2 execve states that EACCES is returned for:
>
> /*
> * Just a style, but we tend to write multi-line comment like
> * this, without anything else on opening and closing lines of
> * the comment block.
> */
>
>> + * - The file system is mounted noexec
>> + */
>> + struct strbuf sb = STRBUF_INIT;
>> + char *path = getenv("PATH");
>> + char *next;
>> +
>> + if (strchr(cmd, '/')) {
>> + if (!have_read_execute_permissions(cmd))
>> + error("no read/execute permissions on '%s'\n", cmd);
>> + return;
>> + }
>
> Ok, execvp() failed and "cmd" has at least one slash, so we know we did
> not look for it in $PATH. We check only one and return (did you need
> getenv() in that case?).
Obviously not. Missed that.
>
>> + for (;;) {
>> + next = strchrnul(path, ':');
>> + if (path < next)
>> + strbuf_add(&sb, path, next - path);
>> + else
>> + strbuf_addch(&sb, '.');
>
> Nice touch that you did not forget an empty component on $PATH.
Yes, that's a relic from me starting work based on one of your
proposed patches[1]. So that one goes to you.
[1] http://article.gmane.org/gmane.comp.version-control.git/171838
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n", sb.buf);
>
> Don't you want to continue here upon error, after resetting sb? You just
> saw the directory is unreadble, so you know next file_exists() will fail
> before you try it.
Yes. I thought about that. I didn't do that because of the fact that I
had to do more than just resetting sb. The path variable has to be
updated as well. I had the choice of adding a level of indentation {},
duplicating the code, or just do a check I know before will fail.
There's probably something to say for each one of them. I'll probably
refactor that a bit more.
>> + if (sb.len && sb.buf[sb.len - 1] != '/')
>> + strbuf_addch(&sb, '/');
>> + strbuf_addstr(&sb, cmd);
>> +
>> + if (file_exists(sb.buf)) {
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n",
>> + sb.buf);
>> + else
>> + warn("file '%s' exists and permissions "
>> + "seem OK.\nIf this is a script, see if you "
>> + "have sufficient privileges to run the "
>> + "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?
I don't know/remember. It seemed like a natural thing to do, but I'll find out.
^ permalink raw reply
* Re: [PATCH 2/2] run-command: Add interpreter permissions check
From: Frans Klaver @ 2011-12-07 8:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk469e2rn.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> If a script is started and the interpreter of that script given in the
>> shebang cannot be started due to permissions, we can get a rather
>> obscure situation. All permission checks pass for the script itself,
>> but we still get EACCES from execvp.
>>
>> Try to find out if the above is the case and warn the user about it.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>> run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
>> t/t0061-run-command.sh | 22 ++++++++++++++++
>> 2 files changed, 82 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 5e38c5a..b8cf8d4 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>> return 0;
>> }
>>
>> +static void check_interpreter(const char *cmd)
>> +{
>> + FILE *f;
>> + struct strbuf sb = STRBUF_INIT;
>> + /* bash reads an 80 character line when determining the interpreter.
>> + * BSD apparently only allows 32 characters, as it is the size of
>> + * your average binary executable header.
>> + */
>> + char firstline[80];
>> + char *interpreter = NULL;
>> + size_t s, i;
>> +
>> + f = fopen(cmd, "r");
>> + if (!f) {
>> + error("cannot open file '%s': %s\n", cmd, strerror(errno));
>> + return;
>> + }
>> +
>> + s = fread(firstline, 1, sizeof(firstline), f);
>> + if (s < 2) {
>> + trace_printf("cannot determine file type");
>> + fclose(f);
>> + return;
>> + }
>> +
>> + if (firstline[0] != '#' || firstline[1] != '!') {
>> + trace_printf("file '%s' is not a script or"
>> + " is a script without '#!'", cmd);
>> + fclose(f);
>> + return;
>> + }
>
> Nice touches to silently pass scripts that do not begin with she-bang.
>
>> +
>> + /* see if the given path has the executable bit set */
>> + for (i = 2; i < s; i++) {
>> + if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
>> + interpreter = firstline + i;
>> +
>> + if (interpreter && (firstline[i] == ' ' ||
>> + firstline[i] == '\n')) {
>
> Curious.
>
> "#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?
Apparently so. Thanks for catching.
>> + strbuf_add(&sb, interpreter,
>> + (firstline + i) - interpreter);
>> + break;
>> + }
>
> Wouldn't strcspn() work better instead of this loop?
Probably. Will revise.
>> + }
>> + if (!sb.len) {
>> + error("could not determine interpreter");
>> + strbuf_release(&sb);
>> + return;
>> + }
>> +
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("bad interpreter: no read/execute permissions on '%s'\n",
>> + sb.buf);
>> +
>> + strbuf_release(&sb);
>> +}
>> +
>> static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> {
>> /* man 2 execve states that EACCES is returned for:
>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> char *next;
>>
>> if (strchr(cmd, '/')) {
>> - if (!have_read_execute_permissions(cmd))
>> - error("no read/execute permissions on '%s'\n", cmd);
>> + if (have_read_execute_permissions(cmd))
>> + check_interpreter(cmd);
>
> I would have expected the overall logic to be more like this:
>
> if we cannot read and execute it then
> that in itself is an error (i.e. the error message from [1/2])
> else if we can read it then
> let's see if there is an error in the interpreter.
>
> It is unnatural to see "if we can read and execute, then see if there is
> anything wrong with the interpreter" and _nothing else_ here. If you made
> the "have_read_execute_permissions()" to issue the error message you used
> to give in your [1/2] patch here, that is OK from the point of view of the
> overall code structure, but then the function is no longer "do we have
> permissions" boolean check and needs to be renamed. And if you didn't,
> then I have to wonder why we do not need the error message you added in
> your [1/2].
Hm, yea makes sense. I'll rethink this a bit.
Again, thanks for the review.
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Andreas T.Auer @ 2011-12-07 9:07 UTC (permalink / raw)
To: git
In-Reply-To: <4ED5E9D2.4060503@web.de>
Jens Lehmann wrote:
> Am 30.11.2011 01:55, schrieb Max Krasnyansky:
> I'm working on a patch series to teach Git to optionally update the
> submodules work trees on checkout, reset merge and so on, but I'm not
> there yet.
>
>> I'm thinking about adding a config option that would enable automatic
>> submodule update but wanted to see if there is some fundamental reason
>> why it would not be accepted.
Because there is no good way to do so. It would be fine when you just track
the submodules "read-only", but if you are actually working on submodules,
it is a bad idea to always get a detached HEAD. It is also a bad idea to
merge or rebase on the currently checkedout branch. Because if you are
working on a maint branch in the submodule and then you checkout a pu branch
in the superproject, because you have forgotten that maint branch in the
submodule then all the proposed updates go to the maintenance branch -> bad.
So auto-update is not easy. But below I describe an idea that might solve
these issues and help auto-udpate to work in a sane way.
> I think adding something like an "submodule.autoupdate" config makes lots
> of sense, but IMO it should affect all work tree updating porcelain
> commands, not just merge.
I was thinking about submodule integration and had the idea to bind a
submodule to the superproject by having special references in the submodule
like refs/super/master, refs/super/featureX... So these references are like
tracking branches for the refs/heads/* of the superproject.
If you have tracking branches, the supermodule can just update the
corresponding branch. If this branch is currently checkedout and the work
area is clean, then the work area is updated, too. If there is currently a
local branch or a diffent super-branch checked out then the working area
should be considered "detached" from the superproject and not updated.
With this concept you could even switch branches in the superproject and the
attached submodules follow - still having no detached HEAD. When you want to
do some local work on the submodule you checkout a local branch and merge
back into the super branch later. The head of that super branch might have
changed by the update procedure meanwhile, but that is fine, then you just
have a merge instead of a fast-forward.
Another nice feature would be a recursive commit. So all changed index files
in the _attached_ submodules would first be committed in their submodules
and then the superproject commits too - all with one command. Currently it
feels a little bit like CVS - commit one file(submodule), commit the other
file(submodule) and then apply a label(commit the superproject) to keep the
changes together.
If the submodule is not attached the commit in the superproject can still
detect changes that have been made to the corresponding tracking branch and
pick these up.
As a summary: Tracking submodule branches in the superproject instead of
only the current HEAD of the submodule gives you more freedom to install
sane auto-update procedures. Even though it will raise a lot of detailed
questions like "should the refs/super/* be pushed/pulled when syncing the
submodule repositories".
^ permalink raw reply
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Sebastian Schuberth @ 2011-12-07 9:44 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
On 06.12.2011 21:32, Pat Thoyts wrote:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Thanks a lot, Pat, for making the release!
--
Sebastian Schuberth
^ permalink raw reply
* Re: [RFC PATCH] git-p4: introduce asciidoc documentation
From: Luke Diamand @ 2011-12-07 9:46 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git
In-Reply-To: <20111203235328.GA3866@arf.padd.com>
On 03/12/11 23:53, Pete Wyckoff wrote:
> Add proper documentation for git-p4. Delete the old .txt
> documentation from contrib/fast-import.
> ---
>
> I'd appreciate review by git-p4 people to make sure I captured
> everything from the old documentation, and to catch any errors.
> There's a fair amount of new content in here, describing all
> the options and variables. I left out some obscure commands
> on purpose.
>
> Comments from anyone else would be welcome too. Especially
> in the area of generic asciidoc formatting or style in git
> command pages.
That looks good to me, I can't see anything of note, just one minor nit,
below.
Luke
>
> Thanks,
> -- Pete
>
> Documentation/git-p4.txt | 456 ++++++++++++++++++++++++++++++++++++++++
> contrib/fast-import/git-p4.txt | 289 -------------------------
> 2 files changed, 456 insertions(+), 289 deletions(-)
> create mode 100644 Documentation/git-p4.txt
> delete mode 100644 contrib/fast-import/git-p4.txt
> +
>
> +------------
> +$ cd project
> +$ vi foo.h
I think it works with other editors as well, although I've not tried it
myself. Obviously with reduced functionality :-)
> +git-p4.allowSubmit::
> + By default, any branch can be used as the source for a 'git p4
> + submit' operation. This configuration variable , if set, permits only
Spacing around comma in "variable , if set"
Luke
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 9:56 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0k+GF9qjwYd_TzA3o8FqAH6G+cxREZ5r9x8E2G5k-mmmg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> I suppose they were a combination of all three. I'm sorry- I'll try
> to be more careful and communicative in the future.
Hm, that is not exactly what I was going for, either. First, I didn't
mean to harp on anything in the past --- it was a request regarding
this current review. And on the other hand, I am not sure it is a
matter of you being uncareful or uncommunicative, exactly. It's more
that _I_ am doing a poor job of clearly conveying what Junio described
as easier to learn on one's own at [1].
To put it more concretely:
I am going to be stubborn. I will not ack a patch unless I can
imagine myself, years from now, after running into a bug in a related
area, not being confused by it.
[1] http://thread.gmane.org/gmane.comp.version-control.git/186402/focus=186415
^ permalink raw reply
* &&-chaining tester
From: Jonathan Nieder @ 2011-12-07 10:08 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, git
In-Reply-To: <CALkWK0nNtvrLHxQv17jfrFQ=BcwLfgh7eE9X-nDCCYY0nsOskg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Missing the chaining '&&' seems to be quite a common
> error: I vaguely remember seeing a patch to detect this sometime ago.
> Jonathan?
I guess you mean [1].
If you want to deploy it, you have three choices:
- maintain a list of tests known to have broken &&-chaining, and
set GIT_SKIP_TESTS to avoid them
- fix all cases of broken &&-chaining. For extra bonus points,
send out those patches, respond to reviews, and gently usher
them into Junio's tree
- modify the &&-chaining tester to output a list of tests encountered
with broken &&-chaining instead of halting on the first one.
Some day, I'd like to (help) carry out option (b), but please don't
hold your breath.
[1] http://thread.gmane.org/gmane.comp.version-control.git/157903/focus=158265
^ permalink raw reply
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Stefan Näwe @ 2011-12-07 10:26 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
Am 06.12.2011 21:32, schrieb Pat Thoyts:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Good News!
Thanks to all involved!
Stefan
--
----------------------------------------------------------------
/dev/random says: Do steam rollers really roll steam?
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
^ permalink raw reply
* Undo a commit that is already pushed to central server and merged to several branches
From: Matthias Fechner @ 2011-12-07 14:15 UTC (permalink / raw)
To: Git Mailing List
Dear git list,
I commited two files into the master branch (later I figured out that
the files included a lot bugs).
I continued to work then on different branches and merged the bad master
branch to all my other branches.
I pushed the master branch then to the server including the bogus commit.
What I would like to do is move this bogus commit into a different
branch and remove all changes from this bogus commit from every branch.
Is this possible and how?
Bye
Matthias
--
"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs, and the universe trying to
produce bigger and better idiots. So far, the universe is winning." --
Rich Cook
^ permalink raw reply
* [PATCH/RFC 0/4] Re: commit: honor --no-edit
From: Jonathan Nieder @ 2011-12-07 14:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <7vk469fm9j.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> And this should fix it (only lightly tested).
>
> -- >8 --
> Subject: [PATCH] commit: honor --no-edit
>
> After making fixes to the contents to be committed, it is not unusual to
> update the current commit without rewording the message. Idioms to do
> tell "commit --amend" that we do not need an editor have been:
>
> $ EDITOR=: git commit --amend
> $ git commit --amend -C HEAD
>
> but that was only because a more natural
>
> $ git commit --amend --no-edit
>
> did not honour "--no-edit" option.
I like it.
Here are a couple of tests. The three patches before are just to make
it less frightening to add to the relevant test script.
Jonathan Nieder (4):
test: add missing "&&" after echo command
test: remove a test of porcelain that hardcodes commit ids
t7501 (commit): modernize style
test: commit --amend should honor --no-edit
t/t7501-commit.sh | 335 ++++++++++++++++++++++++++++-------------------------
1 files changed, 175 insertions(+), 160 deletions(-)
^ permalink raw reply
* Re: Undo a commit that is already pushed to central server and merged to several branches
From: Ramkumar Ramachandra @ 2011-12-07 14:42 UTC (permalink / raw)
To: Matthias Fechner; +Cc: Git Mailing List
In-Reply-To: <4EDF74EC.6090504@fechner.net>
Hi Matthias,
Matthias Fechner wrote:
> [...]
> I continued to work then on different branches and merged the bad master
> branch to all my other branches.
> [...]
>
> What I would like to do is move this bogus commit into a different branch
> and remove all changes from this bogus commit from every branch.
If I understand correctly, each of your branches looks like*:
o <--- HEAD of branch; merge commit referencing bogus commit
| \
o \ <--- This is where you want to move the HEAD to
| \
o o <-- Bogus commit from master branch
|
o
|
o <-- Branch born
Assuming that you actually want to rewrite the history, the situation
calls for a git-reset(1). Just "git reset --hard HEAD~1" on each of
your branches (Caution: first understand what it does!) and you'll
rewind the HEAD to "undo" the bad merge. After that you can just "git
push +foo:foo" to overwrite the foo branch on your server. If you
don't want to rewrite anything and simply commit the inverse of the
bad commit, see git-revert(1).
* If you're having difficulty understanding the diagram, please read:
http://eagain.net/articles/git-for-computer-scientists/
Cheers.
-- Ram
^ permalink raw reply
* [PATCH 1/4] test: add missing "&&" after echo command
From: Jonathan Nieder @ 2011-12-07 14:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <20111207144217.GA30157@elie.hsd1.il.comcast.net>
This test wants to modify a file and commit the change, but because of
a missing separator between commands it is parsed as a single "echo"
command.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7501-commit.sh | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3ad04363..da75abc1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -60,7 +60,7 @@ test_expect_success \
test_expect_success \
"next commit" \
- "echo 'bongo bongo bongo' >file \
+ "echo 'bongo bongo bongo' >file && \
git commit -m next -a"
test_expect_success \
@@ -172,11 +172,12 @@ test_expect_success \
# easier to isolate bugs.
cat >expected <<\EOF
-72c0dc9855b0c9dadcbfd5a31cab072e0cb774ca
-9b88fc14ce6b32e3d9ee021531a54f18a5cf38a2
-3536bbb352c3a1ef9a420f5b4242d48578b92aa7
-d381ac431806e53f3dd7ac2f1ae0534f36d738b9
-4fd44095ad6334f3ef72e4c5ec8ddf108174b54a
+285fcf7ec0d61b14249dfdb4c1e1fe03eaf15ee0
+0b8148b9afce917b87d71199b900466dc8ea8b6e
+43fb8826314939ce79a856face7953557fdca3d1
+eaa04bc3ae0f0b003f7f1d86bf869ec5d73eaf3e
+ee1963b250ee0f02a3fe37be0e4a02bb5af6a1ad
+b49f306003c627361a0304d151a6b4c8b26af6a1
402702b49136e7587daa9280e91e4bb7cb2179f7
EOF
--
1.7.8.rc3
^ permalink raw reply related
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