Re: [PATCH v2] advice: add stashBeforeCheckout advice for dirty branch switches
- From
Arsh Srivastava <arshsrivastava00@gmail.com>
- Date
- Mar 10, 2026, 14:40 UTC
- Message-ID
- <CAOAgETOcivRUskCi4PCLnXzn1qGs9jx39JzgBA0jE=CirSkZJQ@mail.gmail.com>
- In-Reply-To
- <CAOAgETMmLKcz2CWqfKCJeoTCfACMXz7M0d2g_zO5M53tnGqQuA@mail.gmail.com>
Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches
Patrick Steinhardt <ps@pks.im> writes:
> It is used in "add.c", but not magically so. The function that you have > introduced is the only site that uses the new advice, but the function > is never called as far as I can see. So ultimately, the proposed change > does not have any effect on the user-observable behaviour.
Thank you for the correction and for the bottom-posting reminder.
You are right. The function advise_on_checkout_dirty_files() is defined but never called anywhere, so the patch has no user-observable effect. I also looked into the existing behaviour more carefully and found that unpack-trees.c already handles this case and prints a message telling the user to commit or stash their changes before switching branches.
So the patch as written is both incomplete and duplicates existing behaviour. I will rework it in v3 to instead enhance the existing message in unpack-trees.c to also mention 'git checkout -m' for users who want to carry their local changes over to the new branch.
Signed-off-by: Arsh Srivastava <arshsrivastava00@gmail.com>
On Tue, 10 Mar 2026 at 20:07, Arsh Srivastava <arshsrivastava00@gmail.com> wrote:
Show 154 quoted lines
>
> Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty
> branch switches
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this?
> > So won't this simply be duplicating the same message?
>
> Thank you for the detailed review. You are correct, the existing message
> in unpack-trees.c already handles this case and my patch duplicates it.
> I also acknowledge the other issues raised:
>
> - The newly introduced function was never called anywhere in the codebase
> - No tests were added
> - The bullet points in the commit message used '>' instead of '-' or '*'
> - The advice message was not formatted with tabs
>
> Rather than duplicating the existing behaviour, I think the better
> approach would be to enhance the existing message in unpack-trees.c to
> also mention 'git checkout -m' as an option for users who want to carry
> their local changes over to the new branch, since the current message
> only says "commit or stash" without mentioning that option.
>
> I will rework the patch in that direction and send a v4.
>
> Signed-off-by: Arsh Srivastava <arshsrivastava00@gmail.com>
>
> On Tue, 10 Mar 2026 at 20:01, Karthik Nayak <karthik.188@gmail.com> wrote:
> >
> > "Arsh Srivastava via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > From: Arsh Srivastava <arshsrivastava00@gmail.com>
> > >
> > > Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users
> > > when they attempt to switch branches with local modifications that
> > > would be overwritten by the operation.
> > >
> > > This includes:
> > >> New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h
> > >> Corresponding "stashBeforeCheckout" entry in advice_setting[]
> > >> New advise_on_checkout_dirty_files() function that lists the
> > > affected files and suggests using git stash push/pop
> > >> Documentation entry in Documentation/config/advice.txt
> > >
> >
> > Nit: Did you mean to add bullet point here? '>' is generally used to
> > quote text. Perhaps use '-' or '*'.
> >
> > [snip]
> >
> > >
> > > Documentation/config/advice.adoc | 5 +++++
> > > advice.c | 27 +++++++++++++++++++++++++++
> > > advice.h | 2 ++
> > > 3 files changed, 34 insertions(+)
> > >
> >
> > Hmm. Shouldn't there be changes which actually call the newly introduced
> > function? Also shouldn't there be tests added?
> >
> > > diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
> > > index 257db58918..8752e05636 100644
> > > --- a/Documentation/config/advice.adoc
> > > +++ b/Documentation/config/advice.adoc
> > > @@ -126,6 +126,11 @@ all advice messages.
> > > Shown when a sparse index is expanded to a full index, which is likely
> > > due to an unexpected set of files existing outside of the
> > > sparse-checkout.
> > > + stashBeforeCheckout::
> > > + Shown when the user attempts to switch branches but has
> > > + local modifications that would be overwritten by the
> > > + operation, to suggest using linkgit:git-stash[1] to
> > > + save changes before switching.
> >
> > Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this?
> >
> > In one of my repos:
> >
> > ❯ git status
> > On branch master
> > Your branch is up to date with 'origin/master'.
> >
> > nothing to commit, working tree clean
> >
> > ❯ echo "aldjf" >> LICENSE
> >
> > ❯ git status
> > On branch master
> > Your branch is up to date with 'origin/master'.
> >
> > Changes not staged for commit:
> > (use "git add <file>..." to update what will be committed)
> > (use "git restore <file>..." to discard changes in working directory)
> > modified: LICENSE
> >
> > no changes added to commit (use "git add" and/or "git commit -a")
> >
> > ❯ git checkout 0-1-stable
> > error: Your local changes to the following files would be overwritten
> > by checkout:
> > LICENSE
> > Please commit your changes or stash them before you switch branches.
> > Aborting
> >
> > So won't this simply be duplicating the same message?
> >
> > > statusAheadBehind::
> > > Shown when linkgit:git-status[1] computes the ahead/behind
> > > counts for a local ref compared to its remote tracking ref,
> > > diff --git a/advice.c b/advice.c
> > > index 0018501b7b..e1264f525c 100644
> > > --- a/advice.c
> > > +++ b/advice.c
> > > @@ -81,6 +81,7 @@ static struct {
> > > [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
> > > [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
> > > [ADVICE_SPARSE_INDEX_EXPANDED] = { "sparseIndexExpanded" },
> > > + [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
> > > [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" },
> > > [ADVICE_STATUS_HINTS] = { "statusHints" },
> > > [ADVICE_STATUS_U_OPTION] = { "statusUoption" },
> > > @@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
> > > "* Use \"git add --sparse <paths>\" to update the index\n"
> > > "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
> > > }
> > > +
> > > +void advise_on_checkout_dirty_files(struct string_list *file_list)
> > > +{
> > > + struct string_list_item *item;
> > > +
> > > + if (!file_list->nr)
> > > + return;
> > > +
> > > + fprintf(stderr, _("The following files have local modifications that would\n"
> > > + "be overwritten by switching branches:\n"));
> > > + for_each_string_list_item(item, file_list)
> > > + fprintf(stderr, "\t%s\n", item->string);
> > > +
> > > + advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
> > > + _("You can save your local changes before switching by running:\n"
> > > + "\n"
> > > + "\tgit stash push\n"
> > > + "\n"
> > > + "Then restore them after switching with:\n"
> > > + "\n"
> > > + "\tgit stash pop\n"
> > > + "\n"
> > > + "Or to discard your local changes, use:\n"
> > > + "\n"
> > > + "\tgit checkout -- <file>"));
> > > +}
> >
> > This doesn't seem to be formatted with tabs.