git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Sangeeta NB <sangunb09@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git List <git@vger.kernel.org>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Kaartic Sivaraam <kaartic.sivaraam@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [Outreachy] [PATCH v3] diff: do not show submodule with untracked files as "-dirty"
Date: Fri, 23 Oct 2020 10:53:19 +0530	[thread overview]
Message-ID: <CAHjREB4qGO1=XCy-F+H39FP_KU_zZjKCDA=b9JxCe0WZdM06KQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqft66m8wm.fsf@gitster.c.googlers.com>

Hey,

Sorry for sending it over and over again. I couldn't figure out why
the mail is not reaching the community.

>
> Oops?
>

Sorry about this. Would correct it in the next patch.

> I'd suggest to follow the excellent first paragraph above with
> something like the following.
>
>         Make `--ignore-submodules=untracked` the default for `git
>         diff` when there is no configuration variable or command
>         line option, so that the command would not give '-dirty'
>         suffix to a submodule whose working tree has untracked
>         files, to make it consistent with `git describe --dirty`
>         that is run in the submodule working tree.

Very well written. Would update the description to this. Thanks.

> This new boolean is meant to be set only when non-default
> ignore-submodules option is given either from the command line or
> from the configuration---when the bit is unset, we are told to do
> whatever it is that is the default for us.

I fear I might not get your question clearly here. But from what I
understood this boolean( flags.ignore_submodule_set )  is set only
when diff.ignoreSubmodules is set in user-config.

> And the default is to flip only this bit on.  Do we need to turn off
> other bits that submodule.c::handle_ignore_submodules_arg() function
> touches?  [*1*]
>

We are not flipping the bit, we are setting it to 1. I guess we don't
have to turn off other bits because in case if
submodule.c::handle_ignore_submodules_arg() is called we don't have to
set the default value. So there's no point in flipping any other bits
as if any of them is set, the user should have added some arguments in
 ignoreSubmodules and therefore flags.ignore_submodule_set would be
set to 1 and we won't be setting the default value.

>
> I like the general idea of having one bit that is set if and only if
> the command line or configuration told us specifically what to do,
> so that we can dictate the default after they were taken care of.
>
> But I am not sure if this is a good implementation of that idea.
>
> Case in point.  I was wondering if the most future-proof way to
> answer the question I asked (marked with [*1*] above) was to avoid
> flipping the bits in options->flags ourselves, but to make a call
>
>         handle_ignore_submodules_arg(&options, "untracked");
>
> in repo_diff_setup().  When such an improvement is made after this
> patch lands, the assumption that only the end-user preference will
> call this function no longer holds.

I tried making the call directly like this:
handle_ignore_submodules_arg(&options, "untracked") without using the
extra bit, but in the case when the user specifies
diff.ignoreSubmodules in user-config, the
handle_ignore_submodules_arg() is called even before repo_diff_setup()
and therefore the default value overwrites the value mentioned in
user-config.

> Even without anticipating such a change in the future, there already
> is a callsite of this function in wt-status.c that hands a hardcoded
> string "dirty" to it.  That is *not* caused by an end-user request
> (be it a configuration variable or a command line option), so in a
> sense, the assumption is already broken.
>

I couldn't fully understand what assumption are you talking about. I
would be glad if you can explain it to me in a little more detail.
Thanks!

> I wonder, if we can do things in a more natural way (at least the
> natural way in this codebase).  Usually we do things in this order:
>
>  - initialize the status and option variables to their default state.
>
>  - read the configuration files to allow the state of these
>    variables to be modified from their default state.
>
>  - parse the command line arguments to further allow the state of
>    these variables to be modified.
>
> and then use the final state of these variables.  That way, we do
> not even need the extra bit that is only required if we did things
> in an unnatural way, which is
>
>  - read the config; remember if any bits were toggled
>  - parse the command line; remember if any bits were toggled
>  - only if bits weren't toggled in the above, set the default
>
> I dunno.

Ya, that makes sense. I would look into when the config values are
being read and bits are set and would try to find a way so that we can
get rid of the extra bit. Thanks for pointing this out.

>
> Noise.
>
I hope this is a substitute for nice. XD. Hearing this after a long
time. I used to hear this more frequently when I was in college.

>
> Misindented.

Noted. Would change that in the next patch.

Thanks and regards,
Sangeeta

  reply	other threads:[~2020-10-23  5:23 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 17:08 [PATCH] diff: do not show submodule with untracked files as "-dirty" Sangeeta via GitGitGadget
2020-10-20 13:38 ` [OUTREACHY][PATCH] " Phillip Wood
2020-10-20 18:10   ` Sangeeta NB
2020-10-21 11:28     ` Phillip Wood
2020-10-21 13:10 ` [Outreachy] [PATCH v2] " Sangeeta Jain
2020-10-21 17:43   ` Eric Sunshine
2020-10-21 19:40     ` Sangeeta NB
2020-10-21 23:04       ` Eric Sunshine
2020-10-22 11:22 ` [Outreachy] [PATCH v3] " Sangeeta Jain
2020-10-22 18:07   ` Junio C Hamano
2020-10-23  5:23     ` Sangeeta NB [this message]
2020-10-23 15:19       ` Junio C Hamano
2020-10-23 18:17         ` Sangeeta NB
2020-10-23 18:55           ` Junio C Hamano
2020-10-23 19:08             ` Sangeeta NB
2020-10-23 11:17 ` [PATCH v4] " Sangeeta Jain
2020-10-23 15:56   ` Junio C Hamano
2020-10-23 18:32     ` Sangeeta NB
2020-10-23 20:22       ` Junio C Hamano
2020-10-23 11:18 ` [Outreachy] " Sangeeta Jain
2020-10-23 21:28   ` Junio C Hamano
2020-10-25 10:23     ` Sangeeta NB
2020-10-26 17:36       ` Junio C Hamano
2020-10-23 19:29 ` [Outreachy] [PATCH v5] " Sangeeta Jain
2020-10-26 17:57 ` [Outreachy][PATCH v6] " Sangeeta Jain
2020-11-03 10:46   ` Sangeeta
2020-11-03 17:55     ` Junio C Hamano
2020-11-07 10:47       ` Sangeeta
2020-12-08 21:02         ` Junio C Hamano
2020-11-07 11:10   ` Đoàn Trần Công Danh
2020-11-09 15:19     ` Sangeeta
2020-11-09 17:01       ` Junio C Hamano
2020-11-10  8:39 ` [Outreachy][PATCH v7] " Sangeeta Jain
2020-11-10 17:09   ` Đoàn Trần Công Danh
2020-12-08 13:36   ` Sangeeta
2020-12-08 22:26     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHjREB4qGO1=XCy-F+H39FP_KU_zZjKCDA=b9JxCe0WZdM06KQ@mail.gmail.com' \
    --to=sangunb09@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).