Re: [RFC PATCH v4 1/3] Add support for nested aliases
Tim Schumacher writes: > it is located at the top of the while() loop. Giving an example is nice, but > wouldn't > it be better to say something like the following? > > /* >* Check if av[0] is a command before seeing if it is an >* alias to avoid taking over existing commands >*/ If we have more concrete and constructive things to explain why we choose to forbid it, that may be worth saying, but I agree that it does not add much value to this comment to declare that an attempt to take over existing commands is "insane". Thanks.
Re: [RFC PATCH v4 1/3] Add support for nested aliases
On 17.09.18 17:37, Junio C Hamano wrote:
Tim Schumacher writes:
On 08.09.18 15:28, Duy Nguyen wrote:
On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher wrote:
+ /*
+* It could be an alias -- this works around the insanity
* of overriding "git log" with "git show" by having
* alias.log = show
*/
I think this comment block is about the next two lines you just
deleted. So delete it to instead of fixing style.
I think that comment is talking about the code that is handing the alias,
so it still would be valid.
"this" in "this works around" refers to the fact that we first check
the builtins and on-GIT_EXEC_PATH commands before trying an alias,
which is an effective way to forbid an alias from taking over
existing command names. So it is not about a particular code but is
about how the two sections of code are laid out.
It probably will make it clear if we reworded and made it a comment
about the whole while() loop may make sense, i.e.
/*
* Check if av[0] is a command before seeing if it is an
* alias to avoid the insanity of overriding ...
*/
while (1) {
...
Imho, the "insanity" part makes the intention of that comment unclear, even if
it is located at the top of the while() loop. Giving an example is nice, but
wouldn't
it be better to say something like the following?
/*
* Check if av[0] is a command before seeing if it is an
* alias to avoid taking over existing commands
*/
but that can be done after the dust settles as a clean-up, I would
think.
I'll keep the changed comment in my local repository for now and publish it
together
with other changes in v6, but I assume there won't be much additional feedback.
Re: [RFC PATCH v4 1/3] Add support for nested aliases
Tim Schumacher writes:
> On 08.09.18 15:28, Duy Nguyen wrote:
>> On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher wrote:
>>> + /*
>>> +* It could be an alias -- this works around the insanity
>>> * of overriding "git log" with "git show" by having
>>> * alias.log = show
>>> */
>>
>> I think this comment block is about the next two lines you just
>> deleted. So delete it to instead of fixing style.
>
> I think that comment is talking about the code that is handing the alias,
> so it still would be valid.
"this" in "this works around" refers to the fact that we first check
the builtins and on-GIT_EXEC_PATH commands before trying an alias,
which is an effective way to forbid an alias from taking over
existing command names. So it is not about a particular code but is
about how the two sections of code are laid out.
It probably will make it clear if we reworded and made it a comment
about the whole while() loop may make sense, i.e.
/*
* Check if av[0] is a command before seeing if it is an
* alias to avoid the insanity of overriding ...
*/
while (1) {
...
but that can be done after the dust settles as a clean-up, I would
think.
Re: [RFC PATCH v4 1/3] Add support for nested aliases
On 08.09.18 15:28, Duy Nguyen wrote: On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher wrote: + /* +* It could be an alias -- this works around the insanity * of overriding "git log" with "git show" by having * alias.log = show */ I think this comment block is about the next two lines you just deleted. So delete it to instead of fixing style. I think that comment is talking about the code that is handing the alias, so it still would be valid. The check might have peen placed in between to keep it logically grouped. - if (done_alias) - break; if (!handle_alias(argcp, argv)) break; done_alias = 1; }
Re: [RFC PATCH v4 1/3] Add support for nested aliases
On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher wrote: > + /* > +* It could be an alias -- this works around the insanity > * of overriding "git log" with "git show" by having > * alias.log = show > */ I think this comment block is about the next two lines you just deleted. So delete it to instead of fixing style. > - if (done_alias) > - break; > if (!handle_alias(argcp, argv)) > break; > done_alias = 1; > } -- Duy
